Message-ID: <AM7PR04MB70467B746B6CA03CD2FB0982983E9@AM7PR04MB7046.eurprd04.prod.outlook.com>
Date:   Wed, 9 Nov 2022 12:50:55 +0000
From:   Ying Liu <victor.liu@....com>
To:     Lee Jones <lee@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH RESEND] mfd: Add Freescale i.MX8qxp Control and Status
 Registers (CSR) module driver

> -----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@....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@...21000: $nodename:0: 'syscon@...21000' 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@...21000: '#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@...21000: '#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@...21000: '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@...21000' 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@...21000' to 'bus@...21000', then the
> below error comes along:
> --------------------------------8<------------------------------------
> /kernel/linux/Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> csr.example.dtb: bus@...21000: $nodename:0: 'bus@...21000' 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

Powered by blists - more mailing lists