| Message ID | 1407946582-20927-5-git-send-email-grygorii.strashko@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Wed, 13 Aug 2014 19:16:22 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: > Add Keystone 2 DSP GPIO nodes. > DSP GPIO banks 0-7 correspond to DSP0-DSP7 > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > arch/arm/boot/dts/k2hk.dtsi | 56 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/arch/arm/boot/dts/k2hk.dtsi b/arch/arm/boot/dts/k2hk.dtsi > index 321ba2f..009e180 100644 > --- a/arch/arm/boot/dts/k2hk.dtsi > +++ b/arch/arm/boot/dts/k2hk.dtsi > @@ -50,5 +50,61 @@ > #interrupt-cells = <1>; > ti,syscon-dev = <&devctrl 0x2a0>; > }; > + > + dspgpio0: keystone_dsp_gpio@02620240 { > + compatible = "ti,keystone-mctrl-gpio"; > + gpio-controller; > + #gpio-cells = <2>; > + gpio,syscon-dev = <&devctrl 0x240>; > + }; > + > + dspgpio1: keystone_dsp_gpio@2620244 { > + compatible = "ti,keystone-mctrl-gpio"; > + gpio-controller; > + #gpio-cells = <2>; > + gpio,syscon-dev = <&devctrl 0x244>; > + }; ... > + dspgpio7: keystone_dsp_gpio@262025C { > + compatible = "ti,keystone-mctrl-gpio"; > + gpio-controller; > + #gpio-cells = <2>; > + gpio,syscon-dev = <&devctrl 0x25c>; > + }; So, devctrl is a syscon device and this DTS introduce several identical GPIO descriptions? On my opinion this should be placed in the gpio-syscon.c, where you can add support for ti,keystone-dsp0{..7}-gpio. Such change will avoid parts 2 and 3 of this patch. static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { .compatible = "ti,keystone-syscon", .dat_bit_offset = 0x240 * 8, ... .set = etc... }; ---
Thu, 14 Aug 2014 15:13:39 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: > Hi Alexander, > > On 08/13/2014 07:06 PM, Alexander Shiyan wrote: > > Wed, 13 Aug 2014 19:16:22 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: > >> Add Keystone 2 DSP GPIO nodes. > >> DSP GPIO banks 0-7 correspond to DSP0-DSP7 > >> > >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > >> --- > >> arch/arm/boot/dts/k2hk.dtsi | 56 +++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 56 insertions(+) > >> > >> diff --git a/arch/arm/boot/dts/k2hk.dtsi b/arch/arm/boot/dts/k2hk.dtsi > >> index 321ba2f..009e180 100644 > >> --- a/arch/arm/boot/dts/k2hk.dtsi > >> +++ b/arch/arm/boot/dts/k2hk.dtsi > >> @@ -50,5 +50,61 @@ > >> #interrupt-cells = <1>; > >> ti,syscon-dev = <&devctrl 0x2a0>; > >> }; > >> + > >> + dspgpio0: keystone_dsp_gpio@02620240 { > >> + compatible = "ti,keystone-mctrl-gpio"; > >> + gpio-controller; > >> + #gpio-cells = <2>; > >> + gpio,syscon-dev = <&devctrl 0x240>; > >> + }; > >> + > >> + dspgpio1: keystone_dsp_gpio@2620244 { > >> + compatible = "ti,keystone-mctrl-gpio"; > >> + gpio-controller; > >> + #gpio-cells = <2>; > >> + gpio,syscon-dev = <&devctrl 0x244>; > >> + }; > > ... > >> + dspgpio7: keystone_dsp_gpio@262025C { > >> + compatible = "ti,keystone-mctrl-gpio"; > >> + gpio-controller; > >> + #gpio-cells = <2>; > >> + gpio,syscon-dev = <&devctrl 0x25c>; > >> + }; > > > > So, devctrl is a syscon device and this DTS introduce several > > identical GPIO descriptions? > > > > On my opinion this should be placed in the gpio-syscon.c, > > where you can add support for ti,keystone-dsp0{..7}-gpio. > > Such change will avoid parts 2 and 3 of this patch. > > > > static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { > > .compatible = "ti,keystone-syscon", > > .dat_bit_offset = 0x240 * 8, > > ... > > .set = etc... > > }; > > So, if I understand you right, you propose to add 8 additional compatible > strings just to encode different register offsets. Is it? > 1) add "ti,keystone-mctrl-gpio0"..."ti,keystone-mctrl-gpio7" Yes, but replace "mctrl" with "dsp" since mctrl is not applicable here. > 2) add 8 structures keystone_mctrl_gpio0..keystone_mctrl_gpio7 in gpio-syscon.c > (which will have only different values of field - .dat_bit_offset = 0x2yy * 8,) > > 3) add 8 additional items in array syscon_gpio_ids > { > .compatible = "ti,keystone-mctrl-gpio0", > .data = &keystone_mctrl_gpio0, > }, ... > > I can do it if you strictly insist, BUT I don't like it :( > - just imagine how your driver will look look like if 5 or 6 SoCs will re-use it ;) > - as I mentioned in cover letter and commit messages even each SoC revision can have > different Syscon implementation with different registers offsets and with different > number of Syscon register ranges (for example for Keystone 2 we already have two > Syscon devices defined in DT). The initial version of this driver contains addresses and offsets in, but this approach has been criticized by DT maintainers. "different Syscon with different registers" - is OK, since we use compatible string in definition. If you have two identical syscon, just append an unique additional string and use it in this driver. > Now we already have 3 Keystone 2 SoC revisions (K2HK, K2L, K2E) and there are no guarantee > that the next revision k2X will have the same register offset for GPIO0. Then use more exact string for syscon-gpio, like ti,keystone-k2hk-dsp-gpio0. ---
Hi Alexander, On 08/13/2014 07:06 PM, Alexander Shiyan wrote: > Wed, 13 Aug 2014 19:16:22 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: >> Add Keystone 2 DSP GPIO nodes. >> DSP GPIO banks 0-7 correspond to DSP0-DSP7 >> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> arch/arm/boot/dts/k2hk.dtsi | 56 +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/arch/arm/boot/dts/k2hk.dtsi b/arch/arm/boot/dts/k2hk.dtsi >> index 321ba2f..009e180 100644 >> --- a/arch/arm/boot/dts/k2hk.dtsi >> +++ b/arch/arm/boot/dts/k2hk.dtsi >> @@ -50,5 +50,61 @@ >> #interrupt-cells = <1>; >> ti,syscon-dev = <&devctrl 0x2a0>; >> }; >> + >> + dspgpio0: keystone_dsp_gpio@02620240 { >> + compatible = "ti,keystone-mctrl-gpio"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + gpio,syscon-dev = <&devctrl 0x240>; >> + }; >> + >> + dspgpio1: keystone_dsp_gpio@2620244 { >> + compatible = "ti,keystone-mctrl-gpio"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + gpio,syscon-dev = <&devctrl 0x244>; >> + }; > ... >> + dspgpio7: keystone_dsp_gpio@262025C { >> + compatible = "ti,keystone-mctrl-gpio"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + gpio,syscon-dev = <&devctrl 0x25c>; >> + }; > > So, devctrl is a syscon device and this DTS introduce several > identical GPIO descriptions? > > On my opinion this should be placed in the gpio-syscon.c, > where you can add support for ti,keystone-dsp0{..7}-gpio. > Such change will avoid parts 2 and 3 of this patch. > > static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { > .compatible = "ti,keystone-syscon", > .dat_bit_offset = 0x240 * 8, > ... > .set = etc... > }; So, if I understand you right, you propose to add 8 additional compatible strings just to encode different register offsets. Is it? 1) add "ti,keystone-mctrl-gpio0"..."ti,keystone-mctrl-gpio7" 2) add 8 structures keystone_mctrl_gpio0..keystone_mctrl_gpio7 in gpio-syscon.c (which will have only different values of field - .dat_bit_offset = 0x2yy * 8,) 3) add 8 additional items in array syscon_gpio_ids { .compatible = "ti,keystone-mctrl-gpio0", .data = &keystone_mctrl_gpio0, }, ... I can do it if you strictly insist, BUT I don't like it :( - just imagine how your driver will look look like if 5 or 6 SoCs will re-use it ;) - as I mentioned in cover letter and commit messages even each SoC revision can have different Syscon implementation with different registers offsets and with different number of Syscon register ranges (for example for Keystone 2 we already have two Syscon devices defined in DT). Now we already have 3 Keystone 2 SoC revisions (K2HK, K2L, K2E) and there are no guarantee that the next revision k2X will have the same register offset for GPIO0. Best regards, -grygorii
Thu, 14 Aug 2014 18:57:08 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: ... > >>>> + dspgpio7: keystone_dsp_gpio@262025C { > >>>> + compatible = "ti,keystone-mctrl-gpio"; > >>>> + gpio-controller; > >>>> + #gpio-cells = <2>; > >>>> + gpio,syscon-dev = <&devctrl 0x25c>; > >>>> + }; > >>> > >>> So, devctrl is a syscon device and this DTS introduce several > >>> identical GPIO descriptions? > >>> > >>> On my opinion this should be placed in the gpio-syscon.c, > >>> where you can add support for ti,keystone-dsp0{..7}-gpio. > >>> Such change will avoid parts 2 and 3 of this patch. > >>> > >>> static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { > >>> .compatible = "ti,keystone-syscon", > >>> .dat_bit_offset = 0x240 * 8, > >>> ... > >>> .set = etc... > >>> }; > >> > >> So, if I understand you right, you propose to add 8 additional compatible > >> strings just to encode different register offsets. Is it? > >> 1) add "ti,keystone-mctrl-gpio0"..."ti,keystone-mctrl-gpio7" > > > > Yes, but replace "mctrl" with "dsp" since mctrl is not applicable here. > > > >> 2) add 8 structures keystone_mctrl_gpio0..keystone_mctrl_gpio7 in gpio-syscon.c > >> (which will have only different values of field - .dat_bit_offset = 0x2yy * 8,) > >> > >> 3) add 8 additional items in array syscon_gpio_ids > >> { > >> .compatible = "ti,keystone-mctrl-gpio0", > >> .data = &keystone_mctrl_gpio0, > >> }, ... > >> > >> I can do it if you strictly insist, BUT I don't like it :( > >> - just imagine how your driver will look look like if 5 or 6 SoCs will re-use it ;) > >> - as I mentioned in cover letter and commit messages even each SoC revision can have > >> different Syscon implementation with different registers offsets and with different > >> number of Syscon register ranges (for example for Keystone 2 we already have two > >> Syscon devices defined in DT). > > > > The initial version of this driver contains addresses and offsets in, but this approach has > > been criticized by DT maintainers. > > Could you provide link on this thread^, pls? Here is a link to first version: https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg01616.html Unfortunately, I lost thread for DT-related stuffs. ---
On 08/14/2014 03:12 PM, Alexander Shiyan wrote: > Thu, 14 Aug 2014 15:13:39 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: >> Hi Alexander, >> >> On 08/13/2014 07:06 PM, Alexander Shiyan wrote: >>> Wed, 13 Aug 2014 19:16:22 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: >>>> Add Keystone 2 DSP GPIO nodes. >>>> DSP GPIO banks 0-7 correspond to DSP0-DSP7 >>>> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>>> arch/arm/boot/dts/k2hk.dtsi | 56 +++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 56 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/k2hk.dtsi b/arch/arm/boot/dts/k2hk.dtsi >>>> index 321ba2f..009e180 100644 >>>> --- a/arch/arm/boot/dts/k2hk.dtsi >>>> +++ b/arch/arm/boot/dts/k2hk.dtsi >>>> @@ -50,5 +50,61 @@ >>>> #interrupt-cells = <1>; >>>> ti,syscon-dev = <&devctrl 0x2a0>; >>>> }; >>>> + >>>> + dspgpio0: keystone_dsp_gpio@02620240 { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x240>; >>>> + }; >>>> + >>>> + dspgpio1: keystone_dsp_gpio@2620244 { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x244>; >>>> + }; >>> ... >>>> + dspgpio7: keystone_dsp_gpio@262025C { >>>> + compatible = "ti,keystone-mctrl-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + gpio,syscon-dev = <&devctrl 0x25c>; >>>> + }; >>> >>> So, devctrl is a syscon device and this DTS introduce several >>> identical GPIO descriptions? >>> >>> On my opinion this should be placed in the gpio-syscon.c, >>> where you can add support for ti,keystone-dsp0{..7}-gpio. >>> Such change will avoid parts 2 and 3 of this patch. >>> >>> static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { >>> .compatible = "ti,keystone-syscon", >>> .dat_bit_offset = 0x240 * 8, >>> ... >>> .set = etc... >>> }; >> >> So, if I understand you right, you propose to add 8 additional compatible >> strings just to encode different register offsets. Is it? >> 1) add "ti,keystone-mctrl-gpio0"..."ti,keystone-mctrl-gpio7" > > Yes, but replace "mctrl" with "dsp" since mctrl is not applicable here. > >> 2) add 8 structures keystone_mctrl_gpio0..keystone_mctrl_gpio7 in gpio-syscon.c >> (which will have only different values of field - .dat_bit_offset = 0x2yy * 8,) >> >> 3) add 8 additional items in array syscon_gpio_ids >> { >> .compatible = "ti,keystone-mctrl-gpio0", >> .data = &keystone_mctrl_gpio0, >> }, ... >> >> I can do it if you strictly insist, BUT I don't like it :( >> - just imagine how your driver will look look like if 5 or 6 SoCs will re-use it ;) >> - as I mentioned in cover letter and commit messages even each SoC revision can have >> different Syscon implementation with different registers offsets and with different >> number of Syscon register ranges (for example for Keystone 2 we already have two >> Syscon devices defined in DT). > > The initial version of this driver contains addresses and offsets in, but this approach has > been criticized by DT maintainers. Could you provide link on this thread^, pls? Curious, it looks like Rob Herring has just proposed to encode offsets & bits in DT: "Re: [PATCH 2/6] leds: add device tree bindings for syscon LEDs" http://www.spinics.net/lists/arm-kernel/msg354182.html > > "different Syscon with different registers" - is OK, since we use compatible string in definition. > If you have two identical syscon, just append an unique additional string and use it in this driver. > >> Now we already have 3 Keystone 2 SoC revisions (K2HK, K2L, K2E) and there are no guarantee >> that the next revision k2X will have the same register offset for GPIO0. > > Then use more exact string for syscon-gpio, like ti,keystone-k2hk-dsp-gpio0. Nothing to say :) - DT is funny thing. It has such good feature as phandle, but people continue hard-coding relation between HW blocks in code :( Any way, Thanks for your comments. I'll wait a bit then update and resend. Best regards, -grygorii
On 08/14/2014 06:26 PM, Alexander Shiyan wrote: > Thu, 14 Aug 2014 18:57:08 +0300 ?? Grygorii Strashko <grygorii.strashko@ti.com>: > ... >>>>>> + dspgpio7: keystone_dsp_gpio@262025C { >>>>>> + compatible = "ti,keystone-mctrl-gpio"; >>>>>> + gpio-controller; >>>>>> + #gpio-cells = <2>; >>>>>> + gpio,syscon-dev = <&devctrl 0x25c>; >>>>>> + }; >>>>> >>>>> So, devctrl is a syscon device and this DTS introduce several >>>>> identical GPIO descriptions? >>>>> >>>>> On my opinion this should be placed in the gpio-syscon.c, >>>>> where you can add support for ti,keystone-dsp0{..7}-gpio. >>>>> Such change will avoid parts 2 and 3 of this patch. >>>>> >>>>> static const struct syscon_gpio_data ti_keystone_dsp0_gpio = { >>>>> .compatible = "ti,keystone-syscon", >>>>> .dat_bit_offset = 0x240 * 8, >>>>> ... >>>>> .set = etc... >>>>> }; >>>> >>>> So, if I understand you right, you propose to add 8 additional compatible >>>> strings just to encode different register offsets. Is it? >>>> 1) add "ti,keystone-mctrl-gpio0"..."ti,keystone-mctrl-gpio7" >>> >>> Yes, but replace "mctrl" with "dsp" since mctrl is not applicable here. >>> >>>> 2) add 8 structures keystone_mctrl_gpio0..keystone_mctrl_gpio7 in gpio-syscon.c >>>> (which will have only different values of field - .dat_bit_offset = 0x2yy * 8,) >>>> >>>> 3) add 8 additional items in array syscon_gpio_ids >>>> { >>>> .compatible = "ti,keystone-mctrl-gpio0", >>>> .data = &keystone_mctrl_gpio0, >>>> }, ... >>>> >>>> I can do it if you strictly insist, BUT I don't like it :( >>>> - just imagine how your driver will look look like if 5 or 6 SoCs will re-use it ;) >>>> - as I mentioned in cover letter and commit messages even each SoC revision can have >>>> different Syscon implementation with different registers offsets and with different >>>> number of Syscon register ranges (for example for Keystone 2 we already have two >>>> Syscon devices defined in DT). >>> >>> The initial version of this driver contains addresses and offsets in, but this approach has >>> been criticized by DT maintainers. >> >> Could you provide link on this thread^, pls? > > Here is a link to first version: > https://www.mail-archive.com/linux-gpio@vger.kernel.org/msg01616.html 10x > > Unfortunately, I lost thread for DT-related stuffs. > Oh, I've just checked ARM dts directory and found that constructions like this: vendor,syscon = <&syscon_phandle>; /// or even this vendor,syscon = <&syscon_phandle 0x60 2>; are widely used as of now. Also, According to the bindings doc, the Syscon is not a Linux specific definition (mfd/syscon.txt). Regards, -grygorii
Hi All, Alexander, I've updated gpio-syscon as requested in [3]. I still don't like it, but any way I did it :( Linus, I'd very appreciated if you can comment on these series. Personally, I like v1 [3], because this v2 is not elegant and will require constant code patching in case of adding new SoCs or new SoC's versions. This series intended to integrate Keystone 2 DSP GPIO controller functionality into gpio-syscon driver (drivers/gpio/gpio-syscon.c) as requested by Linus Walleij in [1]. On Keystone SOCs, ARM host can send interrupts to DSP cores using the DSP GPIO controller IP. Each DSP GPIO controller provides 28 IRQ signals for each DSP core. This is one of the component used by the IPC mechanism used on Keystone SOCs. Keystone 2 DSP GPIO controller has specific features: - each GPIO can be configured only as output pin; - setting GPIO value to 1 causes IRQ generation on target DSP core; - reading pin value returns 0 - if IRQ was handled or 1 - IRQ is still pending. The gpio-syscon driver was need to be updated to satisfy Keystone 2 SoC requirements: - special sequence of operations need to be used to assign output GPIO value. As result, first patch introduces SoC specific callback .set() to configure output GPIO value. Also, patch 3 was added to illustrate DSP GPIO configuration in DT used by Keystone 2. Related sicussions: [1] https://lkml.org/lkml/2014/7/16/170 [2] https://lkml.org/lkml/2014/7/23/352 [3] https://www.mail-archive.com/devicetree@vger.kernel.org/msg37863.html Grygorii Strashko (3): gpio: syscon: add soc specific callback to assign output value gpio: syscon: reuse for keystone 2 socs ARM: dts: keystone-k2hk: add dsp gpio controllers nodes .../bindings/gpio/gpio-mctrl-keystone.txt | 42 ++++++ arch/arm/boot/dts/k2hk.dtsi | 48 +++++++ drivers/gpio/gpio-syscon.c | 140 ++++++++++++++++++++ 3 files changed, 230 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mctrl-keystone.txt
diff --git a/arch/arm/boot/dts/k2hk.dtsi b/arch/arm/boot/dts/k2hk.dtsi index 321ba2f..009e180 100644 --- a/arch/arm/boot/dts/k2hk.dtsi +++ b/arch/arm/boot/dts/k2hk.dtsi @@ -50,5 +50,61 @@ #interrupt-cells = <1>; ti,syscon-dev = <&devctrl 0x2a0>; }; + + dspgpio0: keystone_dsp_gpio@02620240 { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x240>; + }; + + dspgpio1: keystone_dsp_gpio@2620244 { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x244>; + }; + + dspgpio2: keystone_dsp_gpio@2620248 { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x248>; + }; + + dspgpio3: keystone_dsp_gpio@262024c { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x24c>; + }; + + dspgpio4: keystone_dsp_gpio@2620250 { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x250>; + }; + + dspgpio5: keystone_dsp_gpio@2620254 { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x254>; + }; + + dspgpio6: keystone_dsp_gpio@2620258 { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x258>; + }; + + dspgpio7: keystone_dsp_gpio@262025C { + compatible = "ti,keystone-mctrl-gpio"; + gpio-controller; + #gpio-cells = <2>; + gpio,syscon-dev = <&devctrl 0x25c>; + }; }; };
Add Keystone 2 DSP GPIO nodes. DSP GPIO banks 0-7 correspond to DSP0-DSP7 Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- arch/arm/boot/dts/k2hk.dtsi | 56 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)