| Message ID | 20210421055047.22858-1-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 357a07c26697a770d39d28b6b111f978deb4017d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net: phy: intel-xway: enable integrated led functions | 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 |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| 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 | warning | WARNING: line length of 92 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Wed, 21 Apr 2021 07:50:47 +0200 you wrote: > The Intel xway phys offer the possibility to deactivate the integrated > LED function and to control the LEDs manually. > If this was set by the bootloader, it must be ensured that the > integrated LED function is enabled for all LEDs when loading the driver. > > Before commit 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > the LEDs were enabled by a soft-reset of the PHY (using > genphy_soft_reset). Initialize the XWAY_MDIO_LED with it's default > value (which is applied during a soft reset) instead of adding back > the soft reset. This brings back the default LED configuration while > still preventing an excessive amount of soft resets. > > [...] Here is the summary with links: - [net,v3] net: phy: intel-xway: enable integrated led functions https://git.kernel.org/netdev/net/c/357a07c26697 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Wed, Apr 21, 2021 at 7:51 AM Martin Schiller <ms@dev.tdt.de> wrote: > > The Intel xway phys offer the possibility to deactivate the integrated > LED function and to control the LEDs manually. > If this was set by the bootloader, it must be ensured that the > integrated LED function is enabled for all LEDs when loading the driver. > > Before commit 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > the LEDs were enabled by a soft-reset of the PHY (using > genphy_soft_reset). Initialize the XWAY_MDIO_LED with it's default > value (which is applied during a soft reset) instead of adding back > the soft reset. This brings back the default LED configuration while > still preventing an excessive amount of soft resets. > > Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > Signed-off-by: Martin Schiller <ms@dev.tdt.de> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
On Tue, Apr 20, 2021 at 10:51 PM Martin Schiller <ms@dev.tdt.de> wrote: > > The Intel xway phys offer the possibility to deactivate the integrated > LED function and to control the LEDs manually. > If this was set by the bootloader, it must be ensured that the > integrated LED function is enabled for all LEDs when loading the driver. > > Before commit 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > the LEDs were enabled by a soft-reset of the PHY (using > genphy_soft_reset). Initialize the XWAY_MDIO_LED with it's default > value (which is applied during a soft reset) instead of adding back > the soft reset. This brings back the default LED configuration while > still preventing an excessive amount of soft resets. > > Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") > Signed-off-by: Martin Schiller <ms@dev.tdt.de> > --- > > Changes to v2: > o Fixed commit message > o Fixed email recipients once again. > > Changes to v1: > Added additional email recipients. > > --- > drivers/net/phy/intel-xway.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c > index 6eac50d4b42f..d453ec016168 100644 > --- a/drivers/net/phy/intel-xway.c > +++ b/drivers/net/phy/intel-xway.c > @@ -11,6 +11,18 @@ > > #define XWAY_MDIO_IMASK 0x19 /* interrupt mask */ > #define XWAY_MDIO_ISTAT 0x1A /* interrupt status */ > +#define XWAY_MDIO_LED 0x1B /* led control */ > + > +/* bit 15:12 are reserved */ > +#define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the integrated function of LED3 */ > +#define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the integrated function of LED2 */ > +#define XWAY_MDIO_LED_LED1_EN BIT(9) /* Enable the integrated function of LED1 */ > +#define XWAY_MDIO_LED_LED0_EN BIT(8) /* Enable the integrated function of LED0 */ > +/* bit 7:4 are reserved */ > +#define XWAY_MDIO_LED_LED3_DA BIT(3) /* Direct Access to LED3 */ > +#define XWAY_MDIO_LED_LED2_DA BIT(2) /* Direct Access to LED2 */ > +#define XWAY_MDIO_LED_LED1_DA BIT(1) /* Direct Access to LED1 */ > +#define XWAY_MDIO_LED_LED0_DA BIT(0) /* Direct Access to LED0 */ > > #define XWAY_MDIO_INIT_WOL BIT(15) /* Wake-On-LAN */ > #define XWAY_MDIO_INIT_MSRE BIT(14) > @@ -159,6 +171,15 @@ static int xway_gphy_config_init(struct phy_device *phydev) > /* Clear all pending interrupts */ > phy_read(phydev, XWAY_MDIO_ISTAT); > > + /* Ensure that integrated led function is enabled for all leds */ > + err = phy_write(phydev, XWAY_MDIO_LED, > + XWAY_MDIO_LED_LED0_EN | > + XWAY_MDIO_LED_LED1_EN | > + XWAY_MDIO_LED_LED2_EN | > + XWAY_MDIO_LED_LED3_EN); > + if (err) > + return err; > + > phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LEDCH, > XWAY_MMD_LEDCH_NACS_NONE | > XWAY_MMD_LEDCH_SBF_F02HZ | > -- > 2.20.1 > Hi Martin, Similar to my response in another thread to how be393dd685d2 ("net: phy: intel-xway: Add RGMII internal delay configuration") which changes the tx/rx interna delays to default values of 2ns if the common delay properties are not found in the dt and thus may override what boot firmware configured, I do not like the fact that this patch just overrides LED configuration that boot firmware may have setup. I am aware that there is not much consistency in PHY's for LED configuration which makes coming up with common dt bindings impossible but I feel that if PHY drivers add LED configuration they should only apply it if new bindings are found instructing it to. Perhaps it makes sense to at least create a common binding that allows configuration of LED's here? As a person responsible for boot firmware through kernel for a set of boards I continue to do the following to keep Linux from mucking with various PHY configurations: - remove PHY reset pins from Linux DT's to keep Linux from hard resetting PHY's - disabling PHY drivers What are your thoughts about this? Best regards, Tim
> As a person responsible for boot firmware through kernel for a set of > boards I continue to do the following to keep Linux from mucking with > various PHY configurations: > - remove PHY reset pins from Linux DT's to keep Linux from hard resetting PHY's > - disabling PHY drivers > > What are your thoughts about this? Hi Tim I don't like the idea that the bootloader is controlling the hardware, not linux. There are well defined ways for telling Linux how RGMII delays should be set, and most PHY drivers do this. Any which don't should be extended to actually set the delay as configured. LEDs are trickier. There is a slow on going effort to allow PHY LEDs to be configured as standard Linux LEDs. That should result in a DT binding which can be used to configure LEDs from DT. You probably are going to have more and more issues if you think the bootloader is controlling the hardware, so you really should stop trying to do that. Andrew
On 2/2/2022 5:01 PM, Andrew Lunn wrote: >> As a person responsible for boot firmware through kernel for a set of >> boards I continue to do the following to keep Linux from mucking with >> various PHY configurations: >> - remove PHY reset pins from Linux DT's to keep Linux from hard resetting PHY's >> - disabling PHY drivers >> >> What are your thoughts about this? > > Hi Tim > > I don't like the idea that the bootloader is controlling the hardware, > not linux. This is really trying to take advantage of the boot loader setting things up in a way that Linux can play dumb by using the Generic PHY driver and being done with it. This works... until it stops, which happens very very quickly in general. The perfect counter argument to using the Generic PHY driver is when your system implements a low power mode where the PHY loses its power/settings, comes up from suspend and the strap configuration is insufficient and the boot loader is not part of the resume path *prior* to Linux. In that case Linux needs to restore the settings, but it needs a PHY driver for that. If your concern Tim is with minimizing the amount of time the link gets dropped and re-established, then there is not really much that can be done that is compatible with Linux setting things up, short of minimizing the amount of register writes that do need the "commit phase" via BMCR.RESET. I do agree that blindly imposing LED settings that are different than those you want is not great, and should be remedied. Maybe you can comment this part out in your downstream tree for a while until the LED binding shows up (we have never been so close I am told).
On Wed, Feb 2, 2022 at 5:01 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > As a person responsible for boot firmware through kernel for a set of > > boards I continue to do the following to keep Linux from mucking with > > various PHY configurations: > > - remove PHY reset pins from Linux DT's to keep Linux from hard resetting PHY's > > - disabling PHY drivers > > > > What are your thoughts about this? > > Hi Tim > > I don't like the idea that the bootloader is controlling the hardware, > not linux. > > There are well defined ways for telling Linux how RGMII delays should > be set, and most PHY drivers do this. Any which don't should be > extended to actually set the delay as configured. Andrew, I agree with the goal of having PHY drivers and dt-bindings in Linux to configure everything but in the case I mention in the other thread adding rgmii delay configuration which sets a default if a new dt binding is missing is wrong in my opinion as it breaks backward compatibility. If a new dt binding is missing then I feel that the register fields those bindings act on should not be changed. > > LEDs are trickier. There is a slow on going effort to allow PHY LEDs > to be configured as standard Linux LEDs. That should result in a DT > binding which can be used to configure LEDs from DT. Can you point me to something I can look at? PHY LED bindings don't at all behave like normal LED's as they are blinked internally depending on a large set of rules that differ per PHY. > > You probably are going to have more and more issues if you think the > bootloader is controlling the hardware, so you really should stop > trying to do that. > Trust me, I would love to stop trying to hide PHY config from Linux. It's painful to bump to a new kernel and find something broken. In this case I found that my LED configuration broke and in the other patch I found that my RGMII delays broke. Completely off topic, but due to the chip shortage we have had to redesign many of our boards with different PHY's that now have different bindings for RGMII delays so I have to add multiple PHY configurations to DT's if I am going to support the use of PHY drivers. What is your suggestion there? Using DT overlays I suppose is the right approach. Tim
On Wed, Feb 2, 2022 at 7:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 2/2/2022 5:01 PM, Andrew Lunn wrote: > >> As a person responsible for boot firmware through kernel for a set of > >> boards I continue to do the following to keep Linux from mucking with > >> various PHY configurations: > >> - remove PHY reset pins from Linux DT's to keep Linux from hard resetting PHY's > >> - disabling PHY drivers > >> > >> What are your thoughts about this? > > > > Hi Tim > > > > I don't like the idea that the bootloader is controlling the hardware, > > not linux. > > This is really trying to take advantage of the boot loader setting > things up in a way that Linux can play dumb by using the Generic PHY > driver and being done with it. This works... until it stops, which > happens very very quickly in general. The perfect counter argument to > using the Generic PHY driver is when your system implements a low power > mode where the PHY loses its power/settings, comes up from suspend and > the strap configuration is insufficient and the boot loader is not part > of the resume path *prior* to Linux. In that case Linux needs to restore > the settings, but it needs a PHY driver for that. Florian, That makes sense - I'm always trying to figure out what the advantage of using some of these PHY drivers really is vs disabling them. > > If your concern Tim is with minimizing the amount of time the link gets > dropped and re-established, then there is not really much that can be > done that is compatible with Linux setting things up, short of > minimizing the amount of register writes that do need the "commit phase" > via BMCR.RESET. No, my reasoning has nothing to do with link time - I have just run into several cases where some new change in a PHY driver blatantly either resets the PHY reverting to pin-strapping config which is wrong (happend to me with DP83867 but replacing the 'reset' to a 'restart' solved that) or imposes some settings without dt bindings to guide it (this case with the LEDs) or imposes some settings based on 'new' dt-bindings which I was simply not aware of (a lesser issue as dt bindings can be added to resolve it). > > I do agree that blindly imposing LED settings that are different than > those you want is not great, and should be remedied. Maybe you can > comment this part out in your downstream tree for a while until the LED > binding shows up (we have never been so close I am told). or disable the driver in defconfig, or blacklist the module if I want to do it via rootfs. Can you point me to something I can look at for these new LED bindings that are being worked on? Best Regards, Tim
> Andrew, > > I agree with the goal of having PHY drivers and dt-bindings in Linux > to configure everything but in the case I mention in the other thread > adding rgmii delay configuration which sets a default if a new dt > binding is missing is wrong in my opinion as it breaks backward > compatibility. If a new dt binding is missing then I feel that the > register fields those bindings act on should not be changed. I would like that understand this specific case in more detail. We have seen a few cases were the DT is broken, yet works. This is often caused by having a wrong phy-mode, which historically the PHY driver was ignoring. Then support for honouring the phy-mode was added to the PHY driver, and all the boards with broken DT files actually break. So it could be that is what has happened here. Or it could be the driver is plan wrong. If i understand correctly, you say it is adding a default delay of 2ns. That would be correct for a phy-mode of rgmii-id, but wrong for a phy-mode of rgmii. > > LEDs are trickier. There is a slow on going effort to allow PHY LEDs > > to be configured as standard Linux LEDs. That should result in a DT > > binding which can be used to configure LEDs from DT. > > Can you point me to something I can look at? PHY LED bindings don't at > all behave like normal LED's as they are blinked internally depending > on a large set of rules that differ per PHY. Yes, this is what is slowing the work done, agreeing on details like this, and how the user space API would actually work. In the end, i suspect a subset of LED modes will be supported, covering the common blink patterns. > Completely off topic, but due to the chip shortage we have had to > redesign many of our boards with different PHY's that now have > different bindings for RGMII delays so I have to add multiple PHY > configurations to DT's if I am going to support the use of PHY > drivers. What is your suggestion there? Using DT overlays I suppose is > the right approach. I would try to only use phy-mode, and avoid all PHY specific tweaks. So long as the track lengths don't change too much on your redesign, and are kept about the same length, the standard 2ns delay should work. Andrew
On Thu, Feb 3, 2022 at 8:37 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > Andrew, > > > > I agree with the goal of having PHY drivers and dt-bindings in Linux > > to configure everything but in the case I mention in the other thread > > adding rgmii delay configuration which sets a default if a new dt > > binding is missing is wrong in my opinion as it breaks backward > > compatibility. If a new dt binding is missing then I feel that the > > register fields those bindings act on should not be changed. > > I would like that understand this specific case in more detail. We > have seen a few cases were the DT is broken, yet works. This is often > caused by having a wrong phy-mode, which historically the PHY driver > was ignoring. Then support for honouring the phy-mode was added to the > PHY driver, and all the boards with broken DT files actually break. > > So it could be that is what has happened here. Or it could be the > driver is plan wrong. If i understand correctly, you say it is adding > a default delay of 2ns. That would be correct for a phy-mode of > rgmii-id, but wrong for a phy-mode of rgmii. > > > > LEDs are trickier. There is a slow on going effort to allow PHY LEDs > > > to be configured as standard Linux LEDs. That should result in a DT > > > binding which can be used to configure LEDs from DT. > > > > Can you point me to something I can look at? PHY LED bindings don't at > > all behave like normal LED's as they are blinked internally depending > > on a large set of rules that differ per PHY. > > Yes, this is what is slowing the work done, agreeing on details like > this, and how the user space API would actually work. In the end, i > suspect a subset of LED modes will be supported, covering the common > blink patterns. > > > Completely off topic, but due to the chip shortage we have had to > > redesign many of our boards with different PHY's that now have > > different bindings for RGMII delays so I have to add multiple PHY > > configurations to DT's if I am going to support the use of PHY > > drivers. What is your suggestion there? Using DT overlays I suppose is > > the right approach. > > I would try to only use phy-mode, and avoid all PHY specific tweaks. > So long as the track lengths don't change too much on your redesign, > and are kept about the same length, the standard 2ns delay should > work. > Andrew, The issue is that in an ideal world where all parts adhere to the JEDEC standards 2ns would be correct but that is not reality. In my experience with the small embedded boards I help design and support not about trace lengths as it would take inches to skew 0.5ns but instead differences in setup/hold values for various MAC/PHY parts that are likely not adhering the standards. Some examples from current boards I support: - CN8030 MAC rgmii-rxid with intel-xway GPY111 PHY: we need to configure this for rx_delay=1.5ns and tx_delay=0.5ns - CN8030 MAC rgmii-rxid with dp83867 GPY111 PHY: we need to configure this for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected) - IMX8MM FEC MAC rgmii-id with intel-xway PHY: we need to configure this for rx_delay=2ns and tx_delay=2.5ns - IMX8MM FEC MAC rgmii-id with dp83867 PHY: we need to configure this for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected) The two boards above have identical well matched trace-lengths between the two PHY variant designs so you can see here that the intel-xway GPY111 PHY needs an extra 0.5ps delay to avoid RX errors on the far-off board. I really hate this GPY111 PHY but it's the only PHY we can source currently; it's full of errata and to make things worse the only available pin strapping options are for 0ns or 1.5ns (how is 1.5ns useful?) so we must rely on software configuration. So having the intel-xway PHY driver set the delays to 2.0ns delays in the absence of dt props (vs leaving them untouched) is what bothered me there. The LED blink configuration has much more flexible strapping options which we are able to use until this driver goes and changes them to something else. The way I determine the correct delay values is by looking for MAC RX errors on the RX side and by looking for RX errors on the off-board receiving end (typically via a managed switch). I know of no better way to do this because the timing happens inside the MAC and PHY thus scope measurements don't help here. Best regards, Tim
> Andrew, > > The issue is that in an ideal world where all parts adhere to the > JEDEC standards 2ns would be correct but that is not reality. In my > experience with the small embedded boards I help design and support > not about trace lengths as it would take inches to skew 0.5ns but > instead differences in setup/hold values for various MAC/PHY parts > that are likely not adhering the standards. > > Some examples from current boards I support: > - CN8030 MAC rgmii-rxid with intel-xway GPY111 PHY: we need to > configure this for rx_delay=1.5ns and tx_delay=0.5ns So i assume this is what broke for you. Your bootloader sets these delays, phy-type is rgmii-id, and since you don't have the properties for what exact delays you want it default to what 802.3 specifies, 2ns. I actually think the current behaviour of the driver is correct. By saying phy-type = rgmii-id you are telling the PHY driver to insert the delays and that is what it is doing. In retrospect, i would say you had two choices to cleanly solve this. 1) Do exactly what the patch does, add properties to define what actual delay you want, when you don't want 2ns. 2) Have the bootloader set the delay, but also tell Linux you have set the phy mode including the delays, and it should not touch them. There is a well defined way to do this: PHY_INTERFACE_MODE_NA It has two main use cases. The first is that the MAC and PHY are integrated, there is no MII between them, phy-mode is completely unnecessary. This is the primary meaning. The second meaning is, something else has setup the phy mode, e.g. the bootloader. Don't touch it. This second meaning does not always work, since the driver might reset the PHY, and depending on the PHY, that might clear whatever the bootloader set. So it is not reliable. > - CN8030 MAC rgmii-rxid with dp83867 GPY111 PHY: we need to configure > this for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected) > - IMX8MM FEC MAC rgmii-id with intel-xway PHY: we need to configure > this for rx_delay=2ns and tx_delay=2.5ns > - IMX8MM FEC MAC rgmii-id with dp83867 PHY: we need to configure this > for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected) > > The two boards above have identical well matched trace-lengths between > the two PHY variant designs so you can see here that the intel-xway > GPY111 PHY needs an extra 0.5ps delay to avoid RX errors on the > far-off board. So a couple of ideas. Since GPY111 and dp83867 use different properties for the delays, just put both in DT. The PHY driver will look for the property it understands and ignore the other. So you can have different delays for different PHYs. We have started to standardize on the delay property. And new driver should be using rx-internal-delay-ps and tx-internal-delay-ps. If you have two drivers using the same property name, what i just suggested will not work. Can you control the address the PHY uses? Put the GPY111 on a different address to the dp83867. List them both in DT. If you don't have a phy-handle in DT, the FEC will go hunting on its MDIO bus and use the first PHY it finds. You can put the needed delay properties in DT for the specific PHY. Andrew
On 2/3/2022 8:02 AM, Tim Harvey wrote: > On Wed, Feb 2, 2022 at 7:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> >> >> On 2/2/2022 5:01 PM, Andrew Lunn wrote: >>>> As a person responsible for boot firmware through kernel for a set of >>>> boards I continue to do the following to keep Linux from mucking with >>>> various PHY configurations: >>>> - remove PHY reset pins from Linux DT's to keep Linux from hard resetting PHY's >>>> - disabling PHY drivers >>>> >>>> What are your thoughts about this? >>> >>> Hi Tim >>> >>> I don't like the idea that the bootloader is controlling the hardware, >>> not linux. >> >> This is really trying to take advantage of the boot loader setting >> things up in a way that Linux can play dumb by using the Generic PHY >> driver and being done with it. This works... until it stops, which >> happens very very quickly in general. The perfect counter argument to >> using the Generic PHY driver is when your system implements a low power >> mode where the PHY loses its power/settings, comes up from suspend and >> the strap configuration is insufficient and the boot loader is not part >> of the resume path *prior* to Linux. In that case Linux needs to restore >> the settings, but it needs a PHY driver for that. > > Florian, > > That makes sense - I'm always trying to figure out what the advantage > of using some of these PHY drivers really is vs disabling them. > >> >> If your concern Tim is with minimizing the amount of time the link gets >> dropped and re-established, then there is not really much that can be >> done that is compatible with Linux setting things up, short of >> minimizing the amount of register writes that do need the "commit phase" >> via BMCR.RESET. > > No, my reasoning has nothing to do with link time - I have just run > into several cases where some new change in a PHY driver blatantly > either resets the PHY reverting to pin-strapping config which is wrong > (happend to me with DP83867 but replacing the 'reset' to a 'restart' > solved that) or imposes some settings without dt bindings to guide it > (this case with the LEDs) or imposes some settings based on 'new' > dt-bindings which I was simply not aware of (a lesser issue as dt > bindings can be added to resolve it). > >> >> I do agree that blindly imposing LED settings that are different than >> those you want is not great, and should be remedied. Maybe you can >> comment this part out in your downstream tree for a while until the LED >> binding shows up (we have never been so close I am told). > > or disable the driver in defconfig, or blacklist the module if I want > to do it via rootfs. > > Can you point me to something I can look at for these new LED bindings > that are being worked on? > This is the latest attempt AFAICT: https://lore.kernel.org/netdev/20211112153557.26941-1-ansuelsmth@gmail.com/
On Thu, Feb 3, 2022 at 4:04 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Andrew, > > > > The issue is that in an ideal world where all parts adhere to the > > JEDEC standards 2ns would be correct but that is not reality. In my > > experience with the small embedded boards I help design and support > > not about trace lengths as it would take inches to skew 0.5ns but > > instead differences in setup/hold values for various MAC/PHY parts > > that are likely not adhering the standards. > > > > Some examples from current boards I support: > > - CN8030 MAC rgmii-rxid with intel-xway GPY111 PHY: we need to > > configure this for rx_delay=1.5ns and tx_delay=0.5ns > > So i assume this is what broke for you. Your bootloader sets these > delays, phy-type is rgmii-id, and since you don't have the properties > for what exact delays you want it default to what 802.3 specifies, > 2ns. > > I actually think the current behaviour of the driver is correct. By > saying phy-type = rgmii-id you are telling the PHY driver to insert > the delays and that is what it is doing. > > In retrospect, i would say you had two choices to cleanly solve this. > > 1) Do exactly what the patch does, add properties to define what > actual delay you want, when you don't want 2ns. > > 2) Have the bootloader set the delay, but also tell Linux you have set > the phy mode including the delays, and it should not touch them. There > is a well defined way to do this: > > PHY_INTERFACE_MODE_NA > > It has two main use cases. The first is that the MAC and PHY are > integrated, there is no MII between them, phy-mode is completely > unnecessary. This is the primary meaning. > > The second meaning is, something else has setup the phy mode, e.g. the > bootloader. Don't touch it. > > This second meaning does not always work, since the driver might reset > the PHY, and depending on the PHY, that might clear whatever the > bootloader set. So it is not reliable. Andrew, I appreciate your suggestions. The PHY_INTERRFACE_MODE_NA is a neat trick that I will remember but it would only help with the rgmii delay issue and not the LED issue (this patch). The GPY111 has some really nasty errata that is going to cause me to have a very hackish work-around anyway and I will be disabling the PHY driver to stay out of the way of that workaround so in this case here I'm not looking for a solution. > > > - CN8030 MAC rgmii-rxid with dp83867 GPY111 PHY: we need to configure > > this for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected) > > - IMX8MM FEC MAC rgmii-id with intel-xway PHY: we need to configure > > this for rx_delay=2ns and tx_delay=2.5ns > > - IMX8MM FEC MAC rgmii-id with dp83867 PHY: we need to configure this > > for rx_delay=2.0ns and tx_delay=2.0ns (as would be expected) > > > > The two boards above have identical well matched trace-lengths between > > the two PHY variant designs so you can see here that the intel-xway > > GPY111 PHY needs an extra 0.5ps delay to avoid RX errors on the > > far-off board. > > So a couple of ideas. > > Since GPY111 and dp83867 use different properties for the delays, just > put both in DT. The PHY driver will look for the property it > understands and ignore the other. So you can have different delays for > different PHYs. Yes, this is what I am currently doing but I'm not sure that would ever be accepted as a dt change upstream and at some point when the common property for the delays is supported in both PHY drivers that will not work (as you point out below). I do a lot of dt fixups in boot firmware already to deal with board revision's so I'll probably deal with it that way at some point. I believe I have made a good case why defaulting to 2ns delays does not work for all RGMII MAC/PHY scenarios. If 2ns was all you needed then all these PHY's would not have adjustable delays (its not just about trace-lengths as they have to be off more than an insh for 0.5ps and there are a ton of tiny embedded boards out there with delays between 0.5 and 2.5ns). As far as changing a driver to force a LED configuration with no dt binding input (like this patch does) it feels wrong for exactly the same reason - LED configuration for this PHY can be done via pin-strapping and this driver now undoes that with this patch. I still feel very strongly that a PHY driver should not change configuration which may be based on pin-strapping or boot firmware changes when no dt properties exist telling it to make that specific change. However if my opinion is not the popular one I will stop bringing it up. Best regards, Tim > > We have started to standardize on the delay property. And new driver > should be using rx-internal-delay-ps and tx-internal-delay-ps. If you > have two drivers using the same property name, what i just suggested > will not work. Can you control the address the PHY uses? Put the > GPY111 on a different address to the dp83867. List them both in DT. If > you don't have a phy-handle in DT, the FEC will go hunting on its MDIO > bus and use the first PHY it finds. You can put the needed delay > properties in DT for the specific PHY. > > Andrew >
> The PHY_INTERRFACE_MODE_NA is a neat trick that I will remember but it > would only help with the rgmii delay issue and not the LED issue (this > patch). The GPY111 has some really nasty errata that is going to cause > me to have a very hackish work-around anyway and I will be disabling > the PHY driver to stay out of the way of that workaround Well, ideally we want the workaround for the erratas in the kernel drivers. In the long run, you will get less surprises if you add what you need to Linux, not hide stuff away in the bootloader. > As far as changing a driver to force a LED configuration with no dt > binding input (like this patch does) it feels wrong for exactly the > same reason - LED configuration for this PHY can be done via > pin-strapping and this driver now undoes that with this patch. Is it possible to read the pin strapping pins? If we know it has been pin strapped, then a patch to detect this and not change the LED configuration seems very likely to be accepted. Andrew
On Fri, Feb 4, 2022 at 2:54 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > The PHY_INTERRFACE_MODE_NA is a neat trick that I will remember but it > > would only help with the rgmii delay issue and not the LED issue (this > > patch). The GPY111 has some really nasty errata that is going to cause > > me to have a very hackish work-around anyway and I will be disabling > > the PHY driver to stay out of the way of that workaround > > Well, ideally we want the workaround for the erratas in the kernel > drivers. In the long run, you will get less surprises if you add what > you need to Linux, not hide stuff away in the bootloader. Andrew, I agree it is best to get workarounds for errata in the kernel where possible but this one is going to be a mess. There are 3 errata for this part and 2 of them require a toggle of the reset pin as a work around. Even if the mii bus can export a function for the phy to call to toggle reset in my case one of my boards has 2 of these PHY's (1 on a RGMII MAC and 1 on a SGMII MAC) on different MDIO busses that share a reset pin so if I trigger a reset I have to re-configure the other phy as well. The errata can be summarized as: - 1 out of 100 boots or cable plug events RGMII GbE link will end up going down and up 3 to 4 times then resort to a 100m link; workaround has been found to require a pin level reset - 1 out of 100 boots or cable plug events (varies per board) SGMII will fail link between the MAC and PHY; workaround has been found to require a pin level reset - occasionally the phy will come up with a high bit error rate between the MAC and PHY; workaround has been found to require a soft reset or ANEG restart > > > As far as changing a driver to force a LED configuration with no dt > > binding input (like this patch does) it feels wrong for exactly the > > same reason - LED configuration for this PHY can be done via > > pin-strapping and this driver now undoes that with this patch. > > Is it possible to read the pin strapping pins? If we know it has been > pin strapped, then a patch to detect this and not change the LED > configuration seems very likely to be accepted. > No, you can read the current LED configuration but you don't know if it was set via strapping or boot firmware. Best regards, Tim
> The errata can be summarized as: > - 1 out of 100 boots or cable plug events RGMII GbE link will end up > going down and up 3 to 4 times then resort to a 100m link; workaround > has been found to require a pin level reset So that sounds like it is downshifting because it thinks there is a broken pair. Can you disable downshift? Problem is, that might just result in link down. > - 1 out of 100 boots or cable plug events (varies per board) SGMII > will fail link between the MAC and PHY; workaround has been found to > require a pin level reset I don't suppose there is a register to restart SGMII sync? Sometimes there is. Anyway, shared reset makes this messy, as you said. Unfortunate design. But i don't see how you can work around this in the bootloader, especially the cable plug events. Andrew
On Wed, Feb 9, 2022 at 4:04 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > The errata can be summarized as: > > - 1 out of 100 boots or cable plug events RGMII GbE link will end up > > going down and up 3 to 4 times then resort to a 100m link; workaround > > has been found to require a pin level reset > > So that sounds like it is downshifting because it thinks there is a > broken pair. Can you disable downshift? Problem is, that might just > result in link down. Its a bad situation. The actual errata is that the device latches into a bad state where there is some noise on an ADC or something like that that cause a high packet error rate. The firmware baked into the PHY has a detection mechanism looking at these errors (SSD errors) and if there are enough of them it takes the link down and up again and if that doesn't resolve in 3 times it shifts down to 100mbs. They call this 'ADS' or 'auto-down-speed' and you can disable it but it would just result in leaving your bad gbe link up. It's unclear yet if it's better to just detect the ADS event and reset or to disable ADS and look for the SSD errors myself (which I can do). > > > - 1 out of 100 boots or cable plug events (varies per board) SGMII > > will fail link between the MAC and PHY; workaround has been found to > > require a pin level reset > > I don't suppose there is a register to restart SGMII sync? Sometimes > there is. Not that I see but I haven't really investigated too much into mitigating that issue yet. The errata for that issue says you need to assert reset but then it also says it can occur on a cable plug event which makes me think an MDI ANEG restart may be sufficient. > > Anyway, shared reset makes this messy, as you said. Unfortunate > design. But i don't see how you can work around this in the > bootloader, especially the cable plug events. > Ya, in hindsight the shared reset was a really bad idea, of course the last PHY we used on this particular board for years before the supply chain crashed didn't have any issues like this. I agree that I can't do anything in boot firmware. I was planning on having some static code that registered a PHY fixup to get a call when these PHYs were detected and I could then kick off a polling thread to watch for errors and trigger a reset. The reset could have knowledge of the PHY devices that called the fixup handler so that I can at least setup each PHY again. Regardless of how I go about this the end result may be unreliable networking for up to a couple of minutes after board power-up or cable plug event. Best Regards, Tim
On Thu, Feb 10, 2022 at 07:52:49AM -0800, Tim Harvey wrote: > On Wed, Feb 9, 2022 at 4:04 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > The errata can be summarized as: > > > - 1 out of 100 boots or cable plug events RGMII GbE link will end up > > > going down and up 3 to 4 times then resort to a 100m link; workaround > > > has been found to require a pin level reset > > > > So that sounds like it is downshifting because it thinks there is a > > broken pair. Can you disable downshift? Problem is, that might just > > result in link down. > > Its a bad situation. The actual errata is that the device latches into > a bad state where there is some noise on an ADC or something like that > that cause a high packet error rate. The firmware baked into the PHY > has a detection mechanism looking at these errors (SSD errors) and if > there are enough of them it takes the link down and up again and if > that doesn't resolve in 3 times it shifts down to 100mbs. They call > this 'ADS' or 'auto-down-speed' and you can disable it but it would > just result in leaving your bad gbe link up. It's unclear yet if it's > better to just detect the ADS event and reset or to disable ADS and > look for the SSD errors myself (which I can do). I don't think it matters too much which way you detect there is a problem. But ideally you need a recovery which does not need a hardware reset. Than you don't need to worry about the other PHY sharing the reset line. But you know that... > I agree that I can't do anything in boot firmware. I was planning on > having some static code that registered a PHY fixup to get a call when > these PHYs were detected and I could then kick off a polling thread to > watch for errors and trigger a reset. The reset could have knowledge > of the PHY devices that called the fixup handler so that I can at > least setup each PHY again. That sounds like a reasonable architecture. Your thread would need to do: phy_stop() phy_init_hw() phy_start() and phylib probably will do the reset. Maybe you can put the problem detection code in the .read_status callback, which sets am 'im_fubar' flag in the drivers private structure. That gives some building blocks for other users of this PHY who don't have a shared reset line, and can maybe solve the problem within the driver. Andrew
diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c index 6eac50d4b42f..d453ec016168 100644 --- a/drivers/net/phy/intel-xway.c +++ b/drivers/net/phy/intel-xway.c @@ -11,6 +11,18 @@ #define XWAY_MDIO_IMASK 0x19 /* interrupt mask */ #define XWAY_MDIO_ISTAT 0x1A /* interrupt status */ +#define XWAY_MDIO_LED 0x1B /* led control */ + +/* bit 15:12 are reserved */ +#define XWAY_MDIO_LED_LED3_EN BIT(11) /* Enable the integrated function of LED3 */ +#define XWAY_MDIO_LED_LED2_EN BIT(10) /* Enable the integrated function of LED2 */ +#define XWAY_MDIO_LED_LED1_EN BIT(9) /* Enable the integrated function of LED1 */ +#define XWAY_MDIO_LED_LED0_EN BIT(8) /* Enable the integrated function of LED0 */ +/* bit 7:4 are reserved */ +#define XWAY_MDIO_LED_LED3_DA BIT(3) /* Direct Access to LED3 */ +#define XWAY_MDIO_LED_LED2_DA BIT(2) /* Direct Access to LED2 */ +#define XWAY_MDIO_LED_LED1_DA BIT(1) /* Direct Access to LED1 */ +#define XWAY_MDIO_LED_LED0_DA BIT(0) /* Direct Access to LED0 */ #define XWAY_MDIO_INIT_WOL BIT(15) /* Wake-On-LAN */ #define XWAY_MDIO_INIT_MSRE BIT(14) @@ -159,6 +171,15 @@ static int xway_gphy_config_init(struct phy_device *phydev) /* Clear all pending interrupts */ phy_read(phydev, XWAY_MDIO_ISTAT); + /* Ensure that integrated led function is enabled for all leds */ + err = phy_write(phydev, XWAY_MDIO_LED, + XWAY_MDIO_LED_LED0_EN | + XWAY_MDIO_LED_LED1_EN | + XWAY_MDIO_LED_LED2_EN | + XWAY_MDIO_LED_LED3_EN); + if (err) + return err; + phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LEDCH, XWAY_MMD_LEDCH_NACS_NONE | XWAY_MMD_LEDCH_SBF_F02HZ |
The Intel xway phys offer the possibility to deactivate the integrated LED function and to control the LEDs manually. If this was set by the bootloader, it must be ensured that the integrated LED function is enabled for all LEDs when loading the driver. Before commit 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") the LEDs were enabled by a soft-reset of the PHY (using genphy_soft_reset). Initialize the XWAY_MDIO_LED with it's default value (which is applied during a soft reset) instead of adding back the soft reset. This brings back the default LED configuration while still preventing an excessive amount of soft resets. Fixes: 6e2d85ec0559 ("net: phy: Stop with excessive soft reset") Signed-off-by: Martin Schiller <ms@dev.tdt.de> --- Changes to v2: o Fixed commit message o Fixed email recipients once again. Changes to v1: Added additional email recipients. --- drivers/net/phy/intel-xway.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)