| Message ID | 20240417-mcp251xfd-gpio-feature-v1-1-bc0c61fd0c80@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: mcp251xfd: add gpio functionality | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject, async |
On 17.04.2024 15:43:54, Gregor Herburger wrote: > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip > is send to sleep and the timestamp workqueue is not stopped chip is > waked by SPI transfer of mcp251xfd_timestamp_read. > > So before sending chip to sleep stop timestamp otherwise the > mcp251xfd_timestamp_read callback would wake chip up. > > Also there are error paths in mcp251xfd_chip_start where workqueue has > not been initialized but mcp251xfd_chip_stop is called. So check for > initialized func before canceling delayed_work. Can you move the mcp251xfd_timestamp_init() (which starts the timestamping worker) into mcp251xfd_chip_start() to keep things symmetrical? I think then you don't need to check for "work->func" in mcp251xfd_timestamp_stop(). Marc
On 17.04.2024 15:43:54, Gregor Herburger wrote: > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip > is send to sleep and the timestamp workqueue is not stopped chip is > waked by SPI transfer of mcp251xfd_timestamp_read. How does the Low-Power Mode affect the GPIO lines? Is there a difference if the device is only in sleep mode? regards, Marc
On Wed, Apr 24, 2024 at 10:24:58AM +0200, Marc Kleine-Budde wrote: > On 17.04.2024 15:43:54, Gregor Herburger wrote: > > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip > > is send to sleep and the timestamp workqueue is not stopped chip is > > waked by SPI transfer of mcp251xfd_timestamp_read. > > > > So before sending chip to sleep stop timestamp otherwise the > > mcp251xfd_timestamp_read callback would wake chip up. > > > > Also there are error paths in mcp251xfd_chip_start where workqueue has > > not been initialized but mcp251xfd_chip_stop is called. So check for > > initialized func before canceling delayed_work. > > Can you move the mcp251xfd_timestamp_init() (which starts the > timestamping worker) into mcp251xfd_chip_start() to keep things > symmetrical? I think then you don't need to check for "work->func" in > mcp251xfd_timestamp_stop(). > Hi Marc, I realise now I confused mcp251xfd_timestamp_init with mcp251xfd_chip_timestamp_init. The only call chip mcp251xfd_chip_stop without call to mcp251xfd_timestamp_stop is from mcp251xfd_handle_cerrif. So it should be sufficient to stop the worker there and the check for "work->func" can be also omitted. Best regards, Gregor
On Wed, Apr 24, 2024 at 01:54:54PM +0200, Marc Kleine-Budde wrote: > On 17.04.2024 15:43:54, Gregor Herburger wrote: > > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip > > is send to sleep and the timestamp workqueue is not stopped chip is > > waked by SPI transfer of mcp251xfd_timestamp_read. > > How does the Low-Power Mode affect the GPIO lines? Is there a difference > if the device is only in sleep mode? The MCP251XFD_REG_IOCON is cleared when leaving Low-Power Mode. This is why I implemented regcache. Best regards Gregor
On 25.04.2024 07:17:11, Gregor Herburger wrote: > On Wed, Apr 24, 2024 at 01:54:54PM +0200, Marc Kleine-Budde wrote: > > On 17.04.2024 15:43:54, Gregor Herburger wrote: > > > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip > > > is send to sleep and the timestamp workqueue is not stopped chip is > > > waked by SPI transfer of mcp251xfd_timestamp_read. > > > > How does the Low-Power Mode affect the GPIO lines? Is there a difference > > if the device is only in sleep mode? > > The MCP251XFD_REG_IOCON is cleared when leaving Low-Power Mode. This is > why I implemented regcache. But that means you have to power the chip if a GPIO is requested. You have to power up the chip in the request() callback and power it down in the free() callback. I've 2 patches laying around, one that moves the timestamp init/start/stop into the chip_start/stop. And another one that moves the soft reset and basic configuration of the chip into the runtime pm functions. I have to make both patches compatible and send them to the list. Feel free to pick them up and integrate them into your series. regards, Marc
On Thu, Apr 25, 2024 at 08:29:13AM +0200, Marc Kleine-Budde wrote: > On 25.04.2024 07:17:11, Gregor Herburger wrote: > > On Wed, Apr 24, 2024 at 01:54:54PM +0200, Marc Kleine-Budde wrote: > > > On 17.04.2024 15:43:54, Gregor Herburger wrote: > > > > MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip > > > > is send to sleep and the timestamp workqueue is not stopped chip is > > > > waked by SPI transfer of mcp251xfd_timestamp_read. > > > > > > How does the Low-Power Mode affect the GPIO lines? Is there a difference > > > if the device is only in sleep mode? > > > > The MCP251XFD_REG_IOCON is cleared when leaving Low-Power Mode. This is > > why I implemented regcache. > > But that means you have to power the chip if a GPIO is requested. You > have to power up the chip in the request() callback and power it down in > the free() callback. Ah I see. Currently the GPIO rigister is cached and only written to the chip if the netdevice is set up. I think to have a more generic gpio controller the chip should wake up when the GPIO is requested. Also the chip should not go to sleep while GPIO is requested and netdevice is set down. > I've 2 patches laying around, one that moves the timestamp > init/start/stop into the chip_start/stop. And another one that moves the > soft reset and basic configuration of the chip into the runtime pm > functions. I have to make both patches compatible and send them to the > list. Feel free to pick them up and integrate them into your series. I will have a look at them. > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | Best regards Gregor
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index 1d9057dc44f2..eb699288c076 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c @@ -744,6 +744,7 @@ static void mcp251xfd_chip_stop(struct mcp251xfd_priv *priv, mcp251xfd_chip_interrupts_disable(priv); mcp251xfd_chip_rx_int_disable(priv); + mcp251xfd_timestamp_stop(priv); mcp251xfd_chip_sleep(priv); } diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c index 712e09186987..537c31890838 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c @@ -67,5 +67,8 @@ void mcp251xfd_timestamp_init(struct mcp251xfd_priv *priv) void mcp251xfd_timestamp_stop(struct mcp251xfd_priv *priv) { - cancel_delayed_work_sync(&priv->timestamp); + struct work_struct *work = &priv->timestamp.work; + + if (work->func) + cancel_delayed_work_sync(&priv->timestamp); }
MCP2518FD exits Low-Power Mode (LPM) when CS is asserted. When chip is send to sleep and the timestamp workqueue is not stopped chip is waked by SPI transfer of mcp251xfd_timestamp_read. So before sending chip to sleep stop timestamp otherwise the mcp251xfd_timestamp_read callback would wake chip up. Also there are error paths in mcp251xfd_chip_start where workqueue has not been initialized but mcp251xfd_chip_stop is called. So check for initialized func before canceling delayed_work. Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN") Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com> --- drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 1 + drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-)