| Message ID | 20240312192117.7789-1-justin.swartz@risingedge.co.za (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dsa: mt7530: increase reset hold time | expand |
On 2024-03-13 10:59, Arınç ÜNAL wrote: > On 12.03.2024 22:21, Justin Swartz wrote: >> Increase the MT7530 reset hold period from 1000-1100 usec >> to 5000-5100 usec. >> >> This should reduce the likelihood of an incorrect external >> crystal frequency selection which may occur when reset is >> deasserted too early under certain link conditions. >> >> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> >> --- >> drivers/net/dsa/mt7530.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 3c1f65759..5e9e1381a 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds) >> */ >> if (priv->mcm) { >> reset_control_assert(priv->rstc); >> - usleep_range(1000, 1100); >> + usleep_range(5000, 5100); >> reset_control_deassert(priv->rstc); >> } else { >> gpiod_set_value_cansleep(priv->reset, 0); >> - usleep_range(1000, 1100); >> + usleep_range(5000, 5100); >> gpiod_set_value_cansleep(priv->reset, 1); >> } >> > > Again, the patch subject is missing the indication of a tree. Read the > networking subsystem (netdev) development page [1]. Thanks for that pointer. > This ship has sailed anyway. Now the changes the first patch did must > be > reverted too. I will deal with this from now on, you can stop sending > patches regarding this. At least if the first patch isn't reverted, the approach used is less likely to result in the problem occuring, IMHO. The delay between deliberately switching the LEDs off, instead of only waiting on chip reset logic to handle that, and the reset assertion could be considered a "reset setup" period to complement the original reset hold period. Increasing the hold period to what should be 5000 - 5100 usec, definitely made the problem go away my testing, but the previous approach is (if nothing else) more explicit in its intent. > [1] > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html > > Arınç
On 2024-03-13 14:06, Arınç ÜNAL wrote: > On 13.03.2024 14:52, Justin Swartz wrote: >> >> On 2024-03-13 10:59, Arınç ÜNAL wrote: >>> This ship has sailed anyway. Now the changes the first patch did must >>> be >>> reverted too. I will deal with this from now on, you can stop sending >>> patches regarding this. >> >> At least if the first patch isn't reverted, the approach used is >> less likely to result in the problem occuring, IMHO. > > I disagree that the previous approach is less likely to result in the > problem occurring. If you don't like the delay amount we agreed on, > feel > free to express a higher amount. I created and tested a patch to entertain your input about what you thought could be a suitable hold period to address the problem, and it appeared to work. The criteria being that the crystal frequency selection was correct over 20 tests. So if only the reset hold period is going to change, I'm good with what you had suggested: 5000 - 5100 usec. Ultimately the selection of this period should be guided by the timing information provided in a datasheet or design guide from the manufacturer. If you, or anyone else, has such a document that provides this information and is able to confirm or deny speculation about any/all timing periods related to reset, please do so. > I also disagree on introducing a solution that is in addition to > another > solution, both of which fix the same problem. I'm not sure I understand why you say that. These patches were intended to be applied exclusively, or in other words: in isolation - not together. Although if they were applied together, it wouldn't really matter. For the record, I've only continued to keep this thread alive in the hope that some solution to this problem will make it into mainline eventually. I don't care if it was my original patch, the subsequent patch, or a later patch provided by you or someone else. :) >> >> The delay between deliberately switching the LEDs off, instead of >> only waiting on chip reset logic to handle that, and the reset >> assertion could be considered a "reset setup" period to complement >> the original reset hold period. >> >> Increasing the hold period to what should be 5000 - 5100 usec, >> definitely made the problem go away my testing, but the previous >> approach is (if nothing else) more explicit in its intent. > > I don't want any unnecessary complications on the code I'm maintaining. > I > already gave a clear intent on the patch log that introduces a simpler > and > more efficient approach, it doesn't need to be on the code. > > Arınç Kind Regards Justin
On 2024-03-13 17:04, Arınç ÜNAL wrote: > On 13.03.2024 16:13, Justin Swartz wrote: >> On 2024-03-13 14:06, Arınç ÜNAL wrote: >>> On 13.03.2024 14:52, Justin Swartz wrote: >>>> >>>> On 2024-03-13 10:59, Arınç ÜNAL wrote: >>>>> This ship has sailed anyway. Now the changes the first patch did >>>>> must be >>>>> reverted too. I will deal with this from now on, you can stop >>>>> sending >>>>> patches regarding this. >>>> >>>> At least if the first patch isn't reverted, the approach used is >>>> less likely to result in the problem occuring, IMHO. >>> >>> I disagree that the previous approach is less likely to result in the >>> problem occurring. If you don't like the delay amount we agreed on, >>> feel >>> free to express a higher amount. >> >> I created and tested a patch to entertain your input about what you >> thought could be a suitable hold period to address the problem, and it >> appeared to work. The criteria being that the crystal frequency >> selection >> was correct over 20 tests. >> >> So if only the reset hold period is going to change, I'm good with >> what >> you had suggested: 5000 - 5100 usec. >> >> Ultimately the selection of this period should be guided by the timing >> information provided in a datasheet or design guide from the >> manufacturer. > > That's a good point, I agree. > >> >> If you, or anyone else, has such a document that provides this >> information >> and is able to confirm or deny speculation about any/all timing >> periods >> related to reset, please do so. > > These are the documents I use to program this switch family. I did not > stumble upon a page going over this. > > MT7621 Giga Switch Programming Guide v0.3: > > https://github.com/vschagen/documents/blob/main/MT7621_ProgrammingGuide_GSW_v0_3.pdf > > MT7531 switch chip datasheet: > > https://wiki.banana-pi.org/Banana_Pi_BPI-R64#Documents Thanks for the links. Have you encountered this one? MT7530 Giga Switch Programming Guide v0.1: https://github.com/libc0607/arduino_mt7530/blob/master/MT7530_programming_guide_V00.pdf It has some timing diagrams, but nothing I found for reset. The HWTRAP description has some bits swapped, so I guess those were typos, in the case of XTAL_SELECT. >>> I also disagree on introducing a solution that is in addition to >>> another >>> solution, both of which fix the same problem. >> >> I'm not sure I understand why you say that. These patches were >> intended >> to be applied exclusively, or in other words: in isolation - not >> together. >> >> Although if they were applied together, it wouldn't really matter. >> >> For the record, I've only continued to keep this thread alive in the >> hope that some solution to this problem will make it into mainline >> eventually. >> >> I don't care if it was my original patch, the subsequent patch, or a >> later patch provided by you or someone else. :) > > I think you've missed that your patch is already applied. And it won't > be > reverted for reasons explained by Paolo in this mail thread. > > https://git.kernel.org/netdev/net-next/c/2920dd92b980 > > So if your patch here were to be applied too, the final mt7530.c would > have > the LEDs disabled AND before reset deassertion delay increased. Yes, I seem to have missed that. I thought your request for the patch to be reverted definitely would have been performed, or at least queued, seeing as you're the maintainer.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 3c1f65759..5e9e1381a 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds) */ if (priv->mcm) { reset_control_assert(priv->rstc); - usleep_range(1000, 1100); + usleep_range(5000, 5100); reset_control_deassert(priv->rstc); } else { gpiod_set_value_cansleep(priv->reset, 0); - usleep_range(1000, 1100); + usleep_range(5000, 5100); gpiod_set_value_cansleep(priv->reset, 1); }
Increase the MT7530 reset hold period from 1000-1100 usec to 5000-5100 usec. This should reduce the likelihood of an incorrect external crystal frequency selection which may occur when reset is deasserted too early under certain link conditions. Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> --- drivers/net/dsa/mt7530.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)