[RFC v1 0/3] mmc-spi - support controllers incapable of getting as low as 400KHz

[RFC v1 0/3] mmc-spi - support controllers incapable of getting as low as 400KHz

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Yo,

RFC for some stuff that I've got in-progress for a customer's board
where they want to use mmc-spi-slot with a QSPI controller that is
incapable of getting as low as 400KHz with the way clocks have been
configured on the system. At the moment, if a controller cannot get that
low, linux continuously reports that queuing a transfer fails. The first
couple of transfers will complete on this system, until mmc_start_host()
kicks in and clamps the frequency to the larger of host->f_min and 400
KHz.
Doing something like patch 1 of this set would allow the mmc-spi-slot to
function for some sd cards, an improvement on the current none. I don't
have any sd cards on hand that don't support the 5 MHz minimum frequency
that this controller is limited to, so I amn't sure at what point this
will blow up if such a card was used.

Is this sort of change something that would fly?

Patches 2 & 3 are just here b/c without #3 the qspi driver for this
platform doesn't support anything other than mem ops and #2 is required
to set the minimum frequency for mmc_spi to pick up.

Cheers,
Conor.

CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: cyril.jean@microchip.com
CC: Mark Brown <broonie@kernel.org>
CC: linux-mmc@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-riscv@lists.infradead.org
CC: linux-spi@vger.kernel.org

Conor Dooley (2):
  mmc: mmc_spi: allow for spi controllers incapable of getting as low as
    400k
  spi: microchip-core-qspi: set min_speed_hz during probe

Cyril Jean (1):
  spi: microchip-core-qspi: Add regular transfers

 drivers/mmc/host/mmc_spi.c            |   5 +-
 drivers/spi/spi-microchip-core-qspi.c | 224 +++++++++++++++++++++++++-
 2 files changed, 226 insertions(+), 3 deletions(-)

-- 
2.43.0

[RFC v1 1/3] mmc: mmc_spi: allow for spi controllers incapable of getting as low as 400k

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Some controllers may not be able to reach a bus clock as low as 400 KHz
due to a lack of sufficient divisors. In these cases, the SD card slot
becomes non-functional as Linux continuously attempts to set the bus
clock to 400 KHz. If the controller is incapable of getting that low,
set its minimum frequency instead. While this may eliminate some SD
cards, it allows those capable of operating at the controller's minimum
frequency to be used.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/mmc/host/mmc_spi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 09d7a6a0dc1a..c9caa1ece7ef 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1208,7 +1208,10 @@ static int mmc_spi_probe(struct spi_device *spi)
 	 * that's the only reason not to use a few MHz for f_min (until
 	 * the upper layer reads the target frequency from the CSD).
 	 */
-	mmc->f_min = 400000;
+	if (spi->controller->min_speed_hz > 400000)
+		dev_warn(&spi->dev,"Controller unable to reduce bus clock to 400 KHz\n");
+
+	mmc->f_min = max(spi->controller->min_speed_hz, 400000);
 	mmc->f_max = spi->max_speed_hz;
 
 	host = mmc_priv(mmc);
-- 
2.43.0

Re: [RFC v1 1/3] mmc: mmc_spi: allow for spi controllers incapable of getting as low as 400k

[Ulf Hansson]

On Wed, 12 Jun 2024 at 17:48, Conor Dooley <conor@kernel.org> wrote:
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> Some controllers may not be able to reach a bus clock as low as 400 KHz
> due to a lack of sufficient divisors. In these cases, the SD card slot
> becomes non-functional as Linux continuously attempts to set the bus
> clock to 400 KHz. If the controller is incapable of getting that low,
> set its minimum frequency instead. While this may eliminate some SD
> cards, it allows those capable of operating at the controller's minimum
> frequency to be used.
>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Looks reasonable to me. I assume you intend to send a non-RFC for
this, that I can pick up?

Kind regards
Uffe

> ---
>  drivers/mmc/host/mmc_spi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index 09d7a6a0dc1a..c9caa1ece7ef 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -1208,7 +1208,10 @@ static int mmc_spi_probe(struct spi_device *spi)
>          * that's the only reason not to use a few MHz for f_min (until
>          * the upper layer reads the target frequency from the CSD).
>          */
> -       mmc->f_min = 400000;
> +       if (spi->controller->min_speed_hz > 400000)
> +               dev_warn(&spi->dev,"Controller unable to reduce bus clock to 400 KHz\n");
> +
> +       mmc->f_min = max(spi->controller->min_speed_hz, 400000);
>         mmc->f_max = spi->max_speed_hz;
>
>         host = mmc_priv(mmc);
> --
> 2.43.0
>

Re: [RFC v1 1/3] mmc: mmc_spi: allow for spi controllers incapable of getting as low as 400k

[Conor Dooley]

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

On Thu, Jun 20, 2024 at 02:50:15PM +0200, Ulf Hansson wrote:
> On Wed, 12 Jun 2024 at 17:48, Conor Dooley <conor@kernel.org> wrote:
> >
> > From: Conor Dooley <conor.dooley@microchip.com>
> >
> > Some controllers may not be able to reach a bus clock as low as 400 KHz
> > due to a lack of sufficient divisors. In these cases, the SD card slot
> > becomes non-functional as Linux continuously attempts to set the bus
> > clock to 400 KHz. If the controller is incapable of getting that low,
> > set its minimum frequency instead. While this may eliminate some SD
> > cards, it allows those capable of operating at the controller's minimum
> > frequency to be used.
> >
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Looks reasonable to me. I assume you intend to send a non-RFC for
> this, that I can pick up?

I do intend doing that. How soon depends on whether or not you are
willing to take it on its own, or require it to come in a series with
the spi driver changes.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC v1 1/3] mmc: mmc_spi: allow for spi controllers incapable of getting as low as 400k

[Ulf Hansson]

On Thu, 20 Jun 2024 at 16:12, Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Jun 20, 2024 at 02:50:15PM +0200, Ulf Hansson wrote:
> > On Wed, 12 Jun 2024 at 17:48, Conor Dooley <conor@kernel.org> wrote:
> > >
> > > From: Conor Dooley <conor.dooley@microchip.com>
> > >
> > > Some controllers may not be able to reach a bus clock as low as 400 KHz
> > > due to a lack of sufficient divisors. In these cases, the SD card slot
> > > becomes non-functional as Linux continuously attempts to set the bus
> > > clock to 400 KHz. If the controller is incapable of getting that low,
> > > set its minimum frequency instead. While this may eliminate some SD
> > > cards, it allows those capable of operating at the controller's minimum
> > > frequency to be used.
> > >
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> >
> > Looks reasonable to me. I assume you intend to send a non-RFC for
> > this, that I can pick up?
>
> I do intend doing that. How soon depends on whether or not you are
> willing to take it on its own, or require it to come in a series with
> the spi driver changes.

I can pick it separately, if that makes sense to you.

Kind regards
Uffe

[RFC v1 2/3] spi: microchip-core-qspi: Add regular transfers

[Conor Dooley]

From: Cyril Jean <cyril.jean@microchip.com>

The driver for CoreQSPI only supports memory operations at present, so
add support for regular transfers so that the SD card slot and ADC on
the BeagleV Fire can be used.

Signed-off-by: Cyril Jean <cyril.jean@microchip.com>
Co-developed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/spi/spi-microchip-core-qspi.c | 223 +++++++++++++++++++++++++-
 1 file changed, 221 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-microchip-core-qspi.c b/drivers/spi/spi-microchip-core-qspi.c
index 09f16471c537..1b23a38c155c 100644
--- a/drivers/spi/spi-microchip-core-qspi.c
+++ b/drivers/spi/spi-microchip-core-qspi.c
@@ -222,6 +222,86 @@ static inline void mchp_coreqspi_write_op(struct mchp_coreqspi *qspi, bool word)
 	}
 }
 
+static inline void mchp_coreqspi_write_read_op(struct mchp_coreqspi *qspi)
+{
+	u32 control, data;
+
+	qspi->rx_len = qspi->tx_len;
+
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+	control |= CONTROL_FLAGSX4;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	while (qspi->tx_len >= 4) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_TXFIFOFULL)
+			;
+
+		data = *(u32 *)qspi->txbuf;
+		qspi->txbuf += 4;
+		qspi->tx_len -= 4;
+		writel_relaxed(data, qspi->regs + REG_X4_TX_DATA);
+
+		/*
+		 * The rx FIFO is twice the size of the tx FIFO, so there is
+		 * no requirement to block transmission if receive data is not
+		 * ready, and it is fine to let the tx FIFO completely fill
+		 * without reading anything from the rx FIFO. Once the tx FIFO
+		 * has been filled and becomes non-full due to a transmission
+		 * occurring there will always be something to receive.
+		 * IOW, this is safe as TX_FIFO_SIZE + 4 < 2 * TX_FIFO_SIZE
+		 */
+		if (qspi->rx_len >= 4) {
+			if (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_RXAVAILABLE) {
+				data = readl_relaxed(qspi->regs + REG_X4_RX_DATA);
+				*(u32 *)qspi->rxbuf = data;
+				qspi->rxbuf += 4;
+				qspi->rx_len -= 4;
+			}
+		}
+	}
+
+	/*
+	 * Since transmission is not being blocked by clearing the rx FIFO,
+	 * loop here until all received data "leaked" by the loop above has
+	 * been dealt with.
+	 */
+	while (qspi->rx_len >= 4) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_RXFIFOEMPTY)
+			;
+		data = readl_relaxed(qspi->regs + REG_X4_RX_DATA);
+		*(u32 *)qspi->rxbuf = data;
+		qspi->rxbuf += 4;
+		qspi->rx_len -= 4;
+	}
+
+	/*
+	 * Since rx_len and tx_len must be < 4 bytes at this point, there's no
+	 * concern about overflowing the rx or tx FIFOs any longer. It's
+	 * therefore safe to loop over the remainder of the transmit data before
+	 * handling the remaining receive data.
+	 */
+	if (!qspi->tx_len)
+		return;
+
+	control &= ~CONTROL_FLAGSX4;
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	while (qspi->tx_len--) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_TXFIFOFULL)
+			;
+		data = *qspi->txbuf++;
+		writel_relaxed(data, qspi->regs + REG_TX_DATA);
+	}
+
+	while (qspi->rx_len--) {
+		while (readl_relaxed(qspi->regs + REG_STATUS) & STATUS_RXFIFOEMPTY)
+			;
+		data = readl_relaxed(qspi->regs + REG_RX_DATA);
+		*qspi->rxbuf++ = (data & 0xFF);
+	}
+}
+
+
 static void mchp_coreqspi_enable_ints(struct mchp_coreqspi *qspi)
 {
 	u32 mask = IEN_TXDONE |
@@ -366,7 +446,7 @@ static inline void mchp_coreqspi_config_op(struct mchp_coreqspi *qspi, const str
 	writel_relaxed(frames, qspi->regs + REG_FRAMES);
 }
 
-static int mchp_qspi_wait_for_ready(struct spi_mem *mem)
+static int mchp_coreqspi_wait_for_ready(struct spi_mem *mem)
 {
 	struct mchp_coreqspi *qspi = spi_controller_get_devdata
 				    (mem->spi->controller);
@@ -395,7 +475,7 @@ static int mchp_coreqspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *o
 	int err, i;
 
 	mutex_lock(&qspi->op_lock);
-	err = mchp_qspi_wait_for_ready(mem);
+	err = mchp_coreqspi_wait_for_ready(mem);
 	if (err)
 		goto error;
 
@@ -498,6 +578,142 @@ static const struct spi_controller_mem_ops mchp_coreqspi_mem_ops = {
 	.exec_op = mchp_coreqspi_exec_op,
 };
 
+
+static void mchp_coreqspi_activate_cs(struct spi_device *spi)
+{
+	if (spi_get_csgpiod(spi, 0))
+		gpiod_set_value(spi_get_csgpiod(spi, 0), 1);
+}
+
+static void mchp_coreqspi_deactivate_cs(struct spi_device *spi)
+{
+	if (spi_get_csgpiod(spi, 0))
+		gpiod_set_value(spi_get_csgpiod(spi, 0), 0);
+}
+
+static int mchp_coreqspi_transfer_one_message(struct spi_controller *ctlr,
+					struct spi_message *m)
+{
+	struct mchp_coreqspi *qspi = spi_controller_get_devdata(ctlr);
+	struct spi_transfer *t = NULL;
+	u32 control, frames, status;
+	u32 total_bytes = 0, cmd_bytes = 0, idle_cycles = 0;
+	int ret;
+	bool quad = false, dual = false;
+	bool cs_change = true;
+
+	mutex_lock(&qspi->op_lock);
+	ret = readl_poll_timeout(qspi->regs + REG_STATUS, status,
+				 (status & STATUS_READY), 0,
+				 TIMEOUT_MS);
+	if (ret) {
+		mutex_unlock(&qspi->op_lock);
+		dev_err(&ctlr->dev,
+			"Timeout waiting on QSPI ready.\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = mchp_coreqspi_setup_clock(qspi, m->spi);
+	if (ret)
+		goto error;
+
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+	control &= ~(CONTROL_MODE12_MASK | CONTROL_MODE0);
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+
+	reinit_completion(&qspi->data_completion);
+
+	/* Check the total bytes and command bytes */
+	list_for_each_entry(t, &m->transfers, transfer_list) {
+		total_bytes += t->len;
+		if ((!cmd_bytes) && !(t->tx_buf && t->rx_buf))
+			cmd_bytes = t->len;
+		if (!t->rx_buf)
+			cmd_bytes = total_bytes;
+		if (t->tx_nbits == SPI_NBITS_QUAD || t->rx_nbits == SPI_NBITS_QUAD)
+			quad = true;
+		else if (t->tx_nbits == SPI_NBITS_DUAL || t->rx_nbits == SPI_NBITS_DUAL)
+			dual = true;
+	}
+
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+	if (quad) {
+		control |= (CONTROL_MODE0 | CONTROL_MODE12_EX_RW);
+	} else if (dual) {
+		control &= ~CONTROL_MODE0;
+		control |= CONTROL_MODE12_FULL;
+	} else {
+		control &= ~(CONTROL_MODE12_MASK | CONTROL_MODE0);
+	}
+
+	writel_relaxed(control, qspi->regs + REG_CONTROL);
+	frames = total_bytes & BYTESUPPER_MASK;
+	writel_relaxed(frames, qspi->regs + REG_FRAMESUP);
+	frames = total_bytes & BYTESLOWER_MASK;
+	frames |= cmd_bytes << FRAMES_CMDBYTES_SHIFT;
+	frames |= idle_cycles << FRAMES_IDLE_SHIFT;
+	control = readl_relaxed(qspi->regs + REG_CONTROL);
+	if (control & CONTROL_MODE12_MASK)
+		frames |= (1 << FRAMES_SHIFT);
+
+	frames |= FRAMES_FLAGWORD;
+	writel_relaxed(frames, qspi->regs + REG_FRAMES);
+
+	//TODO: questionable robustness if both cs_change and cs_off toggle
+	list_for_each_entry(t, &m->transfers, transfer_list) {
+		//cs_change being set means we need to re-enable
+		if (cs_change && !t->cs_off)
+			mchp_coreqspi_activate_cs(m->spi);
+		else if (cs_change) //cs_off set but we changed it after last xfer so it may be on
+			mchp_coreqspi_deactivate_cs(m->spi);
+
+		if ((t->tx_buf) && (t->rx_buf)){
+			qspi->txbuf = (u8 *)t->tx_buf;
+			qspi->rxbuf = (u8 *)t->rx_buf;
+			qspi->tx_len = t->len;
+			mchp_coreqspi_write_read_op(qspi);
+		} else if (t->tx_buf) {
+			qspi->txbuf = (u8 *)t->tx_buf;
+			qspi->tx_len = t->len;
+			mchp_coreqspi_write_op(qspi, true);
+		} else {
+			qspi->rxbuf = (u8 *)t->rx_buf;
+			qspi->rx_len = t->len;
+			mchp_coreqspi_read_op(qspi);
+		}
+
+		spi_transfer_delay_exec(t);
+
+		cs_change = t->cs_change;
+		if (cs_change) {
+			if (list_is_last(&t->transfer_list, &m->transfers)) {
+				break; //special meaning, see below
+			}
+
+			// cs_change is set, so disable between xfers
+			if (!t->cs_off)
+				mchp_coreqspi_deactivate_cs(m->spi);
+			else
+				mchp_coreqspi_activate_cs(m->spi);
+
+			spi_transfer_cs_change_delay_exec(m, t);
+		}
+	}
+
+	udelay(750);
+
+	m->actual_length = total_bytes;
+
+error:
+	if (ret != 0 || !cs_change) //special case of cs_change for last xfer, set means dont touch line state
+		mchp_coreqspi_deactivate_cs(m->spi);
+
+	m->status = ret;
+	spi_finalize_current_message(ctlr);
+	mutex_unlock(&qspi->op_lock);
+	return ret;
+}
+
 static int mchp_coreqspi_probe(struct platform_device *pdev)
 {
 	struct spi_controller *ctlr;
@@ -544,6 +760,9 @@ static int mchp_coreqspi_probe(struct platform_device *pdev)
 	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
 			  SPI_TX_DUAL | SPI_TX_QUAD;
 	ctlr->dev.of_node = np;
+	ctlr->transfer_one_message = mchp_coreqspi_transfer_one_message;
+	ctlr->num_chipselect = 2;
+	ctlr->use_gpio_descriptors = true;
 
 	ret = devm_spi_register_controller(&pdev->dev, ctlr);
 	if (ret)
-- 
2.43.0

Re: [RFC v1 2/3] spi: microchip-core-qspi: Add regular transfers

[Mark Brown]

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

On Wed, Jun 12, 2024 at 04:48:32PM +0100, Conor Dooley wrote:

> +	//TODO: questionable robustness if both cs_change and cs_off toggle
> +	list_for_each_entry(t, &m->transfers, transfer_list) {
> +		//cs_change being set means we need to re-enable

Is it not possible to implement prepare_message() and transfer_one()
rather than open coding all this?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v1 2/3] spi: microchip-core-qspi: Add regular transfers

[Conor Dooley]

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

On Wed, Jun 12, 2024 at 05:40:39PM +0100, Mark Brown wrote:
> On Wed, Jun 12, 2024 at 04:48:32PM +0100, Conor Dooley wrote:
> 
> > +	//TODO: questionable robustness if both cs_change and cs_off toggle
> > +	list_for_each_entry(t, &m->transfers, transfer_list) {
> > +		//cs_change being set means we need to re-enable
> 
> Is it not possible to implement prepare_message() and transfer_one()
> rather than open coding all this?

If I can, I will. I already found one issue with the cs toggling in the
code Cyril gave me and I need to figure out why there's a udelay(750)
required later on in the function anyway!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC v1 2/3] spi: microchip-core-qspi: Add regular transfers

[Conor Dooley]

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

On Wed, Jun 12, 2024 at 09:48:16PM +0100, Conor Dooley wrote:
> On Wed, Jun 12, 2024 at 05:40:39PM +0100, Mark Brown wrote:
> > On Wed, Jun 12, 2024 at 04:48:32PM +0100, Conor Dooley wrote:
> > 
> > > +	//TODO: questionable robustness if both cs_change and cs_off toggle
> > > +	list_for_each_entry(t, &m->transfers, transfer_list) {
> > > +		//cs_change being set means we need to re-enable
> > 
> > Is it not possible to implement prepare_message() and transfer_one()
> > rather than open coding all this?
> 
> If I can, I will. I already found one issue with the cs toggling in the
> code Cyril gave me and I need to figure out why there's a udelay(750)
> required later on in the function anyway!

Actually wasn't too bad, glad to be rid of the open coded CS handling.
I still need the as of yet not understood udelay(), but it looks even
worse now:

+static int mchp_coreqspi_unprepare_message(struct spi_controller *ctlr, struct spi_message *m)
+{
+	udelay(750);
+
+	return 0;
+}



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[RFC v1 3/3] spi: microchip-core-qspi: set min_speed_hz during probe

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The controller's minimum possible bus clock is 1/30 the rate of the
input clock. Naively set the minimum bus clock speed the controller
is capable of during probe, assuming that the rate will never reduce
further.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/spi/spi-microchip-core-qspi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-microchip-core-qspi.c b/drivers/spi/spi-microchip-core-qspi.c
index 1b23a38c155c..a549911b2f66 100644
--- a/drivers/spi/spi-microchip-core-qspi.c
+++ b/drivers/spi/spi-microchip-core-qspi.c
@@ -763,6 +763,7 @@ static int mchp_coreqspi_probe(struct platform_device *pdev)
 	ctlr->transfer_one_message = mchp_coreqspi_transfer_one_message;
 	ctlr->num_chipselect = 2;
 	ctlr->use_gpio_descriptors = true;
+	ctlr->min_speed_hz = clk_get_rate(qspi->clk) / 30;
 
 	ret = devm_spi_register_controller(&pdev->dev, ctlr);
 	if (ret)
-- 
2.43.0