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

[Conor Dooley <conor@kernel.org>]

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

On Thu, Aug 27, 2026 at 06:22:20AM +0000, Changhuang Liang wrote:
> Hi, Conor
> 
> > Thanks for the review.
> > 
> > > 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
> > > > > > > > > > > > -m
> > > > > > > > > > > > armo
> > > > > > > > > > > > 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?
> > 
> > Yes.
> > 
> > >
> > > > > > 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.
> > 
> > Okay, I will use this method.
> 
> During the process of attempting this modification, there will be accompanying changes to the PLL driver. 
> The main changes include the following:
> 
> 1. Addition of platform device ID table: A new platform_device_id array is introduced, enabling the driver to
> be matched by device name when no OF compatible string is available.
> 
> 2. Dynamic device node resolution: The driver now attempts to obtain the device node using dev_of_node(dev).
>  If the device has no node of its own (as is the case when instantiated as an MFD child cell), it falls back to 
> using the parent device's DT node (dev_of_node(dev->parent)). This ensures the syscon regmap can still be 
> correctly retrieved.
> 
> The current initial draft of the PLL changes is generally as follows: Do you have any better suggestions on
> your side?

It's not beautiful, but it is sane - or at least I think so.

> 
> =====================================================
> 
> 	const struct jhb100_pll_match_data *match_data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np;
>  	struct jhb100_pll_priv *priv;
>  	unsigned int idx;
>  	int ret;
>  
> -	match_data = of_device_get_match_data(&pdev->dev);
> -	if (!match_data)
> -		return -EINVAL;
> +	match_data = of_device_get_match_data(dev);
> +	if (!match_data) {
> +		const struct platform_device_id *id = platform_get_device_id(pdev);
> +
> +		if (!id)
> +			return dev_err_probe(dev, -EINVAL, "no match data\n");
> +
> +		match_data = (const struct jhb100_pll_match_data *)id->driver_data;
> +	}
>  
>  	priv = devm_kzalloc(&pdev->dev,
> 			    struct_size(priv, pll, match_data->num_pll),
>  			    GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	priv->match_data = match_data;
> -	priv->dev = &pdev->dev;
> -	priv->regmap = syscon_node_to_regmap(priv->dev->of_node);
> -	if (IS_ERR(priv->regmap))
> -		return PTR_ERR(priv->regmap);
> +	priv->dev = dev;
> +
> +	/*
> +	 * When instantiated as an MFD cell of the sys0 system controller this
> +	 * device has no DT node of its own, so use the parent syscon node.
> +	 */
> +	np = dev_of_node(dev) ? : dev_of_node(dev->parent);

I think I'd probably check that you're on the device you think you are
(sys0) before doing the dev_of_node(dev->parent), but...

> +	priv->regmap = syscon_node_to_regmap(np);

...this will fail if you are not on sys0 and somehow dev_of_node(dev)
returned null so it's probably fine.

Thanks,
Conor.

> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "failed to get syscon regmap\n");
> 
>  static const struct of_device_id jhb100_pll_match[] = {
> -	{
> -		.compatible = "starfive,jhb100-sys0-syscon",
> -		.data = &jhb100_sys0_pll,
> -	},
>  	{
>  		.compatible = "starfive,jhb100-per0-syscon",
>  		.data = &jhb100_per0_pll,
>  	},
>  	{
>  		.compatible = "starfive,jhb100-per1-syscon",
>  		.data = &jhb100_per1_pll,
>  	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, jhb100_pll_match);
>  
> +static const struct platform_device_id jhb100_pll_ids[] = {
> +	{
> +		.name = "jhb100-sys0-pll",
> +		.driver_data = (unsigned long)&jhb100_sys0_pll,
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, jhb100_pll_ids);
> +
>  static struct platform_driver jhb100_pll_driver = {
>  	.probe = jhb100_pll_probe,
> +	.id_table = jhb100_pll_ids,
>  	.driver = {
>  		.name = "clk-starfive-jhb100-pll",
>  		.of_match_table = jhb100_pll_match,
>  		.suppress_bind_attrs = true,
>  	},
>  };
> 
> ==============================================================
> 
> > >
> > > >
> > > > >
> > > > > > 	},
> > > > > > };
> > > > > >
> > > > > > 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 --]