diff mbox series

[RESEND] mfd: Add Freescale i.MX8qxp Control and Status Registers (CSR) module driver

Message ID 20221017075702.4182846-1-victor.liu@nxp.com (mailing list archive)
State New, archived
Headers show
Series [RESEND] mfd: Add Freescale i.MX8qxp Control and Status Registers (CSR) module driver | expand

Commit Message

Liu Ying Oct. 17, 2022, 7:57 a.m. UTC 
Freescale i.MX8qxp Control and Status Registers (CSR) module is a system
controller. It represents a set of miscellaneous registers of a specific
subsystem. It may provide control and/or status report interfaces to a
mix of standalone hardware devices within that subsystem.

The CSR module in i.MX8qm/qxp SoCs is a child node of a simple power-managed
bus(i.MX8qxp pixel link MSI bus). To propagate power management operations
of the CSR module's child devices to that simple power-managed bus, add a
dedicated driver for the CSR module. Also, the driver would populate the CSR
module's child devices.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
The Freescale i.MX8qxp CSR DT bindings is at
Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.

Resend the patch based on v6.1-rc1.

 drivers/mfd/Kconfig           | 10 +++++++
 drivers/mfd/Makefile          |  1 +
 drivers/mfd/fsl-imx8qxp-csr.c | 53 +++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)
 create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c

Comments

Liu Ying Oct. 31, 2022, 8:44 a.m. UTC | #1 
On Mon, 2022-10-17 at 15:57 +0800, Liu Ying wrote:
> Freescale i.MX8qxp Control and Status Registers (CSR) module is a system
> controller. It represents a set of miscellaneous registers of a specific
> subsystem. It may provide control and/or status report interfaces to a
> mix of standalone hardware devices within that subsystem.
> 
> The CSR module in i.MX8qm/qxp SoCs is a child node of a simple power-managed
> bus(i.MX8qxp pixel link MSI bus). To propagate power management operations
> of the CSR module's child devices to that simple power-managed bus, add a
> dedicated driver for the CSR module. Also, the driver would populate the CSR
> module's child devices.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> The Freescale i.MX8qxp CSR DT bindings is at
> Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> 
> Resend the patch based on v6.1-rc1.
> 
>  drivers/mfd/Kconfig           | 10 +++++++
>  drivers/mfd/Makefile          |  1 +
>  drivers/mfd/fsl-imx8qxp-csr.c | 53 +++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+)
>  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c

[...]

Gentle ping...

Any comments please?

Thanks,
Liu Ying
Lee Jones Oct. 31, 2022, 9:30 a.m. UTC | #2 
On Mon, 31 Oct 2022, Liu Ying wrote:

> On Mon, 2022-10-17 at 15:57 +0800, Liu Ying wrote:
> > Freescale i.MX8qxp Control and Status Registers (CSR) module is a system
> > controller. It represents a set of miscellaneous registers of a specific
> > subsystem. It may provide control and/or status report interfaces to a
> > mix of standalone hardware devices within that subsystem.
> > 
> > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple power-managed
> > bus(i.MX8qxp pixel link MSI bus). To propagate power management operations
> > of the CSR module's child devices to that simple power-managed bus, add a
> > dedicated driver for the CSR module. Also, the driver would populate the CSR
> > module's child devices.
> > 
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> > The Freescale i.MX8qxp CSR DT bindings is at
> > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > 
> > Resend the patch based on v6.1-rc1.
> > 
> >  drivers/mfd/Kconfig           | 10 +++++++
> >  drivers/mfd/Makefile          |  1 +
> >  drivers/mfd/fsl-imx8qxp-csr.c | 53 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 64 insertions(+)
> >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> 
> [...]
> 
> Gentle ping...

Content-less pings will never be received well.

> Any comments please?

You're in the queue.
Lee Jones Oct. 31, 2022, 3:40 p.m. UTC | #3 
On Mon, 17 Oct 2022, Liu Ying wrote:

> Freescale i.MX8qxp Control and Status Registers (CSR) module is a system
> controller. It represents a set of miscellaneous registers of a specific
> subsystem. It may provide control and/or status report interfaces to a
> mix of standalone hardware devices within that subsystem.
> 
> The CSR module in i.MX8qm/qxp SoCs is a child node of a simple power-managed
> bus(i.MX8qxp pixel link MSI bus). To propagate power management operations
> of the CSR module's child devices to that simple power-managed bus, add a
> dedicated driver for the CSR module. Also, the driver would populate the CSR
> module's child devices.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> The Freescale i.MX8qxp CSR DT bindings is at
> Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> 
> Resend the patch based on v6.1-rc1.
> 
>  drivers/mfd/Kconfig           | 10 +++++++
>  drivers/mfd/Makefile          |  1 +
>  drivers/mfd/fsl-imx8qxp-csr.c | 53 +++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+)
>  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..966a250d7910 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -499,6 +499,16 @@ config MFD_MX25_TSADC
>  	  i.MX25 processors. They consist of a conversion queue for general
>  	  purpose ADC and a queue for Touchscreens.
>  
> +config MFD_MX8QXP_CSR
> +	tristate "Freescale i.MX8qxp Control and Status Registers (CSR) Module"
> +	depends on (ARCH_MXC && OF) || COMPILE_TEST
> +	help
> +	  Enable support for Freescale i.MX8qm/qxp Control and Status Registers
> +	  (CSR) Module. As a system controller, CSR represents a set of
> +	  miscellaneous registers of a specific subsystem. It may provide
> +	  control and/or status report interfaces to a mix of standalone
> +	  hardware devices within that subsystem.
> +
>  config MFD_HI6421_PMIC
>  	tristate "HiSilicon Hi6421 PMU/Codec IC"
>  	depends on OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..5c3c13d7ce85 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -113,6 +113,7 @@ obj-$(CONFIG_MFD_TWL4030_AUDIO)	+= twl4030-audio.o
>  obj-$(CONFIG_TWL6040_CORE)	+= twl6040.o
>  
>  obj-$(CONFIG_MFD_MX25_TSADC)	+= fsl-imx25-tsadc.o
> +obj-$(CONFIG_MFD_MX8QXP_CSR)	+= fsl-imx8qxp-csr.o
>  
>  obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
>  obj-$(CONFIG_MFD_MC13XXX_SPI)	+= mc13xxx-spi.o
> diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-imx8qxp-csr.c
> new file mode 100644
> index 000000000000..3915d3d6ca65
> --- /dev/null
> +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright 2022 NXP
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +static int imx8qxp_csr_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = devm_of_platform_populate(&pdev->dev);

The use of this API does not constitute a MFD.

Please use "simple-mfd" instead.

> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to populate sub-devices: %d\n", ret);
> +		pm_runtime_disable(&pdev->dev);
> +	}
> +
> +	return ret;
> +}
> +
> +static int imx8qxp_csr_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
> +}
> +
> +static const struct of_device_id imx8qxp_csr_of_match[] = {
> +	{ .compatible = "fsl,imx8qxp-mipi-lvds-csr", },
> +	{ .compatible = "fsl,imx8qm-lvds-csr", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx8qxp_csr_of_match);
> +
> +static struct platform_driver imx8qxp_csr_driver = {
> +	.probe = imx8qxp_csr_probe,
> +	.remove = imx8qxp_csr_remove,
> +	.driver = {
> +		.name = "imx8qxp-csr",
> +		.of_match_table = imx8qxp_csr_of_match,
> +	},
> +};
> +
> +module_platform_driver(imx8qxp_csr_driver);
> +
> +MODULE_DESCRIPTION("Freescale i.MX8qm/qxp Control and Status Registers Module Driver");
> +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
> +MODULE_LICENSE("GPL v2");
Liu Ying Nov. 1, 2022, 5:53 a.m. UTC | #4 
Hi Lee,

On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> On Mon, 17 Oct 2022, Liu Ying wrote:
> 
> > Freescale i.MX8qxp Control and Status Registers (CSR) module is a system
> > controller. It represents a set of miscellaneous registers of a specific
> > subsystem. It may provide control and/or status report interfaces to a
> > mix of standalone hardware devices within that subsystem.
> > 
> > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple power-managed
> > bus(i.MX8qxp pixel link MSI bus). To propagate power management operations
> > of the CSR module's child devices to that simple power-managed bus, add a
> > dedicated driver for the CSR module. Also, the driver would populate the CSR
> > module's child devices.
> > 
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> > The Freescale i.MX8qxp CSR DT bindings is at
> > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > 
> > Resend the patch based on v6.1-rc1.
> > 
> >  drivers/mfd/Kconfig           | 10 +++++++
> >  drivers/mfd/Makefile          |  1 +
> >  drivers/mfd/fsl-imx8qxp-csr.c | 53 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 64 insertions(+)
> >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c

[...]

> > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-imx8qxp-csr.c
> > new file mode 100644
> > index 000000000000..3915d3d6ca65
> > --- /dev/null
> > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > @@ -0,0 +1,53 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/*
> > + * Copyright 2022 NXP
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	ret = devm_of_platform_populate(&pdev->dev);
> 
> The use of this API does not constitute a MFD.
> 
> Please use "simple-mfd" instead.

simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
simple-pm-bus driver would not populate child devices of simple-mfd
devices.  Also, the simple-pm-bus driver would not enable runtime power
management for simple-mfd devices due to "ONLY_BUS", which means it
would not propagate power management operations from child devices of
simple-mfd devices to parent devices of simple-mfd devices.  That's why
a dedicated fsl-imx8qxp-csr driver is needed.   

Regards,
Liu Ying

[...]
Liu Ying Nov. 2, 2022, 3:49 a.m. UTC | #5 
Hi Lee,

On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> Hi Lee,
> 
> On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > On Mon, 17 Oct 2022, Liu Ying wrote:
> > 
> > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a
> > > system
> > > controller. It represents a set of miscellaneous registers of a
> > > specific
> > > subsystem. It may provide control and/or status report interfaces
> > > to a
> > > mix of standalone hardware devices within that subsystem.
> > > 
> > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple
> > > power-managed
> > > bus(i.MX8qxp pixel link MSI bus). To propagate power management
> > > operations
> > > of the CSR module's child devices to that simple power-managed
> > > bus, add a
> > > dedicated driver for the CSR module. Also, the driver would
> > > populate the CSR
> > > module's child devices.
> > > 
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > ---
> > > The Freescale i.MX8qxp CSR DT bindings is at
> > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > 
> > > Resend the patch based on v6.1-rc1.
> > > 
> > >  drivers/mfd/Kconfig           | 10 +++++++
> > >  drivers/mfd/Makefile          |  1 +
> > >  drivers/mfd/fsl-imx8qxp-csr.c | 53
> > > +++++++++++++++++++++++++++++++++++
> > >  3 files changed, 64 insertions(+)
> > >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> 
> [...]
> 
> > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-
> > > imx8qxp-csr.c
> > > new file mode 100644
> > > index 000000000000..3915d3d6ca65
> > > --- /dev/null
> > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > > @@ -0,0 +1,53 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +/*
> > > + * Copyright 2022 NXP
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +
> > > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > > +{
> > > +	int ret;
> > > +
> > > +	pm_runtime_enable(&pdev->dev);
> > > +
> > > +	ret = devm_of_platform_populate(&pdev->dev);
> > 
> > The use of this API does not constitute a MFD.
> > 
> > Please use "simple-mfd" instead.
> 
> simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> simple-pm-bus driver would not populate child devices of simple-mfd
> devices.  Also, the simple-pm-bus driver would not enable runtime
> power management for simple-mfd devices due to "ONLY_BUS", which
> means it would not propagate power management operations from child
> devices of simple-mfd devices to parent devices of simple-mfd
> devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed. 

One more point which might be overlooked - as mentioned in commit
message, the CSR module is a child node of a simple power-managed
bus(i.MX8qxp pixel link MSI bus), which means the child devices of the
CSR module(as a simple-mfd device) won't be populated by
of_platform_default_populate() from of_platform_default_populate_init()
because "simple-pm-bus" is not listed in of_default_bus_match_table[]
and hence recursion of of_platform_bus_create() will stop at the
simple-pm-bus. This is also a reason why a dedicated fsl-imx8qxp-csr
driver is needed to populated those child devices of the CSR module.

The patch set to support i.MX8qxp pixel link MSI bus can be found at:

https://lore.kernel.org/lkml/20221017074039.4181843-1-victor.liu@nxp.com/

Regards,
Liu Ying
Lee Jones Nov. 7, 2022, 9:05 a.m. UTC | #6 
On Wed, 02 Nov 2022, Liu Ying wrote:

> Hi Lee,
> 
> On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > Hi Lee,
> > 
> > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > 
> > > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a
> > > > system
> > > > controller. It represents a set of miscellaneous registers of a
> > > > specific
> > > > subsystem. It may provide control and/or status report interfaces
> > > > to a
> > > > mix of standalone hardware devices within that subsystem.
> > > > 
> > > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple
> > > > power-managed
> > > > bus(i.MX8qxp pixel link MSI bus). To propagate power management
> > > > operations
> > > > of the CSR module's child devices to that simple power-managed
> > > > bus, add a
> > > > dedicated driver for the CSR module. Also, the driver would
> > > > populate the CSR
> > > > module's child devices.
> > > > 
> > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > ---
> > > > The Freescale i.MX8qxp CSR DT bindings is at
> > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > > 
> > > > Resend the patch based on v6.1-rc1.
> > > > 
> > > >  drivers/mfd/Kconfig           | 10 +++++++
> > > >  drivers/mfd/Makefile          |  1 +
> > > >  drivers/mfd/fsl-imx8qxp-csr.c | 53
> > > > +++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 64 insertions(+)
> > > >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> > 
> > [...]
> > 
> > > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-
> > > > imx8qxp-csr.c
> > > > new file mode 100644
> > > > index 000000000000..3915d3d6ca65
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > > > @@ -0,0 +1,53 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +
> > > > +/*
> > > > + * Copyright 2022 NXP
> > > > + */
> > > > +
> > > > +#include <linux/module.h>
> > > > +#include <linux/of_platform.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +
> > > > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	pm_runtime_enable(&pdev->dev);
> > > > +
> > > > +	ret = devm_of_platform_populate(&pdev->dev);
> > > 
> > > The use of this API does not constitute a MFD.
> > > 
> > > Please use "simple-mfd" instead.
> > 
> > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > simple-pm-bus driver would not populate child devices of simple-mfd
> > devices.

Right, simple-pm-bus will not populate child devices, because:

  /*                                                                     
   * These are transparent bus devices (not simple-pm-bus matches) that  
   * have their child nodes populated automatically.  So, don't need to  
   * do anything more. We only match with the device if this driver is   
   * the most specific match because we don't want to incorrectly bind to
   * a device that has a more specific driver.                           
   */                                                                    

So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.

> > Also, the simple-pm-bus driver would not enable runtime
> > power management for simple-mfd devices due to "ONLY_BUS", which
> > means it would not propagate power management operations from child
> > devices of simple-mfd devices to parent devices of simple-mfd
> > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed. 

This is more of an issue.

Why can't this device use "simple-pm-bus" to have support for both
auto-registration AND Runtime PM?

> One more point which might be overlooked - as mentioned in commit
> message, the CSR module is a child node of a simple power-managed
> bus(i.MX8qxp pixel link MSI bus), which means the child devices of the
> CSR module(as a simple-mfd device) won't be populated by
> of_platform_default_populate() from of_platform_default_populate_init()
> because "simple-pm-bus" is not listed in of_default_bus_match_table[]
> and hence recursion of of_platform_bus_create() will stop at the
> simple-pm-bus. This is also a reason why a dedicated fsl-imx8qxp-csr
> driver is needed to populated those child devices of the CSR module.

Not sure I know the semantics well enough (anymore) to get a
meaningful picture of what you're trying to explain, and I do not have
any suitable H/W here to mock-up a real-world test-bed / concept
demonstrator to debug this for you.

The long and the short of it is; we have a bunch of automatic
child-device-registering helpers in the kernel.  One of the mechanisms
is bound to work for you if you structure your code appropriately.

Introducing an empty, meaningless driver, simply to call a single
function it not acceptable.  Please make the infrastructure already
offered specifically to solve this category of issue work for your
use-case.
Liu Ying Nov. 8, 2022, 3:56 a.m. UTC | #7 
Hi Lee,

On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> On Wed, 02 Nov 2022, Liu Ying wrote:
> 
> > Hi Lee,
> > 
> > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > Hi Lee,
> > > 
> > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > 
> > > > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a
> > > > > system
> > > > > controller. It represents a set of miscellaneous registers of a
> > > > > specific
> > > > > subsystem. It may provide control and/or status report interfaces
> > > > > to a
> > > > > mix of standalone hardware devices within that subsystem.
> > > > > 
> > > > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple
> > > > > power-managed
> > > > > bus(i.MX8qxp pixel link MSI bus). To propagate power management
> > > > > operations
> > > > > of the CSR module's child devices to that simple power-managed
> > > > > bus, add a
> > > > > dedicated driver for the CSR module. Also, the driver would
> > > > > populate the CSR
> > > > > module's child devices.
> > > > > 
> > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > > ---
> > > > > The Freescale i.MX8qxp CSR DT bindings is at
> > > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > > > 
> > > > > Resend the patch based on v6.1-rc1.
> > > > > 
> > > > >  drivers/mfd/Kconfig           | 10 +++++++
> > > > >  drivers/mfd/Makefile          |  1 +
> > > > >  drivers/mfd/fsl-imx8qxp-csr.c | 53
> > > > > +++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 64 insertions(+)
> > > > >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> > > 
> > > [...]
> > > 
> > > > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-
> > > > > imx8qxp-csr.c
> > > > > new file mode 100644
> > > > > index 000000000000..3915d3d6ca65
> > > > > --- /dev/null
> > > > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > > > > @@ -0,0 +1,53 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +
> > > > > +/*
> > > > > + * Copyright 2022 NXP
> > > > > + */
> > > > > +
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of_platform.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > > +
> > > > > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	pm_runtime_enable(&pdev->dev);
> > > > > +
> > > > > +	ret = devm_of_platform_populate(&pdev->dev);
> > > > 
> > > > The use of this API does not constitute a MFD.
> > > > 
> > > > Please use "simple-mfd" instead.
> > > 
> > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > devices.
> 
> Right, simple-pm-bus will not populate child devices, because:

simple-pm-bus.c may populate child devices of simple-pm-bus devices
because "ONLY_BUS" is _not_ set for simple-pm-bus devices.

simple-pm-bus.c would _not_ populate child devices of simple-mfd
devices because "ONLY_BUS" is set for simple-mfd devices.

> 
>   /*                                                                     
>    * These are transparent bus devices (not simple-pm-bus matches) that  
>    * have their child nodes populated automatically.  So, don't need to  
>    * do anything more. We only match with the device if this driver is   
>    * the most specific match because we don't want to incorrectly bind to
>    * a device that has a more specific driver.                           
>    */                                                                    
> 
> So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.

If simple-mfd device is a child device of one device listed in
of_default_bus_match_table[], then it may be populated by
drivers/of/platform.c.  But, in my case, simple-mfd device is a child
device of simple-pm-bus device(not listed in that table), so it will
not be populated by drivers/of/platform.c.  Instead,
drivers/bus/simple-pm-bus.c would populate the simple-mfd device.

> 
> > > Also, the simple-pm-bus driver would not enable runtime
> > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > means it would not propagate power management operations from child
> > > devices of simple-mfd devices to parent devices of simple-mfd
> > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed. 
> 
> This is more of an issue.
> 
> Why can't this device use "simple-pm-bus" to have support for both
> auto-registration AND Runtime PM?

If I change the compatible string of the CSR module from
"fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
to
"fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
all devices I'm interested in are populated and all relevant drivers
can probe.  But, this change makes 'make dt_binding_check' for the
existing fsl,imx8qxp-csr.yaml fail:

--------------------------------8<------------------------------------
/kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
csr.example.dtb: syscon@56221000: $nodename:0: 'syscon@56221000' does
not match '^bus(@[0-9a-f]+)?$'
	From schema:
/kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
/kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
csr.example.dtb: syscon@56221000: '#address-cells' is a required
property
	From schema:
/kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
/kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
csr.example.dtb: syscon@56221000: '#size-cells' is a required property
	From schema:
/kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
/kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
csr.example.dtb: syscon@56221000: 'ranges' is a required property
	From schema:
/kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
--------------------------------8<------------------------------------

The error log basically complains two things:
1) The example nodename 'syscon@56221000' should match
'^bus(@[0-9a-f]+)?$'.
2) Missing '#address-cells', '#size-cells' and 'ranges' properties as
required by simple-pm-bus.

Regarding 1), if I change 'syscon@56221000' to 'bus@56221000', then the
below error comes along:
--------------------------------8<------------------------------------
/kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
csr.example.dtb: bus@56221000: $nodename:0: 'bus@56221000' does not
match '^syscon@[0-9a-f]+$'
	From schema:
/kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
csr.yaml
--------------------------------8<------------------------------------
So, it looks like "syscon" and "simple-pm-bus" can not be in compatbile
string at the same time.  Note that "syscon" is needed because other
device nodes may reference the CSR module through a phandle, like the
"fsl,imx8qxp-mipi-dphy" device.

Regarding 2), I may try to add those required properties, but it would
break the existing schemas of the child devices of the CSR module, like
the "fsl,imx8qxp-ldb" device, because "reg" property is not allowed.

So, it looks like the CSR module still should be simple-mfd device but
not simple-pm-bus device, right?

> 
> > One more point which might be overlooked - as mentioned in commit
> > message, the CSR module is a child node of a simple power-managed
> > bus(i.MX8qxp pixel link MSI bus), which means the child devices of the
> > CSR module(as a simple-mfd device) won't be populated by
> > of_platform_default_populate() from of_platform_default_populate_init()
> > because "simple-pm-bus" is not listed in of_default_bus_match_table[]
> > and hence recursion of of_platform_bus_create() will stop at the
> > simple-pm-bus. This is also a reason why a dedicated fsl-imx8qxp-csr
> > driver is needed to populated those child devices of the CSR module.
> 
> Not sure I know the semantics well enough (anymore) to get a
> meaningful picture of what you're trying to explain, and I do not have
> any suitable H/W here to mock-up a real-world test-bed / concept
> demonstrator to debug this for you.

I understand you have no hardware to debug this directly.  But, the
example in dt-binding doc for the i.MX8qxp pixel link MSI bus(a simple-
pm-bus) may give you a kinda full picture of what the relevant devices
look like.  I mentioned the patch set to add the MSI bus previously,
however, you may find the binding doc directly here -
https://lore.kernel.org/lkml/20221017074039.4181843-3-victor.liu@nxp.com/

> 
> The long and the short of it is; we have a bunch of automatic
> child-device-registering helpers in the kernel.  One of the mechanisms
> is bound to work for you if you structure your code appropriately.
> 
> Introducing an empty, meaningless driver, simply to call a single
> function it not acceptable.  Please make the infrastructure already
> offered specifically to solve this category of issue work for your
> use-case.

Yeah, I tried to not to introduce a new driver for the CSR module, but
it seems that it is needed.

Regards,
Liu Ying
Liu Ying Nov. 9, 2022, 12:50 p.m. UTC | #8 
> -----Original Message-----
> From: Ying Liu
> Subject: Re: [PATCH RESEND] mfd: Add Freescale i.MX8qxp Control and Status
> Registers (CSR) module driver
> 
> Hi Lee,
> 
> On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > On Wed, 02 Nov 2022, Liu Ying wrote:
> >
> > > Hi Lee,
> > >
> > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > > Hi Lee,
> > > >
> > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > >
> > > > > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a
> > > > > > system
> > > > > > controller. It represents a set of miscellaneous registers of a
> > > > > > specific
> > > > > > subsystem. It may provide control and/or status report interfaces
> > > > > > to a
> > > > > > mix of standalone hardware devices within that subsystem.
> > > > > >
> > > > > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple
> > > > > > power-managed
> > > > > > bus(i.MX8qxp pixel link MSI bus). To propagate power management
> > > > > > operations
> > > > > > of the CSR module's child devices to that simple power-managed
> > > > > > bus, add a
> > > > > > dedicated driver for the CSR module. Also, the driver would
> > > > > > populate the CSR
> > > > > > module's child devices.
> > > > > >
> > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > > > ---
> > > > > > The Freescale i.MX8qxp CSR DT bindings is at
> > > > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > > > >
> > > > > > Resend the patch based on v6.1-rc1.
> > > > > >
> > > > > >  drivers/mfd/Kconfig           | 10 +++++++
> > > > > >  drivers/mfd/Makefile          |  1 +
> > > > > >  drivers/mfd/fsl-imx8qxp-csr.c | 53
> > > > > > +++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 64 insertions(+)
> > > > > >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> > > >
> > > > [...]
> > > >
> > > > > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-
> > > > > > imx8qxp-csr.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..3915d3d6ca65
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > > > > > @@ -0,0 +1,53 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +
> > > > > > +/*
> > > > > > + * Copyright 2022 NXP
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/of_platform.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > > +
> > > > > > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	pm_runtime_enable(&pdev->dev);
> > > > > > +
> > > > > > +	ret = devm_of_platform_populate(&pdev->dev);
> > > > >
> > > > > The use of this API does not constitute a MFD.
> > > > >
> > > > > Please use "simple-mfd" instead.
> > > >
> > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > > devices.
> >
> > Right, simple-pm-bus will not populate child devices, because:
> 
> simple-pm-bus.c may populate child devices of simple-pm-bus devices
> because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
> 
> simple-pm-bus.c would _not_ populate child devices of simple-mfd
> devices because "ONLY_BUS" is set for simple-mfd devices.
> 
> >
> >   /*
> >    * These are transparent bus devices (not simple-pm-bus matches) that
> >    * have their child nodes populated automatically.  So, don't need to
> >    * do anything more. We only match with the device if this driver is
> >    * the most specific match because we don't want to incorrectly bind to
> >    * a device that has a more specific driver.
> >    */
> >
> > So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
> 
> If simple-mfd device is a child device of one device listed in
> of_default_bus_match_table[], then it may be populated by
> drivers/of/platform.c.  But, in my case, simple-mfd device is a child
> device of simple-pm-bus device(not listed in that table), so it will
> not be populated by drivers/of/platform.c.  Instead,
> drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
> 
> >
> > > > Also, the simple-pm-bus driver would not enable runtime
> > > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > > means it would not propagate power management operations from
> child
> > > > devices of simple-mfd devices to parent devices of simple-mfd
> > > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed.
> >
> > This is more of an issue.
> >
> > Why can't this device use "simple-pm-bus" to have support for both
> > auto-registration AND Runtime PM?
> 
> If I change the compatible string of the CSR module from
> "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> to
> "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> all devices I'm interested in are populated and all relevant drivers
> can probe.  But, this change makes 'make dt_binding_check' for the
> existing fsl,imx8qxp-csr.yaml fail:
> 
> --------------------------------8<------------------------------------
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: $nodename:0: 'syscon@56221000' does
> not match '^bus(@[0-9a-f]+)?$'
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: '#address-cells' is a required
> property
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: '#size-cells' is a required property
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: 'ranges' is a required property
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> --------------------------------8<------------------------------------
> 
> The error log basically complains two things:
> 1) The example nodename 'syscon@56221000' should match
> '^bus(@[0-9a-f]+)?$'.
> 2) Missing '#address-cells', '#size-cells' and 'ranges' properties as
> required by simple-pm-bus.
> 
> Regarding 1), if I change 'syscon@56221000' to 'bus@56221000', then the
> below error comes along:
> --------------------------------8<------------------------------------
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: bus@56221000: $nodename:0: 'bus@56221000' does not
> match '^syscon@[0-9a-f]+$'
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.yaml
> --------------------------------8<------------------------------------
> So, it looks like "syscon" and "simple-pm-bus" can not be in compatbile
> string at the same time.  Note that "syscon" is needed because other
> device nodes may reference the CSR module through a phandle, like the
> "fsl,imx8qxp-mipi-dphy" device.
> 
> Regarding 2), I may try to add those required properties, but it would
> break the existing schemas of the child devices of the CSR module, like
> the "fsl,imx8qxp-ldb" device, because "reg" property is not allowed.
> 
> So, it looks like the CSR module still should be simple-mfd device but
> not simple-pm-bus device, right?
> 
> >
> > > One more point which might be overlooked - as mentioned in commit
> > > message, the CSR module is a child node of a simple power-managed
> > > bus(i.MX8qxp pixel link MSI bus), which means the child devices of the
> > > CSR module(as a simple-mfd device) won't be populated by
> > > of_platform_default_populate() from of_platform_default_populate_init()
> > > because "simple-pm-bus" is not listed in of_default_bus_match_table[]
> > > and hence recursion of of_platform_bus_create() will stop at the
> > > simple-pm-bus. This is also a reason why a dedicated fsl-imx8qxp-csr
> > > driver is needed to populated those child devices of the CSR module.
> >
> > Not sure I know the semantics well enough (anymore) to get a
> > meaningful picture of what you're trying to explain, and I do not have
> > any suitable H/W here to mock-up a real-world test-bed / concept
> > demonstrator to debug this for you.
> 
> I understand you have no hardware to debug this directly.  But, the
> example in dt-binding doc for the i.MX8qxp pixel link MSI bus(a simple-
> pm-bus) may give you a kinda full picture of what the relevant devices
> look like.  I mentioned the patch set to add the MSI bus previously,
> however, you may find the binding doc directly here -
> https://lore.kernel.org/lkml/20221017074039.4181843-3-victor.liu@nxp.com/
> 
> >
> > The long and the short of it is; we have a bunch of automatic
> > child-device-registering helpers in the kernel.  One of the mechanisms
> > is bound to work for you if you structure your code appropriately.
> >
> > Introducing an empty, meaningless driver, simply to call a single
> > function it not acceptable.  Please make the infrastructure already
> > offered specifically to solve this category of issue work for your
> > use-case.
> 
> Yeah, I tried to not to introduce a new driver for the CSR module, but
> it seems that it is needed.

After more experiments, I think it's appropriate to take the CSR module as a
simple-pm-bus device instead of a simple-mfd device.  This requires to change
the existing DT bindings for the CSR module and it's child devices.  I'll send the
change out once it is ready.  Thanks for Lee's review and comments.

Liu Ying
Lee Jones Nov. 14, 2022, 10:01 a.m. UTC | #9 
On Tue, 08 Nov 2022, Liu Ying wrote:

> Hi Lee,
> 
> On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > On Wed, 02 Nov 2022, Liu Ying wrote:
> > 
> > > Hi Lee,
> > > 
> > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > > Hi Lee,
> > > > 
> > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > > 
> > > > > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a
> > > > > > system
> > > > > > controller. It represents a set of miscellaneous registers of a
> > > > > > specific
> > > > > > subsystem. It may provide control and/or status report interfaces
> > > > > > to a
> > > > > > mix of standalone hardware devices within that subsystem.
> > > > > > 
> > > > > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple
> > > > > > power-managed
> > > > > > bus(i.MX8qxp pixel link MSI bus). To propagate power management
> > > > > > operations
> > > > > > of the CSR module's child devices to that simple power-managed
> > > > > > bus, add a
> > > > > > dedicated driver for the CSR module. Also, the driver would
> > > > > > populate the CSR
> > > > > > module's child devices.
> > > > > > 
> > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > > > ---
> > > > > > The Freescale i.MX8qxp CSR DT bindings is at
> > > > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > > > > 
> > > > > > Resend the patch based on v6.1-rc1.
> > > > > > 
> > > > > >  drivers/mfd/Kconfig           | 10 +++++++
> > > > > >  drivers/mfd/Makefile          |  1 +
> > > > > >  drivers/mfd/fsl-imx8qxp-csr.c | 53
> > > > > > +++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 64 insertions(+)
> > > > > >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> > > > 
> > > > [...]
> > > > 
> > > > > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-
> > > > > > imx8qxp-csr.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..3915d3d6ca65
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > > > > > @@ -0,0 +1,53 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +
> > > > > > +/*
> > > > > > + * Copyright 2022 NXP
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/of_platform.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > > +
> > > > > > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	pm_runtime_enable(&pdev->dev);
> > > > > > +
> > > > > > +	ret = devm_of_platform_populate(&pdev->dev);
> > > > > 
> > > > > The use of this API does not constitute a MFD.
> > > > > 
> > > > > Please use "simple-mfd" instead.
> > > > 
> > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > > devices.
> > 
> > Right, simple-pm-bus will not populate child devices, because:
> 
> simple-pm-bus.c may populate child devices of simple-pm-bus devices
> because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
> 
> simple-pm-bus.c would _not_ populate child devices of simple-mfd
> devices because "ONLY_BUS" is set for simple-mfd devices.
> 
> > 
> >   /*                                                                     
> >    * These are transparent bus devices (not simple-pm-bus matches) that  
> >    * have their child nodes populated automatically.  So, don't need to  
> >    * do anything more. We only match with the device if this driver is   
> >    * the most specific match because we don't want to incorrectly bind to
> >    * a device that has a more specific driver.                           
> >    */                                                                    
> > 
> > So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
> 
> If simple-mfd device is a child device of one device listed in
> of_default_bus_match_table[], then it may be populated by
> drivers/of/platform.c.  But, in my case, simple-mfd device is a child
> device of simple-pm-bus device(not listed in that table), so it will
> not be populated by drivers/of/platform.c.  Instead,
> drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
> 
> > 
> > > > Also, the simple-pm-bus driver would not enable runtime
> > > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > > means it would not propagate power management operations from child
> > > > devices of simple-mfd devices to parent devices of simple-mfd
> > > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed. 
> > 
> > This is more of an issue.
> > 
> > Why can't this device use "simple-pm-bus" to have support for both
> > auto-registration AND Runtime PM?
> 
> If I change the compatible string of the CSR module from
> "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> to
> "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> all devices I'm interested in are populated and all relevant drivers
> can probe.

Okay, that's good.

> But, this change makes 'make dt_binding_check' for the
> existing fsl,imx8qxp-csr.yaml fail:
> 
> --------------------------------8<------------------------------------
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: $nodename:0: 'syscon@56221000' does
> not match '^bus(@[0-9a-f]+)?$'
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: '#address-cells' is a required
> property
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: '#size-cells' is a required property
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: syscon@56221000: 'ranges' is a required property
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/bus/simple-pm-bus.yaml
> --------------------------------8<------------------------------------
> 
> The error log basically complains two things:
> 1) The example nodename 'syscon@56221000' should match
> '^bus(@[0-9a-f]+)?$'.
> 2) Missing '#address-cells', '#size-cells' and 'ranges' properties as
> required by simple-pm-bus.
> 
> Regarding 1), if I change 'syscon@56221000' to 'bus@56221000', then the
> below error comes along:
> --------------------------------8<------------------------------------
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: bus@56221000: $nodename:0: 'bus@56221000' does not
> match '^syscon@[0-9a-f]+$'
> 	From schema:
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.yaml
> --------------------------------8<------------------------------------
> So, it looks like "syscon" and "simple-pm-bus" can not be in compatbile
> string at the same time.  Note that "syscon" is needed because other
> device nodes may reference the CSR module through a phandle, like the
> "fsl,imx8qxp-mipi-dphy" device.
> 
> Regarding 2), I may try to add those required properties, but it would
> break the existing schemas of the child devices of the CSR module, like
> the "fsl,imx8qxp-ldb" device, because "reg" property is not allowed.
> 
> So, it looks like the CSR module still should be simple-mfd device but
> not simple-pm-bus device, right?

It sounds like the generic auto-probing functionality provided by the
kernel works well to give you with a fully self-registering mechanism.
All you need to do now is figure out why the DT checker is not happy
with your solution.

Please work with the Device Tree maintainers on this.

> > > One more point which might be overlooked - as mentioned in commit
> > > message, the CSR module is a child node of a simple power-managed
> > > bus(i.MX8qxp pixel link MSI bus), which means the child devices of the
> > > CSR module(as a simple-mfd device) won't be populated by
> > > of_platform_default_populate() from of_platform_default_populate_init()
> > > because "simple-pm-bus" is not listed in of_default_bus_match_table[]
> > > and hence recursion of of_platform_bus_create() will stop at the
> > > simple-pm-bus. This is also a reason why a dedicated fsl-imx8qxp-csr
> > > driver is needed to populated those child devices of the CSR module.
> > 
> > Not sure I know the semantics well enough (anymore) to get a
> > meaningful picture of what you're trying to explain, and I do not have
> > any suitable H/W here to mock-up a real-world test-bed / concept
> > demonstrator to debug this for you.
> 
> I understand you have no hardware to debug this directly.  But, the
> example in dt-binding doc for the i.MX8qxp pixel link MSI bus(a simple-
> pm-bus) may give you a kinda full picture of what the relevant devices
> look like.  I mentioned the patch set to add the MSI bus previously,
> however, you may find the binding doc directly here -
> https://lore.kernel.org/lkml/20221017074039.4181843-3-victor.liu@nxp.com/
> 
> > 
> > The long and the short of it is; we have a bunch of automatic
> > child-device-registering helpers in the kernel.  One of the mechanisms
> > is bound to work for you if you structure your code appropriately.
> > 
> > Introducing an empty, meaningless driver, simply to call a single
> > function it not acceptable.  Please make the infrastructure already
> > offered specifically to solve this category of issue work for your
> > use-case.
> 
> Yeah, I tried to not to introduce a new driver for the CSR module, but
> it seems that it is needed.

It's not. :)
Lee Jones Nov. 14, 2022, 10:03 a.m. UTC | #10 
On Wed, 09 Nov 2022, Ying Liu wrote:
> > Yeah, I tried to not to introduce a new driver for the CSR module, but
> > it seems that it is needed.
> 
> After more experiments, I think it's appropriate to take the CSR module as a
> simple-pm-bus device instead of a simple-mfd device.  This requires to change
> the existing DT bindings for the CSR module and it's child devices.  I'll send the
> change out once it is ready.  Thanks for Lee's review and comments.

Excellent, well done.

No problem, you're very welcome.
Rob Herring Nov. 14, 2022, 8:54 p.m. UTC | #11 
On Mon, Nov 7, 2022 at 9:58 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> Hi Lee,
>
> On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > On Wed, 02 Nov 2022, Liu Ying wrote:
> >
> > > Hi Lee,
> > >
> > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > > Hi Lee,
> > > >
> > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > >
> > > > > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a
> > > > > > system
> > > > > > controller. It represents a set of miscellaneous registers of a
> > > > > > specific
> > > > > > subsystem. It may provide control and/or status report interfaces
> > > > > > to a
> > > > > > mix of standalone hardware devices within that subsystem.
> > > > > >
> > > > > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple
> > > > > > power-managed
> > > > > > bus(i.MX8qxp pixel link MSI bus). To propagate power management
> > > > > > operations
> > > > > > of the CSR module's child devices to that simple power-managed
> > > > > > bus, add a
> > > > > > dedicated driver for the CSR module. Also, the driver would
> > > > > > populate the CSR
> > > > > > module's child devices.
> > > > > >
> > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > > > ---
> > > > > > The Freescale i.MX8qxp CSR DT bindings is at
> > > > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > > > >
> > > > > > Resend the patch based on v6.1-rc1.
> > > > > >
> > > > > >  drivers/mfd/Kconfig           | 10 +++++++
> > > > > >  drivers/mfd/Makefile          |  1 +
> > > > > >  drivers/mfd/fsl-imx8qxp-csr.c | 53
> > > > > > +++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 64 insertions(+)
> > > > > >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> > > >
> > > > [...]
> > > >
> > > > > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-
> > > > > > imx8qxp-csr.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..3915d3d6ca65
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > > > > > @@ -0,0 +1,53 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +
> > > > > > +/*
> > > > > > + * Copyright 2022 NXP
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/of_platform.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > > +
> > > > > > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     pm_runtime_enable(&pdev->dev);
> > > > > > +
> > > > > > +     ret = devm_of_platform_populate(&pdev->dev);
> > > > >
> > > > > The use of this API does not constitute a MFD.
> > > > >
> > > > > Please use "simple-mfd" instead.
> > > >
> > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > > devices.
> >
> > Right, simple-pm-bus will not populate child devices, because:
>
> simple-pm-bus.c may populate child devices of simple-pm-bus devices
> because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
>
> simple-pm-bus.c would _not_ populate child devices of simple-mfd
> devices because "ONLY_BUS" is set for simple-mfd devices.
>
> >
> >   /*
> >    * These are transparent bus devices (not simple-pm-bus matches) that
> >    * have their child nodes populated automatically.  So, don't need to
> >    * do anything more. We only match with the device if this driver is
> >    * the most specific match because we don't want to incorrectly bind to
> >    * a device that has a more specific driver.
> >    */
> >
> > So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
>
> If simple-mfd device is a child device of one device listed in
> of_default_bus_match_table[], then it may be populated by
> drivers/of/platform.c.  But, in my case, simple-mfd device is a child
> device of simple-pm-bus device(not listed in that table), so it will
> not be populated by drivers/of/platform.c.  Instead,
> drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
>
> >
> > > > Also, the simple-pm-bus driver would not enable runtime
> > > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > > means it would not propagate power management operations from child
> > > > devices of simple-mfd devices to parent devices of simple-mfd
> > > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed.
> >
> > This is more of an issue.
> >
> > Why can't this device use "simple-pm-bus" to have support for both
> > auto-registration AND Runtime PM?
>
> If I change the compatible string of the CSR module from
> "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> to
> "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> all devices I'm interested in are populated and all relevant drivers
> can probe.  But, this change makes 'make dt_binding_check' for the
> existing fsl,imx8qxp-csr.yaml fail:

As 'simple-bus' is for MMIO devices, so is 'simple-pm-bus' with the
addition of PM capabilities. That means you have registers defined
(reg), but you don't.

Either you have a block with mixed functions or you have separate
blocks. If the register space is all mixed together, then it is the
former and an MFD. Don't be designing your binding based on Linux
behavior.

Rob
Liu Ying Nov. 15, 2022, 5:21 a.m. UTC | #12 
On Mon, 2022-11-14 at 14:54 -0600, Rob Herring wrote:
> On Mon, Nov 7, 2022 at 9:58 PM Liu Ying <victor.liu@nxp.com> wrote:
> > Hi Lee,
> > 
> > On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > > On Wed, 02 Nov 2022, Liu Ying wrote:
> > > 
> > > > Hi Lee,
> > > > 
> > > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > > > Hi Lee,
> > > > > 
> > > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > > > 
> > > > > > > Freescale i.MX8qxp Control and Status Registers (CSR) module is a
> > > > > > > system
> > > > > > > controller. It represents a set of miscellaneous registers of a
> > > > > > > specific
> > > > > > > subsystem. It may provide control and/or status report interfaces
> > > > > > > to a
> > > > > > > mix of standalone hardware devices within that subsystem.
> > > > > > > 
> > > > > > > The CSR module in i.MX8qm/qxp SoCs is a child node of a simple
> > > > > > > power-managed
> > > > > > > bus(i.MX8qxp pixel link MSI bus). To propagate power management
> > > > > > > operations
> > > > > > > of the CSR module's child devices to that simple power-managed
> > > > > > > bus, add a
> > > > > > > dedicated driver for the CSR module. Also, the driver would
> > > > > > > populate the CSR
> > > > > > > module's child devices.
> > > > > > > 
> > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > > > > ---
> > > > > > > The Freescale i.MX8qxp CSR DT bindings is at
> > > > > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml.
> > > > > > > 
> > > > > > > Resend the patch based on v6.1-rc1.
> > > > > > > 
> > > > > > >  drivers/mfd/Kconfig           | 10 +++++++
> > > > > > >  drivers/mfd/Makefile          |  1 +
> > > > > > >  drivers/mfd/fsl-imx8qxp-csr.c | 53
> > > > > > > +++++++++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 64 insertions(+)
> > > > > > >  create mode 100644 drivers/mfd/fsl-imx8qxp-csr.c
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-
> > > > > > > imx8qxp-csr.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..3915d3d6ca65
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/mfd/fsl-imx8qxp-csr.c
> > > > > > > @@ -0,0 +1,53 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Copyright 2022 NXP
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/of_platform.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > +
> > > > > > > +static int imx8qxp_csr_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +     int ret;
> > > > > > > +
> > > > > > > +     pm_runtime_enable(&pdev->dev);
> > > > > > > +
> > > > > > > +     ret = devm_of_platform_populate(&pdev->dev);
> > > > > > 
> > > > > > The use of this API does not constitute a MFD.
> > > > > > 
> > > > > > Please use "simple-mfd" instead.
> > > > > 
> > > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > > > devices.
> > > 
> > > Right, simple-pm-bus will not populate child devices, because:
> > 
> > simple-pm-bus.c may populate child devices of simple-pm-bus devices
> > because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
> > 
> > simple-pm-bus.c would _not_ populate child devices of simple-mfd
> > devices because "ONLY_BUS" is set for simple-mfd devices.
> > 
> > >   /*
> > >    * These are transparent bus devices (not simple-pm-bus matches) that
> > >    * have their child nodes populated automatically.  So, don't need to
> > >    * do anything more. We only match with the device if this driver is
> > >    * the most specific match because we don't want to incorrectly bind to
> > >    * a device that has a more specific driver.
> > >    */
> > > 
> > > So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
> > 
> > If simple-mfd device is a child device of one device listed in
> > of_default_bus_match_table[], then it may be populated by
> > drivers/of/platform.c.  But, in my case, simple-mfd device is a child
> > device of simple-pm-bus device(not listed in that table), so it will
> > not be populated by drivers/of/platform.c.  Instead,
> > drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
> > 
> > > > > Also, the simple-pm-bus driver would not enable runtime
> > > > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > > > means it would not propagate power management operations from child
> > > > > devices of simple-mfd devices to parent devices of simple-mfd
> > > > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed.
> > > 
> > > This is more of an issue.
> > > 
> > > Why can't this device use "simple-pm-bus" to have support for both
> > > auto-registration AND Runtime PM?
> > 
> > If I change the compatible string of the CSR module from
> > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> > to
> > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> > all devices I'm interested in are populated and all relevant drivers
> > can probe.  But, this change makes 'make dt_binding_check' for the
> > existing fsl,imx8qxp-csr.yaml fail:
> 
> As 'simple-bus' is for MMIO devices, so is 'simple-pm-bus' with the
> addition of PM capabilities. That means you have registers defined
> (reg), but you don't.
> 
> Either you have a block with mixed functions or you have separate
> blocks. If the register space is all mixed together, then it is the
> former and an MFD. Don't be designing your binding based on Linux
> behavior.

Thanks for clarifying how to differetiate MFD and 'simple-bus'/'simple-
pm-bus'.  I would say the register space of the CSR module is mixed
together, e.g., LVDS PHY child device has a register offset 0x00,
PXL2DPI child device has a register offset 0x40 and LDB child device
has register offsets 0x20 and 0xe0 in i.MX8qxp MIPI DSI/LVDS combo
subsystem CSR module register space.  So, it appears to be a MFD.

Lee, what do you think?  If it is indeed an MFD, a new MFD driver for
the CSR module is needed then.

Regards,
Liu Ying
Rob Herring Nov. 15, 2022, 1:33 p.m. UTC | #13 
On Mon, Nov 14, 2022 at 11:22 PM Liu Ying <victor.liu@nxp.com> wrote:
> On Mon, 2022-11-14 at 14:54 -0600, Rob Herring wrote:
> > On Mon, Nov 7, 2022 at 9:58 PM Liu Ying <victor.liu@nxp.com> wrote:
> > > Hi Lee,
> > >
> > > On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > > > On Wed, 02 Nov 2022, Liu Ying wrote:
> > > >
> > > > > Hi Lee,
> > > > >
> > > > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > > > > Hi Lee,
> > > > > >
> > > > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > > > >

[...]

> > > > > > > > +     ret = devm_of_platform_populate(&pdev->dev);
> > > > > > >
> > > > > > > The use of this API does not constitute a MFD.
> > > > > > >
> > > > > > > Please use "simple-mfd" instead.
> > > > > >
> > > > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > > > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > > > > devices.
> > > >
> > > > Right, simple-pm-bus will not populate child devices, because:
> > >
> > > simple-pm-bus.c may populate child devices of simple-pm-bus devices
> > > because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
> > >
> > > simple-pm-bus.c would _not_ populate child devices of simple-mfd
> > > devices because "ONLY_BUS" is set for simple-mfd devices.
> > >
> > > >   /*
> > > >    * These are transparent bus devices (not simple-pm-bus matches) that
> > > >    * have their child nodes populated automatically.  So, don't need to
> > > >    * do anything more. We only match with the device if this driver is
> > > >    * the most specific match because we don't want to incorrectly bind to
> > > >    * a device that has a more specific driver.
> > > >    */
> > > >
> > > > So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
> > >
> > > If simple-mfd device is a child device of one device listed in
> > > of_default_bus_match_table[], then it may be populated by
> > > drivers/of/platform.c.  But, in my case, simple-mfd device is a child
> > > device of simple-pm-bus device(not listed in that table), so it will
> > > not be populated by drivers/of/platform.c.  Instead,
> > > drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
> > >
> > > > > > Also, the simple-pm-bus driver would not enable runtime
> > > > > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > > > > means it would not propagate power management operations from child
> > > > > > devices of simple-mfd devices to parent devices of simple-mfd
> > > > > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed.
> > > >
> > > > This is more of an issue.
> > > >
> > > > Why can't this device use "simple-pm-bus" to have support for both
> > > > auto-registration AND Runtime PM?
> > >
> > > If I change the compatible string of the CSR module from
> > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> > > to
> > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> > > all devices I'm interested in are populated and all relevant drivers
> > > can probe.  But, this change makes 'make dt_binding_check' for the
> > > existing fsl,imx8qxp-csr.yaml fail:
> >
> > As 'simple-bus' is for MMIO devices, so is 'simple-pm-bus' with the
> > addition of PM capabilities. That means you have registers defined
> > (reg), but you don't.
> >
> > Either you have a block with mixed functions or you have separate
> > blocks. If the register space is all mixed together, then it is the
> > former and an MFD. Don't be designing your binding based on Linux
> > behavior.
>
> Thanks for clarifying how to differetiate MFD and 'simple-bus'/'simple-
> pm-bus'.  I would say the register space of the CSR module is mixed
> together, e.g., LVDS PHY child device has a register offset 0x00,
> PXL2DPI child device has a register offset 0x40 and LDB child device
> has register offsets 0x20 and 0xe0 in i.MX8qxp MIPI DSI/LVDS combo
> subsystem CSR module register space.  So, it appears to be a MFD.
>
> Lee, what do you think?  If it is indeed an MFD, a new MFD driver for
> the CSR module is needed then.

There already exists a driver which does what you need, so why create
a 2nd one? Just add the "fsl,imx8qxp-mipi-lvds-csr" compatible to the
simple-pm-bus match table and don't set ONLY_BUS. Isn't that enough?

Rob
Liu Ying Nov. 16, 2022, 3:12 a.m. UTC | #14 
On Tue, 2022-11-15 at 07:33 -0600, Rob Herring wrote:
> On Mon, Nov 14, 2022 at 11:22 PM Liu Ying <victor.liu@nxp.com> wrote:
> > On Mon, 2022-11-14 at 14:54 -0600, Rob Herring wrote:
> > > On Mon, Nov 7, 2022 at 9:58 PM Liu Ying <victor.liu@nxp.com> wrote:
> > > > Hi Lee,
> > > > 
> > > > On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > > > > On Wed, 02 Nov 2022, Liu Ying wrote:
> > > > > 
> > > > > > Hi Lee,
> > > > > > 
> > > > > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > > > > > Hi Lee,
> > > > > > > 
> > > > > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > > > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > > > > > 
> 
> [...]
> 
> > > > > > > > > +     ret = devm_of_platform_populate(&pdev->dev);
> > > > > > > > 
> > > > > > > > The use of this API does not constitute a MFD.
> > > > > > > > 
> > > > > > > > Please use "simple-mfd" instead.
> > > > > > > 
> > > > > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > > > > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > > > > > devices.
> > > > > 
> > > > > Right, simple-pm-bus will not populate child devices, because:
> > > > 
> > > > simple-pm-bus.c may populate child devices of simple-pm-bus devices
> > > > because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
> > > > 
> > > > simple-pm-bus.c would _not_ populate child devices of simple-mfd
> > > > devices because "ONLY_BUS" is set for simple-mfd devices.
> > > > 
> > > > >   /*
> > > > >    * These are transparent bus devices (not simple-pm-bus matches) that
> > > > >    * have their child nodes populated automatically.  So, don't need to
> > > > >    * do anything more. We only match with the device if this driver is
> > > > >    * the most specific match because we don't want to incorrectly bind to
> > > > >    * a device that has a more specific driver.
> > > > >    */
> > > > > 
> > > > > So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
> > > > 
> > > > If simple-mfd device is a child device of one device listed in
> > > > of_default_bus_match_table[], then it may be populated by
> > > > drivers/of/platform.c.  But, in my case, simple-mfd device is a child
> > > > device of simple-pm-bus device(not listed in that table), so it will
> > > > not be populated by drivers/of/platform.c.  Instead,
> > > > drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
> > > > 
> > > > > > > Also, the simple-pm-bus driver would not enable runtime
> > > > > > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > > > > > means it would not propagate power management operations from child
> > > > > > > devices of simple-mfd devices to parent devices of simple-mfd
> > > > > > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed.
> > > > > 
> > > > > This is more of an issue.
> > > > > 
> > > > > Why can't this device use "simple-pm-bus" to have support for both
> > > > > auto-registration AND Runtime PM?
> > > > 
> > > > If I change the compatible string of the CSR module from
> > > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> > > > to
> > > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> > > > all devices I'm interested in are populated and all relevant drivers
> > > > can probe.  But, this change makes 'make dt_binding_check' for the
> > > > existing fsl,imx8qxp-csr.yaml fail:
> > > 
> > > As 'simple-bus' is for MMIO devices, so is 'simple-pm-bus' with the
> > > addition of PM capabilities. That means you have registers defined
> > > (reg), but you don't.
> > > 
> > > Either you have a block with mixed functions or you have separate
> > > blocks. If the register space is all mixed together, then it is the
> > > former and an MFD. Don't be designing your binding based on Linux
> > > behavior.
> > 
> > Thanks for clarifying how to differetiate MFD and 'simple-bus'/'simple-
> > pm-bus'.  I would say the register space of the CSR module is mixed
> > together, e.g., LVDS PHY child device has a register offset 0x00,
> > PXL2DPI child device has a register offset 0x40 and LDB child device
> > has register offsets 0x20 and 0xe0 in i.MX8qxp MIPI DSI/LVDS combo
> > subsystem CSR module register space.  So, it appears to be a MFD.
> > 
> > Lee, what do you think?  If it is indeed an MFD, a new MFD driver for
> > the CSR module is needed then.
> 
> There already exists a driver which does what you need, so why create
> a 2nd one? Just add the "fsl,imx8qxp-mipi-lvds-csr" compatible to the
> simple-pm-bus match table and don't set ONLY_BUS. Isn't that enough?

Adding "fsl,imx8qxp-mipi-lvds-csr" and "fsl,imx8qm-lvds-csr" compatible
strings to the simple-pm-bus match table does work, but it tends to
make people think the CSR module is a bus instead of a MFD.  It's kinka
straightforward if a MFD device driver lives in drivers/mfd directory.
However, since "simple-mfd" compatible is also in that match table, I
would add the CSR module's compatible strings to that match table if no
objections.

Thanks,
Liu Ying
Lee Jones Nov. 16, 2022, 1:57 p.m. UTC | #15 
On Wed, 16 Nov 2022, Liu Ying wrote:

> On Tue, 2022-11-15 at 07:33 -0600, Rob Herring wrote:
> > On Mon, Nov 14, 2022 at 11:22 PM Liu Ying <victor.liu@nxp.com> wrote:
> > > On Mon, 2022-11-14 at 14:54 -0600, Rob Herring wrote:
> > > > On Mon, Nov 7, 2022 at 9:58 PM Liu Ying <victor.liu@nxp.com> wrote:
> > > > > Hi Lee,
> > > > > 
> > > > > On Mon, 2022-11-07 at 09:05 +0000, Lee Jones wrote:
> > > > > > On Wed, 02 Nov 2022, Liu Ying wrote:
> > > > > > 
> > > > > > > Hi Lee,
> > > > > > > 
> > > > > > > On Tue, 2022-11-01 at 13:53 +0800, Liu Ying wrote:
> > > > > > > > Hi Lee,
> > > > > > > > 
> > > > > > > > On Mon, 2022-10-31 at 15:40 +0000, Lee Jones wrote:
> > > > > > > > > On Mon, 17 Oct 2022, Liu Ying wrote:
> > > > > > > > > 
> > 
> > [...]
> > 
> > > > > > > > > > +     ret = devm_of_platform_populate(&pdev->dev);
> > > > > > > > > 
> > > > > > > > > The use of this API does not constitute a MFD.
> > > > > > > > > 
> > > > > > > > > Please use "simple-mfd" instead.
> > > > > > > > 
> > > > > > > > simple-mfd devices have "ONLY_BUS" set in simple-pm-bus.c, so the
> > > > > > > > simple-pm-bus driver would not populate child devices of simple-mfd
> > > > > > > > devices.
> > > > > > 
> > > > > > Right, simple-pm-bus will not populate child devices, because:
> > > > > 
> > > > > simple-pm-bus.c may populate child devices of simple-pm-bus devices
> > > > > because "ONLY_BUS" is _not_ set for simple-pm-bus devices.
> > > > > 
> > > > > simple-pm-bus.c would _not_ populate child devices of simple-mfd
> > > > > devices because "ONLY_BUS" is set for simple-mfd devices.
> > > > > 
> > > > > >   /*
> > > > > >    * These are transparent bus devices (not simple-pm-bus matches) that
> > > > > >    * have their child nodes populated automatically.  So, don't need to
> > > > > >    * do anything more. We only match with the device if this driver is
> > > > > >    * the most specific match because we don't want to incorrectly bind to
> > > > > >    * a device that has a more specific driver.
> > > > > >    */
> > > > > > 
> > > > > > So "simple-mfd" must be populated elsewhere i.e. drivers/of/platform.c.
> > > > > 
> > > > > If simple-mfd device is a child device of one device listed in
> > > > > of_default_bus_match_table[], then it may be populated by
> > > > > drivers/of/platform.c.  But, in my case, simple-mfd device is a child
> > > > > device of simple-pm-bus device(not listed in that table), so it will
> > > > > not be populated by drivers/of/platform.c.  Instead,
> > > > > drivers/bus/simple-pm-bus.c would populate the simple-mfd device.
> > > > > 
> > > > > > > > Also, the simple-pm-bus driver would not enable runtime
> > > > > > > > power management for simple-mfd devices due to "ONLY_BUS", which
> > > > > > > > means it would not propagate power management operations from child
> > > > > > > > devices of simple-mfd devices to parent devices of simple-mfd
> > > > > > > > devices.  That's why a dedicated fsl-imx8qxp-csr driver is needed.
> > > > > > 
> > > > > > This is more of an issue.
> > > > > > 
> > > > > > Why can't this device use "simple-pm-bus" to have support for both
> > > > > > auto-registration AND Runtime PM?
> > > > > 
> > > > > If I change the compatible string of the CSR module from
> > > > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-mfd"
> > > > > to
> > > > > "fsl,imx8qxp-mipi-lvds-csr", "syscon", "simple-pm-bus",
> > > > > all devices I'm interested in are populated and all relevant drivers
> > > > > can probe.  But, this change makes 'make dt_binding_check' for the
> > > > > existing fsl,imx8qxp-csr.yaml fail:
> > > > 
> > > > As 'simple-bus' is for MMIO devices, so is 'simple-pm-bus' with the
> > > > addition of PM capabilities. That means you have registers defined
> > > > (reg), but you don't.
> > > > 
> > > > Either you have a block with mixed functions or you have separate
> > > > blocks. If the register space is all mixed together, then it is the
> > > > former and an MFD. Don't be designing your binding based on Linux
> > > > behavior.
> > > 
> > > Thanks for clarifying how to differetiate MFD and 'simple-bus'/'simple-
> > > pm-bus'.  I would say the register space of the CSR module is mixed
> > > together, e.g., LVDS PHY child device has a register offset 0x00,
> > > PXL2DPI child device has a register offset 0x40 and LDB child device
> > > has register offsets 0x20 and 0xe0 in i.MX8qxp MIPI DSI/LVDS combo
> > > subsystem CSR module register space.  So, it appears to be a MFD.
> > > 
> > > Lee, what do you think?  If it is indeed an MFD, a new MFD driver for
> > > the CSR module is needed then.
> > 
> > There already exists a driver which does what you need, so why create
> > a 2nd one? Just add the "fsl,imx8qxp-mipi-lvds-csr" compatible to the
> > simple-pm-bus match table and don't set ONLY_BUS. Isn't that enough?
> 
> Adding "fsl,imx8qxp-mipi-lvds-csr" and "fsl,imx8qm-lvds-csr" compatible
> strings to the simple-pm-bus match table does work, but it tends to
> make people think the CSR module is a bus instead of a MFD.

MFDs don't exist.  They're a figment of our imagination.

If you want PM enabled and wish for your child devices to be
auto-magically registered for you, simple-pm-bus is what you need.

> It's kinka
> straightforward if a MFD device driver lives in drivers/mfd directory.
> However, since "simple-mfd" compatible is also in that match table, I
> would add the CSR module's compatible strings to that match table if no
> objections.

Fine by me if it saves authoring a pointless driver.
diff mbox series

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b93856de432..966a250d7910 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -499,6 +499,16 @@  config MFD_MX25_TSADC
 	  i.MX25 processors. They consist of a conversion queue for general
 	  purpose ADC and a queue for Touchscreens.
 
+config MFD_MX8QXP_CSR
+	tristate "Freescale i.MX8qxp Control and Status Registers (CSR) Module"
+	depends on (ARCH_MXC && OF) || COMPILE_TEST
+	help
+	  Enable support for Freescale i.MX8qm/qxp Control and Status Registers
+	  (CSR) Module. As a system controller, CSR represents a set of
+	  miscellaneous registers of a specific subsystem. It may provide
+	  control and/or status report interfaces to a mix of standalone
+	  hardware devices within that subsystem.
+
 config MFD_HI6421_PMIC
 	tristate "HiSilicon Hi6421 PMU/Codec IC"
 	depends on OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 7ed3ef4a698c..5c3c13d7ce85 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -113,6 +113,7 @@  obj-$(CONFIG_MFD_TWL4030_AUDIO)	+= twl4030-audio.o
 obj-$(CONFIG_TWL6040_CORE)	+= twl6040.o
 
 obj-$(CONFIG_MFD_MX25_TSADC)	+= fsl-imx25-tsadc.o
+obj-$(CONFIG_MFD_MX8QXP_CSR)	+= fsl-imx8qxp-csr.o
 
 obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
 obj-$(CONFIG_MFD_MC13XXX_SPI)	+= mc13xxx-spi.o
diff --git a/drivers/mfd/fsl-imx8qxp-csr.c b/drivers/mfd/fsl-imx8qxp-csr.c
new file mode 100644
index 000000000000..3915d3d6ca65
--- /dev/null
+++ b/drivers/mfd/fsl-imx8qxp-csr.c
@@ -0,0 +1,53 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * Copyright 2022 NXP
+ */
+
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+static int imx8qxp_csr_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = devm_of_platform_populate(&pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to populate sub-devices: %d\n", ret);
+		pm_runtime_disable(&pdev->dev);
+	}
+
+	return ret;
+}
+
+static int imx8qxp_csr_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
+static const struct of_device_id imx8qxp_csr_of_match[] = {
+	{ .compatible = "fsl,imx8qxp-mipi-lvds-csr", },
+	{ .compatible = "fsl,imx8qm-lvds-csr", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx8qxp_csr_of_match);
+
+static struct platform_driver imx8qxp_csr_driver = {
+	.probe = imx8qxp_csr_probe,
+	.remove = imx8qxp_csr_remove,
+	.driver = {
+		.name = "imx8qxp-csr",
+		.of_match_table = imx8qxp_csr_of_match,
+	},
+};
+
+module_platform_driver(imx8qxp_csr_driver);
+
+MODULE_DESCRIPTION("Freescale i.MX8qm/qxp Control and Status Registers Module Driver");
+MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
+MODULE_LICENSE("GPL v2");