Re: [PATCH net-next v5 13/17] net: pse-pd: Use regulator framework within PSE framework

[Köry Maincent]

Hello Oleksij,

Thanks for your review!!

On Sat, 2 Mar 2024 22:35:52 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:

> On Tue, Feb 27, 2024 at 03:42:55PM +0100, Kory Maincent wrote:
> > Integrate the regulator framework to the PSE framework for enhanced
> > access to features such as voltage, power measurement, and limits, which
> > are akin to regulators. Additionally, PSE features like port priorities
> > could potentially enhance the regulator framework. Note that this
> > integration introduces some implementation complexity, including wrapper
> > callbacks and nested locks, but the potential benefits make it worthwhile.
> > 
> > Regulator are using enable counter with specific behavior.
> > Two calls to regulator_disable will trigger kernel warnings.
> > If the counter exceeds one, regulator_disable call won't disable the
> > PSE PI. These behavior isn't suitable for PSE control.
> > Added a boolean 'enabled' state to prevent multiple calls to  
> 
> Please rename rename "enabled" to "admin_state_enabled". This variable
> do not reflect real device state, it reflects only user configuration.

Right,

> 
> ...  
> > @@ -120,15 +118,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> >  	u32 phy_id;
> >  	int rc;
> >  
> > -	psec = fwnode_find_pse_control(child);
> > -	if (IS_ERR(psec))
> > -		return PTR_ERR(psec);
> > -
> >  	mii_ts = fwnode_find_mii_timestamper(child);
> > -	if (IS_ERR(mii_ts)) {
> > -		rc = PTR_ERR(mii_ts);
> > -		goto clean_pse;
> > -	}
> > +	if (IS_ERR(mii_ts))
> > +		return PTR_ERR(mii_ts);
> >  
> >  	is_c45 = fwnode_device_is_compatible(child,
> > "ethernet-phy-ieee802.3-c45"); if (is_c45 || fwnode_get_phy_id(child,
> > &phy_id)) @@ -161,6 +153,12 @@ int fwnode_mdiobus_register_phy(struct
> > mii_bus *bus, goto clean_phy;
> >  	}
> >  
> > +	psec = dev_find_pse_control(&phy->mdio.dev);
> > +	if (IS_ERR(psec)) {
> > +		rc = PTR_ERR(psec);
> > +		goto unregister_phy;
> > +	}
> > +  
> 
> I do not think it is a good idea to make PSE controller depend on
> phy->mdio.dev. The only reason why we have fwnode_find_pse_control()
> here was the missing port abstraction.

I totally agree that having port abstraction would be more convenient.
Maxime Chevallier is currently working on this and will post it after his
multi-phy series get merged.
Meanwhile, we still need a device pointer for getting the regulator. The
phy->mdio.dev is the only one I can think of as a regulator consumer.
Another idea?

> ...
> > +static int
> > +devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,
> > +			       char *name, int id)
> > +{
> > +	struct regulator_config rconfig = {0};
> > +	struct regulator_desc *rdesc;
> > +	struct regulator_dev *rdev;
> > +
> > +	rdesc = devm_kzalloc(pcdev->dev, sizeof(*rdesc), GFP_KERNEL);
> > +	if (!rdesc)
> > +		return -ENOMEM;
> > +
> > +	/* Regulator descriptor id have to be the same as its associated
> > +	 * PSE PI id for the well functioning of the PSE controls.
> > +	 */
> > +	rdesc->id = id;
> > +	rdesc->name = name;
> > +	rdesc->type = REGULATOR_CURRENT;
> > +	rdesc->ops = &pse_pi_ops;
> > +	rdesc->owner = pcdev->owner;
> > +
> > +	rconfig.dev = pcdev->dev;
> > +	rconfig.driver_data = pcdev;
> > +	rconfig.init_data = &pse_pi_initdata;  
> 
> Please add input supply to track all dependencies:
>         if (of_property_present(np, "vin-supply"))
> config->input_supply = "vin";
> 
> May be better to make it not optional...

Does the "vin-supply" property be added at the pse-pi node level or the
pse-controller node level or at the hardware port node level or the manager node
level for the pd692x0?
Maybe better at the pse-pi node level and each PIs of the manager will get the
same regulator?
What do you think?
 
> Should be tested, but if, instead of "vin-supply", we will use
> "pse-supply" it will make most part of pse_regulator.c obsolete.

Don't know, if it is done at the pse-pi node level it may not break
pse_regulator.c. Not sure about it.

> ....  
> > @@ -310,6 +452,20 @@ pse_control_get_internal(struct pse_controller_dev
> > *pcdev, unsigned int index) return ERR_PTR(-ENODEV);
> >  	}
> >  
> > +	psec->ps = devm_regulator_get_exclusive(dev,
> > +
> > rdev_get_name(pcdev->pi[index].rdev));
> > +	if (IS_ERR(psec->ps)) {
> > +		kfree(psec);
> > +		return ERR_CAST(psec->ps);
> > +	}
> > +
> > +	ret = regulator_is_enabled(psec->ps);
> > +	if (ret < 0) {
> > +		kfree(psec);
> > +		return ERR_PTR(ret);
> > +	}
> > +	pcdev->pi[index].enabled = ret;  
> 
> If I see it correctly, it will prevent us to refcount a request from
> user space. So, the runtime PM may suspend PI.

I don't think so as the regulator_get_exclusive() does the same and refcount it:
https://elixir.bootlin.com/linux/v6.7.8/source/drivers/regulator/core.c#L2268

> > +	switch (config->c33_admin_control) {
> > +	case ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED:
> > +		if (!psec->pcdev->pi[psec->id].enabled)
> > +			err = regulator_enable(psec->ps);
> > +		break;
> > +	case ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED:
> > +		if (psec->pcdev->pi[psec->id].enabled)
> > +			err = regulator_disable(psec->ps);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> >  	}  
> 
> This change seems to break PoDL support

Oh that's totally true sorry for that mistake.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com