Re: [PATCH 12/12] arm64: dts: exynos: Add Exynos850 SoC support

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx>]

On 04/08/2021 23:30, Sam Protsenko wrote:
>>>
>>> Nice catch! Actually there is an error (typo?) in SoC's TRM, saying
>>> that Virtual Interface Control Register starts at 0x3000 offset (from
>>> 0x12a00000), where it obviously should be 0x4000, that's probably
>>> where this dts error originates from. Btw, I'm also seeing the same
>>> error in exynos7.dtsi.
>>
>> What's the error exactly? The "Virtual interface control register"
>> offset (3rd region) is set properly to 0x4000 on Exynos7. Also one for
>> the Exynos5433 looks correct.
>>
> 
> The issue is that 2nd region's size is 0x1000, but it must be 0x2000.
> It's defined by GIC-400 architecture, as I understand. Please look at
> [1], table 3-1 has very specific offsets and sizes for each functional
> block, and each particular SoC must adhere to that spec. So having
> 0x1000 for 2nd region can't be correct. And because exynos7.dtsi has
> GIC-400 as well, and 0x1000 is specified there for 2nd region size
> too, so I presume there is the same mistake there.

I understand, the range length has indeed same mistake. However it does
not matter that much There are no registers pass 0x10C (so pass 0x1000).
This address space is not used.

> Can you please check the TRM for Exynos7 SoC (if you have one in your
> possession), and see if there is a typo there? E.g. in case of
> Exynos850 TRM I can see that in "Register Map Summary" section the
> offset for the first register (GICH_HCR) in "Virtual Interface Control
> Register" region is specified as 0x3000, where it should be 0x4000, so
> it's probably a typo. But the register description is correct, saying
> that: "Address = Base Address + 0x4000".

The starting addresses of each registers range is different issue and
this one matters. Except same typo as you say, all looks good - they
start at 0x4000.

> 
> [1] https://developer.arm.com/documentation/ddi0471/b/programmers-model/gic-400-register-map

> 
>>> Though I don't have a TRM for Exynos7 SoCs, so
>>> not sure if I should go ahead and fix that too. Anyway, for Exynos850,
>>> I'll fix that in v2 series.
>>
>>
>> However while we are at addresses - why are you using address-cells 2?
>> It adds everywhere additional 0x0 before actual address.
>>
> 
> Right. For "cpus" node I'll change the address-cells to 1 in my v2
> series. I'll keep address-cells=2 for the root node, but I'm going to
> encapsulate some nodes into soc node (as you suggested earlier), where
> I'll make address-cells=1. That's pretty much how it's done in
> exynos7.dtsi and in exynos5433.dtsi, so I guess that's should be fine
> (to get rid of superfluous 0x0 and conform with other Exynos DTs)?

Yes, thanks.


Best regards,
Krzysztof