| Message ID | Zi68sDje4wfgftyZ@builder (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand |
On Sun, Apr 28, 2024 at 11:16:32PM +0200, Ramón Nordin Rodriguez wrote: > >From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?= > <ramon.nordin.rodriguez@ferroamp.se> > Date: Sun, 28 Apr 2024 22:25:12 +0200 > Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit You sent this patch in an odd way. We don't normally see headers like this. I've been using b4 recently for patch management: https://b4.docs.kernel.org/en/latest/contributor/prep.html Using `b4 send` is a good idea. Otherwise git format-patch; git send-email > index 9abefa8b9d9f..bed9033574b2 100644 > --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c > +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c > @@ -9,6 +9,7 @@ > #include <linux/kernel.h> > #include <linux/phy.h> > #include <linux/oa_tc6.h> > +#include <linux/gpio/driver.h> This is not a gpio driver, it is a gpio consumer. So you should be using linux/gpio/consumer.h. Also, i _think_ the includes are sorted, so it probably should go earlier. > > #define DRV_NAME "lan865x" > > @@ -33,6 +34,7 @@ > > struct lan865x_priv { > struct work_struct multicast_work; > + struct gpio_desc *reset_gpio; > struct net_device *netdev; > struct spi_device *spi; > struct oa_tc6 *tc6; > @@ -283,6 +285,24 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv) > return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval); > } > > +static int lan865x_probe_reset_gpio(struct lan865x_priv *priv) > +{ > + priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(priv->reset_gpio)) > + return PTR_ERR(priv->reset_gpio); > + > + return 0; > +} > + > +static void lan865x_hw_reset(struct lan865x_priv *priv) > +{ > + gpiod_set_value_cansleep(priv->reset_gpio, 1); > + // section 9.6.3 RESET_N Timing specifies a minimum hold of 5us > + usleep_range(5, 10); > + gpiod_set_value_cansleep(priv->reset_gpio, 0); > +} Do you see a need to do a reset at any time other than probe? If not, i would probably combine these two functions into one. Also, since you pass GPIOD_OUT_HIGH, you have already put it into reset. So setting the gpio to 1 is pointless. Does the datasheet say anything about how long you should wait after releasing the reset? > + > static int lan865x_probe(struct spi_device *spi) > { > struct net_device *netdev; > @@ -297,6 +317,14 @@ static int lan865x_probe(struct spi_device *spi) > priv->netdev = netdev; > priv->spi = spi; > spi_set_drvdata(spi, priv); > + if (lan865x_probe_reset_gpio(priv)) { > + dev_err(&spi->dev, "failed to probe reset pin"); > + ret = -ENODEV; It is normal that a function like lan865x_probe_reset_gpio() would return an error code. You should then return that error code, rather than replace it with ENODEV. Andrew
Hi Ramon, On 29/04/24 2:46 am, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?= > <ramon.nordin.rodriguez@ferroamp.se> > Date: Sun, 28 Apr 2024 22:25:12 +0200 > Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > This commit optionally enables a hardware reset of the lan8650/1 > mac-phy. These chips have a software reset that is discourage from use > in the manual since it only resets the internal phy. The software reset done by the current driver is not only resetting the internal PHY, it resets the entire MAC-PHY including the integrated PHY. The reset bit of the Clause 22 basic control register only will reset the internal PHY alone. But oa_tc6_sw_reset_macphy() function is writing software reset bit in the Reset Control and Status register which resets the entire MAC-PHY including the internal PHY. OPEN Alliance spec says the following which is done in the oa_tc6_sw_reset_macphy(). 9.2.4 Reset Control and Status Register (0x0003) 9.2.4.1 RSVD Reserved for future use. Writing to these bits shall have no effect on the MAC-PHY. 9.2.4.2 SWRESET MAC-PHY Software Reset. The action of writing a ‘1’ to this bit shall fully reset the MAC-PHY, including the integrated PHY, to an initial state including but not limited to resetting all state machines and registers to their default value. When this bit is set, the reset shall not occur until CSn is deasserted to allow for the control command write to complete. This bit is self-clearing. LAN8650 spec says the following, 4.1.1.3 Software Reset A software reset of the LAN8650/1 is available via the Soft Reset (SW_RESET) bit in the standard OA_CONFIG0 register. Note: The SW_RESET bit of the Clause 22 Basic Control register will reset only the internal PHY, not the entire device. This PHY only reset is not recommended for use. If such a reset is detected, by reading the RESETC bit of the STS2 register, reset the entire device. The above note is given in the lan8650 datasheet to let the user to know that clause 22 software reset will reset only internal PHY but I don't think they mean it for the MAC-PHY software reset done from Reset Control and Status register. So in my opinion, I don't see the need of external pin reset as the existing oa_tc6_sw_reset_macphy() function does the software reset of the entire MAC-PHY. Still if you see a need to have this external pin reset as an optional function then it may be needed for all the vendor specific MAC drivers. In that case, reset-gpios parameter value alone can be taken from the chip specific device tree and the remaining code for operating the reset gpio can be moved to oa_tc6.c and the function name can be oa_tc6_hw_reset_macphy(). Best regards, Parthiban V > > Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se> > --- > .../bindings/net/microchip,lan865x.yaml | 4 +++ > .../net/ethernet/microchip/lan865x/lan865x.c | 28 +++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml > index 4fdec0ba3532..0f11f431df06 100644 > --- a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml > +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml > @@ -44,6 +44,9 @@ properties: > minimum: 15000000 > maximum: 25000000 > > + reset-gpios: > + maxItems: 1 > + > "#address-cells": > const: 1 > > @@ -76,5 +79,6 @@ examples: > interrupts = <6 IRQ_TYPE_EDGE_FALLING>; > local-mac-address = [04 05 06 01 02 03]; > spi-max-frequency = <15000000>; > + reset-gpios = <&gpio2 8 GPIO_ACTIVE_HIGH>; > }; > }; > diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c > index 9abefa8b9d9f..bed9033574b2 100644 > --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c > +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c > @@ -9,6 +9,7 @@ > #include <linux/kernel.h> > #include <linux/phy.h> > #include <linux/oa_tc6.h> > +#include <linux/gpio/driver.h> > > #define DRV_NAME "lan865x" > > @@ -33,6 +34,7 @@ > > struct lan865x_priv { > struct work_struct multicast_work; > + struct gpio_desc *reset_gpio; > struct net_device *netdev; > struct spi_device *spi; > struct oa_tc6 *tc6; > @@ -283,6 +285,24 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv) > return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval); > } > > +static int lan865x_probe_reset_gpio(struct lan865x_priv *priv) > +{ > + priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(priv->reset_gpio)) > + return PTR_ERR(priv->reset_gpio); > + > + return 0; > +} > + > +static void lan865x_hw_reset(struct lan865x_priv *priv) > +{ > + gpiod_set_value_cansleep(priv->reset_gpio, 1); > + // section 9.6.3 RESET_N Timing specifies a minimum hold of 5us > + usleep_range(5, 10); > + gpiod_set_value_cansleep(priv->reset_gpio, 0); > +} > + > static int lan865x_probe(struct spi_device *spi) > { > struct net_device *netdev; > @@ -297,6 +317,14 @@ static int lan865x_probe(struct spi_device *spi) > priv->netdev = netdev; > priv->spi = spi; > spi_set_drvdata(spi, priv); > + if (lan865x_probe_reset_gpio(priv)) { > + dev_err(&spi->dev, "failed to probe reset pin"); > + ret = -ENODEV; > + goto free_netdev; > + } > + > + if (priv->reset_gpio) > + lan865x_hw_reset(priv); > INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler); > > priv->tc6 = oa_tc6_init(spi, netdev); > -- > 2.43.0 > >
On 28/04/2024 23:16, Ramón Nordin Rodriguez wrote: > From c65e42982684d5fd8b2294eb6acf755aa0fcab83 Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Ram=C3=B3n=20Nordin=20Rodriguez?= > <ramon.nordin.rodriguez@ferroamp.se> > Date: Sun, 28 Apr 2024 22:25:12 +0200 > Subject: [PATCH net-next v4 13/12] net: lan865x: optional hardware reset > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > This commit optionally enables a hardware reset of the lan8650/1 > mac-phy. These chips have a software reset that is discourage from use > in the manual since it only resets the internal phy. > > Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se> > --- > .../bindings/net/microchip,lan865x.yaml | 4 +++ > .../net/ethernet/microchip/lan865x/lan865x.c | 28 +++++++++++++++++++ Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Best regards, Krzysztof
> > This commit optionally enables a hardware reset of the lan8650/1 > > mac-phy. These chips have a software reset that is discourage from use > > in the manual since it only resets the internal phy. > The software reset done by the current driver is not only resetting the > internal PHY, it resets the entire MAC-PHY including the integrated PHY. > The reset bit of the Clause 22 basic control register only will reset > the internal PHY alone. But oa_tc6_sw_reset_macphy() function is writing > software reset bit in the Reset Control and Status register which resets > the entire MAC-PHY including the internal PHY. All right, I did not dig deep enough obviously. > The above note is given in the lan8650 datasheet to let the user to know > that clause 22 software reset will reset only internal PHY but I don't > think they mean it for the MAC-PHY software reset done from Reset > Control and Status register. Could still be relevant to implement the .soft_reset with -EOPNOTSUPP as Andrew has suggested in the phy driver. > > So in my opinion, I don't see the need of external pin reset as the > existing oa_tc6_sw_reset_macphy() function does the software reset of > the entire MAC-PHY. I agree with your assesment that this invalidates the problem I was aiming at solving. Additionally I figured out why my setup did not work without the HW reset, I had missed a pull resistor in the schematic that held the IC in reset. To me it seems more feature complete to have a driver option for the physical capabilities of the chip, but if it doesn't actually solve a problem it might just be bloat. > > Still if you see a need to have this external pin reset as an optional > function then it may be needed for all the vendor specific MAC drivers. > In that case, reset-gpios parameter value alone can be taken from the > chip specific device tree and the remaining code for operating the reset > gpio can be moved to oa_tc6.c and the function name can be > oa_tc6_hw_reset_macphy(). > If the consensus is to keep a HW reset I do like this suggestion. I won't push for this to be included, if it is, I'm happy to address the feedback of patch 13. R
> You sent this patch in an odd way. We don't normally see headers like > this. I've been using b4 recently for patch management: Sorry about that, I appreciate the suggestion! You left good comments on the implementation side as well, I'll wait with addressing these. Parthibans feedback does invalidate the core of the problem I set out to solve with this patch. If the patch is not dropped I'll ofc address these points! R
> Please run scripts/checkpatch.pl and fix reported warnings. Then please > run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code > here looks like it needs a fix. Feel free to get in touch if the warning > is not clear. > Very much appreciated! I'll hold off on any further submissions for this patch until it's clear that it's desriable to include. R
> Additionally I figured out why my setup did not work without the HW > reset, I had missed a pull resistor in the schematic that held the IC in > reset. Having a reset controlled by software is a pretty common design. Something needs to ensure the device is out of reset. It could be the bootloader, but i don't particularly like that, hiding away critical things where they are hard to see. So i think having it in the Linux driver is better. There is an open question of does the driver need to actually reset the device, or is it sufficient to ensure it is out of reset? The wording of the standard suggests a hardware reset cycle is probably not required, but why did Microchip provide a reset pin? Andrew
> > Additionally I figured out why my setup did not work without the HW > > reset, I had missed a pull resistor in the schematic that held the IC in > > reset. > > Having a reset controlled by software is a pretty common > design. Something needs to ensure the device is out of reset. It could > be the bootloader, but i don't particularly like that, hiding away > critical things where they are hard to see. So i think having it in > the Linux driver is better. Wholeheartedly agree, beyond the basics of bringing up ram, cores etc. it becomes really weird when/if the bootloaders behaviour defines kernel functionality. In this case the oa_tc6 module does a soft reset and waits for a status reg to signal ready. What seems to be missing here is that the chip signals ready/out of reset by asserting the irq pin and setting the RESETC bit of ther OA_STATUS0 reg (which is defined behaviour in the OA spec as I understand it). Neither the lan865x_probe or oa_tc6_init checks for the initial condition, but I'm guessing it's a fair assumption that the chip is out of reset by the point when the oa_tc6_sw_reset_machphy function is invoked. Far as I can tell no timing information is given in the datasheet. Might be unecessary but the setup could be made more explicit/clear with a func such as: int oa_tc6_out_of_reset(struct oa_tc*); Which should be invoked before any reg access/modification code in either oa_tc6 or the mac driver. If this fails my (opinionated) preferred style would be to do one hw reset, recheck, and on subsequent failure bail. Such a change would probably lead to the HW reset being invoked on reboots (if there is enough capacitors to keep the IC powered) and definetly as a result of kexec calls. > > There is an open question of does the driver need to actually reset > the device, or is it sufficient to ensure it is out of reset? The > wording of the standard suggests a hardware reset cycle is probably > not required, but why did Microchip provide a reset pin? > This has me tripped up. The lan8650/1 has a configuration write protection mechanism with an unlock sequence, described in "4.6.3 Configuration Protection" of the datasheet. The unlocking can be bypassed/simplified with a HW reset, still does not seem like an explanation for the functionality. I can't define one scenario where the HW reset is definetly necessary, but will probably do it anyways on the systems I work on. Ramón
On 29/04/24 5:49 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> Additionally I figured out why my setup did not work without the HW >> reset, I had missed a pull resistor in the schematic that held the IC in >> reset. > > Having a reset controlled by software is a pretty common > design. Something needs to ensure the device is out of reset. It could > be the bootloader, but i don't particularly like that, hiding away > critical things where they are hard to see. So i think having it in > the Linux driver is better. > > There is an open question of does the driver need to actually reset > the device, or is it sufficient to ensure it is out of reset? The > wording of the standard suggests a hardware reset cycle is probably > not required, but why did Microchip provide a reset pin? There are three resets mentioned, 1. Power ON reset - A Power-On Reset occurs when power is initially applied to the device. Once the reset completes, the IRQ_N pin will be asserted and the RESETC (Reset Completed) bit of the OA_STATUS0 Register will be set to 1, as specified by the Open Alliance. This indicates to the driver that the device has been reset and ready to configure. For a pre configured system this is enough to start the configuration. 2. Software reset - This reset can be done from software. In the driver, it is done as part of initialization. Actually it may not be needed if we are not going to reload the driver, as the Power ON reset does the same job for us. But if we compiled the driver as a loadable module and out of tree then it is needed for each time when we reload the driver without power reset. 3. Ext pin reset - A hardware reset will occur when the RESET_N pin is asserted. Once the RESET_N input is deasserted, the LAN8650/1 will restart operation. The device will indicate to the station's controller that it has been reset and must be configured in the same way as if a Power-On Reset had occurred: the IRQ_N pin will be asserted and the RESETC (Reset Completed) bit of the OA_STATUS Register will be set to 1. This is also basically does the same job but it may be useful when the chip gets into a situation where it can't perform SPI anymore to do a software reset. This may be needed in the initial phase testing. But once the development is completed and the chip is ready to use then there will not be any possibility to get into the above situation unless there is a permanent hardware failure where this reset also will not make any sense. OPEN Alliance spec says the following in the section 8.2 Variables reset This variable reflects the logical-OR of all reset sources of the MAC-PHY and is TRUE when any of the reset sources are asserted. Reset sources include power-on reset (POR), software reset (see Section 9.2.4.2), and an external RESET pin (if implemented). In the spec, external RESET pin is mentioned as "if implemented", in my understanding it is MAC-PHY vendors choice of implementing it where Microchip is implemented it. Using this reset, can be a application requirement/decision. It can be controlled from an external application where it is not needed SPI to operate. Best regards, Parthiban V > > Andrew
> In the spec, external RESET pin is mentioned as "if implemented", in my > understanding it is MAC-PHY vendors choice of implementing it where > Microchip is implemented it. Using this reset, can be a application > requirement/decision. It can be controlled from an external application > where it is not needed SPI to operate. Since it is optional, controlling the reset pin is clearly not something for the TC6 core. However, i doubt having an external application controlling the reset is a good idea. You don't want to reset during operation. So to me, this reset should be controlled by the driver. I tend to agree, that actually performing a reset is optional, but i would expect the driver to ensure the device is taken out of reset during probe, if the power on default of the board is to hold it in reset. Andrew
Hi Andrew, On 30/04/24 7:44 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> In the spec, external RESET pin is mentioned as "if implemented", in my >> understanding it is MAC-PHY vendors choice of implementing it where >> Microchip is implemented it. Using this reset, can be a application >> requirement/decision. It can be controlled from an external application >> where it is not needed SPI to operate. > > Since it is optional, controlling the reset pin is clearly not > something for the TC6 core. > > However, i doubt having an external application controlling the reset > is a good idea. You don't want to reset during operation. So to me, > this reset should be controlled by the driver. I tend to agree, that > actually performing a reset is optional, but i would expect the driver > to ensure the device is taken out of reset during probe, if the power > on default of the board is to hold it in reset. POR - Power ON Reset. If I understand you correctly, we need to ensure that whether the MAC-PHY is out of POR before doing any SPI operation right? so you expect oa_tc6_sw_reset_macphy() also may fail as the SWRESET bit write may fail if the device is still in POR. Basically oa_tc6_sw_reset_macphy() does the following, - Writes 1 to SWRESET bit in the Reset Control and Status Register to trigger software reset. - Polls for software reset complete for every 1ms until 1s timeout. - Clears reset status complete status. Can we rename the function like oa_tc6_check_power_on_reset_complete() and the definition like below? - Poll for software reset complete for every 1ms until 1s timeout. The return value of oa_tc6_read_register() may not be needed to check for error as it might fail if the POR is taking place right? - Clear reset complete status. But this case will work fine with initial power ON but after that if we are only reloading the driver for next time without powering OFF and ON then it will fail. I am just thinking from TC6 framework point of view. Controlling from ext reset pin will not be an option as it is mentioned like an optional implementation in the OPEN Alliance. So some MAC-PHY vendors might not implement this. Just an open question, after powering ON the Linux system, there are different stages are involved in the booting sequence of a system before kernel driver loads. A device like MAC-PHY which takes hardly around 5 to 10us will complete the POR before that? OR is my understanding here is wrong? Best regards, Parthiban V > > Andrew
diff --git a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml index 4fdec0ba3532..0f11f431df06 100644 --- a/Documentation/devicetree/bindings/net/microchip,lan865x.yaml +++ b/Documentation/devicetree/bindings/net/microchip,lan865x.yaml @@ -44,6 +44,9 @@ properties: minimum: 15000000 maximum: 25000000 + reset-gpios: + maxItems: 1 + "#address-cells": const: 1 @@ -76,5 +79,6 @@ examples: interrupts = <6 IRQ_TYPE_EDGE_FALLING>; local-mac-address = [04 05 06 01 02 03]; spi-max-frequency = <15000000>; + reset-gpios = <&gpio2 8 GPIO_ACTIVE_HIGH>; }; }; diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c index 9abefa8b9d9f..bed9033574b2 100644 --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c @@ -9,6 +9,7 @@ #include <linux/kernel.h> #include <linux/phy.h> #include <linux/oa_tc6.h> +#include <linux/gpio/driver.h> #define DRV_NAME "lan865x" @@ -33,6 +34,7 @@ struct lan865x_priv { struct work_struct multicast_work; + struct gpio_desc *reset_gpio; struct net_device *netdev; struct spi_device *spi; struct oa_tc6 *tc6; @@ -283,6 +285,24 @@ static int lan865x_set_zarfe(struct lan865x_priv *priv) return oa_tc6_write_register(priv->tc6, OA_TC6_REG_CONFIG0, regval); } +static int lan865x_probe_reset_gpio(struct lan865x_priv *priv) +{ + priv->reset_gpio = devm_gpiod_get_optional(&priv->spi->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(priv->reset_gpio)) + return PTR_ERR(priv->reset_gpio); + + return 0; +} + +static void lan865x_hw_reset(struct lan865x_priv *priv) +{ + gpiod_set_value_cansleep(priv->reset_gpio, 1); + // section 9.6.3 RESET_N Timing specifies a minimum hold of 5us + usleep_range(5, 10); + gpiod_set_value_cansleep(priv->reset_gpio, 0); +} + static int lan865x_probe(struct spi_device *spi) { struct net_device *netdev; @@ -297,6 +317,14 @@ static int lan865x_probe(struct spi_device *spi) priv->netdev = netdev; priv->spi = spi; spi_set_drvdata(spi, priv); + if (lan865x_probe_reset_gpio(priv)) { + dev_err(&spi->dev, "failed to probe reset pin"); + ret = -ENODEV; + goto free_netdev; + } + + if (priv->reset_gpio) + lan865x_hw_reset(priv); INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler); priv->tc6 = oa_tc6_init(spi, netdev);