diff mbox series

[2/6] ARM: dts: r7s72100: Add SPIBSC clocks

Message ID 20191203034519.5640-3-chris.brandt@renesas.com (mailing list archive)
State Not Applicable, archived
Headers show
Series spi: Add Renesas SPIBSC controller | expand

Commit Message

Chris Brandt Dec. 3, 2019, 3:45 a.m. UTC
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(-)

Comments

Geert Uytterhoeven Dec. 3, 2019, 6:42 p.m. UTC | #1
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
Chris Brandt Dec. 3, 2019, 6:57 p.m. UTC | #2
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
Geert Uytterhoeven Dec. 3, 2019, 7:12 p.m. UTC | #3
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
Lee Jones Dec. 4, 2019, 8:38 a.m. UTC | #4
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
>
Geert Uytterhoeven Dec. 4, 2019, 9:03 a.m. UTC | #5
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
Lee Jones Dec. 4, 2019, 9:47 a.m. UTC | #6
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
>
Chris Brandt Dec. 4, 2019, 11 a.m. UTC | #7
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 mbox series

Patch

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 {