Hi Jonas, William, On Thu, Jan 26, 2023 at 10:13 AM Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote: > > On Tue, 24 Jan 2023 at 23:33, William Zhang <william.zhang@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > Signed-off-by: William Zhang <william.zhang@xxxxxxxxxxxx> > > > > --- > > > > 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:
smime.p7s
Description: S/MIME Cryptographic Signature
