Well it is possible we may want to return to some specific error code so I would prefer to keep it as is.+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?
That's reasonable. The function kind of evolved from the checking only.+
+ 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()
True following your above function name change suggestion.+{
+
+ 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.
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+
+ /*
+ * 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.
cs->change is technically supported when all that's requested is aIf 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.
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.
+ 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
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.+ }
+
+ 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.
Same here. Let's just fallback to dummy cs workaround mode.+ 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 ;-)
+ }
+ }
+ 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.
+}
+
probably copy/paste error from the dummy cs loop. Will fix.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?
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
Sound good to me.+ 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).
That make sense. Should add a warning here.+
+ /*
+ * 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.
+ }
+
status = bcm63xx_hsspi_do_txrx(spi, t);
if (status)
break;
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature