[U-Boot] [PATCHv4 3/3] ARM64: poplar: hi3798cv200: u-boot support for Poplar 96Boards

[Simon Glass]

Hi,

On 10 May 2017 at 15:19, Jorge Ramirez <jorge.ramirez-ortiz at linaro.org> wrote:
>
> On 05/10/2017 09:42 PM, Simon Glass wrote:
>
> updating pl01x is not a big deal I dont think; however this will
> mean requiring a platform specific clock driver to just use the
> pl01x - which will take me some time to get into uboot for my
> platform (and the same might happen to other users).
>
> Ah right.  So the flip side here, is one not allowed to have the clock
> property anymore?  Yes, it may not be used in the kernel, but has
> someone argued that it's not part of the hardware description?
>
> First I've ever seen a "clock" property. We have "clocks" from the
> clock binding which is a phandle plus #clock-cells number of args. We
> also have "clock-frequency" which is just the frequency value and has
> been around forever. Why u-boot went off and did something different i
> don't know. Sigh. What I can say is a 3rd way is not going to be
> accepted.
>
> Aw crap, I'm in the wrong.  I was thinking this was "clock-frequency"
> and not that we had invented our own property here.
>
> Generally, the clock binding replaces clock-frequency, but there are
> some cases where clock binding would be overkill or too complicated
> for early boot and using clock-frequency would be okay. But I don't
> think "I haven't written my platform clock controller driver yet" is a
> reason to use clock-frequency as generally most platforms are going to
> have to have one. Providing a stub driver that just returns a fixed
> frequency shouldn't be too hard IMO.
>
> So, trying to dig out of the hole we made here.  The generic serial
> binding (bindings/serial/serial.txt) documents clock-frequency.  The
> pl011 binding (and primecell which it also references) does not.  Would
> adding clock-frequency to a pl011 node be valid or invalid?
>
> Valid in general. It's a common property in the DT spec. Though, it
> should be listed in the pl011 binding doc as used just like we list
> reg or interrupts.
>
>  If valid,
> would it also be acceptable to include in dts files that also fill in
> clocks/clock-names from that binding?
>
> Generally we treat that as not valid as they are mutually exclusive.
>
> There's 2 better options. You can define fixed clocks with
> "fixed-clock" compatible and you already have infrastructure in u-boot
> to use that. However, the upstream dts file already defines clocks, so
> that doesn't really work here. The 2nd option is have a table of clock
> ids and frequencies and register that list of clocks based on matching
> the clock controller. You'd need a little bit of infrastructure to
> support that, but otherwise a platform would just need to define a
> table.
>
> It sounds like you are saying the first option isn't an option. The
> second option adds another layer of pain - we are trying to avoid
> having platform data.
>
> I'd prefer (in this order):
>
> 1. A clock driver
>
>
> please could you explain the rationale for this request on this particular platform?
>
> And I mean for a platform where a clock driver will:
>
> 1. NOT access any hardware
> 2. *only* provide single hard-coded value (a compiled in frequency) for the pl01x driver.
>
> Consider also (another option) that the pl01x driver can be compiled without OF support and that said frequency can be provided by CONFIG.
>
> It is just that before adding layers of stub code I would like to understand the technical need when there is only one consumer (I don't want to pollute U-Boot's code base with code that is not providing value).
>
> What do we want to achieve by writing a SoC clock driver that hard-codes the frequency rate for the console?
> what is the benefit of having such a driver and why is this not an overkill on this platform?

For just one device I can accept that it is overkill. Once you have
another device, or (e.g.) the ability to change clocks in U-Boot at
run time, a clock driver makes sense.

>
>
> 2. Use the existing clock-frequency property
>
>
> yes, this I could understand.
> And it doesn't even need to add a single line to the linux kernel dts files which would be imported from the linux kernel tree and keep unmodified.

Then it sounds like this approach works for you?

Regards,
Simon