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 <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
Acked-by: Martin Blumenstingl <[email protected]>
On Tue, Apr 20, 2021 at 10:51 PM Martin Schiller <[email protected]> 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 <[email protected]>
> ---
>
> 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
On Wed, Feb 2, 2022 at 5:01 PM Andrew Lunn <[email protected]> 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 Thu, Feb 3, 2022 at 8:37 AM Andrew Lunn <[email protected]> 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
> 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 Wed, Feb 2, 2022 at 7:12 PM Florian Fainelli <[email protected]> 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
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).
--
Florian
> 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 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 2/3/2022 8:02 AM, Tim Harvey wrote:
> On Wed, Feb 2, 2022 at 7:12 PM Florian Fainelli <[email protected]> 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/[email protected]/
--
Florian
On Fri, Feb 4, 2022 at 2:54 PM Andrew Lunn <[email protected]> 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 <[email protected]> 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 <[email protected]> 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