[PATCH v2 1/7] clk/qcom: add initial clock driver for sc7280

[Caleb Connolly]

On 21/08/2024 18:16, Simon Glass wrote:
> Hi Caleb,
> 
> On Wed, 21 Aug 2024 at 08:49, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>
>>
>>
>> On 21/08/2024 16:37, Simon Glass wrote:
>>> Hi Caleb,
>>>
>>> On Wed, 21 Aug 2024 at 08:11, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>>>
>>>> Hi Simon,
>>>>>> +U_BOOT_DRIVER(gcc_sc7280) = {
>>>>>> +       .name           = "gcc_sc7280",
>>>>>> +       .id             = UCLASS_NOP,
>>>>>> +       .of_match       = gcc_sc7280_of_match,
>>>>>> +       .bind           = qcom_cc_bind,
>>>>>> +       .flags          = DM_FLAG_PRE_RELOC | DM_FLAG_DEFAULT_PD_CTRL_OFF,
>>>>>> +};
>>>>>
>>>>> This should use driver model, with a UCLASS_CLK and UCLASS_RESET
>>>>
>>>> Please refer to qcom_cc_bind() which binds the clock, reset, and power
>>>> domain drivers.
>>>
>>> Gosh, why? Are these devices not in the devicetree?
>>
>> They are, the gcc block contains clock, reset, and pd parts. On Linux
>> this is not an issue because a single device can be multiple different
>> classes (e..g when you register a reset you do it for a device) whereas
>> U-Boot requires a device per class.
>>
>> e.g. see devm_reset_controller_register() in Linux, it populates a
>> struct reset_controller_dev which references the struct device created
>> for the node. In U-Boot you have to create a new device which _is_ the
>> reset controller.
> 
> OK, I see. Rockchip has a CRU (Clock & Reset Unit) which uses syscon
> to access registers. The clock driver 'owns' the node in U-Boot and it
> manually binds a reset driver. It isn't great, either.
> 
> Looking at drivers/clk/qcom/clock-sdm845.c (for example), I can't
> actually find "qcom,gcc-sdm845" (for example) in U-Boot, except as a
> binding. Can you please point me to the node?

It's in dts/upstream/src/arm64/qcom/sdm845.dtsi

> 
> Re devm_reset_controller_register(), yes the U-Boot driver model is a
> lot more regular, e.g. we don't really want drivers creating their own
> struct udevice. We want all devices to be created automatically by the
> devicetree and most memory allocations to be done automatically. This
> helps to reduce code size and execution time. You probably know all
> this :-)

Yeah, U-Boot's model is simpler for most cases. This makes sense. But it 
doesn't reflect the reality of DT so well in cases like this.
> 
> To a significant degree, the devicetree bindings are created without
> much thought to efficient operation in U-Boot. I hope that eventually
> this might change.

I strongly disagree with this mental model. This is the approach I see 
vendors take in their BSP sources and the result is not pleasant.

DT should (within reason) never be written with the OS in mind. It is an 
agnostic structure to describe the hardware. I think the new power 
sequencing subsystem in Linux does a good job at embodying how we should 
approach consuming DT.

> 
> Anyway, with the devicetree we have, I wonder how we could do this better?
> 
> Some ideas:
> 
> 1. Allow DM to bind multiple devices to each devicetree node, perhaps
> as a Kconfig option (to avoid time penalty for all boards), or by
> requiring a new DM_FLAG_MULTI_NODE flag. The devices would then be
> independent, with no common parent device

This would make sharing match data hard, and likely cause issues with 
generic compatible strings.
> 
> 2. As 1 but have DM create a parent 'UCLASS_MULTI' device
> automatically. I am thinking that we should have a new uclass for
> these things, or rename the NOP uclass. This option would allow easy
> access to the parent device, if needed.

This is the current approach, we just bind the clk/reset/pd drivers 
explicitly, allowing us to create and share common data. I don't believe 
there is a sensible way to do this generically.
> 
> 3. Implement devices which are in more than one uclass. There would
> still be a primary uclass, but others can be added too. This would
> involve declaring a list of secondary uclasses in the U_BOOT_DRIVER()
> macro. We would then have a struct dmtag_node for each secondary
> uclass, containing the ID and the uclass pointer. Certain functions
> would need to be updated to support this, and again it could be behind
> a Kconfig.

Many device classes in U-Boot rely on going from a struct udevice to 
some uclass specific data or ops. I have always found this to be a bit 
odd, though simpler to deal with than Linux.
> 
> What do you think?

I think if we're to try and solve this at all, the Linux model is by far 
the most sensible. It is already tried and tested, and would have the 
huge bonus of simplifying driver porting.

barebox went with this approach and it seems to have worked out quite 
well for them.

All that being said, while it's taken me some time to get my head around 
"the U-Boot way", I think there is still value in the simplicity of 
U-Boot's approach. I also think the solution we've ended up with (after 
many iterations I might add) in clock-qcom is clean, simple, and easy to 
understand; though I do agree that U-Boot's DM is definitely hitting the 
limit of what complexity it can handle.

I would honestly be much more interested in seeing early init get 
cleaned up, OF_LIVE becoming the default, and the ofnode abstraction 
going away.

Kind regards,
> 
> Regards,
> Simon

-- 
// Caleb (they/them)