[PATCH 0/2] ARM: dra7: fix DCAN glitch

[PATCH 0/2] ARM: dra7: fix DCAN glitch

[Roger Quadros]

Hi,

If "default" pins are active for CAN then there is a slight
window during boot when CAN pins are in active mode and can
affect DRA7 CAN stability. This is because device core
automatically selects "default" before probe.

These patches change "default" pins to inactive and adds
a new "active" pin state so that we don't encounter
the glitch mentioned above.

cheers,
-roger

J.D. Schroeder (1):
  net: can: c_can: Fix default pinmux glitch at init

Roger Quadros (1):
  ARM: dts: dra7x-evm: Prevent glitch on DCAN1 pinmux

 arch/arm/boot/dts/dra7-evm.dts  |  5 +++--
 arch/arm/boot/dts/dra72-evm.dts |  5 +++--
 drivers/net/can/c_can/c_can.c   | 13 ++++++++++++-
 3 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Roger Quadros]

From: "J.D. Schroeder" <jay.schroeder@garmin.com>

The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
is down) causes a slight glitch on the pinctrl settings when used. Since
commit ab78029 (drivers/pinctrl: grab default handles from device core),
the device core will automatically set the default pins. This causes
the pins to be momentarily set to the default and then to the sleep
state in register_c_can_dev(). By adding an optional "enable" state,
boards can set the default pin state to be disabled and avoid the
glitch when the switch from default to sleep first occurs. If the
"enable" state is not available c_can_pinctrl_select_state() falls
back to using the "default" pinctrl state.

[Roger Q] - Forward port to v4.2

Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 041525d..66e98e7 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	/* activate pins */
-	pinctrl_pm_select_default_state(dev->dev.parent);
+#ifdef CONFIG_PINCTRL
+	if (priv->device->pins) {
+		struct pinctrl_state *s;
+
+		/* Attempt to use "active" if available else use "default" */
+		s = pinctrl_lookup_state(priv->device->pins->p, "active");
+		if (!IS_ERR(s))
+			pinctrl_select_state(priv->device->pins->p, s);
+		else
+			pinctrl_pm_select_default_state(dev->dev.parent);
+	}
+#endif
 	return 0;
 }
 
-- 
2.1.4

[PATCH 2/2] ARM: dts: dra7x-evm: Prevent glitch on DCAN1 pinmux

[Roger Quadros]

Driver core sets "default" pinmux on on probe and CAN driver
sets "sleep" pinmux during register. This causes a small window
where the CAN pins are in "default" state with the DCAN module
being disabled.

Change the "default" state to be like sleep so this glitch is
avoided. Add a new "active" state that is used by the driver
when CAN is actually active.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/boot/dts/dra7-evm.dts  | 5 +++--
 arch/arm/boot/dts/dra72-evm.dts | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts
index aa46590..096f68b 100644
--- a/arch/arm/boot/dts/dra7-evm.dts
+++ b/arch/arm/boot/dts/dra7-evm.dts
@@ -686,7 +686,8 @@
 
 &dcan1 {
 	status = "ok";
-	pinctrl-names = "default", "sleep";
-	pinctrl-0 = <&dcan1_pins_default>;
+	pinctrl-names = "default", "sleep", "active";
+	pinctrl-0 = <&dcan1_pins_sleep>;
 	pinctrl-1 = <&dcan1_pins_sleep>;
+	pinctrl-2 = <&dcan1_pins_default>;
 };
diff --git a/arch/arm/boot/dts/dra72-evm.dts b/arch/arm/boot/dts/dra72-evm.dts
index 4e1b605..8037384 100644
--- a/arch/arm/boot/dts/dra72-evm.dts
+++ b/arch/arm/boot/dts/dra72-evm.dts
@@ -587,9 +587,10 @@
 
 &dcan1 {
 	status = "ok";
-	pinctrl-names = "default", "sleep";
-	pinctrl-0 = <&dcan1_pins_default>;
+	pinctrl-names = "default", "sleep", "active";
+	pinctrl-0 = <&dcan1_pins_sleep>;
 	pinctrl-1 = <&dcan1_pins_sleep>;
+	pinctrl-2 = <&dcan1_pins_default>;
 };
 
 &qspi {
-- 
2.1.4

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

On 07/07/2015 04:27 PM, Roger Quadros wrote:
> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
> 
> The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
> is down) causes a slight glitch on the pinctrl settings when used. Since
> commit ab78029 (drivers/pinctrl: grab default handles from device core),
> the device core will automatically set the default pins. This causes
> the pins to be momentarily set to the default and then to the sleep
> state in register_c_can_dev(). By adding an optional "enable" state,
> boards can set the default pin state to be disabled and avoid the
> glitch when the switch from default to sleep first occurs. If the
> "enable" state is not available c_can_pinctrl_select_state() falls
> back to using the "default" pinctrl state.
> 
> [Roger Q] - Forward port to v4.2
> 
> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 041525d..66e98e7 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
>  	/* activate pins */
> -	pinctrl_pm_select_default_state(dev->dev.parent);
> +#ifdef CONFIG_PINCTRL

Please remove the ifdef, AFAICS there are static inline noop functions
if CONFIG_PINCTRL switched off.

> +	if (priv->device->pins) {
> +		struct pinctrl_state *s;
> +
> +		/* Attempt to use "active" if available else use "default" */
> +		s = pinctrl_lookup_state(priv->device->pins->p, "active");
> +		if (!IS_ERR(s))
> +			pinctrl_select_state(priv->device->pins->p, s);
> +		else
> +			pinctrl_pm_select_default_state(dev->dev.parent);
> +	}
> +#endif
>  	return 0;
>  }
>  
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Roger Quadros]

On 07/07/15 17:33, Marc Kleine-Budde wrote:
> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>
>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
>> is down) causes a slight glitch on the pinctrl settings when used. Since
>> commit ab78029 (drivers/pinctrl: grab default handles from device core),
>> the device core will automatically set the default pins. This causes
>> the pins to be momentarily set to the default and then to the sleep
>> state in register_c_can_dev(). By adding an optional "enable" state,
>> boards can set the default pin state to be disabled and avoid the
>> glitch when the switch from default to sleep first occurs. If the
>> "enable" state is not available c_can_pinctrl_select_state() falls
>> back to using the "default" pinctrl state.
>>
>> [Roger Q] - Forward port to v4.2
>>
>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 041525d..66e98e7 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>   	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>
>>   	/* activate pins */
>> -	pinctrl_pm_select_default_state(dev->dev.parent);
>> +#ifdef CONFIG_PINCTRL
>
> Please remove the ifdef, AFAICS there are static inline noop functions
> if CONFIG_PINCTRL switched off.

yes, you are right.

cheers,
-roger

>
>> +	if (priv->device->pins) {
>> +		struct pinctrl_state *s;
>> +
>> +		/* Attempt to use "active" if available else use "default" */
>> +		s = pinctrl_lookup_state(priv->device->pins->p, "active");
>> +		if (!IS_ERR(s))
>> +			pinctrl_select_state(priv->device->pins->p, s);
>> +		else
>> +			pinctrl_pm_select_default_state(dev->dev.parent);
>> +	}
>> +#endif
>>   	return 0;
>>   }
>>
>>
>
> Marc
>

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Roger Quadros]

On 07/07/15 17:35, Roger Quadros wrote:
> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>
>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>> CAN interface
>>> is down) causes a slight glitch on the pinctrl settings when used. Since
>>> commit ab78029 (drivers/pinctrl: grab default handles from device core),
>>> the device core will automatically set the default pins. This causes
>>> the pins to be momentarily set to the default and then to the sleep
>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>> boards can set the default pin state to be disabled and avoid the
>>> glitch when the switch from default to sleep first occurs. If the
>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>> back to using the "default" pinctrl state.
>>>
>>> [Roger Q] - Forward port to v4.2
>>>
>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/c_can/c_can.c
>>> b/drivers/net/can/c_can/c_can.c
>>> index 041525d..66e98e7 100644
>>> --- a/drivers/net/can/c_can/c_can.c
>>> +++ b/drivers/net/can/c_can/c_can.c
>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>
>>>       /* activate pins */
>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>> +#ifdef CONFIG_PINCTRL
>>
>> Please remove the ifdef, AFAICS there are static inline noop functions
>> if CONFIG_PINCTRL switched off.
>
> yes, you are right.

On second thoughts

device->pins are not defined if CONFIG_PINCTRL is not set.
so we can't remove the #ifdef.

cheers,
-roger

>
>>
>>> +    if (priv->device->pins) {
>>> +        struct pinctrl_state *s;
>>> +
>>> +        /* Attempt to use "active" if available else use "default" */
>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>> +        if (!IS_ERR(s))
>>> +            pinctrl_select_state(priv->device->pins->p, s);
>>> +        else
>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
>>> +    }
>>> +#endif
>>>       return 0;
>>>   }
>>>
>>>
>>
>> Marc
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" 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] net: can: c_can: Fix default pinmux glitch at init

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

On 07/07/2015 04:37 PM, Roger Quadros wrote:
>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>> b/drivers/net/can/c_can/c_can.c
>>>> index 041525d..66e98e7 100644
>>>> --- a/drivers/net/can/c_can/c_can.c
>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>
>>>>       /* activate pins */
>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>> +#ifdef CONFIG_PINCTRL
>>>
>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>> if CONFIG_PINCTRL switched off.
>>
>> yes, you are right.
> 
> On second thoughts
> 
> device->pins are not defined if CONFIG_PINCTRL is not set.
> so we can't remove the #ifdef.

Too bad :(

okay - should I add stable@v.k.o on Cc?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Grygorii Strashko]

On 07/07/2015 05:37 PM, Roger Quadros wrote:
> On 07/07/15 17:35, Roger Quadros wrote:
>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>
>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>> CAN interface
>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>> Since
>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>> core),
>>>> the device core will automatically set the default pins. This causes
>>>> the pins to be momentarily set to the default and then to the sleep
>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>> boards can set the default pin state to be disabled and avoid the
>>>> glitch when the switch from default to sleep first occurs. If the
>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>> back to using the "default" pinctrl state.
>>>>
>>>> [Roger Q] - Forward port to v4.2
>>>>
>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>> b/drivers/net/can/c_can/c_can.c
>>>> index 041525d..66e98e7 100644
>>>> --- a/drivers/net/can/c_can/c_can.c
>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>
>>>>       /* activate pins */
>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>> +#ifdef CONFIG_PINCTRL
>>>
>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>> if CONFIG_PINCTRL switched off.
>>
>> yes, you are right.
>
> On second thoughts
>
> device->pins are not defined if CONFIG_PINCTRL is not set.
> so we can't remove the #ifdef.

May be you can use [devm_]pinctrl_get?

>>>
>>>> +    if (priv->device->pins) {
>>>> +        struct pinctrl_state *s;
>>>> +
>>>> +        /* Attempt to use "active" if available else use "default" */
>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>> +        if (!IS_ERR(s))
>>>> +            pinctrl_select_state(priv->device->pins->p, s);
>>>> +        else
>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
>>>> +    }
>>>> +#endif
>>>>       return 0;
>>>>   }
>>>>

-- 
regards,
-grygorii

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Roger Quadros]

Marc,

On 07/07/15 17:43, Marc Kleine-Budde wrote:
> On 07/07/2015 04:37 PM, Roger Quadros wrote:
>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>> b/drivers/net/can/c_can/c_can.c
>>>>> index 041525d..66e98e7 100644
>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>        priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>
>>>>>        /* activate pins */
>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>> +#ifdef CONFIG_PINCTRL
>>>>
>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>> if CONFIG_PINCTRL switched off.
>>>
>>> yes, you are right.
>>
>> On second thoughts
>>
>> device->pins are not defined if CONFIG_PINCTRL is not set.
>> so we can't remove the #ifdef.
>
> Too bad :(
>
> okay - should I add stable@v.k.o on Cc?

I'm not sure if it would help. This patch by itself won't fix anything. 
It needs to go in together with the DTS change in patch 2.

Maybe if Tony can Ack the second patch then both should be applicable
on v4.0+ for stable.

cheers,
-roger

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Roger Quadros]

On 07/07/15 18:49, Grygorii Strashko wrote:
> On 07/07/2015 05:37 PM, Roger Quadros wrote:
>> On 07/07/15 17:35, Roger Quadros wrote:
>>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>>
>>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>>> CAN interface
>>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>>> Since
>>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>>> core),
>>>>> the device core will automatically set the default pins. This causes
>>>>> the pins to be momentarily set to the default and then to the sleep
>>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>>> boards can set the default pin state to be disabled and avoid the
>>>>> glitch when the switch from default to sleep first occurs. If the
>>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>>> back to using the "default" pinctrl state.
>>>>>
>>>>> [Roger Q] - Forward port to v4.2
>>>>>
>>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> ---
>>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>> b/drivers/net/can/c_can/c_can.c
>>>>> index 041525d..66e98e7 100644
>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>
>>>>>       /* activate pins */
>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>> +#ifdef CONFIG_PINCTRL
>>>>
>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>> if CONFIG_PINCTRL switched off.
>>>
>>> yes, you are right.
>>
>> On second thoughts
>>
>> device->pins are not defined if CONFIG_PINCTRL is not set.
>> so we can't remove the #ifdef.
>
> May be you can use [devm_]pinctrl_get?

Why should we do that? The device core has already done the 
pinctrl_get() and it is available in device->pins->p.

cheers,
-roger

>
>>>>
>>>>> +    if (priv->device->pins) {
>>>>> +        struct pinctrl_state *s;
>>>>> +
>>>>> +        /* Attempt to use "active" if available else use "default" */
>>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>>> +        if (!IS_ERR(s))
>>>>> +            pinctrl_select_state(priv->device->pins->p, s);
>>>>> +        else
>>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
>>>>> +    }
>>>>> +#endif
>>>>>       return 0;
>>>>>   }
>>>>>
>

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

On 07/08/2015 10:09 AM, Roger Quadros wrote:

>>> device->pins are not defined if CONFIG_PINCTRL is not set.
>>> so we can't remove the #ifdef.
>>
>> Too bad :(
>>
>> okay - should I add stable@v.k.o on Cc?
> 
> I'm not sure if it would help. This patch by itself won't fix anything. 
> It needs to go in together with the DTS change in patch 2.

Of course.

> Maybe if Tony can Ack the second patch then both should be applicable
> on v4.0+ for stable.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Grygorii Strashko]

On 07/08/2015 11:13 AM, Roger Quadros wrote:
> 
> On 07/07/15 18:49, Grygorii Strashko wrote:
>> On 07/07/2015 05:37 PM, Roger Quadros wrote:
>>> On 07/07/15 17:35, Roger Quadros wrote:
>>>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>>>
>>>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>>>> CAN interface
>>>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>>>> Since
>>>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>>>> core),
>>>>>> the device core will automatically set the default pins. This causes
>>>>>> the pins to be momentarily set to the default and then to the sleep
>>>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>>>> boards can set the default pin state to be disabled and avoid the
>>>>>> glitch when the switch from default to sleep first occurs. If the
>>>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>>>> back to using the "default" pinctrl state.
>>>>>>
>>>>>> [Roger Q] - Forward port to v4.2
>>>>>>
>>>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>> ---
>>>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>>> b/drivers/net/can/c_can/c_can.c
>>>>>> index 041525d..66e98e7 100644
>>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>
>>>>>>       /* activate pins */
>>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>>> +#ifdef CONFIG_PINCTRL
>>>>>
>>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>>> if CONFIG_PINCTRL switched off.
>>>>
>>>> yes, you are right.
>>>
>>> On second thoughts
>>>
>>> device->pins are not defined if CONFIG_PINCTRL is not set.
>>> so we can't remove the #ifdef.
>>
>> May be you can use [devm_]pinctrl_get?
> 
> Why should we do that? The device core has already done the 
> pinctrl_get() and it is available in device->pins->p.

True. But It seems, you are going to be only one direct user of pin->p in whole
kernel (outside of pictrl core) !? ;)

pinctrl_get() will just return a pointer on pinctrl associated with dev
if called not the first time and increment kbobj use-counter.

Also, there is nice API pinctrl_get_select() and example in kernel 2c7c2c1d.

So, according to the documentation and pinctrl code, below sequence should work:

struct pinctrl *p;
p = pinctrl_get_select(priv->device, "active");

if (!IS_ERR(p))
   pinctrl_put(p);
else
   pinctrl_pm_select_default_state(priv->device);




>>>>>
>>>>>> +    if (priv->device->pins) {
>>>>>> +        struct pinctrl_state *s;
>>>>>> +
>>>>>> +        /* Attempt to use "active" if available else use 
>>>>>> "default" */
>>>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>>>> +        if (!IS_ERR(s))
>>>>>> +            pinctrl_select_state(priv->device->pins->p, s);

 ^^ different devices passed here

>>>>>> +        else
>>>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);

^^ and here ? Is it ok?

>>>>>> +    }
>>>>>> +#endif
>>>>>>       return 0;
>>>>>>   }
>>>>>>
>>


-- 
regards,
-grygorii

Re: [PATCH 1/2] net: can: c_can: Fix default pinmux glitch at init

[Roger Quadros]

On 08/07/15 13:31, Grygorii Strashko wrote:
> On 07/08/2015 11:13 AM, Roger Quadros wrote:
>>
>> On 07/07/15 18:49, Grygorii Strashko wrote:
>>> On 07/07/2015 05:37 PM, Roger Quadros wrote:
>>>> On 07/07/15 17:35, Roger Quadros wrote:
>>>>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>>>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>>>>
>>>>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>>>>> CAN interface
>>>>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>>>>> Since
>>>>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>>>>> core),
>>>>>>> the device core will automatically set the default pins. This causes
>>>>>>> the pins to be momentarily set to the default and then to the sleep
>>>>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>>>>> boards can set the default pin state to be disabled and avoid the
>>>>>>> glitch when the switch from default to sleep first occurs. If the
>>>>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>>>>> back to using the "default" pinctrl state.
>>>>>>>
>>>>>>> [Roger Q] - Forward port to v4.2
>>>>>>>
>>>>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>> ---
>>>>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>>>> b/drivers/net/can/c_can/c_can.c
>>>>>>> index 041525d..66e98e7 100644
>>>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>>
>>>>>>>       /* activate pins */
>>>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>>>> +#ifdef CONFIG_PINCTRL
>>>>>>
>>>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>>>> if CONFIG_PINCTRL switched off.
>>>>>
>>>>> yes, you are right.
>>>>
>>>> On second thoughts
>>>>
>>>> device->pins are not defined if CONFIG_PINCTRL is not set.
>>>> so we can't remove the #ifdef.
>>>
>>> May be you can use [devm_]pinctrl_get?
>>
>> Why should we do that? The device core has already done the 
>> pinctrl_get() and it is available in device->pins->p.
> 
> True. But It seems, you are going to be only one direct user of pin->p in whole
> kernel (outside of pictrl core) !? ;)

good point :).

> 
> pinctrl_get() will just return a pointer on pinctrl associated with dev
> if called not the first time and increment kbobj use-counter.
> 
> Also, there is nice API pinctrl_get_select() and example in kernel 2c7c2c1d.
> 
> So, according to the documentation and pinctrl code, below sequence should work:
> 
> struct pinctrl *p;
> p = pinctrl_get_select(priv->device, "active");
> 
> if (!IS_ERR(p))
>    pinctrl_put(p);
> else
>    pinctrl_pm_select_default_state(priv->device);

This is much neater. I will give that a try. Thanks.

cheers,
-roger

> 
> 
> 
> 
>>>>>>
>>>>>>> +    if (priv->device->pins) {
>>>>>>> +        struct pinctrl_state *s;
>>>>>>> +
>>>>>>> +        /* Attempt to use "active" if available else use 
>>>>>>> "default" */
>>>>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>>>>> +        if (!IS_ERR(s))
>>>>>>> +            pinctrl_select_state(priv->device->pins->p, s);
> 
>  ^^ different devices passed here
> 
>>>>>>> +        else
>>>>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
> 
> ^^ and here ? Is it ok?
> 
>>>>>>> +    }
>>>>>>> +#endif
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>
>>>
> 
> 

[PATCH v2 1/2] net: can: c_can: Fix default pinmux glitch at init

[Roger Quadros]

From: "J.D. Schroeder" <jay.schroeder@garmin.com>

The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
is down) causes a slight glitch on the pinctrl settings when used. Since
commit ab78029 (drivers/pinctrl: grab default handles from device core),
the device core will automatically set the default pins. This causes
the pins to be momentarily set to the default and then to the sleep
state in register_c_can_dev(). By adding an optional "enable" state,
boards can set the default pin state to be disabled and avoid the
glitch when the switch from default to sleep first occurs. If the
"enable" state is not available c_can_pinctrl_select_state() falls
back to using the "default" pinctrl state.

[Roger Q] - Forward port to v4.2 and use pinctrl_get_select().

Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 041525d..5d214d1 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -592,6 +592,7 @@ static int c_can_start(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 	int err;
+	struct pinctrl *p;
 
 	/* basic c_can configuration */
 	err = c_can_chip_config(dev);
@@ -604,8 +605,13 @@ static int c_can_start(struct net_device *dev)
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	/* activate pins */
-	pinctrl_pm_select_default_state(dev->dev.parent);
+	/* Attempt to use "active" if available else use "default" */
+	p = pinctrl_get_select(priv->device, "active");
+	if (!IS_ERR(p))
+		pinctrl_put(p);
+	else
+		pinctrl_pm_select_default_state(priv->device);
+
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH v2 1/2] net: can: c_can: Fix default pinmux glitch at init

[Grygorii Strashko]

On 07/08/2015 02:38 PM, Roger Quadros wrote:
> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>
> The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
> is down) causes a slight glitch on the pinctrl settings when used. Since
> commit ab78029 (drivers/pinctrl: grab default handles from device core),
> the device core will automatically set the default pins. This causes
> the pins to be momentarily set to the default and then to the sleep
> state in register_c_can_dev(). By adding an optional "enable" state,
> boards can set the default pin state to be disabled and avoid the
> glitch when the switch from default to sleep first occurs. If the
> "enable" state is not available c_can_pinctrl_select_state() falls
> back to using the "default" pinctrl state.
>
> [Roger Q] - Forward port to v4.2 and use pinctrl_get_select().
>
> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>   drivers/net/can/c_can/c_can.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 041525d..5d214d1 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -592,6 +592,7 @@ static int c_can_start(struct net_device *dev)
>   {
>   	struct c_can_priv *priv = netdev_priv(dev);
>   	int err;
> +	struct pinctrl *p;
>
>   	/* basic c_can configuration */
>   	err = c_can_chip_config(dev);
> @@ -604,8 +605,13 @@ static int c_can_start(struct net_device *dev)
>
>   	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>
> -	/* activate pins */
> -	pinctrl_pm_select_default_state(dev->dev.parent);
> +	/* Attempt to use "active" if available else use "default" */
> +	p = pinctrl_get_select(priv->device, "active");
> +	if (!IS_ERR(p))
> +		pinctrl_put(p);
> +	else
> +		pinctrl_pm_select_default_state(priv->device);
> +

Thanks. This part looks good to me now.

>   	return 0;
>   }
>
>


-- 
regards,
-grygorii

Re: [PATCH v2 1/2] net: can: c_can: Fix default pinmux glitch at init

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2425 bytes --]

On 07/09/2015 12:58 PM, Grygorii Strashko wrote:
> On 07/08/2015 02:38 PM, Roger Quadros wrote:
>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>
>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
>> is down) causes a slight glitch on the pinctrl settings when used. Since
>> commit ab78029 (drivers/pinctrl: grab default handles from device core),
>> the device core will automatically set the default pins. This causes
>> the pins to be momentarily set to the default and then to the sleep
>> state in register_c_can_dev(). By adding an optional "enable" state,
>> boards can set the default pin state to be disabled and avoid the
>> glitch when the switch from default to sleep first occurs. If the
>> "enable" state is not available c_can_pinctrl_select_state() falls
>> back to using the "default" pinctrl state.
>>
>> [Roger Q] - Forward port to v4.2 and use pinctrl_get_select().
>>
>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>   drivers/net/can/c_can/c_can.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 041525d..5d214d1 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -592,6 +592,7 @@ static int c_can_start(struct net_device *dev)
>>   {
>>   	struct c_can_priv *priv = netdev_priv(dev);
>>   	int err;
>> +	struct pinctrl *p;
>>
>>   	/* basic c_can configuration */
>>   	err = c_can_chip_config(dev);
>> @@ -604,8 +605,13 @@ static int c_can_start(struct net_device *dev)
>>
>>   	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>
>> -	/* activate pins */
>> -	pinctrl_pm_select_default_state(dev->dev.parent);
>> +	/* Attempt to use "active" if available else use "default" */
>> +	p = pinctrl_get_select(priv->device, "active");
>> +	if (!IS_ERR(p))
>> +		pinctrl_put(p);
>> +	else
>> +		pinctrl_pm_select_default_state(priv->device);
>> +
> 
> Thanks. This part looks good to me now.

Is this an Acked-by?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH v2 1/2] net: can: c_can: Fix default pinmux glitch at init

[Grygorii Strashko]

On 07/09/2015 01:59 PM, Marc Kleine-Budde wrote:
> On 07/09/2015 12:58 PM, Grygorii Strashko wrote:
>> On 07/08/2015 02:38 PM, Roger Quadros wrote:
>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>
>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
>>> is down) causes a slight glitch on the pinctrl settings when used. Since
>>> commit ab78029 (drivers/pinctrl: grab default handles from device core),
>>> the device core will automatically set the default pins. This causes
>>> the pins to be momentarily set to the default and then to the sleep
>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>> boards can set the default pin state to be disabled and avoid the
>>> glitch when the switch from default to sleep first occurs. If the
>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>> back to using the "default" pinctrl state.
>>>
>>> [Roger Q] - Forward port to v4.2 and use pinctrl_get_select().
>>>
>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>>    drivers/net/can/c_can/c_can.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>>> index 041525d..5d214d1 100644
>>> --- a/drivers/net/can/c_can/c_can.c
>>> +++ b/drivers/net/can/c_can/c_can.c
>>> @@ -592,6 +592,7 @@ static int c_can_start(struct net_device *dev)
>>>    {
>>>    	struct c_can_priv *priv = netdev_priv(dev);
>>>    	int err;
>>> +	struct pinctrl *p;
>>>
>>>    	/* basic c_can configuration */
>>>    	err = c_can_chip_config(dev);
>>> @@ -604,8 +605,13 @@ static int c_can_start(struct net_device *dev)
>>>
>>>    	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>
>>> -	/* activate pins */
>>> -	pinctrl_pm_select_default_state(dev->dev.parent);
>>> +	/* Attempt to use "active" if available else use "default" */
>>> +	p = pinctrl_get_select(priv->device, "active");
>>> +	if (!IS_ERR(p))
>>> +		pinctrl_put(p);
>>> +	else
>>> +		pinctrl_pm_select_default_state(priv->device);
>>> +
>>
>> Thanks. This part looks good to me now.
>
> Is this an Acked-by?

Not sure I can ack overall patch - is up to can driver maintainer
for this part Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

-- 
regards,
-grygorii

Re: [PATCH 2/2] ARM: dts: dra7x-evm: Prevent glitch on DCAN1 pinmux

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On 07/07/2015 04:27 PM, Roger Quadros wrote:
> Driver core sets "default" pinmux on on probe and CAN driver
> sets "sleep" pinmux during register. This causes a small window
> where the CAN pins are in "default" state with the DCAN module
> being disabled.
> 
> Change the "default" state to be like sleep so this glitch is
> avoided. Add a new "active" state that is used by the driver
> when CAN is actually active.

Who is taking care of this patch? I'm applying 1/2 (v2) to linux-can,
should I take this patch, too?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 2/2] ARM: dts: dra7x-evm: Prevent glitch on DCAN1 pinmux

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On 07/09/2015 08:34 PM, Marc Kleine-Budde wrote:
> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>> Driver core sets "default" pinmux on on probe and CAN driver
>> sets "sleep" pinmux during register. This causes a small window
>> where the CAN pins are in "default" state with the DCAN module
>> being disabled.
>>
>> Change the "default" state to be like sleep so this glitch is
>> avoided. Add a new "active" state that is used by the driver
>> when CAN is actually active.
> 
> Who is taking care of this patch? I'm applying 1/2 (v2) to linux-can,
> should I take this patch, too?

I've included this patch in my pull request.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 2/2] ARM: dts: dra7x-evm: Prevent glitch on DCAN1 pinmux

[Tony Lindgren]

* Marc Kleine-Budde <mkl@pengutronix.de> [150712 12:22]:
> On 07/09/2015 08:34 PM, Marc Kleine-Budde wrote:
> > On 07/07/2015 04:27 PM, Roger Quadros wrote:
> >> Driver core sets "default" pinmux on on probe and CAN driver
> >> sets "sleep" pinmux during register. This causes a small window
> >> where the CAN pins are in "default" state with the DCAN module
> >> being disabled.
> >>
> >> Change the "default" state to be like sleep so this glitch is
> >> avoided. Add a new "active" state that is used by the driver
> >> when CAN is actually active.
> > 
> > Who is taking care of this patch? I'm applying 1/2 (v2) to linux-can,
> > should I take this patch, too?
> 
> I've included this patch in my pull request.

That's fine thanks there should not be any merge conflicts.

For things going in during the merge windows we need to worry
about the dts merge conflicts. Usually not a problem for fixes.

Regards,

Tony