| Message ID | 1513119114-17511-1-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: thunderx: add support for rgmii internal delay | expand |
> +void xcv_init_hw(int phy_mode) > { > u64 cfg; > > @@ -81,12 +81,31 @@ void xcv_init_hw(void) > /* Wait for DLL to lock */ > msleep(1); > > - /* Configure DLL - enable or bypass > - * TX no bypass, RX bypass > - */ > + /* enable/bypass DLL providing MAC based internal TX/RX delays */ > cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL); > - cfg &= ~0xFF03; > - cfg |= CLKRX_BYP; > + cfg &= ~0xffff00; > + switch (phy_mode) { > + /* RX and TX delays are added by the MAC */ > + case PHY_INTERFACE_MODE_RGMII: > + break; > + /* internal RX and TX delays provided by the PHY */ > + case PHY_INTERFACE_MODE_RGMII_ID: > + cfg |= CLKRX_BYP; > + cfg |= CLKTX_BYP; > + break; > + /* internal RX delay provided by the PHY, the MAC > + * should not add an RX delay in this case > + */ > + case PHY_INTERFACE_MODE_RGMII_RXID: > + cfg |= CLKRX_BYP; > + break; > + /* internal TX delay provided by the PHY, the MAC > + * should not add an TX delay in this case > + */ > + case PHY_INTERFACE_MODE_RGMII_TXID: > + cfg |= CLKRX_BYP; > + break; > + } Hi Tim This i don't get. Normally, you leave the PHY to handle delays, if needed. The MAC should not add any. Here you seem to assume a delay is always needed, and if the PHY is not providing it, the MAC should. Andrew
On Wed, Dec 13, 2017 at 3:10 AM, Andrew Lunn <andrew@lunn.ch> wrote: >> +void xcv_init_hw(int phy_mode) >> { >> u64 cfg; >> >> @@ -81,12 +81,31 @@ void xcv_init_hw(void) >> /* Wait for DLL to lock */ >> msleep(1); >> >> - /* Configure DLL - enable or bypass >> - * TX no bypass, RX bypass >> - */ >> + /* enable/bypass DLL providing MAC based internal TX/RX delays */ >> cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL); >> - cfg &= ~0xFF03; >> - cfg |= CLKRX_BYP; >> + cfg &= ~0xffff00; >> + switch (phy_mode) { >> + /* RX and TX delays are added by the MAC */ >> + case PHY_INTERFACE_MODE_RGMII: >> + break; >> + /* internal RX and TX delays provided by the PHY */ >> + case PHY_INTERFACE_MODE_RGMII_ID: >> + cfg |= CLKRX_BYP; >> + cfg |= CLKTX_BYP; >> + break; >> + /* internal RX delay provided by the PHY, the MAC >> + * should not add an RX delay in this case >> + */ >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + cfg |= CLKRX_BYP; >> + break; >> + /* internal TX delay provided by the PHY, the MAC >> + * should not add an TX delay in this case >> + */ >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> + cfg |= CLKRX_BYP; >> + break; >> + } > > Hi Tim > > This i don't get. Normally, you leave the PHY to handle delays, if > needed. The MAC should not add any. Here you seem to assume a delay is > always needed, and if the PHY is not providing it, the MAC should. > > Andrew Andrew, The thunder RGX inserts a delay via an on-board DLL. The 'bypass' register will bypass this DLL and not insert a delay from the MAC side. By default out of reset CLKTX_BYP=1 causing the RGX transmit interface to not introduce a delay and CLKRX_BYP=0 causing the RGX receive interface to introduce a delay. The current code assumes the opposite setting CLKRX_BYP and clearing CLKTX_BYP such that the RGX interface introduces a TX delay but not RX which would be appropriate for PHY_INTERFACE_MODE_RGMII_RXID/rgmii-txid (right, or is my logic there backwards?). I may have my commit msg wrong in this case. At any rate, I've got a board where the phy provides both TX/RX delay and thus I don't want the RGX to insert any delays which means I need to set both CLKRX_BYP and CLKTX_BYP which the driver currently doesn't support. Tim
On Wed, Dec 13, 2017 at 03:28:33PM -0800, Tim Harvey wrote: > On Wed, Dec 13, 2017 at 3:10 AM, Andrew Lunn <andrew@lunn.ch> wrote: > >> +void xcv_init_hw(int phy_mode) > >> { > >> u64 cfg; > >> > >> @@ -81,12 +81,31 @@ void xcv_init_hw(void) > >> /* Wait for DLL to lock */ > >> msleep(1); > >> > >> - /* Configure DLL - enable or bypass > >> - * TX no bypass, RX bypass > >> - */ > >> + /* enable/bypass DLL providing MAC based internal TX/RX delays */ > >> cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL); > >> - cfg &= ~0xFF03; > >> - cfg |= CLKRX_BYP; > >> + cfg &= ~0xffff00; > >> + switch (phy_mode) { > >> + /* RX and TX delays are added by the MAC */ > >> + case PHY_INTERFACE_MODE_RGMII: > >> + break; > >> + /* internal RX and TX delays provided by the PHY */ > >> + case PHY_INTERFACE_MODE_RGMII_ID: > >> + cfg |= CLKRX_BYP; > >> + cfg |= CLKTX_BYP; > >> + break; > >> + /* internal RX delay provided by the PHY, the MAC > >> + * should not add an RX delay in this case > >> + */ > >> + case PHY_INTERFACE_MODE_RGMII_RXID: > >> + cfg |= CLKRX_BYP; > >> + break; > >> + /* internal TX delay provided by the PHY, the MAC > >> + * should not add an TX delay in this case > >> + */ > >> + case PHY_INTERFACE_MODE_RGMII_TXID: > >> + cfg |= CLKRX_BYP; > >> + break; > >> + } > > > > Hi Tim > > > > This i don't get. Normally, you leave the PHY to handle delays, if > > needed. The MAC should not add any. Here you seem to assume a delay is > > always needed, and if the PHY is not providing it, the MAC should. > > > > Andrew > > Andrew, > > The thunder RGX inserts a delay via an on-board DLL. The 'bypass' > register will bypass this DLL and not insert a delay from the MAC > side. By default out of reset CLKTX_BYP=1 causing the RGX transmit > interface to not introduce a delay and CLKRX_BYP=0 causing the RGX > receive interface to introduce a delay. Hi Tim So the MAC by default is doing PHY_INTERFACE_MODE_RGMII_RXID. And it calls phy_connect_direct() passing PHY_INTERFACE_MODE_RGMII. It does not get anything from device tree. So it looks like we have a chance to clean this up. So the correct thing to do is set the MAC to PHY_INTERFACE_MODE_RGMII, i.e. no delays. By default call phy_connect_direct() PHY_INTERFACE_MODE_RGMII_RXID. That should give you the same behaviour as today. Then add code to look in device tree, to find a per board setting. In your case, you want PHY_INTERFACE_MODE_RGMII_ID. And make sure the PHY driver respects the value passed. Andrew
On Thu, Dec 14, 2017 at 12:45 AM, Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, Dec 13, 2017 at 03:28:33PM -0800, Tim Harvey wrote: >> On Wed, Dec 13, 2017 at 3:10 AM, Andrew Lunn <andrew@lunn.ch> wrote: >> >> +void xcv_init_hw(int phy_mode) >> >> { >> >> u64 cfg; >> >> >> >> @@ -81,12 +81,31 @@ void xcv_init_hw(void) >> >> /* Wait for DLL to lock */ >> >> msleep(1); >> >> >> >> - /* Configure DLL - enable or bypass >> >> - * TX no bypass, RX bypass >> >> - */ >> >> + /* enable/bypass DLL providing MAC based internal TX/RX delays */ >> >> cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL); >> >> - cfg &= ~0xFF03; >> >> - cfg |= CLKRX_BYP; >> >> + cfg &= ~0xffff00; >> >> + switch (phy_mode) { >> >> + /* RX and TX delays are added by the MAC */ >> >> + case PHY_INTERFACE_MODE_RGMII: >> >> + break; >> >> + /* internal RX and TX delays provided by the PHY */ >> >> + case PHY_INTERFACE_MODE_RGMII_ID: >> >> + cfg |= CLKRX_BYP; >> >> + cfg |= CLKTX_BYP; >> >> + break; >> >> + /* internal RX delay provided by the PHY, the MAC >> >> + * should not add an RX delay in this case >> >> + */ >> >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> >> + cfg |= CLKRX_BYP; >> >> + break; >> >> + /* internal TX delay provided by the PHY, the MAC >> >> + * should not add an TX delay in this case >> >> + */ >> >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> >> + cfg |= CLKRX_BYP; >> >> + break; >> >> + } >> > >> > Hi Tim >> > >> > This i don't get. Normally, you leave the PHY to handle delays, if >> > needed. The MAC should not add any. Here you seem to assume a delay is >> > always needed, and if the PHY is not providing it, the MAC should. >> > >> > Andrew >> >> Andrew, >> >> The thunder RGX inserts a delay via an on-board DLL. The 'bypass' >> register will bypass this DLL and not insert a delay from the MAC >> side. By default out of reset CLKTX_BYP=1 causing the RGX transmit >> interface to not introduce a delay and CLKRX_BYP=0 causing the RGX >> receive interface to introduce a delay. > > Hi Tim > > So the MAC by default is doing PHY_INTERFACE_MODE_RGMII_RXID. And it > calls phy_connect_direct() passing PHY_INTERFACE_MODE_RGMII. It does > not get anything from device tree. So it looks like we have a chance > to clean this up. > > So the correct thing to do is set the MAC to PHY_INTERFACE_MODE_RGMII, > i.e. no delays. By default call phy_connect_direct() > PHY_INTERFACE_MODE_RGMII_RXID. That should give you the same behaviour > as today. I don't understand - PHY_INTERFACE_MODE_RGMII means delays are added by the MAC The way I see it today the driver is making an assumption that is not always correct. What is the right way to configure a MAC when no phy-mode is present in the dts? I assumed it would be RGMII_ID such that the MAC introduces no delay. > > Then add code to look in device tree, to find a per board setting. In > your case, you want PHY_INTERFACE_MODE_RGMII_ID. And make sure the PHY > driver respects the value passed. > > Andrew > Should I be attempting to make the default if no phy-mode is in the dts be PHY_INTERFACE_MODE_RGMII_RXID so that existing boards do not break (as I assume they configure the phy's that way in firmware). My original goal was to make the bgx driver flexible for different delay configurations as well as allow phy drivers to be used. However I found that the dp83867 driver doesn't work with my board anyway as it issues a soft reset that disables CLKOUT which I setup in firmware and require. Is it standard for phy drivers to issue hard or soft resets during init and if so how do boards deal with custom LED or CLKOUT configs as those don't seem to be supported by phy drivers? I only have experience with phy drivers that support an optional hard reset and if you don't want to reset any custom regs you simply don't expose the phy_rst gpio (assuming there is on) to the driver by not defining it in device-tree. Tim
diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c index 5e5c4d7..805c02a 100644 --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c @@ -55,6 +55,7 @@ struct bgx { struct pci_dev *pdev; bool is_dlm; bool is_rgx; + int phy_mode; }; static struct bgx *bgx_vnic[MAX_BGX_THUNDER]; @@ -841,12 +842,12 @@ static void bgx_poll_for_link(struct work_struct *work) queue_delayed_work(lmac->check_link, &lmac->dwork, HZ * 2); } -static int phy_interface_mode(u8 lmac_type) +static int phy_interface_mode(struct bgx *bgx, u8 lmac_type) { if (lmac_type == BGX_MODE_QSGMII) return PHY_INTERFACE_MODE_QSGMII; if (lmac_type == BGX_MODE_RGMII) - return PHY_INTERFACE_MODE_RGMII; + return bgx->phy_mode; return PHY_INTERFACE_MODE_SGMII; } @@ -912,7 +913,8 @@ static int bgx_lmac_enable(struct bgx *bgx, u8 lmacid) if (phy_connect_direct(&lmac->netdev, lmac->phydev, bgx_lmac_handler, - phy_interface_mode(lmac->lmac_type))) + phy_interface_mode(bgx, + lmac->lmac_type))) return -ENODEV; phy_start_aneg(lmac->phydev); @@ -1287,6 +1289,8 @@ static int bgx_init_of_phy(struct bgx *bgx) bgx->lmac[lmac].lmacid = lmac; phy_np = of_parse_phandle(node, "phy-handle", 0); + if (phy_np) + bgx->phy_mode = of_get_phy_mode(phy_np); /* If there is no phy or defective firmware presents * this cortina phy, for which there is no driver * support, ignore it. @@ -1390,7 +1394,6 @@ static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent) bgx->max_lmac = 1; bgx->bgx_id = MAX_BGX_PER_CN81XX - 1; bgx_vnic[bgx->bgx_id] = bgx; - xcv_init_hw(); } /* On 81xx all are DLMs and on 83xx there are 3 BGX QLMs and one @@ -1407,6 +1410,8 @@ static int bgx_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (err) goto err_enable; + if (bgx->is_rgx) + xcv_init_hw(bgx->phy_mode); bgx_init_hw(bgx); /* Enable all LMACs */ diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.h b/drivers/net/ethernet/cavium/thunder/thunder_bgx.h index 23acdc5..2bba9d1 100644 --- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.h +++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.h @@ -226,7 +226,7 @@ void bgx_lmac_internal_loopback(int node, int bgx_idx, void bgx_lmac_get_pfc(int node, int bgx_idx, int lmacid, void *pause); void bgx_lmac_set_pfc(int node, int bgx_idx, int lmacid, void *pause); -void xcv_init_hw(void); +void xcv_init_hw(int phy_mode); void xcv_setup_link(bool link_up, int link_speed); u64 bgx_get_rx_stats(int node, int bgx_idx, int lmac, int idx); diff --git a/drivers/net/ethernet/cavium/thunder/thunder_xcv.c b/drivers/net/ethernet/cavium/thunder/thunder_xcv.c index 578c7f8..7e0c4cb 100644 --- a/drivers/net/ethernet/cavium/thunder/thunder_xcv.c +++ b/drivers/net/ethernet/cavium/thunder/thunder_xcv.c @@ -65,7 +65,7 @@ MODULE_LICENSE("GPL v2"); MODULE_VERSION(DRV_VERSION); MODULE_DEVICE_TABLE(pci, xcv_id_table); -void xcv_init_hw(void) +void xcv_init_hw(int phy_mode) { u64 cfg; @@ -81,12 +81,31 @@ void xcv_init_hw(void) /* Wait for DLL to lock */ msleep(1); - /* Configure DLL - enable or bypass - * TX no bypass, RX bypass - */ + /* enable/bypass DLL providing MAC based internal TX/RX delays */ cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL); - cfg &= ~0xFF03; - cfg |= CLKRX_BYP; + cfg &= ~0xffff00; + switch (phy_mode) { + /* RX and TX delays are added by the MAC */ + case PHY_INTERFACE_MODE_RGMII: + break; + /* internal RX and TX delays provided by the PHY */ + case PHY_INTERFACE_MODE_RGMII_ID: + cfg |= CLKRX_BYP; + cfg |= CLKTX_BYP; + break; + /* internal RX delay provided by the PHY, the MAC + * should not add an RX delay in this case + */ + case PHY_INTERFACE_MODE_RGMII_RXID: + cfg |= CLKRX_BYP; + break; + /* internal TX delay provided by the PHY, the MAC + * should not add an TX delay in this case + */ + case PHY_INTERFACE_MODE_RGMII_TXID: + cfg |= CLKRX_BYP; + break; + } writeq_relaxed(cfg, xcv->reg_base + XCV_DLL_CTL); /* Enable compensation controller and force the
The XCV_DLL_CTL is being configured with the assumption that phy-mode is rgmii-txid (PHY_INTERFACE_MODE_RGMII_TXID) which is not always the case. This patch parses the phy-mode property and uses it to configure CXV_DLL_CTL properly. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/net/ethernet/cavium/thunder/thunder_bgx.c | 13 +++++++--- drivers/net/ethernet/cavium/thunder/thunder_bgx.h | 2 +- drivers/net/ethernet/cavium/thunder/thunder_xcv.c | 31 ++++++++++++++++++----- 3 files changed, 35 insertions(+), 11 deletions(-)