| Message ID | CAD0U-hJsYuTU8eeBgF1gmDGRhyWo1GNog=mjNYRU=PtyaDc-ng@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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();