Add support for VSC8531_02 PHY ID.
Also provide an option to tune RGMII delay value via devicetree.
The default delays are retained in the driver.
v2 changes:
- Added patch to use a common vendor phy id match
- Removed dt include header patch because delays should be specied in
ps, not register values
- Updated DT binding description and commit for optional delay tuning to
be clearer on the precedence
- Updated dt property name to include vendor instead of phy device name
- Switch both VSC8531 and VSC8531-02 to use exact phy id match as they
share the same model number
- Ensure RCT
- Improve optional property read
RFC link: https://lore.kernel.org/all/[email protected]/
Harini Katakam (3):
phy: mscc: Use PHY_ID_MATCH_VENDOR to minimize PHY ID table
dt-bindings: mscc: Add RGMII RX and TX delay tuning
phy: mscc: Add support for VSC8531_02 with RGMII tuning
.../bindings/net/mscc-phy-vsc8531.txt | 2 +
drivers/net/phy/mscc/mscc.h | 3 ++
drivers/net/phy/mscc/mscc_main.c | 54 +++++++++++++------
3 files changed, 42 insertions(+), 17 deletions(-)
--
2.17.1
All the PHY devices variants specified have the same mask and
hence can be simplified to one vendor look up for 0xfffffff0.
Any individual config can be identified by PHY_ID_MATCH_EXACT
in the respective structure.
Signed-off-by: Harini Katakam <[email protected]>
---
v2:
New patch
drivers/net/phy/mscc/mscc_main.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 62bf99e45af1..75d9582e5784 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2656,19 +2656,7 @@ static struct phy_driver vsc85xx_driver[] = {
module_phy_driver(vsc85xx_driver);
static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
- { PHY_ID_VSC8504, 0xfffffff0, },
- { PHY_ID_VSC8514, 0xfffffff0, },
- { PHY_ID_VSC8530, 0xfffffff0, },
- { PHY_ID_VSC8531, 0xfffffff0, },
- { PHY_ID_VSC8540, 0xfffffff0, },
- { PHY_ID_VSC8541, 0xfffffff0, },
- { PHY_ID_VSC8552, 0xfffffff0, },
- { PHY_ID_VSC856X, 0xfffffff0, },
- { PHY_ID_VSC8572, 0xfffffff0, },
- { PHY_ID_VSC8574, 0xfffffff0, },
- { PHY_ID_VSC8575, 0xfffffff0, },
- { PHY_ID_VSC8582, 0xfffffff0, },
- { PHY_ID_VSC8584, 0xfffffff0, },
+ { PHY_ID_MATCH_VENDOR(0xfffffff0) },
{ }
};
--
2.17.1
From: Harini Katakam <[email protected]>
Add optional properties to tune RGMII RX and TX delay. The current
default value in the Linux driver, when the phy-mode is rgmii-id,
is 2ns for both. These properties take priority if specified.
Signed-off-by: Harini Katakam <[email protected]>
Signed-off-by: Radhey Shyam Pandey <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
v2:
- Updated DT binding description and commit for optional delay tuning
to be clearer on the precedence
- Updated dt property name to include vendor instead of phy device name
Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 0a3647fe331b..2b779bc3096b 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -33,6 +33,8 @@ Optional properties:
VSC8531_DUPLEX_COLLISION (8).
- load-save-gpios : GPIO used for the load/save operation of the PTP
hardware clock (PHC).
+- mscc,rx-delay : RGMII RX delay. Allowed values are 0.2 - 3.4 ns.
+- mscc,tx-delay : RGMII TX delay. Allowed values are 0.2 - 3.4 ns.
Table: 1 - Edge rate change
--
2.17.1
From: Harini Katakam <[email protected]>
Add support for VSC8531_02 (Rev 2) device.
Add support for optional RGMII RX and TX delay tuning via devicetree.
The hierarchy is:
- Retain the defaul 0.2ns delay when RGMII tuning is not set.
- Retain the default 2ns delay when RGMII tuning is set and DT delay
property is NOT specified.
- Use the DT delay value when RGMII tuning is set and a DT delay
property is specified.
Signed-off-by: Harini Katakam <[email protected]>
Signed-off-by: Radhey Shyam Pandey <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
v2:
- Switch both VSC8531 and VSC8531-02 to use exact phy id match as they
share the same model number
- Ensure RCT
- Improve optional property read
drivers/net/phy/mscc/mscc.h | 3 +++
drivers/net/phy/mscc/mscc_main.c | 40 ++++++++++++++++++++++++++++----
2 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index a50235fdf7d9..5a26eba0ace0 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -281,6 +281,7 @@ enum rgmii_clock_delay {
#define PHY_ID_VSC8514 0x00070670
#define PHY_ID_VSC8530 0x00070560
#define PHY_ID_VSC8531 0x00070570
+#define PHY_ID_VSC8531_02 0x00070572
#define PHY_ID_VSC8540 0x00070760
#define PHY_ID_VSC8541 0x00070770
#define PHY_ID_VSC8552 0x000704e0
@@ -373,6 +374,8 @@ struct vsc8531_private {
* package.
*/
unsigned int base_addr;
+ u32 rx_delay;
+ u32 tx_delay;
#if IS_ENABLED(CONFIG_MACSEC)
/* MACsec fields:
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 75d9582e5784..80cc90a23d57 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -525,6 +525,7 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
{
u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1;
u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
+ struct vsc8531_private *vsc8531 = phydev->priv;
u16 reg_val = 0;
int rc;
@@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
- reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_rx_delay_pos;
+ reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos;
if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
- reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_tx_delay_pos;
+ reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos;
rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
rgmii_cntl,
@@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device *phydev)
{
int rc, i, phy_id;
struct vsc8531_private *vsc8531 = phydev->priv;
+ struct device_node *of_node = phydev->mdio.dev.of_node;
+
+ vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
+ rc = of_property_read_u32(of_node, "mscc,rx-delay",
+ &vsc8531->rx_delay);
+
+ vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
+ rc = of_property_read_u32(of_node, "mscc,tx-delay",
+ &vsc8531->tx_delay);
rc = vsc85xx_default_config(phydev);
if (rc)
@@ -2413,9 +2423,8 @@ static struct phy_driver vsc85xx_driver[] = {
.get_stats = &vsc85xx_get_stats,
},
{
- .phy_id = PHY_ID_VSC8531,
+ PHY_ID_MATCH_EXACT(PHY_ID_VSC8531),
.name = "Microsemi VSC8531",
- .phy_id_mask = 0xfffffff0,
/* PHY_GBIT_FEATURES */
.soft_reset = &genphy_soft_reset,
.config_init = &vsc85xx_config_init,
@@ -2436,6 +2445,29 @@ static struct phy_driver vsc85xx_driver[] = {
.get_strings = &vsc85xx_get_strings,
.get_stats = &vsc85xx_get_stats,
},
+{
+ PHY_ID_MATCH_EXACT(PHY_ID_VSC8531_02),
+ .name = "Microsemi VSC8531-02",
+ /* PHY_GBIT_FEATURES */
+ .soft_reset = &genphy_soft_reset,
+ .config_init = &vsc85xx_config_init,
+ .config_aneg = &vsc85xx_config_aneg,
+ .read_status = &vsc85xx_read_status,
+ .handle_interrupt = vsc85xx_handle_interrupt,
+ .config_intr = &vsc85xx_config_intr,
+ .suspend = &genphy_suspend,
+ .resume = &genphy_resume,
+ .probe = &vsc85xx_probe,
+ .set_wol = &vsc85xx_wol_set,
+ .get_wol = &vsc85xx_wol_get,
+ .get_tunable = &vsc85xx_get_tunable,
+ .set_tunable = &vsc85xx_set_tunable,
+ .read_page = &vsc85xx_phy_read_page,
+ .write_page = &vsc85xx_phy_write_page,
+ .get_sset_count = &vsc85xx_get_sset_count,
+ .get_strings = &vsc85xx_get_strings,
+ .get_stats = &vsc85xx_get_stats,
+},
{
.phy_id = PHY_ID_VSC8540,
.name = "Microsemi FE VSC8540 SyncE",
--
2.17.1
On Wed, Apr 26, 2023 at 04:13:12PM +0530, Harini Katakam wrote:
> From: Harini Katakam <[email protected]>
>
> Add optional properties to tune RGMII RX and TX delay. The current
> default value in the Linux driver, when the phy-mode is rgmii-id,
> is 2ns for both. These properties take priority if specified.
>
> Signed-off-by: Harini Katakam <[email protected]>
> Signed-off-by: Radhey Shyam Pandey <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> v2:
> - Updated DT binding description and commit for optional delay tuning
> to be clearer on the precedence
> - Updated dt property name to include vendor instead of phy device name
>
> Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 0a3647fe331b..2b779bc3096b 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -33,6 +33,8 @@ Optional properties:
> VSC8531_DUPLEX_COLLISION (8).
> - load-save-gpios : GPIO used for the load/save operation of the PTP
> hardware clock (PHC).
> +- mscc,rx-delay : RGMII RX delay. Allowed values are 0.2 - 3.4 ns.
> +- mscc,tx-delay : RGMII TX delay. Allowed values are 0.2 - 3.4 ns.
We have rx-internal-delay-ps and tx-internal-delay-ps in the common
ethernet-phy.yaml, there is no need for a custom property.
On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> From: Harini Katakam <[email protected]>
>
> Add support for VSC8531_02 (Rev 2) device.
> Add support for optional RGMII RX and TX delay tuning via devicetree.
> The hierarchy is:
> - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> - Retain the default 2ns delay when RGMII tuning is set and DT delay
> property is NOT specified.
> - Use the DT delay value when RGMII tuning is set and a DT delay
> property is specified.
>
> Signed-off-by: Harini Katakam <[email protected]>
> Signed-off-by: Radhey Shyam Pandey <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> v2:
> - Switch both VSC8531 and VSC8531-02 to use exact phy id match as they
> share the same model number
> - Ensure RCT
> - Improve optional property read
>
> drivers/net/phy/mscc/mscc.h | 3 +++
> drivers/net/phy/mscc/mscc_main.c | 40 ++++++++++++++++++++++++++++----
> 2 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index a50235fdf7d9..5a26eba0ace0 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -281,6 +281,7 @@ enum rgmii_clock_delay {
> #define PHY_ID_VSC8514 0x00070670
> #define PHY_ID_VSC8530 0x00070560
> #define PHY_ID_VSC8531 0x00070570
> +#define PHY_ID_VSC8531_02 0x00070572
> #define PHY_ID_VSC8540 0x00070760
> #define PHY_ID_VSC8541 0x00070770
> #define PHY_ID_VSC8552 0x000704e0
> @@ -373,6 +374,8 @@ struct vsc8531_private {
> * package.
> */
> unsigned int base_addr;
> + u32 rx_delay;
> + u32 tx_delay;
>
> #if IS_ENABLED(CONFIG_MACSEC)
> /* MACsec fields:
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 75d9582e5784..80cc90a23d57 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -525,6 +525,7 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
> {
> u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1;
> u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
> + struct vsc8531_private *vsc8531 = phydev->priv;
> u16 reg_val = 0;
> int rc;
>
> @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
>
> if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> - reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_rx_delay_pos;
> + reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos;
> if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> - reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_tx_delay_pos;
> + reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos;
>
> rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> rgmii_cntl,
> @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device *phydev)
> {
> int rc, i, phy_id;
> struct vsc8531_private *vsc8531 = phydev->priv;
> + struct device_node *of_node = phydev->mdio.dev.of_node;
> +
> + vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
> + rc = of_property_read_u32(of_node, "mscc,rx-delay",
> + &vsc8531->rx_delay);
> +
> + vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
> + rc = of_property_read_u32(of_node, "mscc,tx-delay",
> + &vsc8531->tx_delay);
Since the dt-bindings document says "If this property is present then
the PHY applies the RX|TX delay", then I guess the precedence as applied
by vsc85xx_rgmii_set_skews() should be different. The RX delays should
be applied based on rx-internal-delay-ps (if present) regardless of
phy-mode, or set to RGMII_CLK_DELAY_2_0_NS if we are in the rgmii-rxid
or rgmii-id modes. Similar for tx.
Also, please split the VSC8531-02 addition into a separate patch, since
the configurable RGMII delays also affect existing PHYs (like VSC8502).
Hi Vladimir,
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: Wednesday, April 26, 2023 4:48 PM
> To: Katakam, Harini <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; wsa+renesas@sang-
> engineering.com; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Simek, Michal <[email protected]>;
> Pandey, Radhey Shyam <[email protected]>
> Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
> with RGMII tuning
>
> On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> > From: Harini Katakam <[email protected]>
> >
> > Add support for VSC8531_02 (Rev 2) device.
> > Add support for optional RGMII RX and TX delay tuning via devicetree.
> > The hierarchy is:
> > - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> > - Retain the default 2ns delay when RGMII tuning is set and DT delay
> > property is NOT specified.
> > - Use the DT delay value when RGMII tuning is set and a DT delay
> > property is specified.
> >
> > Signed-off-by: Harini Katakam <[email protected]>
> > Signed-off-by: Radhey Shyam Pandey
> <[email protected]>
> > Signed-off-by: Michal Simek <[email protected]>
> > ---
<snip>
> > @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct
> phy_device *phydev, u32 rgmii_cntl,
> >
> > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> > phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> > - reg_val |= RGMII_CLK_DELAY_2_0_NS <<
> rgmii_rx_delay_pos;
> > + reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos;
> > if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> > phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> > - reg_val |= RGMII_CLK_DELAY_2_0_NS <<
> rgmii_tx_delay_pos;
> > + reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos;
> >
> > rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> > rgmii_cntl,
> > @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device
> *phydev)
> > {
> > int rc, i, phy_id;
> > struct vsc8531_private *vsc8531 = phydev->priv;
> > + struct device_node *of_node = phydev->mdio.dev.of_node;
> > +
> > + vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
> > + rc = of_property_read_u32(of_node, "mscc,rx-delay",
> > + &vsc8531->rx_delay);
> > +
> > + vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
> > + rc = of_property_read_u32(of_node, "mscc,tx-delay",
> > + &vsc8531->tx_delay);
>
> Since the dt-bindings document says "If this property is present then
> the PHY applies the RX|TX delay", then I guess the precedence as applied
> by vsc85xx_rgmii_set_skews() should be different. The RX delays should
> be applied based on rx-internal-delay-ps (if present) regardless of
> phy-mode, or set to RGMII_CLK_DELAY_2_0_NS if we are in the rgmii-rxid phy_get_internal_delay
> or rgmii-id modes. Similar for tx.
Thanks for the review.
The intention is to have the following precedence (I'll rephrase the commit if required)
-> If phy-mode is rgmii, current behavior persists for all devices
-> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior persists for all devices
(i.e. delay of RGMII_CLK_DELAY_2_0_NS)
-> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND rx-internal-delay-ps/tx-internal-delay-ps
is defined, then the value from DT is considered instead of 2ns. (NOT irrespective of phy-mode)
I'm checking the phy drivers that use phy_get_internal_delay and the description phy-mode
in ethernet-controller.yaml and rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like
the above is allowed. Please do let me know otherwise.
I will re-spin the series using phy_get_internal_delay.
Regards,
Harini
On Wed, Apr 26, 2023 at 04:13:11PM +0530, Harini Katakam wrote:
> All the PHY devices variants specified have the same mask and
> hence can be simplified to one vendor look up for 0xfffffff0.
> Any individual config can be identified by PHY_ID_MATCH_EXACT
> in the respective structure.
>
> Signed-off-by: Harini Katakam <[email protected]>
net-next is closed at the moment, so you will need to report in two
weeks time.
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 62bf99e45af1..75d9582e5784 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -2656,19 +2656,7 @@ static struct phy_driver vsc85xx_driver[] = {
> module_phy_driver(vsc85xx_driver);
>
> static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
> - { PHY_ID_VSC8504, 0xfffffff0, },
> - { PHY_ID_VSC8514, 0xfffffff0, },
> - { PHY_ID_VSC8530, 0xfffffff0, },
> - { PHY_ID_VSC8531, 0xfffffff0, },
> - { PHY_ID_VSC8540, 0xfffffff0, },
> - { PHY_ID_VSC8541, 0xfffffff0, },
> - { PHY_ID_VSC8552, 0xfffffff0, },
> - { PHY_ID_VSC856X, 0xfffffff0, },
> - { PHY_ID_VSC8572, 0xfffffff0, },
> - { PHY_ID_VSC8574, 0xfffffff0, },
> - { PHY_ID_VSC8575, 0xfffffff0, },
> - { PHY_ID_VSC8582, 0xfffffff0, },
> - { PHY_ID_VSC8584, 0xfffffff0, },
> + { PHY_ID_MATCH_VENDOR(0xfffffff0) },
The vendor ID is 0xfffffff0 ???
Andrew
On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> From: Harini Katakam <[email protected]>
>
> Add support for VSC8531_02 (Rev 2) device.
> Add support for optional RGMII RX and TX delay tuning via devicetree.
> The hierarchy is:
> - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> - Retain the default 2ns delay when RGMII tuning is set and DT delay
> property is NOT specified.
tuning is probably the wrong word here. I normally consider tuning as
small changes from 0ns/2ns. The course setting of 0ns or 2ns is not
tuning. I normally use RGMII internal delays to refer to that.
However, i'm not sure there is consistency among drivers.
Andrew
On Wed, Apr 26, 2023 at 04:13:12PM +0530, Harini Katakam wrote:
> From: Harini Katakam <[email protected]>
>
> Add optional properties to tune RGMII RX and TX delay. The current
> default value in the Linux driver, when the phy-mode is rgmii-id,
> is 2ns for both. These properties take priority if specified.
>
> Signed-off-by: Harini Katakam <[email protected]>
> Signed-off-by: Radhey Shyam Pandey <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
Your signed-off-by needs to come last, since you are submitting it.
But as Vladimir pointed out, you don't need to add anything.
You could however convert to yaml, if you want.
Andrew
On Wed, Apr 26, 2023 at 12:21:47PM +0000, Katakam, Harini wrote:
> Thanks for the review.
> The intention is to have the following precedence (I'll rephrase the commit if required)
> -> If phy-mode is rgmii, current behavior persists for all devices
> -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior persists for all devices
> (i.e. delay of RGMII_CLK_DELAY_2_0_NS)
> -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND rx-internal-delay-ps/tx-internal-delay-ps
> is defined, then the value from DT is considered instead of 2ns. (NOT irrespective of phy-mode)
>
> I'm checking the phy drivers that use phy_get_internal_delay and the description phy-mode
> in ethernet-controller.yaml and rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like
> the above is allowed. Please do let me know otherwise.
I understood what your intention was. What I meant was:
phy-mode rgmii rgmii-rxid/rgmii-id
--------------------------------------------------------------------------------------------
rx-internal-delay-ps absent 0.2 ns 2 ns
rx-internal-delay-ps present follow rx-internal-delay-ps follow rx-internal-delay-ps
I agree with Andrew that probably there isn't consistency among PHY
drivers for this interpretation - see dp83822 vs intel-xway for example.
My interpretation was based on the wording from the dt-bindings document,
which seems to suggest that rx-internal-delay-ps takes precedence.
On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> From: Harini Katakam <[email protected]>
>
> Add support for VSC8531_02 (Rev 2) device.
> Add support for optional RGMII RX and TX delay tuning via devicetree.
> The hierarchy is:
> - Retain the defaul 0.2ns delay when RGMII tuning is not set.
nit: s/defaul/default/
...
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Wednesday, April 26, 2023 7:10 PM
> To: Katakam, Harini <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; wsa+renesas@sang-
> engineering.com; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Simek, Michal <[email protected]>;
> Pandey, Radhey Shyam <[email protected]>
> Subject: Re: [PATCH net-next v2 1/3] phy: mscc: Use
> PHY_ID_MATCH_VENDOR to minimize PHY ID table
>
> On Wed, Apr 26, 2023 at 04:13:11PM +0530, Harini Katakam wrote:
> > All the PHY devices variants specified have the same mask and hence
> > can be simplified to one vendor look up for 0xfffffff0.
> > Any individual config can be identified by PHY_ID_MATCH_EXACT in the
> > respective structure.
> >
> > Signed-off-by: Harini Katakam <[email protected]>
>
<snip>
> > + { PHY_ID_MATCH_VENDOR(0xfffffff0) },
>
> The vendor ID is 0xfffffff0 ???
Sorry this is supposed to be 0x00070400 (Microsemi OUI).
Will fix in the next version.
Regards,
Harini
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Wednesday, April 26, 2023 7:18 PM
> To: Katakam, Harini <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; wsa+renesas@sang-
> engineering.com; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Simek, Michal <[email protected]>;
> Pandey, Radhey Shyam <[email protected]>
> Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
> with RGMII tuning
>
> On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> > From: Harini Katakam <[email protected]>
> >
> > Add support for VSC8531_02 (Rev 2) device.
> > Add support for optional RGMII RX and TX delay tuning via devicetree.
> > The hierarchy is:
> > - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> > - Retain the default 2ns delay when RGMII tuning is set and DT delay
> > property is NOT specified.
>
> tuning is probably the wrong word here. I normally consider tuning as small
> changes from 0ns/2ns. The course setting of 0ns or 2ns is not tuning. I
> normally use RGMII internal delays to refer to that.
>
> However, i'm not sure there is consistency among drivers.
Thanks for the input, I understand. I'm planning on re-phrasing this
with phy-mode and property values to describe rather than say tuning.
Regards,
Harini
Hi Vladimir,
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: Wednesday, April 26, 2023 7:49 PM
> To: Katakam, Harini <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; wsa+renesas@sang-
> engineering.com; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Simek, Michal <[email protected]>;
> Pandey, Radhey Shyam <[email protected]>
> Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
> with RGMII tuning
>
> On Wed, Apr 26, 2023 at 12:21:47PM +0000, Katakam, Harini wrote:
> > Thanks for the review.
> > The intention is to have the following precedence (I'll rephrase the
> > commit if required)
> > -> If phy-mode is rgmii, current behavior persists for all devices If
> > -> phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior
> > -> persists for all devices
> > (i.e. delay of RGMII_CLK_DELAY_2_0_NS)
> > -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND
> > -> rx-internal-delay-ps/tx-internal-delay-ps
> > is defined, then the value from DT is considered instead of 2ns. (NOT
> > irrespective of phy-mode)
> >
> > I'm checking the phy drivers that use phy_get_internal_delay and the
> > description phy-mode in ethernet-controller.yaml and
> > rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like the above is
> allowed. Please do let me know otherwise.
>
> I understood what your intention was. What I meant was:
>
> phy-mode rgmii rgmii-rxid/rgmii-id
> --------------------------------------------------------------------------------------------
> rx-internal-delay-ps absent 0.2 ns 2 ns
> rx-internal-delay-ps present follow rx-internal-delay-ps follow rx-internal-
> delay-ps
>
> I agree with Andrew that probably there isn't consistency among PHY drivers
> for this interpretation - see dp83822 vs intel-xway for example.
Thanks, yes I noticed the difference here and also in older PHY drivers that
used custom properties (see dp83867 which is what I originally aligned it to).
But the table you mentioned above makes sense; I'll re-spin accordingly.
> My interpretation was based on the wording from the dt-bindings document,
> which seems to suggest that rx-internal-delay-ps takes precedence.
OK I understand.
Regards,
Harini