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

[Marcin Wojtas <mw@semihalf.com>]

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