| 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 | expand |
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
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 :)
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
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?
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
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 :) )
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
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
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
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
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
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
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
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
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
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.
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
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?
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
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
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
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?
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
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
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
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
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
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
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
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!
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
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? >
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? >> >
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
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? >
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
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
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
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.
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 {
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
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
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
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?
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
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.
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. > > > > > > > > > > >
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
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 = ""; }; };
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_ */
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