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

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

[Michal Simek]

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);
 
 	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE);
 	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE);
@@ -468,7 +469,9 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	}
 
 	udelay(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);
 
 	/* Enable only interrupts served by the SD controller */
 	sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,
-- 
2.36.1

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

[Jaehoon Chung]

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

Best Regards,
Jaehoon Chung


> 
>  	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE);
>  	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE);
> @@ -468,7 +469,9 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  	}
> 
>  	udelay(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);
> 
>  	/* Enable only interrupts served by the SD controller */
>  	sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,
> --
> 2.36.1

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

[Michal Simek]

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?

Thanks,
Michal

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

[Jaehoon Chung]

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

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

[Michal Simek]

On 7/10/23 14:11, Michal Simek wrote:
> 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);
>   
>   	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_INT_ENABLE);
>   	sdhci_writel(host, SDHCI_INT_DATA_AVAIL, SDHCI_SIGNAL_ENABLE);
> @@ -468,7 +469,9 @@ static int arasan_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>   	}
>   
>   	udelay(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);
>   
>   	/* Enable only interrupts served by the SD controller */
>   	sdhci_writel(host, SDHCI_INT_DATA_MASK | SDHCI_INT_CMD_MASK,

Applied.
M