Message ID | CAD0U-hJsYuTU8eeBgF1gmDGRhyWo1GNog=mjNYRU=PtyaDc-ng@mail.gmail.com |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
On Mon, Nov 16, 2015 at 09:46:55AM +0000, Ryan Harkin wrote: > On 13 November 2015 at 13:39, Ryan Harkin <ryan.harkin@linaro.org> wrote: > > [trying again with Linus on the to: list] > > > > Hi Linus, > > > > Tom asked for a small change to your patch. Would mind having a look > > and re-working? > > > > Thanks, > > Ryan. > > > > On 21 October 2015 at 14:16, Ryan Harkin <ryan.harkin@linaro.org> wrote: > >> On 21 October 2015 at 13:50, Tom Rini <trini@konsulko.com> wrote: > >>> On Wed, Oct 21, 2015 at 12:00:03PM +0100, Ryan Harkin wrote: > >>>> On 20 October 2015 at 16:38, Tom Rini <trini@konsulko.com> wrote: > >>>> > >>>> > On Tue, Oct 20, 2015 at 08:05:40AM +0200, Linus Walleij wrote: > >>>> > > >>>> > > Only compile in PCIe support if the board really uses it. Provide > >>>> > > a stub for the init function if e.g. FVP is being built. > >>>> > > > >>>> > > Cc: Liviu Dudau <Liviu.Dudau@foss.arm.com> > >>>> > > Cc: Ryan Harkin <ryan.harkin@linaro.org> > >>>> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > >>>> > > --- > >>>> > > board/armltd/vexpress64/Makefile | 3 ++- > >>>> > > board/armltd/vexpress64/pcie.c | 2 -- > >>>> > > board/armltd/vexpress64/pcie.h | 4 ++++ > >>>> > > 3 files changed, 6 insertions(+), 3 deletions(-) > >>>> > > > >>>> > > diff --git a/board/armltd/vexpress64/Makefile > >>>> > b/board/armltd/vexpress64/Makefile > >>>> > > index a35db401b684..b4391a71249a 100644 > >>>> > > --- a/board/armltd/vexpress64/Makefile > >>>> > > +++ b/board/armltd/vexpress64/Makefile > >>>> > > @@ -5,4 +5,5 @@ > >>>> > > # SPDX-License-Identifier: GPL-2.0+ > >>>> > > # > >>>> > > > >>>> > > -obj-y := vexpress64.o pcie.o > >>>> > > +obj-y := vexpress64.o > >>>> > > +obj-$(CONFIG_TARGET_VEXPRESS64_JUNO) += pcie.o > >>>> > > diff --git a/board/armltd/vexpress64/pcie.c > >>>> > b/board/armltd/vexpress64/pcie.c > >>>> > > index 7b999e8ef40b..311c4500e3ff 100644 > >>>> > > --- a/board/armltd/vexpress64/pcie.c > >>>> > > +++ b/board/armltd/vexpress64/pcie.c > >>>> > > @@ -191,7 +191,5 @@ void xr3pci_init(void) > >>>> > > > >>>> > > void vexpress64_pcie_init(void) > >>>> > > { > >>>> > > -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO > >>>> > > xr3pci_init(); > >>>> > > -#endif > >>>> > > } > >>>> > > diff --git a/board/armltd/vexpress64/pcie.h > >>>> > b/board/armltd/vexpress64/pcie.h > >>>> > > index 14642f4f5c43..55b276d6af11 100644 > >>>> > > --- a/board/armltd/vexpress64/pcie.h > >>>> > > +++ b/board/armltd/vexpress64/pcie.h > >>>> > > @@ -1,6 +1,10 @@ > >>>> > > #ifndef __VEXPRESS64_PCIE_H__ > >>>> > > #define __VEXPRESS64_PCIE_H__ > >>>> > > > >>>> > > +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO > >>>> > > void vexpress64_pcie_init(void); > >>>> > > +#else > >>>> > > +static inline void vexpress64_pcie_init(void) {} > >>>> > > +#endif > >>>> > > > >>>> > > #endif /* __VEXPRESS64_PCIE_H__ */ > >>>> > > >>>> > Alright, so the versatile platform makes life fun at times. > >>>> > >>>> > >>>> This comes back to the refactoring thread we had recently... > >>>> > >>>> > >>>> > >>>> > First, my > >>>> > preference is for weak functions (and I _think_ the linker ends up being > >>>> > smart enough about them to optimize things away, if not, arrg). Second, > >>>> > the question I have is, is this xr3pci_init() bit really a Juno thing or > >>>> > a baseboard thing (I assume no) > >>>> > >>>> > >>>> It's sort-of the same thing on Juno. Juno has a baseboard and SoC in one > >>>> build, unlike the previous 32-bit Versatile Express motherboard that takes > >>>> core tiles with different cores on it. > >>>> > >>>> Juno R0 has the PCIe controller, but it doesn't work. So the code is safe > >>>> to run at this level. Juno R1 has the controller and it works. > >>>> > >>>> or a going to be the same on the next > >>>> > Cortex-Asomething module thing? > >>>> > >>>> > >>>> Juno R2 will have the PCIe controller too and it should hopefully work. > >>> > >>> But it will also be the A52. Or no, the A72? Or can't say.. > >>> > >> > >> If I knew the answer, "can't say" would probably be the official line. > >> But I haven't been told of any plans to change the cores, so I am > >> assuming the next Juno respin will be A57/A53 big.LITTLE. Just like > >> we have now but with fixes. > >> > >> > >>>> But this controller is not Cortex-Asomething related. Another vendor could > >>>> put the same IP block on their board, of course. > >>> > >>> Right, but it wouldn't be under board/armltd/vexpress64/ either.. > >>> > >>>> FVP models don't have it and other ARM platforms won't necessarily have it. > >>>> > >>>> Does that help?! > >>> > >>> Yes, thanks. I think it's just a style thing then. We don't do a lot > >>> of static inline nop functions, we do __weak functions in the main file > >>> (and comment about what it should be doing in a real function) and then > >>> provide the strong version in another file. So just the pcie.h part > >>> needs changing then. > >> > > I'm not familiar with how __weak functions work, but reading Tom's > advice and grepping the code leads me to the diff below. Tom, is this > what you were looking for? > > diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile > index a35db40..b4391a7 100644 > --- a/board/armltd/vexpress64/Makefile > +++ b/board/armltd/vexpress64/Makefile > @@ -5,4 +5,5 @@ > # SPDX-License-Identifier: GPL-2.0+ > # > > -obj-y := vexpress64.o pcie.o > +obj-y := vexpress64.o > +obj-$(CONFIG_TARGET_VEXPRESS64_JUNO) += pcie.o > diff --git a/board/armltd/vexpress64/pcie.c b/board/armltd/vexpress64/pcie.c > index 7b999e8..311c450 100644 > --- a/board/armltd/vexpress64/pcie.c > +++ b/board/armltd/vexpress64/pcie.c > @@ -191,7 +191,5 @@ void xr3pci_init(void) > > void vexpress64_pcie_init(void) > { > -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO > xr3pci_init(); > -#endif > } > diff --git a/board/armltd/vexpress64/vexpress64.c > b/board/armltd/vexpress64/vexpress64.c > index f4e8084..09a3166 100644 > --- a/board/armltd/vexpress64/vexpress64.c > +++ b/board/armltd/vexpress64/vexpress64.c > @@ -28,6 +28,8 @@ U_BOOT_DEVICE(vexpress_serials) = { > .platdata = &serial_platdata, > }; > > +__weak void vexpress64_pcie_init(void) {} > + > int board_init(void) > { > vexpress64_pcie_init(); Pretty much. checkpatch should probably warn that you didn't do { } and it would be good to have a comment about what the function is expected to do but I suppose this is obvious enough by name. Thanks!
On 16 November 2015 at 18:03, Tom Rini <trini@konsulko.com> wrote: > On Mon, Nov 16, 2015 at 09:46:55AM +0000, Ryan Harkin wrote: >> On 13 November 2015 at 13:39, Ryan Harkin <ryan.harkin@linaro.org> wrote: >> > [trying again with Linus on the to: list] >> > >> > Hi Linus, >> > >> > Tom asked for a small change to your patch. Would mind having a look >> > and re-working? >> > >> > Thanks, >> > Ryan. >> > >> > On 21 October 2015 at 14:16, Ryan Harkin <ryan.harkin@linaro.org> wrote: >> >> On 21 October 2015 at 13:50, Tom Rini <trini@konsulko.com> wrote: >> >>> On Wed, Oct 21, 2015 at 12:00:03PM +0100, Ryan Harkin wrote: >> >>>> On 20 October 2015 at 16:38, Tom Rini <trini@konsulko.com> wrote: >> >>>> >> >>>> > On Tue, Oct 20, 2015 at 08:05:40AM +0200, Linus Walleij wrote: >> >>>> > >> >>>> > > Only compile in PCIe support if the board really uses it. Provide >> >>>> > > a stub for the init function if e.g. FVP is being built. >> >>>> > > >> >>>> > > Cc: Liviu Dudau <Liviu.Dudau@foss.arm.com> >> >>>> > > Cc: Ryan Harkin <ryan.harkin@linaro.org> >> >>>> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> >>>> > > --- >> >>>> > > board/armltd/vexpress64/Makefile | 3 ++- >> >>>> > > board/armltd/vexpress64/pcie.c | 2 -- >> >>>> > > board/armltd/vexpress64/pcie.h | 4 ++++ >> >>>> > > 3 files changed, 6 insertions(+), 3 deletions(-) >> >>>> > > >> >>>> > > diff --git a/board/armltd/vexpress64/Makefile >> >>>> > b/board/armltd/vexpress64/Makefile >> >>>> > > index a35db401b684..b4391a71249a 100644 >> >>>> > > --- a/board/armltd/vexpress64/Makefile >> >>>> > > +++ b/board/armltd/vexpress64/Makefile >> >>>> > > @@ -5,4 +5,5 @@ >> >>>> > > # SPDX-License-Identifier: GPL-2.0+ >> >>>> > > # >> >>>> > > >> >>>> > > -obj-y := vexpress64.o pcie.o >> >>>> > > +obj-y := vexpress64.o >> >>>> > > +obj-$(CONFIG_TARGET_VEXPRESS64_JUNO) += pcie.o >> >>>> > > diff --git a/board/armltd/vexpress64/pcie.c >> >>>> > b/board/armltd/vexpress64/pcie.c >> >>>> > > index 7b999e8ef40b..311c4500e3ff 100644 >> >>>> > > --- a/board/armltd/vexpress64/pcie.c >> >>>> > > +++ b/board/armltd/vexpress64/pcie.c >> >>>> > > @@ -191,7 +191,5 @@ void xr3pci_init(void) >> >>>> > > >> >>>> > > void vexpress64_pcie_init(void) >> >>>> > > { >> >>>> > > -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO >> >>>> > > xr3pci_init(); >> >>>> > > -#endif >> >>>> > > } >> >>>> > > diff --git a/board/armltd/vexpress64/pcie.h >> >>>> > b/board/armltd/vexpress64/pcie.h >> >>>> > > index 14642f4f5c43..55b276d6af11 100644 >> >>>> > > --- a/board/armltd/vexpress64/pcie.h >> >>>> > > +++ b/board/armltd/vexpress64/pcie.h >> >>>> > > @@ -1,6 +1,10 @@ >> >>>> > > #ifndef __VEXPRESS64_PCIE_H__ >> >>>> > > #define __VEXPRESS64_PCIE_H__ >> >>>> > > >> >>>> > > +#ifdef CONFIG_TARGET_VEXPRESS64_JUNO >> >>>> > > void vexpress64_pcie_init(void); >> >>>> > > +#else >> >>>> > > +static inline void vexpress64_pcie_init(void) {} >> >>>> > > +#endif >> >>>> > > >> >>>> > > #endif /* __VEXPRESS64_PCIE_H__ */ >> >>>> > >> >>>> > Alright, so the versatile platform makes life fun at times. >> >>>> >> >>>> >> >>>> This comes back to the refactoring thread we had recently... >> >>>> >> >>>> >> >>>> >> >>>> > First, my >> >>>> > preference is for weak functions (and I _think_ the linker ends up being >> >>>> > smart enough about them to optimize things away, if not, arrg). Second, >> >>>> > the question I have is, is this xr3pci_init() bit really a Juno thing or >> >>>> > a baseboard thing (I assume no) >> >>>> >> >>>> >> >>>> It's sort-of the same thing on Juno. Juno has a baseboard and SoC in one >> >>>> build, unlike the previous 32-bit Versatile Express motherboard that takes >> >>>> core tiles with different cores on it. >> >>>> >> >>>> Juno R0 has the PCIe controller, but it doesn't work. So the code is safe >> >>>> to run at this level. Juno R1 has the controller and it works. >> >>>> >> >>>> or a going to be the same on the next >> >>>> > Cortex-Asomething module thing? >> >>>> >> >>>> >> >>>> Juno R2 will have the PCIe controller too and it should hopefully work. >> >>> >> >>> But it will also be the A52. Or no, the A72? Or can't say.. >> >>> >> >> >> >> If I knew the answer, "can't say" would probably be the official line. >> >> But I haven't been told of any plans to change the cores, so I am >> >> assuming the next Juno respin will be A57/A53 big.LITTLE. Just like >> >> we have now but with fixes. >> >> >> >> >> >>>> But this controller is not Cortex-Asomething related. Another vendor could >> >>>> put the same IP block on their board, of course. >> >>> >> >>> Right, but it wouldn't be under board/armltd/vexpress64/ either.. >> >>> >> >>>> FVP models don't have it and other ARM platforms won't necessarily have it. >> >>>> >> >>>> Does that help?! >> >>> >> >>> Yes, thanks. I think it's just a style thing then. We don't do a lot >> >>> of static inline nop functions, we do __weak functions in the main file >> >>> (and comment about what it should be doing in a real function) and then >> >>> provide the strong version in another file. So just the pcie.h part >> >>> needs changing then. >> >> >> >> I'm not familiar with how __weak functions work, but reading Tom's >> advice and grepping the code leads me to the diff below. Tom, is this >> what you were looking for? >> >> diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile >> index a35db40..b4391a7 100644 >> --- a/board/armltd/vexpress64/Makefile >> +++ b/board/armltd/vexpress64/Makefile >> @@ -5,4 +5,5 @@ >> # SPDX-License-Identifier: GPL-2.0+ >> # >> >> -obj-y := vexpress64.o pcie.o >> +obj-y := vexpress64.o >> +obj-$(CONFIG_TARGET_VEXPRESS64_JUNO) += pcie.o >> diff --git a/board/armltd/vexpress64/pcie.c b/board/armltd/vexpress64/pcie.c >> index 7b999e8..311c450 100644 >> --- a/board/armltd/vexpress64/pcie.c >> +++ b/board/armltd/vexpress64/pcie.c >> @@ -191,7 +191,5 @@ void xr3pci_init(void) >> >> void vexpress64_pcie_init(void) >> { >> -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO >> xr3pci_init(); >> -#endif >> } >> diff --git a/board/armltd/vexpress64/vexpress64.c >> b/board/armltd/vexpress64/vexpress64.c >> index f4e8084..09a3166 100644 >> --- a/board/armltd/vexpress64/vexpress64.c >> +++ b/board/armltd/vexpress64/vexpress64.c >> @@ -28,6 +28,8 @@ U_BOOT_DEVICE(vexpress_serials) = { >> .platdata = &serial_platdata, >> }; >> >> +__weak void vexpress64_pcie_init(void) {} >> + >> int board_init(void) >> { >> vexpress64_pcie_init(); > > Pretty much. checkpatch should probably warn that you didn't do > { > } > > and it would be good to have a comment about what the function is > expected to do but I suppose this is obvious enough by name. Thanks! > Thanks Tom. I updated it to this: /* This function gets replaced by platforms supporting PCIe. * The replacement function, eg. on Juno, initialises the PCIe bus. */ __weak void vexpress64_pcie_init(void) { } ... and checkpatch is happy. I'll await Linus' preference on how I submit the patch before sending out a new version. I'll put it in a v2 series with the other two patches I have pending for NOR support so they are all kept together in order.
On Mon, Nov 16, 2015 at 7:16 PM, Ryan Harkin <ryan.harkin@linaro.org> wrote: > Thanks Tom. I updated it to this: > > /* This function gets replaced by platforms supporting PCIe. > * The replacement function, eg. on Juno, initialises the PCIe bus. > */ > __weak void vexpress64_pcie_init(void) > { > } > > ... and checkpatch is happy. > > I'll await Linus' preference on how I submit the patch before sending > out a new version. I'll put it in a v2 series with the other two > patches I have pending for NOR support so they are all kept together > in order. Bah I can live with it but will never like it :) Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
diff --git a/board/armltd/vexpress64/Makefile b/board/armltd/vexpress64/Makefile index a35db40..b4391a7 100644 --- a/board/armltd/vexpress64/Makefile +++ b/board/armltd/vexpress64/Makefile @@ -5,4 +5,5 @@ # SPDX-License-Identifier: GPL-2.0+ # -obj-y := vexpress64.o pcie.o +obj-y := vexpress64.o +obj-$(CONFIG_TARGET_VEXPRESS64_JUNO) += pcie.o diff --git a/board/armltd/vexpress64/pcie.c b/board/armltd/vexpress64/pcie.c index 7b999e8..311c450 100644 --- a/board/armltd/vexpress64/pcie.c +++ b/board/armltd/vexpress64/pcie.c @@ -191,7 +191,5 @@ void xr3pci_init(void) void vexpress64_pcie_init(void) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init(); -#endif } diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c index f4e8084..09a3166 100644 --- a/board/armltd/vexpress64/vexpress64.c +++ b/board/armltd/vexpress64/vexpress64.c @@ -28,6 +28,8 @@ U_BOOT_DEVICE(vexpress_serials) = { .platdata = &serial_platdata, }; +__weak void vexpress64_pcie_init(void) {} + int board_init(void) { vexpress64_pcie_init();