[PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Marek Vasut]

In case the tap delay required by Arasan SDHCI is set to 0, the current
embeddedsw firmware unconditionally writes IOU_SLCR SD_ITAPDLY to 0x100
(SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior was to keep the
IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of difference in the
behavior between SD0_ITAPDLYENA=1/0 with the same SD0_ITAPDLYSEL=0, even
though the behavior should be identical -- zero delay added to rxclk_in
line. The former breaks HS200 training in low temperature conditions.

Avoid writing the IOU_SLCR SD_ITAPDLY register in case value is 0 to
keep the register at value 0x0 to reinstate the previous behavior that
was present in Xilinx downstream fork of Linux 4.19.y and which prevented
breakage of HS200 training in low temperature conditions.

Note that the embeddedsw firmware does not permit clearing the SD_ITAPDLY
SD0_ITAPDLYENA bit, this bit can only ever be set by the firmware and it
is often impossible to update the possibly broken firmware.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>
Cc: Harsha <harsha.harsha@xilinx.com>
Cc: Michal Simek <michal.simek@amd.com>
Cc: Rajan Vaja <rajan.vaja@xilinx.com>
Cc: Ronak Jain <ronak.jain@xilinx.com>
Cc: Tanmay Shah <tanmay.shah@xilinx.com>
To: linux-arm-kernel@lists.infradead.org
---
 drivers/firmware/xilinx/zynqmp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index ff5cabe70a2b2..12712f64fb932 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -738,6 +738,9 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_get_pll_frac_data);
  */
 int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value)
 {
+	if (!value)
+		return 0;
+
 	return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, IOCTL_SET_SD_TAPDELAY,
 				   type, value, NULL);
 }
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Michal Simek]

+Sai

On 10/17/22 14:35, Marek Vasut wrote:
> In case the tap delay required by Arasan SDHCI is set to 0, the current
> embeddedsw firmware unconditionally writes IOU_SLCR SD_ITAPDLY to 0x100
> (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior was to keep the
> IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of difference in the
> behavior between SD0_ITAPDLYENA=1/0 with the same SD0_ITAPDLYSEL=0, even
> though the behavior should be identical -- zero delay added to rxclk_in
> line. The former breaks HS200 training in low temperature conditions.
> 
> Avoid writing the IOU_SLCR SD_ITAPDLY register in case value is 0 to
> keep the register at value 0x0 to reinstate the previous behavior that
> was present in Xilinx downstream fork of Linux 4.19.y and which prevented
> breakage of HS200 training in low temperature conditions.
> 
> Note that the embeddedsw firmware does not permit clearing the SD_ITAPDLY
> SD0_ITAPDLYENA bit, this bit can only ever be set by the firmware and it
> is often impossible to update the possibly broken firmware.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>
> Cc: Harsha <harsha.harsha@xilinx.com>
> Cc: Michal Simek <michal.simek@amd.com>
> Cc: Rajan Vaja <rajan.vaja@xilinx.com>
> Cc: Ronak Jain <ronak.jain@xilinx.com>
> Cc: Tanmay Shah <tanmay.shah@xilinx.com>
> To: linux-arm-kernel@lists.infradead.org
> ---
>   drivers/firmware/xilinx/zynqmp.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index ff5cabe70a2b2..12712f64fb932 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -738,6 +738,9 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_get_pll_frac_data);
>    */
>   int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value)
>   {
> +	if (!value)
> +		return 0;
> +
>   	return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, IOCTL_SET_SD_TAPDELAY,
>   				   type, value, NULL);
>   }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Potthuri, Sai Krishna]

Hi Marek Vasut,

> -----Original Message-----
> From: Simek, Michal <michal.simek@amd.com>
> Sent: Monday, October 17, 2022 6:16 PM
> To: Marek Vasut <marex@denx.de>; linux-arm-kernel@lists.infradead.org;
> Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>
> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>; Harsha
> <harsha.harsha@xilinx.com>; Rajan Vaja <rajan.vaja@xilinx.com>; Ronak
> Jain <ronak.jain@xilinx.com>; Tanmay Shah <tanmay.shah@xilinx.com>
> Subject: Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY
> for value 0
> 
> +Sai
> 
> On 10/17/22 14:35, Marek Vasut wrote:
> > In case the tap delay required by Arasan SDHCI is set to 0, the
> > current embeddedsw firmware unconditionally writes IOU_SLCR
> SD_ITAPDLY
> > to 0x100 (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior was
> > to keep the IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of
> > difference in the behavior between SD0_ITAPDLYENA=1/0 with the same
> > SD0_ITAPDLYSEL=0, even though the behavior should be identical -- zero
> > delay added to rxclk_in line. The former breaks HS200 training in low
> temperature conditions.
We got a similar issue from one of the customers saying tuning was failing
at lower temperatures with 4.19 kernel.
Root cause of this issue is incorrect tap delay sequence in 4.19 kernel which
got fixed in 5.4 Xilinx Linux tree (2022.2 release).
4.19 sequence: 
DLL assert->ITAP->DLL de-assert->DLL assert->OTAP->DLL de-assert
5.4 sequence:
DLL assert->ITAP->OTAP->DLL de-assert

Please give a try with the updated sequence.
Setting the tap delay value to Zero is required in some use cases (Boot mode)
to clear the previous software stack tap delay configurations.

Regards
Sai Krishna
> >
> > Avoid writing the IOU_SLCR SD_ITAPDLY register in case value is 0 to
> > keep the register at value 0x0 to reinstate the previous behavior that
> > was present in Xilinx downstream fork of Linux 4.19.y and which
> > prevented breakage of HS200 training in low temperature conditions.
> >
> > Note that the embeddedsw firmware does not permit clearing the
> > SD_ITAPDLY SD0_ITAPDLYENA bit, this bit can only ever be set by the
> > firmware and it is often impossible to update the possibly broken
> firmware.
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > ---
> > Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>
> > Cc: Harsha <harsha.harsha@xilinx.com>
> > Cc: Michal Simek <michal.simek@amd.com>
> > Cc: Rajan Vaja <rajan.vaja@xilinx.com>
> > Cc: Ronak Jain <ronak.jain@xilinx.com>
> > Cc: Tanmay Shah <tanmay.shah@xilinx.com>
> > To: linux-arm-kernel@lists.infradead.org
> > ---
> >   drivers/firmware/xilinx/zynqmp.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > b/drivers/firmware/xilinx/zynqmp.c
> > index ff5cabe70a2b2..12712f64fb932 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -738,6 +738,9 @@
> EXPORT_SYMBOL_GPL(zynqmp_pm_get_pll_frac_data);
> >    */
> >   int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value)
> >   {
> > +	if (!value)
> > +		return 0;
> > +
> >   	return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
> IOCTL_SET_SD_TAPDELAY,
> >   				   type, value, NULL);
> >   }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Marek Vasut]

On 10/17/22 16:04, Potthuri, Sai Krishna wrote:
> Hi Marek Vasut,

Hi,

[...]

>> On 10/17/22 14:35, Marek Vasut wrote:
>>> In case the tap delay required by Arasan SDHCI is set to 0, the
>>> current embeddedsw firmware unconditionally writes IOU_SLCR
>> SD_ITAPDLY
>>> to 0x100 (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior was
>>> to keep the IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of
>>> difference in the behavior between SD0_ITAPDLYENA=1/0 with the same
>>> SD0_ITAPDLYSEL=0, even though the behavior should be identical -- zero
>>> delay added to rxclk_in line. The former breaks HS200 training in low
>> temperature conditions.
> We got a similar issue from one of the customers saying tuning was failing
> at lower temperatures with 4.19 kernel.
> Root cause of this issue is incorrect tap delay sequence in 4.19 kernel which
> got fixed in 5.4 Xilinx Linux tree (2022.2 release).

I am not using the Xilinx downstream stuff, I am using linux-next and 
Linux 5.10.y from linux-stable for these tests.

> 4.19 sequence:
> DLL assert->ITAP->DLL de-assert->DLL assert->OTAP->DLL de-assert
> 5.4 sequence:
> DLL assert->ITAP->OTAP->DLL de-assert
> 
> Please give a try with the updated sequence.

Whatever fixes are in Linux 5.4 should already be present, and no, that 
does NOT work.

> Setting the tap delay value to Zero is required in some use cases (Boot mode)
> to clear the previous software stack tap delay configurations.

The problem is that with SD0_ITAPDLYENA=1 and SD0_ITAPDLYSEL=0 the HS200 
calibration fails, with SD0_ITAPDLYENA=0 and SD0_ITAPDLYSEL=0 
calibration succeeds.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Potthuri, Sai Krishna]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, October 17, 2022 7:41 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek, Michal
> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>; Harsha
> <harsha.harsha@xilinx.com>; Rajan Vaja <rajan.vaja@xilinx.com>; Ronak
> Jain <ronak.jain@xilinx.com>; Tanmay Shah <tanmay.shah@xilinx.com>
> Subject: Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY
> for value 0
> 
> On 10/17/22 16:04, Potthuri, Sai Krishna wrote:
> > Hi Marek Vasut,
> 
> Hi,
> 
> [...]
> 
> >> On 10/17/22 14:35, Marek Vasut wrote:
> >>> In case the tap delay required by Arasan SDHCI is set to 0, the
> >>> current embeddedsw firmware unconditionally writes IOU_SLCR
> >> SD_ITAPDLY
> >>> to 0x100 (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior
> was
> >>> to keep the IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of
> >>> difference in the behavior between SD0_ITAPDLYENA=1/0 with the same
> >>> SD0_ITAPDLYSEL=0, even though the behavior should be identical --
> >>> zero delay added to rxclk_in line. The former breaks HS200 training
> >>> in low
> >> temperature conditions.
> > We got a similar issue from one of the customers saying tuning was
> > failing at lower temperatures with 4.19 kernel.
> > Root cause of this issue is incorrect tap delay sequence in 4.19
> > kernel which got fixed in 5.4 Xilinx Linux tree (2022.2 release).
> 
> I am not using the Xilinx downstream stuff, I am using linux-next and Linux
> 5.10.y from linux-stable for these tests.
> 
> > 4.19 sequence:
> > DLL assert->ITAP->DLL de-assert->DLL assert->OTAP->DLL de-assert
> > 5.4 sequence:
> > DLL assert->ITAP->OTAP->DLL de-assert
> >
> > Please give a try with the updated sequence.
> 
> Whatever fixes are in Linux 5.4 should already be present, and no, that does
> NOT work.
This fix has dependency on ATF version, are you testing with >=2.5 version?
https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5

Regards
Sai Krishna
> 
> > Setting the tap delay value to Zero is required in some use cases
> > (Boot mode) to clear the previous software stack tap delay configurations.
> 
> The problem is that with SD0_ITAPDLYENA=1 and SD0_ITAPDLYSEL=0 the
> HS200 calibration fails, with SD0_ITAPDLYENA=0 and SD0_ITAPDLYSEL=0
> calibration succeeds.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Marek Vasut]

On 10/18/22 09:07, Potthuri, Sai Krishna wrote:
> Hi,

Hi,

>>>> On 10/17/22 14:35, Marek Vasut wrote:
>>>>> In case the tap delay required by Arasan SDHCI is set to 0, the
>>>>> current embeddedsw firmware unconditionally writes IOU_SLCR
>>>> SD_ITAPDLY
>>>>> to 0x100 (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior
>> was
>>>>> to keep the IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of
>>>>> difference in the behavior between SD0_ITAPDLYENA=1/0 with the same
>>>>> SD0_ITAPDLYSEL=0, even though the behavior should be identical --
>>>>> zero delay added to rxclk_in line. The former breaks HS200 training
>>>>> in low
>>>> temperature conditions.
>>> We got a similar issue from one of the customers saying tuning was
>>> failing at lower temperatures with 4.19 kernel.
>>> Root cause of this issue is incorrect tap delay sequence in 4.19
>>> kernel which got fixed in 5.4 Xilinx Linux tree (2022.2 release).
>>
>> I am not using the Xilinx downstream stuff, I am using linux-next and Linux
>> 5.10.y from linux-stable for these tests.
>>
>>> 4.19 sequence:
>>> DLL assert->ITAP->DLL de-assert->DLL assert->OTAP->DLL de-assert
>>> 5.4 sequence:
>>> DLL assert->ITAP->OTAP->DLL de-assert
>>>
>>> Please give a try with the updated sequence.
>>
>> Whatever fixes are in Linux 5.4 should already be present, and no, that does
>> NOT work.
> This fix has dependency on ATF version, are you testing with >=2.5 version?
> https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5

No, I am still running whatever downstream fork of ATF came with the 
hardware and this cannot be updated.

Can you point me to specific commit(s) in the aforementioned repository 
which are related to this topic ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Potthuri, Sai Krishna]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Tuesday, October 18, 2022 2:37 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek, Michal
> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>; Harsha
> <harsha.harsha@xilinx.com>; Rajan Vaja <rajan.vaja@xilinx.com>; Ronak
> Jain <ronak.jain@xilinx.com>; Tanmay Shah <tanmay.shah@xilinx.com>
> Subject: Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY
> for value 0
> 
> On 10/18/22 09:07, Potthuri, Sai Krishna wrote:
> > Hi,
> 
> Hi,
> 
> >>>> On 10/17/22 14:35, Marek Vasut wrote:
> >>>>> In case the tap delay required by Arasan SDHCI is set to 0, the
> >>>>> current embeddedsw firmware unconditionally writes IOU_SLCR
> >>>> SD_ITAPDLY
> >>>>> to 0x100 (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior
> >> was
> >>>>> to keep the IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of
> >>>>> difference in the behavior between SD0_ITAPDLYENA=1/0 with the
> >>>>> same SD0_ITAPDLYSEL=0, even though the behavior should be
> >>>>> identical -- zero delay added to rxclk_in line. The former breaks
> >>>>> HS200 training in low
> >>>> temperature conditions.
> >>> We got a similar issue from one of the customers saying tuning was
> >>> failing at lower temperatures with 4.19 kernel.
> >>> Root cause of this issue is incorrect tap delay sequence in 4.19
> >>> kernel which got fixed in 5.4 Xilinx Linux tree (2022.2 release).
> >>
> >> I am not using the Xilinx downstream stuff, I am using linux-next and
> >> Linux 5.10.y from linux-stable for these tests.
> >>
> >>> 4.19 sequence:
> >>> DLL assert->ITAP->DLL de-assert->DLL assert->OTAP->DLL de-assert
> >>> 5.4 sequence:
> >>> DLL assert->ITAP->OTAP->DLL de-assert
> >>>
> >>> Please give a try with the updated sequence.
> >>
> >> Whatever fixes are in Linux 5.4 should already be present, and no,
> >> that does NOT work.
> > This fix has dependency on ATF version, are you testing with >=2.5 version?
> > https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> 
> No, I am still running whatever downstream fork of ATF came with the
> hardware and this cannot be updated.
> 
> Can you point me to specific commit(s) in the aforementioned repository
> which are related to this topic ?
ATF change I am talking about is
https://github.com/ARM-software/arm-trusted-firmware/commit/2ab0ef8db9561699fef0f77f5a1735e4903f6b3e

Looks like we are already taking care of disabling the ITAP in ATF(below commit)
if we get ZERO tap.
https://github.com/ARM-software/arm-trusted-firmware/commit/fe1fa205fca4d1dd4a1b1755942956dbca65d573

Above changes are part of ATF 2.5 version.

Regards
Sai Krishna
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Marek Vasut]

On 10/18/22 12:01, Potthuri, Sai Krishna wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Tuesday, October 18, 2022 2:37 PM
>> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek, Michal
>> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
>> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>; Harsha
>> <harsha.harsha@xilinx.com>; Rajan Vaja <rajan.vaja@xilinx.com>; Ronak
>> Jain <ronak.jain@xilinx.com>; Tanmay Shah <tanmay.shah@xilinx.com>
>> Subject: Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY
>> for value 0
>>
>> On 10/18/22 09:07, Potthuri, Sai Krishna wrote:
>>> Hi,
>>
>> Hi,
>>
>>>>>> On 10/17/22 14:35, Marek Vasut wrote:
>>>>>>> In case the tap delay required by Arasan SDHCI is set to 0, the
>>>>>>> current embeddedsw firmware unconditionally writes IOU_SLCR
>>>>>> SD_ITAPDLY
>>>>>>> to 0x100 (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous behavior
>>>> was
>>>>>>> to keep the IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort of
>>>>>>> difference in the behavior between SD0_ITAPDLYENA=1/0 with the
>>>>>>> same SD0_ITAPDLYSEL=0, even though the behavior should be
>>>>>>> identical -- zero delay added to rxclk_in line. The former breaks
>>>>>>> HS200 training in low
>>>>>> temperature conditions.
>>>>> We got a similar issue from one of the customers saying tuning was
>>>>> failing at lower temperatures with 4.19 kernel.
>>>>> Root cause of this issue is incorrect tap delay sequence in 4.19
>>>>> kernel which got fixed in 5.4 Xilinx Linux tree (2022.2 release).
>>>>
>>>> I am not using the Xilinx downstream stuff, I am using linux-next and
>>>> Linux 5.10.y from linux-stable for these tests.
>>>>
>>>>> 4.19 sequence:
>>>>> DLL assert->ITAP->DLL de-assert->DLL assert->OTAP->DLL de-assert
>>>>> 5.4 sequence:
>>>>> DLL assert->ITAP->OTAP->DLL de-assert
>>>>>
>>>>> Please give a try with the updated sequence.
>>>>
>>>> Whatever fixes are in Linux 5.4 should already be present, and no,
>>>> that does NOT work.
>>> This fix has dependency on ATF version, are you testing with >=2.5 version?
>>> https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
>>
>> No, I am still running whatever downstream fork of ATF came with the
>> hardware and this cannot be updated.
>>
>> Can you point me to specific commit(s) in the aforementioned repository
>> which are related to this topic ?
> ATF change I am talking about is
> https://github.com/ARM-software/arm-trusted-firmware/commit/2ab0ef8db9561699fef0f77f5a1735e4903f6b3e
> 
> Looks like we are already taking care of disabling the ITAP in ATF(below commit)
> if we get ZERO tap.
> https://github.com/ARM-software/arm-trusted-firmware/commit/fe1fa205fca4d1dd4a1b1755942956dbca65d573
> 
> Above changes are part of ATF 2.5 version.

So what's the fix for systems which run older version of the firmware 
and which also need to be fixed ?

The ATF change seems unrelated to SD0_ITAPDLYENA=1/0 toggling, right ? 
So how can that fix the problem ? Why does the system fail calibration 
when SD0_ITAPDLYENA=1 and pass with SD0_ITAPDLYENA=0 ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Potthuri, Sai Krishna]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Tuesday, October 18, 2022 3:35 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek, Michal
> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>; Harsha
> <harsha.harsha@xilinx.com>; Rajan Vaja <rajan.vaja@xilinx.com>; Ronak
> Jain <ronak.jain@xilinx.com>; Tanmay Shah <tanmay.shah@xilinx.com>
> Subject: Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY
> for value 0
> 
> On 10/18/22 12:01, Potthuri, Sai Krishna wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: Tuesday, October 18, 2022 2:37 PM
> >> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek,
> >> Michal <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> >> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>; Harsha
> >> <harsha.harsha@xilinx.com>; Rajan Vaja <rajan.vaja@xilinx.com>; Ronak
> >> Jain <ronak.jain@xilinx.com>; Tanmay Shah <tanmay.shah@xilinx.com>
> >> Subject: Re: [PATCH] firmware: xilinx: Do not call
> >> IOCTL_SET_SD_TAPDELAY for value 0
> >>
> >> On 10/18/22 09:07, Potthuri, Sai Krishna wrote:
> >>> Hi,
> >>
> >> Hi,
> >>
> >>>>>> On 10/17/22 14:35, Marek Vasut wrote:
> >>>>>>> In case the tap delay required by Arasan SDHCI is set to 0, the
> >>>>>>> current embeddedsw firmware unconditionally writes IOU_SLCR
> >>>>>> SD_ITAPDLY
> >>>>>>> to 0x100 (SD0_ITAPDLYENA=1, SD0_ITAPDLYSEL=0). Previous
> behavior
> >>>> was
> >>>>>>> to keep the IOU_SLCR SD_ITAPDLY set to 0x0. There is some sort
> >>>>>>> of difference in the behavior between SD0_ITAPDLYENA=1/0 with
> >>>>>>> the same SD0_ITAPDLYSEL=0, even though the behavior should be
> >>>>>>> identical -- zero delay added to rxclk_in line. The former
> >>>>>>> breaks
> >>>>>>> HS200 training in low
> >>>>>> temperature conditions.
> >>>>> We got a similar issue from one of the customers saying tuning was
> >>>>> failing at lower temperatures with 4.19 kernel.
> >>>>> Root cause of this issue is incorrect tap delay sequence in 4.19
> >>>>> kernel which got fixed in 5.4 Xilinx Linux tree (2022.2 release).
> >>>>
> >>>> I am not using the Xilinx downstream stuff, I am using linux-next
> >>>> and Linux 5.10.y from linux-stable for these tests.
> >>>>
> >>>>> 4.19 sequence:
> >>>>> DLL assert->ITAP->DLL de-assert->DLL assert->OTAP->DLL de-assert
> >>>>> 5.4 sequence:
> >>>>> DLL assert->ITAP->OTAP->DLL de-assert
> >>>>>
> >>>>> Please give a try with the updated sequence.
> >>>>
> >>>> Whatever fixes are in Linux 5.4 should already be present, and no,
> >>>> that does NOT work.
> >>> This fix has dependency on ATF version, are you testing with >=2.5
> version?
> >>> https://github.com/ARM-software/arm-trusted-firmware/tree/v2.5
> >>
> >> No, I am still running whatever downstream fork of ATF came with the
> >> hardware and this cannot be updated.
> >>
> >> Can you point me to specific commit(s) in the aforementioned
> >> repository which are related to this topic ?
> > ATF change I am talking about is
> > https://github.com/ARM-software/arm-trusted-
> firmware/commit/2ab0ef8db9
> > 561699fef0f77f5a1735e4903f6b3e
> >
> > Looks like we are already taking care of disabling the ITAP in
> > ATF(below commit) if we get ZERO tap.
> > https://github.com/ARM-software/arm-trusted-
> firmware/commit/fe1fa205fc
> > a4d1dd4a1b1755942956dbca65d573
> >
> > Above changes are part of ATF 2.5 version.
> 
> So what's the fix for systems which run older version of the firmware and
> which also need to be fixed ?
> 
> The ATF change seems unrelated to SD0_ITAPDLYENA=1/0 toggling, right ?
> So how can that fix the problem ? Why does the system fail calibration when
> SD0_ITAPDLYENA=1 and pass with SD0_ITAPDLYENA=0 ?
https://github.com/ARM-software/arm-trusted-firmware/commit/fe1fa205fca4d1dd4a1b1755942956dbca65d573
This commit does what ever this patch is trying to achieve. If my understanding is correct, you want SD0_ITAPDLYENA to be 0 if user tries to write 0 to SD0_ITAPDLYSEL. This commit does the same by writing 0 to ITAP register if ITAPSEL is 0.

This is the recommendation from IP designers to clear the external controls (SDx_ITAPDLYENA should be reset) for auto-tuning. Same mentioned in the ZynqMP TRM under "Receive Clock Tap Delay" section.

Regards
Sai Krishna
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Marek Vasut]

On 10/18/22 12:37, Potthuri, Sai Krishna wrote:

Hi,

[...]

>>>> No, I am still running whatever downstream fork of ATF came with the
>>>> hardware and this cannot be updated.
>>>>
>>>> Can you point me to specific commit(s) in the aforementioned
>>>> repository which are related to this topic ?
>>> ATF change I am talking about is
>>> https://github.com/ARM-software/arm-trusted-
>> firmware/commit/2ab0ef8db9
>>> 561699fef0f77f5a1735e4903f6b3e
>>>
>>> Looks like we are already taking care of disabling the ITAP in
>>> ATF(below commit) if we get ZERO tap.
>>> https://github.com/ARM-software/arm-trusted-
>> firmware/commit/fe1fa205fc
>>> a4d1dd4a1b1755942956dbca65d573
>>>
>>> Above changes are part of ATF 2.5 version.
>>
>> So what's the fix for systems which run older version of the firmware and
>> which also need to be fixed ?
>>
>> The ATF change seems unrelated to SD0_ITAPDLYENA=1/0 toggling, right ?
>> So how can that fix the problem ? Why does the system fail calibration when
>> SD0_ITAPDLYENA=1 and pass with SD0_ITAPDLYENA=0 ?
> https://github.com/ARM-software/arm-trusted-firmware/commit/fe1fa205fca4d1dd4a1b1755942956dbca65d573
> This commit does what ever this patch is trying to achieve. If my understanding is correct, you want SD0_ITAPDLYENA to be 0 if user tries to write 0 to SD0_ITAPDLYSEL. This commit does the same by writing 0 to ITAP register if ITAPSEL is 0.

This is not helpful, since I cannot update firmware on this platform.
A Linux-only fix for this bug is necessary. What are the options here ?

(this here is really a good example of why burying important 
functionality into firmware instead of keeping it in Linux is 
problematic harmful practice)

> This is the recommendation from IP designers to clear the external controls (SDx_ITAPDLYENA should be reset) for auto-tuning. Same mentioned in the ZynqMP TRM under "Receive Clock Tap Delay" section.

What is the difference in behavior between:
- SD0_ITAPDLYENA=0 SD0_ITAPDLYSEL=0
and
- SD0_ITAPDLYENA=1 SD0_ITAPDLYSEL=0
?

My understanding is that the behavior should be identical, but it is 
not. Why?

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY for value 0

[Potthuri, Sai Krishna]

Hi,

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Tuesday, October 18, 2022 6:02 PM
> To: Potthuri, Sai Krishna <sai.krishna.potthuri@amd.com>; Simek, Michal
> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> Cc: Abhyuday Godhasara <abhyuday.godhasara@xilinx.com>; Harsha
> <harsha.harsha@xilinx.com>; Rajan Vaja <rajan.vaja@xilinx.com>; Ronak
> Jain <ronak.jain@xilinx.com>; Tanmay Shah <tanmay.shah@xilinx.com>
> Subject: Re: [PATCH] firmware: xilinx: Do not call IOCTL_SET_SD_TAPDELAY
> for value 0
> 
> On 10/18/22 12:37, Potthuri, Sai Krishna wrote:
> 
> Hi,
> 
> [...]
> 
> >>>> No, I am still running whatever downstream fork of ATF came with
> >>>> the hardware and this cannot be updated.
> >>>>
> >>>> Can you point me to specific commit(s) in the aforementioned
> >>>> repository which are related to this topic ?
> >>> ATF change I am talking about is
> >>> https://github.com/ARM-software/arm-trusted-
> >> firmware/commit/2ab0ef8db9
> >>> 561699fef0f77f5a1735e4903f6b3e
> >>>
> >>> Looks like we are already taking care of disabling the ITAP in
> >>> ATF(below commit) if we get ZERO tap.
> >>> https://github.com/ARM-software/arm-trusted-
> >> firmware/commit/fe1fa205fc
> >>> a4d1dd4a1b1755942956dbca65d573
> >>>
> >>> Above changes are part of ATF 2.5 version.
> >>
> >> So what's the fix for systems which run older version of the firmware
> >> and which also need to be fixed ?
> >>
> >> The ATF change seems unrelated to SD0_ITAPDLYENA=1/0 toggling, right ?
> >> So how can that fix the problem ? Why does the system fail
> >> calibration when
> >> SD0_ITAPDLYENA=1 and pass with SD0_ITAPDLYENA=0 ?
> > https://github.com/ARM-software/arm-trusted-
> firmware/commit/fe1fa205fc
> > a4d1dd4a1b1755942956dbca65d573 This commit does what ever this
> patch
> > is trying to achieve. If my understanding is correct, you want
> SD0_ITAPDLYENA to be 0 if user tries to write 0 to SD0_ITAPDLYSEL. This
> commit does the same by writing 0 to ITAP register if ITAPSEL is 0.
> 
> This is not helpful, since I cannot update firmware on this platform.
> A Linux-only fix for this bug is necessary. What are the options here ?
I don’t see any option where we can write directly from the Linux, as tap
related registers are in secure space, and it requires ATF interface.
Since ATF interface is broken in this case, we cannot write 0x0 to ITAPDLYENA
(also Tap delay sequence is also incorrect in this ATF version).
Only option i can see to make eMMC interface work with firmware version
you are using is fallback to High-Speed mode.

> 
> (this here is really a good example of why burying important functionality
> into firmware instead of keeping it in Linux is problematic harmful practice)
> 
> > This is the recommendation from IP designers to clear the external controls
> (SDx_ITAPDLYENA should be reset) for auto-tuning. Same mentioned in the
> ZynqMP TRM under "Receive Clock Tap Delay" section.
> 
> What is the difference in behavior between:
> - SD0_ITAPDLYENA=0 SD0_ITAPDLYSEL=0
> and
> - SD0_ITAPDLYENA=1 SD0_ITAPDLYSEL=0
> ?
> 
> My understanding is that the behavior should be identical, but it is not. Why?
eMMC HS200 requires auto-tuning, as part of auto-tuning, controller calculates
the required ITAP delay and used it for further operations. As per RTL logic, if
ITAPDLYENA bit is 1 then it takes the taps from ITAP register (manual tap
programmed by user) otherwise taps will be considered from auto-tuning logic.

Regards
Sai Krishna
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel