Re: [PATCH v1 1/4] irqchip/gic-v3: Add Rockchip 3568002 erratum workaround

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 2/16/25 12:55, Marc Zyngier wrote:
> On Sat, 15 Feb 2025 23:54:28 +0000,
> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
>>
>> Rockchip RK3566/RK3568 GIC600 integration has DDR addressing
>> limited to first 4GB of DRAM. Rockchip assigned Erratum ID #3568002
>> for this issue. Add driver quirk for this Rockchip GIC Erratum.
> 
> Thanks for taking the time to submit this. It only took 5 years for
> this erratum to be published...

The erratum document itself actually is dated by 5 years ago. Only wish
the doc was made publicly available, which would've accelerated the
upstreaming process.

> However, my understanding of this issue is that the integration is
> limited to the first 32bit of physical address space, not the first
> 32bit of RAM. If the memory is placed as physical address 0, then they
> represent the same space. But this is still an important distinction.

Indeed, will correct the description in v2.

>> Note, that the 0x0201743b ID is not Rockchip 356x specific and thus
>> there is an extra of_machine_is_compatible() check. Rockchip 3588 uses
>> same ID and it is not affected by this errata.
> 
> This ID is that of ARM's GIC600, which is a very common GICv3
> implementation, and is not Rockchip-specific. Please capture this in
> the commit message.

Ack

...
>> +config ROCKCHIP_ERRATUM_3568002
>> +	bool "Rockchip 3568002: can not support DDR addresses higher than 4G"
>> +	default y
>> +	help
>> +	  The Rockchip RK3566 and RK3568 GIC600 SoC integrations have DDR
>> +	  addressing limited to first 4GB.
>> +
>> +	  If unsure, say Y.
>> +
> 
> s/DDR addresses/physical addresses/

Ack

...
> Another thing is that this patch conflates ITS and redistributors. As
> it turns out, we use the same allocator for both, but they are
> distinct architectural concepts, even if GIC600 is a monolithic
> implementation. It is OK for now, but it will have to be revisited if
> we ever move the redistributor management outside of the ITS driver.
> 
> With the other comments addressed:
> 
> Acked-by: Marc Zyngier <maz@kernel.org>

Thanks for the review!

-- 
Best regards,
Dmitry