| Message ID | 20221121171239.2041835-2-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a6eeafba1141c48b0b38e96f1aa715de4035b6d5 |
| Headers | show |
| Series | [GIT,PULL,v2,1/7] soc/tegra: Changes for v6.2-rc1 | expand |
On Mon, Nov 21, 2022, at 18:12, Thierry Reding wrote:
> firmware: tegra: bpmp: Do not support big-endian
I pulled the branch, but I think this patch is inconsistent with
our normal approach: Since all ARMv7 and ARMv8 processors can
run with both big-endian and little-endian kernels, we normally
try to keep drivers portable between both ways, even though we
don't expect anyone to actually want a big-endian kernel any
more. Changing portable code to nonportable code doesn't seem
helpful here.
On the other hand, there are already examples of important
drivers that are fundamentally incompatible with big-endian
mode, notably drivers/efi/, which is required on a lot of
machines.
You don't have to revert this patch, but it would be helpful
to mark code that is explicitly unportable with a 'depends
on !CPU_BIG_ENDIAN' line in Kconfig. If you agree, I can
add that.
Do you know of other tegra drivers that only work on
little-endian?
Arnd
On Tue, Nov 22, 2022 at 10:25:50PM +0100, Arnd Bergmann wrote: > On Mon, Nov 21, 2022, at 18:12, Thierry Reding wrote: > > firmware: tegra: bpmp: Do not support big-endian > > I pulled the branch, but I think this patch is inconsistent with > our normal approach: Since all ARMv7 and ARMv8 processors can > run with both big-endian and little-endian kernels, we normally > try to keep drivers portable between both ways, even though we > don't expect anyone to actually want a big-endian kernel any > more. Changing portable code to nonportable code doesn't seem > helpful here. The only reason I dropped this is because the driver is in itself inconsistent. Parts of it use byte-swapping for 32-bit values and other parts don't. I was originally going to fix big-endian support but it would've required changes to the BPMP ABI header to avoid sparse warnings in lots of places, then these ABI changes would've needed to trickle up to the canonical source, etc. All of that didn't seem worth the effort if we couldn't even test this in any way. So the easiest fix was to stop pretending and drop the partial support. > On the other hand, there are already examples of important > drivers that are fundamentally incompatible with big-endian > mode, notably drivers/efi/, which is required on a lot of > machines. > > You don't have to revert this patch, but it would be helpful > to mark code that is explicitly unportable with a 'depends > on !CPU_BIG_ENDIAN' line in Kconfig. If you agree, I can > add that. Yes, feel free to add that. > Do you know of other tegra drivers that only work on > little-endian? I'm not aware of any that explicitly wouldn't work with big endian, but it's not something we've ever tested. I know that people have in the past done experiments with running emulated Tegra on QEMU in big endian mode, but it's probably not something that's very common. Thierry
On Wed, Nov 23, 2022, at 12:33, Thierry Reding wrote: > On Tue, Nov 22, 2022 at 10:25:50PM +0100, Arnd Bergmann wrote: >> On Mon, Nov 21, 2022, at 18:12, Thierry Reding wrote: >> > firmware: tegra: bpmp: Do not support big-endian >> >> I pulled the branch, but I think this patch is inconsistent with >> our normal approach: Since all ARMv7 and ARMv8 processors can >> run with both big-endian and little-endian kernels, we normally >> try to keep drivers portable between both ways, even though we >> don't expect anyone to actually want a big-endian kernel any >> more. Changing portable code to nonportable code doesn't seem >> helpful here. > > The only reason I dropped this is because the driver is in itself > inconsistent. Parts of it use byte-swapping for 32-bit values and other > parts don't. I was originally going to fix big-endian support but it > would've required changes to the BPMP ABI header to avoid sparse > warnings in lots of places, then these ABI changes would've needed to > trickle up to the canonical source, etc. All of that didn't seem worth > the effort if we couldn't even test this in any way. So the easiest fix > was to stop pretending and drop the partial support. Right >> On the other hand, there are already examples of important >> drivers that are fundamentally incompatible with big-endian >> mode, notably drivers/efi/, which is required on a lot of >> machines. >> >> You don't have to revert this patch, but it would be helpful >> to mark code that is explicitly unportable with a 'depends >> on !CPU_BIG_ENDIAN' line in Kconfig. If you agree, I can >> add that. > > Yes, feel free to add that. Added this commit to the soc/drivers branch now: commit 4ddb1bf1a83783cebdb174b0efaf62f63ad64e0b Author: Arnd Bergmann <arnd@arndb.de> Date: Wed Nov 23 14:21:16 2022 +0100 tegra: mark BPMP driver as little-endian only The BPMP firmware driver never worked on big-endian kernels, and cannot easily be made portable. Add a dependency to make this clear in case anyone ever wants to try a big-endian kernel on this hardware. Link: https://lore.kernel.org/linux-arm-kernel/Y34FCQ3xTmcjqKRT@orome/ Signed-off-by: Arnd Bergmann <arnd@arndb.de> diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig index 1c8ba1f47c7c..cde1ab8bd9d1 100644 --- a/drivers/firmware/tegra/Kconfig +++ b/drivers/firmware/tegra/Kconfig @@ -14,6 +14,7 @@ config TEGRA_IVC config TEGRA_BPMP bool "Tegra BPMP driver" depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC + depends on !CPU_BIG_ENDIAN help BPMP (Boot and Power Management Processor) is designed to off-loading the PM functions which include clock/DVFS/thermal/power from the CPU. >> Do you know of other tegra drivers that only work on >> little-endian? > > I'm not aware of any that explicitly wouldn't work with big endian, but > it's not something we've ever tested. I know that people have in the > past done experiments with running emulated Tegra on QEMU in big endian > mode, but it's probably not something that's very common. Ok, thanks! Arnd
On Wed, Nov 23, 2022 at 02:23:36PM +0100, Arnd Bergmann wrote: > On Wed, Nov 23, 2022, at 12:33, Thierry Reding wrote: > > On Tue, Nov 22, 2022 at 10:25:50PM +0100, Arnd Bergmann wrote: > >> On Mon, Nov 21, 2022, at 18:12, Thierry Reding wrote: > >> > firmware: tegra: bpmp: Do not support big-endian > >> > >> I pulled the branch, but I think this patch is inconsistent with > >> our normal approach: Since all ARMv7 and ARMv8 processors can > >> run with both big-endian and little-endian kernels, we normally > >> try to keep drivers portable between both ways, even though we > >> don't expect anyone to actually want a big-endian kernel any > >> more. Changing portable code to nonportable code doesn't seem > >> helpful here. > > > > The only reason I dropped this is because the driver is in itself > > inconsistent. Parts of it use byte-swapping for 32-bit values and other > > parts don't. I was originally going to fix big-endian support but it > > would've required changes to the BPMP ABI header to avoid sparse > > warnings in lots of places, then these ABI changes would've needed to > > trickle up to the canonical source, etc. All of that didn't seem worth > > the effort if we couldn't even test this in any way. So the easiest fix > > was to stop pretending and drop the partial support. > > Right > > >> On the other hand, there are already examples of important > >> drivers that are fundamentally incompatible with big-endian > >> mode, notably drivers/efi/, which is required on a lot of > >> machines. > >> > >> You don't have to revert this patch, but it would be helpful > >> to mark code that is explicitly unportable with a 'depends > >> on !CPU_BIG_ENDIAN' line in Kconfig. If you agree, I can > >> add that. > > > > Yes, feel free to add that. > > Added this commit to the soc/drivers branch now: > > commit 4ddb1bf1a83783cebdb174b0efaf62f63ad64e0b > Author: Arnd Bergmann <arnd@arndb.de> > Date: Wed Nov 23 14:21:16 2022 +0100 > > tegra: mark BPMP driver as little-endian only > > The BPMP firmware driver never worked on big-endian kernels, and > cannot easily be made portable. Add a dependency to make this clear > in case anyone ever wants to try a big-endian kernel on this hardware. > > Link: https://lore.kernel.org/linux-arm-kernel/Y34FCQ3xTmcjqKRT@orome/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > diff --git a/drivers/firmware/tegra/Kconfig b/drivers/firmware/tegra/Kconfig > index 1c8ba1f47c7c..cde1ab8bd9d1 100644 > --- a/drivers/firmware/tegra/Kconfig > +++ b/drivers/firmware/tegra/Kconfig > @@ -14,6 +14,7 @@ config TEGRA_IVC > config TEGRA_BPMP > bool "Tegra BPMP driver" > depends on ARCH_TEGRA && TEGRA_HSP_MBOX && TEGRA_IVC > + depends on !CPU_BIG_ENDIAN > help > BPMP (Boot and Power Management Processor) is designed to off-loading > the PM functions which include clock/DVFS/thermal/power from the CPU. Sounds good, thanks. Thierry
Hi ARM SoC maintainers, The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780: Linux 6.1-rc1 (2022-10-16 15:36:24 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git tags/tegra-for-6.2-firmware-v2 for you to fetch changes up to 198d4649b0b813bc9fc1605cfb843b6624518f92: firmware: tegra: Remove surplus dev_err() when using platform_get_irq_byname() (2022-11-17 23:52:39 +0100) Thanks, Thierry ---------------------------------------------------------------- firmware: tegra: Changes for v6.2-rc1 This adds new BPMP ABI so that newer features can be enabled. Furthermore, the BPMP driver is updated to use iosys-map helpers to allow working with shared memory regions that are located in system memory. Apart from that, several minor cleanups are included. ---------------------------------------------------------------- Peter De Schrijver (1): firmware: tegra: Update BPMP ABI Thierry Reding (3): firmware: tegra: bpmp: Prefer u32 over uint32_t firmware: tegra: bpmp: Use iosys-map helpers firmware: tegra: bpmp: Do not support big-endian Yang Li (1): firmware: tegra: Remove surplus dev_err() when using platform_get_irq_byname() drivers/firmware/tegra/bpmp-debugfs.c | 62 +- drivers/firmware/tegra/bpmp-tegra186.c | 36 +- drivers/firmware/tegra/bpmp-tegra210.c | 15 +- drivers/firmware/tegra/bpmp.c | 33 +- drivers/firmware/tegra/ivc.c | 150 ++- drivers/thermal/tegra/tegra-bpmp-thermal.c | 15 +- include/soc/tegra/bpmp-abi.h | 1796 +++++++++++++++++++++------- include/soc/tegra/bpmp.h | 17 +- include/soc/tegra/ivc.h | 11 +- 9 files changed, 1545 insertions(+), 590 deletions(-)