Re: [PATCH 3/3] pinctrl: rockchip: add rk3308b SoC support

[Jonas Karlman <jonas@xxxxxxxxx>]

On 2024-05-28 10:17, Heiko Stübner wrote:
> Am Freitag, 17. Mai 2024, 08:58:32 CEST schrieb Luca Ceresoli:
>> Hello Dmitry,
>>
>> On Thu, 16 May 2024 17:06:46 +0500
>> Dmitry Yashin <dmt.yashin@xxxxxxxxx> wrote:
>>
>>> Hi Luca,
>>>
>>> On 15.05.24 21:29, Luca Ceresoli wrote:
>>>> I'm skeptical about this being bound to a new DT compatible. As far as I
>>>> know the RK3308 and RK3308B are mostly equivalent, so it looks as the
>>>> pinctrl implementation could be detected at runtime. This would let
>>>> products to be built with either chip version and work on any without
>>>> any DT change.  
>>>
>>>
>>> Thanks for your feedback.
>>>
>>> Indeed, these SoC's have a lot in common, but as I can see the rk3308b
>>> has more blocks, like extra PWM's (rk3308 datasheet 1.5 [0] shows only
>>> 1x PWM 4ch, when rk3308b and rk3308b-s have 3x PWM 4ch), 1-wire and
>>> CAN controller (mentioned in the TRM, but dropped from rk3308b
>>> datasheet for some reason).
>>>
>>> So, in my view, it really makes sense to add rk3308b.dtsi, where extra
>>> PWM's, pinctrl compatible and its pin functions can be moved. And if
>>> its not worth it, then I will try to adapt the entire series to runtime
>>> config based on cpuid like you suggested.
>>
>> Having a rk3308b.dtsi would probably make sense, yes, as there are
>> several differences as you described. However for the pinctrl it seems
>> probably not necessary.
>>
>> I've seen actual products being manufactured with two different RK3308
>> variants in different lots of production, but with the same DT that has
>> rockchip,rk3308-pinctrl in it. Those would need a _selective_ DT
>> upgrade in order to benefit from your changes.
>>
>> And even if a product had always used the B variant, it would need DT
>> upgrade when upgrading to a kernel with your changes. Otherwise with
>> patch 1/3 of this series the pictrl driver would lose many routes after
>> upgrading the kernel (but not the DT): can this lead to
>> previously-working devices to stop working? I think this is a
>> fundamental question to reply.
> 
> If things can be runtime-detectable, they should be detected at runtime.
> So yes, while we need to know that it is a rk3308-something before
> via the dt, if we can distinguish between the rk3308 variants at runtime
> we should definitly do so.

The GRF_CHIP_ID reg (0xFF000800) can be used to identify what model is
used at runtime:

RK3308: 0xCEA (errata: chip id value is 32'h0cea (32'd3306))
RK3308B: 0x3308
RK3308BS: 0x3308C

Vendor U-Boot make use of this reg to identify what model is running:
https://github.com/rockchip-linux/u-boot/blob/next-dev/arch/arm/include/asm/arch-rockchip/cpu.h#L68-L82

I can only validate on real hw that the reg value is 0x3308 for RK3308B.

Regards,
Jonas

> 
> Heiko
>