On 20/02/2024 11:26, Marek Vasut wrote: > On 2/20/24 11:50, Paul Barker wrote: >> On 20/02/2024 08:36, Marek Vasut wrote: >>> The cmd_error parameter is not used, remove it. >>> [snip] >>> >>> diff --git a/drivers/mmc/mtk-sd.c b/drivers/mmc/mtk-sd.c >>> index 5a0c61daed5..296aaee7331 100644 >>> --- a/drivers/mmc/mtk-sd.c >>> +++ b/drivers/mmc/mtk-sd.c >>> @@ -1131,7 +1131,7 @@ static int hs400_tune_response(struct udevice *dev, >>> u32 opcode) >>> i << PAD_CMD_TUNE_RX_DLY3_S); >>> >>> for (j = 0; j < 3; j++) { >>> - mmc_send_tuning(mmc, opcode, &cmd_err); >>> + cmd_err = mmc_send_tuning(mmc, opcode); >>> if (!cmd_err) { >>> cmd_delay |= (1 << i); >>> } else { >>> @@ -1181,7 +1181,7 @@ static int msdc_tune_response(struct udevice *dev, >>> u32 opcode) >>> i << MSDC_PAD_TUNE_CMDRDLY_S); >>> >>> for (j = 0; j < 3; j++) { >>> - mmc_send_tuning(mmc, opcode, &cmd_err); >>> + cmd_err = mmc_send_tuning(mmc, opcode); >>> if (!cmd_err) { >>> rise_delay |= (1 << i); >>> } else { >>> @@ -1203,7 +1203,7 @@ static int msdc_tune_response(struct udevice *dev, >>> u32 opcode) >>> i << MSDC_PAD_TUNE_CMDRDLY_S); >>> >>> for (j = 0; j < 3; j++) { >>> - mmc_send_tuning(mmc, opcode, &cmd_err); >>> + cmd_err = mmc_send_tuning(mmc, opcode); >>> if (!cmd_err) { >>> fall_delay |= (1 << i); >>> } else { >>> @@ -1238,7 +1238,7 @@ skip_fall: >>> clrsetbits_le32(tune_reg, MSDC_PAD_TUNE_CMDRRDLY_M, >>> i << MSDC_PAD_TUNE_CMDRRDLY_S); >>> >>> - mmc_send_tuning(mmc, opcode, &cmd_err); >>> + cmd_err = mmc_send_tuning(mmc, opcode); >>> if (!cmd_err) >>> internal_delay |= (1 << i); >>> } >>> @@ -1264,7 +1264,6 @@ static int msdc_tune_data(struct udevice *dev, u32 >>> opcode) >>> struct msdc_delay_phase final_rise_delay, final_fall_delay = { 0, }; >>> u8 final_delay, final_maxlen; >>> void __iomem *tune_reg = &host->base->pad_tune; >>> - int cmd_err; >>> int i, ret; >>> >>> if (host->dev_comp->pad_tune0) >>> @@ -1277,10 +1276,10 @@ static int msdc_tune_data(struct udevice *dev, u32 >>> opcode) >>> clrsetbits_le32(tune_reg, MSDC_PAD_TUNE_DATRRDLY_M, >>> i << MSDC_PAD_TUNE_DATRRDLY_S); >>> >>> - ret = mmc_send_tuning(mmc, opcode, &cmd_err); >>> + ret = mmc_send_tuning(mmc, opcode); >>> if (!ret) { >>> rise_delay |= (1 << i); >>> - } else if (cmd_err) { >>> + } else { >>> /* in this case, retune response is needed */ >>> ret = msdc_tune_response(dev, opcode); >>> if (ret) >>> @@ -1300,10 +1299,10 @@ static int msdc_tune_data(struct udevice *dev, u32 >>> opcode) >>> clrsetbits_le32(tune_reg, MSDC_PAD_TUNE_DATRRDLY_M, >>> i << MSDC_PAD_TUNE_DATRRDLY_S); >>> >>> - ret = mmc_send_tuning(mmc, opcode, &cmd_err); >>> + ret = mmc_send_tuning(mmc, opcode); >>> if (!ret) { >>> fall_delay |= (1 << i); >>> - } else if (cmd_err) { >>> + } else { >>> /* in this case, retune response is needed */ >>> ret = msdc_tune_response(dev, opcode); >>> if (ret) >> >> This driver (mtk-sd.c) seems to be the only one that really uses the >> `cmd_error` parameter. >> >> Looking at the implementation of mmc_send_tuning() in Linux, this >> parameter is used so that a caller can differentiate between a command >> error and a data error. I don't know enough details about MMC to >> understand the distinction, but I assume there is some reason for this. >> So I wonder if the mtk-sd driver will still work if those error paths >> are taken for data errors and not just command errors. Has this change >> been tested on some board which uses this driver? > > Not by me, so far this driver used uninitialized error value and assumed > it was initialized as far as I can tell, so it is likely already broken.
+To: Ryder Lee, Weijie Gao, Chunfeng Yun +Cc: gss_mtk_uboot_upstr...@mediatek.com Do you have any input as ARM MEDIATEK maintainers? -- Paul Barker
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature