Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Jonas Jensen]

On 20 June 2017 at 10:06, Linus Walleij  wrote:
> Moxart is deploying the DTBs with the kernel, they go hand-in-hand.
> Backward compatibility with old DTBs here would just be an academic
> exercise.

I've used CONFIG_ARM_APPENDED_DTB=y since the beginning mainly because
UC-7112-LX's proprietary bootloader lacks DTB capabilities (that I'm
aware of).
Compiling both (kernel and dtb) at the same time always made sense to
me, and it does, even when the hardware has a sane bootloader (i.e.
DTB enabled).


   Jonas

Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Jonas Jensen]

On 20 June 2017 at 10:06, Linus Walleij  wrote:
> Moxart is deploying the DTBs with the kernel, they go hand-in-hand.
> Backward compatibility with old DTBs here would just be an academic
> exercise.

I've used CONFIG_ARM_APPENDED_DTB=y since the beginning mainly because
UC-7112-LX's proprietary bootloader lacks DTB capabilities (that I'm
aware of).
Compiling both (kernel and dtb) at the same time always made sense to
me, and it does, even when the hardware has a sane bootloader (i.e.
DTB enabled).


   Jonas

Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Linus Walleij]

On Fri, Jun 9, 2017 at 2:52 PM, Andrew Jeffery  wrote:

> So the fttmr010 bindings describe the clocks and clock-names properties
> as optional (a little confusingly, "Optionally required properties").

We should remove that. The timer frequency is strictly required.

> I guess keeping in mind the bindings describe the hardware and not the
> driver this might be reasonable, but the driver fails init if they're
> not present. arch/arm/boot/dts/moxart.dtsi doesn't specify clock-names
> either so I would have thought systems based on it would also fail.

It was added in
commit f46b563f2f270e451b2e1cee78573508cc1de256
"ARM: dts: augment Moxa and Aspeed DTS for FTTMR010"

Moxart only uses DT for boot, and Jonas controls all deployments of the
mainline kernel.

> Regardless, if it's the case that Moxa systems now fail to init the
> clocksource then the Aspeed-specific init_time() solution is even less
> attractive. moxart.dtsi dates back to December 2013 ("448e7edefa92 ARM:
> moxart: add MOXA ART SoC device tree files").

Moxart is deploying the DTBs with the kernel, they go hand-in-hand.
Backward compatibility with old DTBs here would just be an academic
exercise.

> The old binding is less of a problem for Aspeed systems as we don't yet
> have a clk driver upstream. Joel only recently added fixed-clock nodes
> in 4.12 so Aspeed systems could boot without DTS modifications.

The mentioned commit also adds the clocks to Aspeed AFAICT, is there
any problem in the real world, like did I miss some Aspeed platform?

Patching around drivers to make old DTBs work is something we should
only do when there are wide deployments for common users, such as people
buying products with a DTB inside them. Until such devices are shipped,
I consider the DT bindings still in flux.

Yours,
Linus Walleij

Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Linus Walleij]

On Fri, Jun 9, 2017 at 2:52 PM, Andrew Jeffery  wrote:

> So the fttmr010 bindings describe the clocks and clock-names properties
> as optional (a little confusingly, "Optionally required properties").

We should remove that. The timer frequency is strictly required.

> I guess keeping in mind the bindings describe the hardware and not the
> driver this might be reasonable, but the driver fails init if they're
> not present. arch/arm/boot/dts/moxart.dtsi doesn't specify clock-names
> either so I would have thought systems based on it would also fail.

It was added in
commit f46b563f2f270e451b2e1cee78573508cc1de256
"ARM: dts: augment Moxa and Aspeed DTS for FTTMR010"

Moxart only uses DT for boot, and Jonas controls all deployments of the
mainline kernel.

> Regardless, if it's the case that Moxa systems now fail to init the
> clocksource then the Aspeed-specific init_time() solution is even less
> attractive. moxart.dtsi dates back to December 2013 ("448e7edefa92 ARM:
> moxart: add MOXA ART SoC device tree files").

Moxart is deploying the DTBs with the kernel, they go hand-in-hand.
Backward compatibility with old DTBs here would just be an academic
exercise.

> The old binding is less of a problem for Aspeed systems as we don't yet
> have a clk driver upstream. Joel only recently added fixed-clock nodes
> in 4.12 so Aspeed systems could boot without DTS modifications.

The mentioned commit also adds the clocks to Aspeed AFAICT, is there
any problem in the real world, like did I miss some Aspeed platform?

Patching around drivers to make old DTBs work is something we should
only do when there are wide deployments for common users, such as people
buying products with a DTB inside them. Until such devices are shipped,
I consider the DT bindings still in flux.

Yours,
Linus Walleij

Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Andrew Jeffery]

On Fri, 2017-06-09 at 11:19 +0200, Arnd Bergmann wrote:
> > On Fri, Jun 9, 2017 at 9:30 AM, Andrew Jeffery  wrote:
> > Add the clock-names property in init_timer() to work-around Aspeed
> > devicetrees from times prior to merging the Moxart/Aspeed and Faraday
> > drivers.
> > 
> > > > Signed-off-by: Andrew Jeffery 
> > ---
> > Well, here's an implementation I knocked up. It's a fair chunk of code for
> > marginal benefit. Joel is against it.
> > 
> > At least it's something to debate.
> > 
> > Tested under QEMU for both AST2400 and AST2500 SoCs.
> 
> Right, I think having the one-line fixup in the driver makes way more sense
> here, and that is what we usually do, but we could do this if Daniel has good
> reasons to keep the driver free of backwards-compatibility support.

Yeah, choosing between the two with no further information I'd prefer
adding support to the driver.

> 
> It also depends a bit on how common the old binding version already
> is, and if anyone is shipping systems with that.

So the fttmr010 bindings describe the clocks and clock-names properties
as optional (a little confusingly, "Optionally required properties"). 
I guess keeping in mind the bindings describe the hardware and not the
driver this might be reasonable, but the driver fails init if they're
not present. arch/arm/boot/dts/moxart.dtsi doesn't specify clock-names
either so I would have thought systems based on it would also fail.
However, Linus' fttmr010 series has Tested-by tags from Jonas, so maybe
I've missed something.

Regardless, if it's the case that Moxa systems now fail to init the
clocksource then the Aspeed-specific init_time() solution is even less
attractive. moxart.dtsi dates back to December 2013 ("448e7edefa92 ARM:
moxart: add MOXA ART SoC device tree files").

The old binding is less of a problem for Aspeed systems as we don't yet
have a clk driver upstream. Joel only recently added fixed-clock nodes
in 4.12 so Aspeed systems could boot without DTS modifications.

Andrew

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Andrew Jeffery]

On Fri, 2017-06-09 at 11:19 +0200, Arnd Bergmann wrote:
> > On Fri, Jun 9, 2017 at 9:30 AM, Andrew Jeffery  wrote:
> > Add the clock-names property in init_timer() to work-around Aspeed
> > devicetrees from times prior to merging the Moxart/Aspeed and Faraday
> > drivers.
> > 
> > > > Signed-off-by: Andrew Jeffery 
> > ---
> > Well, here's an implementation I knocked up. It's a fair chunk of code for
> > marginal benefit. Joel is against it.
> > 
> > At least it's something to debate.
> > 
> > Tested under QEMU for both AST2400 and AST2500 SoCs.
> 
> Right, I think having the one-line fixup in the driver makes way more sense
> here, and that is what we usually do, but we could do this if Daniel has good
> reasons to keep the driver free of backwards-compatibility support.

Yeah, choosing between the two with no further information I'd prefer
adding support to the driver.

> 
> It also depends a bit on how common the old binding version already
> is, and if anyone is shipping systems with that.

So the fttmr010 bindings describe the clocks and clock-names properties
as optional (a little confusingly, "Optionally required properties"). 
I guess keeping in mind the bindings describe the hardware and not the
driver this might be reasonable, but the driver fails init if they're
not present. arch/arm/boot/dts/moxart.dtsi doesn't specify clock-names
either so I would have thought systems based on it would also fail.
However, Linus' fttmr010 series has Tested-by tags from Jonas, so maybe
I've missed something.

Regardless, if it's the case that Moxa systems now fail to init the
clocksource then the Aspeed-specific init_time() solution is even less
attractive. moxart.dtsi dates back to December 2013 ("448e7edefa92 ARM:
moxart: add MOXA ART SoC device tree files").

The old binding is less of a problem for Aspeed systems as we don't yet
have a clk driver upstream. Joel only recently added fixed-clock nodes
in 4.12 so Aspeed systems could boot without DTS modifications.

Andrew

signature.asc
Description: This is a digitally signed message part

Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Arnd Bergmann]

On Fri, Jun 9, 2017 at 9:30 AM, Andrew Jeffery  wrote:
> Add the clock-names property in init_timer() to work-around Aspeed
> devicetrees from times prior to merging the Moxart/Aspeed and Faraday
> drivers.
>
> Signed-off-by: Andrew Jeffery 
> ---
> Well, here's an implementation I knocked up. It's a fair chunk of code for
> marginal benefit. Joel is against it.
>
> At least it's something to debate.
>
> Tested under QEMU for both AST2400 and AST2500 SoCs.

Right, I think having the one-line fixup in the driver makes way more sense
here, and that is what we usually do, but we could do this if Daniel has good
reasons to keep the driver free of backwards-compatibility support.

It also depends a bit on how common the old binding version already
is, and if anyone is shipping systems with that.

   Arnd

Re: [PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Arnd Bergmann]

On Fri, Jun 9, 2017 at 9:30 AM, Andrew Jeffery  wrote:
> Add the clock-names property in init_timer() to work-around Aspeed
> devicetrees from times prior to merging the Moxart/Aspeed and Faraday
> drivers.
>
> Signed-off-by: Andrew Jeffery 
> ---
> Well, here's an implementation I knocked up. It's a fair chunk of code for
> marginal benefit. Joel is against it.
>
> At least it's something to debate.
>
> Tested under QEMU for both AST2400 and AST2500 SoCs.

Right, I think having the one-line fixup in the driver makes way more sense
here, and that is what we usually do, but we could do this if Daniel has good
reasons to keep the driver free of backwards-compatibility support.

It also depends a bit on how common the old binding version already
is, and if anyone is shipping systems with that.

   Arnd

[PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Andrew Jeffery]

Add the clock-names property in init_timer() to work-around Aspeed
devicetrees from times prior to merging the Moxart/Aspeed and Faraday
drivers.

Signed-off-by: Andrew Jeffery 
---
Well, here's an implementation I knocked up. It's a fair chunk of code for
marginal benefit. Joel is against it.

At least it's something to debate.

Tested under QEMU for both AST2400 and AST2500 SoCs.

Cheers,

Andrew

 arch/arm/Makefile |  1 +
 arch/arm/mach-aspeed/Makefile |  1 +
 arch/arm/mach-aspeed/aspeed.c | 73 +++
 3 files changed, 75 insertions(+)
 create mode 100644 arch/arm/mach-aspeed/Makefile
 create mode 100644 arch/arm/mach-aspeed/aspeed.c

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index ab30cc634d02..f3ed359e5b28 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -154,6 +154,7 @@ textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
 machine-$(CONFIG_ARCH_ALPINE)  += alpine
 machine-$(CONFIG_ARCH_ARTPEC)  += artpec
 machine-$(CONFIG_ARCH_AT91)+= at91
+machine-$(CONFIG_ARCH_ASPEED)  += aspeed
 machine-$(CONFIG_ARCH_AXXIA)   += axxia
 machine-$(CONFIG_ARCH_BCM) += bcm
 machine-$(CONFIG_ARCH_BERLIN)  += berlin
diff --git a/arch/arm/mach-aspeed/Makefile b/arch/arm/mach-aspeed/Makefile
new file mode 100644
index ..de8cd76fcf5d
--- /dev/null
+++ b/arch/arm/mach-aspeed/Makefile
@@ -0,0 +1 @@
+obj-y   += aspeed.o
diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c
new file mode 100644
index ..feaac8eb5d2d
--- /dev/null
+++ b/arch/arm/mach-aspeed/aspeed.c
@@ -0,0 +1,73 @@
+/*
+ * Copyright 2017 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+const char *aspeed_timer_compatibles[] = {
+   "aspeed,ast2400-timer",
+   "aspeed,ast2500-timer",
+   NULL,
+};
+
+/*
+ * For backwards compatibility with pre-4.13 devicetrees, populate the
+ * clock-names property in the clocksource node
+ */
+static void __init aspeed_timer_set_clock_names(void)
+{
+   const char **compatible = aspeed_timer_compatibles;
+   struct device_node *np;
+
+   while (*compatible) {
+   for_each_compatible_node(np, NULL, *compatible) {
+   struct property *clock_names;
+   int rc;
+
+   rc = of_property_count_strings(np, "clock-names");
+   if (rc != -EINVAL)
+   continue;
+
+   clock_names = kzalloc(sizeof(*clock_names), GFP_KERNEL);
+
+   clock_names->name = kstrdup("clock-names", GFP_KERNEL);
+   clock_names->length = sizeof("PCLK");
+   clock_names->value = kstrdup("PCLK", GFP_KERNEL);
+
+   of_add_property(np, clock_names);
+   }
+
+   compatible++;
+   }
+}
+
+static void __init aspeed_init_time(void)
+{
+   aspeed_timer_set_clock_names();
+
+#ifdef CONFIG_COMMON_CLK
+   of_clk_init(NULL);
+#endif
+   timer_probe();
+}
+
+static const char *const aspeed_dt_match[] __initconst = {
+   "aspeed,ast2400",
+   "aspeed,ast2500",
+   NULL,
+};
+
+DT_MACHINE_START(aspeed_dt, "Aspeed SoC")
+   .init_time  = aspeed_init_time,
+   .dt_compat  = aspeed_dt_match,
+MACHINE_END
-- 
2.11.0

[PATCH] arm: aspeed: Add Aspeed board file with clocksource devicetree fixup

[Andrew Jeffery]

Add the clock-names property in init_timer() to work-around Aspeed
devicetrees from times prior to merging the Moxart/Aspeed and Faraday
drivers.

Signed-off-by: Andrew Jeffery 
---
Well, here's an implementation I knocked up. It's a fair chunk of code for
marginal benefit. Joel is against it.

At least it's something to debate.

Tested under QEMU for both AST2400 and AST2500 SoCs.

Cheers,

Andrew

 arch/arm/Makefile |  1 +
 arch/arm/mach-aspeed/Makefile |  1 +
 arch/arm/mach-aspeed/aspeed.c | 73 +++
 3 files changed, 75 insertions(+)
 create mode 100644 arch/arm/mach-aspeed/Makefile
 create mode 100644 arch/arm/mach-aspeed/aspeed.c

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index ab30cc634d02..f3ed359e5b28 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -154,6 +154,7 @@ textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
 machine-$(CONFIG_ARCH_ALPINE)  += alpine
 machine-$(CONFIG_ARCH_ARTPEC)  += artpec
 machine-$(CONFIG_ARCH_AT91)+= at91
+machine-$(CONFIG_ARCH_ASPEED)  += aspeed
 machine-$(CONFIG_ARCH_AXXIA)   += axxia
 machine-$(CONFIG_ARCH_BCM) += bcm
 machine-$(CONFIG_ARCH_BERLIN)  += berlin
diff --git a/arch/arm/mach-aspeed/Makefile b/arch/arm/mach-aspeed/Makefile
new file mode 100644
index ..de8cd76fcf5d
--- /dev/null
+++ b/arch/arm/mach-aspeed/Makefile
@@ -0,0 +1 @@
+obj-y   += aspeed.o
diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c
new file mode 100644
index ..feaac8eb5d2d
--- /dev/null
+++ b/arch/arm/mach-aspeed/aspeed.c
@@ -0,0 +1,73 @@
+/*
+ * Copyright 2017 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+const char *aspeed_timer_compatibles[] = {
+   "aspeed,ast2400-timer",
+   "aspeed,ast2500-timer",
+   NULL,
+};
+
+/*
+ * For backwards compatibility with pre-4.13 devicetrees, populate the
+ * clock-names property in the clocksource node
+ */
+static void __init aspeed_timer_set_clock_names(void)
+{
+   const char **compatible = aspeed_timer_compatibles;
+   struct device_node *np;
+
+   while (*compatible) {
+   for_each_compatible_node(np, NULL, *compatible) {
+   struct property *clock_names;
+   int rc;
+
+   rc = of_property_count_strings(np, "clock-names");
+   if (rc != -EINVAL)
+   continue;
+
+   clock_names = kzalloc(sizeof(*clock_names), GFP_KERNEL);
+
+   clock_names->name = kstrdup("clock-names", GFP_KERNEL);
+   clock_names->length = sizeof("PCLK");
+   clock_names->value = kstrdup("PCLK", GFP_KERNEL);
+
+   of_add_property(np, clock_names);
+   }
+
+   compatible++;
+   }
+}
+
+static void __init aspeed_init_time(void)
+{
+   aspeed_timer_set_clock_names();
+
+#ifdef CONFIG_COMMON_CLK
+   of_clk_init(NULL);
+#endif
+   timer_probe();
+}
+
+static const char *const aspeed_dt_match[] __initconst = {
+   "aspeed,ast2400",
+   "aspeed,ast2500",
+   NULL,
+};
+
+DT_MACHINE_START(aspeed_dt, "Aspeed SoC")
+   .init_time  = aspeed_init_time,
+   .dt_compat  = aspeed_dt_match,
+MACHINE_END
-- 
2.11.0