Re: [PATCH v3 2/2] spi: rockchip: Disable Runtime PM when chip select is asserted

[Doug Anderson]

Jeffy,

On Tue, Jun 27, 2017 at 9:38 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 this
> when the CS asserted.
>
> So let's keep the spi alive when chip select is asserted.
>
> Also 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 v3:
> Store cs states in struct rockchip_spi, and use it to detect cs state
> instead of hw register.
>
> Changes in v2:
> Improve commit message and comments and coding style.
>
>  drivers/spi/spi-rockchip.c | 51 +++++++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 52ea160..0b4a52b 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -25,6 +25,11 @@
>
>  #define DRIVER_NAME "rockchip-spi"
>
> +#define ROCKCHIP_SPI_CLR_BITS(reg, bits) \
> +               writel_relaxed(readl_relaxed(reg) & ~(bits), reg)
> +#define ROCKCHIP_SPI_SET_BITS(reg, bits) \
> +               writel_relaxed(readl_relaxed(reg) | (bits), reg)
> +
>  /* SPI register offsets */
>  #define ROCKCHIP_SPI_CTRLR0                    0x0000
>  #define ROCKCHIP_SPI_CTRLR1                    0x0004
> @@ -149,6 +154,8 @@
>   */
>  #define ROCKCHIP_SPI_MAX_TRANLEN               0xffff
>
> +#define ROCKCHIP_SPI_MAX_CS_NUM                        2
> +
>  enum rockchip_ssi_type {
>         SSI_MOTO_SPI = 0,
>         SSI_TI_SSP,
> @@ -193,6 +200,8 @@ struct rockchip_spi {
>         /* protect state */
>         spinlock_t lock;
>
> +       bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
> +
>         u32 use_dma;
>         struct sg_table tx_sg;
>         struct sg_table rx_sg;
> @@ -264,37 +273,29 @@ static inline u32 rx_max(struct rockchip_spi *rs)
>
>  static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
>  {
> -       u32 ser;
>         struct spi_master *master = spi->master;
>         struct rockchip_spi *rs = spi_master_get_devdata(master);
> +       bool cs_asserted = !enable;
>
> -       pm_runtime_get_sync(rs->dev);
> +       /* Return immediately for no-op */
> +       if (cs_asserted == rs->cs_asserted[spi->chip_select])
> +               return;
>
> -       ser = readl_relaxed(rs->regs + ROCKCHIP_SPI_SER) & SER_MASK;
> +       if (cs_asserted) {
> +               /* Keep things powered as long as CS is asserted */
> +               pm_runtime_get_sync(rs->dev);
>
> -       /*
> -        * drivers/spi/spi.c:
> -        * static void spi_set_cs(struct spi_device *spi, bool enable)
> -        * {
> -        *              if (spi->mode & SPI_CS_HIGH)
> -        *                      enable = !enable;
> -        *
> -        *              if (spi->cs_gpio >= 0)
> -        *                      gpio_set_value(spi->cs_gpio, !enable);
> -        *              else if (spi->master->set_cs)
> -        *              spi->master->set_cs(spi, !enable);
> -        * }
> -        *
> -        * Note: enable(rockchip_spi_set_cs) = !enable(spi_set_cs)
> -        */
> -       if (!enable)
> -               ser |= 1 << spi->chip_select;
> -       else
> -               ser &= ~(1 << spi->chip_select);
> +               ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER,
> +                                     BIT(spi->chip_select));
> +       } else {
> +               ROCKCHIP_SPI_CLR_BITS(rs->regs + ROCKCHIP_SPI_SER,
> +                                     BIT(spi->chip_select));
>
> -       writel_relaxed(ser, rs->regs + ROCKCHIP_SPI_SER);
> +               /* Drop reference from when we first asserted CS */
> +               pm_runtime_put(rs->dev);
> +       }
>
> -       pm_runtime_put_sync(rs->dev);
> +       rs->cs_asserted[spi->chip_select] = cs_asserted;

Looks great!

>  }
>
>  static int rockchip_spi_prepare_message(struct spi_master *master,
> @@ -739,7 +740,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>         master->auto_runtime_pm = true;
>         master->bus_num = pdev->id;
>         master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP;
> -       master->num_chipselect = 2;
> +       master->num_chipselect = ROCKCHIP_SPI_MAX_CS_NUM;

Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>