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

[Caleb Connolly]

On 22/08/2024 04:59, Simon Glass wrote:
> Hi Caleb,
> 
> On Wed, 21 Aug 2024 at 14:33, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>
>>
>>
>> On 21/08/2024 20:27, Simon Glass wrote:
>>> Hi Caleb,
>>>
>>> On Wed, 21 Aug 2024 at 10:47, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> 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.
>>>
>>> I'm only really involved in mainline and don't really see vendor trees
>>> much. An example is where pinctrl has a GPIO controller but it is not
>>> mentioned in the devicetree. It would be better for U-Boot to add a
>>> subnode for each GPIO controller. In general, if the SoC has a device,
>>> it should be in the devicetree.
>>
>> The concept of a device is an OS one. DT is not "telling the OS how to
>> use the hardware", it is describing the hardware.
>>
>> This distinction is important because it's the only way to ensure that
>> future OS changes can be done regardless of the DT. And also of course
>> because different OS's will have different ideas of how to model devices
>> (case in point).
>>
>> The GCC block on Qualcomm platforms is a single hardware block. The
>> datasheets and hardware programming guides describe it as such. The
>> clocks, resets, and GDSCs are all entwined at the hardware level. They
>> also have overlapping register addresses.
>>
>> The further away DT gets from describing the hardware in favour of
>> simplifying the OS, the more likely we are to start running into issues
>> with fitting hardware changes into our arbitrary model.
> 
> I completely agree with everything you are saying, but you don't go
> far enough. We should additionally require that all hardware has a
> description in the devicetree. See for example the GPIO controller I
> mentioned. When Linux wants it, it gets it, when it doesn't, it isn't
> there. Sorry to have to say it, but that's not right.

If it's only used in U-Boot, that's justification enough to add the node 
upstream?
> 
>>
>>>
>>> Part of this difference (between U-Boot and Linux) comes about because
>>> Linux device setup is fairly manual, whereas U-Boot tries to put all
>>> of that in common DM code. Whenever you are including
>>> dm/device-internal.h that is often a sign that the binding is causing
>>> issues.
>>
>> To me this indicates an inability for U-Boot's DM to handle complicated
>> devices. I don't think U-Boot should dictate the design of devicetree.
>>
>> I don't know how else to describe this. This issue has been litigated
>> over and over again on the kernel mailing list. Every time someone
>> suggests changing DT because of a limitation in Linux they are
>> (rightfully) shut down. If these changes were accepted then it would be
>> impossible for DT to be an OS agnostic hardware description, because it
>> would be full of OS specific hacks.
> 
> Given that an OS has no size limitations so can afford to do just
> about anything to deal with one-off cases in each SoC, sure. But let's
> face it, it isn't project-agnostic. I could provide dozens of examples
> where the bindings are a pain for U-Boot. Even the use of strings in
> some of the SoCs' pinctrl bindings is painful. My point is that there
> are many ways to model and describe the hardware and taking more
> account of all users would be a big step forward.

Sure, I can understand wanting to prioritize size/speed. I think 
livetree would help a lot here.
> 
>>>
>>> I'm happy for you to change my mind.
>>>
>>>>
>>>>>
>>>>> 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.
>>>
>>> You would have to repeat the same compatible string in each driver.
>>>
>>> For generic compatible strings we could a) worry about it later or b)
>>> restrict this technique to only nodes with a single compatible string,
>>> or c) use the driver flag as mentioned
>>>
>>>>>
>>>>> 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.
>>>
>>> How come? What is qcom_cc_bind() doing which DM couldn't?
>>
>> Maybe DM could look at the "#clock-cells", "#reset-cells", and
>> "#power-domain-cells" properties and match on compatible string +
>> uclass? I think that could work.
> 
> Hmmm you mean when it sees those in the node it knows the additional
> uclasses it is allowed to use?

Yes, the "#reset-cells" property literally means "this is a reset 
controller".
> 
>>>
>>>>>
>>>>> 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.
>>>
>>> In U-Boot the uclass is a stronger concept, e.g. you can generically
>>> iterate through all devices in a uclass, and all devices have one.
>>>
>>>>>
>>>>> 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.
>>>
>>> What is the Linux model, in this sense? Whenever I see barebox I
>>> wonder why we can't fold whatever new features it needs into
>>> U-Boot...perhaps the code bases have converged too much...?
>>
>> barebox is livetree only (like Linux) as I understand it, and I think it
>> aims to be literally copy/paste compatible with Linux. I would love for
>> U-Boot to adopt this approach.
> 
> OK. We do have code-size restrictions though.
> 
>>>
>>>>
>>>> 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.
>>>
>>> Well I would like to tidy this up in DM, so let's figure out which
>>> option makes the most sense...once I understand what you mean by
>>> 'Linux model' above.
>>
>> I mean where a single struct device can be multiple classes, so you
>> create a device and then create a reset controller and associate the
>> device with it.
> 
> It's the bit about having one driver manually creating devices that I
> very much want to avoid. If the devicetree really does (fully)
> describe hardware, it shouldn't be necessary.

For sure
> 
>>>
>>>>
>>>> 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.
>>>
>>> So far as I can tell, you are always going to have a flat tree, even
>>> if only before relocation or in SPL. How would we get around that?
>>> Also, what don't you like about ofnode?
>>
>> Yeah, you start with a flat tree and then should build a live one as
>> early as possible.
>>
>> The problem with ofnode is that it is more or less restricted to the
>> subset of operations fdt supports. It lacks some of the fancier features
>> of a live tree. It also means that every use of the of_* API has to be
>> preceded by a check that we're actually running the live tree, otherwise
>> some kind of hard bail.
> 
> Indeed. In principle, ofnode could support anything, but the cost
> might be high when implemented in the flat tree. People do add new
> functions from time to time.
> 
>>
>> Would be great to use of_* API by default, for capable platforms anyway.
>>>
>>> Given that Qualcomm is only using U-Boot as a second-stage loader so
>>> far, (please, not for long!!) everything looks quite different. But
>>> most platforms use U-Boot from SoC-boot-ROM handoff, so the
>>> constraints are different (tighter).
>>
>> For U-Boot SPL yeah I expect the constraints to be different. We're
>> getting a bit closer to the metal on the rb3gen2 (now it's just bootROM
>> -> SBL1 -> (tz -> hyp) -> U-Boot, without going through edk2 first haha).
> 
> You should teach rpi to do that too! At the moment I think it boots
> edk2 before U-Boot.
> 
>>
>> I hope we'll get to do U-Boot SPL on some Qualcomm platforms eventually.
>>>
>>> Anyway, certainly OF_LIVE being the default would be good. I have
>>> often wondered if we can (at build-time) convert the devicetree into a
>>> 'live' version, where the pointers are replaced by integers, such that
>>> the early U-Boot code can easily compute the pointer value for each
>>> node. It should make the unflattening much faster. For pre-relocation
>>> and SPL, since we know the load address, we can (I am pretty sure)
>>> have Binman put a full, live tree in the image and avoid the
>>> unflattening code at all. Relocating a livetree is fairly easy too.
>>
>> This might be a nice optimisation? I think it would be acceptable to
>> just read the memory layout -> enable the MMU -> build the live tree.
>> This would probably be fast enough.
>>
>> And of course, the great thing we have on Qualcomm platforms is that we
>> can run the same U-Boot binary on every sdm845 and newer platform.
> 
> That's nice, and a good demo of devicetree done right.
> 
> If I don't hear any strong preference from you I am likely to have a
> go (in the fullness of time) at implementing the easiest DM-based
> solution to get rid of that binding function you have. We can iterate
> on it in the review in any case.

That makes sense. I assume in the mean time it's ok for me to take this 
patch?

Kind regards,
> 
> Regards,
> Simon

-- 
// Caleb (they/them)