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

[Jorge Ramirez]

On 05/10/2017 08:21 PM, Rob Herring wrote:
> 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.

No, the BOOT ROM takes care of them in this platform (pinux, clocks, DDR 
initialization, etc).
in uboot we are using the console (for which we only need the 
clock-frequency), eMMC and USB ehci (for storage and networking) and 
there are no clocks that need to be set in BL33 (uboot runs as BL33)


>
>> 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:

sorry I dont understand your point. is this a question about uboot?

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

yes I noticed that as well while reading the bindings
>
> Rob