linux-kernel - Re: [PATCH] mmc: mtk-sd: optimize register settings for hs400(es) mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8375b00527c8b2a3b6a1206498f9ba671588f5de.camel@mediatek.com>
Date: Thu, 23 Jan 2025 09:02:34 +0000
From: Andy-ld Lu (卢东) <Andy-ld.Lu@...iatek.com>
To: "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>,
	Wenbin Mei (梅文彬) <Wenbin.Mei@...iatek.com>
CC: "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-mmc@...r.kernel.org"
	<linux-mmc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH] mmc: mtk-sd: optimize register settings for hs400(es)
 mode

On Thu, 2025-01-23 at 09:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 23/01/25 08:44, Andy-ld Lu ha scritto:
> > For hs400(es) mode, the 'hs400-ds-delay' is typically configured in
> > the
> > dts. However, some projects may only define 'mediatek,hs400-ds-
> > dly3',
> > which can lead to initialization failures in hs400es mode. CMD13
> > reported
> > response crc error in the mmc_switch_status() just after switching
> > to
> > hs400es mode.
> > 
> > [    1.914038][   T82] mmc0: mmc_select_hs400es failed, error -84
> > [    1.914954][   T82] mmc0: error -84 whilst initialising MMC card
> > 
> > Currently, the hs400_ds_dly3 value is set within the tuning
> > function. This
> > means that the PAD_DS_DLY3 field is not configured before tuning
> > process,
> > which is the reason for the above-mentioned CMD13 response crc
> > error.
> > 
> > Move the PAD_DS_DLY3 field configuration into
> > msdc_prepare_hs400_tuning(),
> > and add a value check of hs400_ds_delay to prevent overwriting by
> > zero when
> > the 'hs400-ds-delay' is not set in the dts. In addition, since
> > hs400(es)
> > only tune the PAD_DS_DLY1, the PAD_DS_DLY2_SEL bit should be
> > cleared to
> > bypass it.
> 
> The commit title is a bit misleading: you're not "optimizing" the
> settings,
> but actually "fixing" a bug here!
> 
> Please fix the commit title.
> 
> > 
> > Signed-off-by: Andy-ld Lu <andy-ld.lu@...iatek.com>
> 
> This commit needs a Fixes tag, please add the relevant one and
> resend.
> 
Thanks for your review, I will update in next change.

> 
> > ---
> >   drivers/mmc/host/mtk-sd.c | 37 +++++++++++++++++++++++-----------
> > ---
> >   1 file changed, 23 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 4b6e91372526..4e3e31ddf84b 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -273,6 +273,7 @@
> >   #define MSDC_PAD_TUNE_CMD2_SEL        BIT(21)   /* RW */
> > 
> >   #define PAD_DS_TUNE_DLY_SEL       BIT(0)      /* RW */
> > +#define PAD_DS_TUNE_DLY2_SEL      BIT(1)       /* RW */
> >   #define PAD_DS_TUNE_DLY1      GENMASK(6, 2)   /* RW */
> >   #define PAD_DS_TUNE_DLY2      GENMASK(11, 7)  /* RW */
> >   #define PAD_DS_TUNE_DLY3      GENMASK(16, 12) /* RW */
> > @@ -317,9 +318,10 @@
> >   #define PAD_CMD_TX_DLY          GENMASK(16, 12)     /* RW */
> > 
> >   /* EMMC50_PAD_DS_TUNE mask */
> > -#define PAD_DS_DLY_SEL               BIT(16) /* RW */
> > -#define PAD_DS_DLY1          GENMASK(14, 10) /* RW */
> > -#define PAD_DS_DLY3          GENMASK(4, 0)   /* RW */
> > +#define PAD_DS_DLY_SEL          BIT(16) /* RW */
> > +#define PAD_DS_DLY2_SEL         BIT(15) /* RW */
> > +#define PAD_DS_DLY1             GENMASK(14, 10) /* RW */
> > +#define PAD_DS_DLY3             GENMASK(4, 0)   /* RW */
> 
> Why are you changing tabulations with spaces?
> Please use tabs.
> 
> Cheers,
> Angelo
> 
Because the new added line seems a little misaligned in the patch file,
I have used spaces instead. However, I will change back to tabs in next
change.

Best regards
Andy-ld Lu