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
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 <[email protected]>
Reviewed-by: Markus Mayer <[email protected]>
Reviewed-by: Matt Porter <[email protected]>
---
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
Specify the external clock label in the timer node.
Signed-off-by: Tim Kryger <[email protected]>
Reviewed-by: Markus Mayer <[email protected]>
Reviewed-by: Matt Porter <[email protected]>
---
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
Enable the external clock needed by the host controller during the
probe and disable it during the remove.
Signed-off-by: Tim Kryger <[email protected]>
Reviewed-by: Markus Mayer <[email protected]>
Reviewed-by: Matt Porter <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Markus Mayer <[email protected]>
Reviewed-by: Matt Porter <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Markus Mayer <[email protected]>
Reviewed-by: Matt Porter <[email protected]>
---
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
Specify the external clock label in each SDHCI node.
Signed-off-by: Tim Kryger <[email protected]>
Reviewed-by: Markus Mayer <[email protected]>
Reviewed-by: Matt Porter <[email protected]>
---
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
On Wed, Oct 16, 2013 at 2:47 PM, Tim Kryger <[email protected]> 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 <[email protected]>
> Reviewed-by: Markus Mayer <[email protected]>
> Reviewed-by: Matt Porter <[email protected]>
> ---
> 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
On 10/17/2013 07:41 AM, Christian Daudt wrote:
> On Wed, Oct 16, 2013 at 2:47 PM, Tim Kryger <[email protected]> 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 <[email protected]>
>> Reviewed-by: Markus Mayer <[email protected]>
>> Reviewed-by: Matt Porter <[email protected]>
>> ---
>> 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
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.
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 <[email protected]>
> Reviewed-by: Markus Mayer <[email protected]>
> Reviewed-by: Matt Porter <[email protected]>
> ---
> 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
>
>
>
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 <[email protected]>
> Reviewed-by: Markus Mayer <[email protected]>
> Reviewed-by: Matt Porter <[email protected]>
> ---
> 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.
> 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.
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 <[email protected]>
> Reviewed-by: Markus Mayer <[email protected]>
> Reviewed-by: Matt Porter <[email protected]>
> ---
> 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.
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
On Thu, Oct 17, 2013 at 7:06 AM, Mark Rutland <[email protected]> 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.
On Thu, Oct 17, 2013 at 6:56 AM, Mark Rutland <[email protected]> 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 <[email protected]>
>> Reviewed-by: Markus Mayer <[email protected]>
>> Reviewed-by: Matt Porter <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Thu, Oct 17, 2013 at 7:05 AM, Mark Rutland <[email protected]> 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 <[email protected]>
>> Reviewed-by: Markus Mayer <[email protected]>
>> Reviewed-by: Matt Porter <[email protected]>
>> ---
>> 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.
On Thu, Oct 17, 2013 at 7:13 AM, Mark Rutland <[email protected]> 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 <[email protected]>
>> Reviewed-by: Markus Mayer <[email protected]>
>> Reviewed-by: Matt Porter <[email protected]>
>> ---
>> 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.