Re: [PATCH v2 4/6] mci: Add sunxi-mmc driver

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 07.01.25 15:37, Jules Maselbas wrote:
> This driver is adapted from different sources: Linux, u-boot and p-boot.
> The latter, p-boot (forked from u-boot), is a bootloader for pinephones.
> 
> It currently only support PIO xfer but could be further improved to also
> support DMA xfer. This driver is split in three files so it can be used
> by the PBL and barebox proper.
> 
> Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> +	ret = sdxc_xfer_complete(host, 1000 * MSECOND, SDXC_INT_COMMAND_DONE);

That's

> +		ret = sdxc_xfer_complete(host, 1000 * MSECOND, data->blocks > 1 ?
> +					 SDXC_INT_AUTO_COMMAND_DONE :
> +					 SDXC_INT_DATA_OVER);

a lot

> +	if (cmd->resp_type & MMC_RSP_BUSY) {
> +		u32 status;
> +		u64 start;
> +		start = get_time_ns();
> +		do {
> +			status = sdxc_readl(host, SDXC_REG_STAS);
> +			if (is_timeout(start, 2000 * MSECOND)) {

of timeouts :o

> +				err_why = "resp timeout";
> +				ret = -ETIMEDOUT;
> +				goto err;
> +			}
> +		} while (status & SDXC_STATUS_BUSY);
> +	}
> +
> +	if (wait_on_timeout(1000 * MSECOND, !sdxc_is_card_busy(host))) {

There's mci_cmd::busy_timeout in ms now. Could you use it and fallback to
1000 * MSECOND only if it's zero?

This is ging to be useful down the line when we do longer operations
in barebox like large erases.

> +static int sunxi_mmc_read_block(struct sunxi_mmc_host *host,
> +				void *dst, unsigned int blocknum,
> +				unsigned int blocks)
> +{
> +	struct mci_data data;
> +	struct mci_cmd cmd = {
> +		.cmdidx = (blocks > 1) ? MMC_CMD_READ_MULTIPLE_BLOCK : MMC_CMD_READ_SINGLE_BLOCK,
> +		 /* mci->high_capacity ? blocknum : blocknum * mci->read_bl_len, */
> +		 /* TODO: detect if card is high-capacity */

I had an idea about that, but haven't come around to give it a try.
Quoting myself from IRC 2 years back:

"ye, that's what is used normally, but the problem is doing it while in transmission state
I looked into Part1_Physical_Layer_Simplified_Specification_Ver9.00.pdf again and for the
first time I see "Figure 4-13 : SD Memory Card State Diagram (data transfer mode)"
Apparently,  you don't necessarily need CMD0 (Go idle) to be able to get to a state where
you can execute CMD9 (SEND CSD), but one could also execute CMD7 (SELECT CARD) to deselect
card and then run CMD9 and get back to transmission mode One would need to issue a CMD3
(SEND_RELATIVE_ADDR) to get the RCA to communicate with the correct SD (it's argument to
CMD9), but this might work"

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |