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;