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

[jeffy]

Hi doug,

thanx for your comments.

On 06/28/2017 03:27 AM, Doug Anderson wrote:
> 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().
ok, that make sense :)
> > +               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.
ok, will do.
> -Doug