drivers: misc: Add support for WIZ550io module

[gramsay0]

WIZ550io is a W5500 breakout board with an extra PIC12F519 MCU.
The PIC12F519 MCU initialises the W5500 with a unique MAC address after a GPIO reset.
The PIC12F519 requires a 150ms delay after GPIO reset to configure the W5500.
There is an optional "ready" GPIO signal that can be used to reduce that delay.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[decsny]

@parthitce @gramsay0 how are we resolving this PR?

[gramsay0]

how are we resolving this PR?

@decsny @parthitce I have pushed a commit to remove the device tree specialization

[parthitce]

Thanks for the changes.

[raveious]

I'm wondering if this should be in a new device tree binding that inherits from this one. You're sorta imposing this requirement onto everyone even if they aren't using this module which is what I believe to be the more common case.

[gramsay0]

@raveious an earlier revision of this PR had that, the maintainer did not agree, so it was removed

[decsny]

Ready gpio should not be on the w5500 binding because it is not on the w5500 chip itself.

[parthitce]

I understand the differences between this module and underlying W5500 IC itself. Meaning, there is no ready-gpio from W5500. So to address that we can potentially introduce binding consuming the w5500 binding in include.

[raveious]

Closing my review as my issues were addressed previously.

[parthitce]

LGTM, please rebase. To add, does it makes sense to add the overlay based on WIZ550IO?

[gramsay0]

LGTM, please rebase. To add, does it makes sense to add the overlay based on WIZ550IO?

I have added a WIZ550io shield, similar to the arceli_eth_w5500 shield

[decsny]

need to determine a different solution for the w5500 io board that fits into the zephyr architecture better

[decsny]

Ready gpio should not be on the w5500 binding because it is not on the w5500 chip itself.

[decsny]

BTW, I changed the assignee to myself since I am on the list for both DT binding and ethernet, and galak is inactive

[decsny]

@gramsay0 I'm a bit confused because I looked for this W5500io module on the internet and found these docs: https://docs.wiznet.io/Product/ioModule/W5500-io#pin-description but they don't say anything about a ready pin as far as I can tell, can you clarify the discrepancy?

[decsny]

nevermind, I just realized there is a w5500io and a w550io.... that's a bit confusing itself

[decsny]

@gramsay0

I noticed the RST pin on the w550io board is actually not the same signal as the RST going to the w5500 controller itself. It actually is a signal that goes to the PIC12F519, and a different pin from the PIC12F519 is connected to the w5500. And this signal is even documented differently, having different timing requirements.

I think this breakout board should have a different compatible, adding a ready signal to the w5500 compatible is incorrect. The DT could look something like this

&arduino_spi {
 	status = "okay";

	eth_w5500: eth_w5500@0 {
		compatible = "wiznet,w5500";
                ....
               /* do not put the reset gpio property here */
               no-reset;
	};
}

w550io {
		compatible = "wiznet,w550io";
		reset-gpios = <&arduino_header 14 GPIO_ACTIVE_LOW>;  /* D8 */
		ready-gpios = <&arduino_header 13 GPIO_ACTIVE_HIGH>; /* D7 */
                wiznet,w5500 = <&eth_w5500>
}

The issue of having board specific code in the file is actually not a huge problem, but I don't like how it's done right now which is just putting the code within some #if and checking if that property is present, which it shouldn't be since it shouldn't be part of that binding. Also, in my opinion, since these reset signals are not the same as each other, and the whole reset sequence really is different, code should reflect that, right now it's a weird hack looking code that can be misleading to future readers and potentially annoying to maintain.

To handle the different reset sequence, I would put a boolean DT property in the w5500 binding called "no-reset". I have done some research to confirm that properties like this are common in linux DT bindings to clarify that a device may be reset by another mechanism which is the case here, as the mechanism doing the reset will be the PIC12F519 rather than the host zephyr mcu. If this property is set, don't do the reset sequence in the w5500 init. Instead, you can have a SYS_INIT function in the same file that runs for every w550 node with higher priority than the w5500 init (ie runs before) that will do the appropriate reset sequence for this w550 module involving the longer wait after RST asserted and checking the RDY pin etc. You can also put in this function the spi read to read the mac address and I think you should have no problem setting the data in the w5500_runtime struct from here for the corresponding w5500 device in the phandle.

This way the code for the w550io is separate from the code for the w5500 and the DT bindings are accurate.

[gramsay0]

@decsny I agree and really like the approach you have proposed.
I'll have a look into implementing these changes, thanks

[decsny]

@decsny I agree and really like the approach you have proposed. I'll have a look into implementing these changes, thanks

I didn't look into this part too much but it looked like there is maybe some switch on the board for the spi signals to switch between the PIC MCU and the w5500, I'm not sure what's the purpose but if that's the case then maybe consider also putting the w550io node on the spi bus, not sure

[gramsay0]

@decsny I have made the modifications you suggested.

Although I'm not certain the WIZ550io code is in the correct location, as it's not really an Ethernet controller. This could be moved elsewhere (drivers/misc?) if you prefer.

Rather than adding a "no-reset" property to W5500 I put a static assert in the WIZ550io driver to ensure the "reset-gpios" property is not set in the W5500 device

[decsny]

@decsny I have made the modifications you suggested.

Although I'm not certain the WIZ550io code is in the correct location, as it's not really an Ethernet controller. This could be moved elsewhere (drivers/misc?) if you prefer.

Rather than adding a "no-reset" property to W5500 I put a static assert in the WIZ550io driver to ensure the "reset-gpios" property is not set in the W5500 device

I was going to suggest you put it in drivers/misc originally but I thought it might add some challenge, so if you want to do it then go ahead

And the idea for the assert is fine by me

[decsny]

won't the init sequence in the eth_w5500 have some problems now?

[gramsay0]

won't the init sequence in the eth_w5500 have some problems now?

@decsny can you be more specific, I'm not sure what you are referring to?

[decsny]

approving because I'm assuming it works but what I meant is, doesn't the w5500 driver init still happen and will it conflict with what the w550io init is doing

[gramsay0]

approving because I'm assuming it works but what I meant is, doesn't the w5500 driver init still happen and will it conflict with what the w550io init is doing

Both the wiz550io and w5500 init occur. wiz550io has two asserts to prevent conflicts:

• wiz550io init occurs first
• w5500 device does not have GPIO reset enabled (SW reset is ok)

If neither "zephyr,random-mac-address" or "local-mac-address" are configured, w5500 leaves the mac address untouched, which in this case will use the wiz550io embedded MAC address

[parthitce]

IMO the core view of the PR is fully changed. This is how I understand it as a whole,

• WIZ550io uses W5500 + X IC to configure the MAC to be unique
• This is notified to the consumer over a GPIO line to inform that the W5500 chip ready with unique mac address

So the only difference between W5500 and WIZ550io is to have unique MAC address. Regarding the bindings part, I ACK'd the ready-gpio part, which is arguably in WIZ550io binding (which uses w5500 binding). But technically this is MAC address handling for W5500, which shall reside in w5500 driver itself.

But I don't really understand the reason behind moving ethernet code to drivers/misc? Also additional Kconfig? Really IMO this is off track and unmanageable AFAIK. NACK!