Hi,
This series aims to add Freescale i.MX8qxp pixel link MSI bus support
by using the existing simple-pm-bus driver. A power domain and two input
clocks need to be enabled before the MSI bus accesses it's child devices,
which matches what a simple power-managed bus is(See simple-pm-bus.yaml).
Patch 1 adds support to populate simple MFD child devices in the
simple-pm-bus driver, since the MSI bus may connect those devices.
Patch 2 enables/disables functional clock(s) as a bulk in the
simple-pm-bus driver when the simple-pm-bus is being power managed,
since the MSI bus takes the two input clocks as functional clocks.
Patch 3 adds dt-bindings for the MSI bus.
v2->v3:
* Add a pattern property to allow child nodes in the MSI bus dt-binding. (Rob)
v1->v2:
Address Krzysztof's comments on patch 3:
* Add a select to explicitly select the MSI bus dt-binding.
* List 'simple-pm-bus' explicitly as one item of compatible strings.
* Require compatible and reg properties.
* Put reg property just after compatible property in example.
Liu Ying (3):
drivers: bus: simple-pm-bus: Populate simple MFD child devices
drivers: bus: simple-pm-bus: Use clocks
dt-bindings: bus: Add Freescale i.MX8qxp pixel link MSI bus binding
.../bus/fsl,imx8qxp-pixel-link-msi-bus.yaml | 108 ++++++++++++++++++
drivers/bus/simple-pm-bus.c | 61 +++++++++-
2 files changed, 168 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-msi-bus.yaml
--
2.25.1
Freescale i.MX8qxp pixel link MSI bus is a simple memory-mapped bus.
It is used to access peripherals in i.MX8qm/qxp imaging, LVDS, MIPI
DSI and HDMI TX subsystems, like I2C controller, PWM controller,
MIPI DSI controller and Control and Status Registers (CSR) module.
Reference simple-pm-bus bindings and add Freescale i.MX8qxp pixel
link MSI bus specific bindings.
Signed-off-by: Liu Ying <[email protected]>
---
v2->v3:
* Add a pattern property to allow child nodes. (Rob)
v1->v2:
Address Krzysztof's comments:
* Add a select to explicitly select the MSI bus dt-binding.
* List 'simple-pm-bus' explicitly as one item of compatible strings.
* Require compatible and reg properties.
* Put reg property just after compatible property in example.
.../bus/fsl,imx8qxp-pixel-link-msi-bus.yaml | 108 ++++++++++++++++++
1 file changed, 108 insertions(+)
create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-msi-bus.yaml
diff --git a/Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-msi-bus.yaml b/Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-msi-bus.yaml
new file mode 100644
index 000000000000..9fc6623a7061
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-msi-bus.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bus/fsl,imx8qxp-pixel-link-msi-bus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX8qxp Pixel Link Medium Speed Interconnect (MSI) Bus
+
+maintainers:
+ - Liu Ying <[email protected]>
+
+description: |
+ i.MX8qxp pixel link MSI bus is used to control settings of PHYs, I/Os
+ sitting together with the PHYs. It is not the same as the MSI bus coming
+ from i.MX8 System Controller Unit (SCU) which is used to control power,
+ clock and reset through the i.MX8 Distributed Slave System Controller (DSC).
+
+ i.MX8qxp pixel link MSI bus is a simple memory-mapped bus. Two input clocks,
+ that is, MSI clock and AHB clock, need to be enabled so that peripherals
+ connected to the bus can be accessed. Also, the bus is part of a power
+ domain. The power domain needs to be enabled before the peripherals can
+ be accessed.
+
+ Peripherals in i.MX8qm/qxp imaging, LVDS, MIPI DSI and HDMI TX subsystems,
+ like I2C controller, PWM controller, MIPI DSI controller and Control and
+ Status Registers (CSR) module, are accessed through the bus.
+
+ The i.MX System Controller Firmware (SCFW) owns and uses the i.MX8qm/qxp
+ pixel link MSI bus controller and does not allow SCFW user to control it.
+ So, the controller's registers cannot be accessed by SCFW user. Hence,
+ the interrupts generated by the controller don't make any sense from SCFW
+ user's point of view.
+
+allOf:
+ - $ref: simple-pm-bus.yaml#
+
+# We need a select here so we don't match all nodes with 'simple-pm-bus'.
+select:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - fsl,imx8qxp-display-pixel-link-msi-bus
+ - fsl,imx8qm-display-pixel-link-msi-bus
+ required:
+ - compatible
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - fsl,imx8qxp-display-pixel-link-msi-bus
+ - fsl,imx8qm-display-pixel-link-msi-bus
+ - const: simple-pm-bus
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: master gated clock from system
+ - description: AHB clock
+
+ clock-names:
+ items:
+ - const: msi
+ - const: ahb
+
+patternProperties:
+ "^.*@[0-9a-f]+$":
+ description: Devices attached to the bus
+ type: object
+ properties:
+ reg:
+ maxItems: 1
+
+ required:
+ - reg
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - power-domains
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx8-lpcg.h>
+ #include <dt-bindings/firmware/imx/rsrc.h>
+ bus@56200000 {
+ compatible = "fsl,imx8qxp-display-pixel-link-msi-bus", "simple-pm-bus";
+ reg = <0x56200000 0x20000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ interrupt-parent = <&dc0_irqsteer>;
+ interrupts = <320>;
+ ranges;
+ clocks = <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>,
+ <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>;
+ clock-names = "msi", "ahb";
+ power-domains = <&pd IMX_SC_R_DC_0>;
+ };
--
2.25.1
Simple Power-Managed bus controller may need functional clock(s)
to be enabled before child devices connected to the bus can be
accessed. Get the clock(s) as a bulk and enable/disable the
clock(s) when the bus is being power managed.
One example is that Freescale i.MX8qxp pixel link MSI bus controller
needs MSI clock and AHB clock to be enabled before accessing child
devices.
Signed-off-by: Liu Ying <[email protected]>
---
v1->v3:
* No change.
drivers/bus/simple-pm-bus.c | 54 +++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index ff5f8ca5c024..876a906724b3 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -8,11 +8,17 @@
* for more details.
*/
+#include <linux/clk.h>
#include <linux/module.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+struct simple_pm_bus {
+ struct clk_bulk_data *clks;
+ int num_clks;
+};
+
static const struct of_device_id simple_pm_bus_child_matches[] = {
{ .compatible = "simple-mfd", },
{}
@@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
struct device_node *np = dev->of_node;
const struct of_device_id *match;
+ struct simple_pm_bus *bus;
/*
* Allow user to use driver_override to bind this driver to a
@@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
return -ENODEV;
}
+ bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+ if (!bus)
+ return -ENOMEM;
+
+ bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus->clks);
+ if (bus->num_clks < 0)
+ return dev_err_probe(&pdev->dev, bus->num_clks, "failed to get clocks\n");
+
+ dev_set_drvdata(&pdev->dev, bus);
+
dev_dbg(&pdev->dev, "%s\n", __func__);
pm_runtime_enable(&pdev->dev);
@@ -72,6 +89,42 @@ static int simple_pm_bus_remove(struct platform_device *pdev)
return 0;
}
+static int simple_pm_bus_runtime_suspend(struct device *dev)
+{
+ struct simple_pm_bus *bus = dev_get_drvdata(dev);
+
+ if (!bus)
+ return 0;
+
+ clk_bulk_disable_unprepare(bus->num_clks, bus->clks);
+
+ return 0;
+}
+
+static int simple_pm_bus_runtime_resume(struct device *dev)
+{
+ struct simple_pm_bus *bus = dev_get_drvdata(dev);
+ int ret;
+
+ if (!bus)
+ return 0;
+
+ ret = clk_bulk_prepare_enable(bus->num_clks, bus->clks);
+ if (ret) {
+ dev_err(dev, "failed to enable clocks: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops simple_pm_bus_pm_ops = {
+ SET_RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend,
+ simple_pm_bus_runtime_resume, NULL)
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
+};
+
#define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */
static const struct of_device_id simple_pm_bus_of_match[] = {
@@ -90,6 +143,7 @@ static struct platform_driver simple_pm_bus_driver = {
.driver = {
.name = "simple-pm-bus",
.of_match_table = simple_pm_bus_of_match,
+ .pm = &simple_pm_bus_pm_ops,
},
};
--
2.25.1
There could be simple MFD device(s) connected to a simple PM bus as child
node(s), like Freescale i.MX8qxp pixel link MSI bus. Add a child match
table as an argument to of_platform_populate() function call to specify
the simple MFD devices so that they can be populated.
Signed-off-by: Liu Ying <[email protected]>
---
v1->v3:
* No change.
drivers/bus/simple-pm-bus.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index 6b8d6257ed8a..ff5f8ca5c024 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -13,6 +13,11 @@
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+static const struct of_device_id simple_pm_bus_child_matches[] = {
+ { .compatible = "simple-mfd", },
+ {}
+};
+
static int simple_pm_bus_probe(struct platform_device *pdev)
{
const struct device *dev = &pdev->dev;
@@ -49,7 +54,7 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
if (np)
- of_platform_populate(np, NULL, lookup, &pdev->dev);
+ of_platform_populate(np, simple_pm_bus_child_matches, lookup, &pdev->dev);
return 0;
}
--
2.25.1
On 04/08/2022 08:11, Liu Ying wrote:
> Freescale i.MX8qxp pixel link MSI bus is a simple memory-mapped bus.
> It is used to access peripherals in i.MX8qm/qxp imaging, LVDS, MIPI
> DSI and HDMI TX subsystems, like I2C controller, PWM controller,
> MIPI DSI controller and Control and Status Registers (CSR) module.
>
> Reference simple-pm-bus bindings and add Freescale i.MX8qxp pixel
> link MSI bus specific bindings.
>
> Signed-off-by: Liu Ying <[email protected]>
Thank you for your patch. There is something to discuss/improve.
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/imx8-lpcg.h>
> + #include <dt-bindings/firmware/imx/rsrc.h>
> + bus@56200000 {
> + compatible = "fsl,imx8qxp-display-pixel-link-msi-bus", "simple-pm-bus";
> + reg = <0x56200000 0x20000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + interrupt-parent = <&dc0_irqsteer>;
> + interrupts = <320>;
> + ranges;
> + clocks = <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>,
> + <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>;
> + clock-names = "msi", "ahb";
> + power-domains = <&pd IMX_SC_R_DC_0>;
The example should be complete, so you should have here children.
Otherwise it is not a bus.
Best regards,
Krzysztof
On Thu, Aug 4, 2022 at 12:10 AM Liu Ying <[email protected]> wrote:
>
> There could be simple MFD device(s) connected to a simple PM bus as child
> node(s), like Freescale i.MX8qxp pixel link MSI bus. Add a child match
> table as an argument to of_platform_populate() function call to specify
> the simple MFD devices so that they can be populated.
There could be a simple-bus under it as well. You should just use
of_platform_default_populate() instead.
>
> Signed-off-by: Liu Ying <[email protected]>
> ---
> v1->v3:
> * No change.
>
> drivers/bus/simple-pm-bus.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index 6b8d6257ed8a..ff5f8ca5c024 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -13,6 +13,11 @@
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
>
> +static const struct of_device_id simple_pm_bus_child_matches[] = {
> + { .compatible = "simple-mfd", },
> + {}
> +};
> +
> static int simple_pm_bus_probe(struct platform_device *pdev)
> {
> const struct device *dev = &pdev->dev;
> @@ -49,7 +54,7 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> pm_runtime_enable(&pdev->dev);
>
> if (np)
> - of_platform_populate(np, NULL, lookup, &pdev->dev);
> + of_platform_populate(np, simple_pm_bus_child_matches, lookup, &pdev->dev);
>
> return 0;
> }
> --
> 2.25.1
>
On Thu, Aug 4, 2022 at 5:18 AM Rob Herring <[email protected]> wrote:
>
> On Thu, Aug 4, 2022 at 12:10 AM Liu Ying <[email protected]> wrote:
> >
> > There could be simple MFD device(s) connected to a simple PM bus as child
> > node(s), like Freescale i.MX8qxp pixel link MSI bus. Add a child match
> > table as an argument to of_platform_populate() function call to specify
> > the simple MFD devices so that they can be populated.
>
> There could be a simple-bus under it as well. You should just use
> of_platform_default_populate() instead.
I'm confused why we even need this patch. Wouldn't this driver
automatically probe simple-mfd buses and populate its child devices?
We already have it in simple_pm_bus_of_match.
I'm wondering if you are trying to workaround the behavior of having
"ONLY_BUS" set in simple_pm_bus_of_match for "simple-mfd". Have you
tried deleting that field and see if it does what you want?
And we wouldn't need to use of_platform_default_populate() because
this driver would take care of doing that recursively. Especially when
you need the clocks and power domain to be able to access the child
devices, you want the driver to probe and do that at each level before
automatically recursively adding all the grand-children devices.
-Saravana
>
> >
> > Signed-off-by: Liu Ying <[email protected]>
> > ---
> > v1->v3:
> > * No change.
> >
> > drivers/bus/simple-pm-bus.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> > index 6b8d6257ed8a..ff5f8ca5c024 100644
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -13,6 +13,11 @@
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> >
> > +static const struct of_device_id simple_pm_bus_child_matches[] = {
> > + { .compatible = "simple-mfd", },
> > + {}
> > +};
> > +
> > static int simple_pm_bus_probe(struct platform_device *pdev)
> > {
> > const struct device *dev = &pdev->dev;
> > @@ -49,7 +54,7 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> > pm_runtime_enable(&pdev->dev);
> >
> > if (np)
> > - of_platform_populate(np, NULL, lookup, &pdev->dev);
> > + of_platform_populate(np, simple_pm_bus_child_matches, lookup, &pdev->dev);
> >
> > return 0;
> > }
> > --
> > 2.25.1
> >
On Thu, 2022-08-04 at 11:26 -0700, Saravana Kannan wrote:
> On Thu, Aug 4, 2022 at 5:18 AM Rob Herring <[email protected]>
> wrote:
> >
> > On Thu, Aug 4, 2022 at 12:10 AM Liu Ying <[email protected]>
> > wrote:
> > >
> > > There could be simple MFD device(s) connected to a simple PM bus
> > > as child
> > > node(s), like Freescale i.MX8qxp pixel link MSI bus. Add a child
> > > match
> > > table as an argument to of_platform_populate() function call to
> > > specify
> > > the simple MFD devices so that they can be populated.
> >
> > There could be a simple-bus under it as well. You should just use
> > of_platform_default_populate() instead.
>
> I'm confused why we even need this patch. Wouldn't this driver
> automatically probe simple-mfd buses and populate its child devices?
> We already have it in simple_pm_bus_of_match.
First of all, this driver doesn't populate simple-mfd bus's child
devices because "ONLY_BUS" is set in simple_pm_bus_of_match[] for
simple-mfd.
The device tree I'm working with is something like this:
bus@560000000 {
compatible = "fsl,aips-bus", "simple-bus";
...
bus@562000000 {
compatible = "fsl,imx8qm-display-pixel-link-msi-bus", "simple-
pm-bus";
...
syscon@56241000 {
compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-mfd";
...
syscon_child {};
};
/* more regular mmap devices */
};
};
IIUC, default buses listed in of_default_bus_match_table[], including
simple-bus and simple-mfd, are populated by
of_platform_default_populate() in a recursive fashion, when
of_platform_default_populate_init() is called. However, simple-pm-bus
is not listed in that table. So, bus@562000000 (simple-pm-bus) is the
last one to be populated successfully and syscon@56241000 (simple-mfd)
is not populated (recursion stops).
Then, this patch adds a match table to populate syscon@56241000 (simple
-mfd) _and_ it's child nodes when bus@562000000 (simple-pm-bus) is
probed. of_platform_populate() will populate syscon@56241000 (simple-
mfd) and it's child devices (sycon_child) together. Hence, sycon_child
devices will be probed ok.
The problem is that syscon@56241000 (simple-mfd) fails to be probed
with return code -ENODEV as "ONLY_BUS" is set and "simple-mfd" is the
3rd compatible string. Even if it's probed ok, syscon@56241000 (simple-
mfd) is not power managed, which means syscon_child devices' PM
operations won't be propagated to bus@562000000 (simple-pm-bus) (?).
Anyway, somehow, syscon_child devices do work, based on my test.
With regard to PM, simple-bus is the same if it sits at simple-mfd's
place. So, maybe, simple-mfd and simple-bus should be power managed as
well? Or, simple-pm-bus should have no simple-mfd and simple-bus child
nodes at all?
>
> I'm wondering if you are trying to workaround the behavior of having
> "ONLY_BUS" set in simple_pm_bus_of_match for "simple-mfd". Have you
> tried deleting that field and see if it does what you want?
Without this patch, deleting "ONLY_BUS" works for me, as syscon_child
devices are populated when syscon@56241000 (simple-mfd) is probed.
Deleting "ONLY_BUS" may make simple-mfd a power managed device. Is it a
right thing to do?
Regards,
Liu Ying
>
> And we wouldn't need to use of_platform_default_populate() because
> this driver would take care of doing that recursively. Especially
> when
> you need the clocks and power domain to be able to access the child
> devices, you want the driver to probe and do that at each level
> before
> automatically recursively adding all the grand-children devices.
>
> -Saravana
>
> >
> > >
> > > Signed-off-by: Liu Ying <[email protected]>
> > > ---
> > > v1->v3:
> > > * No change.
> > >
> > > drivers/bus/simple-pm-bus.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-
> > > bus.c
> > > index 6b8d6257ed8a..ff5f8ca5c024 100644
> > > --- a/drivers/bus/simple-pm-bus.c
> > > +++ b/drivers/bus/simple-pm-bus.c
> > > @@ -13,6 +13,11 @@
> > > #include <linux/platform_device.h>
> > > #include <linux/pm_runtime.h>
> > >
> > > +static const struct of_device_id simple_pm_bus_child_matches[] =
> > > {
> > > + { .compatible = "simple-mfd", },
> > > + {}
> > > +};
> > > +
> > > static int simple_pm_bus_probe(struct platform_device *pdev)
> > > {
> > > const struct device *dev = &pdev->dev;
> > > @@ -49,7 +54,7 @@ static int simple_pm_bus_probe(struct
> > > platform_device *pdev)
> > > pm_runtime_enable(&pdev->dev);
> > >
> > > if (np)
> > > - of_platform_populate(np, NULL, lookup, &pdev-
> > > >dev);
> > > + of_platform_populate(np,
> > > simple_pm_bus_child_matches, lookup, &pdev->dev);
> > >
> > > return 0;
> > > }
> > > --
> > > 2.25.1
> > >
On Thu, 2022-08-04 at 13:17 +0200, Krzysztof Kozlowski wrote:
> On 04/08/2022 08:11, Liu Ying wrote:
> > Freescale i.MX8qxp pixel link MSI bus is a simple memory-mapped
> > bus.
> > It is used to access peripherals in i.MX8qm/qxp imaging, LVDS, MIPI
> > DSI and HDMI TX subsystems, like I2C controller, PWM controller,
> > MIPI DSI controller and Control and Status Registers (CSR) module.
> >
> > Reference simple-pm-bus bindings and add Freescale i.MX8qxp pixel
> > link MSI bus specific bindings.
> >
> > Signed-off-by: Liu Ying <[email protected]>
>
> Thank you for your patch. There is something to discuss/improve.
Thanks for the review.
>
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/imx8-lpcg.h>
> > + #include <dt-bindings/firmware/imx/rsrc.h>
> > + bus@56200000 {
> > + compatible = "fsl,imx8qxp-display-pixel-link-msi-bus",
> > "simple-pm-bus";
> > + reg = <0x56200000 0x20000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + interrupt-parent = <&dc0_irqsteer>;
> > + interrupts = <320>;
> > + ranges;
> > + clocks = <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>,
> > + <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>;
> > + clock-names = "msi", "ahb";
> > + power-domains = <&pd IMX_SC_R_DC_0>;
>
> The example should be complete, so you should have here children.
> Otherwise it is not a bus.
I may add some children whose compatible strings are in-tree for
i.MX8qxp. It seems that simple-pm-bus driver part(patch 1) will be
changed due to comments, so maybe I'll respin when it's ready.
Regards,
Liu Ying
>
> Best regards,
> Krzysztof
On Fri, Aug 5, 2022 at 3:07 AM Liu Ying <[email protected]> wrote:
>
> On Thu, 2022-08-04 at 11:26 -0700, Saravana Kannan wrote:
> > On Thu, Aug 4, 2022 at 5:18 AM Rob Herring <[email protected]>
> > wrote:
> > >
> > > On Thu, Aug 4, 2022 at 12:10 AM Liu Ying <[email protected]>
> > > wrote:
> > > >
> > > > There could be simple MFD device(s) connected to a simple PM bus
> > > > as child
> > > > node(s), like Freescale i.MX8qxp pixel link MSI bus. Add a child
> > > > match
> > > > table as an argument to of_platform_populate() function call to
> > > > specify
> > > > the simple MFD devices so that they can be populated.
> > >
> > > There could be a simple-bus under it as well. You should just use
> > > of_platform_default_populate() instead.
> >
> > I'm confused why we even need this patch. Wouldn't this driver
> > automatically probe simple-mfd buses and populate its child devices?
> > We already have it in simple_pm_bus_of_match.
>
> First of all, this driver doesn't populate simple-mfd bus's child
> devices because "ONLY_BUS" is set in simple_pm_bus_of_match[] for
> simple-mfd.
>
> The device tree I'm working with is something like this:
>
> bus@560000000 {
> compatible = "fsl,aips-bus", "simple-bus";
> ...
>
> bus@562000000 {
> compatible = "fsl,imx8qm-display-pixel-link-msi-bus", "simple-
> pm-bus";
> ...
>
> syscon@56241000 {
> compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-mfd";
> ...
>
> syscon_child {};
> };
>
> /* more regular mmap devices */
> };
> };
>
> IIUC, default buses listed in of_default_bus_match_table[], including
> simple-bus and simple-mfd, are populated by
> of_platform_default_populate() in a recursive fashion, when
> of_platform_default_populate_init() is called. However, simple-pm-bus
> is not listed in that table. So, bus@562000000 (simple-pm-bus) is the
> last one to be populated successfully and syscon@56241000 (simple-mfd)
> is not populated (recursion stops).
Ok, it's working as intended so far.
> Then, this patch adds a match table to populate syscon@56241000 (simple
> -mfd) _and_ it's child nodes when bus@562000000 (simple-pm-bus) is
> probed. of_platform_populate() will populate syscon@56241000 (simple-
> mfd) and it's child devices (sycon_child) together. Hence, sycon_child
> devices will be probed ok.
I think of_platform_default_populate() is the right solution here
instead of spinning up a new table. Because a tree of simple-bus
children of simple-pm-bus would have the same problem you are facing
with simple-mfd's children not being populated.
> The problem is that syscon@56241000 (simple-mfd) fails to be probed
> with return code -ENODEV as "ONLY_BUS" is set and "simple-mfd" is the
> 3rd compatible string.
Ah, thanks for the example of your DT. My bad, I had forgotten the
"simple-mfd" is one of the default populate busses and can be a 2nd or
later entry in the compatible string and still have its children
populated by default by OF platform code.
> Even if it's probed ok, syscon@56241000 (simple-
> mfd) is not power managed, which means syscon_child devices' PM
> operations won't be propagated to bus@562000000 (simple-pm-bus) (?).
> Anyway, somehow, syscon_child devices do work, based on my test.
Aren't you seeing this propagation issue even with your current patches?
> With regard to PM, simple-bus is the same if it sits at simple-mfd's
> place. So, maybe, simple-mfd and simple-bus should be power managed as
> well? Or, simple-pm-bus should have no simple-mfd and simple-bus child
> nodes at all?
The problem is that there are cases of devices with real drivers that
also list simple-bus as their secondary compatible string. So we can't
really remove any of the existing ONLY_BUS as that could cause
simple-pm-bus driver to probe them instead of the real driver.
In your case, why even list this as "fsl,imx8qm-lvds-csr"? Can't you
just change your compatible string from:
"fsl,imx8qm-lvds-csr", "syscon", "simple-mfd";
To:
"simple-pm-bus", "syscon", "simple-mfd";
You are treating it exactly as a simple-pm-bus. So I don't see what
this extra "fsl,imx8qm-lvds-csr" distinction brings. Or make it if you
really want the "fsl,imx8qm-lvds-csr" in there:
"fsl,imx8qm-lvds-csr", "simple-pm-bus", "syscon", "simple-mfd";
If you are actually going to write a driver for "fsl,imx8qm-lvds-csr"
then you need to have that driver bind to this device of yours and do
PM management and populate the child devices if they aren't already.
Long story short, with what I understand so far, I think what you need
to do are:
1. Patch to manage clock.
2. Patch to use of_platform_default_populate()
3. Fix up the compatible string to list simple-pm-bus in it.
>
> >
> > I'm wondering if you are trying to workaround the behavior of having
> > "ONLY_BUS" set in simple_pm_bus_of_match for "simple-mfd". Have you
> > tried deleting that field and see if it does what you want?
>
> Without this patch, deleting "ONLY_BUS" works for me, as syscon_child
> devices are populated when syscon@56241000 (simple-mfd) is probed.
> Deleting "ONLY_BUS" may make simple-mfd a power managed device. Is it a
> right thing to do?
Ignore my point about deleting ONLY_BUS. That's wrong because then the
simple-pm-bus driver can end up probing any device that lists
simple-mfd even if there's another driver that could (like
"fsl,imx8qm-lvds-csr") and we don't want that.
-Saravana
>
> Regards,
> Liu Ying
>
> >
> > And we wouldn't need to use of_platform_default_populate() because
> > this driver would take care of doing that recursively. Especially
> > when
> > you need the clocks and power domain to be able to access the child
> > devices, you want the driver to probe and do that at each level
> > before
> > automatically recursively adding all the grand-children devices.
> >
> > -Saravana
> >
> > >
> > > >
> > > > Signed-off-by: Liu Ying <[email protected]>
> > > > ---
> > > > v1->v3:
> > > > * No change.
> > > >
> > > > drivers/bus/simple-pm-bus.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-
> > > > bus.c
> > > > index 6b8d6257ed8a..ff5f8ca5c024 100644
> > > > --- a/drivers/bus/simple-pm-bus.c
> > > > +++ b/drivers/bus/simple-pm-bus.c
> > > > @@ -13,6 +13,11 @@
> > > > #include <linux/platform_device.h>
> > > > #include <linux/pm_runtime.h>
> > > >
> > > > +static const struct of_device_id simple_pm_bus_child_matches[] =
> > > > {
> > > > + { .compatible = "simple-mfd", },
> > > > + {}
> > > > +};
> > > > +
> > > > static int simple_pm_bus_probe(struct platform_device *pdev)
> > > > {
> > > > const struct device *dev = &pdev->dev;
> > > > @@ -49,7 +54,7 @@ static int simple_pm_bus_probe(struct
> > > > platform_device *pdev)
> > > > pm_runtime_enable(&pdev->dev);
> > > >
> > > > if (np)
> > > > - of_platform_populate(np, NULL, lookup, &pdev-
> > > > >dev);
> > > > + of_platform_populate(np,
> > > > simple_pm_bus_child_matches, lookup, &pdev->dev);
> > > >
> > > > return 0;
> > > > }
> > > > --
> > > > 2.25.1
> > > >
>
On Fri, 2022-08-05 at 10:55 -0700, Saravana Kannan wrote:
> On Fri, Aug 5, 2022 at 3:07 AM Liu Ying <[email protected]> wrote:
> >
> > On Thu, 2022-08-04 at 11:26 -0700, Saravana Kannan wrote:
> > > On Thu, Aug 4, 2022 at 5:18 AM Rob Herring <[email protected]>
> > > wrote:
> > > >
> > > > On Thu, Aug 4, 2022 at 12:10 AM Liu Ying <[email protected]>
> > > > wrote:
> > > > >
> > > > > There could be simple MFD device(s) connected to a simple PM
> > > > > bus
> > > > > as child
> > > > > node(s), like Freescale i.MX8qxp pixel link MSI bus. Add a
> > > > > child
> > > > > match
> > > > > table as an argument to of_platform_populate() function call
> > > > > to
> > > > > specify
> > > > > the simple MFD devices so that they can be populated.
> > > >
> > > > There could be a simple-bus under it as well. You should just
> > > > use
> > > > of_platform_default_populate() instead.
> > >
> > > I'm confused why we even need this patch. Wouldn't this driver
> > > automatically probe simple-mfd buses and populate its child
> > > devices?
> > > We already have it in simple_pm_bus_of_match.
> >
> > First of all, this driver doesn't populate simple-mfd bus's child
> > devices because "ONLY_BUS" is set in simple_pm_bus_of_match[] for
> > simple-mfd.
> >
> > The device tree I'm working with is something like this:
> >
> > bus@560000000 {
> > compatible = "fsl,aips-bus", "simple-bus";
> > ...
> >
> > bus@562000000 {
> > compatible = "fsl,imx8qm-display-pixel-link-msi-
> > bus", "simple-
> > pm-bus";
> > ...
> >
> > syscon@56241000 {
> > compatible = "fsl,imx8qm-lvds-csr",
> > "syscon", "simple-mfd";
> > ...
> >
> > syscon_child {};
> > };
> >
> > /* more regular mmap devices */
> > };
> > };
> >
> > IIUC, default buses listed in of_default_bus_match_table[],
> > including
> > simple-bus and simple-mfd, are populated by
> > of_platform_default_populate() in a recursive fashion, when
> > of_platform_default_populate_init() is called. However, simple-pm-
> > bus
> > is not listed in that table. So, bus@562000000 (simple-pm-bus) is
> > the
> > last one to be populated successfully and syscon@56241000 (simple-
> > mfd)
> > is not populated (recursion stops).
>
> Ok, it's working as intended so far.
>
> > Then, this patch adds a match table to populate syscon@56241000
> > (simple
> > -mfd) _and_ it's child nodes when bus@562000000 (simple-pm-bus) is
> > probed. of_platform_populate() will populate syscon@56241000
> > (simple-
> > mfd) and it's child devices (sycon_child) together. Hence,
> > sycon_child
> > devices will be probed ok.
>
> I think of_platform_default_populate() is the right solution here
> instead of spinning up a new table. Because a tree of simple-bus
> children of simple-pm-bus would have the same problem you are facing
> with simple-mfd's children not being populated.
It seems that of_platform_default_populate() is _not_ the right
solution, because simple-bus/simple-mfd devices probed by this driver,
as child nodes of simple-pm-bus, are not power managed buses, which
means simple-bus/simple-mfd's child devices PM operations cannot be
propagated to simple-pm-bus. I'm assuming that simple-bus/simple-mfd
devices probed by this driver should not be child nodes of simple-pm-
bus at all.
>
> > The problem is that syscon@56241000 (simple-mfd) fails to be probed
> > with return code -ENODEV as "ONLY_BUS" is set and "simple-mfd" is
> > the
> > 3rd compatible string.
>
> Ah, thanks for the example of your DT. My bad, I had forgotten the
> "simple-mfd" is one of the default populate busses and can be a 2nd
> or
> later entry in the compatible string and still have its children
> populated by default by OF platform code.
>
> > Even if it's probed ok, syscon@56241000 (simple-
> > mfd) is not power managed, which means syscon_child devices' PM
> > operations won't be propagated to bus@562000000 (simple-pm-bus)
> > (?).
> > Anyway, somehow, syscon_child devices do work, based on my test.
>
> Aren't you seeing this propagation issue even with your current
> patches?
I realized this propagation issue with this patch of mine when I looked
at Rob's comment - to use of_platform_default_populate() to cover
simple-bus. This propagation issue is the reason why I'm assuming
simple-bus/simple-mfd devices probed by this driver should not be child
nodes of simple-pm-bus at all.
>
> > With regard to PM, simple-bus is the same if it sits at simple-
> > mfd's
> > place. So, maybe, simple-mfd and simple-bus should be power
> > managed as
> > well? Or, simple-pm-bus should have no simple-mfd and simple-bus
> > child
> > nodes at all?
>
> The problem is that there are cases of devices with real drivers that
> also list simple-bus as their secondary compatible string. So we
> can't
> really remove any of the existing ONLY_BUS as that could cause
> simple-pm-bus driver to probe them instead of the real driver.
I don't attempt to remove any of the existing ONLY_BUS.
>
> In your case, why even list this as "fsl,imx8qm-lvds-csr"? Can't you
> just change your compatible string from:
> "fsl,imx8qm-lvds-csr", "syscon", "simple-mfd";
> To:
> "simple-pm-bus", "syscon", "simple-mfd";
fsl,imx8qxp-csr.yaml[1](already upstreamed) mentions "fsl,imx8qm-lvds-
csr" and "fsl,imx8qxp-mipi-lvds-csr" compatible string entries. It
follows the SoC name(e.g., imx8qm) + subsytem name(e.g., lvds) + IP
name(csr) fashion, which exactly tells what the IP is. Although no real
device tree is using the IP yet, I tend not to change the compatible
string (DT maintainers usually don't like the change I'm assuming).
[1] Documentation/devicetree/bindings/mfd/fsl,imx8qxp-csr.yaml
>
> You are treating it exactly as a simple-pm-bus. So I don't see what
> this extra "fsl,imx8qm-lvds-csr" distinction brings. Or make it if
> you
> really want the "fsl,imx8qm-lvds-csr" in there:
> "fsl,imx8qm-lvds-csr", "simple-pm-bus", "syscon", "simple-mfd";
If you take a look at the examples in fsl,imx8qxp-csr.yaml[1], you'll
find that a "ipg" clock is syscon@56221000's property. Drivers[2][3]
for child nodes pxl2dpi/ldb call syscon_node_to_regmap() to get regmaps
from the syscon. The problem is that syscon_node_to_regmap() attaches
the "ipg" clock to the regmaps by calling device_node_get_regmap() with
"check_clk = true". Then, the clock will be managed by both the regmap
driver and the simple-pm-bus driver, thus unreasonable clock enable
count. I know drivers[2][3] may call device_node_to_regmap() instead
with "check_clk = false", but the drivers will be changed and they
don't really know which funtion should be called.
[2] drivers/gpu/drm/bridge/imx/imx8qxp-pxl2dpi.c
[3] drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
And, like I mentioned, "fsl,imx8qm-lvds-csr" tells the exact IP ...
>
> If you are actually going to write a driver for "fsl,imx8qm-lvds-csr"
> then you need to have that driver bind to this device of yours and do
> PM management and populate the child devices if they aren't already.
... and can be used by a dedicated driver - yes, I'm going to write a
driver for "fsl,imx8qm-lvds-csr". The driver probe function just
essentially calls pm_runtime_enable() and devm_of_platform_populate(),
that's it. Leave the "ipg" clock managed by the regmap driver.
Make sense?
>
> Long story short, with what I understand so far, I think what you
> need
> to do are:
> 1. Patch to manage clock.
I'll do this, just like patch 2 does.
> 2. Patch to use of_platform_default_populate()
Nope for this one.
Based on my understanding, I won't use of_platform_default_populate()
in this driver to populate child nodes of simple-bus/simple-mfd devices
.
> 3. Fix up the compatible string to list simple-pm-bus in it.
Nope, I'm afraid I'm not willing to change the compatible string.
So, it looks like what I need to do are:
1. Drop this patch (patch 1).
2. Patch to manage clocks (patch 2).
3. Add a dedicated driver for "fsl,imx8qm-lvds-csr"/"fsl,imx8qxp-mipi-
lvds-csr".
Regards,
Liu Ying
>
> >
> > >
> > > I'm wondering if you are trying to workaround the behavior of
> > > having
> > > "ONLY_BUS" set in simple_pm_bus_of_match for "simple-mfd". Have
> > > you
> > > tried deleting that field and see if it does what you want?
> >
> > Without this patch, deleting "ONLY_BUS" works for me, as
> > syscon_child
> > devices are populated when syscon@56241000 (simple-mfd) is probed.
> > Deleting "ONLY_BUS" may make simple-mfd a power managed device. Is
> > it a
> > right thing to do?
>
> Ignore my point about deleting ONLY_BUS. That's wrong because then
> the
> simple-pm-bus driver can end up probing any device that lists
> simple-mfd even if there's another driver that could (like
> "fsl,imx8qm-lvds-csr") and we don't want that.
>
> -Saravana
>
>
>
> >
> > Regards,
> > Liu Ying
> >
> > >
> > > And we wouldn't need to use of_platform_default_populate()
> > > because
> > > this driver would take care of doing that recursively. Especially
> > > when
> > > you need the clocks and power domain to be able to access the
> > > child
> > > devices, you want the driver to probe and do that at each level
> > > before
> > > automatically recursively adding all the grand-children devices.
> > >
> > > -Saravana
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Liu Ying <[email protected]>
> > > > > ---
> > > > > v1->v3:
> > > > > * No change.
> > > > >
> > > > > drivers/bus/simple-pm-bus.c | 7 ++++++-
> > > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/bus/simple-pm-bus.c
> > > > > b/drivers/bus/simple-pm-
> > > > > bus.c
> > > > > index 6b8d6257ed8a..ff5f8ca5c024 100644
> > > > > --- a/drivers/bus/simple-pm-bus.c
> > > > > +++ b/drivers/bus/simple-pm-bus.c
> > > > > @@ -13,6 +13,11 @@
> > > > > #include <linux/platform_device.h>
> > > > > #include <linux/pm_runtime.h>
> > > > >
> > > > > +static const struct of_device_id
> > > > > simple_pm_bus_child_matches[] =
> > > > > {
> > > > > + { .compatible = "simple-mfd", },
> > > > > + {}
> > > > > +};
> > > > > +
> > > > > static int simple_pm_bus_probe(struct platform_device *pdev)
> > > > > {
> > > > > const struct device *dev = &pdev->dev;
> > > > > @@ -49,7 +54,7 @@ static int simple_pm_bus_probe(struct
> > > > > platform_device *pdev)
> > > > > pm_runtime_enable(&pdev->dev);
> > > > >
> > > > > if (np)
> > > > > - of_platform_populate(np, NULL, lookup, &pdev-
> > > > > > dev);
> > > > >
> > > > > + of_platform_populate(np,
> > > > > simple_pm_bus_child_matches, lookup, &pdev->dev);
> > > > >
> > > > > return 0;
> > > > > }
> > > > > --
> > > > > 2.25.1
> > > > >
Hi Liu,
On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <[email protected]> wrote:
> Simple Power-Managed bus controller may need functional clock(s)
> to be enabled before child devices connected to the bus can be
> accessed. Get the clock(s) as a bulk and enable/disable the
> clock(s) when the bus is being power managed.
>
> One example is that Freescale i.MX8qxp pixel link MSI bus controller
> needs MSI clock and AHB clock to be enabled before accessing child
> devices.
>
> Signed-off-by: Liu Ying <[email protected]>
Thanks for your patch!
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -8,11 +8,17 @@
> * for more details.
> */
>
> +#include <linux/clk.h>
> #include <linux/module.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
>
> +struct simple_pm_bus {
> + struct clk_bulk_data *clks;
> + int num_clks;
> +};
> +
> static const struct of_device_id simple_pm_bus_child_matches[] = {
> { .compatible = "simple-mfd", },
> {}
> @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
> struct device_node *np = dev->of_node;
> const struct of_device_id *match;
> + struct simple_pm_bus *bus;
>
> /*
> * Allow user to use driver_override to bind this driver to a
> @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus->clks);
> + if (bus->num_clks < 0)
> + return dev_err_probe(&pdev->dev, bus->num_clks, "failed to get clocks\n");
> +
> + dev_set_drvdata(&pdev->dev, bus);
> +
> dev_dbg(&pdev->dev, "%s\n", __func__);
>
> pm_runtime_enable(&pdev->dev);
While I agree this patch has merits on its own[*], I am wondering
if you really need it for your use case.
In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel
link MSI bus binding", I see your bus has both "clocks" and
"power-domains" properties. Perhaps your PM Domain can be a clock
domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing
generic_pm_domain.{at,de}tach_dev() callbacks), thus handing clock
handling over to Runtime PM?
[*] The simple-pm-bus DT bindings state:
clocks: true
# Functional clocks
# Required if power-domains is absent, optional otherwise
power-domains:
# Required if clocks is absent, optional otherwise
minItems: 1
While "power-domains" (+ "clocks" in case of a clock domain) is
handled through Runtime PM, the current driver indeed does not handle
"clocks" in the absence of the "power-domains" property. It looks
like all existing users that use "clocks" rely on the PM Domain being
a clock domain.
With your patch, the clocks might be controlled twice: once explicitly,
through clk_bulk_*(), and a second time implicitly, through Runtime PM.
While this works fine to do clock enable counters, it looks suboptimal
to me. This could be avoided by making the new explicit clock code
depend on the absence of the "power-domains" property, but that would
break users that have both a PM Domain which is not a clock domain,
and clocks. So we may have no other option.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 2022-08-11 at 14:34 +0200, Geert Uytterhoeven wrote:
> Hi Liu,
Hi Geert,
>
> On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <[email protected]> wrote:
> > Simple Power-Managed bus controller may need functional clock(s)
> > to be enabled before child devices connected to the bus can be
> > accessed. Get the clock(s) as a bulk and enable/disable the
> > clock(s) when the bus is being power managed.
> >
> > One example is that Freescale i.MX8qxp pixel link MSI bus
> > controller
> > needs MSI clock and AHB clock to be enabled before accessing child
> > devices.
> >
> > Signed-off-by: Liu Ying <[email protected]>
>
> Thanks for your patch!
Thanks for the review.
>
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -8,11 +8,17 @@
> > * for more details.
> > */
> >
> > +#include <linux/clk.h>
> > #include <linux/module.h>
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> >
> > +struct simple_pm_bus {
> > + struct clk_bulk_data *clks;
> > + int num_clks;
> > +};
> > +
> > static const struct of_device_id simple_pm_bus_child_matches[] = {
> > { .compatible = "simple-mfd", },
> > {}
> > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct
> > platform_device *pdev)
> > const struct of_dev_auxdata *lookup =
> > dev_get_platdata(dev);
> > struct device_node *np = dev->of_node;
> > const struct of_device_id *match;
> > + struct simple_pm_bus *bus;
> >
> > /*
> > * Allow user to use driver_override to bind this driver to
> > a
> > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct
> > platform_device *pdev)
> > return -ENODEV;
> > }
> >
> > + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> > + if (!bus)
> > + return -ENOMEM;
> > +
> > + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus-
> > > clks);
> >
> > + if (bus->num_clks < 0)
> > + return dev_err_probe(&pdev->dev, bus->num_clks,
> > "failed to get clocks\n");
> > +
> > + dev_set_drvdata(&pdev->dev, bus);
> > +
> > dev_dbg(&pdev->dev, "%s\n", __func__);
> >
> > pm_runtime_enable(&pdev->dev);
>
> While I agree this patch has merits on its own[*], I am wondering
> if you really need it for your use case.
>
> In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel
> link MSI bus binding", I see your bus has both "clocks" and
> "power-domains" properties. Perhaps your PM Domain can be a clock
> domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing
> generic_pm_domain.{at,de}tach_dev() callbacks), thus handing clock
> handling over to Runtime PM?
It looks like most(if not all) PM domains can be clock domains with
GENPD_FLAG_PM_CLK and generic_pm_domain.{at,de}tach_dev() callbacks
set. So, technically, my PM domain(scu-pd.c) can be a clock domain with
all those set and a special check for "simple-pm-bus" in the
{at,de}tach_dev callbacks. But, I'm not sure if it is appropriate to
do that. How do we determine clocks should be managed by PM domains or
device drivers? Technically, both would work...
>
> [*] The simple-pm-bus DT bindings state:
>
> clocks: true
> # Functional clocks
> # Required if power-domains is absent, optional otherwise
>
> power-domains:
> # Required if clocks is absent, optional otherwise
> minItems: 1
>
> While "power-domains" (+ "clocks" in case of a clock domain) is
> handled through Runtime PM, the current driver indeed does not handle
> "clocks" in the absence of the "power-domains" property. It looks
Right.
> like all existing users that use "clocks" rely on the PM Domain being
> a clock domain.
"renesas,bsc" seems to be one of the users.
>
> With your patch, the clocks might be controlled twice: once
> explicitly,
> through clk_bulk_*(), and a second time implicitly, through Runtime
> PM.
> While this works fine to do clock enable counters, it looks
> suboptimal
> to me. This could be avoided by making the new explicit clock code
> depend on the absence of the "power-domains" property, but that would
> break users that have both a PM Domain which is not a clock domain,
> and clocks. So we may have no other option.
Hmm, I'm not sure if there are really users that have both a PM domain
which is not a clock domain and clocks, given that a PM domain can sort
of always be a clock domain by setting that GENPD flag and those
callbacks. So, what do you suggest? Keep the patch as-is? Or, maybe,
make my PM domain additionally be a clock domain?
Regards,
Liu Ying
Hi Liu,
On Fri, Aug 12, 2022 at 10:13 AM Liu Ying <[email protected]> wrote:
> On Thu, 2022-08-11 at 14:34 +0200, Geert Uytterhoeven wrote:
> > On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <[email protected]> wrote:
> > > Simple Power-Managed bus controller may need functional clock(s)
> > > to be enabled before child devices connected to the bus can be
> > > accessed. Get the clock(s) as a bulk and enable/disable the
> > > clock(s) when the bus is being power managed.
> > >
> > > One example is that Freescale i.MX8qxp pixel link MSI bus
> > > controller
> > > needs MSI clock and AHB clock to be enabled before accessing child
> > > devices.
> > >
> > > Signed-off-by: Liu Ying <[email protected]>
> >
> > Thanks for your patch!
>
> Thanks for the review.
>
> >
> > > --- a/drivers/bus/simple-pm-bus.c
> > > +++ b/drivers/bus/simple-pm-bus.c
> > > @@ -8,11 +8,17 @@
> > > * for more details.
> > > */
> > >
> > > +#include <linux/clk.h>
> > > #include <linux/module.h>
> > > #include <linux/of_platform.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/pm_runtime.h>
> > >
> > > +struct simple_pm_bus {
> > > + struct clk_bulk_data *clks;
> > > + int num_clks;
> > > +};
> > > +
> > > static const struct of_device_id simple_pm_bus_child_matches[] = {
> > > { .compatible = "simple-mfd", },
> > > {}
> > > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct
> > > platform_device *pdev)
> > > const struct of_dev_auxdata *lookup =
> > > dev_get_platdata(dev);
> > > struct device_node *np = dev->of_node;
> > > const struct of_device_id *match;
> > > + struct simple_pm_bus *bus;
> > >
> > > /*
> > > * Allow user to use driver_override to bind this driver to
> > > a
> > > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct
> > > platform_device *pdev)
> > > return -ENODEV;
> > > }
> > >
> > > + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> > > + if (!bus)
> > > + return -ENOMEM;
> > > +
> > > + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus-
> > > > clks);
> > >
> > > + if (bus->num_clks < 0)
> > > + return dev_err_probe(&pdev->dev, bus->num_clks,
> > > "failed to get clocks\n");
> > > +
> > > + dev_set_drvdata(&pdev->dev, bus);
> > > +
> > > dev_dbg(&pdev->dev, "%s\n", __func__);
> > >
> > > pm_runtime_enable(&pdev->dev);
> >
> > While I agree this patch has merits on its own[*], I am wondering
> > if you really need it for your use case.
> >
> > In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel
> > link MSI bus binding", I see your bus has both "clocks" and
> > "power-domains" properties. Perhaps your PM Domain can be a clock
> > domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing
> > generic_pm_domain.{at,de}tach_dev() callbacks), thus handing clock
> > handling over to Runtime PM?
>
> It looks like most(if not all) PM domains can be clock domains with
> GENPD_FLAG_PM_CLK and generic_pm_domain.{at,de}tach_dev() callbacks
> set. So, technically, my PM domain(scu-pd.c) can be a clock domain with
> all those set and a special check for "simple-pm-bus" in the
> {at,de}tach_dev callbacks. But, I'm not sure if it is appropriate to
> do that. How do we determine clocks should be managed by PM domains or
> device drivers? Technically, both would work...
It depends on the hardware topology: is there really a clock domain
(i.e. lots of consumer modules are clocked by a single clock
controller and can be power-managed that way), or is it just a
coincidence that your bus has clocks.
E.g. drivers/clk/renesas/renesas-cpg-mssr.c:cpg_mssr_attach_dev()
looks for clocks from the right clock provider and of the right type.
> > [*] The simple-pm-bus DT bindings state:
> >
> > clocks: true
> > # Functional clocks
> > # Required if power-domains is absent, optional otherwise
> >
> > power-domains:
> > # Required if clocks is absent, optional otherwise
> > minItems: 1
> >
> > While "power-domains" (+ "clocks" in case of a clock domain) is
> > handled through Runtime PM, the current driver indeed does not handle
> > "clocks" in the absence of the "power-domains" property. It looks
>
> Right.
>
> > like all existing users that use "clocks" rely on the PM Domain being
> > a clock domain.
>
> "renesas,bsc" seems to be one of the users.
Yes it is. And it doesn't need a special driver, as it just relies
on Runtime PM controlling both the power area and the clocks through
the PM Domain.
> > With your patch, the clocks might be controlled twice: once
> > explicitly,
> > through clk_bulk_*(), and a second time implicitly, through Runtime
> > PM.
> > While this works fine to do clock enable counters, it looks
> > suboptimal
> > to me. This could be avoided by making the new explicit clock code
> > depend on the absence of the "power-domains" property, but that would
> > break users that have both a PM Domain which is not a clock domain,
> > and clocks. So we may have no other option.
>
> Hmm, I'm not sure if there are really users that have both a PM domain
> which is not a clock domain and clocks, given that a PM domain can sort
> of always be a clock domain by setting that GENPD flag and those
> callbacks. So, what do you suggest? Keep the patch as-is? Or, maybe,
> make my PM domain additionally be a clock domain?
It depends. Is "dc0_disp_ctrl_link_mst0_lpcg" a clock controller that
controls the clock inputs to multiple modules? Based on the name, it
seems to be used only for display-related clocks, while "pd" controls
power to various modules, not limited to display?
Hence you may want to keep your patch as-is.
Renesas R-Car SoCs have separate power area and clock controllers,
too, but they apply to most/all devices. Hence we moduled this as
a single PM Domain (also because "power-domains" (used to) support
only a single provider), through a close integration of the power
area and clock drivers.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Fri, 2022-08-12 at 10:58 +0200, Geert Uytterhoeven wrote:
> On Fri, Aug 12, 2022 at 10:13 AM Liu Ying <[email protected]> wrote:
> > On Thu, 2022-08-11 at 14:34 +0200, Geert Uytterhoeven wrote:
> > > On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <[email protected]>
> > > wrote:
> > > > Simple Power-Managed bus controller may need functional
> > > > clock(s)
> > > > to be enabled before child devices connected to the bus can be
> > > > accessed. Get the clock(s) as a bulk and enable/disable the
> > > > clock(s) when the bus is being power managed.
> > > >
> > > > One example is that Freescale i.MX8qxp pixel link MSI bus
> > > > controller
> > > > needs MSI clock and AHB clock to be enabled before accessing
> > > > child
> > > > devices.
> > > >
> > > > Signed-off-by: Liu Ying <[email protected]>
> > >
> > > Thanks for your patch!
> >
> > Thanks for the review.
> >
> > >
> > > > --- a/drivers/bus/simple-pm-bus.c
> > > > +++ b/drivers/bus/simple-pm-bus.c
> > > > @@ -8,11 +8,17 @@
> > > > * for more details.
> > > > */
> > > >
> > > > +#include <linux/clk.h>
> > > > #include <linux/module.h>
> > > > #include <linux/of_platform.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/pm_runtime.h>
> > > >
> > > > +struct simple_pm_bus {
> > > > + struct clk_bulk_data *clks;
> > > > + int num_clks;
> > > > +};
> > > > +
> > > > static const struct of_device_id simple_pm_bus_child_matches[]
> > > > = {
> > > > { .compatible = "simple-mfd", },
> > > > {}
> > > > @@ -24,6 +30,7 @@ static int simple_pm_bus_probe(struct
> > > > platform_device *pdev)
> > > > const struct of_dev_auxdata *lookup =
> > > > dev_get_platdata(dev);
> > > > struct device_node *np = dev->of_node;
> > > > const struct of_device_id *match;
> > > > + struct simple_pm_bus *bus;
> > > >
> > > > /*
> > > > * Allow user to use driver_override to bind this
> > > > driver to
> > > > a
> > > > @@ -49,6 +56,16 @@ static int simple_pm_bus_probe(struct
> > > > platform_device *pdev)
> > > > return -ENODEV;
> > > > }
> > > >
> > > > + bus = devm_kzalloc(&pdev->dev, sizeof(*bus),
> > > > GFP_KERNEL);
> > > > + if (!bus)
> > > > + return -ENOMEM;
> > > > +
> > > > + bus->num_clks = devm_clk_bulk_get_all(&pdev->dev, &bus-
> > > > > clks);
> > > >
> > > > + if (bus->num_clks < 0)
> > > > + return dev_err_probe(&pdev->dev, bus->num_clks,
> > > > "failed to get clocks\n");
> > > > +
> > > > + dev_set_drvdata(&pdev->dev, bus);
> > > > +
> > > > dev_dbg(&pdev->dev, "%s\n", __func__);
> > > >
> > > > pm_runtime_enable(&pdev->dev);
> > >
> > > While I agree this patch has merits on its own[*], I am wondering
> > > if you really need it for your use case.
> > >
> > > In "[PATCH v3 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel
> > > link MSI bus binding", I see your bus has both "clocks" and
> > > "power-domains" properties. Perhaps your PM Domain can be a
> > > clock
> > > domain, too (i.e. setting GENPD_FLAG_PM_CLK and providing
> > > generic_pm_domain.{at,de}tach_dev() callbacks), thus handing
> > > clock
> > > handling over to Runtime PM?
> >
> > It looks like most(if not all) PM domains can be clock domains with
> > GENPD_FLAG_PM_CLK and generic_pm_domain.{at,de}tach_dev() callbacks
> > set. So, technically, my PM domain(scu-pd.c) can be a clock domain
> > with
> > all those set and a special check for "simple-pm-bus" in the
> > {at,de}tach_dev callbacks. But, I'm not sure if it is appropriate
> > to
> > do that. How do we determine clocks should be managed by PM domains
> > or
> > device drivers? Technically, both would work...
>
> It depends on the hardware topology: is there really a clock domain
> (i.e. lots of consumer modules are clocked by a single clock
> controller and can be power-managed that way), or is it just a
> coincidence that your bus has clocks.
It's just a coincidence that my bus has the two clocks.
>
> E.g. drivers/clk/renesas/renesas-cpg-mssr.c:cpg_mssr_attach_dev()
> looks for clocks from the right clock provider and of the right type.
Ok, I see the function.
>
> > > [*] The simple-pm-bus DT bindings state:
> > >
> > > clocks: true
> > > # Functional clocks
> > > # Required if power-domains is absent, optional otherwise
> > >
> > > power-domains:
> > > # Required if clocks is absent, optional otherwise
> > > minItems: 1
> > >
> > > While "power-domains" (+ "clocks" in case of a clock domain) is
> > > handled through Runtime PM, the current driver indeed does not
> > > handle
> > > "clocks" in the absence of the "power-domains" property. It
> > > looks
> >
> > Right.
> >
> > > like all existing users that use "clocks" rely on the PM Domain
> > > being
> > > a clock domain.
> >
> > "renesas,bsc" seems to be one of the users.
>
> Yes it is. And it doesn't need a special driver, as it just relies
> on Runtime PM controlling both the power area and the clocks through
> the PM Domain.
Yes, I see.
>
> > > With your patch, the clocks might be controlled twice: once
> > > explicitly,
> > > through clk_bulk_*(), and a second time implicitly, through
> > > Runtime
> > > PM.
> > > While this works fine to do clock enable counters, it looks
> > > suboptimal
> > > to me. This could be avoided by making the new explicit clock
> > > code
> > > depend on the absence of the "power-domains" property, but that
> > > would
> > > break users that have both a PM Domain which is not a clock
> > > domain,
> > > and clocks. So we may have no other option.
> >
> > Hmm, I'm not sure if there are really users that have both a PM
> > domain
> > which is not a clock domain and clocks, given that a PM domain can
> > sort
> > of always be a clock domain by setting that GENPD flag and those
> > callbacks. So, what do you suggest? Keep the patch as-is? Or,
> > maybe,
> > make my PM domain additionally be a clock domain?
>
> It depends. Is "dc0_disp_ctrl_link_mst0_lpcg" a clock controller that
> controls the clock inputs to multiple modules? Based on the name, it
> seems to be used only for display-related clocks, while "pd" controls
> power to various modules, not limited to display?
> Hence you may want to keep your patch as-is.
"dc0_disp_ctrl_link_mst0_lpcg" only controls the clock inputs to my
bus, not multiple modules. "<&pd IMX_SC_R_DC_0>" controls power to all
modules in Display Controller Subsystem, including display controller
engines, irq controller, clock controllers and this MSI bus.
Hmm, based on your information, it looks like I need to keep the patch
as-is, as the clocks are only for the bus. If that's the case, may I
add your 'Reviewed-by' tag or sth like that on this patch that when I
send v4?
>
> Renesas R-Car SoCs have separate power area and clock controllers,
> too, but they apply to most/all devices. Hence we moduled this as
> a single PM Domain (also because "power-domains" (used to) support
> only a single provider), through a close integration of the power
> area and clock drivers.
Thanks for the information.
Liu Ying
Hi Liu,
On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <[email protected]> wrote:
> Simple Power-Managed bus controller may need functional clock(s)
> to be enabled before child devices connected to the bus can be
> accessed. Get the clock(s) as a bulk and enable/disable the
> clock(s) when the bus is being power managed.
>
> One example is that Freescale i.MX8qxp pixel link MSI bus controller
> needs MSI clock and AHB clock to be enabled before accessing child
> devices.
>
> Signed-off-by: Liu Ying <[email protected]>
> ---
> v1->v3:
> * No change.
Thanks for your patch!
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -72,6 +89,42 @@ static int simple_pm_bus_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static int simple_pm_bus_runtime_suspend(struct device *dev)
> +{
> + struct simple_pm_bus *bus = dev_get_drvdata(dev);
> +
> + if (!bus)
> + return 0;
Can this really happen?
> +
> + clk_bulk_disable_unprepare(bus->num_clks, bus->clks);
> +
> + return 0;
> +}
> +
> +static int simple_pm_bus_runtime_resume(struct device *dev)
> +{
> + struct simple_pm_bus *bus = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!bus)
> + return 0;
Likewise.
> +
> + ret = clk_bulk_prepare_enable(bus->num_clks, bus->clks);
> + if (ret) {
> + dev_err(dev, "failed to enable clocks: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Fri, 2022-08-12 at 11:49 +0200, Geert Uytterhoeven wrote:
> On Thu, Aug 4, 2022 at 8:10 AM Liu Ying <[email protected]> wrote:
> > Simple Power-Managed bus controller may need functional clock(s)
> > to be enabled before child devices connected to the bus can be
> > accessed. Get the clock(s) as a bulk and enable/disable the
> > clock(s) when the bus is being power managed.
> >
> > One example is that Freescale i.MX8qxp pixel link MSI bus
> > controller
> > needs MSI clock and AHB clock to be enabled before accessing child
> > devices.
> >
> > Signed-off-by: Liu Ying <[email protected]>
> > ---
> > v1->v3:
> > * No change.
>
> Thanks for your patch!
>
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -72,6 +89,42 @@ static int simple_pm_bus_remove(struct
> > platform_device *pdev)
> > return 0;
> > }
> >
> > +static int simple_pm_bus_runtime_suspend(struct device *dev)
> > +{
> > + struct simple_pm_bus *bus = dev_get_drvdata(dev);
> > +
> > + if (!bus)
> > + return 0;
>
> Can this really happen?
Good question. Should not happen in any cases. pm_runtime_init() sets
runtime_status to RPM_SUSPENDED and needs_force_resume to 0, so
simple_pm_bus_runtime_{suspend,resume} are only called for
simple-pm-bus, not the others. Unless I miss something, I would remove
the check here and ...
>
> > +
> > + clk_bulk_disable_unprepare(bus->num_clks, bus->clks);
> > +
> > + return 0;
> > +}
> > +
> > +static int simple_pm_bus_runtime_resume(struct device *dev)
> > +{
> > + struct simple_pm_bus *bus = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (!bus)
> > + return 0;
>
> Likewise.
... here.
>
> > +
> > + ret = clk_bulk_prepare_enable(bus->num_clks, bus->clks);
> > + if (ret) {
> > + dev_err(dev, "failed to enable clocks: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> The rest LGTM, so
> Reviewed-by: Geert Uytterhoeven <[email protected]>
Thanks,
Liu Ying