| Message ID | 20220712160308.13253-8-arun.ramadoss@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: add support for phylink mac config and link up | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 109 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Jul 12, 2022 at 09:33:05PM +0530, Arun Ramadoss wrote: > This patch apply the rgmii delay to the xmii tune adjust register based > on the interface selected in phylink mac config. There are two rgmii > port in LAN937x and value to be loaded in the register vary depends on > the port selected. > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > --- > drivers/net/dsa/microchip/lan937x_main.c | 61 ++++++++++++++++++++++++ > drivers/net/dsa/microchip/lan937x_reg.h | 18 +++++++ > 2 files changed, 79 insertions(+) > > diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c > index d86ffdf976b0..db88ea567ba6 100644 > --- a/drivers/net/dsa/microchip/lan937x_main.c > +++ b/drivers/net/dsa/microchip/lan937x_main.c > @@ -315,6 +315,45 @@ int lan937x_change_mtu(struct ksz_device *dev, int port, int new_mtu) > return 0; > } > > +static void lan937x_set_tune_adj(struct ksz_device *dev, int port, > + u16 reg, u8 val) > +{ > + u16 data16; > + > + ksz_pread16(dev, port, reg, &data16); > + > + /* Update tune Adjust */ > + data16 |= FIELD_PREP(PORT_TUNE_ADJ, val); > + ksz_pwrite16(dev, port, reg, data16); > + > + /* write DLL reset to take effect */ > + data16 |= PORT_DLL_RESET; > + ksz_pwrite16(dev, port, reg, data16); > +} > + > +static void lan937x_set_rgmii_tx_delay(struct ksz_device *dev, int port) > +{ > + u8 val; > + > + /* Apply different codes based on the ports as per characterization > + * results > + */ What characterization result are you referring to? Individual board designers should do their own characterization, that's why they provide a p->rgmii_tx_val in the device tree. The value provided there seems to be ignored and unconditionally replaced with 2 ns here. > + val = (port == LAN937X_RGMII_1_PORT) ? RGMII_1_TX_DELAY_2NS : > + RGMII_2_TX_DELAY_2NS; > + > + lan937x_set_tune_adj(dev, port, REG_PORT_XMII_CTRL_5, val); > +} > + > +static void lan937x_set_rgmii_rx_delay(struct ksz_device *dev, int port) > +{ > + u8 val; > + > + val = (port == LAN937X_RGMII_1_PORT) ? RGMII_1_RX_DELAY_2NS : > + RGMII_2_RX_DELAY_2NS; > + > + lan937x_set_tune_adj(dev, port, REG_PORT_XMII_CTRL_4, val); > +} > + > void lan937x_phylink_get_caps(struct ksz_device *dev, int port, > struct phylink_config *config) > { > @@ -331,6 +370,9 @@ void lan937x_phylink_mac_config(struct ksz_device *dev, int port, > unsigned int mode, > const struct phylink_link_state *state) > { > + phy_interface_t interface = state->interface; > + struct ksz_port *p = &dev->ports[port]; > + > /* Internal PHYs */ > if (dev->info->internal_phy[port]) > return; > @@ -341,6 +383,25 @@ void lan937x_phylink_mac_config(struct ksz_device *dev, int port, > } > > ksz_set_xmii(dev, port, state->interface); > + > + /* if the delay is 0, do not enable DLL */ > + if (interface == PHY_INTERFACE_MODE_RGMII_ID || > + interface == PHY_INTERFACE_MODE_RGMII_RXID) { Why not all RGMII modes and only these 2? There was a discussion a long time ago that the "_*ID" values refer to delays applied by an attached PHY. Here you are refusing to apply RGMII TX delays in the "rgmii" and "rgmii-txid" modes. > + if (p->rgmii_tx_val) { > + lan937x_set_rgmii_tx_delay(dev, port); > + dev_info(dev->dev, "Applied rgmii tx delay for the port %d\n", > + port); > + } > + } > + > + if (interface == PHY_INTERFACE_MODE_RGMII_ID || > + interface == PHY_INTERFACE_MODE_RGMII_TXID) { > + if (p->rgmii_rx_val) { > + lan937x_set_rgmii_rx_delay(dev, port); > + dev_info(dev->dev, "Applied rgmii rx delay for the port %d\n", > + port); > + } > + } > } >
On Tue, 2022-07-19 at 13:25 +0300, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, Jul 12, 2022 at 09:33:05PM +0530, Arun Ramadoss wrote: > > This patch apply the rgmii delay to the xmii tune adjust register > > based > > on the interface selected in phylink mac config. There are two > > rgmii > > port in LAN937x and value to be loaded in the register vary depends > > on > > the port selected. > > > > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> > > --- > > drivers/net/dsa/microchip/lan937x_main.c | 61 > > ++++++++++++++++++++++++ > > drivers/net/dsa/microchip/lan937x_reg.h | 18 +++++++ > > 2 files changed, 79 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/lan937x_main.c > > b/drivers/net/dsa/microchip/lan937x_main.c > > index d86ffdf976b0..db88ea567ba6 100644 > > --- a/drivers/net/dsa/microchip/lan937x_main.c > > +++ b/drivers/net/dsa/microchip/lan937x_main.c > > @@ -315,6 +315,45 @@ int lan937x_change_mtu(struct ksz_device *dev, > > int port, int new_mtu) > > return 0; > > } > > > > + > > +static void lan937x_set_rgmii_tx_delay(struct ksz_device *dev, int > > port) > > +{ > > + u8 val; > > + > > + /* Apply different codes based on the ports as per > > characterization > > + * results > > + */ > > What characterization result are you referring to? Individual board > designers should do their own characterization, that's why they > provide > a p->rgmii_tx_val in the device tree. The value provided there seems > to > be ignored and unconditionally replaced with 2 ns here. This is the value we got from the post silicon validation team which has to be programmed to dll register to get the proper delay. The value is different for rgmii 1 and rgmii2. > > > + val = (port == LAN937X_RGMII_1_PORT) ? RGMII_1_TX_DELAY_2NS : > > + RGMII_2_TX_DELAY_2NS; > > + > > + lan937x_set_tune_adj(dev, port, REG_PORT_XMII_CTRL_5, val); > > +} > > + > > + > > @@ -341,6 +383,25 @@ void lan937x_phylink_mac_config(struct > > ksz_device *dev, int port, > > } > > > > ksz_set_xmii(dev, port, state->interface); > > + > > + /* if the delay is 0, do not enable DLL */ > > + if (interface == PHY_INTERFACE_MODE_RGMII_ID || > > + interface == PHY_INTERFACE_MODE_RGMII_RXID) { > > Why not all RGMII modes and only these 2? There was a discussion a > long > time ago that the "_*ID" values refer to delays applied by an > attached PHY. > Here you are refusing to apply RGMII TX delays in the "rgmii" and > "rgmii-txid" > modes. I have reused the code of ksz9477 cpu config function and added the dll configuration for lan937x family alone. And understood that if device tree specificies as rgmii_txid then apply the egress delay, for rgmii_rxid apply ingress delay, for rgmii_id apply both. From your comment, I am inferring that apply the mac delay for all the rgmii interface "rgmii*". Can you correct me if am I wrong and bit elaborate on it. > > > + if (p->rgmii_tx_val) { > > + lan937x_set_rgmii_tx_delay(dev, port); > > + dev_info(dev->dev, "Applied rgmii tx delay > > for the port %d\n", > > + port); > > + } > > + } > > + > > + if (interface == PHY_INTERFACE_MODE_RGMII_ID || > > + interface == PHY_INTERFACE_MODE_RGMII_TXID) { > > + if (p->rgmii_rx_val) { > > + lan937x_set_rgmii_rx_delay(dev, port); > > + dev_info(dev->dev, "Applied rgmii rx delay > > for the port %d\n", > > + port); > > + } > > + } > > } > >
On Wed, Jul 20, 2022 at 02:51:42PM +0000, Arun.Ramadoss@microchip.com wrote: > > Why not all RGMII modes and only these 2? There was a discussion a long > > time ago that the "_*ID" values refer to delays applied by an attached PHY. > > Here you are refusing to apply RGMII TX delays in the "rgmii" and "rgmii-txid" > > modes. > > I have reused the code of ksz9477 cpu config function and added the dll > configuration for lan937x family alone. And understood that if device > tree specificies as rgmii_txid then apply the egress delay, for > rgmii_rxid apply ingress delay, for rgmii_id apply both. > From your comment, I am inferring that apply the mac delay for all the > rgmii interface "rgmii*". > Can you correct me if am I wrong and bit elaborate on it. What we are trying is to interpret what's written in Documentation/devicetree/bindings/net/ethernet-controller.yaml in a way that is consistent, even though its wording makes it evident that that it was written in simpler times. There it says: # RX and TX delays are added by the MAC when required - rgmii # RGMII with internal RX and TX delays provided by the PHY, # the MAC should not add the RX or TX delays in this case - rgmii-id # RGMII with internal RX delay provided by the PHY, the MAC # should not add an RX delay in this case - rgmii-rxid # RGMII with internal TX delay provided by the PHY, the MAC # should not add an TX delay in this case - rgmii-txid The fact that the PHY adds delays in the directions specified by the phy-mode value is established behavior; yet the MAC's behavior is pretty much subject to interpretation, and this has resulted in varying implementations in drivers. The wise words above say that "RX and TX delays are added by the MAC when required" - ok but when are they required? Determining this is impossible based on the phy-mode alone, since there aren't just 2 degrees of freedom who may add delays (the PHY and the MAC). There also exists the possibility of using serpentine traces (PCB delays), which practically speaking, can't be described in phy-mode because then, a potential PHY on the same link would also think they're for itself to apply. So the modern interpretation of phy-mode is to say that it simply does not describe whether a MAC should apply delays in a direction or another. So a separate set of properties was introduced, "rx-internal-delay-ps" and "tx-internal-delay-ps". These have the advantage of being located under the OF node of the MAC, and under the OF node of the PHY respectively. So it's clearer which side is supposed to configure which delays, and you also have finer control over the clock skew. Initially when Prasanna first tried to upstream the lan937x, we discusssed that since it's a new driver, it should support the modern interpretation of RGMII delays, and we agreed on that strategy. Now, with your recent refactoring that makes all switches share the same probing logic (and OF node parsing), things are in a bit of a greyer area. For one thing, you seem to have involuntarily inherited support for dubious legacy bindings, such as the "phy-mode under the switch node" that is described by commit edecfa98f602 ("net: dsa: microchip: look for phy-mode in port nodes"). For another, you've inherited the existing interpretation of RGMII delays from the KSZ9477 driver, which is that the driver interprets the "phy-mode" as if it's a PHY (when in fact it's a MAC). The KSZ9477 isn't by far the only MAC driver that applies RGMII delays based on the phy-mode string, however in the past we made an effort to update existing DSA drivers to the modern interpretation, see commits: 9ca482a246f0 ("net: dsa: sja1105: parse {rx, tx}-internal-delay-ps properties for RGMII delays") 5654ec78dd7e ("net: dsa: qca8k: rework rgmii delay logic and scan for cpu port 6") There is a possibility to have a transitioning scheme: if the "rx-internal-delay-ps" or "tx-internal-delay-ps" properties are present under the MAC OF node, apply delays based on those. Otherwise, apply delays based on phy-mode, and warn. I'd appreciate if you could consider updating the KSZ common driver to this interpretation of RGMII delays, so that the modern interpretation becomes more widespread, and there are fewer places from which to copy a non-standard one.
diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c index d86ffdf976b0..db88ea567ba6 100644 --- a/drivers/net/dsa/microchip/lan937x_main.c +++ b/drivers/net/dsa/microchip/lan937x_main.c @@ -315,6 +315,45 @@ int lan937x_change_mtu(struct ksz_device *dev, int port, int new_mtu) return 0; } +static void lan937x_set_tune_adj(struct ksz_device *dev, int port, + u16 reg, u8 val) +{ + u16 data16; + + ksz_pread16(dev, port, reg, &data16); + + /* Update tune Adjust */ + data16 |= FIELD_PREP(PORT_TUNE_ADJ, val); + ksz_pwrite16(dev, port, reg, data16); + + /* write DLL reset to take effect */ + data16 |= PORT_DLL_RESET; + ksz_pwrite16(dev, port, reg, data16); +} + +static void lan937x_set_rgmii_tx_delay(struct ksz_device *dev, int port) +{ + u8 val; + + /* Apply different codes based on the ports as per characterization + * results + */ + val = (port == LAN937X_RGMII_1_PORT) ? RGMII_1_TX_DELAY_2NS : + RGMII_2_TX_DELAY_2NS; + + lan937x_set_tune_adj(dev, port, REG_PORT_XMII_CTRL_5, val); +} + +static void lan937x_set_rgmii_rx_delay(struct ksz_device *dev, int port) +{ + u8 val; + + val = (port == LAN937X_RGMII_1_PORT) ? RGMII_1_RX_DELAY_2NS : + RGMII_2_RX_DELAY_2NS; + + lan937x_set_tune_adj(dev, port, REG_PORT_XMII_CTRL_4, val); +} + void lan937x_phylink_get_caps(struct ksz_device *dev, int port, struct phylink_config *config) { @@ -331,6 +370,9 @@ void lan937x_phylink_mac_config(struct ksz_device *dev, int port, unsigned int mode, const struct phylink_link_state *state) { + phy_interface_t interface = state->interface; + struct ksz_port *p = &dev->ports[port]; + /* Internal PHYs */ if (dev->info->internal_phy[port]) return; @@ -341,6 +383,25 @@ void lan937x_phylink_mac_config(struct ksz_device *dev, int port, } ksz_set_xmii(dev, port, state->interface); + + /* if the delay is 0, do not enable DLL */ + if (interface == PHY_INTERFACE_MODE_RGMII_ID || + interface == PHY_INTERFACE_MODE_RGMII_RXID) { + if (p->rgmii_tx_val) { + lan937x_set_rgmii_tx_delay(dev, port); + dev_info(dev->dev, "Applied rgmii tx delay for the port %d\n", + port); + } + } + + if (interface == PHY_INTERFACE_MODE_RGMII_ID || + interface == PHY_INTERFACE_MODE_RGMII_TXID) { + if (p->rgmii_rx_val) { + lan937x_set_rgmii_rx_delay(dev, port); + dev_info(dev->dev, "Applied rgmii rx delay for the port %d\n", + port); + } + } } int lan937x_setup(struct dsa_switch *ds) diff --git a/drivers/net/dsa/microchip/lan937x_reg.h b/drivers/net/dsa/microchip/lan937x_reg.h index a6cb3ca22dc3..ba4adaddb3ec 100644 --- a/drivers/net/dsa/microchip/lan937x_reg.h +++ b/drivers/net/dsa/microchip/lan937x_reg.h @@ -136,6 +136,12 @@ #define PORT_MII_SEL_EDGE BIT(5) +#define REG_PORT_XMII_CTRL_4 0x0304 +#define REG_PORT_XMII_CTRL_5 0x0306 + +#define PORT_DLL_RESET BIT(15) +#define PORT_TUNE_ADJ GENMASK(13, 7) + /* 4 - MAC */ #define REG_PORT_MAC_CTRL_0 0x0400 #define PORT_CHECK_LENGTH BIT(2) @@ -161,6 +167,18 @@ #define P_PRIO_CTRL REG_PORT_MRI_PRIO_CTRL +/* The port number as per the datasheet */ +#define RGMII_2_PORT_NUM 5 +#define RGMII_1_PORT_NUM 6 + +#define LAN937X_RGMII_2_PORT (RGMII_2_PORT_NUM - 1) +#define LAN937X_RGMII_1_PORT (RGMII_1_PORT_NUM - 1) + +#define RGMII_1_TX_DELAY_2NS 2 +#define RGMII_2_TX_DELAY_2NS 0 +#define RGMII_1_RX_DELAY_2NS 0x1B +#define RGMII_2_RX_DELAY_2NS 0x14 + #define LAN937X_TAG_LEN 2 #endif
This patch apply the rgmii delay to the xmii tune adjust register based on the interface selected in phylink mac config. There are two rgmii port in LAN937x and value to be loaded in the register vary depends on the port selected. Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com> --- drivers/net/dsa/microchip/lan937x_main.c | 61 ++++++++++++++++++++++++ drivers/net/dsa/microchip/lan937x_reg.h | 18 +++++++ 2 files changed, 79 insertions(+)