[PATCH 0/2] Adding support to show SoC revision in /proc/cpuinfo

[PATCH 0/2] Adding support to show SoC revision in /proc/cpuinfo

[Romain Perier]

This serie adds support to retrieve the SoC serial and revision of the current
Meson8 platform. Basically, in the first patch I register a soc_device and retrieve
information from a syscon register, then I populate system_rev and system_serial.
In the second patch I add the device_node to the devicetree.

Romain Perier (2):
  ARM: meson: Adding support to retrieve serial and SoC revision
  ARM: dts: meson: Adding hwrev syscon node

 arch/arm/boot/dts/meson8b.dtsi |  5 ++++
 arch/arm/mach-meson/meson.c    | 55 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

-- 
2.5.0

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

[PATCH 1/2] ARM: meson: Adding support to retrieve serial and SoC revision

[Romain Perier]

This path adds support to get the revision and the serial of the running
SoC, on Meson8. On these plaforms, these informations can be found into
CBUS registers. To do so, we instanciate a syscon register, then create
a soc_device, and finally we expose everything to the system.

Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/mach-meson/meson.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm/mach-meson/meson.c b/arch/arm/mach-meson/meson.c
index 4e23571..2816e30 100644
--- a/arch/arm/mach-meson/meson.c
+++ b/arch/arm/mach-meson/meson.c
@@ -13,8 +13,16 @@
  *
  */
 
+#include <linux/of_address.h>
 #include <linux/of_platform.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
 #include <asm/mach/arch.h>
+#include <asm/system_info.h>
+
+#define MESON_REVISION_REG (0x45c)
 
 static const char * const meson_common_board_compat[] = {
 	"amlogic,meson6",
@@ -23,7 +31,54 @@ static const char * const meson_common_board_compat[] = {
 	NULL,
 };
 
+static void __init meson_init_machine(void)
+{
+	struct soc_device_attribute *soc_dev_attr;
+	struct soc_device *soc_dev;
+	struct regmap *hwrev;
+	unsigned int val;
+	int ret;
+
+	hwrev = syscon_regmap_lookup_by_compatible("amlogic,meson8b-hwrev");
+	if (IS_ERR(hwrev)) {
+		pr_err("hwrev node not found\n");
+		return;
+	}
+
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr)
+		return;
+	ret = regmap_read(hwrev, 0, &val);
+	if (ret < 0) {
+		pr_err("Could not get SoC id\n");
+		return;
+	}
+	system_serial_high = val << 24;
+
+	ret = regmap_read(hwrev, MESON_REVISION_REG, &val);
+	if (ret < 0) {
+		pr_err("Could not get SoC revision\n");
+		return;
+	}
+	system_rev = val == 0x11111111 ? 0xA : 0xB;
+
+	soc_dev_attr->family = "Amlogic Meson";
+	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "0x%x", system_rev);
+	soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", system_serial_high);
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		pr_err("Could not register soc device\n");
+		kfree(soc_dev_attr);
+		return;
+	}
+
+	pr_info("Amlogic Meson SoC Rev%X (%X:%X)\n", system_rev, system_serial_high, system_rev);
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, soc_dev);
+}
+
 DT_MACHINE_START(MESON, "Amlogic Meson platform")
+	.init_machine	= meson_init_machine,
 	.dt_compat	= meson_common_board_compat,
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
-- 
2.5.0

--
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: [PATCH 1/2] ARM: meson: Adding support to retrieve serial and SoC revision

[Carlo Caione]

On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This path adds support to get the revision and the serial of the running
> SoC, on Meson8. On these plaforms, these informations can be found into

no Meson8b or Meson6?

> CBUS registers. To do so, we instanciate a syscon register, then create

What you mean with syscon register?

> a soc_device, and finally we expose everything to the system.
>
> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/mach-meson/meson.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/arch/arm/mach-meson/meson.c b/arch/arm/mach-meson/meson.c
> index 4e23571..2816e30 100644
> --- a/arch/arm/mach-meson/meson.c
> +++ b/arch/arm/mach-meson/meson.c
> @@ -13,8 +13,16 @@
>   *
>   */
>
> +#include <linux/of_address.h>
>  #include <linux/of_platform.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
>  #include <asm/mach/arch.h>
> +#include <asm/system_info.h>
> +
> +#define MESON_REVISION_REG (0x45c)
>
>  static const char * const meson_common_board_compat[] = {
>         "amlogic,meson6",
> @@ -23,7 +31,54 @@ static const char * const meson_common_board_compat[] = {
>         NULL,
>  };
>
> +static void __init meson_init_machine(void)
> +{
> +       struct soc_device_attribute *soc_dev_attr;
> +       struct soc_device *soc_dev;
> +       struct regmap *hwrev;
> +       unsigned int val;
> +       int ret;
> +
> +       hwrev = syscon_regmap_lookup_by_compatible("amlogic,meson8b-hwrev");

Is this specific only for Meson8b?

> +       if (IS_ERR(hwrev)) {
> +               pr_err("hwrev node not found\n");
> +               return;
> +       }
> +
> +       soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +       if (!soc_dev_attr)
> +               return;

Missing blank line

> +       ret = regmap_read(hwrev, 0, &val);
> +       if (ret < 0) {
> +               pr_err("Could not get SoC id\n");

kfree(soc_dev_attr)

> +               return;
> +       }
> +       system_serial_high = val << 24;
> +
> +       ret = regmap_read(hwrev, MESON_REVISION_REG, &val);
> +       if (ret < 0) {
> +               pr_err("Could not get SoC revision\n");

ditto

> +               return;
> +       }
> +       system_rev = val == 0x11111111 ? 0xA : 0xB;
> +
> +       soc_dev_attr->family = "Amlogic Meson";
> +       soc_dev_attr->revision = kasprintf(GFP_KERNEL, "0x%x", system_rev);
> +       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", system_serial_high);
> +
> +       soc_dev = soc_device_register(soc_dev_attr);
> +       if (IS_ERR(soc_dev)) {
> +               pr_err("Could not register soc device\n");
> +               kfree(soc_dev_attr);

leaking soc_dev_attr->revision and  soc_dev_attr->soc_id also any
reason why you are not kasprintf-ing also family?

> +               return;
> +       }
> +
> +       pr_info("Amlogic Meson SoC Rev%X (%X:%X)\n", system_rev, system_serial_high, system_rev);
> +       of_platform_populate(NULL, of_default_bus_match_table, NULL, soc_dev);

Compiling I got:

  CC      arch/arm/mach-meson/meson.o
arch/arm/mach-meson/meson.c: In function 'meson_init_machine':
arch/arm/mach-meson/meson.c:77:63: warning: passing argument 4 of
'of_platform_populate' from incompatible pointer type
  of_platform_populate(NULL, of_default_bus_match_table, NULL, soc_dev);
                                                               ^
In file included from arch/arm/mach-meson/meson.c:17:0:
include/linux/of_platform.h:71:12: note: expected 'struct device *'
but argument is of type 'struct soc_device *'
 extern int of_platform_populate(struct device_node *root,
            ^

> +}
> +
>  DT_MACHINE_START(MESON, "Amlogic Meson platform")
> +       .init_machine   = meson_init_machine,

Uhm, you are assuming that this code is valid for Meson8, Meson8b and
also Meson6. Are you sure about that?

>         .dt_compat      = meson_common_board_compat,
>         .l2c_aux_val    = 0,
>         .l2c_aux_mask   = ~0,
> --
> 2.5.0

Thanks for working on this!

-- 
Carlo Caione
--
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: [PATCH 1/2] ARM: meson: Adding support to retrieve serial and SoC revision

[Romain Perier]

Hi all,

2016-02-17 21:34 GMT+01:00 Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>:
> On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> This path adds support to get the revision and the serial of the running
>> SoC, on Meson8. On these plaforms, these informations can be found into
>
> no Meson8b or Meson6?

Well, I only tested on Meson8b, I don't know if it works in the same
way on older Meson, however I can take a look to the vendor kernel
probably...

>
>> CBUS registers. To do so, we instanciate a syscon register, then create
>
> What you mean with syscon register?


I mean that we use a syscon regmap to access these registers.


>
>> a soc_device, and finally we expose everything to the system.
>>
>> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/arm/mach-meson/meson.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/arch/arm/mach-meson/meson.c b/arch/arm/mach-meson/meson.c
>> index 4e23571..2816e30 100644
>> --- a/arch/arm/mach-meson/meson.c
>> +++ b/arch/arm/mach-meson/meson.c
>> @@ -13,8 +13,16 @@
>>   *
>>   */
>>
>> +#include <linux/of_address.h>
>>  #include <linux/of_platform.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/sys_soc.h>
>>  #include <asm/mach/arch.h>
>> +#include <asm/system_info.h>
>> +
>> +#define MESON_REVISION_REG (0x45c)
>>
>>  static const char * const meson_common_board_compat[] = {
>>         "amlogic,meson6",
>> @@ -23,7 +31,54 @@ static const char * const meson_common_board_compat[] = {
>>         NULL,
>>  };
>>
>> +static void __init meson_init_machine(void)
>> +{
>> +       struct soc_device_attribute *soc_dev_attr;
>> +       struct soc_device *soc_dev;
>> +       struct regmap *hwrev;
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       hwrev = syscon_regmap_lookup_by_compatible("amlogic,meson8b-hwrev");
>
> Is this specific only for Meson8b?


For now, yes. However, As I said, I can to do something generic. What
do you think ?


>
>> +       if (IS_ERR(hwrev)) {
>> +               pr_err("hwrev node not found\n");
>> +               return;
>> +       }
>> +
>> +       soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
>> +       if (!soc_dev_attr)
>> +               return;
>
> Missing blank line
>
>> +       ret = regmap_read(hwrev, 0, &val);
>> +       if (ret < 0) {
>> +               pr_err("Could not get SoC id\n");
>
> kfree(soc_dev_attr)
>
>> +               return;
>> +       }
>> +       system_serial_high = val << 24;
>> +
>> +       ret = regmap_read(hwrev, MESON_REVISION_REG, &val);
>> +       if (ret < 0) {
>> +               pr_err("Could not get SoC revision\n");
>
> ditto
>
>> +               return;
>> +       }
>> +       system_rev = val == 0x11111111 ? 0xA : 0xB;
>> +
>> +       soc_dev_attr->family = "Amlogic Meson";
>> +       soc_dev_attr->revision = kasprintf(GFP_KERNEL, "0x%x", system_rev);
>> +       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", system_serial_high);
>> +
>> +       soc_dev = soc_device_register(soc_dev_attr);
>> +       if (IS_ERR(soc_dev)) {
>> +               pr_err("Could not register soc device\n");
>> +               kfree(soc_dev_attr);
>
> leaking soc_dev_attr->revision and  soc_dev_attr->soc_id also any
> reason why you are not kasprintf-ing also family?
>

My problem is that I cannot use a devm allocation there, right ? I
mean I have no device... Well, I will think about it.

>> +               return;
>> +       }
>> +
>> +       pr_info("Amlogic Meson SoC Rev%X (%X:%X)\n", system_rev, system_serial_high, system_rev);
>> +       of_platform_populate(NULL, of_default_bus_match_table, NULL, soc_dev);
>
> Compiling I got:
>
>   CC      arch/arm/mach-meson/meson.o
> arch/arm/mach-meson/meson.c: In function 'meson_init_machine':
> arch/arm/mach-meson/meson.c:77:63: warning: passing argument 4 of
> 'of_platform_populate' from incompatible pointer type
>   of_platform_populate(NULL, of_default_bus_match_table, NULL, soc_dev);
>                                                                ^
> In file included from arch/arm/mach-meson/meson.c:17:0:
> include/linux/of_platform.h:71:12: note: expected 'struct device *'
> but argument is of type 'struct soc_device *'
>  extern int of_platform_populate(struct device_node *root,
>             ^
>

Ah, good catch ! I build everything using yocto, so I did not see
these warnings ^^

>> +}
>> +
>>  DT_MACHINE_START(MESON, "Amlogic Meson platform")
>> +       .init_machine   = meson_init_machine,
>
> Uhm, you are assuming that this code is valid for Meson8, Meson8b and
> also Meson6. Are you sure about that?

No, you're right.

Thanks,
Romain
--
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: [PATCH 1/2] ARM: meson: Adding support to retrieve serial and SoC revision

[Carlo Caione]

On Thu, Feb 18, 2016 at 1:20 PM, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi all,

[cut]

>>> +static void __init meson_init_machine(void)
>>> +{
>>> +       struct soc_device_attribute *soc_dev_attr;
>>> +       struct soc_device *soc_dev;
>>> +       struct regmap *hwrev;
>>> +       unsigned int val;
>>> +       int ret;
>>> +
>>> +       hwrev = syscon_regmap_lookup_by_compatible("amlogic,meson8b-hwrev");
>>
>> Is this specific only for Meson8b?
>
>
> For now, yes. However, As I said, I can to do something generic. What
> do you think ?

my guess is that it works fine for meson8 and meson8b. Not sure about
meson6. You should take a look to the Amlogic SDK to confirm that or
just exclude meson6.
I was actually referring to the name of the compatible that seems a
bit too specific to me.

>>
>>> +               return;
>>> +       }
>>> +       system_rev = val == 0x11111111 ? 0xA : 0xB;
>>> +
>>> +       soc_dev_attr->family = "Amlogic Meson";
>>> +       soc_dev_attr->revision = kasprintf(GFP_KERNEL, "0x%x", system_rev);
>>> +       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", system_serial_high);
>>> +
>>> +       soc_dev = soc_device_register(soc_dev_attr);
>>> +       if (IS_ERR(soc_dev)) {
>>> +               pr_err("Could not register soc device\n");
>>> +               kfree(soc_dev_attr);
>>
>> leaking soc_dev_attr->revision and  soc_dev_attr->soc_id also any
>> reason why you are not kasprintf-ing also family?
>>
>
> My problem is that I cannot use a devm allocation there, right ? I
> mean I have no device... Well, I will think about it.

I mean: you are kfree-ing soc_dev_attr but not soc_dev_attr->revision
and soc_dev_attr->soc_id

>>> +               return;
>>> +       }
>>> +
>>> +       pr_info("Amlogic Meson SoC Rev%X (%X:%X)\n", system_rev, system_serial_high, system_rev);
>>> +       of_platform_populate(NULL, of_default_bus_match_table, NULL, soc_dev);
>>
>> Compiling I got:
>>
>>   CC      arch/arm/mach-meson/meson.o
>> arch/arm/mach-meson/meson.c: In function 'meson_init_machine':
>> arch/arm/mach-meson/meson.c:77:63: warning: passing argument 4 of
>> 'of_platform_populate' from incompatible pointer type
>>   of_platform_populate(NULL, of_default_bus_match_table, NULL, soc_dev);
>>                                                                ^
>> In file included from arch/arm/mach-meson/meson.c:17:0:
>> include/linux/of_platform.h:71:12: note: expected 'struct device *'
>> but argument is of type 'struct soc_device *'
>>  extern int of_platform_populate(struct device_node *root,
>>             ^
>>
>
> Ah, good catch ! I build everything using yocto, so I did not see
> these warnings ^^

Please, do not use yocto for this ;)

Cheers,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless
--
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

[PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Romain Perier]

These are the CBUS registers used to retrieve the revision and the
identifier of the SoC on Meson8.

Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/meson8b.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 0477a81..71009dc 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -99,6 +99,11 @@
 		};
 	};
 
+	hwrev@c1107d4c {
+		compatible = "amlogic,meson8b-hwrev", "syscon";
+		reg = <0xc1107d4c 0x460>;
+	};
+
 	sram: sram@d9000000 {
 		compatible = "mmio-sram";
 		reg = <0xd9000000 0x20000>;
-- 
2.5.0

--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> These are the CBUS registers used to retrieve the revision and the
> identifier of the SoC on Meson8.
>
> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/boot/dts/meson8b.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
> index 0477a81..71009dc 100644
> --- a/arch/arm/boot/dts/meson8b.dtsi
> +++ b/arch/arm/boot/dts/meson8b.dtsi
> @@ -99,6 +99,11 @@
>                 };
>         };
>
> +       hwrev@c1107d4c {
> +               compatible = "amlogic,meson8b-hwrev", "syscon";
> +               reg = <0xc1107d4c 0x460>;

Interesting. Where did you get 0x460?

> +       };
> +
>         sram: sram@d9000000 {
>                 compatible = "mmio-sram";
>                 reg = <0xd9000000 0x20000>;

This patch fails to apply on the current master. Probably because you
have based this patch on a repo containing (as I can see) also my WiP
patches on SMP.

-- 
Carlo Caione
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Romain Perier]

Hi,

2016-02-17 21:36 GMT+01:00 Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>:
> On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> These are the CBUS registers used to retrieve the revision and the
>> identifier of the SoC on Meson8.
>>
>> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/meson8b.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
>> index 0477a81..71009dc 100644
>> --- a/arch/arm/boot/dts/meson8b.dtsi
>> +++ b/arch/arm/boot/dts/meson8b.dtsi
>> @@ -99,6 +99,11 @@
>>                 };
>>         };
>>
>> +       hwrev@c1107d4c {
>> +               compatible = "amlogic,meson8b-hwrev", "syscon";
>> +               reg = <0xc1107d4c 0x460>;
>
> Interesting. Where did you get 0x460?

Carlo, Arnd.

Well, what I did is the following :
- CBUS_PHY_BASE is 0xc1100000   (CBUS is a larger block of registers,
like slcr on zynq)
- the serial is at CBUS_PHY_BASE + 0x7d4c
- the revision is at CBUS_PHY_BASE + 0x81a8

So I decided to create a device_node for hw revision at 0xc1107d4c, in
this case the lenght is 0x460...
Am I wrong ?


>
>> +       };
>> +
>>         sram: sram@d9000000 {
>>                 compatible = "mmio-sram";
>>                 reg = <0xd9000000 0x20000>;
>
> This patch fails to apply on the current master. Probably because you
> have based this patch on a repo containing (as I can see) also my WiP
> patches on SMP.

I used linux-next with your patches on top of it yes... I will rebase it

Thanks,
Romain
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Arnd Bergmann]

On Thursday 18 February 2016 13:33:04 Romain Perier wrote:
> 2016-02-17 21:36 GMT+01:00 Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>:
> > On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> These are the CBUS registers used to retrieve the revision and the
> >> identifier of the SoC on Meson8.
> >>
> >> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  arch/arm/boot/dts/meson8b.dtsi | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
> >> index 0477a81..71009dc 100644
> >> --- a/arch/arm/boot/dts/meson8b.dtsi
> >> +++ b/arch/arm/boot/dts/meson8b.dtsi
> >> @@ -99,6 +99,11 @@
> >>                 };
> >>         };
> >>
> >> +       hwrev@c1107d4c {
> >> +               compatible = "amlogic,meson8b-hwrev", "syscon";
> >> +               reg = <0xc1107d4c 0x460>;
> >
> > Interesting. Where did you get 0x460?
> 
> Carlo, Arnd.
> 
> Well, what I did is the following :
> - CBUS_PHY_BASE is 0xc1100000   (CBUS is a larger block of registers,
> like slcr on zynq)
> - the serial is at CBUS_PHY_BASE + 0x7d4c
> - the revision is at CBUS_PHY_BASE + 0x81a8
> 
> So I decided to create a device_node for hw revision at 0xc1107d4c, in
> this case the lenght is 0x460...
> Am I wrong ?

Yes, you should describe the device that is actually there, which would be
something like

	cbus@c1100000 {
		compatible = "amlogic,meson8b-cbus", "syscon";
		reg = <0xc1100000 0x10000>; /* no idea what the length is, fix it */
	};

Don't just make things up because you happen to access them in a particular
way, but try to stay as close as you can to describing the actual hardware.

Please also check that the register ranges don't overlap with something
that might already be there, in case someone else made the same mistake.

	Arnd
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Thu, Feb 18, 2016 at 1:43 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 18 February 2016 13:33:04 Romain Perier wrote:
>> 2016-02-17 21:36 GMT+01:00 Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>:
>> > On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >> These are the CBUS registers used to retrieve the revision and the
>> >> identifier of the SoC on Meson8.
>> >>
>> >> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> ---
>> >>  arch/arm/boot/dts/meson8b.dtsi | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
>> >> index 0477a81..71009dc 100644
>> >> --- a/arch/arm/boot/dts/meson8b.dtsi
>> >> +++ b/arch/arm/boot/dts/meson8b.dtsi
>> >> @@ -99,6 +99,11 @@
>> >>                 };
>> >>         };
>> >>
>> >> +       hwrev@c1107d4c {
>> >> +               compatible = "amlogic,meson8b-hwrev", "syscon";
>> >> +               reg = <0xc1107d4c 0x460>;
>> >
>> > Interesting. Where did you get 0x460?
>>
>> Carlo, Arnd.
>>
>> Well, what I did is the following :
>> - CBUS_PHY_BASE is 0xc1100000   (CBUS is a larger block of registers,
>> like slcr on zynq)
>> - the serial is at CBUS_PHY_BASE + 0x7d4c
>> - the revision is at CBUS_PHY_BASE + 0x81a8
>>
>> So I decided to create a device_node for hw revision at 0xc1107d4c, in
>> this case the lenght is 0x460...
>> Am I wrong ?
>
> Yes, you should describe the device that is actually there, which would be
> something like
>
>         cbus@c1100000 {
>                 compatible = "amlogic,meson8b-cbus", "syscon";
>                 reg = <0xc1100000 0x10000>; /* no idea what the length is, fix it */
>         };
>
> Don't just make things up because you happen to access them in a particular
> way, but try to stay as close as you can to describing the actual hardware.

Arnd,
in the cbus region are mapped a lot of different devices, do you think
that it is a good idea mapping the whole region as a single syscon
device?

-- 
Carlo Caione
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Arnd Bergmann]

On Thursday 18 February 2016 15:14:45 Carlo Caione wrote:
> >
> > Yes, you should describe the device that is actually there, which would be
> > something like
> >
> >         cbus@c1100000 {
> >                 compatible = "amlogic,meson8b-cbus", "syscon";
> >                 reg = <0xc1100000 0x10000>; /* no idea what the length is, fix it */
> >         };
> >
> > Don't just make things up because you happen to access them in a particular
> > way, but try to stay as close as you can to describing the actual hardware.
> 
> Arnd,
> in the cbus region are mapped a lot of different devices, do you think
> that it is a good idea mapping the whole region as a single syscon
> device?

It really depends on what those other devices do with the registers:

If each device just uses a couple of random registers from the cbus,
they should probably all be changed to go through syscon.

However, if some or all of the other devices actually are entirely
made up of register ranges within cbus, that would indicate that
cbus itself is not just a collection of random registers but something
that could be considered a bus of itself in hardware, and then
we could represent the other devices as children of this bus.

Usually once you have a list of all the register locations, you can
identify the hardware structure to a certain degree from that, even
if you don't have access to a data sheet that would clarify this
better.

	Arnd
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Thu, Feb 18, 2016 at 3:22 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 18 February 2016 15:14:45 Carlo Caione wrote:
>> >
>> > Yes, you should describe the device that is actually there, which would be
>> > something like
>> >
>> >         cbus@c1100000 {
>> >                 compatible = "amlogic,meson8b-cbus", "syscon";
>> >                 reg = <0xc1100000 0x10000>; /* no idea what the length is, fix it */
>> >         };
>> >
>> > Don't just make things up because you happen to access them in a particular
>> > way, but try to stay as close as you can to describing the actual hardware.
>>
>> Arnd,
>> in the cbus region are mapped a lot of different devices, do you think
>> that it is a good idea mapping the whole region as a single syscon
>> device?
>
> It really depends on what those other devices do with the registers:
>
> If each device just uses a couple of random registers from the cbus,
> they should probably all be changed to go through syscon.

In our case each device (pinmux, GPIO IRQ, clock controller, ...) uses
a consistent and contiguous set of registers inside cbus.
But in the same cbus sometimes we have also some registers that are
used for something different or peculiar like in this case. Usually
it's matter of one or two registers though, that's why I was a bit
curious about such a huge size 0x460.

> However, if some or all of the other devices actually are entirely
> made up of register ranges within cbus, that would indicate that
> cbus itself is not just a collection of random registers but something
> that could be considered a bus of itself in hardware, and then
> we could represent the other devices as children of this bus.

I think that this is exactly the case. We are missing this cbus in the
DTS because (for lacking of proper documentation) we are not sure
about start / end / size.

> Usually once you have a list of all the register locations, you can
> identify the hardware structure to a certain degree from that, even
> if you don't have access to a data sheet that would clarify this
> better.

Yes, in general for each single device the registers of interest are
contiguous inside cbus.

Thanks,

-- 
Carlo Caione
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Thu, Feb 18, 2016 at 10:04 PM, Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Feb 18, 2016 at 3:22 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

>> However, if some or all of the other devices actually are entirely
>> made up of register ranges within cbus, that would indicate that
>> cbus itself is not just a collection of random registers but something
>> that could be considered a bus of itself in hardware, and then
>> we could represent the other devices as children of this bus.
>
> I think that this is exactly the case. We are missing this cbus in the
> DTS because (for lacking of proper documentation) we are not sure
> about start / end / size.

This is not entirely correct actually.
CBUS starts at 0xc1100000 with a size of 0x00100000 according to the Amlogic SDK

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Daniel Drake]

On Thu, Feb 18, 2016 at 3:04 PM, Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
>> However, if some or all of the other devices actually are entirely
>> made up of register ranges within cbus, that would indicate that
>> cbus itself is not just a collection of random registers but something
>> that could be considered a bus of itself in hardware, and then
>> we could represent the other devices as children of this bus.
>
> I think that this is exactly the case. We are missing this cbus in the
> DTS because (for lacking of proper documentation) we are not sure
> about start / end / size.

Here's a hint from the vendor kernel
https://github.com/endlessm/linux-meson/blob/master/arch/arm/mach-meson8b/iomapping.c#L87

Having a cbus bus node with child devices does sound like it would
reflect this particular view of the hardware design. How would we then
represent the hwrev registers under that?

I am also curious if this is the common practice. We were working with
Exynos devices before, and even though many of the components are on
the AXI bus there, there is no AXI bus representation in the DT. But
now that I go digging, I see other SoCs that do have a DT bus
representation very similar to what's being described, such as the apb
and axi busses in mmp2.dtsi. Is one approach preferred over the other
for new SoC support?

Daniel
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Arnd Bergmann]

On Thursday 18 February 2016 15:27:06 Daniel Drake wrote:
> On Thu, Feb 18, 2016 at 3:04 PM, Carlo Caione <carlo@caione.org> wrote:
> >> However, if some or all of the other devices actually are entirely
> >> made up of register ranges within cbus, that would indicate that
> >> cbus itself is not just a collection of random registers but something
> >> that could be considered a bus of itself in hardware, and then
> >> we could represent the other devices as children of this bus.
> >
> > I think that this is exactly the case. We are missing this cbus in the
> > DTS because (for lacking of proper documentation) we are not sure
> > about start / end / size.
> 
> Here's a hint from the vendor kernel
> https://github.com/endlessm/linux-meson/blob/master/arch/arm/mach-meson8b/iomapping.c#L87
> 
> Having a cbus bus node with child devices does sound like it would
> reflect this particular view of the hardware design. How would we then
> represent the hwrev registers under that?
> 
> I am also curious if this is the common practice. We were working with
> Exynos devices before, and even though many of the components are on
> the AXI bus there, there is no AXI bus representation in the DT. But
> now that I go digging, I see other SoCs that do have a DT bus
> representation very similar to what's being described, such as the apb
> and axi busses in mmp2.dtsi. Is one approach preferred over the other
> for new SoC support?

I would always prefer having the dts files describe the hardware as best
as they can.

	Arnd

Re: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Arnd Bergmann]

On Thursday 18 February 2016 15:27:06 Daniel Drake wrote:
> 
> Having a cbus bus node with child devices does sound like it would
> reflect this particular view of the hardware design. How would we then
> represent the hwrev registers under that?
> 

The question is still how the CBUS is structured internally.

https://github.com/endlessm/linux-meson/blob/master/arch/arm/mach-meson8b/include/mach/register.h

seems to have a good list of registers, but it's a bit hard to
read, so I reformatted it slightly and put it up at http://pastebin.com/GrzYM7Ti

In many cases, the register name prefix gives a good idea of what things
are. In particular, it seems that all registers numbers are between 0x1000
and 0x2fff, which would be offsets 0x4000 through 0xbfff.

The revisison register is in a block called ASSIST, which is mixed with two
AC3 (audio?) regions, and the only other ASSIST register that is ever
used is the ASSIST_POR_CONFIG.

The METAL_REVISION is in the middle of some apparently all unrelated registers:

#define PREG_ETH_REG0 0x2050
#define PREG_ETH_REG1 0x2051
#define PROD_TEST_REG1 0x2067
#define PROD_TEST_REG0 0x2068
#define METAL_REVISION 0x206a
#define ADC_TOP_MISC 0x206b
#define DPLL_TOP_MISC 0x206c
#define ANALOG_TOP_MISC 0x206d
#define AM_ANALOG_TOP_REG0 0x206e
#define AM_ANALOG_TOP_REG1 0x206f
#define PREG_STICKY_REG0 0x207c
#define PREG_STICKY_REG1 0x207d

This still really sounds like a mixed bag to me, which should better get represented
as a syscon node, except that there are also some more structured areas in
CBUS.

Having just the registers between METAL_REVISION and HW_REV in a syscon
is clearly wrong, as that would include the pinctrl area that already has
a driver, but would not include some other parts that want the syscon.

Maybe the best way is to make it compatible with both syscon and
simple-bus and put the other nodes underneath. That is still rather
ugly, but at least it works and can be extended.

	Arnd
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 18 February 2016 15:27:06 Daniel Drake wrote:
>>
>> Having a cbus bus node with child devices does sound like it would
>> reflect this particular view of the hardware design. How would we then
>> represent the hwrev registers under that?
>>
>
> The question is still how the CBUS is structured internally.
>
> https://github.com/endlessm/linux-meson/blob/master/arch/arm/mach-meson8b/include/mach/register.h
>
> seems to have a good list of registers, but it's a bit hard to
> read, so I reformatted it slightly and put it up at http://pastebin.com/GrzYM7Ti
>
> In many cases, the register name prefix gives a good idea of what things
> are. In particular, it seems that all registers numbers are between 0x1000
> and 0x2fff, which would be offsets 0x4000 through 0xbfff.
>
> The revisison register is in a block called ASSIST, which is mixed with two
> AC3 (audio?) regions, and the only other ASSIST register that is ever
> used is the ASSIST_POR_CONFIG.
>
> The METAL_REVISION is in the middle of some apparently all unrelated registers:
>
> #define PREG_ETH_REG0 0x2050
> #define PREG_ETH_REG1 0x2051
> #define PROD_TEST_REG1 0x2067
> #define PROD_TEST_REG0 0x2068
> #define METAL_REVISION 0x206a
> #define ADC_TOP_MISC 0x206b
> #define DPLL_TOP_MISC 0x206c
> #define ANALOG_TOP_MISC 0x206d
> #define AM_ANALOG_TOP_REG0 0x206e
> #define AM_ANALOG_TOP_REG1 0x206f
> #define PREG_STICKY_REG0 0x207c
> #define PREG_STICKY_REG1 0x207d
>
> This still really sounds like a mixed bag to me, which should better get represented
> as a syscon node, except that there are also some more structured areas in
> CBUS.
>
> Having just the registers between METAL_REVISION and HW_REV in a syscon
> is clearly wrong, as that would include the pinctrl area that already has
> a driver, but would not include some other parts that want the syscon.

Agree

> Maybe the best way is to make it compatible with both syscon and
> simple-bus and put the other nodes underneath. That is still rather
> ugly, but at least it works and can be extended.

This is a good idea actually. I'll prepare a patch.
Thank you for having spent time on this.

Cheers,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Fri, Feb 19, 2016 at 2:25 PM, Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org> wrote:

>> Maybe the best way is to make it compatible with both syscon and
>> simple-bus and put the other nodes underneath. That is still rather
>> ugly, but at least it works and can be extended.
>
> This is a good idea actually. I'll prepare a patch.
> Thank you for having spent time on this.

Apparently I talked too soon. The problem is the pinctrl driver where
one of the bank (ao-bank) is mapped into the aobus not cbus
http://lxr.free-electrons.com/source/arch/arm/boot/dts/meson8b.dtsi#L169
If you don't have a better idea probably I should try to decouple the
two banks so that we have two different pinctrl devices for each bus
(aobus and cbus).

Cheers,

-- 
Carlo Caione
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

<cut>

> This still really sounds like a mixed bag to me, which should better get represented
> as a syscon node, except that there are also some more structured areas in
> CBUS.
>
> Having just the registers between METAL_REVISION and HW_REV in a syscon
> is clearly wrong, as that would include the pinctrl area that already has
> a driver, but would not include some other parts that want the syscon.
>
> Maybe the best way is to make it compatible with both syscon and
> simple-bus and put the other nodes underneath. That is still rather
> ugly, but at least it works and can be extended.

More on this topic.

On the meson platforms (at least on the meson8 / meson8b) we have two
buses: cbus and aobus. Since in cbus we have a bunch of scattered
registers, Arnd suggested to make it compatible with both syscon and
simple-bus. So my idea was actually to update the meson8b DTSI file
adding the two buses to make it closer to the actual hardware.

In the most simple cases moving the devices under the correct bus is a
trivial operation since (of course) the same driver can be used when
the device is attached to a different bus, like in the uart_AO case
(http://lxr.free-electrons.com/source/arch/arm/boot/dts/meson8b.dtsi#L114).

Unfortunately pinctrl is a different beast since it requires
(http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson.c#L659)
at least two subnodes: one accessing registers from aobus, and the
other one from cbus.

I know this is quite a peculiar case but I'm wondering what is the
best way to approach this issue.

1) We could move only the pinctrl device outside both aobus and cbus
but IMO this is ugly (at this point is probably better not having the
two buses at all described in the DTS).
2) The second option is just to fix the driver so that the two
subnodes are not strictly required. The problem with this second
solution is that in the driver we still need to access some data
(http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson8b.c#L873)
that is specific to each bus. So we will end up having two different
compatibles for the two buses (meson8b-pinctrl-aobus using data from
'ao-bank', and meson8b-pinctrl-cbus using data from 'banks').
3) Another option is just to have the driver with a unique compatible
poking the parent node (or just to another property) to determine on
which bus it is so that it can use the correct bus-specific data.

Any idea / feedback?

-- 
Carlo Caione
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Arnd Bergmann]

On Friday 26 February 2016 16:34:59 Carlo Caione wrote:
> On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> 
> <cut>
> 
> > This still really sounds like a mixed bag to me, which should better get represented
> > as a syscon node, except that there are also some more structured areas in
> > CBUS.
> >
> > Having just the registers between METAL_REVISION and HW_REV in a syscon
> > is clearly wrong, as that would include the pinctrl area that already has
> > a driver, but would not include some other parts that want the syscon.
> >
> > Maybe the best way is to make it compatible with both syscon and
> > simple-bus and put the other nodes underneath. That is still rather
> > ugly, but at least it works and can be extended.
> 
> More on this topic.
> 
> On the meson platforms (at least on the meson8 / meson8b) we have two
> buses: cbus and aobus. Since in cbus we have a bunch of scattered
> registers, Arnd suggested to make it compatible with both syscon and
> simple-bus. So my idea was actually to update the meson8b DTSI file
> adding the two buses to make it closer to the actual hardware.
> 
> In the most simple cases moving the devices under the correct bus is a
> trivial operation since (of course) the same driver can be used when
> the device is attached to a different bus, like in the uart_AO case
> (http://lxr.free-electrons.com/source/arch/arm/boot/dts/meson8b.dtsi#L114).
> 
> Unfortunately pinctrl is a different beast since it requires
> (http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson.c#L659)
> at least two subnodes: one accessing registers from aobus, and the
> other one from cbus.
> 
> I know this is quite a peculiar case but I'm wondering what is the
> best way to approach this issue.
> 
> 1) We could move only the pinctrl device outside both aobus and cbus
> but IMO this is ugly (at this point is probably better not having the
> two buses at all described in the DTS).
> 2) The second option is just to fix the driver so that the two
> subnodes are not strictly required. The problem with this second
> solution is that in the driver we still need to access some data
> (http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson8b.c#L873)
> that is specific to each bus. So we will end up having two different
> compatibles for the two buses (meson8b-pinctrl-aobus using data from
> 'ao-bank', and meson8b-pinctrl-cbus using data from 'banks').
> 3) Another option is just to have the driver with a unique compatible
> poking the parent node (or just to another property) to determine on
> which bus it is so that it can use the correct bus-specific data.
> 
> Any idea / feedback?

Would it be possible to split the pin controller driver into two drivers
that each only access registers on one of the buses? Is that a split
that makes sense from the point of view of that driver?

	Arnd
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Fri, Feb 26, 2016 at 5:00 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday 26 February 2016 16:34:59 Carlo Caione wrote:
>> On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>
>> <cut>
>>
>> > This still really sounds like a mixed bag to me, which should better get represented
>> > as a syscon node, except that there are also some more structured areas in
>> > CBUS.
>> >
>> > Having just the registers between METAL_REVISION and HW_REV in a syscon
>> > is clearly wrong, as that would include the pinctrl area that already has
>> > a driver, but would not include some other parts that want the syscon.
>> >
>> > Maybe the best way is to make it compatible with both syscon and
>> > simple-bus and put the other nodes underneath. That is still rather
>> > ugly, but at least it works and can be extended.
>>
>> More on this topic.
>>
>> On the meson platforms (at least on the meson8 / meson8b) we have two
>> buses: cbus and aobus. Since in cbus we have a bunch of scattered
>> registers, Arnd suggested to make it compatible with both syscon and
>> simple-bus. So my idea was actually to update the meson8b DTSI file
>> adding the two buses to make it closer to the actual hardware.
>>
>> In the most simple cases moving the devices under the correct bus is a
>> trivial operation since (of course) the same driver can be used when
>> the device is attached to a different bus, like in the uart_AO case
>> (http://lxr.free-electrons.com/source/arch/arm/boot/dts/meson8b.dtsi#L114).
>>
>> Unfortunately pinctrl is a different beast since it requires
>> (http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson.c#L659)
>> at least two subnodes: one accessing registers from aobus, and the
>> other one from cbus.
>>
>> I know this is quite a peculiar case but I'm wondering what is the
>> best way to approach this issue.
>>
>> 1) We could move only the pinctrl device outside both aobus and cbus
>> but IMO this is ugly (at this point is probably better not having the
>> two buses at all described in the DTS).
>> 2) The second option is just to fix the driver so that the two
>> subnodes are not strictly required. The problem with this second
>> solution is that in the driver we still need to access some data
>> (http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson8b.c#L873)
>> that is specific to each bus. So we will end up having two different
>> compatibles for the two buses (meson8b-pinctrl-aobus using data from
>> 'ao-bank', and meson8b-pinctrl-cbus using data from 'banks').
>> 3) Another option is just to have the driver with a unique compatible
>> poking the parent node (or just to another property) to determine on
>> which bus it is so that it can use the correct bus-specific data.
>>
>> Any idea / feedback?
>
> Would it be possible to split the pin controller driver into two drivers
> that each only access registers on one of the buses? Is that a split
> that makes sense from the point of view of that driver?

AFAICT (I'm not the driver author) there is no a really strict reason
to have one single driver accessing registers on both buses. Of course
the driver has to be changed a bit.
Are you suggesting to have two different drivers with two different compatibles?

-- 
Carlo Caione
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Arnd Bergmann]

On Friday 26 February 2016 17:43:54 Carlo Caione wrote:
> On Fri, Feb 26, 2016 at 5:00 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Friday 26 February 2016 16:34:59 Carlo Caione wrote:
> >> On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >>
> >> <cut>
> >>
> >> > This still really sounds like a mixed bag to me, which should better get represented
> >> > as a syscon node, except that there are also some more structured areas in
> >> > CBUS.
> >> >
> >> > Having just the registers between METAL_REVISION and HW_REV in a syscon
> >> > is clearly wrong, as that would include the pinctrl area that already has
> >> > a driver, but would not include some other parts that want the syscon.
> >> >
> >> > Maybe the best way is to make it compatible with both syscon and
> >> > simple-bus and put the other nodes underneath. That is still rather
> >> > ugly, but at least it works and can be extended.
> >>
> >> More on this topic.
> >>
> >> On the meson platforms (at least on the meson8 / meson8b) we have two
> >> buses: cbus and aobus. Since in cbus we have a bunch of scattered
> >> registers, Arnd suggested to make it compatible with both syscon and
> >> simple-bus. So my idea was actually to update the meson8b DTSI file
> >> adding the two buses to make it closer to the actual hardware.
> >>
> >> In the most simple cases moving the devices under the correct bus is a
> >> trivial operation since (of course) the same driver can be used when
> >> the device is attached to a different bus, like in the uart_AO case
> >> (http://lxr.free-electrons.com/source/arch/arm/boot/dts/meson8b.dtsi#L114).
> >>
> >> Unfortunately pinctrl is a different beast since it requires
> >> (http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson.c#L659)
> >> at least two subnodes: one accessing registers from aobus, and the
> >> other one from cbus.
> >>
> >> I know this is quite a peculiar case but I'm wondering what is the
> >> best way to approach this issue.
> >>
> >> 1) We could move only the pinctrl device outside both aobus and cbus
> >> but IMO this is ugly (at this point is probably better not having the
> >> two buses at all described in the DTS).
> >> 2) The second option is just to fix the driver so that the two
> >> subnodes are not strictly required. The problem with this second
> >> solution is that in the driver we still need to access some data
> >> (http://lxr.free-electrons.com/source/drivers/pinctrl/meson/pinctrl-meson8b.c#L873)
> >> that is specific to each bus. So we will end up having two different
> >> compatibles for the two buses (meson8b-pinctrl-aobus using data from
> >> 'ao-bank', and meson8b-pinctrl-cbus using data from 'banks').
> >> 3) Another option is just to have the driver with a unique compatible
> >> poking the parent node (or just to another property) to determine on
> >> which bus it is so that it can use the correct bus-specific data.
> >>
> >> Any idea / feedback?
> >
> > Would it be possible to split the pin controller driver into two drivers
> > that each only access registers on one of the buses? Is that a split
> > that makes sense from the point of view of that driver?
> 
> AFAICT (I'm not the driver author) there is no a really strict reason
> to have one single driver accessing registers on both buses. Of course
> the driver has to be changed a bit.
> Are you suggesting to have two different drivers with two different compatibles?

Just an idea, but yes: if the register layout is different, then they
would also need different compatible strings.

This is mostly a question of how the hardware design really looks:
are these two separate pin controllers that each are responsible for
a clear subset of the pins?
	
	Arnd
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Carlo Caione]

On Fri, Feb 26, 2016 at 6:00 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday 26 February 2016 17:43:54 Carlo Caione wrote:

[cut]

>> AFAICT (I'm not the driver author) there is no a really strict reason
>> to have one single driver accessing registers on both buses. Of course
>> the driver has to be changed a bit.
>> Are you suggesting to have two different drivers with two different compatibles?
>
> Just an idea, but yes: if the register layout is different, then they
> would also need different compatible strings.

The meson pin controller driver basically defines the register layout
in the DTS: http://lxr.free-electrons.com/source/arch/arm/boot/dts/meson8.dtsi
#L102 and #L111, so yes, we have a different layouts.

> This is mostly a question of how the hardware design really looks:
> are these two separate pin controllers that each are responsible for
> a clear subset of the pins?

Exactly. All the GPIOAO_XX pins are managed by the pin controller on the AO bus.
At this point I guess it is needed to rework a bit the pinctrl driver
to address this problem switching to two drivers with different
compatibles for the two buses.

Thanks,

-- 
Carlo Caione
--
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: [PATCH 2/2] ARM: dts: meson: Adding hwrev syscon node

[Arnd Bergmann]

On Wednesday 17 February 2016 18:28:34 Romain Perier wrote:
> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
> index 0477a81..71009dc 100644
> --- a/arch/arm/boot/dts/meson8b.dtsi
> +++ b/arch/arm/boot/dts/meson8b.dtsi
> @@ -99,6 +99,11 @@
>                 };
>         };
>  
> +       hwrev@c1107d4c {
> +               compatible = "amlogic,meson8b-hwrev", "syscon";
> +               reg = <0xc1107d4c 0x460>;
> +       };
> 

That is a very odd address. Are you sure this is not part of a larger
block of registers?

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