w5500: mac: poll VERSIONR to ensure the chip is initialised (IDFGH-10061)

[huming2207]

See discussion: #11331 and #10554 (comment)

This addresses the fix for the version check. Based on our experiments, we have to wait for at least 10ms (one FreeRTOS tick) to let the W5500 fully come up and talks to the host ESP32. We've also implemented a poll to retry 20 times just in case a simple wait doesn't work in some cases.

[CLAassistant]

All committers have signed the CLA.

[suda-morris]

because the w5500_verify_id is called directly after the w5500_reset, maybe what we need is a better exit path for the w5500_reset function? cc @kostaond

[huming2207]

because the w5500_verify_id is called directly after the w5500_reset, maybe what we need is a better exit path for the w5500_reset function? cc @kostaond

Thanks for your reply @suda-morris

I've read the code again and I wonder if we should enforce a delay before the MR mode register read instead?

[huming2207]

@suda-morris @kostaond Done, I've 1. removed the excessive retry count macro and used the reset timeout value instead, and 2. enforce at least a 10ms wait to poll the RST in the mode register just in case that poll read causes any undefined behavior.

[kostaond]

because the w5500_verify_id is called directly after the w5500_reset, maybe what we need is a better exit path for the w5500_reset function? cc @kostaond

Good point, however I am afraid there is no better condition to exit the reset function... Checking the state of autoclear reset bit is kind of standard way. The only thing which crossed my mind was to check the reset bit and the Version ID in the same condition. On the other hand, I don't see much value added in such approach and having it spitted into two calls seems good enough to me.

I've read the code again and I wonder if we should enforce a delay before the MR mode register read instead?

I would keep it as it was since reading immediately seems to work for majority of chips and it ensures the faster startup for this majority. Polling the Version ID will capture the corner cases when the fastest path does not work.

[huming2207]

I would keep it as it was since reading immediately seems to work for majority of chips and it ensures the faster startup for this majority. Polling the Version ID will capture the corner cases when the fastest path does not work.

Sure, I will revert this change ASAP. Is there anything else I can do for this PR?

[huming2207]

@kostaond Done, ready for review. I've also squashed the commits into one to make the commit message cleaner.

[huming2207]

Any updates on this? @kostaond @suda-morris

[huming2207]

Any further updates? @kostaond

[kostaond]

I left two final comments. @suda-morris could you please also have a look?

[kostaond]

I missed this one previously because I though we already discussed it. However, please place the delay at the end of the loop. We should try the fastest way at first.

[kostaond]

Remove or make it debug level.

[huming2207]

@kostaond @suda-morris Done, waiting for review

[KaeLL]

Assuming you're not using a counterfeit chip, you should file an issue on Wiznet forums, since this is something worthy of an update of the datasheet at least.

[huming2207]

Assuming you're not using a counterfeit chip, you should file an issue on Wiznet forums, since this is something worthy of an update of the datasheet at least.

I think this is necessary but a bit off-topic. We wish we have time to confirm with Wiznet but we've already bought a handful of these W5500 modules and chips. We are also experiencing a short deadline, but we also want to help anyone with the same issue as ours, so we push this fix to the mainline ESP-IDF. As long as this fix works for us (and anyone else, hopefully), it's acceptable anyway.

[kostaond]

sha=506b372d28bea8899c7958e5095fc21487ce4aed