[v2,01/11] dt-bindings: mmc: arasan: Update documentation for SD Card Clock
diff mbox series

Message ID 1561958991-21935-2-git-send-email-manish.narani@xilinx.com
State New
Headers show
Series
  • Arasan SDHCI enhancements and ZynqMP Tap Delays Handling
Related show

Commit Message

Manish Narani July 1, 2019, 5:29 a.m. UTC
The clock handling is to be updated in the Arasan SDHCI. As the
'devm_clk_register' is deprecated in the clock framework, this needs to
specify one more clock named 'clk_sdcard' to get the clock in the driver
via 'devm_clk_get()'. This clock represents the clock from controller to
the card.

Signed-off-by: Manish Narani <manish.narani@xilinx.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Rob Herring July 22, 2019, 9:54 p.m. UTC | #1
On Mon, Jul 01, 2019 at 10:59:41AM +0530, Manish Narani wrote:
> The clock handling is to be updated in the Arasan SDHCI. As the
> 'devm_clk_register' is deprecated in the clock framework, this needs to
> specify one more clock named 'clk_sdcard' to get the clock in the driver
> via 'devm_clk_get()'. This clock represents the clock from controller to
> the card.

Please explain why in terms of the binding, not some driver calls.

 
> Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 1edbb04..15c6397 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -23,6 +23,10 @@ Required Properties:
>    - reg: From mmc bindings: Register location and length.
>    - clocks: From clock bindings: Handles to clock inputs.
>    - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
> +		 Apart from these two there is one more optional clock which
> +		 is "clk_sdcard". This clock represents output clock from
> +		 controller and card. This must be specified when #clock-cells
> +		 is specified.
>    - interrupts: Interrupt specifier
>  
>  Required Properties for "arasan,sdhci-5.1":
> @@ -36,9 +40,10 @@ Optional Properties:
>    - clock-output-names: If specified, this will be the name of the card clock
>      which will be exposed by this device.  Required if #clock-cells is
>      specified.
> -  - #clock-cells: If specified this should be the value <0>.  With this property
> -    in place we will export a clock representing the Card Clock.  This clock
> -    is expected to be consumed by our PHY.  You must also specify
> +  - #clock-cells: If specified this should be the value <0>. With this
> +    property in place we will export one clock representing the Card
> +    Clock. This clock is expected to be consumed by our PHY. You must also
> +    specify

specify what?

The 3rd clock input I assume? This statement means any existing users 
with 2 clock inputs and #clock-cells are in error now. Is that correct? 

>    - xlnx,fails-without-test-cd: when present, the controller doesn't work when
>      the CD line is not connected properly, and the line is not connected
>      properly. Test mode can be used to force the controller to function.
> @@ -70,8 +75,8 @@ Example:
>  		compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
>  		reg = <0x0 0xfe330000 0x0 0x10000>;
>  		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> -		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
> -		clock-names = "clk_xin", "clk_ahb";
> +		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>, <&sdhci 0>;
> +		clock-names = "clk_xin", "clk_ahb", "clk_sdcard";
>  		arasan,soc-ctl-syscon = <&grf>;
>  		assigned-clocks = <&cru SCLK_EMMC>;
>  		assigned-clock-rates = <200000000>;
> -- 
> 2.1.1
>
Manish Narani July 23, 2019, 8:23 a.m. UTC | #2
Hi Rob,

Thanks a lot for the review!


> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, July 23, 2019 3:24 AM
> To: Manish Narani <MNARANI@xilinx.com>
> Cc: ulf.hansson@linaro.org; mark.rutland@arm.com; heiko@sntech.de; Michal
> Simek <michals@xilinx.com>; adrian.hunter@intel.com;
> christoph.muellner@theobroma-systems.com; philipp.tomsich@theobroma-
> systems.com; viresh.kumar@linaro.org; scott.branden@broadcom.com;
> ayaka@soulik.info; kernel@esmil.dk; tony.xie@rock-chips.com; Rajan Vaja
> <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>; Nava kishore Manne
> <navam@xilinx.com>; mdf@kernel.org; olof@lixom.net; linux-
> mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> rockchip@lists.infradead.org
> Subject: Re: [PATCH v2 01/11] dt-bindings: mmc: arasan: Update
> documentation for SD Card Clock
> 
> On Mon, Jul 01, 2019 at 10:59:41AM +0530, Manish Narani wrote:
> > The clock handling is to be updated in the Arasan SDHCI. As the
> > 'devm_clk_register' is deprecated in the clock framework, this needs to
> > specify one more clock named 'clk_sdcard' to get the clock in the driver
> > via 'devm_clk_get()'. This clock represents the clock from controller to
> > the card.
> 
> Please explain why in terms of the binding, not some driver calls.
Okay.

> 
> 
> > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15 ++++++++++-
> ----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > index 1edbb04..15c6397 100644
> > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > @@ -23,6 +23,10 @@ Required Properties:
> >    - reg: From mmc bindings: Register location and length.
> >    - clocks: From clock bindings: Handles to clock inputs.
> >    - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
> > +		 Apart from these two there is one more optional clock which
> > +		 is "clk_sdcard". This clock represents output clock from
> > +		 controller and card. This must be specified when #clock-cells
> > +		 is specified.
> >    - interrupts: Interrupt specifier
> >
> >  Required Properties for "arasan,sdhci-5.1":
> > @@ -36,9 +40,10 @@ Optional Properties:
> >    - clock-output-names: If specified, this will be the name of the card clock
> >      which will be exposed by this device.  Required if #clock-cells is
> >      specified.
> > -  - #clock-cells: If specified this should be the value <0>.  With this property
> > -    in place we will export a clock representing the Card Clock.  This clock
> > -    is expected to be consumed by our PHY.  You must also specify
> > +  - #clock-cells: If specified this should be the value <0>. With this
> > +    property in place we will export one clock representing the Card
> > +    Clock. This clock is expected to be consumed by our PHY. You must also
> > +    specify
> 
> specify what?
I think this line was already there, I missed to correct it, Will update in v3.

> 
> The 3rd clock input I assume? This statement means any existing users
> with 2 clock inputs and #clock-cells are in error now. Is that correct?
Yes, this is correct. So far there was only one vendor using '#clock-cells'  which is Rockchip. I have sent DT patch (02/11) for that also.
Here this is needed as earlier implementation isn't correct as suggested by Uffe. (https://lkml.org/lkml/2019/6/20/486) .

Thanks,
Manish
Ulf Hansson July 25, 2019, 1 p.m. UTC | #3
On Tue, 23 Jul 2019 at 10:23, Manish Narani <MNARANI@xilinx.com> wrote:
>
> Hi Rob,
>
> Thanks a lot for the review!
>
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Tuesday, July 23, 2019 3:24 AM
> > To: Manish Narani <MNARANI@xilinx.com>
> > Cc: ulf.hansson@linaro.org; mark.rutland@arm.com; heiko@sntech.de; Michal
> > Simek <michals@xilinx.com>; adrian.hunter@intel.com;
> > christoph.muellner@theobroma-systems.com; philipp.tomsich@theobroma-
> > systems.com; viresh.kumar@linaro.org; scott.branden@broadcom.com;
> > ayaka@soulik.info; kernel@esmil.dk; tony.xie@rock-chips.com; Rajan Vaja
> > <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>; Nava kishore Manne
> > <navam@xilinx.com>; mdf@kernel.org; olof@lixom.net; linux-
> > mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > rockchip@lists.infradead.org
> > Subject: Re: [PATCH v2 01/11] dt-bindings: mmc: arasan: Update
> > documentation for SD Card Clock
> >
> > On Mon, Jul 01, 2019 at 10:59:41AM +0530, Manish Narani wrote:
> > > The clock handling is to be updated in the Arasan SDHCI. As the
> > > 'devm_clk_register' is deprecated in the clock framework, this needs to
> > > specify one more clock named 'clk_sdcard' to get the clock in the driver
> > > via 'devm_clk_get()'. This clock represents the clock from controller to
> > > the card.
> >
> > Please explain why in terms of the binding, not some driver calls.
> Okay.
>
> >
> >
> > > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > > ---
> > >  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15 ++++++++++-
> > ----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > index 1edbb04..15c6397 100644
> > > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > @@ -23,6 +23,10 @@ Required Properties:
> > >    - reg: From mmc bindings: Register location and length.
> > >    - clocks: From clock bindings: Handles to clock inputs.
> > >    - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
> > > +            Apart from these two there is one more optional clock which
> > > +            is "clk_sdcard". This clock represents output clock from
> > > +            controller and card. This must be specified when #clock-cells
> > > +            is specified.
> > >    - interrupts: Interrupt specifier
> > >
> > >  Required Properties for "arasan,sdhci-5.1":
> > > @@ -36,9 +40,10 @@ Optional Properties:
> > >    - clock-output-names: If specified, this will be the name of the card clock
> > >      which will be exposed by this device.  Required if #clock-cells is
> > >      specified.
> > > -  - #clock-cells: If specified this should be the value <0>.  With this property
> > > -    in place we will export a clock representing the Card Clock.  This clock
> > > -    is expected to be consumed by our PHY.  You must also specify
> > > +  - #clock-cells: If specified this should be the value <0>. With this
> > > +    property in place we will export one clock representing the Card
> > > +    Clock. This clock is expected to be consumed by our PHY. You must also
> > > +    specify
> >
> > specify what?
> I think this line was already there, I missed to correct it, Will update in v3.
>
> >
> > The 3rd clock input I assume? This statement means any existing users
> > with 2 clock inputs and #clock-cells are in error now. Is that correct?
> Yes, this is correct. So far there was only one vendor using '#clock-cells'  which is Rockchip. I have sent DT patch (02/11) for that also.
> Here this is needed as earlier implementation isn't correct as suggested by Uffe. (https://lkml.org/lkml/2019/6/20/486) .

I am not sure how big of a problem the backwards compatible thingy
with DT is, in general we must not break it. What do you say Manish?

As a workaround, would it be possible to use
of_clk_get_from_provider() somehow to address the compatibility issue?
Or maybe there is another clock API that can help.

Kind regards
Uffe
Manish Narani Aug. 19, 2019, 9:21 a.m. UTC | #4
Hi Uffe,


> -----Original Message-----
> From: Ulf Hansson <ulf.hansson@linaro.org>
> Sent: Thursday, July 25, 2019 6:31 PM
> To: Manish Narani <MNARANI@xilinx.com>
> Cc: Rob Herring <robh@kernel.org>; mark.rutland@arm.com;
> heiko@sntech.de; Michal Simek <michals@xilinx.com>;
> adrian.hunter@intel.com; christoph.muellner@theobroma-systems.com;
> philipp.tomsich@theobroma-systems.com; viresh.kumar@linaro.org;
> scott.branden@broadcom.com; ayaka@soulik.info; kernel@esmil.dk;
> tony.xie@rock-chips.com; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah
> <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>;
> mdf@kernel.org; olof@lixom.net; linux-mmc@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-rockchip@lists.infradead.org
> Subject: Re: [PATCH v2 01/11] dt-bindings: mmc: arasan: Update
> documentation for SD Card Clock
> 
> On Tue, 23 Jul 2019 at 10:23, Manish Narani <MNARANI@xilinx.com> wrote:
> >
> > Hi Rob,
> >
> > Thanks a lot for the review!
> >
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Tuesday, July 23, 2019 3:24 AM
> > > To: Manish Narani <MNARANI@xilinx.com>
> > > Cc: ulf.hansson@linaro.org; mark.rutland@arm.com; heiko@sntech.de;
> Michal
> > > Simek <michals@xilinx.com>; adrian.hunter@intel.com;
> > > christoph.muellner@theobroma-systems.com; philipp.tomsich@theobroma-
> > > systems.com; viresh.kumar@linaro.org; scott.branden@broadcom.com;
> > > ayaka@soulik.info; kernel@esmil.dk; tony.xie@rock-chips.com; Rajan Vaja
> > > <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>; Nava kishore
> Manne
> > > <navam@xilinx.com>; mdf@kernel.org; olof@lixom.net; linux-
> > > mmc@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > > rockchip@lists.infradead.org
> > > Subject: Re: [PATCH v2 01/11] dt-bindings: mmc: arasan: Update
> > > documentation for SD Card Clock
> > >
> > > On Mon, Jul 01, 2019 at 10:59:41AM +0530, Manish Narani wrote:
> > > > The clock handling is to be updated in the Arasan SDHCI. As the
> > > > 'devm_clk_register' is deprecated in the clock framework, this needs to
> > > > specify one more clock named 'clk_sdcard' to get the clock in the driver
> > > > via 'devm_clk_get()'. This clock represents the clock from controller to
> > > > the card.
> > >
> > > Please explain why in terms of the binding, not some driver calls.
> > Okay.
> >
> > >
> > >
> > > > Signed-off-by: Manish Narani <manish.narani@xilinx.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15
> ++++++++++-
> > > ----
> > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > index 1edbb04..15c6397 100644
> > > > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > @@ -23,6 +23,10 @@ Required Properties:
> > > >    - reg: From mmc bindings: Register location and length.
> > > >    - clocks: From clock bindings: Handles to clock inputs.
> > > >    - clock-names: From clock bindings: Tuple including "clk_xin" and
> "clk_ahb"
> > > > +            Apart from these two there is one more optional clock which
> > > > +            is "clk_sdcard". This clock represents output clock from
> > > > +            controller and card. This must be specified when #clock-cells
> > > > +            is specified.
> > > >    - interrupts: Interrupt specifier
> > > >
> > > >  Required Properties for "arasan,sdhci-5.1":
> > > > @@ -36,9 +40,10 @@ Optional Properties:
> > > >    - clock-output-names: If specified, this will be the name of the card
> clock
> > > >      which will be exposed by this device.  Required if #clock-cells is
> > > >      specified.
> > > > -  - #clock-cells: If specified this should be the value <0>.  With this
> property
> > > > -    in place we will export a clock representing the Card Clock.  This clock
> > > > -    is expected to be consumed by our PHY.  You must also specify
> > > > +  - #clock-cells: If specified this should be the value <0>. With this
> > > > +    property in place we will export one clock representing the Card
> > > > +    Clock. This clock is expected to be consumed by our PHY. You must
> also
> > > > +    specify
> > >
> > > specify what?
> > I think this line was already there, I missed to correct it, Will update in v3.
> >
> > >
> > > The 3rd clock input I assume? This statement means any existing users
> > > with 2 clock inputs and #clock-cells are in error now. Is that correct?
> > Yes, this is correct. So far there was only one vendor using '#clock-cells'
> which is Rockchip. I have sent DT patch (02/11) for that also.
> > Here this is needed as earlier implementation isn't correct as suggested by
> Uffe. (https://lkml.org/lkml/2019/6/20/486) .
> 
> I am not sure how big of a problem the backwards compatible thingy
> with DT is, in general we must not break it. What do you say Manish?

Though I agree with Uffe on this, there is no other way from my understanding. Please suggest.

> 
> As a workaround, would it be possible to use
> of_clk_get_from_provider() somehow to address the compatibility issue?

For this to be used we have to parse 'clkspec' from the DT node and pass the same as an argument to this function. In this case also the DT node needs to be updated, which is same as we have done in this series.



Thanks,
Manish
Ulf Hansson Aug. 22, 2019, 1:38 p.m. UTC | #5
[...]

> > > > > ---
> > > > >  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15
> > ++++++++++-
> > > > ----
> > > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > index 1edbb04..15c6397 100644
> > > > > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > @@ -23,6 +23,10 @@ Required Properties:
> > > > >    - reg: From mmc bindings: Register location and length.
> > > > >    - clocks: From clock bindings: Handles to clock inputs.
> > > > >    - clock-names: From clock bindings: Tuple including "clk_xin" and
> > "clk_ahb"
> > > > > +            Apart from these two there is one more optional clock which
> > > > > +            is "clk_sdcard". This clock represents output clock from
> > > > > +            controller and card. This must be specified when #clock-cells
> > > > > +            is specified.
> > > > >    - interrupts: Interrupt specifier
> > > > >
> > > > >  Required Properties for "arasan,sdhci-5.1":
> > > > > @@ -36,9 +40,10 @@ Optional Properties:
> > > > >    - clock-output-names: If specified, this will be the name of the card
> > clock
> > > > >      which will be exposed by this device.  Required if #clock-cells is
> > > > >      specified.
> > > > > -  - #clock-cells: If specified this should be the value <0>.  With this
> > property
> > > > > -    in place we will export a clock representing the Card Clock.  This clock
> > > > > -    is expected to be consumed by our PHY.  You must also specify
> > > > > +  - #clock-cells: If specified this should be the value <0>. With this
> > > > > +    property in place we will export one clock representing the Card
> > > > > +    Clock. This clock is expected to be consumed by our PHY. You must
> > also
> > > > > +    specify
> > > >
> > > > specify what?
> > > I think this line was already there, I missed to correct it, Will update in v3.
> > >
> > > >
> > > > The 3rd clock input I assume? This statement means any existing users
> > > > with 2 clock inputs and #clock-cells are in error now. Is that correct?
> > > Yes, this is correct. So far there was only one vendor using '#clock-cells'
> > which is Rockchip. I have sent DT patch (02/11) for that also.
> > > Here this is needed as earlier implementation isn't correct as suggested by
> > Uffe. (https://lkml.org/lkml/2019/6/20/486) .
> >
> > I am not sure how big of a problem the backwards compatible thingy
> > with DT is, in general we must not break it. What do you say Manish?
>
> Though I agree with Uffe on this, there is no other way from my understanding. Please suggest.
>
> >
> > As a workaround, would it be possible to use
> > of_clk_get_from_provider() somehow to address the compatibility issue?
>
> For this to be used we have to parse 'clkspec' from the DT node and pass the same as an argument to this function. In this case also the DT node needs to be updated, which is same as we have done in this series.

Alright. I guess breaking DTBs for Rockchip platforms isn't
acceptable, especially if those are already widely deployed, which I
have no idea of....

And having support for both options in the driver seems not a great
option either, so it looks like you need to convert back into the old
v1 approach. Huh, sorry.

Kind regards
Uffe
Heiko Stuebner Aug. 22, 2019, 6:23 p.m. UTC | #6
Am Donnerstag, 22. August 2019, 15:38:26 CEST schrieb Ulf Hansson:
> [...]
> 
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15
> > > ++++++++++-
> > > > > ----
> > > > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > > index 1edbb04..15c6397 100644
> > > > > > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > > @@ -23,6 +23,10 @@ Required Properties:
> > > > > >    - reg: From mmc bindings: Register location and length.
> > > > > >    - clocks: From clock bindings: Handles to clock inputs.
> > > > > >    - clock-names: From clock bindings: Tuple including "clk_xin" and
> > > "clk_ahb"
> > > > > > +            Apart from these two there is one more optional clock which
> > > > > > +            is "clk_sdcard". This clock represents output clock from
> > > > > > +            controller and card. This must be specified when #clock-cells
> > > > > > +            is specified.
> > > > > >    - interrupts: Interrupt specifier
> > > > > >
> > > > > >  Required Properties for "arasan,sdhci-5.1":
> > > > > > @@ -36,9 +40,10 @@ Optional Properties:
> > > > > >    - clock-output-names: If specified, this will be the name of the card
> > > clock
> > > > > >      which will be exposed by this device.  Required if #clock-cells is
> > > > > >      specified.
> > > > > > -  - #clock-cells: If specified this should be the value <0>.  With this
> > > property
> > > > > > -    in place we will export a clock representing the Card Clock.  This clock
> > > > > > -    is expected to be consumed by our PHY.  You must also specify
> > > > > > +  - #clock-cells: If specified this should be the value <0>. With this
> > > > > > +    property in place we will export one clock representing the Card
> > > > > > +    Clock. This clock is expected to be consumed by our PHY. You must
> > > also
> > > > > > +    specify
> > > > >
> > > > > specify what?
> > > > I think this line was already there, I missed to correct it, Will update in v3.
> > > >
> > > > >
> > > > > The 3rd clock input I assume? This statement means any existing users
> > > > > with 2 clock inputs and #clock-cells are in error now. Is that correct?
> > > > Yes, this is correct. So far there was only one vendor using '#clock-cells'
> > > which is Rockchip. I have sent DT patch (02/11) for that also.
> > > > Here this is needed as earlier implementation isn't correct as suggested by
> > > Uffe. (https://lkml.org/lkml/2019/6/20/486) .
> > >
> > > I am not sure how big of a problem the backwards compatible thingy
> > > with DT is, in general we must not break it. What do you say Manish?
> >
> > Though I agree with Uffe on this, there is no other way from my understanding. Please suggest.
> >
> > >
> > > As a workaround, would it be possible to use
> > > of_clk_get_from_provider() somehow to address the compatibility issue?
> >
> > For this to be used we have to parse 'clkspec' from the DT node and pass the same as an argument to this function. In this case also the DT node needs to be updated, which is same as we have done in this series.
> 
> Alright. I guess breaking DTBs for Rockchip platforms isn't
> acceptable, especially if those are already widely deployed, which I
> have no idea of....

The arasan sdhci is part of the rk3399, so every SBC using that SoC, but
also the whole Gru series of ChromeOS devices (Samsung Chromebook Plus
among them) would be affected.

Heiko
Manish Narani Aug. 23, 2019, 10:01 a.m. UTC | #7
Hi Heiko/Uffe,



> -----Original Message-----
> From: Heiko Stuebner <heiko@sntech.de>
> Sent: Thursday, August 22, 2019 11:53 PM
> To: Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org
> Cc: Manish Narani <MNARANI@xilinx.com>; Rob Herring <robh@kernel.org>;
> mark.rutland@arm.com; Michal Simek <michals@xilinx.com>;
> adrian.hunter@intel.com; christoph.muellner@theobroma-systems.com;
> philipp.tomsich@theobroma-systems.com; viresh.kumar@linaro.org;
> scott.branden@broadcom.com; ayaka@soulik.info; kernel@esmil.dk;
> tony.xie@rock-chips.com; Rajan Vaja <RAJANV@xilinx.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-rockchip@lists.infradead.org
> Subject: Re: [PATCH v2 01/11] dt-bindings: mmc: arasan: Update
> documentation for SD Card Clock
> 
> Am Donnerstag, 22. August 2019, 15:38:26 CEST schrieb Ulf Hansson:
> > [...]
> >
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 15
> > > > ++++++++++-
> > > > > > ----
> > > > > > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git
> a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > > b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > > > index 1edbb04..15c6397 100644
> > > > > > > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > > > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > > > > > > @@ -23,6 +23,10 @@ Required Properties:
> > > > > > >    - reg: From mmc bindings: Register location and length.
> > > > > > >    - clocks: From clock bindings: Handles to clock inputs.
> > > > > > >    - clock-names: From clock bindings: Tuple including "clk_xin" and
> > > > "clk_ahb"
> > > > > > > +            Apart from these two there is one more optional clock
> which
> > > > > > > +            is "clk_sdcard". This clock represents output clock from
> > > > > > > +            controller and card. This must be specified when #clock-
> cells
> > > > > > > +            is specified.
> > > > > > >    - interrupts: Interrupt specifier
> > > > > > >
> > > > > > >  Required Properties for "arasan,sdhci-5.1":
> > > > > > > @@ -36,9 +40,10 @@ Optional Properties:
> > > > > > >    - clock-output-names: If specified, this will be the name of the card
> > > > clock
> > > > > > >      which will be exposed by this device.  Required if #clock-cells is
> > > > > > >      specified.
> > > > > > > -  - #clock-cells: If specified this should be the value <0>.  With this
> > > > property
> > > > > > > -    in place we will export a clock representing the Card Clock.  This
> clock
> > > > > > > -    is expected to be consumed by our PHY.  You must also specify
> > > > > > > +  - #clock-cells: If specified this should be the value <0>. With this
> > > > > > > +    property in place we will export one clock representing the Card
> > > > > > > +    Clock. This clock is expected to be consumed by our PHY. You
> must
> > > > also
> > > > > > > +    specify
> > > > > >
> > > > > > specify what?
> > > > > I think this line was already there, I missed to correct it, Will update in
> v3.
> > > > >
> > > > > >
> > > > > > The 3rd clock input I assume? This statement means any existing users
> > > > > > with 2 clock inputs and #clock-cells are in error now. Is that correct?
> > > > > Yes, this is correct. So far there was only one vendor using '#clock-cells'
> > > > which is Rockchip. I have sent DT patch (02/11) for that also.
> > > > > Here this is needed as earlier implementation isn't correct as suggested
> by
> > > > Uffe. (https://lkml.org/lkml/2019/6/20/486) .
> > > >
> > > > I am not sure how big of a problem the backwards compatible thingy
> > > > with DT is, in general we must not break it. What do you say Manish?
> > >
> > > Though I agree with Uffe on this, there is no other way from my
> understanding. Please suggest.
> > >
> > > >
> > > > As a workaround, would it be possible to use
> > > > of_clk_get_from_provider() somehow to address the compatibility issue?
> > >
> > > For this to be used we have to parse 'clkspec' from the DT node and pass
> the same as an argument to this function. In this case also the DT node needs to
> be updated, which is same as we have done in this series.
> >
> > Alright. I guess breaking DTBs for Rockchip platforms isn't
> > acceptable, especially if those are already widely deployed, which I
> > have no idea of....
> 
> The arasan sdhci is part of the rk3399, so every SBC using that SoC, but
> also the whole Gru series of ChromeOS devices (Samsung Chromebook Plus
> among them) would be affected.

Thanks for confirming. This will be taken care of. I will go back to v1 changes and will send v3.

Thanks,
Manish

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 1edbb04..15c6397 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -23,6 +23,10 @@  Required Properties:
   - reg: From mmc bindings: Register location and length.
   - clocks: From clock bindings: Handles to clock inputs.
   - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
+		 Apart from these two there is one more optional clock which
+		 is "clk_sdcard". This clock represents output clock from
+		 controller and card. This must be specified when #clock-cells
+		 is specified.
   - interrupts: Interrupt specifier
 
 Required Properties for "arasan,sdhci-5.1":
@@ -36,9 +40,10 @@  Optional Properties:
   - clock-output-names: If specified, this will be the name of the card clock
     which will be exposed by this device.  Required if #clock-cells is
     specified.
-  - #clock-cells: If specified this should be the value <0>.  With this property
-    in place we will export a clock representing the Card Clock.  This clock
-    is expected to be consumed by our PHY.  You must also specify
+  - #clock-cells: If specified this should be the value <0>. With this
+    property in place we will export one clock representing the Card
+    Clock. This clock is expected to be consumed by our PHY. You must also
+    specify
   - xlnx,fails-without-test-cd: when present, the controller doesn't work when
     the CD line is not connected properly, and the line is not connected
     properly. Test mode can be used to force the controller to function.
@@ -70,8 +75,8 @@  Example:
 		compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
 		reg = <0x0 0xfe330000 0x0 0x10000>;
 		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
-		clock-names = "clk_xin", "clk_ahb";
+		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>, <&sdhci 0>;
+		clock-names = "clk_xin", "clk_ahb", "clk_sdcard";
 		arasan,soc-ctl-syscon = <&grf>;
 		assigned-clocks = <&cru SCLK_EMMC>;
 		assigned-clock-rates = <200000000>;