1. Patchew
  2. EDK2
  3. [edk2] [platforms: PATCH 00/10] Armada 7k/8k - misc improvements pt.2
  4. View patch

[edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency

Marcin Wojtas posted 10 patches 7 years, 6 months ago
There is a newer version of this series
[edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Marcin Wojtas 7 years, 6 months ago 
Incorrectly the clock divisor was calculated relatively
to 255MHz instead of actual 400MHz. Fix this.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
index ccbf355..0b9328b 100644
--- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
+++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
@@ -16,6 +16,7 @@
 **/
 
 #include "SdMmcPciHcDxe.h"
+#include "XenonSdhci.h"
 
 /**
   Dump the content of SD/MMC host controller's Capability Register.
@@ -703,9 +704,8 @@ SdMmcHcClockSupply (
   //
   // Calculate a divisor for SD clock frequency
   //
-  ASSERT (Capability.BaseClkFreq != 0);
 
-  BaseClkFreq = Capability.BaseClkFreq;
+  BaseClkFreq = XENON_MMC_MAX_CLK / 1000 / 1000;
   if (ClockFreq == 0) {
     return EFI_INVALID_PARAMETER;
   }
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Leif Lindholm 7 years, 6 months ago 
On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
> Incorrectly the clock divisor was calculated relatively
> to 255MHz instead of actual 400MHz.

This describes the specific symptom, not the problem with the existing
code.

> Fix this.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> index ccbf355..0b9328b 100644
> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> @@ -16,6 +16,7 @@
>  **/
>  
>  #include "SdMmcPciHcDxe.h"
> +#include "XenonSdhci.h"
>  
>  /**
>    Dump the content of SD/MMC host controller's Capability Register.
> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>    //
>    // Calculate a divisor for SD clock frequency
>    //
> -  ASSERT (Capability.BaseClkFreq != 0);
>  
> -  BaseClkFreq = Capability.BaseClkFreq;

Why is Capability.BaseClkFreq the wrong frequency to use?

/
    Leif

> +  BaseClkFreq = XENON_MMC_MAX_CLK / 1000 / 1000;
>    if (ClockFreq == 0) {
>      return EFI_INVALID_PARAMETER;
>    }
> -- 
> 2.7.4
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Marcin Wojtas 7 years, 6 months ago 
2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>> Incorrectly the clock divisor was calculated relatively
>> to 255MHz instead of actual 400MHz.
>
> This describes the specific symptom, not the problem with the existing
> code.
>
>> Fix this.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> index ccbf355..0b9328b 100644
>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> @@ -16,6 +16,7 @@
>>  **/
>>
>>  #include "SdMmcPciHcDxe.h"
>> +#include "XenonSdhci.h"
>>
>>  /**
>>    Dump the content of SD/MMC host controller's Capability Register.
>> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>>    //
>>    // Calculate a divisor for SD clock frequency
>>    //
>> -  ASSERT (Capability.BaseClkFreq != 0);
>>
>> -  BaseClkFreq = Capability.BaseClkFreq;
>
> Why is Capability.BaseClkFreq the wrong frequency to use?
>

The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
An alternative would be change this generic type to UINT16 and update
field properly during initialization - do you prefer that?

Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Ard Biesheuvel 7 years, 6 months ago 
On 26 October 2017 at 14:54, Marcin Wojtas <mw@semihalf.com> wrote:
> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>>> Incorrectly the clock divisor was calculated relatively
>>> to 255MHz instead of actual 400MHz.
>>
>> This describes the specific symptom, not the problem with the existing
>> code.
>>
>>> Fix this.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>> index ccbf355..0b9328b 100644
>>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>> @@ -16,6 +16,7 @@
>>>  **/
>>>
>>>  #include "SdMmcPciHcDxe.h"
>>> +#include "XenonSdhci.h"
>>>
>>>  /**
>>>    Dump the content of SD/MMC host controller's Capability Register.
>>> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>>>    //
>>>    // Calculate a divisor for SD clock frequency
>>>    //
>>> -  ASSERT (Capability.BaseClkFreq != 0);
>>>
>>> -  BaseClkFreq = Capability.BaseClkFreq;
>>
>> Why is Capability.BaseClkFreq the wrong frequency to use?
>>
>
> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
> An alternative would be change this generic type to UINT16 and update
> field properly during initialization - do you prefer that?
>

Isn't that value read from device registers?
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Marcin Wojtas 7 years, 6 months ago 
2017-10-26 15:55 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 26 October 2017 at 14:54, Marcin Wojtas <mw@semihalf.com> wrote:
>> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>>> On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>>>> Incorrectly the clock divisor was calculated relatively
>>>> to 255MHz instead of actual 400MHz.
>>>
>>> This describes the specific symptom, not the problem with the existing
>>> code.
>>>
>>>> Fix this.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>>> ---
>>>>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>>> index ccbf355..0b9328b 100644
>>>> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>>> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>>>> @@ -16,6 +16,7 @@
>>>>  **/
>>>>
>>>>  #include "SdMmcPciHcDxe.h"
>>>> +#include "XenonSdhci.h"
>>>>
>>>>  /**
>>>>    Dump the content of SD/MMC host controller's Capability Register.
>>>> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>>>>    //
>>>>    // Calculate a divisor for SD clock frequency
>>>>    //
>>>> -  ASSERT (Capability.BaseClkFreq != 0);
>>>>
>>>> -  BaseClkFreq = Capability.BaseClkFreq;
>>>
>>> Why is Capability.BaseClkFreq the wrong frequency to use?
>>>
>>
>> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
>> An alternative would be change this generic type to UINT16 and update
>> field properly during initialization - do you prefer that?
>>
>
> Isn't that value read from device registers?

This field in generic Capability1 register is only 8-bit wide, hence
the 255MHz limitation. Xenon controller however is fed with 400MHz and
it clearly cannot be obtained from there.

Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Leif Lindholm 7 years, 6 months ago 
On Thu, Oct 26, 2017 at 03:54:41PM +0200, Marcin Wojtas wrote:
> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
> >> Incorrectly the clock divisor was calculated relatively
> >> to 255MHz instead of actual 400MHz.
> >
> > This describes the specific symptom, not the problem with the existing
> > code.
> >
> >> Fix this.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> >> index ccbf355..0b9328b 100644
> >> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> >> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
> >> @@ -16,6 +16,7 @@
> >>  **/
> >>
> >>  #include "SdMmcPciHcDxe.h"
> >> +#include "XenonSdhci.h"
> >>
> >>  /**
> >>    Dump the content of SD/MMC host controller's Capability Register.
> >> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
> >>    //
> >>    // Calculate a divisor for SD clock frequency
> >>    //
> >> -  ASSERT (Capability.BaseClkFreq != 0);
> >>
> >> -  BaseClkFreq = Capability.BaseClkFreq;
> >
> > Why is Capability.BaseClkFreq the wrong frequency to use?
> 
> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
> An alternative would be change this generic type to UINT16 and update
> field properly during initialization - do you prefer that?

No, I'm still dreaming we might be able to reintegrate this into the
MdeModulePkg driver in some glorious future.

So what you are basically saying is that this controller is running at
a higher frequency than is permitted (or even describable) by the
specification? If so, _that_ needs to be in the commit message (and
really, a comment by the code as well).

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Marcin Wojtas 7 years, 6 months ago 
2017-10-26 16:02 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 03:54:41PM +0200, Marcin Wojtas wrote:
>> 2017-10-26 15:46 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> > On Thu, Oct 26, 2017 at 03:19:36AM +0200, Marcin Wojtas wrote:
>> >> Incorrectly the clock divisor was calculated relatively
>> >> to 255MHz instead of actual 400MHz.
>> >
>> > This describes the specific symptom, not the problem with the existing
>> > code.
>> >
>> >> Fix this.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> >> ---
>> >>  Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> >> index ccbf355..0b9328b 100644
>> >> --- a/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> >> +++ b/Platform/Marvell/Drivers/SdMmc/XenonDxe/SdMmcPciHci.c
>> >> @@ -16,6 +16,7 @@
>> >>  **/
>> >>
>> >>  #include "SdMmcPciHcDxe.h"
>> >> +#include "XenonSdhci.h"
>> >>
>> >>  /**
>> >>    Dump the content of SD/MMC host controller's Capability Register.
>> >> @@ -703,9 +704,8 @@ SdMmcHcClockSupply (
>> >>    //
>> >>    // Calculate a divisor for SD clock frequency
>> >>    //
>> >> -  ASSERT (Capability.BaseClkFreq != 0);
>> >>
>> >> -  BaseClkFreq = Capability.BaseClkFreq;
>> >
>> > Why is Capability.BaseClkFreq the wrong frequency to use?
>>
>> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
>> An alternative would be change this generic type to UINT16 and update
>> field properly during initialization - do you prefer that?
>
> No, I'm still dreaming we might be able to reintegrate this into the
> MdeModulePkg driver in some glorious future.

Yes, that would be great. I imagine some SDMMC_HOST_PROTOCOL exposing
callbacks to set UHS, custom clock handling, etc (like it's done in
the Linux).

>
> So what you are basically saying is that this controller is running at
> a higher frequency than is permitted (or even describable) by the
> specification? If so, _that_ needs to be in the commit message (and
> really, a comment by the code as well).

Yes, this clock value is Xenon controller's quirk. I will mention it
in commit log/comment, but please let know if you wish me to:
a. extend BaseClkFreq to UINT16 and configure it properly during init
b. leave as is with better description only
I lean towards a., it's a very low-cost quirk to be applied.

Best regards,
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Leif Lindholm 7 years, 6 months ago 
On Thu, Oct 26, 2017 at 04:29:39PM +0200, Marcin Wojtas wrote:
> 2017-10-26 16:02 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> >> > Why is Capability.BaseClkFreq the wrong frequency to use?
> >>
> >> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
> >> An alternative would be change this generic type to UINT16 and update
> >> field properly during initialization - do you prefer that?
> >
> > No, I'm still dreaming we might be able to reintegrate this into the
> > MdeModulePkg driver in some glorious future.
> 
> Yes, that would be great. I imagine some SDMMC_HOST_PROTOCOL exposing
> callbacks to set UHS, custom clock handling, etc (like it's done in
> the Linux).

Yes, *waves hands*, something like that.

> > So what you are basically saying is that this controller is running at
> > a higher frequency than is permitted (or even describable) by the
> > specification? If so, _that_ needs to be in the commit message (and
> > really, a comment by the code as well).
> 
> Yes, this clock value is Xenon controller's quirk. I will mention it
> in commit log/comment, but please let know if you wish me to:
> a. extend BaseClkFreq to UINT16 and configure it properly during init
> b. leave as is with better description only
> I lean towards a., it's a very low-cost quirk to be applied.

I would actually lean towards b. That way it stands out and is less
likely to actually _confuse_ someone in the future.

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [platforms: PATCH 09/10] Marvell/Drivers: XenonDxe: Fix base clock frequency
Posted by Marcin Wojtas 7 years, 6 months ago 
2017-10-26 16:52 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Oct 26, 2017 at 04:29:39PM +0200, Marcin Wojtas wrote:
>> 2017-10-26 16:02 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> >> > Why is Capability.BaseClkFreq the wrong frequency to use?
>> >>
>> >> The Capability.BaseClkFreq is UINT8 and can hold up to 0xff -> 255MHz.
>> >> An alternative would be change this generic type to UINT16 and update
>> >> field properly during initialization - do you prefer that?
>> >
>> > No, I'm still dreaming we might be able to reintegrate this into the
>> > MdeModulePkg driver in some glorious future.
>>
>> Yes, that would be great. I imagine some SDMMC_HOST_PROTOCOL exposing
>> callbacks to set UHS, custom clock handling, etc (like it's done in
>> the Linux).
>
> Yes, *waves hands*, something like that.

Just 'a bit' spare time needed.

>
>> > So what you are basically saying is that this controller is running at
>> > a higher frequency than is permitted (or even describable) by the
>> > specification? If so, _that_ needs to be in the commit message (and
>> > really, a comment by the code as well).
>>
>> Yes, this clock value is Xenon controller's quirk. I will mention it
>> in commit log/comment, but please let know if you wish me to:
>> a. extend BaseClkFreq to UINT16 and configure it properly during init
>> b. leave as is with better description only
>> I lean towards a., it's a very low-cost quirk to be applied.
>
> I would actually lean towards b. That way it stands out and is less
> likely to actually _confuse_ someone in the future.
>

If we dream about possible merge with edk2 original code, I'd really
suggest not to spoil (well, I'm criticizing my own patch:)) the
SdMmcHcClockSupply and continue to rely on BaseClkFreq field there.
This way the code can be common for various hosts.

Let's take look at linux - there is SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN
flag, thanks to which the host controller driver can override the
capability register value during initialization. Most common method
here ends up in clk_get_rate () call and this quirk is used by overall
10 different controllers. Modifying BaseClkFreq type and assignment
during driver initialization (if needed) will be IMO better for
maintenance than creating a custom version of SdMmcHcClockSupply
routine.

Is there any chance I might have convinced you?:)

Thanks,
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.