mac_prepare is called every time the interface is changed, so we can do
all of our configuration there, instead of in mac_config. This will be
useful for the next patch where we will set the PCS bit based on whether
we are using our internal PCS. No functional change intended.
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
(no changes since v2)
Changes in v2:
- Fix docs for macb_pcs_config_an
- Include change to macb_pcs_get_state which was previously in the next
patch
drivers/net/ethernet/cadence/macb_main.c | 209 ++++++++++++++---------
1 file changed, 132 insertions(+), 77 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 1fe8ec37491b..ed37b1d85212 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -549,19 +549,91 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
netdev_err(bp->dev, "adjusting tx_clk failed.\n");
}
-static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
- phy_interface_t interface, int speed,
- int duplex)
-{
- struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
- u32 config;
-
- config = gem_readl(bp, USX_CONTROL);
- config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config);
- config = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, config);
- config &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
- config |= GEM_BIT(TX_EN);
- gem_writel(bp, USX_CONTROL, config);
+static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
+ struct phylink_link_state *state)
+{
+ struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
+
+ phylink_mii_c22_pcs_decode_state(state, neg_mode, gem_readl(bp, PCSSTS),
+ gem_readl(bp, PCSANLPBASE));
+}
+
+/**
+ * macb_pcs_config_an() - Configure autonegotiation settings for PCSs
+ * @bp: The macb to operate on
+ * @neg_mode: The autonegotiation mode
+ * @interface: The interface to use
+ * @advertising: The advertisement mask
+ *
+ * This provides common configuration for PCS autonegotiation.
+ *
+ * Context: Call with @bp->lock held.
+ * Return: 1 if any registers were changed; 0 otherwise
+ */
+static int macb_pcs_config_an(struct macb *bp, unsigned int neg_mode,
+ phy_interface_t interface,
+ const unsigned long *advertising)
+{
+ bool changed = false;
+ int old, new;
+
+ old = gem_readl(bp, PCSANADV);
+ new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
+ if (new != -EINVAL && old != new) {
+ changed = true;
+ gem_writel(bp, PCSANADV, new);
+ }
+
+ old = new = gem_readl(bp, PCSCNTRL);
+ if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+ new |= BMCR_ANENABLE;
+ else
+ new &= ~BMCR_ANENABLE;
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, PCSCNTRL, new);
+ }
+ return changed;
+}
+
+static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+ phy_interface_t interface,
+ const unsigned long *advertising,
+ bool permit_pause_to_mac)
+{
+ struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
+ bool changed = false;
+ unsigned long flags;
+ u32 old, new;
+
+ spin_lock_irqsave(&bp->lock, flags);
+ old = new = gem_readl(bp, NCFGR);
+ new |= GEM_BIT(SGMIIEN);
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, NCFGR, new);
+ }
+
+ if (macb_pcs_config_an(bp, mode, interface, advertising))
+ changed = true;
+
+ spin_unlock_irqrestore(&bp->lock, flags);
+ return changed;
+}
+
+static void macb_pcs_an_restart(struct phylink_pcs *pcs)
+{
+ struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
+ u32 bmcr;
+ unsigned long flags;
+
+ spin_lock_irqsave(&bp->lock, flags);
+
+ bmcr = gem_readl(bp, PCSCNTRL);
+ bmcr |= BMCR_ANENABLE;
+ gem_writel(bp, PCSCNTRL, bmcr);
+
+ spin_lock_irqsave(&bp->lock, flags);
}
static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
@@ -589,45 +661,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
bool permit_pause_to_mac)
{
struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
+ unsigned long flags;
+ bool changed;
+ u16 old, new;
- gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
- GEM_BIT(SIGNAL_OK));
+ spin_lock_irqsave(&bp->lock, flags);
+ if (macb_pcs_config_an(bp, neg_mode, interface, advertising))
+ changed = true;
- return 0;
-}
+ old = new = gem_readl(bp, USX_CONTROL);
+ new |= GEM_BIT(SIGNAL_OK);
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, USX_CONTROL, new);
+ }
-static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
- struct phylink_link_state *state)
-{
- state->link = 0;
-}
+ old = new = gem_readl(bp, USX_CONTROL);
+ new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
+ new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
+ new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
+ new |= GEM_BIT(TX_EN);
+ if (old != new) {
+ changed = true;
+ gem_writel(bp, USX_CONTROL, new);
+ }
-static void macb_pcs_an_restart(struct phylink_pcs *pcs)
-{
- /* Not supported */
-}
-
-static int macb_pcs_config(struct phylink_pcs *pcs,
- unsigned int neg_mode,
- phy_interface_t interface,
- const unsigned long *advertising,
- bool permit_pause_to_mac)
-{
- return 0;
+ spin_unlock_irqrestore(&bp->lock, flags);
+ return changed;
}
static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
.pcs_get_state = macb_usx_pcs_get_state,
.pcs_config = macb_usx_pcs_config,
- .pcs_link_up = macb_usx_pcs_link_up,
};
static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
.pcs_get_state = macb_pcs_get_state,
- .pcs_an_restart = macb_pcs_an_restart,
.pcs_config = macb_pcs_config,
+ .pcs_an_restart = macb_pcs_an_restart,
};
+static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
+ phy_interface_t interface)
+{
+ struct net_device *ndev = to_net_dev(config->dev);
+ struct macb *bp = netdev_priv(ndev);
+
+ if (interface == PHY_INTERFACE_MODE_10GBASER)
+ return &bp->phylink_usx_pcs;
+ else if (interface == PHY_INTERFACE_MODE_SGMII)
+ return &bp->phylink_sgmii_pcs;
+ else
+ return NULL;
+}
+
static void macb_mac_config(struct phylink_config *config, unsigned int mode,
const struct phylink_link_state *state)
{
@@ -646,18 +733,14 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
if (state->interface == PHY_INTERFACE_MODE_RMII)
ctrl |= MACB_BIT(RM9200_RMII);
} else if (macb_is_gem(bp)) {
- ctrl &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
- ncr &= ~GEM_BIT(ENABLE_HS_MAC);
-
- if (state->interface == PHY_INTERFACE_MODE_SGMII) {
- ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
- } else if (state->interface == PHY_INTERFACE_MODE_10GBASER) {
+ if (macb_mac_select_pcs(config, state->interface))
ctrl |= GEM_BIT(PCSSEL);
- ncr |= GEM_BIT(ENABLE_HS_MAC);
- } else if (bp->caps & MACB_CAPS_MIIONRGMII &&
- bp->phy_interface == PHY_INTERFACE_MODE_MII) {
+ else
+ ctrl &= ~GEM_BIT(PCSSEL);
+
+ if (bp->caps & MACB_CAPS_MIIONRGMII &&
+ bp->phy_interface == PHY_INTERFACE_MODE_MII)
ncr |= MACB_BIT(MIIONRGMII);
- }
}
/* Apply the new configuration, if any */
@@ -667,22 +750,6 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
if (old_ncr ^ ncr)
macb_or_gem_writel(bp, NCR, ncr);
- /* Disable AN for SGMII fixed link configuration, enable otherwise.
- * Must be written after PCSSEL is set in NCFGR,
- * otherwise writes will not take effect.
- */
- if (macb_is_gem(bp) && state->interface == PHY_INTERFACE_MODE_SGMII) {
- u32 pcsctrl, old_pcsctrl;
-
- old_pcsctrl = gem_readl(bp, PCSCNTRL);
- if (mode == MLO_AN_FIXED)
- pcsctrl = old_pcsctrl & ~GEM_BIT(PCSAUTONEG);
- else
- pcsctrl = old_pcsctrl | GEM_BIT(PCSAUTONEG);
- if (old_pcsctrl != pcsctrl)
- gem_writel(bp, PCSCNTRL, pcsctrl);
- }
-
spin_unlock_irqrestore(&bp->lock, flags);
}
@@ -735,10 +802,12 @@ static void macb_mac_link_up(struct phylink_config *config,
if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
ctrl &= ~MACB_BIT(PAE);
if (macb_is_gem(bp)) {
- ctrl &= ~GEM_BIT(GBE);
+ ctrl &= ~(GEM_BIT(GBE) | GEM_BIT(ENABLE_HS_MAC));
if (speed == SPEED_1000)
ctrl |= GEM_BIT(GBE);
+ else if (speed == SPEED_10000)
+ ctrl |= GEM_BIT(ENABLE_HS_MAC);
}
if (rx_pause)
@@ -776,20 +845,6 @@ static void macb_mac_link_up(struct phylink_config *config,
netif_tx_wake_all_queues(ndev);
}
-static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
- phy_interface_t interface)
-{
- struct net_device *ndev = to_net_dev(config->dev);
- struct macb *bp = netdev_priv(ndev);
-
- if (interface == PHY_INTERFACE_MODE_10GBASER)
- return &bp->phylink_usx_pcs;
- else if (interface == PHY_INTERFACE_MODE_SGMII)
- return &bp->phylink_sgmii_pcs;
- else
- return NULL;
-}
-
static const struct phylink_mac_ops macb_phylink_ops = {
.mac_select_pcs = macb_mac_select_pcs,
.mac_config = macb_mac_config,
--
2.35.1.1320.gc452695387.dirtyHi Sean,
Sorry for the delayed response.
We are working on MACB with two internal PCS's (10G-BASER, 1000-BASEX)
supporting 1G, 2.5G, 5G, and 10G with AN disabled.
I have sent an initial RFC :
https://lore.kernel.org/netdev/20241009053946.3198805-1-vineeth.karumanchi@amd.com/
Currently, we are working on integrating the MAC in fixed-link and
phy-mode.
Please see my inline comments.
On 5/12/2025 9:44 PM, Sean Anderson wrote:
> mac_prepare is called every time the interface is changed, so we can do
> all of our configuration there, instead of in mac_config. This will be
> useful for the next patch where we will set the PCS bit based on whether
> we are using our internal PCS. No functional change intended.
>
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>
<...>
> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
> + struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
> + bool changed = false;
> + unsigned long flags;
> + u32 old, new;
> +
> + spin_lock_irqsave(&bp->lock, flags);
> + old = new = gem_readl(bp, NCFGR);
> + new |= GEM_BIT(SGMIIEN);
This bit represents the AN feature, can we make it conditional to
facilitate IP's with AN disabled.
> + if (old != new) {
> + changed = true;
> + gem_writel(bp, NCFGR, new);
> + }
<..>
>
> static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
> @@ -589,45 +661,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
> bool permit_pause_to_mac)
> {
> struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
> + unsigned long flags;
> + bool changed;
> + u16 old, new;
>
> - gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
> - GEM_BIT(SIGNAL_OK));
> + spin_lock_irqsave(&bp->lock, flags);
> + if (macb_pcs_config_an(bp, neg_mode, interface, advertising))
> + changed = true;
>
> - return 0;
> -}
> + old = new = gem_readl(bp, USX_CONTROL);
> + new |= GEM_BIT(SIGNAL_OK);
> + if (old != new) {
> + changed = true;
> + gem_writel(bp, USX_CONTROL, new);
> + }
>
> -static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> - struct phylink_link_state *state)
> -{
> - state->link = 0;
> -}
> + old = new = gem_readl(bp, USX_CONTROL);
> + new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
> + new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
> + new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
> + new |= GEM_BIT(TX_EN);
> + if (old != new) {
> + changed = true;
> + gem_writel(bp, USX_CONTROL, new);
> + }
The above speed/rate configuration was moved from macb_usx_pcs_link_up()
where speed is an argument, which can be leveraged to configure multiple
speeds.
Can we achieve configuring for multiple speeds from
macb_usx_pcs_config() in fixed-link and phy-mode ?
--
🙏 vineeth
On 5/13/25 11:29, Karumanchi, Vineeth wrote: > Hi Sean, > > Sorry for the delayed response. > > We are working on MACB with two internal PCS's (10G-BASER, 1000-BASEX) supporting 1G, 2.5G, 5G, and 10G with AN disabled. > > I have sent an initial RFC : https://lore.kernel.org/netdev/20241009053946.3198805-1-vineeth.karumanchi@amd.com/ > > Currently, we are working on integrating the MAC in fixed-link and phy-mode. I had a look your series and based on the feedback you got I think this patch will help you ensure the PCS changes stay separate from the MAC stuff. I found it confusing on first read that you were configuring the "1G" PCS from the USX PCS callback. I think you are using 1G/2G speeds with the "1G" PCS and 5G/10G speeds with the USX PCS? Do you know if there is any public documentation for 10G support (even on non-versal SoCs)? That will make it easier to review your patch. --Sean
On 5/13/2025 10:10 PM, Sean Anderson wrote: > On 5/13/25 11:29, Karumanchi, Vineeth wrote: >> Hi Sean, >> >> Sorry for the delayed response. >> >> We are working on MACB with two internal PCS's (10G-BASER, 1000-BASEX) supporting 1G, 2.5G, 5G, and 10G with AN disabled. >> >> I have sent an initial RFC : https://lore.kernel.org/netdev/20241009053946.3198805-1-vineeth.karumanchi@amd.com/ >> >> Currently, we are working on integrating the MAC in fixed-link and phy-mode. > > I had a look your series and based on the feedback you got I think this > patch will help you ensure the PCS changes stay separate from the MAC > stuff. I found it confusing on first read that you were configuring the > "1G" PCS from the USX PCS callback. I think you are using 1G/2G speeds > with the "1G" PCS and 5G/10G speeds with the USX PCS? > yes, this patch does help. The IP was designed to configure all speeds (1G, 2.5G, 5G and 10G) from USX registers only, hence we are using USX PCS callback. > Do you know if there is any public documentation for 10G support > (even on non-versal SoCs)? That will make it easier to review your > patch. > > --Sean The Cadence IP document is internal, but we can share TRM of our board, which goes public later this month. I will add the link of it in our patch series. -- 🙏 vineeth
On 5/13/25 11:29, Karumanchi, Vineeth wrote:
> Hi Sean,
>
> Sorry for the delayed response.
>
> We are working on MACB with two internal PCS's (10G-BASER, 1000-BASEX) supporting 1G, 2.5G, 5G, and 10G with AN disabled.
>
> I have sent an initial RFC : https://lore.kernel.org/netdev/20241009053946.3198805-1-vineeth.karumanchi@amd.com/
>
> Currently, we are working on integrating the MAC in fixed-link and phy-mode.
>
> Please see my inline comments.
>
> On 5/12/2025 9:44 PM, Sean Anderson wrote:
>> mac_prepare is called every time the interface is changed, so we can do
>> all of our configuration there, instead of in mac_config. This will be
>> useful for the next patch where we will set the PCS bit based on whether
>> we are using our internal PCS. No functional change intended.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>
> <...>
>
>> +static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>> + phy_interface_t interface,
>> + const unsigned long *advertising,
>> + bool permit_pause_to_mac)
>> +{
>> + struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
>> + bool changed = false;
>> + unsigned long flags;
>> + u32 old, new;
>> +
>> + spin_lock_irqsave(&bp->lock, flags);
>> + old = new = gem_readl(bp, NCFGR);
>> + new |= GEM_BIT(SGMIIEN);
>
> This bit represents the AN feature, can we make it conditional to facilitate IP's with AN disabled.
To clarify, this bit enables SGMII timings for AN (as opposed to
1000Base-X). If you don't have AN enabled at 1G, then this bit affects
nothing.
1000Base-X is not currently supported by the built-in PCS. Therefore,
this bit should be set unconditionally at 1G speeds. This patch aims to
avoid functional changes so I have not made it conditional. Making this
bit conditional would be appropriate for a patch adding support for
1000Base-X using the internal PCS.
>> + if (old != new) {
>> + changed = true;
>> + gem_writel(bp, NCFGR, new);
>> + }
>
> <..>
>
>> static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
>> @@ -589,45 +661,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
>> bool permit_pause_to_mac)
>> {
>> struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
>> + unsigned long flags;
>> + bool changed;
>> + u16 old, new;
>> - gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
>> - GEM_BIT(SIGNAL_OK));
>> + spin_lock_irqsave(&bp->lock, flags);
>> + if (macb_pcs_config_an(bp, neg_mode, interface, advertising))
>> + changed = true;
>> - return 0;
>> -}
>> + old = new = gem_readl(bp, USX_CONTROL);
>> + new |= GEM_BIT(SIGNAL_OK);
>> + if (old != new) {
>> + changed = true;
>> + gem_writel(bp, USX_CONTROL, new);
>> + }
>> -static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>> - struct phylink_link_state *state)
>> -{
>> - state->link = 0;
>> -}
>> + old = new = gem_readl(bp, USX_CONTROL);
>> + new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
>> + new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
>> + new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
>> + new |= GEM_BIT(TX_EN);
>> + if (old != new) {
>> + changed = true;
>> + gem_writel(bp, USX_CONTROL, new);
>> + }
>
> The above speed/rate configuration was moved from macb_usx_pcs_link_up() where speed is an argument, which can be leveraged to configure multiple speeds.
>
> Can we achieve configuring for multiple speeds from macb_usx_pcs_config() in fixed-link and phy-mode ?
Form what I can tell, the USX PCS is only used for 10G interfaces. If
you want to add support for using it at other link speeds, then yes some
of these register writes should be moved to link_up. For the moment it
doesn't matter where they happen.
--Sean
Hi Sean,
On 5/13/2025 9:19 PM, Sean Anderson wrote:
> On 5/13/25 11:29, Karumanchi, Vineeth wrote:
>> Hi Sean,
<..>
>>> + spin_lock_irqsave(&bp->lock, flags);
>>> + old = new = gem_readl(bp, NCFGR);
>>> + new |= GEM_BIT(SGMIIEN);
>>
>> This bit represents the AN feature, can we make it conditional to facilitate IP's with AN disabled.
>
> To clarify, this bit enables SGMII timings for AN (as opposed to
> 1000Base-X). If you don't have AN enabled at 1G, then this bit affects
> nothing.
>
> 1000Base-X is not currently supported by the built-in PCS. Therefore,
> this bit should be set unconditionally at 1G speeds. This patch aims to
> avoid functional changes so I have not made it conditional. Making this
> bit conditional would be appropriate for a patch adding support for
> 1000Base-X using the internal PCS.
>
Yes, agreed.
>>> + if (old != new) {
>>> + changed = true;
>>> + gem_writel(bp, NCFGR, new);
>>> + }
>>
>> <..>
>>
>>> static void macb_usx_pcs_get_state(struct phylink_pcs *pcs,
>>> @@ -589,45 +661,60 @@ static int macb_usx_pcs_config(struct phylink_pcs *pcs,
>>> bool permit_pause_to_mac)
>>> {
>>> struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
>>> + unsigned long flags;
>>> + bool changed;
>>> + u16 old, new;
>>> - gem_writel(bp, USX_CONTROL, gem_readl(bp, USX_CONTROL) |
>>> - GEM_BIT(SIGNAL_OK));
>>> + spin_lock_irqsave(&bp->lock, flags);
>>> + if (macb_pcs_config_an(bp, neg_mode, interface, advertising))
>>> + changed = true;
>>> - return 0;
>>> -}
>>> + old = new = gem_readl(bp, USX_CONTROL);
>>> + new |= GEM_BIT(SIGNAL_OK);
>>> + if (old != new) {
>>> + changed = true;
>>> + gem_writel(bp, USX_CONTROL, new);
>>> + }
>>> -static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>>> - struct phylink_link_state *state)
>>> -{
>>> - state->link = 0;
>>> -}
>>> + old = new = gem_readl(bp, USX_CONTROL);
>>> + new = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, new);
>>> + new = GEM_BFINS(USX_CTRL_SPEED, HS_SPEED_10000M, new);
>>> + new &= ~(GEM_BIT(TX_SCR_BYPASS) | GEM_BIT(RX_SCR_BYPASS));
>>> + new |= GEM_BIT(TX_EN);
>>> + if (old != new) {
>>> + changed = true;
>>> + gem_writel(bp, USX_CONTROL, new);
>>> + }
>>
>> The above speed/rate configuration was moved from macb_usx_pcs_link_up() where speed is an argument, which can be leveraged to configure multiple speeds.
>>
>> Can we achieve configuring for multiple speeds from macb_usx_pcs_config() in fixed-link and phy-mode ?
>
> Form what I can tell, the USX PCS is only used for 10G interfaces. If
> you want to add support for using it at other link speeds, then yes some
> of these register writes should be moved to link_up. For the moment it
> doesn't matter where they happen.
>
> --Sean
Ok, in the latest cadence IP, all speed configurations are mapped to USX
registers for both internal PCS's (1000Base-X & 10GBase-R).
--
🙏 vineeth
This adds support for external PCSs. For example, the Xilinx UltraScale+
processor exposes its GMII interface to the FPGA fabric. This fabric may
implement PCS to convert GMII to a serial interface such as SGMII or
1000BASE-X. When present, the external PCS takes precedence over the
internal PCSs.
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
Changes in v4:
- Convert to dev-less pcs_put
Changes in v2:
- Move update to macb_pcs_get_state to previous patch
drivers/net/ethernet/cadence/macb.h | 1 +
drivers/net/ethernet/cadence/macb_main.c | 26 ++++++++++++++++++++++--
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index c9a5c8beb2fa..9d310814f052 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1291,6 +1291,7 @@ struct macb {
struct phylink_config phylink_config;
struct phylink_pcs phylink_usx_pcs;
struct phylink_pcs phylink_sgmii_pcs;
+ struct phylink_pcs *phylink_ext_pcs;
u32 caps;
unsigned int dma_burst_length;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ed37b1d85212..c644e5055022 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -21,6 +21,7 @@
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/dma-mapping.h>
+#include <linux/pcs.h>
#include <linux/platform_device.h>
#include <linux/phylink.h>
#include <linux/of.h>
@@ -707,7 +708,10 @@ static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
struct net_device *ndev = to_net_dev(config->dev);
struct macb *bp = netdev_priv(ndev);
- if (interface == PHY_INTERFACE_MODE_10GBASER)
+ if (bp->phylink_ext_pcs &&
+ test_bit(interface, bp->phylink_ext_pcs->supported_interfaces))
+ return bp->phylink_ext_pcs;
+ else if (interface == PHY_INTERFACE_MODE_10GBASER)
return &bp->phylink_usx_pcs;
else if (interface == PHY_INTERFACE_MODE_SGMII)
return &bp->phylink_sgmii_pcs;
@@ -733,7 +737,10 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
if (state->interface == PHY_INTERFACE_MODE_RMII)
ctrl |= MACB_BIT(RM9200_RMII);
} else if (macb_is_gem(bp)) {
- if (macb_mac_select_pcs(config, state->interface))
+ struct phylink_pcs *pcs = macb_mac_select_pcs(config,
+ state->interface);
+
+ if (pcs && pcs != bp->phylink_ext_pcs)
ctrl |= GEM_BIT(PCSSEL);
else
ctrl &= ~GEM_BIT(PCSSEL);
@@ -907,6 +914,14 @@ static int macb_mii_probe(struct net_device *dev)
bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;
+ bp->phylink_ext_pcs = pcs_get_by_fwnode_optional(&bp->pdev->dev,
+ bp->pdev->dev.fwnode,
+ NULL);
+ if (IS_ERR(bp->phylink_ext_pcs))
+ return dev_err_probe(&bp->pdev->dev,
+ PTR_ERR(bp->phylink_ext_pcs),
+ "Could not get external PCS\n");
+
bp->phylink_config.dev = &dev->dev;
bp->phylink_config.type = PHYLINK_NETDEV;
bp->phylink_config.mac_managed_pm = true;
@@ -924,6 +939,11 @@ static int macb_mii_probe(struct net_device *dev)
__set_bit(PHY_INTERFACE_MODE_RMII,
bp->phylink_config.supported_interfaces);
+ if (bp->phylink_ext_pcs)
+ phy_interface_or(bp->phylink_config.supported_interfaces,
+ bp->phylink_config.supported_interfaces,
+ bp->phylink_ext_pcs->supported_interfaces);
+
/* Determine what modes are supported */
if (macb_is_gem(bp) && (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
bp->phylink_config.mac_capabilities |= MAC_1000FD;
@@ -950,6 +970,7 @@ static int macb_mii_probe(struct net_device *dev)
if (IS_ERR(bp->phylink)) {
netdev_err(dev, "Could not create a phylink instance (%ld)\n",
PTR_ERR(bp->phylink));
+ pcs_put(bp->phylink_ext_pcs);
return PTR_ERR(bp->phylink);
}
@@ -5462,6 +5483,7 @@ static void macb_remove(struct platform_device *pdev)
bp->rx_clk, bp->tsu_clk);
pm_runtime_set_suspended(&pdev->dev);
}
+ pcs_put(bp->phylink_ext_pcs);
phylink_destroy(bp->phylink);
free_netdev(dev);
}
--
2.35.1.1320.gc452695387.dirtyThis adds device link support for PCS devices, providing
better probe ordering.
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Saravana Kannan <saravanak@google.com>
---
(no changes since v2)
Changes in v2:
- Reorder pcs_handle to come before suffix props
drivers/of/property.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index c1feb631e383..1aa28bfadb12 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1377,6 +1377,7 @@ DEFINE_SIMPLE_PROP(post_init_providers, "post-init-providers", NULL)
DEFINE_SIMPLE_PROP(access_controllers, "access-controllers", "#access-controller-cells")
DEFINE_SIMPLE_PROP(pses, "pses", "#pse-cells")
DEFINE_SIMPLE_PROP(power_supplies, "power-supplies", NULL)
+DEFINE_SIMPLE_PROP(pcs_handle, "pcs-handle", NULL)
DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
@@ -1528,6 +1529,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
{ .parse_prop = parse_interrupts, },
{ .parse_prop = parse_interrupt_map, },
{ .parse_prop = parse_access_controllers, },
+ { .parse_prop = parse_pcs_handle, },
{ .parse_prop = parse_regulators, },
{ .parse_prop = parse_gpio, },
{ .parse_prop = parse_gpios, },
--
2.35.1.1320.gc452695387.dirty© 2016 - 2025 Red Hat, Inc.