vexpress64: compile Juno PCIe conditionally
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
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.
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)
board/armltd/vexpress64/Makefile +2 -1
@@ -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
board/armltd/vexpress64/pcie.c +0 -2
@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }
board/armltd/vexpress64/pcie.h +4 -0
@@ -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__ */
Signature · 1 line
2.4.3
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 44 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)board/armltd/vexpress64/Makefile +2
-1@@ -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.oboard/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }board/armltd/vexpress64/pcie.h +0
-0
board/armltd/vexpress64/pcie.h line 10
Quoted 10 lines
board/armltd/vexpress64/pcie.h +4
-0@@ -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__ */
Tom specifically dropped this hunk when he merged Liviu's patch. But is it necessary with the rest of your patch? If pcie.h is only included in pcie.c, which is only built for JUNO, then the code shouldn't need conditional compilation. So I think we can drop the hunk, no?
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 57 lines · Ryan Harkin
Quoted 52 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)board/armltd/vexpress64/Makefile +2
-1@@ -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.oboard/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }board/armltd/vexpress64/pcie.h +4
-0@@ -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
Quoted 8 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 2 lines
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Tom specifically dropped this hunk when he merged Liviu's patch. But is it necessary with the rest of your patch? If
pcie.his only included inpcie.c, which is only built for JUNO, then the code shouldn't need conditional compilation. So I think we can drop the hunk, no?
Sorry, I'm wrong here. vexpress64_pcie_init is called unconditionally from board_init(), so needs a stub definition for non-Juno targets.
I guess another way to do it would be to make the call in board_init() conditional, but that's not really any different than your patch, so I'm happy enough with yours.
(I tested on FVP dram configuration, Juno R0 and Juno R1)
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
In reply to Linus WalleijQuoted 40 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)board/armltd/vexpress64/Makefile +2
-1@@ -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.oboard/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }board/armltd/vexpress64/pcie.h +0
-0
board/armltd/vexpress64/pcie.h line 10
Quoted 10 lines
board/armltd/vexpress64/pcie.h +4
-0@@ -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. 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) or a going to be the same on the next Cortex-Asomething module thing? The way the code stands today a hypothethical A72-based vexpress64 module would need to call vexpress64_pcie_init() but not strictly xr3pci_init(). It would be a little ugly as pcie.c would #if/#elif/#endif the enabler call however so we could __weak a no-op in board.c and rename pcie.c to juno.c and have a72codenameboard.c later on with its own function.
Signature · 8 lines
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/20151020/0a9f9a53/attachment.sig>
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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
Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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
Quoted 1 line
Cortex-Asomething module thing?
Juno R2 will have the PCIe controller too and it should hopefully work.
But this controller is not Cortex-Asomething related. Another vendor could put the same IP block on their board, of course.
FVP models don't have it and other ARM platforms won't necessarily have it.
Does that help?!
Quoted 3 lines
The way the code stands today a
hypothethical A72-based vexpress64 module would need to callvexpress64_pcie_init()
Hmmm, that doesn't seem right, does it?
but not strictly xr3pci_init(). It would be a
Quoted 13 lines
little ugly as
pcie.cwould #if/#elif/#endif the enabler call however so we could__weaka no-op inboard.cand renamepcie.ctojuno.cand havea72codenameboard.clater on with its own function.--
Tom_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 76 lines · Ryan Harkin
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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 Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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.
Quoted 8 lines
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
Quoted 1 line
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..
Quoted 2 lines
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..
Quoted 3 lines
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.
Signature · 8 lines
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/20151021/dd429aee/attachment.sig>
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 80 lines · Tom Rini
Quoted 79 lines · Ryan Harkin
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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 Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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.
Quoted 8 lines
Quoted 5 lines
or a going to be the same on the next
Quoted 1 line
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.
Quoted 7 lines
Quoted 2 lines
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..
Quoted 2 lines
FVP models don't have it and other ARM platforms won't necessarily have it.
Quoted 7 lines
Quoted 1 line
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
__weakfunctions 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 thepcie.hpart needs changing then.
Then it's over to Linus...
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Hi Linus,
Tom asked for a small change to your patch. Would mind having a look and re-working?
Thanks,
Ryan.
Quoted 112 lines · Ryan Harkin
Quoted 80 lines · Tom Rini
Quoted 79 lines · Ryan Harkin
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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 Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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.
Quoted 8 lines
Quoted 5 lines
or a going to be the same on the next
Quoted 1 line
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.
Quoted 7 lines
Quoted 2 lines
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..
Quoted 2 lines
FVP models don't have it and other ARM platforms won't necessarily have it.
Quoted 7 lines
Quoted 1 line
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
__weakfunctions 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 thepcie.hpart needs changing then.Then it's over to Linus...
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
In reply to Ryan Harkin[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.
Quoted 112 lines · Ryan Harkin
Quoted 80 lines · Tom Rini
Quoted 79 lines · Ryan Harkin
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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 Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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.
Quoted 8 lines
Quoted 5 lines
or a going to be the same on the next
Quoted 1 line
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.
Quoted 7 lines
Quoted 2 lines
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..
Quoted 2 lines
FVP models don't have it and other ARM platforms won't necessarily have it.
Quoted 7 lines
Quoted 1 line
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
__weakfunctions 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 thepcie.hpart needs changing then.Then it's over to Linus...
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 4 lines · Ryan Harkin
[trying again with Linus on the to: list]
Hi Linus,
Quoted 118 lines
Tom asked for a small change to your patch. Would mind having a look and re-working?
Thanks,
Ryan.Quoted 111 lines · Ryan Harkin
Quoted 80 lines · Tom Rini
Quoted 79 lines · Ryan Harkin
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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 Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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.
Quoted 8 lines
Quoted 5 lines
or a going to be the same on the next
Quoted 1 line
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.
Quoted 14 lines
Quoted 2 lines
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..
Quoted 3 lines
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
__weakfunctions 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 thepcie.hpart 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?
board/armltd/vexpress64/Makefile +2 -1
@@ -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
board/armltd/vexpress64/pcie.c +0 -2
@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }
(quoted diff) +0 -0
b/board/armltd/vexpress64/vexpress64.c
index f4e8084..09a3166 100644
board/armltd/vexpress64/vexpress64.c +2 -0
@@ -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 ();
Signature · 8 lines
Linus, you've indicated that you didn't want to make this change but are OK with me taking it over. In my tree, I've just squashed my patch into yours. Would you like me to remove your author and signed-off-by or would you prefer me to post a fixed up version of your patch? Cheers, Ryan.
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 157 lines · Ryan Harkin
Quoted 4 lines · Ryan Harkin
[trying again with Linus on the to: list]
Hi Linus,
Quoted 118 lines
Tom asked for a small change to your patch. Would mind having a look and re-working?
Thanks,
Ryan.Quoted 111 lines · Ryan Harkin
Quoted 80 lines · Tom Rini
Quoted 79 lines · Ryan Harkin
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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 Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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.
Quoted 8 lines
Quoted 5 lines
or a going to be the same on the next
Quoted 1 line
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.
Quoted 14 lines
Quoted 2 lines
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..
Quoted 3 lines
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
__weakfunctions 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 thepcie.hpart needs changing then.I'm not familiar with how
__weakfunctions work, but reading Tom's advice and grepping the code leads me to the diff below. Tom, is this what you were looking for?board/armltd/vexpress64/Makefile +2
-1@@ -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.oboard/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }board/armltd/vexpress64/vexpress64.c +0
-0
board/armltd/vexpress64/vexpress64.c U_BOOT_DEVICE(vexpress_serials) = { · line 35
Quoted 8 lines
board/armltd/vexpress64/vexpress64.c +2
-0@@ -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!
Signature · 8 lines
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>
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 171 lines · Tom Rini
Quoted 157 lines · Ryan Harkin
Quoted 4 lines · Ryan Harkin
[trying again with Linus on the to: list]
Hi Linus,
Quoted 118 lines
Tom asked for a small change to your patch. Would mind having a look and re-working?
Thanks,
Ryan.Quoted 111 lines · Ryan Harkin
Quoted 80 lines · Tom Rini
Quoted 79 lines · Ryan Harkin
Quoted 54 lines · Tom Rini
Quoted 6 lines · Linus Walleij
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 <[email protected]>Cc Ryan Harkin <[email protected]>Signed-off-by Linus Walleij <[email protected]>Review
Quoted 7 lines
--- board/armltd/vexpress64/Makefile | 3 ++- board/armltd/vexpress64/pcie.c | 2 -- board/armltd/vexpress64/pcie.h | 4 ++++ 3 files changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0b/board/armltd/vexpress64/Makefile
Review
Quoted 11 lines
index
a35db401b684..b4391a71249a100644board/armltd/vexpress64/Makefile +2
-1@@ -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(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.cReview
Quoted 12 lines
index
7b999e8ef40b..311c4500e3ff100644board/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }(quoted diff) +0
-0
b/board/armltd/vexpress64/pcie.hQuoted 13 lines
index
14642f4f5c43..55b276d6af11100644board/armltd/vexpress64/pcie.h +4
-0@@ -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 Quoted 3 lines
board/armltd/vexpress64/pcie.h line 10
Quoted 1 line
board/armltd/vexpress64/pcie.h +0
-0#endif /* __VEXPRESS64_PCIE_H__ */ Alright, so the versatile platform makes life fun at times.
This comes back to the refactoring thread we had recently...
Quoted 5 lines
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 thisxr3pci_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.
Quoted 8 lines
Quoted 5 lines
or a going to be the same on the next
Quoted 1 line
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.
Quoted 14 lines
Quoted 2 lines
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..
Quoted 3 lines
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
__weakfunctions 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 thepcie.hpart needs changing then.I'm not familiar with how
__weakfunctions work, but reading Tom's advice and grepping the code leads me to the diff below. Tom, is this what you were looking for?board/armltd/vexpress64/Makefile +2
-1@@ -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.oboard/armltd/vexpress64/pcie.c +0
-2@@ -191,7 +191,5 @@ void xr3pci_init(void)void vexpress64_pcie_init (void ) { -#ifdef CONFIG_TARGET_VEXPRESS64_JUNO xr3pci_init (); -#endif }board/armltd/vexpress64/vexpress64.c +0
-0board/armltd/vexpress64/vexpress64.c U_BOOT_DEVICE(vexpress_serials) = { · line 35
Quoted 8 lines
board/armltd/vexpress64/vexpress64.c +2
-0@@ -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 {
}
Quoted 3 lines
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.
PATCH 1/1 vexpress64: compile Juno PCIe conditionally
Quoted 9 lines · Ryan Harkin
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. */__weakvoidvexpress64_pcie_init(void)
{
}
Quoted 6 lines
... 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 :)
Yours,
Linus Walleij