RE: [EXT] Re: [PATCH net-next] net: mvpp2: Add parsing support for different IPv4 IHL values
> -Original Message- > From: Marcin Wojtas > Sent: Tuesday, April 13, 2021 12:59 PM > To: Stefan Chulski > Cc: Russell King - ARM Linux admin ; > netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan > Markman ; linux-ker...@vger.kernel.org; > k...@kernel.org; and...@lunn.ch; aten...@kernel.org; Liron Himi > ; Dana Vardi > Subject: Re: [EXT] Re: [PATCH net-next] net: mvpp2: Add parsing support for > different IPv4 IHL values > > Hi Stefan, > > wt., 13 kwi 2021 o 11:56 Stefan Chulski napisał(a): > > > > > > -Original Message- > > > > From: Russell King - ARM Linux admin > > > > Sent: Tuesday, April 13, 2021 12:18 PM > > > > To: Stefan Chulski > > > > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > > > > da...@davemloft.net; Nadav Haklai ; Yan > > > Markman > > > > ; linux-ker...@vger.kernel.org; > > > k...@kernel.org; > > > > m...@semihalf.com; and...@lunn.ch; aten...@kernel.org; Liron Himi > > > > ; Dana Vardi > > > > Subject: [EXT] Re: [PATCH net-next] net: mvpp2: Add parsing > > > > support for different IPv4 IHL values > > > > > > > > External Email > > > > > > > > -- > > > > On Tue, Apr 13, 2021 at 11:45:31AM +0300, stef...@marvell.com > > > > wrote: > > > > > From: Stefan Chulski > > > > > > > > > > Add parser entries for different IPv4 IHL values. > > > > > Each entry will set the L4 header offset according to the IPv4 IHL > > > > > field. > > > > > L3 header offset will set during the parsing of the IPv4 protocol. > > > > > > > > What is the impact of this commit? Is something broken at the > > > > moment, if so what? Does this need to be backported to stable > kernels? > > > > > > > > These are key questions, of which the former two should be covered > > > > in every commit message so that the reason for the change can be > known. > > > > It's no good just describing what is being changed in the commit > > > > without also describing why the change is being made. > > > > > > > > Thanks. > > > > > > Due to missed parser support for IP header length > 20, RX IPv4 > > > checksum offload fail. > > > > > > Regards. > > > > Currently driver set skb->ip_summed = CHECKSUM_NONE and checksum > done by software. > > So this just improve performance for packets with IP header length > 20. > > IMO we can keep it in net-next. > > > > Stefan. > > Please update the commit message in v2 with the explanation. > > Also - is there an easy way to test it? L3 forwarding with forced header > length? > > Thanks, > Marcin I will wait for additional comments and resend it tomorrow. We probably should see this in "perf top" in L3 forwarding. Less cycles consumed by Network stack checksum callback. Regards.
RE: [EXT] Re: [PATCH net-next] net: mvpp2: Add parsing support for different IPv4 IHL values
> > -Original Message- > > From: Russell King - ARM Linux admin > > Sent: Tuesday, April 13, 2021 12:18 PM > > To: Stefan Chulski > > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > > da...@davemloft.net; Nadav Haklai ; Yan > Markman > > ; linux-ker...@vger.kernel.org; > k...@kernel.org; > > m...@semihalf.com; and...@lunn.ch; aten...@kernel.org; Liron Himi > > ; Dana Vardi > > Subject: [EXT] Re: [PATCH net-next] net: mvpp2: Add parsing support > > for different IPv4 IHL values > > > > External Email > > > > ------ > > On Tue, Apr 13, 2021 at 11:45:31AM +0300, stef...@marvell.com wrote: > > > From: Stefan Chulski > > > > > > Add parser entries for different IPv4 IHL values. > > > Each entry will set the L4 header offset according to the IPv4 IHL field. > > > L3 header offset will set during the parsing of the IPv4 protocol. > > > > What is the impact of this commit? Is something broken at the moment, > > if so what? Does this need to be backported to stable kernels? > > > > These are key questions, of which the former two should be covered in > > every commit message so that the reason for the change can be known. > > It's no good just describing what is being changed in the commit > > without also describing why the change is being made. > > > > Thanks. > > Due to missed parser support for IP header length > 20, RX IPv4 checksum > offload fail. > > Regards. Currently driver set skb->ip_summed = CHECKSUM_NONE and checksum done by software. So this just improve performance for packets with IP header length > 20. IMO we can keep it in net-next. Stefan.
RE: [EXT] Re: [PATCH net-next] net: mvpp2: Add parsing support for different IPv4 IHL values
> -Original Message- > From: Russell King - ARM Linux admin > Sent: Tuesday, April 13, 2021 12:18 PM > To: Stefan Chulski > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan > Markman ; linux-ker...@vger.kernel.org; > k...@kernel.org; m...@semihalf.com; and...@lunn.ch; > aten...@kernel.org; Liron Himi ; Dana Vardi > > Subject: [EXT] Re: [PATCH net-next] net: mvpp2: Add parsing support for > different IPv4 IHL values > > External Email > > -- > On Tue, Apr 13, 2021 at 11:45:31AM +0300, stef...@marvell.com wrote: > > From: Stefan Chulski > > > > Add parser entries for different IPv4 IHL values. > > Each entry will set the L4 header offset according to the IPv4 IHL field. > > L3 header offset will set during the parsing of the IPv4 protocol. > > What is the impact of this commit? Is something broken at the moment, if so > what? Does this need to be backported to stable kernels? > > These are key questions, of which the former two should be covered in > every commit message so that the reason for the change can be known. > It's no good just describing what is being changed in the commit without also > describing why the change is being made. > > Thanks. Due to missed parser support for IP header length > 20, RX IPv4 checksum offload fail. Regards.
RE: [EXT] Re: [V2 net-next] net: mvpp2: Add reserved port private flag configuration
> > CM3 won't use this interface till ethtool priv flag was set, it can be done > > by > communication over CM3 SRAM memory. > > > > > How does CM3 know the status of the link? > > > > CM3 has access to MAC registers and can read port status bit. > > > > > How does CM3 set its > > > flow control depending on what auto-neg determines, etc? > > > > Same as PPv2 Packet Processor RX and TX flow don't really care about auto- > neg, flow control, etc. > > CM3 can ignore it, all this stuff handled in MAC layer. CM3 just polling RX > descriptor ring and using TX ring for transmit. > > > > > > > > > 3. In some cases we need to dynamically switch the port "user" > > > > between CM3 and kernel. So I would like to preserve this > > > > functionality. > > > > > > And how do you synchronize between Linux and CM3 so you know how > is > > > using it and who cannot use it? > > > > > > Andrew > > > > I can add CM3 SRAM update into ethtool priv flag callback, so CM3 won't > use port till it was reserved to CM3. > > I really think you need to step back here and look at the bigger picture. If > linux should not use the interface, it should not exist. If it does not > exist, you > cannot use ethtool on it. > > What you might want to consider is adding remoteproc support between > Linux on the main processor and whatever you have on the CM3. You can > use RPMsg to send requests back and forth between Linux and the CM3. It > can request that the shared parts of the packet processor are set up for it. > Linux can tell it when the link comes up? It can request how the PHY auto- > neg should be configured. > > Andrew I would check this option. Thanks, Stefan.
RE: [EXT] Re: [V2 net-next] net: mvpp2: Add reserved port private flag configuration
> > 2. CM3 code has very small footprint requirement, we cannot > > implement the complete Serdes and PHY infrastructure that kernel > > provides as part of CM3 application. Therefore I would like to > > continue relying on kernel configuration for that. > > How can that work? How does Linux know when CM3 has up'ed the > interface? CM3 won't use this interface till ethtool priv flag was set, it can be done by communication over CM3 SRAM memory. > How does CM3 know the status of the link? CM3 has access to MAC registers and can read port status bit. > How does CM3 set its > flow control depending on what auto-neg determines, etc? Same as PPv2 Packet Processor RX and TX flow don't really care about auto-neg, flow control, etc. CM3 can ignore it, all this stuff handled in MAC layer. CM3 just polling RX descriptor ring and using TX ring for transmit. > > > 3. In some cases we need to dynamically switch the port "user" > > between CM3 and kernel. So I would like to preserve this > > functionality. > > And how do you synchronize between Linux and CM3 so you know how is > using it and who cannot use it? > > Andrew I can add CM3 SRAM update into ethtool priv flag callback, so CM3 won't use port till it was reserved to CM3. Stefan.
RE: [EXT] Re: [V2 net-next] net: mvpp2: Add reserved port private flag configuration
> I really, really hope that someone has thought this through: > > Packet Processor I/O Interface (PPIO) > >The MUSDK PPIO driver provides low-level network interface API for >User-Space network drivers/applications. The PPIO infrastrcuture maps >Marvell's Packet Processor (PPv2) configuration space and I/O descriptors >space directly to user-space memory. This allows user-space >driver/application to directly process the packet processor I/O rings from >user space, without any overhead of a copy operation. > > I realy, really hope that you are not exposing the I/O descriptors to > userspace, allowing userspace to manipulate the physical addresses in those > descriptors, and that userspace is not dealing with physical addresses. > > If userspace has access to the I/O descriptors with physical addresses, or > userspace is dealing with physical addresses, then you can say good bye to > any kind of security on the platform. Essentially, in such a scenario, the > entire > system memory becomes accessible to userspace, which includes the kernel. Hi Russel, This patch doesn't relate to MUSDK Packet Processor I/O Interface functionality. MUSDK is just another possible use case I could think of for the port reservation feature. I am not responsible for the MUSDK code, but as far as I know it is based on the generic UIO Kernel interface (uio_pdrv_genirq) so the user can decide whether he wants to enable it or not for his platform. For the main CM3 management port use case, security is not an issue since the CM3 processor is secured by hardware in the device and its code is authenticated. Stefan.
RE: [EXT] Re: [V2 net-next] net: mvpp2: Add reserved port private flag configuration
> Hi Stefan > > Thanks for the strings change. Looks a lot better. > > Now i took a look at the bigger picture. > > > According to Armada SoC architecture and design, all the PPv2 ports > > which are populated on the same communication processor silicon die > > (CP11x) share the same Classifier and Parser engines. > > > > Armada is an embedded platform and therefore there is a need to > > reserve some of the PPv2 ports for different use cases. > > > > For example, a port can be reserved for a CM3 CPU running FreeRTOS for > > management purposes Hi Andrew and Jakub, There are multiple reasons why we must let the kernel initialize and manage the reserved port: 1. According to pp2 hardware design, all classifier and parser configuration access are indirect. In order to prevent race conditions, it needs to be configured by single entity. 2. CM3 code has very small footprint requirement, we cannot implement the complete Serdes and PHY infrastructure that kernel provides as part of CM3 application. Therefore I would like to continue relying on kernel configuration for that. 3. In some cases we need to dynamically switch the port "user" between CM3 and kernel. So I would like to preserve this functionality. > So the CM3 CPU has its own driver for this hardware? It seems like we should > not even instantiate the Linux driver for this port. Does the > CM3 have its own DT blob? I think the better solution is that the Armada DT > for the board does not list the port, and the DT for the CM3 does. Linux > never sees the port, since Linux should not be using it. > > > or by user-space data plane application. > > You mean XDP/AF_XDP? I don't see any other XDP capable drivers having a > flag like this. If this was required, i would expect it to be a common > properly, > not driver private. No XDP doesn't require this. One of the use cases of the port reservation feature is the Marvell User Space SDK (MUSDK) which its latest code is publicly available here: https://github.com/MarvellEmbeddedProcessors/musdk-marvell You can find example use case for this application here: http://wiki.macchiatobin.net/tiki-index.php?page=MUSDK+Introduction Stefan.
RE: [EXT] Re: [net-next] net: mvpp2: Add reserved port private flag configuration
> > From: Stefan Chulski > > > > According to Armada SoC architecture and design, all the PPv2 ports > > which are populated on the same communication processor silicon die > > (CP11x) share the same Classifier and Parser engines. > > > > Armada is an embedded platform and therefore there is a need to > > reserve some of the PPv2 ports for different use cases. > > > > For example, a port can be reserved for a CM3 CPU running FreeRTOS for > > management purposes or by user-space data plane application. > > > > During port reservation all common configurations are preserved and > > only RXQ, TXQ, and interrupt vectors are disabled. > > If a port is reserved for use by the CM3, what are the implications for Linux > running on the AP? Should Linux have knowledge of the port? > What configurations of the port should be permitted? In reserved mode all port TXQ's closed(Linux won't transmit any packet from this ports) and RX interrupts disabled(Linux won't receive any packet). We still can change port MAC address, do port UP/DOWN from Linux running on the AP. Only permitted configurations Is MTU change. Driver .ndo_change_mtu callback has logic that switch between percpu_pools and shared pools buffer mode, we should avoid this since buffer management not done by Kernel. > I think describing how a port reserved for use by the CM3 CPU should appear > to Linux is particularly important for the commit commentry to cover. Ok, I would add more info to commit message. Thanks, Stefan.
RE: [EXT] Re: [net-next] net: mvpp2: Add reserved port private flag configuration
> -Original Message-
> From: Andrew Lunn
> Sent: Wednesday, March 10, 2021 5:51 PM
> To: Stefan Chulski
> Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com;
> da...@davemloft.net; Nadav Haklai ; Yan
> Markman ; linux-ker...@vger.kernel.org;
> k...@kernel.org; li...@armlinux.org.uk; m...@semihalf.com;
> rmk+ker...@armlinux.org.uk; aten...@kernel.org; rab...@solid-run.com
> Subject: [EXT] Re: [net-next] net: mvpp2: Add reserved port private flag
> configuration
>
> External Email
>
> --
> > static void mvpp2_ethtool_get_strings(struct net_device *netdev, u32
> sset,
> > u8 *data)
> > {
> > struct mvpp2_port *port = netdev_priv(netdev);
> > int i, q;
> >
> > - if (sset != ETH_SS_STATS)
> > - return;
> > + switch (sset) {
> > + case ETH_SS_STATS:
> > + for (i = 0; i < ARRAY_SIZE(mvpp2_ethtool_mib_regs); i++) {
> > + strscpy(data, mvpp2_ethtool_mib_regs[i].string,
> > + ETH_GSTRING_LEN);
> > + data += ETH_GSTRING_LEN;
> > + }
>
> Hi Stefan
>
> Maybe rename the existing function to
> mvpp2_ethtool_get_strings_stats() and turn it into a helper. Add a new
> mvpp2_ethtool_get_strings_priv() helper. And a new
> mvpp2_ethtool_get_strings() which just calls the two helpers.
OK, I can do this.
> Overall the
> patch should be smaller and much easier to review.
>
> Andrew
Make it patch series? I can split it to 2/3 patches.
Thanks,
Stefan.
RE: [EXT] Re: Phylink flow control support on ports with MLO_AN_FIXED auto negotiation
> > > > > I discussed it with Andrew earlier last year, and his response was: > > > > > > > > > > DT configuration of pause for fixed link probably is > > > > > sufficient. I don't remember it ever been really discussed for > > > > > DSA. It was a Melanox discussion about limiting pause for the > > > > > CPU. So I think it is safe to not implement ethtool -A, at > > > > > least until somebody has a real use case for it. > > > > > > > > > > So I chose not to support it - no point supporting features that > > > > > people aren't using. If you have a "real use case" then it can be > > > > > added. > > > > > > > > This patch may be sufficient - I haven't fully considered all the > > > > implications of changing this though. > > > > > > Did you try this patch? What's the outcome? > > > > For me patch worked as expected. > > Hi Stefan > > Russell's patch allows it, but i would be interested in knows why you actually > need it. What is your use case for changing this on the fly? > >Andrew Usually, user prefer have the capability disable/enable flow control on the fly. For example: Armada connected by 10G fixed link to SOHO switch and SOHO has 10 external 1G LAN interfaces. When we have 2 flows (from Armada to LAN) from two different ports, we have an obvious congestion issue. Bursts on 10G interface would cause FC frames on Armada<->SOHO 10G port and one flow would affect another. In some use cases, the user would prefer lossless traffic and keep FC, for another use case (probably UDP streaming) disable FC. Regards, Stefan.
RE: [EXT] Re: Phylink flow control support on ports with MLO_AN_FIXED auto negotiation
> > > I discussed it with Andrew earlier last year, and his response was:
> > >
> > > DT configuration of pause for fixed link probably is sufficient. I
> > > don't remember it ever been really discussed for DSA. It was a
> > > Melanox discussion about limiting pause for the CPU. So I think it
> > > is safe to not implement ethtool -A, at least until somebody has a
> > > real use case for it.
> > >
> > > So I chose not to support it - no point supporting features that
> > > people aren't using. If you have a "real use case" then it can be added.
> >
> > This patch may be sufficient - I haven't fully considered all the
> > implications of changing this though.
>
> Did you try this patch? What's the outcome?
For me patch worked as expected.
Thanks,
Stefan.
> >
> > drivers/net/phy/phylink.c | 9 +++--
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 7e0fdc17c8ee..2ee0d4dcf9a5 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -673,7 +673,6 @@ static void phylink_resolve(struct work_struct *w)
> > switch (pl->cur_link_an_mode) {
> > case MLO_AN_PHY:
> > link_state = pl->phy_state;
> > - phylink_apply_manual_flow(pl, &link_state);
> > mac_config = link_state.link;
> > break;
> >
> > @@ -698,11 +697,12 @@ static void phylink_resolve(struct work_struct
> *w)
> > link_state.pause = pl->phy_state.pause;
> > mac_config = true;
> > }
> > - phylink_apply_manual_flow(pl, &link_state);
> > break;
> > }
> > }
> >
> > + phylink_apply_manual_flow(pl, &link_state);
> > +
> > if (mac_config) {
> > if (link_state.interface != pl->link_config.interface) {
> > /* The interface has changed, force the link down
> and @@ -1639,9
> > +1639,6 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
> >
> > ASSERT_RTNL();
> >
> > - if (pl->cur_link_an_mode == MLO_AN_FIXED)
> > - return -EOPNOTSUPP;
> > -
> > if (!phylink_test(pl->supported, Pause) &&
> > !phylink_test(pl->supported, Asym_Pause))
> > return -EOPNOTSUPP;
> > @@ -1684,7 +1681,7 @@ int phylink_ethtool_set_pauseparam(struct
> phylink *pl,
> > /* Update our in-band advertisement, triggering a renegotiation if
> > * the advertisement changed.
> > */
> > - if (!pl->phydev)
> > + if (!pl->phydev && pl->cur_link_an_mode != MLO_AN_FIXED)
> > phylink_change_inband_advert(pl);
> >
> > mutex_unlock(&pl->state_mutex);
> >
> > --
> > RMK's Patch system:
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.armlinux.org.
> >
> uk_developer_patches_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ
> 3dKwkTIxK
> >
> Al6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=bLvAkwDrYioAER_dvXEqutiRiU
> W57bKfscMh
> > 71TGDxw&s=p5jgFDs7cxtpIE9LZ3ogOahGzpuKjG4PHOcPF6qXnXI&e=
> > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>
> --
> RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__www.armlinux.org.uk_developer_patches_&d=DwIBAg&c=nKjWec2b6
> R0mOyPaz7xtfQ&r=DDQ3dKwkTIxKAl6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&
> m=bLvAkwDrYioAER_dvXEqutiRiUW57bKfscMh71TGDxw&s=p5jgFDs7cxtpIE9
> LZ3ogOahGzpuKjG4PHOcPF6qXnXI&e=
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
RE: [EXT] Re: Phylink flow control support on ports with MLO_AN_FIXED auto negotiation
> > On Sun, Jan 31, 2021 at 11:12:14AM +, Russell King - ARM Linux admin > wrote: > > > I discussed it with Andrew earlier last year, and his response was: > > > > > > DT configuration of pause for fixed link probably is sufficient. I > > > don't remember it ever been really discussed for DSA. It was a > > > Melanox discussion about limiting pause for the CPU. So I think it > > > is safe to not implement ethtool -A, at least until somebody has a > > > real use case for it. > > > > > > So I chose not to support it - no point supporting features that > > > people aren't using. If you have a "real use case" then it can be added. > > > > This patch may be sufficient - I haven't fully considered all the > > implications of changing this though. > > Did you try this patch? What's the outcome? > Hi, Didn't tried it yet, its in my TODO list. Regards, Stefan.
RE: [EXT] Re: [PATCH v12 net-next 12/15] net: mvpp2: add BM protection underrun feature support
> > > Or we have also found out, that pushing back on parameters like > > > this, the developers goes back and looks at the code, and sometimes > > > figures out a way to automatically do the right thing, removing the > > > configuration knob, and just making it all simpler for the user to > > > use. > > > > I think of 2 alternatives: > > * `ethtool --set-priv-flags` - in such case there is a question if > > switching this particular feature in runtime is a good idea. > > * New DT/ACPI property - it is a hardware feature after all, so maybe > > let the user decide whether to enable it on the platform description > > level. > > Does this even need to be configurable? What is the cost of turning it on? > How does having less pools affect the system? Does average latency go up? > When would i consider an underrun actually a good thing? > > Maybe it should just be hard coded on? Or we should try to detect when > underruns are happening a lot, and dynamically turn it on for a while? > > Andrew The cost of this change is that the number of pools reduced from 16 to 8. The current driver uses only 4pools, but some future features like QoS can use over 4 pools. Regards, Stefan.
RE: [EXT] Re: [PATCH v13 net-next 00/15] net: mvpp2: Add TX Flow Control support
> -- > Hello: > > This series was applied to netdev/net-next.git (refs/heads/master): > > On Thu, 11 Feb 2021 12:48:47 +0200 you wrote: > > From: Stefan Chulski > > > > Armada hardware has a pause generation mechanism in GOP (MAC). > > The GOP generate flow control frames based on an indication programmed > in Ports Control 0 Register. There is a bit per port. > > However assertion of the PortX Pause bits in the ports control 0 register > only sends a one time pause. > > To complement the function the GOP has a mechanism to periodically send > pause control messages based on periodic counters. > > This mechanism ensures that the pause is effective as long as the > Appropriate PortX Pause is asserted. > > > > [...] > > Here is the summary with links: > - [v13,net-next,01/15] doc: marvell: add CM3 address space and PPv2.3 > description Next week I would prepare small patch series to address Russell King comments. Thanks, Stefan.
RE: [EXT] Re: [PATCH v13 net-next 08/15] net: mvpp2: add FCA RXQ non occupied descriptor threshold
> Ditto. > > I don't think these need to be fixed in the net tree, but it would still be > nice > to fix the problem. Please do so, as an initial patch in your series - so we > can > then backport if it turns out to eventually be necessary. > > Thanks. My series already has 15 patches and patchwork not happy about series with over 15 patches. Maybe I can send this as separate patch to net-next(or net) first and base this series on this net-next tree with this patch? Regards, Stefan.
RE: [EXT] Re: [PATCH v13 net-next 13/15] net: mvpp2: add PPv23 RX FIFO flow control
> > +/* Configure Rx FIFO Flow control thresholds */ void
> > +mvpp23_rx_fifo_fc_en(struct mvpp2 *priv, int port, bool en) {
> > + int val;
>
> u32 ?
OK.
> > +
> > + val = mvpp2_read(priv, MVPP2_RX_FC_REG(port));
> > +
> > + if (en)
> > + val |= MVPP2_RX_FC_EN;
> > + else
> > + val &= ~MVPP2_RX_FC_EN;
> > +
> > + mvpp2_write(priv, MVPP2_RX_FC_REG(port), val);
>
> if (en)
> val = MVPP2_RX_FC_EN;
> else
> val = 0;
>
> mvpp2_modify(priv + MVPP2_RX_FC_REG(port),
> MVPP2_RX_FC_EN, val);
>
> ?
OK, I would use mvpp2_modify Here.
Thanks,
Stefan.
RE: [EXT] Re: [PATCH v13 net-next 08/15] net: mvpp2: add FCA RXQ non occupied descriptor threshold
> -Original Message-
> From: Russell King - ARM Linux admin
> Sent: Thursday, February 11, 2021 2:50 PM
> To: Stefan Chulski
> Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com;
> da...@davemloft.net; Nadav Haklai ; Yan
> Markman ; linux-ker...@vger.kernel.org;
> k...@kernel.org; m...@semihalf.com; and...@lunn.ch;
> aten...@kernel.org; devicet...@vger.kernel.org; robh...@kernel.org;
> sebastian.hesselba...@gmail.com; gregory.clem...@bootlin.com; linux-
> arm-ker...@lists.infradead.org
> Subject: [EXT] Re: [PATCH v13 net-next 08/15] net: mvpp2: add FCA RXQ non
> occupied descriptor threshold
>
> External Email
>
> --
> On Thu, Feb 11, 2021 at 12:48:55PM +0200, stef...@marvell.com wrote:
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > index 761f745..8b4073c 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -1133,14 +1133,19 @@ static inline void
> > mvpp2_qvec_interrupt_disable(struct mvpp2_queue_vector *qvec) static
> > void mvpp2_interrupts_mask(void *arg) {
> > struct mvpp2_port *port = arg;
> > + int cpu = smp_processor_id();
> > + u32 thread;
> >
> > /* If the thread isn't used, don't do anything */
> > - if (smp_processor_id() > port->priv->nthreads)
> > + if (cpu > port->priv->nthreads)
> > return;
>
> What happened to a patch fixing this? Did I miss it? Was it submitted
> independently to the net tree?
Some reviewers asked to remove this from the series. I would send it as
separate patch to net.
Regards.
RE: [EXT] Re: [PATCH v13 net-next 05/15] net: mvpp2: add PPv23 version definition
> --
> On Thu, Feb 11, 2021 at 12:48:52PM +0200, stef...@marvell.com wrote:
> > From: Stefan Chulski
> >
> > This patch add PPv23 version definition.
> > PPv23 is new packet processor in CP115.
> > Everything that supported by PPv22, also supported by PPv23.
> > No functional changes in this stage.
> >
> > Signed-off-by: Stefan Chulski
> > Acked-by: Marcin Wojtas
>
> Reviewed-by: Russell King
>
> > @@ -7049,6 +7049,11 @@ static int mvpp2_probe(struct platform_device
> *pdev)
> > priv->port_map |= BIT(i);
> > }
> >
> > + if (priv->hw_version != MVPP21) {
> > + if (mvpp2_read(priv, MVPP2_VER_ID_REG) ==
> MVPP2_VER_PP23)
> > + priv->hw_version = MVPP23;
> > + }
> > +
>
> The only minor comment I have on this is... the formatting of the above.
> Wouldn't:
>
> if (priv->hw_version >= MVPP22 &&
> mvpp2_read(priv, MVPP2_VER_ID_REG) == MVPP2_VER_PP23)
> priv->hw_version = MVPP23;
>
> read better?
>
> Do we need to even check priv->hw_version here? Isn't this register
> implemented in PPv2.1 where it contains the value zero?
Yes, we can just:
if (mvpp2_read(priv, MVPP2_VER_ID_REG) == MVPP2_VER_PP23)
priv->hw_version = MVPP23;
Thanks,
Stefan.
RE: [EXT] Re: [PATCH v13 net-next 04/15] net: mvpp2: always compare hw-version vs MVPP21
> On Thu, Feb 11, 2021 at 12:48:51PM +0200, stef...@marvell.com wrote:
> > @@ -1199,7 +1199,7 @@ static bool mvpp2_port_supports_xlg(struct
> > mvpp2_port *port)
> >
> > static bool mvpp2_port_supports_rgmii(struct mvpp2_port *port) {
> > - return !(port->priv->hw_version == MVPP22 && port->gop_id == 0);
> > + return !(port->priv->hw_version != MVPP21 && port->gop_id == 0);
>
> I'm still very much of the opinion (as raised several revisions back) that
> using
> > MVPP21 or >= MVPP22 would be a lot better - especially when we have
> situations like this. Having negatives within negatives does not help
> readability.
Ok, I would update in next series.
Thanks,
Stefan
RE: [EXT] Re: [PATCH v12 net-next 12/15] net: mvpp2: add BM protection underrun feature support
> > -- > From: > Date: Wed, 10 Feb 2021 11:48:17 +0200 > > > > > +static int bm_underrun_protect = 1; > > + > > +module_param(bm_underrun_protect, int, 0444); > > +MODULE_PARM_DESC(bm_underrun_protect, "Set BM underrun protect > > +feature (0-1), def=1"); > > No new module parameters, please. Ok, I would remove new module parameters. By the way why new module parameters forbitten? Thanks, Stefan
RE: [EXT] Re: [PATCH v11 net-next 15/15] net: mvpp2: add TX FC firmware check
> > if (priv->global_tx_fc && priv->hw_version != MVPP21) {
> > - val = mvpp2_cm3_read(priv, MSS_FC_COM_REG);
> > - val |= FLOW_CONTROL_ENABLE_BIT;
> > - mvpp2_cm3_write(priv, MSS_FC_COM_REG, val);
> > + err = mvpp2_enable_global_fc(priv);
> > + if (err) {
> > + dev_warn(&pdev->dev, "CM3 firmware not running,
> version should be higher than 18.09 ");
> > + dev_warn(&pdev->dev, "and chip revision B0\n");
> > + dev_warn(&pdev->dev, "Flow control not
> supported\n");
>
> I would much rather this was:
>
> dev_warn(&pdev->dev, "Minimum of CM3 firmware
> 18.09 and chip revision B0 required for flow control\n");
>
> rather than trying to split it across several kernel messages.
I would repots v12 with this change soon.
Thanks,
Stefan.
RE: [EXT] Re: [PATCH v9 net-next 01/15] doc: marvell: add CM3 address space and PPv2.3 description
> > @@ -12,7 +13,7 @@ Required properties: > > - common controller registers > > - LMS registers > > - one register area per Ethernet port > > - For "marvell,armada-7k-pp2", must contain the following register > > + For "marvell,armada-7k-pp2" used by 7K/8K and CN913X, must contain > > + the following register > >sets: > > - packet processor registers > > - networking interfaces registers > > Hi Stefan > > Shouldn't there be an entry here describing what the third register set is? Would add it in next patchset. Thanks, Stefan.
RE: [EXT] Re: [RESEND PATCH v8 net-next 03/15] net: mvpp2: add CM3 SRAM memory map
> > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0);
> > + if (!priv->sram_pool) {
> > + if (!defer_once) {
> > + defer_once = true;
> > + /* Try defer once */
> > + return -EPROBE_DEFER;
> > + }
> > + dev_warn(&pdev->dev, "DT is too old, Flow control
> not supported\n");
> > + return -ENOMEM;
> > + }
> > + /* cm3_base allocated with offset zero into the SRAM since
> mapping size
> > +* is equal to requested size.
> > +*/
> > + priv->cm3_base = (void __iomem *)gen_pool_alloc(priv-
> >sram_pool,
> > +
> MSS_SRAM_SIZE);
> > + if (!priv->cm3_base)
> > + return -ENOMEM;
> > + }
>
> For v2 i asked:
>
> > I'm wondering if using a pool even makes sense. The ACPI case just
> > ioremap() the memory region. Either this memory is dedicated, and then
> > there is no need to use a pool, or the memory is shared, and at some
> > point the ACPI code is going to run into problems when some other
> > driver also wants access.
>
> There was never an answer to this.
Sorry probably missed this. Currently this memory not shared and I can just
ioremap same way as in ACPI case.
In this case I can remove EPROBE_DEFER.
Thanks,
Stefan.
RE: [EXT] Re: [RESEND PATCH v8 net-next 03/15] net: mvpp2: add CM3 SRAM memory map
> > }
> >
> > +static int mvpp2_get_sram(struct platform_device *pdev,
> > + struct mvpp2 *priv)
> > +{
> > + struct device_node *dn = pdev->dev.of_node;
> > + static bool defer_once;
> > + struct resource *res;
> > +
> > + if (has_acpi_companion(&pdev->dev)) {
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > + if (!res) {
> > + dev_warn(&pdev->dev, "ACPI is too old, Flow control
> not supported\n");
> > + return 0;
> > + }
> > + priv->cm3_base = devm_ioremap_resource(&pdev->dev,
> res);
> > + if (IS_ERR(priv->cm3_base))
> > + return PTR_ERR(priv->cm3_base);
> > + } else {
> > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0);
> > + if (!priv->sram_pool) {
> > + if (!defer_once) {
> > + defer_once = true;
> > + /* Try defer once */
> > + return -EPROBE_DEFER;
> > + }
> > + dev_warn(&pdev->dev, "DT is too old, Flow control
> not
> > +supported\n");
>
> This warning will show on every DT system with no cm3-mem property,
> right?
>
All DT system would has cm3-mem property if " add CM3 SRAM memory to cp11x
ethernet device tree " patch applied.
This is also only warning message, without any functional impact.
Regards,
Stefan.
RE: [EXT] Re: [PATCH v8 net-next 02/15] dts: marvell: add CM3 SRAM memory to cp11x ethernet device tree
> -- > On Sat, 6 Feb 2021 18:45:48 +0200 stef...@marvell.com wrote: > > From: Konstantin Porotchkin > > > > CM3 SRAM address space would be used for Flow Control configuration. > > > > Signed-off-by: Stefan Chulski > > Signed-off-by: Konstantin Porotchkin > > Isn't there are requirement to CC the DT mailing list and Rob on all device > tree patches? Maybe someone can clarify I know it's required when adding > bindings.. I would repost with robh...@kernel.org, gregory.clem...@bootlin.com and devicet...@vger.kernel.org in CC Thanks, Stefan.
RE: [EXT] Re: [PATCH v7 net-next 00/15] net: mvpp2: Add TX Flow Control support
> On Sun, 31 Jan 2021 16:33:43 +0200 stef...@marvell.com wrote: > > v6 --> v7 > > - Reduce patch set from 18 to 15 patches > > - Documentation change combined into a single patch > > - RXQ and BM size change combined into a single patch > > - Ring size change check moved into "add RXQ flow control configurations" > commit > > It still did not get into patchwork :S Reposted: https://patchwork.kernel.org/project/netdevbpf/list/?series=425967 Regards, Stefan.
RE: [EXT] Re: Phylink flow control support on ports with MLO_AN_FIXED auto negotiation
> > Hi, > > > > Armada has options for 1G/10G ports without PHY's(for example > community board Macchiato single shot). > > This port doesn't have PHY's and cannot negotiate Flow Control support, > but we can for example connect two ports without PHY's and manually(by > ethtool) configure FC. > > On the Macchiatobin single shot, you use the existing SFP support rather > than forcing them to fixed link. > > > Current phylink return error if I do this on ports with > MLO_AN_FIXED(callback phylink_ethtool_set_pauseparam): > > if (pl->cur_link_an_mode == MLO_AN_FIXED) > > return -EOPNOTSUPP; > > > > How can we enable FC configurations for these ports? Do you have any > suggestions or should I post my proposal as an RFC patch? > > If you really must use fixed-link, you specify the pause modes via firmware, > just as you specify the speed and duplex - you specify the link partner's flow > control abilities. In this case we cannot change this by ethtool during runtime? Regards, Stefan.
RE: [EXT] Re: [PATCH v5 net-next 00/18] net: mvpp2: Add TX Flow Control support
> > Ok, kernel.org has now dropped spamcop.net, so email should flow normally > now. > > Are you sure all your emails are being received by vger.kernel.org? No, I get Undeliverable Email response. I probably would wait till tomorrow and repost them again as v8. Regards, Stefan.
RE: [EXT] Re: [PATCH v5 net-next 00/18] net: mvpp2: Add TX Flow Control support
> > > > Hi Stefan, looks like patchwork and lore didn't get all the emails: > > > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__lore.kernel.org_r_1611858682-2D9845-2D1-2Dgit-2Dsend-2Demail- > > 2Dstefanc- > > > 40marvell.com&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ3dKwkTIx > > > KAl6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=AFp2yfjV7t2l3c7dCM9lllj7Mz1V > > - > > > 57354rTjjMB9_o&s=TK5AoswsdBLZNKNMI_rMiQQFtoLZf9UqEGQ40u7OHGI& > > e= > > https://urldefense.proofpoint.com/v2/url?u=https- > > 3A__patchwork.kernel.org_project_netdevbpf_list_-3Fseries- > > > 3D423983&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ3dKwkTIxKAl6_ > > Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=AFp2yfjV7t2l3c7dCM9lllj7Mz1V- > > 57354rTjjMB9_o&s=BC2VcCRP0O0r4wywUHOgkqvleArWUsCGaT3Ue1- > > O6VE&e= > > > > Unless it fixes itself soon - please repost. > > Reposted. > > Best Regards, > Stefan. I still don't see all patches in https://patchwork.kernel.org/project/netdevbpf/list/?series=424949 I would reduce patch series to 15 patches and repost again. Regards, Stefan.
RE: [EXT] Re: [PATCH v5 net-next 00/18] net: mvpp2: Add TX Flow Control support
> > Hi Stefan, looks like patchwork and lore didn't get all the emails: > > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__lore.kernel.org_r_1611858682-2D9845-2D1-2Dgit-2Dsend-2Demail- > 2Dstefanc- > 40marvell.com&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ3dKwkTIx > KAl6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=AFp2yfjV7t2l3c7dCM9lllj7Mz1V > - > 57354rTjjMB9_o&s=TK5AoswsdBLZNKNMI_rMiQQFtoLZf9UqEGQ40u7OHGI& > e= > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__patchwork.kernel.org_project_netdevbpf_list_-3Fseries- > 3D423983&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ3dKwkTIxKAl6_ > Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=AFp2yfjV7t2l3c7dCM9lllj7Mz1V- > 57354rTjjMB9_o&s=BC2VcCRP0O0r4wywUHOgkqvleArWUsCGaT3Ue1- > O6VE&e= > > Unless it fixes itself soon - please repost. Reposted. Best Regards, Stefan.
Phylink flow control support on ports with MLO_AN_FIXED auto negotiation
Hi, Armada has options for 1G/10G ports without PHY's(for example community board Macchiato single shot). This port doesn't have PHY's and cannot negotiate Flow Control support, but we can for example connect two ports without PHY's and manually(by ethtool) configure FC. Current phylink return error if I do this on ports with MLO_AN_FIXED(callback phylink_ethtool_set_pauseparam): if (pl->cur_link_an_mode == MLO_AN_FIXED) return -EOPNOTSUPP; How can we enable FC configurations for these ports? Do you have any suggestions or should I post my proposal as an RFC patch? Thanks, Stefan.
RE: [EXT] Re: [PATCH v4 net-next 10/19] net: mvpp2: add FCA RXQ non occupied descriptor threshold
> > > From: Stefan Chulski > > > > RXQ non occupied descriptor threshold would be used by Flow Control > > Firmware feature to move to the XOFF mode. > > RXQ non occupied threshold would change interrupt cause that polled by > > CM3 Firmware. > > Actual non occupied interrupt masked and won't trigger interrupt. > > Does this mean that this change enables a feature, but it is unused due to a > masked interrupt? Firmware poll RXQ non occupied cause register to indicate if number of registers bellow threshold. We do not trigger any interrupt, just poll this bit in CM3. So this cause always masked. > > > > Signed-off-by: Stefan Chulski > > --- > > drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 3 ++ > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 46 > > +--- > > 2 files changed, 42 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > index 73f087c..9d8993f 100644 > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > @@ -295,6 +295,8 @@ > > #define MVPP2_PON_CAUSE_TXP_OCCUP_DESC_ALL_MASK > 0x3fc0 > > #define MVPP2_PON_CAUSE_MISC_SUM_MASK BIT(31) > > #define MVPP2_ISR_MISC_CAUSE_REG 0x55b0 > > +#define MVPP2_ISR_RX_ERR_CAUSE_REG(port) (0x5520 + 4 * (port)) > > +#defineMVPP2_ISR_RX_ERR_CAUSE_NONOCC_MASK 0x00ff > > The indentation in this file is inconsistent. Here even between the two newly > introduced lines. Ok, I would fix this. > > /* If the thread isn't used, don't do anything */ > > - if (smp_processor_id() > port->priv->nthreads) > > + if (cpu >= port->priv->nthreads) > > return; > > Here and below, the change from greater than to greater than is really a > (standalone) fix? Ok, I would move this to separate patch. > > +/* Routine set the number of non-occupied descriptors threshold that > > +change > > + * interrupt error cause polled by FW Flow Control */ > > nit: no need for "Routine". Also, does "change .. cause" mean "that triggers > an interrupt"? Ok. Thanks, Stefan.
RE: [EXT] Re: [PATCH v4 net-next 11/19] net: mvpp2: add spinlock for FW FCA configuration path
> > index 9d8993f..f34e260 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > @@ -1021,6 +1021,11 @@ struct mvpp2 {
> >
> > /* CM3 SRAM pool */
> > struct gen_pool *sram_pool;
> > +
> > + bool custom_dma_mask;
> > +
> > + /* Spinlocks for CM3 shared memory configuration */
> > + spinlock_t mss_spinlock;
>
> Does this need to be a stand-alone patch? This introduces a spinlock, but
> does not use it.
>
> Also, is the introduction of custom_dma_mask in this commit on purpose?
I would add this change to another patch. custom_dma_mask should be removed.
Thanks,
Stefan.
RE: [EXT] Re: [PATCH v4 net-next 19/19] net: mvpp2: add TX FC firmware check
> > You can devmem 0xF2400240(Device ID Status Register). > > #define A8040_B0_DEVICE_ID 0x8045 > > #define A8040_AX_DEVICE_ID 0x8040 > > #define A7040_B0_DEVICE_ID 0x7045 > > #define A7040_AX_DEVICE_ID 0x7040 > > #define A3900_A1_DEVICE_ID 0x6025 > > #define CN9130_DEVICE_ID0x7025 > > Thanks. 0x00028040, so it's AX silicon. Is there nothing that can be done for > flow control on that? No, we cannot support FC with AX on A8040. > It would probably also be a good idea to state this requirement in the > message as well, rather than just suggesting the firmware revision. Ok, I would update this. Thanks, Stefan.
RE: [EXT] Re: [PATCH v4 net-next 19/19] net: mvpp2: add TX FC firmware check
> -Original Message- > From: Russell King - ARM Linux admin > Sent: Wednesday, January 27, 2021 5:00 PM > To: Stefan Chulski > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan > Markman ; linux-ker...@vger.kernel.org; > k...@kernel.org; m...@semihalf.com; and...@lunn.ch; > aten...@kernel.org > Subject: Re: [EXT] Re: [PATCH v4 net-next 19/19] net: mvpp2: add TX FC > firmware check > > On Wed, Jan 27, 2021 at 02:37:34PM +, Stefan Chulski wrote: > > Your mcbin-ss is A8K AX or A8K B0? On AX revisions we do not have FC > support in firmware. > > How do I tell? I don't want to remove the heatsink, and I don't see anything > in MV-S88-00E. I didn't grab a copy of the Errata before I accidentally > let > me extranet access expire. You can devmem 0xF2400240(Device ID Status Register). #define A8040_B0_DEVICE_ID 0x8045 #define A8040_AX_DEVICE_ID 0x8040 #define A7040_B0_DEVICE_ID 0x7045 #define A7040_AX_DEVICE_ID 0x7040 #define A3900_A1_DEVICE_ID 0x6025 #define CN9130_DEVICE_ID0x7025 Regards.
RE: [EXT] Re: [PATCH v4 net-next 19/19] net: mvpp2: add TX FC firmware check
> -Original Message-
> From: Russell King - ARM Linux admin
> Sent: Wednesday, January 27, 2021 4:06 PM
> To: Stefan Chulski
> Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com;
> da...@davemloft.net; Nadav Haklai ; Yan
> Markman ; linux-ker...@vger.kernel.org;
> k...@kernel.org; m...@semihalf.com; and...@lunn.ch;
> aten...@kernel.org
> Subject: [EXT] Re: [PATCH v4 net-next 19/19] net: mvpp2: add TX FC
> firmware check
>
> External Email
>
> --
> On Wed, Jan 27, 2021 at 01:43:35PM +0200, stef...@marvell.com wrote:
> > if (priv->global_tx_fc && priv->hw_version != MVPP21) {
> > - val = mvpp2_cm3_read(priv, MSS_FC_COM_REG);
> > - val |= FLOW_CONTROL_ENABLE_BIT;
> > - mvpp2_cm3_write(priv, MSS_FC_COM_REG, val);
> > + err = mvpp2_enable_global_fc(priv);
> > + if (err) {
> > + dev_warn(&pdev->dev, "CM3 firmware not running,
> version should be higher than 18.09\n");
> > + dev_warn(&pdev->dev, "Flow control not
> supported\n");
> > + }
>
> I've just booted this on my mcbin-ss, and I get:
>
> mvpp2 f200.ethernet: CM3 firmware not running, version should be
> higher than 18.09
> mvpp2 f400.ethernet: CM3 firmware not running, version should be
> higher than 18.09
>
> which is rather odd, because I believe I'm running the 18.12 firmware from
> git://github.com/MarvellEmbeddedProcessors/binaries-marvell
> branch binaries-marvell-armada-18.12.
>
> Any ideas?
Your mcbin-ss is A8K AX or A8K B0? On AX revisions we do not have FC support in
firmware.
Regards.
RE: [EXT] Re: [PATCH v2 RFC net-next 08/18] net: mvpp2: add FCA periodic timer configurations
> > > > > > > > > -- On Sun, Jan 24, 2021 at 01:43:57PM +0200, stef...@marvell.com > > > wrote: > > > > +/* Set Flow Control timer x140 faster than pause quanta to ensure > > > > +that link > > > > + * partner won't send taffic if port in XOFF mode. > > > > > > Can you explain more why 140 times faster is desirable here? Why 140 > > > times and not, say, 10 times faster? Where does this figure come > > > from, and what is the reasoning? Is there a switch that requires it? > > > > I tested with 140. > > Actually regarding to spec each quanta should be equal to 512 bit times. > > In 10G bit time is 0.1ns. > > And if the link has been negotiated to 10Mbps? Or is the clock already scaled > to the link speed? > >Andrew Currently its static, probably I can add function that reconfigure timer during runtime(based on link speed). Should it be part of this series or add it afterwards? Regards, Stefan.
RE: [EXT] Re: [PATCH v2 RFC net-next 03/18] net: mvpp2: add CM3 SRAM memory map
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -91,6 +92,18 @@ static inline u32 mvpp2_cpu_to_thread(struct
> mvpp2 *priv, int cpu)
> > return cpu % priv->nthreads;
> > }
> >
> > +static inline
> > +void mvpp2_cm3_write(struct mvpp2 *priv, u32 offset, u32 data) {
> > + writel(data, priv->cm3_base + offset); }
> > +
> > +static inline
> > +u32 mvpp2_cm3_read(struct mvpp2 *priv, u32 offset) {
> > + return readl(priv->cm3_base + offset); }
>
> No inline functions in .c file please. Let the compiler decide.
>
>Andrew
Ok, I would fix.
Thanks,
Stefan.
RE: [EXT] Re: [PATCH v2 RFC net-next 04/18] net: mvpp2: add PPv23 version definition
> > We cannot access PPv2 register space before enabling clocks(done in > mvpp2_probe) , PP21 and PP22/23 have different sets of clocks. > > > So diff between PP21 and PP22/23 should be stored in device tree(in > > of_device_id), with MVPP22 and MVPP21 stored as .data > > Hi Stefan > > As far as i can see, you are not adding a new compatible. So 'marvell,armada- > 7k-pp2' means PPv2.2 and PPv2.3? It would be good to update the comment > at the beginning of marvell-pp2.txt to indicate this. > > Andrew Ok, I would add comment. Thanks, Stefan.
RE: [EXT] Re: [PATCH v2 RFC net-next 08/18] net: mvpp2: add FCA periodic timer configurations
> > -- > On Sun, Jan 24, 2021 at 01:43:57PM +0200, stef...@marvell.com wrote: > > +/* Set Flow Control timer x140 faster than pause quanta to ensure > > +that link > > + * partner won't send taffic if port in XOFF mode. > > Can you explain more why 140 times faster is desirable here? Why 140 times > and not, say, 10 times faster? Where does this figure come from, and what is > the reasoning? Is there a switch that requires it? I tested with 140. Actually regarding to spec each quanta should be equal to 512 bit times. In 10G bit time is 0.1ns. So It actually should be: FC_CLK_DIVIDER = 1 / 512 = ~20. I took some buffer and made it 140. So maybe I can do it 100? Regards, Stefan.
RE: [EXT] Re: [PATCH v2 RFC net-next 04/18] net: mvpp2: add PPv23 version definition
> > Signed-off-by: Stefan Chulski > > --- > > drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 24 -- > -- > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 17 +- > > 2 files changed, 25 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > index aec9179..89b3ede 100644 > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h > > @@ -60,6 +60,9 @@ > > /* Top Registers */ > > #define MVPP2_MH_REG(port) (0x5040 + 4 * (port)) > > #define MVPP2_DSA_EXTENDED BIT(5) > > +#define MVPP2_VER_ID_REG 0x50b0 > > +#define MVPP2_VER_PP22 0x10 > > +#define MVPP2_VER_PP23 0x11 > > Looking at the Armada 8040 docs, it seems this register exists on > PPv2.1 as well, and holds the value zero there. > > I wonder whether we should instead read it's value directly into hw_version, > and test against these values, rather than inventing our own verison enum. > > I've also been wondering whether your != MVPP21 comparisons should > instead be >= MVPP22. > > Any thoughts? We cannot access PPv2 register space before enabling clocks(done in mvpp2_probe) , PP21 and PP22/23 have different sets of clocks. So diff between PP21 and PP22/23 should be stored in device tree(in of_device_id), with MVPP22 and MVPP21 stored as .data Maybe we can do it differently, but I prefer to make this change not in the Flow Control patch series. I'm OK with both >= MVPP22 and != MVPP21 options. Regards, Stefan.
RE: [EXT] Re: [PATCH v2 RFC net-next 09/18] net: mvpp2: add FCA RXQ non occupied descriptor threshold
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -1154,6 +1154,9 @@ static void mvpp2_interrupts_mask(void *arg)
> > mvpp2_thread_write(port->priv,
> >mvpp2_cpu_to_thread(port->priv,
> smp_processor_id()),
> >MVPP2_ISR_RX_TX_MASK_REG(port->id), 0);
> > + mvpp2_thread_write(port->priv,
> > + mvpp2_cpu_to_thread(port->priv,
> smp_processor_id()),
> > + MVPP2_ISR_RX_ERR_CAUSE_REG(port->id), 0);
>
> I wonder if this should be refactored:
>
> u32 thread = mvpp2_cpu_to_thread(port->priv,
> smp_processor_id());
>
> mvpp2_thread_write(port->priv, thread,
> MVPP2_ISR_RX_TX_MASK_REG(port->id), 0);
> mvpp2_thread_write(port->priv, thread,
> MVPP2_ISR_RX_ERR_CAUSE_REG(port->id), 0);
>
> to avoid having to recompute mvpp2_cpu_to_thread() for each write?
>
> However, looking deeper...
>
> static void mvpp2_interrupts_mask(void *arg) {
> struct mvpp2_port *port = arg;
> u32 thread;
> int cpu;
>
> cpu = smp_processor_id();
> if (cpu > port->priv->nthreads)
> return
>
> thread = mvpp2_cpu_to_thread(port->priv, cpu);
> ...
>
> and I wonder about that condition - "cpu > port->priv->nthreads". If cpu ==
> port->priv->nthreads, then mvpp2_cpu_to_thread() will return zero, just like
> the cpu=0 case. This leads me to suspect that this comparison off by one.
I can push patch that make it if (cpu => port->priv->nthreads). Or even remove
this if.
Anyway on current Armada platforms we have only 4 CPU's and maximum 9 PPv2
threads(nthreads is min between num_present_cpus and maximum HW PPv2 threads),
so this would be always false.
Regards,
Stefan.
RE: [EXT] Re: [PATCH v2 RFC net-next 13/18] net: mvpp2: add ethtool flow control configuration support
>
> --
> On Sun, Jan 24, 2021 at 01:44:02PM +0200, stef...@marvell.com wrote:
> > @@ -6407,6 +6490,29 @@ static void mvpp2_mac_link_up(struct
> phylink_config *config,
> > val);
> > }
> >
> > + if (tx_pause && port->priv->global_tx_fc) {
> > + port->tx_fc = true;
> > + mvpp2_rxq_enable_fc(port);
> > + if (port->priv->percpu_pools) {
> > + for (i = 0; i < port->nrxqs; i++)
> > + mvpp2_bm_pool_update_fc(port, &port-
> >priv->bm_pools[i], true);
> > + } else {
> > + mvpp2_bm_pool_update_fc(port, port->pool_long,
> true);
> > + mvpp2_bm_pool_update_fc(port, port->pool_short,
> true);
> > + }
> > +
> > + } else if (port->priv->global_tx_fc) {
> > + port->tx_fc = false;
> > + mvpp2_rxq_disable_fc(port);
> > + if (port->priv->percpu_pools) {
> > + for (i = 0; i < port->nrxqs; i++)
> > + mvpp2_bm_pool_update_fc(port, &port-
> >priv->bm_pools[i], false);
> > + } else {
> > + mvpp2_bm_pool_update_fc(port, port->pool_long,
> false);
> > + mvpp2_bm_pool_update_fc(port, port->pool_short,
> false);
> > + }
> > + }
> > +
>
> It seems this can be written more succinctly:
>
> if (port->priv->global_tx_fc) {
> port->tx_fc = tx_pause;
> if (tx_pause)
> mvpp2_rxq_enable_fc(port);
> else
> mvpp2_rxq_disable_fc(port);
> if (port->priv->percpu_pools) {
> for (i = 0; i < port->nrxqs; i++)
> mvpp2_bm_pool_update_fc(port,
> &port->priv->bm_pools[i],
> tx_pause);
> } else {
> mvpp2_bm_pool_update_fc(port, port->pool_long,
> tx_pause);
> mvpp2_bm_pool_update_fc(port, port->pool_short,
> tx_pause);
> }
> }
>
Ok, I would update.
Thanks,
Stefan.
RE: [EXT] Re: [PATCH net-next] net: mvpp2: extend mib-fragments name to mib-fragments-err
> > From: Stefan Chulski
> >
> > This patch doesn't change any functionality, but just extend MIB
> > counter register and ethtool-statistic names with "err".
> >
> > The counter MVPP2_MIB_FRAGMENTS_RCVD in fact is Error counter.
> > Extend REG name and appropriated ethtool statistic reg-name with the
> > ERR/err.
>
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -1566,7 +1566,7 @@ static u32 mvpp2_read_index(struct mvpp2
> *priv, u32 index, u32 reg)
> > { MVPP2_MIB_FC_RCVD, "fc_received" },
> > { MVPP2_MIB_RX_FIFO_OVERRUN, "rx_fifo_overrun" },
> > { MVPP2_MIB_UNDERSIZE_RCVD, "undersize_received" },
> > - { MVPP2_MIB_FRAGMENTS_RCVD, "fragments_received" },
> > + { MVPP2_MIB_FRAGMENTS_ERR_RCVD, "fragments_err_received" },
>
> Hi Stefan
>
> I suspect this is now ABI and you cannot change it. You at least need to argue
> why it is not ABI.
>
> Andrew
Hi Andrew,
I not familiar with ABI concept. Does this mean we cannot change, fix or extend
driver ethtool counters?
Thanks,
Stefan.
RE: [EXT] Re: [PATCH RFC net-next 00/19] net: mvpp2: Add TX Flow Control support
> > On Sun, Jan 10, 2021 at 06:55:11PM +, Stefan Chulski wrote: > > > > not connected to the GOP flow control generation mechanism. > > > > To solve this issue Armada has firmware running on CM3 CPU > > > > dedectated for Flow Control support. Firmware monitors Packet > > > > Processor resources and asserts XON/XOFF by writing to Ports Control 0 > Register. > > > > > > What is the minimum firmware version that supports this? > > > > > > > Support were added to firmware about two years ago. > > All releases from 18.09 should has it. > > Can you query the firmware and ask its version? We should keep all this code > disabled if the firmware it too old. > > Andrew This is exactly what " net: mvpp2: add TX FC firmware check " patch do. If handshake of flow control support fail, FC won't be supported. Stefan.
RE: [EXT] Re: [PATCH RFC net-next 00/19] net: mvpp2: Add TX Flow Control support
> > not connected to the GOP flow control generation mechanism. > > To solve this issue Armada has firmware running on CM3 CPU dedectated > > for Flow Control support. Firmware monitors Packet Processor resources > > and asserts XON/XOFF by writing to Ports Control 0 Register. > > What is the minimum firmware version that supports this? > Support were added to firmware about two years ago. All releases from 18.09 should has it. Stefan, Regards.
RE: [EXT] Re: [PATCH RFC net-next 11/19] net: mvpp2: add flow control RXQ and BM pool config callbacks
> Sorry, but that is not really a decision the driver can make. It is part of a > kernel > that _does_ support CPU hotplug, and the online CPUs can be changed today. > > It is likely that every distro out there builds the kernel with CPU hotplug > enabled. > > If changing the online CPUs causes the driver to misbehave, that is a(nother) > bug with the driver. This function doesn't really need to know num_active_cpus, only host ID used by used by shared RX interrupt in single queue mode. Host ID is just register address space used to access PPv2 register space. So I can remove this use of num_active_cpus. Stefan, Regards.
RE: [EXT] Re: [PATCH RFC net-next 14/19] net: mvpp2: add ethtool flow control configuration support
> > @@ -5373,6 +5402,30 @@ static int
> mvpp2_ethtool_set_pause_param(struct net_device *dev,
> > struct ethtool_pauseparam *pause) {
> > struct mvpp2_port *port = netdev_priv(dev);
> > + int i;
> > +
> > + if (pause->tx_pause && port->priv->global_tx_fc) {
> > + port->tx_fc = true;
> > + mvpp2_rxq_enable_fc(port);
> > + if (port->priv->percpu_pools) {
> > + for (i = 0; i < port->nrxqs; i++)
> > + mvpp2_bm_pool_update_fc(port, &port-
> >priv->bm_pools[i], true);
> > + } else {
> > + mvpp2_bm_pool_update_fc(port, port->pool_long,
> true);
> > + mvpp2_bm_pool_update_fc(port, port->pool_short,
> true);
> > + }
> > +
> > + } else if (port->priv->global_tx_fc) {
> > + port->tx_fc = false;
> > + mvpp2_rxq_disable_fc(port);
> > + if (port->priv->percpu_pools) {
> > + for (i = 0; i < port->nrxqs; i++)
> > + mvpp2_bm_pool_update_fc(port, &port-
> >priv->bm_pools[i], false);
> > + } else {
> > + mvpp2_bm_pool_update_fc(port, port->pool_long,
> false);
> > + mvpp2_bm_pool_update_fc(port, port->pool_short,
> false);
> > + }
> > + }
>
> This doesn't look correct to me. This function is only called when ethtool -A
> is
> used to change the flow control settings. This is not the place to be
> configuring flow control, as flow control is negotiated with the link partner.
>
> The final resolved flow control settings are available in
> mvpp2_mac_link_up() via the tx_pause and rx_pause parameters.
I would move this to mvpp2_mac_link_up.
Thanks.
> What also concerns me is whether flow control is supported in the existing
> driver at all, given this patch set. If it isn't supported without the
> firmware's
> help, then we should _not_ be negotiating flow control with the link partner
> unless we actually support it, so the Pause and Asym_Pause bits in
> mvpp2_phylink_validate() should be cleared.
RX FC supported, issue only with TX FC.
Stefan,
Regards.
RE: [EXT] Re: [PATCH RFC net-next 11/19] net: mvpp2: add flow control RXQ and BM pool config callbacks
> >
> > +/* Routine calculate single queue shares address space */ static int
> > +mvpp22_calc_shared_addr_space(struct mvpp2_port *port) {
> > + /* If number of CPU's greater than number of threads, return last
> > +* address space
> > +*/
> > + if (num_active_cpus() >= MVPP2_MAX_THREADS)
> > + return MVPP2_MAX_THREADS - 1;
> > +
> > + return num_active_cpus();
>
> Firstly - this can be written as:
>
> return min(num_active_cpus(), MVPP2_MAX_THREADS - 1);
OK.
> Secondly - what if the number of active CPUs change, for example due to
> hotplug activity. What if we boot with maxcpus=1 and then bring the other
> CPUs online after networking has been started? The number of active CPUs is
> dynamically managed via the scheduler as CPUs are brought online or offline.
>
> > +/* Routine enable flow control for RXQs conditon */ void
> > +mvpp2_rxq_enable_fc(struct mvpp2_port *port)
> ...
> > +/* Routine disable flow control for RXQs conditon */ void
> > +mvpp2_rxq_disable_fc(struct mvpp2_port *port)
>
> Nothing seems to call these in this patch, so on its own, it's not obvious how
> these are being called, and therefore what remedy to suggest for
> num_active_cpus().
I don't think that current driver support CPU hotplug, anyway I can remove
num_active_cpus
and just use shared RX IRQ ID.
Thanks.
.
RE: [EXT] Re: [PATCH RFC net-next 03/19] net: mvpp2: add CM3 SRAM memory map
> > > > + } else {
> > > > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0);
> > > > + if (!priv->sram_pool) {
> > > > + dev_warn(&pdev->dev, "DT is too old, TX FC
> > > disabled\n");
> > >
> > > I don't see anything in this patch that disables TX flow control,
> > > which means this warning message is misleading.
> >
> > OK, I would change to TX FC not supported.
>
> And you should tell phlylink, so it knows to disable it in autoneg.
>
> Which make me wonder, do we need a fix for stable? Has flow control never
> been support in this device up until these patches get merged?
> It should not be negotiated if it is not supported, which means telling
> phylink.
>
>Andrew
TX FC never were really supported. MAC or PHY can negotiated flow control.
But MAC would never trigger FC frame.
Should I prepare separate patch that disable TX FC till we merge this patches?
Regards,
Stefan.
RE: [EXT] Re: [PATCH RFC net-next 03/19] net: mvpp2: add CM3 SRAM memory map
> > + } else {
> > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0);
> > + if (!priv->sram_pool) {
> > + dev_warn(&pdev->dev, "DT is too old, TX FC
> disabled\n");
>
> I don't see anything in this patch that disables TX flow control, which means
> this warning message is misleading.
OK, I would change to TX FC not supported.
Stefan.
RE: [EXT] Re: [PATCH RFC net-next 14/19] net: mvpp2: add ethtool flow control configuration support
> > @@ -5373,6 +5402,30 @@ static int
> mvpp2_ethtool_set_pause_param(struct net_device *dev,
> > struct ethtool_pauseparam *pause) {
> > struct mvpp2_port *port = netdev_priv(dev);
> > + int i;
> > +
> > + if (pause->tx_pause && port->priv->global_tx_fc) {
> > + port->tx_fc = true;
> > + mvpp2_rxq_enable_fc(port);
> > + if (port->priv->percpu_pools) {
> > + for (i = 0; i < port->nrxqs; i++)
> > + mvpp2_bm_pool_update_fc(port, &port-
> >priv->bm_pools[i], true);
> > + } else {
> > + mvpp2_bm_pool_update_fc(port, port->pool_long,
> true);
> > + mvpp2_bm_pool_update_fc(port, port->pool_short,
> true);
> > + }
> > +
> > + } else if (port->priv->global_tx_fc) {
> > + port->tx_fc = false;
> > + mvpp2_rxq_disable_fc(port);
> > + if (port->priv->percpu_pools) {
> > + for (i = 0; i < port->nrxqs; i++)
> > + mvpp2_bm_pool_update_fc(port, &port-
> >priv->bm_pools[i], false);
> > + } else {
> > + mvpp2_bm_pool_update_fc(port, port->pool_long,
> false);
> > + mvpp2_bm_pool_update_fc(port, port->pool_short,
> false);
> > + }
> > + }
>
>
> This looks wrong. Flow control is normally the result of auto negotiation.
> Both
> ends need to agree to it. Which is why
> mvpp2_ethtool_set_pause_param() passes the users request onto phylink.
> phylink will handle the autoneg and then ask the MAC to setup flow control
> depending on the result in mvpp2_mac_link_up().
>
> Andrew
Ok, I would move it to mvpp2_mac_link_up.
Stefan,
Thanks.
RE: [EXT] Re: [PATCH RFC net-next 03/19] net: mvpp2: add CM3 SRAM memory map
>
> > > Should there be -EPROBE_DEFER handling in here somewhere? The SRAM
> > > is a device, so it might not of been probed yet?
> >
>
> > No, firmware probed during bootloader boot and we can use SRAM. SRAM
> > memory can be safely used.
>
> A previous patch added:
>
> + CP11X_LABEL(cm3_sram): cm3@22 {
> + compatible = "mmio-sram";
> + reg = <0x22 0x800>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x22 0x800>;
> + };
> +
>
> So it looks like the SRAM is a device, in the linux driver model. And there
> is a
> driver for this, driver/misc/sram.c. How do you know this device has been
> probed before the Ethernet driver?
>
>Andrew
You right, I would add EPROBE_DEFER if of_gen_pool_get return NULL.
Thanks.
RE: [EXT] Re: [PATCH RFC net-next 06/19] net: mvpp2: increase BM pool size to 2048 buffers
> External Email
>
> --
> On Sun, Jan 10, 2021 at 05:30:10PM +0200, stef...@marvell.com wrote:
> > From: Stefan Chulski
> >
> > BM pool size increased to support Firmware Flow Control.
> > Minimum depletion thresholds to support FC is 1024 buffers.
> > BM pool size increased to 2048 to have some 1024 buffers space between
> > depletion thresholds and BM pool size.
> >
> > Signed-off-by: Stefan Chulski
> > ---
> > drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > index 89b3ede..8dc669d 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> > @@ -851,8 +851,8 @@ enum mvpp22_ptp_packet_format {
> > #define MVPP22_PTP_TIMESTAMPQUEUESELECTBIT(18)
> >
> > /* BM constants */
> > -#define MVPP2_BM_JUMBO_BUF_NUM 512
> > -#define MVPP2_BM_LONG_BUF_NUM 1024
> > +#define MVPP2_BM_JUMBO_BUF_NUM 2048
> > +#define MVPP2_BM_LONG_BUF_NUM 2048
>
> Hi Stefan
>
> Jumbo used to be 1/2 of regular. Do you know why?
>
> It would be nice to have a comment in the commit message about why it is
> O.K. to change the ratio of jumbo to regular frames, and what if anything this
> does for memory requirements.
>
>Andrew
I don't know why it is half(no hardware restrictions for this). I would add to
commit message new memory requirements for buffer allocations.
Thanks.
RE: [EXT] Re: [PATCH RFC net-next 03/19] net: mvpp2: add CM3 SRAM memory map
> -Original Message-
> From: Andrew Lunn
> Sent: Sunday, January 10, 2021 7:05 PM
> To: Stefan Chulski
> Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com;
> da...@davemloft.net; Nadav Haklai ; Yan Markman
> ; linux-ker...@vger.kernel.org; k...@kernel.org;
> li...@armlinux.org.uk; m...@semihalf.com; rmk+ker...@armlinux.org.uk;
> aten...@kernel.org
> Subject: [EXT] Re: [PATCH RFC net-next 03/19] net: mvpp2: add CM3 SRAM
> memory map
>
> External Email
>
> --
> > +static int mvpp2_get_sram(struct platform_device *pdev,
> > + struct mvpp2 *priv)
> > +{
> > + struct device_node *dn = pdev->dev.of_node;
> > + struct resource *res;
> > +
> > + if (has_acpi_companion(&pdev->dev)) {
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > + if (!res) {
> > + dev_warn(&pdev->dev, "ACPI is too old, TX FC
> disabled\n");
> > + return 0;
> > + }
> > + priv->cm3_base = devm_ioremap_resource(&pdev->dev,
> res);
> > + if (IS_ERR(priv->cm3_base))
> > + return PTR_ERR(priv->cm3_base);
> > + } else {
> > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0);
> > + if (!priv->sram_pool) {
> > + dev_warn(&pdev->dev, "DT is too old, TX FC
> disabled\n");
> > + return 0;
> > + }
> > + priv->cm3_base = (void __iomem *)gen_pool_alloc(priv-
> >sram_pool,
> > +
> MSS_SRAM_SIZE);
> > + if (!priv->cm3_base)
> > + return -ENOMEM;
>
> Should there be -EPROBE_DEFER handling in here somewhere? The SRAM is a
> device, so it might not of been probed yet?
No, firmware probed during bootloader boot and we can use SRAM. SRAM memory can
be safely used.
Regards.
RE: [EXT] Re: [PATCH net-next] net: mvpp2: prs: improve ipv4 parse flow
> > -- > On Thu, 17 Dec 2020 18:07:58 +0200 stef...@marvell.com wrote: > > From: Stefan Chulski > > > > Patch didn't fix any issue, just improve parse flow and align ipv4 > > parse flow with ipv6 parse flow. > > > > Currently ipv4 kenguru parser first check IP protocol(TCP/UDP) and > > then destination IP address. > > Patch introduce reverse ipv4 parse, first destination IP address > > parsed and only then IP protocol. > > This would allow extend capability for packet L4 parsing and align > > ipv4 parsing flow with ipv6. > > > > Suggested-by: Liron Himi > > Signed-off-by: Stefan Chulski > > This one will need to wait until after the merge window > > -- > > # Form letter - net-next is closed > > We have already sent the networking pull request for 5.11 and therefore net- > next is closed for new drivers, features, code refactoring and optimizations. > We are currently accepting bug fixes only. > > Please repost when net-next reopens after 5.11-rc1 is cut. OK, Thanks. > Look out for the announcement on the mailing list or check: > https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_- > 7Edavem_net- > 2Dnext.html&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ3dKwkTIxKAl > 6_Bs7GMx4zhJArrXKN2mDMOXGh7lg&m=2CcDqbEJMvxpx15rGBe2og6oh1eZ > hVee8xvK-mjfd0E&s=r1d6bSIPQmjwJqe- > mkU_s5wyqHOU82D18G6SkVuUg5A&e= > > RFC patches sent for review only are obviously welcome at any time. If I post RFC patches for review only, should I add some prefix or tag for this? And if all reviewers OK with change(or no comments at all), should I repost this patch again after net-next opened? Thanks, Stefan.
RE: [EXT] Re: [PATCH net v3] net: mvpp2: Fix GoP port 3 Networking Complex Control configurations
> External Email > > -- > On Thu, 17 Dec 2020 14:37:28 +0200 stef...@marvell.com wrote: > > From: Stefan Chulski > > > > During GoP port 2 Networking Complex Control mode of operation > > configurations, also GoP port 3 mode of operation was wrongly set. > > Patch removes these configurations. > > GENCONF_CTRL0_PORTX naming also fixed. > > Testing the stable backport it looks like this addition change will be > problematic. Not to mention it goes against the "fixes should be minimal" > rule. > > Could you please send just a one liner which removes the offending ORing in > of the bad bit? > > We can do the rename soon after in net-next, the trees are merged pretty > much every week so it won't be a long wait. I would repost with single line change. Regards, Stefan.
RE: [EXT] Re: [PATCH net] net: mvpp2: Add TCAM entry to drop flow control pause frames
> Quoting stef...@marvell.com (2020-12-17 18:45:06)
> > From: Stefan Chulski
> >
> > Issue:
> > Flow control frame used to pause GoP(MAC) was delivered to the CPU and
> > created a load on the CPU. Since XOFF/XON frames are used only by MAC,
> > these frames should be dropped inside MAC.
> >
> > Fix:
> > According to 802.3-2012 - IEEE Standard for Ethernet pause frame has
> > unique destination MAC address 01-80-C2-00-00-01.
> > Add TCAM parser entry to track and drop pause frames by destination MAC.
> >
> > Fixes: db9d7d36eecc ("net: mvpp2: Split the PPv2 driver to a dedicated
> > directory")
>
> Same here, you should go further in the git history.
>
> Also, was that introduced when the TCAM support landed in (overriding its
> default configuration?)? Or is that the behaviour since the beginning? I'm
> asking because while this could very be a fix, it could also fall in the
> improvements category.
I believe this behavior since the beginning of the driver.
I would repost this with fixes tag with patch that introduced this driver.
Regards.
RE: [EXT] Re: [PATCH net] net: mvpp2: prs: fix PPPoE with ipv6 packet parse
> External Email
>
> --
> Hi Stefan,
>
> Quoting stef...@marvell.com (2020-12-17 18:23:11)
> > From: Stefan Chulski
> >
> > Current PPPoE+IPv6 entry is jumping to 'next-hdr'
> > field and not to 'DIP' field as done for IPv4.
> >
> > Fixes: db9d7d36eecc ("net: mvpp2: Split the PPv2 driver to a dedicated
> > directory")
>
> That's not the commit introducing the issue. You can use `git log --follow`
> to go
> further back (or directly pointing to the old mvpp2.c file).
>
> Thanks!
> Antoine
Ok. Probably this issue exist since mvpp2.c driver were pushed to mainline.
Thanks,
Stefan.
RE: [EXT] Re: [PATCH net v2 2/2] net: mvpp2: disable force link UP during port init procedure
> On Thu, Dec 17, 2020 at 12:00:49PM +0100, Marcin Wojtas wrote: > > Hi Stefan, > > > > czw., 17 gru 2020 o 10:42 napisał(a): > > > > > > From: Stefan Chulski > > > > > > Force link UP can be enabled by bootloader during tftpboot and > > > breaks NFS support. > > > Force link UP disabled during port init procedure. > > > > > > Signed-off-by: Stefan Chulski > > > --- > > > > What are the updates against v1? Please note them in this place for > > individual patches and list all in the cover letter (in case sending a > > group of patches). > > It seems the only reason this has been resent is because it's > (incorrectly) part of a series that involved a change to patch 1 (adding the > Fixes: tag). > > As this is a stand-alone fix, it shouldn't be part of a series unless there > really is > some kind of dependency with the other patch(es) of that series. I would repost this two patches separately. Regards, Stefan.
RE: [EXT] Re: [PATCH net 1/2] net: mvpp2: Fix GoP port 3 Networking Complex Control configurations
> -Original Message- > From: Jakub Kicinski > Sent: Thursday, December 17, 2020 2:42 AM > To: Stefan Chulski > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan Markman > ; linux-ker...@vger.kernel.org; > li...@armlinux.org.uk; m...@semihalf.com; and...@lunn.ch; > rmk+ker...@armlinux.org.uk > Subject: [EXT] Re: [PATCH net 1/2] net: mvpp2: Fix GoP port 3 Networking > Complex Control configurations > > External Email > > -- > On Tue, 15 Dec 2020 15:32:12 +0200 stef...@marvell.com wrote: > > From: Stefan Chulski > > > > During GoP port 2 Networking Complex Control mode of operation > > configurations, also GoP port 3 mode of operation was wrongly set mode. > > Patch removes these configurations. > > GENCONF_CTRL0_PORTX naming also fixed. > > Can we get a Fixes tag? Reposting with Fixes tag. Stefan.
RE: [EXT] Re: [PATCH v1] net: mvpp2: divide fifo for dts-active ports only
> -Original Message- > From: Russell King - ARM Linux admin > Sent: Monday, November 23, 2020 7:30 PM > To: Stefan Chulski > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan Markman > ; linux-ker...@vger.kernel.org; k...@kernel.org; > m...@semihalf.com; and...@lunn.ch > Subject: Re: [EXT] Re: [PATCH v1] net: mvpp2: divide fifo for dts-active ports > only > > On Mon, Nov 23, 2020 at 04:03:00PM +, Stefan Chulski wrote: > > I agree with you. We can use "max-speed" for better FIFO allocations. > > I plan to upstream more fixes from the "Marvell" devel branch then I can > prepare this patch. > > So you OK with this patch and then follow-on improvement? > > Yes - but I would like to see the commit description say that this results in > no > change the situation where all three ports are in use. Ok, I would repost patch. Regards.
RE: [EXT] Re: [PATCH v1] net: mvpp2: divide fifo for dts-active ports only
> -Original Message- > From: Russell King - ARM Linux admin > Sent: Monday, November 23, 2020 5:52 PM > To: Stefan Chulski > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan Markman > ; linux-ker...@vger.kernel.org; k...@kernel.org; > m...@semihalf.com; and...@lunn.ch > Subject: Re: [EXT] Re: [PATCH v1] net: mvpp2: divide fifo for dts-active ports > only > > On Mon, Nov 23, 2020 at 03:44:05PM +, Stefan Chulski wrote: > > Yes, but this allocation exists also in current code. > > From HW point of view(MAC and PPv2) maximum supported speed in CP110: > > port 0 - 10G, port 1 - 2.5G, port 2 - 2.5G. > > in CP115: port 0 - 10G, port 1 - 5G, port 2 - 2.5G. > > > > So this allocation looks correct at least for CP115. > > Problem that we cannot reallocate FIFO during runtime, after specific speed > negotiation. > > We could do much better. DT has a "max-speed" property for ethernet > controllers. If we have that property, then I think we should use that to > determine the initialisation time FIFO allocation. > > As I say, on Macchiatobin, the allocations we end up with are just crazy when > you consider the port speeds that the hardware supports. > Maybe that should be done as a follow-on patch - but I think it needs to be > done. I agree with you. We can use "max-speed" for better FIFO allocations. I plan to upstream more fixes from the "Marvell" devel branch then I can prepare this patch. So you OK with this patch and then follow-on improvement? Regards.
RE: [EXT] Re: [PATCH v1] net: mvpp2: divide fifo for dts-active ports only
> > > On Mon, Nov 23, 2020 at 04:52:40PM +0200, stef...@marvell.com wrote: > > > > From: Stefan Chulski > > > > > > > > Tx/Rx FIFO is a HW resource limited by total size, but shared by > > > > all ports of same CP110 and impacting port-performance. > > > > Do not divide the FIFO for ports which are not enabled in DTS, so > > > > active ports could have more FIFO. > > > > > > > > The active port mapping should be done in probe before FIFO-init. > > > > > > It would be nice to know what the effect is from this - is it a > > > small or large boost in performance? > > > > I didn't saw any significant improvement with LINUX bridge or > > forwarding, but this reduced PPv2 overruns drops, reduced CRC sent errors > with DPDK user space application. > > So this improved zero loss throughput. Probably with XDP we would see a > similar effect. > > > > > What is the effect when the ports on a CP110 are configured for 10G, > > > 1G, and 2.5G in that order, as is the case on the Macchiatobin board? > > > > Macchiatobin has two CP's. CP1 has 3 ports, so the distribution of FIFO > > would > be the same as before. > > On CP0 which has a single port, all FIFO would be allocated for 10G port. > > Your code allocates for CP1: > > 32K to port 0 (the 10G port on Macchiatobin) 8K to port 1 (the 1G dedicated > ethernet port on Macchiatobin) 4K to port 2 (the 1G/2.5G SFP port on > Macchiatobin) > > I'm questioning that allocation for port 1 and 2. Yes, but this allocation exists also in current code. >From HW point of view(MAC and PPv2) maximum supported speed in CP110: port 0 - 10G, port 1 - 2.5G, port 2 - 2.5G. in CP115: port 0 - 10G, port 1 - 5G, port 2 - 2.5G. So this allocation looks correct at least for CP115. Problem that we cannot reallocate FIFO during runtime, after specific speed negotiation. Regards.
RE: [EXT] Re: [PATCH v1] net: mvpp2: divide fifo for dts-active ports only
> -Original Message- > From: Russell King - ARM Linux admin > Sent: Monday, November 23, 2020 5:11 PM > To: Stefan Chulski > Cc: netdev@vger.kernel.org; thomas.petazz...@bootlin.com; > da...@davemloft.net; Nadav Haklai ; Yan Markman > ; linux-ker...@vger.kernel.org; k...@kernel.org; > m...@semihalf.com; antoine.ten...@bootlin.com; and...@lunn.ch > Subject: [EXT] Re: [PATCH v1] net: mvpp2: divide fifo for dts-active ports > only > > External Email > > -- > Hi, > > On Mon, Nov 23, 2020 at 04:52:40PM +0200, stef...@marvell.com wrote: > > From: Stefan Chulski > > > > Tx/Rx FIFO is a HW resource limited by total size, but shared by all > > ports of same CP110 and impacting port-performance. > > Do not divide the FIFO for ports which are not enabled in DTS, so > > active ports could have more FIFO. > > > > The active port mapping should be done in probe before FIFO-init. > > It would be nice to know what the effect is from this - is it a small or large > boost in performance? I didn't saw any significant improvement with LINUX bridge or forwarding, but this reduced PPv2 overruns drops, reduced CRC sent errors with DPDK user space application. So this improved zero loss throughput. Probably with XDP we would see a similar effect. > What is the effect when the ports on a CP110 are configured for 10G, 1G, and > 2.5G in that order, as is the case on the Macchiatobin board? Macchiatobin has two CP's. CP1 has 3 ports, so the distribution of FIFO would be the same as before. On CP0 which has a single port, all FIFO would be allocated for 10G port. Regards.
RE: [PATCH] net: mvpp2: divide fifo for dts-active ports only
> -Original Message-
> From: stef...@marvell.com
> Sent: Wednesday, November 18, 2020 8:21 PM
> To: netdev@vger.kernel.org
> Cc: thomas.petazz...@bootlin.com; da...@davemloft.net; Nadav Haklai
> ; Yan Markman ; linux-
> ker...@vger.kernel.org; Stefan Chulski
> Subject: [PATCH] net: mvpp2: divide fifo for dts-active ports only
>
> From: Stefan Chulski
>
> Tx/Rx FIFO is a HW resource limited by total size, but shared by all ports of
> same CP110 and impacting port-performance.
> Do not divide the FIFO for ports which are not enabled in DTS, so active ports
> could have more FIFO.
>
> The active port mapping should be done in probe before FIFO-init.
>
> Signed-off-by: Stefan Chulski
> ---
> drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 23 +++--
> drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 129
> +---
> 2 files changed, 108 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> index 8347758..6bd7e40 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> @@ -695,6 +695,9 @@
> /* Maximum number of supported ports */
> #define MVPP2_MAX_PORTS 4
>
> +/* Loopback port index */
> +#define MVPP2_LOOPBACK_PORT_INDEX3
> +
> /* Maximum number of TXQs used by single port */
> #define MVPP2_MAX_TXQ8
>
> @@ -729,22 +732,21 @@
> #define MVPP2_TX_DESC_ALIGN (MVPP2_DESC_ALIGNED_SIZE
> - 1)
>
> /* RX FIFO constants */
> +#define MVPP2_RX_FIFO_PORT_DATA_SIZE_44KB0xb000
> #define MVPP2_RX_FIFO_PORT_DATA_SIZE_32KB0x8000
> #define MVPP2_RX_FIFO_PORT_DATA_SIZE_8KB 0x2000
> #define MVPP2_RX_FIFO_PORT_DATA_SIZE_4KB 0x1000
> -#define MVPP2_RX_FIFO_PORT_ATTR_SIZE_32KB0x200
> -#define MVPP2_RX_FIFO_PORT_ATTR_SIZE_8KB 0x80
> +#define MVPP2_RX_FIFO_PORT_ATTR_SIZE(data_size) ((data_size) >> 6)
> #define MVPP2_RX_FIFO_PORT_ATTR_SIZE_4KB 0x40
> #define MVPP2_RX_FIFO_PORT_MIN_PKT 0x80
>
> /* TX FIFO constants */
> -#define MVPP22_TX_FIFO_DATA_SIZE_10KB0xa
> -#define MVPP22_TX_FIFO_DATA_SIZE_3KB 0x3
> -#define MVPP2_TX_FIFO_THRESHOLD_MIN 256
> -#define MVPP2_TX_FIFO_THRESHOLD_10KB \
> - (MVPP22_TX_FIFO_DATA_SIZE_10KB * 1024 -
> MVPP2_TX_FIFO_THRESHOLD_MIN)
> -#define MVPP2_TX_FIFO_THRESHOLD_3KB \
> - (MVPP22_TX_FIFO_DATA_SIZE_3KB * 1024 -
> MVPP2_TX_FIFO_THRESHOLD_MIN)
> +#define MVPP22_TX_FIFO_DATA_SIZE_16KB16
> +#define MVPP22_TX_FIFO_DATA_SIZE_10KB10
> +#define MVPP22_TX_FIFO_DATA_SIZE_3KB 3
> +#define MVPP2_TX_FIFO_THRESHOLD_MIN 256 /* Bytes */
> +#define MVPP2_TX_FIFO_THRESHOLD(kb) \
> + ((kb) * 1024 - MVPP2_TX_FIFO_THRESHOLD_MIN)
>
> /* RX buffer constants */
> #define MVPP2_SKB_SHINFO_SIZE \
> @@ -946,6 +948,9 @@ struct mvpp2 {
> /* List of pointers to port structures */
> int port_count;
> struct mvpp2_port *port_list[MVPP2_MAX_PORTS];
> + /* Map of enabled ports */
> + unsigned long port_map;
> +
> struct mvpp2_tai *tai;
>
> /* Number of Tx threads used */
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index f6616c8..9ff5f57 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -6601,32 +6601,56 @@ static void mvpp2_rx_fifo_init(struct mvpp2 *priv)
> mvpp2_write(priv, MVPP2_RX_FIFO_INIT_REG, 0x1); }
>
> -static void mvpp22_rx_fifo_init(struct mvpp2 *priv)
> +static void mvpp22_rx_fifo_set_hw(struct mvpp2 *priv, int port, int
> +data_size)
> {
> - int port;
> + int attr_size = MVPP2_RX_FIFO_PORT_ATTR_SIZE(data_size);
>
> - /* The FIFO size parameters are set depending on the maximum speed
> a
> - * given port can handle:
> - * - Port 0: 10Gbps
> - * - Port 1: 2.5Gbps
> - * - Ports 2 and 3: 1Gbps
> - */
> + mvpp2_write(priv, MVPP2_RX_DATA_FIFO_SIZE_REG(port), data_size);
> + mvpp2_write(priv, MVPP2_RX_ATTR_FIFO_SIZE_REG(port), attr_size); }
>
> - mvpp2_write(priv, MVPP2_RX_DATA_FIFO_SIZE_REG(0),
> - MVPP2_RX_FIFO_PORT_DATA_SIZE_32KB);
> - mvpp2_write(priv, MVPP2_RX_ATTR_FIFO_SIZE_REG(0),
> - MVPP2_RX_FIFO_PORT_ATTR_SIZE_32KB);
> +/* Initialize TX FIFO's: the total FIFO size is 48kB on PPv2.2.
> + * 4kB fixed space must be assigned for the loopback port.
> + * Redistribute remaining avialable 44kB sp
RE: [EXT] Re: [PATCH net-next 3/5] net: mvpp2: cls: Use RSS contexts to handle RSS tables
> -Original Message-
> From: Matteo Croce
> Sent: Saturday, May 9, 2020 3:16 PM
> To: Stefan Chulski
> Cc: David S . Miller ; Maxime Chevallier
> ; netdev ; LKML
> ; Antoine Tenart
> ; Thomas Petazzoni
> ; gregory.clem...@bootlin.com;
> miquel.ray...@bootlin.com; Nadav Haklai ; Marcin
> Wojtas ; Linux ARM ker...@lists.infradead.org>; Russell King - ARM Linux admin
>
> Subject: Re: [EXT] Re: [PATCH net-next 3/5] net: mvpp2: cls: Use RSS contexts
> to
> handle RSS tables
>
> On Sat, May 9, 2020 at 1:16 PM Stefan Chulski wrote:
> > > Hi,
> > >
> > > What do you think about temporarily disabling it like this?
> > >
> > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > > @@ -5775,7 +5775,8 @@ static int mvpp2_port_probe(struct
> > > platform_device *pdev,
> > > NETIF_F_HW_VLAN_CTAG_FILTER;
> > >
> > > if (mvpp22_rss_is_supported()) {
> > > - dev->hw_features |= NETIF_F_RXHASH;
> > > + if (port->phy_interface != PHY_INTERFACE_MODE_SGMII)
> > > + dev->hw_features |= NETIF_F_RXHASH;
> > > dev->features |= NETIF_F_NTUPLE;
> > > }
> > >
> > >
> > > David, is this "workaround" too bad to get accepted?
> >
> > Not sure that RSS related to physical interface(SGMII), better just remove
> NETIF_F_RXHASH as "workaround".
> >
> > Stefan.
>
> Hi,
>
> The point is that RXHASH works fine on all interfaces, but on the gigabit one
> (eth2 usually).
> And on the 10 gbit interface is very very effective, the throughput goes 4x
> when
> enabled, so it would be a big drawback to disable it on all interfaces.
>
> Honestly I don't have any 2.5 gbit hardware to test it on eth3, so I don't
> know if
> rxhash actually only works on the first interface of a unit (so eth0 and
> eth1), or
> if it just doesn't work on the gigabit one.
>
> If someone could test it on the 2.5 gbit port, this will be helpful.
RSS tables is part of Packet Processor IP, not MAC(so it's not related to
specific speed). Probably issue exist on specific packet processor ports.
Since RSS work fine on first port of the CP, we can do the following:
if (port-> id == 0)
dev->hw_features |= NETIF_F_RXHASH;
Regards.
RE: [EXT] Re: [PATCH net-next 3/5] net: mvpp2: cls: Use RSS contexts to handle RSS tables
> -Original Message- > From: Matteo Croce > Sent: Saturday, May 9, 2020 3:13 AM > To: David S . Miller > Cc: Maxime Chevallier ; netdev > ; LKML ; Antoine > Tenart ; Thomas Petazzoni > ; gregory.clem...@bootlin.com; > miquel.ray...@bootlin.com; Nadav Haklai ; Stefan > Chulski ; Marcin Wojtas ; Linux > ARM ; Russell King - ARM Linux admin > > Subject: [EXT] Re: [PATCH net-next 3/5] net: mvpp2: cls: Use RSS contexts to > handle RSS tables > > External Email > > -- > On Thu, Apr 23, 2020 at 7:00 PM Russell King - ARM Linux admin > wrote: > > > > On Tue, Apr 14, 2020 at 01:43:02AM +0200, Matteo Croce wrote: > > > On Tue, Apr 14, 2020 at 1:21 AM Maxime Chevallier > > > wrote: > > > > > > > > The PPv2 controller has 8 RSS tables that are shared across all > > > > ports on a given PPv2 instance. The previous implementation > > > > allocated one table per port, leaving others unused. > > > > > > > > By using RSS contexts, we can make use of multiple RSS tables per > > > > port, one being the default table (always id 0), the other ones > > > > being used as destinations for flow steering, in the same way as rx > > > > rings. > > > > > > > > This commit introduces RSS contexts management in the PPv2 driver. > > > > We always reserve one table per port, allocated when the port is probed. > > > > > > > > The global table list is stored in the struct mvpp2, as it's a > > > > global resource. Each port then maintains a list of indices in > > > > that global table, that way each port can have it's own numbering > > > > scheme starting from 0. > > > > > > > > One limitation that seems unavoidable is that the hashing > > > > parameters are shared across all RSS contexts for a given port. > > > > Hashing parameters for ctx 0 will be applied to all contexts. > > > > > > > > Signed-off-by: Maxime Chevallier > > > > > > Hi all, > > > > > > I noticed that enabling rxhash blocks the RX on my Macchiatobin. It > > > works fine with the 10G ports (the RX rate goes 4x up) but it > > > completely kills the gigabit interface. > > > > > > # 10G port > > > root@macchiatobin:~# iperf3 -c 192.168.0.2 Connecting to host > > > 192.168.0.2, port 5201 [ 5] local 192.168.0.1 port 42394 connected > > > to 192.168.0.2 port 5201 > > > [ ID] Interval Transfer Bitrate Retr Cwnd > > > [ 5] 0.00-1.00 sec 941 MBytes 7.89 Gbits/sec 4030250 KBytes > > > [ 5] 1.00-2.00 sec 933 MBytes 7.82 Gbits/sec 4393240 KBytes > > > root@macchiatobin:~# ethtool -K eth0 rxhash on root@macchiatobin:~# > > > iperf3 -c 192.168.0.2 Connecting to host 192.168.0.2, port 5201 [ > > > 5] local 192.168.0.1 port 42398 connected to 192.168.0.2 port 5201 > > > [ ID] Interval Transfer Bitrate Retr Cwnd > > > [ 5] 0.00-1.00 sec 860 MBytes 7.21 Gbits/sec 428410 KBytes > > > [ 5] 1.00-2.00 sec 859 MBytes 7.20 Gbits/sec 185563 KBytes > > > > > > # gigabit port > > > root@macchiatobin:~# iperf3 -c turbo Connecting to host turbo, port > > > 5201 [ 5] local 192.168.85.42 port 45144 connected to 192.168.85.6 > > > port 5201 > > > [ ID] Interval Transfer Bitrate Retr Cwnd > > > [ 5] 0.00-1.00 sec 113 MBytes 948 Mbits/sec0407 KBytes > > > [ 5] 1.00-2.00 sec 112 MBytes 942 Mbits/sec0428 KBytes > > > root@macchiatobin:~# ethtool -K eth2 rxhash on root@macchiatobin:~# > > > iperf3 -c turbo > > > iperf3: error - unable to connect to server: Resource temporarily > > > unavailable > > > > > > I've bisected and it seems that this commit causes the issue. I > > > tried to revert it on nex-next as a second test, but the code has > > > changed a lot much since, generating too much conflicts. > > > Can you have a look into this? > > > > This behaviour on eth2 is confirmed here on v5.6. Turning on rxhash > > appears to prevent eth2 working. > > > > Maxime, please look into this regression, thanks. > > > > -- > > RMK's Patch system: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.armlinux.org. > > > uk_developer_patches_&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=DDQ3dK > wk
RE: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
> -Original Message- > From: Russell King - ARM Linux admin > Sent: Monday, February 18, 2019 1:28 PM > To: Stefan Chulski > Cc: Antoine Tenart ; da...@davemloft.net; > netdev@vger.kernel.org; linux-ker...@vger.kernel.org; > thomas.petazz...@bootlin.com; maxime.chevall...@bootlin.com; > gregory.clem...@bootlin.com; miquel.ray...@bootlin.com; Nadav Haklai > ; Yan Markman ; > m...@semihalf.com > Subject: Re: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS > while reconfiguring the serdes lanes > > Hi Stefan, > > On Mon, Feb 18, 2019 at 11:08:34AM +, Stefan Chulski wrote: > > HW recommendation upon Serdes reconfiguration are the following: > > > > 1. Disable port(CTRL0_REG - in XLG/GMAC) 2. Put port in reset (both > > XLG/GMAC) 3. For KR - put in reset MPCS (MAC control clock, RX SD > > clock, TX SD clock), XPSC is RXAUI/XAUI clock domain 4. Power down > > Serdes lane > > > > Do reconfiguration of Serdes. > > > > 5. Enable Serdes lane > > 6. Disable MPCS reset for KR > > 7. Disable port reset (both XLG/GMAC) > > 8. Enable port (both XLG/GMAC) > > For clarity, presumably either the XLG or the GMAC should be released from > reset and enabled at any one time depending on the configured mode, but > never both together? Yes. Only one of them the XLG or the GMAC ,depending on the configured mode. Best Regards.
RE: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
> -Original Message- > From: Antoine Tenart > Sent: Monday, February 18, 2019 12:52 PM > To: Russell King - ARM Linux admin > Cc: Antoine Tenart ; da...@davemloft.net; > netdev@vger.kernel.org; linux-ker...@vger.kernel.org; > thomas.petazz...@bootlin.com; maxime.chevall...@bootlin.com; > gregory.clem...@bootlin.com; miquel.ray...@bootlin.com; Nadav Haklai > ; Stefan Chulski ; Yan > Markman ; m...@semihalf.com > Subject: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while > reconfiguring the serdes lanes > > External Email > > -- > Russell, > > On Mon, Feb 18, 2019 at 10:47:57AM +, Russell King - ARM Linux admin > wrote: > > > > Another case that needs to be considered: if the XPCS should be placed > > into reset while reconfiguring the serdes lanes, is the same treatment > > needed for the GMAC? > > That's something I wanted to check as well. I don't know for the GMAC, while > I'm sure the documentation explicitly state to put the XPCS in reset when > reconfiguring the lanes. HW recommendation upon Serdes reconfiguration are the following: 1. Disable port(CTRL0_REG - in XLG/GMAC) 2. Put port in reset (both XLG/GMAC) 3. For KR - put in reset MPCS (MAC control clock, RX SD clock, TX SD clock), XPSC is RXAUI/XAUI clock domain 4. Power down Serdes lane Do reconfiguration of Serdes. 5. Enable Serdes lane 6. Disable MPCS reset for KR 7. Disable port reset (both XLG/GMAC) 8. Enable port (both XLG/GMAC) Stefan, Best Regards.
RE: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
> On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote: > > The documentation advises to set the XPCS in reset while reconfiguring > > the serdes lanes. This seems to be a good thing to do, but the PPv2 > > driver wasn't doing it. This patch fixes it. > > Hmm. That statment seems to have some ambiguity in it - we do two > "reconfigurations" - one may be upon initialisation, where the lane is already > configured for 10Gbase-KR, and we're re-initialising it for the same mode. > The other case is when we're switching between 10Gbase-KR and SGMII, or > as will be the case with 2.5G support for the Alaska PHYs, 2500base-X. Exist one mode that we should add to PPv2&COMPHY 5Gbase-KR(5GBASE-T). Stefan, Best Regards.
RE: [PATCH net] net: mvpp2: 10G modes aren't supported on all ports
> -Original Message-
> From: netdev-ow...@vger.kernel.org
> On Behalf Of Russell King - ARM Linux
> Sent: Tuesday, December 11, 2018 6:37 PM
> To: Antoine Tenart
> Cc: da...@davemloft.net; netdev@vger.kernel.org; linux-
> ker...@vger.kernel.org; thomas.petazz...@bootlin.com;
> maxime.chevall...@bootlin.com; gregory.clem...@bootlin.com;
> miquel.ray...@bootlin.com; Nadav Haklai ; Stefan
> Chulski ; Yan Markman ;
> m...@semihalf.com; Baruch Siach
> Subject: Re: [PATCH net] net: mvpp2: 10G modes aren't supported on all
> ports
>
> On Tue, Dec 11, 2018 at 05:32:28PM +0100, Antoine Tenart wrote:
> > The mvpp2_phylink_validate() function sets all modes that are
> > supported by a given PPv2 port. A recent change made all ports to
> > advertise they support 10G modes in certain cases. This is not true,
> > as only the port #0 can do so. This patch fixes it.
> >
> > Fixes: 01b3fd5ac97c ("net: mvpp2: fix detection of 10G SFP modules")
> > Cc: Baruch Siach
> > Signed-off-by: Antoine Tenart
> > ---
> > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 14 --
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > index 125ea99418df..88aa488054a8 100644
> > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> > @@ -4405,12 +4405,14 @@ static void mvpp2_phylink_validate(struct
> net_device *dev,
> > case PHY_INTERFACE_MODE_10GKR:
> > case PHY_INTERFACE_MODE_XAUI:
>
> Are these modes supported on anything except port 0? If not, you should be
> rejecting these, rather than just treating them as RGMII.
In CP115(which has PPv2) PHY_INTERFACE_MODE_10GKR supported on ports 0 and 1
PHY_INTERFACE_MODE_XAUI/PHY_INTERFACE_MODE_RXAUI supported only on port 0.
Best Regards.
RE: [EXT] Re: [PATCH net-next 02/10] net: phy: phylink: allow 10GKR interface to use in-band negotiation
> -Original Message- > From: Andrew Lunn [mailto:and...@lunn.ch] > Sent: Monday, March 19, 2018 3:08 PM > To: Stefan Chulski > Cc: Antoine Tenart ; Russell King - ARM Linux > ; da...@davemloft.net; kis...@ti.com; > gregory.clem...@bootlin.com; ja...@lakedaemon.net; > sebastian.hesselba...@gmail.com; netdev@vger.kernel.org; linux- > ker...@vger.kernel.org; thomas.petazz...@bootlin.com; > maxime.chevall...@bootlin.com; miquel.ray...@bootlin.com; Nadav Haklai > ; Yan Markman ; > m...@semihalf.com; linux-arm-ker...@lists.infradead.org > Subject: Re: [EXT] Re: [PATCH net-next 02/10] net: phy: phylink: allow 10GKR > interface to use in-band negotiation > > > > If they don't have PHYs, how are the connected to the outside world? > > > > > >Andrew > > > > By external SFP or direct attached cable. > > Maybe i'm missing something, but don't you just need to add an SFP device > in the device tree. The SFP code and PHYLINK will work together, query what > the SFP module is, use the GPIOs to determine link up/down and module > present, and tell the MAC how to configure the MAC-SFP link? > > Andrew phylink pool SFP loss signal to determine link up/down? Stefan, Best Regards.
RE: [EXT] Re: [PATCH net-next 02/10] net: phy: phylink: allow 10GKR interface to use in-band negotiation
> -Original Message- > From: Andrew Lunn [mailto:and...@lunn.ch] > Sent: Monday, March 19, 2018 3:00 PM > To: Antoine Tenart > Cc: Russell King - ARM Linux ; > da...@davemloft.net; kis...@ti.com; gregory.clem...@bootlin.com; > ja...@lakedaemon.net; sebastian.hesselba...@gmail.com; > netdev@vger.kernel.org; linux-ker...@vger.kernel.org; > thomas.petazz...@bootlin.com; maxime.chevall...@bootlin.com; > miquel.ray...@bootlin.com; Nadav Haklai ; Stefan > Chulski ; Yan Markman ; > m...@semihalf.com; linux-arm-ker...@lists.infradead.org > Subject: [EXT] Re: [PATCH net-next 02/10] net: phy: phylink: allow 10GKR > interface to use in-band negotiation > > External Email > > -- > Hi Antoine > > > You guessed right, that's exactly my use case. The DB boards (7k and > > 8k) have 10G interfaces without PHYs. > > If they don't have PHYs, how are the connected to the outside world? > >Andrew By external SFP or direct attached cable. Stefan.
RE: [EXT] Re: [PATCH net-next 02/10] net: phy: phylink: allow 10GKR interface to use in-band negotiation
> > > There is no inband negotiation like there is with 802.3z or SGMII, > > > so this makes no sense. > > > > Oh, that's what I feared. I read some docs but probably will need more > > :) > > > > Anyway, the reason to use in-band negotiation was also to avoid using > > fixed-link. It would work but always report the link is up, which for > > the user isn't a great experience as we have a way to detect this. > > > > What would you suggest to achieve this in a reasonable way? > > The intention of this test in phylink_of_phy_connect() is to avoid failing > when there is no requirement for a PHY to be present (such as a fixed link, or > an 802.3z link.) However, with 10G PHYs such as the 3310, we need the PHY > so we can read the speed from it, and so know whether to downgrade the > MAC to SGMII mode, or having downgraded the MAC, upgrade it back to 10G > mode when the PHY switches to 10G. > > I'm guessing that you're wanting this for the DB boards, but I don't see why. > Do they not have PHYs? New Solid Run board MACCHIATObin Single Shot doesn't has 3310 PHY either, like DB boards. https://www.cnx-software.com/2017/12/20/solidrun-macchiatobin-single-shot-networking-board-launched-for-269-and-up/ Stefan, Best Regards.
RE: [PATCH net-next 5/5] net: mvpp2: jumbo frames support
> > To perform checksum in HW, HW obviously should work in store and > forward mode. Store all frame in TX FIFO and then check checksum. > > If mtu 1500B, everything fine and all port can do this. > > > > If mtu is 9KB and 9KB frame transmitted, Port 0 still can do HW checksum. > But ports 1 and 2 doesn't has enough FIFO for this. > > So we cannot offload this feature and SW should perform checksum. > > So perhaps the real check should not be "port 0", but whether the MTU is > higher or lower than the TX FIFO size assigned to the current port. > This would express in much better way the reason why HW checksum can be > used or not. I really don't want involve MTU size here, for each packet we should add to MTU overhead added by HW(offset, CRC, DSA tags and etc). I prefer just to check: port TX FIFO size is 10KB -> port can support HW checksum offload. Do you suggest to keep some shadow table with ports TX FIFO sizes for this? Thanks, Stefan.
RE: [PATCH net-next 3/5] net: mvpp2: use a data size of 10kB for Tx FIFO on port 0
> -Original Message- > From: Thomas Petazzoni [mailto:thomas.petazz...@bootlin.com] > Sent: Sunday, March 04, 2018 11:25 AM > To: Stefan Chulski > Cc: Antoine Tenart ; da...@davemloft.net; > Yan Markman ; netdev@vger.kernel.org; linux- > ker...@vger.kernel.org; maxime.chevall...@bootlin.com; > gregory.clem...@bootlin.com; miquel.ray...@bootlin.com; Nadav Haklai > ; m...@semihalf.com > Subject: Re: [PATCH net-next 3/5] net: mvpp2: use a data size of 10kB for Tx > FIFO on port 0 > > Hello, > > On Sun, 4 Mar 2018 06:29:59 +, Stefan Chulski wrote: > > > > Is there a reason to hardcode 10KB for port 0, and 3KB for the other ports > ? > > > Would there be use cases where the user may want different > > > configurations ? > > > > Design requirement are 10KB TX FIFO for the 10Gb/sec and 2.5KB for the > 2.5Gb/sec. > > What is a "design requirement" ? Is it a HW design limitation ? We can call it HW design limitation. Anyway to support 10Gb/sec port should have at least 10KB TX FIFO. > So, the limitation has nothing to do with CP110 really, it's just a > limitation of > PPv2.2, and mentioning CP110 in the comment doesn't make much sense, > correct ? I will change it. Stefan.
RE: [PATCH net-next 1/5] net: mvpp2: use the same buffer pool for all ports
> Hello,
>
> On Fri, 2 Mar 2018 16:40:40 +0100, Antoine Tenart wrote:
> > +static struct {
> > + int pkt_size;
> > + int buf_num;
> > +} mvpp2_pools[MVPP2_BM_POOLS_NUM];
>
> Any reason for not doing:
>
> } mvpp2_pools[MVPP2_BM_POOLS_NUM] = {
> [MVPP2_BM_SHORT] = {
> .pkt_size = MVPP2_BM_SHORT_PKT_SIZE,
> .buf_num = MVPP2_BM_SHORT_BUF_NUM
> },
> [MVPP2_BM_LONG] = {
> .pkt_size = MVPP2_BM_LONG_PKT_SIZE,
> .buf_num = MVPP2_BM_LONG_BUF_NUM,
> },
> };
>
> And get rid of:
>
> > +static void mvpp2_setup_bm_pool(void) {
> > + /* Short pool */
> > + mvpp2_pools[MVPP2_BM_SHORT].buf_num =
> MVPP2_BM_SHORT_BUF_NUM;
> > + mvpp2_pools[MVPP2_BM_SHORT].pkt_size =
> MVPP2_BM_SHORT_PKT_SIZE;
> > +
> > + /* Long pool */
> > + mvpp2_pools[MVPP2_BM_LONG].buf_num =
> MVPP2_BM_LONG_BUF_NUM;
> > + mvpp2_pools[MVPP2_BM_LONG].pkt_size =
> MVPP2_BM_LONG_PKT_SIZE; }
>
> ?
>
No, we can change it.
Stefan.
RE: [PATCH net-next 5/5] net: mvpp2: jumbo frames support
> > netdev_err(port->dev, "Invalid pool %d\n", pool);
> > return NULL;
> > }
> > @@ -4596,11 +4604,24 @@ mvpp2_bm_pool_use(struct mvpp2_port
> *port, int
> > pool, int pkt_size) static int mvpp2_swf_bm_pool_init(struct
> > mvpp2_port *port) {
> > int rxq;
> > + enum mvpp2_bm_pool_log_num long_log_pool, short_log_pool;
> > +
> > + /* If port pkt_size is higher than 1518B:
> > +* HW Long pool - SW Jumbo pool, HW Short pool - SW Short pool
>
> The comment is wrong. In this case, the HW short pool is the SW long pool.
You right. Comment is wrong.
> > + if (port->pool_long->id == MVPP2_BM_JUMBO && port->id != 0) {
>
> Again, all over the place we hardcode the fact that Jumbo frames can only be
> used on port 0. I know port 0 is the only one that can do 10G, but are there
> possibly some use cases where you may want Jumbo frame on another port
> ?
>
> This all really feels very hardcoded to me.
>
All ports support Jumbo frames.
But only port 0 can do TX HW checksum offload(due to TX FIFO size).
Packet processor 2.2 has only 19KB TX FIFO size.
So in TX FIFO config code assign for Port 0 - 10KB, Port 1 - 3KB and Port 1 -
3KB.
To perform checksum in HW, HW obviously should work in store and forward mode.
Store all frame in TX FIFO and then check checksum.
If mtu 1500B, everything fine and all port can do this.
If mtu is 9KB and 9KB frame transmitted, Port 0 still can do HW checksum. But
ports 1 and 2 doesn't has enough FIFO for this.
So we cannot offload this feature and SW should perform checksum.
> > + /* 9704 == 9728 - 20 and rounding to 8 */
> > + dev->max_mtu = MVPP2_BM_JUMBO_PKT_SIZE;
>
> Is this correct for all ports ? Shouldn't the maximum MTU be different
> between port 0 (that supports Jumbo frames) and the other ports ?
This is correct for all ports. All ports can support Jumbo frames.
Stefan.
RE: [PATCH net-next 3/5] net: mvpp2: use a data size of 10kB for Tx FIFO on port 0
> On Fri, 2 Mar 2018 16:40:42 +0100, Antoine Tenart wrote: > > > -/* Initialize Tx FIFO's */ > > +/* Initialize Tx FIFO's > > + * The CP110's total tx-fifo size is 19kB. > > + * Use large-size 10kB for fast port but 3kB for others. > > + */ > > Is there a reason to hardcode 10KB for port 0, and 3KB for the other ports ? > Would there be use cases where the user may want different configurations > ? > Design requirement are 10KB TX FIFO for the 10Gb/sec and 2.5KB for the 2.5Gb/sec. Since only port 0 support 10Gb/sec and ports 1&2 support up to 2.5Gb/sec. I don't see any reason to change this configurations. Also TX FIFO size could be set only during probe. > It's just that it feels very "hardcoded" to enforce specifically those > numbers. > > Also, does it make sense to mention the CP110 here ? Is this 19 KB limitation > a limit of the PPv2.2 IP, or of the CP110 ? PPv2.2 IP is part of 110 communication processor. Next communication processor will has different Packet processor or next generation of PPv2.x Limit is PPv2.2 TX FIFO. Stefan.
RE: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface
> Sorry, I find this very confusing. > > It seems we have some people telling me that when there's no PHY described > in DT, we use this link interrupt, and have a functional network interface > (presumably at whatever speed.) I give you couple of examples when we have functional network interface with link interrupt: 1) 10G XLG mode without PHY - since XLG doesn't have auto negation mechanism 2) SGMII with in-band auto negation 3) RGMII with auto negation done previously by boot loader or by change default fixed speed same as speed on cable. Of course correct way to do it in RGMII mode is use values provided by phylink/phylib. Stefan. > > I can't see this working from what you describe - what you describe basically > tells me that when in-band autonegotiation is disabled, and we have no PHY in > the kernel, then effectively we are in fixed-link mode - since we need to know > what speed, duplex and flow control settings to use. > > So, this means that mvpp2 should be enforcing the presence of a fixed-link > description in DT if there is no PHY node at the moment, but it doesn't. > > Instead, it looks to me like the speed and duplex settings are inherited from > the > boot loader or whatever was running before - I can't find anything that > configures MVPP2_GMAC_AUTONEG_CONFIG in this case. That seems quite a > mess. > > Maybe I'm missing something, but I don't see how mvpp2 can be converted to > phylink given this without causing some kind of regression, unless someone can > be much clearer about exactly what kinds of link mvpp2 supports and how they > work (which is basically what I asked back in > October.) > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up > According to speedtest.net: 8.21Mbps down 510kbps up
RE: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface
> > > -Original Message- > > > Hi Russell, > > > > > > Indeed. RGMII MAC behaves same way, although it shouldn't be named > > > as 'in- band' to be on par with the specifications. Anyway - this > > > one is rather a stub for being able to work with ACPI, so once the > > > MDIO bus works there, this will be out of any concerns. > > > > Hi Marcin, > > > > This is correct. > > "in-band" supported only for SGMII mode. > > IRQ link interrupt depend on "in-band"' auto negation only if "in-band"' > enabled. > > But IRQ link interrupt could be triggered with "in-band", "out-band" or with > specific fixed speed/duplex/flow_contol. > > Hi Stefan, > > How does this work in RGMII mode - is this handled by the PP2 polling the PHY > to get the speed, duplex and flow control settings? > IRQ interrupt doesn't handled speed, duplex and flow control settings. It's just raised if number of criterions met: 1) Physical signal detected by MAC 2) MAC auto negotiation succeeded(valid only auto negotiation enabled in MAC and "in-band" bypass disabled) So if auto negotiation mechanism disabled in MAC or bypassed, link status would changes to up and IRQ interrupt be triggered. In case of RGMII mode obviously we don't have "in-band" auto negotiation and "out-band" cannot be used in Kernel(due to missed locks). So auto negotiation should be disabled on MAC level and speed/duplex/flow_contol would be negotiate by PHY. phylink/phylib infrastructure should provide speed/duplex/flow_contol(that were agreed between PHY's) to ppv2 driver. Stefan.
RE: [PATCH net-next v2 4/4] net: mvpp2: 2500baseX support
> -Original Message- > From: Andrew Lunn [mailto:and...@lunn.ch] > Sent: Wednesday, January 03, 2018 5:53 PM > To: Antoine Tenart > Cc: da...@davemloft.net; kis...@ti.com; gregory.clement@free- > electrons.com; li...@armlinux.org.uk; m...@semihalf.com; Stefan Chulski > ; Yan Markman ; > thomas.petazz...@free-electrons.com; miquel.ray...@free-electrons.com; > Nadav Haklai ; netdev@vger.kernel.org; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH net-next v2 4/4] net: mvpp2: 2500baseX support > > On Wed, Jan 03, 2018 at 04:32:27PM +0100, Antoine Tenart wrote: > > Hi Andrew, > > > > On Wed, Jan 03, 2018 at 04:20:36PM +0100, Andrew Lunn wrote: > > > > @@ -4612,6 +4616,9 @@ static int mvpp22_comphy_init(struct > mvpp2_port *port) > > > > case PHY_INTERFACE_MODE_1000BASEX: > > > > mode = PHY_MODE_SGMII; > > > > break; > > > > + case PHY_INTERFACE_MODE_2500BASEX: > > > > + mode = PHY_MODE_2500SGMII; > > > > + break; > > > > > > I think this is the source of confusion with linux/phy.h and > > > linux/phy/phy.h. > > > > > > What would PHY_INTERFACE_MODE_2500SGMII use? > > > > > > Where is this all getting confused? Should the caller to > > > mvpp22_comphy_init() actually be passing > PHY_INTERFACE_MODE_2500SGMII? > > > What is the MAC actually doing at this point? 2500BASEX or 2500SGMII? > > > > PHY_INTERFACE_MODE_2500BASEX is the PHY mode whereas > > PHY_MODE_2500SGMII is the mode used by the common PHY driver (i.e. the > > one configuring the serdes lanes). > > > There's no PHY_INTERFACE_MODE_2500SGMII mode. > > Hi Antoine > > At the moment there is no PHY_INTERFACE_MODE_2500SGMII. However, > there are some devices which can do 2.5G SGMII. So it will appear sometime. > This piece of code then looks even stranger. > > > Sure, I can add a comment to state this function is a translation > > between the net PHY mode and the generic PHY mode (it's a n-to-1 > > translation). > > I think from an API design point of view, passing PHY_MODE_2500BASEX to > comphy makes more sense. That is what the MAC wants to do. How the > comphy achieves that should be internal to the comphy. > >Andrew COMPHY driver configure SERDES lanes to high speed SGMII(2500SGMII) mode and doesn't care about mode used by physical layer(2500BASEX in this case). There are possibility that MAC SERDES lane is routed on a backplane to another SERDES. I think we should use SGMII naming. Stefan.
RE: [EXT] Re: [PATCH net-next v2 4/4] net: mvpp2: 2500baseX support
> -Original Message- > From: Antoine Tenart [mailto:antoine.ten...@free-electrons.com] > Sent: Wednesday, January 03, 2018 5:32 PM > To: Andrew Lunn > Cc: Antoine Tenart ; > da...@davemloft.net; kis...@ti.com; gregory.clem...@free-electrons.com; > li...@armlinux.org.uk; m...@semihalf.com; Stefan Chulski > ; Yan Markman ; > thomas.petazz...@free-electrons.com; miquel.ray...@free-electrons.com; > Nadav Haklai ; netdev@vger.kernel.org; linux- > ker...@vger.kernel.org > Subject: [EXT] Re: [PATCH net-next v2 4/4] net: mvpp2: 2500baseX support > > External Email > > -- > Hi Andrew, > > On Wed, Jan 03, 2018 at 04:20:36PM +0100, Andrew Lunn wrote: > > > @@ -4612,6 +4616,9 @@ static int mvpp22_comphy_init(struct > mvpp2_port *port) > > > case PHY_INTERFACE_MODE_1000BASEX: > > > mode = PHY_MODE_SGMII; > > > break; > > > + case PHY_INTERFACE_MODE_2500BASEX: > > > + mode = PHY_MODE_2500SGMII; > > > + break; > > > > I think this is the source of confusion with linux/phy.h and > > linux/phy/phy.h. > > > > What would PHY_INTERFACE_MODE_2500SGMII use? > > > > Where is this all getting confused? Should the caller to > > mvpp22_comphy_init() actually be passing > PHY_INTERFACE_MODE_2500SGMII? > > What is the MAC actually doing at this point? 2500BASEX or 2500SGMII? > > PHY_INTERFACE_MODE_2500BASEX is the PHY mode whereas > PHY_MODE_2500SGMII is the mode used by the common PHY driver (i.e. the > one configuring the serdes lanes). > > There's no PHY_INTERFACE_MODE_2500SGMII mode. > > > At minimum there needs to be a comment that this is not a typ0, > > otherwise you are going to get patches submitted to 'fix' this. > > Sure, I can add a comment to state this function is a translation between the > net PHY mode and the generic PHY mode (it's a n-to-1 translation). > Maybe we should rename enum phy_mode to comphy_mode and PHY_MODE_2500SGMII to COMPHY_MODE_2500SGMII. Since this enum set MAC to PHY serdes communication mode, not PHY to PHY communication mode. Stefan.
RE: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the fourth network interface
> -Original Message- > From: Marcin Wojtas [mailto:m...@semihalf.com] > Sent: Monday, January 01, 2018 12:18 PM > To: Russell King - ARM Linux > Cc: Stefan Chulski ; Thomas Petazzoni > ; Andrew Lunn ; > Florian Fainelli ; Yan Markman > ; Jason Cooper ; netdev > ; Antoine Tenart electrons.com>; linux-ker...@vger.kernel.org; kis...@ti.com; Nadav Haklai > ; Miquèl Raynal ; > Gregory Clément ; David S. Miller > ; linux-arm-ker...@lists.infradead.org; Sebastian > Hesselbarth > Subject: [EXT] Re: [PATCH net-next 5/6] arm64: dts: marvell: mcbin: enable the > fourth network interface > > External Email > > -- > Hi Russell, > > 2017-12-30 18:31 GMT+01:00 Russell King - ARM Linux > : > > Hi Marcin, > > > > On Sat, Dec 30, 2017 at 05:34:23PM +0100, Marcin Wojtas wrote: > >> Yes, I already split the series and will send first one right away. I > >> will be followed by MDIO bus / PHY handling proposal, including the > >> bits related to phylink. I'm looking forward to your opinion on that > >> once sent. > > > > I'm looking forward to the patches. :) > > > >> This my understanding of how the PP2 HW works in terms of signalling > >> the link interrupt: > >> > >> The full in-band management, similar to mvneta is supported only in > >> the SGMII mode (1G, not sure how it looks like in 2.5G mode). Such > >> handling is not yet implemented in the mvpp2.c > >> > >> 10G: > >> The XGMII MAC (XLG) is capable of generating link status change > >> interrupt upon information provided from the reconciliation layer > >> (RS) of the interface. > >> > >> 2.5G/1G SGMII: > >> Apart from the in-band management, the MAC is also capable of > >> generating IRQ during link-status change. > >> > >> 1G RGMII: > >> I was a bit surprised, but checked on my own - the link change IRQ > >> can be generated here as well. > >> > >> In addition to above the clause 22 PHYs can be automatically polled > >> via SMI bus and provide complete information about link status, > >> speed, etc., reflecting it directly in GMAC status registers. > >> However, this feature had to be disabled, in order not to conflict > >> with SW PHY management of the phylib. > >> > >> Stefan, is above correct? > > > > This sounds very much like mvneta's 'managed = "in-band"' mode. > > Indeed. RGMII MAC behaves same way, although it shouldn't be named as 'in- > band' to be on par with the specifications. Anyway - this one is rather a > stub for > being able to work with ACPI, so once the MDIO bus works there, this will be > out of any concerns. Hi Marcin, This is correct. "in-band" supported only for SGMII mode. IRQ link interrupt depend on "in-band"' auto negation only if "in-band"' enabled. But IRQ link interrupt could be triggered with "in-band", "out-band" or with specific fixed speed/duplex/flow_contol. Best Regards.
RE: [EXT] Re: [PATCH net-next v2] net: mvpp2: add ethtool GOP statistics
> -Original Message- > From: Miquel RAYNAL [mailto:miquel.ray...@free-electrons.com] > Sent: Tuesday, November 07, 2017 12:45 AM > To: Stefan Chulski > Cc: Thomas Petazzoni ; Antoine Tenart > ; David S. Miller > ; netdev@vger.kernel.org > Subject: [EXT] Re: [PATCH net-next v2] net: mvpp2: add ethtool GOP statistics > > External Email > > -- > Hi Stefan, > > +David Miller/Net ML > > > > @@ -6844,6 +7023,10 @@ static int mvpp2_open(struct net_device > > > *dev) > > > > > > mvpp2_start_dev(port); > > > > > > + /* Start hardware statistics gathering */ > > > + queue_delayed_work(priv->stats_queue, &priv->stats_work, > > > +MVPP2_MIB_COUNTERS_STATS_DELAY); > > > + > > > return 0; > > > > > > err_free_link_irq: > > > @@ -6888,6 +7071,9 @@ static int mvpp2_stop(struct net_device *dev) > > > mvpp2_cleanup_rxqs(port); > > > mvpp2_cleanup_txqs(port); > > > > > > + cancel_delayed_work_sync(&priv->stats_work); > > > + flush_workqueue(priv->stats_queue); > > > + > > > > Hi Miquel, > > > > I think there are bug here. > > priv is common for all ports on same CPN and they have same > > priv->stats_work. > > > > For example on A7K board with 3 Ports. queue_delayed_work and > > cancel_delayed_work_sync called for each port stop and start > > procedure. For example: > > If Port0 and Port1 were started, then if only Port0 stopped, delayed > > work would be canceled for both ports. > > > Thanks for spotting it, you are right this is a bug since I moved > starting/stopping > the queues in the opening and close procedure of the ports (to avoid using CPU > time while no interface is actually up). > > Maybe I should have a work per port, it would be easier to handle. > > Thank you, > Miquèl I agree with you. If delayed_work enable/disabled upon port start/stop procedure, it should be per port and gather MIB statistics for single port. Stefan.
RE: [EXT] Re: [PATCH net-next 09/13] net: mvpp2: dynamic reconfiguration of the PHY mode
> > .--- RJ45 > > MVPP2 - 88x3310 PHY > > `--- SFP+ > > And the "MVPP2" part can be expended to: > > .-- GoP #0 --. > MVPP2 - GoP #1 Comphy lane #X -- 88x3310 > `-- GoP #2 --' > > Thanks! > Antoine One more point, Alaska 3310 PHY SoC and Marvell Embedded Processor SoC(A8K) are different SoC's. Comphy driver configures Serdes IP in Marvell Embedded Processor SoC and media(SFP or RJ45) autodetect is done in Alaska 3310 PHY SoC(mostly by firmware). Regards, Stefan
RE: [EXT] Re: [PATCH net-next 09/13] net: mvpp2: dynamic reconfiguration of the PHY mode
> So maybe one confusion was to name them PHY_MODE_10GKR and > PHY_MODE_SGMII. It could be PHY_MODE_10G and PHY_MODE_1G instead. 1G can be RGMII... Regards, Stefan.
RE: [EXT] Re: [PATCH net-next 09/13] net: mvpp2: dynamic reconfiguration of the PHY mode
> > Imagine phylib is using the copper Ethernet PHY, but the MAC is using > > the SFP port. Somebody pulls out the copper cable, phylib says the > > link is down, turns the carrier off and calls the callback. Not good, > > since your SFP cable is still plugged in... Ethtool is > > returning/setting stuff in the Copper Ethernet PHY, when in fact you > > intend to be setting SFP settings. > > I see what could be the issue but I do not understand one aspect though: > how could we switch from one PHY to another, as there's only one output > between the SoC (and so a given GoP#) and the board. So if a given PHY can > handle multiple modes I see, but in the other case a muxing somewhere would > be needed? Or did I miss something? I think PHY name and PHY mode struct that describe here both MAC to PHY and PHY to PHY connection create confusion... Serdes IP lane doesn't care if connector is SFP, RJ45 or direct attached cable. mvpp22_comphy_init only configures MAC to PHY connection. SFI for 10G(KR in mainline), SGMII for 1G and HS_SGMII for 2.5G. Regards, Stefan.
RE: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
> -Original Message- > From: Andrew Lunn [mailto:and...@lunn.ch] > Sent: Thursday, August 24, 2017 4:51 PM > To: Antoine Tenart > Cc: da...@davemloft.net; kis...@ti.com; ja...@lakedaemon.net; > sebastian.hesselba...@gmail.com; gregory.clem...@free-electrons.com; > thomas.petazz...@free-electrons.com; Nadav Haklai ; > li...@armlinux.org.uk; linux-ker...@vger.kernel.org; m...@semihalf.com; > Stefan Chulski ; miquel.ray...@free-electrons.com; > netdev@vger.kernel.org > Subject: Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver > > Hi Antione. > > > How would you name it if not "comphy-cp110"? > > Good question... > > '7000-cpmphy-cp110' > '8000-cpmphy-cp110' > > ?? > > Andrew A8K Marvell SoC has two South Bridge communication controllers and A7K only one communication controllers, but its identical communication controllers 110. Next generation will has another number 1xx. Stefan, Regards.
RE: [EXT] Re: [PATCH net-next 10/18] net: mvpp2: use the GoP interrupt for link status changes
> When the cable is connected (there is signal) and the serdes is in sync and AN > succeeded. > > > With SFF/SFP ports, you generally need a gpio line the fibre module > > can use to indicate if it has link. Fixed-phy has such support, and > > your link_change function will get called when the link changes. > > So that would work when using SFP modules but I wonder if the GoP irq isn't > needed when using passive cable, in which case this patch would still be > needed > (and of course we should support the new Russell phylib capabilities). > > What's your thoughts on this? > > Thanks! > Antoine > Even if new phylib driver supports passive direct cables connection, GoP IRQ required for SOHO/Peridot external switch support. SOHO/Peridot external switch could be connected directly to Serdes line. Regards, Stefan
RE: [EXT] Re: [PATCH net-next 03/18] net: mvpp2: set the SMI PHY address when connecting to the PHY
> In order to safely read/write the PHY, you need to hold the PHY mutex. > Unless the hardware is very smart, please don't enable this. Let the phylib > and > the appropriate PHY driver do the work. > >Andrew Hi Andrew, This feature work only for Out-of-Band Auto-Negotiation in SGMII Mode. Current GoP(MAC) code configure SGMII In-band Auto-Negotiation performed by the PCS layer without PHY polling. Regards, Stefan.
RE: [EXT] Re: [PATCH net-next 03/18] net: mvpp2: set the SMI PHY address when connecting to the PHY
> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Friday, July 28, 2017 7:22 AM
> To: Antoine Tenart
> Cc: da...@davemloft.net; ja...@lakedaemon.net; gregory.clement@free-
> electrons.com; sebastian.hesselba...@gmail.com; thomas.petazzoni@free-
> electrons.com; Nadav Haklai ; li...@armlinux.org.uk;
> m...@semihalf.com; Stefan Chulski ;
> netdev@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Subject: [EXT] Re: [PATCH net-next 03/18] net: mvpp2: set the SMI PHY address
> when connecting to the PHY
>
> External Email
>
> --
> On Thu, Jul 27, 2017 at 06:49:05PM -0700, Antoine Tenart wrote:
> > Hi Andrew,
> >
> > On Wed, Jul 26, 2017 at 06:08:06PM +0200, Andrew Lunn wrote:
> > > On Mon, Jul 24, 2017 at 03:48:33PM +0200, Antoine Tenart wrote:
> > > >
> > > > + if (priv->hw_version != MVPP22)
> > > > + return 0;
> > > > +
> > > > + /* Set the SMI PHY address */
> > > > + if (of_property_read_u32(port->phy_node, "reg", &phy_addr)) {
> > > > + netdev_err(port->dev, "cannot find the PHY address\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + writel(phy_addr, priv->iface_base +
> > > > +MVPP22_SMI_PHY_ADDR(port->gop_id));
> > > > return 0;
> > > > }
> > >
> > > You could use phy_dev->mdiodev->addr, rather than parse the DT.
> >
> > OK.
> >
> > > Why does the MAC need to know this address? The phylib and PHY
> > > driver should be the only thing accessing the PHY, otherwise you are
> > > asking for trouble.
> >
> > This is part of the SMI/xSMI interface. I added into the mvpp2 driver
> > and not in the mvmdio one because the GoP port number must be known to
> > set this register (so that would be even less clean to do it).
>
> Hi Antoine
>
> It is still not clear to my why you need to program the address into the
> hardware. Is the hardware talking to the PHY?
>
> Andrew
Hi Andrew,
This register configures SMI(Serial Management Interface) hardware unit, not
PPv2(Packet Processor) hardware unit.
The SB incorporates the following SMI management interfaces:
MDC - Serial Management Interface Clock , MDIO - Serial Management Interface
Data and complies with IEEE 802.3 Clause 22.
SMI interface used for:
1. PHY register read/write.
The device provides a mechanism for PHY registers read and write access.
2. Auto-Negotiation with PHY devices connected to the GMAC ports.
The device uses a standard master Serial Management Interface for reading
from/writing to the PHY
registers. In addition, the PHY polling unit performs Auto-Negotiation status
update with PHY devices attached
to the Network ports via the Master SMI Interface.
The device polls the Status register of each PHY in a round-robin manner.
If the device detects a change in the link from down to up on 1 of the ports,
it performs a series of
register reads from the PHY and updates the Auto-Negotiation results in the
device's registers. The
Port MAC Status register is updated with these results only if Auto-Negotiation
is enabled.
So SMI interface should know GoP(MAC) id.
Regards,
Stefan.
