Re: [PATCH v2] spi: rockchip: Disable Runtime PM when chip select is asserted

From: Doug Anderson
Date: Tue Jun 27 2017 - 15:27:54 EST


Hi,

On Mon, Jun 26, 2017 at 7:20 PM, Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx> wrote:
> The rockchip spi would stop driving pins when runtime suspended, which
> might break slave's xfer(for example cros_ec).
>
> Since we have pullups on those pins, we only need to care about the CS
> asserted case.
>
> So let's keep the spi alive when chip select is asserted for that.
>
> Also change use pm_runtime_put instead of pm_runtime_put_sync.
>
> Suggested-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
>
> ---
>
> Changes in v2:
> Improve commit message and comments and coding style.
>
> drivers/spi/spi-rockchip.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index acf31f3..ea0edd7 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -264,7 +264,7 @@ static inline u32 rx_max(struct rockchip_spi *rs)
>
> static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
> {
> - u32 ser;
> + u32 ser, new_ser;
> struct spi_master *master = spi->master;
> struct rockchip_spi *rs = spi_master_get_devdata(master);
>
> @@ -288,13 +288,26 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
> * Note: enable(rockchip_spi_set_cs) = !enable(spi_set_cs)
> */
> if (!enable)
> - ser |= 1 << spi->chip_select;
> + new_ser = ser | BIT(spi->chip_select);
> else
> - ser &= ~(1 << spi->chip_select);
> + new_ser = ser & ~BIT(spi->chip_select);
>
> - writel_relaxed(ser, rs->regs + ROCKCHIP_SPI_SER);
> + if (new_ser != ser) {

IMHO it's not a great idea to use the state of the hardware register
here. Using the state of the hardware register probably works OK, but
it makes me just a little nervous. If something happened to the state
of the register (someone used /dev/mem to tweak, or the peripherals
got reset, or ...) then you could end up with an unbalanced set of PM
Runtime calls. I know none of those things are common, but it still
seems less than great to me.

I'd rather we kept track in "struct rockchip_spi" whether we already
called pm_runtime_get_sync(). AKA the following (untested):

bool cs_asserted = !enable;

/* Return immediately for no-op */
if (cs_asserted == rs->cs_asserted)
return;

/* Keep things powered as long as CS is asserted */
if (cs_asserted) {
pm_runtime_get_sync(rs->dev);
rs->cs_asserted = true;
}

ser = ...
...
...

if (!cs_asserted)
pm_runtime_put(rs->dev);


NOTE: another advantage of storing the state in 'struct rockchip_spi'
is that you can access it without pm_runtime_get_sync().


> + writel_relaxed(new_ser, rs->regs + ROCKCHIP_SPI_SER);
>
> - pm_runtime_put_sync(rs->dev);
> + /*
> + * The rockchip spi would stop driving all pins when powered
> + * down.
> + * So hold a runtime PM reference as long as CS is asserted.
> + */
> + if (!enable)
> + return;
> +
> + /* Drop reference from when we first asserted CS */
> + pm_runtime_put(rs->dev);
> + }
> +
> + pm_runtime_put(rs->dev);

One note is that you should still submit your patch to add
"SPI_MASTER_GPIO_SS" to spi-rockchip.c. It's important that the SPI
driver see the CS transitions so that it can do the PM Runtime get/put
even when someone uses a GPIO chip select. Even though the GPIO chip
select will keep state, we don't want the rest of the lines to stop
being driven.


-Doug