diff mbox series

[v1,net-next] mscc.c: Add support for additional VSC PHYs

Message ID 1573574048-12251-1-git-send-email-Bryan.Whitehead@microchip.com
State Changes Requested
Delegated to: David Miller
Headers show
Series [v1,net-next] mscc.c: Add support for additional VSC PHYs | expand

Commit Message

Bryan Whitehead Nov. 12, 2019, 3:54 p.m. UTC 
Add support for the following VSC PHYs
	VSC8504, VSC8552, VSC8572,
	VSC8562, VSC8564, VSC8575, VSC8582

Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
---
 drivers/net/phy/mscc.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)

Comments

David Miller Nov. 12, 2019, 8:25 p.m. UTC | #1 
From: Bryan Whitehead <Bryan.Whitehead@microchip.com>
Date: Tue, 12 Nov 2019 10:54:08 -0500

>  		phy = container_of(map[addr], struct phy_device, mdio);

Unnecessary space added between this assignment and the test.

> +		if (!phy)
> +			continue;

And this test makes no sense, the result of container_of() is never NULL
unless it is for the first member of a structure of a NULL pointer.
Andrew Lunn Nov. 12, 2019, 8:40 p.m. UTC | #2 
On Tue, Nov 12, 2019 at 10:54:08AM -0500, Bryan Whitehead wrote:
> Add support for the following VSC PHYs
> 	VSC8504, VSC8552, VSC8572,
> 	VSC8562, VSC8564, VSC8575, VSC8582
> 
> Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
> ---
>  drivers/net/phy/mscc.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 182 insertions(+)
> 
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index 805cda3..8933681 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -253,12 +253,18 @@ enum rgmii_rx_clock_delay {
>  #define MSCC_PHY_TR_MSB			  18
>  
>  /* Microsemi PHY ID's */
> +#define PHY_ID_VSC8504			  0x000704c0
>  #define PHY_ID_VSC8514			  0x00070670
>  #define PHY_ID_VSC8530			  0x00070560
>  #define PHY_ID_VSC8531			  0x00070570
>  #define PHY_ID_VSC8540			  0x00070760
>  #define PHY_ID_VSC8541			  0x00070770
> +#define PHY_ID_VSC8552			  0x000704e0
> +#define PHY_ID_VSC856X			  0x000707e0
> +#define PHY_ID_VSC8572			  0x000704d0
>  #define PHY_ID_VSC8574			  0x000704a0
> +#define PHY_ID_VSC8575			  0x000707d0
> +#define PHY_ID_VSC8582			  0x000707b0
>  #define PHY_ID_VSC8584			  0x000707c0
>  
>  #define MSCC_VDDMAC_1500		  1500
> @@ -1597,6 +1603,8 @@ static bool vsc8584_is_pkg_init(struct phy_device *phydev, bool reversed)
>  
>  		phy = container_of(map[addr], struct phy_device, mdio);
>  
> +		if (!phy)
> +			continue;
> +
>  		if ((phy->phy_id & phydev->drv->phy_id_mask) !=
>  		    (phydev->drv->phy_id & phydev->drv->phy_id_mask))
>  			continue;
> @@ -1648,9 +1656,27 @@ static int vsc8584_config_init(struct phy_device *phydev)
>  	 */
>  	if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0)) {
>  		if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
> +		    (PHY_ID_VSC8504 & phydev->drv->phy_id_mask))
> +			ret = vsc8574_config_pre_init(phydev);
> +		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
> +		    (PHY_ID_VSC8552 & phydev->drv->phy_id_mask))
> +			ret = vsc8574_config_pre_init(phydev);
> +		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
> +		    (PHY_ID_VSC856X & phydev->drv->phy_id_mask))
> +			ret = vsc8584_config_pre_init(phydev);

Could we turn this into a switch statement? I think

      switch (phydev->phy_id & phydev->drv->phy_id_mask) {
      case PHY_ID_VSC8504:
      case PHY_ID_VSC8552:
      	   ret = vsc8574_config_pre_init(phydev);
	   break
      case PHY_ID_VSC856X:
      	   ret = vsc8584_config_pre_init(phydev);
	   break;

etc should work, since PHY_ID_VSC8<FOO> always has the lower nibble set
to 0.

   Andrew
Bryan Whitehead Nov. 12, 2019, 8:43 p.m. UTC | #3 
> From: Bryan Whitehead <Bryan.Whitehead@microchip.com>
> Date: Tue, 12 Nov 2019 10:54:08 -0500
> 
> >  		phy = container_of(map[addr], struct phy_device, mdio);
> 
> Unnecessary space added between this assignment and the test.
> 
> > +		if (!phy)
> > +			continue;
> 
> And this test makes no sense, the result of container_of() is never NULL
> unless it is for the first member of a structure of a NULL pointer.

Hi David,

I do have a NULL case, so I can check map[addr] for NULL instead.

Thanks,
Bryan
Bryan Whitehead Nov. 12, 2019, 8:56 p.m. UTC | #4 
> On Tue, Nov 12, 2019 at 10:54:08AM -0500, Bryan Whitehead wrote:
> > Add support for the following VSC PHYs
> > 	VSC8504, VSC8552, VSC8572,
> > 	VSC8562, VSC8564, VSC8575, VSC8582
> >
> > Signed-off-by: Bryan Whitehead <Bryan.Whitehead@microchip.com>
> > ---
> >  drivers/net/phy/mscc.c | 182
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 182 insertions(+)
> >
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c index
> > 805cda3..8933681 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -253,12 +253,18 @@ enum rgmii_rx_clock_delay {
> >  #define MSCC_PHY_TR_MSB			  18
> >
> >  /* Microsemi PHY ID's */
> > +#define PHY_ID_VSC8504			  0x000704c0
> >  #define PHY_ID_VSC8514			  0x00070670
> >  #define PHY_ID_VSC8530			  0x00070560
> >  #define PHY_ID_VSC8531			  0x00070570
> >  #define PHY_ID_VSC8540			  0x00070760
> >  #define PHY_ID_VSC8541			  0x00070770
> > +#define PHY_ID_VSC8552			  0x000704e0
> > +#define PHY_ID_VSC856X			  0x000707e0
> > +#define PHY_ID_VSC8572			  0x000704d0
> >  #define PHY_ID_VSC8574			  0x000704a0
> > +#define PHY_ID_VSC8575			  0x000707d0
> > +#define PHY_ID_VSC8582			  0x000707b0
> >  #define PHY_ID_VSC8584			  0x000707c0
> >
> >  #define MSCC_VDDMAC_1500		  1500
> > @@ -1597,6 +1603,8 @@ static bool vsc8584_is_pkg_init(struct
> > phy_device *phydev, bool reversed)
> >
> >  		phy = container_of(map[addr], struct phy_device, mdio);
> >
> > +		if (!phy)
> > +			continue;
> > +
> >  		if ((phy->phy_id & phydev->drv->phy_id_mask) !=
> >  		    (phydev->drv->phy_id & phydev->drv->phy_id_mask))
> >  			continue;
> > @@ -1648,9 +1656,27 @@ static int vsc8584_config_init(struct phy_device
> *phydev)
> >  	 */
> >  	if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0))
> {
> >  		if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
> > +		    (PHY_ID_VSC8504 & phydev->drv->phy_id_mask))
> > +			ret = vsc8574_config_pre_init(phydev);
> > +		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
> > +		    (PHY_ID_VSC8552 & phydev->drv->phy_id_mask))
> > +			ret = vsc8574_config_pre_init(phydev);
> > +		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
> > +		    (PHY_ID_VSC856X & phydev->drv->phy_id_mask))
> > +			ret = vsc8584_config_pre_init(phydev);
> 
> Could we turn this into a switch statement? I think
> 
>       switch (phydev->phy_id & phydev->drv->phy_id_mask) {
>       case PHY_ID_VSC8504:
>       case PHY_ID_VSC8552:
>       	   ret = vsc8574_config_pre_init(phydev);
> 	   break
>       case PHY_ID_VSC856X:
>       	   ret = vsc8584_config_pre_init(phydev);
> 	   break;
> 
> etc should work, since PHY_ID_VSC8<FOO> always has the lower nibble set
> to 0.

Hi Andrew,

I would like to do exactly that, but I was concerned future changes might change the phy_id_mask, so to keep code less brittle, and more flexible I thought I should keep the "AND mask" operations such as (PHY_ID_VSC8<FOO> & phydev->drv->phy_id_mask)

If you judge this is an unreasonable concern, then I will change it to a switch statement.
Let me know,
Thanks,
Bryan
Andrew Lunn Nov. 13, 2019, 3:40 p.m. UTC | #5 
> Hi Andrew,
> 
> I would like to do exactly that, but I was concerned future changes might change the phy_id_mask, so to keep code less brittle, and more flexible I thought I should keep the "AND mask" operations such as (PHY_ID_VSC8<FOO> & phydev->drv->phy_id_mask)

Hi Bryan

You could add a WARN_ON(phydev->drv->phy_id_mask & 0xf); That should
catch any new PHY breaking the assumption.

    Andrew
Bryan Whitehead Nov. 13, 2019, 3:58 p.m. UTC | #6 
> Hi Bryan
> 
> You could add a WARN_ON(phydev->drv->phy_id_mask & 0xf); That should
> catch any new PHY breaking the assumption.
> 
>     Andrew

Good idea, Thanks Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 805cda3..8933681 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -253,12 +253,18 @@  enum rgmii_rx_clock_delay {
 #define MSCC_PHY_TR_MSB			  18
 
 /* Microsemi PHY ID's */
+#define PHY_ID_VSC8504			  0x000704c0
 #define PHY_ID_VSC8514			  0x00070670
 #define PHY_ID_VSC8530			  0x00070560
 #define PHY_ID_VSC8531			  0x00070570
 #define PHY_ID_VSC8540			  0x00070760
 #define PHY_ID_VSC8541			  0x00070770
+#define PHY_ID_VSC8552			  0x000704e0
+#define PHY_ID_VSC856X			  0x000707e0
+#define PHY_ID_VSC8572			  0x000704d0
 #define PHY_ID_VSC8574			  0x000704a0
+#define PHY_ID_VSC8575			  0x000707d0
+#define PHY_ID_VSC8582			  0x000707b0
 #define PHY_ID_VSC8584			  0x000707c0
 
 #define MSCC_VDDMAC_1500		  1500
@@ -1597,6 +1603,8 @@  static bool vsc8584_is_pkg_init(struct phy_device *phydev, bool reversed)
 
 		phy = container_of(map[addr], struct phy_device, mdio);
 
+		if (!phy)
+			continue;
+
 		if ((phy->phy_id & phydev->drv->phy_id_mask) !=
 		    (phydev->drv->phy_id & phydev->drv->phy_id_mask))
 			continue;
@@ -1648,9 +1656,27 @@  static int vsc8584_config_init(struct phy_device *phydev)
 	 */
 	if (!vsc8584_is_pkg_init(phydev, val & PHY_ADDR_REVERSED ? 1 : 0)) {
 		if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
+		    (PHY_ID_VSC8504 & phydev->drv->phy_id_mask))
+			ret = vsc8574_config_pre_init(phydev);
+		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
+		    (PHY_ID_VSC8552 & phydev->drv->phy_id_mask))
+			ret = vsc8574_config_pre_init(phydev);
+		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
+		    (PHY_ID_VSC856X & phydev->drv->phy_id_mask))
+			ret = vsc8584_config_pre_init(phydev);
+		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
+		    (PHY_ID_VSC8572 & phydev->drv->phy_id_mask))
+			ret = vsc8574_config_pre_init(phydev);
+		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
 		    (PHY_ID_VSC8574 & phydev->drv->phy_id_mask))
 			ret = vsc8574_config_pre_init(phydev);
 		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
+		    (PHY_ID_VSC8575 & phydev->drv->phy_id_mask))
+			ret = vsc8584_config_pre_init(phydev);
+		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
+		    (PHY_ID_VSC8582 & phydev->drv->phy_id_mask))
+			ret = vsc8584_config_pre_init(phydev);
+		else if ((phydev->phy_id & phydev->drv->phy_id_mask) ==
 			 (PHY_ID_VSC8584 & phydev->drv->phy_id_mask))
 			ret = vsc8584_config_pre_init(phydev);
 		else
@@ -2322,6 +2348,32 @@  static int vsc85xx_probe(struct phy_device *phydev)
 /* Microsemi VSC85xx PHYs */
 static struct phy_driver vsc85xx_driver[] = {
 {
+	.phy_id		= PHY_ID_VSC8504,
+	.name		= "Microsemi GE VSC8504 SyncE",
+	.phy_id_mask	= 0xfffffff0,
+	/* PHY_GBIT_FEATURES */
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc8584_config_init,
+	.config_aneg    = &vsc85xx_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status	= &vsc85xx_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.did_interrupt  = &vsc8584_did_interrupt,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
+	.probe		= &vsc8574_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_VSC8514,
 	.name		= "Microsemi GE VSC8514 SyncE",
 	.phy_id_mask	= 0xfffffff0,
@@ -2445,6 +2497,82 @@  static struct phy_driver vsc85xx_driver[] = {
 	.get_stats      = &vsc85xx_get_stats,
 },
 {
+	.phy_id		= PHY_ID_VSC8552,
+	.name		= "Microsemi GE VSC8552 SyncE",
+	.phy_id_mask	= 0xfffffff0,
+	/* PHY_GBIT_FEATURES */
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc8584_config_init,
+	.config_aneg    = &vsc85xx_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status	= &vsc85xx_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.did_interrupt  = &vsc8584_did_interrupt,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
+	.probe		= &vsc8574_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_VSC856X,
+	.name		= "Microsemi GE VSC856X SyncE",
+	.phy_id_mask	= 0xfffffff0,
+	/* PHY_GBIT_FEATURES */
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc8584_config_init,
+	.config_aneg    = &vsc85xx_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status	= &vsc85xx_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.did_interrupt  = &vsc8584_did_interrupt,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
+	.probe		= &vsc8584_probe,
+	.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_VSC8572,
+	.name		= "Microsemi GE VSC8572 SyncE",
+	.phy_id_mask	= 0xfffffff0,
+	/* PHY_GBIT_FEATURES */
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc8584_config_init,
+	.config_aneg    = &vsc85xx_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status	= &vsc85xx_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.did_interrupt  = &vsc8584_did_interrupt,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
+	.probe		= &vsc8574_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_VSC8574,
 	.name		= "Microsemi GE VSC8574 SyncE",
 	.phy_id_mask	= 0xfffffff0,
@@ -2471,6 +2599,54 @@  static struct phy_driver vsc85xx_driver[] = {
 	.get_stats      = &vsc85xx_get_stats,
 },
 {
+	.phy_id		= PHY_ID_VSC8575,
+	.name		= "Microsemi GE VSC8575 SyncE",
+	.phy_id_mask	= 0xfffffff0,
+	/* PHY_GBIT_FEATURES */
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc8584_config_init,
+	.config_aneg    = &vsc85xx_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status	= &vsc85xx_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.did_interrupt  = &vsc8584_did_interrupt,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
+	.probe		= &vsc8584_probe,
+	.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_VSC8582,
+	.name		= "Microsemi GE VSC8582 SyncE",
+	.phy_id_mask	= 0xfffffff0,
+	/* PHY_GBIT_FEATURES */
+	.soft_reset	= &genphy_soft_reset,
+	.config_init    = &vsc8584_config_init,
+	.config_aneg    = &vsc85xx_config_aneg,
+	.aneg_done	= &genphy_aneg_done,
+	.read_status	= &vsc85xx_read_status,
+	.ack_interrupt  = &vsc85xx_ack_interrupt,
+	.config_intr    = &vsc85xx_config_intr,
+	.did_interrupt  = &vsc8584_did_interrupt,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
+	.probe		= &vsc8584_probe,
+	.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_VSC8584,
 	.name		= "Microsemi GE VSC8584 SyncE",
 	.phy_id_mask	= 0xfffffff0,
@@ -2500,12 +2676,18 @@  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, },
 	{ }
 };