Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

["Köry Maincent" <kory.maincent@bootlin.com>]

On Sat, 18 Nov 2023 19:54:30 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> 
> > +	unsigned long last_cmd_key_time;
> > +
> > +	enum ethtool_pse_admin_state
> > admin_state[PD692X0_MAX_LOGICAL_PORTS]; +};
> > +
> > +/* Template list of the fixed bytes of the communication messages */
> > +static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_CNT]
> > = {
> > +	[PD692X0_MSG_RESET] = {
> > +		.content = {
> > +			.key = PD692X0_KEY_CMD,
> > +			.sub = {0x07, 0x55, 0x00},
> > +			.data = {0x55, 0x00, 0x55, 0x4e,
> > +				 0x4e, 0x4e, 0x4e, 0x4e},
> > +		},
> > +	},  
> 
> Is there any documentation about what all these magic number mean?
> 
> > +/* Implementation of the i2c communication in particular when there is
> > + * a communication loss. See the "Synchronization During Communication
> > Loss"
> > + * paragraph of the Communication Protocol document.
> > + */  
> 
> Is this document public?

Yes:
https://www.microchip.com/en-us/software-library/p3-55-firmware-for-pd69200-for-ieee802-3bt

> 
> > +static int pd692x0_recv_msg(struct pd692x0_priv *priv,
> > +			    struct pd692x0_msg *msg,
> > +			    struct pd692x0_msg_content *buf)
> > +{
> > +	const struct i2c_client *client = priv->client;
> > +	int ret;
> > +
> > +	memset(buf, 0, sizeof(*buf));
> > +	if (msg->delay_recv)
> > +		msleep(msg->delay_recv);
> > +	else
> > +		msleep(30);
> > +
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		goto out;  
> 
> This is the first attempt to receive the message. I assume buf->key
> not being 0 indicates something has been received?

Yes, 

> 
> > +
> > +	msleep(100);
> > +
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		goto out;  
> 
> So this is a second attempt. Should there be another memset? Could the
> first failed transfer fill the buffer with random junk in the higher
> bytes, and a successful read here could be a partial read and the end
> of the buffer still contains the junk.

Not sure. The message read should have the right size.
I will maybe add the memset to prevent any weird behavior.

> Another resend and two more attempts to receive.
> 
> Is there a reason to not uses for loops here? And maybe put
> send/receive/receive into a helper? And maybe make the first send part
> of this, rather then separate? I think the code will be more readable
> when restructured.

I have written it like that to describe literally the communication loss
procedure. I may restructured it for better understanding.

> > +static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
> > +				      unsigned long id,
> > +				      struct netlink_ext_ack *extack,
> > +				      const struct pse_control_config
> > *config) +{
> > +	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
> > +	struct pd692x0_msg_content buf = {0};
> > +	struct pd692x0_msg msg;
> > +	int ret;
> > +
> > +	ret = pd692x0_fw_unavailable(priv);
> > +	if (ret)
> > +		return ret;  
> 
> It seems a bit late to check if the device has any firmware. I would
> of expected probe to check that, and maybe attempt to download
> firmware. If that fails, fail the probe, since the PSE is a brick.

The PSE can still be flashed it never become an unusable brick.
We can flash the wrong Firmware, or having issue in the flashing process. This
way we could reflash the controller firmware several times. 

> > +static struct pd692x0_msg_ver pd692x0_get_sw_version(struct pd692x0_priv
> > *priv) +{
> > +	struct pd692x0_msg msg =
> > pd692x0_msg_template_list[PD692X0_MSG_GET_SW_VER];
> > +	struct device *dev = &priv->client->dev;
> > +	struct pd692x0_msg_content buf = {0};
> > +	struct pd692x0_msg_ver ver = {0};
> > +	int ret;
> > +
> > +	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to get PSE version (%pe)\n",
> > ERR_PTR(ret));
> > +		return ver;  
> 
> I _think_ that return is wrong ???

No, this return will return an empty struct pd692x0_msg_ver.
Which means the firmware has not any version.

> Is the firmware in Motorola SREC format? I thought the kernel had a
> helper for that, but a quick search did not find it. So maybe i'm
> remembering wrongly. But it seems silly for every driver to implement
> an SREC parser.

Oh, I didn't know this format. The firmware seems indeed to match this format
specification.
I found two reference of this Firmware format in the kernel:
https://elixir.bootlin.com/linux/v6.5.7/source/sound/soc/codecs/zl38060.c#L178
https://elixir.bootlin.com/linux/v6.5.7/source/drivers/staging/wlan-ng/prism2fw.c

Any preference in the choice? The zl38060 fw usage is maybe the easiest.

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