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

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

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