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
