[RFC PATCH v1 3/9] clk: renesas: add R906G032 driver
Sean Anderson
seanga2 at gmail.com
Tue Aug 23 06:14:31 CEST 2022
On 8/14/22 10:48 PM, Ralph Siemsen wrote:
> On Sat, Aug 13, 2022 at 01:30:19AM -0400, Sean Anderson wrote:
>> +
>>> + u16 gate, reset, ready, midle,
>>> + scon, mirack, mistat;
>>
>> What are the scon/mirack/mistat fields for? You define them for a lot
>> of clocks, but I don't see them used in the driver.
>
> These came from the Linux driver of the same name. I can only speculate that in turn, the Linux driver definitions were auto-generated from vendor provided XML or similar documentation.
>
> I figured that it would be best to match the Linux kernel clock driver. That way fixes can easily be shared. In fact while doing this work, I found an error in the clock table [1] and I also made some simplifications [2].
>
> 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.
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2dee50ab9e72a3cae75b65e5934c8dd3e9bf01bc
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f46efcc4746f5c1a539df9db625c04321f75e494
>
>>> +};
>>> +
>>> +/* This is used to describe a clock for instantiation */
>>> +struct r9a06g032_clkdesc {
>>> + const char *name;
>>> + uint32_t managed: 1;
>>> + uint32_t type: 3;
>>
>> I wonder if we could define the enum here?
>
> This is still part of the code which I've intentionally kept identical to the Linux driver. I do agree that moving the enum seems reasonable, I'll put together a patch for this (and test it) and see if I can get it accepted on the kernel side.
>
>>> + uint32_t index: 8;
>>> + uint32_t source : 8; /* source index + 1 (0 == none) */
>>> + /* these are used to populate the bitsel struct */
>>> + union {
>>> + struct r9a06g032_gate gate;
>>> + /* for dividers */
>>> + struct {
>>> + unsigned int div_min : 10, div_max : 10, reg: 10;
>>> + u16 div_table[4];
>>> + };
>>> + /* For fixed-factor ones */
>>> + struct {
>>> + u16 div, mul;
>>> + };
>>> + /* for dual gate */
>>> + struct {
>>> + uint16_t group : 1;
>>> + u16 sel, g1, r1, g2, r2;
>>> + } dual;
>>> + };
>>> +};
>>> +
>>> +#define I_GATE(_clk, _rst, _rdy, _midle, _scon, _mirack, _mistat) \
>>> + { .gate = _clk, .reset = _rst, \
>>
>> If these fields have bitfield inside them, then those bitfields should
>> be assigned/constructed separately. That is, if .reset is actually a combined
>> offset/bit, then you need to expose those in the macro. Since you have a lot of these, you might want to do something like
>>
>> #define BIT_OFFSET GENMASK(15, 5)
>> #define BIT_SHIFT GENMASK(4, 0)
>>
>> #define PACK_BIT(offset, shift) (FIELD_PREP(BIT_OFFSET, offset) | FIELD_PREP(BIT_SHIFT, shift))
>
> 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/
>>> +
>>> +/* Internal clock IDs */
>>> +#define R9A06G032_CLKOUT 0
>>> +#define R9A06G032_CLKOUT_D10 2
>>> +#define R9A06G032_CLKOUT_D16 3
>>> +#define R9A06G032_CLKOUT_D160 4
>>> [...]
>>> +#define R9A06G032_UART_GROUP_012 154
>>> +#define R9A06G032_UART_GROUP_34567 155
>>
>> Can you put these in your dt-bindings header? I think that would make it
>> much clearer why there are gaps, and would avoid someone accidentally
>> duplicating a clock id (although I suppose your array initializer below
>> might complain?)
>
> 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.
>>> + parent->id = ~0; /* Top-level clock */
>>
>> Can you use a define for this (instead of referring to ~0 everywhere)
>
> Yes, that sounds reasonable. My list of fixes (for both Linux driver and the u-boot one) is going to get rather long ;-)
>
>>> + 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.
>>> +/* register/bit pairs are encoded as an uint16_t */
>>> +static void
>>> +clk_rdesc_set(struct r9a06g032_priv *clocks,
>>> + u16 one, unsigned int on)
>>> +{
>>> + uint offset = 4 * (one >> 5);
>>> + uint mask = 1U << (one & 0x1f);
>>> + uint val = ((!!on) << (one & 0x1f));
>>
>> Please either use bitfields for this, or use FIELD_GET() and friends.
>
> Yes, this would be clearer - however as mentioned above, there was already quite a bit of teeth-gnashing about this encoding. I will prepare a patch for the Linux side and see what kind of reply I get.
>
> I would very much prefer to keep both in sync as much as possible.
>
>>> +/*
>>> + * Fixed factor clock
>>> + */
>>> +static ulong r9a06g032_ffc_get_rate(struct clk *clk)
>>> +{
>>> + const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>> + unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
>>> + unsigned long long rate;
>>> +
>>> + if (parent_rate == 0) {
>>> + debug("%s: parent_rate is zero\n", __func__);
>>> + return 0;
>>> + }
>>> +
>>> + rate = (unsigned long long)parent_rate * desc->mul;
>>> + rate = DIV_ROUND_UP(rate, desc->div);
>>> + return (ulong)rate;
>>> +}
>>> +
>>> +/*
>>> + * This implements R9A06G032 clock divider 'driver'. This differs from the
>>> + * standard clk_divider because the set_rate method must also set b[31] to
>>> + * trigger the hardware rate change. In theory it should also wait for this
>>> + * bit to clear.
>>> + */
>>> +static ulong r9a06g032_div_get_rate(struct clk *clk)
>>> +{
>>> + struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
>>> + const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>> + unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
>>> + u32 div = 0;
>>> +
>>> + if (parent_rate == 0) {
>>> + debug("%s: parent_rate is zero\n", __func__);
>>
>> Didn't you already log this?
>
> 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)
...
>>
>>> + return 0;
>>> + }
>>> +
>>> + regmap_read(clocks->regmap, 4 * desc->reg, &div);
>>> +
>>> + if (div < desc->div_min)
>>> + div = desc->div_min;
>>> + else if (div > desc->div_max)
>>> + div = desc->div_max;
>>> + return DIV_ROUND_UP(parent_rate, div);
>>
>> 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.
>>> + /* TODO: use the .div_table if provided */
>>> + if (desc->div_table[0])
>>> + pr_err("ERROR: %s: div_table not implemented\n", __func__);
>>
>> dev_err
>>
>> But can't you just leave out the div_table member?
>
> Only a few of the clocks have a div_table, but for those, the table specifies the allowable divider values. So right now, the code cannot correctly set such clocks, as it may try programming illegal values (most likely with the result that you don't get the expected rate).
>
> The Linux driver does implement the div_table logic, I simply did not carry it over yet to u-boot. Mostly because I have not yet had need for one of the few clocks which does use the table. I do plan to add it.
OK
>>> +/*
>>> + * Main clock driver
>>> + */
>>> +static int r9a06g032_clk_enable(struct clk *clk)
>>> +{
>>> + const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>> +
>>> + switch (desc->type) {
>>> + case K_GATE:
>>> + return r9a06g032_clk_gate_enable(clk);
>>> + case K_DUALGATE:
>>> + return r9a06g032_clk_dualgate_enable(clk);
>>> + default:
>>> + printf("ERROR: %s:%d unhandled type=%d\n", __func__, __LINE__, desc->type);
>>
>> Assert or dev_dbg is better here. This is "impossible" so we try and
>> avoid increasing image size in these cases.
>
> 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.
>> The overall structure looks good; most of these things you
>> should be able to iron out fairly easily.
>
> I have skipped over a few of the smaller points regarding return values, etc, but I will address them in next version of the patch. Thanks again for your time reviewing this.
>
> Regards,
> -Ralph
More information about the U-Boot
mailing list