Re: [PATCH] mci: only write blocks when card out of programming mode

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

Hi,

On 14.12.22 11:03, Sascha Hauer wrote:
> On Tue, Dec 13, 2022 at 03:12:04PM +0100, Ahmad Fatoum wrote:
>>  /**
>>   * @param p Command definition to setup
>>   * @param cmd Valid SD/MMC command (refer MMC_CMD_* / SD_CMD_*)
>> @@ -119,6 +141,69 @@ static int mci_set_blocklen(struct mci *mci, unsigned len)
>>  
>>  static void *sector_buf;
>>  
>> +static int mci_send_status(struct mci *mci, unsigned int *status)
>> +{
>> +	struct mci_host *host = mci->host;
>> +	struct mci_cmd cmd;
>> +	int ret;
>> +
>> +	cmd.cmdidx = MMC_CMD_SEND_STATUS;
>> +	cmd.resp_type = MMC_RSP_R1;
>> +	if (!mmc_host_is_spi(host))
>> +		cmd.cmdarg = mci->rca << 16;
> 
> cmd.cmdarg will be uninitialized for SPI MMC hosts.

Ouch. It seems CMD13 is valid for SPI hosts too, but they report
different bits. I have skipped this for SPI hosts in v2.

>> +	if (retries >=  timeout_ms) {
>> +		dev_err(&mci->dev, "Timeout waiting card ready\n");
>> +		return -ETIMEDOUT;
>> +	}
> 
> You could move this into the timeout test in the loop above.

Alright.

>> +
>> +	/*
>> +	 * With small UART FIFOs and slow CPUs, printing in the loop above
>> +	 * may take enough time to render the polling loop above unnecessary
>> +	 * falsifying the debugging info. Thus we report the retries here.
>> +	 */
> 
> People reading this code are likely aware of that, I think you can remove
> this comment.
> 
>> +	dev_info(&mci->dev, "Ready polling took %ums\n", retries);
> 
> dev_dbg()?

Both done in v2.

>> +	/*
>> +	 * Quoting eMMC Spec v5.1 (JEDEC Standard No. 84-B51):
>> +	 * Due to legacy reasons, a Device may still treat CMD24/25 during
>> +	 * prg-state (while busy is active) as a legal or illegal command.
>> +	 * A host should not send CMD24/25 while the Device is in the prg
>> +	 * state and busy is active.
>> +	 */
>> +	ret = mci_poll_until_ready(mci, 10 /* ms */);
> 
> Is 10ms enough? U-Boot has 1s here.

My line of thinking was that we got away without this so far, so if 10ms
isn't enough, there's probably something wrong elsewhere. That's little
consolation to users that would get breakage if they indeed need to wait
longer, so I bumped it to 1000ms.

Thanks,
Ahmad

> 
> Sascha
> 

-- 
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 |