stmmac: GMAC_RGSMIIIS reports bogus values

stmmac: GMAC_RGSMIIIS reports bogus values

[Alexey Brodkin]

Hello,

I'm seeing pretty strange issue with GMAC reporting a lot of link state changes
based on bits in GMAC_RGSMIIIS. It looks like that:
-------------------------->8-----------------------
Link is Down
Link is Up - 10/Full
Link is Down
Link is Up - 10/Half
Link is Down
Link is Down
Link is Up - 10/Half
Link is Up -
1000/Half
Link is Down
Link is Down
Link is Down
Link is Down
Link is Up - 10/Half
Link is Down
Link is Down
Link is Up -
1000/Half
Link is Up - 1000/Full
-------------------------->8-----------------------

What's especially interesting my board with GMAC is connected to 100Mbit device
which means there's no chance for 1Gb mode to be set.

Also this has nothing to do with link state detected and reported by PHY via MDIO.
So obviously GMAC_RGSMIIIS bits are wrong. But given the fact that GMAC_RGSMIIIS bits
are set based on state of RXD[3:0] lines of RGMII I may only thing that it's
PHY (in my case DP83865) who's sending random data on the RXD during inter-frame gap.

Note data transferred through that networking connection is perfectly correct and
actually I haven't see those link state prints before kernel v4.8 basically
because the prints in question were implemented with pr_debug() and then with [1]
we got pr_info() that made prints visible by default.

Since I don't have any means to capture all required GMII signals to do better
analysis and my data is not corrupted in the end I'm thinking about way how to
mute these pretty senseless messages.

One thing I may think of we may disable checking of GMAC_RGSMIIIS if a particular
board uses MDIO for PHY setup. Something like that:
-------------------------->8-----------------------
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -337,7 +337,7 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
 
        dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
 
-       if (intr_status & PCS_RGSMIIIS_IRQ)
+       if (!priv->use_mdio && (intr_status & PCS_RGSMIIIS_IRQ))
                dwmac1000_rgsmii(ioaddr, x);
 
        return ret;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 6c85b61aaa0b..34e9de0450ba 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3356,11 +3356,13 @@ int stmmac_dvr_probe(struct device *device,
 
        stmmac_check_pcs_mode(priv);
 
+       priv->use_mdio = 0;
        if (priv->hw->pcs != STMMAC_PCS_RGMII  &&
            priv->hw->pcs != STMMAC_PCS_TBI &&
            priv->hw->pcs != STMMAC_PCS_RTBI) {
                /* MDIO bus Registration */
                ret = stmmac_mdio_register(ndev);
+               priv->use_mdio = 1;
                if (ret < 0) {
                        pr_debug("%s: MDIO bus (id: %d) registration failed",
                                 __func__, priv->plat->bus_id);
-------------------------->8-----------------------

Any thoughts on that are much appreciated!

Regards,
Alexey

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=70523e639bf8ca09b3357371c3546cee55c06351

Re: stmmac: GMAC_RGSMIIIS reports bogus values

[Giuseppe CAVALLARO]

Hello Alexey

Sorry for my late reply. In that case, I think that the stmmac
is using own RGMII/SGMII support (PCS) to dialog with the
transceiver. I think, from the HW capability register
this feature is present and the driver is using it as default.

I kindly ask you to verify if this is your setup or if you
do not want to use it. In that case, it is likely we need to
add some code in the driver.

Also I wonder if, other version of the stmmac worked on this platform
before.

Regards
Peppe

On 11/3/2016 5:17 PM, Alexey Brodkin wrote:
> Hello,
>
> I'm seeing pretty strange issue with GMAC reporting a lot of link state changes
> based on bits in GMAC_RGSMIIIS. It looks like that:
> -------------------------->8-----------------------
> Link is Down
> Link is Up - 10/Full
> Link is Down
> Link is Up - 10/Half
> Link is Down
> Link is Down
> Link is Up - 10/Half
> Link is Up -
> 1000/Half
> Link is Down
> Link is Down
> Link is Down
> Link is Down
> Link is Up - 10/Half
> Link is Down
> Link is Down
> Link is Up -
> 1000/Half
> Link is Up - 1000/Full
> -------------------------->8-----------------------
>
> What's especially interesting my board with GMAC is connected to 100Mbit device
> which means there's no chance for 1Gb mode to be set.
>
> Also this has nothing to do with link state detected and reported by PHY via MDIO.
> So obviously GMAC_RGSMIIIS bits are wrong. But given the fact that GMAC_RGSMIIIS bits
> are set based on state of RXD[3:0] lines of RGMII I may only thing that it's
> PHY (in my case DP83865) who's sending random data on the RXD during inter-frame gap.
>
> Note data transferred through that networking connection is perfectly correct and
> actually I haven't see those link state prints before kernel v4.8 basically
> because the prints in question were implemented with pr_debug() and then with [1]
> we got pr_info() that made prints visible by default.
>
> Since I don't have any means to capture all required GMII signals to do better
> analysis and my data is not corrupted in the end I'm thinking about way how to
> mute these pretty senseless messages.
>
> One thing I may think of we may disable checking of GMAC_RGSMIIIS if a particular
> board uses MDIO for PHY setup. Something like that:
> -------------------------->8-----------------------
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> @@ -337,7 +337,7 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
>
>         dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
>
> -       if (intr_status & PCS_RGSMIIIS_IRQ)
> +       if (!priv->use_mdio && (intr_status & PCS_RGSMIIIS_IRQ))
>                 dwmac1000_rgsmii(ioaddr, x);
>
>         return ret;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6c85b61aaa0b..34e9de0450ba 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3356,11 +3356,13 @@ int stmmac_dvr_probe(struct device *device,
>
>         stmmac_check_pcs_mode(priv);
>
> +       priv->use_mdio = 0;
>         if (priv->hw->pcs != STMMAC_PCS_RGMII  &&
>             priv->hw->pcs != STMMAC_PCS_TBI &&
>             priv->hw->pcs != STMMAC_PCS_RTBI) {
>                 /* MDIO bus Registration */
>                 ret = stmmac_mdio_register(ndev);
> +               priv->use_mdio = 1;
>                 if (ret < 0) {
>                         pr_debug("%s: MDIO bus (id: %d) registration failed",
>                                  __func__, priv->plat->bus_id);
> -------------------------->8-----------------------
>
> Any thoughts on that are much appreciated!
>
> Regards,
> Alexey
>
> [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=70523e639bf8ca09b3357371c3546cee55c06351
>

Re: stmmac: GMAC_RGSMIIIS reports bogus values

[Alexey Brodkin]

Hi Giuseppe,

On Mon, 2016-11-14 at 09:14 +0100, Giuseppe CAVALLARO wrote:
> Hello Alexey
> 
> Sorry for my late reply. In that case, I think that the stmmac
> is using own RGMII/SGMII support (PCS) to dialog with the
> transceiver. I think, from the HW capability register
> this feature is present and the driver is using it as default.

Yep looks like that.

> I kindly ask you to verify if this is your setup or if you
> do not want to use it. In that case, it is likely we need to
> add some code in the driver.

Well GMAC's databook says:
--------------------------->8--------------------------
The DWC_gmac provides an IEEE 802.3z compliant 10-bit
Physical Coding Sublayer (PCS) interface that you can use
when the MAC is configured for the TBI, RTBI, or SGMII PHY
interface.

...

You can include the optional PCS module in the DWC_gmac by
selecting the TBI, RTBI, or SGMII interface in coreConsultant
during configuration.
--------------------------->8--------------------------

But we use RGMII for communication with the PHY.
And from the quote above I would conclude that for RGMII we should
have PCS excluded from GMAC.

I just sent email to GMAC developers here in Synopsys and
hopefully will get some clarifications from them soon.

Probably correct solution is as simple as:
--------------------------->8--------------------------
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a276a32d57f2..f4b67dc7d530 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -803,13 +803,7 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
        int interface = priv->plat->interface;
 
        if (priv->dma_cap.pcs) {
-               if ((interface == PHY_INTERFACE_MODE_RGMII) ||
-                   (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
-                   (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
-                   (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
-                       netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
-                       priv->hw->pcs = STMMAC_PCS_RGMII;
-               } else if (interface == PHY_INTERFACE_MODE_SGMII) {
+               if (interface == PHY_INTERFACE_MODE_SGMII) {
                        netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
                        priv->hw->pcs = STMMAC_PCS_SGMII;
                }
--------------------------->8--------------------------

> Also I wonder if, other version of the stmmac worked on this platform
> before.

It did work and still works. The only problem is we're getting
a lot of noise now about bogus link status change. That's because
this info is now in pr_info() compared to being previously in pr_debug().

-Alexey

Re: stmmac: GMAC_RGSMIIIS reports bogus values

[Vineet Gupta]

On 01/25/2017 10:39 AM, Alexey Brodkin wrote:
>> Also I wonder if, other version of the stmmac worked on this platform
>> before.
> It did work and still works. The only problem is we're getting
> a lot of noise now about bogus link status change. That's because
> this info is now in pr_info() compared to being previously in pr_debug().

While we sort out the real technical details, it is OK to go back to pr_debug
please - print_once or some such !
There is lot of useless console noise when we run any networking loads here !

echo 0 >  /proc/sys/kernel/printk

only helps in that it is not printed on console but clobbers dmesg nonetheless !

Thx,
-Vineet

Re: stmmac: GMAC_RGSMIIIS reports bogus values

[Alexey Brodkin]

Hi Giuseppe,

On Wed, 2017-01-25 at 21:39 +0300, Alexey Brodkin wrote:
> Hi Giuseppe,
> 
> On Mon, 2016-11-14 at 09:14 +0100, Giuseppe CAVALLARO wrote:
> > 
> > Hello Alexey
> > 
> > Sorry for my late reply. In that case, I think that the stmmac
> > is using own RGMII/SGMII support (PCS) to dialog with the
> > transceiver. I think, from the HW capability register
> > this feature is present and the driver is using it as default.
> 
> Yep looks like that.

Hm, so I took a look at what am I reading from
"Register 22 (HW Feature Register)" in dwmac1000_get_hw_feature()
and was really surprised to see the register value = 0x100509bf.

See bit 6 "PCSSEL" is zeroed which stands for
"PCS registers (TBI, SGMII, or RTBI PHY interface)".

I.e. PCS doesn't exist in our GMAC so most of the previous comments
below should be discarded.

> > I kindly ask you to verify if this is your setup or if you
> > do not want to use it. In that case, it is likely we need to
> > add some code in the driver.
> 
> Well GMAC's databook says:
> --------------------------->8--------------------------
> The DWC_gmac provides an IEEE 802.3z compliant 10-bit
> Physical Coding Sublayer (PCS) interface that you can use
> when the MAC is configured for the TBI, RTBI, or SGMII PHY
> interface.
> 
> ...
> 
> You can include the optional PCS module in the DWC_gmac by
> selecting the TBI, RTBI, or SGMII interface in coreConsultant
> during configuration.
> --------------------------->8--------------------------
> 
> But we use RGMII for communication with the PHY.
> And from the quote above I would conclude that for RGMII we should
> have PCS excluded from GMAC.
> 
> I just sent email to GMAC developers here in Synopsys and
> hopefully will get some clarifications from them soon.
> 
> Probably correct solution is as simple as:
> --------------------------->8--------------------------
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a276a32d57f2..f4b67dc7d530 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -803,13 +803,7 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
>         int interface = priv->plat->interface;
>  
>         if (priv->dma_cap.pcs) {
> -               if ((interface == PHY_INTERFACE_MODE_RGMII) ||
> -                   (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
> -                   (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
> -                   (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
> -                       netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
> -                       priv->hw->pcs = STMMAC_PCS_RGMII;
> -               } else if (interface == PHY_INTERFACE_MODE_SGMII) {
> +               if (interface == PHY_INTERFACE_MODE_SGMII) {
>                         netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
>                         priv->hw->pcs = STMMAC_PCS_SGMII;
>                 }
> --------------------------->8--------------------------
> 
> > 
> > Also I wonder if, other version of the stmmac worked on this platform
> > before.
> 
> It did work and still works. The only problem is we're getting
> a lot of noise now about bogus link status change. That's because
> this info is now in pr_info() compared to being previously in pr_debug().

So it all really boils down to the following problem:
Link state reported via "Register 54 (SGMII/RGMII/SMII Control and Status Register)"
doesn't match status read via MDIO from the PHY.

That's why my initial proposal was to ignore whatever we read from this register
if we have MDIO bus instantiated already.

-Alexey

_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: stmmac: GMAC_RGSMIIIS reports bogus values

[Giuseppe CAVALLARO]

On 1/27/2017 11:23 AM, Alexey Brodkin wrote:
> That's why my initial proposal was to ignore whatever we read from this register
> if we have MDIO bus instantiated already.

sorry for my late reply, I agree with this approach, according to the
HW and platform configuration the driver has to understand and then
work to use either SMA or PCS module.

Regards
Peppe

Re: stmmac: GMAC_RGSMIIIS reports bogus values

[Alexey Brodkin]

Hi Giuseppe,

On Tue, 2017-01-31 at 10:55 +0100, Giuseppe CAVALLARO wrote:
> On 1/27/2017 11:23 AM, Alexey Brodkin wrote:
> > 
> > That's why my initial proposal was to ignore whatever we read from this register
> > if we have MDIO bus instantiated already.
> 
> sorry for my late reply, I agree with this approach, according to the
> HW and platform configuration the driver has to understand and then
> work to use either SMA or PCS module.

I already submitted another solution which IMHO is much cleaner and appropriate,
see David has it already in master tree here:
http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=0a764db103376cf69d04449b10688f3516cc0b88

-Alexey
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: stmmac: GMAC_RGSMIIIS reports bogus values

[Giuseppe CAVALLARO]

Hello Alexey

On 1/31/2017 2:24 PM, Alexey Brodkin wrote:
> Hi Giuseppe,
>
> On Tue, 2017-01-31 at 10:55 +0100, Giuseppe CAVALLARO wrote:
>> On 1/27/2017 11:23 AM, Alexey Brodkin wrote:
>>>
>>> That's why my initial proposal was to ignore whatever we read from this register
>>> if we have MDIO bus instantiated already.
>>
>> sorry for my late reply, I agree with this approach, according to the
>> HW and platform configuration the driver has to understand and then
>> work to use either SMA or PCS module.
>
> I already submitted another solution which IMHO is much cleaner and appropriate,
> see David has it already in master tree here:
> http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=0a764db103376cf69d04449b10688f3516cc0b88
>

it's ok, thx for that

Peppe

> -Alexey
>