Re: [PATCH v2 10/14] spi: bcm63xx-hsspi: Add prepend mode support

[William Zhang <william.zhang@broadcom.com>]

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



On 01/26/2023 07:15 AM, Jonas Gorski wrote:
>> +static int bcm63xx_hsspi_wait_cmd(struct bcm63xx_hsspi *bs)
>> +{
>> +       unsigned long limit;
>> +       u32 reg = 0;
>> +       int rc = 0;
> 
> If the only possible return values are 0 and 1, maybe this should be a bool?
> 
Well it is possible we may want to return to some specific error code so 
I would prefer to keep it as is.

>> +
>> +       if (bs->wait_mode == HSSPI_WAIT_MODE_INTR) {
>> +               if (wait_for_completion_timeout(&bs->done, HZ) == 0)
>> +                       rc = 1;
>> +       } else {
>> +               /* polling mode checks for status busy bit */
>> +               limit = jiffies + msecs_to_jiffies(HSSPI_POLL_STATUS_TIMEOUT_MS);
>> +
>> +               while (!time_after(jiffies, limit)) {
>> +                       reg = __raw_readl(bs->regs + HSSPI_PINGPONG_STATUS_REG(0));
>> +                       if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
>> +                               cpu_relax();
>> +                       else
>> +                               break;
>> +               }
>> +               if (reg & HSSPI_PINGPONG_STATUS_SRC_BUSY)
>> +                       rc = 1;
>> +       }
>> +
>> +       if (rc)
>> +               dev_err(&bs->pdev->dev, "transfer timed out!\n");
>> +
>> +       return rc;
>> +}
>> +
>> +static bool bcm63xx_check_msg_prependable(struct spi_master *master,
>> +                                         struct spi_message *msg,
>> +                                         struct spi_transfer *t_prepend)
> 
> This function does more than just checking, so I think a more
> appropriate name would be something like
> 
> bcm63xx_prepare_prepend_transfer()
> 
That's reasonable. The function kind of evolved from the checking only.

>> +{
>> +
>> +       struct bcm63xx_hsspi *bs = spi_master_get_devdata(master);
>> +       bool prepend = false, tx_only = false;
>> +       struct spi_transfer *t;
>> +
>> +       /* If it is forced cs dummy workaround mode, no need to prepend message */
>> +       if (bs->xfer_mode == HSSPI_XFER_MODE_DUMMYCS)
>> +               return false;
> 
> That's a weird point for that, why not just move this to the caller
> and check it before calling the function.
> 
True following your above function name change suggestion.

>> +
>> +       /*
>> +        * Multiple transfers within a message may be combined into one transfer
>> +        * to the controller using its prepend feature. A SPI message is prependable
>> +        * only if the following are all true:
>> +        *   1. One or more half duplex write transfer in single bit mode
>> +        *   2. Optional full duplex read/write at the end
>> +        *   3. No delay and cs_change between transfers
>> +        */
>> +       bs->prepend_cnt = 0;
>> +       list_for_each_entry(t, &msg->transfers, transfer_list) {
>> +               if ((spi_delay_to_ns(&t->delay, t) > 0) || t->cs_change) {
>> +                       dev_warn(&bs->pdev->dev,
>> +                                "Delay or cs change not supported in prepend mode!\n");
> 
> I don't think warn is the right level. If we are on XFER_MODE_AUTO,
> this should be _dbg, since we will just fall back to the dummy cs
> mode, if we are on XFER_MODE_PREPEND, this should be dev_err, since we
> cannot do the message.
> 
I was relying on this to see the message when we fall back. But 
certainly I can fine tune the message level as you suggested

> cs->change is technically supported when all that's requested is a
> between transfer cs toggle (t->cs_change is true, t->cs_off is false
> and next transfer's cs_off is also false), which automatically happens
> after the transfer. Not sure if it is worth the effort implementing
> that though.
> 
If this cs toggling is between the transfers that we are merging here, 
then no as the cs will be always active during merged transfer in 
prepend mode.

>> +                       break;
>> +               }
>> +
>> +               tx_only = false;
>> +               if (t->tx_buf && !t->rx_buf) {
>> +                       tx_only = true;
>> +                       if (bs->prepend_cnt + t->len >
>> +                               (HSSPI_BUFFER_LEN - HSSPI_OPCODE_LEN)) {
>> +                               dev_warn(&bs->pdev->dev,
>> +                                        "exceed max buf len, abort prepending transfers!\n");
>> +                               break;
> 
> why not just return false here directly? And everywhere else where you
> decided that you cannot use prepend.
> Sure I can eliminate the prepend variable and return directly

>> +                       }
>> +
>> +                       if (t->tx_nbits > SPI_NBITS_SINGLE &&
>> +                               !list_is_last(&t->transfer_list, &msg->transfers)) {
>> +                               dev_warn(&bs->pdev->dev,
>> +                                        "multi-bit prepend buf not supported!\n");
>> +                               break;
>> +                       }
>> +
>> +                       if (t->tx_nbits == SPI_NBITS_SINGLE) {
>> +                               memcpy(bs->prepend_buf + bs->prepend_cnt, t->tx_buf, t->len);
>> +                               bs->prepend_cnt += t->len;
>> +                       }
>> +               } else {
>> +                       if (!list_is_last(&t->transfer_list, &msg->transfers)) {
>> +                               dev_warn(&bs->pdev->dev,
>> +                                        "rx/tx_rx transfer not supported when it is not last one!\n");
> 
> This is only an issue if doing multi-bit RX/TX; for single bit you can
> just upgrade the whole transfer/message to duplex, you just need to
> pick the read bytes then from the right offsets.
> 
I am not sure if this will work all the case.  Considering two transfers 
rx 3 bytes then tx 3 bytes in the message(not sure if there is any 
device requires this kind of message but just for discussion 
purpose...),  if I upgrade it to duplex message,  the controller will 
transfer and receive 6 bytes data in duplex mode where prepend count is 
zero.  So the extra 3 tx bytes while receiving the first 3 bytes may 
disturb the device as they are not expected.  It may or may not work. I 
would rather just fallback to dummy cs workaround instead of causing 
potentially issue.

>> +                               break;
>> +                       }
>> +               }
>> +
>> +               if (list_is_last(&t->transfer_list, &msg->transfers)) {
>> +                       memcpy(t_prepend, t, sizeof(struct spi_transfer));
>> +
>> +                       if (tx_only && t->tx_nbits == SPI_NBITS_SINGLE) {
>> +                               /*
>> +                                * if the last one is also a single bit tx only transfer, merge
>> +                                * all of them into one single tx transfer
>> +                                */
>> +                               t_prepend->len = bs->prepend_cnt;
>> +                               t_prepend->tx_buf = bs->prepend_buf;
>> +                               bs->prepend_cnt = 0;
>> +                       } else {
>> +                               /*
>> +                                * if the last one is not a tx only transfer or dual tx xfer, all
>> +                                * the previous transfers are sent through prepend bytes and
>> +                                * make sure it does not exceed the max prepend len
>> +                                */
>> +                               if (bs->prepend_cnt > HSSPI_MAX_PREPEND_LEN) {
>> +                                       dev_warn(&bs->pdev->dev,
>> +                                               "exceed max prepend len, abort prepending transfers!\n");
>> +                                       break;
> 
> Likewise, you can merge any amount or rx/tx/rxtx single bit transfers
> together as a duplex transfer with prepend len set to 0 (so
> technically not a prepend anymore ;-)
Same here.  Let's just fallback to dummy cs workaround mode.

> 
>> +                               }
>> +                       }
>> +                       prepend = true;
>> +               }
>> +       }
>> +
>> +       return prepend;
> 
> and then if you already returned false if you cannot do prepend, you
> just need to return true here and don't need the prepend variable.
> 
>> +}
>> +
>>


>>   static int bcm63xx_hsspi_setup(struct spi_device *spi)
>> @@ -344,9 +578,23 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
>>          int status = -EINVAL;
>>          int dummy_cs;
>>          bool restore_polarity = true;
>> +       struct spi_transfer t_prepend;
>>
>>          mutex_lock(&bs->msg_mutex);
>> -       /* This controller does not support keeping CS active during idle.
>> +       if (bcm63xx_check_msg_prependable(master, msg, &t_prepend)) {
>> +               status = bcm63xx_hsspi_do_prepend_txrx(spi, &t_prepend);
>> +               msg->actual_length += (t_prepend.len + bs->prepend_cnt);
> 
> why +=? shouldn't this be the only place in this case where this is set?
> 
probably copy/paste error from the dummy cs loop.  Will fix.

>> +               goto msg_done;
>> +       }
>> +
>> +       if (bs->xfer_mode == HSSPI_XFER_MODE_PREPEND) {
>> +               dev_warn(&bs->pdev->dev,
>> +                       "User set prepend mode but msg not prependable! Fail the xfer!\n");
> 
> If we are failing, this should be a dev_err, not a dev_warn
> 
Will fix.

>> +               goto msg_done;
>> +       }
> 
> I think from a readability standpoint it would be better to move the
> cs_workaround parts into their own function, and have this as
> 
>      bool prependable = false;
> 
>      if (bs->xfer_mode != HSSPI_XFER_MODE_DUMMYCS)
>          prependable = bcm63xx_prepare_prepend_transfer(...);
> 
>      if (prependable) {
>        status = bcm63xx_hsspi_do_prepend_txrx(...);
>        msg->actual_legth += ...;
>      } else {
>        if (bs->xfer_mode == HSSPI_XFER_MODE_PREPEND) {
>             /* we may not use dummy cs */
>             dev_err(...);
>             status = -EINVAL;
>        } else {
>             status = bcm63xx_hsspi_do_dummy_cs_txrx(...);
>        }
>      }
> 
> with (bcm63xx_hsspi_do_dummy_cs_txrx being the proposed function name).
> 
Sound good to me.

>> +
>> +       /*
>> +        * This controller does not support keeping CS active during idle.
>>           * To work around this, we use the following ugly hack:
>>           *
>>           * a. Invert the target chip select's polarity so it will be active.
>> @@ -364,6 +612,17 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
>>          bcm63xx_hsspi_set_cs(bs, dummy_cs, true);
>>
>>          list_for_each_entry(t, &msg->transfers, transfer_list) {
>> +               /*
>> +                * We are here because one of reasons below:
>> +                * a. Message is not prependable and in default auto xfer mode. This mean
>> +                *    we fallback to dummy cs mode at maximum 25MHz safe clock rate.
>> +                * b. User set to use the dummy cs mode.
>> +                */
>> +               if (bs->xfer_mode == HSSPI_XFER_MODE_AUTO) {
>> +                       if (t->speed_hz > HSSPI_MAX_SYNC_CLOCK)
>> +                               t->speed_hz = HSSPI_MAX_SYNC_CLOCK;
> 
> OTOH, this may be a point where a dev_warn (once?) might be a good
> idea, since the device may depend on a certain speed to avoid buffer
> overruns (e.g. audio streams - not sure if that exists), so a warning
> that the transfer speed was reduced will help identifying the source.
> 
> 
That make sense. Should add a warning here.

> 
>> +               }
>> +
>>                  status = bcm63xx_hsspi_do_txrx(spi, t);
>>                  if (status)
>>                          break;

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