RE: [PATCH] mmc: zynq_sdhci: Dll reset only for ZynqMP platform

["Jaehoon Chung" <jh80.chung@samsung.com>]

Hi,

> -----Original Message-----
> From: Michal Simek <michal.simek@amd.com>
> Sent: Tuesday, July 11, 2023 3:28 PM
> To: Jaehoon Chung <jh80.chung@samsung.com>; u-boot@lists.denx.de; git@xilinx.com
> Cc: 'Ashok Reddy Soma' <ashok.reddy.soma@amd.com>; 'Peng Fan' <peng.fan@nxp.com>
> Subject: Re: [PATCH] mmc: zynq_sdhci: Dll reset only for ZynqMP platform
> 
> Hi,
> 
> On 7/11/23 07:00, Jaehoon Chung wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Michal Simek <michal.simek@amd.com>
> >> Sent: Monday, July 10, 2023 9:12 PM
> >> To: u-boot@lists.denx.de; git@xilinx.com
> >> Cc: Ashok Reddy Soma <ashok.reddy.soma@amd.com>; Jaehoon Chung <jh80.chung@samsung.com>; Peng Fan
> >> <peng.fan@nxp.com>
> >> Subject: [PATCH] mmc: zynq_sdhci: Dll reset only for ZynqMP platform
> >>
> >> From: Ashok Reddy Soma <ashok.reddy.soma@amd.com>
> >>
> >> Dll reset is needed only for ZynqMP platforms, add condition in tuning
> >> to call arasan_zynqmp_dll_reset() just for ZynqMP platforms.
> >>
> >> On other platforms like Versal NET, If this condition is not added, we
> >> see PLM error messages when dll reset smc is called.
> >>
> >> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@amd.com>
> >> Signed-off-by: Michal Simek <michal.simek@amd.com>
> >> ---
> >>
> >>   drivers/mmc/zynq_sdhci.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> >> index e779251ce34f..935540d17194 100644
> >> --- a/drivers/mmc/zynq_sdhci.c
> >> +++ b/drivers/mmc/zynq_sdhci.c
> >> @@ -422,7 +422,8 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
> >>
> >>   	mdelay(1);
> >>
> >> -	arasan_zynqmp_dll_reset(host, priv->node_id);
> >> +	if (device_is_compatible(mmc->dev, "xlnx,zynqmp-8.9a"))
> >> +		arasan_zynqmp_dll_reset(host, priv->node_id);
> >
> > How about using local variable to check whether it needs to reset or not?
> > It's not efficient to call device_is_compatible() everytime.
> > (I'm not sure that it will be added more in future.)
> >
> > e.g)
> > bool reset = device_is_compatible(mmc->dev, "xlx,zynmp-8.8a");
> >
> > if (reset)
> > 	arasan_zynqmp_dll_reset(host, priv->node_id);
> >
> > ..
> >
> > If (reset)
> > 	arasan_zynqmp_dll_reset(host, priv->node_id);
> 
> This is very valid request and TBH I have already added this to our TODO list to
> convert all device_is_compatible() to flags because over time the driver was
> extended and this construct is used more than it should be.
> 
> This is going to be the last device_is_compatible() patch.
> Is it fine for you?

It's fine. Thanks! 

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> 
> Thanks,
> Michal
> 
>