| Message ID | 20210120030502.617185-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,V2] net: dsa: microchip: Adjust reset release timing to match reference reset circuit | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: olteanv@gmail.com vivien.didelot@gmail.com davem@davemloft.net woojung.huh@microchip.com UNGLinuxDriver@microchip.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, 20 Jan 2021 at 03:05, Marek Vasut <marex@denx.de> wrote: > > KSZ8794CNX datasheet section 8.0 RESET CIRCUIT describes recommended > circuit for interfacing with CPU/FPGA reset consisting of 10k pullup > resistor and 10uF capacitor to ground. This circuit takes ~100 ms to > rise enough to release the reset. > > For maximum supply voltage VDDIO=3.3V VIH=2.0V R=10kR C=10uF that is > VDDIO - VIH > t = R * C * -ln( ------------- ) = 10000*0.00001*-(-0.93)=0.093 s > VDDIO > so we need ~95 ms for the reset to really de-assert, and then the > original 100us for the switch itself to come out of reset. Simply > msleep() for 100 ms which fits the constraint with a bit of extra > space. This makes sense if someone is using that device and following the reference circuit exactly. Working with the ksz9477 I can tell you that the reference reset circuit in figure 7.2 of the datasheet doesn't work with a VDDIO of 1.8V. And hardware engineers like to take some liberties anyway... But 100ms is reasonable in general. It will allow for the expected rise time of a wide range of possible reset circuit designs and isn't so long that it will have a major impact on start-up time. So it looks good to me. Reviewed-by: Paul Barker <pbarker@konsulko.com>
On Wed, Jan 20, 2021 at 04:05:02AM +0100, Marek Vasut wrote: > KSZ8794CNX datasheet section 8.0 RESET CIRCUIT describes recommended > circuit for interfacing with CPU/FPGA reset consisting of 10k pullup > resistor and 10uF capacitor to ground. This circuit takes ~100 ms to > rise enough to release the reset. > > For maximum supply voltage VDDIO=3.3V VIH=2.0V R=10kR C=10uF that is > VDDIO - VIH > t = R * C * -ln( ------------- ) = 10000*0.00001*-(-0.93)=0.093 s > VDDIO > so we need ~95 ms for the reset to really de-assert, and then the > original 100us for the switch itself to come out of reset. Simply > msleep() for 100 ms which fits the constraint with a bit of extra > space. > > Fixes: 5b797980908a ("net: dsa: microchip: Implement recommended reset timing") > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Michael Grzeschik <m.grzeschik@pengutronix.de> > Cc: Paul Barker <pbarker@konsulko.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, 20 Jan 2021 04:05:02 +0100 Marek Vasut wrote: > KSZ8794CNX datasheet section 8.0 RESET CIRCUIT describes recommended > circuit for interfacing with CPU/FPGA reset consisting of 10k pullup > resistor and 10uF capacitor to ground. This circuit takes ~100 ms to > rise enough to release the reset. > > For maximum supply voltage VDDIO=3.3V VIH=2.0V R=10kR C=10uF that is > VDDIO - VIH > t = R * C * -ln( ------------- ) = 10000*0.00001*-(-0.93)=0.093 s > VDDIO > so we need ~95 ms for the reset to really de-assert, and then the > original 100us for the switch itself to come out of reset. Simply > msleep() for 100 ms which fits the constraint with a bit of extra > space. > > Fixes: 5b797980908a ("net: dsa: microchip: Implement recommended reset timing") > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Signed-off-by: Marek Vasut <marex@denx.de> I'm slightly confused whether this is just future proofing or you actually have a board where this matters. The tree is tagged as net-next but there is a Fixes tag which normally indicates net+stable. Please advise.
On 1/21/21 2:31 AM, Jakub Kicinski wrote: > On Wed, 20 Jan 2021 04:05:02 +0100 Marek Vasut wrote: >> KSZ8794CNX datasheet section 8.0 RESET CIRCUIT describes recommended >> circuit for interfacing with CPU/FPGA reset consisting of 10k pullup >> resistor and 10uF capacitor to ground. This circuit takes ~100 ms to >> rise enough to release the reset. >> >> For maximum supply voltage VDDIO=3.3V VIH=2.0V R=10kR C=10uF that is >> VDDIO - VIH >> t = R * C * -ln( ------------- ) = 10000*0.00001*-(-0.93)=0.093 s >> VDDIO >> so we need ~95 ms for the reset to really de-assert, and then the >> original 100us for the switch itself to come out of reset. Simply >> msleep() for 100 ms which fits the constraint with a bit of extra >> space. >> >> Fixes: 5b797980908a ("net: dsa: microchip: Implement recommended reset timing") >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> Signed-off-by: Marek Vasut <marex@denx.de> > > I'm slightly confused whether this is just future proofing or you > actually have a board where this matters. The tree is tagged as > net-next but there is a Fixes tag which normally indicates net+stable. I have a board where I trigger this problem, that's how I found it. It should be passed to stable too. So the correct tree / tag is "net" ?
On 1/20/2021 5:51 PM, Marek Vasut wrote: > On 1/21/21 2:31 AM, Jakub Kicinski wrote: >> On Wed, 20 Jan 2021 04:05:02 +0100 Marek Vasut wrote: >>> KSZ8794CNX datasheet section 8.0 RESET CIRCUIT describes recommended >>> circuit for interfacing with CPU/FPGA reset consisting of 10k pullup >>> resistor and 10uF capacitor to ground. This circuit takes ~100 ms to >>> rise enough to release the reset. >>> >>> For maximum supply voltage VDDIO=3.3V VIH=2.0V R=10kR C=10uF that is >>> VDDIO - VIH >>> t = R * C * -ln( ------------- ) = 10000*0.00001*-(-0.93)=0.093 s >>> VDDIO >>> so we need ~95 ms for the reset to really de-assert, and then the >>> original 100us for the switch itself to come out of reset. Simply >>> msleep() for 100 ms which fits the constraint with a bit of extra >>> space. >>> >>> Fixes: 5b797980908a ("net: dsa: microchip: Implement recommended >>> reset timing") >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >>> Signed-off-by: Marek Vasut <marex@denx.de> >> >> I'm slightly confused whether this is just future proofing or you >> actually have a board where this matters. The tree is tagged as >> net-next but there is a Fixes tag which normally indicates net+stable. > > I have a board where I trigger this problem, that's how I found it. It > should be passed to stable too. So the correct tree / tag is "net" ? If this is a bug fix for a commit that is not only in 'net-next', then yes, targeting 'net' is more appropriate: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst#n28
On Wed, 20 Jan 2021 18:10:59 -0800 Florian Fainelli wrote: > On 1/20/2021 5:51 PM, Marek Vasut wrote: > > On 1/21/21 2:31 AM, Jakub Kicinski wrote: > >> On Wed, 20 Jan 2021 04:05:02 +0100 Marek Vasut wrote: > >>> KSZ8794CNX datasheet section 8.0 RESET CIRCUIT describes recommended > >>> circuit for interfacing with CPU/FPGA reset consisting of 10k pullup > >>> resistor and 10uF capacitor to ground. This circuit takes ~100 ms to > >>> rise enough to release the reset. > >>> > >>> For maximum supply voltage VDDIO=3.3V VIH=2.0V R=10kR C=10uF that is > >>> VDDIO - VIH > >>> t = R * C * -ln( ------------- ) = 10000*0.00001*-(-0.93)=0.093 s > >>> VDDIO > >>> so we need ~95 ms for the reset to really de-assert, and then the > >>> original 100us for the switch itself to come out of reset. Simply > >>> msleep() for 100 ms which fits the constraint with a bit of extra > >>> space. > >>> > >>> Fixes: 5b797980908a ("net: dsa: microchip: Implement recommended > >>> reset timing") > >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >> > >> I'm slightly confused whether this is just future proofing or you > >> actually have a board where this matters. The tree is tagged as > >> net-next but there is a Fixes tag which normally indicates net+stable. > > > > I have a board where I trigger this problem, that's how I found it. It > > should be passed to stable too. So the correct tree / tag is "net" ? > > If this is a bug fix for a commit that is not only in 'net-next', then > yes, targeting 'net' is more appropriate: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst#n28 Yup, in that case applied this one and the port map fix to net. Thanks everyone!
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 489963664443..389abfd27770 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -400,7 +400,7 @@ int ksz_switch_register(struct ksz_device *dev, gpiod_set_value_cansleep(dev->reset_gpio, 1); usleep_range(10000, 12000); gpiod_set_value_cansleep(dev->reset_gpio, 0); - usleep_range(100, 1000); + msleep(100); } mutex_init(&dev->dev_mutex);
KSZ8794CNX datasheet section 8.0 RESET CIRCUIT describes recommended circuit for interfacing with CPU/FPGA reset consisting of 10k pullup resistor and 10uF capacitor to ground. This circuit takes ~100 ms to rise enough to release the reset. For maximum supply voltage VDDIO=3.3V VIH=2.0V R=10kR C=10uF that is VDDIO - VIH t = R * C * -ln( ------------- ) = 10000*0.00001*-(-0.93)=0.093 s VDDIO so we need ~95 ms for the reset to really de-assert, and then the original 100us for the switch itself to come out of reset. Simply msleep() for 100 ms which fits the constraint with a bit of extra space. Fixes: 5b797980908a ("net: dsa: microchip: Implement recommended reset timing") Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: Marek Vasut <marex@denx.de> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Michael Grzeschik <m.grzeschik@pengutronix.de> Cc: Paul Barker <pbarker@konsulko.com> --- V2: Use lowercase 's' for second --- drivers/net/dsa/microchip/ksz_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)