| Message ID | 1455730114-2547-3-git-send-email-romain.perier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier@gmail.com> 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@gmail.com> > --- > 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.
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
Hi, 2016-02-17 21:36 GMT+01:00 Carlo Caione <carlo@caione.org>: > On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier@gmail.com> 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@gmail.com> >> --- >> 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
On Thursday 18 February 2016 13:33:04 Romain Perier wrote: > 2016-02-17 21:36 GMT+01:00 Carlo Caione <carlo@caione.org>: > > On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier@gmail.com> 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@gmail.com> > >> --- > >> 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
On Thu, Feb 18, 2016 at 1:43 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 18 February 2016 13:33:04 Romain Perier wrote: >> 2016-02-17 21:36 GMT+01:00 Carlo Caione <carlo@caione.org>: >> > On Wed, Feb 17, 2016 at 6:28 PM, Romain Perier <romain.perier@gmail.com> 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@gmail.com> >> >> --- >> >> 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?
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
On Thu, Feb 18, 2016 at 3:22 PM, Arnd Bergmann <arnd@arndb.de> 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,
On Thu, Feb 18, 2016 at 10:04 PM, Carlo Caione <carlo@caione.org> wrote: > On Thu, Feb 18, 2016 at 3:22 PM, Arnd Bergmann <arnd@arndb.de> 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
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? Daniel
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
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
On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd@arndb.de> 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,
On Fri, Feb 19, 2016 at 2:25 PM, Carlo Caione <carlo@endlessm.com> 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,
On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd@arndb.de> 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?
On Friday 26 February 2016 16:34:59 Carlo Caione wrote: > On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd@arndb.de> 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
On Fri, Feb 26, 2016 at 5:00 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 26 February 2016 16:34:59 Carlo Caione wrote: >> On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd@arndb.de> 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?
On Friday 26 February 2016 17:43:54 Carlo Caione wrote: > On Fri, Feb 26, 2016 at 5:00 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 26 February 2016 16:34:59 Carlo Caione wrote: > >> On Fri, Feb 19, 2016 at 1:54 PM, Arnd Bergmann <arnd@arndb.de> 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
On Fri, Feb 26, 2016 at 6:00 PM, Arnd Bergmann <arnd@arndb.de> 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,
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>;
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@gmail.com> --- arch/arm/boot/dts/meson8b.dtsi | 5 +++++ 1 file changed, 5 insertions(+)