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

[Ralph Siemsen <ralph.siemsen@linaro.org>]

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.

[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.

>>+
>>+/* 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.

>>+		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).

>>+/* 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.

>
>>+		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.

>>+	/* 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.

>>+/*
>>+ * 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.

>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