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

[Simon Glass]

Hi,

On 10 May 2017 at 13:09, Rob Herring <robh at kernel.org> wrote:
> On Wed, May 10, 2017 at 12:45 PM, Tom Rini <trini at konsulko.com> wrote:
>> On Wed, May 10, 2017 at 11:33:12AM -0500, Rob Herring wrote:
>>> On Wed, May 10, 2017 at 9:49 AM, Tom Rini <trini at konsulko.com> wrote:
>>> > On Wed, May 10, 2017 at 11:33:35AM +0200, Jorge Ramirez wrote:
>>> >> On 05/10/2017 04:30 AM, Tom Rini wrote:
>>> >> >>hey Tom, I am not sure how to move this forward really so let me
>>> >> >>clarify where I think we stand:
>>> >> >>
>>> >> >>1. The linux kernel does not need the clock property in the uart
>>> >> >>nodes (only u-boot does: serial_pl01x.c needs fixing).
>>> >> >>2. ehci is not present in the linux kernel poplar dts yet but it
>>> >> >>will be eventually.
>>> >> >>
>>> >> >>with this in mind, what is blocking the acceptance of the patchset?
>>> >> >>
>>> >> >>I can do v5 using the linux kernel dts as is and creating a
>>> >> >>hi3798cv200-u-boot.dtsi that simply adds the nodes above (this time
>>> >> >>no #include required:)  )
>>> >> >>
>>> >> >>Then when ehci is added to the kernel, the ehci node can be removed
>>> >> >>from u-boot.dtsi
>>> >> >>And when uboot updates its pl01x.c serial driver,  the uart0 node
>>> >> >>can be removed and the u-boot.dtsi filed completely deleted.
>>> >> >Can you take a stab at updating the pl01x driver?  Thanks!
>>> >>
>>> >> 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
2. Use the existing clock-frequency property

Regards,
SImon