Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Andrew Lunn]

> +/* map from SD25G28 interface width to configuration value */
> +static u8 sd25g28_get_iw_setting(const u8 interface_width)
> +{
> +	switch (interface_width) {
> +	case 10: return 0;
> +	case 16: return 1;
> +	case 32: return 3;
> +	case 40: return 4;
> +	case 64: return 5;
> +	default:
> +		pr_err("%s: Illegal value %d for interface width\n",
> +		       __func__, interface_width);

Please make use of dev_err(phy->dev, so we know which PHY has
configuration problems.

> +static int sparx5_serdes_validate(struct phy *phy, enum phy_mode mode,
> +					int submode,
> +					union phy_configure_opts *opts)
> +{
> +	struct sparx5_serdes_macro *macro = phy_get_drvdata(phy);
> +	struct sparx5_serdes_private *priv = macro->priv;
> +	u32 value, analog_sd;
> +
> +	if (mode != PHY_MODE_ETHERNET)
> +		return -EINVAL;
> +
> +	switch (submode) {
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_QSGMII:
> +	case PHY_INTERFACE_MODE_10GBASER:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	if (macro->serdestype == SPX5_SDT_6G) {
> +		value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx));
> +		analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
> +	} else if (macro->serdestype == SPX5_SDT_10G) {
> +		value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx));
> +		analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
> +	} else {
> +		value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx));
> +		analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value);
> +	}
> +	/* Link up is when analog_sd == 0 */
> +	return analog_sd;
> +}

What i have not yet seen is how this code plugs together with
phylink_pcs_ops?

Can this hardware also be used for SATA, USB? As far as i understand,
the Marvell Comphy is multi-purpose, it is used for networking, USB,
and SATA, etc. Making it a generic PHY then makes sense, because
different subsystems need to use it.

But it looks like this is for networking only? So i'm wondering if it
belongs in driver/net/pcs and it should be accessed using
phylink_pcs_ops?

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Russell King - ARM Linux admin]

On Thu, Dec 03, 2020 at 10:52:53PM +0100, Andrew Lunn wrote:
> > +/* map from SD25G28 interface width to configuration value */
> > +static u8 sd25g28_get_iw_setting(const u8 interface_width)
> > +{
> > +	switch (interface_width) {
> > +	case 10: return 0;
> > +	case 16: return 1;
> > +	case 32: return 3;
> > +	case 40: return 4;
> > +	case 64: return 5;
> > +	default:
> > +		pr_err("%s: Illegal value %d for interface width\n",
> > +		       __func__, interface_width);
> 
> Please make use of dev_err(phy->dev, so we know which PHY has
> configuration problems.
> 
> > +static int sparx5_serdes_validate(struct phy *phy, enum phy_mode mode,
> > +					int submode,
> > +					union phy_configure_opts *opts)
> > +{
> > +	struct sparx5_serdes_macro *macro = phy_get_drvdata(phy);
> > +	struct sparx5_serdes_private *priv = macro->priv;
> > +	u32 value, analog_sd;
> > +
> > +	if (mode != PHY_MODE_ETHERNET)
> > +		return -EINVAL;
> > +
> > +	switch (submode) {
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +	case PHY_INTERFACE_MODE_10GBASER:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	if (macro->serdestype == SPX5_SDT_6G) {
> > +		value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx));
> > +		analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
> > +	} else if (macro->serdestype == SPX5_SDT_10G) {
> > +		value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx));
> > +		analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
> > +	} else {
> > +		value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx));
> > +		analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value);
> > +	}
> > +	/* Link up is when analog_sd == 0 */
> > +	return analog_sd;
> > +}

You still have not Cc'd me on your patches. Please can you either:

1) use get_maintainer.pl to find out whom you should be sending
   your patches to
or
2) include me in your cc for this patch set as phylink maintainer in
   your patch set so I can review your use of phylink.

Consider your patches NAK'd until you send them to me so that I can
review them.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Alexandre Belloni]

Hi Russell,

On 03/12/2020 22:52:33+0000, Russell King - ARM Linux admin wrote:
> You still have not Cc'd me on your patches. Please can you either:
> 
> 1) use get_maintainer.pl to find out whom you should be sending
>    your patches to
> or
> 2) include me in your cc for this patch set as phylink maintainer in
>    your patch set so I can review your use of phylink.
> 

Note that this series is different from the network (switchdev) driver
series and doesn't make use of phylink.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Russell King - ARM Linux admin]

On Fri, Dec 04, 2020 at 08:56:33AM +0100, Alexandre Belloni wrote:
> Hi Russell,
> 
> On 03/12/2020 22:52:33+0000, Russell King - ARM Linux admin wrote:
> > You still have not Cc'd me on your patches. Please can you either:
> > 
> > 1) use get_maintainer.pl to find out whom you should be sending
> >    your patches to
> > or
> > 2) include me in your cc for this patch set as phylink maintainer in
> >    your patch set so I can review your use of phylink.
> > 
> 
> Note that this series is different from the network (switchdev) driver
> series and doesn't make use of phylink.

Oh, didn't realise that; it's difficult to tell when you don't get the
original messages, and only end up being Cc'd on a very trimmed down
reply.

Email is a broken form of communication...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Steen Hegelund]

On 03.12.2020 22:52, Russell King - ARM Linux admin wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Thu, Dec 03, 2020 at 10:52:53PM +0100, Andrew Lunn wrote:
>> > +/* map from SD25G28 interface width to configuration value */
>> > +static u8 sd25g28_get_iw_setting(const u8 interface_width)
>> > +{
>> > +   switch (interface_width) {
>> > +   case 10: return 0;
>> > +   case 16: return 1;
>> > +   case 32: return 3;
>> > +   case 40: return 4;
>> > +   case 64: return 5;
>> > +   default:
>> > +           pr_err("%s: Illegal value %d for interface width\n",
>> > +                  __func__, interface_width);
>>
>> Please make use of dev_err(phy->dev, so we know which PHY has
>> configuration problems.
>>
>> > +static int sparx5_serdes_validate(struct phy *phy, enum phy_mode mode,
>> > +                                   int submode,
>> > +                                   union phy_configure_opts *opts)
>> > +{
>> > +   struct sparx5_serdes_macro *macro = phy_get_drvdata(phy);
>> > +   struct sparx5_serdes_private *priv = macro->priv;
>> > +   u32 value, analog_sd;
>> > +
>> > +   if (mode != PHY_MODE_ETHERNET)
>> > +           return -EINVAL;
>> > +
>> > +   switch (submode) {
>> > +   case PHY_INTERFACE_MODE_1000BASEX:
>> > +   case PHY_INTERFACE_MODE_SGMII:
>> > +   case PHY_INTERFACE_MODE_QSGMII:
>> > +   case PHY_INTERFACE_MODE_10GBASER:
>> > +           break;
>> > +   default:
>> > +           return -EINVAL;
>> > +   }
>> > +   if (macro->serdestype == SPX5_SDT_6G) {
>> > +           value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx));
>> > +           analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> > +   } else if (macro->serdestype == SPX5_SDT_10G) {
>> > +           value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx));
>> > +           analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> > +   } else {
>> > +           value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx));
>> > +           analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value);
>> > +   }
>> > +   /* Link up is when analog_sd == 0 */
>> > +   return analog_sd;
>> > +}
>
>You still have not Cc'd me on your patches. Please can you either:
>
>1) use get_maintainer.pl to find out whom you should be sending
>   your patches to
>or
>2) include me in your cc for this patch set as phylink maintainer in
>   your patch set so I can review your use of phylink.
>
>Consider your patches NAK'd until you send them to me so that I can
>review them.
>
>Thanks.

Hi Russell,

I will CC you on the next version of this series.  In reality I was
just asked by Vladimir Oltean to crosspost to netdev.  This is a
series for the Generic PHY subsystem.
Sorry about the confusion.
>
>--
>RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

BR
Steen

---------------------------------------
Steen Hegelund
steen.hegelund@microchip.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Steen Hegelund]

On 03.12.2020 22:52, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +/* map from SD25G28 interface width to configuration value */
>> +static u8 sd25g28_get_iw_setting(const u8 interface_width)
>> +{
>> +     switch (interface_width) {
>> +     case 10: return 0;
>> +     case 16: return 1;
>> +     case 32: return 3;
>> +     case 40: return 4;
>> +     case 64: return 5;
>> +     default:
>> +             pr_err("%s: Illegal value %d for interface width\n",
>> +                    __func__, interface_width);
>
>Please make use of dev_err(phy->dev, so we know which PHY has
>configuration problems.

I will update that.

>
>> +static int sparx5_serdes_validate(struct phy *phy, enum phy_mode mode,
>> +                                     int submode,
>> +                                     union phy_configure_opts *opts)
>> +{
>> +     struct sparx5_serdes_macro *macro = phy_get_drvdata(phy);
>> +     struct sparx5_serdes_private *priv = macro->priv;
>> +     u32 value, analog_sd;
>> +
>> +     if (mode != PHY_MODE_ETHERNET)
>> +             return -EINVAL;
>> +
>> +     switch (submode) {
>> +     case PHY_INTERFACE_MODE_1000BASEX:
>> +     case PHY_INTERFACE_MODE_SGMII:
>> +     case PHY_INTERFACE_MODE_QSGMII:
>> +     case PHY_INTERFACE_MODE_10GBASER:
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +     if (macro->serdestype == SPX5_SDT_6G) {
>> +             value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx));
>> +             analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> +     } else if (macro->serdestype == SPX5_SDT_10G) {
>> +             value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx));
>> +             analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> +     } else {
>> +             value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx));
>> +             analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value);
>> +     }
>> +     /* Link up is when analog_sd == 0 */
>> +     return analog_sd;
>> +}
>
>What i have not yet seen is how this code plugs together with
>phylink_pcs_ops?
>
>Can this hardware also be used for SATA, USB? As far as i understand,
>the Marvell Comphy is multi-purpose, it is used for networking, USB,
>and SATA, etc. Making it a generic PHY then makes sense, because
>different subsystems need to use it.
>
>But it looks like this is for networking only? So i'm wondering if it
>belongs in driver/net/pcs and it should be accessed using
>phylink_pcs_ops?
>
>        Andrew

This is a PHY that communicates on a SerDes link to an ethernet PHY or a
SFP. So I took the lead from earlier work: the Microsemi Ocelot SerDes driver,
and added the Sparx5 SerDes PHY driver here since it is very similar in intent.
It is not an ethernet PHY as such.

BR
Steen

---------------------------------------
Steen Hegelund
steen.hegelund@microchip.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Russell King - ARM Linux admin]

On Fri, Dec 04, 2020 at 02:48:26PM +0100, Steen Hegelund wrote:
> On 03.12.2020 22:52, Andrew Lunn wrote:
> > What i have not yet seen is how this code plugs together with
> > phylink_pcs_ops?
> > 
> > Can this hardware also be used for SATA, USB? As far as i understand,
> > the Marvell Comphy is multi-purpose, it is used for networking, USB,
> > and SATA, etc. Making it a generic PHY then makes sense, because
> > different subsystems need to use it.
> > 
> > But it looks like this is for networking only? So i'm wondering if it
> > belongs in driver/net/pcs and it should be accessed using
> > phylink_pcs_ops?
> > 
> >        Andrew
> 
> This is a PHY that communicates on a SerDes link to an ethernet PHY or a
> SFP. So I took the lead from earlier work: the Microsemi Ocelot SerDes driver,
> and added the Sparx5 SerDes PHY driver here since it is very similar in intent.
> It is not an ethernet PHY as such.

Okay, that is the normal situation in real hardware:

MAC <---> PCS PHY <---> SerDes PHY <---> SerDes lanes

where the PCS PHY handles the protocol level, and the SerDes PHY handles
the clocking and electrical characteristics of the SerDes lanes.

Maybe we should ask for a diagram of the setups when new support is
submitted to make the process of understanding the hardware easier?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Alexandre Belloni]

On 03/12/2020 22:52:53+0100, Andrew Lunn wrote:
> > +	if (macro->serdestype == SPX5_SDT_6G) {
> > +		value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx));
> > +		analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
> > +	} else if (macro->serdestype == SPX5_SDT_10G) {
> > +		value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx));
> > +		analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
> > +	} else {
> > +		value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx));
> > +		analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value);
> > +	}
> > +	/* Link up is when analog_sd == 0 */
> > +	return analog_sd;
> > +}
> 
> What i have not yet seen is how this code plugs together with
> phylink_pcs_ops?
> 
> Can this hardware also be used for SATA, USB? As far as i understand,
> the Marvell Comphy is multi-purpose, it is used for networking, USB,
> and SATA, etc. Making it a generic PHY then makes sense, because
> different subsystems need to use it.
> 
> But it looks like this is for networking only? So i'm wondering if it
> belongs in driver/net/pcs and it should be accessed using
> phylink_pcs_ops?
> 

Ocelot had PCie on the phys, doesn't Sparx5 have it?

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver

[Steen Hegelund]

On 04.12.2020 15:16, Alexandre Belloni wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On 03/12/2020 22:52:53+0100, Andrew Lunn wrote:
>> > +   if (macro->serdestype == SPX5_SDT_6G) {
>> > +           value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx));
>> > +           analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> > +   } else if (macro->serdestype == SPX5_SDT_10G) {
>> > +           value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx));
>> > +           analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value);
>> > +   } else {
>> > +           value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx));
>> > +           analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value);
>> > +   }
>> > +   /* Link up is when analog_sd == 0 */
>> > +   return analog_sd;
>> > +}
>>
>> What i have not yet seen is how this code plugs together with
>> phylink_pcs_ops?
>>
>> Can this hardware also be used for SATA, USB? As far as i understand,
>> the Marvell Comphy is multi-purpose, it is used for networking, USB,
>> and SATA, etc. Making it a generic PHY then makes sense, because
>> different subsystems need to use it.
>>
>> But it looks like this is for networking only? So i'm wondering if it
>> belongs in driver/net/pcs and it should be accessed using
>> phylink_pcs_ops?
>>
>
>Ocelot had PCie on the phys, doesn't Sparx5 have it?

Yes Ocelot has that, but on Sparx5 the PCIe is separate...

>
>--
>Alexandre Belloni, Bootlin
>Embedded Linux and Kernel engineering
>https://bootlin.com

BR
Steen

---------------------------------------
Steen Hegelund
steen.hegelund@microchip.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel