[PATCH v5 0/3] mmc: sh_mobile_sdhi: fix missing r7s72100 clocks

[PATCH v5 0/3] mmc: sh_mobile_sdhi: fix missing r7s72100 clocks

[Chris Brandt]

At first this started out as a simple typo fix, until I realized
that the SDHI in the RZ/A1 has 2 clocks per channel and both need
to be turned on/off.

This patch series adds the ability to specify 2 clocks instead of
just 1, and does so for the RZ/A1 r7s72100.

This patch has been tested on an RZ/A1 RSK board. Card detect works
fine as well as bind/unbind.

v5:
* Take implementation details out of DT documentation and put into
  the driver code.

v4:
* No code changes, only text udpates to explain why there are 2 clocks
  and why we need to treat them as 1 clock.

Chris Brandt (3):
  mmc: sh_mobile_sdhi: add support for 2 clocks
  mmc: sh_mobile_sdhi: explain clock bindings
  ARM: dts: r7s72100: update sdhi clock bindings

 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 24 ++++++++++++++++++++++
 arch/arm/boot/dts/r7s72100.dtsi                    | 17 ++++++++++-----
 drivers/mmc/host/sh_mobile_sdhi.c                  | 24 ++++++++++++++++++++++
 include/dt-bindings/clock/r7s72100-clock.h         |  6 ++++--
 4 files changed, 64 insertions(+), 7 deletions(-)

-- 
2.10.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 0/3] mmc: sh_mobile_sdhi: fix missing r7s72100 clocks

[Chris Brandt]

At first this started out as a simple typo fix, until I realized
that the SDHI in the RZ/A1 has 2 clocks per channel and both need
to be turned on/off.

This patch series adds the ability to specify 2 clocks instead of
just 1, and does so for the RZ/A1 r7s72100.

This patch has been tested on an RZ/A1 RSK board. Card detect works
fine as well as bind/unbind.

v5:
* Take implementation details out of DT documentation and put into
  the driver code.

v4:
* No code changes, only text udpates to explain why there are 2 clocks
  and why we need to treat them as 1 clock.

Chris Brandt (3):
  mmc: sh_mobile_sdhi: add support for 2 clocks
  mmc: sh_mobile_sdhi: explain clock bindings
  ARM: dts: r7s72100: update sdhi clock bindings

 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 24 ++++++++++++++++++++++
 arch/arm/boot/dts/r7s72100.dtsi                    | 17 ++++++++++-----
 drivers/mmc/host/sh_mobile_sdhi.c                  | 24 ++++++++++++++++++++++
 include/dt-bindings/clock/r7s72100-clock.h         |  6 ++++--
 4 files changed, 64 insertions(+), 7 deletions(-)

-- 
2.10.1

[PATCH v5 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

[Chris Brandt]

Some controllers have 2 clock sources instead of 1. The 2nd clock
is for the internal card detect logic and must be enabled/disabled
along with the main core clock for proper operation.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v4:
* add technical explanation within probe routine
v3:
* add more clarification to the commit log
v2:
* changed clk2 to clk_cd
* disable clk if clk_cd enable fails
* changed clock name from "carddetect" to "cd"
---
 drivers/mmc/host/sh_mobile_sdhi.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 59db14b..360d922 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -143,6 +143,7 @@ MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match);
 
 struct sh_mobile_sdhi {
 	struct clk *clk;
+	struct clk *clk_cd;
 	struct tmio_mmc_data mmc_data;
 	struct tmio_mmc_dma dma_priv;
 	struct pinctrl *pinctrl;
@@ -190,6 +191,12 @@ static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
 	if (ret < 0)
 		return ret;
 
+	ret = clk_prepare_enable(priv->clk_cd);
+	if (ret < 0) {
+		clk_disable_unprepare(priv->clk);
+		return ret;
+	}
+
 	/*
 	 * The clock driver may not know what maximum frequency
 	 * actually works, so it should be set with the max-frequency
@@ -255,6 +262,8 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
 
 	clk_disable_unprepare(priv->clk);
+	if (priv->clk_cd)
+		clk_disable_unprepare(priv->clk_cd);
 }
 
 static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc)
@@ -572,6 +581,21 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		goto eprobe;
 	}
 
+	/*
+	 * Some controllers provide a 2nd clock just to run the internal card
+	 * detection logic. Unfortunately, the existing driver architecture does
+	 * not support a separation of clocks for runtime PM usage. When
+	 * native hotplug is used, the tmio driver assumes that the core
+	 * must continue to run for card detect to stay active, so we cannot
+	 * disable it.
+	 * Additionally, it is prohibited to supply a clock to the core but not
+	 * to the card detect circuit. That leaves us with if separate clocks
+	 * are presented, we must treat them both as virtually 1 clock.
+	 */
+	priv->clk_cd = devm_clk_get(&pdev->dev, "cd");
+	if (IS_ERR(priv->clk_cd))
+		priv->clk_cd = NULL;
+
 	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
 	if (!IS_ERR(priv->pinctrl)) {
 		priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
-- 
2.10.1

[PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Chris Brandt]

In the case of a single clock source, you don't need names. However,
if the controller has 2 clock sources, you need to name them correctly
so the driver can find the 2nd one. The 2nd clock is for the internal
card detect logic.

Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v4:
* just explain there might be 2 clocks, don't explain how
  we will use them in the driver
v3:
* add more clarification about why there are sometimes 2 clocks
  and what you should do with them.
* remove 'status = "disabled"' from example
v2:
* fix spelling and change wording
* changed clock name from "carddetect" to "cd"
---
 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index a1650ed..1464c16 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -25,8 +25,32 @@ Required properties:
 		"renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
 		"renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
 
+- clocks: Most controllers only have 1 clock source per channel. However, on
+	  some variations of this controller, the internal card detection
+	  logic that exists in this controller is sectioned off to be run by a
+	  separate second clock source to allow the main core clock to be turned
+	  off to save power.
+	  If 2 clocks are specified by the hardware, you must name them as
+	  "core" and "cd".
+	  If the controller only has 1 clock, naming is not required.
+
 Optional properties:
 - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable
 - pinctrl-names: should be "default", "state_uhs"
 - pinctrl-0: should contain default/high speed pin ctrl
 - pinctrl-1: should contain uhs mode pin ctrl
+
+Example showing 2 clocks:
+	sdhi0: sd@e804e000 {
+		compatible = "renesas,sdhi-r7s72100";
+		reg = <0xe804e000 0x100>;
+		interrupts = <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
+
+		clocks = <&mstp12_clks R7S72100_CLK_SDHI00>,
+			 <&mstp12_clks R7S72100_CLK_SDHI01>;
+		clock-names = "core", "cd";
+		cap-sd-highspeed;
+		cap-sdio-irq;
+	};
-- 
2.10.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Chris Brandt]

In the case of a single clock source, you don't need names. However,
if the controller has 2 clock sources, you need to name them correctly
so the driver can find the 2nd one. The 2nd clock is for the internal
card detect logic.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v4:
* just explain there might be 2 clocks, don't explain how
  we will use them in the driver
v3:
* add more clarification about why there are sometimes 2 clocks
  and what you should do with them.
* remove 'status = "disabled"' from example
v2:
* fix spelling and change wording
* changed clock name from "carddetect" to "cd"
---
 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index a1650ed..1464c16 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -25,8 +25,32 @@ Required properties:
 		"renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
 		"renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
 
+- clocks: Most controllers only have 1 clock source per channel. However, on
+	  some variations of this controller, the internal card detection
+	  logic that exists in this controller is sectioned off to be run by a
+	  separate second clock source to allow the main core clock to be turned
+	  off to save power.
+	  If 2 clocks are specified by the hardware, you must name them as
+	  "core" and "cd".
+	  If the controller only has 1 clock, naming is not required.
+
 Optional properties:
 - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable
 - pinctrl-names: should be "default", "state_uhs"
 - pinctrl-0: should contain default/high speed pin ctrl
 - pinctrl-1: should contain uhs mode pin ctrl
+
+Example showing 2 clocks:
+	sdhi0: sd@e804e000 {
+		compatible = "renesas,sdhi-r7s72100";
+		reg = <0xe804e000 0x100>;
+		interrupts = <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
+
+		clocks = <&mstp12_clks R7S72100_CLK_SDHI00>,
+			 <&mstp12_clks R7S72100_CLK_SDHI01>;
+		clock-names = "core", "cd";
+		cap-sd-highspeed;
+		cap-sdio-irq;
+	};
-- 
2.10.1

[PATCH v5 3/3] ARM: dts: r7s72100: update sdhi clock bindings

[Chris Brandt]

The SDHI controller in the RZ/A1 has 2 clock sources per channel and both
need to be enabled/disabled for proper operation. This fixes the fact that
the define for R7S72100_CLK_SDHI1 was not correct to begin with (typo), and
that all 4 clock sources need to be defined an used.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
* add missing clock sources instead of just fixing typo
* changed clock name from "carddetect" to "cd"
---
 arch/arm/boot/dts/r7s72100.dtsi            | 17 ++++++++++++-----
 include/dt-bindings/clock/r7s72100-clock.h |  6 ++++--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index 3dd427d..9d0b8d0 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -153,9 +153,12 @@
 			#clock-cells = <1>;
 			compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
 			reg = <0xfcfe0444 4>;
-			clocks = <&p1_clk>, <&p1_clk>;
-			clock-indices = <R7S72100_CLK_SDHI1 R7S72100_CLK_SDHI0>;
-			clock-output-names = "sdhi1", "sdhi0";
+			clocks = <&p1_clk>, <&p1_clk>, <&p1_clk>, <&p1_clk>;
+			clock-indices = <
+				R7S72100_CLK_SDHI00 R7S72100_CLK_SDHI01
+				R7S72100_CLK_SDHI10 R7S72100_CLK_SDHI11
+			>;
+			clock-output-names = "sdhi00", "sdhi01", "sdhi10", "sdhi11";
 		};
 	};
 
@@ -478,7 +481,9 @@
 			      GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH
 			      GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
 
-		clocks = <&mstp12_clks R7S72100_CLK_SDHI0>;
+		clocks = <&mstp12_clks R7S72100_CLK_SDHI00>,
+			 <&mstp12_clks R7S72100_CLK_SDHI01>;
+		clock-names = "core", "cd";
 		cap-sd-highspeed;
 		cap-sdio-irq;
 		status = "disabled";
@@ -491,7 +496,9 @@
 			      GIC_SPI 274 IRQ_TYPE_LEVEL_HIGH
 			      GIC_SPI 275 IRQ_TYPE_LEVEL_HIGH>;
 
-		clocks = <&mstp12_clks R7S72100_CLK_SDHI1>;
+		clocks = <&mstp12_clks R7S72100_CLK_SDHI10>,
+			 <&mstp12_clks R7S72100_CLK_SDHI11>;
+		clock-names = "core", "cd";
 		cap-sd-highspeed;
 		cap-sdio-irq;
 		status = "disabled";
diff --git a/include/dt-bindings/clock/r7s72100-clock.h b/include/dt-bindings/clock/r7s72100-clock.h
index 29e01ed..f2d8428 100644
--- a/include/dt-bindings/clock/r7s72100-clock.h
+++ b/include/dt-bindings/clock/r7s72100-clock.h
@@ -45,7 +45,9 @@
 #define R7S72100_CLK_SPI4	3
 
 /* MSTP12 */
-#define R7S72100_CLK_SDHI0	3
-#define R7S72100_CLK_SDHI1	2
+#define R7S72100_CLK_SDHI00	3
+#define R7S72100_CLK_SDHI01	2
+#define R7S72100_CLK_SDHI10	1
+#define R7S72100_CLK_SDHI11	0
 
 #endif /* __DT_BINDINGS_CLOCK_R7S72100_H__ */
-- 
2.10.1

Re: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

On Fri, Jan 20, 2017 at 10:06:03PM -0500, Chris Brandt wrote:
> In the case of a single clock source, you don't need names. However,
> if the controller has 2 clock sources, you need to name them correctly
> so the driver can find the 2nd one. The 2nd clock is for the internal
> card detect logic.
> 
> Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>

Reviewed-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On Fri, Jan 20, 2017 at 10:06:03PM -0500, Chris Brandt wrote:
> In the case of a single clock source, you don't need names. However,
> if the controller has 2 clock sources, you need to name them correctly
> so the driver can find the 2nd one. The 2nd clock is for the internal
> card detect logic.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Fri, Jan 20, 2017 at 10:06:02PM -0500, Chris Brandt wrote:
> Some controllers have 2 clock sources instead of 1. The 2nd clock
> is for the internal card detect logic and must be enabled/disabled
> along with the main core clock for proper operation.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Geert Uytterhoeven]

On Sat, Jan 21, 2017 at 4:06 AM, Chris Brandt <chris.brandt@renesas.com> wrote:
> In the case of a single clock source, you don't need names. However,
> if the controller has 2 clock sources, you need to name them correctly
> so the driver can find the 2nd one. The 2nd clock is for the internal
> card detect logic.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v5 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

[Geert Uytterhoeven]

Hi Chris,

On Sat, Jan 21, 2017 at 4:06 AM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Some controllers have 2 clock sources instead of 1. The 2nd clock
> is for the internal card detect logic and must be enabled/disabled
> along with the main core clock for proper operation.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v4:
> * add technical explanation within probe routine
> v3:
> * add more clarification to the commit log
> v2:
> * changed clk2 to clk_cd
> * disable clk if clk_cd enable fails
> * changed clock name from "carddetect" to "cd"

Thanks for the updates!

> ---
>  drivers/mmc/host/sh_mobile_sdhi.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 59db14b..360d922 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -143,6 +143,7 @@ MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match);
>
>  struct sh_mobile_sdhi {
>         struct clk *clk;
> +       struct clk *clk_cd;
>         struct tmio_mmc_data mmc_data;
>         struct tmio_mmc_dma dma_priv;
>         struct pinctrl *pinctrl;
> @@ -190,6 +191,12 @@ static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
>         if (ret < 0)
>                 return ret;
>
> +       ret = clk_prepare_enable(priv->clk_cd);
> +       if (ret < 0) {
> +               clk_disable_unprepare(priv->clk);
> +               return ret;
> +       }
> +

As enabling the "core" clock but not the "cd" clock is not a valid setting,
shouldn't the cd clock be enabled first?

>         /*
>          * The clock driver may not know what maximum frequency
>          * actually works, so it should be set with the max-frequency
> @@ -255,6 +262,8 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
>         struct sh_mobile_sdhi *priv = host_to_priv(host);
>
>         clk_disable_unprepare(priv->clk);
> +       if (priv->clk_cd)

No need to check for a NULL pointer first.

> +               clk_disable_unprepare(priv->clk_cd);

Disabling is already done in the correct order ;-)

>  }

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

RE: [PATCH v5 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

[Chris Brandt]

Hi Geert,

On Monday, January 23, 2017, Geert Uytterhoeven:
> > @@ -190,6 +191,12 @@ static int sh_mobile_sdhi_clk_enable(struct
> tmio_mmc_host *host)
> >         if (ret < 0)
> >                 return ret;
> >
> > +       ret = clk_prepare_enable(priv->clk_cd);
> > +       if (ret < 0) {
> > +               clk_disable_unprepare(priv->clk);
> > +               return ret;
> > +       }
> > +
> 
> As enabling the "core" clock but not the "cd" clock is not a valid setting,
> shouldn't the cd clock be enabled first?

I'm pretty sure the chip designers just didn't want you to 'use' the
peripheral with that clock configuration, and we are not using it in
between those 2 lines so I personally think it's fine.

If I need to change it to get it approved, let me know and I'll change
the code around.

(In reality, I checked and the SDHI does work with the cd clock off,
but I don't know if the restriction in the spec was because they never
fully tested the use of it that way because it wasn't a design goal for
them.)



> >         /*
> >          * The clock driver may not know what maximum frequency
> >          * actually works, so it should be set with the max-frequency
> > @@ -255,6 +262,8 @@ static void sh_mobile_sdhi_clk_disable(struct
> tmio_mmc_host *host)
> >         struct sh_mobile_sdhi *priv = host_to_priv(host);
> >
> >         clk_disable_unprepare(priv->clk);
> > +       if (priv->clk_cd)
> 
> No need to check for a NULL pointer first.

OK, I see that IS_ERR_OR_NULL(clk) is done for clk_disable_unprepare so
I'll take the check out.
Thank you.


> > +               clk_disable_unprepare(priv->clk_cd);
> 
> Disabling is already done in the correct order ;-)

I guess you could get into some theoretical technical discussion around
if it is OK to enable/disable the clocks in any order (as long as you
don't use the SDHI in between), then you would want to enable the cd
circuit last, and disable it first so you would ensure that no cd
interrupts are being registered when the core clock is off?
In that case, the current enable order should remain but the disable
order should change.

Opinions???


Thank you,
Chris

Re: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Rob Herring]

On Fri, Jan 20, 2017 at 10:06:03PM -0500, Chris Brandt wrote:
> In the case of a single clock source, you don't need names. However,
> if the controller has 2 clock sources, you need to name them correctly
> so the driver can find the 2nd one. The 2nd clock is for the internal
> card detect logic.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v4:
> * just explain there might be 2 clocks, don't explain how
>   we will use them in the driver
> v3:
> * add more clarification about why there are sometimes 2 clocks
>   and what you should do with them.
> * remove 'status = "disabled"' from example
> v2:
> * fix spelling and change wording
> * changed clock name from "carddetect" to "cd"
> ---
>  Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> index a1650ed..1464c16 100644
> --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> @@ -25,8 +25,32 @@ Required properties:
>  		"renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
>  		"renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
>  
> +- clocks: Most controllers only have 1 clock source per channel. However, on
> +	  some variations of this controller, the internal card detection

This should be explicit as to which compatible strings have 2 clocks and 
which have 1 clock.

> +	  logic that exists in this controller is sectioned off to be run by a
> +	  separate second clock source to allow the main core clock to be turned
> +	  off to save power.
> +	  If 2 clocks are specified by the hardware, you must name them as
> +	  "core" and "cd".
> +	  If the controller only has 1 clock, naming is not required.
> +
>  Optional properties:
>  - toshiba,mmc-wrprotect-disable: write-protect detection is unavailable
>  - pinctrl-names: should be "default", "state_uhs"
>  - pinctrl-0: should contain default/high speed pin ctrl
>  - pinctrl-1: should contain uhs mode pin ctrl
> +
> +Example showing 2 clocks:
> +	sdhi0: sd@e804e000 {

mmc@...

> +		compatible = "renesas,sdhi-r7s72100";
> +		reg = <0xe804e000 0x100>;
> +		interrupts = <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH
> +			      GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH
> +			      GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
> +
> +		clocks = <&mstp12_clks R7S72100_CLK_SDHI00>,
> +			 <&mstp12_clks R7S72100_CLK_SDHI01>;
> +		clock-names = "core", "cd";
> +		cap-sd-highspeed;
> +		cap-sdio-irq;
> +	};
> -- 
> 2.10.1
> 
> 

RE: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Chris Brandt]

Hello Rob,


On Monday, January 23, 2017, Rob Herring wrote:
> > --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> > +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> > @@ -25,8 +25,32 @@ Required properties:
> >  		"renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
> >  		"renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
> >
> > +- clocks: Most controllers only have 1 clock source per channel.
> However, on
> > +	  some variations of this controller, the internal card detection
> 
> This should be explicit as to which compatible strings have 2 clocks and
> which have 1 clock.

OK. I'll make a list like I did for sh_mmcif:

https://patchwork.kernel.org/patch/9512091/



> > +
> > +Example showing 2 clocks:
> > +	sdhi0: sd@e804e000 {
> 
> mmc@...

I'm confused. I see that for all SDHI controllers, it either "sd@" or "sdhci@".

$ grep sdhi $(find arch/arm/boot/dts -name "*.dtsi")

$ grep sdhci $(find arch/arm/boot/dts -name "*.dtsi")


Regards,
Chris

Re: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Rob Herring]

On Mon, Jan 23, 2017 at 11:56 AM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> Hello Rob,
>
>
> On Monday, January 23, 2017, Rob Herring wrote:
>> > --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
>> > +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
>> > @@ -25,8 +25,32 @@ Required properties:
>> >             "renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
>> >             "renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
>> >
>> > +- clocks: Most controllers only have 1 clock source per channel.
>> However, on
>> > +     some variations of this controller, the internal card detection
>>
>> This should be explicit as to which compatible strings have 2 clocks and
>> which have 1 clock.
>
> OK. I'll make a list like I did for sh_mmcif:
>
> https://patchwork.kernel.org/patch/9512091/
>
>
>
>> > +
>> > +Example showing 2 clocks:
>> > +   sdhi0: sd@e804e000 {
>>
>> mmc@...
>
> I'm confused. I see that for all SDHI controllers, it either "sd@" or "sdhci@".
>
> $ grep sdhi $(find arch/arm/boot/dts -name "*.dtsi")
>
> $ grep sdhci $(find arch/arm/boot/dts -name "*.dtsi")

Yes, there's lots of variation. Node names are supposed to be generic
for their class of device (e.g. ethernet, pci, usb,
interrupt-controller, etc.). I'd be fine with "sd", but think "mmc" is
more common. Either way, we should pick one moving forward. "sdhci"
should not be used as that's a specific implementation.

Rob

RE: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Chris Brandt]

On Thursday, January 26, 2017, Rob Herring wrote:
> >> > +
> >> > +Example showing 2 clocks:
> >> > +   sdhi0: sd@e804e000 {
> >>
> >> mmc@...
> >
> > I'm confused. I see that for all SDHI controllers, it either "sd@" or
> "sdhci@".
> >
> > $ grep sdhi $(find arch/arm/boot/dts -name "*.dtsi")
> >
> > $ grep sdhci $(find arch/arm/boot/dts -name "*.dtsi")
> 
> Yes, there's lots of variation. Node names are supposed to be generic for
> their class of device (e.g. ethernet, pci, usb, interrupt-controller,
> etc.). I'd be fine with "sd", but think "mmc" is more common. Either way,
> we should pick one moving forward. "sdhci"
> should not be used as that's a specific implementation.
> 
> Rob

For now, I took the example back out of tmio_mmc.txt since I figured
people can refer back to mmc.txt for what the naming should be (since
the directory name is mmc, you would think the golden rules were in
mmc.txt)


> I'd be fine with "sd", but think "mmc" is more common.

NOTE: mmc.txt currently has 2 examples:

	sdhci@ab000000 {

	mmc3: mmc@01c12000 {

Chris

Re: [PATCH v5 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Ulf Hansson]

On 26 January 2017 at 15:39, Rob Herring <robh@kernel.org> wrote:
> On Mon, Jan 23, 2017 at 11:56 AM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
>> Hello Rob,
>>
>>
>> On Monday, January 23, 2017, Rob Herring wrote:
>>> > --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
>>> > +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
>>> > @@ -25,8 +25,32 @@ Required properties:
>>> >             "renesas,sdhi-r8a7795" - SDHI IP on R8A7795 SoC
>>> >             "renesas,sdhi-r8a7796" - SDHI IP on R8A7796 SoC
>>> >
>>> > +- clocks: Most controllers only have 1 clock source per channel.
>>> However, on
>>> > +     some variations of this controller, the internal card detection
>>>
>>> This should be explicit as to which compatible strings have 2 clocks and
>>> which have 1 clock.
>>
>> OK. I'll make a list like I did for sh_mmcif:
>>
>> https://patchwork.kernel.org/patch/9512091/
>>
>>
>>
>>> > +
>>> > +Example showing 2 clocks:
>>> > +   sdhi0: sd@e804e000 {
>>>
>>> mmc@...
>>
>> I'm confused. I see that for all SDHI controllers, it either "sd@" or "sdhci@".
>>
>> $ grep sdhi $(find arch/arm/boot/dts -name "*.dtsi")
>>
>> $ grep sdhci $(find arch/arm/boot/dts -name "*.dtsi")
>
> Yes, there's lots of variation. Node names are supposed to be generic
> for their class of device (e.g. ethernet, pci, usb,
> interrupt-controller, etc.). I'd be fine with "sd", but think "mmc" is
> more common. Either way, we should pick one moving forward. "sdhci"
> should not be used as that's a specific implementation.

I think we discussed this earlier. The problem with these devices is
that it covers several type of devices, so it's hard to find a generic
node name.

The devices are: MMC, eMMC, SD, SDIO. I don't have strong opinions on
whether to chose a generic node name, and perhaps "mmc" is the best we
can get?

Whatever we decide, we should update all mmc DTS docs, to avoid future
confusion.

Kind regards
Uffe