Hello Tien Hock
On 6/9/2016 7:48 AM, Tien Hock Loh wrote:
[snip]
.../devicetree/bindings/net/socfpga-dwmac.txt | 4 +
drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 140 +++++++++--
drivers/net/ethernet/stmicro/stmmac/tse_pcs.c | 261 +++++++++++++++++++++
drivers/net/ethernet/stmicro/stmmac/tse_pcs.h | 36 +++
5 files changed, 419 insertions(+), 24 deletions(-)
create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.c
create mode 100644 drivers/net/ethernet/stmicro/stmmac/tse_pcs.h
I just wonder if it could make sense to rename the
tse_pcs.[hc] files or creating a sub-directory for altera devel.
It seems that tse_pcs.[hc] are common files but this support
is for Altera.
Anyway, I let you decide and I also ask you to update the stmmac.txt
file.
Yeah the PCS support for TSE is Altera. To avoid confusion, let's rename
them, would altr_tse_pcs.[hc] be good? I don't think creating a
sub-directory with only 2 files is necessary though.
ok for two files w/o sub-dir.
I see that stmmac.txt is generic, and other vendor's PCS support
documents their specific uses in their own *-dwmac.txt (eg.
rockchip-dwmac.txt). Is this not the case?
yes you can use this name convention. I let you decide.
[snip]
+
+ index = of_property_match_string(np_sgmii_adapter, "reg-names",
+ "eth_tse_control_port");
reg-names looks to be specific and mandatory, IMO they should be
documented in the binding.
That's the binding for the adapter (the phandle to the sgmii adapter),
not the stmac binding itself. Do you mean I should document the sgmii
adapter as well?
no I just meant for the adapter binding, I had understood that
eth_tse_control_port and gmii_to_sgmii_adapter_avalon_slave
were not documented and these seem to be mandatory.
[snip]
+
+static void auto_nego_timer_callback(unsigned long data)
+{
+ u16 val = 0;
+ u16 speed = 0;
+ u16 duplex = 0;
+
+ struct tse_pcs *pcs = (struct tse_pcs *)data;
+ void __iomem *tse_pcs_base = pcs->tse_pcs_base;
+ void __iomem *sgmii_adapter_base = pcs->sgmii_adapter_base;
+
+ val = readw(tse_pcs_base + TSE_PCS_STATUS_REG);
+ val &= TSE_PCS_STATUS_AN_COMPLETED_MASK;
[snip]
ANE is completed but speed or duplex is NOK. Any failure to signalling?
I see that you then enable the adpter in any case.
Maybe we could try to restart ANE again or force it (reducing the speed)
I wonder what happens if, for some reason, there is some hw problem. In
that case the timer is always running signalling an invalid Parter
speed. Anyway, this is jus a question... I expect that this error will
always point us to a problem to debug further (if occurs).
Let me look at how we can handle the case. Perhaps we could do a restart
and register the timer again. I'm just worried it will go into an
infinite timer registering hogging up the kernel if the hardware really
fails. Maybe I can do a n-time retry here. Looking into this. Let me
know if you have any opinions on this.
I haven't been able to check for this behaviour though, negative testing
on this code isn't too easy to simulate.
yes and I expect this can occur on hw / conf problems. Take a look at
how the Physical Abstraction Layer manages this.
Indeed, we can try to restart ANE for a while and then just report the
failure (dev_err). Or we can try to fix other speed or duplex. But not
sure this is a good solution. We just mask a problem.
[snip]
+
+ setup_timer(&pcs->an_timer,
+ auto_nego_timer_callback,
+ (unsigned long)pcs);
+ mod_timer(&pcs->an_timer, jiffies +
+ msecs_to_jiffies(AUTONEGO_TIMER));
+ } else if (phy_dev->autoneg == AUTONEG_DISABLE) {
+ val = readw(tse_pcs_base + TSE_PCS_CONTROL_REG);
+ val &= ~TSE_PCS_CONTROL_AN_EN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_CONTROL_REG);
+
+ val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
+ val &= ~TSE_PCS_USE_SGMII_AN_MASK;
+ writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
+
+ val = readw(tse_pcs_base + TSE_PCS_IF_MODE_REG);
+ val &= ~TSE_PCS_SGMII_SPEED_MASK;
+
+ switch (speed) {
+ case 1000:
+ val |= TSE_PCS_SGMII_SPEED_1000;
+ break;
+ case 100:
+ val |= TSE_PCS_SGMII_SPEED_100;
+ break;
+ case 10:
+ val |= TSE_PCS_SGMII_SPEED_10;
+ break;
+ default:
+ return;
+ }
+ writew(val, tse_pcs_base + TSE_PCS_IF_MODE_REG);
+
+ tse_pcs_reset(tse_pcs_base, pcs);
+
+ setup_timer(&pcs->link_timer,
+ pcs_link_timer_callback,
+ (unsigned long)pcs);
+ mod_timer(&pcs->link_timer, jiffies +
+ msecs_to_jiffies(LINK_TIMER));
I wonder if we can have just one timer to manage ANE and LINK.
That would increase the complexity of the code because we would need to
check the callback type on when the callback is triggered and call the
correct function.
hmm, in that case, you have two timers and no lock protection:
I suspect there could be some hidden problem. The link goes down
and a timer polls this then another one try to restart ANE and
both timers read the TSE_PCS_STATUS_REG.
IMO, a timer is enough and you could keep the code to manage ANE and
LINK in two different functions. Pls take a look at if this is feasible.
Peppe
