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

[Ryan Harkin]

On 16 November 2015 at 18:03, Tom Rini <trini at 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 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!
>

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.