| Message ID | 1419967718-26909-6-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <robh@kernel.org> > > The Versatile boards have the same sysregs as other ARM Ltd boards. Add > the nodes in preparation to enable support for 24MHz counter as > sched_clock and MMC card detect and write protect support. > > Signed-off-by: Rob Herring <robh@kernel.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Linus Walleij <linus.walleij@linaro.org> > --- > arch/arm/boot/dts/versatile-ab.dts | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts > index 27d0d9c..62f04b0 100644 > --- a/arch/arm/boot/dts/versatile-ab.dts > +++ b/arch/arm/boot/dts/versatile-ab.dts > @@ -252,6 +252,29 @@ > #size-cells = <1>; > ranges = <0 0x10000000 0x10000>; > > + sysreg@0 { > + compatible = "arm,vexpress-sysreg"; vexpress? No... compatible = "syscon"; > + reg = <0x00000 0x1000>; > + > + v2m_led_gpios: sys_led@08 { > + compatible = "arm,vexpress-sysreg,sys_led"; > + gpio-controller; > + #gpio-cells = <2>; > + }; These are not GPIOs. These are LED registers really. see how to use LEDs from drivers/leds/leds-syscon.c and bindings. example in: arch/arm/boot/dts/integrator.dtsi Very straight-forward I think. > + > + v2m_mmc_gpios: sys_mci@48 { > + compatible = "arm,vexpress-sysreg,sys_mci"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + v2m_flash_gpios: sys_flash@4c { > + compatible = "arm,vexpress-sysreg,sys_flash"; > + gpio-controller; > + #gpio-cells = <2>; > + }; I don't have drivers for these "gpio controllers" and I don't think they are GPIOs either, since they are not general purpose at all. For the latter I have some code boiling for the Integrators: https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=multiplatform&id=9c28e114ec957fadf1dade382d8e8b8aa43c8426 https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=multiplatform&id=a3d4512abb85de290cdca0586bcff9147833917f Yours, Linus Walleij
Adding VExpress maintainers... On Thu, Jan 8, 2015 at 1:44 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: > >> From: Rob Herring <robh@kernel.org> >> >> The Versatile boards have the same sysregs as other ARM Ltd boards. Add >> the nodes in preparation to enable support for 24MHz counter as >> sched_clock and MMC card detect and write protect support. >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> --- >> arch/arm/boot/dts/versatile-ab.dts | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts >> index 27d0d9c..62f04b0 100644 >> --- a/arch/arm/boot/dts/versatile-ab.dts >> +++ b/arch/arm/boot/dts/versatile-ab.dts >> @@ -252,6 +252,29 @@ >> #size-cells = <1>; >> ranges = <0 0x10000000 0x10000>; >> >> + sysreg@0 { >> + compatible = "arm,vexpress-sysreg"; > > vexpress? No... Compatible with yes. Should perhaps be '"arm,versatile-sysreg", "arm,vexpress-sysreg"' instead. Kind of backwards, as really the versatile came first, but it would work. Or we just need another match entry in the kernel. I copied this whole chunk as is from VExpress and verified these sub nodes are all the same hence why it is here. > compatible = "syscon"; maybe? VExpress is missing that then... >> + reg = <0x00000 0x1000>; >> + >> + v2m_led_gpios: sys_led@08 { >> + compatible = "arm,vexpress-sysreg,sys_led"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; > > These are not GPIOs. These are LED registers really. A register bit that controls an i/o signal sounds like a GPIO to me. > see how to use LEDs from drivers/leds/leds-syscon.c and bindings. > example in: > arch/arm/boot/dts/integrator.dtsi > > Very straight-forward I think. So we have 2 implementations and bindings for roughly the same hardware? Great! >> + v2m_mmc_gpios: sys_mci@48 { >> + compatible = "arm,vexpress-sysreg,sys_mci"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + >> + v2m_flash_gpios: sys_flash@4c { >> + compatible = "arm,vexpress-sysreg,sys_flash"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; > > I don't have drivers for these "gpio controllers" and I don't think they are > GPIOs either, since they are not general purpose at all. They are only general purpose until you connect the i/o lines to something. But yes, if we went around making every misc internal control line in SOCs a 1-bit GPIO controller that would be pretty crazy. Anyway, most of this is not actually used ATM on Versatile, but it is present in VExpress. I added it only for sched_clock. We need to resolve this with VExpress platforms, but it's already in use for MMC and LEDs. Rob > For the latter I have some code boiling for the Integrators: > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=multiplatform&id=9c28e114ec957fadf1dade382d8e8b8aa43c8426 > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=multiplatform&id=a3d4512abb85de290cdca0586bcff9147833917f > > Yours, > Linus Walleij
On Fri, Jan 9, 2015 at 12:53 AM, Rob Herring <robherring2@gmail.com> wrote: > On Thu, Jan 8, 2015 at 1:44 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: >> compatible = "syscon"; > > maybe? VExpress is missing that then... I don't like the way some vexpress stuff has been done I think, just not enough reviewing power :( I've tried to make an as clean separation as possible in the Integrator as it has been refactored with a minimum of time pressure and I tried to make it as reusable as possible. But it doesn't necessarily mean I did the right thing all the time ... >>> + reg = <0x00000 0x1000>; >>> + >>> + v2m_led_gpios: sys_led@08 { >>> + compatible = "arm,vexpress-sysreg,sys_led"; >>> + gpio-controller; >>> + #gpio-cells = <2>; >>> + }; >> >> These are not GPIOs. These are LED registers really. > > A register bit that controls an i/o signal sounds like a GPIO to me. Are they described as general purpose in the manual for the board? In the ARM reference design manuals I've seen these bits are described as for one purpose only. I mean you can claim the memory RE signal is "a bit that controls an I/O signal" as well, but we have to think about the abstraction here. >> see how to use LEDs from drivers/leds/leds-syscon.c and bindings. >> example in: >> arch/arm/boot/dts/integrator.dtsi >> >> Very straight-forward I think. > > So we have 2 implementations and bindings for roughly the same hardware? Great! The syscon stuff is designed to be reusable for machines outside the Versatile and ARM reference families. Yours, Linus Walleij
Cc'ing Pawel since he wrote the sysreg code On Fri, Jan 09, 2015 at 07:10:36AM +0000, Linus Walleij wrote: > On Fri, Jan 9, 2015 at 12:53 AM, Rob Herring <robherring2@gmail.com> wrote: > > On Thu, Jan 8, 2015 at 1:44 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > >> On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: > >> compatible = "syscon"; > > > > maybe? VExpress is missing that then... > > I don't like the way some vexpress stuff has been done I think, just > not enough reviewing power :( I think that "syscon" is not meant to be there in the first place and it was done on purpose for vexpress but I need Pawel to confirm and shed light on this. > I've tried to make an as clean separation as possible in the Integrator > as it has been refactored with a minimum of time pressure and > I tried to make it as reusable as possible. But it doesn't necessarily > mean I did the right thing all the time ... > > >>> + reg = <0x00000 0x1000>; > >>> + > >>> + v2m_led_gpios: sys_led@08 { > >>> + compatible = "arm,vexpress-sysreg,sys_led"; > >>> + gpio-controller; > >>> + #gpio-cells = <2>; > >>> + }; > >> > >> These are not GPIOs. These are LED registers really. > > > > A register bit that controls an i/o signal sounds like a GPIO to me. > > Are they described as general purpose in the manual for the > board? > > In the ARM reference design manuals I've seen these bits are > described as for one purpose only. I mean you can claim the > memory RE signal is "a bit that controls an I/O signal" as well, > but we have to think about the abstraction here. I will have a look into this. Thanks, Lorenzo
On Thu, Jan 08, 2015 at 11:53:15PM +0000, Rob Herring wrote: > Adding VExpress maintainers... > > On Thu, Jan 8, 2015 at 1:44 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: > > > >> From: Rob Herring <robh@kernel.org> > >> > >> The Versatile boards have the same sysregs as other ARM Ltd boards. Add > >> the nodes in preparation to enable support for 24MHz counter as > >> sched_clock and MMC card detect and write protect support. > >> > >> Signed-off-by: Rob Herring <robh@kernel.org> > >> Cc: Russell King <linux@arm.linux.org.uk> > >> Cc: Linus Walleij <linus.walleij@linaro.org> > >> --- > >> arch/arm/boot/dts/versatile-ab.dts | 23 +++++++++++++++++++++++ > >> 1 file changed, 23 insertions(+) > >> > >> diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts > >> index 27d0d9c..62f04b0 100644 > >> --- a/arch/arm/boot/dts/versatile-ab.dts > >> +++ b/arch/arm/boot/dts/versatile-ab.dts > >> @@ -252,6 +252,29 @@ > >> #size-cells = <1>; > >> ranges = <0 0x10000000 0x10000>; > >> > >> + sysreg@0 { > >> + compatible = "arm,vexpress-sysreg"; > > > > vexpress? No... > > Compatible with yes. Should perhaps be '"arm,versatile-sysreg", > "arm,vexpress-sysreg"' instead. Kind of backwards, as really the > versatile came first, but it would work. Or we just need another match > entry in the kernel. versatile and vexpress sys registers are not compatible, so they should not be treated as such. Actually, versatile regs are not managed by a single entity driver in the kernel, so basically I really think you should not leave "arm,vexpress-sysreg" in the compatible list. > I copied this whole chunk as is from VExpress and verified these sub > nodes are all the same hence why it is here. > > > compatible = "syscon"; > > maybe? VExpress is missing that then... vexpress-sysreg is more than a "syscon" interface, so we can't match on it, it would be wrong. > > >> + reg = <0x00000 0x1000>; > >> + > >> + v2m_led_gpios: sys_led@08 { > >> + compatible = "arm,vexpress-sysreg,sys_led"; > >> + gpio-controller; > >> + #gpio-cells = <2>; > >> + }; > > > > These are not GPIOs. These are LED registers really. > > A register bit that controls an i/o signal sounds like a GPIO to me. To me too. I agree that definining a gpio-controller for every possible gpio pin would soon get unwieldy, but hey, the choice made for vexpress leds makes perfect sense to me, after all they are gpio signals connected to leds, and there is a driver for that in the kernel: drivers/leds/leds-gpio.c we could move this stuff to syscon-leds, but honestly I think is one of those things we could argue forever about that. > > see how to use LEDs from drivers/leds/leds-syscon.c and bindings. > > example in: > > arch/arm/boot/dts/integrator.dtsi > > > > Very straight-forward I think. > > So we have 2 implementations and bindings for roughly the same hardware? Great! See above. > >> + v2m_mmc_gpios: sys_mci@48 { > >> + compatible = "arm,vexpress-sysreg,sys_mci"; > >> + gpio-controller; > >> + #gpio-cells = <2>; > >> + }; > >> + > >> + v2m_flash_gpios: sys_flash@4c { > >> + compatible = "arm,vexpress-sysreg,sys_flash"; > >> + gpio-controller; > >> + #gpio-cells = <2>; > >> + }; > > > > I don't have drivers for these "gpio controllers" and I don't think they are > > GPIOs either, since they are not general purpose at all. > > They are only general purpose until you connect the i/o lines to > something. But yes, if we went around making every misc internal > control line in SOCs a 1-bit GPIO controller that would be pretty > crazy. > > Anyway, most of this is not actually used ATM on Versatile, but it is > present in VExpress. I added it only for sched_clock. We need to > resolve this with VExpress platforms, but it's already in use for MMC > and LEDs. I did not get what you meant here, in particular which bits you want us to fix/change/update on vexpress. Thanks, Lorenzo
On Thu, Jan 15, 2015 at 5:06 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Thu, Jan 08, 2015 at 11:53:15PM +0000, Rob Herring wrote: >> On Thu, Jan 8, 2015 at 1:44 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> > On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: >> >> + reg = <0x00000 0x1000>; >> >> + >> >> + v2m_led_gpios: sys_led@08 { >> >> + compatible = "arm,vexpress-sysreg,sys_led"; >> >> + gpio-controller; >> >> + #gpio-cells = <2>; >> >> + }; >> > >> > These are not GPIOs. These are LED registers really. >> >> A register bit that controls an i/o signal sounds like a GPIO to me. > > To me too. I agree that definining a gpio-controller for every possible > gpio pin would soon get unwieldy, but hey, the choice made for vexpress > leds makes perfect sense to me, after all they are gpio signals > connected to leds, and there is a driver for that in the kernel: > > drivers/leds/leds-gpio.c > > we could move this stuff to syscon-leds, but honestly I think is one of those > things we could argue forever about that. OK it's just so that the GPIO maintainer disagrees with the way gpio-controller is being used here, and I consequently NACK it so for the record add my: Nacked-by: Linus Walleij <linus.walleij@linaro.org> To this patch if/when merging it through ARM SoC. If the DT people think it is a good way to describe the hardware and override this then I will live with it I guess... Yours, Linus Walleij
diff --git a/arch/arm/boot/dts/versatile-ab.dts b/arch/arm/boot/dts/versatile-ab.dts index 27d0d9c..62f04b0 100644 --- a/arch/arm/boot/dts/versatile-ab.dts +++ b/arch/arm/boot/dts/versatile-ab.dts @@ -252,6 +252,29 @@ #size-cells = <1>; ranges = <0 0x10000000 0x10000>; + sysreg@0 { + compatible = "arm,vexpress-sysreg"; + reg = <0x00000 0x1000>; + + v2m_led_gpios: sys_led@08 { + compatible = "arm,vexpress-sysreg,sys_led"; + gpio-controller; + #gpio-cells = <2>; + }; + + v2m_mmc_gpios: sys_mci@48 { + compatible = "arm,vexpress-sysreg,sys_mci"; + gpio-controller; + #gpio-cells = <2>; + }; + + v2m_flash_gpios: sys_flash@4c { + compatible = "arm,vexpress-sysreg,sys_flash"; + gpio-controller; + #gpio-cells = <2>; + }; + }; + aaci@4000 { compatible = "arm,primecell"; reg = <0x4000 0x1000>;