| Message ID | 20191203034519.5640-3-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | spi: Add Renesas SPIBSC controller | expand |
Hi Chris, On Tue, Dec 3, 2019 at 4:46 AM Chris Brandt <chris.brandt@renesas.com> wrote: > The SPIBSC-0 clock is marked as critical because for XIP systems, this > is the SPI flash controller it will boot from and the kernel will also > be running from so it cannot be turned off. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Thanks for your patch! > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -101,6 +101,26 @@ > #size-cells = <1>; > ranges; > > + spibsc0: spi@3fefa000 { > + compatible = "renesas,r7s72100-spibsc", "renesas,spibsc"; > + reg = <0x3fefa000 0x100>, <0x18000000 0x4000000>; The second region conflicts with the XIP flash@18000000 in arch/arm/boot/dts/r7s72100-gr-peach.dts. Yes, I know it is the same device ;-) > + clocks = <&mstp9_clks R7S72100_CLK_SPIBSC0>; > + power-domains = <&cpg_clocks>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > + > + spibsc1: spi@3fefb000 { > + compatible = "renesas,r7s72100-spibsc", "renesas,spibsc"; > + reg = <0x3fefb000 0x100>, <0x1c000000 0x4000000>; > + clocks = <&mstp9_clks R7S72100_CLK_SPIBSC1>; > + power-domains = <&cpg_clocks>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > + > L2: cache-controller@3ffff000 { > compatible = "arm,pl310-cache"; > reg = <0x3ffff000 0x1000>; > @@ -467,11 +487,13 @@ > #clock-cells = <1>; > compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks"; > reg = <0xfcfe0438 4>; > - clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>; > + clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>, <&b_clk>, <&b_clk>; > clock-indices = < > R7S72100_CLK_I2C0 R7S72100_CLK_I2C1 R7S72100_CLK_I2C2 R7S72100_CLK_I2C3 > + R7S72100_CLK_SPIBSC0 R7S72100_CLK_SPIBSC1 > >; > - clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3"; > + clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "spibsc0", "spibsc1"; > + clock-critical = <4>; /* spibsc0 */ Iff we go this clock-critical route, I think this should be specified in the board-specific .dts instead of in the SoC-specific .dtsi. Gr{oetje,eeting}s, Geert
Hi Geert, On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > + reg = <0x3fefa000 0x100>, <0x18000000 > > + 0x4000000>; > > The second region conflicts with the XIP flash@18000000 in > arch/arm/boot/dts/r7s72100-gr-peach.dts. > Yes, I know it is the same device ;-) Is that just an FYI?? Or do you have some suggestion?? > > + clock-critical = <4>; /* spibsc0 */ > > Iff we go this clock-critical route, I think this should be specified in the > board-specific .dts instead of in the SoC-specific .dtsi. OK, that's fine. It makes more sense to be in the .dts because it's a board design decision. I will remove it from the patch. The one 'tricky' thing is that the <4> is the index into the number of clocks. So in the Renesas BSP where it includes the VDC5 LCD controllers, clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; the <4> needs to become a <6>. Chris
Hi Chris, CC Lee (for clock-critical) On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > + 0x4000000>; > > > > The second region conflicts with the XIP flash@18000000 in > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > Yes, I know it is the same device ;-) > > Is that just an FYI?? Or do you have some suggestion?? Can the flash subnode be compatible with "mtd-rom", even if the parent node is kept disabled? > > > + clock-critical = <4>; /* spibsc0 */ > > > > Iff we go this clock-critical route, I think this should be specified in the > > board-specific .dts instead of in the SoC-specific .dtsi. > > OK, that's fine. It makes more sense to be in the .dts because it's a board > design decision. I will remove it from the patch. > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > the <4> needs to become a <6>. Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? Unfortunately the exact semantics of clock-critical were never documented. Lee? Thanks! [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device tree support" https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 Tue, 03 Dec 2019, Geert Uytterhoeven wrote: > Hi Chris, > > CC Lee (for clock-critical) > > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > > + 0x4000000>; > > > > > > The second region conflicts with the XIP flash@18000000 in > > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > > Yes, I know it is the same device ;-) > > > > Is that just an FYI?? Or do you have some suggestion?? > > Can the flash subnode be compatible with "mtd-rom", even if the parent node > is kept disabled? > > > > > + clock-critical = <4>; /* spibsc0 */ > > > > > > Iff we go this clock-critical route, I think this should be specified in the > > > board-specific .dts instead of in the SoC-specific .dtsi. > > > > OK, that's fine. It makes more sense to be in the .dts because it's a board > > design decision. I will remove it from the patch. > > > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > > > the <4> needs to become a <6>. > > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? > > Unfortunately the exact semantics of clock-critical were never documented. > Lee? /** * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree * @np: Device node pointer associated with clock provider * @index: clock index * @flags: pointer to top-level framework flags * * Detects if the clock-critical property exists and, if so, sets the * corresponding CLK_IS_CRITICAL flag. * * Do not use this function. It exists only for legacy Device Tree * bindings, such as the one-clock-per-node style that are outdated. * Those bindings typically put all clock data into .dts and the Linux * driver has no clock data, thus making it impossible to set this flag * correctly from the driver. Only those drivers may call * of_clk_detect_critical from their setup functions. * * Return: error code or zero on success */ If this meets the criteria, the API is pretty simple/self explanatory. What are you having trouble with? > Thanks! > > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device > tree support" > https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ > > > Gr{oetje,eeting}s, > > Geert >
Hi Lee, On Wed, Dec 4, 2019 at 9:38 AM Lee Jones <lee.jones@linaro.org> wrote: > On Tue, 03 Dec 2019, Geert Uytterhoeven wrote: > > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > > > + 0x4000000>; > > > > > > > > The second region conflicts with the XIP flash@18000000 in > > > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > > > Yes, I know it is the same device ;-) > > > > > > Is that just an FYI?? Or do you have some suggestion?? > > > > Can the flash subnode be compatible with "mtd-rom", even if the parent node > > is kept disabled? > > > > > > > + clock-critical = <4>; /* spibsc0 */ > > > > > > > > Iff we go this clock-critical route, I think this should be specified in the > > > > board-specific .dts instead of in the SoC-specific .dtsi. > > > > > > OK, that's fine. It makes more sense to be in the .dts because it's a board > > > design decision. I will remove it from the patch. > > > > > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > > > > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > > > > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > > > > > the <4> needs to become a <6>. > > > > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], > > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? > > > > Unfortunately the exact semantics of clock-critical were never documented. > > Lee? > > /** > * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree > * @np: Device node pointer associated with clock provider > * @index: clock index > * @flags: pointer to top-level framework flags > * > * Detects if the clock-critical property exists and, if so, sets the > * corresponding CLK_IS_CRITICAL flag. > * > * Do not use this function. It exists only for legacy Device Tree > * bindings, such as the one-clock-per-node style that are outdated. > * Those bindings typically put all clock data into .dts and the Linux > * driver has no clock data, thus making it impossible to set this flag > * correctly from the driver. Only those drivers may call > * of_clk_detect_critical from their setup functions. > * > * Return: error code or zero on success > */ > > If this meets the criteria, the API is pretty simple/self > explanatory. What are you having trouble with? What exactly is the "index" parameter? This value is matched against the values of the "clock-critical" (array) property, but it is described nowhere in the DT bindings. stih407-clock.dtsi seems to assume this value is an index into the clock-output-names array? Can we use it as a value of "clock-indices" instead? > > Thanks! > > > > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device > > tree support" > > https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ Gr{oetje,eeting}s, Geert
On Wed, 04 Dec 2019, Geert Uytterhoeven wrote: > Hi Lee, > > On Wed, Dec 4, 2019 at 9:38 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Tue, 03 Dec 2019, Geert Uytterhoeven wrote: > > > On Tue, Dec 3, 2019 at 7:58 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > > > > On Tue, Dec 3, 2019, Geert Uytterhoeven wrote: > > > > > > + reg = <0x3fefa000 0x100>, <0x18000000 > > > > > > + 0x4000000>; > > > > > > > > > > The second region conflicts with the XIP flash@18000000 in > > > > > arch/arm/boot/dts/r7s72100-gr-peach.dts. > > > > > Yes, I know it is the same device ;-) > > > > > > > > Is that just an FYI?? Or do you have some suggestion?? > > > > > > Can the flash subnode be compatible with "mtd-rom", even if the parent node > > > is kept disabled? > > > > > > > > > + clock-critical = <4>; /* spibsc0 */ > > > > > > > > > > Iff we go this clock-critical route, I think this should be specified in the > > > > > board-specific .dts instead of in the SoC-specific .dtsi. > > > > > > > > OK, that's fine. It makes more sense to be in the .dts because it's a board > > > > design decision. I will remove it from the patch. > > > > > > > > The one 'tricky' thing is that the <4> is the index into the number of clocks. > > > > > > > > So in the Renesas BSP where it includes the VDC5 LCD controllers, > > > > > > > > clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "vdc50", "vdc51", "spibsc0", "spibsc1"; > > > > > > > > the <4> needs to become a <6>. > > > > > > Unless you pass "clkidx" instead of "i" to of_clk_detect_critical() in [1], > > > and use "clock-critical = <R7S72100_CLK_SPIBSC0>" in DT? > > > > > > Unfortunately the exact semantics of clock-critical were never documented. > > > Lee? > > > > /** > > * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree > > * @np: Device node pointer associated with clock provider > > * @index: clock index > > * @flags: pointer to top-level framework flags > > * > > * Detects if the clock-critical property exists and, if so, sets the > > * corresponding CLK_IS_CRITICAL flag. > > * > > * Do not use this function. It exists only for legacy Device Tree > > * bindings, such as the one-clock-per-node style that are outdated. > > * Those bindings typically put all clock data into .dts and the Linux > > * driver has no clock data, thus making it impossible to set this flag > > * correctly from the driver. Only those drivers may call > > * of_clk_detect_critical from their setup functions. > > * > > * Return: error code or zero on success > > */ > > > > If this meets the criteria, the API is pretty simple/self > > explanatory. What are you having trouble with? > > What exactly is the "index" parameter? This value is matched against > the values of the "clock-critical" (array) property, but it is described > nowhere in the DT bindings. > stih407-clock.dtsi seems to assume this value is an index into the > clock-output-names array? > Can we use it as a value of "clock-indices" instead? of_clk_detect_critical(), the consumer of the device tree property 'clock-critical', is a helper. Neither deliver any auto-magical functionality. Simply providing an index into the property will not do anything useful. It's up to the vendor's clock driver to handle. The vendor driver can call of_clk_detect_critical() with *any* index it sees fit. If it matches a number listed in the 'clock-critical' array, the CLK_IS_CRITICAL flag is set in the flags pointed to by the 3rd function parameter. Take a look at some of the call sites in drivers/clk/st/* for further clarification. > > > Thanks! > > > > > > [1] "[PATCH 1/6] clk: renesas: mstp: Add critical clock from device > > > tree support" > > > https://lore.kernel.org/linux-renesas-soc/20191203034519.5640-2-chris.brandt@renesas.com/ > > Gr{oetje,eeting}s, > > Geert >
Hi Lee, Thank you. Hi Geert, On Wed, Dec 4, 2019, Lee Jones wrote: > The vendor driver can call of_clk_detect_critical() with *any* index it sees > fit. If it matches a number listed in the 'clock-critical' > array, the CLK_IS_CRITICAL flag is set in the flags pointed to by the 3rd > function parameter. Since this is the case, I'll change the driver code so we can use it how we prefer: ie, clock-critical = <R7S72100_CLK_SPIBSC0>; Chris
diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi index d03dcd919d6f..a422bbe872bc 100644 --- a/arch/arm/boot/dts/r7s72100.dtsi +++ b/arch/arm/boot/dts/r7s72100.dtsi @@ -101,6 +101,26 @@ #size-cells = <1>; ranges; + spibsc0: spi@3fefa000 { + compatible = "renesas,r7s72100-spibsc", "renesas,spibsc"; + reg = <0x3fefa000 0x100>, <0x18000000 0x4000000>; + clocks = <&mstp9_clks R7S72100_CLK_SPIBSC0>; + power-domains = <&cpg_clocks>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; + + spibsc1: spi@3fefb000 { + compatible = "renesas,r7s72100-spibsc", "renesas,spibsc"; + reg = <0x3fefb000 0x100>, <0x1c000000 0x4000000>; + clocks = <&mstp9_clks R7S72100_CLK_SPIBSC1>; + power-domains = <&cpg_clocks>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; + L2: cache-controller@3ffff000 { compatible = "arm,pl310-cache"; reg = <0x3ffff000 0x1000>; @@ -467,11 +487,13 @@ #clock-cells = <1>; compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks"; reg = <0xfcfe0438 4>; - clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>; + clocks = <&p0_clk>, <&p0_clk>, <&p0_clk>, <&p0_clk>, <&b_clk>, <&b_clk>; clock-indices = < R7S72100_CLK_I2C0 R7S72100_CLK_I2C1 R7S72100_CLK_I2C2 R7S72100_CLK_I2C3 + R7S72100_CLK_SPIBSC0 R7S72100_CLK_SPIBSC1 >; - clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3"; + clock-output-names = "i2c0", "i2c1", "i2c2", "i2c3", "spibsc0", "spibsc1"; + clock-critical = <4>; /* spibsc0 */ }; mstp10_clks: mstp10_clks@fcfe043c {
The SPIBSC-0 clock is marked as critical because for XIP systems, this is the SPI flash controller it will boot from and the kernel will also be running from so it cannot be turned off. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- arch/arm/boot/dts/r7s72100.dtsi | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)