Re: [PATCH v4 7/8] soc: starfive: Add socinfo driver for JHB100 SoC

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5656 bytes --]

On Mon, Aug 17, 2026 at 01:27:08AM +0000, Changhuang Liang wrote:
> Hi, Conor
> 
> Thanks for the review.
> 
> > On Fri, Aug 14, 2026 at 01:13:38AM +0000, Changhuang Liang wrote:
> > > Hi,Conor
> > >
> > > Thanks for the review.
> > >
> > > > On Thu, Aug 13, 2026 at 08:10:12AM +0000, Changhuang Liang wrote:
> > > > > Hi ,Conor
> > > > >
> > > > > Thanks for the review.
> > > > >
> > > > > > On Wed, Aug 12, 2026 at 10:37:16AM +0000, Changhuang Liang
> > wrote:
> > > > > > > Hi, Conor
> > > > > > >
> > > > > > > Thanks for the review.
> > > > > > >
> > > > > > > > On Tue, Aug 11, 2026 at 05:36:19AM +0000, Changhuang Liang
> > wrote:
> > > > > > > >
> > > > > > > > > > On Sat, Aug 08, 2026 at 06:50:53PM -0700, Changhuang
> > > > > > > > > > Liang
> > > > wrote:
> > > > > > > >
> > > > > > > > > > > +static int __init starfive_socinfo_init(void) {
> > > > > > > > > > > +	struct soc_device_attribute *attrs;
> > > > > > > > > > > +	struct soc_device *soc_dev;
> > > > > > > > > > > +	const char *machine = NULL;
> > > > > > > > > > > +	struct device_node *np;
> > > > > > > > > > > +	struct regmap *regmap;
> > > > > > > > > > > +	char rev_char;
> > > > > > > > > > > +	u32 rev_id;
> > > > > > > > > > > +	int ret;
> > > > > > > > > > > +
> > > > > > > > > > > +	np = of_find_compatible_node(NULL, NULL,
> > > > > > > > > > > +"starfive,jhb100-sys0-syscon");
> > > > > > > > > >
> > > > > > > > > > Remind me again why this is not just probed as an mfd
> > > > > > > > > > cell from the syscon driver?
> > > > > > > > > >
> > > > > > > > > > (Hint: please put this info in the commit message).
> > > > > > > > >
> > > > > > > > > The discussion result with Krzysztof at that time is recorded here:
> > > > > > > > > https://lore.kernel.org/all/20260405-strong-watchful-marmo
> > > > > > > > > t-fd
> > > > > > > > > fad6
> > > > > > > > > @quo
> > > > > > > > > ll/
> > > > > > > > >
> > > > > > > > > One register should not be treated as a separate device node.
> > > > > > > >
> > > > > > > > That's a different question. In the case of an mfd cell
> > > > > > > > probed from the syscon driver there would be no devicetree
> > > > > > > > modifications required compared to what you have now. e.g.
> > > > > > > > sg2044-topsys.c
> > > > > > >
> > > > > > > This seems feasible, and I can introduce this modification in
> > > > > > > the next
> > > > > > version.
> > > > > >
> > > > > >
> > > > > > In fact, you don't even need the mfd cell, because your driver
> > > > > > is going to be in drivers/soc/starfive anyway? You can just bind
> > > > > > directly to the
> > > > > > sys0 syscon I think.
> > > > >
> > > > > I suddenly realized that I missed one issue:
> > > > > starfive,jhb100-sys0-syscon is already bound to the PLL driver by
> > > > > default, I
> > > > can't use it to bind another device anymore.
> > > > >
> > > > > which is why I used
> > > > >
> > > > > np = of_find_compatible_node(NULL, NULL,
> > > > > "starfive,jhb100-sys0-syscon");
> > > > >
> > > > > in jhb100-socinfo.c.
> > > > >
> > > > > So it seems the MFD cell approach isn't really feasible either?
> > > >
> > > > You could, in that case, probe the clock driver using the mfd_cell
> > > > like sg2044-topsys. What else other than the pll controls and the
> > > > soc info register is in this register region?
> > >
> > > We don't have any other drivers right now in this register region.
> > 
> > I'm not asking about what's got a driver right now though, I am wondering
> > what else the registers in this region are for.
> 
> Here are some registers related to debug functionality.

So probably nothing else that linux will ever need?

> > > So does that mean I can revise it to the format below:
> > >
> > > static const struct mfd_cell jhb100_sys0_subdev[] = {
> > > 	{
> > > 		.name = "jhb100-sys0-pll",
> > > 	},
> > 
> > > 	{
> > > 		.name = "jhb100-socinfo",
> > 
> > Why would this be needed? The driver with the mfd_cell would be the same
> > one that provides the soc info, no?
> 
> My understanding is that one driver can only bind to one device, so the PLL driver 
> and the socinfo driver need these two devices respectively, and thus cannot share
> one?

Correct, but I am talking about the socinfo driver being the one that
calls devm_mfd_add_devices() to create a device for the pll so that in
the end there are a total of two drivers, rather than create something
identical to sg2044-topsys.c that does nothing other than initialise two
mfd cells.
May as well consolidate the mfd_cell definition and call to
devm_mfd_add_devices() in one file, since it would be in the same
directory as the socinfo driver anyway.

> 
> > 
> > > 	},
> > > };
> > >
> > > static const struct mfd_cell jhb100_per0_subdev[] = {
> > > 	{
> > > 		.name = "jhb100-per0-pll",
> > 
> > Why do these other ones need to be modified?
> 
> I was thinking, since sys0-pll has been adjusted, would it be better to also adjust per0-pll and per1-pll together in a unified way?

I wouldn't complicate them with things that they don't need, so it
depends on if these other syscon regions contain more than just PLLs
and if there'll ever be drivers for the other stuff in those regions.

> 
> > 
> > > 	},
> > > };
> > >
> > > static const struct mfd_cell jhb100_per1_subdev[] = {
> > > 	{
> > > 		.name = "jhb100-per1-pll",
> > > 	},
> > > };
> > >
> > > static const struct mfd_cell jhb100_ pcierp _subdev[] = {
> > > 	{
> > > 		.name = "jhb100-pcierp-reset",
> > > 	},
> > > };
> 
> Best Regards,
> Changhuang
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]