On Sun, Oct 10, 2021 at 03:47:32PM +0300, Vladimir Oltean wrote:
> On Sun, Oct 10, 2021 at 01:15:49PM +0200, Ansuel Smith wrote:
> > Future proof commit. This switch have 2 CPU port and one valid
> > configuration is first CPU port set to sgmii and second CPU port set to
> > regmii-id. The current implementation detects delay only for CPU port
> > zero set to rgmii and doesn't count any delay set in a secondary CPU
> > port. Drop the current delay scan function and move it to the phylink
> > mac_config to generilize and implicitly add support for secondary CPU
> > port set to rgmii-id.
> >
> > Signed-off-by: Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > ---
> > drivers/net/dsa/qca8k.c | 121 +++++++++++++++-------------------------
> > drivers/net/dsa/qca8k.h | 2 -
> > 2 files changed, 44 insertions(+), 79 deletions(-)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index df0a622acdd7..126f20b0b94c 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -888,68 +888,6 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
> > return 0;
> > }
> >
> > -static int
> > -qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
>
> I was actually going to say that since RGMII delays are runtime
> invariants, you should move their entire programming to probe time, now
> you move device tree parsing to runtime :-/
>
The main idea here was to move everything to mac config and scan the DT
node of the current port that is being configured.
> > -{
> > - struct device_node *port_dn;
> > - phy_interface_t mode;
> > - struct dsa_port *dp;
> > - u32 val;
> > -
> > - /* CPU port is already checked */
> > - dp = dsa_to_port(priv->ds, 0);
> > -
> > - port_dn = dp->dn;
> > -
> > - /* Check if port 0 is set to the correct type */
> > - of_get_phy_mode(port_dn, &mode);
> > - if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > - mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > - mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > - return 0;
> > - }
> > -
> > - switch (mode) {
> > - case PHY_INTERFACE_MODE_RGMII_ID:
> > - case PHY_INTERFACE_MODE_RGMII_RXID:
>
> Also, since you touch this area.
> There have been tons of discussions on this topic, but I believe that
> your interpretation of the RGMII delays is wrong.
> Basically a MAC should not apply delays based on the phy-mode string (so
> it should treat "rgmii" same as "rgmii-id"), but based on the value of
> "rx-internal-delay-ps" and "tx-internal-delay-ps".
> The phy-mode is for a PHY to use.
>
Ok so we can just drop the case and directly check for the
internal-delay-ps presence?
> > - if (of_property_read_u32(port_dn, "rx-internal-delay-ps", &val))
> > - val = 2;
> > - else
> > - /* Switch regs accept value in ns, convert ps to ns */
> > - val = val / 1000;
> > -
> > - if (val > QCA8K_MAX_DELAY) {
> > - dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
> > - val = 3;
> > - }
> > -
> > - priv->rgmii_rx_delay = val;
> > - /* Stop here if we need to check only for rx delay */
> > - if (mode != PHY_INTERFACE_MODE_RGMII_ID)
> > - break;
> > -
> > - fallthrough;
> > - case PHY_INTERFACE_MODE_RGMII_TXID:
> > - if (of_property_read_u32(port_dn, "tx-internal-delay-ps", &val))
> > - val = 1;
> > - else
> > - /* Switch regs accept value in ns, convert ps to ns */
> > - val = val / 1000;
> > -
> > - if (val > QCA8K_MAX_DELAY) {
> > - dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
> > - val = 3;
> > - }
> > -
> > - priv->rgmii_tx_delay = val;
> > - break;
> > - default:
> > - return 0;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > static int
> > qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
> > {
> > @@ -1019,10 +957,6 @@ qca8k_setup(struct dsa_switch *ds)
> > if (ret)
> > return ret;
> >
> > - ret = qca8k_setup_of_rgmii_delay(priv);
> > - if (ret)
> > - return ret;
> > -
> > ret = qca8k_setup_mac_pwr_sel(priv);
> > if (ret)
> > return ret;
> > @@ -1190,7 +1124,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > {
> > struct qca8k_priv *priv = ds->priv;
> > struct dsa_port *dp;
> > - u32 reg, val;
> > + u32 reg, val, delay;
> > int ret;
> >
> > switch (port) {
> > @@ -1241,17 +1175,50 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > case PHY_INTERFACE_MODE_RGMII_ID:
> > case PHY_INTERFACE_MODE_RGMII_TXID:
> > case PHY_INTERFACE_MODE_RGMII_RXID:
> > - /* RGMII_ID needs internal delay. This is enabled through
> > - * PORT5_PAD_CTRL for all ports, rather than individual port
> > - * registers
> > + dp = dsa_to_port(ds, port);
> > + val = QCA8K_PORT_PAD_RGMII_EN;
> > +
> > + if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + state->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > + if (of_property_read_u32(dp->dn, "tx-internal-delay-ps", &delay))
> > + delay = 1;
> > + else
> > + /* Switch regs accept value in ns, convert ps to ns */
> > + delay = delay / 1000;
> > +
> > + if (delay > QCA8K_MAX_DELAY) {
> > + dev_err(priv->dev, "rgmii tx delay is limited to a max value of 3ns, setting to the max value");
> > + delay = 3;
> > + }
> > +
> > + val |= QCA8K_PORT_PAD_RGMII_TX_DELAY(delay) |
> > + QCA8K_PORT_PAD_RGMII_TX_DELAY_EN;
> > + }
> > +
> > + if (state->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + state->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> > + if (of_property_read_u32(dp->dn, "rx-internal-delay-ps", &delay))
> > + delay = 2;
> > + else
> > + /* Switch regs accept value in ns, convert ps to ns */
> > + delay = delay / 1000;
> > +
> > + if (delay > QCA8K_MAX_DELAY) {
> > + dev_err(priv->dev, "rgmii rx delay is limited to a max value of 3ns, setting to the max value");
> > + delay = 3;
> > + }
> > +
> > + val |= QCA8K_PORT_PAD_RGMII_RX_DELAY(delay) |
> > + QCA8K_PORT_PAD_RGMII_RX_DELAY_EN;
> > + }
> > +
> > + /* Set RGMII delay based on the selected values */
> > + qca8k_write(priv, reg, val);
> > +
> > + /* QCA8337 requires to set rgmii rx delay for all ports.
> > + * This is enabled through PORT5_PAD_CTRL for all ports,
> > + * rather than individual port registers.
> > */
> > - qca8k_write(priv, reg,
> > - QCA8K_PORT_PAD_RGMII_EN |
> > - QCA8K_PORT_PAD_RGMII_TX_DELAY(priv->rgmii_tx_delay) |
> > - QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) |
> > - QCA8K_PORT_PAD_RGMII_TX_DELAY_EN |
> > - QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> > - /* QCA8337 requires to set rgmii rx delay */
> > if (priv->switch_id == QCA8K_ID_QCA8337)
> > qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
> > QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index 5df0f0ef6526..a790b27bc310 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -259,8 +259,6 @@ struct qca8k_match_data {
> > struct qca8k_priv {
> > u8 switch_id;
> > u8 switch_revision;
> > - u8 rgmii_tx_delay;
> > - u8 rgmii_rx_delay;
> > bool legacy_phy_port_mapping;
> > struct regmap *regmap;
> > struct mii_bus *bus;
> > --
> > 2.32.0
> >
>
--
Ansuel
