| Message ID | 1423671332-24580-2-git-send-email-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 11 Feb 2015, Antoine Tenart wrote: > Marvell Berlin SoC have two nodes providing multiple devices (clk, > pinctrl, reset). While until now these drivers were initialized using > initcalls, this wasn't a proper solution. This mfd driver will be > responsible of adding these devices, to be probed properly. > > It currently registers the pin controllers and the reset drivers for > BG2, BG2CD and BG2Q in the soc and system controller nodes. > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > drivers/mfd/Kconfig | 5 ++ > drivers/mfd/Makefile | 2 + > drivers/mfd/berlin-ctrl.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 187 insertions(+) > create mode 100644 drivers/mfd/berlin-ctrl.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 2e6b7311fabc..eda6dbec02ff 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -840,6 +840,11 @@ config STMPE_SPI > This is used to enable SPI interface of STMPE > endmenu > > +config MFD_BERLIN_CTRL > + bool Missing description. Why can't this driver be built as a module? > + select MFD_CORE > + select MFD_SYSCON Missing help. > config MFD_STA2X11 > bool "STMicroelectronics STA2X11" > depends on STA2X11 > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 53467e211381..adf60e85df20 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2) += dln2.o > > intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o > obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o > + > +obj-$(CONFIG_MFD_BERLIN_CTRL) += berlin-ctrl.o > diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c > new file mode 100644 > index 000000000000..e3ce6f069f16 > --- /dev/null > +++ b/drivers/mfd/berlin-ctrl.c > @@ -0,0 +1,180 @@ > +/* > + * Copyright (C) 2015 Marvell Technology Group Ltd. > + * > + * Antoine Tenart <antoine.tenart@free-electrons.com> Author: Antoine Tenart <antoine.tenart@free-electrons.com> > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/of.h> kernel.h? > +struct berlin_ctrl_priv { > + const struct mfd_cell *devs; > + unsigned ndevs; > +}; > + > +/* > + * BG2 devices > + */ > +static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = { > + { > + .name = "berlin2-soc-pinctrl", > + .of_compatible = "marvell,berlin2-soc-pinctrl", > + }, > + { > + .name = "berlin2-reset", > + .of_compatible = "marvell,berlin2-reset", > + }, > +}; > + > +static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = { > + { > + .name = "berlin2-system-pinctrl", > + .of_compatible = "marvell,berlin2-system-pinctrl", > + }, > +}; > + > +static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = { > + .devs = berlin2_ctrl_chip_ctrl_subdevs, > + .ndevs = ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs), > +}; > + > +static const struct berlin_ctrl_priv berlin2_ctrl_system_data = { > + .devs = berlin2_ctrl_system_ctrl_subdevs, > + .ndevs = ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs), > +}; > + > + Superfluous '\n' > +/* > + * BG2CD devices > + */ > +static const struct mfd_cell berlin2cd_ctrl_chip_ctrl_subdevs[] = { > + { > + .name = "berlin2cd-soc-pinctrl", > + .of_compatible = "marvell,berlin2cd-soc-pinctrl", > + }, > + { > + .name = "berlin2-reset", > + .of_compatible = "marvell,berlin2-reset", > + }, > +}; > + > +static const struct mfd_cell berlin2cd_ctrl_system_ctrl_subdevs[] = { > + { > + .name = "berlin2cd-system-pinctrl", > + .of_compatible = "marvell,berlin2cd-system-pinctrl", > + }, > +}; > + > +static const struct berlin_ctrl_priv berlin2cd_ctrl_chip_ctrl_data = { > + .devs = berlin2cd_ctrl_chip_ctrl_subdevs, > + .ndevs = ARRAY_SIZE(berlin2cd_ctrl_chip_ctrl_subdevs), > +}; > + > +static const struct berlin_ctrl_priv berlin2cd_ctrl_system_data = { > + .devs = berlin2cd_ctrl_system_ctrl_subdevs, > + .ndevs = ARRAY_SIZE(berlin2cd_ctrl_system_ctrl_subdevs), > +}; > + > +/* > + * BG2Q devices > + */ > +static const struct mfd_cell berlin2q_ctrl_chip_ctrl_subdevs[] = { > + { > + .name = "berlin2q-soc-pinctrl", > + .of_compatible = "marvell,berlin2q-soc-pinctrl", > + }, > + { > + .name = "berlin2-reset", > + .of_compatible = "marvell,berlin2-reset", > + }, > +}; > + > +static const struct mfd_cell berlin2q_ctrl_system_ctrl_subdevs[] = { > + { > + .name = "berlin2q-system-pinctrl", > + .of_compatible = "marvell,berlin2q-system-pinctrl", > + }, > +}; > + > +static const struct berlin_ctrl_priv berlin2q_ctrl_chip_ctrl_data = { > + .devs = berlin2q_ctrl_chip_ctrl_subdevs, > + .ndevs = ARRAY_SIZE(berlin2q_ctrl_chip_ctrl_subdevs), > +}; > + > +static const struct berlin_ctrl_priv berlin2q_ctrl_system_data = { > + .devs = berlin2q_ctrl_system_ctrl_subdevs, > + .ndevs = ARRAY_SIZE(berlin2q_ctrl_system_ctrl_subdevs), > +}; > + > + > +static const struct of_device_id berlin_ctrl_of_match[] = { > + /* BG2 */ > + { > + .compatible = "marvell,berlin2-chip-ctrl", > + .data = &berlin2_ctrl_chip_ctrl_data, > + }, > + { > + .compatible = "marvell,berlin2-system-ctrl", > + .data = &berlin2_ctrl_system_data, > + }, > + /* BG2CD */ > + { > + .compatible = "marvell,berlin2cd-chip-ctrl", > + .data = &berlin2cd_ctrl_chip_ctrl_data, > + }, > + { > + .compatible = "marvell,berlin2cd-system-ctrl", > + .data = &berlin2cd_ctrl_system_data, > + }, > + /* BG2Q */ > + { > + .compatible = "marvell,berlin2q-chip-ctrl", > + .data = &berlin2q_ctrl_chip_ctrl_data, > + }, > + { > + .compatible = "marvell,berlin2q-system-ctrl", > + .data = &berlin2q_ctrl_system_data, > + }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, berlin_ctrl_of_match); > + > +static int berlin_ctrl_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + const struct of_device_id *match; > + const struct berlin_ctrl_priv *priv; > + int ret; > + > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > + if (!match) > + return -EINVAL; > + > + priv = match->data; > + > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > + if (ret) { > + dev_err(dev, "Failed to add devices: %d\n", ret); > + return ret; > + } > + > + return 0; > +} I'm not sure I see the point in this driver. Why can't you just register these devices directly from DT? > +static struct platform_driver berlin_ctrl_driver = { > + .probe = berlin_ctrl_probe, > + .driver = { > + .name = "berlin-ctrl", > + .of_match_table = berlin_ctrl_of_match, > + }, > +}; > +module_platform_driver(berlin_ctrl_driver); > + > +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>"); > +MODULE_DESCRIPTION("Marvell Berlin controller driver (mfd)"); > +MODULE_LICENSE("GPL"); v2
Lee, On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote: > On Wed, 11 Feb 2015, Antoine Tenart wrote: > > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -840,6 +840,11 @@ config STMPE_SPI > > This is used to enable SPI interface of STMPE > > endmenu > > > > +config MFD_BERLIN_CTRL > > + bool > > Missing description. > Why can't this driver be built as a module? Well, this mfd driver registers various devices as the pinctrl and the reset driver. I think we want the pinctrl driver to always be registered. IMHO we want this driver to always be selected when using a Berlin SoC. > > > + select MFD_CORE > > + select MFD_SYSCON > > Missing help. > > > config MFD_STA2X11 > > bool "STMicroelectronics STA2X11" > > depends on STA2X11 > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 53467e211381..adf60e85df20 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2) += dln2.o > > > > intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o > > obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o > > + > > +obj-$(CONFIG_MFD_BERLIN_CTRL) += berlin-ctrl.o > > diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c > > new file mode 100644 > > index 000000000000..e3ce6f069f16 > > --- /dev/null > > +++ b/drivers/mfd/berlin-ctrl.c > > @@ -0,0 +1,180 @@ > > +/* > > + * Copyright (C) 2015 Marvell Technology Group Ltd. > > + * > > + * Antoine Tenart <antoine.tenart@free-electrons.com> > > Author: Antoine Tenart <antoine.tenart@free-electrons.com> Hmmm, okay. > > > + > > +#include <linux/mfd/core.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > kernel.h? Is there a reason to add this header here? > > +/* > > + * BG2 devices > > + */ > > +static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = { > > + { > > + .name = "berlin2-soc-pinctrl", > > + .of_compatible = "marvell,berlin2-soc-pinctrl", > > + }, > > + { > > + .name = "berlin2-reset", > > + .of_compatible = "marvell,berlin2-reset", > > + }, > > +}; > > + > > +static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = { > > + { > > + .name = "berlin2-system-pinctrl", > > + .of_compatible = "marvell,berlin2-system-pinctrl", > > + }, > > +}; > > + > > +static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = { > > + .devs = berlin2_ctrl_chip_ctrl_subdevs, > > + .ndevs = ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs), > > +}; > > + > > +static const struct berlin_ctrl_priv berlin2_ctrl_system_data = { > > + .devs = berlin2_ctrl_system_ctrl_subdevs, > > + .ndevs = ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs), > > +}; > > + > > + > > Superfluous '\n' Sure. > > > + > > +static int berlin_ctrl_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + const struct of_device_id *match; > > + const struct berlin_ctrl_priv *priv; > > + int ret; > > + > > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > > + if (!match) > > + return -EINVAL; > > + > > + priv = match->data; > > + > > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > > + if (ret) { > > + dev_err(dev, "Failed to add devices: %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > I'm not sure I see the point in this driver. Why can't you just > register these devices directly from DT? All these devices share the same bank of registers and we previously used a single node. But with many devices sharing a single node, this is problematic to register all the devices from DT. Using this MFD driver to do it is a proper solution in this case. To provide a regmap to the devices' drivers we also use syscon on the chip/system controller nodes. > > +MODULE_LICENSE("GPL"); > > v2 "GPL" is a valid choice, quoting include/linux.module.h: "GPL" [GNU Public License v2 or later] "GPL v2" [GNU Public License v2] Is there a reason you explicitly want to use GPLv2, and only GPLv2? Antoine
On Tue, 17 Feb 2015, Antoine Tenart wrote: > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote: > > On Wed, 11 Feb 2015, Antoine Tenart wrote: > > > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -840,6 +840,11 @@ config STMPE_SPI > > > This is used to enable SPI interface of STMPE > > > endmenu > > > > > > +config MFD_BERLIN_CTRL > > > + bool > > > > Missing description. > > Why can't this driver be built as a module? > > Well, this mfd driver registers various devices as the pinctrl and the > reset driver. I think we want the pinctrl driver to always be > registered. > > IMHO we want this driver to always be selected when using a Berlin SoC. [...] > > > +#include <linux/mfd/core.h> > > > +#include <linux/module.h> > > > +#include <linux/of.h> > > > > kernel.h? > > Is there a reason to add this header here? I guess not if you're not using any of its macros. Although, I don't recall every seeing s driver without it. I guess you learn something every day. [...] > > > +static int berlin_ctrl_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + const struct of_device_id *match; > > > + const struct berlin_ctrl_priv *priv; > > > + int ret; > > > + > > > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > > > + if (!match) > > > + return -EINVAL; > > > + > > > + priv = match->data; > > > + > > > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > > > + if (ret) { > > > + dev_err(dev, "Failed to add devices: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > > I'm not sure I see the point in this driver. Why can't you just > > register these devices directly from DT? > > All these devices share the same bank of registers and we previously > used a single node. But with many devices sharing a single node, this is > problematic to register all the devices from DT. Using this MFD driver > to do it is a proper solution in this case. Tell me more. What are the problems you encountered? > To provide a regmap to the devices' drivers we also use syscon on the > chip/system controller nodes. > > > > +MODULE_LICENSE("GPL"); I wonder if these are actually useful if you're always going to be built-in? > > > +MODULE_LICENSE("GPL"); > > > > v2 > > "GPL" is a valid choice, quoting include/linux.module.h: > > "GPL" [GNU Public License v2 or later] > "GPL v2" [GNU Public License v2] > > Is there a reason you explicitly want to use GPLv2, and only GPLv2? Yes, there is disparity between this and the comment in the file header. I don't mind which one you amend, but a change is required.
On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote: > On Tue, 17 Feb 2015, Antoine Tenart wrote: > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote: > > > On Wed, 11 Feb 2015, Antoine Tenart wrote: > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev) > > > > +{ > > > > + struct device *dev = &pdev->dev; > > > > + const struct of_device_id *match; > > > > + const struct berlin_ctrl_priv *priv; > > > > + int ret; > > > > + > > > > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > > > > + if (!match) > > > > + return -EINVAL; > > > > + > > > > + priv = match->data; > > > > + > > > > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > > > > + if (ret) { > > > > + dev_err(dev, "Failed to add devices: %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > > > I'm not sure I see the point in this driver. Why can't you just > > > register these devices directly from DT? > > > > All these devices share the same bank of registers and we previously > > used a single node. But with many devices sharing a single node, this is > > problematic to register all the devices from DT. Using this MFD driver > > to do it is a proper solution in this case. > > Tell me more. What are the problems you encountered? So we had a single node, chip-controller, accessed by multiple devices -and drivers-. We ended up with: chip: chip-control@ea0000 { compatible = "marvell,berlin2q-chip-ctrl"; reg = <0xea0000 0x400>, <0xdd0170 0x10>; #clock-cells = <1>; #reset-cells = <2>; clocks = <&refclk>; clock-names = "refclk"; [pinmux nodes] }; In addition to being a mess, how can you probe various drivers with this single node? We had to probe a clock driver in addition to the pin-controller and reset drivers. We ended up using arch_initcall() in the reset driver, which was *not* acceptable. These chip and system controllers are not an IP, but helps not spreading this bank of registers all over the DT. The solution to this problem is to introduce an mtd driver which registers all the sub-devices described by these chip and system controller nodes. Antoine
On Wed, 18 Feb 2015, Antoine Tenart wrote: > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote: > > On Tue, 17 Feb 2015, Antoine Tenart wrote: > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote: > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote: > > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev) > > > > > +{ > > > > > + struct device *dev = &pdev->dev; > > > > > + const struct of_device_id *match; > > > > > + const struct berlin_ctrl_priv *priv; > > > > > + int ret; > > > > > + > > > > > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > > > > > + if (!match) > > > > > + return -EINVAL; > > > > > + > > > > > + priv = match->data; > > > > > + > > > > > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > > > > > + if (ret) { > > > > > + dev_err(dev, "Failed to add devices: %d\n", ret); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > > > > I'm not sure I see the point in this driver. Why can't you just > > > > register these devices directly from DT? > > > > > > All these devices share the same bank of registers and we previously > > > used a single node. But with many devices sharing a single node, this is > > > problematic to register all the devices from DT. Using this MFD driver > > > to do it is a proper solution in this case. > > > > Tell me more. What are the problems you encountered? > > So we had a single node, chip-controller, accessed by multiple > devices -and drivers-. We ended up with: > > chip: chip-control@ea0000 { > compatible = "marvell,berlin2q-chip-ctrl"; > reg = <0xea0000 0x400>, <0xdd0170 0x10>; > #clock-cells = <1>; > #reset-cells = <2>; > clocks = <&refclk>; > clock-names = "refclk"; > > [pinmux nodes] > }; > > In addition to being a mess, how can you probe various drivers with this > single node? We had to probe a clock driver in addition to the > pin-controller and reset drivers. We ended up using arch_initcall() in > the reset driver, which was *not* acceptable. > > These chip and system controllers are not an IP, but helps not spreading > this bank of registers all over the DT. > > The solution to this problem is to introduce an mtd driver which > registers all the sub-devices described by these chip and system > controller nodes. I'm still not convinced that your problem can't be solved in DT, but creating a single psudo-hardware node is not correct either. What does the h/w _really_ look like? Is all of this stuff on a single chip? If so, I would expect to see something like: control@ea0000 { compatible = "marvel,control"; pinctrl@xxxxx { compatible = "marvel,pinctrl"; }; reset@xxxxx { compatible = "marvel,reset"; }; };
On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote: > On Wed, 18 Feb 2015, Antoine Tenart wrote: > > > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote: > > > On Tue, 17 Feb 2015, Antoine Tenart wrote: > > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote: > > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote: > > > > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev) > > > > > > +{ > > > > > > + struct device *dev = &pdev->dev; > > > > > > + const struct of_device_id *match; > > > > > > + const struct berlin_ctrl_priv *priv; > > > > > > + int ret; > > > > > > + > > > > > > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > > > > > > + if (!match) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + priv = match->data; > > > > > > + > > > > > > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > > > > > > + if (ret) { > > > > > > + dev_err(dev, "Failed to add devices: %d\n", ret); > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > > > > > I'm not sure I see the point in this driver. Why can't you just > > > > > register these devices directly from DT? > > > > > > > > All these devices share the same bank of registers and we previously > > > > used a single node. But with many devices sharing a single node, this is > > > > problematic to register all the devices from DT. Using this MFD driver > > > > to do it is a proper solution in this case. > > > > > > Tell me more. What are the problems you encountered? > > > > So we had a single node, chip-controller, accessed by multiple > > devices -and drivers-. We ended up with: > > > > chip: chip-control@ea0000 { > > compatible = "marvell,berlin2q-chip-ctrl"; > > reg = <0xea0000 0x400>, <0xdd0170 0x10>; > > #clock-cells = <1>; > > #reset-cells = <2>; > > clocks = <&refclk>; > > clock-names = "refclk"; > > > > [pinmux nodes] > > }; > > > > In addition to being a mess, how can you probe various drivers with this > > single node? We had to probe a clock driver in addition to the > > pin-controller and reset drivers. We ended up using arch_initcall() in > > the reset driver, which was *not* acceptable. > > > > These chip and system controllers are not an IP, but helps not spreading > > this bank of registers all over the DT. > > > > The solution to this problem is to introduce an mtd driver which > > registers all the sub-devices described by these chip and system > > controller nodes. > > I'm still not convinced that your problem can't be solved in DT, but > creating a single psudo-hardware node is not correct either. What > does the h/w _really_ look like? Is all of this stuff on a single > chip? There is no specific IP for these registers, but we do not want them spread all over the DT nodes so they're summed up into this chip node. SO we have all those registers into a chip/system node and sub-nodes for the devices using them. > If so, I would expect to see something like: > > control@ea0000 { > compatible = "marvel,control"; > > pinctrl@xxxxx { > compatible = "marvel,pinctrl"; > }; > > reset@xxxxx { > compatible = "marvel,reset"; > }; > }; That's exactly the point of this series: having one sub-node per device. With this series applied, we have (the clock being a sub-node of the chip-controller node is part of another series following this one): chip: chip-controller@ea0000 { compatible = "marvell,berlin2q-chip-ctrl", "syscon"; reg = <0xea0000 0x400>, <0xdd0170 0x10>; #clock-cells = <1>; clocks = <&refclk>; clock-names = "refclk"; soc_pinctrl: pin-controller { compatible = "marvell,berlin2q-soc-pinctrl"; twsi0_pmux: twsi0-pmux { groups = "G6"; function = "twsi0"; }; twsi1_pmux: twsi1-pmux { groups = "G7"; function = "twsi1"; }; }; chip_rst: reset { compatible = "marvell,berlin2-reset"; #reset-cells = <2>; }; }; Antoine
On 18.02.2015 10:09, Lee Jones wrote: > On Wed, 18 Feb 2015, Antoine Tenart wrote: [...] >> So we had a single node, chip-controller, accessed by multiple >> devices -and drivers-. We ended up with: >> >> chip: chip-control@ea0000 { >> compatible = "marvell,berlin2q-chip-ctrl"; >> reg = <0xea0000 0x400>, <0xdd0170 0x10>; >> #clock-cells = <1>; >> #reset-cells = <2>; >> clocks = <&refclk>; >> clock-names = "refclk"; >> >> [pinmux nodes] >> }; >> >> In addition to being a mess, how can you probe various drivers with this >> single node? We had to probe a clock driver in addition to the >> pin-controller and reset drivers. We ended up using arch_initcall() in >> the reset driver, which was *not* acceptable. >> >> These chip and system controllers are not an IP, but helps not spreading >> this bank of registers all over the DT. >> >> The solution to this problem is to introduce an mtd driver which >> registers all the sub-devices described by these chip and system >> controller nodes. > > I'm still not convinced that your problem can't be solved in DT, but > creating a single psudo-hardware node is not correct either. What > does the h/w _really_ look like? Is all of this stuff on a single > chip? If so, I would expect to see something like: > > control@ea0000 { > compatible = "marvel,control"; > > pinctrl@xxxxx { > compatible = "marvel,pinctrl"; > }; > > reset@xxxxx { > compatible = "marvel,reset"; > }; > }; Lee, I guess you never get what you expect ;) Honestly, Antoine is right. The structure you describe above is a bus and that is definitely not true for chip control registers. Also, clock, reset, and pinctrl "units" are SW concepts that HW engineers don't care much about. Each of the "units" is heavily spread among the chip control registers and even within a single register. We simply give up on carving out every single register range to squeeze them into SW driven units. I think Marvell understood that this kind of chip control dumpster puts a high burden on the SW guys and newer SoCs will have a cleaner register set - but for the current SoCs we are stuck with the situation. So, what _sane_ options do we have in DT that is also sane for SW? (a) We could follow your suggestion of a chipctrl bus or move the nodes back to the SoC bus. Now with that we would end up with reg = <0x000 0x4>, <0x024 0x10>, <0x78 0x8>, ... for each of the nodes. Plus, we'll certainly have to overlap some of the reg ranges because bit0 of one register is reset but bits 31:1 is clock. This not only exposes the mess in DT but still we have to deal with it in the driver. Also as soon as we overlap, we loose devm_ for the ioremap. (b) We follow Antoine's proposal and have a single chip-ctrl node with a single reg property spanning over the whole register set. Then have clock, pinctrl, reset as sub-nodes. Looks sane, but we need some SW that hooks up to each of the nodes, i.e. as it is not a bus we have to register devices for each sub-node ourselves. That is why mfd is used here (and for sunxi SoCs BTW too). So, back when we wrote the clock driver and tried to find a sane representation of the corresponding registers, Mike finally suggested to just have a single node and deal with the register mess in the driver. This series goes a little further and provides a single regmap to all sub-drivers that we can think of that have to deal with chip ctrl registers. We have looked at the register layout and corresponding driver concepts over and over again - there is no way to chop this up. Sebastian
On Wed, 18 Feb 2015, Antoine Tenart wrote: > On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote: > > On Wed, 18 Feb 2015, Antoine Tenart wrote: > > > > > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote: > > > > On Tue, 17 Feb 2015, Antoine Tenart wrote: > > > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote: > > > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote: > > > > > > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev) > > > > > > > +{ > > > > > > > + struct device *dev = &pdev->dev; > > > > > > > + const struct of_device_id *match; > > > > > > > + const struct berlin_ctrl_priv *priv; > > > > > > > + int ret; > > > > > > > + > > > > > > > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > > > > > > > + if (!match) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > + priv = match->data; > > > > > > > + > > > > > > > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > > > > > > > + if (ret) { > > > > > > > + dev_err(dev, "Failed to add devices: %d\n", ret); > > > > > > > + return ret; > > > > > > > + } > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > > > > > > I'm not sure I see the point in this driver. Why can't you just > > > > > > register these devices directly from DT? > > > > > > > > > > All these devices share the same bank of registers and we previously > > > > > used a single node. But with many devices sharing a single node, this is > > > > > problematic to register all the devices from DT. Using this MFD driver > > > > > to do it is a proper solution in this case. > > > > > > > > Tell me more. What are the problems you encountered? > > > > > > So we had a single node, chip-controller, accessed by multiple > > > devices -and drivers-. We ended up with: > > > > > > chip: chip-control@ea0000 { > > > compatible = "marvell,berlin2q-chip-ctrl"; > > > reg = <0xea0000 0x400>, <0xdd0170 0x10>; > > > #clock-cells = <1>; > > > #reset-cells = <2>; > > > clocks = <&refclk>; > > > clock-names = "refclk"; > > > > > > [pinmux nodes] > > > }; > > > > > > In addition to being a mess, how can you probe various drivers with this > > > single node? We had to probe a clock driver in addition to the > > > pin-controller and reset drivers. We ended up using arch_initcall() in > > > the reset driver, which was *not* acceptable. > > > > > > These chip and system controllers are not an IP, but helps not spreading > > > this bank of registers all over the DT. > > > > > > The solution to this problem is to introduce an mtd driver which > > > registers all the sub-devices described by these chip and system > > > controller nodes. > > > > I'm still not convinced that your problem can't be solved in DT, but > > creating a single psudo-hardware node is not correct either. What > > does the h/w _really_ look like? Is all of this stuff on a single > > chip? > > There is no specific IP for these registers, but we do not want them > spread all over the DT nodes so they're summed up into this chip node. > > SO we have all those registers into a chip/system node and sub-nodes for > the devices using them. > > > If so, I would expect to see something like: > > > > control@ea0000 { > > compatible = "marvel,control"; > > > > pinctrl@xxxxx { > > compatible = "marvel,pinctrl"; > > }; > > > > reset@xxxxx { > > compatible = "marvel,reset"; > > }; > > }; > > That's exactly the point of this series: having one sub-node per device. > > With this series applied, we have (the clock being a sub-node of the > chip-controller node is part of another series following this one): > > chip: chip-controller@ea0000 { > compatible = "marvell,berlin2q-chip-ctrl", "syscon"; > reg = <0xea0000 0x400>, <0xdd0170 0x10>; > #clock-cells = <1>; > clocks = <&refclk>; > clock-names = "refclk"; > > soc_pinctrl: pin-controller { > compatible = "marvell,berlin2q-soc-pinctrl"; > > twsi0_pmux: twsi0-pmux { > groups = "G6"; > function = "twsi0"; > }; > > twsi1_pmux: twsi1-pmux { > groups = "G7"; > function = "twsi1"; > }; > }; > > chip_rst: reset { > compatible = "marvell,berlin2-reset"; > #reset-cells = <2>; > }; > }; This is what I'd expect to see in DT, so we're heading in the right direction. So make to my original question, what's the point of this MFD driver, and why don't you just let DT framework register these devices for you? You issue a compatible string here, then duplicate it in the driver, why do you think this is necessary?
On Wed, Feb 18, 2015 at 10:40:23AM +0000, Lee Jones wrote: > On Wed, 18 Feb 2015, Antoine Tenart wrote: > > > On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote: > > > On Wed, 18 Feb 2015, Antoine Tenart wrote: > > > > > > > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote: > > > > > On Tue, 17 Feb 2015, Antoine Tenart wrote: > > > > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote: > > > > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote: > > > > > > > > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev) > > > > > > > > +{ > > > > > > > > + struct device *dev = &pdev->dev; > > > > > > > > + const struct of_device_id *match; > > > > > > > > + const struct berlin_ctrl_priv *priv; > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + match = of_match_node(berlin_ctrl_of_match, dev->of_node); > > > > > > > > + if (!match) > > > > > > > > + return -EINVAL; > > > > > > > > + > > > > > > > > + priv = match->data; > > > > > > > > + > > > > > > > > + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); > > > > > > > > + if (ret) { > > > > > > > > + dev_err(dev, "Failed to add devices: %d\n", ret); > > > > > > > > + return ret; > > > > > > > > + } > > > > > > > > + > > > > > > > > + return 0; > > > > > > > > +} > > > > > > > > > > > > > > I'm not sure I see the point in this driver. Why can't you just > > > > > > > register these devices directly from DT? > > > > > > > > > > > > All these devices share the same bank of registers and we previously > > > > > > used a single node. But with many devices sharing a single node, this is > > > > > > problematic to register all the devices from DT. Using this MFD driver > > > > > > to do it is a proper solution in this case. > > > > > > > > > > Tell me more. What are the problems you encountered? > > > > > > > > So we had a single node, chip-controller, accessed by multiple > > > > devices -and drivers-. We ended up with: > > > > > > > > chip: chip-control@ea0000 { > > > > compatible = "marvell,berlin2q-chip-ctrl"; > > > > reg = <0xea0000 0x400>, <0xdd0170 0x10>; > > > > #clock-cells = <1>; > > > > #reset-cells = <2>; > > > > clocks = <&refclk>; > > > > clock-names = "refclk"; > > > > > > > > [pinmux nodes] > > > > }; > > > > > > > > In addition to being a mess, how can you probe various drivers with this > > > > single node? We had to probe a clock driver in addition to the > > > > pin-controller and reset drivers. We ended up using arch_initcall() in > > > > the reset driver, which was *not* acceptable. > > > > > > > > These chip and system controllers are not an IP, but helps not spreading > > > > this bank of registers all over the DT. > > > > > > > > The solution to this problem is to introduce an mtd driver which > > > > registers all the sub-devices described by these chip and system > > > > controller nodes. > > > > > > I'm still not convinced that your problem can't be solved in DT, but > > > creating a single psudo-hardware node is not correct either. What > > > does the h/w _really_ look like? Is all of this stuff on a single > > > chip? > > > > There is no specific IP for these registers, but we do not want them > > spread all over the DT nodes so they're summed up into this chip node. > > > > SO we have all those registers into a chip/system node and sub-nodes for > > the devices using them. > > > > > If so, I would expect to see something like: > > > > > > control@ea0000 { > > > compatible = "marvel,control"; > > > > > > pinctrl@xxxxx { > > > compatible = "marvel,pinctrl"; > > > }; > > > > > > reset@xxxxx { > > > compatible = "marvel,reset"; > > > }; > > > }; > > > > That's exactly the point of this series: having one sub-node per device. > > > > With this series applied, we have (the clock being a sub-node of the > > chip-controller node is part of another series following this one): > > > > chip: chip-controller@ea0000 { > > compatible = "marvell,berlin2q-chip-ctrl", "syscon"; > > reg = <0xea0000 0x400>, <0xdd0170 0x10>; > > #clock-cells = <1>; > > clocks = <&refclk>; > > clock-names = "refclk"; > > > > soc_pinctrl: pin-controller { > > compatible = "marvell,berlin2q-soc-pinctrl"; > > > > twsi0_pmux: twsi0-pmux { > > groups = "G6"; > > function = "twsi0"; > > }; > > > > twsi1_pmux: twsi1-pmux { > > groups = "G7"; > > function = "twsi1"; > > }; > > }; > > > > chip_rst: reset { > > compatible = "marvell,berlin2-reset"; > > #reset-cells = <2>; > > }; > > }; > > This is what I'd expect to see in DT, so we're heading in the right > direction. So make to my original question, what's the point of this > MFD driver, and why don't you just let DT framework register these > devices for you? > > You issue a compatible string here, then duplicate it in the driver, > why do you think this is necessary? The chip-controller node is *not* a bus. Please have a look on Sebastian's answer. Antoine
On 18.02.2015 11:40, Lee Jones wrote: > On Wed, 18 Feb 2015, Antoine Tenart wrote: [...] >> chip: chip-controller@ea0000 { >> compatible = "marvell,berlin2q-chip-ctrl", "syscon"; >> reg = <0xea0000 0x400>, <0xdd0170 0x10>; >> #clock-cells = <1>; >> clocks = <&refclk>; >> clock-names = "refclk"; >> >> soc_pinctrl: pin-controller { >> compatible = "marvell,berlin2q-soc-pinctrl"; >> >> twsi0_pmux: twsi0-pmux { >> groups = "G6"; >> function = "twsi0"; >> }; >> >> twsi1_pmux: twsi1-pmux { >> groups = "G7"; >> function = "twsi1"; >> }; >> }; >> >> chip_rst: reset { >> compatible = "marvell,berlin2-reset"; >> #reset-cells = <2>; >> }; >> }; > > This is what I'd expect to see in DT, so we're heading in the right > direction. So make to my original question, what's the point of this > MFD driver, and why don't you just let DT framework register these > devices for you? > > You issue a compatible string here, then duplicate it in the driver, > why do you think this is necessary? Lee, there is no DT framework that automatically probes for compatible<->driver matches. You either make it "simple-bus" compatible which will call of_foo_populate() or you have to register each of the devices yourself. It clearly is not a bus, so if we use this as a workaround, we'll get yelled at by others. Sebastian
On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote: > On 18.02.2015 11:40, Lee Jones wrote: > >On Wed, 18 Feb 2015, Antoine Tenart wrote: > [...] > >>chip: chip-controller@ea0000 { > >> compatible = "marvell,berlin2q-chip-ctrl", "syscon"; > >> reg = <0xea0000 0x400>, <0xdd0170 0x10>; > >> #clock-cells = <1>; > >> clocks = <&refclk>; > >> clock-names = "refclk"; > >> > >> soc_pinctrl: pin-controller { > >> compatible = "marvell,berlin2q-soc-pinctrl"; > >> > >> twsi0_pmux: twsi0-pmux { > >> groups = "G6"; > >> function = "twsi0"; > >> }; > >> > >> twsi1_pmux: twsi1-pmux { > >> groups = "G7"; > >> function = "twsi1"; > >> }; > >> }; > >> > >> chip_rst: reset { > >> compatible = "marvell,berlin2-reset"; > >> #reset-cells = <2>; > >> }; > >>}; > > > >This is what I'd expect to see in DT, so we're heading in the right > >direction. So make to my original question, what's the point of this > >MFD driver, and why don't you just let DT framework register these > >devices for you? > > > >You issue a compatible string here, then duplicate it in the driver, > >why do you think this is necessary? > > there is no DT framework that automatically probes for > compatible<->driver matches. You either make it "simple-bus" compatible > which will call of_foo_populate() or you have to register each of the > devices yourself. It clearly is not a bus, so if we use this as a > workaround, we'll get yelled at by others. I do agree that using 'simple-bus' to describe only this IP would be an abuse. However, my foundation thought/argument is unchanged. This 'driver' is a hack. It has no functional use besides to work around a problem of semantics and as such has no place in MFD. Back onto the simple-bus theme, as this is a syscon device it is a bus of sorts. Have you thought about making it a child of your its syscon node, then using simple-bus to get the OF framework to register the child devices?
On 02/18/2015 12:58 PM, Lee Jones wrote: > I do agree that using 'simple-bus' to describe only this IP would be > an abuse. However, my foundation thought/argument is unchanged. This > 'driver' is a hack. It has no functional use besides to work around a > problem of semantics and as such has no place in MFD. Lee, sorry I don't get it. Here you say that using simple-bus is an abuse... > Back onto the simple-bus theme, as this is a syscon device it is a bus > of sorts. Have you thought about making it a child of your its syscon > node, then using simple-bus to get the OF framework to register the > child devices? ... and here you suggest to use simple-bus to register the child devices? I fundamentally disagree that either this registers or syscon in general should in any way be seen as a bus. The chip control registers is an highly unsorted bunch of bits that we try to match with cleanly separated subsystems. This makes it a resource but no bus of any sort. The problem that we try to solve here is not a DT problem but solely driven by the fact that we need something to register platform_devices for pinctrl and reset. The unit we describe in DT is a pinctrl-clock- power-reset-unit - or short chip control. If you argue that mfd is not the right place for this "driver" we'll have to find a different place for it. I remember Mike has no problem with extending early probed clock drivers to register additional platform_devices - so I guess we end up putting it in there ignoring mfd's ability to do it for us. Do we agree on that? Sebastian
On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote: > On 02/18/2015 12:58 PM, Lee Jones wrote: > >I do agree that using 'simple-bus' to describe only this IP would be > >an abuse. However, my foundation thought/argument is unchanged. This > >'driver' is a hack. It has no functional use besides to work around a > >problem of semantics and as such has no place in MFD. > > Lee, > > sorry I don't get it. Here you say that using simple-bus is an abuse... > > >Back onto the simple-bus theme, as this is a syscon device it is a bus > >of sorts. Have you thought about making it a child of your its syscon > >node, then using simple-bus to get the OF framework to register the > >child devices? > > ... and here you suggest to use simple-bus to register the child > devices? Nope, that's not what I said: "I do agree that using 'simple-bus' to describe *ONLY THIS IP* would be an abuse." ... although I believe there is a need to treat syscon devices as simple buses. There are examples of devices doing this already: git grep -El 'syscon.*simple-bus' next/master next/master:arch/arm/boot/dts/imx6qdl.dtsi next/master:arch/arm/boot/dts/imx6sl.dtsi next/master:arch/arm/boot/dts/imx6sx.dtsi next/master:arch/arm/boot/dts/zynq-7000.dtsi > I fundamentally disagree that either this registers or syscon in general > should in any way be seen as a bus. The chip control registers is an > highly unsorted bunch of bits that we try to match with cleanly > separated subsystems. This makes it a resource but no bus of any sort. This is where my comment about semantics comes into play. syscon may not be a bus is the truest sense; however, this is clearly a requirement for sub devices to be probed in the same way a simple-bus is currently. So we're just missing a framework somewhere. We can fix that. > The problem that we try to solve here is not a DT problem but solely > driven by the fact that we need something to register platform_devices > for pinctrl and reset. The unit we describe in DT is a pinctrl-clock- > power-reset-unit - or short chip control. I agree with the last part, but this is a DT problem. It lacks the functionality to be able to cleanly register these types of (sub-)devices. Devices which, in my opinion should be described inside the parent syscon node. > If you argue that mfd is not the right place for this "driver" we'll > have to find a different place for it. I remember Mike has no problem > with extending early probed clock drivers to register additional > platform_devices - so I guess we end up putting it in there ignoring > mfd's ability to do it for us. My argument is not that this fake driver doesn't belong in MFD, it's that it doesn't belong. That includes shoving it in drivers/clk. I will be only too happy to have a chat with Mike and provide him with my reasons why. What I think we should do however, it write some framework code which can neatly handle these use-cases, which may just be a case of: s/of_platform_bus_probe/of_platform_subdevice_probe/ ... obviously I'm oversimplifying by quite some margin, but I'm sure you catch my drift.
On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote: > On 02/18/2015 12:58 PM, Lee Jones wrote: > > I do agree that using 'simple-bus' to describe only this IP would be > > an abuse. However, my foundation thought/argument is unchanged. This > > 'driver' is a hack. It has no functional use besides to work around a > > problem of semantics and as such has no place in MFD. > > Lee, > > sorry I don't get it. Here you say that using simple-bus is an abuse... > > > Back onto the simple-bus theme, as this is a syscon device it is a bus > > of sorts. Have you thought about making it a child of your its syscon > > node, then using simple-bus to get the OF framework to register the > > child devices? > > ... and here you suggest to use simple-bus to register the child > devices? > > I fundamentally disagree that either this registers or syscon in general > should in any way be seen as a bus. The chip control registers is an > highly unsorted bunch of bits that we try to match with cleanly > separated subsystems. This makes it a resource but no bus of any sort. It really depends on what you mean by 'bus'. It's certainly not a bus_type in Linux, but if you have a node in DT with multiple children, we sometimes think of that as a bus. I believe it makes sense to have the child devices under the syscon node here, at least the ones that have no other registers. > The problem that we try to solve here is not a DT problem but solely > driven by the fact that we need something to register platform_devices > for pinctrl and reset. The unit we describe in DT is a pinctrl-clock- > power-reset-unit - or short chip control. There are two problems here that we need to look at separately, even though there is some interaction: * For the DT representation, we need to make it something that corresponds to the hardware. We could either have a single device node for the set of registers, or we could have one node for each child. With syscon, we could also put the functional device nodes somewhere else, which we have to do if any device uses multiple syscon parents. * For the driver code, we need a way to fit into the kernel model while at the same time using the information that is in DT. I agree with Lee that your current driver is not a good solution here: you create a driver for the parent device that knows what child devices there are, which is not a good abstraction. Instead we should have a way for the child devices to get probed automatically, just like we do for simple-bus, whether we use simple-bus or not. > If you argue that mfd is not the right place for this "driver" we'll > have to find a different place for it. I remember Mike has no problem > with extending early probed clock drivers to register additional > platform_devices - so I guess we end up putting it in there ignoring > mfd's ability to do it for us. If you have a driver that is responsible for the entire register area, I think it would be best to make that driver just register to all the subsystems itself rather than creating child devices. The alternative is to come up with a way to probe all the child devices automatically, but then we should make that parent device have a generic driver that does not need to know about the children and that can work on any platform with similar requirements. Arnd
On Wed, 18 Feb 2015, Lee Jones wrote: > On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote: > > > On 02/18/2015 12:58 PM, Lee Jones wrote: > > >I do agree that using 'simple-bus' to describe only this IP would be > > >an abuse. However, my foundation thought/argument is unchanged. This > > >'driver' is a hack. It has no functional use besides to work around a > > >problem of semantics and as such has no place in MFD. > > > > Lee, > > > > sorry I don't get it. Here you say that using simple-bus is an abuse... > > > > >Back onto the simple-bus theme, as this is a syscon device it is a bus > > >of sorts. Have you thought about making it a child of your its syscon > > >node, then using simple-bus to get the OF framework to register the > > >child devices? > > > > ... and here you suggest to use simple-bus to register the child > > devices? > > Nope, that's not what I said: > > "I do agree that using 'simple-bus' to describe *ONLY THIS IP* would > be an abuse." > > ... although I believe there is a need to treat syscon devices as > simple buses. There are examples of devices doing this already: > > git grep -El 'syscon.*simple-bus' next/master > next/master:arch/arm/boot/dts/imx6qdl.dtsi > next/master:arch/arm/boot/dts/imx6sl.dtsi > next/master:arch/arm/boot/dts/imx6sx.dtsi > next/master:arch/arm/boot/dts/zynq-7000.dtsi > > > I fundamentally disagree that either this registers or syscon in general > > should in any way be seen as a bus. The chip control registers is an > > highly unsorted bunch of bits that we try to match with cleanly > > separated subsystems. This makes it a resource but no bus of any sort. > > This is where my comment about semantics comes into play. syscon may > not be a bus is the truest sense; however, this is clearly a > requirement for sub devices to be probed in the same way a simple-bus > is currently. So we're just missing a framework somewhere. We can > fix that. > > > The problem that we try to solve here is not a DT problem but solely > > driven by the fact that we need something to register platform_devices > > for pinctrl and reset. The unit we describe in DT is a pinctrl-clock- > > power-reset-unit - or short chip control. > > I agree with the last part, but this is a DT problem. It lacks the > functionality to be able to cleanly register these types of > (sub-)devices. Devices which, in my opinion should be described > inside the parent syscon node. > > > If you argue that mfd is not the right place for this "driver" we'll > > have to find a different place for it. I remember Mike has no problem > > with extending early probed clock drivers to register additional > > platform_devices - so I guess we end up putting it in there ignoring > > mfd's ability to do it for us. > > My argument is not that this fake driver doesn't belong in MFD, it's > that it doesn't belong. That includes shoving it in drivers/clk. I > will be only too happy to have a chat with Mike and provide him with > my reasons why. > > What I think we should do however, it write some framework code which > can neatly handle these use-cases, which may just be a case of: > > s/of_platform_bus_probe/of_platform_subdevice_probe/ > > ... obviously I'm oversimplifying by quite some margin, but I'm sure > you catch my drift. I should extend that a little. In the meantime I certainly wouldn't have a problem with you using the "syscon", "simple-bus" approach as others have done already.
On 02/18/2015 04:06 PM, Arnd Bergmann wrote: > On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote: >> I fundamentally disagree that either this registers or syscon in general >> should in any way be seen as a bus. The chip control registers is an >> highly unsorted bunch of bits that we try to match with cleanly >> separated subsystems. This makes it a resource but no bus of any sort. > > It really depends on what you mean by 'bus'. It's certainly not a bus_type > in Linux, but if you have a node in DT with multiple children, we > sometimes think of that as a bus. I believe it makes sense to have > the child devices under the syscon node here, at least the ones that > have no other registers. Arnd, Lee, First of all, I think I get the points of both of you. With 'bus' I was referring to anything that implies a fixed number to address any of the sub-units. As the register is more or less unsorted and unordered, I'd see it as a resource - but that isn't important really. >> The problem that we try to solve here is not a DT problem but solely >> driven by the fact that we need something to register platform_devices >> for pinctrl and reset. The unit we describe in DT is a pinctrl-clock- >> power-reset-unit - or short chip control. > > There are two problems here that we need to look at separately, > even though there is some interaction: > > * For the DT representation, we need to make it something that > corresponds to the hardware. We could either have a single device > node for the set of registers, or we could have one node for each > child. With syscon, we could also put the functional device nodes > somewhere else, which we have to do if any device uses multiple > syscon parents. Well, during the discussion, I think we can also get along with a single node for the whole chip-registers - even in DT. Clock and reset just require corresponding #foo-cells and pinctrl will have its pinmux sub-nodes right in that single node. If we have separate sub-nodes for each of the subsystems is mainly a matter of taste, right? > * For the driver code, we need a way to fit into the kernel model > while at the same time using the information that is in DT. > I agree with Lee that your current driver is not a good solution > here: you create a driver for the parent device that knows what > child devices there are, which is not a good abstraction. Instead > we should have a way for the child devices to get probed automatically, > just like we do for simple-bus, whether we use simple-bus or not. With no sub-nodes, we'd have to have a driver that knows the linux subsystem platform_devices. With sub-nodes, you are proposing a "syscon-bus"-like compatible hook to populate sub-devices. Ok, I get this. >> If you argue that mfd is not the right place for this "driver" we'll >> have to find a different place for it. I remember Mike has no problem >> with extending early probed clock drivers to register additional >> platform_devices - so I guess we end up putting it in there ignoring >> mfd's ability to do it for us. > > If you have a driver that is responsible for the entire register > area, I think it would be best to make that driver just register > to all the subsystems itself rather than creating child devices. Hmm. That would create a beast of a driver wouldn't it? We could get along with a library-like structure we each of foo-related code resides in drivers/foo but still we'd need some common include to reference to the sub-driver. > The alternative is to come up with a way to probe all the child > devices automatically, but then we should make that parent device > have a generic driver that does not need to know about the children > and that can work on any platform with similar requirements. Ok, this is most likely the part that Lee doesn't like on the current driver: a platform_device for registering platform_devices *only* and only for Berlin. So, out of the two options: (a) Go for syscon_of_populate_devices() with a new compatible (I guess) and having sub-nodes for each Linux subsystem that we want to have a platform_device for. I fear that this will clash with early registration of clk and we still have to find a way, i.e. device naming policy, to match the drivers with their devices. (b) Join clk, pinctrl, reset into a single chip/soc-control node and rewrite the sub-drivers to not directly rely on DT compatible. With this, joining all sub-drivers into drivers/soc/berlin would be a sane approach, right? Also, I have the strong feeling, that we will encounter situations later that will require the clk driver to pull a reset before changing a specific clk rate, e.g. for GPU. Anyway, I appreciate your discussion but still I am a little lost between the two options. I thought that Antoine's mfd approach is good, but I understand your concerns. Any direction we should go for? Sebastian
On Wednesday 18 February 2015 16:59:42 Sebastian Hesselbarth wrote: > > > The alternative is to come up with a way to probe all the child > > devices automatically, but then we should make that parent device > > have a generic driver that does not need to know about the children > > and that can work on any platform with similar requirements. > > Ok, this is most likely the part that Lee doesn't like on the current > driver: a platform_device for registering platform_devices *only* and > only for Berlin. > > So, out of the two options: > > (a) Go for syscon_of_populate_devices() with a new compatible (I guess) > and having sub-nodes for each Linux subsystem that we want to have > a platform_device for. I fear that this will clash with early > registration of clk and we still have to find a way, i.e. device > naming policy, to match the drivers with their devices. I don't see the problem with early clk registration, AFAICT it should just work as expected, you just end up with an extra platform_device for the clocks that does not get bound to a driver later because the device node is already in use by the clock driver. > (b) Join clk, pinctrl, reset into a single chip/soc-control node and > rewrite the sub-drivers to not directly rely on DT compatible. > With this, joining all sub-drivers into drivers/soc/berlin would > be a sane approach, right? Also, I have the strong feeling, that > we will encounter situations later that will require the clk driver > to pull a reset before changing a specific clk rate, e.g. for GPU. If we do this, I think it should be a single driver as well, without subdrivers. We should probably just do this if there is a small number of subsystems to bind to, so the driver doesn't get out of hand. This driver could live in drivers/soc then. If you want to have subdrivers after all, that would be a classic MFD and should live in drivers/mfd. The binding would be the same for both approaches. Arnd
On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote: > On 02/18/2015 04:06 PM, Arnd Bergmann wrote: > >On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote: > >>I fundamentally disagree that either this registers or syscon in general > >>should in any way be seen as a bus. The chip control registers is an > >>highly unsorted bunch of bits that we try to match with cleanly > >>separated subsystems. This makes it a resource but no bus of any sort. > > > >It really depends on what you mean by 'bus'. It's certainly not a bus_type > >in Linux, but if you have a node in DT with multiple children, we > >sometimes think of that as a bus. I believe it makes sense to have > >the child devices under the syscon node here, at least the ones that > >have no other registers. > > Arnd, Lee, > > First of all, I think I get the points of both of you. > > With 'bus' I was referring to anything that implies a fixed number to > address any of the sub-units. As the register is more or less unsorted > and unordered, I'd see it as a resource - but that isn't important > really. > > >>The problem that we try to solve here is not a DT problem but solely > >>driven by the fact that we need something to register platform_devices > >>for pinctrl and reset. The unit we describe in DT is a pinctrl-clock- > >>power-reset-unit - or short chip control. > > > >There are two problems here that we need to look at separately, > >even though there is some interaction: > > > >* For the DT representation, we need to make it something that > >corresponds to the hardware. We could either have a single device > >node for the set of registers, or we could have one node for each > >child. With syscon, we could also put the functional device nodes > >somewhere else, which we have to do if any device uses multiple > >syscon parents. > > Well, during the discussion, I think we can also get along with a > single node for the whole chip-registers - even in DT. Clock and > reset just require corresponding #foo-cells and pinctrl will have > its pinmux sub-nodes right in that single node. If we have separate > sub-nodes for each of the subsystems is mainly a matter of taste, > right? > > >* For the driver code, we need a way to fit into the kernel model > >while at the same time using the information that is in DT. > >I agree with Lee that your current driver is not a good solution > >here: you create a driver for the parent device that knows what > >child devices there are, which is not a good abstraction. Instead > >we should have a way for the child devices to get probed automatically, > >just like we do for simple-bus, whether we use simple-bus or not. > > With no sub-nodes, we'd have to have a driver that knows the linux > subsystem platform_devices. With sub-nodes, you are proposing a > "syscon-bus"-like compatible hook to populate sub-devices. Ok, I > get this. > > >>If you argue that mfd is not the right place for this "driver" we'll > >>have to find a different place for it. I remember Mike has no problem > >>with extending early probed clock drivers to register additional > >>platform_devices - so I guess we end up putting it in there ignoring > >>mfd's ability to do it for us. > > > >If you have a driver that is responsible for the entire register > >area, I think it would be best to make that driver just register > >to all the subsystems itself rather than creating child devices. > > Hmm. That would create a beast of a driver wouldn't it? We could > get along with a library-like structure we each of foo-related > code resides in drivers/foo but still we'd need some common include > to reference to the sub-driver. > > >The alternative is to come up with a way to probe all the child > >devices automatically, but then we should make that parent device > >have a generic driver that does not need to know about the children > >and that can work on any platform with similar requirements. > > Ok, this is most likely the part that Lee doesn't like on the current > driver: a platform_device for registering platform_devices *only* and > only for Berlin. > > So, out of the two options: > > (a) Go for syscon_of_populate_devices() with a new compatible (I guess) > and having sub-nodes for each Linux subsystem that we want to have > a platform_device for. I fear that this will clash with early > registration of clk and we still have to find a way, i.e. device > naming policy, to match the drivers with their devices. > > (b) Join clk, pinctrl, reset into a single chip/soc-control node and > rewrite the sub-drivers to not directly rely on DT compatible. > With this, joining all sub-drivers into drivers/soc/berlin would > be a sane approach, right? Also, I have the strong feeling, that > we will encounter situations later that will require the clk driver > to pull a reset before changing a specific clk rate, e.g. for GPU. > > Anyway, I appreciate your discussion but still I am a little lost > between the two options. I thought that Antoine's mfd approach is > good, but I understand your concerns. > > Any direction we should go for? FWIW, my person opinion would be to keep the drivers separate (not least to keep drivers/soc from becoming the next dumping ground after drivers/mfd and drivers/misc had had enough) and place them inside their own subsystems, keep the device tree node hierarchy and place the parent node under syscon. After all, that is what this device represents. Then, as you say, create a nice framework which elegantly probes each of the platform device drivers in turn. That way each of the drivers get to keep their own compatible string which you can use to match on using current framework helpers. simple-bus already does what you want. The only issue here is semantics (naming). As I've alluded to already, people are currently using (abusing?) this stuff, therefore it must be suitable, at least on a functional (rather than religious) level.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 2e6b7311fabc..eda6dbec02ff 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -840,6 +840,11 @@ config STMPE_SPI This is used to enable SPI interface of STMPE endmenu +config MFD_BERLIN_CTRL + bool + select MFD_CORE + select MFD_SYSCON + config MFD_STA2X11 bool "STMicroelectronics STA2X11" depends on STA2X11 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 53467e211381..adf60e85df20 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2) += dln2.o intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o + +obj-$(CONFIG_MFD_BERLIN_CTRL) += berlin-ctrl.o diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c new file mode 100644 index 000000000000..e3ce6f069f16 --- /dev/null +++ b/drivers/mfd/berlin-ctrl.c @@ -0,0 +1,180 @@ +/* + * Copyright (C) 2015 Marvell Technology Group Ltd. + * + * Antoine Tenart <antoine.tenart@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/mfd/core.h> +#include <linux/module.h> +#include <linux/of.h> + +struct berlin_ctrl_priv { + const struct mfd_cell *devs; + unsigned ndevs; +}; + +/* + * BG2 devices + */ +static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = { + { + .name = "berlin2-soc-pinctrl", + .of_compatible = "marvell,berlin2-soc-pinctrl", + }, + { + .name = "berlin2-reset", + .of_compatible = "marvell,berlin2-reset", + }, +}; + +static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = { + { + .name = "berlin2-system-pinctrl", + .of_compatible = "marvell,berlin2-system-pinctrl", + }, +}; + +static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = { + .devs = berlin2_ctrl_chip_ctrl_subdevs, + .ndevs = ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs), +}; + +static const struct berlin_ctrl_priv berlin2_ctrl_system_data = { + .devs = berlin2_ctrl_system_ctrl_subdevs, + .ndevs = ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs), +}; + + +/* + * BG2CD devices + */ +static const struct mfd_cell berlin2cd_ctrl_chip_ctrl_subdevs[] = { + { + .name = "berlin2cd-soc-pinctrl", + .of_compatible = "marvell,berlin2cd-soc-pinctrl", + }, + { + .name = "berlin2-reset", + .of_compatible = "marvell,berlin2-reset", + }, +}; + +static const struct mfd_cell berlin2cd_ctrl_system_ctrl_subdevs[] = { + { + .name = "berlin2cd-system-pinctrl", + .of_compatible = "marvell,berlin2cd-system-pinctrl", + }, +}; + +static const struct berlin_ctrl_priv berlin2cd_ctrl_chip_ctrl_data = { + .devs = berlin2cd_ctrl_chip_ctrl_subdevs, + .ndevs = ARRAY_SIZE(berlin2cd_ctrl_chip_ctrl_subdevs), +}; + +static const struct berlin_ctrl_priv berlin2cd_ctrl_system_data = { + .devs = berlin2cd_ctrl_system_ctrl_subdevs, + .ndevs = ARRAY_SIZE(berlin2cd_ctrl_system_ctrl_subdevs), +}; + +/* + * BG2Q devices + */ +static const struct mfd_cell berlin2q_ctrl_chip_ctrl_subdevs[] = { + { + .name = "berlin2q-soc-pinctrl", + .of_compatible = "marvell,berlin2q-soc-pinctrl", + }, + { + .name = "berlin2-reset", + .of_compatible = "marvell,berlin2-reset", + }, +}; + +static const struct mfd_cell berlin2q_ctrl_system_ctrl_subdevs[] = { + { + .name = "berlin2q-system-pinctrl", + .of_compatible = "marvell,berlin2q-system-pinctrl", + }, +}; + +static const struct berlin_ctrl_priv berlin2q_ctrl_chip_ctrl_data = { + .devs = berlin2q_ctrl_chip_ctrl_subdevs, + .ndevs = ARRAY_SIZE(berlin2q_ctrl_chip_ctrl_subdevs), +}; + +static const struct berlin_ctrl_priv berlin2q_ctrl_system_data = { + .devs = berlin2q_ctrl_system_ctrl_subdevs, + .ndevs = ARRAY_SIZE(berlin2q_ctrl_system_ctrl_subdevs), +}; + + +static const struct of_device_id berlin_ctrl_of_match[] = { + /* BG2 */ + { + .compatible = "marvell,berlin2-chip-ctrl", + .data = &berlin2_ctrl_chip_ctrl_data, + }, + { + .compatible = "marvell,berlin2-system-ctrl", + .data = &berlin2_ctrl_system_data, + }, + /* BG2CD */ + { + .compatible = "marvell,berlin2cd-chip-ctrl", + .data = &berlin2cd_ctrl_chip_ctrl_data, + }, + { + .compatible = "marvell,berlin2cd-system-ctrl", + .data = &berlin2cd_ctrl_system_data, + }, + /* BG2Q */ + { + .compatible = "marvell,berlin2q-chip-ctrl", + .data = &berlin2q_ctrl_chip_ctrl_data, + }, + { + .compatible = "marvell,berlin2q-system-ctrl", + .data = &berlin2q_ctrl_system_data, + }, + { }, +}; +MODULE_DEVICE_TABLE(of, berlin_ctrl_of_match); + +static int berlin_ctrl_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + const struct of_device_id *match; + const struct berlin_ctrl_priv *priv; + int ret; + + match = of_match_node(berlin_ctrl_of_match, dev->of_node); + if (!match) + return -EINVAL; + + priv = match->data; + + ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL); + if (ret) { + dev_err(dev, "Failed to add devices: %d\n", ret); + return ret; + } + + return 0; +} + +static struct platform_driver berlin_ctrl_driver = { + .probe = berlin_ctrl_probe, + .driver = { + .name = "berlin-ctrl", + .of_match_table = berlin_ctrl_of_match, + }, +}; +module_platform_driver(berlin_ctrl_driver); + +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>"); +MODULE_DESCRIPTION("Marvell Berlin controller driver (mfd)"); +MODULE_LICENSE("GPL");
Marvell Berlin SoC have two nodes providing multiple devices (clk, pinctrl, reset). While until now these drivers were initialized using initcalls, this wasn't a proper solution. This mfd driver will be responsible of adding these devices, to be probed properly. It currently registers the pin controllers and the reset drivers for BG2, BG2CD and BG2Q in the soc and system controller nodes. Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/mfd/Kconfig | 5 ++ drivers/mfd/Makefile | 2 + drivers/mfd/berlin-ctrl.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+) create mode 100644 drivers/mfd/berlin-ctrl.c