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

[Marcin Wojtas <mw@semihalf.com>]

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