Adding support to show SoC revision in /proc/cpuinfo
cover Adding support to show SoC revision in /proc/cpuinfo
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(+)
Signature · 1 line
2.5.0
PATCH 1/2 ARM: meson: Adding support to retrieve serial and SoC revision
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.
--- arch/arm/mach-meson/meson.c | 55 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
arch/arm/mach-meson/meson.c +55 -0
@@ -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,
Signature · 1 line
2.5.0
PATCH 1/2 ARM: meson: Adding support to retrieve serial and SoC revision
Quoted 2 lines · 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
no Meson8b or Meson6?
Quoted 1 line
CBUS registers. To do so, we instanciate a syscon register, then create
What you mean with syscon register?
Quoted 31 lines
a
soc_device, and finally we expose everything to the system.Signed-off-by Romain Perier <[email protected]>--- arch/arm/mach-meson/meson.c | 55 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)arch/arm/mach-meson/meson.c +8
-0@@ -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, };
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 42
Quoted 10 lines
arch/arm/mach-meson/meson.c +9
-0+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?
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 50
Quoted 8 lines
arch/arm/mach-meson/meson.c +8
-0+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
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 53
Quoted 3 lines
arch/arm/mach-meson/meson.c +3
-0+ ret =regmap_read (hwrev,0 , &val); +if (ret <0 ) { +pr_err ("Could not get SoC id\n" );
kfree(soc_dev_attr)
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 60
Quoted 7 lines
arch/arm/mach-meson/meson.c +7
-0+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
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 72
Quoted 12 lines
arch/arm/mach-meson/meson.c +12
-0+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?
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 77
Quoted 5 lines
arch/arm/mach-meson/meson.c +5
-0+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.oarch/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,
^
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 81
Quoted 4 lines
arch/arm/mach-meson/meson.c +3
-0+} +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?
Quoted 5 lines
arch/arm/mach-meson/meson.c +0
-1.dt_compat = meson_common_board_compat, .l2c_aux_val = 0, .l2c_aux_mask = ~0, --2.5.0
Thanks for working on this!
Signature · 1 line
Carlo Caione
PATCH 1/2 ARM: meson: Adding support to retrieve serial and SoC revision
Hi all,
2016-02-17 21:34 GMT+01:00 Carlo Caione <[email protected]>:
Quoted 5 lines · Romain Perier
Quoted 2 lines
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...
Quoted 4 lines
Quoted 2 lines
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.
Quoted 33 lines
Quoted 33 lines
a
soc_device, and finally we expose everything to the system.Signed-off-by Romain Perier <[email protected]>--- arch/arm/mach-meson/meson.c | 55 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)arch/arm/mach-meson/meson.c +8
-0@@ -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, };
Quoted 11 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 42
Quoted 9 lines
arch/arm/mach-meson/meson.c +9
-0+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 ?
Quoted 28 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 50
Quoted 9 lines
arch/arm/mach-meson/meson.c +8
-0+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
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 53
Quoted 3 lines
arch/arm/mach-meson/meson.c +3
-0+ ret =regmap_read (hwrev,0 , &val); +if (ret <0 ) { +pr_err ("Could not get SoC id\n" );
kfree(soc_dev_attr)arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 60
Quoted 7 lines
arch/arm/mach-meson/meson.c +7
-0+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
Quoted 16 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 72
Quoted 12 lines
arch/arm/mach-meson/meson.c +12
-0+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->revisionandsoc_dev_attr->soc_idalso 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.
Quoted 12 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 77
Quoted 5 lines
arch/arm/mach-meson/meson.c +5
-0+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.oarch/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
Quoted 8 lines
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 'structsoc_device*'
extern int of_platform_populate(structdevice_node*root,^
Ah, good catch ! I build everything using yocto, so I did not see these warnings ^^
Quoted 7 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 81
Quoted 4 lines
arch/arm/mach-meson/meson.c +3
-0+} +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
PATCH 1/2 ARM: meson: Adding support to retrieve serial and SoC revision
Quoted 1 line · Romain Perier
Hi all,
[cut]
Quoted 13 lines
Quoted 11 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 42
Quoted 9 lines
arch/arm/mach-meson/meson.c +9
-0+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?
Quoted 2 lines
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.
Quoted 18 lines
Quoted 17 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 72
Quoted 13 lines
arch/arm/mach-meson/meson.c +12
-0+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->revisionandsoc_dev_attr->soc_idalso any reason why you are not kasprintf-ing also family?
Quoted 2 lines
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
Quoted 21 lines
Quoted 12 lines
arch/arm/mach-meson/meson.c static const char * const meson_common_board_compat[] = { · line 77
Quoted 5 lines
arch/arm/mach-meson/meson.c +5
-0+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.oarch/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 typeQuoted 8 lines
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 'structsoc_device*'
extern int of_platform_populate(structdevice_node*root,^
Quoted 2 lines
Ah, good catch ! I build everything using yocto, so I did not see these warnings ^^
Please, do not use yocto for this ;)
Cheers,
Signature · 1 line
Carlo Caione | +39.340.80.30.096 | Endless
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
In reply to Romain PerierThese are the CBUS registers used to retrieve the revision and the identifier of the SoC on Meson8.
--- arch/arm/boot/dts/meson8b.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm/boot/dts/meson8b.dtsi +5 -0
@@ -99,6 +99,11 @@ }; }; + hwrev at c1107d4c { + compatible = "amlogic,meson8b-hwrev", "syscon"; + reg = <0xc1107d4c 0x460>; + }; + sram: sram at d9000000 { compatible = "mmio-sram"; reg = <0xd9000000 0x20000>;
Signature · 1 line
2.5.0
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 13 lines · 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 <[email protected]>--- arch/arm/boot/dts/meson8b.dtsi | 5 +++++ 1 file changed, 5 insertions(+)arch/arm/boot/dts/meson8b.dtsi +0
-0
arch/arm/boot/dts/meson8b.dtsi line 104
Quoted 6 lines
arch/arm/boot/dts/meson8b.dtsi +3
-0@@ -99,6 +99,11 @@ }; }; + hwrev at c1107d4c { + compatible = "amlogic,meson8b-hwrev", "syscon"; + reg = <0xc1107d4c 0x460>;
Interesting. Where did you get 0x460?
arch/arm/boot/dts/meson8b.dtsi line 109
Quoted 5 lines
arch/arm/boot/dts/meson8b.dtsi +2
-0+ }; + sram: sram at 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.
Signature · 1 line
Carlo Caione
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Hi,
2016-02-17 21:36 GMT+01:00 Carlo Caione <[email protected]>:
Quoted 17 lines · Romain Perier
Quoted 16 lines
These are the CBUS registers used to retrieve the revision and the identifier of the SoC on Meson8.
Signed-off-by Romain Perier <[email protected]>--- arch/arm/boot/dts/meson8b.dtsi | 5 +++++ 1 file changed, 5 insertions(+)arch/arm/boot/dts/meson8b.dtsi +0
-0@@ -99,6 +99,11 @@ }; };
Quoted 5 lines
arch/arm/boot/dts/meson8b.dtsi line 104
Quoted 3 lines
arch/arm/boot/dts/meson8b.dtsi +3
-0+ hwrev at 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 ?
Quoted 10 lines
arch/arm/boot/dts/meson8b.dtsi line 109
Quoted 6 lines
arch/arm/boot/dts/meson8b.dtsi +2
-0+ }; + sram: sram at 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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 32 lines · On Thursday 18 February 2016 13:33:04 Romain Perier wrote:
2016-02-17 21:36 GMT+01:00 Carlo Caione <[email protected]>:
Quoted 17 lines · Romain Perier
Quoted 16 lines
These are the CBUS registers used to retrieve the revision and the identifier of the SoC on Meson8.
Signed-off-by Romain Perier <[email protected]>--- arch/arm/boot/dts/meson8b.dtsi | 5 +++++ 1 file changed, 5 insertions(+)arch/arm/boot/dts/meson8b.dtsi +0
-0@@ -99,6 +99,11 @@ }; };Quoted 5 lines
arch/arm/boot/dts/meson8b.dtsi line 104
Quoted 3 lines
arch/arm/boot/dts/meson8b.dtsi +3
-0+ hwrev at 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_BASEis0xc1100000(CBUS is a larger block of registers,
like slcr on zynq)
- the serial is atCBUS_PHY_BASE+0x7d4c
- the revision is atCBUS_PHY_BASE+0x81a8
Quoted 3 lines
So I decided to create a
device_nodefor hw revision at0xc1107d4c, in this case the lenght is0x460...
Am I wrong ?
Yes, you should describe the device that is actually there, which would be something like
cbus at 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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 40 lines · Arnd Bergmann
Quoted 32 lines · On Thursday 18 February 2016 13:33:04 Romain Perier wrote:
2016-02-17 21:36 GMT+01:00 Carlo Caione <[email protected]>:
Quoted 17 lines · Romain Perier
Quoted 16 lines
These are the CBUS registers used to retrieve the revision and the identifier of the SoC on Meson8.
Signed-off-by Romain Perier <[email protected]>--- arch/arm/boot/dts/meson8b.dtsi | 5 +++++ 1 file changed, 5 insertions(+)arch/arm/boot/dts/meson8b.dtsi +0
-0@@ -99,6 +99,11 @@ }; };Quoted 5 lines
arch/arm/boot/dts/meson8b.dtsi line 104
Quoted 3 lines
arch/arm/boot/dts/meson8b.dtsi +3
-0+ hwrev at 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_BASEis0xc1100000(CBUS is a larger block of registers,
like slcr on zynq)
- the serial is atCBUS_PHY_BASE+0x7d4c
- the revision is atCBUS_PHY_BASE+0x81a8Quoted 3 lines
So I decided to create a
device_nodefor hw revision at0xc1107d4c, in this case the lenght is0x460...
Am I wrong ?Yes, you should describe the device that is actually there, which would be something like
Quoted 7 lines
cbus at 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?
Signature · 1 line
Carlo Caione
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 9 lines · On Thursday 18 February 2016 15:14:45 Carlo Caione wrote:
Quoted 9 lines
Yes, you should describe the device that is actually there, which would be something like
cbus at c1100000 { compatible = "amlogic,meson8b-cbus", "syscon"; reg = <0xc1100000 0x10000>; /* no idea what the length is, fix it */ };
Quoted 7 lines
Quoted 2 lines
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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 18 lines · Arnd Bergmann
Quoted 9 lines · On Thursday 18 February 2016 15:14:45 Carlo Caione wrote:
Quoted 9 lines
Yes, you should describe the device that is actually there, which would be something like
cbus at c1100000 { compatible = "amlogic,meson8b-cbus", "syscon"; reg = <0xc1100000 0x10000>; /* no idea what the length is, fix it */ };Quoted 7 lines
Quoted 2 lines
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?
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
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,
Signature · 1 line
Carlo Caione
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 11 lines · Carlo Caione
Quoted 5 lines · Arnd Bergmann
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
Signature · 1 line
Carlo Caione | +39.340.80.30.096 | Endless
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
In reply to Carlo CaioneQuoted 9 lines · Carlo Caione
Quoted 5 lines
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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 18 lines · On Thursday 18 February 2016 15:27:06 Daniel Drake wrote:
Quoted 9 lines · Carlo Caione
Quoted 5 lines
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#L87Having 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?
Quoted 7 lines
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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
In reply to Daniel DrakeQuoted 5 lines · 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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 37 lines · Arnd Bergmann
Quoted 5 lines · 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
0x1000and0x2fff, which would be offsets0x4000through0xbfff.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_REVISIONis in the middle of some apparently all unrelated registers:#define
PREG_ETH_REG00x2050
#definePREG_ETH_REG10x2051
#definePROD_TEST_REG10x2067
#definePROD_TEST_REG00x2068
#defineMETAL_REVISION0x206a
#defineADC_TOP_MISC0x206b
#defineDPLL_TOP_MISC0x206c
#defineANALOG_TOP_MISC0x206d
#defineAM_ANALOG_TOP_REG00x206e
#defineAM_ANALOG_TOP_REG10x206f
#definePREG_STICKY_REG00x207c
#definePREG_STICKY_REG10x207d
Quoted 7 lines
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_REVISIONandHW_REVin 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
Quoted 3 lines
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,
Signature · 1 line
Carlo Caione | +39.340.80.30.096 | Endless
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 6 lines · Carlo Caione
Quoted 3 lines
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,
Signature · 1 line
Carlo Caione
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
In reply to Arnd BergmannOn Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <[email protected]> wrote:
<cut>
Quoted 11 lines
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_REVISIONandHW_REVin 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.
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?
Signature · 1 line
Carlo Caione
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 51 lines · On Friday 26 February 2016 16:34:59 Carlo Caione wrote:
On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <[email protected]> wrote:
<cut>
Quoted 11 lines
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_REVISIONandHW_REVin 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.
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_AOcase
(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.
Quoted 1 line
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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 52 lines · Arnd Bergmann
Quoted 51 lines · On Friday 26 February 2016 16:34:59 Carlo Caione wrote:
On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <[email protected]> wrote:
<cut>
Quoted 11 lines
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_REVISIONandHW_REVin 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.
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_AOcase
(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.
Quoted 5 lines
Quoted 1 line
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?
Signature · 1 line
Carlo Caione
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 55 lines · On Friday 26 February 2016 17:43:54 Carlo Caione wrote:
Quoted 54 lines · Arnd Bergmann
Quoted 52 lines · On Friday 26 February 2016 16:34:59 Carlo Caione wrote:
On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <[email protected]> wrote:
<cut>
Quoted 11 lines
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_REVISIONandHW_REVin 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.
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_AOcase
(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?
Quoted 8 lines
Quoted 3 lines
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
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
Quoted 1 line · Arnd Bergmann
On Friday 26 February 2016 17:43:54 Carlo Caione wrote:
[cut]
Quoted 7 lines
Quoted 4 lines
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.
Quoted 3 lines
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,
Signature · 1 line
Carlo Caione
PATCH 2/2 ARM: dts: meson: Adding hwrev syscon node
In reply to Romain Perierarch/arm/boot/dts/meson8b.dtsi line 106
Quoted 13 lines · On Wednesday 17 February 2016 18:28:34 Romain Perier wrote:
arch/arm/boot/dts/meson8b.dtsi +4
-0@@ -99,6 +99,11 @@ }; }; + hwrev at 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