Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij  writes:

> On Thu, Dec 10, 2015 at 6:31 PM, Robert Jarzmik  
> wrote:
>> Linus Walleij  writes:
>>
- the GPDR (gpio direction register) shared access bothers me a bit
>>>
>>> How is it shared and between what users?
>>
>> It's shared between the pin controller and the gpio controller.
>
> OK then it may be one of these cases where we should jit the pin controller
> and the GPIO controller together in the same file (under drivers/pinctrl)
> to simplify the mess. We can do that in the NEXT merge window because
> right now I don't want any more crisscross between gpio and pin control
> as there are refactorings I'm piling up.

Well, maybe, but I don't know how to do it, due to the number of possibilities,
given that :
 - gpio-pxa.c should work for pxa27x device-tree
   (this would be possible with a pinctrl+gpio fusion)
 - gpio-pxa.c should work for pxa3xx device-tree
   (I don't know how to do this, pxa3xx uses pinctrl-single AFAIK)
 - gpio-pxa.c should work for pxa168 + mmp* device-tree
   (same as for pxa3xx)
 - gpio-pxa.c might should with pxa27x platform_data
   (this doesn't work yet fully, the wake-up pin give me headaches)
 - gpio-pxa.c should work with pxa25x platform_data
   (I don't see either how a merged pinctrl+gpio driver could address this)

All of this to say it looks a bit complicated to me to have a gpio+pinctrl in
the same file, but I might be missing something obvious.
 
> Another option is e.g. accessing the registers through regmap-mmio but
> it feels a bit like overkill for this...
Yes, overkill maybe, but maybe an idea. The nice thing about regmap is the debug
capabilities, the less good is the permanent need of locks ...

>> The odd thing with the pxa architecture is that the GPDR bit selects between 
>> 2
>> different alternate functions, even when the pin is not a GPIO. Strange 
>> design,
>> isn't it ?
>
> Probably just unfortunate naming.
>
> In my presentation "building GPIO and pin control from the ground up" I
> try to explain a bit how hardware engineers design these things...
> http://dflund.se/~triad/papers/pincontrol.pdf
Nice, I had already read it :)

>> As a consequence, both the gpio driver and pinctrl have to modify it, for
>> different purposes :
>>  - pinctrl will modify it to select a specific alternate function
>>  - gpio driver will modify it when the pin is a GPIO, to modify its 
>> direction.
>
> OK. Solutions per above, I guess it currently just optimistically hope
> we do not fiddle the same bit in parallell from the two drivers (which
> is maybe even possible to prove to be true).
Ok. I will think about it again in the next days.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Thu, Dec 10, 2015 at 6:31 PM, Robert Jarzmik  wrote:
> Linus Walleij  writes:
>
>>>- the GPDR (gpio direction register) shared access bothers me a bit
>>
>> How is it shared and between what users?
>
> It's shared between the pin controller and the gpio controller.

OK then it may be one of these cases where we should jit the pin controller
and the GPIO controller together in the same file (under drivers/pinctrl)
to simplify the mess. We can do that in the NEXT merge window because
right now I don't want any more crisscross between gpio and pin control
as there are refactorings I'm piling up.

Another option is e.g. accessing the registers through regmap-mmio but
it feels a bit like overkill for this...

> The odd thing with the pxa architecture is that the GPDR bit selects between 2
> different alternate functions, even when the pin is not a GPIO. Strange 
> design,
> isn't it ?

Probably just unfortunate naming.

In my presentation "building GPIO and pin control from the ground up" I
try to explain a bit how hardware engineers design these things...
http://dflund.se/~triad/papers/pincontrol.pdf

> As a consequence, both the gpio driver and pinctrl have to modify it, for
> different purposes :
>  - pinctrl will modify it to select a specific alternate function
>  - gpio driver will modify it when the pin is a GPIO, to modify its direction.

OK. Solutions per above, I guess it currently just optimistically hope
we do not fiddle the same bit in parallell from the two drivers (which
is maybe even possible to prove to be true).

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Thu, Dec 10, 2015 at 6:31 PM, Robert Jarzmik  wrote:
> Linus Walleij  writes:
>
>>>- the GPDR (gpio direction register) shared access bothers me a bit
>>
>> How is it shared and between what users?
>
> It's shared between the pin controller and the gpio controller.

OK then it may be one of these cases where we should jit the pin controller
and the GPIO controller together in the same file (under drivers/pinctrl)
to simplify the mess. We can do that in the NEXT merge window because
right now I don't want any more crisscross between gpio and pin control
as there are refactorings I'm piling up.

Another option is e.g. accessing the registers through regmap-mmio but
it feels a bit like overkill for this...

> The odd thing with the pxa architecture is that the GPDR bit selects between 2
> different alternate functions, even when the pin is not a GPIO. Strange 
> design,
> isn't it ?

Probably just unfortunate naming.

In my presentation "building GPIO and pin control from the ground up" I
try to explain a bit how hardware engineers design these things...
http://dflund.se/~triad/papers/pincontrol.pdf

> As a consequence, both the gpio driver and pinctrl have to modify it, for
> different purposes :
>  - pinctrl will modify it to select a specific alternate function
>  - gpio driver will modify it when the pin is a GPIO, to modify its direction.

OK. Solutions per above, I guess it currently just optimistically hope
we do not fiddle the same bit in parallell from the two drivers (which
is maybe even possible to prove to be true).

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij  writes:

> On Thu, Dec 10, 2015 at 6:31 PM, Robert Jarzmik  
> wrote:
>> Linus Walleij  writes:
>>
- the GPDR (gpio direction register) shared access bothers me a bit
>>>
>>> How is it shared and between what users?
>>
>> It's shared between the pin controller and the gpio controller.
>
> OK then it may be one of these cases where we should jit the pin controller
> and the GPIO controller together in the same file (under drivers/pinctrl)
> to simplify the mess. We can do that in the NEXT merge window because
> right now I don't want any more crisscross between gpio and pin control
> as there are refactorings I'm piling up.

Well, maybe, but I don't know how to do it, due to the number of possibilities,
given that :
 - gpio-pxa.c should work for pxa27x device-tree
   (this would be possible with a pinctrl+gpio fusion)
 - gpio-pxa.c should work for pxa3xx device-tree
   (I don't know how to do this, pxa3xx uses pinctrl-single AFAIK)
 - gpio-pxa.c should work for pxa168 + mmp* device-tree
   (same as for pxa3xx)
 - gpio-pxa.c might should with pxa27x platform_data
   (this doesn't work yet fully, the wake-up pin give me headaches)
 - gpio-pxa.c should work with pxa25x platform_data
   (I don't see either how a merged pinctrl+gpio driver could address this)

All of this to say it looks a bit complicated to me to have a gpio+pinctrl in
the same file, but I might be missing something obvious.
 
> Another option is e.g. accessing the registers through regmap-mmio but
> it feels a bit like overkill for this...
Yes, overkill maybe, but maybe an idea. The nice thing about regmap is the debug
capabilities, the less good is the permanent need of locks ...

>> The odd thing with the pxa architecture is that the GPDR bit selects between 
>> 2
>> different alternate functions, even when the pin is not a GPIO. Strange 
>> design,
>> isn't it ?
>
> Probably just unfortunate naming.
>
> In my presentation "building GPIO and pin control from the ground up" I
> try to explain a bit how hardware engineers design these things...
> http://dflund.se/~triad/papers/pincontrol.pdf
Nice, I had already read it :)

>> As a consequence, both the gpio driver and pinctrl have to modify it, for
>> different purposes :
>>  - pinctrl will modify it to select a specific alternate function
>>  - gpio driver will modify it when the pin is a GPIO, to modify its 
>> direction.
>
> OK. Solutions per above, I guess it currently just optimistically hope
> we do not fiddle the same bit in parallell from the two drivers (which
> is maybe even possible to prove to be true).
Ok. I will think about it again in the next days.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij  writes:

>>- the GPDR (gpio direction register) shared access bothers me a bit
>
> How is it shared and between what users?
It's shared between the pin controller and the gpio controller.

The odd thing with the pxa architecture is that the GPDR bit selects between 2
different alternate functions, even when the pin is not a GPIO. Strange design,
isn't it ?

As a consequence, both the gpio driver and pinctrl have to modify it, for
different purposes :
 - pinctrl will modify it to select a specific alternate function
 - gpio driver will modify it when the pin is a GPIO, to modify its direction.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Thu, Dec 10, 2015 at 8:28 AM, Robert Jarzmik  wrote:
> Linus Walleij  writes:
>
>> On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik  
>> wrote:
>>
>>> Hi Linus, Alexandre and Haojian,
>>>
>>> This serie aims at several cleanups and improvements in the pxa gpio 
>>> driver, to
>>
>> I have concerns about this series.
>>
>> I am worried that joining the banks into one gpio_chip makes it
>> impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
>> preferrable when using a chained handler if e.g. one bank has
>> one IRQ line.
>>
>> But overall that depends on how the IRQs map on this hardware.
>> Can you describe how the GPIO IRQs work on the PXA27x?
> Of course.
>
> For PXA27x, there are 3 interrupts directly connected to the CPU of the SoC,
> ie. the primary irq controller :
>  - one is only triggered if GPIO0 has a rising/falling edge
>  - one is only triggered if GPIO1 has a rising/falling edge
>  - the last is triggered if any GPIOn has a rising/falling edge (n >= 2)
>
> The condition to program the rising/falling edge which implies the interrupt 
> to
> be asserted is in a GPIO block register, GFER and GRER (1 bit per GPIO).
>
> The fact that the last interrupt (let's call it gpiomux_irq) is triggered by
> GPIOs from _all_ the banks makes me believe it's a single IP block, ie. a 
> single
> chip.

OK you're probably right. So GPIO0 and 1 are special cases and the
rest a muxed GPIO case. That's sufficiently odd to warrant its own
irqdomain and not use GPIOLIB_IRQCHIP.

I guess I will go ahead and merge this, simply.

> Now if you have concerns with this, then maybe you can advise another 
> approach,
> I'm pretty open. The final goal will be for me :
>  - gpio and pinctrl have to cooperate
>- today, with the current state, it's impossible to map pins 0..127 to 
> gpios
>  0..127, at least in a device-tree .dts file

OK sounds good.

>- the GPDR (gpio direction register) shared access bothers me a bit

How is it shared and between what users?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij  writes:

>>- the GPDR (gpio direction register) shared access bothers me a bit
>
> How is it shared and between what users?
It's shared between the pin controller and the gpio controller.

The odd thing with the pxa architecture is that the GPDR bit selects between 2
different alternate functions, even when the pin is not a GPIO. Strange design,
isn't it ?

As a consequence, both the gpio driver and pinctrl have to modify it, for
different purposes :
 - pinctrl will modify it to select a specific alternate function
 - gpio driver will modify it when the pin is a GPIO, to modify its direction.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Thu, Dec 10, 2015 at 8:28 AM, Robert Jarzmik  wrote:
> Linus Walleij  writes:
>
>> On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik  
>> wrote:
>>
>>> Hi Linus, Alexandre and Haojian,
>>>
>>> This serie aims at several cleanups and improvements in the pxa gpio 
>>> driver, to
>>
>> I have concerns about this series.
>>
>> I am worried that joining the banks into one gpio_chip makes it
>> impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
>> preferrable when using a chained handler if e.g. one bank has
>> one IRQ line.
>>
>> But overall that depends on how the IRQs map on this hardware.
>> Can you describe how the GPIO IRQs work on the PXA27x?
> Of course.
>
> For PXA27x, there are 3 interrupts directly connected to the CPU of the SoC,
> ie. the primary irq controller :
>  - one is only triggered if GPIO0 has a rising/falling edge
>  - one is only triggered if GPIO1 has a rising/falling edge
>  - the last is triggered if any GPIOn has a rising/falling edge (n >= 2)
>
> The condition to program the rising/falling edge which implies the interrupt 
> to
> be asserted is in a GPIO block register, GFER and GRER (1 bit per GPIO).
>
> The fact that the last interrupt (let's call it gpiomux_irq) is triggered by
> GPIOs from _all_ the banks makes me believe it's a single IP block, ie. a 
> single
> chip.

OK you're probably right. So GPIO0 and 1 are special cases and the
rest a muxed GPIO case. That's sufficiently odd to warrant its own
irqdomain and not use GPIOLIB_IRQCHIP.

I guess I will go ahead and merge this, simply.

> Now if you have concerns with this, then maybe you can advise another 
> approach,
> I'm pretty open. The final goal will be for me :
>  - gpio and pinctrl have to cooperate
>- today, with the current state, it's impossible to map pins 0..127 to 
> gpios
>  0..127, at least in a device-tree .dts file

OK sounds good.

>- the GPDR (gpio direction register) shared access bothers me a bit

How is it shared and between what users?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij  writes:

> On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik  
> wrote:
>
>> Hi Linus, Alexandre and Haojian,
>>
>> This serie aims at several cleanups and improvements in the pxa gpio driver, 
>> to
>
> I have concerns about this series.
>
> I am worried that joining the banks into one gpio_chip makes it
> impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
> preferrable when using a chained handler if e.g. one bank has
> one IRQ line.
>
> But overall that depends on how the IRQs map on this hardware.
> Can you describe how the GPIO IRQs work on the PXA27x?
Of course.

For PXA27x, there are 3 interrupts directly connected to the CPU of the SoC,
ie. the primary irq controller :
 - one is only triggered if GPIO0 has a rising/falling edge
 - one is only triggered if GPIO1 has a rising/falling edge
 - the last is triggered if any GPIOn has a rising/falling edge (n >= 2)

The condition to program the rising/falling edge which implies the interrupt to
be asserted is in a GPIO block register, GFER and GRER (1 bit per GPIO).

The fact that the last interrupt (let's call it gpiomux_irq) is triggered by
GPIOs from _all_ the banks makes me believe it's a single IP block, ie. a single
chip.

Now if you have concerns with this, then maybe you can advise another approach,
I'm pretty open. The final goal will be for me :
 - gpio and pinctrl have to cooperate
   - today, with the current state, it's impossible to map pins 0..127 to gpios
 0..127, at least in a device-tree .dts file
   - the GPDR (gpio direction register) shared access bothers me a bit

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik  wrote:

> Hi Linus, Alexandre and Haojian,
>
> This serie aims at several cleanups and improvements in the pxa gpio driver, 
> to

I have concerns about this series.

I am worried that joining the banks into one gpio_chip makes it
impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
preferrable when using a chained handler if e.g. one bank has
one IRQ line.

But overall that depends on how the IRQs map on this hardware.
Can you describe how the GPIO IRQs work on the PXA27x?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Linus Walleij]

On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik  wrote:

> Hi Linus, Alexandre and Haojian,
>
> This serie aims at several cleanups and improvements in the pxa gpio driver, 
> to

I have concerns about this series.

I am worried that joining the banks into one gpio_chip makes it
impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
preferrable when using a chained handler if e.g. one bank has
one IRQ line.

But overall that depends on how the IRQs map on this hardware.
Can you describe how the GPIO IRQs work on the PXA27x?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Linus Walleij  writes:

> On Sat, Nov 28, 2015 at 10:37 PM, Robert Jarzmik  
> wrote:
>
>> Hi Linus, Alexandre and Haojian,
>>
>> This serie aims at several cleanups and improvements in the pxa gpio driver, 
>> to
>
> I have concerns about this series.
>
> I am worried that joining the banks into one gpio_chip makes it
> impossible for you GPIOLIB_IRQCHIP. Usually that is possible and
> preferrable when using a chained handler if e.g. one bank has
> one IRQ line.
>
> But overall that depends on how the IRQs map on this hardware.
> Can you describe how the GPIO IRQs work on the PXA27x?
Of course.

For PXA27x, there are 3 interrupts directly connected to the CPU of the SoC,
ie. the primary irq controller :
 - one is only triggered if GPIO0 has a rising/falling edge
 - one is only triggered if GPIO1 has a rising/falling edge
 - the last is triggered if any GPIOn has a rising/falling edge (n >= 2)

The condition to program the rising/falling edge which implies the interrupt to
be asserted is in a GPIO block register, GFER and GRER (1 bit per GPIO).

The fact that the last interrupt (let's call it gpiomux_irq) is triggered by
GPIOs from _all_ the banks makes me believe it's a single IP block, ie. a single
chip.

Now if you have concerns with this, then maybe you can advise another approach,
I'm pretty open. The final goal will be for me :
 - gpio and pinctrl have to cooperate
   - today, with the current state, it's impossible to map pins 0..127 to gpios
 0..127, at least in a device-tree .dts file
   - the GPDR (gpio direction register) shared access bothers me a bit

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Hi Linus, Alexandre and Haojian,

This serie aims at several cleanups and improvements in the pxa gpio driver, to
support integration with pin control.

Amongst the changes, the main points are :
 - removal of as many global variables as possible
 - use of devm_*() function when applicable
 - use of irqdomain
 - use of pinctrl functions when available

Haojian, I've tested that on pxa27x, in both platform_data and devicetree
builds. Nevertheless I'd feel better if others architectures were tested,
especially mmp ones and pxa1928, which I don't have home.

Happy review.

Robert Jarzmik (4):
  gpio: pxa: convert to one gpiochip
  gpio: pxa: convert to devm_ioremap
  gpio: pxa: change the interrupt management
  gpio: pxa: add pin control gpio direction and request

 drivers/gpio/gpio-pxa.c | 370 
 1 file changed, 214 insertions(+), 156 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/4] gpio: pxa: integrate with pincontrol

[Robert Jarzmik]

Hi Linus, Alexandre and Haojian,

This serie aims at several cleanups and improvements in the pxa gpio driver, to
support integration with pin control.

Amongst the changes, the main points are :
 - removal of as many global variables as possible
 - use of devm_*() function when applicable
 - use of irqdomain
 - use of pinctrl functions when available

Haojian, I've tested that on pxa27x, in both platform_data and devicetree
builds. Nevertheless I'd feel better if others architectures were tested,
especially mmp ones and pxa1928, which I don't have home.

Happy review.

Robert Jarzmik (4):
  gpio: pxa: convert to one gpiochip
  gpio: pxa: convert to devm_ioremap
  gpio: pxa: change the interrupt management
  gpio: pxa: add pin control gpio direction and request

 drivers/gpio/gpio-pxa.c | 370 
 1 file changed, 214 insertions(+), 156 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/