Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
Hi Michael, On Fri, 29 Oct 2021 at 05:54, Michael Walle wrote: > > Am 2021-10-29 00:36, schrieb Simon Glass: > > On Wed, 27 Oct 2021 at 10:55, Michael Walle wrote: > >> > >> Please stop throwing every ad-hoc information in the device tree. Use > >> the > >> official bindings (or maybe some bindings which will get approved > >> soon). > > > > Can I suggest that your commit subject be a little more specific? > > Sure. Actually, it should have be a bit provocant; and the cover letter > subject doesn't really matter, does it? I suppose not and it is somewhat entertaining. > > > Perhaps 'Drop use of unwanted binding' ? It seems quite vague to me, > > like 'Fix a horrible bug' or 'Sort out all the problems' :-) Regards, SImon
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
Am 2021-10-29 00:36, schrieb Simon Glass: On Wed, 27 Oct 2021 at 10:55, Michael Walle wrote: Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon). Can I suggest that your commit subject be a little more specific? Sure. Actually, it should have be a bit provocant; and the cover letter subject doesn't really matter, does it? Perhaps 'Drop use of unwanted binding' ? It seems quite vague to me, like 'Fix a horrible bug' or 'Sort out all the problems' :-) -michael
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
Hi Michael, On Wed, 27 Oct 2021 at 10:55, Michael Walle wrote: > > Please stop throwing every ad-hoc information in the device tree. Use the > official bindings (or maybe some bindings which will get approved soon). Can I suggest that your commit subject be a little more specific? Perhaps 'Drop use of unwanted binding' ? It seems quite vague to me, like 'Fix a horrible bug' or 'Sort out all the problems' :-) Regards, Simon
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
On Thu, 28 Oct 2021 12:21:59 +0100, Michael Walle wrote: > > Am 2021-10-28 11:20, schrieb Bharat Gooty: > > On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier wrote: > > > For GIC V3, once the LPI tables are programmed, we can not update it, > > unless we do a reset. > > For the kexec kernel, where the reboot does not happen, in this case, > > during the new kernel boot, the new LPI tables address will not be > > updated. > > kexec.. this should have really gone into both the commit message _and_ > the kconfig menu. In fact, it is really just a workaround for the kexec > case. If I understand it correctly, the kernel is able to communicate > the reserved memory area, but only if you have EFI support. So, as a > workaround, the bootloader can pre-allocate the memory and put it in > the device tree, which is then passed from the old to the new kernel > and the reservation is preserved. Correct, Marc? See my reply to Bharat. Either you use EFI, or you reserve the memory and program the RDs. In either of these two cases, kexec will just work. > But all of this doesn't need any new device tree node. Exactly. The whole syscon stuff is a point hack that makes no sense, and which isn't parsed by any upstream SW. M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
On Thu, 28 Oct 2021 11:27:44 +0100, Bharat Gooty wrote: > > [1 ] > On Thu, Oct 28, 2021 at 3:19 PM Marc Zyngier wrote: > > > On Thu, 28 Oct 2021 10:20:53 +0100, > > Bharat Gooty wrote: > > > > > > For GIC V3, once the LPI tables are programmed, we can not update it, > > > unless we do a reset. > > > > Please tell me something I don't know... > > > > For GIC V3 , once the Locality specific peripheral interrupts (LPI) table > is programmed and enabled, unless the GIC is reset, we can not re-program > the LPI tables. For these reasons, reserve the memory and program the GIC > redistributor PROPBASER and LPI_PENDBASE registers. > If we do not program the LPI table in the bootloader, during the Linux > boot, Linux will allocate the LPI table memory. Assume you want to boot a > new kernel and do the kexec kernel. For the kexec kernel boot, Linux will > again allocate the LPI table memory. But writing to the GIC registers will > fail. As the LPI for the redistributors is already enabled by the previous > Linux kernel, unless we do GIC reset, we can not update the LPI > tables. This really was a rhetorical question. I am painfully aware of most of the GIC's "features", having dealt with it in Linux and at the architecture level for the past 10+ years. > > > > > For the kexec kernel, where the reboot does not happen, in this case, > > > during the new kernel boot, the new LPI tables address will not be > > updated. > > > > Since July 2018, the Linux kernel is perfectly able to deal with this > > without any extra support. It will communicate the reservation to the > > secondary kernel, and the secondary kernel will happily use this. > > > > Can you specify the kernel version and the GIC versions which were used? Any kernel containing this commit: commit 5e2c9f9a627772672accd80fa15359c0de6aa894 Author: Marc Zyngier Date: Fri Jul 27 16:23:18 2018 +0100 irqchip/gic-v3-its: Allow use of LPI tables in reserved memory If the LPI tables have been reserved with the EFI reservation mechanism, we assume that these tables are safe to use even when we find the redistributors to have LPIs enabled at boot time, meaning that kexec can now work with GICv3. You're welcome. Tested-by: Jeremy Linton Tested-by: Bhupesh Sharma Tested-by: Lei Zhang Signed-off-by: Marc Zyngier And the version of the IP doesn't matter at all. > > > > For these reasons, We allocate the LPI table memory in u-boot and > > > reserve that memory. > > > > What sort of antiquated kernel are you using? And even if you are > > running something that predates the kernel changes, reserving the > > memory in the DT serves the same purpose. Why the custom node > > declaring the allocation? > > > > Tried using LTS 5.4 and 5.9 Linux kernels. Problem is updating the GIC V3 > registers with the new LPI table memory after enabling the LPI for the > redistributors. Then you are doing something wrong. There are two cases that work: (1) You are using EFI: nothing to do. The kernel will reserve the memory, configure the RDs, and the secondary kernel will happily use the same memory, as the address is conveyed in an EFI-specific table, without attempting to reprogram the RDs (2) You are not using EFI, and you need to reserve the memory *using the appropriate DT reservation mechanism* as well as program the RDs. The kernel will detect the reservation, and will not attempt to reprogram the RDs. If you invent your own binding, use another reservation mechanism or otherwise, this will not work. u-boot shouldn't expose this syscon node, because it makes no sense at all, and no upstream software knows about it. This code must die. M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
On Thu, Oct 28, 2021 at 02:38:26PM +0100, Marc Zyngier wrote:
> On Thu, 28 Oct 2021 13:31:13 +0100,
> Tom Rini wrote:
> >
> > On Thu, Oct 28, 2021 at 10:01:34AM +0100, Marc Zyngier wrote:
> > > On Wed, 27 Oct 2021 17:54:52 +0100,
> > > Michael Walle wrote:
> > > >
> > > > Please stop throwing every ad-hoc information in the device tree. Use
> > > > the
> > > > official bindings (or maybe some bindings which will get approved soon).
> > > >
> > > > On the quest of syncing the device tree used in u-boot with the one
> > > > used in
> > > > linux, there is this nice piece:
> > > >
> > > > gic_lpi_base: syscon@0x8000 {
> > > > compatible = "gic-lpi-base";
> > > > reg = <0x0 0x8000 0x0 0x10>;
> > > > max-gic-redistributors = <2>;
> > > > };
> > > >
> > > > There is no offical binding for it. Also, the chances that there will be
> > > > one are virtually zero. We need to get rid of it. In fact, most
> > > > information
> > > > there are already known or can be deduced via the offical binding.
> > >
> > > It is not "virtually zero". It is *exactly* zero. This node only shows
> > > that the author didn't understand the nature of the problem, nor were
> > > they aware of the existing solution which has been around since July
> > > 2018. This solution doesn't require any update to the binding, only to
> > > reserve the memory.
> > >
> > > I really wish people would stop piling crap in u-boot, and that the
> > > u-boot maintainers would reach out to people familiar with the
> > > architecture before merging this sort of changes.
> >
> > I'd be happy to reach out to people if I knew who would be receptive to
> > spending some of their already I assume overload spare time looking in
> > to things. If you're volunteering for "GIC related things" I'd be happy
> > to CC you when patches come up. Thanks!
>
> Absolutely. It is far less painful for me to quickly eyeball a change
> and ask pointed questions on the spot, rather than having to reverse
> engineer a set of dubious changes months after they have been merged.
>
> I already provide similar "services" for EDK2, for example, so getting
> the odd u-boot patch in my k.org inbox isn't a big deal.
Will do, thanks!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
On Thu, 28 Oct 2021 13:31:13 +0100,
Tom Rini wrote:
>
> On Thu, Oct 28, 2021 at 10:01:34AM +0100, Marc Zyngier wrote:
> > On Wed, 27 Oct 2021 17:54:52 +0100,
> > Michael Walle wrote:
> > >
> > > Please stop throwing every ad-hoc information in the device tree. Use the
> > > official bindings (or maybe some bindings which will get approved soon).
> > >
> > > On the quest of syncing the device tree used in u-boot with the one used
> > > in
> > > linux, there is this nice piece:
> > >
> > > gic_lpi_base: syscon@0x8000 {
> > > compatible = "gic-lpi-base";
> > > reg = <0x0 0x8000 0x0 0x10>;
> > > max-gic-redistributors = <2>;
> > > };
> > >
> > > There is no offical binding for it. Also, the chances that there will be
> > > one are virtually zero. We need to get rid of it. In fact, most
> > > information
> > > there are already known or can be deduced via the offical binding.
> >
> > It is not "virtually zero". It is *exactly* zero. This node only shows
> > that the author didn't understand the nature of the problem, nor were
> > they aware of the existing solution which has been around since July
> > 2018. This solution doesn't require any update to the binding, only to
> > reserve the memory.
> >
> > I really wish people would stop piling crap in u-boot, and that the
> > u-boot maintainers would reach out to people familiar with the
> > architecture before merging this sort of changes.
>
> I'd be happy to reach out to people if I knew who would be receptive to
> spending some of their already I assume overload spare time looking in
> to things. If you're volunteering for "GIC related things" I'd be happy
> to CC you when patches come up. Thanks!
Absolutely. It is far less painful for me to quickly eyeball a change
and ask pointed questions on the spot, rather than having to reverse
engineer a set of dubious changes months after they have been merged.
I already provide similar "services" for EDK2, for example, so getting
the odd u-boot patch in my k.org inbox isn't a big deal.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
On Thu, Oct 28, 2021 at 10:01:34AM +0100, Marc Zyngier wrote:
> On Wed, 27 Oct 2021 17:54:52 +0100,
> Michael Walle wrote:
> >
> > Please stop throwing every ad-hoc information in the device tree. Use the
> > official bindings (or maybe some bindings which will get approved soon).
> >
> > On the quest of syncing the device tree used in u-boot with the one used in
> > linux, there is this nice piece:
> >
> > gic_lpi_base: syscon@0x8000 {
> > compatible = "gic-lpi-base";
> > reg = <0x0 0x8000 0x0 0x10>;
> > max-gic-redistributors = <2>;
> > };
> >
> > There is no offical binding for it. Also, the chances that there will be
> > one are virtually zero. We need to get rid of it. In fact, most information
> > there are already known or can be deduced via the offical binding.
>
> It is not "virtually zero". It is *exactly* zero. This node only shows
> that the author didn't understand the nature of the problem, nor were
> they aware of the existing solution which has been around since July
> 2018. This solution doesn't require any update to the binding, only to
> reserve the memory.
>
> I really wish people would stop piling crap in u-boot, and that the
> u-boot maintainers would reach out to people familiar with the
> architecture before merging this sort of changes.
I'd be happy to reach out to people if I knew who would be receptive to
spending some of their already I assume overload spare time looking in
to things. If you're volunteering for "GIC related things" I'd be happy
to CC you when patches come up. Thanks!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
Am 2021-10-28 13:35, schrieb Bharat Gooty: On Thu, Oct 28, 2021 at 4:52 PM Michael Walle wrote: Am 2021-10-28 11:20, schrieb Bharat Gooty: On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier wrote: For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset. For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be updated. kexec.. this should have really gone into both the commit message _and_ the kconfig menu. In fact, it is really just a workaround for the kexec case. If I understand it correctly, the kernel is able to communicate the reserved memory area, but only if you have EFI support. So, as a workaround, the bootloader can pre-allocate the memory and put it in the device tree, which is then passed from the old to the new kernel and the reservation is preserved. Correct, Marc? If EFI support is enabled, that's true, Pre-allocate the memory and Kernel can get that memory using EFI. What if EFI support is not enabled, like in a Broadcom NS3 or NXP platform? What is your suggestion for solving the kexec problem? Iff that is correct what I've said above, then (1) rename the config symbol (I'm not sure, Tom?) and provide a better help text (2) drop the device tree node. after all you only have to allocate the node (3) keep most of the current code, but instead of reading the address from the device tree. Just allocate memory (within the alignment restrictions or whatever) and mark it as reserve it in the device tree. If I understood Marc correct, you can read the number of redistributors from the current gic-v3 binding. Now, how and if this will fit into the u-boot device model, that's up to you. In the meantime, it would be great if you can have a look at these two patches and trying to get them work for the ns3, so I can move forward with the device tree sync. -michael
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
Am 2021-10-28 11:20, schrieb Bharat Gooty: On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier wrote: For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset. For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be updated. kexec.. this should have really gone into both the commit message _and_ the kconfig menu. In fact, it is really just a workaround for the kexec case. If I understand it correctly, the kernel is able to communicate the reserved memory area, but only if you have EFI support. So, as a workaround, the bootloader can pre-allocate the memory and put it in the device tree, which is then passed from the old to the new kernel and the reservation is preserved. Correct, Marc? But all of this doesn't need any new device tree node. -michael
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
On Thu, 28 Oct 2021 10:20:53 +0100, Bharat Gooty wrote: > > For GIC V3, once the LPI tables are programmed, we can not update it, > unless we do a reset. Please tell me something I don't know... > For the kexec kernel, where the reboot does not happen, in this case, > during the new kernel boot, the new LPI tables address will not be updated. Since July 2018, the Linux kernel is perfectly able to deal with this without any extra support. It will communicate the reservation to the secondary kernel, and the secondary kernel will happily use this. > For these reasons, We allocate the LPI table memory in u-boot and > reserve that memory. What sort of antiquated kernel are you using? And even if you are running something that predates the kernel changes, reserving the memory in the DT serves the same purpose. Why the custom node declaring the allocation? And since your kernel is able to kexec, it obviously knows about the reservation/pre-programming trick. This hardly makes any sense. M. -- Without deviation from the norm, progress is not possible.
Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
On Wed, 27 Oct 2021 17:54:52 +0100,
Michael Walle wrote:
>
> Please stop throwing every ad-hoc information in the device tree. Use the
> official bindings (or maybe some bindings which will get approved soon).
>
> On the quest of syncing the device tree used in u-boot with the one used in
> linux, there is this nice piece:
>
> gic_lpi_base: syscon@0x8000 {
> compatible = "gic-lpi-base";
> reg = <0x0 0x8000 0x0 0x10>;
> max-gic-redistributors = <2>;
> };
>
> There is no offical binding for it. Also, the chances that there will be
> one are virtually zero. We need to get rid of it. In fact, most information
> there are already known or can be deduced via the offical binding.
It is not "virtually zero". It is *exactly* zero. This node only shows
that the author didn't understand the nature of the problem, nor were
they aware of the existing solution which has been around since July
2018. This solution doesn't require any update to the binding, only to
reserve the memory.
I really wish people would stop piling crap in u-boot, and that the
u-boot maintainers would reach out to people familiar with the
architecture before merging this sort of changes.
>
> More than a month ago NXP [1] said they will look into it and try to get
> the bindings together. I don't think this will happen. Actually, I don't
> think the culprit is that commit, but rather the one which introduced that
> broken binding in the first place [2]. Therefore, revert it, too.
>
> The funny thing is, I don't even know why this is needed at all. Linux will
> happily setup the LPI for us. At least for the LS1028A (but I guess also
> for all other layerscape SoC) the u-boot LPI setup code is only called
> right before we jump to linux. So u-boot doesn't even seem to use the
> interrupts. Now I can't speak of the Broadcom NS3 SoC where this 'feature'
> was introduced in the first place [3]. Unfortunately, it's not mentioned in
> the commit log *why* it was introduced. But this also seem to have crept
> into the layerscape SoCs [4]. All layerscape boards have CONFIG_GIC_V3_ITS
> enabled, well except for one; mine, the kontron_sl28 (which has a LS1028A).
> I haven't noticed anything out of the ordinary, though. Why is
> CONFIG_GIC_V3_ITS needed?
Now, that's a very interesting question: u-boot doesn't know how to
drive an ITS (there is no support code for it, despite what
arch/arm/lib/gic-v3-its.c suggest). Only the LPI allocation code is
there. For what purpose? This is a pretty useless piece of code as far
as I can see, unless the author had some unspecified ulterior motives
(in which case a bit of documentation and a renaming of this file
wouldn't go amiss).
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
[PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
Please stop throwing every ad-hoc information in the device tree. Use the
official bindings (or maybe some bindings which will get approved soon).
On the quest of syncing the device tree used in u-boot with the one used in
linux, there is this nice piece:
gic_lpi_base: syscon@0x8000 {
compatible = "gic-lpi-base";
reg = <0x0 0x8000 0x0 0x10>;
max-gic-redistributors = <2>;
};
There is no offical binding for it. Also, the chances that there will be
one are virtually zero. We need to get rid of it. In fact, most information
there are already known or can be deduced via the offical binding.
More than a month ago NXP [1] said they will look into it and try to get
the bindings together. I don't think this will happen. Actually, I don't
think the culprit is that commit, but rather the one which introduced that
broken binding in the first place [2]. Therefore, revert it, too.
The funny thing is, I don't even know why this is needed at all. Linux will
happily setup the LPI for us. At least for the LS1028A (but I guess also
for all other layerscape SoC) the u-boot LPI setup code is only called
right before we jump to linux. So u-boot doesn't even seem to use the
interrupts. Now I can't speak of the Broadcom NS3 SoC where this 'feature'
was introduced in the first place [3]. Unfortunately, it's not mentioned in
the commit log *why* it was introduced. But this also seem to have crept
into the layerscape SoCs [4]. All layerscape boards have CONFIG_GIC_V3_ITS
enabled, well except for one; mine, the kontron_sl28 (which has a LS1028A).
I haven't noticed anything out of the ordinary, though. Why is
CONFIG_GIC_V3_ITS needed?
I briefly tested this on my board with CONFIG_GIC_V3_ITS enabled, at least
linux prints:
[0.00] GICv3: Using preallocated redistributor tables
and there will be a reserved memory area:
reserved-memory {
#address-cells = <0x02>;
#size-cells = <0x02>;
ranges;
gic-rd-tables@20,fff0 {
reg = <0x20 0xfff0 0x00 0x10>;
};
};
Also please note, that the reverts needed some manual adjustments because
there were changes in the meantime.
While this will hopefully not break the layerscape boards, it will
definetly break the Broadcom NS3, because the gic_lpi_tables_init() is
called without arguments. But, these information are also not available in
u-boot't device tree for the ns3. S..
[1] https://lore.kernel.org/all/20210825210510.24766-1-tr...@konsulko.com/
[2]
https://lore.kernel.org/u-boot/20200726170733.30214-1-rayagonda.kokata...@broadcom.com/
[3]
https://lore.kernel.org/u-boot/20200610104120.30668-10-rayagonda.kokata...@broadcom.com/
[4] https://lore.kernel.org/u-boot/20200428021935.27659-1-zhiqiang@nxp.com/
Michael Walle (1):
Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Tom Rini (1):
Revert "arm64: Layerscape: Survive LPI one-way reset workaround"
arch/arm/Kconfig| 2 -
arch/arm/cpu/armv8/fsl-layerscape/soc.c | 41 +--
arch/arm/dts/fsl-ls1028a.dtsi | 6 ---
arch/arm/dts/fsl-ls1088a.dtsi | 6 ---
arch/arm/dts/fsl-ls2080a.dtsi | 6 ---
arch/arm/dts/fsl-lx2160a.dtsi | 6 ---
arch/arm/include/asm/gic-v3.h | 4 +-
arch/arm/lib/gic-v3-its.c | 66 +++--
8 files changed, 35 insertions(+), 102 deletions(-)
--
2.30.2
