Messages in this thread | | | Subject | Re: [PATCH RESEND] mfd: Add Freescale i.MX8qxp Control and Status Registers (CSR) module driver | From | Liu Ying <> | Date | Tue, 15 Nov 2022 13:21:58 +0800 |
| |
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
| |