diff mbox

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

Message ID 20170121030604.7672-3-chris.brandt@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Brandt Jan. 21, 2017, 3:06 a.m. UTC
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(+)

Comments

Wolfram Sang Jan. 21, 2017, 9:36 a.m. UTC | #1
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>
Geert Uytterhoeven Jan. 23, 2017, 9:18 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) Jan. 23, 2017, 4:55 p.m. UTC | #3
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
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Jan. 23, 2017, 5:56 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) Jan. 26, 2017, 2:39 p.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Jan. 26, 2017, 3:20 p.m. UTC | #6
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
Ulf Hansson Jan. 27, 2017, 8:36 a.m. UTC | #7
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
+	};