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@...
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/...
[3] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/arch/arm/boo...
[4] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/drivers/clk/...
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@m...
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