by Kory Maincent

[permalink] [raw]

Subject: Re: [PATCH net-next v2 7/8] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller

On Tue, 5 Dec 2023 07:36:06 +0100
Oleksij Rempel <[email protected]> wrote:

> > > +examples:
> > > + - |
> > > + i2c {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + ethernet-pse@3c {
> > > + compatible = "microchip,pd69200";
> > > + reg = <0x3c>;
> > > + #pse-cells = <1>;
> > > + ports-matrix = <0 2 5
> > > + 1 3 6
> > > + 2 0 0xff
> > > + 3 1 0xff>;
> >
> > Hm... this will probably not scale. PSE is kind of PMIC for ethernet. I
> > has bunch of regulators which can be grouped to one more powerful
> > regulator. Since it is regulators, we will wont to represent them in a
> > system as regulators too. We will probably have physical, board
> > specific limitation, so we will need to describe regulator limits for
> > each separate channel.
>
> After diving a bit deeper to the chip manual and communication protocol
> manual I would recommend to recreate system topology as good as possible
> in the devicetree. The reason is that we actually able to communicate
> with with "manager" behind the "controller" and the "port-matrix" is all
> about the "managers" and physical ports layout.

Ok, but the "managers communication" implementation will be added later as
for now only the basics of the the PSE controller is implemented.

> Typical system architecture looks like this:
>
> SoC --- i2c/uart --> controller -- spi --> manager0 -- phys_port0 -->
> log_port0 (PoE4) | \- phys_port1 -/
> | \- phys_port2 -->
> log_port1 (PoE2) | \- phys_port3 --> log_port2 (PoE2)
> \- manager1 -- phys_port0 ..
> ....
>
> Please include some ASCII topology to the documentation :)

Ok.

> I would expect a devicetree like this:
>
> ethernet-pse@3c {
> // controller compatible should be precise
> compatible = "microchip,pd69210";
> reg = <0x3c>;
> #pse-cells = <1>;
>
> managers {
> manager@0 {
> // manager compatible should be included, since we are
> // able to campare it with communication results
> compatible = "microchip,pd69208t4"
> // addressing corresponding to the chip select addressing
> reg = <0>;
>
> physical-ports {
> phys0: port@0 {
> // each of physical ports is actually a regulator
> reg = <0>;
> };
> phys1: port@1 {
> reg = <1>;
> };
> phys2: port@2 {
> reg = <2>;
> };
>
> ...
> }
>
> // port matrix can be calculated by using this information
> logical-ports {
> log_port0: port@0 {
> // PoE4 port
> physical-ports = <&phys0, &phys1>;
> };
> log_port1: port@1 {
> // PoE2 port
> physical-ports = <&phys2>;
> };
> };
>
> ....
> ethernet-phy@1 {
> reg = <1>;
> pses = <&log_port0>;
> }
> ethernet-phy@2 {
> reg = <2>;
> pses = <&log_port1>;
> }

The pse node will become massive (more than 140 subnodes) but indeed this will
fit the real topology architecture.

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

2023-12-05 12:54:52

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH net-next v2 7/8] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller

On Tue, Dec 05, 2023 at 07:36:06AM +0100, Oleksij Rempel wrote:

> CC regulator devs here. PSE is a regulator for network devices :)

Is there some question related to regulators here? I couldn't spot one.

Attachments:

(No filename) (213.00 B)
signature.asc (499.00 B)
Download all attachments

2023-12-05 12:55:39

On Tue, Dec 05, 2023 at 03:02:03PM +0100, Oleksij Rempel wrote:
> On Tue, Dec 05, 2023 at 12:55:18PM +0000, Mark Brown wrote:
> > On Tue, Dec 05, 2023 at 07:45:27AM +0100, Oleksij Rempel wrote:

> > > CC regulator devs here too.

> > Again, I'm not sure what if any question there is?

> PSE is kind of PMIC for Ethernet ports. I assume, it is good to let you
> know at least about existence drivers.

OK... I mean, if they're not using the regulator framework I'm not sure
it has much impact - there are plenty of internal regulators in devices
already so it wouldn't be *too* unusual other than the fact that AFAICT
this is somewhat split between devices within the subsystem? Neither of
the messages was super clear.

Attachments:

(No filename) (738.00 B)
signature.asc (499.00 B)
Download all attachments

2023-12-05 18:02:12

by Christophe JAILLET

[permalink] [raw]

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

Le 04/12/2023 à 23:16, Köry Maincent a écrit :
> Thanks for your review!
>
> On Sun, 3 Dec 2023 22:11:46 +0100
> Christophe JAILLET <[email protected]> wrote:
>
>>> +
>>> + 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;
>>
>> Nit: return dev_err_probe()?
>
> No EPROBE_DEFER error can be catch from firmware_upload_register() function, so
> it's not needed.

Hi,

up to you to take it or not, but dev_err_probe() also logs the error
code in a human readable way and it saves a few lines of code.

If I remember correctly, it also saves some bytes in the .o file.

Other than that, it is a matter of style.

CJ

>
> Regards,

2023-12-05 21:36:40

by kernel test robot

[permalink] [raw]

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

Hi Kory,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url: https://github.com/intel-lab-lkp/linux/commits/Kory-Maincent/ethtool-Expand-Ethernet-Power-Equipment-with-c33-PoE-alongside-PoDL/20231202-021033
base: net-next/main
patch link: https://lore.kernel.org/r/20231201-feature_poe-v2-8-56d8cac607fa%40bootlin.com
patch subject: [PATCH net-next v2 8/8] net: pse-pd: Add PD692x0 PSE controller driver
config: alpha-kismet-CONFIG_FW_UPLOAD-CONFIG_PSE_PD692X0-0-0 (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
reproduce: (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for FW_UPLOAD when selected by PSE_PD692X0

WARNING: unmet direct dependencies detected for FW_UPLOAD
Depends on [n]: FW_LOADER [=n]
Selected by [y]:
- PSE_PD692X0 [=y] && NETDEVICES [=y] && PSE_CONTROLLER [=y] && I2C [=y]

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-12-06 08:41:58

by Kory Maincent

[permalink] [raw]

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

by Kory Maincent

[permalink] [raw]

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

Hell Andrew,

Thanks for your reviews and sorry for replying so late, I was working on the
core to fit the new bindings and requirements lifted by Oleksij.

On Sun, 3 Dec 2023 20:34:54 +0100
Andrew Lunn <[email protected]> wrote:

> > +static int pd692x0_try_recv_msg(const struct i2c_client *client,
> > + struct pd692x0_msg *msg,
> > + struct pd692x0_msg *buf)
> > +{
> > + msleep(30);
> > +
> > + memset(buf, 0, sizeof(*buf));
> > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > + if (buf->key)
> > + return 1;
> > +
> > + msleep(100);
> > +
> > + memset(buf, 0, sizeof(*buf));
> > + i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > + if (buf->key)
> > + return 1;
> > +
> > + return 0;
>
> Maybe make this function return a bool? Or 0 on success, -EIO on
> error?

Indeed, I will move on to bool.

> > +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 0xff
>
> That probably works. Most linkers producing SREC output do limit
> themselves to lines of 80 charactors max. But the SREC format actually
> allows longer lines.

I set it to SREC limit but indeed the firmware lines does not exceed 80
characters except the comments. 0xff line size limit won't break anything
though.

> > +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;
> > + }
>
> Does the Vendor Documentation indicate Windoze line endings will
> always be used? Motorola SREC allow both Windows or rest of the world
> line endings to be used.

All the firmware lines end with "\r\n" but indeed it is not specifically
written that the firmware content would follow this. IMHO it is implicit that
it would be the case as all i2c messages use this line termination.
Do you prefer that I add support to the world line endings possibility?

> > +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) {
>
> That is probably too strong a condition. You need to allow firmware
> upgrades, etc. Does it need to be an exact match, or would < be
> enough?

The major version is not compatible with the last one, the i2c messages
content changed. I supposed a change in major version would imply a change in
the i2c messages content and would need a driver update that's why I used this
strong condition.

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

2024-01-16 13:18:47

by Andrew Lunn

[permalink] [raw]

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

>
> > > +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;
> > > + }
> >
> > Does the Vendor Documentation indicate Windoze line endings will
> > always be used? Motorola SREC allow both Windows or rest of the world
> > line endings to be used.
>
> All the firmware lines end with "\r\n" but indeed it is not specifically
> written that the firmware content would follow this. IMHO it is implicit that
> it would be the case as all i2c messages use this line termination.
> Do you prefer that I add support to the world line endings possibility?

No need, just hack an SREC file, and test the parser does not explode
with an opps, and you get an sensible error message about the firmware
being corrupt. I would not be too surprised if there are some mail
systems still out there which might convert the line ending.

> > > +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) {
> >
> > That is probably too strong a condition. You need to allow firmware
> > upgrades, etc. Does it need to be an exact match, or would < be
> > enough?
>
> The major version is not compatible with the last one, the i2c messages
> content changed. I supposed a change in major version would imply a change in
> the i2c messages content and would need a driver update that's why I used this
> strong condition.

Do you know the next major version will change the message contents?
Is this documented somewhere? If so add a comment. Otherwise, i would
allow higher major versions. When the vendor breaks backwards
compatibility, its going to need code changes anyway, and at that
point the test can be made more strict.

We try to make vendors not make firmware ABI breaking changes, and we
have pushed back against a number of vendors who do. So i think its
best we assume they won't break the ABI.

Andrew

2024-01-16 14:12:30

by Kory Maincent

[permalink] [raw]

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

On Tue, 16 Jan 2024 14:18:04 +0100
Andrew Lunn <[email protected]> wrote:

> >
> > > > +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;
> > > > + }
> > >
> > > Does the Vendor Documentation indicate Windoze line endings will
> > > always be used? Motorola SREC allow both Windows or rest of the world
> > > line endings to be used.
> >
> > All the firmware lines end with "\r\n" but indeed it is not specifically
> > written that the firmware content would follow this. IMHO it is implicit
> > that it would be the case as all i2c messages use this line termination.
> > Do you prefer that I add support to the world line endings possibility?
>
> No need, just hack an SREC file, and test the parser does not explode
> with an opps, and you get an sensible error message about the firmware
> being corrupt. I would not be too surprised if there are some mail
> systems still out there which might convert the line ending.

Ok I will do so.

>
> > > > +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) {
> > >
> > > That is probably too strong a condition. You need to allow firmware
> > > upgrades, etc. Does it need to be an exact match, or would < be
> > > enough?
> >
> > The major version is not compatible with the last one, the i2c messages
> > content changed. I supposed a change in major version would imply a change
> > in the i2c messages content and would need a driver update that's why I
> > used this strong condition.
>
> Do you know the next major version will change the message contents?

No.

> Is this documented somewhere? If so add a comment. Otherwise, i would
> allow higher major versions. When the vendor breaks backwards
> compatibility, its going to need code changes anyway, and at that
> point the test can be made more strict.
>
> We try to make vendors not make firmware ABI breaking changes, and we
> have pushed back against a number of vendors who do. So i think its
> best we assume they won't break the ABI.

Alright, thanks!

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