| Message ID | 20231116-feature_poe-v1-9-be48044bf249@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Add support for Power over Ethernet (PoE) | expand |
On 16/11/2023 15:01, Kory Maincent wrote: > Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller. > This driver only support i2c communication for now. > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > --- > MAINTAINERS | 1 + > drivers/net/pse-pd/Kconfig | 11 + > drivers/net/pse-pd/Makefile | 1 + > drivers/net/pse-pd/pd692x0.c | 1049 ++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1062 insertions(+) .... > + > +err_fw_unregister: > + firmware_upload_unregister(priv->fwl); > + return ret; > +} > + > +static void pd692x0_i2c_remove(struct i2c_client *client) > +{ > + struct pd692x0_priv *priv = i2c_get_clientdata(client); > + > + firmware_upload_unregister(priv->fwl); > +} > + > +static const struct i2c_device_id pd692x0_id[] = { > + { PD692X0_PSE_NAME, 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, pd692x0_id); > + > +static const struct of_device_id pd692x0_of_match[] = { > + { .compatible = "microchip,pd69200", }, > + { .compatible = "microchip,pd69210", }, > + { .compatible = "microchip,pd69220", }, So they are the same from driver point of view. > + { }, > +}; > +MODULE_DEVICE_TABLE(of, pd692x0_of_match); > + > +static struct i2c_driver pd692x0_driver = { > + .probe = pd692x0_i2c_probe, > + .remove = pd692x0_i2c_remove, > + .id_table = pd692x0_id, > + .driver = { > + .name = PD692X0_PSE_NAME, > + .of_match_table = of_match_ptr(pd692x0_of_match), Drop of_match_ptr, leads to warnings. Best regards, Krzysztof
Thanks Krzysztof for your reviews! On Thu, 16 Nov 2023 15:29:24 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 16/11/2023 15:01, Kory Maincent wrote: > > Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller. > > This driver only support i2c communication for now. > > > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > > --- > > MAINTAINERS | 1 + > > drivers/net/pse-pd/Kconfig | 11 + > > drivers/net/pse-pd/Makefile | 1 + > > drivers/net/pse-pd/pd692x0.c | 1049 > > ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1062 > > insertions(+) > > .... > > > + > > +err_fw_unregister: > > + firmware_upload_unregister(priv->fwl); > > + return ret; > > +} > > + > > +static void pd692x0_i2c_remove(struct i2c_client *client) > > +{ > > + struct pd692x0_priv *priv = i2c_get_clientdata(client); > > + > > + firmware_upload_unregister(priv->fwl); > > +} > > + > > +static const struct i2c_device_id pd692x0_id[] = { > > + { PD692X0_PSE_NAME, 0 }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(i2c, pd692x0_id); > > + > > +static const struct of_device_id pd692x0_of_match[] = { > > + { .compatible = "microchip,pd69200", }, > > + { .compatible = "microchip,pd69210", }, > > + { .compatible = "microchip,pd69220", }, > > So they are the same from driver point of view. Yes. I only have the pd69200 version but the three versions are theoretically compatible and microchip advise obviously to use the last one. I describe the three names in case of future specific things even if I hope there won't be and to have a clear version of which version is supported. Do you prefer to use pd692x0 compatible instead? Regards,
> +static int pd692x0_send_msg(struct pd692x0_priv *priv, struct pd692x0_msg *msg) > +{ > + const struct i2c_client *client = priv->client; > + int ret; > + > + if (msg->content.key == PD692X0_KEY_CMD && priv->last_cmd_key) { > + while (time_is_after_jiffies(msecs_to_jiffies(30) + priv->last_cmd_key_time)) > + usleep_range(1000, 2000); That is a bit odd. Could you not just calculate how long a sleep is needed, rather than loop? Andrew
> +struct pd692x0_msg { > + struct pd692x0_msg_content content; > + u16 delay_recv; > +}; > + if (msg->delay_recv) > + msleep(msg->delay_recv); > + else > + msleep(30); > + if (msg->delay_recv) > + msleep(msg->delay_recv); > + else > + msleep(30); > + if (msg->delay_recv) > + msleep(msg->delay_recv); > + else > + msleep(30); > + As far as i can see with a quick search, nothing ever sets delay_recv? Andrew
I'm reading this patch first, so this might be a dumb question... > +static int pd692x0_recv_msg(struct pd692x0_priv *priv, > + struct pd692x0_msg *msg, > + struct pd692x0_msg_content *buf) > +{ ... > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); > + if (buf->key) > + goto out; > + > + msleep(10000); That is 10 seconds, right? > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv, > + struct pd692x0_msg *msg, > + struct pd692x0_msg_content *buf) > +{ > + struct device *dev = &priv->client->dev; > + int ret; > + > + ret = pd692x0_send_msg(priv, msg); > + if (ret) > + return ret; > + > + ret = pd692x0_recv_msg(priv, msg, buf); So this function takes at least 10 seconds? > +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) > +{ .... > + switch (config->admin_control) { > + case ETHTOOL_PSE_ADMIN_STATE_ENABLED: > + msg.content.data[0] = 0x1; > + break; > + case ETHTOOL_PSE_ADMIN_STATE_DISABLED: > + msg.content.data[0] = 0x0; > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + msg.content.sub[2] = id; > + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); So this is also 10 seconds? Given its name, it looks like this is called via ethtool? Is the ethtool core holding RTNL? It is generally considered bad to hold RTNL for that long. Andrew
On Thu, 16 Nov 2023 23:38:08 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > +static int pd692x0_send_msg(struct pd692x0_priv *priv, struct pd692x0_msg > > *msg) +{ > > + const struct i2c_client *client = priv->client; > > + int ret; > > + > > + if (msg->content.key == PD692X0_KEY_CMD && priv->last_cmd_key) { > > + while (time_is_after_jiffies(msecs_to_jiffies(30) + > > priv->last_cmd_key_time)) > > + usleep_range(1000, 2000); > > That is a bit odd. Could you not just calculate how long a sleep is > needed, rather than loop? Oh, right indeed! Don't know why my brain wanted a loop here. Regards,
Thanks for your review! On Thu, 16 Nov 2023 23:41:55 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > +struct pd692x0_msg { > > + struct pd692x0_msg_content content; > > + u16 delay_recv; > > +}; > > > + if (msg->delay_recv) > > + msleep(msg->delay_recv); > > + else > > + msleep(30); > > > + if (msg->delay_recv) > > + msleep(msg->delay_recv); > > + else > > + msleep(30); > > > + if (msg->delay_recv) > > + msleep(msg->delay_recv); > > + else > > + msleep(30); > > + > > As far as i can see with a quick search, nothing ever sets delay_recv? > > Andrew In fact I wrote the driver taking into account that there are two commands (save and restore) that need a different delay response. As currently we do not support them I can indeed drop it for now and add it back when I will add their support. Regards,
On Fri, Nov 17, 2023 at 12:22:36PM +0100, Köry Maincent wrote: > Thanks for your review! > > On Thu, 16 Nov 2023 23:41:55 +0100 > Andrew Lunn <andrew@lunn.ch> wrote: > > > > +struct pd692x0_msg { > > > + struct pd692x0_msg_content content; > > > + u16 delay_recv; > > > +}; > > > > > + if (msg->delay_recv) > > > + msleep(msg->delay_recv); > > > + else > > > + msleep(30); > > > > > + if (msg->delay_recv) > > > + msleep(msg->delay_recv); > > > + else > > > + msleep(30); > > > > > + if (msg->delay_recv) > > > + msleep(msg->delay_recv); > > > + else > > > + msleep(30); > > > + > > > > As far as i can see with a quick search, nothing ever sets delay_recv? > > > > Andrew > > In fact I wrote the driver taking into account that there are two commands (save > and restore) that need a different delay response. As currently we do not > support them I can indeed drop it for now and add it back when I will add their > support. When you add it back, maybe just arrange for delay_recv to be set to 30? Andrew
> +struct pd692x0_priv { > + struct i2c_client *client; > + struct pse_controller_dev pcdev; > + > + enum pd692x0_fw_state fw_state; > + struct fw_upload *fwl; > + bool cancel_request:1; > + > + u8 msg_id; > + bool last_cmd_key:1; Does a bool bit field of size 1 make any sense? I would also put the two bitfields next to each other, and the compiler might then pack them into the same word. The base type of a u8 would allow the compile to put it next to the msg_id without any padding. > + 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? > +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? > + > + 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. > + > + ret = pd692x0_send_msg(priv, msg); > + if (ret) > + return ret; So now we are re-transmitting the request. > + > + if (msg->delay_recv) > + msleep(msg->delay_recv); > + else > + msleep(30); > + > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); > + if (buf->key) > + goto out; > + > + msleep(100); > + > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); > + if (buf->key) > + goto out; > + > + msleep(10000); And two more attemps to receive it. > + > + ret = pd692x0_send_msg(priv, msg); > + if (ret) > + return ret; > + > + if (msg->delay_recv) > + msleep(msg->delay_recv); > + else > + msleep(30); > + > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); > + if (buf->key) > + goto out; > + > + msleep(100); > + > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); > + if (buf->key) > + goto out; 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. > +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. > +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 ??? > +static enum fw_upload_err pd692x0_fw_write(struct fw_upload *fwl, > + const u8 *data, u32 offset, > + u32 size, u32 *written) > +{ > + struct pd692x0_priv *priv = fwl->dd_handle; > + char line[PD692X0_FW_LINE_MAX_SZ]; > + const struct i2c_client *client; > + int ret, i; > + char cmd; > + > + client = priv->client; > + priv->fw_state = PD692X0_FW_WRITE; > + > + /* Erase */ > + cmd = 'E'; > + ret = i2c_master_send(client, &cmd, 1); > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to boot programming mode (%pe)\n", > + ERR_PTR(ret)); > + return FW_UPLOAD_ERR_RW_ERROR; > + } > + > + ret = pd692x0_fw_recv_resp(client, 100, "TOE\r\n", sizeof("TOE\r\n") - 1); > + if (ret) > + return ret; > + > + ret = pd692x0_fw_recv_resp(client, 5000, "TE\r\n", sizeof("TE\r\n") - 1); > + if (ret) > + dev_warn(&client->dev, > + "Failed to erase internal memory, however still try to write Firmware\n"); > + > + ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1); > + if (ret) > + dev_warn(&client->dev, > + "Failed to erase internal memory, however still try to write Firmware\n"); > + > + if (priv->cancel_request) > + return FW_UPLOAD_ERR_CANCELED; > + > + /* Program */ > + cmd = 'P'; > + ret = i2c_master_send(client, &cmd, sizeof(char)); > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to boot programming mode (%pe)\n", > + ERR_PTR(ret)); > + return ret; > + } > + > + ret = pd692x0_fw_recv_resp(client, 100, "TOP\r\n", sizeof("TOP\r\n") - 1); > + if (ret) > + return ret; > + > + i = 0; > + while (i < size) { > + ret = pd692x0_fw_get_next_line(data, line, size - i); > + if (!ret) { > + ret = FW_UPLOAD_ERR_FW_INVALID; > + goto err; > + } > + > + i += ret; > + data += ret; > + if (line[0] == 'S' && line[1] == '0') { > + continue; > + } else if (line[0] == 'S' && line[1] == '7') { > + ret = pd692x0_fw_write_line(client, line, true); > + if (ret) > + goto err; 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. Andrew
Thanks for your reviews! On Thu, 16 Nov 2023 23:54:02 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > I'm reading this patch first, so this might be a dumb question... > > > +static int pd692x0_recv_msg(struct pd692x0_priv *priv, > > + struct pd692x0_msg *msg, > > + struct pd692x0_msg_content *buf) > > +{ > > ... > > > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); > > + if (buf->key) > > + goto out; > > + > > + msleep(10000); > > That is 10 seconds, right? Yes, it is in the communication loss procedure. > > > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv, > > + struct pd692x0_msg *msg, > > + struct pd692x0_msg_content *buf) > > +{ > > + struct device *dev = &priv->client->dev; > > + int ret; > > + > > + ret = pd692x0_send_msg(priv, msg); > > + if (ret) > > + return ret; > > + > > + ret = pd692x0_recv_msg(priv, msg, buf); > > So this function takes at least 10 seconds? No, on normal communication it takes a bit more than 30ms. It could be more if there are communication loss, even reset the controller. See the communication loss procedure in "PD692x0 BT Serial Communication Protocol User Guide" for details. > > +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) +{ > > .... > > > + switch (config->admin_control) { > > + case ETHTOOL_PSE_ADMIN_STATE_ENABLED: > > + msg.content.data[0] = 0x1; > > + break; > > + case ETHTOOL_PSE_ADMIN_STATE_DISABLED: > > + msg.content.data[0] = 0x0; > > + break; > > + default: > > + return -EOPNOTSUPP; > > + } > > + > > + msg.content.sub[2] = id; > > + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); > > So this is also 10 seconds? > > Given its name, it looks like this is called via ethtool? Is the > ethtool core holding RTNL? It is generally considered bad to hold RTNL for > that long. Yes it is holding RTNL lock. Should I consider another behavior in case of communication loss to not holding RTNL lock so long? Regards,
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,
> > > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv, > > > + struct pd692x0_msg *msg, > > > + struct pd692x0_msg_content *buf) > > > +{ > > > + struct device *dev = &priv->client->dev; > > > + int ret; > > > + > > > + ret = pd692x0_send_msg(priv, msg); > > > + if (ret) > > > + return ret; > > > + > > > + ret = pd692x0_recv_msg(priv, msg, buf); > > > > So this function takes at least 10 seconds? > > No, on normal communication it takes a bit more than 30ms. So i think the first step is to refactor this code to make it clear what the normal path is, and what the exception path is, and the timing of each. > > > + msg.content.sub[2] = id; > > > + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); > > > > So this is also 10 seconds? > > > > Given its name, it looks like this is called via ethtool? Is the > > ethtool core holding RTNL? It is generally considered bad to hold RTNL for > > that long. > > Yes it is holding RTNL lock. Should I consider another behavior in case of > communication loss to not holding RTNL lock so long? How often does it happen? On the scale of its a theoretical possibility, through to it happens every N calls? Also, does it happen on cold boot and reboot? If its never supposed to happen, i would keep holding RTNL, and add a pr_warn() that the PSE has crashed and burned, waiting for it to reboot. If this is likely to happen on the first communication with the device, we might want to do a dummy transfer during probe to get is synchronized before we start using it with the RTNL held. Andrew
> > 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. Its often used in small deeply embedded systems. Microcontrollers, rather than something which can run Linux. > 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 Ah, all inside a header file. Probably why i missed it. But ihex is not SREC. ihex came from Intel. SREC from Motorola. So i would follow the basic flow in include/linux/ihex.h, add an include/linux/srec.h but adapt it for SREC. Andrew
On Wed, 22 Nov 2023 17:54:53 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > > > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv, > > > > + struct pd692x0_msg *msg, > > > > + struct pd692x0_msg_content *buf) > > > > +{ > > > > + struct device *dev = &priv->client->dev; > > > > + int ret; > > > > + > > > > + ret = pd692x0_send_msg(priv, msg); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = pd692x0_recv_msg(priv, msg, buf); > > > > > > So this function takes at least 10 seconds? > > > > No, on normal communication it takes a bit more than 30ms. > > So i think the first step is to refactor this code to make it clear > what the normal path is, and what the exception path is, and the > timing of each. Ok I will try to refactor it to makes it more readable. > > > > + msg.content.sub[2] = id; > > > > + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); > > > > > > So this is also 10 seconds? > > > > > > Given its name, it looks like this is called via ethtool? Is the > > > ethtool core holding RTNL? It is generally considered bad to hold RTNL for > > > that long. > > > > Yes it is holding RTNL lock. Should I consider another behavior in case of > > communication loss to not holding RTNL lock so long? > > How often does it happen? On the scale of its a theoretical > possibility, through to it happens every N calls? Also, does it happen > on cold boot and reboot? > > If its never supposed to happen, i would keep holding RTNL, and add a > pr_warn() that the PSE has crashed and burned, waiting for it to > reboot. If this is likely to happen on the first communication with > the device, we might want to do a dummy transfer during probe to get > is synchronized before we start using it with the RTNL held. I would say it never supposed to happen. I never faced the issue playing with the controller. The first communication is a simple i2c_master_recv of the controller status without entering the pd692x0_sendrecv_msg function, therefore it won't be an issue. Another solution could be to raise a flag if I enter in communication loss and release the rtnlock. We would lock again the rtnl when the flags got disabled. The controler won't be accessible until the flag is disabled. Regards,
> I would say it never supposed to happen. > I never faced the issue playing with the controller. The first communication is > a simple i2c_master_recv of the controller status without entering the > pd692x0_sendrecv_msg function, therefore it won't be an issue. Great. Do a pr_info() or similar and keep holding the lock. KISS. Andrew
On Thu, Nov 16, 2023 at 03:01:41PM +0100, Kory Maincent wrote: > Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller. > This driver only support i2c communication for now. > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> Hi Kory, some minor feedback from my side. ... > diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c ... > +static int > +pd692x0_get_of_matrix(struct device *dev, > + struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS]) > +{ > + int ret, i, ports_matrix_size; > + u32 val[PD692X0_MAX_LOGICAL_PORTS * 3]; nit: reverse xmas tree please. ... > +static int pd692x0_fw_write_line(const struct i2c_client *client, > + const char line[PD692X0_FW_LINE_MAX_SZ], > + const bool last_line) > +{ > + int ret; > + > + while (*line != 0) { > + ret = i2c_master_send(client, line, 1); > + if (ret < 0) > + return FW_UPLOAD_ERR_RW_ERROR; > + line++; > + } > + > + if (last_line) { > + ret = pd692x0_fw_recv_resp(client, 100, "TP\r\n", > + sizeof("TP\r\n") - 1); > + if (ret) > + return ret; > + } else { > + ret = pd692x0_fw_recv_resp(client, 100, "T*\r\n", > + sizeof("T*\r\n") - 1); > + if (ret) > + return ret; > + } > + > + return FW_UPLOAD_ERR_NONE; > +} ... > +static enum fw_upload_err pd692x0_fw_write(struct fw_upload *fwl, > + const u8 *data, u32 offset, > + u32 size, u32 *written) > +{ > + struct pd692x0_priv *priv = fwl->dd_handle; > + char line[PD692X0_FW_LINE_MAX_SZ]; > + const struct i2c_client *client; > + int ret, i; > + char cmd; > + > + client = priv->client; > + priv->fw_state = PD692X0_FW_WRITE; > + > + /* Erase */ > + cmd = 'E'; > + ret = i2c_master_send(client, &cmd, 1); > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to boot programming mode (%pe)\n", > + ERR_PTR(ret)); > + return FW_UPLOAD_ERR_RW_ERROR; > + } > + > + ret = pd692x0_fw_recv_resp(client, 100, "TOE\r\n", sizeof("TOE\r\n") - 1); > + if (ret) > + return ret; > + > + ret = pd692x0_fw_recv_resp(client, 5000, "TE\r\n", sizeof("TE\r\n") - 1); > + if (ret) > + dev_warn(&client->dev, > + "Failed to erase internal memory, however still try to write Firmware\n"); > + > + ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1); > + if (ret) > + dev_warn(&client->dev, > + "Failed to erase internal memory, however still try to write Firmware\n"); > + > + if (priv->cancel_request) > + return FW_UPLOAD_ERR_CANCELED; > + > + /* Program */ > + cmd = 'P'; > + ret = i2c_master_send(client, &cmd, sizeof(char)); > + if (ret < 0) { > + dev_err(&client->dev, > + "Failed to boot programming mode (%pe)\n", > + ERR_PTR(ret)); > + return ret; > + } > + > + ret = pd692x0_fw_recv_resp(client, 100, "TOP\r\n", sizeof("TOP\r\n") - 1); > + if (ret) > + return ret; > + > + i = 0; > + while (i < size) { > + ret = pd692x0_fw_get_next_line(data, line, size - i); > + if (!ret) { > + ret = FW_UPLOAD_ERR_FW_INVALID; > + goto err; > + } > + > + i += ret; > + data += ret; > + if (line[0] == 'S' && line[1] == '0') { > + continue; > + } else if (line[0] == 'S' && line[1] == '7') { > + ret = pd692x0_fw_write_line(client, line, true); > + if (ret) > + goto err; > + } else { > + ret = pd692x0_fw_write_line(client, line, false); > + if (ret) > + goto err; > + } > + > + if (priv->cancel_request) { > + ret = FW_UPLOAD_ERR_CANCELED; > + goto err; > + } > + } > + *written = i; > + > + msleep(400); > + > + return FW_UPLOAD_ERR_NONE; > + > +err: > + pd692x0_fw_write_line(client, "S7\r\n", true); gcc-13 (W=1) seems a bit upset about this. drivers/net/pse-pd/pd692x0.c: In function 'pd692x0_fw_write': drivers/net/pse-pd/pd692x0.c:861:9: warning: 'pd692x0_fw_write_line' reading 128 bytes from a region of size 5 [-Wstringop-overread] 861 | pd692x0_fw_write_line(client, "S7\r\n", true); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/pse-pd/pd692x0.c:861:9: note: referencing argument 2 of type 'const char[128]' drivers/net/pse-pd/pd692x0.c:642:12: note: in a call to function 'pd692x0_fw_write_line' 642 | static int pd692x0_fw_write_line(const struct i2c_client *client, | ^~~~~~~~~~~~~~~~~~~~~ > + return ret; > +} ...
On Wed, 22 Nov 2023 18:11:25 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > > 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. > > Its often used in small deeply embedded systems. Microcontrollers, > rather than something which can run Linux. > > > 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 > > > > Ah, all inside a header file. Probably why i missed it. But ihex is > not SREC. ihex came from Intel. SREC from Motorola. > > So i would follow the basic flow in include/linux/ihex.h, add an > include/linux/srec.h but adapt it for SREC. In fact the ihex.h header is only adding the ihex_validate_fw and the request_ihex_firmware functions. In my case I do not use request firmware but sysfs firmware loader. I could add srec_validate_fw but I am already checking the firmware during the flashing process due to its special flashing process. I could not treat the firmware to one blob to be send. Each byte need to be send in one i2c messages and at the end of eaxch line we need to wait a "\r\n" (within 30ms) before sending next line. Yes, it takes time to be flashed! Do you see generic helper that I could add? Regards,
diff --git a/MAINTAINERS b/MAINTAINERS index 1e154dacef67..34cf007965a8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14253,6 +14253,7 @@ M: Kory Maincent <kory.maincent@bootlin.com> L: netdev@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml +F: drivers/net/pse-pd/pd69200.c MICROCHIP POLARFIRE FPGA DRIVERS M: Conor Dooley <conor.dooley@microchip.com> diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig index 687dec49c1e1..e3a6ba669f20 100644 --- a/drivers/net/pse-pd/Kconfig +++ b/drivers/net/pse-pd/Kconfig @@ -20,4 +20,15 @@ config PSE_REGULATOR Sourcing Equipment without automatic classification support. For example for basic implementation of PoDL (802.3bu) specification. +config PSE_PD692X0 + tristate "PD692X0 PSE controller" + depends on I2C + select FW_UPLOAD + help + This module provides support for PD692x0 regulator based Ethernet + Power Sourcing Equipment. + + To compile this driver as a module, choose M here: the + module will be called pd692x0. + endif diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile index 1b8aa4c70f0b..9c12c4a65730 100644 --- a/drivers/net/pse-pd/Makefile +++ b/drivers/net/pse-pd/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o +obj-$(CONFIG_PSE_PD692X0) += pd692x0.o diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c new file mode 100644 index 000000000000..f6b8f89a3afe --- /dev/null +++ b/drivers/net/pse-pd/pd692x0.c @@ -0,0 +1,1049 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Driver for the Microchip PD692X0 PoE PSE Controller driver (I2C bus) + * + * Copyright (c) 2023 Bootlin, Kory Maincent <kory.maincent@bootlin.com> + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pse-pd/pse.h> +#include <linux/i2c.h> +#include <linux/delay.h> +#include <linux/firmware.h> + +#define PD692X0_PSE_NAME "pd692x0_pse" + +#define PD692X0_MAX_LOGICAL_PORTS 48 +#define PD692X0_MAX_HW_PORTS 96 + +#define PD69200_BT_PROD_VER 24 +#define PD69210_BT_PROD_VER 26 +#define PD69220_BT_PROD_VER 29 + +#define PD692X0_FW_MAJ_VER 3 +#define PD692X0_FW_MIN_VER 5 +#define PD692X0_FW_PATCH_VER 5 + +enum pd692x0_fw_state { + PD692X0_FW_UNKNOWN, + PD692X0_FW_OK, + PD692X0_FW_BROKEN, + PD692X0_FW_NEED_UPDATE, + PD692X0_FW_PREPARE, + PD692X0_FW_WRITE, + PD692X0_FW_COMPLETE, +}; + +struct pd692x0_msg_content { + u8 key; + u8 echo; + u8 sub[3]; + u8 data[8]; + __be16 chksum; +} __packed; + +struct pd692x0_msg_ver { + u8 prod; + u8 maj_sw_ver; + u8 min_sw_ver; + u8 pa_sw_ver; + u8 param; + u8 build; +}; + +enum { + PD692X0_KEY_CMD, + PD692X0_KEY_PRG, + PD692X0_KEY_REQ, + PD692X0_KEY_TLM, + PD692X0_KEY_TEST, + PD692X0_KEY_REPORT = 0x52 +}; + +enum { + PD692X0_MSG_RESET, + PD692X0_MSG_GET_SW_VER, + PD692X0_MSG_SET_TMP_PORT_MATRIX, + PD692X0_MSG_PRG_PORT_MATRIX, + PD692X0_MSG_SET_PORT_PARAM, + PD692X0_MSG_GET_PORT_STATUS, + PD692X0_MSG_DOWNLOAD_CMD, + + /* add new message above here */ + PD692X0_MSG_CNT +}; + +struct pd692x0_msg { + struct pd692x0_msg_content content; + u16 delay_recv; +}; + +struct pd692x0_priv { + struct i2c_client *client; + struct pse_controller_dev pcdev; + + enum pd692x0_fw_state fw_state; + struct fw_upload *fwl; + bool cancel_request:1; + + u8 msg_id; + bool last_cmd_key:1; + 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}, + }, + }, + [PD692X0_MSG_GET_SW_VER] = { + .content = { + .key = PD692X0_KEY_REQ, + .sub = {0x07, 0x1e, 0x21}, + .data = {0x4e, 0x4e, 0x4e, 0x4e, + 0x4e, 0x4e, 0x4e, 0x4e}, + }, + }, + [PD692X0_MSG_SET_TMP_PORT_MATRIX] = { + .content = { + .key = PD692X0_KEY_CMD, + .sub = {0x05, 0x43}, + .data = { 0, 0x4e, 0x4e, 0x4e, + 0x4e, 0x4e, 0x4e, 0x4e}, + }, + }, + [PD692X0_MSG_PRG_PORT_MATRIX] = { + .content = { + .key = PD692X0_KEY_CMD, + .sub = {0x07, 0x43, 0x4e}, + .data = {0x4e, 0x4e, 0x4e, 0x4e, + 0x4e, 0x4e, 0x4e, 0x4e}, + }, + }, + [PD692X0_MSG_SET_PORT_PARAM] = { + .content = { + .key = PD692X0_KEY_CMD, + .sub = {0x05, 0xc0}, + .data = { 0, 0xff, 0xff, 0xff, + 0x4e, 0x4e, 0x4e, 0x4e}, + }, + }, + [PD692X0_MSG_GET_PORT_STATUS] = { + .content = { + .key = PD692X0_KEY_REQ, + .sub = {0x05, 0xc1}, + .data = {0x4e, 0x4e, 0x4e, 0x4e, + 0x4e, 0x4e, 0x4e, 0x4e}, + }, + }, + [PD692X0_MSG_DOWNLOAD_CMD] = { + .content = { + .key = PD692X0_KEY_PRG, + .sub = {0xff, 0x99, 0x15}, + .data = {0x16, 0x16, 0x99, 0x4e, + 0x4e, 0x4e, 0x4e, 0x4e}, + }, + }, +}; + +static u8 pd692x0_build_msg(struct pd692x0_msg_content *msg, u8 echo) +{ + u8 *data = (u8 *)msg; + u16 chksum = 0; + int i; + + msg->echo = echo++; + if (echo == 0xff) + echo = 0; + + for (i = 0; i < sizeof(*msg) - sizeof(msg->chksum); i++) + chksum += data[i]; + + msg->chksum = cpu_to_be16(chksum); + + return echo; +} + +static int pd692x0_send_msg(struct pd692x0_priv *priv, struct pd692x0_msg *msg) +{ + const struct i2c_client *client = priv->client; + int ret; + + if (msg->content.key == PD692X0_KEY_CMD && priv->last_cmd_key) { + while (time_is_after_jiffies(msecs_to_jiffies(30) + priv->last_cmd_key_time)) + usleep_range(1000, 2000); + } + + /* Add echo and checksum bytes to the message */ + priv->msg_id = pd692x0_build_msg(&msg->content, priv->msg_id); + + ret = i2c_master_send(client, (u8 *)&msg->content, + sizeof(msg->content)); + if (ret != sizeof(msg->content)) + return -EIO; + + return 0; +} + +static int pd692x0_reset(struct pd692x0_priv *priv) +{ + struct pd692x0_msg msg = pd692x0_msg_template_list[PD692X0_MSG_RESET]; + const struct i2c_client *client = priv->client; + struct pd692x0_msg_content buf = {0}; + int ret; + + ret = pd692x0_send_msg(priv, &msg); + if (ret) { + dev_err(&client->dev, + "Failed to reset the controller (%pe)\n", ERR_PTR(ret)); + return ret; + } + + msleep(30); + + ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf)); + if (ret != sizeof(buf)) + return ret < 0 ? ret : -EIO; + + /* Is the reply a successful report message */ + if (buf.key != PD692X0_KEY_REPORT || buf.sub[0] || buf.sub[1]) + return -EIO; + + msleep(300); + + ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf)); + if (ret != sizeof(buf)) + return ret < 0 ? ret : -EIO; + + /* Is the boot status without error */ + if (buf.key != 0x03 || buf.echo != 0xff || buf.sub[0] & 0x1) { + dev_err(&client->dev, "PSE controller error\n"); + return -EIO; + } + + return 0; +} + +/* 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. + */ +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; + + msleep(100); + + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); + if (buf->key) + goto out; + + ret = pd692x0_send_msg(priv, msg); + if (ret) + return ret; + + if (msg->delay_recv) + msleep(msg->delay_recv); + else + msleep(30); + + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); + if (buf->key) + goto out; + + msleep(100); + + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); + if (buf->key) + goto out; + + msleep(10000); + + ret = pd692x0_send_msg(priv, msg); + if (ret) + return ret; + + if (msg->delay_recv) + msleep(msg->delay_recv); + else + msleep(30); + + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); + if (buf->key) + goto out; + + msleep(100); + + i2c_master_recv(client, (u8 *)buf, sizeof(*buf)); + if (buf->key) + goto out; + + return pd692x0_reset(priv); + +out: + if (msg->content.key == PD692X0_KEY_CMD) { + priv->last_cmd_key = true; + priv->last_cmd_key_time = jiffies; + } else { + priv->last_cmd_key = false; + } + + return 0; +} + +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv, + struct pd692x0_msg *msg, + struct pd692x0_msg_content *buf) +{ + struct device *dev = &priv->client->dev; + int ret; + + ret = pd692x0_send_msg(priv, msg); + if (ret) + return ret; + + ret = pd692x0_recv_msg(priv, msg, buf); + if (ret) + return ret; + + if (msg->content.echo != buf->echo) { + dev_err(dev, "Wrong match in message ID\n"); + return -EIO; + } + + /* If the reply is a report message is it successful */ + if (buf->key == PD692X0_KEY_REPORT && + (buf->sub[0] || buf->sub[1])) { + return -EIO; + } + + return 0; +} + +static struct pd692x0_priv *to_pd692x0_priv(struct pse_controller_dev *pcdev) +{ + return container_of(pcdev, struct pd692x0_priv, pcdev); +} + +static int pd692x0_fw_unavailable(struct pd692x0_priv *priv) +{ + switch (priv->fw_state) { + case PD692X0_FW_OK: + return 0; + case PD692X0_FW_PREPARE: + case PD692X0_FW_WRITE: + case PD692X0_FW_COMPLETE: + dev_err(&priv->client->dev, "Firmware update in progress!\n"); + return -EBUSY; + case PD692X0_FW_BROKEN: + case PD692X0_FW_NEED_UPDATE: + default: + dev_err(&priv->client->dev, + "Firmware issue. Please update it!\n"); + return -EOPNOTSUPP; + } +} + +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; + + if (priv->admin_state[id] == config->admin_control) + return 0; + + msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM]; + switch (config->admin_control) { + case ETHTOOL_PSE_ADMIN_STATE_ENABLED: + msg.content.data[0] = 0x1; + break; + case ETHTOOL_PSE_ADMIN_STATE_DISABLED: + msg.content.data[0] = 0x0; + break; + default: + return -EOPNOTSUPP; + } + + msg.content.sub[2] = id; + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); + if (ret < 0) + return ret; + + priv->admin_state[id] = config->admin_control; + + return 0; +} + +static int pd692x0_ethtool_get_status(struct pse_controller_dev *pcdev, + unsigned long id, + struct netlink_ext_ack *extack, + struct pse_control_status *status) +{ + 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; + + msg = pd692x0_msg_template_list[PD692X0_MSG_GET_PORT_STATUS]; + msg.content.sub[2] = id; + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); + if (ret < 0) + return ret; + + /* Compare Port Status (Communication Protocol Document par. 7.1) */ + if ((buf.sub[0] & 0xf0) == 0x80 || (buf.sub[0] & 0xf0) == 0x90) + status->pw_status = ETHTOOL_PSE_PW_D_STATUS_DELIVERING; + else if (buf.sub[0] == 0x1b || buf.sub[0] == 0x22) + status->pw_status = ETHTOOL_PSE_PW_D_STATUS_SEARCHING; + else if (buf.sub[0] == 0x12) + status->pw_status = ETHTOOL_PSE_PW_D_STATUS_FAULT; + else + status->pw_status = ETHTOOL_PSE_PW_D_STATUS_DISABLED; + + if (buf.sub[1]) + status->admin_state = ETHTOOL_PSE_ADMIN_STATE_ENABLED; + else + status->admin_state = ETHTOOL_PSE_ADMIN_STATE_DISABLED; + + priv->admin_state[id] = status->admin_state; + + return 0; +} + +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; + } + + /* Extract version from the message */ + ver.prod = buf.sub[2]; + ver.maj_sw_ver = (buf.data[0] << 8 | buf.data[1]) / 100; + ver.min_sw_ver = ((buf.data[0] << 8 | buf.data[1]) / 10) % 10; + ver.pa_sw_ver = (buf.data[0] << 8 | buf.data[1]) % 10; + ver.param = buf.data[2]; + ver.build = buf.data[3]; + + return ver; +} + +static const struct pse_controller_ops pd692x0_ops = { + .ethtool_get_status = pd692x0_ethtool_get_status, + .ethtool_set_config = pd692x0_ethtool_set_config, +}; + +struct matrix { + u8 hw_port_a; + u8 hw_port_b; +}; + +static int +pd692x0_set_ports_matrix(struct pd692x0_priv *priv, + const struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS]) +{ + struct pd692x0_msg_content buf; + struct pd692x0_msg msg; + int ret, i; + + /* Write temporary Matrix */ + msg = pd692x0_msg_template_list[PD692X0_MSG_SET_TMP_PORT_MATRIX]; + for (i = 0; i < PD692X0_MAX_LOGICAL_PORTS; i++) { + msg.content.sub[2] = i; + msg.content.data[0] = port_matrix[i].hw_port_a; + msg.content.data[1] = port_matrix[i].hw_port_b; + + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); + if (ret < 0) + return ret; + } + + /* Program Matrix */ + msg = pd692x0_msg_template_list[PD692X0_MSG_PRG_PORT_MATRIX]; + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); + if (ret < 0) + return ret; + + return 0; +} + +static int +pd692x0_get_of_matrix(struct device *dev, + struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS]) +{ + int ret, i, ports_matrix_size; + u32 val[PD692X0_MAX_LOGICAL_PORTS * 3]; + + ports_matrix_size = device_property_count_u32(dev, "ports-matrix"); + if (ports_matrix_size <= 0) + return -EINVAL; + if (ports_matrix_size % 3 || + ports_matrix_size > PD692X0_MAX_LOGICAL_PORTS * 3) { + dev_err(dev, "Not valid ports-matrix property size: %d\n", + ports_matrix_size); + return -EINVAL; + } + + ret = device_property_read_u32_array(dev, "ports-matrix", val, + ports_matrix_size); + if (ret < 0) + return ret; + + /* Init Matrix */ + for (i = 0; i < PD692X0_MAX_LOGICAL_PORTS; i++) { + port_matrix[i].hw_port_a = 0xff; + port_matrix[i].hw_port_b = 0xff; + } + + /* Update with values from DT */ + for (i = 0; i < ports_matrix_size; i += 3) { + unsigned int logical_port; + + if (val[i] >= PD692X0_MAX_LOGICAL_PORTS) { + dev_err(dev, "Not valid ports-matrix property\n"); + return -EINVAL; + } + logical_port = val[i]; + + if (val[i + 1] < PD692X0_MAX_HW_PORTS) + port_matrix[logical_port].hw_port_a = val[i + 1]; + + if (val[i + 2] < PD692X0_MAX_HW_PORTS) + port_matrix[logical_port].hw_port_b = val[i + 2]; + } + + return 0; +} + +static int pd692x0_update_matrix(struct pd692x0_priv *priv) +{ + struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS]; + struct device *dev = &priv->client->dev; + int ret; + + ret = pd692x0_get_of_matrix(dev, port_matrix); + if (ret < 0) { + dev_warn(dev, + "Unable to parse port-matrix, saved matrix will be used\n"); + return 0; + } + + ret = pd692x0_set_ports_matrix(priv, port_matrix); + if (ret < 0) + return ret; + + return 0; +} + +#define PD692X0_FW_LINE_MAX_SZ 128 +static int pd692x0_fw_get_next_line(const u8 *data, + char *line, size_t size) +{ + size_t line_size; + int i; + + line_size = min_t(size_t, size, (size_t)PD692X0_FW_LINE_MAX_SZ); + + memset(line, 0, PD692X0_FW_LINE_MAX_SZ); + for (i = 0; i < line_size - 1; i++) { + if (*data == '\r' && *(data + 1) == '\n') { + line[i] = '\r'; + line[i + 1] = '\n'; + return i + 2; + } + line[i] = *data; + data++; + } + + return 0; +} + +static enum fw_upload_err +pd692x0_fw_recv_resp(const struct i2c_client *client, unsigned long ms_timeout, + const char *msg_ok, unsigned int msg_size) +{ + /* Maximum controller response size */ + char fw_msg_buf[5] = {0}; + unsigned long timeout; + int ret; + + if (msg_size > sizeof(fw_msg_buf)) + return FW_UPLOAD_ERR_RW_ERROR; + + /* Read until we get something */ + timeout = msecs_to_jiffies(ms_timeout) + jiffies; + while (true) { + if (time_is_before_jiffies(timeout)) + return FW_UPLOAD_ERR_TIMEOUT; + + ret = i2c_master_recv(client, fw_msg_buf, 1); + if (ret < 0 || *fw_msg_buf == 0) { + usleep_range(1000, 2000); + continue; + } else { + break; + } + } + + /* Read remaining characters */ + ret = i2c_master_recv(client, fw_msg_buf + 1, msg_size - 1); + if (!strncmp(fw_msg_buf, msg_ok, msg_size)) { + return FW_UPLOAD_ERR_NONE; + } else { + dev_err(&client->dev, + "Wrong FW download process answer (%*pE)\n", + msg_size, fw_msg_buf); + return FW_UPLOAD_ERR_HW_ERROR; + } +} + +static int pd692x0_fw_write_line(const struct i2c_client *client, + const char line[PD692X0_FW_LINE_MAX_SZ], + const bool last_line) +{ + int ret; + + while (*line != 0) { + ret = i2c_master_send(client, line, 1); + if (ret < 0) + return FW_UPLOAD_ERR_RW_ERROR; + line++; + } + + if (last_line) { + ret = pd692x0_fw_recv_resp(client, 100, "TP\r\n", + sizeof("TP\r\n") - 1); + if (ret) + return ret; + } else { + ret = pd692x0_fw_recv_resp(client, 100, "T*\r\n", + sizeof("T*\r\n") - 1); + if (ret) + return ret; + } + + return FW_UPLOAD_ERR_NONE; +} + +static enum fw_upload_err pd692x0_fw_reset(const struct i2c_client *client) +{ + const struct pd692x0_msg_content zero = {0}; + struct pd692x0_msg_content buf = {0}; + unsigned long timeout; + char cmd[] = "RST"; + int ret; + + ret = i2c_master_send(client, cmd, strlen(cmd)); + if (ret < 0) { + dev_err(&client->dev, + "Failed to reset the controller (%pe)\n", + ERR_PTR(ret)); + return ret; + } + + timeout = msecs_to_jiffies(10000) + jiffies; + while (true) { + if (time_is_before_jiffies(timeout)) + return FW_UPLOAD_ERR_TIMEOUT; + + ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf)); + if (ret < 0 || + !memcmp(&buf, &zero, sizeof(buf))) + usleep_range(1000, 2000); + else + break; + } + + /* Is the reply a successful report message */ + if (buf.key != PD692X0_KEY_TLM || buf.echo != 0xff || + buf.sub[0] & 0x01) { + dev_err(&client->dev, "PSE controller error\n"); + return FW_UPLOAD_ERR_HW_ERROR; + } + + /* Is the firmware operational */ + if (buf.sub[0] & 0x02) { + dev_err(&client->dev, + "PSE firmware error. Please update it.\n"); + return FW_UPLOAD_ERR_HW_ERROR; + } + + return FW_UPLOAD_ERR_NONE; +} + +static enum fw_upload_err pd692x0_fw_prepare(struct fw_upload *fwl, + const u8 *data, u32 size) +{ + struct pd692x0_priv *priv = fwl->dd_handle; + const struct i2c_client *client = priv->client; + enum pd692x0_fw_state last_fw_state; + int ret; + + priv->cancel_request = false; + last_fw_state = priv->fw_state; + + priv->fw_state = PD692X0_FW_PREPARE; + + /* Enter program mode */ + if (last_fw_state == PD692X0_FW_BROKEN) { + const char *msg = "ENTR"; + const char *c; + + c = msg; + do { + ret = i2c_master_send(client, c, 1); + if (ret < 0) + return FW_UPLOAD_ERR_RW_ERROR; + if (*(c + 1)) + usleep_range(10000, 20000); + } while (*(++c)); + } else { + struct pd692x0_msg_content buf; + struct pd692x0_msg msg; + + msg = pd692x0_msg_template_list[PD692X0_MSG_DOWNLOAD_CMD]; + ret = pd692x0_sendrecv_msg(priv, &msg, &buf); + if (ret < 0) { + dev_err(&client->dev, + "Failed to enter programming mode (%pe)\n", + ERR_PTR(ret)); + return FW_UPLOAD_ERR_RW_ERROR; + } + } + + ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1); + if (ret) + goto err_out; + + if (priv->cancel_request) { + ret = FW_UPLOAD_ERR_CANCELED; + goto err_out; + } + + return FW_UPLOAD_ERR_NONE; + +err_out: + pd692x0_fw_reset(priv->client); + priv->fw_state = last_fw_state; + return ret; +} + +static enum fw_upload_err pd692x0_fw_write(struct fw_upload *fwl, + const u8 *data, u32 offset, + u32 size, u32 *written) +{ + struct pd692x0_priv *priv = fwl->dd_handle; + char line[PD692X0_FW_LINE_MAX_SZ]; + const struct i2c_client *client; + int ret, i; + char cmd; + + client = priv->client; + priv->fw_state = PD692X0_FW_WRITE; + + /* Erase */ + cmd = 'E'; + ret = i2c_master_send(client, &cmd, 1); + if (ret < 0) { + dev_err(&client->dev, + "Failed to boot programming mode (%pe)\n", + ERR_PTR(ret)); + return FW_UPLOAD_ERR_RW_ERROR; + } + + ret = pd692x0_fw_recv_resp(client, 100, "TOE\r\n", sizeof("TOE\r\n") - 1); + if (ret) + return ret; + + ret = pd692x0_fw_recv_resp(client, 5000, "TE\r\n", sizeof("TE\r\n") - 1); + if (ret) + dev_warn(&client->dev, + "Failed to erase internal memory, however still try to write Firmware\n"); + + ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1); + if (ret) + dev_warn(&client->dev, + "Failed to erase internal memory, however still try to write Firmware\n"); + + if (priv->cancel_request) + return FW_UPLOAD_ERR_CANCELED; + + /* Program */ + cmd = 'P'; + ret = i2c_master_send(client, &cmd, sizeof(char)); + if (ret < 0) { + dev_err(&client->dev, + "Failed to boot programming mode (%pe)\n", + ERR_PTR(ret)); + return ret; + } + + ret = pd692x0_fw_recv_resp(client, 100, "TOP\r\n", sizeof("TOP\r\n") - 1); + if (ret) + return ret; + + i = 0; + while (i < size) { + ret = pd692x0_fw_get_next_line(data, line, size - i); + if (!ret) { + ret = FW_UPLOAD_ERR_FW_INVALID; + goto err; + } + + i += ret; + data += ret; + if (line[0] == 'S' && line[1] == '0') { + continue; + } else if (line[0] == 'S' && line[1] == '7') { + ret = pd692x0_fw_write_line(client, line, true); + if (ret) + goto err; + } else { + ret = pd692x0_fw_write_line(client, line, false); + if (ret) + goto err; + } + + if (priv->cancel_request) { + ret = FW_UPLOAD_ERR_CANCELED; + goto err; + } + } + *written = i; + + msleep(400); + + return FW_UPLOAD_ERR_NONE; + +err: + pd692x0_fw_write_line(client, "S7\r\n", true); + return ret; +} + +static enum fw_upload_err pd692x0_fw_poll_complete(struct fw_upload *fwl) +{ + struct pd692x0_priv *priv = fwl->dd_handle; + const struct i2c_client *client = priv->client; + struct pd692x0_msg_ver ver; + int ret; + + priv->fw_state = PD692X0_FW_COMPLETE; + + ret = pd692x0_fw_reset(client); + if (ret) + return ret; + + ver = pd692x0_get_sw_version(priv); + if (ver.maj_sw_ver != PD692X0_FW_MAJ_VER) { + dev_err(&client->dev, + "Too old firmware version. Please update it\n"); + priv->fw_state = PD692X0_FW_NEED_UPDATE; + return FW_UPLOAD_ERR_FW_INVALID; + } + + ret = pd692x0_update_matrix(priv); + if (ret < 0) { + dev_err(&client->dev, "Error configuring ports matrix (%pe)\n", + ERR_PTR(ret)); + priv->fw_state = PD692X0_FW_NEED_UPDATE; + return FW_UPLOAD_ERR_HW_ERROR; + } + + priv->fw_state = PD692X0_FW_OK; + return FW_UPLOAD_ERR_NONE; +} + +static void pd692x0_fw_cancel(struct fw_upload *fwl) +{ + struct pd692x0_priv *priv = fwl->dd_handle; + + priv->cancel_request = true; +} + +static void pd692x0_fw_cleanup(struct fw_upload *fwl) +{ + struct pd692x0_priv *priv = fwl->dd_handle; + + switch (priv->fw_state) { + case PD692X0_FW_WRITE: + pd692x0_fw_reset(priv->client); + fallthrough; + case PD692X0_FW_COMPLETE: + priv->fw_state = PD692X0_FW_BROKEN; + break; + default: + break; + } +} + +static const struct fw_upload_ops pd692x0_fw_ops = { + .prepare = pd692x0_fw_prepare, + .write = pd692x0_fw_write, + .poll_complete = pd692x0_fw_poll_complete, + .cancel = pd692x0_fw_cancel, + .cleanup = pd692x0_fw_cleanup, +}; + +static int pd692x0_i2c_probe(struct i2c_client *client) +{ + struct pd692x0_msg_content buf = {0}; + struct device *dev = &client->dev; + struct pd692x0_msg_ver ver; + struct pd692x0_priv *priv; + struct fw_upload *fwl; + int ret; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { + dev_err(dev, "i2c check functionality failed\n"); + return -ENXIO; + } + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->client = client; + i2c_set_clientdata(client, priv); + + priv->pcdev.owner = THIS_MODULE; + priv->pcdev.ops = &pd692x0_ops; + priv->pcdev.dev = dev; + priv->pcdev.types = PSE_POE; + priv->pcdev.of_pse_n_cells = 1; + priv->pcdev.nr_lines = PD692X0_MAX_LOGICAL_PORTS; + ret = devm_pse_controller_register(dev, &priv->pcdev); + if (ret) { + return dev_err_probe(dev, ret, + "failed to register PSE controller\n"); + } + + fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev), + &pd692x0_fw_ops, priv); + if (IS_ERR(fwl)) { + dev_err(dev, "Failed to register to the Firmware Upload API\n"); + ret = PTR_ERR(fwl); + return ret; + } + priv->fwl = fwl; + + ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf)); + if (ret != sizeof(buf)) { + dev_err(dev, "Failed to get device status\n"); + ret = -EIO; + goto err_fw_unregister; + } + + if (buf.key != 0x03 || buf.echo != 0xff || buf.sub[0] & 0x01) { + dev_err(dev, "PSE controller error\n"); + ret = -EIO; + goto err_fw_unregister; + } + + if (buf.sub[0] & 0x02) { + dev_err(dev, "PSE firmware error. Please update it.\n"); + priv->fw_state = PD692X0_FW_BROKEN; + return 0; + } + + ver = pd692x0_get_sw_version(priv); + dev_info(&client->dev, "Software version %d.%02d.%d.%d\n", ver.prod, + ver.maj_sw_ver, ver.min_sw_ver, ver.pa_sw_ver); + + if (ver.maj_sw_ver != PD692X0_FW_MAJ_VER) { + dev_err(dev, "Too old firmware version. Please update it\n"); + priv->fw_state = PD692X0_FW_NEED_UPDATE; + return 0; + } + + ret = pd692x0_update_matrix(priv); + if (ret < 0) { + dev_err(dev, "Error configuring ports matrix (%pe)\n", + ERR_PTR(ret)); + goto err_fw_unregister; + } + + priv->fw_state = PD692X0_FW_OK; + return 0; + +err_fw_unregister: + firmware_upload_unregister(priv->fwl); + return ret; +} + +static void pd692x0_i2c_remove(struct i2c_client *client) +{ + struct pd692x0_priv *priv = i2c_get_clientdata(client); + + firmware_upload_unregister(priv->fwl); +} + +static const struct i2c_device_id pd692x0_id[] = { + { PD692X0_PSE_NAME, 0 }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, pd692x0_id); + +static const struct of_device_id pd692x0_of_match[] = { + { .compatible = "microchip,pd69200", }, + { .compatible = "microchip,pd69210", }, + { .compatible = "microchip,pd69220", }, + { }, +}; +MODULE_DEVICE_TABLE(of, pd692x0_of_match); + +static struct i2c_driver pd692x0_driver = { + .probe = pd692x0_i2c_probe, + .remove = pd692x0_i2c_remove, + .id_table = pd692x0_id, + .driver = { + .name = PD692X0_PSE_NAME, + .of_match_table = of_match_ptr(pd692x0_of_match), + }, +}; +module_i2c_driver(pd692x0_driver); + +MODULE_AUTHOR("Kory Maincent <kory.maincent@bootlin.com>"); +MODULE_DESCRIPTION("Microchip PD692x0 PoE PSE Controller driver"); +MODULE_LICENSE("GPL");
Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller. This driver only support i2c communication for now. Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> --- MAINTAINERS | 1 + drivers/net/pse-pd/Kconfig | 11 + drivers/net/pse-pd/Makefile | 1 + drivers/net/pse-pd/pd692x0.c | 1049 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1062 insertions(+)