diff mbox series

[net-next] net: dpaa2-mac: add support for more ethtool 10G link modes

Message ID E1m5mVT-00032g-Km@rmk-PC.armlinux.org.uk (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: dpaa2-mac: add support for more ethtool 10G link modes | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 1 maintainers not CCed: linux@armlinux.org.uk
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Russell King (Oracle) July 20, 2021, 9:57 a.m. UTC
Phylink documentation says:
  Note that the PHY may be able to transform from one connection
  technology to another, so, eg, don't clear 1000BaseX just
  because the MAC is unable to BaseX mode. This is more about
  clearing unsupported speeds and duplex settings. The port modes
  should not be cleared; phylink_set_port_modes() will help with this.

So add the missing 10G modes.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Acked-by: Marek Behún <kabel@kernel.org>
Acked-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andrew Lunn July 20, 2021, 1:51 p.m. UTC | #1
On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> Phylink documentation says:
>   Note that the PHY may be able to transform from one connection
>   technology to another, so, eg, don't clear 1000BaseX just
>   because the MAC is unable to BaseX mode. This is more about
>   clearing unsupported speeds and duplex settings. The port modes
>   should not be cleared; phylink_set_port_modes() will help with this.
> 
> So add the missing 10G modes.

Hi Russell

Would a phylink_set_10g(mask) helper make sense? As you say, it is
about the speed, not the individual modes.

      Andrew
Russell King (Oracle) July 20, 2021, 2:11 p.m. UTC | #2
On Tue, Jul 20, 2021 at 03:51:35PM +0200, Andrew Lunn wrote:
> On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> > Phylink documentation says:
> >   Note that the PHY may be able to transform from one connection
> >   technology to another, so, eg, don't clear 1000BaseX just
> >   because the MAC is unable to BaseX mode. This is more about
> >   clearing unsupported speeds and duplex settings. The port modes
> >   should not be cleared; phylink_set_port_modes() will help with this.
> > 
> > So add the missing 10G modes.
> 
> Hi Russell
> 
> Would a phylink_set_10g(mask) helper make sense? As you say, it is
> about the speed, not the individual modes.

Yes, good point, and that will probably help avoid this in the future.
We can't do that for things like e.g. SGMII though, because 1000/half
isn't universally supported.

Shall we get this patch merged anyway and then clean it up - as such
a change will need to cover multiple drivers anyway?
Andrew Lunn July 20, 2021, 2:13 p.m. UTC | #3
> Shall we get this patch merged anyway and then clean it up - as such
> a change will need to cover multiple drivers anyway?

Yes, do it as a cleanup later.

     Andrew
Andrew Lunn July 20, 2021, 2:13 p.m. UTC | #4
On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> Phylink documentation says:
>   Note that the PHY may be able to transform from one connection
>   technology to another, so, eg, don't clear 1000BaseX just
>   because the MAC is unable to BaseX mode. This is more about
>   clearing unsupported speeds and duplex settings. The port modes
>   should not be cleared; phylink_set_port_modes() will help with this.
> 
> So add the missing 10G modes.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Acked-by: Marek Behún <kabel@kernel.org>
> Acked-by: Ioana Ciornei <ioana.ciornei@nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Ioana Ciornei Aug. 16, 2021, 2:47 p.m. UTC | #5
On Tue, Jul 20, 2021 at 03:11:34PM +0100, Russell King (Oracle) wrote:
> On Tue, Jul 20, 2021 at 03:51:35PM +0200, Andrew Lunn wrote:
> > On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> > > Phylink documentation says:
> > >   Note that the PHY may be able to transform from one connection
> > >   technology to another, so, eg, don't clear 1000BaseX just
> > >   because the MAC is unable to BaseX mode. This is more about
> > >   clearing unsupported speeds and duplex settings. The port modes
> > >   should not be cleared; phylink_set_port_modes() will help with this.
> > > 
> > > So add the missing 10G modes.
> > 
> > Hi Russell
> > 
> > Would a phylink_set_10g(mask) helper make sense? As you say, it is
> > about the speed, not the individual modes.
> 
> Yes, good point, and that will probably help avoid this in the future.
> We can't do that for things like e.g. SGMII though, because 1000/half
> isn't universally supported.
> 
> Shall we get this patch merged anyway and then clean it up - as such
> a change will need to cover multiple drivers anyway?
> 

This didn't get merged unfortunately.

Could you please resend it? Alternatively, I can take a look into adding
that phylink_set_10g() helper if that is what's keeping it from being
merged.

Ioana
Russell King (Oracle) Sept. 3, 2021, 10:33 a.m. UTC | #6
On Mon, Aug 16, 2021 at 05:47:52PM +0300, Ioana Ciornei wrote:
> On Tue, Jul 20, 2021 at 03:11:34PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jul 20, 2021 at 03:51:35PM +0200, Andrew Lunn wrote:
> > > On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> > > > Phylink documentation says:
> > > >   Note that the PHY may be able to transform from one connection
> > > >   technology to another, so, eg, don't clear 1000BaseX just
> > > >   because the MAC is unable to BaseX mode. This is more about
> > > >   clearing unsupported speeds and duplex settings. The port modes
> > > >   should not be cleared; phylink_set_port_modes() will help with this.
> > > > 
> > > > So add the missing 10G modes.
> > > 
> > > Hi Russell
> > > 
> > > Would a phylink_set_10g(mask) helper make sense? As you say, it is
> > > about the speed, not the individual modes.
> > 
> > Yes, good point, and that will probably help avoid this in the future.
> > We can't do that for things like e.g. SGMII though, because 1000/half
> > isn't universally supported.
> > 
> > Shall we get this patch merged anyway and then clean it up - as such
> > a change will need to cover multiple drivers anyway?
> > 
> 
> This didn't get merged unfortunately.
> 
> Could you please resend it? Alternatively, I can take a look into adding
> that phylink_set_10g() helper if that is what's keeping it from being
> merged.

It looks like the original patch didn't appear in patchwork for some
reason - at least google can find it in lore's netdev archives, but
not in patchwork. I can only put this down to some kernel.org
unreliability - we've seen this unreliability in the past with netdev,
and it seems to be an ongoing issue.

It's now too late to re-send for this merge window - net-next is
currently closed. Whether I remember in a fortnight or so time when
net-next re-opens is another problem.

And yes, I also have the phylink_set_10g() patches in my tree, which
was waiting for this patch to have been merged.
Ioana Ciornei Sept. 3, 2021, 11:09 a.m. UTC | #7
On Fri, Sep 03, 2021 at 11:33:58AM +0100, Russell King (Oracle) wrote:
> On Mon, Aug 16, 2021 at 05:47:52PM +0300, Ioana Ciornei wrote:
> > On Tue, Jul 20, 2021 at 03:11:34PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Jul 20, 2021 at 03:51:35PM +0200, Andrew Lunn wrote:
> > > > On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> > > > > Phylink documentation says:
> > > > >   Note that the PHY may be able to transform from one connection
> > > > >   technology to another, so, eg, don't clear 1000BaseX just
> > > > >   because the MAC is unable to BaseX mode. This is more about
> > > > >   clearing unsupported speeds and duplex settings. The port modes
> > > > >   should not be cleared; phylink_set_port_modes() will help with this.
> > > > > 
> > > > > So add the missing 10G modes.
> > > > 
> > > > Hi Russell
> > > > 
> > > > Would a phylink_set_10g(mask) helper make sense? As you say, it is
> > > > about the speed, not the individual modes.
> > > 
> > > Yes, good point, and that will probably help avoid this in the future.
> > > We can't do that for things like e.g. SGMII though, because 1000/half
> > > isn't universally supported.
> > > 
> > > Shall we get this patch merged anyway and then clean it up - as such
> > > a change will need to cover multiple drivers anyway?
> > > 
> > 
> > This didn't get merged unfortunately.
> > 
> > Could you please resend it? Alternatively, I can take a look into adding
> > that phylink_set_10g() helper if that is what's keeping it from being
> > merged.
> 
> It looks like the original patch didn't appear in patchwork for some
> reason - at least google can find it in lore's netdev archives, but
> not in patchwork. I can only put this down to some kernel.org
> unreliability - we've seen this unreliability in the past with netdev,
> and it seems to be an ongoing issue.
> 

Yes, it cannot be found though google but the patch appears in
patchwork, it was tagged with 'Changes requested'.
https://patchwork.kernel.org/project/netdevbpf/patch/E1m5mVT-00032g-Km@rmk-PC.armlinux.org.uk/


> It's now too late to re-send for this merge window - net-next is
> currently closed. Whether I remember in a fortnight or so time when
> net-next re-opens is another problem.
> 
> And yes, I also have the phylink_set_10g() patches in my tree, which
> was waiting for this patch to have been merged.
> 

Ok, thanks!

Ioana
Russell King (Oracle) Sept. 3, 2021, 11:34 a.m. UTC | #8
On Fri, Sep 03, 2021 at 02:09:16PM +0300, Ioana Ciornei wrote:
> On Fri, Sep 03, 2021 at 11:33:58AM +0100, Russell King (Oracle) wrote:
> > On Mon, Aug 16, 2021 at 05:47:52PM +0300, Ioana Ciornei wrote:
> > > On Tue, Jul 20, 2021 at 03:11:34PM +0100, Russell King (Oracle) wrote:
> > > > On Tue, Jul 20, 2021 at 03:51:35PM +0200, Andrew Lunn wrote:
> > > > > On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> > > > > > Phylink documentation says:
> > > > > >   Note that the PHY may be able to transform from one connection
> > > > > >   technology to another, so, eg, don't clear 1000BaseX just
> > > > > >   because the MAC is unable to BaseX mode. This is more about
> > > > > >   clearing unsupported speeds and duplex settings. The port modes
> > > > > >   should not be cleared; phylink_set_port_modes() will help with this.
> > > > > > 
> > > > > > So add the missing 10G modes.
> > > > > 
> > > > > Hi Russell
> > > > > 
> > > > > Would a phylink_set_10g(mask) helper make sense? As you say, it is
> > > > > about the speed, not the individual modes.
> > > > 
> > > > Yes, good point, and that will probably help avoid this in the future.
> > > > We can't do that for things like e.g. SGMII though, because 1000/half
> > > > isn't universally supported.
> > > > 
> > > > Shall we get this patch merged anyway and then clean it up - as such
> > > > a change will need to cover multiple drivers anyway?
> > > > 
> > > 
> > > This didn't get merged unfortunately.
> > > 
> > > Could you please resend it? Alternatively, I can take a look into adding
> > > that phylink_set_10g() helper if that is what's keeping it from being
> > > merged.
> > 
> > It looks like the original patch didn't appear in patchwork for some
> > reason - at least google can find it in lore's netdev archives, but
> > not in patchwork. I can only put this down to some kernel.org
> > unreliability - we've seen this unreliability in the past with netdev,
> > and it seems to be an ongoing issue.
> > 
> 
> Yes, it cannot be found though google but the patch appears in
> patchwork, it was tagged with 'Changes requested'.
> https://patchwork.kernel.org/project/netdevbpf/patch/E1m5mVT-00032g-Km@rmk-PC.armlinux.org.uk/

Thanks. I wonder why searching for it via google and also via patchworks
search facility didn't find it.

So, it got incorrectly tagged by netdev maintainers, presumably because
they're too quick to classify a patch while discussion on the patch was
still ongoing - and there's no way for those discussing that to ever
know without finding it in patchwork. Which is pretty much impossible
unless you know the patchwork URL format and message ID, and are
prepared to regularly poll the patchwork website.

The netdev process, as a patch submitter or reviewer, is really very
unfriendly.
Andrew Lunn Sept. 3, 2021, 1:11 p.m. UTC | #9
On Fri, Sep 03, 2021 at 12:34:34PM +0100, Russell King (Oracle) wrote:
> On Fri, Sep 03, 2021 at 02:09:16PM +0300, Ioana Ciornei wrote:
> > On Fri, Sep 03, 2021 at 11:33:58AM +0100, Russell King (Oracle) wrote:
> > > On Mon, Aug 16, 2021 at 05:47:52PM +0300, Ioana Ciornei wrote:
> > > > On Tue, Jul 20, 2021 at 03:11:34PM +0100, Russell King (Oracle) wrote:
> > > > > On Tue, Jul 20, 2021 at 03:51:35PM +0200, Andrew Lunn wrote:
> > > > > > On Tue, Jul 20, 2021 at 10:57:43AM +0100, Russell King wrote:
> > > > > > > Phylink documentation says:
> > > > > > >   Note that the PHY may be able to transform from one connection
> > > > > > >   technology to another, so, eg, don't clear 1000BaseX just
> > > > > > >   because the MAC is unable to BaseX mode. This is more about
> > > > > > >   clearing unsupported speeds and duplex settings. The port modes
> > > > > > >   should not be cleared; phylink_set_port_modes() will help with this.
> > > > > > > 
> > > > > > > So add the missing 10G modes.
> > > > > > 
> > > > > > Hi Russell
> > > > > > 
> > > > > > Would a phylink_set_10g(mask) helper make sense? As you say, it is
> > > > > > about the speed, not the individual modes.
> > > > > 
> > > > > Yes, good point, and that will probably help avoid this in the future.
> > > > > We can't do that for things like e.g. SGMII though, because 1000/half
> > > > > isn't universally supported.
> > > > > 
> > > > > Shall we get this patch merged anyway and then clean it up - as such
> > > > > a change will need to cover multiple drivers anyway?
> > > > > 
> > > > 
> > > > This didn't get merged unfortunately.
> > > > 
> > > > Could you please resend it? Alternatively, I can take a look into adding
> > > > that phylink_set_10g() helper if that is what's keeping it from being
> > > > merged.
> > > 
> > > It looks like the original patch didn't appear in patchwork for some
> > > reason - at least google can find it in lore's netdev archives, but
> > > not in patchwork. I can only put this down to some kernel.org
> > > unreliability - we've seen this unreliability in the past with netdev,
> > > and it seems to be an ongoing issue.
> > > 
> > 
> > Yes, it cannot be found though google but the patch appears in
> > patchwork, it was tagged with 'Changes requested'.
> > https://patchwork.kernel.org/project/netdevbpf/patch/E1m5mVT-00032g-Km@rmk-PC.armlinux.org.uk/
> 
> Thanks. I wonder why searching for it via google and also via patchworks
> search facility didn't find it.
> 
> So, it got incorrectly tagged by netdev maintainers, presumably because
> they're too quick to classify a patch while discussion on the patch was
> still ongoing - and there's no way for those discussing that to ever
> know without finding it in patchwork. Which is pretty much impossible
> unless you know the patchwork URL format and message ID, and are
> prepared to regularly poll the patchwork website.
> 
> The netdev process, as a patch submitter or reviewer, is really very
> unfriendly.

H Russell, Ioana

It sounds like at LPC there is going to be a time slot to talk about
netdev processes. I would like to find out and discuss the new policy
for the time it takes to merge patches. Patchwork issues, and the lack
of integration with email workflows could be another interesting topic
to discuss.

	Andrew
Ioana Ciornei Sept. 3, 2021, 5:06 p.m. UTC | #10
On Fri, Sep 03, 2021 at 03:11:36PM +0200, Andrew Lunn wrote:

> > Thanks. I wonder why searching for it via google and also via patchworks
> > search facility didn't find it.
> > 
> > So, it got incorrectly tagged by netdev maintainers, presumably because
> > they're too quick to classify a patch while discussion on the patch was
> > still ongoing - and there's no way for those discussing that to ever
> > know without finding it in patchwork. Which is pretty much impossible
> > unless you know the patchwork URL format and message ID, and are
> > prepared to regularly poll the patchwork website.
> > 
> > The netdev process, as a patch submitter or reviewer, is really very
> > unfriendly.
> 
> H Russell, Ioana
> 
> It sounds like at LPC there is going to be a time slot to talk about
> netdev processes.

Is this on the Networking track? I didn't find any session that would
appear to target this topic.

> I would like to find out and discuss the new policy
> for the time it takes to merge patches. Patchwork issues, and the lack
> of integration with email workflows could be another interesting topic
> to discuss.

It would be interesting to have patchwork send a notification of some
sorts to the submitter when a certain patch set's state was changed.

Ioana
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index ae6d382d8735..543c1f202420 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -140,6 +140,11 @@  static void dpaa2_mac_validate(struct phylink_config *config,
 	case PHY_INTERFACE_MODE_10GBASER:
 	case PHY_INTERFACE_MODE_USXGMII:
 		phylink_set(mask, 10000baseT_Full);
+		phylink_set(mask, 10000baseCR_Full);
+		phylink_set(mask, 10000baseSR_Full);
+		phylink_set(mask, 10000baseLR_Full);
+		phylink_set(mask, 10000baseLRM_Full);
+		phylink_set(mask, 10000baseER_Full);
 		if (state->interface == PHY_INTERFACE_MODE_10GBASER)
 			break;
 		phylink_set(mask, 5000baseT_Full);