Message ID | 20210817012754.8710-1-digetx@gmail.com |
---|---|
Headers | show |
Series | NVIDIA Tegra power management patches for 5.16 | expand |
Hi Dmitry, Dmitry Osipenko <digetx@gmail.com> wrote on Tue, 17 Aug 2021 04:27:41 +0300: > The NAND on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now NAND must be resumed using > runtime PM API in order to initialize the NAND power state. Add runtime PM > and OPP support to the NAND driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/mtd/nand/raw/tegra_nand.c | 62 +++++++++++++++++++++++++++---- > 1 file changed, 54 insertions(+), 8 deletions(-) > Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
On Tue, 17 Aug 2021 04:27:44 +0300, Dmitry Osipenko wrote: > Document new OPP table and power domain properties of the video decoder > hardware. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > .../devicetree/bindings/media/nvidia,tegra-vde.yaml | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
18.08.2021 04:15, Rob Herring пишет: >> + tegra-clocks: >> + description: child nodes are the output clocks from the CAR >> + type: object >> + >> + patternProperties: >> + "^[a-z]+[0-9]+$": >> + type: object >> + properties: >> + compatible: >> + allOf: >> + - items: >> + - enum: >> + - nvidia,tegra20-sclk >> + - nvidia,tegra30-sclk >> + - nvidia,tegra30-pllc >> + - nvidia,tegra30-plle >> + - nvidia,tegra30-pllm >> + - const: nvidia,tegra-clock > You are saying the first string must be both one of the enums and > 'nvidia,tegra-clock'. You don't get an error because your pattern > doesn't match 'sclk'. > Could you please rephrase or clarify? If pattern doesn't match 'sclk', then it must match any other enum. I'm not sure what you're meaning. The 'nvidia,tegra-clock' actually could be removed since it's superfluous now. I'll consider the removal in v9.
On Wed, Aug 18, 2021 at 04:44:30AM +0300, Dmitry Osipenko wrote: > 18.08.2021 04:15, Rob Herring пишет: > >> + tegra-clocks: > >> + description: child nodes are the output clocks from the CAR > >> + type: object > >> + > >> + patternProperties: > >> + "^[a-z]+[0-9]+$": > >> + type: object > >> + properties: > >> + compatible: > >> + allOf: > >> + - items: > >> + - enum: > >> + - nvidia,tegra20-sclk > >> + - nvidia,tegra30-sclk > >> + - nvidia,tegra30-pllc > >> + - nvidia,tegra30-plle > >> + - nvidia,tegra30-pllm > >> + - const: nvidia,tegra-clock > > You are saying the first string must be both one of the enums and > > 'nvidia,tegra-clock'. You don't get an error because your pattern > > doesn't match 'sclk'. > > > > Could you please rephrase or clarify? If pattern doesn't match 'sclk', > then it must match any other enum. I'm not sure what you're meaning. "sclk" doesn't match "^[a-z]+[0-9]+$" because it's missing at least one digit at the end. Perhaps that last + was supposed to be *? > > The 'nvidia,tegra-clock' actually could be removed since it's > superfluous now. I'll consider the removal in v9. It also looks like your schema was meant to be something like: compatible: - items: - enum: - nvidia,tegra20-sclk - nvidia,tegra30-sclk - nvidia,tegra30-pllc - nvidia,tegra30-plle - nvidia,tegra30-pllm - const: nvidia,tegra-clock Note how the const: element is indented one more level. Now this means: one of the enumeration values, followed by the constant value. That matches what the example has. That said, I agree that nvidia,tegra-clock seems a bit useless. There's really no such thing as a generic clock, they're all different in some way. Thierry
18.08.2021 16:59, Thierry Reding пишет: > On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote: >> Document tegra-clocks sub-node which describes Tegra SoC clocks that >> require a higher voltage of the core power domain in order to operate >> properly on a higher clock rates. Each node contains a phandle to OPP >> table and power domain. >> >> The root PLLs and system clocks don't have any specific device dedicated >> to them, clock controller is in charge of managing power for them. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> .../bindings/clock/nvidia,tegra20-car.yaml | 51 +++++++++++++++++++ >> 1 file changed, 51 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml >> index 459d2a525393..7f5cd27e4ce0 100644 >> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml >> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml >> @@ -42,6 +42,48 @@ properties: >> "#reset-cells": >> const: 1 >> >> + tegra-clocks: >> + description: child nodes are the output clocks from the CAR >> + type: object >> + >> + patternProperties: >> + "^[a-z]+[0-9]+$": >> + type: object >> + properties: >> + compatible: >> + allOf: >> + - items: >> + - enum: >> + - nvidia,tegra20-sclk >> + - nvidia,tegra30-sclk >> + - nvidia,tegra30-pllc >> + - nvidia,tegra30-plle >> + - nvidia,tegra30-pllm >> + - const: nvidia,tegra-clock >> + >> + operating-points-v2: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + Phandle to OPP table that contains frequencies, voltages and >> + opp-supported-hw property, which is a bitfield indicating >> + SoC process or speedo ID mask. >> + >> + clocks: >> + items: >> + - description: node's clock >> + >> + power-domains: >> + maxItems: 1 >> + description: phandle to the core SoC power domain >> + >> + required: >> + - compatible >> + - operating-points-v2 >> + - clocks >> + - power-domains >> + >> + additionalProperties: false >> + >> required: >> - compatible >> - reg >> @@ -59,6 +101,15 @@ examples: >> reg = <0x60006000 0x1000>; >> #clock-cells = <1>; >> #reset-cells = <1>; >> + >> + tegra-clocks { >> + sclk { >> + compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock"; >> + operating-points-v2 = <&opp_table>; >> + clocks = <&tegra_car TEGRA20_CLK_SCLK>; >> + power-domains = <&domain>; >> + }; >> + }; > > I wonder if it'd be better to match on the name of the node rather than > add an artificial compatible string. We usually use the compatible > string to match a device, but here you're really trying to add > information about a resource provided by the CAR controller. > > We do similar things for example in PMIC bindings where the individual > regulators are represented in the device tree via nodes named after the > regulator. > > You could then also leave out the clocks property, which is weird as it > is because it's basically a self-reference. But you don't really need > the reference here in the first place because the CAR is already the > parent of SCLK. We don't have a platform device for CaR. I don't see how it's going to work. We need to create a platform device for each RPM-capable clock because that's how RPM works. The compatible string is required for instantiating OF-devices from a node, otherwise we will have to re-invent the OF core. > Also, I don't think the tegra- prefix is necessary here. The parent node > is already identified as Tegra via the compatible string. > > In the case of CAR, I'd imagine something like: > > clocks { > sclk { > operating-points-v2 = <&opp_table>; > power-domains = <&domain>; > }; > }; > > Now you've only got the bare minimum in here that you actually add. All > the other data that you used to have is simply derived from the parent. 'clocks' is already a generic keyword in DT. It's probably not okay to redefine it.
18.08.2021 19:39, Thierry Reding пишет: >> We don't have a platform device for CaR. I don't see how it's going to >> work. We need to create a platform device for each RPM-capable clock >> because that's how RPM works. The compatible string is required for >> instantiating OF-devices from a node, otherwise we will have to >> re-invent the OF core. > I think we do have a platform device for CAR. It's just not bound > against by the driver because these clock drivers are "special". But > from other parts of the series you're already trying to fix that, at > least partially. > > But it doesn't seem right to create a platform device for each RPM- > capable clock. Why do they need to be devices? They aren't, so why > pretend? Is it that some API that we want to use here requires the > struct device? The "device" representation is internal to the kernel. It's okay to me to have PLLs represented by a device, it's a distinct h/w by itself. CCF supports managing of clock's RPM and it requires to have clock to be backed by a device. That's what we are using here. Please see https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109 >>> Also, I don't think the tegra- prefix is necessary here. The parent node >>> is already identified as Tegra via the compatible string. >>> >>> In the case of CAR, I'd imagine something like: >>> >>> clocks { >>> sclk { >>> operating-points-v2 = <&opp_table>; >>> power-domains = <&domain>; >>> }; >>> }; >>> >>> Now you've only got the bare minimum in here that you actually add. All >>> the other data that you used to have is simply derived from the parent. >> 'clocks' is already a generic keyword in DT. It's probably not okay to >> redefine it. > "clocks" is not a generic keyword. It's the name of a property and given > that we're talking about the clock provider here, it doesn't need a > clocks property of its own, so it should be fine to use that for the > node. I'm curious what Rob thinks about it. Rob, does this sound okay to you?
18.08.2021 19:57, Dmitry Osipenko пишет: >>>> Also, I don't think the tegra- prefix is necessary here. The parent node >>>> is already identified as Tegra via the compatible string. >>>> >>>> In the case of CAR, I'd imagine something like: >>>> >>>> clocks { >>>> sclk { >>>> operating-points-v2 = <&opp_table>; >>>> power-domains = <&domain>; >>>> }; >>>> }; >>>> >>>> Now you've only got the bare minimum in here that you actually add. All >>>> the other data that you used to have is simply derived from the parent. >>> 'clocks' is already a generic keyword in DT. It's probably not okay to >>> redefine it. >> "clocks" is not a generic keyword. It's the name of a property and given >> that we're talking about the clock provider here, it doesn't need a >> clocks property of its own, so it should be fine to use that for the >> node. > I'm curious what Rob thinks about it. Rob, does this sound okay to you? I assume dt-schema won't be happy with a different meaning for the 'clocks'.
On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > The PWM on Tegra belongs to the core power domain and we're going to > enable GENPD support for the core domain. Now PWM must be resumed using > runtime PM API in order to initialize the PWM power state. The PWM clock > rate must be changed using OPP API that will reconfigure the power domain > performance state in accordance to the rate. Add runtime PM and OPP > support to the PWM driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/pwm/pwm-tegra.c | 104 ++++++++++++++++++++++++++++++++-------- > 1 file changed, 85 insertions(+), 19 deletions(-) Can this be safely applied independently of the rest of the series, or are there any dependencies on earlier patches? Thierry
On Thu, 19 Aug 2021 at 15:21, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > > The PWM on Tegra belongs to the core power domain and we're going to > > enable GENPD support for the core domain. Now PWM must be resumed using > > runtime PM API in order to initialize the PWM power state. The PWM clock > > rate must be changed using OPP API that will reconfigure the power domain > > performance state in accordance to the rate. Add runtime PM and OPP > > support to the PWM driver. > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > drivers/pwm/pwm-tegra.c | 104 ++++++++++++++++++++++++++++++++-------- > > 1 file changed, 85 insertions(+), 19 deletions(-) > > Can this be safely applied independently of the rest of the series, or > are there any dependencies on earlier patches? Just to make sure we don't rush something in, I would rather withhold all runtime PM related patches in the series, until we have agreed on how to fix the in genpd/opp core parts. Simply, because those may very well affect the deployments in the drivers. > > Thierry Kind regards Uffe
On Thu, Aug 19, 2021 at 04:04:50PM +0200, Ulf Hansson wrote: > On Thu, 19 Aug 2021 at 15:21, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Tue, Aug 17, 2021 at 04:27:39AM +0300, Dmitry Osipenko wrote: > > > The PWM on Tegra belongs to the core power domain and we're going to > > > enable GENPD support for the core domain. Now PWM must be resumed using > > > runtime PM API in order to initialize the PWM power state. The PWM clock > > > rate must be changed using OPP API that will reconfigure the power domain > > > performance state in accordance to the rate. Add runtime PM and OPP > > > support to the PWM driver. > > > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > > > --- > > > drivers/pwm/pwm-tegra.c | 104 ++++++++++++++++++++++++++++++++-------- > > > 1 file changed, 85 insertions(+), 19 deletions(-) > > > > Can this be safely applied independently of the rest of the series, or > > are there any dependencies on earlier patches? > > Just to make sure we don't rush something in, I would rather withhold > all runtime PM related patches in the series, until we have agreed on > how to fix the in genpd/opp core parts. Simply, because those may very > well affect the deployments in the drivers. Okay, understood. I didn't realize this may have an impact on how drivers need to cooperate. I'll hold off on applying any of these patches until the discussion has settled, then. Thierry
19.08.2021 19:31, Thierry Reding пишет: >>>>> Also, I don't think the tegra- prefix is necessary here. The parent node >>>>> is already identified as Tegra via the compatible string. >>>>> >>>>> In the case of CAR, I'd imagine something like: >>>>> >>>>> clocks { >>>>> sclk { >>>>> operating-points-v2 = <&opp_table>; >>>>> power-domains = <&domain>; >>>>> }; >>>>> }; >>>>> >>>>> Now you've only got the bare minimum in here that you actually add. All >>>>> the other data that you used to have is simply derived from the parent. >>>> 'clocks' is already a generic keyword in DT. It's probably not okay to >>>> redefine it. >>> "clocks" is not a generic keyword. It's the name of a property and given >>> that we're talking about the clock provider here, it doesn't need a >>> clocks property of its own, so it should be fine to use that for the >>> node. >> I'm curious what Rob thinks about it. Rob, does this sound okay to you? > Another alternative would be to omit that level altogether and just make > sclk and siblings direct children of the CAR node. That can be done.
19.08.2021 19:31, Thierry Reding пишет: >> The "device" representation is internal to the kernel. It's okay to me >> to have PLLs represented by a device, it's a distinct h/w by itself. >> >> CCF supports managing of clock's RPM and it requires to have clock to be >> backed by a device. That's what we are using here. >> >> Please see >> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109 > Looking at the implementation of __clk_register() and where that device > pointer typically comes from, I don't think the way this is used here is > what was intended. The way I interpret the code is that a clock is > registered with a parent device (i.e. its provider) and > clk_pm_runtime_get() is then used internally as a way to make sure that > when a clock is prepared, it's parent device is runtime resumed. This is > presumably to ensure that any registers that the driver might need to > access in order to prepare and enable the clock are accessible (i.e. the > CAR is not powered off or in reset). > > So the struct device that is passed to __clk_register() (or its callers) > should be that of the CAR rather than virtual struct devices created by > the CAR. > > And it's a bit debatable whether or not PLLs represent distinct > hardware. Ultimately every transistor on a chip could be considered > distinct hardware. But a platform device is a device on a platform bus, > which is really just another way of saying it's a hardware block that's > accessible from the CPU via a memory-mapped address. A PLL (just like > other clocks) is merely a resource exposed by means of access to these > registers. So I don't think they should be platform devices. Even making > them struct device:s seems a bit of a stretch. > > Is there any reason why struct clk can't be used for this? I mean, the > whole purpose of that structure is to represent clocks. Why do we need > to make them special? Because we need to perform DVFS for PLLs. The only way to do it without having to reinvent existing frameworks is to use these frameworks and they require a device.
On Fri, Aug 20, 2021 at 01:37:13AM +0300, Dmitry Osipenko wrote: > 19.08.2021 20:03, Thierry Reding пишет: > > On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote: > >> The SDHCI on Tegra belongs to the core power domain and we're going to > >> enable GENPD support for the core domain. Now SDHCI must be resumed using > >> runtime PM API in order to initialize the SDHCI power state. The SDHCI > >> clock rate must be changed using OPP API that will reconfigure the power > >> domain performance state in accordance to the rate. Add runtime PM and OPP > >> support to the SDHCI driver. > >> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- > >> 1 file changed, 105 insertions(+), 41 deletions(-) > >> > >> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > >> index 387ce9cdbd7c..a3583359c972 100644 > >> --- a/drivers/mmc/host/sdhci-tegra.c > >> +++ b/drivers/mmc/host/sdhci-tegra.c > >> @@ -15,6 +15,8 @@ > >> #include <linux/of.h> > >> #include <linux/of_device.h> > >> #include <linux/pinctrl/consumer.h> > >> +#include <linux/pm_opp.h> > >> +#include <linux/pm_runtime.h> > >> #include <linux/regulator/consumer.h> > >> #include <linux/reset.h> > >> #include <linux/mmc/card.h> > >> @@ -24,6 +26,8 @@ > >> #include <linux/gpio/consumer.h> > >> #include <linux/ktime.h> > >> > >> +#include <soc/tegra/common.h> > >> + > >> #include "sdhci-pltfm.h" > >> #include "cqhci.h" > >> > >> @@ -123,6 +127,12 @@ > >> SDHCI_TRNS_BLK_CNT_EN | \ > >> SDHCI_TRNS_DMA) > >> > >> +enum { > >> + TEGRA_CLK_BULK_SDHCI, > >> + TEGRA_CLK_BULK_TMCLK, > >> + TEGRA_CLK_BULK_NUM, > >> +}; > >> + > >> struct sdhci_tegra_soc_data { > >> const struct sdhci_pltfm_data *pdata; > >> u64 dma_mask; > >> @@ -171,6 +181,8 @@ struct sdhci_tegra { > >> bool enable_hwcq; > >> unsigned long curr_clk_rate; > >> u8 tuned_tap_delay; > >> + > >> + struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM]; > > > > This doesn't seem worth it to me. There's a lot of churn in this driver > > that's only needed to convert this to the clk_bulk API and it makes the > > code a lot more difficult to read, in my opinion. > > > > It looks like the only benefit that this gives us is that runtime > > suspend and resume become a few lines shorter. > > The driver probe code looks cleaner with that. You should be looking at > the final result and not at the patch to see it. I did look at the final result and didn't find it cleaner at all. =) Thierry