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
From: "J.D. Schroeder" <[email protected]>
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 <[email protected]>
Signed-off-by: Roger Quadros <[email protected]>
---
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
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 <[email protected]>
---
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
On 07/07/2015 04:27 PM, Roger Quadros wrote:
> From: "J.D. Schroeder" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> 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 |
On 07/07/15 17:33, Marc Kleine-Budde wrote:
> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>> From: "J.D. Schroeder" <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Roger Quadros <[email protected]>
>> ---
>> 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
>
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" <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Roger Quadros <[email protected]>
>>> ---
>>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 [email protected] 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 |
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" <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Roger Quadros <[email protected]>
>>>> ---
>>>> 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
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 [email protected] 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
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" <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Roger Quadros <[email protected]>
>>>>> ---
>>>>> 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;
>>>>> }
>>>>>
>
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 [email protected] 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 |
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" <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> Signed-off-by: Roger Quadros <[email protected]>
>>>>>> ---
>>>>>> 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
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" <[email protected]>
>>>>>>>
>>>>>>> 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 <[email protected]>
>>>>>>> Signed-off-by: Roger Quadros <[email protected]>
>>>>>>> ---
>>>>>>> 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;
>>>>>>> }
>>>>>>>
>>>
>
>
From: "J.D. Schroeder" <[email protected]>
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 <[email protected]>
Signed-off-by: Roger Quadros <[email protected]>
---
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
On 07/08/2015 02:38 PM, Roger Quadros wrote:
> From: "J.D. Schroeder" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Roger Quadros <[email protected]>
> ---
> 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
On 07/09/2015 12:58 PM, Grygorii Strashko wrote:
> On 07/08/2015 02:38 PM, Roger Quadros wrote:
>> From: "J.D. Schroeder" <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Roger Quadros <[email protected]>
>> ---
>> 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 |
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" <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Roger Quadros <[email protected]>
>>> ---
>>> 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 <[email protected]>
--
regards,
-grygorii
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 |
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 |
* Marc Kleine-Budde <[email protected]> [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