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

[PATCH v3 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.


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 | 21 +++++++++++++++++++++
 arch/arm/boot/dts/r7s72100.dtsi                    | 17 ++++++++++++-----
 drivers/mmc/host/sh_mobile_sdhi.c                  | 13 +++++++++++++
 include/dt-bindings/clock/r7s72100-clock.h         |  6 ++++--
 4 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.10.1

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

[Chris Brandt]

Some controllers have 2 clock sources instead of 1, so they both need
to be turned on/off.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 59db14b..a3f995e 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,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		goto eprobe;
 	}
 
+	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 v3 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.

Signed-off-by: Chris Brandt <chris.brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v2:
* fix spelling and change wording
* changed clock name from "carddetect" to "cd"
---
 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index a1650ed..90370cd 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -25,8 +25,29 @@ 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, some
+	  have a second clock dedicated to card detection. If 2 clocks are
+	  specified, 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;
+		status = "disabled";
+	};
-- 
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 v3 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.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
* fix spelling and change wording
* changed clock name from "carddetect" to "cd"
---
 Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index a1650ed..90370cd 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -25,8 +25,29 @@ 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, some
+	  have a second clock dedicated to card detection. If 2 clocks are
+	  specified, 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;
+		status = "disabled";
+	};
-- 
2.10.1

[PATCH v3 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 v3 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

[Wolfram Sang]

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

On Wed, Jan 18, 2017 at 12:25:00PM -0500, Chris Brandt wrote:
> Some controllers have 2 clock sources instead of 1, so they both need
> to be turned on/off.
> 
> 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 v3 2/3] mmc: sh_mobile_sdhi: explain clock bindings

[Wolfram Sang]

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

On Wed, Jan 18, 2017 at 12:25:01PM -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.
> 
> 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 v3 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

[Ulf Hansson]

On 18 January 2017 at 18:25, Chris Brandt <chris.brandt@renesas.com> wrote:
> Some controllers have 2 clock sources instead of 1, so they both need
> to be turned on/off.

This doesn't tell me enough. Please elaborate.

For example, tell how you treat the clocks, which of them that is
optional and why.

>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> 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 | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 59db14b..a3f995e 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,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
>                 goto eprobe;
>         }
>
> +       priv->clk_cd = devm_clk_get(&pdev->dev, "cd");
> +       if (IS_ERR(priv->clk_cd))
> +               priv->clk_cd = NULL;

Is this clock solely about card detection? So in cases when you have a
GPIO card detect, the clock isn't needed?

Just trying to understand things a bit better...

> +
>         priv->pinctrl = devm_pinctrl_get(&pdev->dev);
>         if (!IS_ERR(priv->pinctrl)) {
>                 priv->pins_default = pinctrl_lookup_state(priv->pinctrl,
> --
> 2.10.1
>
>

Kind regards
Uffe

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

[Ulf Hansson]

On 18 January 2017 at 18:25, 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.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v2:
> * fix spelling and change wording
> * changed clock name from "carddetect" to "cd"
> ---
>  Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> index a1650ed..90370cd 100644
> --- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
> @@ -25,8 +25,29 @@ 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, some
> +         have a second clock dedicated to card detection. If 2 clocks are
> +         specified, you must name them as "core" and "cd". If the controller
> +         only has 1 clock, naming is not required.

Could you please elaborate a bit on the card detection clock?

I guess that there is some kind of internal card detection logic
(native card detect) in the SDHI IP, which requires a separate clock
for it to work? Perhaps you can state that somehow?

> +
>  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;
> +               status = "disabled";

The last line seems a bit odd to include in an example.

> +       };
> --
> 2.10.1
>
>

Kind regards
Uffe

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

[Chris Brandt]

Hello Ulf,


On Friday, January 20, 2017, Ulf Hansson wrote:
> On 18 January 2017 at 18:25, Chris Brandt <chris.brandt@renesas.com>
> wrote:
> > Some controllers have 2 clock sources instead of 1, so they both need
> > to be turned on/off.
> 
> This doesn't tell me enough. Please elaborate.
> 
> For example, tell how you treat the clocks, which of them that is optional
> and why.

Basically, the chip designers went in and broke out the logic that just does
the card detect (more or less it's probably just some simple IRQ logic) so
that you could shut off the IP block, but if you put in a card, the internal
interrupt signal still went to the interrupt controller and registered the
interrupt even though the rest of the IP block (including the register space)
was dead.

The idea seems to be that you could put the entire chip into low power mode
and wake it up if you stuck a card in.

My guess is that this was for some customer request or something.

Personally...you could have done the same thing if you laid out a board
and tied one of the extra IRQ lines to the cd signal...but I'm not sure if
anyone thought about that at the time.


So to your request, I could put all this ugly info into the documentation,
but basically all I'm trying to do is join the 2 clocks back together
to make it work like all the other SoCs since this new 'feature' might
not really be practical to every use in this environment.



> >  static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc) @@ -572,6
> > +581,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
> >                 goto eprobe;
> >         }
> >
> > +       priv->clk_cd = devm_clk_get(&pdev->dev, "cd");
> > +       if (IS_ERR(priv->clk_cd))
> > +               priv->clk_cd = NULL;
> 
> Is this clock solely about card detection? So in cases when you have a
> GPIO card detect, the clock isn't needed?
> 
> Just trying to understand things a bit better...

According to the hardware manual, enabling the "core" clock but not the
"cd" clock is not a valid setting. So in our case, it's always all or
nothing.


Chris

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

[Chris Brandt]

Hello Ulf,


On Friday, January 20, 2017, Ulf Hansson wrote:
> On 18 January 2017 at 18:25, Chris Brandt <chris.brandt@renesas.com>
> wrote:
> > Some controllers have 2 clock sources instead of 1, so they both need
> > to be turned on/off.
> 
> This doesn't tell me enough. Please elaborate.
> 
> For example, tell how you treat the clocks, which of them that is optional
> and why.

Basically, the chip designers went in and broke out the logic that just does
the card detect (more or less it's probably just some simple IRQ logic) so
that you could shut off the IP block, but if you put in a card, the internal
interrupt signal still went to the interrupt controller and registered the
interrupt even though the rest of the IP block (including the register space)
was dead.

The idea seems to be that you could put the entire chip into low power mode
and wake it up if you stuck a card in.

My guess is that this was for some customer request or something.

Personally...you could have done the same thing if you laid out a board
and tied one of the extra IRQ lines to the cd signal...but I'm not sure if
anyone thought about that at the time.


So to your request, I could put all this ugly info into the documentation,
but basically all I'm trying to do is join the 2 clocks back together
to make it work like all the other SoCs since this new 'feature' might
not really be practical to every use in this environment.



> >  static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc) @@ -572,6
> > +581,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev)
> >                 goto eprobe;
> >         }
> >
> > +       priv->clk_cd = devm_clk_get(&pdev->dev, "cd");
> > +       if (IS_ERR(priv->clk_cd))
> > +               priv->clk_cd = NULL;
> 
> Is this clock solely about card detection? So in cases when you have a
> GPIO card detect, the clock isn't needed?
> 
> Just trying to understand things a bit better...

According to the hardware manual, enabling the "core" clock but not the
"cd" clock is not a valid setting. So in our case, it's always all or
nothing.


Chris

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

[Chris Brandt]

Hello Ulf,

Friday, January 20, 2017, Ulf Hansson wrote:
> > +- clocks: Most controllers only have 1 clock source per channel.
> However, some
> > +         have a second clock dedicated to card detection. If 2 clocks
> are
> > +         specified, you must name them as "core" and "cd". If the
> controller
> > +         only has 1 clock, naming is not required.
> 
> Could you please elaborate a bit on the card detection clock?
> 
> I guess that there is some kind of internal card detection logic (native
> card detect) in the SDHI IP, which requires a separate clock for it to
> work? Perhaps you can state that somehow?


The reality is that the chip guys cut up the standard SDHI IP to add a
'cool new feature', but all I want to do is put it back the way it was.

NOTE: The design guys like to reuse IP blocks from previous designs that they
know worked and didn't have bugs. So, there is a good chance you will see this
change in future RZ/A devices (or even other future Renesas SoC devices).
To remove an unwanted feature adds additional design risk of breaking
something else....and given the cost of redoing silicon mask layers...no
engineer wants that mistake on their hands.



> > +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;
> > +               status = "disabled";
> 
> The last line seems a bit odd to include in an example.

You're right. I'll take that out.

Thanks,
Chris

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

[Chris Brandt]

Hello Ulf,

Friday, January 20, 2017, Ulf Hansson wrote:
> > +- clocks: Most controllers only have 1 clock source per channel.
> However, some
> > +         have a second clock dedicated to card detection. If 2 clocks
> are
> > +         specified, you must name them as "core" and "cd". If the
> controller
> > +         only has 1 clock, naming is not required.
> 
> Could you please elaborate a bit on the card detection clock?
> 
> I guess that there is some kind of internal card detection logic (native
> card detect) in the SDHI IP, which requires a separate clock for it to
> work? Perhaps you can state that somehow?


The reality is that the chip guys cut up the standard SDHI IP to add a
'cool new feature', but all I want to do is put it back the way it was.

NOTE: The design guys like to reuse IP blocks from previous designs that they
know worked and didn't have bugs. So, there is a good chance you will see this
change in future RZ/A devices (or even other future Renesas SoC devices).
To remove an unwanted feature adds additional design risk of breaking
something else....and given the cost of redoing silicon mask layers...no
engineer wants that mistake on their hands.



> > +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;
> > +               status = "disabled";
> 
> The last line seems a bit odd to include in an example.

You're right. I'll take that out.

Thanks,
Chris

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

[Ulf Hansson]

On 20 January 2017 at 17:05, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> Hello Ulf,
>
> Friday, January 20, 2017, Ulf Hansson wrote:
>> > +- clocks: Most controllers only have 1 clock source per channel.
>> However, some
>> > +         have a second clock dedicated to card detection. If 2 clocks
>> are
>> > +         specified, you must name them as "core" and "cd". If the
>> controller
>> > +         only has 1 clock, naming is not required.
>>
>> Could you please elaborate a bit on the card detection clock?
>>
>> I guess that there is some kind of internal card detection logic (native
>> card detect) in the SDHI IP, which requires a separate clock for it to
>> work? Perhaps you can state that somehow?
>
>
> The reality is that the chip guys cut up the standard SDHI IP to add a
> 'cool new feature', but all I want to do is put it back the way it was.
>
> NOTE: The design guys like to reuse IP blocks from previous designs that they
> know worked and didn't have bugs. So, there is a good chance you will see this
> change in future RZ/A devices (or even other future Renesas SoC devices).
> To remove an unwanted feature adds additional design risk of breaking
> something else....and given the cost of redoing silicon mask layers...no
> engineer wants that mistake on their hands.

So, one should be aware of that runtime PM support in these case is
going to be suboptimal.
For example, when using this native card detect, you will need to keep
the controller runtime PM resumed as the be able to keep the clock on
and to be able to fetch the irq. Wasting power.

Most SoC vendors are therefore using a GPIO card detect instead,
although I assume you already knew that. :-)

[...]

Kind regards
Uffe

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

[Chris Brandt]

Hello Uffe,

On Friday, January 20, 2017, Ulf Hansson wrote:
> > The reality is that the chip guys cut up the standard SDHI IP to add a
> > 'cool new feature', but all I want to do is put it back the way it was.
> >
> > NOTE: The design guys like to reuse IP blocks from previous designs
> > that they know worked and didn't have bugs. So, there is a good chance
> > you will see this change in future RZ/A devices (or even other future
> Renesas SoC devices).
> > To remove an unwanted feature adds additional design risk of breaking
> > something else....and given the cost of redoing silicon mask
> > layers...no engineer wants that mistake on their hands.
> 
> So, one should be aware of that runtime PM support in these case is going
> to be suboptimal.
> For example, when using this native card detect, you will need to keep the
> controller runtime PM resumed as the be able to keep the clock on and to
> be able to fetch the irq. Wasting power.


Wolfram already pointed that out in a reply to Geert:

On Tuesday, January 17, 2017, Wolfram Sang wrote:
> > If we handle them as one, won't we miss card detect events due to the
> > card detect clock being disabled while SDHI is idle?
> 
> You mean this?
> 
> 1208         /*
> 1209          * While using internal tmio hardware logic for card
> detection, we need
> 1210          * to ensure it stays powered for it to work.
> 1211          */
> 1212         if (_host->native_hotplug)
> 1213                 pm_runtime_get_noresume(&pdev->dev);


As per your request, I'll go back and add some text to describing that even though
this specific HW has a separate clock for card detect for PM, the existing driver
infrastructure doesn't handle that so both clocks need to be treated as one.


> Most SoC vendors are therefore using a GPIO card detect instead, although
> I assume you already knew that. :-)

My first objective for the RZ/A1 platform is to move things from a local
custom BSP into upstream and get things 'working'. Later I can go back
and tweak things here and there for runtime PM and such.


Thank you,
Chris

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

[Wolfram Sang]

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


> > Is this clock solely about card detection? So in cases when you have a
> > GPIO card detect, the clock isn't needed?
> > 
> > Just trying to understand things a bit better...
> 
> According to the hardware manual, enabling the "core" clock but not the
> "cd" clock is not a valid setting. So in our case, it's always all or
> nothing.

It was my suggestion to either handle both clocks as "virtually" one
clock so it simulates how other SDHI instances behave, or to implement
proper and intended handling of the cd clock to save some power. Chris
chose the first option and I have full understanding for that decision.


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

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

[Wolfram Sang]

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


> > Is this clock solely about card detection? So in cases when you have a
> > GPIO card detect, the clock isn't needed?
> > 
> > Just trying to understand things a bit better...
> 
> According to the hardware manual, enabling the "core" clock but not the
> "cd" clock is not a valid setting. So in our case, it's always all or
> nothing.

It was my suggestion to either handle both clocks as "virtually" one
clock so it simulates how other SDHI instances behave, or to implement
proper and intended handling of the cd clock to save some power. Chris
chose the first option and I have full understanding for that decision.


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