* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-03 21:52 ` [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver Andrew Lunn
@ 2020-12-03 22:52 ` Russell King - ARM Linux admin
2020-12-04 7:56 ` Alexandre Belloni
2020-12-04 13:51 ` Steen Hegelund
2020-12-04 13:48 ` Steen Hegelund
2020-12-04 14:16 ` Alexandre Belloni
2 siblings, 2 replies; 9+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-03 22:52 UTC (permalink / raw)
To: Steen Hegelund
Cc: Andrew Lunn, Alexandre Belloni, Bjarni Jonasson, netdev,
linux-kernel, Kishon Vijay Abraham I, Vinod Koul,
linux-arm-kernel, Microchip UNG Driver List, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-03 22:52 ` Russell King - ARM Linux admin
@ 2020-12-04 7:56 ` Alexandre Belloni
2020-12-04 10:20 ` Russell King - ARM Linux admin
2020-12-04 13:51 ` Steen Hegelund
1 sibling, 1 reply; 9+ messages in thread
From: Alexandre Belloni @ 2020-12-04 7:56 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Bjarni Jonasson, netdev, Steen Hegelund,
linux-kernel, Kishon Vijay Abraham I, Vinod Koul,
linux-arm-kernel, Microchip UNG Driver List, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-04 7:56 ` Alexandre Belloni
@ 2020-12-04 10:20 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 9+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-04 10:20 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Andrew Lunn, Bjarni Jonasson, netdev, Steen Hegelund,
linux-kernel, Kishon Vijay Abraham I, Vinod Koul,
linux-arm-kernel, Microchip UNG Driver List, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-03 22:52 ` Russell King - ARM Linux admin
2020-12-04 7:56 ` Alexandre Belloni
@ 2020-12-04 13:51 ` Steen Hegelund
1 sibling, 0 replies; 9+ messages in thread
From: Steen Hegelund @ 2020-12-04 13:51 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Alexandre Belloni, Bjarni Jonasson, netdev,
linux-kernel, Kishon Vijay Abraham I, Vinod Koul,
linux-arm-kernel, Microchip UNG Driver List, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-03 21:52 ` [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver Andrew Lunn
2020-12-03 22:52 ` Russell King - ARM Linux admin
@ 2020-12-04 13:48 ` Steen Hegelund
2020-12-04 13:55 ` Russell King - ARM Linux admin
2020-12-04 14:16 ` Alexandre Belloni
2 siblings, 1 reply; 9+ messages in thread
From: Steen Hegelund @ 2020-12-04 13:48 UTC (permalink / raw)
To: Andrew Lunn
Cc: Bjarni Jonasson, Alexandre Belloni, netdev, linux-kernel,
Kishon Vijay Abraham I, Vinod Koul, linux-arm-kernel,
Microchip UNG Driver List, Russell King, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-04 13:48 ` Steen Hegelund
@ 2020-12-04 13:55 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 9+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-04 13:55 UTC (permalink / raw)
To: Steen Hegelund
Cc: Andrew Lunn, Alexandre Belloni, Bjarni Jonasson, netdev,
linux-kernel, Kishon Vijay Abraham I, Vinod Koul,
linux-arm-kernel, Microchip UNG Driver List, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-03 21:52 ` [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver Andrew Lunn
2020-12-03 22:52 ` Russell King - ARM Linux admin
2020-12-04 13:48 ` Steen Hegelund
@ 2020-12-04 14:16 ` Alexandre Belloni
2020-12-07 8:13 ` Steen Hegelund
2 siblings, 1 reply; 9+ messages in thread
From: Alexandre Belloni @ 2020-12-04 14:16 UTC (permalink / raw)
To: Andrew Lunn
Cc: Bjarni Jonasson, netdev, Steen Hegelund, linux-kernel,
Kishon Vijay Abraham I, Vinod Koul, linux-arm-kernel,
Microchip UNG Driver List, Russell King, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v8 3/4] phy: Add Sparx5 ethernet serdes PHY driver
2020-12-04 14:16 ` Alexandre Belloni
@ 2020-12-07 8:13 ` Steen Hegelund
0 siblings, 0 replies; 9+ messages in thread
From: Steen Hegelund @ 2020-12-07 8:13 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Bjarni Jonasson, Andrew Lunn, netdev, linux-kernel,
Kishon Vijay Abraham I, Vinod Koul, linux-arm-kernel,
Microchip UNG Driver List, Russell King, Lars Povlsen
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
^ permalink raw reply [flat|nested] 9+ messages in thread