From: Peter Geis <hidden> Date: 2021-04-13 15:04:21
On Tue, Apr 13, 2021 at 10:01 AM Marc Zyngier [off-list ref] wrote:
On Tue, 13 Apr 2021 14:29:32 +0100,
Peter Geis [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 5:23 AM Marc Zyngier [off-list ref] wrote:
quoted
Hi Peter,
On Mon, 12 Apr 2021 21:49:59 +0100,
Peter Geis [off-list ref] wrote:
quoted
Good Afternoon,
I am assisting with early bringup of the rk3566 based quartz64
development board for mainline linux.
I've encountered a few issues with allocating ITS on their version of
the GIC-V3.
The first issue is the ITS controller can only use 32bit addresses.
This leads to the following error:
[ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ 0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[ 0.000000] GICv3: 320 SPIs implemented
[ 0.000000] GICv3: 0 Extended SPIs implemented
[ 0.000000] GICv3: Distributor has no Range Selector support
[ 0.000000] Root IRQ handler: gic_handle_irq
[ 0.000000] GICv3: 16 PPIs implemented
[ 0.000000] GICv3: CPU0: found redistributor 0 region 0:0x00000000fd460000
[ 0.000000] ITS [mem 0xfd440000-0xfd45ffff]
[ 0.000000] ITS@0x00000000fd440000: Devices doesn't stick:
f907000100190600 f907000000190600
Ouch. That looks pretty bad. Bit 32 of the register doesn't stick, and
that's right in the middle of the address. The register should be
fully writable as far as the address field is concerned.
Please dump the distributor and ITS IIDR registers so that I can find
the TRM for the exact IP.
Yeah, I'm getting a crash course in how GIC-V3 sets up, so this is a
learning experience too.
The GICD_IIDR, GICR_IIDR, and GITS_IIDR are all 0x0201743B
Looks like our mate GIC600 r1p6, which is the most recent revision,
and reasonably bug free AFAIK. Other examples of this GIC integrated
on HW have access to seem to work rather well, and that's with (a lot)
more than 4GB of RAM.
quoted
quoted
quoted
[ 0.000000] ITS@0x00000000fd440000: failed probing (-6)
[ 0.000000] ITS: No ITS available, not enabling LPIs
Downstream fixed this by adding the GFP_DMA32 flag to the memory
allocations.
Urgh... this really looks like broken silicon to me.
I was afraid of that.
quoted
quoted
They also force clear the GITS_BASER_SHAREABILITY_MASK.
Why? Does this also apply to the command queue? Are they forcing cache
flushing?
The patch doesn't have a description, but the name of the function is
"force_inner_cache mode".
It disables shareability and caching, but I've experimented with
removing this part and it doesn't make a difference in upstream setup.
(Things are still broken in the same way). It ends up setting
ITS_FLAGS_CMDQ_NEEDS_FLUSHING as well.
quoted
quoted
Unfortunately while this allowed ITS to allocate on downstream, as
soon as MSIs attempted to use it all interrupts would time out.
On upstream, we observe this during allocation:
[ 0.000000] ITS [mem 0xfd440000-0xfd45ffff]
[ 0.000000] ITS@0x00000000fd440000: allocated 8192 Devices @3810000
(indirect, esz 8, psz 64K, shr 1)
[ 0.000000] ITS@0x00000000fd440000: allocated 32768 Interrupt
Collections @3820000 (flat, esz 2, psz 64K, shr 1)
[ 0.000000] GICv3: using LPI property table @0x0000000003830000
[ 0.000000] GICv3: CPU0: using allocated LPI pending table
@0x0000000003840000
[ 0.000000] ITS queue timeout (64 1)
[ 0.000000] ITS cmd its_build_mapc_cmd failed
[ 0.000000] ITS queue timeout (96 1)
[ 0.000000] ITS cmd its_build_invall_cmd failed
<snip>
So the command queue is not making forward progress. Either because
the ITS cannot access the commands, or because it cannot use the
memory it has been allocated. Please dump GITS_CBASER (and the value
that has been written to it), just in case it shows the same
brokenness as the GITS_BASER registers...
It seems yes, CBASER is broken in the same way:
Value written: b8000001001b040f
Value read: b8000000001b040f
Here you go. They haven't only half broken it. Please also check the
GICR_PENDBASER/GICR_PROPBASER values on each RD to see if they have
similar behaviours.
Yes, it seems all accesses are limited to 32bit.
What happens if you hack all the allocations to happen in the low 4GB
of the PA space?
It seems to work correctly.
The downstream hacks used GFP_DMA32 which gets discarded by
kmalloc_fix_flags on certain allocations.
Switching to GFP_DMA seems to have satisfied it, but it feels wrong
using this code.
Need to check the corner cases to make sure I'm not missing something.
quoted
quoted
[...]
quoted
Any assistance you can provide would be greatly appreciated.
I'm not sure there is much we can do without a lot more details about
the HW. We need to know the exact GIC implementation they are using
(ARM has two versions of the GICv3 IP), and we also need to find out
*how* this has been integrated. Only Rockchip can tell you that.
The rk3568 TRM says to reference the
"ARM_GIC-600_r1p6-00rel0_Technical_Reference_Manual.pdf" for
information on the GIC, if that helps narrow it down too.
Here you go:
https://documentation-service.arm.com/static/5e7ddddacbfe76649ba53034
That doesn't tell us what they did to it though, and whether only the
ITS is affected or the RDs are similarly brain-damaged. I guess that's
yet another piece of HW I will not have to spend any money on...
M.
--
Without deviation from the norm, progress is not possible.
From: Marc Zyngier <maz@kernel.org> Date: 2021-04-13 15:51:21
On Tue, 13 Apr 2021 16:03:51 +0100,
Peter Geis [off-list ref] wrote:
On Tue, Apr 13, 2021 at 10:01 AM Marc Zyngier [off-list ref] wrote:
[...]
quoted
What happens if you hack all the allocations to happen in the low 4GB
of the PA space?
It seems to work correctly.
The downstream hacks used GFP_DMA32 which gets discarded by
kmalloc_fix_flags on certain allocations.
Switching to GFP_DMA seems to have satisfied it, but it feels wrong
using this code.
Need to check the corner cases to make sure I'm not missing something.
The problem is that GFP_DMA doesn't always mean the same thing.
Overall, we need to hear from Rockchip about the exact nature of the
problem, and then we *may* be able to work something out.
I'd also like to understand whether it is broken because you happen to
have pre-release silicon that will never make it into the wild, or if
this is the real thing that is going to ship on millions of devices.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
From: Peter Geis <hidden> Date: 2021-04-14 11:41:41
On Tue, Apr 13, 2021 at 11:51 AM Marc Zyngier [off-list ref] wrote:
On Tue, 13 Apr 2021 16:03:51 +0100,
Peter Geis [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 10:01 AM Marc Zyngier [off-list ref] wrote:
[...]
quoted
quoted
What happens if you hack all the allocations to happen in the low 4GB
of the PA space?
It seems to work correctly.
The downstream hacks used GFP_DMA32 which gets discarded by
kmalloc_fix_flags on certain allocations.
Switching to GFP_DMA seems to have satisfied it, but it feels wrong
using this code.
Need to check the corner cases to make sure I'm not missing something.
The problem is that GFP_DMA doesn't always mean the same thing.
Overall, we need to hear from Rockchip about the exact nature of the
problem, and then we *may* be able to work something out.
From what I've read, GFP_DMA allocates as low as possible, while
GFP_DMA32 ensures it's in the 32 bit address range, am I understanding
this correctly?
Is there a reason GFP_DMA is permitted while GFP_DMA32 is not, aside
from backwards compatibility?
(I saw the notes about how we aren't really supposed to rely on these flags)
I've also confirmed that their disabling shareability and caching is necessary.
I'd also like to understand whether it is broken because you happen to
have pre-release silicon that will never make it into the wild, or if
this is the real thing that is going to ship on millions of devices.
My understanding is these chips are samples prior to the full
production run, but we are waiting on official comment from Rockchip
about this particular errata.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
From: Marc Zyngier <maz@kernel.org> Date: 2021-04-14 12:43:05
On Wed, 14 Apr 2021 12:41:20 +0100,
Peter Geis [off-list ref] wrote:
On Tue, Apr 13, 2021 at 11:51 AM Marc Zyngier [off-list ref] wrote:
quoted
On Tue, 13 Apr 2021 16:03:51 +0100,
Peter Geis [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 10:01 AM Marc Zyngier [off-list ref] wrote:
[...]
quoted
quoted
What happens if you hack all the allocations to happen in the low 4GB
of the PA space?
It seems to work correctly.
The downstream hacks used GFP_DMA32 which gets discarded by
kmalloc_fix_flags on certain allocations.
Switching to GFP_DMA seems to have satisfied it, but it feels wrong
using this code.
Need to check the corner cases to make sure I'm not missing something.
The problem is that GFP_DMA doesn't always mean the same thing.
Overall, we need to hear from Rockchip about the exact nature of the
problem, and then we *may* be able to work something out.
From what I've read, GFP_DMA allocates as low as possible, while
GFP_DMA32 ensures it's in the 32 bit address range, am I understanding
this correctly?
ZONE_DMA{,32} aren't necessarily selected, and can vary in size (some
equally broken systems can only DMA over 30bits...).
Is there a reason GFP_DMA is permitted while GFP_DMA32 is not, aside
from backwards compatibility? (I saw the notes about how we aren't
really supposed to rely on these flags)
They are completely independent, and they can either be selected or
not. And plenty of systems do not have any memory in the low
4GB. FWIW, one of my main machines has its first byte of RAM at 1TB.
Which means that supporting this system is going to require some very
specific handling.
I've also confirmed that their disabling shareability and caching is
necessary.
Confirmed how? For which tables? We really cannot guess this kind of
thing.
quoted
I'd also like to understand whether it is broken because you happen to
have pre-release silicon that will never make it into the wild, or if
this is the real thing that is going to ship on millions of devices.
My understanding is these chips are samples prior to the full
production run, but we are waiting on official comment from Rockchip
about this particular errata.
OK. Please let me know once you get a full description of the problem
from Rockchip. We will also need an official erratum number for this
if this is to be worked around in mainline.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
From: Kever Yang <hidden> Date: 2021-04-15 07:32:06
Hi Marc, Peter,
RK356x GIC has two issues:
1. GIC only support 32bit address while rk356x supports 8GB DDR SDRAM, so we use ZONE_DMA32 to fix this issue;
2. GIC version is r1p6-00rel0, RK356x interconnect does not support GIC and CPU snoop to each other, hence the GIC does not support the shareability feature. The read of register value for shareability feature does not return as expect in GICR and GITS, so we have to workaround for it.
Thanks,
- Kever
On 2021/4/14 下午8:42, Marc Zyngier wrote:
On Wed, 14 Apr 2021 12:41:20 +0100,
Peter Geis [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 11:51 AM Marc Zyngier [off-list ref] wrote:
quoted
On Tue, 13 Apr 2021 16:03:51 +0100,
Peter Geis [off-list ref] wrote:
quoted
On Tue, Apr 13, 2021 at 10:01 AM Marc Zyngier [off-list ref] wrote:
[...]
quoted
quoted
What happens if you hack all the allocations to happen in the low 4GB
of the PA space?
It seems to work correctly.
The downstream hacks used GFP_DMA32 which gets discarded by
kmalloc_fix_flags on certain allocations.
Switching to GFP_DMA seems to have satisfied it, but it feels wrong
using this code.
Need to check the corner cases to make sure I'm not missing something.
The problem is that GFP_DMA doesn't always mean the same thing.
Overall, we need to hear from Rockchip about the exact nature of the
problem, and then we *may* be able to work something out.
From what I've read, GFP_DMA allocates as low as possible, while
GFP_DMA32 ensures it's in the 32 bit address range, am I understanding
this correctly?
ZONE_DMA{,32} aren't necessarily selected, and can vary in size (some
equally broken systems can only DMA over 30bits...).
quoted
Is there a reason GFP_DMA is permitted while GFP_DMA32 is not, aside
from backwards compatibility? (I saw the notes about how we aren't
really supposed to rely on these flags)
They are completely independent, and they can either be selected or
not. And plenty of systems do not have any memory in the low
4GB. FWIW, one of my main machines has its first byte of RAM at 1TB.
Which means that supporting this system is going to require some very
specific handling.
quoted
I've also confirmed that their disabling shareability and caching is
necessary.
Confirmed how? For which tables? We really cannot guess this kind of
thing.
quoted
quoted
I'd also like to understand whether it is broken because you happen to
have pre-release silicon that will never make it into the wild, or if
this is the real thing that is going to ship on millions of devices.
My understanding is these chips are samples prior to the full
production run, but we are waiting on official comment from Rockchip
about this particular errata.
OK. Please let me know once you get a full description of the problem
from Rockchip. We will also need an official erratum number for this
if this is to be worked around in mainline.
M.
From: Marc Zyngier <maz@kernel.org> Date: 2021-04-15 08:11:27
Hi Kever,
On Thu, 15 Apr 2021 08:24:33 +0100,
Kever Yang [off-list ref] wrote:
Hi Marc, Peter,
RK356x GIC has two issues:
1. GIC only support 32bit address while rk356x supports 8GB DDR SDRAM,
so we use ZONE_DMA32 to fix this issue;
What transactions does this affect exactly? Only some ITS tables? Or
all of them, including the command queue? What about the configuration
and pending tables associated with the redistributors?
2. GIC version is r1p6-00rel0, RK356x interconnect does not support
GIC and CPU snoop to each other, hence the GIC does not support the
shareability feature. The read of register value for shareability
feature does not return as expect in GICR and GITS, so we have to
workaround for it.
How about the cacheability attribute? Can you please provide the exact
set of attributes that this system actually supports for each of the
ITS and redistributor base registers?
Also, please provide errata numbers for these two issues so that we
can properly document them and track the workarounds.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
From: Kever Yang <hidden> Date: 2021-04-16 01:14:03
Hi Marc,
On 2021/4/15 下午4:11, Marc Zyngier wrote:
Hi Kever,
On Thu, 15 Apr 2021 08:24:33 +0100,
Kever Yang [off-list ref] wrote:
quoted
Hi Marc, Peter,
RK356x GIC has two issues:
1. GIC only support 32bit address while rk356x supports 8GB DDR SDRAM,
so we use ZONE_DMA32 to fix this issue;
What transactions does this affect exactly?
The GIC on rk356x is a 32bit master, which means all the space its logic need to access should be in the 4GB range.
Only some ITS tables? Or
all of them, including the command queue? What about the configuration
and pending tables associated with the redistributors?
quoted
2. GIC version is r1p6-00rel0, RK356x interconnect does not support
GIC and CPU snoop to each other, hence the GIC does not support the
shareability feature. The read of register value for shareability
feature does not return as expect in GICR and GITS, so we have to
workaround for it.
How about the cacheability attribute? Can you please provide the exact
set of attributes that this system actually supports for each of the
ITS and redistributor base registers?
The shareability attributes in GICR_PENDBASEER, GICR_PROPBASER, GITS_BASERn, GITS_CBASER default value is 0b00, when we set 0b01 then read returns 0b01.
Since there is no ACE coherency interface for this GIC controller, all the cacheability in the GIC is not support in hardware.
Also, please provide errata numbers for these two issues so that we
can properly document them and track the workarounds.
What kind of errata do you need, could you please share any kind of example close to this case?
We consider this as a SoC implement design instead of a bug, so we will add document in RK356X TRM to describe the GIC design, but no idea how to provide the errata.
Here is the shareabily attribute from ARM GIC architecture specification:
Shareability, bits [11:10] (from GITS_CBASER)
Indicates the Shareability attributes of accesses to the command queue. The possible values of this field are:
0b00 Non-shareable.
0b01 Inner Shareable.
0b10 Outer Shareable.
0b11 Reserved. Treated as 0b00.
It is IMPLEMENTATION DEFINED whether this field has a fixed value or can be programmed by software. Implementing this field with a fixed value is deprecated.
On a Warm reset, this field resets to an architecturally UNKNOWN value
As you can see, "Implementing this field with a fixed value is deprecated", so software should program this field to '0b00 Non-shareable' if the SoC design does not support the cache shareability.
Thanks,
- Kever
From: Marc Zyngier <maz@kernel.org> Date: 2021-04-16 15:23:41
On Fri, 16 Apr 2021 02:13:38 +0100,
Kever Yang [off-list ref] wrote:
Hi Marc,
On 2021/4/15 下午4:11, Marc Zyngier wrote:
quoted
Hi Kever,
On Thu, 15 Apr 2021 08:24:33 +0100,
Kever Yang [off-list ref] wrote:
quoted
Hi Marc, Peter,
RK356x GIC has two issues:
1. GIC only support 32bit address while rk356x supports 8GB DDR SDRAM,
so we use ZONE_DMA32 to fix this issue;
What transactions does this affect exactly?
The GIC on rk356x is a 32bit master, which means all the space its
logic need to access should be in the 4GB range.
Well, at least this is consistently broken.
quoted
Only some ITS tables? Or
all of them, including the command queue? What about the configuration
and pending tables associated with the redistributors?
quoted
2. GIC version is r1p6-00rel0, RK356x interconnect does not support
GIC and CPU snoop to each other, hence the GIC does not support the
shareability feature. The read of register value for shareability
feature does not return as expect in GICR and GITS, so we have to
workaround for it.
How about the cacheability attribute? Can you please provide the exact
set of attributes that this system actually supports for each of the
ITS and redistributor base registers?
The shareability attributes in GICR_PENDBASEER, GICR_PROPBASER,
GITS_BASERn, GITS_CBASER default value is 0b00, when we set 0b01 then
read returns 0b01.
And I claim that this is a perfectly broken behaviour. How do you
expect software to find about the gory details of the integration?
That's the only way for SW to find out what the HW is capable of...
Since there is no ACE coherency interface for this GIC controller, all
the cacheability in the GIC is not support in hardware.
quoted
Also, please provide errata numbers for these two issues so that we
can properly document them and track the workarounds.
What kind of errata do you need, could you please share any kind of
example close to this case?
I would like something that says:
"ROCKCHIP_ERRATUM_123456: The GIC600 integration in RK356x doesn't
support any of the shareability or cacheability attributes, and
requires both values to be set to 0b00 for all the ITS and
Redistributor tables."
This is pretty similar to the bug affecting ThunderX with its "erratum
24313" (covered by CONFIG_CAVIUM_ERRATUM_22375), where the tables have
to be flagged as non-cacheable. The Rockchip one is just worse.
We need an official erratum number so that we can refer to it in the
source, commit log and documentation, as well as cross-reference it
with the TRM. This number will be part of a configuration symbol that
will make the compilation conditional so that people don't have to
carry the extra burden generated by this bug if they don't need to.
Same thing goes for the 32bit bug.
We consider this as a SoC implement design instead of a bug, so we
will add document in RK356X TRM to describe the GIC design, but no
idea how to provide the errata.
Here is the shareabily attribute from ARM GIC architecture specification:
Shareability, bits [11:10] (from GITS_CBASER)
Indicates the Shareability attributes of accesses to the command
queue. The possible values of this field are:
0b00 Non-shareable.
0b01 Inner Shareable.
0b10 Outer Shareable.
0b11 Reserved. Treated as 0b00.
It is IMPLEMENTATION DEFINED whether this field has a fixed value or
can be programmed by software. Implementing this field with a fixed
value is deprecated.
On a Warm reset, this field resets to an architecturally UNKNOWN value
As you can see, "Implementing this field with a fixed value is
deprecated", so software should program this field to '0b00
Non-shareable' if the SoC design does not support the cache
shareability.
[I really feel special when people quote the GIC spec at me]
That isn't what it says. Hardcoding the field with a fixed value is
indeed deprecated, but that doesn't mean this field should accept
values that the HW cannot support. If anything, what this says is "try
and implement the options that SW is going to use".
But you need to give SW an indication of what is usable, because there
is no other way to *discover* what the SoC is capable of at runtime.
Otherwise, we would need to carry a per-SoC list of what the HW
supports. I don't think that's the right thing to do (and you're about
8 years too late anyway).
Other GIC600 integrations got it perfectly right, by the way. Same for
other GIC implementations, with the notable exception of Cavium and
their first GIC in ThunderX, as described above.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
From: Kever Yang <hidden> Date: 2021-04-21 01:41:17
Hi Marc,
On 2021/4/16 下午11:23, Marc Zyngier wrote:
On Fri, 16 Apr 2021 02:13:38 +0100,
Kever Yang [off-list ref] wrote:
quoted
Hi Marc,
On 2021/4/15 下午4:11, Marc Zyngier wrote:
quoted
Hi Kever,
On Thu, 15 Apr 2021 08:24:33 +0100,
Kever Yang [off-list ref] wrote:
quoted
Hi Marc, Peter,
RK356x GIC has two issues:
1. GIC only support 32bit address while rk356x supports 8GB DDR SDRAM,
so we use ZONE_DMA32 to fix this issue;
What transactions does this affect exactly?
The GIC on rk356x is a 32bit master, which means all the space its
logic need to access should be in the 4GB range.
Well, at least this is consistently broken.
There are many legacy IPs which only support 32bit bus, we have to use them as is in the new
64bit SoCs, I think the 32bit GIC can be considered the same as those case, can we add CONFIG_ZONE_DMA32
support in GIC driver?
eg. Other peripheral driver use dma_alloc_coherent() instead of alloc_pages(), so then can support
ZONE_DMA32 without any code change.
quoted
quoted
Only some ITS tables? Or
all of them, including the command queue? What about the configuration
and pending tables associated with the redistributors?
quoted
2. GIC version is r1p6-00rel0, RK356x interconnect does not support
GIC and CPU snoop to each other, hence the GIC does not support the
shareability feature. The read of register value for shareability
feature does not return as expect in GICR and GITS, so we have to
workaround for it.
How about the cacheability attribute? Can you please provide the exact
set of attributes that this system actually supports for each of the
ITS and redistributor base registers?
The shareability attributes in GICR_PENDBASEER, GICR_PROPBASER,
GITS_BASERn, GITS_CBASER default value is 0b00, when we set 0b01 then
read returns 0b01.
And I claim that this is a perfectly broken behaviour. How do you
expect software to find about the gory details of the integration?
That's the only way for SW to find out what the HW is capable of...
As a software engineer, I totally agree with you on this point, but when we back to the truth,
we should know that these registers has never been defined as "module capability for shareability"
in hardware. The software use it as capability detect, and it works before, but not means this
match the original hardware design, in this case the new hardware may not follow the legacy
design because it's not a standard or a clear enough guide for it.
The capability register should always read-only instead of read-write, the understanding from
software engineer and from hardware engineer always have a gap.
I would proposal to add a optional dts property for driver to identify if the board's GIC support
shareability or not, which is a method used by many other module drivers.
quoted
Since there is no ACE coherency interface for this GIC controller, all
the cacheability in the GIC is not support in hardware.
quoted
Also, please provide errata numbers for these two issues so that we
can properly document them and track the workarounds.
What kind of errata do you need, could you please share any kind of
example close to this case?
I would like something that says:
"ROCKCHIP_ERRATUM_123456: The GIC600 integration in RK356x doesn't
support any of the shareability or cacheability attributes, and
requires both values to be set to 0b00 for all the ITS and
Redistributor tables."
This is pretty similar to the bug affecting ThunderX with its "erratum
24313" (covered by CONFIG_CAVIUM_ERRATUM_22375), where the tables have
to be flagged as non-cacheable. The Rockchip one is just worse.
We need an official erratum number so that we can refer to it in the
source, commit log and documentation, as well as cross-reference it
with the TRM. This number will be part of a configuration symbol that
will make the compilation conditional so that people don't have to
carry the extra burden generated by this bug if they don't need to.
Same thing goes for the 32bit bug.
quoted
We consider this as a SoC implement design instead of a bug, so we
will add document in RK356X TRM to describe the GIC design, but no
idea how to provide the errata.
Here is the shareabily attribute from ARM GIC architecture specification:
Shareability, bits [11:10] (from GITS_CBASER)
Indicates the Shareability attributes of accesses to the command
queue. The possible values of this field are:
0b00 Non-shareable.
0b01 Inner Shareable.
0b10 Outer Shareable.
0b11 Reserved. Treated as 0b00.
It is IMPLEMENTATION DEFINED whether this field has a fixed value or
can be programmed by software. Implementing this field with a fixed
value is deprecated.
On a Warm reset, this field resets to an architecturally UNKNOWN value
As you can see, "Implementing this field with a fixed value is
deprecated", so software should program this field to '0b00
Non-shareable' if the SoC design does not support the cache
shareability.
[I really feel special when people quote the GIC spec at me]
That isn't what it says. Hardcoding the field with a fixed value is
indeed deprecated, but that doesn't mean this field should accept
values that the HW cannot support. If anything, what this says is "try
and implement the options that SW is going to use".
But you need to give SW an indication of what is usable, because there
is no other way to *discover* what the SoC is capable of at runtime.
Otherwise, we would need to carry a per-SoC list of what the HW
supports. I don't think that's the right thing to do (and you're about
8 years too late anyway).
Other GIC600 integrations got it perfectly right, by the way. Same for
other GIC implementations, with the notable exception of Cavium and
their first GIC in ThunderX, as described above.
I'm not sure if you still work for ARM or not, but we do have double check again and again
with IP vendor about this point, the GIC500 works because it's hardware bind to a fix value inside the IP,
but GIC600 does not do the same binding, and there is no any config option to hardcode the reg field
to a fixed value in the IP when we don't need the ACE-lite.
This is why I insist that this is a different SoC implementation instead of a SoC bug, we should add the
support in the GIC driver for different hardware case.
When you say we're about 8 years too late, I think there still not much SoCs using GIC600, maybe the GIC600 IP
is available for user at mid of 2017?
Thanks,
- Kever
From: Marc Zyngier <maz@kernel.org> Date: 2021-04-21 10:23:38
On Wed, 21 Apr 2021 02:40:26 +0100,
Kever Yang [off-list ref] wrote:
Hi Marc,
On 2021/4/16 下午11:23, Marc Zyngier wrote:
quoted
On Fri, 16 Apr 2021 02:13:38 +0100,
Kever Yang [off-list ref] wrote:
quoted
Hi Marc,
On 2021/4/15 下午4:11, Marc Zyngier wrote:
quoted
Hi Kever,
On Thu, 15 Apr 2021 08:24:33 +0100,
Kever Yang [off-list ref] wrote:
quoted
Hi Marc, Peter,
RK356x GIC has two issues:
1. GIC only support 32bit address while rk356x supports 8GB DDR SDRAM,
so we use ZONE_DMA32 to fix this issue;
What transactions does this affect exactly?
The GIC on rk356x is a 32bit master, which means all the space its
logic need to access should be in the 4GB range.
Well, at least this is consistently broken.
There are many legacy IPs which only support 32bit bus, we have to
use them as is in the new 64bit SoCs, I think the 32bit GIC can be
considered the same as those case, can we add CONFIG_ZONE_DMA32
support in GIC driver?
Only as an erratum workaround. The GIC isn't some random device that
can live behind an IOMMU that will sort the mapping problem for
you. By design, it must be untranslated, so limiting it to only a
portion of the physical address space is a bug, full stop.
eg. Other peripheral driver use dma_alloc_coherent() instead of
alloc_pages(), so then can support ZONE_DMA32 without any code
change.
You do realise that dma_alloc_coherent() requires the use of a struct
device, and that the GIC is required to be up and running long before
the device model is available, right?
quoted
quoted
quoted
Only some ITS tables? Or
all of them, including the command queue? What about the configuration
and pending tables associated with the redistributors?
quoted
2. GIC version is r1p6-00rel0, RK356x interconnect does not support
GIC and CPU snoop to each other, hence the GIC does not support the
shareability feature. The read of register value for shareability
feature does not return as expect in GICR and GITS, so we have to
workaround for it.
How about the cacheability attribute? Can you please provide the exact
set of attributes that this system actually supports for each of the
ITS and redistributor base registers?
The shareability attributes in GICR_PENDBASEER, GICR_PROPBASER,
GITS_BASERn, GITS_CBASER default value is 0b00, when we set 0b01 then
read returns 0b01.
And I claim that this is a perfectly broken behaviour. How do you
expect software to find about the gory details of the integration?
That's the only way for SW to find out what the HW is capable of...
As a software engineer, I totally agree with you on this point, but
when we back to the truth, we should know that these registers has
never been defined as "module capability for shareability"
Says who? If SW cannot discover the capability of the HW, then nothing
works.
in hardware. The software use it as capability detect, and it works
before, but not means match the original hardware design, in this
case the new hardware may not follow the design because it's not a
standard or a clear enough guide for it.
The spec has been clear enough for *everyone* so far, and other GIC600
implementations got it perfectly right. There has been *one*
exception, which is documented and handled as a quirk.
The capability register should always read-only instead of
read-write, the understanding software engineer and from hardware
engineer always have a gap.
I would proposal to add a optional dts property for driver to
identify if the board's GIC shareability or not, which is a method
used by many other module drivers.
Are you also going to propose and support an ACPI specification update
to cope with similarly broken devices? Either this is something that
is available across the various firmware implementations, or it
doesn't exist.
quoted
quoted
Since there is no ACE coherency interface for this GIC controller, all
the cacheability in the GIC is not support in hardware.
quoted
Also, please provide errata numbers for these two issues so that we
can properly document them and track the workarounds.
What kind of errata do you need, could you please share any kind of
example close to this case?
I would like something that says:
"ROCKCHIP_ERRATUM_123456: The GIC600 integration in RK356x doesn't
support any of the shareability or cacheability attributes, and
requires both values to be set to 0b00 for all the ITS and
Redistributor tables."
This is pretty similar to the bug affecting ThunderX with its "erratum
24313" (covered by CONFIG_CAVIUM_ERRATUM_22375), where the tables have
to be flagged as non-cacheable. The Rockchip one is just worse.
We need an official erratum number so that we can refer to it in the
source, commit log and documentation, as well as cross-reference it
with the TRM. This number will be part of a configuration symbol that
will make the compilation conditional so that people don't have to
carry the extra burden generated by this bug if they don't need to.
Same thing goes for the 32bit bug.
quoted
We consider this as a SoC implement design instead of a bug, so we
will add document in RK356X TRM to describe the GIC design, but no
idea how to provide the errata.
Here is the shareabily attribute from ARM GIC architecture specification:
Shareability, bits [11:10] (from GITS_CBASER)
Indicates the Shareability attributes of accesses to the command
queue. The possible values of this field are:
0b00 Non-shareable.
0b01 Inner Shareable.
0b10 Outer Shareable.
0b11 Reserved. Treated as 0b00.
It is IMPLEMENTATION DEFINED whether this field has a fixed value or
can be programmed by software. Implementing this field with a fixed
value is deprecated.
On a Warm reset, this field resets to an architecturally UNKNOWN value
As you can see, "Implementing this field with a fixed value is
deprecated", so software should program this field to '0b00
Non-shareable' if the SoC design does not support the cache
shareability.
[I really feel special when people quote the GIC spec at me]
That isn't what it says. Hardcoding the field with a fixed value is
indeed deprecated, but that doesn't mean this field should accept
values that the HW cannot support. If anything, what this says is "try
and implement the options that SW is going to use".
But you need to give SW an indication of what is usable, because there
is no other way to *discover* what the SoC is capable of at runtime.
Otherwise, we would need to carry a per-SoC list of what the HW
supports. I don't think that's the right thing to do (and you're about
8 years too late anyway).
Other GIC600 integrations got it perfectly right, by the way. Same for
other GIC implementations, with the notable exception of Cavium and
their first GIC in ThunderX, as described above.
I'm not sure if you still work for ARM or not, but we do have double
I don't think whoever pays my salary is relevant in this context. I am
writing as the guy who maintains the GIC architecture in Linux, and
the fact that I have worked, work, or will work for ARM or not has no
bearing on what I am saying here.
check again and again with IP vendor about this point, the GIC500
works because it's hardware bind to a fix value inside the IP, but
GIC600 does not do the same binding, and there is no any config
option to hardcode the reg field to a fixed value in the IP when
we don't need the ACE-lite.
This is why I insist that this is a different SoC implementation
instead of a SoC bug, we should add the support in the GIC driver
for different hardware case.
When you say we're about 8 years too late, I think there still not
much SoCs using GIC600, maybe the GIC600 IP is available for user at
mid of 2017?
I'm not sure what the year 2017 does here. SW written as early as 2013
and compliant with the architecture still works on GIC600 when it is
properly integrated. Linux also has specific requirements, and you
have decided to ignore them. That's your choice. But you also get to
keep the pieces when things break.
Now, this discussion has lasted long enough, and isn't going anywhere.
If you want upstream support for your HW, I have explained what needs
to be done: document the two problems, implement the workarounds using
the GIC quirk infrastructure just like any other silicon vendor has
done over the past 8 years.
If you don't want to do it, it suits me just as well. I won't have to
maintain yet another set of SoC-specific workarounds.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip