Message-ID: <e036f7e2-5e5d-40bc-b22c-6dbd6a34eb15@adtran.com>
Date: Wed, 21 May 2025 08:04:23 +0000
From: Piotr Kubik <piotr.kubik@...ran.com>
To: Kory Maincent <kory.maincent@...tlin.com>
CC: Oleksij Rempel <o.rempel@...gutronix.de>, Andrew Lunn
	<andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v3 2/2] net: pse-pd: Add Si3474 PSE controller
 driver

On 5/19/25 11:54, Kory Maincent wrote:
> On Fri, 16 May 2025 13:07:18 +0000
> Piotr Kubik <piotr.kubik@...ran.com> wrote:
> 
>> From: Piotr Kubik <piotr.kubik@...ran.com>
>>
>> Add a driver for the Skyworks Si3474 I2C Power Sourcing Equipment
>> controller.
>>
>> Based on the TPS23881 driver code.
>>
>> Driver supports basic features of Si3474 IC:
>> - get port status,
>> - get port power,
>> - get port voltage,
>> - enable/disable port power.
>>
>> Only 4p configurations are supported at this moment.
> 
> By curiosity, I suppose your hardware have only PoE4 PIs. Could you support and
> test 2p configuration by only configuring 2 of the 4 pairs on one PI?
> 
> Maybe it could be done on a second stage if you prefer.
> 

Yes, exactly. My PSE is hardware 4p type. Some tweaks needs to be done to test/support 2p PSE.
And, yes, I'd prefer to do it in a second stage as a feature.

>>
>> Signed-off-by: Piotr Kubik <piotr.kubik@...ran.com>
> 
> ...
> 
>> +	is_enabled = ((ret & (0x03 << (2 * (chan0 % 4)))) |
>> +		      (ret & (0x03 << (2 * (chan1 % 4))))) != 0;
> 
> There are precedence in the operators. I don't think you need that much of
> parenthesis.
> This should do the work:
> is_enabled = (ret & 0x03 << chan0 % 4 * 2) |
>              (ret & 0x03 << chan1 % 4 * 2) != 0;

Indeed, I left if for readability of what is going on, but your version looks also readable. 
But I'd keep the most surrounding parenthesis as '!=' has precedence over '|' 

is_enabled = ((ret & 0x03 << chan0 % 4 * 2) |
              (ret & 0x03 << chan1 % 4 * 2)) != 0;
> 
> Also I saw that this kind of calculation is made several times. Maybe you could
> add a helper with documentation like I did:
> https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/tps23881.c#L79 

Sure, I will. 

>> +/* Parse pse-pis subnode into chan array of si3474_priv */
>> +static int si3474_get_of_channels(struct si3474_priv *priv)
>> +{
>> +	struct device_node *pse_node;
>> +	struct pse_pi *pi;
>> +	u32 pi_no, chan_id;
>> +	s8 pairset_cnt;
>> +	s32 ret = 0;
>> +
>> +	pse_node = of_get_child_by_name(priv->np, "pse-pis");
>> +	if (!pse_node) {
>> +		dev_warn(&priv->client[0]->dev,
>> +			 "Unable to parse DT PSE power interface matrix, no
>> pse-pis node\n");
>> +		return -EINVAL;
>> +	}
> 
> You should not parse the pse-pis node and subnodes, it is already done before
> the setup_pi_matrix ops call in the core framework.
> https://elixir.bootlin.com/linux/v6.14.7/source/drivers/net/pse-pd/pse_core.c#L131
> 
> You should rather use directly the pcdev->pi[x] table. You have to match the
> the phandle save in pcdev->pi[x].pairset[0/1].np to your channel device node
> and set up the hardware matrix accordingly.

OK, I didn't notice that, will try. 

> 
> That's good to have another development on PSE, this shows me that documentation
> are missing or not enough in PSE core like on this ops. I will update it when I
> have time.
> 
>> +
>> +	for_each_child_of_node_scoped(pse_node, node) {
>> +		if (!of_node_name_eq(node, "pse-pi"))
>> +			continue;
>> +
>> +		ret = of_property_read_u32(node, "reg", &pi_no);
>> +		if (ret) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Failed to read pse-pi reg property\n");
>> +			goto out;
>> +		}
>> +		if (pi_no >= SI3474_MAX_CHANS) {
>> +			dev_err(&priv->client[0]->dev,
>> +				"Invalid power interface number %u\n",
>> pi_no);
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
> 
> ...
> 
>> +static int si3474_pi_enable(struct pse_controller_dev *pcdev, int id)
>> +{
>> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
>> +	struct i2c_client *client;
>> +	u8 chan0, chan1;
>> +	u8 val = 0;
>> +	s32 ret;
>> +
>> +	if (id >= SI3474_MAX_CHANS)
>> +		return -ERANGE;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Release pi from shutdown */
>> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = (u8)ret;
>> +	val |= (0x03 << (2 * (chan0 % 4)));
>> +	val |= (0x03 << (2 * (chan1 % 4)));
> 
> Same calculation as before, use a helper as said before.
> 
>> +
>> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Give time for transition to complete */
>> +	ssleep(1);
> 
> 1s sleep?! It is a lot. Why do you need this? Does it comes from the datasheet?
> 

This comes from my experience. I didn't find it in a datasheet.
I agree this seems a lot, but for 500ms sometimes ports were not powered up. 
I think I'll give a try to another register and instead PB_POWER_ENABLE 
I will try to use PB_RESET in combination with PORT_MODE as this seems promising.

btw. Regarding power enable/disable, I think you may have same issue in tps23881 
as I had here as tps looks very similar to si3474.
For Si3474 POWER_STATUS register cannot be used as an admin state register as it
holds actual power interface status (powered/not powered) instead of its
administrative state (enabled/disabled). 
Ethtool in this approach was showing for both Admin state and Detection status 
always the same state - actual status.
PB_POWER_ENABLE register cannot be used for this purpose as well as it is a write-only register.
That's why I used PORT_MODE register, it acts like an admin state holder in my implementation.

>> +
>> +	/* Trigger pi to power up */
>> +	val = (BIT(chan0 % 4) | BIT(chan1 % 4));
>> +	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
>> +
>> +	return 0;
>> +}
>> +
>> +static int si3474_pi_disable(struct pse_controller_dev *pcdev, int id)
>> +{
>> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
>> +	struct i2c_client *client;
>> +	u8 chan0, chan1;
>> +	u8 val = 0;
>> +	s32 ret;
>> +
>> +	if (id >= SI3474_MAX_CHANS)
>> +		return -ERANGE;
>> +
>> +	chan0 = priv->pi[id].chan[0];
>> +	chan1 = priv->pi[id].chan[1];
>> +
>> +	if (chan0 < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Trigger pi to power down */
>> +	val = (BIT((chan0 % 4) + 4) | BIT((chan1 % 4) + 4));
> 
> This calculation is also used several times. Please use a helper with
> documentation.
> 
>> +	ret = i2c_smbus_write_byte_data(client, PB_POWER_ENABLE_REG, val);
>> +
>> +	/* Shutdown pi */
>> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = (u8)ret;
>> +	val &= ~(0x03 << (2 * (chan0 % 4)));
>> +	val &= ~(0x03 << (2 * (chan1 % 4)));
> 
> Use a helper.
> 
>> +
>> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int si3474_pi_get_chan_current(struct si3474_priv *priv, u8 chan)
>> +{
>> +	struct i2c_client *client;
>> +	s32 ret;
>> +	u8 reg;
>> +	u64 tmp_64;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x30 to 0x3d */
>> +	reg = PORT1_CURRENT_LSB_REG + (chan % 4) * 4;
> 
> Idem
> 
>> +
>> +	ret = i2c_smbus_read_word_data(client, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	tmp_64 = ret * SI3474_NA_STEP;
>> +
>> +	/* uA = nA / 1000 */
>> +	tmp_64 = DIV_ROUND_CLOSEST_ULL(tmp_64, 1000);
>> +	return (int)tmp_64;
>> +}
>> +
>> +static int si3474_pi_get_chan_voltage(struct si3474_priv *priv, u8 chan)
>> +{
>> +	struct i2c_client *client;
>> +	s32 ret;
>> +	u8 reg;
>> +	u32 val;
>> +
>> +	if (chan < 4)
>> +		client = priv->client[0];
>> +	else
>> +		client = priv->client[1];
>> +
>> +	/* Registers 0x32 to 0x3f */
>> +	reg = PORT1_VOLTAGE_LSB_REG + (chan % 4) * 4;
> 
> Idem
> 
>> +
>> +	ret = i2c_smbus_read_word_data(client, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = ret * SI3474_UV_STEP;
>> +
>> +	return (int)val;
>> +}
> 
> Regards,

Thanks Kory!
/Piotr

Powered by blists - more mailing lists