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

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(-)

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.

>
>         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);
}

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



>
> -       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
>

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
> >

Hi Kursad,

On Thu, 26 Jan 2023 at 17:22, Kursad Oney <kursad.oney@broadcom.com> wrote:
> >
> > While there, you might also want to check the cs_off value(s) as well.
>
> Can you explain this please?

I'm talking about the transfer property cs_off:

" @cs_off: performs the transfer with chipselect off."

See how it is handled in the generic spi_transfer_one_message():

        spi_set_cs(msg->spi, !xfer->cs_off, false);
        ...
        list_for_each_entry(xfer, &msg->transfers, transfer_list) {
                ...
                if (xfer->cs_change) {
                        if (list_is_last(&xfer->transfer_list,
                                         &msg->transfers)) {
                                keep_cs = true;
                        } else {
                                if (!xfer->cs_off)
                                        spi_set_cs(msg->spi, false, false);
                                _spi_transfer_cs_change_delay(msg, xfer);
                                if (!list_next_entry(xfer,
transfer_list)->cs_off)
                                        spi_set_cs(msg->spi, true, false);
                        }
                } else if (!list_is_last(&xfer->transfer_list,
&msg->transfers) &&
                           xfer->cs_off != list_next_entry(xfer,
transfer_list)->cs_off) {
                        spi_set_cs(msg->spi, xfer->cs_off, false);
                }
                ...
      }

if we fix the cs_change handling, we might as well bring it up to state.

In theory I would suggest to switch to implementing the set_cs() /
transfer_one() so you could let the core take care of all of that, but
that wouldn't work with dynamically switching to prepend mode. Might
be something for v1.1 though.


Regards
Jonas

On 01/26/2023 09:33 AM, Jonas Gorski wrote:
> Hi Kursad,
> 
> On Thu, 26 Jan 2023 at 17:22, Kursad Oney <kursad.oney@broadcom.com> wrote:
>>>
>>> While there, you might also want to check the cs_off value(s) as well.
>>
>> Can you explain this please?
> 
> I'm talking about the transfer property cs_off:
> 
> " @cs_off: performs the transfer with chipselect off."
> 
> See how it is handled in the generic spi_transfer_one_message():
> 
>          spi_set_cs(msg->spi, !xfer->cs_off, false);
>          ...
>          list_for_each_entry(xfer, &msg->transfers, transfer_list) {
>                  ...
>                  if (xfer->cs_change) {
>                          if (list_is_last(&xfer->transfer_list,
>                                           &msg->transfers)) {
>                                  keep_cs = true;
>                          } else {
>                                  if (!xfer->cs_off)
>                                          spi_set_cs(msg->spi, false, false);
>                                  _spi_transfer_cs_change_delay(msg, xfer);
>                                  if (!list_next_entry(xfer,
> transfer_list)->cs_off)
>                                          spi_set_cs(msg->spi, true, false);
>                          }
>                  } else if (!list_is_last(&xfer->transfer_list,
> &msg->transfers) &&
>                             xfer->cs_off != list_next_entry(xfer,
> transfer_list)->cs_off) {
>                          spi_set_cs(msg->spi, xfer->cs_off, false);
>                  }
>                  ...
>        }
> 
> if we fix the cs_change handling, we might as well bring it up to state.
> 
We can blindly port this logic over but this cs_off stuff (from the 
spi.h comment @cs_off: performs the transfer with chipselect off) sounds 
weird.  What kind of device do transfer when cs is off? I don't have any 
device like this to test.

> In theory I would suggest to switch to implementing the set_cs() /
> transfer_one() so you could let the core take care of all of that, but
> that wouldn't work with dynamically switching to prepend mode. Might
> be something for v1.1 though.
> 
That is good idea and I can certainly try that.

> 
> Regards
> Jonas
>