[U-Boot] [PATCH] vexpress64: compile Juno PCIe conditionally

Tom Rini trini at konsulko.com
Mon Nov 16 19:03:59 CET 2015 

On Mon, Nov 16, 2015 at 09:46:55AM +0000, Ryan Harkin wrote:
> On 13 November 2015 at 13:39, Ryan Harkin <ryan.harkin at 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 at linaro.org> wrote:
> >> On 21 October 2015 at 13:50, Tom Rini <trini at 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 at 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 at foss.arm.com>
> >>>> > > Cc: Ryan Harkin <ryan.harkin at linaro.org>
> >>>> > > Signed-off-by: Linus Walleij <linus.walleij at 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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151116/c47659c5/attachment.sig>

More information about the U-Boot mailing list