On 21/12/2019 13:47, Marek Vasut wrote:
On 12/21/19 12:12 PM, Grygorii Strashko wrote:
On 21/12/2019 13:00, Marek Vasut wrote:
On 12/21/19 11:53 AM, Grygorii Strashko wrote:
On 19/12/2019 23:47, Marek Vasut wrote:
On 12/19/19 2:37 PM, Grygorii Strashko wrote:
On 17/12/2019 12:55, Marek Vasut wrote:
On 12/17/19 11:42 AM, Grygorii Strashko wrote:
On 17/12/2019 12:07, Marek Vasut wrote:
The current code 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_m_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
pinctrl_get_select() falls back to using the "default" pinctrl
state.
Fixes: c9b3bce18da4 ("can: m_can: select pinctrl state in each
suspend/resume function")
Signed-off-by: Marek Vasut <marex@xxxxxxx>
Cc: Bich Hemon <bich.hemon@xxxxxx>
Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
Cc: J.D. Schroeder <jay.schroeder@xxxxxxxxxx>
Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
Cc: Roger Quadros <rogerq@xxxxxx>
Cc: linux-stable <stable@xxxxxxxxxxxxxxx>
To: linux-can@xxxxxxxxxxxxxxx
---
NOTE: This is commit 033365191136 ("can: c_can: Fix default pinmux
glitch at init")
adapted for m_can driver.
---
drivers/net/can/m_can/m_can.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/m_can/m_can.c
b/drivers/net/can/m_can/m_can.c
index 02c5795b73936..afb6760b17427 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1243,12 +1243,20 @@ static void m_can_chip_config(struct
net_device *dev)
static void m_can_start(struct net_device *dev)
{
struct m_can_classdev *cdev = netdev_priv(dev);
+ struct pinctrl *p;
/* basic m_can configuration */
m_can_chip_config(dev);
cdev->can.state = CAN_STATE_ERROR_ACTIVE;
+ /* Attempt to use "active" if available else use
"default" */
+ p = pinctrl_get_select(cdev->dev, "active");
+ if (!IS_ERR(p))
+ pinctrl_put(p);
+ else
+ pinctrl_pm_select_default_state(cdev->dev);
+
m_can_enable_all_interrupts(cdev);
}
May be init state should be used - #define PINCTRL_STATE_INIT "init"
instead?
I'm not sure I quite understand -- how ?
Sry, for delayed reply.
I've looked at m_can code and think issue is a little bit deeper
(but I might be wrong as i'm not can expert and below based on code
review).
First, what going on:
probe:
really_probe()
pinctrl_bind_pins()
if (IS_ERR(dev->pins->init_state)) {
ret = pinctrl_select_state(dev->pins->p,
dev->pins->default_state);
} else {
ret = pinctrl_select_state(dev->pins->p,
dev->pins->init_state);
}
[GS] So at this point default_state or init_state is set
ret = dev->bus->probe(dev);
m_can_plat_probe()
m_can_class_register()
m_can_clk_start()
pm_runtime_get_sync()
m_can_runtime_resume()
[GS] Still default_state or init_state is active
register_m_can_dev()
[GS] at this point m_can netdev is registered, which may lead to
.ndo_open = m_can_open() call
m_can_clk_stop()
pm_runtime_put_sync()
[GS] if .ndo_open() was called before it will be a nop
m_can_runtime_suspend()
m_can_class_suspend()
if (netif_running(ndev)) {
netif_stop_queue(ndev);
netif_device_detach(ndev);
m_can_stop(ndev);
m_can_clk_stop(cdev);
[GS] if .ndo_open() was called before it will lead to deadlock
here
So, most probably, it will cause deadlock in case of "ifconfig
<m_can_dev> up down" case
}
pinctrl_pm_select_sleep_state(dev);
[GS] at this point sleep_state will be set - i assume it's the
root
cause of your glitch.
Note - As per code, the pinctrl default_state will never ever
configured again, so if after
probe m_can will go through PM runtime suspend/resume
cycle it
will not work any more.
pinctrl_init_done()
[GS] will do nothing in case !init_state
As per above, if sleep_state is defined the m_can seems should not
work
at all without your patch,
as there is no code path to switch back sleep_state->default_state.
And over all PM runtime m_can code is mixed with System suspend
code and
so not correct.
Also, the very good question - Is it really required to toggle pinctrl
states as part of PM runtime?
(usually it's enough to handle it only during System suspend).
I suspect this discussion is somewhat a separate topic from what this
patch is trying to fix ?
Not exactly.
I see ?
The reason you need this patch is misuse of PM runtime vs
pin control
in this driver. And this has to be fixed first of all.
But then the C_CAN also misuses the PM runtime ? I mean, this patch does
literally what the same patch for C_CAN does, so maybe this is a more
general problem and needs a separate fix -- unless tristating the pins
when the block if disabled is the right thing to do, which it might be.
I feel that just removing of m_can_class_suspend() call from
m_can_runtime_suspend()
will fix things for you - it will toggle pin states only during Suspend
to RAM cycle.
I need to configure the pins on boot, this has nothing to do with
suspend/resume.
Then just use default_state in DT and do not define sleep state.
Sry, I see no reason for your patch at all.
Sry, my board does not work without this patch, so I see reason enough.
Presumably the author of the C_CAN patch did see similar reason.
You continue referring to C_CAN, but it's implemented in a different way.
1) It has no PM runtime callbacks in c_can_platform.c
2) it manually switches the pin states in .ndo_open()/close
3) it has requirement to enable pins after HW IP is enabled
Mind you, STM32MP1 does define both states already.
And please, try my proposal - don't make me feel I wasted my time doing
all above analysis.
But your proposal stops switching the pin states when the core is
suspended, I don't think that's what's it supposed to do.
Right, but a) it expected to fix your issue b) fix deadlock on if config down
And:
What exactly do you mean by "core is suspended"?
And how is expected to work from you point of view -
can interface dowm-> sleep pins; can interface up -> default/active pins;? smth else?
How has it worked before c_can_platform.c was introduced?
And what other people should after your patch - add non documented "active" state to every m_can DT node?
I understand you issue, but your patch is not a fix -
it's w/a and root cause has to be fixed.
From this discussion, it seems that right way to fix it could be:
1) fix m_can_runtime_suspend() as per above
2) move pinctrl_pm_select_xx() in m_can.c .ndo_open/ndo_close() some where
3) add "init" state == "sleep" state and in m_can_class_register() (or register_m_can_dev())
force "sleep" state. m_can should be kept in "sleep" state this way
until m_can.c .ndo_open() is called.
[1] commit ef0eebc05130 ("drivers/pinctrl: Add the concept of an "init" state")
--
Best regards,
grygorii
