| Message ID | 20221221210917.458537-3-fabrizio.castro.jz@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Driver support for RZ/V2M PWC | expand |
Hi Geert, Thanks for your feedback! > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 03 January 2023 08:37 > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski > <brgl@bgdev.pl>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Sebastian Reichel <sre@kernel.org>; > Geert Uytterhoeven <geert+renesas@glider.be>; Lee Jones <lee@kernel.org>; > linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-pm@vger.kernel.org; Chris Paterson > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>; linux- > renesas-soc@vger.kernel.org; Laurent Pinchart > <laurent.pinchart@ideasonboard.com>; Jacopo Mondi <jacopo@jmondi.org> > Subject: Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver > > Hi Fabrizio, > > On Wed, Dec 21, 2022 at 10:09 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > The External Power Sequence Controller (PWC) IP (found in the > > RZ/V2M SoC) is a controller for external power supplies (regulators > > and power switches), and it supports the following features: it > > generates a power on/off sequence for external power supplies, > > it generates an on/off sequence for the LPDDR4 core power supply > > (LPVDD), it comes with General-Purpose Outputs, and it processes > > key input signals. > > Thanks for your patch! > > > The PWC is basically a Multi-Function Device (MFD), its software > > support comes with a core driver, and specialized drivers for > > its specific features. > > I have to admit I'm not such a big fan of MFD. In this driver, > you are not even sharing resources in the MFD cells, just the mapped > register base. So I think you can easily save +100 LoC and reduce > maintenance synchronization overhead across subsystems by just having > a single non-MFD driver instead. > > Did you pick MFD because the PWC poweroff feature depends on board > wiring, and thus is optional? I am not a big fan of MFD, either. I picked MFD because we were not 100% sure of what the IP could do when we started working on it. I have received more information regarding the IP now (which I don't have the liberty to discuss), I am still not 100% sure that's all of it, but basically its support may require expansion later on. I liked the solution based on syscon and simple-mfd for several reasons, but having dropped syscon and simple-mfd due to issues with the dt-bindings I have moved on with a core driver to instantiate the required SW support. We could of course move to a unified driver if that makes more sense? If we were to move to unified driver, under which directory would you suggest we put it? > > Are there any other MFD cells planned (e.g. regulators) to be added > later? The public datasheet does not list the actual registers of the > block, only a high-level overview with (rather detailed) behavioral > information. No MFD cells are planned for now, but I can't exclude we won't have any in the future. Thanks, Fab > > Thanks! > > > --- /dev/null > > +++ b/drivers/mfd/rzv2m-pwc.h > > > +struct rzv2m_pwc_priv { > > + void __iomem *base; > > +}; > > + > > +#endif /* __LINUX_RZV2M_PWC_H__ */ > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds
On Tue, 03 Jan 2023, Fabrizio Castro wrote: > Hi Geert, > > Thanks for your feedback! > > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: 03 January 2023 08:37 > > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski > > <brgl@bgdev.pl>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Sebastian Reichel <sre@kernel.org>; > > Geert Uytterhoeven <geert+renesas@glider.be>; Lee Jones <lee@kernel.org>; > > linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux- > > kernel@vger.kernel.org; linux-pm@vger.kernel.org; Chris Paterson > > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>; linux- > > renesas-soc@vger.kernel.org; Laurent Pinchart > > <laurent.pinchart@ideasonboard.com>; Jacopo Mondi <jacopo@jmondi.org> > > Subject: Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver > > > > Hi Fabrizio, > > > > On Wed, Dec 21, 2022 at 10:09 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: > > > The External Power Sequence Controller (PWC) IP (found in the > > > RZ/V2M SoC) is a controller for external power supplies (regulators > > > and power switches), and it supports the following features: it > > > generates a power on/off sequence for external power supplies, > > > it generates an on/off sequence for the LPDDR4 core power supply > > > (LPVDD), it comes with General-Purpose Outputs, and it processes > > > key input signals. > > > > Thanks for your patch! > > > > > The PWC is basically a Multi-Function Device (MFD), its software > > > support comes with a core driver, and specialized drivers for > > > its specific features. > > > > I have to admit I'm not such a big fan of MFD. In this driver, > > you are not even sharing resources in the MFD cells, just the mapped > > register base. So I think you can easily save +100 LoC and reduce > > maintenance synchronization overhead across subsystems by just having > > a single non-MFD driver instead. > > > > Did you pick MFD because the PWC poweroff feature depends on board > > wiring, and thus is optional? > > I am not a big fan of MFD, either. Interesting. Could you both elaborate further please? > I picked MFD because we were not 100% sure of what the IP could do > when we started working on it. > I have received more information regarding the IP now (which I don't > have the liberty to discuss), I am still not 100% sure that's all > of it, but basically its support may require expansion later on. > > I liked the solution based on syscon and simple-mfd for several reasons, > but having dropped syscon and simple-mfd due to issues with the dt-bindings > I have moved on with a core driver to instantiate the required SW support. > We could of course move to a unified driver if that makes more sense? > If we were to move to unified driver, under which directory would you > suggest we put it? If you do not have any resources to share, you can simply register each of the devices via Device Tree. I do not see a valid reason to force a parent / child relationship for your use-case. Many people attempt to use MFD as a dumping ground / workaround for a bunch of reasons. Some valid, others not so much.
Hi Geert, Thank you for your feedback! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 03 January 2023 12:10 > Subject: Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver > > Hi Fabrizio, > > On Tue, Jan 3, 2023 at 1:05 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > On Wed, Dec 21, 2022 at 10:09 PM Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com> wrote: > > > > The External Power Sequence Controller (PWC) IP (found in the > > > > RZ/V2M SoC) is a controller for external power supplies (regulators > > > > and power switches), and it supports the following features: it > > > > generates a power on/off sequence for external power supplies, > > > > it generates an on/off sequence for the LPDDR4 core power supply > > > > (LPVDD), it comes with General-Purpose Outputs, and it processes > > > > key input signals. > > > > > > Thanks for your patch! > > > > > > > The PWC is basically a Multi-Function Device (MFD), its software > > > > support comes with a core driver, and specialized drivers for > > > > its specific features. > > > > > > I have to admit I'm not such a big fan of MFD. In this driver, > > > you are not even sharing resources in the MFD cells, just the mapped > > > register base. So I think you can easily save +100 LoC and reduce > > > maintenance synchronization overhead across subsystems by just having > > > a single non-MFD driver instead. > > > > > > Did you pick MFD because the PWC poweroff feature depends on board > > > wiring, and thus is optional? > > > > I am not a big fan of MFD, either. > > I picked MFD because we were not 100% sure of what the IP could do > > when we started working on it. > > I have received more information regarding the IP now (which I don't > > have the liberty to discuss), I am still not 100% sure that's all > > of it, but basically its support may require expansion later on. > > > > I liked the solution based on syscon and simple-mfd for several reasons, > > but having dropped syscon and simple-mfd due to issues with the dt- > bindings > > I have moved on with a core driver to instantiate the required SW > support. > > We could of course move to a unified driver if that makes more sense? > > If we were to move to unified driver, under which directory would you > > suggest we put it? > > As the GPIO part is larger than the poweroff part, I would put it under > drivers/gpio/. Although drivers/soc/renesas/ could be an option, too. I'll try the unified approach then, under drivers/soc/renesas . Thanks, Fab > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds
On Tue, 03 Jan 2023, Fabrizio Castro wrote: > Hi Lees, > > Thanks for your feedback! > > > From: Lee Jones <lee@kernel.org> > > Sent: 03 January 2023 12:52 > > Subject: Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver > > > > On Tue, 03 Jan 2023, Fabrizio Castro wrote: > > > > > Hi Geert, > > > > > > Thanks for your feedback! > > > > > > > -----Original Message----- > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Sent: 03 January 2023 08:37 > > > > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > > > Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski > > > > <brgl@bgdev.pl>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski > > > > <krzysztof.kozlowski+dt@linaro.org>; Sebastian Reichel > > <sre@kernel.org>; > > > > Geert Uytterhoeven <geert+renesas@glider.be>; Lee Jones > > <lee@kernel.org>; > > > > linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux- > > > > kernel@vger.kernel.org; linux-pm@vger.kernel.org; Chris Paterson > > > > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>; > > linux- > > > > renesas-soc@vger.kernel.org; Laurent Pinchart > > > > <laurent.pinchart@ideasonboard.com>; Jacopo Mondi <jacopo@jmondi.org> > > > > Subject: Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver Could you please tell your mailer to remove mail headers from the body please. > > > > > The External Power Sequence Controller (PWC) IP (found in the > > > > > RZ/V2M SoC) is a controller for external power supplies (regulators > > > > > and power switches), and it supports the following features: it > > > > > generates a power on/off sequence for external power supplies, > > > > > it generates an on/off sequence for the LPDDR4 core power supply > > > > > (LPVDD), it comes with General-Purpose Outputs, and it processes > > > > > key input signals. > > > > > > > > Thanks for your patch! > > > > > > > > > The PWC is basically a Multi-Function Device (MFD), its software > > > > > support comes with a core driver, and specialized drivers for > > > > > its specific features. > > > > > > > > I have to admit I'm not such a big fan of MFD. In this driver, > > > > you are not even sharing resources in the MFD cells, just the mapped > > > > register base. So I think you can easily save +100 LoC and reduce > > > > maintenance synchronization overhead across subsystems by just having > > > > a single non-MFD driver instead. > > > > > > > > Did you pick MFD because the PWC poweroff feature depends on board > > > > wiring, and thus is optional? > > > > > > I am not a big fan of MFD, either. > > > > Interesting. > > > > Could you both elaborate further please? > > I have nothing against MFD Okay, just checking. I'll withdraw my next command then. :) $ rm -rf drivers/mfd > > If you do not have any resources to share, you can simply register each > > of the devices via Device Tree. I do not see a valid reason to force a > > parent / child relationship for your use-case. > > There would probably be overlapping on the same memory region, which would > lead to ioremapping the same region multiple times, which is something > I would prefer to avoid if possible. Okay, so you *do* have shared resources. In which case, why is simple-mfd not working for you? > > Many people attempt to use MFD as a dumping ground / workaround for a > > bunch of reasons. Some valid, others not so much. > > As it turns out, it looks like I don't have valid reasons to use MFD, > therefore I'll switch to a single, non MFD, driver. > > Thank you for taking the time to look into this though! Really > appreciated. Although it is considered okay to have a multi-purpose driver in any one of the subsystems, it's sometimes nicer to split the various functionality to be looked after (maintained) by their respective subject matter experts. You have to do what's right in any given situation. Ultimately it's a call you need to make with the maintainer(s).
> > > > If you do not have any resources to share, you can simply register > > each > > > > of the devices via Device Tree. I do not see a valid reason to force > > a > > > > parent / child relationship for your use-case. > > > > > > There would probably be overlapping on the same memory region, which > > would > > > lead to ioremapping the same region multiple times, which is something > > > I would prefer to avoid if possible. > > > > Okay, so you *do* have shared resources. > > > > In which case, why is simple-mfd not working for you? > > The corresponding dt-bindings got rejected, unfortunately. I had to drop > simple-mfd as a result of dropping the children of my simple-mfd DT node. You have to write DT bindings to be OS agnostic. They *must* match the H/W. Little else matters. How we interpret those in Linux is flexible however.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 30db49f31866..ac4403e4f3cb 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -2265,5 +2265,19 @@ config MFD_RSMU_SPI Additional drivers must be enabled in order to use the functionality of the device. +config MFD_RZV2M_PWC_CORE + tristate "Renesas RZ/V2M PWC Core Driver" + select MFD_CORE + depends on ARCH_R9A09G011 || COMPILE_TEST + help + Select this option to enable the RZ/V2M External Power Sequence + Controller (PWC) core driver. + + The PWC is a controller for external power supplies (regulators and + power switches), and it supports the following features: it generates + a power on/off sequence for external power supplies, it generates an + on/off sequence for the LPDDR4 core power supply (LPVDD), it comes + with General-Purpose Outputs, and it processes key input signals. + endmenu endif diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 457471478a93..e39252a2df23 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -278,3 +278,4 @@ rsmu-i2c-objs := rsmu_core.o rsmu_i2c.o rsmu-spi-objs := rsmu_core.o rsmu_spi.o obj-$(CONFIG_MFD_RSMU_I2C) += rsmu-i2c.o obj-$(CONFIG_MFD_RSMU_SPI) += rsmu-spi.o +obj-$(CONFIG_MFD_RZV2M_PWC_CORE) += rzv2m-pwc.o diff --git a/drivers/mfd/rzv2m-pwc.c b/drivers/mfd/rzv2m-pwc.c new file mode 100644 index 000000000000..f9055fcafda2 --- /dev/null +++ b/drivers/mfd/rzv2m-pwc.c @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 Renesas Electronics Corporation + * + * Core driver for the Renesas RZ/V2M External Power Sequence Controller (PWC) + */ + +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/mfd/core.h> +#include "rzv2m-pwc.h" + +static const struct mfd_cell rzv2m_pwc_gpio_devs[] = { + { .name = "gpio_rzv2m_pwc", }, +}; + +static const struct mfd_cell rzv2m_pwc_poweroff_devs[] = { + { .name = "rzv2m_pwc_poweroff", }, +}; + +static int rzv2m_pwc_probe(struct platform_device *pdev) +{ + struct rzv2m_pwc_priv *priv; + int ret; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + + platform_set_drvdata(pdev, priv); + + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO, + rzv2m_pwc_gpio_devs, + ARRAY_SIZE(rzv2m_pwc_gpio_devs), NULL, 0, + NULL); + if (ret) + return ret; + + if (of_property_read_bool(pdev->dev.of_node, "renesas,rzv2m-pwc-power")) + ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO, + rzv2m_pwc_poweroff_devs, + ARRAY_SIZE(rzv2m_pwc_poweroff_devs), + NULL, 0, NULL); + + return ret; +} + +static const struct of_device_id rzv2m_pwc_of_match[] = { + { .compatible = "renesas,rzv2m-pwc" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rzv2m_pwc_of_match); + +static struct platform_driver rzv2m_pwc_driver = { + .probe = rzv2m_pwc_probe, + .driver = { + .name = "rzv2m_pwc", + .of_match_table = of_match_ptr(rzv2m_pwc_of_match), + }, +}; +module_platform_driver(rzv2m_pwc_driver); + +MODULE_SOFTDEP("post: gpio_rzv2m_pwc rzv2m_pwc_poweroff"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Fabrizio Castro <castro.fabrizio.jz@renesas.com>"); +MODULE_DESCRIPTION("Renesas RZ/V2M PWC core driver"); diff --git a/drivers/mfd/rzv2m-pwc.h b/drivers/mfd/rzv2m-pwc.h new file mode 100644 index 000000000000..8f3d777557c9 --- /dev/null +++ b/drivers/mfd/rzv2m-pwc.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2022 Renesas Electronics Corporation + */ + +#ifndef __LINUX_RZV2M_PWC_H__ +#define __LINUX_RZV2M_PWC_H__ + +#define PWC_PWCRST 0x00 +#define PWC_PWCCKEN 0x04 +#define PWC_PWCCTL 0x50 +#define PWC_GPIO 0x80 + +struct rzv2m_pwc_priv { + void __iomem *base; +}; + +#endif /* __LINUX_RZV2M_PWC_H__ */
The External Power Sequence Controller (PWC) IP (found in the RZ/V2M SoC) is a controller for external power supplies (regulators and power switches), and it supports the following features: it generates a power on/off sequence for external power supplies, it generates an on/off sequence for the LPDDR4 core power supply (LPVDD), it comes with General-Purpose Outputs, and it processes key input signals. The PWC is basically a Multi-Function Device (MFD), its software support comes with a core driver, and specialized drivers for its specific features. This patch adds the core driver for the RZ/V2M PWC IP. Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> --- v1->v2: This is a new driver, to match the relevant compatible string and instantiate the relevant mfd device drivers drivers/mfd/Kconfig | 14 +++++++++ drivers/mfd/Makefile | 1 + drivers/mfd/rzv2m-pwc.c | 70 +++++++++++++++++++++++++++++++++++++++++ drivers/mfd/rzv2m-pwc.h | 18 +++++++++++ 4 files changed, 103 insertions(+) create mode 100644 drivers/mfd/rzv2m-pwc.c create mode 100644 drivers/mfd/rzv2m-pwc.h