Message-ID: <1cf00dfe-c987-46ee-9cdf-a9ba243740ad@cherry.de>
Date: Tue, 27 May 2025 11:26:49 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Quentin Schulz <foss+kernel@...il.net>, Lee Jones <lee@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Lukasz Czechowski <lukasz.czechowski@...umatec.com>,
 Daniel Semkowicz <dse@...umatec.com>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] dt-bindings: mfd: rk806: allow to customize PMIC
 reset method

Hi Krzysztof,

On 5/27/25 11:08 AM, Krzysztof Kozlowski wrote:
> On 27/05/2025 10:48, Quentin Schulz wrote:
>> Hi Krzysztof,
>>
>> On 5/27/25 10:25 AM, Krzysztof Kozlowski wrote:
>>> On 26/05/2025 19:05, Quentin Schulz wrote:
>>>> From: Quentin Schulz <quentin.schulz@...rry.de>
>>>>
>>>> The RK806 PMIC (and RK809, RK817; but those aren't handled here) has a
>>>> bitfield for configuring the restart/reset behavior (which I assume
>>>> Rockchip calls "function") whenever the PMIC is reset (at least by
>>>> software; c.f. DEV_RST in the datasheet).
>>>>
>>>> For RK806, the following values are possible for RST_FUN:
>>>>
>>>> 0b00 means "restart PMU"
>>>> 0b01 means "Reset all the power off reset registers, forcing
>>>>               the state to switch to ACTIVE mode"
>>>> 0b10 means "Reset all the power off reset registers, forcing
>>>>               the state to switch to ACTIVE mode, and simultaneously
>>>>               pull down the RESETB PIN for 5mS before releasing"
>>>> 0b11 means the same as for 0b10 just above.
>>>>
>>>> I don't believe this is suitable for a subsystem-generic property hence
>>>> let's make it a vendor property called rockchip,rst-fun.
>>>>
>>>> The first few sentences in the description of the property are
>>>> voluntarily generic so they could be copied to the DT binding for
>>>> RK809/RK817 whenever someone wants to implement that for those PMIC.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@...rry.de>
>>>> ---
>>>>    .../devicetree/bindings/mfd/rockchip,rk806.yaml    | 24 ++++++++++++++++++++++
>>>>    1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml
>>>> index 3c2b06629b75ea94f90712470bf14ed7fc16d68d..0f931a6da93f7596eac89c5f0deb8ee3bd934c31 100644
>>>> --- a/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml
>>>> +++ b/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml
>>>> @@ -31,6 +31,30 @@ properties:
>>>>    
>>>>      system-power-controller: true
>>>>    
>>>> +  rockchip,rst-fun:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [0, 1, 2, 3]
>>>> +    description:
>>>> +      RST_FUN value to set for the PMIC.
>>>> +
>>>> +      This is the value in the RST_FUN bitfield according to the
>>>> +      datasheet. I.e. if RST_FUN is bits 6 and 7 and the desired value
>>>> +      of RST_FUN is 1, this property needs to be set to 1 (and not 64,
>>>> +      0x40, or BIT(6)).
>>>> +
>>>> +      The meaning of this value is specific to the PMIC and is
>>>> +      explained in the datasheet.
>>>
>>> And why would that be exactly board-level configuration? IOW, I expect
>>> all boards to be reset in the same - correct and optimal - way. Looks
>>> close to SW policy.
>>>
>>
>> All RK3588 boards except ours in downstream kernel have their RST_FUN
>> set to 1, we need 0 and I cannot talk for what's the actual expected
>> behavior for other vendors' boards. I do not feel confident
>> indiscriminately changing the PMIC reset behavior for all boards using
>> RK806 (which also includes RK3576 boards). Hence why I made that a property.
>>
>> Additionally, if all boards were "to be reset in the same - correct and
> 
> I don't know if they have to, but that's what I would assume in general.
> Unless you say there is some specific hardware aspect of your boards,
> but so far you just described the register.
> 

The cover letter

>> optimal - way", why would Rockchip even have a register for that in the
>> PMIC? It's not an IP they bought (as far as I could tell), so there's
> 
> To allow SW to make a choice. Just like 1000 other registers for every
> other device which we do not add to DT.
> 
>> likely a purpose to it. Especially if they also change the
>> silicon-default in their own downstream fork AND provide you with a way
>> to change their new default from Device Tree.
>>
>> We can hardcode the change in the driver without using DT, but I wager
>> we're going to see a revert in a few releases because it broke some
>> devices. It may break in subtle ways as well, for example our boards
>> seem to be working just fine except that because the PMIC doesn't
>> entirely reset the power rails, our companion microcontroller doesn't
>> detect the reboot.
>>
>> If it's deemed a SW policy by the DT binding people, is there a way to
>> customize this without having it hardcoded to the same value for all
>> users of RK806 and without relying on module params?
> 
> sysfs, reboot mode etc. I don't know what is the right here, because you
> did not explain the actual hardware issue being fixed here. You only
> described that bootloader does something, so you want to write something
> else there.
> 

We have a companion microcontroller on the PCB of both products which 
needs to detect if the board was reset. When the board is reset, the MCU 
FW does a few things, like essentially resetting its internal logic such 
as the PWM controller (so the beeper stops beeping), watchdogs and 
reinit most user-exposed registers so that it's like "fresh" out of 
reset (even though it actually wasn't reset since it's continuously 
powered, not from the PMIC).

To detect a reboot, it senses one of the power rails (DCDC8; vcc_3v3_s3 
on our boards) from the PMIC. This power rail is only "restarted" when 
RST_FUN is set to 0 ("restart PMU" mode) according to our experiments.

I assume it is possible other boards do not want this (all?) power rail 
to be quickly interrupted when rebooting? But that I do not know.

Cheers,
Quentin

Powered by blists - more mailing lists