[PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Stefan Agner]

If a GPIO gets freed after selecting a new pinctrl configuration
the driver should not change pinctrl anymore. Otherwise this will
likely lead to a unusable pin configuration for the newly selected
pinctrl.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
This turned out to be problematic when using the I2C GPIO bus recovery
functionality. After muxing back to I2C, the GPIO is being freed, which
cased I2C to stop working completely.

--
Stefan

 drivers/pinctrl/freescale/pinctrl-imx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 4761320..61cfa95 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -361,8 +361,13 @@ static void imx_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
 	if (pin_reg->mux_reg == -1)
 		return;
 
-	/* Clear IBE/OBE/PUE to disable the pin (Hi-Z) */
 	reg = readl(ipctl->base + pin_reg->mux_reg);
+
+	/* Only change pad configuration if pad is still a GPIO */
+	if (reg & (0x7 << 20))
+		return;
+
+	/* Clear IBE/OBE/PUE to disable the pin (Hi-Z) */
 	reg &= ~0x7;
 	writel(reg, ipctl->base + pin_reg->mux_reg);
 }
-- 
2.10.0

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

Hi Stefan,

On 09/27/2016 03:26 AM, Stefan Agner wrote:
> If a GPIO gets freed after selecting a new pinctrl configuration
> the driver should not change pinctrl anymore. Otherwise this will
> likely lead to a unusable pin configuration for > the newly selected
> pinctrl.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> This turned out to be problematic when using the I2C GPIO bus recovery
>functionality. After muxing back to I2C, the GPIO is being freed, which
> cased I2C to stop working completely.

IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
for example I believe it breaks I2C bus driver initialization on i.MX31
boards, where today there is no pinctrl driver at all.

IMHO something like I've partially described in the recent "Requesting as
a GPIO a pin already used through pinctrl" topic should be done here.
Could you consider to add another pinctrl-1 group with alternative GPIO
line mux/config settings to an i2c controller device node and apply it,
when you need a bus recovery? You may find references how this kind of
dynamic pinctrl management is done within mmc/sd subsystem.

By the way did I miss a patch, which falls back to mux settings on
.gpio_disable_free call for non-Vybrid platforms?

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

Hi Stefan,

On 09/27/2016 03:26 AM, Stefan Agner wrote:
> If a GPIO gets freed after selecting a new pinctrl configuration
> the driver should not change pinctrl anymore. Otherwise this will
> likely lead to a unusable pin configuration for > the newly selected
> pinctrl.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> This turned out to be problematic when using the I2C GPIO bus recovery
>functionality. After muxing back to I2C, the GPIO is being freed, which
> cased I2C to stop working completely.

IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
for example I believe it breaks I2C bus driver initialization on i.MX31
boards, where today there is no pinctrl driver at all.

IMHO something like I've partially described in the recent "Requesting as
a GPIO a pin already used through pinctrl" topic should be done here.
Could you consider to add another pinctrl-1 group with alternative GPIO
line mux/config settings to an i2c controller device node and apply it,
when you need a bus recovery? You may find references how this kind of
dynamic pinctrl management is done within mmc/sd subsystem.

By the way did I miss a patch, which falls back to mux settings on
.gpio_disable_free call for non-Vybrid platforms?

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Stefan Agner]

On 2016-09-27 05:12, Vladimir Zapolskiy wrote:
> Hi Stefan,
> 
> On 09/27/2016 03:26 AM, Stefan Agner wrote:
>> If a GPIO gets freed after selecting a new pinctrl configuration
>> the driver should not change pinctrl anymore. Otherwise this will
>> likely lead to a unusable pin configuration for > the newly selected
>> pinctrl.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>> This turned out to be problematic when using the I2C GPIO bus recovery
>>functionality. After muxing back to I2C, the GPIO is being freed, which
>> cased I2C to stop working completely.
> 
> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
> for example I believe it breaks I2C bus driver initialization on i.MX31
> boards, where today there is no pinctrl driver at all.

This has been addressed by Li Yang's patch, already in the next branch:
https://lkml.org/lkml/2016/9/12/1161

> 
> IMHO something like I've partially described in the recent "Requesting as
> a GPIO a pin already used through pinctrl" topic should be done here.
> Could you consider to add another pinctrl-1 group with alternative GPIO
> line mux/config settings to an i2c controller device node and apply it,
> when you need a bus recovery? You may find references how this kind of
> dynamic pinctrl management is done within mmc/sd subsystem.

I don't quite understand, that is already the case. This is what device
tree looks like to get the I2C recovery functionality:

&i2c1 {
	pinctrl-names = "default", "gpio";
	pinctrl-0 = <&pinctrl_i2c1>;
	pinctrl-1 = <&pinctrl_i2c1_gpio>;
	scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
	sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
	status = "okay";
};

> 
> By the way did I miss a patch, which falls back to mux settings on
> .gpio_disable_free call for non-Vybrid platforms?

Currently only Vybrid makes use of the .gpio_request_enable... and so
should .gpio_disable_free then.

--
Stefan

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

Hi Stefan,

On 09/27/2016 07:37 PM, Stefan Agner wrote:
> On 2016-09-27 05:12, Vladimir Zapolskiy wrote:
>> Hi Stefan,
>>
>> On 09/27/2016 03:26 AM, Stefan Agner wrote:
>>> If a GPIO gets freed after selecting a new pinctrl configuration
>>> the driver should not change pinctrl anymore. Otherwise this will
>>> likely lead to a unusable pin configuration for > the newly selected
>>> pinctrl.
>>>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>> This turned out to be problematic when using the I2C GPIO bus recovery
>>> functionality. After muxing back to I2C, the GPIO is being freed, which
>>> cased I2C to stop working completely.
>>
>> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
>> for example I believe it breaks I2C bus driver initialization on i.MX31
>> boards, where today there is no pinctrl driver at all.
>
> This has been addressed by Li Yang's patch, already in the next branch:
> https://lkml.org/lkml/2016/9/12/1161

Nice to know about it, thank you for the link.

>>
>> IMHO something like I've partially described in the recent "Requesting as
>> a GPIO a pin already used through pinctrl" topic should be done here.
>> Could you consider to add another pinctrl-1 group with alternative GPIO
>> line mux/config settings to an i2c controller device node and apply it,
>> when you need a bus recovery? You may find references how this kind of
>> dynamic pinctrl management is done within mmc/sd subsystem.
>
> I don't quite understand, that is already the case. This is what device
> tree looks like to get the I2C recovery functionality:
>
> &i2c1 {
> 	pinctrl-names = "default", "gpio";
> 	pinctrl-0 = <&pinctrl_i2c1>;
> 	pinctrl-1 = <&pinctrl_i2c1_gpio>;
> 	scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> 	sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> 	status = "okay";
> };

Great, then why do you experience a problem you've described?

>>> After muxing back to I2C, the GPIO is being freed, which cased I2C
>>> to stop working completely.

Release GPIO firstly, then mux back to I2C, that's the correct sequence
and I believe it obsoletes this change.

>>
>> By the way did I miss a patch, which falls back to mux settings on
>> .gpio_disable_free call for non-Vybrid platforms?
>
> Currently only Vybrid makes use of the .gpio_request_enable... and so
> should .gpio_disable_free then.
>

So, I guess this is a change with a runtime difference for Vybrid only.

I find that it was initially done wrong that a number of Vybrid specific
hooks were added to the shared pinctrl-imx.c, in my opinion it is better
to make needed abstractions and move all code around SHARE_MUX_CONF_REG
to pinctrl-vf610.c:

./pinctrl-imx.c:216:		if (info->flags & SHARE_MUX_CONF_REG) {
./pinctrl-imx.c:317:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:357:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:382:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:425:	if (info->flags & SHARE_MUX_CONF_REG)
./pinctrl-imx.c:450:		if (info->flags & SHARE_MUX_CONF_REG) {
./pinctrl-imx.c:534:	if (info->flags & SHARE_MUX_CONF_REG)
./pinctrl-imx.c:575:		if (info->flags & SHARE_MUX_CONF_REG) {

Nevertheless this is not directly related to the change.

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

Hi Stefan,

On 09/27/2016 07:37 PM, Stefan Agner wrote:
> On 2016-09-27 05:12, Vladimir Zapolskiy wrote:
>> Hi Stefan,
>>
>> On 09/27/2016 03:26 AM, Stefan Agner wrote:
>>> If a GPIO gets freed after selecting a new pinctrl configuration
>>> the driver should not change pinctrl anymore. Otherwise this will
>>> likely lead to a unusable pin configuration for > the newly selected
>>> pinctrl.
>>>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>> This turned out to be problematic when using the I2C GPIO bus recovery
>>> functionality. After muxing back to I2C, the GPIO is being freed, which
>>> cased I2C to stop working completely.
>>
>> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
>> for example I believe it breaks I2C bus driver initialization on i.MX31
>> boards, where today there is no pinctrl driver at all.
>
> This has been addressed by Li Yang's patch, already in the next branch:
> https://lkml.org/lkml/2016/9/12/1161

Nice to know about it, thank you for the link.

>>
>> IMHO something like I've partially described in the recent "Requesting as
>> a GPIO a pin already used through pinctrl" topic should be done here.
>> Could you consider to add another pinctrl-1 group with alternative GPIO
>> line mux/config settings to an i2c controller device node and apply it,
>> when you need a bus recovery? You may find references how this kind of
>> dynamic pinctrl management is done within mmc/sd subsystem.
>
> I don't quite understand, that is already the case. This is what device
> tree looks like to get the I2C recovery functionality:
>
> &i2c1 {
> 	pinctrl-names = "default", "gpio";
> 	pinctrl-0 = <&pinctrl_i2c1>;
> 	pinctrl-1 = <&pinctrl_i2c1_gpio>;
> 	scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> 	sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> 	status = "okay";
> };

Great, then why do you experience a problem you've described?

>>> After muxing back to I2C, the GPIO is being freed, which cased I2C
>>> to stop working completely.

Release GPIO firstly, then mux back to I2C, that's the correct sequence
and I believe it obsoletes this change.

>>
>> By the way did I miss a patch, which falls back to mux settings on
>> .gpio_disable_free call for non-Vybrid platforms?
>
> Currently only Vybrid makes use of the .gpio_request_enable... and so
> should .gpio_disable_free then.
>

So, I guess this is a change with a runtime difference for Vybrid only.

I find that it was initially done wrong that a number of Vybrid specific
hooks were added to the shared pinctrl-imx.c, in my opinion it is better
to make needed abstractions and move all code around SHARE_MUX_CONF_REG
to pinctrl-vf610.c:

./pinctrl-imx.c:216:		if (info->flags & SHARE_MUX_CONF_REG) {
./pinctrl-imx.c:317:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:357:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:382:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:425:	if (info->flags & SHARE_MUX_CONF_REG)
./pinctrl-imx.c:450:		if (info->flags & SHARE_MUX_CONF_REG) {
./pinctrl-imx.c:534:	if (info->flags & SHARE_MUX_CONF_REG)
./pinctrl-imx.c:575:		if (info->flags & SHARE_MUX_CONF_REG) {

Nevertheless this is not directly related to the change.

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Stefan Agner]

On 2016-09-27 11:17, Vladimir Zapolskiy wrote:
> Hi Stefan,
> 
> On 09/27/2016 07:37 PM, Stefan Agner wrote:
>> On 2016-09-27 05:12, Vladimir Zapolskiy wrote:
>>> Hi Stefan,
>>>
>>> On 09/27/2016 03:26 AM, Stefan Agner wrote:
>>>> If a GPIO gets freed after selecting a new pinctrl configuration
>>>> the driver should not change pinctrl anymore. Otherwise this will
>>>> likely lead to a unusable pin configuration for > the newly selected
>>>> pinctrl.
>>>>
>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>> ---
>>>> This turned out to be problematic when using the I2C GPIO bus recovery
>>>> functionality. After muxing back to I2C, the GPIO is being freed, which
>>>> cased I2C to stop working completely.
>>>
>>> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
>>> for example I believe it breaks I2C bus driver initialization on i.MX31
>>> boards, where today there is no pinctrl driver at all.
>>
>> This has been addressed by Li Yang's patch, already in the next branch:
>> https://lkml.org/lkml/2016/9/12/1161
> 
> Nice to know about it, thank you for the link.
> 
>>>
>>> IMHO something like I've partially described in the recent "Requesting as
>>> a GPIO a pin already used through pinctrl" topic should be done here.
>>> Could you consider to add another pinctrl-1 group with alternative GPIO
>>> line mux/config settings to an i2c controller device node and apply it,
>>> when you need a bus recovery? You may find references how this kind of
>>> dynamic pinctrl management is done within mmc/sd subsystem.
>>
>> I don't quite understand, that is already the case. This is what device
>> tree looks like to get the I2C recovery functionality:
>>
>> &i2c1 {
>> 	pinctrl-names = "default", "gpio";
>> 	pinctrl-0 = <&pinctrl_i2c1>;
>> 	pinctrl-1 = <&pinctrl_i2c1_gpio>;
>> 	scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
>> 	sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
>> 	status = "okay";
>> };
> 
> Great, then why do you experience a problem you've described?
> 
>>>> After muxing back to I2C, the GPIO is being freed, which cased I2C
>>>> to stop working completely.
> 
> Release GPIO firstly, then mux back to I2C, that's the correct sequence
> and I believe it obsoletes this change.
> 

Added Viresh Kumar to the discussion, he implemented the I2C recovery
functions.

Yes, reordering the pinctrl/gpio_free calls would fix the problem too.

However, I guess there is no explicit rule to that ("request/free GPIOs
only when they are muxed as GPIO"), so I think of it that the issue is
actually in the pinctrl driver.

On top of that it is not entirely trivial to reorder the calls the way
i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.


>>>
>>> By the way did I miss a patch, which falls back to mux settings on
>>> .gpio_disable_free call for non-Vybrid platforms?
>>
>> Currently only Vybrid makes use of the .gpio_request_enable... and so
>> should .gpio_disable_free then.
>>
> 
> So, I guess this is a change with a runtime difference for Vybrid only.

That is correct.

> 
> I find that it was initially done wrong that a number of Vybrid specific
> hooks were added to the shared pinctrl-imx.c, in my opinion it is better
> to make needed abstractions and move all code around SHARE_MUX_CONF_REG
> to pinctrl-vf610.c:
> 
> ./pinctrl-imx.c:216:		if (info->flags & SHARE_MUX_CONF_REG) {
> ./pinctrl-imx.c:317:	if (!(info->flags & SHARE_MUX_CONF_REG))
> ./pinctrl-imx.c:357:	if (!(info->flags & SHARE_MUX_CONF_REG))
> ./pinctrl-imx.c:382:	if (!(info->flags & SHARE_MUX_CONF_REG))
> ./pinctrl-imx.c:425:	if (info->flags & SHARE_MUX_CONF_REG)
> ./pinctrl-imx.c:450:		if (info->flags & SHARE_MUX_CONF_REG) {
> ./pinctrl-imx.c:534:	if (info->flags & SHARE_MUX_CONF_REG)
> ./pinctrl-imx.c:575:		if (info->flags & SHARE_MUX_CONF_REG) {
> 
> Nevertheless this is not directly related to the change.

Yeah I was really on the fence there. Despite the same name, the IOMUXC
on Vybrid is quite a bit different than on i.MX. The problem is it is
not easily possible to factor out the SoC specific stuff into
pinctrl-vf610.c I would have basically had to add tons of callbacks or
re-implement lots of functions in pinctrl-vf610.c. Maybe it would have
probably been better to basically implement Vybrids IOMUXC as its own
pinctrl driver.

--
Stefan

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

On 09/27/2016 10:34 PM, Stefan Agner wrote:
> On 2016-09-27 11:17, Vladimir Zapolskiy wrote:
>> Hi Stefan,
>>
>> On 09/27/2016 07:37 PM, Stefan Agner wrote:
>>> On 2016-09-27 05:12, Vladimir Zapolskiy wrote:
>>>> Hi Stefan,
>>>>
>>>> On 09/27/2016 03:26 AM, Stefan Agner wrote:
>>>>> If a GPIO gets freed after selecting a new pinctrl configuration
>>>>> the driver should not change pinctrl anymore. Otherwise this will
>>>>> likely lead to a unusable pin configuration for > the newly selected
>>>>> pinctrl.
>>>>>
>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>> ---
>>>>> This turned out to be problematic when using the I2C GPIO bus recovery
>>>>> functionality. After muxing back to I2C, the GPIO is being freed, which
>>>>> cased I2C to stop working completely.
>>>>
>>>> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
>>>> for example I believe it breaks I2C bus driver initialization on i.MX31
>>>> boards, where today there is no pinctrl driver at all.
>>>
>>> This has been addressed by Li Yang's patch, already in the next branch:
>>> https://lkml.org/lkml/2016/9/12/1161
>>
>> Nice to know about it, thank you for the link.
>>
>>>>
>>>> IMHO something like I've partially described in the recent "Requesting as
>>>> a GPIO a pin already used through pinctrl" topic should be done here.
>>>> Could you consider to add another pinctrl-1 group with alternative GPIO
>>>> line mux/config settings to an i2c controller device node and apply it,
>>>> when you need a bus recovery? You may find references how this kind of
>>>> dynamic pinctrl management is done within mmc/sd subsystem.
>>>
>>> I don't quite understand, that is already the case. This is what device
>>> tree looks like to get the I2C recovery functionality:
>>>
>>> &i2c1 {
>>> 	pinctrl-names = "default", "gpio";
>>> 	pinctrl-0 = <&pinctrl_i2c1>;
>>> 	pinctrl-1 = <&pinctrl_i2c1_gpio>;
>>> 	scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
>>> 	sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
>>> 	status = "okay";
>>> };
>>
>> Great, then why do you experience a problem you've described?
>>
>>>>> After muxing back to I2C, the GPIO is being freed, which cased I2C
>>>>> to stop working completely.
>>
>> Release GPIO firstly, then mux back to I2C, that's the correct sequence
>> and I believe it obsoletes this change.
>>
>
> Added Viresh Kumar to the discussion, he implemented the I2C recovery
> functions.
>
> Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
>
> However, I guess there is no explicit rule to that ("request/free GPIOs
> only when they are muxed as GPIO"), so I think of it that the issue is
> actually in the pinctrl driver.

there is a rule applied to strict type pinctrl controllers (and from
the code I see that Vybrid IOMUXC is a strict type pinctrl, the existense
of the problem under discussion testifies that it is strict also) that
pads shall not be requested by GPIO or another controller, when they are
in use by some controller. In your case you have I2C and GPIO controllers
competing for a pad, please manage the resource correctly, request
should be done only after release.

> On top of that it is not entirely trivial to reorder the calls the way
> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.
>

Better to fix it earlier than later.

>>>>
>>>> By the way did I miss a patch, which falls back to mux settings on
>>>> .gpio_disable_free call for non-Vybrid platforms?
>>>
>>> Currently only Vybrid makes use of the .gpio_request_enable... and so
>>> should .gpio_disable_free then.
>>>
>>
>> So, I guess this is a change with a runtime difference for Vybrid only.
>
> That is correct.
>
>>
>> I find that it was initially done wrong that a number of Vybrid specific
>> hooks were added to the shared pinctrl-imx.c, in my opinion it is better
>> to make needed abstractions and move all code around SHARE_MUX_CONF_REG
>> to pinctrl-vf610.c:
>>
>> ./pinctrl-imx.c:216:		if (info->flags & SHARE_MUX_CONF_REG) {
>> ./pinctrl-imx.c:317:	if (!(info->flags & SHARE_MUX_CONF_REG))
>> ./pinctrl-imx.c:357:	if (!(info->flags & SHARE_MUX_CONF_REG))
>> ./pinctrl-imx.c:382:	if (!(info->flags & SHARE_MUX_CONF_REG))
>> ./pinctrl-imx.c:425:	if (info->flags & SHARE_MUX_CONF_REG)
>> ./pinctrl-imx.c:450:		if (info->flags & SHARE_MUX_CONF_REG) {
>> ./pinctrl-imx.c:534:	if (info->flags & SHARE_MUX_CONF_REG)
>> ./pinctrl-imx.c:575:		if (info->flags & SHARE_MUX_CONF_REG) {
>>
>> Nevertheless this is not directly related to the change.
>
> Yeah I was really on the fence there. Despite the same name, the IOMUXC
> on Vybrid is quite a bit different than on i.MX. The problem is it is
> not easily possible to factor out the SoC specific stuff into
> pinctrl-vf610.c I would have basically had to add tons of callbacks or
> re-implement lots of functions in pinctrl-vf610.c. Maybe it would have
> probably been better to basically implement Vybrids IOMUXC as its own
> pinctrl driver.

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

On 09/27/2016 10:34 PM, Stefan Agner wrote:
> On 2016-09-27 11:17, Vladimir Zapolskiy wrote:
>> Hi Stefan,
>>
>> On 09/27/2016 07:37 PM, Stefan Agner wrote:
>>> On 2016-09-27 05:12, Vladimir Zapolskiy wrote:
>>>> Hi Stefan,
>>>>
>>>> On 09/27/2016 03:26 AM, Stefan Agner wrote:
>>>>> If a GPIO gets freed after selecting a new pinctrl configuration
>>>>> the driver should not change pinctrl anymore. Otherwise this will
>>>>> likely lead to a unusable pin configuration for > the newly selected
>>>>> pinctrl.
>>>>>
>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>> ---
>>>>> This turned out to be problematic when using the I2C GPIO bus recovery
>>>>> functionality. After muxing back to I2C, the GPIO is being freed, which
>>>>> cased I2C to stop working completely.
>>>>
>>>> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
>>>> for example I believe it breaks I2C bus driver initialization on i.MX31
>>>> boards, where today there is no pinctrl driver at all.
>>>
>>> This has been addressed by Li Yang's patch, already in the next branch:
>>> https://lkml.org/lkml/2016/9/12/1161
>>
>> Nice to know about it, thank you for the link.
>>
>>>>
>>>> IMHO something like I've partially described in the recent "Requesting as
>>>> a GPIO a pin already used through pinctrl" topic should be done here.
>>>> Could you consider to add another pinctrl-1 group with alternative GPIO
>>>> line mux/config settings to an i2c controller device node and apply it,
>>>> when you need a bus recovery? You may find references how this kind of
>>>> dynamic pinctrl management is done within mmc/sd subsystem.
>>>
>>> I don't quite understand, that is already the case. This is what device
>>> tree looks like to get the I2C recovery functionality:
>>>
>>> &i2c1 {
>>> 	pinctrl-names = "default", "gpio";
>>> 	pinctrl-0 = <&pinctrl_i2c1>;
>>> 	pinctrl-1 = <&pinctrl_i2c1_gpio>;
>>> 	scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
>>> 	sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
>>> 	status = "okay";
>>> };
>>
>> Great, then why do you experience a problem you've described?
>>
>>>>> After muxing back to I2C, the GPIO is being freed, which cased I2C
>>>>> to stop working completely.
>>
>> Release GPIO firstly, then mux back to I2C, that's the correct sequence
>> and I believe it obsoletes this change.
>>
>
> Added Viresh Kumar to the discussion, he implemented the I2C recovery
> functions.
>
> Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
>
> However, I guess there is no explicit rule to that ("request/free GPIOs
> only when they are muxed as GPIO"), so I think of it that the issue is
> actually in the pinctrl driver.

there is a rule applied to strict type pinctrl controllers (and from
the code I see that Vybrid IOMUXC is a strict type pinctrl, the existense
of the problem under discussion testifies that it is strict also) that
pads shall not be requested by GPIO or another controller, when they are
in use by some controller. In your case you have I2C and GPIO controllers
competing for a pad, please manage the resource correctly, request
should be done only after release.

> On top of that it is not entirely trivial to reorder the calls the way
> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.
>

Better to fix it earlier than later.

>>>>
>>>> By the way did I miss a patch, which falls back to mux settings on
>>>> .gpio_disable_free call for non-Vybrid platforms?
>>>
>>> Currently only Vybrid makes use of the .gpio_request_enable... and so
>>> should .gpio_disable_free then.
>>>
>>
>> So, I guess this is a change with a runtime difference for Vybrid only.
>
> That is correct.
>
>>
>> I find that it was initially done wrong that a number of Vybrid specific
>> hooks were added to the shared pinctrl-imx.c, in my opinion it is better
>> to make needed abstractions and move all code around SHARE_MUX_CONF_REG
>> to pinctrl-vf610.c:
>>
>> ./pinctrl-imx.c:216:		if (info->flags & SHARE_MUX_CONF_REG) {
>> ./pinctrl-imx.c:317:	if (!(info->flags & SHARE_MUX_CONF_REG))
>> ./pinctrl-imx.c:357:	if (!(info->flags & SHARE_MUX_CONF_REG))
>> ./pinctrl-imx.c:382:	if (!(info->flags & SHARE_MUX_CONF_REG))
>> ./pinctrl-imx.c:425:	if (info->flags & SHARE_MUX_CONF_REG)
>> ./pinctrl-imx.c:450:		if (info->flags & SHARE_MUX_CONF_REG) {
>> ./pinctrl-imx.c:534:	if (info->flags & SHARE_MUX_CONF_REG)
>> ./pinctrl-imx.c:575:		if (info->flags & SHARE_MUX_CONF_REG) {
>>
>> Nevertheless this is not directly related to the change.
>
> Yeah I was really on the fence there. Despite the same name, the IOMUXC
> on Vybrid is quite a bit different than on i.MX. The problem is it is
> not easily possible to factor out the SoC specific stuff into
> pinctrl-vf610.c I would have basically had to add tons of callbacks or
> re-implement lots of functions in pinctrl-vf610.c. Maybe it would have
> probably been better to basically implement Vybrids IOMUXC as its own
> pinctrl driver.

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Viresh Kumar]

On 27-09-16, 12:34, Stefan Agner wrote:
> Added Viresh Kumar to the discussion, he implemented the I2C recovery
> functions.
> 
> Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
> 
> However, I guess there is no explicit rule to that ("request/free GPIOs
> only when they are muxed as GPIO"), so I think of it that the issue is
> actually in the pinctrl driver.
> 
> On top of that it is not entirely trivial to reorder the calls the way
> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.

AFAICT, these routines don't touch the muxing part at all. Perhaps it is done
internally by the GPIO calls. Can you please elaborate the exact change you are
hinting towards here ?

-- 
viresh

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Stefan Agner]

On 2016-09-27 19:00, Viresh Kumar wrote:
> On 27-09-16, 12:34, Stefan Agner wrote:
>> Added Viresh Kumar to the discussion, he implemented the I2C recovery
>> functions.
>>
>> Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
>>
>> However, I guess there is no explicit rule to that ("request/free GPIOs
>> only when they are muxed as GPIO"), so I think of it that the issue is
>> actually in the pinctrl driver.
>>
>> On top of that it is not entirely trivial to reorder the calls the way
>> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.
> 
> AFAICT, these routines don't touch the muxing part at all. Perhaps it is done
> internally by the GPIO calls. Can you please elaborate the exact change you are
> hinting towards here ?

The i.MX I2C driver touches the pinctrl in its prepare/unprepare
callbacks.

So, on a i.MX or Vybrid, the call chain looks like this:

i2c_generic_gpio_recovery
    -> i2c_get_gpios_for_recovery
       -> gpio_request_one
    -> i2c_generic_recovery
       -> prepare_recovery (i2c_imx_prepare_recovery)
          -> pinctrl_select_state [gpio]
       -> unprepare_recovery (i2c_imx_unprepare_recovery)
          -> pinctrl_select_state [default]
    -> i2c_put_gpios_for_recovery
       -> gpio_free


And for the pinctrl/GPIO driver of Vybrid this is actually a problem
because gpio_free disables the output driver of the pad, and when that
happens after the (I2C) default pinctrl state gets selected the pad is
no longer active.

--
Stefan

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Viresh Kumar]

On 27-09-16, 20:38, Stefan Agner wrote:
> The i.MX I2C driver touches the pinctrl in its prepare/unprepare
> callbacks.
> 
> So, on a i.MX or Vybrid, the call chain looks like this:
> 
> i2c_generic_gpio_recovery
>     -> i2c_get_gpios_for_recovery
>        -> gpio_request_one
>     -> i2c_generic_recovery
>        -> prepare_recovery (i2c_imx_prepare_recovery)
>           -> pinctrl_select_state [gpio]

Why is this done here? And not in gpio_request_one?

>        -> unprepare_recovery (i2c_imx_unprepare_recovery)
>           -> pinctrl_select_state [default]
>     -> i2c_put_gpios_for_recovery
>        -> gpio_free
> 
> 
> And for the pinctrl/GPIO driver of Vybrid this is actually a problem
> because gpio_free disables the output driver of the pad, and when that
> happens after the (I2C) default pinctrl state gets selected the pad is
> no longer active.
> 
> --
> Stefan

-- 
viresh

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Stefan Agner]

On 2016-09-27 21:14, Viresh Kumar wrote:
> On 27-09-16, 20:38, Stefan Agner wrote:
>> The i.MX I2C driver touches the pinctrl in its prepare/unprepare
>> callbacks.
>>
>> So, on a i.MX or Vybrid, the call chain looks like this:
>>
>> i2c_generic_gpio_recovery
>>     -> i2c_get_gpios_for_recovery
>>        -> gpio_request_one
>>     -> i2c_generic_recovery
>>        -> prepare_recovery (i2c_imx_prepare_recovery)
>>           -> pinctrl_select_state [gpio]
> 
> Why is this done here? And not in gpio_request_one?
> 

You need to differentiate between Vybrid and i.MX:

Vybrid muxes a pin to GPIO on gpio_request_one (via .gpio_request_enable
callback)
i.MX does not mux a pin as GPIO on its own, but needs to be muxed
explicitly. That has been always the case...

I don't know what behavior is right, it is just "different"...

In Vybrid I did it that way because I knew that was the behavior of
another SoC I worked on (namely a Tegra)... And I had to touch the
pinctrl register anyway (using gpio_set_direction, to set the
direction).


I guess in the end it boils down to one question: Is the GPIO and
pinctrl API ment to be orthogonal? If so, then we probably should select
the GPIO via pinctrl in the i.MX I2C driver but mux the pin in
gpio_request_one similar as we do it on Vybrid... But that would be
rather invasive change...

@Shawn, Linus Walleij, others, what is your take on that?



>>        -> unprepare_recovery (i2c_imx_unprepare_recovery)
>>           -> pinctrl_select_state [default]
>>     -> i2c_put_gpios_for_recovery
>>        -> gpio_free

Currently free does not restore the last pinmux, so if the API's are
meant to be completely orthogonal we need to store the pinmux in
gpio_request_one so we can restore it in gpio_free.

--
Stefan

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Viresh Kumar]

On 29-09-16, 09:33, Stefan Agner wrote:
> You need to differentiate between Vybrid and i.MX:
> 
> Vybrid muxes a pin to GPIO on gpio_request_one (via .gpio_request_enable
> callback)
> i.MX does not mux a pin as GPIO on its own, but needs to be muxed
> explicitly. That has been always the case...
> 
> I don't know what behavior is right, it is just "different"...

Hmm, I think What Vybrid and Tegra have done is better, but it would be better
to get inputs from Linus, which you already asked for :)

-- 
viresh

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Linus Walleij]

On Thu, Sep 29, 2016 at 6:33 PM, Stefan Agner <stefan@agner.ch> wrote:
> On 2016-09-27 21:14, Viresh Kumar wrote:
>> On 27-09-16, 20:38, Stefan Agner wrote:
>>> The i.MX I2C driver touches the pinctrl in its prepare/unprepare
>>> callbacks.
>>>
>>> So, on a i.MX or Vybrid, the call chain looks like this:
>>>
>>> i2c_generic_gpio_recovery
>>>     -> i2c_get_gpios_for_recovery
>>>        -> gpio_request_one
>>>     -> i2c_generic_recovery
>>>        -> prepare_recovery (i2c_imx_prepare_recovery)
>>>           -> pinctrl_select_state [gpio]
>>
>> Why is this done here? And not in gpio_request_one?
>>
>
> You need to differentiate between Vybrid and i.MX:
>
> Vybrid muxes a pin to GPIO on gpio_request_one (via .gpio_request_enable
> callback)
> i.MX does not mux a pin as GPIO on its own, but needs to be muxed
> explicitly. That has been always the case...
>
> I don't know what behavior is right, it is just "different"...

Exactly. It would have been nice if we had defined clear semantics
on how this should work when creating the pin control subsystem,
had we been able to agree. We didn't so it's up to each driver and
system to deal with this in whatever way they like.

Whoever is interested in stringency may drive it.

> In Vybrid I did it that way because I knew that was the behavior of
> another SoC I worked on (namely a Tegra)... And I had to touch the
> pinctrl register anyway (using gpio_set_direction, to set the
> direction).

Yes and that is one school of pin controlling.

> I guess in the end it boils down to one question: Is the GPIO and
> pinctrl API ment to be orthogonal?

Both and.

The platforms implementing the following in their struct pinmux_ops:
.gpio_request_enable()
.gpio_disable_free()
.gpio_set_direction()

AND have a corresponding GPIO driver calling
gpiochip_add_pin_range() or gpiochip_add_pingroup_range()
so we can cross reference pins and GPIO lines, AND
do something like call pinctrl_request_gpio() and
pinctrl_free_gpio() equivalent to the below:

        if (of_property_read_bool(dev->of_node, "gpio-ranges")) {
                chip->gc.request = gpiochip_generic_request;
                chip->gc.free = gpiochip_generic_free;
        }

And potentially also pinctrl_gpio_direction_input()
and pinctrl_gpio_direction_output() will use the corresponing
pin controller as back end in their GPIO set-up when dealing
with request/free and optionally also direction.

This is cooperative and NOT orthogonal.

On the other hand: a pin control driver implementing none of
the above functions will be orthogonal, and in these cases
they just have to mux the line into GPIO mode on their own
using tables, DT, hogs, whatever.

> If so, then we probably should select
> the GPIO via pinctrl in the i.MX I2C driver but mux the pin in
> gpio_request_one similar as we do it on Vybrid... But that would be
> rather invasive change...
>
> @Shawn, Linus Walleij, others, what is your take on that?

You can choose. Do everything muxwise in the pin controller
or do it all as a back-end per above.

Notice that the above approach has "holes": we can only set
direction. Any other pin configuration (drive strength, push/pull
or open drain, bias etc) still need to be set up from pin control.

I had some ideas of how to deal with that by adding yet another
cross call but never get around to fixing it.

>>>        -> unprepare_recovery (i2c_imx_unprepare_recovery)
>>>           -> pinctrl_select_state [default]
>>>     -> i2c_put_gpios_for_recovery
>>>        -> gpio_free
>
> Currently free does not restore the last pinmux, so if the API's are
> meant to be completely orthogonal we need to store the pinmux in
> gpio_request_one so we can restore it in gpio_free.

I don't understand this but it sounds like some frankenstein
solution. If you can't get the two subsystems orthogonal for your
usecases then rewrite your driver as per above implementing
pin control as a back-end to GPIO.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

On 09/28/2016 06:38 AM, Stefan Agner wrote:
> On 2016-09-27 19:00, Viresh Kumar wrote:
>> On 27-09-16, 12:34, Stefan Agner wrote:
>>> Added Viresh Kumar to the discussion, he implemented the I2C recovery
>>> functions.
>>>
>>> Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
>>>
>>> However, I guess there is no explicit rule to that ("request/free GPIOs
>>> only when they are muxed as GPIO"), so I think of it that the issue is
>>> actually in the pinctrl driver.
>>>
>>> On top of that it is not entirely trivial to reorder the calls the way
>>> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.
>>
>> AFAICT, these routines don't touch the muxing part at all. Perhaps it is done
>> internally by the GPIO calls. Can you please elaborate the exact change you are
>> hinting towards here ?
>
> The i.MX I2C driver touches the pinctrl in its prepare/unprepare
> callbacks.
>
> So, on a i.MX or Vybrid, the call chain looks like this:
>
> i2c_generic_gpio_recovery
>     -> i2c_get_gpios_for_recovery
>        -> gpio_request_one
>     -> i2c_generic_recovery
>        -> prepare_recovery (i2c_imx_prepare_recovery)
>           -> pinctrl_select_state [gpio]
>        -> unprepare_recovery (i2c_imx_unprepare_recovery)
>           -> pinctrl_select_state [default]
>     -> i2c_put_gpios_for_recovery
>        -> gpio_free

I would expect that the change below improves the situation, but I didn't
perform any tests and here the core change is governed by the accepted
i.MX i2c bus driver specific changes, thus conceptually it may be incorrect:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02ef4a31..3a4f59c3c3e6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -697,9 +697,6 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
  	int i = 0, val = 1, ret = 0;
  
-	if (bri->prepare_recovery)
-		bri->prepare_recovery(adap);
-
  	bri->set_scl(adap, val);
  	ndelay(RECOVERY_NDELAY);
  
@@ -725,22 +722,34 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
  		ndelay(RECOVERY_NDELAY);
  	}
  
-	if (bri->unprepare_recovery)
-		bri->unprepare_recovery(adap);
-
  	return ret;
  }
  
  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
  {
-	return i2c_generic_recovery(adap);
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	int ret;
+
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
+	ret = i2c_generic_recovery(adap);
+
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
+	return ret;
  }
  EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
  
  int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
  {
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
  	int ret;
  
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
  	ret = i2c_get_gpios_for_recovery(adap);
  	if (ret)
  		return ret;
@@ -748,6 +757,9 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
  	ret = i2c_generic_recovery(adap);
  	i2c_put_gpios_for_recovery(adap);
  
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
  	return ret;
  }
  EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);


Alternatively you may consider to add contents of i2c_get_gpios_for_recovery()
into i2c_imx_prepare_recovery(), contents of i2c_put_gpios_for_recovery() into
i2c_imx_unprepare_recovery() in the i.MX I2C bus driver and change the recovery
callback .recover_bus to i2c_generic_scl_recovery().

>
> And for the pinctrl/GPIO driver of Vybrid this is actually a problem
> because gpio_free disables the output driver of the pad, and when that
> happens after the (I2C) default pinctrl state gets selected the pad is
> no longer active.
>

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

On 09/28/2016 06:38 AM, Stefan Agner wrote:
> On 2016-09-27 19:00, Viresh Kumar wrote:
>> On 27-09-16, 12:34, Stefan Agner wrote:
>>> Added Viresh Kumar to the discussion, he implemented the I2C recovery
>>> functions.
>>>
>>> Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
>>>
>>> However, I guess there is no explicit rule to that ("request/free GPIOs
>>> only when they are muxed as GPIO"), so I think of it that the issue is
>>> actually in the pinctrl driver.
>>>
>>> On top of that it is not entirely trivial to reorder the calls the way
>>> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.
>>
>> AFAICT, these routines don't touch the muxing part at all. Perhaps it is done
>> internally by the GPIO calls. Can you please elaborate the exact change you are
>> hinting towards here ?
>
> The i.MX I2C driver touches the pinctrl in its prepare/unprepare
> callbacks.
>
> So, on a i.MX or Vybrid, the call chain looks like this:
>
> i2c_generic_gpio_recovery
>     -> i2c_get_gpios_for_recovery
>        -> gpio_request_one
>     -> i2c_generic_recovery
>        -> prepare_recovery (i2c_imx_prepare_recovery)
>           -> pinctrl_select_state [gpio]
>        -> unprepare_recovery (i2c_imx_unprepare_recovery)
>           -> pinctrl_select_state [default]
>     -> i2c_put_gpios_for_recovery
>        -> gpio_free

I would expect that the change below improves the situation, but I didn't
perform any tests and here the core change is governed by the accepted
i.MX i2c bus driver specific changes, thus conceptually it may be incorrect:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02ef4a31..3a4f59c3c3e6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -697,9 +697,6 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
  	int i = 0, val = 1, ret = 0;
  
-	if (bri->prepare_recovery)
-		bri->prepare_recovery(adap);
-
  	bri->set_scl(adap, val);
  	ndelay(RECOVERY_NDELAY);
  
@@ -725,22 +722,34 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
  		ndelay(RECOVERY_NDELAY);
  	}
  
-	if (bri->unprepare_recovery)
-		bri->unprepare_recovery(adap);
-
  	return ret;
  }
  
  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
  {
-	return i2c_generic_recovery(adap);
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	int ret;
+
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
+	ret = i2c_generic_recovery(adap);
+
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
+	return ret;
  }
  EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
  
  int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
  {
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
  	int ret;
  
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
  	ret = i2c_get_gpios_for_recovery(adap);
  	if (ret)
  		return ret;
@@ -748,6 +757,9 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
  	ret = i2c_generic_recovery(adap);
  	i2c_put_gpios_for_recovery(adap);
  
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
  	return ret;
  }
  EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);


Alternatively you may consider to add contents of i2c_get_gpios_for_recovery()
into i2c_imx_prepare_recovery(), contents of i2c_put_gpios_for_recovery() into
i2c_imx_unprepare_recovery() in the i.MX I2C bus driver and change the recovery
callback .recover_bus to i2c_generic_scl_recovery().

>
> And for the pinctrl/GPIO driver of Vybrid this is actually a problem
> because gpio_free disables the output driver of the pad, and when that
> happens after the (I2C) default pinctrl state gets selected the pad is
> no longer active.
>

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Viresh Kumar]

On 28-09-16, 15:07, Vladimir Zapolskiy wrote:
> I would expect that the change below improves the situation, but I didn't
> perform any tests and here the core change is governed by the accepted
> i.MX i2c bus driver specific changes, thus conceptually it may be incorrect:
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index da3a02ef4a31..3a4f59c3c3e6 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -697,9 +697,6 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>  	int i = 0, val = 1, ret = 0;
> -	if (bri->prepare_recovery)
> -		bri->prepare_recovery(adap);
> -
>  	bri->set_scl(adap, val);
>  	ndelay(RECOVERY_NDELAY);
> @@ -725,22 +722,34 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>  		ndelay(RECOVERY_NDELAY);
>  	}
> -	if (bri->unprepare_recovery)
> -		bri->unprepare_recovery(adap);
> -
>  	return ret;
>  }
>  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  {
> -	return i2c_generic_recovery(adap);
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	int ret;
> +
> +	if (bri->prepare_recovery)
> +		bri->prepare_recovery(adap);
> +
> +	ret = i2c_generic_recovery(adap);
> +
> +	if (bri->unprepare_recovery)
> +		bri->unprepare_recovery(adap);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
>  int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
>  {
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>  	int ret;
> +	if (bri->prepare_recovery)
> +		bri->prepare_recovery(adap);
> +
>  	ret = i2c_get_gpios_for_recovery(adap);
>  	if (ret)
>  		return ret;
> @@ -748,6 +757,9 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
>  	ret = i2c_generic_recovery(adap);
>  	i2c_put_gpios_for_recovery(adap);
> +	if (bri->unprepare_recovery)
> +		bri->unprepare_recovery(adap);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);

That looks to like a hack made just to make things work on one platform.

I would rather wait for an answer to my query first, which I asked in a separate
email.
 
-- 
viresh

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

On 09/29/2016 09:46 AM, Viresh Kumar wrote:
> On 28-09-16, 15:07, Vladimir Zapolskiy wrote:
>> I would expect that the change below improves the situation, but I didn't
>> perform any tests and here the core change is governed by the accepted
>> i.MX i2c bus driver specific changes, thus conceptually it may be incorrect:
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index da3a02ef4a31..3a4f59c3c3e6 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -697,9 +697,6 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>>  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>>  	int i = 0, val = 1, ret = 0;
>> -	if (bri->prepare_recovery)
>> -		bri->prepare_recovery(adap);
>> -
>>  	bri->set_scl(adap, val);
>>  	ndelay(RECOVERY_NDELAY);
>> @@ -725,22 +722,34 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>>  		ndelay(RECOVERY_NDELAY);
>>  	}
>> -	if (bri->unprepare_recovery)
>> -		bri->unprepare_recovery(adap);
>> -
>>  	return ret;
>>  }
>>  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>>  {
>> -	return i2c_generic_recovery(adap);
>> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +	int ret;
>> +
>> +	if (bri->prepare_recovery)
>> +		bri->prepare_recovery(adap);
>> +
>> +	ret = i2c_generic_recovery(adap);
>> +
>> +	if (bri->unprepare_recovery)
>> +		bri->unprepare_recovery(adap);
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
>>  int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
>>  {
>> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>>  	int ret;
>> +	if (bri->prepare_recovery)
>> +		bri->prepare_recovery(adap);
>> +
>>  	ret = i2c_get_gpios_for_recovery(adap);
>>  	if (ret)
>>  		return ret;
>> @@ -748,6 +757,9 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
>>  	ret = i2c_generic_recovery(adap);
>>  	i2c_put_gpios_for_recovery(adap);
>> +	if (bri->unprepare_recovery)
>> +		bri->unprepare_recovery(adap);
>> +
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);
>
> That looks to like a hack made just to make things work on one platform.

If you look at the top I agree that this solution may be only one platform
specific, but it fixes the broken driver of i.MX I2C bus controller.

Why do you get an impression that it looks like a hack?

> I would rather wait for an answer to my query first, which I asked in a separate
> email.
>

Why pinctrl_select_state() is not done in gpio_request_one()? Because
the first function gets pin mux/config setting and the second does not.
How do you intend to get pin mux/config setting in gpio_request_one()?

Anyway I don't see any problems in pinctrl or gpio subsystems, the bugs
must be addressed and fixed in i2c.

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

On 09/29/2016 09:46 AM, Viresh Kumar wrote:
> On 28-09-16, 15:07, Vladimir Zapolskiy wrote:
>> I would expect that the change below improves the situation, but I didn't
>> perform any tests and here the core change is governed by the accepted
>> i.MX i2c bus driver specific changes, thus conceptually it may be incorrect:
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index da3a02ef4a31..3a4f59c3c3e6 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -697,9 +697,6 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>>  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>>  	int i = 0, val = 1, ret = 0;
>> -	if (bri->prepare_recovery)
>> -		bri->prepare_recovery(adap);
>> -
>>  	bri->set_scl(adap, val);
>>  	ndelay(RECOVERY_NDELAY);
>> @@ -725,22 +722,34 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>>  		ndelay(RECOVERY_NDELAY);
>>  	}
>> -	if (bri->unprepare_recovery)
>> -		bri->unprepare_recovery(adap);
>> -
>>  	return ret;
>>  }
>>  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>>  {
>> -	return i2c_generic_recovery(adap);
>> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +	int ret;
>> +
>> +	if (bri->prepare_recovery)
>> +		bri->prepare_recovery(adap);
>> +
>> +	ret = i2c_generic_recovery(adap);
>> +
>> +	if (bri->unprepare_recovery)
>> +		bri->unprepare_recovery(adap);
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
>>  int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
>>  {
>> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>>  	int ret;
>> +	if (bri->prepare_recovery)
>> +		bri->prepare_recovery(adap);
>> +
>>  	ret = i2c_get_gpios_for_recovery(adap);
>>  	if (ret)
>>  		return ret;
>> @@ -748,6 +757,9 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
>>  	ret = i2c_generic_recovery(adap);
>>  	i2c_put_gpios_for_recovery(adap);
>> +	if (bri->unprepare_recovery)
>> +		bri->unprepare_recovery(adap);
>> +
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);
>
> That looks to like a hack made just to make things work on one platform.

If you look at the top I agree that this solution may be only one platform
specific, but it fixes the broken driver of i.MX I2C bus controller.

Why do you get an impression that it looks like a hack?

> I would rather wait for an answer to my query first, which I asked in a separate
> email.
>

Why pinctrl_select_state() is not done in gpio_request_one()? Because
the first function gets pin mux/config setting and the second does not.
How do you intend to get pin mux/config setting in gpio_request_one()?

Anyway I don't see any problems in pinctrl or gpio subsystems, the bugs
must be addressed and fixed in i2c.

--
With best wishes,
Vladimir

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Viresh Kumar]

On 29-09-16, 15:16, Vladimir Zapolskiy wrote:
> If you look at the top I agree that this solution may be only one platform
> specific, but it fixes the broken driver of i.MX I2C bus controller.

Yeah, I saw that..

> Why do you get an impression that it looks like a hack?

Because we have to reorder things to make it work on a platform. This may break
things on other platforms and we don't know about it yet.

> Why pinctrl_select_state() is not done in gpio_request_one()? Because
> the first function gets pin mux/config setting and the second does not.
> How do you intend to get pin mux/config setting in gpio_request_one()?

Lets see what Linus has to say on this..

> Anyway I don't see any problems in pinctrl or gpio subsystems, the bugs
> must be addressed and fixed in i2c.

I think it can be a gpio driver specific thing as well and not really subsystem
level one.

-- 
viresh

Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Linus Walleij]

On Tue, Sep 27, 2016 at 2:26 AM, Stefan Agner <stefan@agner.ch> wrote:

> +       /* Only change pad configuration if pad is still a GPIO */
> +       if (reg & (0x7 << 20))
> +               return;
> +
> +       /* Clear IBE/OBE/PUE to disable the pin (Hi-Z) */
>         reg &= ~0x7;
>         writel(reg, ipctl->base + pin_reg->mux_reg);

Please #define the magic 7 and 20 above so we see what is going
on if you choose this approach.

Yours,
Linus Walleij