Re: [PATCH v2 08/14] spi: bcm63xx-hsspi: Handle cs_change correctly

[Kursad Oney <kursad.oney@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4886 bytes --]

Hi Jonas, William,


On Thu, Jan 26, 2023 at 10:13 AM Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
> On Tue, 24 Jan 2023 at 23:33, William Zhang <william.zhang@broadcom.com> wrote:
> >
> > The kernel SPI interface includes the cs_change flag that alters how
> > the CS behaves.
> >
> > If we're in the middle of transfers, it tells us to unselect the
> > CS momentarily since the target device requires that.
> >
> > If we're at the end of a transfer, it tells us to keep the CS
> > selected, perhaps because the next transfer is likely targeted
> > to the same device.
> >
> > We implement this scheme in the HSSPI driver in this change.
> >
> > Prior to this change, the CS would toggle momentarily if cs_change
> > was set for the last transfer. This can be ignored by some or
> > most devices, but the Microchip TPM2 device does not ignore it.
> >
> > With the change, the behavior is corrected and the 'glitch' is
> > eliminated.
> >
> > Signed-off-by: Kursad Oney <kursad.oney@broadcom.com>
> > Signed-off-by: William Zhang <william.zhang@broadcom.com>
> >
> > ---
> >
> > Changes in v2:
> > - Fix unused variable ‘reg’ compile warning
> >
> >  drivers/spi/spi-bcm63xx-hsspi.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
> > index 55cbe7deba08..696e14abba2d 100644
> > --- a/drivers/spi/spi-bcm63xx-hsspi.c
> > +++ b/drivers/spi/spi-bcm63xx-hsspi.c
> > @@ -338,7 +338,7 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
> >         struct spi_device *spi = msg->spi;
> >         int status = -EINVAL;
> >         int dummy_cs;
> > -       u32 reg;
> > +       bool restore_polarity = true;
>
> While restore polarity is how this is implemented, I think using a
> more semantic name like keep_cs would be better.

This sounds reasonable to me.

>
> >
> >         mutex_lock(&bs->msg_mutex);
> >         /* This controller does not support keeping CS active during idle.
> > @@ -367,16 +367,29 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
> >
> >                 spi_transfer_delay_exec(t);
> >
> > -               if (t->cs_change)
> > +               /*
> > +                * cs_change rules:
> > +                * (1) cs_change = 0 && last_xfer = 0:
> > +                *     Do not touch the CS. On to the next xfer.
> > +                * (2) cs_change = 1 && last_xfer = 0:
> > +                *     Set cs = false before the next xfer.
> > +                * (3) cs_change = 0 && last_xfer = 1:
> > +                *     We want CS to be deactivated. So do NOT set cs = false,
> > +                *     instead just restore the original polarity. This has the
> > +                *     same effect of deactivating the CS.
> > +                * (4) cs_change = 1 && last_xfer = 1:
> > +                *     We want to keep CS active. So do NOT set cs = false, and
> > +                *     make sure we do NOT reverse polarity.
> > +                */
> > +               if (t->cs_change && !list_is_last(&t->transfer_list, &msg->transfers))
> >                         bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
> > +
> > +               restore_polarity = !t->cs_change;
> >         }
>
> I still find setting restore_polarity on each loop iteration when only
> its last set value matters confusing and hard to read, so I still
> propose keeping close to the generic implementation (
> https://elixir.bootlin.com/linux/v6.1.8/source/drivers/spi/spi.c#L1560
> ) and do
>
> if (t->cs_change) {
>    if (list_is_last())
>        restore_polarity = false;
>    else
>        bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
> }

OK I think this makes sense too but it might be a bit clearer to do:

if (list_is_last()) {
    if (cs_change)
        keep_cs = false;
    else
        bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
}

The gating condition here is when we reach the final transfer. But
list_is_last() is more expensive, so that's another consideration.

>
> While there, you might also want to check the cs_off value(s) as well.

Can you explain this please?

>
>
>
> >
> > -       mutex_lock(&bs->bus_mutex);
> > -       reg = __raw_readl(bs->regs + HSSPI_GLOBAL_CTRL_REG);
> > -       reg &= ~GLOBAL_CTRL_CS_POLARITY_MASK;
> > -       reg |= bs->cs_polarity;
> > -       __raw_writel(reg, bs->regs + HSSPI_GLOBAL_CTRL_REG);
> > -       mutex_unlock(&bs->bus_mutex);
> > +       bcm63xx_hsspi_set_cs(bs, dummy_cs, false);
> > +       if (restore_polarity)
> > +               bcm63xx_hsspi_set_cs(bs, spi->chip_select, false);
> >
> >         mutex_unlock(&bs->msg_mutex);
> >         msg->status = status;
> > --
> > 2.37.3
> >

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]