jh7110_clkgen: Add StarFive JH7110 clock generator driver

[jrtc27]

Initial review comments. Not exhaustive, hard to be for large diffs like this where there are quite a few comments to make, but hopefully I didn't miss anything too fundamental.

In D43037#1004445, @jsihv_gmx.com wrote:

Restructuring code to the device tree used in Linux, now including separate devices and reset driver's methods.

FreeBSD's sources currently include some early JH7110 header and dts files which need to be updated (for instance since the device tree doesn't include Ethernet device and clock headers don't include PCIE clocks). So this code is made for device tree and header files used in current Linux. These files are partly incompatible with early ones which are still contained in FreeBSD sources.

Do you mean that this patch does not build? If so that's not going to work.

Some other comments:

• It looks to me (but Phab could be mangling it) like in struct declarations you're using spaces to align the identifier names rather than tabs as is customary
• Similarly some of my formatting comments may only be a result of Phab mangling things in weird ways, so some may be undeserved
• Is it really the case that none of these clocks can use any of the various generic clk helpers that let you tell it the layout of the registers in memory and only provide a methods to do the reading and writing of those registers (see sifive_prci for an example of how that's simplifying)?

sys/dev/clk/starfive/jh7110_clk.c

71

lower_snake_case

95

_MEMRES is unnecessary, clearly it's going to be reading from memory, and there's only one such resource (ditto write)

110

You have a lot of functions that start with a blank line

113

This is really ugly. It seems to me you want a common base class that clk_aon and clk_sys extend from, with this functionality in there, using information in the corresponding softc.

162

How come these functions aren't using your macros for read/write?

172

This feels like patching over a problem that should instead be avoided in the first place, whether that's detecting the case or making it invalid to do so.

190

Do you really need to read it one last time? If not you can avoid the infinite loop+break and use proper loop conditions.

195

Why the parens?

217

Why the parens? If they were to go anywhere here I'd expect them to be around id % 32 so people don't have to remember that that's the operator precedence.

325

What's this magic condition about? Needs a comment explaining.

354

Ditto

362

Probably nicer to goto done at the end and reuse the code there (or indent the code below in a negated if block)

sys/dev/clk/starfive/jh7110_clk.h

84

Wonky formatting

sys/dev/clk/starfive/jh7110_clk_aon.c

68

Formatting

119

error != 0

120

Print error

134

I doubt we should continue

140

• Printing and continuing is unlikely to be useful behaviour
• c not C
• This message doesn't fit the usual style (and the style you've used above)

178

Formatting

sys/dev/clk/starfive/jh7110_clk_pll.c

75

Formatting

107

Though does it really need to wrap?

119

Formatting

149

Formatting

153

Formatting

160–161

(or at least flip the operands for the second one, but the switch from val to new_val makes me have to think more than for my suggestion, which is a common pattern)

191

Formatting for these

206

Why the parens?

208

Indentation isn't quite right (looks 1 too small)

211

I see what you're trying to do with lining it up, but please don't. Better to have the (frac * FRAC_PATR_SIZE / (1 << 24)) be a variable that's 0 in this case and then you can have one copy of the rest of the expression.

376

Wrong terminator

400

Please make this a switch and use a macro for the cases, this kind of code is just asking for a copy/paste error otherwise

sys/dev/clk/starfive/jh7110_clk_pll.h

2

Where did you get this file from exactly?..

50

starfive or jh7110?..

sys/dev/clk/starfive/jh7110_clk_sys.c

69

Formatting

187

This is just a stripped-down version of clknode_default_ofw_map; no need to reimplement it and override the mapper

246

Is this just because of your mapper implementation? (Though aon didn't check this, despite using the same implementation...)

265

I doubt we should continue

271

As with aon

327

I'd naively expect this to instead be during BUS_PASS_BUS, which I believe is when clock drivers tend to attach. Is there a reason it's not?

328–329

I don't think this is useful for something like a clock driver that has to be compiled into the kernel?

[jsihv_gmx.com]

Okay, I had still few things to learn about formatting (some things were seen wrongly here because I had relied excessively on using spaces. I will stop that). I agree with almost every formatting related comment and I haven't answered those ones.

Cannot we use generic clk helpers for something?

I don't currently see benefits from that since we need those separate functions that work for combined clocks (GATEDIV, GATEMUX etc.) so we can use them as well for single-type (gate only, div only...) clocks. Using generic functions for single-type clocks would require some additional code.

But vendor's driver used fixed clocks and so did I when I used vendor's alternative FDT. Mainline Linux treats some clocks as DIV which are seen as fixed in vendor's driver and I've followed the mainline after FDT adjustment. I can easily return fixed clocks to the code if needed.

sys/dev/clk/starfive/jh7110_clk.c

172

At this point I think we could remove this timeout polling part from the code. No reset drivers for other computers mention gated clocks. The system boots without this (even though there are deasserting happening with devices that have gated clocks). Not every implementation seems to have this part and those which have are joint JH7100/JH7110 code, so it might be JH7100's hardware issue.

190

I can return to this if we include this part

195

Compiler/build system wants them

325

I'm starting to think we could drop this part. Just one implementation minds this issue. TRM (V2 Preliminary, pages 188-190) shows UART3-5 core clock's register indeed is different but maybe it's nevertheless superfluous to address the issue here?

sys/dev/clk/starfive/jh7110_clk_pll.h

2

It's from vendor's kernel. While I switched away from vendor kernel's FDT, I nevertheless didn't see a reason to drop this file. This is a cropped version of the original file.

sys/dev/clk/starfive/jh7110_clk_sys.c

327

Getting drivers loaded in a proper order felt initially difficult but now when I tried it again, having BUS_PASS_BUS for all of them (and BUS_PASS_ORDER_LATE for SYS & AON) seems to work after all. So I'll use that.

[jsihv_gmx.com]

This patch update fixes style issues, improves module load scheduling, removes reset timeout polling and UART clk special handling, adds PLL macros. I welcome yet discussion about these issues. I was pretty undecided about reset & UART.

It looks challenging to come up with PLL macros that would be essentially smarter. Preprocessor is clumsy with macro tokens close to struct ref operators (dots) or semicolons. These issues can be overcome but not without costs. I added underscores to the end of some variables to make these macros possible.

[jsihv_gmx.com]

mhorne's improvements for jh7110_clk_pll.c and files.starfive added
(note: some lines in jh7110_clk_pll.c are over 80 chars)

[mhorne]

Overall this is looking good to me. The core clock logic seems right, as well as the SYS group. I did not verify the clock hierarchy in detail compared to the TRM.

I note there is no STG clock provider, yet. From the device tree this seems like it will be relevant for USB support, but this clock grouping does not seem critical yet.

I have some suggestions still on how to improve the PLL clock driver.

Thanks for your patience.

sys/dev/clk/starfive/jh7110_clk_aon.c

15

I suspect this header isn't required.

23–27

Also not needed?

29–32

Should only require clk.h, I think?

154

sys/dev/clk/starfive/jh7110_clk_pll.c

35–42

I think the names here need improvement.

These offsets correspond to registers in the SYS_SYSCON space. I think it would be good to introduce a shared header containing JH7110 register definitions, matching what is in the section 2.8 of the TRM.

So, for example, jh7110_reg.h would contain:

/* SYS SYSCON */
#define JH7110_SYS_SYSCON_SYSCFG24        0x18
#define JH7110_SYS_SYSCON_SYSCFG28        0x1c
...

And you would use these definitions instead.

359

The meaning of the arguments to PLL_OFFSET_FILL is not obvious from its usage. I think the code would be clearer if you omit this macro magic and just fill in all the fields explicitly. If you follow my suggestion above PLL_OFFSET_FILL will need to be removed or reworked anyway.

I have no objection to PLL_MASK_FILL or PLL_SHIFT_FILL.

Also consider: Do these hard-coded register offsets need to exist in the softc? Why not in a static array? Is there a difference?

sys/dev/clk/starfive/jh7110_clk_sys.c

231

For this purpose, EBUSY is more widely used.

[jsihv_gmx.com]

sys/dev/clk/starfive/jh7110_clk_aon.c

15

It is required for mutex

sys/dev/clk/starfive/jh7110_clk_pll.c

35–42

I examined the need for a new header file you proposed but for me it looks like there's no much use for it. Existing Linux implementations seem to have just few references like this to system control registers and they don't seem to be shared between drivers. Also a glance at device trees didn't give a picture that there would be a lot of more register offsets that could be used this way. But I may have missed something so if you still think we should have this new header file, I will add it (and it's easy to add later, if it's needed).

I changed PLL_OFFSET_x defines to system register names after your example which is indeed more informative.

359

I previously added those macros after the recommendation of another reviewer. But now I unrolled PLL_OFFSET_FILL as you suggested. Maybe this is a fine compromise. No matter how we do it, the code won't look particularly slick.

I considered the possibility of using static arrays instead of placing offsets to softc, as you suggested. I did not find much difference between these solutions. Using static arrays would imply less assignments in the register function, but then we would need to identify clock's id in freq functions (possibly by using an id variable in softc). Either way, it doesn't seem to make much difference in terms of cycles or lines or readability.

[jsihv_gmx.com]

This update includes small changes that were proposed. See comments for the discussion about a new header file and having more static arrays.

[mhorne]

I am happy with this. There are a couple whitespace/style issues remaining, but I will do a pass to fix them before committing. Thanks for your work, I will aim to merge by the end of the week!

sys/dev/clk/starfive/jh7110_clk_aon.c

15

Then it should be #include <sys/mutex.h>.

[jrtc27]

sys/dev/clk/starfive/jh7110_clk.c

67

I believe this is only ever written to

68

size_t? or uint32_t given they're known to be small

106

I'll note that SYSCRG_RESET_STATUS0 + id / 32 computes the same thing (ditto for selector), no if chain needed

113

Unresolved

sys/dev/clk/starfive/jh7110_clk.h

28

It's not really a flag, it's an enum

65

Indentation

sys/dev/clk/starfive/jh7110_clk_aon.c

61

Inconsistent wrapping, and bad indentation

92

Indentation

106

Indentation and wrapping

sys/dev/clk/starfive/jh7110_clk_pll.c

343

This isn't used?

sys/dev/clk/starfive/jh7110_clk_sys.c

110

Indentation

193

Indentation

[jsihv_gmx.com]

sys/dev/clk/starfive/jh7110_clk.c

67

removed

68

uint32_t

sys/dev/clk/starfive/jh7110_clk.h

28

It looks like a flag to me. Maybe you was thinking of jh7110_reset_crg enum which is now removed.

sys/dev/clk/starfive/jh7110_clk_pll.c

343

was left there by a mistake

[jsihv_gmx.com]

This update fixes smaller issues. The proposal to restructure the reset code by creating a new base class is left for later.

[jsihv_gmx.com]

Now the io_assign() function which was used by reset operations is optimized away within the framework of existing driver classes. At this point I don't see a reason to create more classes.

So instead of using dev_flags and additional struct for reset data, reset register offset variables are now placed to clkgen softc and their actual values are also placed to the header (jh7110_clk.h) as defines. They are assigned in the register functions of AON and SYS codes.

The difference between SYS and other groups is not needed to take into account since only SYS register has id numbers beyond 31 and lower values lead to the same result in any case.

[jsihv_gmx.com]

sys/dev/clk/starfive/jh7110_clk.c

113

Hopefully the new solution is fine.

[mhorne]

LGTM. We will still have to wait until @manu has finished with the DTS upgrade to 6.10.