Re: [PATCH 2/2] drivers/gpio: Port gpio driver to layerscape platform

Re: [PATCH 2/2] drivers/gpio: Port gpio driver to layerscape platform

[Linus Walleij]

On Tue, Nov 3, 2015 at 12:19 PM, Liu Gang <Gang.Liu@freescale.com> wrote:

> Layerscape has the same ip block/controller as
> GPIO on powerpc platform(MPC8XXX).
>
> So use portable i/o accessors, as in_be32/out_be32
> accessors are Power architecture specific whereas
> ioread32/iowrite32 and ioread32be/iowrite32be are
> available in other architectures.
>
> Layerscape GPIO controller's registers may be big
> or little endian, so the code needs to get the
> endian property from DTB, then make additional
> functions to fit right register read/write
> operations.
>
> Currently the code can support ls2080a GPIO with
> little endian registers. And it can also work well
> on other layerscape platform with big endian GPIO
> registers.
>
> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
> Signed-off-by: Shaveta Leekha <shaveta@freescale.com>

This doesn't seem good. You are starting to duplicate stuff
that is already available inside the gpio-generic.c driver.

Why can't this driver just select GPIO_GENERIC in
Kconfig and use the common code for handling
the endianness accessors?

bgpio_init() takes a flag
BGPIOF_BIG_ENDIAN_BYTE_ORDER
to make all accesses big endian, so using the
generic GPIO library would save you a lot of code.

Just look at other drivers for those selecting GPIO_GENERIC,
including <linux/basic_mmio_gpio.h> and calling bgpio_init().
You might want to keep some local custom BE code around
the IRQ parts.

But I would do two patches:
- One that switches MPC8xxx to using GENERIC_GPIO
- One that adds BE support using that infrastructure

It will result in a lot less code. I think.

Yours,
Linus Walleij

Re: [PATCH 1/2] ls2080a/dts: Add little endian property for GPIO IP block

[Linus Walleij]

On Tue, Nov 3, 2015 at 12:19 PM, Liu Gang <Gang.Liu@freescale.com> wrote:

> The GPIO block for ls2080a platform has little endian registers,
> the GPIO driver needs this property to read/write registers by
> right interface.
>
> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> index f2455c5..c836dab 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> @@ -10,6 +10,9 @@ Required properties:
>    the second cell is used to specify the gpio polarity:
>        0 = active high
>        1 = active low
> +- little-endian : Should be set if the GPIO has little endian
> +                 registers. No the property means the GPIO
> +                 registers are big endian mode.

That is a very generic binding and I would like the devicetree
maintainers to say something about this.

I would be OK if this is specified for *all* gpiochips in
Documentation/devicetree/bindings/gpio/gpio.txt
or even higher up in the desriptions.

Just for Freescale seems a bit too local.

Yours,
Linus Walleij

Re: [PATCH 1/2] ls2080a/dts: Add little endian property for GPIO IP block

[Li Yang]

On Mon, Nov 16, 2015 at 9:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Nov 3, 2015 at 12:19 PM, Liu Gang <Gang.Liu@freescale.com> wrote:
>
>> The GPIO block for ls2080a platform has little endian registers,
>> the GPIO driver needs this property to read/write registers by
>> right interface.
>>
>> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
>> index f2455c5..c836dab 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
>> @@ -10,6 +10,9 @@ Required properties:
>>    the second cell is used to specify the gpio polarity:
>>        0 = active high
>>        1 = active low
>> +- little-endian : Should be set if the GPIO has little endian
>> +                 registers. No the property means the GPIO
>> +                 registers are big endian mode.
>
> That is a very generic binding and I would like the devicetree
> maintainers to say something about this.
>
> I would be OK if this is specified for *all* gpiochips in
> Documentation/devicetree/bindings/gpio/gpio.txt
> or even higher up in the desriptions.
>
> Just for Freescale seems a bit too local.

There is already a generic definition at
Documentation/devicetree/bindings/common-properties.txt.  But it will
be special for Freescale controller to say that the default is
big-endian for backward compatibility.

Regards,
Leo

Re: [PATCH 1/2] ls2080a/dts: Add little endian property for GPIO IP block

[Scott Wood]

On Mon, 2015-11-16 at 12:56 -0600, Li Yang wrote:
> On Mon, Nov 16, 2015 at 9:11 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> wrote:
> > On Tue, Nov 3, 2015 at 12:19 PM, Liu Gang <Gang.Liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > The GPIO block for ls2080a platform has little endian registers,
> > > the GPIO driver needs this property to read/write registers by
> > > right interface.
> > > 
> > > Signed-off-by: Liu Gang <Gang.Liu-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > 
> > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> > > b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> > > index f2455c5..c836dab 100644
> > > --- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> > > +++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> > > @@ -10,6 +10,9 @@ Required properties:
> > >    the second cell is used to specify the gpio polarity:
> > >        0 = active high
> > >        1 = active low
> > > +- little-endian : Should be set if the GPIO has little endian
> > > +                 registers. No the property means the GPIO
> > > +                 registers are big endian mode.
> > 
> > That is a very generic binding and I would like the devicetree
> > maintainers to say something about this.
> > 
> > I would be OK if this is specified for *all* gpiochips in
> > Documentation/devicetree/bindings/gpio/gpio.txt
> > or even higher up in the desriptions.
> > 
> > Just for Freescale seems a bit too local.
> 
> There is already a generic definition at
> Documentation/devicetree/bindings/common-properties.txt.  But it will
> be special for Freescale controller to say that the default is
> big-endian for backward compatibility.

Also, common-properties.txt says "if a binding supports these properties" so
this patch serves to indicate that this binding does support the little-endian
property.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ls2080a/dts: Add little endian property for GPIO IP block

[Linus Walleij]

On Mon, Nov 16, 2015 at 7:56 PM, Li Yang <leoli@freescale.com> wrote:
> On Mon, Nov 16, 2015 at 9:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Nov 3, 2015 at 12:19 PM, Liu Gang <Gang.Liu@freescale.com> wrote:
>>
>>> The GPIO block for ls2080a platform has little endian registers,
>>> the GPIO driver needs this property to read/write registers by
>>> right interface.
>>>
>>> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
>>> index f2455c5..c836dab 100644
>>> --- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
>>> @@ -10,6 +10,9 @@ Required properties:
>>>    the second cell is used to specify the gpio polarity:
>>>        0 = active high
>>>        1 = active low
>>> +- little-endian : Should be set if the GPIO has little endian
>>> +                 registers. No the property means the GPIO
>>> +                 registers are big endian mode.
>>
>> That is a very generic binding and I would like the devicetree
>> maintainers to say something about this.
>>
>> I would be OK if this is specified for *all* gpiochips in
>> Documentation/devicetree/bindings/gpio/gpio.txt
>> or even higher up in the desriptions.
>>
>> Just for Freescale seems a bit too local.
>
> There is already a generic definition at
> Documentation/devicetree/bindings/common-properties.txt.  But it will
> be special for Freescale controller to say that the default is
> big-endian for backward compatibility.

OK! Why not reference that just like you reference gpio.txt?
It fooled me so it will fool others.

Yours,
Linus Walleij

[PATCH 1/2] ls2080a/dts: Add little endian property for GPIO IP block

[Liu Gang]

The GPIO block for ls2080a platform has little endian registers,
the GPIO driver needs this property to read/write registers by
right interface.

Signed-off-by: Liu Gang <Gang.Liu@freescale.com>

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
index f2455c5..c836dab 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
@@ -10,6 +10,9 @@ Required properties:
   the second cell is used to specify the gpio polarity:
       0 = active high
       1 = active low
+- little-endian : Should be set if the GPIO has little endian
+		  registers. No the property means the GPIO
+		  registers are big endian mode.
 
 Example:
 
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
index f3c59f9..41bb8c1 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
@@ -277,6 +277,7 @@
 			reg = <0x0 0x2300000 0x0 0x10000>;
 			interrupts = <0 36 0x4>; /* Level high type */
 			gpio-controller;
+			little-endian;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
@@ -287,6 +288,7 @@
 			reg = <0x0 0x2310000 0x0 0x10000>;
 			interrupts = <0 36 0x4>; /* Level high type */
 			gpio-controller;
+			little-endian;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
@@ -297,6 +299,7 @@
 			reg = <0x0 0x2320000 0x0 0x10000>;
 			interrupts = <0 37 0x4>; /* Level high type */
 			gpio-controller;
+			little-endian;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
@@ -307,6 +310,7 @@
 			reg = <0x0 0x2330000 0x0 0x10000>;
 			interrupts = <0 37 0x4>; /* Level high type */
 			gpio-controller;
+			little-endian;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
-- 
2.1.0.27.g96db324

Re: [PATCH 1/2] ls2080a/dts: Add little endian property for GPIO IP block

[Rob Herring]

On Wed, Nov 04, 2015 at 02:48:23PM +0800, Liu Gang wrote:
> The GPIO block for ls2080a platform has little endian registers,
> the GPIO driver needs this property to read/write registers by
> right interface.
> 
> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>

Acked-by: Rob Herring <robh@kernel.org>


> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> index f2455c5..c836dab 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> @@ -10,6 +10,9 @@ Required properties:
>    the second cell is used to specify the gpio polarity:
>        0 = active high
>        1 = active low
> +- little-endian : Should be set if the GPIO has little endian
> +		  registers. No the property means the GPIO
> +		  registers are big endian mode.
>  
>  Example:
>  
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> index f3c59f9..41bb8c1 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> @@ -277,6 +277,7 @@
>  			reg = <0x0 0x2300000 0x0 0x10000>;
>  			interrupts = <0 36 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -287,6 +288,7 @@
>  			reg = <0x0 0x2310000 0x0 0x10000>;
>  			interrupts = <0 36 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -297,6 +299,7 @@
>  			reg = <0x0 0x2320000 0x0 0x10000>;
>  			interrupts = <0 37 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -307,6 +310,7 @@
>  			reg = <0x0 0x2330000 0x0 0x10000>;
>  			interrupts = <0 37 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> -- 
> 2.1.0.27.g96db324
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ls2080a/dts: Add little endian property for GPIO IP block

[Shawn Guo]

On Wed, Nov 04, 2015 at 02:48:23PM +0800, Liu Gang wrote:
> The GPIO block for ls2080a platform has little endian registers,
> the GPIO driver needs this property to read/write registers by
> right interface.
> 
> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>

Please do not mix binding doc together with dts changes in one patch.

Shawn

> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> index f2455c5..c836dab 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mpc8xxx.txt
> @@ -10,6 +10,9 @@ Required properties:
>    the second cell is used to specify the gpio polarity:
>        0 = active high
>        1 = active low
> +- little-endian : Should be set if the GPIO has little endian
> +		  registers. No the property means the GPIO
> +		  registers are big endian mode.
>  
>  Example:
>  
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> index f3c59f9..41bb8c1 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
> @@ -277,6 +277,7 @@
>  			reg = <0x0 0x2300000 0x0 0x10000>;
>  			interrupts = <0 36 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -287,6 +288,7 @@
>  			reg = <0x0 0x2310000 0x0 0x10000>;
>  			interrupts = <0 36 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -297,6 +299,7 @@
>  			reg = <0x0 0x2320000 0x0 0x10000>;
>  			interrupts = <0 37 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> @@ -307,6 +310,7 @@
>  			reg = <0x0 0x2330000 0x0 0x10000>;
>  			interrupts = <0 37 0x4>; /* Level high type */
>  			gpio-controller;
> +			little-endian;
>  			#gpio-cells = <2>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> -- 
> 2.1.0.27.g96db324
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>