[RESEND PATCH v2 0/6] Update Kona drivers to use clocks

[RESEND PATCH v2 0/6] Update Kona drivers to use clocks

[Tim Kryger]

This series declares the clocks present on bcm11351 (aka bcm281xx) SoCs
to be fixed at the rates configured by development bootloaders and then
updates the Kona drivers to make use of clock calls where appropriate.

This patch series depends on the following three commits being present:

https://lkml.org/lkml/2013/9/18/409
https://lkml.org/lkml/2013/9/20/305
http://www.spinics.net/lists/arm-kernel/msg274973.html

Changes in v2:
  - Renamed bsc4_clk to pmu_bsc_clk

Tim Kryger (6):
  ARM: dts: Declare clocks as fixed on bcm11351
  ARM: dts: Specify clocks for UARTs on bcm11351
  ARM: dts: Specify clocks for SDHCIs on bcm11351
  clocksource: kona: Add basic use of external clock
  ARM: dts: Specify clocks for timer on bcm11351
  mmc: sdhci-bcm-kona: Add basic use of clocks

 arch/arm/boot/dts/bcm11351.dtsi      | 111 +++++++++++++++++++++++++++++++++--
 drivers/clocksource/bcm_kona_timer.c |  14 ++++-
 drivers/mmc/host/sdhci-bcm-kona.c    |  30 +++++++++-
 3 files changed, 145 insertions(+), 10 deletions(-)

-- 
1.8.0.1

[RESEND PATCH v2 1/6] ARM: dts: Declare clocks as fixed on bcm11351

[Tim Kryger]

Declare clocks that are enabled and configured by bootloaders as fixed
rate clocks in the DTS such that device drivers may use standard clock
function calls.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 arch/arm/boot/dts/bcm11351.dtsi | 97 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
index 98e6bc0..c6464fb 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -126,4 +126,101 @@
 		status = "disabled";
 	};
 
+	clocks {
+		bsc1_clk: bsc1 {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		bsc2_clk: bsc2 {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		bsc3_clk: bsc3 {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		pmu_bsc_clk: pmu_bsc {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		hub_timer_clk: hub_timer {
+			compatible = "fixed-clock";
+			clock-frequency = <32768>;
+			#clock-cells = <0>;
+		};
+
+		pwm_clk: pwm {
+			compatible = "fixed-clock";
+			clock-frequency = <26000000>;
+			#clock-cells = <0>;
+		};
+
+		sdio1_clk: sdio1 {
+			compatible = "fixed-clock";
+			clock-frequency = <48000000>;
+			#clock-cells = <0>;
+		};
+
+		sdio2_clk: sdio2 {
+			compatible = "fixed-clock";
+			clock-frequency = <48000000>;
+			#clock-cells = <0>;
+		};
+
+		sdio3_clk: sdio3 {
+			compatible = "fixed-clock";
+			clock-frequency = <48000000>;
+			#clock-cells = <0>;
+		};
+
+		sdio4_clk: sdio4 {
+			compatible = "fixed-clock";
+			clock-frequency = <48000000>;
+			#clock-cells = <0>;
+		};
+
+		tmon_1m_clk: tmon_1m {
+			compatible = "fixed-clock";
+			clock-frequency = <1000000>;
+			#clock-cells = <0>;
+		};
+
+		uartb_clk: uartb {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		uartb2_clk: uartb2 {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		uartb3_clk: uartb3 {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		uartb4_clk: uartb4 {
+			compatible = "fixed-clock";
+			clock-frequency = <13000000>;
+			#clock-cells = <0>;
+		};
+
+		usb_otg_ahb_clk: usb_otg_ahb {
+			compatible = "fixed-clock";
+			clock-frequency = <52000000>;
+			#clock-cells = <0>;
+		};
+	};
 };
-- 
1.8.0.1

Re: [RESEND PATCH v2 1/6] ARM: dts: Declare clocks as fixed on bcm11351

[Daniel Lezcano]

On 10/16/2013 11:47 PM, Tim Kryger wrote:
> Declare clocks that are enabled and configured by bootloaders as fixed
> rate clocks in the DTS such that device drivers may use standard clock
> function calls.
>
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---

Hi Tim,

I was wondering what is the status of this patchset ?

The patchset touches different areas maintained by different people. 
Through what tree do you expect this patchset to be merged ??

Thanks
   -- Daniel

>   arch/arm/boot/dts/bcm11351.dtsi | 97 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 97 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
> index 98e6bc0..c6464fb 100644
> --- a/arch/arm/boot/dts/bcm11351.dtsi
> +++ b/arch/arm/boot/dts/bcm11351.dtsi
> @@ -126,4 +126,101 @@
>   		status = "disabled";
>   	};
>
> +	clocks {
> +		bsc1_clk: bsc1 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		bsc2_clk: bsc2 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		bsc3_clk: bsc3 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pmu_bsc_clk: pmu_bsc {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		hub_timer_clk: hub_timer {
> +			compatible = "fixed-clock";
> +			clock-frequency = <32768>;
> +			#clock-cells = <0>;
> +		};
> +
> +		pwm_clk: pwm {
> +			compatible = "fixed-clock";
> +			clock-frequency = <26000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		sdio1_clk: sdio1 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <48000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		sdio2_clk: sdio2 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <48000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		sdio3_clk: sdio3 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <48000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		sdio4_clk: sdio4 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <48000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		tmon_1m_clk: tmon_1m {
> +			compatible = "fixed-clock";
> +			clock-frequency = <1000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		uartb_clk: uartb {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		uartb2_clk: uartb2 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		uartb3_clk: uartb3 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		uartb4_clk: uartb4 {
> +			compatible = "fixed-clock";
> +			clock-frequency = <13000000>;
> +			#clock-cells = <0>;
> +		};
> +
> +		usb_otg_ahb_clk: usb_otg_ahb {
> +			compatible = "fixed-clock";
> +			clock-frequency = <52000000>;
> +			#clock-cells = <0>;
> +		};
> +	};
>   };
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [RESEND PATCH v2 1/6] ARM: dts: Declare clocks as fixed on bcm11351

[Tim Kryger]

On Fri, Nov 8, 2013 at 2:53 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

> I was wondering what is the status of this patchset ?
>
> The patchset touches different areas maintained by different people. Through
> what tree do you expect this patchset to be merged ??

Hi Daniel,

I plan to send an updated version soon to address some concerns raised by Mark.

Once all the maintainers are satisfied and provide their acks, I would
like Christian to take the series into his tree.

I expect Christian will have a few other Broadcom DTS file changes in
his tree so sending the series through him should make it easier to
avoid/manage conflicts.

Christian, would that work for you?

Thanks,
Tim Kryger

Re: [RESEND PATCH v2 1/6] ARM: dts: Declare clocks as fixed on bcm11351

[Christian Daudt]

I can pick up the dt related changes as long as the other code works
as-is both with and without the change (i.e. no backwards/forwards
compatibility issues). If this must be introduced alongside the other
changes, then the patches will have to be reworked to break them up by
subtree so they can go in atomically.
 csd


On Fri, Nov 8, 2013 at 11:42 AM, Tim Kryger <tim.kryger@linaro.org> wrote:
> On Fri, Nov 8, 2013 at 2:53 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>
>> I was wondering what is the status of this patchset ?
>>
>> The patchset touches different areas maintained by different people. Through
>> what tree do you expect this patchset to be merged ??
>
> Hi Daniel,
>
> I plan to send an updated version soon to address some concerns raised by Mark.
>
> Once all the maintainers are satisfied and provide their acks, I would
> like Christian to take the series into his tree.
>
> I expect Christian will have a few other Broadcom DTS file changes in
> his tree so sending the series through him should make it easier to
> avoid/manage conflicts.
>
> Christian, would that work for you?
>
> Thanks,
> Tim Kryger

[RESEND PATCH v2 2/6] ARM: dts: Specify clocks for UARTs on bcm11351

[Tim Kryger]

Rather than declaring the frequency of the external clock, specify the
label of the clock such that the driver may determine the frequency on
its own.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 arch/arm/boot/dts/bcm11351.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
index c6464fb..ce65367 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -43,7 +43,7 @@
 		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
 		status = "disabled";
 		reg = <0x3e000000 0x1000>;
-		clock-frequency = <13000000>;
+		clocks = <&uartb_clk>;
 		interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
 		reg-shift = <2>;
 		reg-io-width = <4>;
@@ -53,7 +53,7 @@
 		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
 		status = "disabled";
 		reg = <0x3e001000 0x1000>;
-		clock-frequency = <13000000>;
+		clocks = <&uartb2_clk>;
 		interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
 		reg-shift = <2>;
 		reg-io-width = <4>;
@@ -63,7 +63,7 @@
 		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
 		status = "disabled";
 		reg = <0x3e002000 0x1000>;
-		clock-frequency = <13000000>;
+		clocks = <&uartb3_clk>;
 		interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
 		reg-shift = <2>;
 		reg-io-width = <4>;
@@ -73,7 +73,7 @@
 		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
 		status = "disabled";
 		reg = <0x3e003000 0x1000>;
-		clock-frequency = <13000000>;
+		clocks = <&uartb4_clk>;
 		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
 		reg-shift = <2>;
 		reg-io-width = <4>;
-- 
1.8.0.1

Re: [RESEND PATCH v2 2/6] ARM: dts: Specify clocks for UARTs on bcm11351

[Christian Daudt]

On Wed, Oct 16, 2013 at 2:47 PM, Tim Kryger <tim.kryger@linaro.org> wrote:
> Rather than declaring the frequency of the external clock, specify the
> label of the clock such that the driver may determine the frequency on
> its own.
>
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---
>  arch/arm/boot/dts/bcm11351.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
> index c6464fb..ce65367 100644
> --- a/arch/arm/boot/dts/bcm11351.dtsi
> +++ b/arch/arm/boot/dts/bcm11351.dtsi
> @@ -43,7 +43,7 @@
>                 compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>                 status = "disabled";
>                 reg = <0x3e000000 0x1000>;
> -               clock-frequency = <13000000>;
> +               clocks = <&uartb_clk>;
>                 interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
>                 reg-shift = <2>;
>                 reg-io-width = <4>;

Hi Sebastian,
 this patch series (and a subsequent one from Tim) both rely on your
"ARM: provide common arch init for DT clocks" patchset in order to
work. Will that patchset be in 3.13 ? I don't want to pull the dt mods
unless they are as they break the boot without them.

 Thanks,
   csd

Re: [RESEND PATCH v2 2/6] ARM: dts: Specify clocks for UARTs on bcm11351

[Sebastian Hesselbarth]

On 10/17/2013 07:41 AM, Christian Daudt wrote:
> On Wed, Oct 16, 2013 at 2:47 PM, Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Rather than declaring the frequency of the external clock, specify the
>> label of the clock such that the driver may determine the frequency on
>> its own.
>>
>> Signed-off-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Reviewed-by: Matt Porter <matt.porter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>   arch/arm/boot/dts/bcm11351.dtsi | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
>> index c6464fb..ce65367 100644
>> --- a/arch/arm/boot/dts/bcm11351.dtsi
>> +++ b/arch/arm/boot/dts/bcm11351.dtsi
>> @@ -43,7 +43,7 @@
>>                  compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>>                  status = "disabled";
>>                  reg = <0x3e000000 0x1000>;
>> -               clock-frequency = <13000000>;
>> +               clocks = <&uartb_clk>;
>>                  interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
>>                  reg-shift = <2>;
>>                  reg-io-width = <4>;
>
> Hi Sebastian,
>   this patch series (and a subsequent one from Tim) both rely on your
> "ARM: provide common arch init for DT clocks" patchset in order to
> work. Will that patchset be in 3.13 ? I don't want to pull the dt mods
> unless they are as they break the boot without them.

Christian,

here is the pull request [1], pull [2], and stable commitment for the
branch [3]. You can merge that branch too and name the dependency when
you send your PR, arm-soc maintainers will sort it out.

Besides a small section mismatch fixup for nomadik that Olof requested,
I guess it is in.

Sebastian

[1] http://www.spinics.net/lists/arm-kernel/msg276175.html
[2] http://www.spinics.net/lists/arm-kernel/msg277816.html
[3] http://www.spinics.net/lists/arm-kernel/msg279736.html

--
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

Re: [RESEND PATCH v2 2/6] ARM: dts: Specify clocks for UARTs on bcm11351

[Mark Rutland]

On Wed, Oct 16, 2013 at 10:47:06PM +0100, Tim Kryger wrote:
> Rather than declaring the frequency of the external clock, specify the
> label of the clock such that the driver may determine the frequency on
> its own.

Nit: we're not specifying the label of the clock. Clocks are represented
py a phand+args pair, and the important part is that we're specifying
the clock itself.

How about something like:

---->8----
Currently the rate of the external clock input to "snps,dw-apb-uart"
devices is described by a clock-frequency property rather than by
reference to the clock itself.

This patch changes the "snps,dw-apb-uart" entries in bcm11351.dtsi to
refer to the parent clock directly, following the common clock bindings.
---->8----

Also, this should probably be moved after the driver change, so as to
not be unbootable temporarily.

Thanks,
Mark.

> 
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---
>  arch/arm/boot/dts/bcm11351.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
> index c6464fb..ce65367 100644
> --- a/arch/arm/boot/dts/bcm11351.dtsi
> +++ b/arch/arm/boot/dts/bcm11351.dtsi
> @@ -43,7 +43,7 @@
>  		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>  		status = "disabled";
>  		reg = <0x3e000000 0x1000>;
> -		clock-frequency = <13000000>;
> +		clocks = <&uartb_clk>;
>  		interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
>  		reg-shift = <2>;
>  		reg-io-width = <4>;
> @@ -53,7 +53,7 @@
>  		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>  		status = "disabled";
>  		reg = <0x3e001000 0x1000>;
> -		clock-frequency = <13000000>;
> +		clocks = <&uartb2_clk>;
>  		interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
>  		reg-shift = <2>;
>  		reg-io-width = <4>;
> @@ -63,7 +63,7 @@
>  		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>  		status = "disabled";
>  		reg = <0x3e002000 0x1000>;
> -		clock-frequency = <13000000>;
> +		clocks = <&uartb3_clk>;
>  		interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
>  		reg-shift = <2>;
>  		reg-io-width = <4>;
> @@ -73,7 +73,7 @@
>  		compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>  		status = "disabled";
>  		reg = <0x3e003000 0x1000>;
> -		clock-frequency = <13000000>;
> +		clocks = <&uartb4_clk>;
>  		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>  		reg-shift = <2>;
>  		reg-io-width = <4>;
> -- 
> 1.8.0.1
> 
> 
> 

Re: [RESEND PATCH v2 2/6] ARM: dts: Specify clocks for UARTs on bcm11351

[Tim Kryger]

On Thu, Oct 17, 2013 at 6:56 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Oct 16, 2013 at 10:47:06PM +0100, Tim Kryger wrote:
>> Rather than declaring the frequency of the external clock, specify the
>> label of the clock such that the driver may determine the frequency on
>> its own.
>
> Nit: we're not specifying the label of the clock. Clocks are represented
> py a phand+args pair, and the important part is that we're specifying
> the clock itself.
>
> How about something like:
>
> ---->8----
> Currently the rate of the external clock input to "snps,dw-apb-uart"
> devices is described by a clock-frequency property rather than by
> reference to the clock itself.
>
> This patch changes the "snps,dw-apb-uart" entries in bcm11351.dtsi to
> refer to the parent clock directly, following the common clock bindings.
> ---->8----

Would this be acceptable?

The frequency property in "snps,dw-apb-uart" entries are no longer
required if the rate of the external clock can be determined using the
clk api (see e302cd9 serial: 8250_dw: add support for clk api).

This patch replaces the frequency property in the UART nodes of
bcm11351.dtsi with references to the relevant clocks following the
common clock binding.

>
> Also, this should probably be moved after the driver change, so as to
> not be unbootable temporarily.

The driver change was part of v3.10 so there is no issue here.

>
> Thanks,
> Mark.
>
>>
>> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
>> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
>> Reviewed-by: Matt Porter <matt.porter@linaro.org>
>> ---
>>  arch/arm/boot/dts/bcm11351.dtsi | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
>> index c6464fb..ce65367 100644
>> --- a/arch/arm/boot/dts/bcm11351.dtsi
>> +++ b/arch/arm/boot/dts/bcm11351.dtsi
>> @@ -43,7 +43,7 @@
>>               compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>>               status = "disabled";
>>               reg = <0x3e000000 0x1000>;
>> -             clock-frequency = <13000000>;
>> +             clocks = <&uartb_clk>;
>>               interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
>>               reg-shift = <2>;
>>               reg-io-width = <4>;
>> @@ -53,7 +53,7 @@
>>               compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>>               status = "disabled";
>>               reg = <0x3e001000 0x1000>;
>> -             clock-frequency = <13000000>;
>> +             clocks = <&uartb2_clk>;
>>               interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
>>               reg-shift = <2>;
>>               reg-io-width = <4>;
>> @@ -63,7 +63,7 @@
>>               compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>>               status = "disabled";
>>               reg = <0x3e002000 0x1000>;
>> -             clock-frequency = <13000000>;
>> +             clocks = <&uartb3_clk>;
>>               interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
>>               reg-shift = <2>;
>>               reg-io-width = <4>;
>> @@ -73,7 +73,7 @@
>>               compatible = "brcm,bcm11351-dw-apb-uart", "snps,dw-apb-uart";
>>               status = "disabled";
>>               reg = <0x3e003000 0x1000>;
>> -             clock-frequency = <13000000>;
>> +             clocks = <&uartb4_clk>;
>>               interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>>               reg-shift = <2>;
>>               reg-io-width = <4>;
>> --
>> 1.8.0.1
>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[RESEND PATCH v2 3/6] ARM: dts: Specify clocks for SDHCIs on bcm11351

[Tim Kryger]

Specify the external clock label in each SDHCI node.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 arch/arm/boot/dts/bcm11351.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
index ce65367..193659c 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -101,6 +101,7 @@
 	sdio1: sdio@3f180000 {
 		compatible = "brcm,kona-sdhci";
 		reg = <0x3f180000 0x10000>;
+		clocks = <&sdio1_clk>;
 		interrupts = <0x0 77 0x4>;
 		status = "disabled";
 	};
@@ -108,6 +109,7 @@
 	sdio2: sdio@3f190000 {
 		compatible = "brcm,kona-sdhci";
 		reg = <0x3f190000 0x10000>;
+		clocks = <&sdio2_clk>;
 		interrupts = <0x0 76 0x4>;
 		status = "disabled";
 	};
@@ -115,6 +117,7 @@
 	sdio3: sdio@3f1a0000 {
 		compatible = "brcm,kona-sdhci";
 		reg = <0x3f1a0000 0x10000>;
+		clocks = <&sdio3_clk>;
 		interrupts = <0x0 74 0x4>;
 		status = "disabled";
 	};
@@ -122,6 +125,7 @@
 	sdio4: sdio@3f1b0000 {
 		compatible = "brcm,kona-sdhci";
 		reg = <0x3f1b0000 0x10000>;
+		clocks = <&sdio4_clk>;
 		interrupts = <0x0 73 0x4>;
 		status = "disabled";
 	};
-- 
1.8.0.1

[RESEND PATCH v2 4/6] clocksource: kona: Add basic use of external clock

[Tim Kryger]

When an clock handle is specified in the device tree, enable it and use
it to determine the external clock frequency.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 drivers/clocksource/bcm_kona_timer.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c
index 0d7d8c3..fd11f96 100644
--- a/drivers/clocksource/bcm_kona_timer.c
+++ b/drivers/clocksource/bcm_kona_timer.c
@@ -17,6 +17,7 @@
 #include <linux/jiffies.h>
 #include <linux/clockchips.h>
 #include <linux/types.h>
+#include <linux/clk.h>
 
 #include <linux/io.h>
 #include <asm/mach/time.h>
@@ -107,11 +108,18 @@ static const struct of_device_id bcm_timer_ids[] __initconst = {
 static void __init kona_timers_init(struct device_node *node)
 {
 	u32 freq;
+	struct clk *external_clk;
 
-	if (!of_property_read_u32(node, "clock-frequency", &freq))
+	external_clk = of_clk_get_by_name(node, NULL);
+
+	if (!IS_ERR(external_clk)) {
+		arch_timer_rate = clk_get_rate(external_clk);
+		clk_prepare_enable(external_clk);
+	} else if (!of_property_read_u32(node, "clock-frequency", &freq)) {
 		arch_timer_rate = freq;
-	else
-		panic("clock-frequency not set in the .dts file");
+	} else {
+		panic("neither clock-frequency or clocks handle in .dts file");
+	}
 
 	/* Setup IRQ numbers */
 	timers.tmr_irq = irq_of_parse_and_map(node, 0);
-- 
1.8.0.1

Re: [RESEND PATCH v2 4/6] clocksource: kona: Add basic use of external clock

[Mark Rutland]

On Wed, Oct 16, 2013 at 10:47:08PM +0100, Tim Kryger wrote:
> When an clock handle is specified in the device tree, enable it and use
> it to determine the external clock frequency.

I'd drop handle here and just say "When a clock is specified".

This will need a binding document update.

> 
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---
>  drivers/clocksource/bcm_kona_timer.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c
> index 0d7d8c3..fd11f96 100644
> --- a/drivers/clocksource/bcm_kona_timer.c
> +++ b/drivers/clocksource/bcm_kona_timer.c
> @@ -17,6 +17,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/clockchips.h>
>  #include <linux/types.h>
> +#include <linux/clk.h>
>  
>  #include <linux/io.h>
>  #include <asm/mach/time.h>
> @@ -107,11 +108,18 @@ static const struct of_device_id bcm_timer_ids[] __initconst = {
>  static void __init kona_timers_init(struct device_node *node)
>  {
>  	u32 freq;
> +	struct clk *external_clk;
>  
> -	if (!of_property_read_u32(node, "clock-frequency", &freq))
> +	external_clk = of_clk_get_by_name(node, NULL);

Is there only a single external clock input to the kona timer, or is
only one relevant here?

Are there any other inputs currently not modelled (e.g. regulators)?

> +
> +	if (!IS_ERR(external_clk)) {
> +		arch_timer_rate = clk_get_rate(external_clk);
> +		clk_prepare_enable(external_clk);
> +	} else if (!of_property_read_u32(node, "clock-frequency", &freq)) {
>  		arch_timer_rate = freq;
> -	else
> -		panic("clock-frequency not set in the .dts file");
> +	} else {
> +		panic("neither clock-frequency or clocks handle in .dts file");

Nit: it's not a handle, it's a phandle+args pair.

Why not just "Unable to determine clock frequency"?

Do we need to panic here? Might a system have other clocks it could use
to continue?

Thanks,
Mark.

Re: [RESEND PATCH v2 4/6] clocksource: kona: Add basic use of external clock

[Tim Kryger]

On Thu, Oct 17, 2013 at 7:05 AM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Wed, Oct 16, 2013 at 10:47:08PM +0100, Tim Kryger wrote:
>> When an clock handle is specified in the device tree, enable it and use
>> it to determine the external clock frequency.
>
> I'd drop handle here and just say "When a clock is specified".
>
> This will need a binding document update.

Can you be more specific about what you are looking for here?

Shall I just say that the frequency property is now deprecated since
it actually belongs to the external clock whose association with a
kona timer instance might be declared in the DT using the common clock
bindings but could also be declared through other means?

>> Signed-off-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Reviewed-by: Matt Porter <matt.porter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/clocksource/bcm_kona_timer.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/bcm_kona_timer.c b/drivers/clocksource/bcm_kona_timer.c
>> index 0d7d8c3..fd11f96 100644
>> --- a/drivers/clocksource/bcm_kona_timer.c
>> +++ b/drivers/clocksource/bcm_kona_timer.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/jiffies.h>
>>  #include <linux/clockchips.h>
>>  #include <linux/types.h>
>> +#include <linux/clk.h>
>>
>>  #include <linux/io.h>
>>  #include <asm/mach/time.h>
>> @@ -107,11 +108,18 @@ static const struct of_device_id bcm_timer_ids[] __initconst = {
>>  static void __init kona_timers_init(struct device_node *node)
>>  {
>>       u32 freq;
>> +     struct clk *external_clk;
>>
>> -     if (!of_property_read_u32(node, "clock-frequency", &freq))
>> +     external_clk = of_clk_get_by_name(node, NULL);
>
> Is there only a single external clock input to the kona timer, or is
> only one relevant here?
>
> Are there any other inputs currently not modelled (e.g. regulators)?

The timer block relies upon a single clock and does not use any regulators.

>> +
>> +     if (!IS_ERR(external_clk)) {
>> +             arch_timer_rate = clk_get_rate(external_clk);
>> +             clk_prepare_enable(external_clk);
>> +     } else if (!of_property_read_u32(node, "clock-frequency", &freq)) {
>>               arch_timer_rate = freq;
>> -     else
>> -             panic("clock-frequency not set in the .dts file");
>> +     } else {
>> +             panic("neither clock-frequency or clocks handle in .dts file");
>
> Nit: it's not a handle, it's a phandle+args pair.
>
> Why not just "Unable to determine clock frequency"?

Sure that sounds fine.

>
> Do we need to panic here? Might a system have other clocks it could use
> to continue?

I completely agree that this driver could be improved.  However, this
is unrelated to the topic of this series so I will address it later in
a separate patch.
--
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

[RESEND PATCH v2 5/6] ARM: dts: Specify clocks for timer on bcm11351

[Tim Kryger]

Specify the external clock label in the timer node.

Signed-off-by: Tim Kryger <tim.kryger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Markus Mayer <markus.mayer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Reviewed-by: Matt Porter <matt.porter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/bcm11351.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
index 193659c..39c1395 100644
--- a/arch/arm/boot/dts/bcm11351.dtsi
+++ b/arch/arm/boot/dts/bcm11351.dtsi
@@ -95,7 +95,7 @@
 		compatible = "brcm,kona-timer";
 		reg = <0x35006000 0x1000>;
 		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
-		clock-frequency = <32768>;
+		clocks = <&hub_timer_clk>;
 	};
 
 	sdio1: sdio@3f180000 {
-- 
1.8.0.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

Re: [RESEND PATCH v2 5/6] ARM: dts: Specify clocks for timer on bcm11351

[Mark Rutland]

> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
> index 193659c..39c1395 100644
> --- a/arch/arm/boot/dts/bcm11351.dtsi
> +++ b/arch/arm/boot/dts/bcm11351.dtsi
> @@ -95,7 +95,7 @@
>  		compatible = "brcm,kona-timer";
>  		reg = <0x35006000 0x1000>;
>  		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> -		clock-frequency = <32768>;
> +		clocks = <&hub_timer_clk>;

This should probably be after the code change that enables support for a
clocks property.

Thanks,
Mark.

Re: [RESEND PATCH v2 5/6] ARM: dts: Specify clocks for timer on bcm11351

[Tim Kryger]

On Thu, Oct 17, 2013 at 7:06 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> diff --git a/arch/arm/boot/dts/bcm11351.dtsi b/arch/arm/boot/dts/bcm11351.dtsi
>> index 193659c..39c1395 100644
>> --- a/arch/arm/boot/dts/bcm11351.dtsi
>> +++ b/arch/arm/boot/dts/bcm11351.dtsi
>> @@ -95,7 +95,7 @@
>>               compatible = "brcm,kona-timer";
>>               reg = <0x35006000 0x1000>;
>>               interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>> -             clock-frequency = <32768>;
>> +             clocks = <&hub_timer_clk>;
>
> This should probably be after the code change that enables support for a
> clocks property.
>
> Thanks,
> Mark.

This is already the case.

The driver change is patch 4 and this dts change is patch 5 of the series.

Re: [RESEND PATCH v2 0/6] Update Kona drivers to use clocks

[Mark Rutland]

On Wed, Oct 16, 2013 at 10:47:04PM +0100, Tim Kryger wrote:
> This series declares the clocks present on bcm11351 (aka bcm281xx) SoCs
> to be fixed at the rates configured by development bootloaders and then
> updates the Kona drivers to make use of clock calls where appropriate.
> 
> This patch series depends on the following three commits being present:
> 
> https://lkml.org/lkml/2013/9/18/409
> https://lkml.org/lkml/2013/9/20/305
> http://www.spinics.net/lists/arm-kernel/msg274973.html
> 
> Changes in v2:
>   - Renamed bsc4_clk to pmu_bsc_clk
> 
> Tim Kryger (6):
>   ARM: dts: Declare clocks as fixed on bcm11351
>   ARM: dts: Specify clocks for UARTs on bcm11351
>   ARM: dts: Specify clocks for SDHCIs on bcm11351
>   clocksource: kona: Add basic use of external clock
>   ARM: dts: Specify clocks for timer on bcm11351
>   mmc: sdhci-bcm-kona: Add basic use of clocks
> 
>  arch/arm/boot/dts/bcm11351.dtsi      | 111 +++++++++++++++++++++++++++++++++--
>  drivers/clocksource/bcm_kona_timer.c |  14 ++++-
>  drivers/mmc/host/sdhci-bcm-kona.c    |  30 +++++++++-

This requires a description in the binding documents, so we can figure
out which clocks we need to describe, and how to describe them.

Thanks,
Mark.
--
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

[RESEND PATCH v2 6/6] mmc: sdhci-bcm-kona: Add basic use of clocks

[Tim Kryger]

Enable the external clock needed by the host controller during the
probe and disable it during the remove.

Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
Reviewed-by: Matt Porter <matt.porter@linaro.org>
---
 drivers/mmc/host/sdhci-bcm-kona.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-bcm-kona.c b/drivers/mmc/host/sdhci-bcm-kona.c
index 85472d3..91db099 100644
--- a/drivers/mmc/host/sdhci-bcm-kona.c
+++ b/drivers/mmc/host/sdhci-bcm-kona.c
@@ -54,6 +54,7 @@
 
 struct sdhci_bcm_kona_dev {
 	struct mutex	write_lock; /* protect back to back writes */
+	struct clk	*external_clk;
 };
 
 
@@ -252,11 +253,29 @@ static int sdhci_bcm_kona_probe(struct platform_device *pdev)
 	mmc_of_parse(host->mmc);
 
 	if (!host->mmc->f_max) {
-		dev_err(&pdev->dev, "Missing max-freq for SDHCI cfg\n");
+		dev_err(dev, "Missing max-freq for SDHCI cfg\n");
 		ret = -ENXIO;
 		goto err_pltfm_free;
 	}
 
+	/* Get and enable the external clock */
+	kona_dev->external_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(kona_dev->external_clk)) {
+		dev_err(dev, "Failed to get external clock\n");
+		ret = PTR_ERR(kona_dev->external_clk);
+		goto err_pltfm_free;
+	}
+
+	if (clk_set_rate(kona_dev->external_clk, host->mmc->f_max) != 0) {
+		dev_err(dev, "Failed to set rate external clock\n");
+		goto err_pltfm_free;
+	}
+
+	if (clk_prepare_enable(kona_dev->external_clk) != 0) {
+		dev_err(dev, "Failed to enable external clock\n");
+		goto err_pltfm_free;
+	}
+
 	dev_dbg(dev, "non-removable=%c\n",
 		(host->mmc->caps & MMC_CAP_NONREMOVABLE) ? 'Y' : 'N');
 	dev_dbg(dev, "cd_gpio %c, wp_gpio %c\n",
@@ -271,7 +290,7 @@ static int sdhci_bcm_kona_probe(struct platform_device *pdev)
 
 	ret = sdhci_bcm_kona_sd_reset(host);
 	if (ret)
-		goto err_pltfm_free;
+		goto err_clk_disable;
 
 	sdhci_bcm_kona_sd_init(host);
 
@@ -307,6 +326,9 @@ err_remove_host:
 err_reset:
 	sdhci_bcm_kona_sd_reset(host);
 
+err_clk_disable:
+	clk_disable_unprepare(kona_dev->external_clk);
+
 err_pltfm_free:
 	sdhci_pltfm_free(pdev);
 
@@ -317,6 +339,8 @@ err_pltfm_free:
 static int __exit sdhci_bcm_kona_remove(struct platform_device *pdev)
 {
 	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_priv = sdhci_priv(host);
+	struct sdhci_bcm_kona_dev *kona_dev = sdhci_pltfm_priv(pltfm_priv);
 	int dead;
 	u32 scratch;
 
@@ -326,6 +350,8 @@ static int __exit sdhci_bcm_kona_remove(struct platform_device *pdev)
 		dead = 1;
 	sdhci_remove_host(host, dead);
 
+	clk_disable_unprepare(kona_dev->external_clk);
+
 	sdhci_free_host(host);
 
 	return 0;
-- 
1.8.0.1

Re: [RESEND PATCH v2 6/6] mmc: sdhci-bcm-kona: Add basic use of clocks

[Mark Rutland]

On Wed, Oct 16, 2013 at 10:47:10PM +0100, Tim Kryger wrote:
> Enable the external clock needed by the host controller during the
> probe and disable it during the remove.

This requires a biding document update.

I note that the binding is already incomplete (it does not describe the
interrupts or reg).

> 
> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
> Reviewed-by: Matt Porter <matt.porter@linaro.org>
> ---
>  drivers/mmc/host/sdhci-bcm-kona.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-bcm-kona.c b/drivers/mmc/host/sdhci-bcm-kona.c
> index 85472d3..91db099 100644
> --- a/drivers/mmc/host/sdhci-bcm-kona.c
> +++ b/drivers/mmc/host/sdhci-bcm-kona.c
> @@ -54,6 +54,7 @@
>  
>  struct sdhci_bcm_kona_dev {
>  	struct mutex	write_lock; /* protect back to back writes */
> +	struct clk	*external_clk;

Is this the only clock input the unit has?

Are there any other external resources the device needs (e.g. gpios,
regulators)?

>  };
>  
>  
> @@ -252,11 +253,29 @@ static int sdhci_bcm_kona_probe(struct platform_device *pdev)
>  	mmc_of_parse(host->mmc);
>  
>  	if (!host->mmc->f_max) {
> -		dev_err(&pdev->dev, "Missing max-freq for SDHCI cfg\n");
> +		dev_err(dev, "Missing max-freq for SDHCI cfg\n");

This seems like an unrelated change.

>  		ret = -ENXIO;
>  		goto err_pltfm_free;
>  	}
>  
> +	/* Get and enable the external clock */
> +	kona_dev->external_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(kona_dev->external_clk)) {
> +		dev_err(dev, "Failed to get external clock\n");
> +		ret = PTR_ERR(kona_dev->external_clk);
> +		goto err_pltfm_free;

This seems like a pretty clear breakage of existing DTBs.

Why?

Thanks,
Mark.

Re: [RESEND PATCH v2 6/6] mmc: sdhci-bcm-kona: Add basic use of clocks

[Matt Porter]

On Thu, Oct 17, 2013 at 03:13:09PM +0100, Mark Rutland wrote:
> On Wed, Oct 16, 2013 at 10:47:10PM +0100, Tim Kryger wrote:
> > +	/* Get and enable the external clock */
> > +	kona_dev->external_clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(kona_dev->external_clk)) {
> > +		dev_err(dev, "Failed to get external clock\n");
> > +		ret = PTR_ERR(kona_dev->external_clk);
> > +		goto err_pltfm_free;
> 
> This seems like a pretty clear breakage of existing DTBs.
> 
> Why?

Normal kernel incremental development. The notion of breaking DTBs is
pretty much incompatible with the long-standing tradition of developing
kernel support in an incremental fashion.

In this case, the bootloader available on these boards has had clocks
and regulators enabled for all the drivers currently upstream. This
allowed development of the sdhci driver (and others) independent of having
the common clk driver and PMU/regulator drivers ready. That's been especially
important given the flux that the common clk framework is in with regards
to bindings.

Are you suggesting that the required clock needs to be made logically
optional in the code to retain backward compatbility?

-Matt

Re: [RESEND PATCH v2 6/6] mmc: sdhci-bcm-kona: Add basic use of clocks

[Tim Kryger]

On Thu, Oct 17, 2013 at 7:13 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Oct 16, 2013 at 10:47:10PM +0100, Tim Kryger wrote:
>> Enable the external clock needed by the host controller during the
>> probe and disable it during the remove.
>
> This requires a biding document update.

The current best practice for declaring the association of a clock
with a consumer device is to embed the common clock bindings 'clocks'
property inside its node but this is not the only way it may be done.

Given that clk_get() still works for clocks found using string
matching, is it appropriate to mandate that all drivers that uses the
clk api mention the common clock bindings 'clocks' property in the
documentation for their device's binding?

> I note that the binding is already incomplete (it does not describe the
> interrupts or reg).

The current document reflects the standard for
Documentation/devicetree/bindings/mmc where normal properties are
described in mmc.txt and only the deviations described in vendor
specific files.

>>
>> Signed-off-by: Tim Kryger <tim.kryger@linaro.org>
>> Reviewed-by: Markus Mayer <markus.mayer@linaro.org>
>> Reviewed-by: Matt Porter <matt.porter@linaro.org>
>> ---
>>  drivers/mmc/host/sdhci-bcm-kona.c | 30 ++++++++++++++++++++++++++++--
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-bcm-kona.c b/drivers/mmc/host/sdhci-bcm-kona.c
>> index 85472d3..91db099 100644
>> --- a/drivers/mmc/host/sdhci-bcm-kona.c
>> +++ b/drivers/mmc/host/sdhci-bcm-kona.c
>> @@ -54,6 +54,7 @@
>>
>>  struct sdhci_bcm_kona_dev {
>>       struct mutex    write_lock; /* protect back to back writes */
>> +     struct clk      *external_clk;
>
> Is this the only clock input the unit has?

The SDHCI block only receives one clock.

> Are there any other external resources the device needs (e.g. gpios,
> regulators)?

Yes but the cd-gpio and vmmc/vqmmc regulators are handled by the
common SDHCI driver.

>
>>  };
>>
>>
>> @@ -252,11 +253,29 @@ static int sdhci_bcm_kona_probe(struct platform_device *pdev)
>>       mmc_of_parse(host->mmc);
>>
>>       if (!host->mmc->f_max) {
>> -             dev_err(&pdev->dev, "Missing max-freq for SDHCI cfg\n");
>> +             dev_err(dev, "Missing max-freq for SDHCI cfg\n");
>
> This seems like an unrelated change.

Agreed.  I will remove this change.

>>               ret = -ENXIO;
>>               goto err_pltfm_free;
>>       }
>>
>> +     /* Get and enable the external clock */
>> +     kona_dev->external_clk = devm_clk_get(dev, NULL);
>> +     if (IS_ERR(kona_dev->external_clk)) {
>> +             dev_err(dev, "Failed to get external clock\n");
>> +             ret = PTR_ERR(kona_dev->external_clk);
>> +             goto err_pltfm_free;
>
> This seems like a pretty clear breakage of existing DTBs.
>
> Why?

The defconfig that builds this driver and DTBs currently sets
CONFIG_ARM_APPENDED_DTB=y so there isn't any actual breakage risk.