[RFC PATCH v1 3/9] clk: renesas: add R906G032 driver

[Ralph Siemsen]

On Tue, Aug 23, 2022 at 12:14:31AM -0400, Sean Anderson wrote:
>>Regarding the unused fields (scon, mirack, mistat): I am not really 
>>sure what their purpose is. Maybe there is some value in having them. 
>>I'll try to find out more information about them. If we do decide to 
>>drop them, I would like to keep it synchronised with the Linux driver.
>
>OK, well if you don't use them then perhaps you can just leave them in
>the macro but remove them from the struct. That way you can add support
>for them later if you need to, but they don't take up space in the mean
>time. A comment summarizing your explanation above would be helpful.

I did figure out (mostly) what they are for, so I can see some value in 
keeping at least some of them. But as you said, they are currently 
unused, so dropping them from the structure make sense.

I have prepared patches for this firstly on the kernel side, and then I 
will make the same change in this u-boot driver. Stay tuned :-)

>>I think it happened before I started working on RZ/N1, but there 
>>seemed to be quite a few iterations on how to represent the clock 
>>tree. At one point there were macros to assign/construct the bitfield 
>>values. And then a different way, and eventually the direct hex values 
>>you now see in the clock tables.
>>
>>At the risk of re-opening old wounds (luckily not mine) I decided to 
>>just leave this part exactly as-is in the Linux driver.
>
>Can you link to that discussion? The earliest discussion of that
>series I could find was [1], and there's no mention of the encoding.
>This encoding scheme seems to be used only for this SoC, and not for
>any of the other renesas drivers. I suspect that this just wasn't
>reviewed in detail the first time around...
>
>[1] https://lore.kernel.org/all/1527154169-32380-6-git-send-email-michel.pollet@bp.renesas.com/

That link [1] is the current driver, which uses the packed encoding 
(with a register offset and bit number stored as a packed uint16_t).

This is based (loosely) on an earlier version of the driver, which you 
can find in the Schneider kernel repo [2] on the 4.19 and older branch.
This version stores clock information is in the device tree [3] and uses 
_BIT() macro in the clock tables [4]

[2] https://github.com/renesas-rz/rzn1_linux/tree/rzn1-stable-v4.19/drivers/clk/rzn1

[3] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/arch/arm/boot/dts/rzn1-clocks.dtsi

[4] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/drivers/clk/rzn1/rzn1-clkctrl-tables.h

Evidently this was not deemed suitable, and thus morphed into the 
version that did get merged [1]. At least that is my guess, I don't know 
for sure what transpired.

I have modified the clock table so that the register offset and bitnum 
are explicit values, rather than packed together. I will run this up the 
kernel side and see if they agree. Am still trying to test it though...


>>In fact quite a few of them are in the dt-bindings already, see 
>>include/dt-bindings/clock/r9a06g032-sysctrl.h
>>
>>I'm not really sure why some of these are defined in the .C file while 
>>others are in the dt-bindings header. Like much of the other bits, 
>>this was something I just carried over as-is from the Linux driver.
>
>I think these are "internal" clocks (that is, clocks which don't really
>exist like intermediate dividers) whereas the others are public-facing
>clocks. It's up to you, but maybe have a comment noting where the other
>ids come from.

I guess that is plausible explanation.. I will add a comment...

>>>>+    else
>>>>+        parent->id = desc->source - 1;
>>>>+
>>>>+    parent->dev = clk->dev;
>>>
>>>I think you need to clk_request here.
>>
>>Normally clk_request is called by a driver wishing to use a particular 
>>clock. That is not the case here. This is in a helper function used to 
>>compute the current rate of a given clock. It only looks at the local 
>>table (struct r9a06g032_clkdsc).
>
>You call clk_get_rate on it. Any time you "create" a new clock, you
>must call clk_request.

So the situation here is similar to that on the mediatek patches from 
Weijie Gao [5], where you made a similar comment. These are not real 
clocks, they have no ops->request, and the only field used is clk->id.
This is done primarily to avoid a bunch of malloc of struct clk, 
particularly in the early u-boot (before relocation).
  
[5] https://lore.kernel.org/all/31b0e1313267c8d342e0e3d1c9f15eaa8e666114.camel@mediatek.com/

>>It is for a different clock type. However I will see if I do something 
>>to avoid the duplication.
>
>I mean in r9a06g032_clk_get_parent_rate. You can also just do
>
>if (!parent_rate)
>	...

I've fixed this (and several similar instances elsewhere).

>>>DIV_ROUND_CLOSEST?
>>
>>I'm hesitant to change the logic on this, as it could subtly alter the values.
>
>Well if you have 2MHz divided by 3, the resulting rate is closer to
>666667 kHz than 666666 Hz.

While I can't argue with the math, the linux driver upon which this is 
based uses DIV_ROUND_UP everywhere. Maybe that is something to also 
re-consider, but for the time being, I don't want to make the change 
even more complicated...

>>Fair enough. Though I have to say I have been bitten by this kind of 
>>thing a few times. After having spent time debugging-via-printf, I 
>>would then discover an assert or dev_dbg that points out the exact 
>>problem. If only I had enabled DEBUG for that file! And yet if I 
>>enable DEBUG globally, there is so much noise, that I don't notice the 
>>one message that might have been helpful.
>
>I generally stick a `#define DEBUG` at the top of a file any time I get
>an error with no message. You still return 0, so I suggest returning
>-EINVAL (or something else uncommon) so you have somewhere to start.

Yep, that works fine when developing a driver... but if you are 
debugging a board, it is often not clear which file(s) should get 
sprinkled with DEBUG... and turning it on globally is too verbose to be 
useful in most cases.

Anyhow, thanks for your review, and v3 will be posted eventually, once I
get the kernel side sorted.

-Ralph