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

Message ID 1494261403-12157-4-git-send-email-jorge.ramirez-ortiz@linaro.org
State Superseded
Headers show
Series
  • [PATCHv4,1/3] ARM64: dts: hi3798cv200-poplar: add device tree bindings
Related show

Commit Message

Jorge Ramirez May 8, 2017, 4:36 p.m.
This port adds support for:
        1) Serial
        2) eMMC
        3) USB

It has been tested with ARM TRUSTED FIRMWARE running u-boot as the
BL33 executable [see board's README]

eMMC has been tested for reading and booting the loader[1] and linux
kernels as well as saving the u-boot environment.

USB has been tested with ASIX networking adapter and SanDisk 7.4GB
drive.

PSCI has been tested via the reset call.

The firwmare upgrade process has been tested via TFTP and USB FAT
filesystem containing the fastboot.bin image in one of the partitions.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 arch/arm/Kconfig                                   |  12 ++
 arch/arm/dts/hi3798cv200.dtsi                      |   3 +
 arch/arm/dts/poplar-uboot.dtsi                     |  24 +++
 arch/arm/include/asm/arch-hi3798cv200/dwmmc.h      |  13 ++
 .../arm/include/asm/arch-hi3798cv200/hi3798cv200.h |  50 +++++
 board/hisilicon/poplar/Kconfig                     |  15 ++
 board/hisilicon/poplar/MAINTAINERS                 |   6 +
 board/hisilicon/poplar/Makefile                    |   7 +
 board/hisilicon/poplar/README                      | 232 +++++++++++++++++++++
 board/hisilicon/poplar/poplar.c                    | 155 ++++++++++++++
 configs/poplar_defconfig                           |  26 +++
 include/configs/poplar.h                           |  86 ++++++++
 12 files changed, 629 insertions(+)
 create mode 100644 arch/arm/dts/poplar-uboot.dtsi
 create mode 100644 arch/arm/include/asm/arch-hi3798cv200/dwmmc.h
 create mode 100644 arch/arm/include/asm/arch-hi3798cv200/hi3798cv200.h
 create mode 100644 board/hisilicon/poplar/Kconfig
 create mode 100644 board/hisilicon/poplar/MAINTAINERS
 create mode 100644 board/hisilicon/poplar/Makefile
 create mode 100644 board/hisilicon/poplar/README
 create mode 100644 board/hisilicon/poplar/poplar.c
 create mode 100644 configs/poplar_defconfig
 create mode 100644 include/configs/poplar.h

Comments

Tom Rini May 8, 2017, 5:29 p.m. | #1
On Mon, May 08, 2017 at 06:36:43PM +0200, Jorge Ramirez-Ortiz wrote:

> This port adds support for:

>         1) Serial

>         2) eMMC

>         3) USB

[snip]
>  arch/arm/dts/hi3798cv200.dtsi                      |   3 +

>  arch/arm/dts/poplar-uboot.dtsi                     |  24 +++

[snip]
> diff --git a/arch/arm/dts/hi3798cv200.dtsi b/arch/arm/dts/hi3798cv200.dtsi

> index 75865f8..caa17de 100644

> --- a/arch/arm/dts/hi3798cv200.dtsi

> +++ b/arch/arm/dts/hi3798cv200.dtsi

> @@ -409,3 +409,6 @@

>  		};

>  	};

>  };

> +

> +#include "poplar-uboot.dtsi"


NAK, that's not the mechanism, we have one that will automatically
include the right file.  IF it's needed.

> diff --git a/arch/arm/dts/poplar-uboot.dtsi b/arch/arm/dts/poplar-uboot.dtsi

> new file mode 100644

> index 0000000..8a9668a

> --- /dev/null

> +++ b/arch/arm/dts/poplar-uboot.dtsi

> @@ -0,0 +1,24 @@

> +/*

> + * U-Boot addition to:

> + *  1) initialize the console clock rate.

> + *  2) provide support for the generic-ehci USB driver currently not available

> + *     in the linux kernel (8/May/2017).

> + *

> + * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

> + *

> + * SPDX-License-Identifier:     GPL-2.0+

> + */

> +

> +&soc {

> +	usb2: ehci@9890000 {

> +		compatible = "generic-ehci";

> +		reg = <0x9890000 0x100>;

> +		status = "okay";

> +	};

> +};

> +

> +&uart0 {

> +	clock = <75000000>;

> +	status = "okay";

> +};


These are NOT U-Boot specific properties, they should be in the generic
board dts file.  Lets wait for Alex to chime in on the status of getting
the dts file / etc merged into Linux before doing v5.  Thanks!

-- 
Tom
Jorge Ramirez May 8, 2017, 6:36 p.m. | #2
On 05/08/2017 07:29 PM, Tom Rini wrote:
> On Mon, May 08, 2017 at 06:36:43PM +0200, Jorge Ramirez-Ortiz wrote:
>
>> This port adds support for:
>>          1) Serial
>>          2) eMMC
>>          3) USB
> [snip]
>>   arch/arm/dts/hi3798cv200.dtsi                      |   3 +
>>   arch/arm/dts/poplar-uboot.dtsi                     |  24 +++
> [snip]
>> diff --git a/arch/arm/dts/hi3798cv200.dtsi b/arch/arm/dts/hi3798cv200.dtsi
>> index 75865f8..caa17de 100644
>> --- a/arch/arm/dts/hi3798cv200.dtsi
>> +++ b/arch/arm/dts/hi3798cv200.dtsi
>> @@ -409,3 +409,6 @@
>>   		};
>>   	};
>>   };
>> +
>> +#include "poplar-uboot.dtsi"
> NAK, that's not the mechanism, we have one that will automatically
> include the right file.  IF it's needed.

yeah I thought so...still what about what is being done for the 
dragonboard? (that is what misled me really)
seems to me that board needs fixing as well...


>
>> diff --git a/arch/arm/dts/poplar-uboot.dtsi b/arch/arm/dts/poplar-uboot.dtsi
>> new file mode 100644
>> index 0000000..8a9668a
>> --- /dev/null
>> +++ b/arch/arm/dts/poplar-uboot.dtsi
>> @@ -0,0 +1,24 @@
>> +/*
>> + * U-Boot addition to:
>> + *  1) initialize the console clock rate.
>> + *  2) provide support for the generic-ehci USB driver currently not available
>> + *     in the linux kernel (8/May/2017).
>> + *
>> + * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + */
>> +
>> +&soc {
>> +	usb2: ehci@9890000 {
>> +		compatible = "generic-ehci";
>> +		reg = <0x9890000 0x100>;
>> +		status = "okay";
>> +	};
>> +};
>> +
>> +&uart0 {
>> +	clock = <75000000>;
>> +	status = "okay";
>> +};
> These are NOT U-Boot specific properties, they should be in the generic
> board dts file.


why did you ask me to remove them from the generic board dts file?


> Lets wait for Alex to chime in on the status of getting
> the dts file / etc merged into Linux before doing v5.  Thanks!
>
sure, no pb. thanks for the quick responses :)
Tom Rini May 8, 2017, 9:37 p.m. | #3
On Mon, May 08, 2017 at 08:36:28PM +0200, Jorge Ramirez wrote:
> On 05/08/2017 07:29 PM, Tom Rini wrote:

> >On Mon, May 08, 2017 at 06:36:43PM +0200, Jorge Ramirez-Ortiz wrote:

> >

> >>This port adds support for:

> >>         1) Serial

> >>         2) eMMC

> >>         3) USB

> >[snip]

> >>  arch/arm/dts/hi3798cv200.dtsi                      |   3 +

> >>  arch/arm/dts/poplar-uboot.dtsi                     |  24 +++

> >[snip]

> >>diff --git a/arch/arm/dts/hi3798cv200.dtsi b/arch/arm/dts/hi3798cv200.dtsi

> >>index 75865f8..caa17de 100644

> >>--- a/arch/arm/dts/hi3798cv200.dtsi

> >>+++ b/arch/arm/dts/hi3798cv200.dtsi

> >>@@ -409,3 +409,6 @@

> >>  		};

> >>  	};

> >>  };

> >>+

> >>+#include "poplar-uboot.dtsi"

> >NAK, that's not the mechanism, we have one that will automatically

> >include the right file.  IF it's needed.

> 

> yeah I thought so...still what about what is being done for the

> dragonboard? (that is what misled me really)

> seems to me that board needs fixing as well...


Yes, patches welcome ;)  Seriously tho, yes, dragonboard is a bad
example here, along with some exynos and broadcom parts, and should be
corrected.  I'd appreciate a patch there.

-- 
Tom
Jorge Ramirez May 9, 2017, 3:27 p.m. | #4
On 05/08/2017 07:29 PM, Tom Rini wrote:
> On Mon, May 08, 2017 at 06:36:43PM +0200, Jorge Ramirez-Ortiz wrote:
>
>> This port adds support for:
>>          1) Serial
>>          2) eMMC
>>          3) USB
> [snip]
>>   arch/arm/dts/hi3798cv200.dtsi                      |   3 +
>>   arch/arm/dts/poplar-uboot.dtsi                     |  24 +++
> [snip]
>> diff --git a/arch/arm/dts/hi3798cv200.dtsi b/arch/arm/dts/hi3798cv200.dtsi
>> index 75865f8..caa17de 100644
>> --- a/arch/arm/dts/hi3798cv200.dtsi
>> +++ b/arch/arm/dts/hi3798cv200.dtsi
>> @@ -409,3 +409,6 @@
>>   		};
>>   	};
>>   };
>> +
>> +#include "poplar-uboot.dtsi"
> NAK, that's not the mechanism, we have one that will automatically
> include the right file.  IF it's needed.
>
>> diff --git a/arch/arm/dts/poplar-uboot.dtsi b/arch/arm/dts/poplar-uboot.dtsi
>> new file mode 100644
>> index 0000000..8a9668a
>> --- /dev/null
>> +++ b/arch/arm/dts/poplar-uboot.dtsi
>> @@ -0,0 +1,24 @@
>> +/*
>> + * U-Boot addition to:
>> + *  1) initialize the console clock rate.
>> + *  2) provide support for the generic-ehci USB driver currently not available
>> + *     in the linux kernel (8/May/2017).
>> + *
>> + * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> + *
>> + * SPDX-License-Identifier:     GPL-2.0+
>> + */
>> +
>> +&soc {
>> +	usb2: ehci@9890000 {
>> +		compatible = "generic-ehci";
>> +		reg = <0x9890000 0x100>;
>> +		status = "okay";
>> +	};
>> +};
>> +
>> +&uart0 {
>> +	clock = <75000000>;
>> +	status = "okay";
>> +};
> These are NOT U-Boot specific properties, they should be in the generic
> board dts file.  Lets wait for Alex to chime in on the status of getting
> the dts file / etc merged into Linux before doing v5.  Thanks!
>

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.

would this be acceptable? if not, what would you see us doing?
Tom Rini May 10, 2017, 2:30 a.m. | #5
On Tue, May 09, 2017 at 05:27:12PM +0200, Jorge Ramirez wrote:
> On 05/08/2017 07:29 PM, Tom Rini wrote:

> >On Mon, May 08, 2017 at 06:36:43PM +0200, Jorge Ramirez-Ortiz wrote:

> >

> >>This port adds support for:

> >>         1) Serial

> >>         2) eMMC

> >>         3) USB

> >[snip]

> >>  arch/arm/dts/hi3798cv200.dtsi                      |   3 +

> >>  arch/arm/dts/poplar-uboot.dtsi                     |  24 +++

> >[snip]

> >>diff --git a/arch/arm/dts/hi3798cv200.dtsi b/arch/arm/dts/hi3798cv200.dtsi

> >>index 75865f8..caa17de 100644

> >>--- a/arch/arm/dts/hi3798cv200.dtsi

> >>+++ b/arch/arm/dts/hi3798cv200.dtsi

> >>@@ -409,3 +409,6 @@

> >>  		};

> >>  	};

> >>  };

> >>+

> >>+#include "poplar-uboot.dtsi"

> >NAK, that's not the mechanism, we have one that will automatically

> >include the right file.  IF it's needed.

> >

> >>diff --git a/arch/arm/dts/poplar-uboot.dtsi b/arch/arm/dts/poplar-uboot.dtsi

> >>new file mode 100644

> >>index 0000000..8a9668a

> >>--- /dev/null

> >>+++ b/arch/arm/dts/poplar-uboot.dtsi

> >>@@ -0,0 +1,24 @@

> >>+/*

> >>+ * U-Boot addition to:

> >>+ *  1) initialize the console clock rate.

> >>+ *  2) provide support for the generic-ehci USB driver currently not available

> >>+ *     in the linux kernel (8/May/2017).

> >>+ *

> >>+ * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

> >>+ *

> >>+ * SPDX-License-Identifier:     GPL-2.0+

> >>+ */

> >>+

> >>+&soc {

> >>+	usb2: ehci@9890000 {

> >>+		compatible = "generic-ehci";

> >>+		reg = <0x9890000 0x100>;

> >>+		status = "okay";

> >>+	};

> >>+};

> >>+

> >>+&uart0 {

> >>+	clock = <75000000>;

> >>+	status = "okay";

> >>+};

> >These are NOT U-Boot specific properties, they should be in the generic

> >board dts file.  Lets wait for Alex to chime in on the status of getting

> >the dts file / etc merged into Linux before doing v5.  Thanks!

> >

> 

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

-- 
Tom
Jorge Ramirez May 10, 2017, 9:33 a.m. | #6
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).

- if the issue for accepting the Poplar patchset is with the dts 
modification I can revert to just not using OF for pl01x like other 
platforms do.
- if the issue is that we wish to enforce each new platform to commit a 
clock driver then I just need to plan for it (to be honest I didn't 
envision this to be the case when all I need is to enable a uart)

maintaining full compatibility with the kernel's dts comes at a price so 
just making sure that this is the direction we want to take (not just a 
one off where I am the lucky one :) )
Tom Rini May 10, 2017, 2:49 p.m. | #7
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?

-- 
Tom
Rob Herring May 10, 2017, 4:33 p.m. | #8
On Wed, May 10, 2017 at 9:49 AM, Tom Rini <trini@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. 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.

Rob
Tom Rini May 10, 2017, 5:45 p.m. | #9
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@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?  If valid,
would it also be acceptable to include in dts files that also fill in
clocks/clock-names from that binding?  Thanks!

-- 
Tom
Jorge Ramirez May 10, 2017, 5:49 p.m. | #10
On 05/10/2017 06:33 PM, Rob Herring wrote:
> On Wed, May 10, 2017 at 9:49 AM, Tom Rini <trini@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)

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



>
> Rob
Simon Glass May 10, 2017, 6:09 p.m. | #11
Hi,

On 10 May 2017 at 11:45, Tom Rini <trini@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@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?  If valid,
> would it also be acceptable to include in dts files that also fill in
> clocks/clock-names from that binding?  Thanks!

clock-frequency should be OK and supported for boards which don't yet
have a clock driver. I don't think we need to explicitly update the
pl011 binding though.

Regards,
Simon
Rob Herring May 10, 2017, 6:21 p.m. | #12
On Wed, May 10, 2017 at 12:49 PM, Jorge Ramirez
<jorge.ramirez-ortiz@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@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@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@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
Jorge Ramirez May 10, 2017, 6:37 p.m. | #13
On 05/10/2017 08:21 PM, Rob Herring wrote:
> On Wed, May 10, 2017 at 12:49 PM, Jorge Ramirez
> <jorge.ramirez-ortiz@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@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@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@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
Rob Herring May 10, 2017, 7:09 p.m. | #14
On Wed, May 10, 2017 at 12:45 PM, Tom Rini <trini@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@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.

Rob
Simon Glass May 10, 2017, 7:42 p.m. | #15
Hi,

On 10 May 2017 at 13:09, Rob Herring <robh@kernel.org> wrote:
> On Wed, May 10, 2017 at 12:45 PM, Tom Rini <trini@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@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
Jorge Ramirez May 10, 2017, 9:19 p.m. | #16
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?


> 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.
Simon Glass May 10, 2017, 9:31 p.m. | #17
Hi,

On 10 May 2017 at 15:19, Jorge Ramirez <jorge.ramirez-ortiz@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
Jorge Ramirez May 11, 2017, 11:45 a.m. | #18
On 05/10/2017 11:31 PM, Simon Glass wrote:
>> 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?

ACK: this works for this platform (eMMC, USB host (net/storage) and UART).
if this were to change in the future extending with a clock driver as 
you said would make sense but I don't see this happening.

But to be clear, using "clock-frequency" would mean that I will have to 
modify serial_pl01x.c (replace "clock" with "clock-frequency") and also 
fix the device trees of all the current users of this driver - all of 
them modified their device trees to add u-boot's "clock" property.

do you concur with this?
Tom Rini May 11, 2017, 12:35 p.m. | #19
On Wed, May 10, 2017 at 01:42:22PM -0600, Simon Glass wrote:
> Hi,

> 

> On 10 May 2017 at 13:09, Rob Herring <robh@kernel.org> wrote:

> > On Wed, May 10, 2017 at 12:45 PM, Tom Rini <trini@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@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.


No, I think we're going for overkill here by not doing serial_pl01x.c as
platform data.  ns16550 does platform data for this already.  This
sounds like the lowest overhead way to get the clock populated and not
have some DT data that's not going to be accepted upstream.

-- 
Tom
Jorge Ramirez May 11, 2017, 2:34 p.m. | #20
On 05/11/2017 02:35 PM, Tom Rini wrote:
> On Wed, May 10, 2017 at 01:42:22PM -0600, Simon Glass wrote:
>> Hi,
>>
>> On 10 May 2017 at 13:09, Rob Herring <robh@kernel.org> wrote:
>>> On Wed, May 10, 2017 at 12:45 PM, Tom Rini <trini@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@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.
> No, I think we're going for overkill here by not doing serial_pl01x.c as
> platform data.  ns16550 does platform data for this already.  This
> sounds like the lowest overhead way to get the clock populated and not
> have some DT data that's not going to be accepted upstream.
>


ummmm I am a bit lost at this point, could we recap please?

let's see: I need to use the pl01x uart on an aarch64 platform and I 
dont need to enable any clocks for uboot in my SoC. Not now, unlikely ever.

Doing what other boards have done to this date is no longer acceptable 
(ie platform data for the pl01x or using uboots "clock" property 
embedded in the hacked device trees)

so, what is the recommended way of configuring this uart?

1. write a dummy clock driver to provide the number of choice for the 
board (in my case 75m)
2. using platform data for the pl01x
3. using u-boot's very own "clock" property in a separate -u-boot.dtsi
4. using a proper clock-frequency property and fixing pl01x and all 
those dts files that incorrectly have coded uboot properties...


TIA
Tom Rini May 11, 2017, 10:32 p.m. | #21
On Thu, May 11, 2017 at 04:34:18PM +0200, Jorge Ramirez wrote:
> On 05/11/2017 02:35 PM, Tom Rini wrote:

> >On Wed, May 10, 2017 at 01:42:22PM -0600, Simon Glass wrote:

> >>Hi,

> >>

> >>On 10 May 2017 at 13:09, Rob Herring <robh@kernel.org> wrote:

> >>>On Wed, May 10, 2017 at 12:45 PM, Tom Rini <trini@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@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.

> >No, I think we're going for overkill here by not doing serial_pl01x.c as

> >platform data.  ns16550 does platform data for this already.  This

> >sounds like the lowest overhead way to get the clock populated and not

> >have some DT data that's not going to be accepted upstream.

> >

> 

> 

> ummmm I am a bit lost at this point, could we recap please?


Sure.

> let's see: I need to use the pl01x uart on an aarch64 platform and I

> dont need to enable any clocks for uboot in my SoC. Not now,

> unlikely ever.

> 

> Doing what other boards have done to this date is no longer

> acceptable (ie platform data for the pl01x or using uboots "clock"

> property embedded in the hacked device trees)


The only thing we all agree on right now is that "clock" is wrong and
must be replaced.  I've decided we need to discuss bringing in platform
data for pl01x.  Once we resolve this, then you can re-spin the series
(and hopefully have the USB nodes be submitted to Linux too, since
they're the standard ones and, uh, should just enable USB on your board
in the kernel too..)  Thanks!

-- 
Tom
Jorge Ramirez May 12, 2017, 8:16 a.m. | #22
On 05/12/2017 12:32 AM, Tom Rini wrote:
>> ummmm I am a bit lost at this point, could we recap please?
> Sure.
>
>> let's see: I need to use the pl01x uart on an aarch64 platform and I
>> dont need to enable any clocks for uboot in my SoC. Not now,
>> unlikely ever.
>>
>> Doing what other boards have done to this date is no longer
>> acceptable (ie platform data for the pl01x or using uboots "clock"
>> property embedded in the hacked device trees)
> The only thing we all agree on right now is that "clock" is wrong and
> must be replaced.  I've decided we need to discuss bringing in platform
> data for pl01x.  Once we resolve this, then you can re-spin the series
> (and hopefully have the USB nodes be submitted to Linux too, since
> they're the standard ones and, uh, should just enable USB on your board
> in the kernel too..)  Thanks!

cool, that sounds great, thanks.

yeah the usb nodes should be ready pretty soon, I have seen them 
circulating already.

btw, what was it that triggered our discussion?  it is not like any of 
the dts files for armv8 boards are verbatim copies of what you find in 
the kernel.

Is there a new rule -explicit somewhere?- enforcing that this has to be 
the case then?
Tom Rini May 12, 2017, 12:35 p.m. | #23
On Fri, May 12, 2017 at 10:16:52AM +0200, Jorge Ramirez wrote:
> On 05/12/2017 12:32 AM, Tom Rini wrote:

> >>ummmm I am a bit lost at this point, could we recap please?

> >Sure.

> >

> >>let's see: I need to use the pl01x uart on an aarch64 platform and I

> >>dont need to enable any clocks for uboot in my SoC. Not now,

> >>unlikely ever.

> >>

> >>Doing what other boards have done to this date is no longer

> >>acceptable (ie platform data for the pl01x or using uboots "clock"

> >>property embedded in the hacked device trees)

> >The only thing we all agree on right now is that "clock" is wrong and

> >must be replaced.  I've decided we need to discuss bringing in platform

> >data for pl01x.  Once we resolve this, then you can re-spin the series

> >(and hopefully have the USB nodes be submitted to Linux too, since

> >they're the standard ones and, uh, should just enable USB on your board

> >in the kernel too..)  Thanks!

> 

> cool, that sounds great, thanks.

> 

> yeah the usb nodes should be ready pretty soon, I have seen them

> circulating already.

> 

> btw, what was it that triggered our discussion?  it is not like any

> of the dts files for armv8 boards are verbatim copies of what you

> find in the kernel.


They've gotten out of sync? Sigh..  I suppose this starts to push me
from the "keep them in the kernel" camp to "push them to a separate
authoritative repository" camp.

I do know some of the early boards were out of sync, but that shouldn't
be the case moving forward, and isn't the case for ARMv7 things.

-- 
Tom
Rob Herring May 15, 2017, 9:38 p.m. | #24
On Fri, May 12, 2017 at 7:35 AM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, May 12, 2017 at 10:16:52AM +0200, Jorge Ramirez wrote:
>> On 05/12/2017 12:32 AM, Tom Rini wrote:
>> >>ummmm I am a bit lost at this point, could we recap please?
>> >Sure.
>> >
>> >>let's see: I need to use the pl01x uart on an aarch64 platform and I
>> >>dont need to enable any clocks for uboot in my SoC. Not now,
>> >>unlikely ever.
>> >>
>> >>Doing what other boards have done to this date is no longer
>> >>acceptable (ie platform data for the pl01x or using uboots "clock"
>> >>property embedded in the hacked device trees)
>> >The only thing we all agree on right now is that "clock" is wrong and
>> >must be replaced.  I've decided we need to discuss bringing in platform
>> >data for pl01x.  Once we resolve this, then you can re-spin the series
>> >(and hopefully have the USB nodes be submitted to Linux too, since
>> >they're the standard ones and, uh, should just enable USB on your board
>> >in the kernel too..)  Thanks!
>>
>> cool, that sounds great, thanks.
>>
>> yeah the usb nodes should be ready pretty soon, I have seen them
>> circulating already.
>>
>> btw, what was it that triggered our discussion?  it is not like any
>> of the dts files for armv8 boards are verbatim copies of what you
>> find in the kernel.
>
> They've gotten out of sync? Sigh..  I suppose this starts to push me
> from the "keep them in the kernel" camp to "push them to a separate
> authoritative repository" camp.

What's wrong with the standalone DT tree[1] and importing that to
u-boot periodically?

I know folks would like a completely separate tree that's not "the
Linux DT tree", but I don't see that happening any time soon. Do we
have some Linuxisms in bindings, yes, but in general I think they are
more the exception than rule and were things that went in with little
review. These days I'm reviewing pretty much all bindings (not all dts
files though), so I think it's less of a problem. Logistically, we
could probably work out how to move bindings and dts files to a
standalone repository as I could apply bindings and most dts files go
thru arm-soc maintainers. My biggest concern with a separate
repository is review because we would quickly loose any review that
Linux subsystem maintainers do, and no one is beating down my door to
help be a DT maintainer.

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git
Tom Rini May 17, 2017, 1:33 p.m. | #25
On Mon, May 15, 2017 at 04:38:14PM -0500, Rob Herring wrote:
> On Fri, May 12, 2017 at 7:35 AM, Tom Rini <trini@konsulko.com> wrote:

> > On Fri, May 12, 2017 at 10:16:52AM +0200, Jorge Ramirez wrote:

> >> On 05/12/2017 12:32 AM, Tom Rini wrote:

> >> >>ummmm I am a bit lost at this point, could we recap please?

> >> >Sure.

> >> >

> >> >>let's see: I need to use the pl01x uart on an aarch64 platform and I

> >> >>dont need to enable any clocks for uboot in my SoC. Not now,

> >> >>unlikely ever.

> >> >>

> >> >>Doing what other boards have done to this date is no longer

> >> >>acceptable (ie platform data for the pl01x or using uboots "clock"

> >> >>property embedded in the hacked device trees)

> >> >The only thing we all agree on right now is that "clock" is wrong and

> >> >must be replaced.  I've decided we need to discuss bringing in platform

> >> >data for pl01x.  Once we resolve this, then you can re-spin the series

> >> >(and hopefully have the USB nodes be submitted to Linux too, since

> >> >they're the standard ones and, uh, should just enable USB on your board

> >> >in the kernel too..)  Thanks!

> >>

> >> cool, that sounds great, thanks.

> >>

> >> yeah the usb nodes should be ready pretty soon, I have seen them

> >> circulating already.

> >>

> >> btw, what was it that triggered our discussion?  it is not like any

> >> of the dts files for armv8 boards are verbatim copies of what you

> >> find in the kernel.

> >

> > They've gotten out of sync? Sigh..  I suppose this starts to push me

> > from the "keep them in the kernel" camp to "push them to a separate

> > authoritative repository" camp.

> 

> What's wrong with the standalone DT tree[1] and importing that to

> u-boot periodically?

> 

> I know folks would like a completely separate tree that's not "the

> Linux DT tree", but I don't see that happening any time soon. Do we

> have some Linuxisms in bindings, yes, but in general I think they are

> more the exception than rule and were things that went in with little

> review. These days I'm reviewing pretty much all bindings (not all dts

> files though), so I think it's less of a problem. Logistically, we

> could probably work out how to move bindings and dts files to a

> standalone repository as I could apply bindings and most dts files go

> thru arm-soc maintainers. My biggest concern with a separate

> repository is review because we would quickly loose any review that

> Linux subsystem maintainers do, and no one is beating down my door to

> help be a DT maintainer.


Thinking about this, the key word is authoritative.  Right now we say
(every time I spot a new dts patch) "is this in the kernel yet?" and the
answers break down to:
- Yes, see v4.x
- Yes, see linux-next $tag (or Yes, see maintainer-tree-$X)
- No, we're working on it.

To me, the first is great, the second is OK only in that we're probably
not relying on anything bleeding edge and likely to change between
linux-next $tag and when it finally goes upstream.  The third is where
we're at with this board.  And a problem is that even with the short
kernel release cycle, there is a lot of not wanting to wait until things
get into the next upstream release.

Making a big and possibly wrong assumption, for something like this
board, that doesn't introduce any new bindings, shouldn't this dts be
able to go in quickly, once it of course is otherwise correct?  And
U-Boot (and barebox and the kernel and anyone else that cares) doesn't
want the tree until it was correct either.  And once a tree is in this
upstream, it's just a matter of saying import dts files for $X, taken
from $hash of the device tree repository (or even just included as I
might make myself get in the habit of syncing the tree in post release,
as it'd be on our release cycle, but honestly, I could / should just do
that, even if it's a -rc).

> [1] git://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git


... also, so it rebases and isn't stable?  That makes life less fun for
tracing back when we synced $X last.


-- 
Tom
Rob Herring May 17, 2017, 2:38 p.m. | #26
On Wed, May 17, 2017 at 8:33 AM, Tom Rini <trini@konsulko.com> wrote:
> On Mon, May 15, 2017 at 04:38:14PM -0500, Rob Herring wrote:
>> On Fri, May 12, 2017 at 7:35 AM, Tom Rini <trini@konsulko.com> wrote:
>> > On Fri, May 12, 2017 at 10:16:52AM +0200, Jorge Ramirez wrote:
>> >> On 05/12/2017 12:32 AM, Tom Rini wrote:
>> >> >>ummmm I am a bit lost at this point, could we recap please?
>> >> >Sure.
>> >> >
>> >> >>let's see: I need to use the pl01x uart on an aarch64 platform and I
>> >> >>dont need to enable any clocks for uboot in my SoC. Not now,
>> >> >>unlikely ever.
>> >> >>
>> >> >>Doing what other boards have done to this date is no longer
>> >> >>acceptable (ie platform data for the pl01x or using uboots "clock"
>> >> >>property embedded in the hacked device trees)
>> >> >The only thing we all agree on right now is that "clock" is wrong and
>> >> >must be replaced.  I've decided we need to discuss bringing in platform
>> >> >data for pl01x.  Once we resolve this, then you can re-spin the series
>> >> >(and hopefully have the USB nodes be submitted to Linux too, since
>> >> >they're the standard ones and, uh, should just enable USB on your board
>> >> >in the kernel too..)  Thanks!
>> >>
>> >> cool, that sounds great, thanks.
>> >>
>> >> yeah the usb nodes should be ready pretty soon, I have seen them
>> >> circulating already.
>> >>
>> >> btw, what was it that triggered our discussion?  it is not like any
>> >> of the dts files for armv8 boards are verbatim copies of what you
>> >> find in the kernel.
>> >
>> > They've gotten out of sync? Sigh..  I suppose this starts to push me
>> > from the "keep them in the kernel" camp to "push them to a separate
>> > authoritative repository" camp.
>>
>> What's wrong with the standalone DT tree[1] and importing that to
>> u-boot periodically?
>>
>> I know folks would like a completely separate tree that's not "the
>> Linux DT tree", but I don't see that happening any time soon. Do we
>> have some Linuxisms in bindings, yes, but in general I think they are
>> more the exception than rule and were things that went in with little
>> review. These days I'm reviewing pretty much all bindings (not all dts
>> files though), so I think it's less of a problem. Logistically, we
>> could probably work out how to move bindings and dts files to a
>> standalone repository as I could apply bindings and most dts files go
>> thru arm-soc maintainers. My biggest concern with a separate
>> repository is review because we would quickly loose any review that
>> Linux subsystem maintainers do, and no one is beating down my door to
>> help be a DT maintainer.
>
> Thinking about this, the key word is authoritative.  Right now we say
> (every time I spot a new dts patch) "is this in the kernel yet?" and the
> answers break down to:
> - Yes, see v4.x
> - Yes, see linux-next $tag (or Yes, see maintainer-tree-$X)
> - No, we're working on it.
>
> To me, the first is great, the second is OK only in that we're probably
> not relying on anything bleeding edge and likely to change between
> linux-next $tag and when it finally goes upstream.  The third is where
> we're at with this board.  And a problem is that even with the short
> kernel release cycle, there is a lot of not wanting to wait until things
> get into the next upstream release.

Maybe it was the 3rd case at the start of this, but it is now in v4.12-rc1.

Generally, commits in -next are not rebased and should match what ends
up in mainline. But that's not guaranteed and syncing with -next would
probably not be the best policy.

> Making a big and possibly wrong assumption, for something like this
> board, that doesn't introduce any new bindings, shouldn't this dts be
> able to go in quickly, once it of course is otherwise correct?

New SoC too, so probably a bit more than just a new assembly of
existing bindings. Worst case is someone submits this just before the
merge window, it's pulled in just after N-rc1, and doesn't get tagged
until O-rc1. In this case, the dts was committed on Apr 6 and rc1 was
tagged May 13. We could look at doing a filtered tree based off of
-next perhaps. Perhaps we should wait to see if the latency is really
a problem.

>  And
> U-Boot (and barebox and the kernel and anyone else that cares) doesn't
> want the tree until it was correct either.

Exactly.

>  And once a tree is in this
> upstream, it's just a matter of saying import dts files for $X, taken
> from $hash of the device tree repository (or even just included as I
> might make myself get in the habit of syncing the tree in post release,
> as it'd be on our release cycle, but honestly, I could / should just do
> that, even if it's a -rc).

Usually an -rcX is safe to take, but sometimes bindings do get changed
during -rc cycle.

>
>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git
>
> ... also, so it rebases and isn't stable?  That makes life less fun for
> tracing back when we synced $X last.

It is generated with git-filter-branch. So it may be regenerated if
the generation scripts change. As long as you are tracking kernel tags
as to what you've imported, then it should not matter. I'm not sure if
we've rebased recently. It was named that somewhat as a CYA. Ian can
better comment on this.

Rob
Tom Rini May 17, 2017, 10:06 p.m. | #27
On Thu, May 11, 2017 at 04:34:18PM +0200, Jorge Ramirez wrote:
> On 05/11/2017 02:35 PM, Tom Rini wrote:

> >On Wed, May 10, 2017 at 01:42:22PM -0600, Simon Glass wrote:

> >>Hi,

> >>

> >>On 10 May 2017 at 13:09, Rob Herring <robh@kernel.org> wrote:

> >>>On Wed, May 10, 2017 at 12:45 PM, Tom Rini <trini@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@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.

> >No, I think we're going for overkill here by not doing serial_pl01x.c as

> >platform data.  ns16550 does platform data for this already.  This

> >sounds like the lowest overhead way to get the clock populated and not

> >have some DT data that's not going to be accepted upstream.

> >

> 

> 

> ummmm I am a bit lost at this point, could we recap please?


Lets update the recap:
- Please re-submit the dts file, now with whatever form is in v4.12-rc1,
  saying as such in the commit (if it's just the commit message that
  changes, that's fine and great).
- Please update serial_pl01x.c to be able to get the clock via platform
  data, update and test your board to confirm it works.
- It'd be awesome if you do, but it won't block your series if you
  don't, update the rest of the platforms that had been using the
  "clock" platform to instead use the platform data method.

Thanks!

-- 
Tom
Tom Rini May 17, 2017, 10:13 p.m. | #28
On Wed, May 17, 2017 at 09:38:06AM -0500, Rob Herring wrote:
> On Wed, May 17, 2017 at 8:33 AM, Tom Rini <trini@konsulko.com> wrote:

> > On Mon, May 15, 2017 at 04:38:14PM -0500, Rob Herring wrote:

> >> On Fri, May 12, 2017 at 7:35 AM, Tom Rini <trini@konsulko.com> wrote:

> >> > On Fri, May 12, 2017 at 10:16:52AM +0200, Jorge Ramirez wrote:

> >> >> On 05/12/2017 12:32 AM, Tom Rini wrote:

> >> >> >>ummmm I am a bit lost at this point, could we recap please?

> >> >> >Sure.

> >> >> >

> >> >> >>let's see: I need to use the pl01x uart on an aarch64 platform and I

> >> >> >>dont need to enable any clocks for uboot in my SoC. Not now,

> >> >> >>unlikely ever.

> >> >> >>

> >> >> >>Doing what other boards have done to this date is no longer

> >> >> >>acceptable (ie platform data for the pl01x or using uboots "clock"

> >> >> >>property embedded in the hacked device trees)

> >> >> >The only thing we all agree on right now is that "clock" is wrong and

> >> >> >must be replaced.  I've decided we need to discuss bringing in platform

> >> >> >data for pl01x.  Once we resolve this, then you can re-spin the series

> >> >> >(and hopefully have the USB nodes be submitted to Linux too, since

> >> >> >they're the standard ones and, uh, should just enable USB on your board

> >> >> >in the kernel too..)  Thanks!

> >> >>

> >> >> cool, that sounds great, thanks.

> >> >>

> >> >> yeah the usb nodes should be ready pretty soon, I have seen them

> >> >> circulating already.

> >> >>

> >> >> btw, what was it that triggered our discussion?  it is not like any

> >> >> of the dts files for armv8 boards are verbatim copies of what you

> >> >> find in the kernel.

> >> >

> >> > They've gotten out of sync? Sigh..  I suppose this starts to push me

> >> > from the "keep them in the kernel" camp to "push them to a separate

> >> > authoritative repository" camp.

> >>

> >> What's wrong with the standalone DT tree[1] and importing that to

> >> u-boot periodically?

> >>

> >> I know folks would like a completely separate tree that's not "the

> >> Linux DT tree", but I don't see that happening any time soon. Do we

> >> have some Linuxisms in bindings, yes, but in general I think they are

> >> more the exception than rule and were things that went in with little

> >> review. These days I'm reviewing pretty much all bindings (not all dts

> >> files though), so I think it's less of a problem. Logistically, we

> >> could probably work out how to move bindings and dts files to a

> >> standalone repository as I could apply bindings and most dts files go

> >> thru arm-soc maintainers. My biggest concern with a separate

> >> repository is review because we would quickly loose any review that

> >> Linux subsystem maintainers do, and no one is beating down my door to

> >> help be a DT maintainer.

> >

> > Thinking about this, the key word is authoritative.  Right now we say

> > (every time I spot a new dts patch) "is this in the kernel yet?" and the

> > answers break down to:

> > - Yes, see v4.x

> > - Yes, see linux-next $tag (or Yes, see maintainer-tree-$X)

> > - No, we're working on it.

> >

> > To me, the first is great, the second is OK only in that we're probably

> > not relying on anything bleeding edge and likely to change between

> > linux-next $tag and when it finally goes upstream.  The third is where

> > we're at with this board.  And a problem is that even with the short

> > kernel release cycle, there is a lot of not wanting to wait until things

> > get into the next upstream release.

> 

> Maybe it was the 3rd case at the start of this, but it is now in v4.12-rc1.

> 

> Generally, commits in -next are not rebased and should match what ends

> up in mainline. But that's not guaranteed and syncing with -next would

> probably not be the best policy.


Right.  I don't like to take the "it's in -next", but the judgement call
is that it's often going to be fine anyhow.

> > Making a big and possibly wrong assumption, for something like this

> > board, that doesn't introduce any new bindings, shouldn't this dts be

> > able to go in quickly, once it of course is otherwise correct?

> 

> New SoC too, so probably a bit more than just a new assembly of

> existing bindings. Worst case is someone submits this just before the

> merge window, it's pulled in just after N-rc1, and doesn't get tagged

> until O-rc1. In this case, the dts was committed on Apr 6 and rc1 was

> tagged May 13. We could look at doing a filtered tree based off of

> -next perhaps. Perhaps we should wait to see if the latency is really

> a problem.

> 

> >  And

> > U-Boot (and barebox and the kernel and anyone else that cares) doesn't

> > want the tree until it was correct either.

> 

> Exactly.

> 

> >  And once a tree is in this

> > upstream, it's just a matter of saying import dts files for $X, taken

> > from $hash of the device tree repository (or even just included as I

> > might make myself get in the habit of syncing the tree in post release,

> > as it'd be on our release cycle, but honestly, I could / should just do

> > that, even if it's a -rc).

> 

> Usually an -rcX is safe to take, but sometimes bindings do get changed

> during -rc cycle.

> 

> >

> >> [1] git://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git

> >

> > ... also, so it rebases and isn't stable?  That makes life less fun for

> > tracing back when we synced $X last.

> 

> It is generated with git-filter-branch. So it may be regenerated if

> the generation scripts change. As long as you are tracking kernel tags

> as to what you've imported, then it should not matter. I'm not sure if

> we've rebased recently. It was named that somewhat as a CYA. Ian can

> better comment on this.


Well, I suppose the thing here now is that I'm the squeaky wheel, and
other projects seem to be fine.  I'll give things a harder try on my end
and see where we get from there, wrt problems.  Thanks!

-- 
Tom
Andreas Färber May 25, 2017, 4:08 p.m. | #29
Am 08.05.2017 um 18:36 schrieb Jorge Ramirez-Ortiz:
> This port adds support for:
>         1) Serial
>         2) eMMC
>         3) USB
> 
> It has been tested with ARM TRUSTED FIRMWARE running u-boot as the
> BL33 executable [see board's README]
> 
> eMMC has been tested for reading and booting the loader[1] and linux
> kernels as well as saving the u-boot environment.
> 
> USB has been tested with ASIX networking adapter and SanDisk 7.4GB
> drive.
> 
> PSCI has been tested via the reset call.
> 
> The firwmare upgrade process has been tested via TFTP and USB FAT
> filesystem containing the fastboot.bin image in one of the partitions.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  arch/arm/Kconfig                                   |  12 ++
>  arch/arm/dts/hi3798cv200.dtsi                      |   3 +
>  arch/arm/dts/poplar-uboot.dtsi                     |  24 +++
>  arch/arm/include/asm/arch-hi3798cv200/dwmmc.h      |  13 ++
>  .../arm/include/asm/arch-hi3798cv200/hi3798cv200.h |  50 +++++
>  board/hisilicon/poplar/Kconfig                     |  15 ++
>  board/hisilicon/poplar/MAINTAINERS                 |   6 +
>  board/hisilicon/poplar/Makefile                    |   7 +
>  board/hisilicon/poplar/README                      | 232 +++++++++++++++++++++
>  board/hisilicon/poplar/poplar.c                    | 155 ++++++++++++++
>  configs/poplar_defconfig                           |  26 +++
>  include/configs/poplar.h                           |  86 ++++++++
>  12 files changed, 629 insertions(+)
>  create mode 100644 arch/arm/dts/poplar-uboot.dtsi
>  create mode 100644 arch/arm/include/asm/arch-hi3798cv200/dwmmc.h
>  create mode 100644 arch/arm/include/asm/arch-hi3798cv200/hi3798cv200.h
>  create mode 100644 board/hisilicon/poplar/Kconfig
>  create mode 100644 board/hisilicon/poplar/MAINTAINERS
>  create mode 100644 board/hisilicon/poplar/Makefile
>  create mode 100644 board/hisilicon/poplar/README
>  create mode 100644 board/hisilicon/poplar/poplar.c
>  create mode 100644 configs/poplar_defconfig
>  create mode 100644 include/configs/poplar.h

Wende an: ARM64: poplar: hi3798cv200: u-boot support for Poplar 96Boards
.git/rebase-apply/patch:237: trailing whitespace.
DRAM DDR3/3L/4 SDRAM interface, maximum 32-bit data width 2 GB
.git/rebase-apply/patch:66: new blank line at EOF.
+
.git/rebase-apply/patch:96: new blank line at EOF.
+
.git/rebase-apply/patch:616: new blank line at EOF.
+
.git/rebase-apply/patch:648: new blank line at EOF.
+
warning: 5 Zeilen fügen Whitespace-Fehler hinzu.

Please address the whitespace warnings.

Regards,
Andreas
Jorge Ramirez May 25, 2017, 6:38 p.m. | #30
On 05/18/2017 12:06 AM, Tom Rini wrote:
>>>> having platform data.
>>> No, I think we're going for overkill here by not doing serial_pl01x.c as
>>> platform data.  ns16550 does platform data for this already.  This
>>> sounds like the lowest overhead way to get the clock populated and not
>>> have some DT data that's not going to be accepted upstream.
>>>
>> ummmm I am a bit lost at this point, could we recap please?
> Lets update the recap:
> - Please re-submit the dts file, now with whatever form is in v4.12-rc1,
>    saying as such in the commit (if it's just the commit message that
>    changes, that's fine and great).

The DTS file in v4.12-rc2 still does NOT contain the usb node.

==> Should I then not use the DM on USB so I can avoid DTS changes?


> - Please update serial_pl01x.c to be able to get the clock via platform
>    data, update and test your board to confirm it works.

um, It gets tricky;
I can not use platform_data since I can not use SERIAL_DM because the 
device tree does have a UART node which gets picked up.

I will have to disable DM_SERIAL and add some configs in 
include/configs/poplar.h

+#define CONFIG_PL011_SERIAL  1
+#define CONFIG_PL011_CLOCK   75000000
+#define CONFIG_PL01x_PORTS   {(void *) 0xF8B00000,}
+#define CONFIG_CONS_INDEX    0

==> is this acceptable? if not, then what should I do?

> - It'd be awesome if you do, but it won't block your series if you
>    don't, update the rest of the platforms that had been using the
>    "clock" platform to instead use the platform data method.
>
> Thanks!
Tom Rini May 25, 2017, 8:31 p.m. | #31
On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:
> On 05/18/2017 12:06 AM, Tom Rini wrote:

> >>>>having platform data.

> >>>No, I think we're going for overkill here by not doing serial_pl01x.c as

> >>>platform data.  ns16550 does platform data for this already.  This

> >>>sounds like the lowest overhead way to get the clock populated and not

> >>>have some DT data that's not going to be accepted upstream.

> >>>

> >>ummmm I am a bit lost at this point, could we recap please?

> >Lets update the recap:

> >- Please re-submit the dts file, now with whatever form is in v4.12-rc1,

> >   saying as such in the commit (if it's just the commit message that

> >   changes, that's fine and great).

> 

> The DTS file in v4.12-rc2 still does NOT contain the usb node.

> 

> ==> Should I then not use the DM on USB so I can avoid DTS changes?


Well, you can either put it in the -u-boot.dtsi file for the board, and
remove that later once it's upstream.

> >- Please update serial_pl01x.c to be able to get the clock via platform

> >   data, update and test your board to confirm it works.

> 

> um, It gets tricky;

> I can not use platform_data since I can not use SERIAL_DM because

> the device tree does have a UART node which gets picked up.


How about disabling the node in -u-boot.dtsi for the board and then you
can use platform data, I think...  But Rob, this loops us back around to
the first problem too, kind of.  Can we really just not make use of
clock-frequency and the kernel just not use it?

-- 
Tom
Jorge Ramirez May 25, 2017, 8:55 p.m. | #32
On 05/25/2017 10:31 PM, Tom Rini wrote:
> On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:
>> On 05/18/2017 12:06 AM, Tom Rini wrote:
>>>>>> having platform data.
>>>>> No, I think we're going for overkill here by not doing serial_pl01x.c as
>>>>> platform data.  ns16550 does platform data for this already.  This
>>>>> sounds like the lowest overhead way to get the clock populated and not
>>>>> have some DT data that's not going to be accepted upstream.
>>>>>
>>>> ummmm I am a bit lost at this point, could we recap please?
>>> Lets update the recap:
>>> - Please re-submit the dts file, now with whatever form is in v4.12-rc1,
>>>    saying as such in the commit (if it's just the commit message that
>>>    changes, that's fine and great).
>> The DTS file in v4.12-rc2 still does NOT contain the usb node.
>>
>> ==> Should I then not use the DM on USB so I can avoid DTS changes?
> Well, you can either put it in the -u-boot.dtsi file for the board, and
> remove that later once it's upstream.

yes I'll do that. thanks.

>
>>> - Please update serial_pl01x.c to be able to get the clock via platform
>>>    data, update and test your board to confirm it works.
>> um, It gets tricky;
>> I can not use platform_data since I can not use SERIAL_DM because
>> the device tree does have a UART node which gets picked up.
> How about disabling the node in -u-boot.dtsi for the board and then you
> can use platform data,

I dont think that would because CONFIG_OF is enabled for USB
So the kernel dtsi (not the u-boot.dtsi) that contains the uart node 
(without the clock!) will be picked by uboot and the uart will not be 
initialized....


I still think that the simplest solution is to let me merge with the 
kernel's device tree plus this u-boot.dtsi [1] and then just get rid of 
the file when possible (and the source code not the configs) would not 
need to change

https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi 


> I think...  But Rob, this loops us back around to
> the first problem too, kind of.  Can we really just not make use of
> clock-frequency and the kernel just not use it?
>
Jorge Ramirez May 25, 2017, 8:58 p.m. | #33
On 05/25/2017 10:55 PM, Jorge Ramirez wrote:
> On 05/25/2017 10:31 PM, Tom Rini wrote:
>> On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:
>>> On 05/18/2017 12:06 AM, Tom Rini wrote:
>>>>>>> having platform data.
>>>>>> No, I think we're going for overkill here by not doing 
>>>>>> serial_pl01x.c as
>>>>>> platform data.  ns16550 does platform data for this already.  This
>>>>>> sounds like the lowest overhead way to get the clock populated 
>>>>>> and not
>>>>>> have some DT data that's not going to be accepted upstream.
>>>>>>
>>>>> ummmm I am a bit lost at this point, could we recap please?
>>>> Lets update the recap:
>>>> - Please re-submit the dts file, now with whatever form is in 
>>>> v4.12-rc1,
>>>>    saying as such in the commit (if it's just the commit message that
>>>>    changes, that's fine and great).
>>> The DTS file in v4.12-rc2 still does NOT contain the usb node.
>>>
>>> ==> Should I then not use the DM on USB so I can avoid DTS changes?
>> Well, you can either put it in the -u-boot.dtsi file for the board, and
>> remove that later once it's upstream. 


yes I'll do that. thanks.

>
>>
>>>> - Please update serial_pl01x.c to be able to get the clock via 
>>>> platform
>>>>    data, update and test your board to confirm it works.
>>> um, It gets tricky;
>>> I can not use platform_data since I can not use SERIAL_DM because
>>> the device tree does have a UART node which gets picked up.
>> How about disabling the node in -u-boot.dtsi for the board and then you
>> can use platform data,
>

I dont think that would because CONFIG_OF is enabled for USB; so the 
kernel dtsi that contains the uart node (without the clock!) will be 
picked by u-boot and the uart will not be initialized properly.
I still think that the simplest solution is to let me merge with the 
kernel's device tree plus this u-boot.dtsi [1];
then just get rid of the file when possible (and NEIHER the source code 
NOR the configs) would need to change

[1] 
https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi 


>
>> I think...  But Rob, this loops us back around to
>> the first problem too, kind of.  Can we really just not make use of
>> clock-frequency and the kernel just not use it?
>>
>
Tom Rini May 25, 2017, 9:12 p.m. | #34
On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:
> On 05/25/2017 10:55 PM, Jorge Ramirez wrote:

> >On 05/25/2017 10:31 PM, Tom Rini wrote:

> >>On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:

> >>>On 05/18/2017 12:06 AM, Tom Rini wrote:

> >>>>>>>having platform data.

> >>>>>>No, I think we're going for overkill here by not doing

> >>>>>>serial_pl01x.c as

> >>>>>>platform data.  ns16550 does platform data for this already.  This

> >>>>>>sounds like the lowest overhead way to get the clock

> >>>>>>populated and not

> >>>>>>have some DT data that's not going to be accepted upstream.

> >>>>>>

> >>>>>ummmm I am a bit lost at this point, could we recap please?

> >>>>Lets update the recap:

> >>>>- Please re-submit the dts file, now with whatever form is

> >>>>in v4.12-rc1,

> >>>>   saying as such in the commit (if it's just the commit message that

> >>>>   changes, that's fine and great).

> >>>The DTS file in v4.12-rc2 still does NOT contain the usb node.

> >>>

> >>>==> Should I then not use the DM on USB so I can avoid DTS changes?

> >>Well, you can either put it in the -u-boot.dtsi file for the board, and

> >>remove that later once it's upstream.

> 

> 

> yes I'll do that. thanks.

> 

> >

> >>

> >>>>- Please update serial_pl01x.c to be able to get the clock

> >>>>via platform

> >>>>   data, update and test your board to confirm it works.

> >>>um, It gets tricky;

> >>>I can not use platform_data since I can not use SERIAL_DM because

> >>>the device tree does have a UART node which gets picked up.

> >>How about disabling the node in -u-boot.dtsi for the board and then you

> >>can use platform data,

> >

> 

> I dont think that would because CONFIG_OF is enabled for USB; so the

> kernel dtsi that contains the uart node (without the clock!) will be

> picked by u-boot and the uart will not be initialized properly.

> I still think that the simplest solution is to let me merge with the

> kernel's device tree plus this u-boot.dtsi [1];

> then just get rid of the file when possible (and NEIHER the source

> code NOR the configs) would need to change

> 

> [1] https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi


Yes, sorry.  [1] needs to be updated to disable uart0 so that you can
use platform data, at least for now.  I do want to talk more with Rob
about the general problem this exposes.

-- 
Tom
Jorge Ramirez May 25, 2017, 9:16 p.m. | #35
On 05/25/2017 11:12 PM, Tom Rini wrote:
> On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:
>> On 05/25/2017 10:55 PM, Jorge Ramirez wrote:
>>> On 05/25/2017 10:31 PM, Tom Rini wrote:
>>>> On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:
>>>>> On 05/18/2017 12:06 AM, Tom Rini wrote:
>>>>>>>>> having platform data.
>>>>>>>> No, I think we're going for overkill here by not doing
>>>>>>>> serial_pl01x.c as
>>>>>>>> platform data.  ns16550 does platform data for this already.  This
>>>>>>>> sounds like the lowest overhead way to get the clock
>>>>>>>> populated and not
>>>>>>>> have some DT data that's not going to be accepted upstream.
>>>>>>>>
>>>>>>> ummmm I am a bit lost at this point, could we recap please?
>>>>>> Lets update the recap:
>>>>>> - Please re-submit the dts file, now with whatever form is
>>>>>> in v4.12-rc1,
>>>>>>    saying as such in the commit (if it's just the commit message that
>>>>>>    changes, that's fine and great).
>>>>> The DTS file in v4.12-rc2 still does NOT contain the usb node.
>>>>>
>>>>> ==> Should I then not use the DM on USB so I can avoid DTS changes?
>>>> Well, you can either put it in the -u-boot.dtsi file for the board, and
>>>> remove that later once it's upstream.
>>
>> yes I'll do that. thanks.
>>
>>>>>> - Please update serial_pl01x.c to be able to get the clock
>>>>>> via platform
>>>>>>    data, update and test your board to confirm it works.
>>>>> um, It gets tricky;
>>>>> I can not use platform_data since I can not use SERIAL_DM because
>>>>> the device tree does have a UART node which gets picked up.
>>>> How about disabling the node in -u-boot.dtsi for the board and then you
>>>> can use platform data,
>> I dont think that would because CONFIG_OF is enabled for USB; so the
>> kernel dtsi that contains the uart node (without the clock!) will be
>> picked by u-boot and the uart will not be initialized properly.
>> I still think that the simplest solution is to let me merge with the
>> kernel's device tree plus this u-boot.dtsi [1];
>> then just get rid of the file when possible (and NEIHER the source
>> code NOR the configs) would need to change
>>
>> [1] https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi
> Yes, sorry.  [1] needs to be updated to disable uart0 so that you can
> use platform data, at least for now.  I do want to talk more with Rob
> about the general problem this exposes.

so you want me to

1) keep the node in 
https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi 

2) add status=disable
3) then add the platform_data

BUT for the pl011 driver to take the platform_data dont I also have to 
disable CONFIG_OF?

but  if I disable CONFIG_OF then I lose USB_DM

am I wrong?




>
Simon Glass May 25, 2017, 9:21 p.m. | #36
Hi Tom,

On 25 May 2017 at 15:12, Tom Rini <trini@konsulko.com> wrote:
> On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:
>> On 05/25/2017 10:55 PM, Jorge Ramirez wrote:
>> >On 05/25/2017 10:31 PM, Tom Rini wrote:
>> >>On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:
>> >>>On 05/18/2017 12:06 AM, Tom Rini wrote:
>> >>>>>>>having platform data.
>> >>>>>>No, I think we're going for overkill here by not doing
>> >>>>>>serial_pl01x.c as
>> >>>>>>platform data.  ns16550 does platform data for this already.  This
>> >>>>>>sounds like the lowest overhead way to get the clock
>> >>>>>>populated and not
>> >>>>>>have some DT data that's not going to be accepted upstream.
>> >>>>>>
>> >>>>>ummmm I am a bit lost at this point, could we recap please?
>> >>>>Lets update the recap:
>> >>>>- Please re-submit the dts file, now with whatever form is
>> >>>>in v4.12-rc1,
>> >>>>   saying as such in the commit (if it's just the commit message that
>> >>>>   changes, that's fine and great).
>> >>>The DTS file in v4.12-rc2 still does NOT contain the usb node.
>> >>>
>> >>>==> Should I then not use the DM on USB so I can avoid DTS changes?
>> >>Well, you can either put it in the -u-boot.dtsi file for the board, and
>> >>remove that later once it's upstream.
>>
>>
>> yes I'll do that. thanks.
>>
>> >
>> >>
>> >>>>- Please update serial_pl01x.c to be able to get the clock
>> >>>>via platform
>> >>>>   data, update and test your board to confirm it works.
>> >>>um, It gets tricky;
>> >>>I can not use platform_data since I can not use SERIAL_DM because
>> >>>the device tree does have a UART node which gets picked up.
>> >>How about disabling the node in -u-boot.dtsi for the board and then you
>> >>can use platform data,
>> >
>>
>> I dont think that would because CONFIG_OF is enabled for USB; so the
>> kernel dtsi that contains the uart node (without the clock!) will be
>> picked by u-boot and the uart will not be initialized properly.
>> I still think that the simplest solution is to let me merge with the
>> kernel's device tree plus this u-boot.dtsi [1];
>> then just get rid of the file when possible (and NEIHER the source
>> code NOR the configs) would need to change
>>
>> [1] https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi
>
> Yes, sorry.  [1] needs to be updated to disable uart0 so that you can
> use platform data, at least for now.  I do want to talk more with Rob
> about the general problem this exposes.

Using platform data because we cannot put a clock frequency in the DT
node seems unfortunate to me. With a little flexibility, DT can be
made to work. But IMO the sort of pedantry makes great the enemy of
good.

Regards,
Simon
Tom Rini May 25, 2017, 10:08 p.m. | #37
On Thu, May 25, 2017 at 11:16:42PM +0200, Jorge Ramirez wrote:
> On 05/25/2017 11:12 PM, Tom Rini wrote:

> >On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:

> >>On 05/25/2017 10:55 PM, Jorge Ramirez wrote:

> >>>On 05/25/2017 10:31 PM, Tom Rini wrote:

> >>>>On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:

> >>>>>On 05/18/2017 12:06 AM, Tom Rini wrote:

> >>>>>>>>>having platform data.

> >>>>>>>>No, I think we're going for overkill here by not doing

> >>>>>>>>serial_pl01x.c as

> >>>>>>>>platform data.  ns16550 does platform data for this already.  This

> >>>>>>>>sounds like the lowest overhead way to get the clock

> >>>>>>>>populated and not

> >>>>>>>>have some DT data that's not going to be accepted upstream.

> >>>>>>>>

> >>>>>>>ummmm I am a bit lost at this point, could we recap please?

> >>>>>>Lets update the recap:

> >>>>>>- Please re-submit the dts file, now with whatever form is

> >>>>>>in v4.12-rc1,

> >>>>>>   saying as such in the commit (if it's just the commit message that

> >>>>>>   changes, that's fine and great).

> >>>>>The DTS file in v4.12-rc2 still does NOT contain the usb node.

> >>>>>

> >>>>>==> Should I then not use the DM on USB so I can avoid DTS changes?

> >>>>Well, you can either put it in the -u-boot.dtsi file for the board, and

> >>>>remove that later once it's upstream.

> >>

> >>yes I'll do that. thanks.

> >>

> >>>>>>- Please update serial_pl01x.c to be able to get the clock

> >>>>>>via platform

> >>>>>>   data, update and test your board to confirm it works.

> >>>>>um, It gets tricky;

> >>>>>I can not use platform_data since I can not use SERIAL_DM because

> >>>>>the device tree does have a UART node which gets picked up.

> >>>>How about disabling the node in -u-boot.dtsi for the board and then you

> >>>>can use platform data,

> >>I dont think that would because CONFIG_OF is enabled for USB; so the

> >>kernel dtsi that contains the uart node (without the clock!) will be

> >>picked by u-boot and the uart will not be initialized properly.

> >>I still think that the simplest solution is to let me merge with the

> >>kernel's device tree plus this u-boot.dtsi [1];

> >>then just get rid of the file when possible (and NEIHER the source

> >>code NOR the configs) would need to change

> >>

> >>[1] https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi

> >Yes, sorry.  [1] needs to be updated to disable uart0 so that you can

> >use platform data, at least for now.  I do want to talk more with Rob

> >about the general problem this exposes.

> 

> so you want me to

> 

> 1) keep the node in https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi


Well, a uart0 node, but no "clock" property as that just needs to go
away.

> 2) add status=disable

> 3) then add the platform_data

> 

> BUT for the pl011 driver to take the platform_data dont I also have

> to disable CONFIG_OF?

> 

> but  if I disable CONFIG_OF then I lose USB_DM


No, the status = "disable" on uart0 should remove it from the dtb, or at
least we should see it and go "Oh, no, we don't have uart0 via DT".

-- 
Tom
Tom Rini May 25, 2017, 10:09 p.m. | #38
On Thu, May 25, 2017 at 03:21:04PM -0600, Simon Glass wrote:
> Hi Tom,

> 

> On 25 May 2017 at 15:12, Tom Rini <trini@konsulko.com> wrote:

> > On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:

> >> On 05/25/2017 10:55 PM, Jorge Ramirez wrote:

> >> >On 05/25/2017 10:31 PM, Tom Rini wrote:

> >> >>On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:

> >> >>>On 05/18/2017 12:06 AM, Tom Rini wrote:

> >> >>>>>>>having platform data.

> >> >>>>>>No, I think we're going for overkill here by not doing

> >> >>>>>>serial_pl01x.c as

> >> >>>>>>platform data.  ns16550 does platform data for this already.  This

> >> >>>>>>sounds like the lowest overhead way to get the clock

> >> >>>>>>populated and not

> >> >>>>>>have some DT data that's not going to be accepted upstream.

> >> >>>>>>

> >> >>>>>ummmm I am a bit lost at this point, could we recap please?

> >> >>>>Lets update the recap:

> >> >>>>- Please re-submit the dts file, now with whatever form is

> >> >>>>in v4.12-rc1,

> >> >>>>   saying as such in the commit (if it's just the commit message that

> >> >>>>   changes, that's fine and great).

> >> >>>The DTS file in v4.12-rc2 still does NOT contain the usb node.

> >> >>>

> >> >>>==> Should I then not use the DM on USB so I can avoid DTS changes?

> >> >>Well, you can either put it in the -u-boot.dtsi file for the board, and

> >> >>remove that later once it's upstream.

> >>

> >>

> >> yes I'll do that. thanks.

> >>

> >> >

> >> >>

> >> >>>>- Please update serial_pl01x.c to be able to get the clock

> >> >>>>via platform

> >> >>>>   data, update and test your board to confirm it works.

> >> >>>um, It gets tricky;

> >> >>>I can not use platform_data since I can not use SERIAL_DM because

> >> >>>the device tree does have a UART node which gets picked up.

> >> >>How about disabling the node in -u-boot.dtsi for the board and then you

> >> >>can use platform data,

> >> >

> >>

> >> I dont think that would because CONFIG_OF is enabled for USB; so the

> >> kernel dtsi that contains the uart node (without the clock!) will be

> >> picked by u-boot and the uart will not be initialized properly.

> >> I still think that the simplest solution is to let me merge with the

> >> kernel's device tree plus this u-boot.dtsi [1];

> >> then just get rid of the file when possible (and NEIHER the source

> >> code NOR the configs) would need to change

> >>

> >> [1] https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi

> >

> > Yes, sorry.  [1] needs to be updated to disable uart0 so that you can

> > use platform data, at least for now.  I do want to talk more with Rob

> > about the general problem this exposes.

> 

> Using platform data because we cannot put a clock frequency in the DT

> node seems unfortunate to me. With a little flexibility, DT can be

> made to work. But IMO the sort of pedantry makes great the enemy of

> good.


This, and the alias issue we talked about the other day (wrt mmc) do
highlight what I see as problems of the dts being too kernel-centric.
But I also really want to wait for Rob to chime in before we get too far
here.

-- 
Tom
Jorge Ramirez May 26, 2017, 7:28 a.m. | #39
On 05/26/2017 12:08 AM, Tom Rini wrote:
> On Thu, May 25, 2017 at 11:16:42PM +0200, Jorge Ramirez wrote:
>> On 05/25/2017 11:12 PM, Tom Rini wrote:
>>> On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:
>>>> On 05/25/2017 10:55 PM, Jorge Ramirez wrote:
>>>>> On 05/25/2017 10:31 PM, Tom Rini wrote:
>>>>>> On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:
>>>>>>> On 05/18/2017 12:06 AM, Tom Rini wrote:
>>>>>>>>>>> having platform data.
>>>>>>>>>> No, I think we're going for overkill here by not doing
>>>>>>>>>> serial_pl01x.c as
>>>>>>>>>> platform data.  ns16550 does platform data for this already.  This
>>>>>>>>>> sounds like the lowest overhead way to get the clock
>>>>>>>>>> populated and not
>>>>>>>>>> have some DT data that's not going to be accepted upstream.
>>>>>>>>>>
>>>>>>>>> ummmm I am a bit lost at this point, could we recap please?
>>>>>>>> Lets update the recap:
>>>>>>>> - Please re-submit the dts file, now with whatever form is
>>>>>>>> in v4.12-rc1,
>>>>>>>>    saying as such in the commit (if it's just the commit message that
>>>>>>>>    changes, that's fine and great).
>>>>>>> The DTS file in v4.12-rc2 still does NOT contain the usb node.
>>>>>>>
>>>>>>> ==> Should I then not use the DM on USB so I can avoid DTS changes?
>>>>>> Well, you can either put it in the -u-boot.dtsi file for the board, and
>>>>>> remove that later once it's upstream.
>>>> yes I'll do that. thanks.
>>>>
>>>>>>>> - Please update serial_pl01x.c to be able to get the clock
>>>>>>>> via platform
>>>>>>>>    data, update and test your board to confirm it works.
>>>>>>> um, It gets tricky;
>>>>>>> I can not use platform_data since I can not use SERIAL_DM because
>>>>>>> the device tree does have a UART node which gets picked up.
>>>>>> How about disabling the node in -u-boot.dtsi for the board and then you
>>>>>> can use platform data,
>>>> I dont think that would because CONFIG_OF is enabled for USB; so the
>>>> kernel dtsi that contains the uart node (without the clock!) will be
>>>> picked by u-boot and the uart will not be initialized properly.
>>>> I still think that the simplest solution is to let me merge with the
>>>> kernel's device tree plus this u-boot.dtsi [1];
>>>> then just get rid of the file when possible (and NEIHER the source
>>>> code NOR the configs) would need to change
>>>>
>>>> [1] https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi
>>> Yes, sorry.  [1] needs to be updated to disable uart0 so that you can
>>> use platform data, at least for now.  I do want to talk more with Rob
>>> about the general problem this exposes.
>> so you want me to
>>
>> 1) keep the node in https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi
> Well, a uart0 node, but no "clock" property as that just needs to go
> away.

Sounds good to me. now see below

>
>> 2) add status=disable
>> 3) then add the platform_data
>>
>> BUT for the pl011 driver to take the platform_data dont I also have
>> to disable CONFIG_OF?
>>
>> but  if I disable CONFIG_OF then I lose USB_DM
> No, the status = "disable" on uart0 should remove it from the dtb, or at
> least we should see it and go "Oh, no, we don't have uart0 via DT".
>


1) Since the UART0 is enabled in the kernel's DTS I will have to modify 
the device tree that I am importing from kernel.org to disable it.

2) But even doing this is not enough: I have to completely remove the 
uart0 node from the tree.


So to sum up:

In order to get the platform data for pl01x I have to either disable OF 
(so I lose the USB node as I said earlier) or *completely* remove the 
UART0 node from from the kernel dts.
I personally would rather not modify the kernel's DTS trees that I am 
importing into uboot but I am really confused about the policy now.

please could you clarify?

I still think what I proposed when we started is the better way to go: a 
uboot specific hi3798cv200-u-boot.dtsifile that contains the two nodes 
that are giving trouble.

The timeline then goes:
- the usb node will disappear as soon as it lands in korg
- the uart node and the whole file will be removed during the cleanup of 
all the pl01x uart offenders.

but if you think modifying the kernels dts is better I can do that as well.
Jorge Ramirez May 26, 2017, 7:46 a.m. | #40
On 05/26/2017 09:28 AM, Jorge Ramirez wrote:
>>>>>
>>>>> [1] 
>>>>> https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi 
>>>>>
>>>> Yes, sorry.  [1] needs to be updated to disable uart0 so that you can
>>>> use platform data, at least for now.  I do want to talk more with Rob
>>>> about the general problem this exposes.
>>> so you want me to
>>>
>>> 1) keep the node in 
>>> https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi 
>>>
>> Well, a uart0 node, but no "clock" property as that just needs to go
>> away.
>
> Sounds good to me. now see below
>
>>
>>> 2) add status=disable
>>> 3) then add the platform_data
>>>
>>> BUT for the pl011 driver to take the platform_data dont I also have
>>> to disable CONFIG_OF?
>>>
>>> but  if I disable CONFIG_OF then I lose USB_DM
>> No, the status = "disable" on uart0 should remove it from the dtb, or at
>> least we should see it and go "Oh, no, we don't have uart0 via DT".
>>
>
>
> 1) Since the UART0 is enabled in the kernel's DTS I will have to 
> modify the device tree that I am importing from kernel.org to disable it.
>
> 2) But even doing this is not enough: I have to completely remove the 
> uart0 node from the tree.
>
>
> So to sum up:
>
> In order to get the platform data for pl01x I have to either disable 
> OF (so I lose the USB node as I said earlier) or *completely* remove 
> the UART0 node from from the kernel dts.
> I personally would rather not modify the kernel's DTS trees that I am 
> importing into uboot but I am really confused about the policy now.

ie: to be clear from my side, doing the following is not enough:




Is this the right way to go then? alter the kernel DTS when merging into 
uboot?

>
> please could you clarify?
>
> I still think what I proposed when we started is the better way to go: 
> a uboot specific hi3798cv200-u-boot.dtsifile that contains the two 
> nodes that are giving trouble.
>
> The timeline then goes:
> - the usb node will disappear as soon as it lands in korg
> - the uart node and the whole file will be removed during the cleanup 
> of all the pl01x uart offenders.
>
> but if you think modifying the kernels dts is better I can do that as 
> well.diff --git a/arch/arm/dts/hi3798cv200-poplar.dts 
b/arch/arm/dts/hi3798cv200-poplar.dts
index b914287..d4ce16d 100644
--- a/arch/arm/dts/hi3798cv200-poplar.dts
+++ b/arch/arm/dts/hi3798cv200-poplar.dts
@@ -152,7 +152,7 @@
  };

  &uart0 {
-   status = "okay";
+ status = "disabled";
  };


I have to actually do

diff --git a/arch/arm/dts/hi3798cv200-poplar.dts 
b/arch/arm/dts/hi3798cv200-poplar.dts
index b914287..028013f 100644
--- a/arch/arm/dts/hi3798cv200-poplar.dts
+++ b/arch/arm/dts/hi3798cv200-poplar.dts
@@ -17,7 +17,6 @@
         compatible = "hisilicon,hi3798cv200-poplar", 
"hisilicon,hi3798cv200";

         aliases {
-           serial0 = &uart0;
                 serial2 = &uart2;
         };

@@ -151,12 +150,9 @@
         label = "LS-SPI0";
  };

-&uart0 {
-   status = "okay";
-};

  &uart2 {
         status = "okay";
         label = "LS-UART0";
  };


OR

[jramirez@titan git.u-boot (upstream *$)]$ git diff
diff --git a/arch/arm/dts/hi3798cv200-poplar.dts 
b/arch/arm/dts/hi3798cv200-poplar.dts
index b914287..5d909b83 100644
--- a/arch/arm/dts/hi3798cv200-poplar.dts
+++ b/arch/arm/dts/hi3798cv200-poplar.dts
@@ -21,10 +21,6 @@
                 serial2 = &uart2;
         };

-   chosen {
-           stdout-path = "serial0:115200n8";
-   };
-
         memory@0 {
                 device_type = "memory";
                 reg = <0x0 0x0 0x0 0x80000000>;
@@ -152,7 +148,7 @@
  };

  &uart0 {
-   status = "okay";
+ status = "disabled";
  };

  &uart2 {

Tom Rini May 26, 2017, 12:46 p.m. | #41
On Fri, May 26, 2017 at 09:28:28AM +0200, Jorge Ramirez wrote:
> On 05/26/2017 12:08 AM, Tom Rini wrote:

> >On Thu, May 25, 2017 at 11:16:42PM +0200, Jorge Ramirez wrote:

> >>On 05/25/2017 11:12 PM, Tom Rini wrote:

> >>>On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:

> >>>>On 05/25/2017 10:55 PM, Jorge Ramirez wrote:

> >>>>>On 05/25/2017 10:31 PM, Tom Rini wrote:

> >>>>>>On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:

> >>>>>>>On 05/18/2017 12:06 AM, Tom Rini wrote:

> >>>>>>>>>>>having platform data.

> >>>>>>>>>>No, I think we're going for overkill here by not doing

> >>>>>>>>>>serial_pl01x.c as

> >>>>>>>>>>platform data.  ns16550 does platform data for this already.  This

> >>>>>>>>>>sounds like the lowest overhead way to get the clock

> >>>>>>>>>>populated and not

> >>>>>>>>>>have some DT data that's not going to be accepted upstream.

> >>>>>>>>>>

> >>>>>>>>>ummmm I am a bit lost at this point, could we recap please?

> >>>>>>>>Lets update the recap:

> >>>>>>>>- Please re-submit the dts file, now with whatever form is

> >>>>>>>>in v4.12-rc1,

> >>>>>>>>   saying as such in the commit (if it's just the commit message that

> >>>>>>>>   changes, that's fine and great).

> >>>>>>>The DTS file in v4.12-rc2 still does NOT contain the usb node.

> >>>>>>>

> >>>>>>>==> Should I then not use the DM on USB so I can avoid DTS changes?

> >>>>>>Well, you can either put it in the -u-boot.dtsi file for the board, and

> >>>>>>remove that later once it's upstream.

> >>>>yes I'll do that. thanks.

> >>>>

> >>>>>>>>- Please update serial_pl01x.c to be able to get the clock

> >>>>>>>>via platform

> >>>>>>>>   data, update and test your board to confirm it works.

> >>>>>>>um, It gets tricky;

> >>>>>>>I can not use platform_data since I can not use SERIAL_DM because

> >>>>>>>the device tree does have a UART node which gets picked up.

> >>>>>>How about disabling the node in -u-boot.dtsi for the board and then you

> >>>>>>can use platform data,

> >>>>I dont think that would because CONFIG_OF is enabled for USB; so the

> >>>>kernel dtsi that contains the uart node (without the clock!) will be

> >>>>picked by u-boot and the uart will not be initialized properly.

> >>>>I still think that the simplest solution is to let me merge with the

> >>>>kernel's device tree plus this u-boot.dtsi [1];

> >>>>then just get rid of the file when possible (and NEIHER the source

> >>>>code NOR the configs) would need to change

> >>>>

> >>>>[1] https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi

> >>>Yes, sorry.  [1] needs to be updated to disable uart0 so that you can

> >>>use platform data, at least for now.  I do want to talk more with Rob

> >>>about the general problem this exposes.

> >>so you want me to

> >>

> >>1) keep the node in https://github.com/ldts/poplar-u-boot/blob/upstream/arch/arm/dts/hi3798cv200-u-boot.dtsi

> >Well, a uart0 node, but no "clock" property as that just needs to go

> >away.

> 

> Sounds good to me. now see below

> 

> >>2) add status=disable

> >>3) then add the platform_data

> >>

> >>BUT for the pl011 driver to take the platform_data dont I also have

> >>to disable CONFIG_OF?

> >>

> >>but  if I disable CONFIG_OF then I lose USB_DM

> >No, the status = "disable" on uart0 should remove it from the dtb, or at

> >least we should see it and go "Oh, no, we don't have uart0 via DT".

> 

> 1) Since the UART0 is enabled in the kernel's DTS I will have to

> modify the device tree that I am importing from kernel.org to

> disable it.


Yes, the dtsi file in [1] is what modifies it.

> 2) But even doing this is not enough: I have to completely remove

> the uart0 node from the tree.


Why?  Is this in theory, or in practice?  I ask since we just had to
disable the kernel-enabled mmc3 node on am335x-evm.dts in
am335x-evm-u-boot.dtsi in order to have it boot as probing mmc3 in
U-Boot fails (we don't enable the relevant clocks).  So disabling uart0
should cause the resulting dtb file to omit entirely, or just have
&uart0 {status="disabled"} or similar and U-Boot should not do anything,
so platform data should be used.  If this is not the case, there's a bug
we need to track down.

> 

> 

> So to sum up:

> 

> In order to get the platform data for pl01x I have to either disable

> OF (so I lose the USB node as I said earlier) or *completely* remove

> the UART0 node from from the kernel dts.

> I personally would rather not modify the kernel's DTS trees that I

> am importing into uboot but I am really confused about the policy

> now.

> 

> please could you clarify?

> 

> I still think what I proposed when we started is the better way to

> go: a uboot specific hi3798cv200-u-boot.dtsifile that contains the

> two nodes that are giving trouble.


I don't understand what we're not understanding, yes, you should be
using a -u-boot.dtsi file to mark uart0 as disabled and not have to
modify the kernel dts file at all.

-- 
Tom
Jorge Ramirez May 26, 2017, 12:58 p.m. | #42
On May 26, 2017 2:46 PM, "Tom Rini" <trini@konsulko.com> wrote:

On Fri, May 26, 2017 at 09:28:28AM +0200, Jorge Ramirez wrote:
> On 05/26/2017 12:08 AM, Tom Rini wrote:
> >On Thu, May 25, 2017 at 11:16:42PM +0200, Jorge Ramirez wrote:
> >>On 05/25/2017 11:12 PM, Tom Rini wrote:
> >>>On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:
> >>>>On 05/25/2017 10:55 PM, Jorge Ramirez wrote:
> >>>>>On 05/25/2017 10:31 PM, Tom Rini wrote:
> >>>>>>On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:
> >>>>>>>On 05/18/2017 12:06 AM, Tom Rini wrote:
> >>>>>>>>>>>having platform data.
> >>>>>>>>>>No, I think we're going for overkill here by not doing
> >>>>>>>>>>serial_pl01x.c as
> >>>>>>>>>>platform data.  ns16550 does platform data for this already.
This
> >>>>>>>>>>sounds like the lowest overhead way to get the clock
> >>>>>>>>>>populated and not
> >>>>>>>>>>have some DT data that's not going to be accepted upstream.
> >>>>>>>>>>
> >>>>>>>>>ummmm I am a bit lost at this point, could we recap please?
> >>>>>>>>Lets update the recap:
> >>>>>>>>- Please re-submit the dts file, now with whatever form is
> >>>>>>>>in v4.12-rc1,
> >>>>>>>>   saying as such in the commit (if it's just the commit message
that
> >>>>>>>>   changes, that's fine and great).
> >>>>>>>The DTS file in v4.12-rc2 still does NOT contain the usb node.
> >>>>>>>
> >>>>>>>==> Should I then not use the DM on USB so I can avoid DTS changes?
> >>>>>>Well, you can either put it in the -u-boot.dtsi file for the board,
and
> >>>>>>remove that later once it's upstream.
> >>>>yes I'll do that. thanks.
> >>>>
> >>>>>>>>- Please update serial_pl01x.c to be able to get the clock
> >>>>>>>>via platform
> >>>>>>>>   data, update and test your board to confirm it works.
> >>>>>>>um, It gets tricky;
> >>>>>>>I can not use platform_data since I can not use SERIAL_DM because
> >>>>>>>the device tree does have a UART node which gets picked up.
> >>>>>>How about disabling the node in -u-boot.dtsi for the board and then
you
> >>>>>>can use platform data,
> >>>>I dont think that would because CONFIG_OF is enabled for USB; so the
> >>>>kernel dtsi that contains the uart node (without the clock!) will be
> >>>>picked by u-boot and the uart will not be initialized properly.
> >>>>I still think that the simplest solution is to let me merge with the
> >>>>kernel's device tree plus this u-boot.dtsi [1];
> >>>>then just get rid of the file when possible (and NEIHER the source
> >>>>code NOR the configs) would need to change
> >>>>
> >>>>[1] https://github.com/ldts/poplar-u-boot/blob/upstream/
arch/arm/dts/hi3798cv200-u-boot.dtsi
> >>>Yes, sorry.  [1] needs to be updated to disable uart0 so that you can
> >>>use platform data, at least for now.  I do want to talk more with Rob
> >>>about the general problem this exposes.
> >>so you want me to
> >>
> >>1) keep the node in https://github.com/ldts/poplar-u-boot/blob/upstream/
arch/arm/dts/hi3798cv200-u-boot.dtsi
> >Well, a uart0 node, but no "clock" property as that just needs to go
> >away.
>
> Sounds good to me. now see below
>
> >>2) add status=disable
> >>3) then add the platform_data
> >>
> >>BUT for the pl011 driver to take the platform_data dont I also have
> >>to disable CONFIG_OF?
> >>
> >>but  if I disable CONFIG_OF then I lose USB_DM
> >No, the status = "disable" on uart0 should remove it from the dtb, or at
> >least we should see it and go "Oh, no, we don't have uart0 via DT".
>
> 1) Since the UART0 is enabled in the kernel's DTS I will have to
> modify the device tree that I am importing from kernel.org to
> disable it.

Yes, the dtsi file in [1] is what modifies it.

> 2) But even doing this is not enough: I have to completely remove
> the uart0 node from the tree.

Why?  Is this in theory, or in practice?  I ask since we just had to
disable the kernel-enabled mmc3 node on am335x-evm.dts in
am335x-evm-u-boot.dtsi in order to have it boot as probing mmc3 in
U-Boot fails (we don't enable the relevant clocks).  So disabling uart0
should cause the resulting dtb file to omit entirely, or just have
&uart0 {status="disabled"} or similar and U-Boot should not do anything,
so platform data should be used.  If this is not the case, there's a bug
we need to track down.

>
>
> So to sum up:
>
> In order to get the platform data for pl01x I have to either disable
> OF (so I lose the USB node as I said earlier) or *completely* remove
> the UART0 node from from the kernel dts.
> I personally would rather not modify the kernel's DTS trees that I
> am importing into uboot but I am really confused about the policy
> now.
>
> please could you clarify?
>
> I still think what I proposed when we started is the better way to
> go: a uboot specific hi3798cv200-u-boot.dtsifile that contains the
> two nodes that are giving trouble.

I don't understand what we're not understanding, yes, you should be
using a -u-boot.dtsi file to mark uart0 as disabled and not have to
modify the kernel dts file at all.



This the bit that is NOT possible. Doing that is not enough.

I will also have to modify the kernel dts.


--
Tom
Tom Rini May 26, 2017, 4:09 p.m. | #43
On Fri, May 26, 2017 at 02:58:04PM +0200, Jorge Ramirez Ortiz wrote:
> On May 26, 2017 2:46 PM, "Tom Rini" <trini@konsulko.com> wrote:

> 

> On Fri, May 26, 2017 at 09:28:28AM +0200, Jorge Ramirez wrote:

> > On 05/26/2017 12:08 AM, Tom Rini wrote:

> > >On Thu, May 25, 2017 at 11:16:42PM +0200, Jorge Ramirez wrote:

> > >>On 05/25/2017 11:12 PM, Tom Rini wrote:

> > >>>On Thu, May 25, 2017 at 10:58:20PM +0200, Jorge Ramirez wrote:

> > >>>>On 05/25/2017 10:55 PM, Jorge Ramirez wrote:

> > >>>>>On 05/25/2017 10:31 PM, Tom Rini wrote:

> > >>>>>>On Thu, May 25, 2017 at 08:38:47PM +0200, Jorge Ramirez wrote:

> > >>>>>>>On 05/18/2017 12:06 AM, Tom Rini wrote:

> > >>>>>>>>>>>having platform data.

> > >>>>>>>>>>No, I think we're going for overkill here by not doing

> > >>>>>>>>>>serial_pl01x.c as

> > >>>>>>>>>>platform data.  ns16550 does platform data for this already.

> This

> > >>>>>>>>>>sounds like the lowest overhead way to get the clock

> > >>>>>>>>>>populated and not

> > >>>>>>>>>>have some DT data that's not going to be accepted upstream.

> > >>>>>>>>>>

> > >>>>>>>>>ummmm I am a bit lost at this point, could we recap please?

> > >>>>>>>>Lets update the recap:

> > >>>>>>>>- Please re-submit the dts file, now with whatever form is

> > >>>>>>>>in v4.12-rc1,

> > >>>>>>>>   saying as such in the commit (if it's just the commit message

> that

> > >>>>>>>>   changes, that's fine and great).

> > >>>>>>>The DTS file in v4.12-rc2 still does NOT contain the usb node.

> > >>>>>>>

> > >>>>>>>==> Should I then not use the DM on USB so I can avoid DTS changes?

> > >>>>>>Well, you can either put it in the -u-boot.dtsi file for the board,

> and

> > >>>>>>remove that later once it's upstream.

> > >>>>yes I'll do that. thanks.

> > >>>>

> > >>>>>>>>- Please update serial_pl01x.c to be able to get the clock

> > >>>>>>>>via platform

> > >>>>>>>>   data, update and test your board to confirm it works.

> > >>>>>>>um, It gets tricky;

> > >>>>>>>I can not use platform_data since I can not use SERIAL_DM because

> > >>>>>>>the device tree does have a UART node which gets picked up.

> > >>>>>>How about disabling the node in -u-boot.dtsi for the board and then

> you

> > >>>>>>can use platform data,

> > >>>>I dont think that would because CONFIG_OF is enabled for USB; so the

> > >>>>kernel dtsi that contains the uart node (without the clock!) will be

> > >>>>picked by u-boot and the uart will not be initialized properly.

> > >>>>I still think that the simplest solution is to let me merge with the

> > >>>>kernel's device tree plus this u-boot.dtsi [1];

> > >>>>then just get rid of the file when possible (and NEIHER the source

> > >>>>code NOR the configs) would need to change

> > >>>>

> > >>>>[1] https://github.com/ldts/poplar-u-boot/blob/upstream/

> arch/arm/dts/hi3798cv200-u-boot.dtsi

> > >>>Yes, sorry.  [1] needs to be updated to disable uart0 so that you can

> > >>>use platform data, at least for now.  I do want to talk more with Rob

> > >>>about the general problem this exposes.

> > >>so you want me to

> > >>

> > >>1) keep the node in https://github.com/ldts/poplar-u-boot/blob/upstream/

> arch/arm/dts/hi3798cv200-u-boot.dtsi

> > >Well, a uart0 node, but no "clock" property as that just needs to go

> > >away.

> >

> > Sounds good to me. now see below

> >

> > >>2) add status=disable

> > >>3) then add the platform_data

> > >>

> > >>BUT for the pl011 driver to take the platform_data dont I also have

> > >>to disable CONFIG_OF?

> > >>

> > >>but  if I disable CONFIG_OF then I lose USB_DM

> > >No, the status = "disable" on uart0 should remove it from the dtb, or at

> > >least we should see it and go "Oh, no, we don't have uart0 via DT".

> >

> > 1) Since the UART0 is enabled in the kernel's DTS I will have to

> > modify the device tree that I am importing from kernel.org to

> > disable it.

> 

> Yes, the dtsi file in [1] is what modifies it.

> 

> > 2) But even doing this is not enough: I have to completely remove

> > the uart0 node from the tree.

> 

> Why?  Is this in theory, or in practice?  I ask since we just had to

> disable the kernel-enabled mmc3 node on am335x-evm.dts in

> am335x-evm-u-boot.dtsi in order to have it boot as probing mmc3 in

> U-Boot fails (we don't enable the relevant clocks).  So disabling uart0

> should cause the resulting dtb file to omit entirely, or just have

> &uart0 {status="disabled"} or similar and U-Boot should not do anything,

> so platform data should be used.  If this is not the case, there's a bug

> we need to track down.

> 

> >

> >

> > So to sum up:

> >

> > In order to get the platform data for pl01x I have to either disable

> > OF (so I lose the USB node as I said earlier) or *completely* remove

> > the UART0 node from from the kernel dts.

> > I personally would rather not modify the kernel's DTS trees that I

> > am importing into uboot but I am really confused about the policy

> > now.

> >

> > please could you clarify?

> >

> > I still think what I proposed when we started is the better way to

> > go: a uboot specific hi3798cv200-u-boot.dtsifile that contains the

> > two nodes that are giving trouble.

> 

> I don't understand what we're not understanding, yes, you should be

> using a -u-boot.dtsi file to mark uart0 as disabled and not have to

> modify the kernel dts file at all.

> 

> 

> 

> This the bit that is NOT possible. Doing that is not enough.


To be clear, are you trying this on current mainline?  Simon reminded me
that if you don't have 7452946e7f37 in your tree, the -u-boot.dtsi file
cannot disable a node.

-- 
Tom
Jorge Ramirez May 29, 2017, 9 a.m. | #44
On 05/26/2017 06:09 PM, Tom Rini wrote:
>>> So to sum up:
>>>
>>> In order to get the platform data for pl01x I have to either disable
>>> OF (so I lose the USB node as I said earlier) or*completely*  remove
>>> the UART0 node from from the kernel dts.
>>> I personally would rather not modify the kernel's DTS trees that I
>>> am importing into uboot but I am really confused about the policy
>>> now.
>>>
>>> please could you clarify?
>>>
>>> I still think what I proposed when we started is the better way to
>>> go: a uboot specific hi3798cv200-u-boot.dtsifile that contains the
>>> two nodes that are giving trouble.
>> I don't understand what we're not understanding, yes, you should be
>> using a -u-boot.dtsi file to mark uart0 as disabled and not have to
>> modify the kernel dts file at all.
>>
>>
>>
>> This the bit that is NOT possible. Doing that is not enough.
> To be clear, are you trying this on current mainline?  Simon reminded me
> that if you don't have 7452946e7f37 in your tree, the -u-boot.dtsi file
> cannot disable a node.

yes I have that commit (thanks Tom for checking this)

The issue is actually with serial-uclass.c when the kernel dts contains 
a chosen node that contains the stdout-path.
     chosen {
         stdout-path = "serial0:115200n8";
     };

Disabling uart0 (ie serial0) in u-boot.dtsi loses the console instead of 
probing the pl01x for the platform_data.

is there a pre-defined way to work around this?
Tom Rini May 29, 2017, 11:57 a.m. | #45
On Mon, May 29, 2017 at 11:00:32AM +0200, Jorge Ramirez wrote:
> On 05/26/2017 06:09 PM, Tom Rini wrote:

> >>>So to sum up:

> >>>

> >>>In order to get the platform data for pl01x I have to either disable

> >>>OF (so I lose the USB node as I said earlier) or*completely*  remove

> >>>the UART0 node from from the kernel dts.

> >>>I personally would rather not modify the kernel's DTS trees that I

> >>>am importing into uboot but I am really confused about the policy

> >>>now.

> >>>

> >>>please could you clarify?

> >>>

> >>>I still think what I proposed when we started is the better way to

> >>>go: a uboot specific hi3798cv200-u-boot.dtsifile that contains the

> >>>two nodes that are giving trouble.

> >>I don't understand what we're not understanding, yes, you should be

> >>using a -u-boot.dtsi file to mark uart0 as disabled and not have to

> >>modify the kernel dts file at all.

> >>

> >>

> >>

> >>This the bit that is NOT possible. Doing that is not enough.

> >To be clear, are you trying this on current mainline?  Simon reminded me

> >that if you don't have 7452946e7f37 in your tree, the -u-boot.dtsi file

> >cannot disable a node.

> 

> yes I have that commit (thanks Tom for checking this)

> 

> The issue is actually with serial-uclass.c when the kernel dts

> contains a chosen node that contains the stdout-path.

>     chosen {

>         stdout-path = "serial0:115200n8";

>     };

> 

> Disabling uart0 (ie serial0) in u-boot.dtsi loses the console

> instead of probing the pl01x for the platform_data.

> 

> is there a pre-defined way to work around this?


Provide a new stdout-path in the -u-boot.dtsi too?  Any changes you
could make to the main dts file so that this would work should be able
to be done in the -u-boot.dtsi too.

-- 
Tom
Jorge Ramirez May 29, 2017, 12:18 p.m. | #46
On 05/29/2017 01:57 PM, Tom Rini wrote:
>> The issue is actually with serial-uclass.c when the kernel dts
>> contains a chosen node that contains the stdout-path.
>>      chosen {
>>          stdout-path = "serial0:115200n8";
>>      };
>>
>> Disabling uart0 (ie serial0) in u-boot.dtsi loses the console
>> instead of probing the pl01x for the platform_data.
>>
>> is there a pre-defined way to work around this?
> Provide a new stdout-path in the -u-boot.dtsi too?  Any changes you

yes I obviously tried that but are you sure that that is allowed with 
"chosen" it being kind of a special type of node?
the compiler just cant find the label when added to u-boot.dtsi hence 
why I was asking (sorry, I should have raised it upfront)
ie:

/*
  * U-Boot addition to:
  *  1) use platform data for the console
  *  2) provide support for the generic-ehci USB driver currently not 
available
  *     in the linux kernel (8/May/2017).
  *
  * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
  *
  * SPDX-License-Identifier:     GPL-2.0+
  */

&soc {
     usb2: ehci@9890000 {
         compatible = "generic-ehci";
         reg = <0x9890000 0x100>;
         status = "okay";
     };
};

&uart0 {
     status = "disabled";
};

&chosen {
     stdout-path = &uart0;

};


leads to

   LD      u-boot
   OBJCOPY u-boot.srec
   OBJCOPY u-boot-nodtb.bin
   SYM     u-boot.sym
start=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_start | cut -f 1 -d 
' '); end=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_end | cut -f 1 
-d ' '); tools/relocate-rela u-boot-nodtb.bin 0x37000000 $start $end
   DTC     arch/arm/dts/hi3798cv200-poplar.dtb
Error: ./arch/arm/dts/hi3798cv200-u-boot.dtsi:27.2-3 label or path, 
'chosen', not found
FATAL ERROR: Syntax error parsing input tree
scripts/Makefile.lib:322: recipe for target 
'arch/arm/dts/hi3798cv200-poplar.dtb' failed
make[2]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 1
dts/Makefile:32: recipe for target 'arch/arm/dts/hi3798cv200-poplar.dtb' 
failed
make[1]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 2
Makefile:865: recipe for target 'dts/dt.dtb' failed
make: *** [dts/dt.dtb] Error 2

> could make to the main dts file so that this would work should be able
> to be done in the -u-boot.dtsi too.
Jorge Ramirez May 29, 2017, 12:23 p.m. | #47
On 05/29/2017 02:18 PM, Jorge Ramirez wrote:
> On 05/29/2017 01:57 PM, Tom Rini wrote:
>>> The issue is actually with serial-uclass.c when the kernel dts
>>> contains a chosen node that contains the stdout-path.
>>>      chosen {
>>>          stdout-path = "serial0:115200n8";
>>>      };
>>>
>>> Disabling uart0 (ie serial0) in u-boot.dtsi loses the console
>>> instead of probing the pl01x for the platform_data.
>>>
>>> is there a pre-defined way to work around this?
>> Provide a new stdout-path in the -u-boot.dtsi too?  Any changes you
>
> yes I obviously tried that but are you sure that that is allowed with 
> "chosen" it being kind of a special type of node?
> the compiler just cant find the label when added to u-boot.dtsi hence 
> why I was asking (sorry, I should have raised it upfront)
> ie:
>
> /*
>  * U-Boot addition to:
>  *  1) use platform data for the console
>  *  2) provide support for the generic-ehci USB driver currently not 
> available
>  *     in the linux kernel (8/May/2017).
>  *
>  * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>  *
>  * SPDX-License-Identifier:     GPL-2.0+
>  */
>
> &soc {
>     usb2: ehci@9890000 {
>         compatible = "generic-ehci";
>         reg = <0x9890000 0x100>;
>         status = "okay";
>     };
> };
>
> &uart0 {
>     status = "disabled";
> };
>
> &chosen {
>     stdout-path = &uart0;
>
> };


oops, wrong syntax. ok done.
will send v5 today

thanks!


>
>
> leads to
>
>   LD      u-boot
>   OBJCOPY u-boot.srec
>   OBJCOPY u-boot-nodtb.bin
>   SYM     u-boot.sym
> start=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_start | cut -f 1 
> -d ' '); end=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_end | cut 
> -f 1 -d ' '); tools/relocate-rela u-boot-nodtb.bin 0x37000000 $start $end
>   DTC     arch/arm/dts/hi3798cv200-poplar.dtb
> Error: ./arch/arm/dts/hi3798cv200-u-boot.dtsi:27.2-3 label or path, 
> 'chosen', not found
> FATAL ERROR: Syntax error parsing input tree
> scripts/Makefile.lib:322: recipe for target 
> 'arch/arm/dts/hi3798cv200-poplar.dtb' failed
> make[2]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 1
> dts/Makefile:32: recipe for target 
> 'arch/arm/dts/hi3798cv200-poplar.dtb' failed
> make[1]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 2
> Makefile:865: recipe for target 'dts/dt.dtb' failed
> make: *** [dts/dt.dtb] Error 2
>
>> could make to the main dts file so that this would work should be able
>> to be done in the -u-boot.dtsi too.
>
>
>
>
>
>
>
>
>
>
>
Tom Rini May 29, 2017, 12:26 p.m. | #48
On Mon, May 29, 2017 at 02:18:48PM +0200, Jorge Ramirez wrote:
> On 05/29/2017 01:57 PM, Tom Rini wrote:

> >>The issue is actually with serial-uclass.c when the kernel dts

> >>contains a chosen node that contains the stdout-path.

> >>     chosen {

> >>         stdout-path = "serial0:115200n8";

> >>     };

> >>

> >>Disabling uart0 (ie serial0) in u-boot.dtsi loses the console

> >>instead of probing the pl01x for the platform_data.

> >>

> >>is there a pre-defined way to work around this?

> >Provide a new stdout-path in the -u-boot.dtsi too?  Any changes you

> 

> yes I obviously tried that but are you sure that that is allowed

> with "chosen" it being kind of a special type of node?

> the compiler just cant find the label when added to u-boot.dtsi

> hence why I was asking (sorry, I should have raised it upfront)

> ie:

> 

> /*

>  * U-Boot addition to:

>  *  1) use platform data for the console

>  *  2) provide support for the generic-ehci USB driver currently not

> available

>  *     in the linux kernel (8/May/2017).

>  *

>  * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

>  *

>  * SPDX-License-Identifier:     GPL-2.0+

>  */

> 

> &soc {

>     usb2: ehci@9890000 {

>         compatible = "generic-ehci";

>         reg = <0x9890000 0x100>;

>         status = "okay";

>     };

> };

> 

> &uart0 {

>     status = "disabled";

> };

> 

> &chosen {

>     stdout-path = &uart0;

> 

> };

> 

> 

> leads to

> 

>   LD      u-boot

>   OBJCOPY u-boot.srec

>   OBJCOPY u-boot-nodtb.bin

>   SYM     u-boot.sym

> start=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_start | cut -f

> 1 -d ' '); end=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_end |

> cut -f 1 -d ' '); tools/relocate-rela u-boot-nodtb.bin 0x37000000

> $start $end

>   DTC     arch/arm/dts/hi3798cv200-poplar.dtb

> Error: ./arch/arm/dts/hi3798cv200-u-boot.dtsi:27.2-3 label or path,

> 'chosen', not found

> FATAL ERROR: Syntax error parsing input tree

> scripts/Makefile.lib:322: recipe for target

> 'arch/arm/dts/hi3798cv200-poplar.dtb' failed

> make[2]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 1

> dts/Makefile:32: recipe for target

> 'arch/arm/dts/hi3798cv200-poplar.dtb' failed

> make[1]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 2

> Makefile:865: recipe for target 'dts/dt.dtb' failed

> make: *** [dts/dt.dtb] Error 2

> 


Well, lets see.  Pantelis has been telling me that what we're doing here
is going to lead to problems like what you see here in some cases.  Any
suggestions?

-- 
Tom
Jorge Ramirez May 29, 2017, 12:28 p.m. | #49
On 05/29/2017 02:26 PM, Tom Rini wrote:
> On Mon, May 29, 2017 at 02:18:48PM +0200, Jorge Ramirez wrote:
>> On 05/29/2017 01:57 PM, Tom Rini wrote:
>>>> The issue is actually with serial-uclass.c when the kernel dts
>>>> contains a chosen node that contains the stdout-path.
>>>>      chosen {
>>>>          stdout-path = "serial0:115200n8";
>>>>      };
>>>>
>>>> Disabling uart0 (ie serial0) in u-boot.dtsi loses the console
>>>> instead of probing the pl01x for the platform_data.
>>>>
>>>> is there a pre-defined way to work around this?
>>> Provide a new stdout-path in the -u-boot.dtsi too?  Any changes you
>> yes I obviously tried that but are you sure that that is allowed
>> with "chosen" it being kind of a special type of node?
>> the compiler just cant find the label when added to u-boot.dtsi
>> hence why I was asking (sorry, I should have raised it upfront)
>> ie:
>>
>> /*
>>   * U-Boot addition to:
>>   *  1) use platform data for the console
>>   *  2) provide support for the generic-ehci USB driver currently not
>> available
>>   *     in the linux kernel (8/May/2017).
>>   *
>>   * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>   *
>>   * SPDX-License-Identifier:     GPL-2.0+
>>   */
>>
>> &soc {
>>      usb2: ehci@9890000 {
>>          compatible = "generic-ehci";
>>          reg = <0x9890000 0x100>;
>>          status = "okay";
>>      };
>> };
>>
>> &uart0 {
>>      status = "disabled";
>> };
>>
>> &chosen {
>>      stdout-path = &uart0;
>>
>> };
>>
>>
>> leads to
>>
>>    LD      u-boot
>>    OBJCOPY u-boot.srec
>>    OBJCOPY u-boot-nodtb.bin
>>    SYM     u-boot.sym
>> start=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_start | cut -f
>> 1 -d ' '); end=$(aarch64-linux-gnu-nm u-boot | grep __rel_dyn_end |
>> cut -f 1 -d ' '); tools/relocate-rela u-boot-nodtb.bin 0x37000000
>> $start $end
>>    DTC     arch/arm/dts/hi3798cv200-poplar.dtb
>> Error: ./arch/arm/dts/hi3798cv200-u-boot.dtsi:27.2-3 label or path,
>> 'chosen', not found
>> FATAL ERROR: Syntax error parsing input tree
>> scripts/Makefile.lib:322: recipe for target
>> 'arch/arm/dts/hi3798cv200-poplar.dtb' failed
>> make[2]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 1
>> dts/Makefile:32: recipe for target
>> 'arch/arm/dts/hi3798cv200-poplar.dtb' failed
>> make[1]: *** [arch/arm/dts/hi3798cv200-poplar.dtb] Error 2
>> Makefile:865: recipe for target 'dts/dt.dtb' failed
>> make: *** [dts/dt.dtb] Error 2
>>
> Well, lets see.  Pantelis has been telling me that what we're doing here
> is going to lead to problems like what you see here in some cases.  Any
> suggestions?
>

sorry Tom, this is the right syntax for "chosen" hi3798cv200-u-boot.dtsi
uart0 is now initialized and functional via platform data

&soc {
     usb2: ehci@9890000 {
         compatible = "generic-ehci";
         reg = <0x9890000 0x100>;
         status = "okay";
     };
};

&uart0 {
     status = "disabled";
};

/{
     chosen {
         stdout-path = "";
     };
};

Patch hide | download patch | download mbox

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 1df6b36..d41e047 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -816,6 +816,17 @@  config TARGET_HIKEY
 	  Support for HiKey 96boards platform. It features a HI6220
 	  SoC, with 8xA53 CPU, mali450 gpu, and 1GB RAM.
 
+config TARGET_POPLAR
+	bool "Support Poplar 96boards Enterprise Edition Platform"
+	select ARM64
+	select DM
+	select OF_CONTROL
+	select DM_SERIAL
+	select DM_USB
+	  help
+	  Support for Poplar 96boards EE platform. It features a HI3798cv200
+	  SoC, with 4xA53 CPU, MaliT720 GPU, and 1GB RAM.
+
 config TARGET_LS1012AQDS
 	bool "Support ls1012aqds"
 	select ARCH_LS1012A
@@ -1145,6 +1156,7 @@  source "board/grinn/chiliboard/Kconfig"
 source "board/gumstix/pepper/Kconfig"
 source "board/h2200/Kconfig"
 source "board/hisilicon/hikey/Kconfig"
+source "board/hisilicon/poplar/Kconfig"
 source "board/imx31_phycore/Kconfig"
 source "board/isee/igep0033/Kconfig"
 source "board/olimex/mx23_olinuxino/Kconfig"
diff --git a/arch/arm/dts/hi3798cv200.dtsi b/arch/arm/dts/hi3798cv200.dtsi
index 75865f8..caa17de 100644
--- a/arch/arm/dts/hi3798cv200.dtsi
+++ b/arch/arm/dts/hi3798cv200.dtsi
@@ -409,3 +409,6 @@ 
 		};
 	};
 };
+
+#include "poplar-uboot.dtsi"
+
diff --git a/arch/arm/dts/poplar-uboot.dtsi b/arch/arm/dts/poplar-uboot.dtsi
new file mode 100644
index 0000000..8a9668a
--- /dev/null
+++ b/arch/arm/dts/poplar-uboot.dtsi
@@ -0,0 +1,24 @@ 
+/*
+ * U-Boot addition to:
+ *  1) initialize the console clock rate.
+ *  2) provide support for the generic-ehci USB driver currently not available
+ *     in the linux kernel (8/May/2017).
+ *
+ * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+&soc {
+	usb2: ehci@9890000 {
+		compatible = "generic-ehci";
+		reg = <0x9890000 0x100>;
+		status = "okay";
+	};
+};
+
+&uart0 {
+	clock = <75000000>;
+	status = "okay";
+};
+
diff --git a/arch/arm/include/asm/arch-hi3798cv200/dwmmc.h b/arch/arm/include/asm/arch-hi3798cv200/dwmmc.h
new file mode 100644
index 0000000..1060d94
--- /dev/null
+++ b/arch/arm/include/asm/arch-hi3798cv200/dwmmc.h
@@ -0,0 +1,13 @@ 
+/*
+ * (C) Copyright 2017 Linaro
+ * Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef _HI3798cv200_DWMMC_H_
+#define _HI3798cv200_DWMMC_H_
+
+int hi6220_dwmci_add_port(int index, u32 regbase, int bus_width);
+
+#endif /* _HI3798cv200_DWMMC_H_ */
diff --git a/arch/arm/include/asm/arch-hi3798cv200/hi3798cv200.h b/arch/arm/include/asm/arch-hi3798cv200/hi3798cv200.h
new file mode 100644
index 0000000..1bd0d78
--- /dev/null
+++ b/arch/arm/include/asm/arch-hi3798cv200/hi3798cv200.h
@@ -0,0 +1,50 @@ 
+/*
+ * (C) Copyright 2017 Linaro
+ * Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __HI3798cv200_H__
+#define __HI3798cv200_H__
+
+#define REG_BASE_PERI_CTRL		0xF8A20000
+#define REG_BASE_CRG			0xF8A22000
+
+/* DEVICES */
+#define REG_BASE_EHCI			0XF9890000
+#define REG_BASE_MCI			0xF9830000
+
+/* PERI control registers (4KB) */
+	/* USB2 PHY01 configuration register */
+#define PERI_CTRL_USB0			(REG_BASE_PERI_CTRL + 0x120)
+
+/* PERI CRG registers (4KB) */
+	/* USB2 CTRL0 clock and soft reset */
+#define PERI_CRG46			(REG_BASE_CRG + 0xb8)
+#define USB2_BUS_CKEN			(1<<0)
+#define USB2_OHCI48M_CKEN		(1<<1)
+#define USB2_OHCI12M_CKEN		(1<<2)
+#define USB2_OTG_UTMI_CKEN		(1<<3)
+#define USB2_HST_PHY_CKEN		(1<<4)
+#define USB2_UTMI0_CKEN			(1<<5)
+#define USB2_BUS_SRST_REQ		(1<<12)
+#define USB2_UTMI0_SRST_REQ		(1<<13)
+#define USB2_HST_PHY_SYST_REQ		(1<<16)
+#define USB2_OTG_PHY_SYST_REQ		(1<<17)
+#define USB2_CLK48_SEL			(1<<20)
+
+	/* USB2 PHY clock and soft reset */
+#define PERI_CRG47			(REG_BASE_CRG + 0xbc)
+#define USB2_PHY01_REF_CKEN		(1 << 0)
+#define USB2_PHY2_REF_CKEN		(1 << 2)
+#define USB2_PHY01_SRST_REQ		(1 << 4)
+#define USB2_PHY2_SRST_REQ		(1 << 6)
+#define USB2_PHY01_SRST_TREQ0		(1 << 8)
+#define USB2_PHY01_SRST_TREQ1		(1 << 9)
+#define USB2_PHY2_SRST_TREQ		(1 << 10)
+#define USB2_PHY01_REFCLK_SEL		(1 << 12)
+#define USB2_PHY2_REFCLK_SEL		(1 << 14)
+
+
+#endif
diff --git a/board/hisilicon/poplar/Kconfig b/board/hisilicon/poplar/Kconfig
new file mode 100644
index 0000000..3397295
--- /dev/null
+++ b/board/hisilicon/poplar/Kconfig
@@ -0,0 +1,15 @@ 
+if TARGET_POPLAR
+
+config SYS_BOARD
+	default "poplar"
+
+config SYS_VENDOR
+	default "hisilicon"
+
+config SYS_SOC
+	default "hi3798cv200"
+
+config SYS_CONFIG_NAME
+	default "poplar"
+
+endif
diff --git a/board/hisilicon/poplar/MAINTAINERS b/board/hisilicon/poplar/MAINTAINERS
new file mode 100644
index 0000000..0cc01c8
--- /dev/null
+++ b/board/hisilicon/poplar/MAINTAINERS
@@ -0,0 +1,6 @@ 
+Poplar BOARD
+M:     Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
+S:     Maintained
+F:     board/hisilicon/poplar
+F:     include/configs/poplar.h
+F:     configs/poplar_defconfig
diff --git a/board/hisilicon/poplar/Makefile b/board/hisilicon/poplar/Makefile
new file mode 100644
index 0000000..101545d
--- /dev/null
+++ b/board/hisilicon/poplar/Makefile
@@ -0,0 +1,7 @@ 
+#
+# (C) Copyright 2017 Linaro
+# Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+obj-y	:= poplar.o
diff --git a/board/hisilicon/poplar/README b/board/hisilicon/poplar/README
new file mode 100644
index 0000000..b35382b
--- /dev/null
+++ b/board/hisilicon/poplar/README
@@ -0,0 +1,232 @@ 
+================================================================================
+			Board Information
+================================================================================
+
+Developed by HiSilicon, the board features the Hi3798C V200 with an
+integrated quad-core 64-bit ARM Cortex A53 processor and high
+performance Mali T720 GPU, making it capable of running any commercial
+set-top solution based on Linux or Android. Its high performance
+specification also supports a premium user experience with up to H.265
+HEVC decoding of 4K video at 60 frames per second.
+
+SOC  Hisilicon Hi3798CV200
+CPU  Quad-core ARM Cortex-A53 64 bit
+DRAM DDR3/3L/4 SDRAM interface, maximum 32-bit data width 2 GB 
+USB  Two USB 2.0 ports One USB 3.0 ports
+CONSOLE  USB-micro port for console support
+ETHERNET  1 GBe Ethernet
+PCIE  One PCIe 2.0 interfaces
+JTAG  8-Pin JTAG
+EXPANSION INTERFACE  Linaro 96Boards Low Speed Expansion slot
+DIMENSION Standard 160×120 mm 96Boards Enterprice Edition form factor
+WIFI  802.11AC 2*2 with Bluetooth
+CONNECTORS  One connector for Smart Card One connector for TSI
+
+
+================================================================================
+			BUILD INSTRUCTIONS
+================================================================================
+
+Compile from source:
+====================
+
+Get all the sources
+
+  > mkdir -p ~/poplar/src ~/poplar/bin
+  > cd ~/poplar/src
+  > git clone https://github.com/Linaro/poplar-l-loader.git l-loader
+  > git clone https://github.com/Linaro/poplar-arm-trusted-firmware.git atf
+  > git clone https://github.com/Linaro/poplar-u-boot.git u-boot
+
+
+Compile U-Boot:
+===============
+
+  Prerequisite:
+  # sudo apt-get install device-tree-compiler
+
+  > cd ~/poplar/src/u-boot
+  > make CROSS_COMPILE=aarch64-linux-gnu- poplar_defconfig
+  > make CROSS_COMPILE=aarch64-linux-gnu-
+  > cp u-boot.bin ~/poplar/bin
+
+Compile ARM Trusted Firmware (ATF):
+===================================
+
+  > cd ~/poplar/src/atf
+  > make CROSS_COMPILE=aarch64-linux-gnu- all fip \
+		SPD=none BL33=~/poplar/bin/u-boot.bin DEBUG=1 PLAT=poplar
+
+Copy resulting binaries
+  > cp build/hi3798cv200/debug/bl1.bin ~/poplar/src/l-loader/atf/
+  > cp build/hi3798cv200/debug/fip.bin ~/poplar/src/l-loader/atf/
+
+Compile l-loader:
+=================
+
+  > cd ~/poplar/src/l-loader
+  > make clean
+  > make CROSS_COMPILE=arm-linux-gnueabi-
+
+   Due to BootROM requiremets, rename l-loader.bin to fastboot.bin:
+  > cp l-loader.bin ~/poplar/bin/fastboot.bin
+
+
+================================================================================
+			FLASH INSTRUCTIONS
+================================================================================
+
+Two methods:
+
+Using USB debrick support:
+	Copy fastboot.bin to a FAT partition on the USB drive and reboot the
+       poplar board while pressing S3(usb_boot).
+
+       The system will execute the new u-boot and boot into a shell which you
+       can then use to write to eMMC.
+
+Using U-BOOT from shell:
+	1) using AXIS usb ethernet dongle and tftp
+	2) using FAT formated USB drive
+
+
+1. TFTP (USB ethernet dongle)
+=============================
+
+Plug a USB AXIS ethernet dongle on any of the USB2 ports on the Poplar board.
+Copy fastboot.bin to your tftp server.
+In u-boot make sure your network is properly setup.
+
+Then
+
+=> tftp 0x30000000 fastboot.bin
+starting USB...
+USB0:   USB EHCI 1.00
+scanning bus 0 for devices... 1 USB Device(s) found
+USB1:   USB EHCI 1.00
+scanning bus 1 for devices... 3 USB Device(s) found
+       scanning usb for storage devices... 0 Storage Device(s) found
+       scanning usb for ethernet devices... 1 Ethernet Device(s) found
+Waiting for Ethernet connection... done.
+Using asx0 device
+TFTP from server 192.168.1.4; our IP address is 192.168.1.10
+Filename 'poplar/fastboot.bin'.
+Load address: 0x30000000
+Loading: #################################################################
+	 #################################################################
+	 ###############################################################
+	 2 MiB/s
+done
+Bytes transferred = 983040 (f0000 hex)
+
+=> mmc write 0x30000000 0 0x780
+
+MMC write: dev # 0, block # 0, count 1920 ... 1920 blocks written: OK
+=> reset
+
+
+2. USING USB FAT DRIVE
+=======================
+
+Copy fastboot.bin to any partition on a FAT32 formated usb flash drive.
+Enter the uboot prompt
+
+=> fatls usb 0:2
+   983040   fastboot.bin
+
+1 file(s), 0 dir(s)
+
+=> fatload usb 0:2 0x30000000 fastboot.bin
+reading fastboot.bin
+983040 bytes read in 44 ms (21.3 MiB/s)
+
+=> mmc write 0x30000000 0 0x780
+
+MMC write: dev # 0, block # 0, count 1920 ... 1920 blocks written: OK
+
+
+================================================================================
+				BOOT TRACE
+================================================================================
+
+Bootrom start
+Boot Media: eMMC
+Decrypt auxiliary code ...OK
+
+lsadc voltage min: 000000FE, max: 000000FF, aver: 000000FE, index: 00000000
+
+Entry boot auxiliary code
+
+Auxiliary code - v1.00
+DDR code - V1.1.2 20160205
+Build: Mar 24 2016 - 17:09:44
+Reg Version:  v134
+Reg Time:     2016/03/18 09:44:55
+Reg Name:     hi3798cv2dmb_hi3798cv200_ddr3_2gbyte_8bitx4_4layers.reg
+
+Boot auxiliary code success
+Bootrom success
+
+LOADER:  Switched to aarch64 mode
+LOADER:  Entering ARM TRUSTED FIRMWARE
+LOADER:  CPU0 executes at 0x000ce000
+
+INFO:    BL1: 0xe1000 - 0xe7000 [size = 24576]
+NOTICE:  Booting Trusted Firmware
+NOTICE:  BL1: v1.3(debug):v1.3-372-g1ba9c60
+NOTICE:  BL1: Built : 17:51:33, Apr 30 2017
+INFO:    BL1: RAM 0xe1000 - 0xe7000
+INFO:    BL1: Loading BL2
+INFO:    Loading image id=1 at address 0xe9000
+INFO:    Image id=1 loaded at address 0xe9000, size = 0x5008
+NOTICE:  BL1: Booting BL2
+INFO:    Entry point address = 0xe9000
+INFO:    SPSR = 0x3c5
+NOTICE:  BL2: v1.3(debug):v1.3-372-g1ba9c60
+NOTICE:  BL2: Built : 17:51:33, Apr 30 2017
+INFO:    BL2: Loading BL31
+INFO:    Loading image id=3 at address 0x129000
+INFO:    Image id=3 loaded at address 0x129000, size = 0x8038
+INFO:    BL2: Loading BL33
+INFO:    Loading image id=5 at address 0x37000000
+INFO:    Image id=5 loaded at address 0x37000000, size = 0x58f17
+NOTICE:  BL1: Booting BL31
+INFO:    Entry point address = 0x129000
+INFO:    SPSR = 0x3cd
+INFO:    Boot bl33 from 0x37000000 for 364311 Bytes
+NOTICE:  BL31: v1.3(debug):v1.3-372-g1ba9c60
+NOTICE:  BL31: Built : 17:51:33, Apr 30 2017
+INFO:    BL31: Initializing runtime services
+INFO:    BL31: Preparing for EL3 exit to normal world
+INFO:    Entry point address = 0x37000000
+INFO:    SPSR = 0x3c9
+
+
+U-Boot 2017.05-rc2-00130-gd2255b0 (Apr 30 2017 - 17:51:28 +0200)poplar
+
+Model: HiSilicon Poplar Development Board
+BOARD: Hisilicon HI3798cv200 Poplar
+DRAM:  1 GiB
+MMC:   Hisilicon DWMMC: 0
+In:    serial@f8b00000
+Out:   serial@f8b00000
+Err:   serial@f8b00000
+Net:   Net Initialization Skipped
+No ethernet found.
+
+Hit any key to stop autoboot:  0
+starting USB...
+USB0:   USB EHCI 1.00
+scanning bus 0 for devices... 1 USB Device(s) found
+USB1:   USB EHCI 1.00
+scanning bus 1 for devices... 4 USB Device(s) found
+       scanning usb for storage devices... 1 Storage Device(s) found
+       scanning usb for ethernet devices... 1 Ethernet Device(s) found
+
+USB device 0:
+    Device 0: Vendor: SanDisk Rev: 1.00 Prod: Cruzer Blade
+	    Type: Removable Hard Disk
+	    Capacity: 7632.0 MB = 7.4 GB (15630336 x 512)
+... is now current device
+Scanning usb 0:1...
+=>
diff --git a/board/hisilicon/poplar/poplar.c b/board/hisilicon/poplar/poplar.c
new file mode 100644
index 0000000..4569187
--- /dev/null
+++ b/board/hisilicon/poplar/poplar.c
@@ -0,0 +1,155 @@ 
+/*
+ * (C) Copyright 2017 Linaro
+ * Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/arch/hi3798cv200.h>
+#include <linux/arm-smccc.h>
+#include <asm/arch/dwmmc.h>
+#include <asm/armv8/mmu.h>
+#include <asm/io.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static struct mm_region poplar_mem_map[] = {
+	{
+		.virt = 0x0UL,
+		.phys = 0x0UL,
+		.size = 0x80000000UL,
+		.attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |
+			 PTE_BLOCK_INNER_SHARE
+	}, {
+		.virt = 0x80000000UL,
+		.phys = 0x80000000UL,
+		.size = 0x80000000UL,
+		.attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |
+			 PTE_BLOCK_NON_SHARE |
+			 PTE_BLOCK_PXN | PTE_BLOCK_UXN
+	}, {
+		0,
+	}
+};
+
+struct mm_region *mem_map = poplar_mem_map;
+
+int checkboard(void)
+{
+	puts("BOARD: Hisilicon HI3798cv200 Poplar\n");
+
+	return 0;
+}
+
+void reset_cpu(ulong addr)
+{
+	psci_system_reset();
+}
+
+int dram_init(void)
+{
+	gd->ram_size = get_ram_size(NULL, 0x80000000);
+
+	return 0;
+}
+
+int dram_init_banksize(void)
+{
+	int i;
+
+	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
+		gd->bd->bi_dram[i].start = i * gd->bd->bi_dram[i - 1].size;
+		gd->bd->bi_dram[i].size = get_ram_size( (void *)
+			gd->bd->bi_dram[i].start, 0x10000000);
+	}
+
+	return 0;
+}
+
+static void usb2_phy_config(void)
+{
+	const u32 config[] = {
+		/* close EOP pre-emphasis. open data pre-emphasis */
+		0xa1001c,
+		/* Rcomp = 150mW, increase DC level */
+		0xa00607,
+		/* keep Rcomp working */
+		0xa10700,
+		/* Icomp = 212mW, increase current drive */
+		0xa00aab,
+		/* EMI fix: rx_active not stay 1 when error packets received */
+		0xa11140,
+		/* Comp mode select */
+		0xa11041,
+		/* adjust eye diagram */
+		0xa0098c,
+		/* adjust eye diagram */
+		0xa10a0a,
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(config); i++) {
+		writel(config[i], PERI_CTRL_USB0);
+		clrsetbits_le32(PERI_CTRL_USB0, BIT(21), BIT(20) | BIT(22));
+		udelay(20);
+	}
+}
+
+static void usb2_phy_init(void)
+{
+	/* reset usb2 controller bus/utmi/roothub */
+	setbits_le32(PERI_CRG46,
+		USB2_BUS_SRST_REQ | USB2_UTMI0_SRST_REQ |
+		USB2_HST_PHY_SYST_REQ | USB2_OTG_PHY_SYST_REQ);
+	udelay(200);
+
+	/* reset usb2 phy por/utmi */
+	setbits_le32(PERI_CRG47, USB2_PHY01_SRST_REQ | USB2_PHY01_SRST_TREQ1);
+	udelay(200);
+
+	/* open usb2 ref clk */
+	setbits_le32(PERI_CRG47, USB2_PHY01_REF_CKEN);
+	udelay(300);
+
+	/* cancel usb2 power on reset */
+	clrbits_le32(PERI_CRG47, USB2_PHY01_SRST_REQ);
+	udelay(500);
+
+	usb2_phy_config();
+
+	/* cancel usb2 port reset, wait comp circuit stable */
+	clrbits_le32(PERI_CRG47, USB2_PHY01_SRST_TREQ1);
+	mdelay(10);
+
+	/* open usb2 controller clk */
+	setbits_le32(PERI_CRG46,
+		USB2_BUS_CKEN | USB2_OHCI48M_CKEN | USB2_OHCI12M_CKEN |
+		USB2_OTG_UTMI_CKEN | USB2_HST_PHY_CKEN | USB2_UTMI0_CKEN);
+	udelay(200);
+
+	/* cancel usb2 control reset */
+	clrbits_le32(PERI_CRG46,
+		USB2_BUS_SRST_REQ | USB2_UTMI0_SRST_REQ |
+		USB2_HST_PHY_SYST_REQ | USB2_OTG_PHY_SYST_REQ);
+	udelay(200);
+}
+
+int board_mmc_init(bd_t *bis)
+{
+	int ret;
+
+	ret = hi6220_dwmci_add_port(0, REG_BASE_MCI, 8);
+	if (ret)
+		printf("mmc init error (%d)\n", ret);
+
+	return ret;
+}
+
+int board_init(void)
+{
+	usb2_phy_init();
+
+	return 0;
+}
+
diff --git a/configs/poplar_defconfig b/configs/poplar_defconfig
new file mode 100644
index 0000000..8f9f40f
--- /dev/null
+++ b/configs/poplar_defconfig
@@ -0,0 +1,26 @@ 
+CONFIG_ARM=y
+CONFIG_TARGET_POPLAR=y
+CONFIG_IDENT_STRING="poplar"
+CONFIG_DEFAULT_DEVICE_TREE="hi3798cv200-poplar"
+CONFIG_SYS_PROMPT="poplar# "
+CONFIG_DISTRO_DEFAULTS=y
+CONFIG_DISPLAY_CPUINFO=n
+CONFIG_DISPLAY_BOARDINFO=y
+CONFIG_ISO_PARTITION=n
+CONFIG_MMC_DW=y
+CONFIG_MMC_DW_K3=y
+CONFIG_PL011_SERIAL=y
+CONFIG_PSCI_RESET=y
+CONFIG_USB=y
+CONFIG_USB_EHCI=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_EHCI_GENERIC=y
+CONFIG_USB_STORAGE=y
+CONFIG_NET=y
+# CONFIG_CMD_IMLS is not set
+# CONFIG_DM_GPIO is not set
+CONFIG_LIB_RAND=y
+CONFIG_CMD_UNZIP=y
+CONFIG_CMD_MMC=y
+CONFIG_CMD_USB=y
+
diff --git a/include/configs/poplar.h b/include/configs/poplar.h
new file mode 100644
index 0000000..db208c1
--- /dev/null
+++ b/include/configs/poplar.h
@@ -0,0 +1,86 @@ 
+/*
+ * (C) Copyright 2017 Linaro
+ *
+ * Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
+ *
+ * Configuration for Poplar 96boards CE. Parts were derived from other ARM
+ * configurations.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef _POPLAR_H_
+#define _POPLAR_H_
+
+#include <linux/sizes.h>
+
+/* DRAM banks */
+#define CONFIG_NR_DRAM_BANKS			4
+
+/* SYS */
+#define CONFIG_SYS_BOOTM_LEN			0x1400000
+#define CONFIG_SYS_INIT_SP_ADDR			0x200000
+#define CONFIG_SYS_LOAD_ADDR			0x800000
+#define CONFIG_SYS_MALLOC_LEN			SZ_32M
+
+/* ATF bl33.bin load address (must match) */
+#define CONFIG_SYS_TEXT_BASE			0x37000000
+
+/* PL010/PL011 */
+#define CONFIG_PL01X_SERIAL
+
+/* USB configuration */
+#define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS	3
+#define CONFIG_USB_MAX_CONTROLLER_COUNT		2
+#define CONFIG_SYS_USB_EVENT_POLL
+#define CONFIG_USB_HOST_ETHER
+#define CONFIG_USB_ETHER_ASIX
+
+/* SD/MMC */
+#define CONFIG_BOUNCE_BUFFER
+
+/*****************************************************************************
+ *  Initial environment variables
+ *****************************************************************************/
+
+#define BOOT_TARGET_DEVICES(func) 					\
+					func(USB, usb, 0)		\
+					func(MMC, mmc, 0) 		\
+					func(DHCP, dhcp, na)
+#ifndef CONFIG_SPL_BUILD
+#include <config_distro_defaults.h>
+#include <config_distro_bootcmd.h>
+#endif
+
+#define CONFIG_EXTRA_ENV_SETTINGS					\
+					"loader_mmc_blknum=0x0\0"	\
+					"loader_mmc_nblks=0x780\0"	\
+					"env_mmc_blknum=0xF0000\0"	\
+					"env_mmc_nblks=0x80\0"		\
+					"kernel_addr_r=0x30000000\0"	\
+					"pxefile_addr_r=0x32000000\0"	\
+					"scriptaddr=0x32000000\0"	\
+					"fdt_addr_r=0x32200000\0"	\
+					"ramdisk_addr_r=0x32400000\0"	\
+	BOOTENV
+
+
+/* Command line configuration */
+#define CONFIG_ENV_IS_IN_MMC		1
+#define CONFIG_SYS_MMC_ENV_DEV		0
+#define CONFIG_ENV_OFFSET		0xF0000  /* env_mmc_blknum */
+#define CONFIG_ENV_SIZE			0x10000  /* env_mmc_nblks bytes */
+#define CONFIG_CMD_ENV
+#define CONFIG_FAT_WRITE
+#define CONFIG_ENV_VARS_UBOOT_CONFIG
+
+/* Monitor Command Prompt */
+#define CONFIG_CMDLINE_EDITING
+#define CONFIG_SYS_LONGHELP
+#define CONFIG_SYS_CBSIZE		512
+#define CONFIG_SYS_MAXARGS		64
+#define CONFIG_SYS_PBSIZE		(CONFIG_SYS_CBSIZE + \
+					sizeof(CONFIG_SYS_PROMPT) + 16)
+#define CONFIG_SYS_BARGSIZE		CONFIG_SYS_CBSIZE
+
+#endif /* _POPLAR_H_ */