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

[Rob Herring]

On Wed, May 10, 2017 at 12:49 PM, Jorge Ramirez
<jorge.ramirez-ortiz at linaro.org> wrote:
> On 05/10/2017 06:33 PM, 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.
>>
>> 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.
>
>
> agreed
>
>> 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.
>
>
> I also agree but please do notice that this was not quite what I was saying.
> what I am saying is that writing a stub driver to only provide a single UART
> clock rate and nothing else is also an overkill (this platform has no need
> for other clocks in u-boot)

Sorry, I find that hard to believe. There's no SD card, display, SPI,
I2C? Those all need clocks at some point.

> Incidentally the value that I need to retrieve is itself hard-coded in an
> array in the kernel source code and set up via clk_register_fixed_rate
> instead of using a fixed-clock node in the device tree.
> So there is not much value that I can see in providing such a stub driver
> really...

If you don't need clock properties in Linux, then you shouldn't need
them in u-boot either. But that's not what I see in the dts under
review:

+               uart0: serial at 8b00000 {
+                       compatible = "arm,pl011", "arm,primecell";
+                       reg = <0x8b00000 0x1000>;
+                       interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
+                       clocks = <&sysctrl HISTB_UART0_CLK>;
+                       clock-names = "apb_pclk";
+                       status = "disabled";
+               };
+
+               uart2: serial at 8b02000 {
+                       compatible = "arm,pl011", "arm,primecell";
+                       reg = <0x8b02000 0x1000>;
+                       interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
+                       clocks = <&crg HISTB_UART2_CLK>;
+                       clock-names = "apb_pclk";
+                       status = "disabled";
+               };
+

Which BTW is also wrong as a single clock is deprecated. There should
be 2 clocks.

Rob