[PATCH 05/11] rockchip: io-domain: Add support for RK3399

[Quentin Schulz]

Hi Jonas,

On 2/21/24 20:05, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 2024-02-21 19:18, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 2/17/24 19:35, Jonas Karlman wrote:
>>> Port the RK3399 part of the Rockchip IO-domain driver from linux.
>>>
>>> This differs from linux version in that pmu io iodomain bit is enabled
>>> in the write ops instead of in an init ops as in linux, this way we can
>>> avoid keeping a full state of all supply that have been configured.
>>>
>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>
>> Suggestions below, otherwise I only have one concern:
>>
>> This actually postpones the moment the IO domain is properly set.
>> Before, we knew almost for sure when this was set, as it was pretty
>> early in the boot process (misc_init_r), I hope before any device was
>> actually probed. Now, because we're using the Device Model for that, we
>> may probe this IO domain device after another Device requires it? e.g.
>> for Puma, we need the IO domains set for PCIe to work. I assume if we
>> were to use PCIe in U-Boot on Puma, we may have this issue (we don't
>> right now use PCIe in U-Boot and I don't think we have any plan of doing
>> so).
> 
> Because this driver makes the following call in its bind ops, the
> opposite is actually true and the IO domain is configured earlier than
> it was previously done with misc_init_r.
> 
> With this the IO domain driver is probed directly after bind and
> all regulators that is referenced by io-domain supply nodes will be
> regulator_autoset() before the uV value is evaluated.
> 
> In rockchip_iodomain_bind():
> 
>    dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> 
> On the boards I can test on the GRF got configured exactly as before,
> just a little bit earlier in the boot process.
> 
> Prior to the "rockchip: Port IO-domain driver for RK3568 from linux" [1]
> series the rk8xx driver had some issues and incorrect voltage could be
> read for some regulators.
> 
> [1] https://patchwork.ozlabs.org/cover/1823769/
> 

Well, maybe I should read the source code before raising a potential 
issue :) Thanks for the correction.

So in short:

common/board_r.c:init_sequence_r calls initr_dm before misc_init_r.

initr_dm -> dm_init_and_scan -> dm_scan -> dm_extended_scan which binds 
the device and because the IO domain driver sets 
DM_FLAG_PROBE_AFTER_BIND in its bind, dm_probe_devices at the end of 
dm_scan will probe the device all that before misc_init_r is called.

>>
>> This all reminds me of the attempts we've made in the Linux kernel to
>> make sure the IO domain devices are probed before the devices that
>> require them to be set up. Maybe we'll need something similar in U-Boot
>> as well.
> 
> The main thing missing compared to the linux counterpart is that changes
> to regulator voltage is not tracked, so there will not be any changes
> made the initial IO domain voltage configuration made by this driver.
> 

There's a common issue for Linux and U-boot parts is that devices 
dependent on the IO domain being properly configured is not properly 
handled. Here in U-Boot, if there are devices which also have 
DM_FLAG_PROBE_AFTER_BIND set, then I think this is dependent on the 
order in udevice->sibling_node/child_head.

In any case, nothing that is specific to this patch series because it 
was already the case before.

Therefore,

Reviewed-by: Quentin Schulz <quentin.schulz at theobroma-systems.com>

Thanks,
Quentin