RE: [PATCH V6 03/12] clk: imx: scu: add two cells binding support | linux-arm-kernel

(off-list ancestor, not in this archive)

From: Stephen Boyd <sboyd@kernel.org>
Sent: Friday, June 19, 2020 12:05 AM

Quoting Aisheng Dong (2020-06-01 07:28:57)
From: Stephen Boyd <sboyd@kernel.org>
Sent: Wednesday, May 27, 2020 4:34 PM

Quoting Aisheng Dong (2020-05-05 06:47:02)
Hi Stephen,

Thanks for the review.

From: Stephen Boyd <sboyd@kernel.org>
Sent: Tuesday, May 5, 2020 1:08 PM

Quoting Dong Aisheng (2020-03-15 06:43:47)
This patch implements the new two cells binding for SCU clocks.
The usage is as follows:
clocks = <&uart0_clk IMX_SC_R_UART_0 IMX_SC_PM_CLK_PER>

Due to each SCU clock is associated with a power domain,
without power on the domain, the SCU clock can't work. So we
create platform devices for each domain clock respectively and
manually attach the required domain before register the clock
devices, then we can register clocks in the clock platform driver
accordingly.
That's odd. See below.

Note because we do not have power domain info in device tree
and the SCU resource ID is the same for power domain and
clock, so we use resource ID to find power domains.

Later, we will also use this clock platform driver to support
suspend/resume and runtime pm.

Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Michael Turquette <mturquette@baylibre.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
[...]
diff --git a/drivers/clk/imx/clk-scu.c
b/drivers/clk/imx/clk-scu.c index b8b2072742a5..4fadff14d8b2
100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c

+               return -EINVAL;
+
+       if (clk_cells == 2) {
+               for (i = 0; i < IMX_SC_R_LAST; i++)
+                       INIT_LIST_HEAD(&imx_scu_clks[i]);
+
+               pd_np = of_find_compatible_node(NULL, NULL,
"fsl,scu-pd");
+               pd_dev = of_find_device_by_node(pd_np);
+               if (!pd_dev || !device_is_bound(&pd_dev->dev))
+ {
What is device_is_bound() check for? Add a comment?
Yes, I can add a comment in the code.
It is because SCU clock driver depends on SCU power domain to be ready
first.
Does EPROBE_DEFER not kick in automatically in that case?
No, there're no power domains under scu clock node in dts.
So can't use PROBE_DEFER automatically.
Why isn't the scu clock node using power domains?
We have tried this in several ways before but finally didn't go that way
due to various reasons.

e.g.
Method 1:
conn-scu-clock-controller {
        compatible = "fsl,imx8qxp-clk", "fsl,scu-clk";
        #address-cells = <1>;
        #size-cells = <0>;

        uart0_clk: clock-scu@57 {
        reg = <57>; 
                #clock-cells = <1>;
                clock-indices = <IMX_SC_PM_CLK_PER>;
                clock-output-names = "uart0_clk";
                power-domains = <&pd IMX_SC_R_UART_0>;
        };
        ...
}

Method 2:
#define IMX_SCU_CLK_ID(rsrc, type)      (type << 16 | rsrc)
scu_clk: scu-clock-controller {
        compatible = "fsl,imx8qxp-scu-clk", "fsl,scu-clk";
        #clock-cells = <1>;
        clock-indices = <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_PER)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_BYPASS)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_ENET_0, IMX_SC_PM_CLK_MISC0)>,
                        <IMX_SCU_CLK_ID(IMX_SC_R_UART_0, IMX_SC_PM_CLK_PER)>,
                        ...

        clock-output-names = "enet0_clk",
                             "enet0_bypass_clk",
                             "enet0_rgmii_clk",
                             "uart0_clk",
                             ...

        power-domains = <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_ENET_0>,
                        <&pd IMX_SC_R_UART_0>,
                        ...
};

See details here:
https://spinics.net/lists/devicetree/msg298642.html

+
+       pdev->driver_override = "imx-scu-clk";
+
+       ret = imx_clk_scu_attach_pd(&pdev->dev, rsrc_id);
Why do we have to allocate a device for each power domain?
This is mainly for each clock runtime pm and suspend/resume
support as they're independent with each other.

Is this because we
don't have support for one device being in multiple power domains?
That is supported now as far as I recall, by basically making
dummy platform devices like this.
I know kernel supports multi power domains, but I didn't realize
it could be used for our case.

So maybe this code isn't necessary and we can have one platform
device for the clk controller and then have it control certain
power domains manually from runtime PM callbacks? It's possible
the runtime PM callbacks are too simple for this case and we
need to tell clk providers what clk is having runtime PM enabled for it.
To make sure I understand correctly, do you mean we use only one
general clk controller
Typically I see HW designers make one hardware block for a certain
group of clks, like general purpose IO hardware, multimedia hardware, DSP
hardware, etc.
That isn't always true though. Sometimes I see the hardware
designers decide to stamp down the same hard macro for a clk in
every IP block they develop in the SoC. It gets interesting when
they have to integrate with a third party core like dwc3. Usually
they look to see if they can add some vendor specific register to the IP block
or make a wrapper around the IP block with the SoC glue bits.
This usually happens when the clk hard macro library is used by all
the hardware designers. Instead of consolidating that library into a
hardware IP block that gets delivered for the particular SoC they
just spread the hardware block around the system and hope that software
will figure it out.
I suspect hardware blocks are given certain addresses on the MMIO
bus because ARM's AMBA/AHB spec mandated that a "device" starts at a 1K
aligned address.
This probably forced SoC hardware engineers to split their hardware
blocks up into different pieces that they could then plug into the
bus and easily route CPU addresses to different slaves on the bus.

What's the situation here? I thought that this was a firmware
interface that logically combines all the hardware block clk
controllers. The firmware interface doesn't do everything though
because it seems we still have to power on power domains to use various
clks?
Yes, it is.
For MX8QM/QXP, the whole SoC is comprised of a few HW Subsystems.
Those HW Subsystems are independent with each other as they have
separate power/clock Controllers.
Within each subsystem, there's a DSC (distributed slave controller)
acting as a standard interface to System Controller (SCU) to provide
power/clock/reset and some other misc control functions.

If it's a few then there are maybe 5 SCUs in the SoC?
There's only one SCU in the SoC running on a dedicated M4 core.
SCU communicated with each Subsystem via standard DSC interface
to provide centralized clock/power/reset/pinctrl/resource management/misc services.

Runtime pm callback to handle all clocks runtime pm status manually?
If doing that, how do we handle different clocks pm requirements
with only one device runtime pm status (clock controller)?
e.g.
One Clock Provider
Consumer A -> Clock A -> Clock Provider resumed -> Clock A resumed
Consumer B -> Clock B (Since Clock Provided is already resumed, no
chance to
run callback to resume Clock B now).
(Note: assume all clocks need runtime pm enabled for i.MX case)

Or you mean we simply resume all clocks? but that seems lose the
granularity and possibly have no chance to enter runtime suspend
anymore
once there was one clock in use.
Not sure if I missed something. Please help clarify a bit more.

Right now, I'm a bit afraid this may make things a bit complicated
as we have ~150 clocks and ~150 power domains. Putting them all
under one
clock controller node in DT may scare people.
And even we did not create platform devices for each clock in the
clock driver, using multi-pd will still result in creating dummy
platform devices for each clock automatically by power domain
framework.
That means we didn't save any platform devices.

What software entity is providing the power domains? I wonder if
things are going about all wrong and the power domains are provided
by the same piece of code that's managing the clks?
There's a separate power domain driver based on SCU protocol for only power
control.
This power control is a lit complicated as it supports multi low power states.
When power is completely gated, the HW state will be lost and need restore
by SW.
Some of those features are still not upstreamed.

If so, why can't we control the power domains directly from the clk
code without having to go through runtime PM layer or genpd?
It's probably hard to do that because:
1. power domain supports multiple low power states for different purpose.
Now genpd seems a better place to handle it 2. Not all power domains
associated with a clock.

Or to flip it the other way, why do we need clk control for
enable/disable in that case?
Can we just expose rate control for the clks and then let genpd
handle turning clks on an off if they're associated with that power
domain?
I wonder this way might also lose the granularity control capability.
For a simple device with only one clock, we might be able to do it at most
times.
But for some complicated devices with multiple clocks and which clocks
to use depends on the real user cases, simply enable all clocks when PD is UP
seems not very suitable.
For MX8, it's a complicated systems and some device resources may have
multi clocks, especially for Audio, Video IO, Display related cases.
Do they have multiple clks and multiple power domains for one device, like
Audio or Video? Is it 1:1 between some number of clks and some power
domain?
For simple devices with only one clock, it might be 1:1.
But for complicated devices with multiple clocks, it's usually not 1:1.
e.g.
AUDIO_PLL_0     PLL     User programmable PLL
AUDIO_PLL_0     MISC0   Audio PLL div 0
AUDIO_PLL_0     MISC1   Audio rec 0

DC_0    MISC0   Display 0
DC_0    MISC1   Display 1

MIPI_0  BYPASS  Bypass
MIPI_0  SLV_BUS DSI rx escape
MIPI_0  MST_BUS DSI tx escape
MIPI_0  PHY     DPHY PLL ref
MIPI_0  PER     DPI - pixel

Maybe we can adjust the core clk framework to introduce a
callback for the clk that is runtime PM enabling and put the
logic there about what to
do?
That may help. Since we still only have one device for runtime pm
state management, Still not understand how to do it as it may mix
the usage
with the runtime pm framework.
Right. I'm thinking that we may need a clk_op that is called when a
clk is runtime PM enabled so the device driver that provides that
clk can decide what genpds to manually enable. This would only be
used in the case where the clks are provided by a clk controller
that exists in multiple power domains. I'm not super clear on
multi-pd and how it interacts with runtime PM but I assume that it
doesn't actually turn on any genpd when the consumer device is runtime
PM active. Instead the consumer driver has to figure out what to power on by
itself.
The callback would do this similarly from the CCF so that the manual
steps could be done again.
After more thinking, I guess we probably don't need a clk_op if we
register the correct struct device (abstract clk provider) to clk register API. E.g.
clk_hw_register(dev, hw).
That abstract struct dev could be the virtual power domain device
created and returned by PD framework when calling multi PD APIs. This
can make us be able to fully reuse all the current rumtime PM support
in CCF. Nothing else special need to care. (synchronization work with
rpm/genpd framework)
Whatever 'dev' is used to register the clk is presumed to be the device that is the
clk provider. If 'dev' isn't the one that's matched to the device node in DT the
CCF will become confused about what parent clks are supposed to be. That's
one problem but it may not be a problem here because this is just one big DT
node?

See: multi pd API will create a virtual PD device to be associated with that PD
for later RPM usages.
struct device *dev_pm_domain_attach_by_name(struct device *dev,
                                            const char *name) {
        if (dev->pm_domain)
                return ERR_PTR(-EEXIST);

        return genpd_dev_pm_attach_by_name(dev, name); }
EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_name);

However, I'm still a bit feeling this probably could work for a clock controller
simply with <10 PDs.
But not very suitable for MX8QM/QXP as it has 150+ PDs. Defining them
all will make DT looks terrible and drivers slightly a bit complicated
to fetch those PDs efficiently from DT for each clock, also make the future
maintain work a bit harder when adding new clocks.
Not sure whether it's worth to do that as compared with current
approach, it really does not Improve too much as multi pd also need create
virtual devices if the virtual devices are the only concern.
In contrast, it might increase, probably unnecessary complexities.
Either way there are going to be 150+ devices for the 150+ power domains in
the system.  Why are there so many power domains? That seems like the
problem. I doubt there are 150 devices in the SoC, or even 75 of them.
It's abstract power domain concept introduced by SCU firmware that each device
Is associated with a power domain.
In real HW, many devices within the same Subsystem might share one of the same HW
power domain. But SCU firmware hides those internal details completely to users.
So users are unaware of it.

Additionally, power domain are also associated with device resource management by
xRDC (Resource Domain Controller) component for security reasons.

Without enable the resource power by SCU firmware, one SW execution environment 
(e.g. OS/ATF/OPTEE/VM) can't access those resource like registers, clocks and etc.

How would you suggest?
Do you think if we should continue to investigate this Or if we can
make the function work first as this improvement work does not affect users?
Currently as this patch is a very fundamental change which blocks a lot other
drivers upstream work.
If you want to go ahead with this patch I guess I will stop caring.
Thanks a lot

Just be aware
that needing to make virtual devices to fit this into the kernel looks wrong. I'd
like to see one device node for the firmware interface of clks, and one struct
device for said firmware interface that attaches to one struct driver for that
firmware interface.
So far, I'm still no sure if we can finally archive that due to complexities of
various requirements in MX8. Personally i feel a bit risky and trying it definitely need
lot more time to ensure it can meet all of our feature requirements on MX8.
But I did understand your concern. Anyway, I will continue to investigate.
However, if you do not mind too much, I'd treat it as a continued improvement work
in order to not block the whole MX8 upstream work too long.
(dt binding part of users won't change)

The current approach has been verified working well in NXP official released 5.4 kernel and
can meet various product requirements. Probably could be a good start point for the reset 
drivers upstreaming work first.

Connecting that to genpd for proper power domain management is another
issue. If the core clk framework needs to be multi-device power domain aware
so that various clks can turn on along with the power domain that's needed to
operate them then we'll need to add that in the core framework.
Yes. I guess the problem for MX8 is how to co-work with the exist power domain driver.
For MX8, there're also more issues because MX8 power domain supports multiple lower power
States. And we need a genpd governer to help choose a state. That's the future work we may do
In the next step.
If we power up clock power in CCF, we need think how to do It well by co-exist with genpd framework.

I'm amazed that the firmware interface doesn't handle the requirement that
genpds need to be powered on to control the clks. I'd think the firmware would
be happy to turn on power domains so that it can go poke clk registers.
I guess one reason may be SCU support multiple low power states.
It wants user to choose which one to enter.
/*
 * Defines for SC PM Power Mode
 */
#define IMX_SC_PM_PW_MODE_OFF   0       /* Power off */
#define IMX_SC_PM_PW_MODE_STBY  1       /* Power in standby */
#define IMX_SC_PM_PW_MODE_LP    2       /* Power in low-power */
#define IMX_SC_PM_PW_MODE_ON    3       /* Power on */

Regards
Aisheng
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help