Message ID | 6d4c6d15a60c93a8aef5e3e03b9cd64cdcf232c8.1482232420.git.jpinto@synopsys.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Joao Pinto <Joao.Pinto@synopsys.com> Date: Tue, 20 Dec 2016 11:21:47 +0000 > When testing stmmac with my QoS reference design I checked a problem in the > CSR clock configuration that was impossibilitating the phy discovery, since > every read operation returned 0x0000ffff. This patch fixes the issue. > > Signed-off-by: Joao Pinto <jpinto@synopsys.com> This isn't enough. It looks like various parts of this driver set the mask field differently. dwmac1000_core.c and dwmac100_core.c set the mask to be the low bits. But dwmac4_core.c uses GENMASK(11, 8) which means the mask is a value which is shifted up already. So your patch will break chips driven by dwmac4_core.c. In order for your change to be correct you must consolidate all of these various pieces to use the same convention.
Às 6:21 PM de 12/21/2016, David Miller escreveu: > From: Joao Pinto <Joao.Pinto@synopsys.com> > Date: Tue, 20 Dec 2016 11:21:47 +0000 > >> When testing stmmac with my QoS reference design I checked a problem in the >> CSR clock configuration that was impossibilitating the phy discovery, since >> every read operation returned 0x0000ffff. This patch fixes the issue. >> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com> > > This isn't enough. > > It looks like various parts of this driver set the mask field > differently. > > dwmac1000_core.c and dwmac100_core.c set the mask to be the low bits. > > But dwmac4_core.c uses GENMASK(11, 8) which means the mask is a value > which is shifted up already. > > So your patch will break chips driven by dwmac4_core.c. I am using a GMAC4 reference design to test the patches. The clock configuration as is, does not work, resulting in the phy discovery failure. By applying this patch I am able to set the clock value properly. I am going to check in the Databook of GMAC4 and older versions in order to justify better. > > In order for your change to be correct you must consolidate all of > these various pieces to use the same convention. > Thanks.
Hi David, Às 10:15 AM de 12/22/2016, Joao Pinto escreveu: > Às 6:21 PM de 12/21/2016, David Miller escreveu: >> From: Joao Pinto <Joao.Pinto@synopsys.com> >> Date: Tue, 20 Dec 2016 11:21:47 +0000 >> >>> When testing stmmac with my QoS reference design I checked a problem in the >>> CSR clock configuration that was impossibilitating the phy discovery, since >>> every read operation returned 0x0000ffff. This patch fixes the issue. >>> >>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >> >> This isn't enough. >> >> It looks like various parts of this driver set the mask field >> differently. >> >> dwmac1000_core.c and dwmac100_core.c set the mask to be the low bits. >> >> But dwmac4_core.c uses GENMASK(11, 8) which means the mask is a value >> which is shifted up already. >> >> So your patch will break chips driven by dwmac4_core.c. > > I am using a GMAC4 reference design to test the patches. The clock configuration > as is, does not work, resulting in the phy discovery failure. By applying this > patch I am able to set the clock value properly. > > I am going to check in the Databook of GMAC4 and older versions in order to > justify better. So from the 4.20 Databook: Bits 11:8 CR parameter 0000: CSR clock = 60-100 MHz; MDC clock = CSR 0001: CSR clock = 100-150 MHz; MDC clock = CSR 0010: CSR clock = 20-35 MHz; MDC clock = CSR 0011: CSR clock = 35-60 MHz; MDC clock = CSR 0100: CSR clock = 150-250 MHz; MDC clock = CSR 0101: CSR clock = 250-300 MHz; MDC clock = CSR 0110, 0111: Reserved For example, if you want configure the CSR clock to 250-300MHZ (my case), you will set the parameter CR to 0x5. The current mechanism simply messes the value. With this fix, the 0x5 is shifted to 11:8 like the databook specifies and applies a GENMASK(11:8) on top, causing the reset of every bit outside the 11:8 which is an assurance. For older cores like 4.10 and 4.00 the logic is the same. The information was confirmed by R&D. Thanks. > >> >> In order for your change to be correct you must consolidate all of >> these various pieces to use the same convention. >> > > Thanks. >
Às 12:23 PM de 12/22/2016, Joao Pinto escreveu: > > Hi David, > > Às 10:15 AM de 12/22/2016, Joao Pinto escreveu: >> Às 6:21 PM de 12/21/2016, David Miller escreveu: >>> From: Joao Pinto <Joao.Pinto@synopsys.com> >>> Date: Tue, 20 Dec 2016 11:21:47 +0000 >>> >>>> When testing stmmac with my QoS reference design I checked a problem in the >>>> CSR clock configuration that was impossibilitating the phy discovery, since >>>> every read operation returned 0x0000ffff. This patch fixes the issue. >>>> >>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >>> >>> This isn't enough. >>> >>> It looks like various parts of this driver set the mask field >>> differently. >>> >>> dwmac1000_core.c and dwmac100_core.c set the mask to be the low bits. >>> >>> But dwmac4_core.c uses GENMASK(11, 8) which means the mask is a value >>> which is shifted up already. >>> >>> So your patch will break chips driven by dwmac4_core.c. >> >> I am using a GMAC4 reference design to test the patches. The clock configuration >> as is, does not work, resulting in the phy discovery failure. By applying this >> patch I am able to set the clock value properly. >> >> I am going to check in the Databook of GMAC4 and older versions in order to >> justify better. > > So from the 4.20 Databook: > > Bits > 11:8 CR parameter > 0000: CSR clock = 60-100 MHz; MDC clock = CSR > 0001: CSR clock = 100-150 MHz; MDC clock = CSR > 0010: CSR clock = 20-35 MHz; MDC clock = CSR > 0011: CSR clock = 35-60 MHz; MDC clock = CSR > 0100: CSR clock = 150-250 MHz; MDC clock = CSR > 0101: CSR clock = 250-300 MHz; MDC clock = CSR > 0110, 0111: Reserved > > For example, if you want configure the CSR clock to 250-300MHZ (my case), you > will set the parameter CR to 0x5. The current mechanism simply messes the value. > With this fix, the 0x5 is shifted to 11:8 like the databook specifies and > applies a GENMASK(11:8) on top, causing the reset of every bit outside the 11:8 > which is an assurance. > > For older cores like 4.10 and 4.00 the logic is the same. The information was > confirmed by R&D. > > Thanks. Bu checking DWMAC100 and DWMAC1000 I understand your concern now. I am going to change their masks also in order to put it right. V2 comming soon. Thanks. > >> >>> >>> In order for your change to be correct you must consolidate all of >>> these various pieces to use the same convention. >>> >> >> Thanks. >> >
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c index 23322fd..fda01f7 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c @@ -81,8 +81,8 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg) value |= (phyaddr << priv->hw->mii.addr_shift) & priv->hw->mii.addr_mask; value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw->mii.reg_mask; - value |= (priv->clk_csr & priv->hw->mii.clk_csr_mask) - << priv->hw->mii.clk_csr_shift; + value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift) + & priv->hw->mii.clk_csr_mask; if (priv->plat->has_gmac4) value |= MII_GMAC4_READ; @@ -122,8 +122,8 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg, & priv->hw->mii.addr_mask; value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw->mii.reg_mask; - value |= ((priv->clk_csr & priv->hw->mii.clk_csr_mask) - << priv->hw->mii.clk_csr_shift); + value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift) + & priv->hw->mii.clk_csr_mask; if (priv->plat->has_gmac4) value |= MII_GMAC4_WRITE;
When testing stmmac with my QoS reference design I checked a problem in the CSR clock configuration that was impossibilitating the phy discovery, since every read operation returned 0x0000ffff. This patch fixes the issue. Signed-off-by: Joao Pinto <jpinto@synopsys.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)