Re: Waiting for the PHY to complete auto-negotiation
Mason <slash@free.fr> writes: > On 11/12/2017 15:36, Måns Rullgård wrote: > >> Mason writes: >> >>> I suppose I should test forcefully setting the enable bit to 0 in >>> the driver, and see if hell breaks loose. >> >> You can't. When the enable bit is 1, writes to that register are >> ignored. It goes back to 0 automatically when the hw runs out of >> descriptors. > > I don't think they are ignored, because toggling the control flow > bit actually breaks RX. Oh, then it's even worse than the docs suggest. -- Måns Rullgård
Re: Waiting for the PHY to complete auto-negotiation
Mason <slash@free.fr> writes: > + Mans > > On 09/12/2017 19:49, Florian Fainelli wrote: > >> Having any HW state machine requiring X number of clock cycles to >> guarantee a full transition to a stopped state is not unusual, however, >> the fact that you need to send 5 packets to guarantee an EOC descriptor >> is hit is completely unusual. Ideally there is a single bit that tells >> the DMA engine: you are enabled, do your thing, or you are now disabled, >> and you must stop all accesses to DRAM *now*. That's how sane hardware works. This hardware is not sane. >> So what would be the correct way to quiesce that controller according to >> your HW folks? > > He (it's a single person) offered to run some RTL-level simulations, > but then moved on to more important tasks. At some point he wrote: > >> If you reset the DMA enable in the middle of a DMA, the hardware >> state machine doesn't return to the IDLE state if it has more >> descriptors to process. It should be noted that the RX DMA has been >> designed by our IP vendor with no intention of stopping the DMA in >> the middle of its operation. > > While the documentation for the IP states: > >> Receive DMA Channel Disabling >> >> When the entire receive frame has been read from the Receive FIFO and >> sent over the AMBA bus, the DMA operation ends, and the Receive DMA >> Channel is automatically disabled. To do this, hardware resets the >> Enable bit in the Receive Channel Control Register to "0" after the >> last data has been read from the Receive FIFO and sent over the AMBA >> bus. >> >> When operating in descriptor mode, upon completion of a receive frame >> DMA operation, if the descriptor chain has not ended when a receive >> frame DMA operation completes, the next receive frame DMA operation >> begins. The last descriptor in a descriptor chain is indicated by >> having its End Of Chain- EOC, flag set to "1". If this EOC flag is >> "0", to begin the next receive frame DMA operation, the next >> descriptor is automatically retrieved and used to configure the >> Receive DMA Channel. The Receive DMA Channel is then automatically >> re-enabled and the next receive frame DMA operation begins. >> >> In descriptor mode, an AMBA bus error can occur when reading receive >> descriptor data. If this happens, receive descriptor processing ends >> and the Receive DMA Channel is turned off. The Descriptor Error bit >> in the Receive Status Register is set to "1". > > I don't see how sending "fake" packets through the loop back would be > considered "resetting the DMA enable in the middle of a DMA". > (I'm afraid the HW dev didn't grasp what the driver is doing.) > > I suppose I should test forcefully setting the enable bit to 0 in > the driver, and see if hell breaks loose. You can't. When the enable bit is 1, writes to that register are ignored. It goes back to 0 automatically when the hw runs out of descriptors. -- Måns Rullgård
Re: [PATCH v3 2/4] net: nb8800: Simplify nb8800_pause_config()
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes: > On 15/11/2017 16:03, Andrew Lunn wrote: > >> On Wed, Nov 15, 2017 at 03:33:47PM +0100, Marc Gonzalez wrote: >> >>> On 15/11/2017 15:17, Andrew Lunn wrote: >>> >>> In our local branch, I have completely disabled flow control support, >>> so I don't have to worry about this problem. >> >> That is an interesting statement. You now know there is an issue here, >> your solution is to fix your private branch and leave mainline as is. > > All my patches are NACKed, what would you have me do? > > Moreover, mainline still has the nb8800_dma_stop() work-around, > which Mans has never seen hang. Here's the thing, if that trick doesn't work, then the dma queue filling up from real traffic will also hang the controller, which is a much bigger problem. Your test today suggests that this might be the case. -- Måns Rullgård
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes: > On 14/11/2017 17:55, Måns Rullgård wrote: > >> Marc Gonzalez wrote: >> >>> I will run iperf3 tests with RX_DESC_COUNT lowered to 2. >>> Would that produce conclusive results? >>> Do you have other suggestions? >> >> Leave RX_DESC_COUNT alone but add a delay in the nb8800_poll() loop. >> That should ensure that queue is drained slowly enough for the buffers >> to run out. > > Using the following patch: > > diff --git a/drivers/net/ethernet/aurora/nb8800.c > b/drivers/net/ethernet/aurora/nb8800.c > index 31c3f0f10fbb..646300bb53b6 100644 > --- a/drivers/net/ethernet/aurora/nb8800.c > +++ b/drivers/net/ethernet/aurora/nb8800.c > @@ -317,6 +317,7 @@ static int nb8800_poll(struct napi_struct *napi, int > budget) > > len = RX_BYTES_TRANSFERRED(rxd->report); > > + udelay(200); > if (IS_RX_ERROR(rxd->report)) > nb8800_rx_error(dev, rxd->report); > else > @@ -1416,9 +1417,9 @@ static int nb8800_probe(struct platform_device *pdev) > netdev_info(dev, "MAC address %pM\n", dev->dev_addr); > > /* Auto-negotiate by default */ > - priv->pause_aneg = true; > - priv->pause_rx = true; > - priv->pause_tx = true; > + priv->pause_aneg = false; > + priv->pause_rx = false; > + priv->pause_tx = false; > > priv->ops = match->data; > priv->ops->power_down(dev); > diff --git a/drivers/net/ethernet/aurora/nb8800.h > b/drivers/net/ethernet/aurora/nb8800.h > index 23fefca54804..9b59ea776e4a 100644 > --- a/drivers/net/ethernet/aurora/nb8800.h > +++ b/drivers/net/ethernet/aurora/nb8800.h > @@ -7,7 +7,7 @@ > #include > #include > > -#define RX_DESC_COUNT 256 > +#define RX_DESC_COUNT 16 > #define TX_DESC_COUNT 256 > > #define NB8800_DESC_LOW4 > > I saw both tango4 and tango5 wedged... :-( Well, that's not good. I'll see if I can replicate it later this week. -- Måns Rullgård
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes: > On 14/11/2017 14:54, Måns Rullgård wrote: > >> Marc Gonzalez writes: >> >>> On 14/11/2017 13:40, Måns Rullgård wrote: >>> >>>> Marc Gonzalez wrote: >>>> >>>>> Power entire ethernet block down in ndo_stop(). >>>>> Power it back up in ndo_open() and perform HW init. >>>>> Delete nb8800_dma_stop. >>>> >>>> Leave it alone, please. Not all chips might have a separate power >>>> domain for this. Also, it works just fine on the older chips. >>> >>> There is no need for separate power domains. The ethernet block is >>> clock-gated when it is held in reset. >> >> So you're not powering it down then. Please be accurate. > > Smirk. That looks like trolling. > >>> The reset register is implemented on all tango3, tango4, tango5 chips. >> >> It's still not a core feature. > > Correct. But it covers 100% of all chips using this driver. > There is no point in trying to implement support for chips that > have never existed, do not exist, and never will. You can't know that. >>> nb8800_dma_stop() is a hack. >> >> The hack originated from your company. > > So why are you so insistent that we keep using it? Because it's the only way to support some chip variants. Ones you'd apparently rather forget, but which nonetheless exist. >> Also, I have repeated asked you what happens if the tango5 runs out of >> DMA buffers under normal operation. Does that also cause it to lock up? >> If so, you have a much bigger problem on your hands. > > I will run iperf3 tests with RX_DESC_COUNT lowered to 2. > Would that produce conclusive results? > Do you have other suggestions? Leave RX_DESC_COUNT alone but add a delay in the nb8800_poll() loop. That should ensure that queue is drained slowly enough for the buffers to run out. -- Måns Rullgård
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes: > On 14/11/2017 13:40, Måns Rullgård wrote: > >> Marc Gonzalez wrote: >> >>> Power entire ethernet block down in ndo_stop(). >>> Power it back up in ndo_open() and perform HW init. >>> Delete nb8800_dma_stop. >> >> Leave it alone, please. Not all chips might have a separate power >> domain for this. Also, it works just fine on the older chips. > > There is no need for separate power domains. The ethernet block is > clock-gated when it is held in reset. So you're not powering it down then. Please be accurate. > The reset register is implemented on all tango3, tango4, tango5 chips. It's still not a core feature. > nb8800_dma_stop() is a hack. The hack originated from your company. > The HW dev has stated that it is not supported. One cannot conclude > that it "works fine" just because you've never triggered the error > condition. (On tango5, the error condition triggers systematically.) That sounds like a problem for tango5. Also, I have repeated asked you what happens if the tango5 runs out of DMA buffers under normal operation. Does that also cause it to lock up? If so, you have a much bigger problem on your hands. > We have several customer bug reports on tango3 and tango4 chips complaining > about "broken" ethernet after a link down / link up cycle. They are using a > different driver, but it implements the same hack in enet_stop_rx(). > There is a high probability that the DMA hack is responsible, and wedged the > RX DMA state machine. But you have no idea what's really the problem? -- Måns Rullgård
Re: [PATCH v3 2/4] net: nb8800: Simplify nb8800_pause_config()
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes: > On 14/11/2017 13:38, Måns Rullgård wrote: > >> Marc Gonzalez writes: >> >>> The "flow control enable" bit can be tweaked, even if DMA is enabled. >> >> No, it can't. Maybe on some of your newer chips it can, but not on the >> older ones. > > Again, I suppose you are referring to your SMP8642-based board. > > Are you saying you tested this patch, and it doesn't work? > What are the symptoms? I didn't test that patch per se. I did initially try simply changing that bit, and this didn't work. Either it had no effect, or it locked up the controller, I forget which. The manual explicitly states that writes are forbidden while the DMA enabled bit is set. If shutting down the DMA really isn't possible, I would rather just allow changing the flow control setting only when the interface is stopped. The normal case of getting the initial setting from the auto-negotiation will still work properly. It just won't be possible to change it while the link is active. -- Måns Rullgård
Re: [PATCH v3 1/4] net: nb8800: Drop generic support
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> On 14/11/2017 13:37, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>>
>>> According to our HW dev, there is no provision for software to safely
>>> disable RX DMA in the AU-NB8800 hardware block (ethernet DMA). Thus,
>>> it is the responsibility of the SoC designer to provide such a feature.
>>>
>>> The nb8800_dma_stop() implementation is a clever hack that works most
>>> of the times, but it breaks the DMA state machine in rare cases.
>>>
>>> Therefore, let's drop generic support.
>>>
>>> FWIW, tango chips provide a reset register. When the ethernet block
>>> comes out of reset, DMA is disabled.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
>>> ---
>>> drivers/net/ethernet/aurora/nb8800.c | 3 ---
>>> 1 file changed, 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>> b/drivers/net/ethernet/aurora/nb8800.c
>>> index e94159507847..26f719e2d6ca 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -1335,9 +1335,6 @@ static const struct nb8800_ops nb8800_tango4_ops = {
>>> };
>>>
>>> static const struct of_device_id nb8800_dt_ids[] = {
>>> - {
>>> - .compatible = "aurora,nb8800",
>>> - },
>>> {
>>> .compatible = "sigma,smp8642-ethernet",
>>> .data = _tangox_ops,
>>> --
>>
>> Please leave this. It works just fine on tango3.
>
> What you call "tango3" is your SMP8642-based board, I suppose.
> That is covered by the "sigma,smp8642-ethernet" string.
>
> There is no need for the generic "aurora,nb8800" string, as no other
> known SoC uses the Aurora SSN8800+NB8800 IP; and as I point out in the
> commit message, the raw IP lacks certain features.
>
> There is no point in keeping this around. It just adds unnecessary
> existence tests for every use of the ops struct.
And if someone discovers another chip using this controller, having the
base support in there makes it much easier to get it working.
--
Måns Rullgård
Re: [PATCH v3 4/4] net: nb8800: Add support for suspend/resume
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
Missing patch description. Don't bother though. I won't approve of
this implementation.
Suspend support has to depend on the chip since the EMAC core doesn't
have anything of the kind.
> ---
> drivers/net/ethernet/aurora/nb8800.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index b71d8fb80610..9af2423aed03 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1437,6 +1437,26 @@ static int nb8800_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static int nb8800_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + if (netif_running(dev))
> + return nb8800_stop(dev);
> +
> + return 0;
> +}
> +
> +static int nb8800_resume(struct platform_device *pdev)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + if (netif_running(dev))
> + return nb8800_open(dev);
> +
> + return 0;
> +}
> +
> static struct platform_driver nb8800_driver = {
> .driver = {
> .name = "nb8800",
> @@ -1444,6 +1464,8 @@ static struct platform_driver nb8800_driver = {
> },
> .probe = nb8800_probe,
> .remove = nb8800_remove,
> + .suspend = nb8800_suspend,
> + .resume = nb8800_resume,
> };
>
> module_platform_driver(nb8800_driver);
> --
> 2.15.0
>
--
Måns Rullgård
Re: [PATCH v3 2/4] net: nb8800: Simplify nb8800_pause_config()
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> The "flow control enable" bit can be tweaked, even if DMA is enabled.
No, it can't. Maybe on some of your newer chips it can, but not on the
older ones.
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 18 +-
> 1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 26f719e2d6ca..09b8001e1ecc 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -633,7 +633,6 @@ static void nb8800_pause_config(struct net_device *dev)
> {
> struct nb8800_priv *priv = netdev_priv(dev);
> struct phy_device *phydev = dev->phydev;
> - u32 rxcr;
>
> if (priv->pause_aneg) {
> if (!phydev || !phydev->link)
> @@ -644,22 +643,7 @@ static void nb8800_pause_config(struct net_device *dev)
> }
>
> nb8800_modb(priv, NB8800_RX_CTL, RX_PAUSE_EN, priv->pause_rx);
> -
> - rxcr = nb8800_readl(priv, NB8800_RXC_CR);
> - if (!!(rxcr & RCR_FL) == priv->pause_tx)
> - return;
> -
> - if (netif_running(dev)) {
> - napi_disable(>napi);
> - netif_tx_lock_bh(dev);
> - nb8800_dma_stop(dev);
> - nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx);
> - nb8800_start_rx(dev);
> - netif_tx_unlock_bh(dev);
> - napi_enable(>napi);
> - } else {
> - nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx);
> - }
> + nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx);
> }
>
> static void nb8800_link_reconfigure(struct net_device *dev)
> --
--
Måns Rullgård
Re: [PATCH v3 3/4] net: nb8800: Move HW init to ndo_open()
}
>
> netif_napi_add(dev, >napi, nb8800_poll, NAPI_POLL_WEIGHT);
>
> netdev_info(dev, "MAC address %pM\n", dev->dev_addr);
>
> + /* Auto-negotiate by default */
> + priv->pause_aneg = true;
> + priv->pause_rx = true;
> + priv->pause_tx = true;
> +
> + priv->ops = match->data;
> + priv->ops->power_down(dev);
> +
> return 0;
>
> -err_free_dma:
> - nb8800_dma_free(dev);
> err_deregister_fixed_link:
> if (of_phy_is_fixed_link(pdev->dev.of_node))
> of_phy_deregister_fixed_link(pdev->dev.of_node);
> diff --git a/drivers/net/ethernet/aurora/nb8800.h
> b/drivers/net/ethernet/aurora/nb8800.h
> index 6ec4a956e1e5..23fefca54804 100644
> --- a/drivers/net/ethernet/aurora/nb8800.h
> +++ b/drivers/net/ethernet/aurora/nb8800.h
> @@ -305,11 +305,13 @@ struct nb8800_priv {
> dma_addr_t tx_desc_dma;
>
> struct clk *clk;
> + const struct nb8800_ops *ops;
> };
>
> struct nb8800_ops {
> int (*init)(struct net_device *dev);
> - int (*reset)(struct net_device *dev);
> + void(*power_down)(struct net_device *dev);
> + void(*power_up)(struct net_device *dev);
> };
>
> #endif /* _NB8800_H_ */
> --
> 2.15.0
>
--
Måns Rullgård
Re: [PATCH v3 1/4] net: nb8800: Drop generic support
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> According to our HW dev, there is no provision for software to safely
> disable RX DMA in the AU-NB8800 hardware block (ethernet DMA). Thus,
> it is the responsibility of the SoC designer to provide such a feature.
>
> The nb8800_dma_stop() implementation is a clever hack that works most
> of the times, but it breaks the DMA state machine in rare cases.
>
> Therefore, let's drop generic support.
>
> FWIW, tango chips provide a reset register. When the ethernet block
> comes out of reset, DMA is disabled.
>
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index e94159507847..26f719e2d6ca 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1335,9 +1335,6 @@ static const struct nb8800_ops nb8800_tango4_ops = {
> };
>
> static const struct of_device_id nb8800_dt_ids[] = {
> - {
> - .compatible = "aurora,nb8800",
> - },
> {
> .compatible = "sigma,smp8642-ethernet",
> .data = _tangox_ops,
> --
Please leave this. It works just fine on tango3.
--
Måns Rullgård
Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support
Mason <slash@free.fr> writes: > IIUC, sender (desktop system) sends datagrams as fast as possible. > Receiver (tango board) drops around 24% of all datagrams. > I think this invalidates the theory that exhausting RX descriptors > wedges RX DMA. No, it doesn't. The bottleneck causing packet loss could be somewhere else. -- Måns Rullgård
Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support
Mason <slash@free.fr> writes: > On 02/08/2017 19:31, Mason wrote: > >> # iperf3 -c 172.27.64.45 -u -b 950M >> Connecting to host 172.27.64.45, port 5201 >> [ 4] local 172.27.64.1 port 55533 connected to 172.27.64.45 port 5201 >> [ ID] Interval Transfer Bandwidth Total Datagrams >> [ 4] 0.00-1.00 sec 102 MBytes 858 Mbits/sec 13091 >> [ 4] 1.00-2.00 sec 114 MBytes 953 Mbits/sec 14541 > > 114 MB in 14541 packets => 7840 bytes per packet > Is iperf3 sending jumbo frames?? It's probably sending fragmented packets. -- Måns Rullgård
Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support
Mason <slash@free.fr> writes: > On 02/08/2017 18:10, Måns Rullgård wrote: > >> Mason writes: >> >>> On 02/08/2017 17:56, Måns Rullgård wrote: >>> >>>> What does the tango5 do if you flood it with packets faster than the >>>> kernel can keep up with? That would make it hit the end of the rx >>>> chain, which is apparently what makes it miserable with the current dma >>>> stop code. >>> >>> The simplest way to test this would be sending tiny packets >>> as fast as possible, right? So ping -f on a GigE link should >>> fit the bill? >> >> ping -f is limited to 100 packets per second. Use something like iperf >> in UDP mode instead. > > ping -f can go 100 times faster than 100 pps: > > # ping -f -q -c 15 -s 300 172.27.64.45 > PING 172.27.64.45 (172.27.64.45) 300(328) bytes of data. > > --- 172.27.64.45 ping statistics --- > 15 packets transmitted, 15 received, 0% packet loss, time 15035ms > rtt min/avg/max/mdev = 0.065/0.084/0.537/0.014 ms, ipg/ewma 0.100/0.087 ms > > 150,000 packets in 15 seconds = 10,000 pps > > (172.27.64.45 is the tango5 board) > > Ergo, dealing with 10,000 packets per second does not hose RX. ping -f goes as fast as the other end replies or 100 per second, whichever is higher, so says the man page. -- Måns Rullgård
Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support
Mason <slash@free.fr> writes: > On 02/08/2017 17:56, Måns Rullgård wrote: > >> Mason writes: >> >>> From my perspective, the older method does not work on newer chips :-) >> >> It does work on tango4. > > Agreed. > >> What does the tango5 do if you flood it with packets faster than the >> kernel can keep up with? That would make it hit the end of the rx >> chain, which is apparently what makes it miserable with the current dma >> stop code. > > The simplest way to test this would be sending tiny packets > as fast as possible, right? So ping -f on a GigE link should > fit the bill? ping -f is limited to 100 packets per second. Use something like iperf in UDP mode instead. -- Måns Rullgård
Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support
Mason <slash@free.fr> writes: > On 02/08/2017 17:36, Måns Rullgård wrote: > >> Mason wrote: >> >>> Looking at the tango-specific integration, I note this nugget: >>> >>> 1.5.4 Stopping & Starting the DMA >>> >>> This feature has been added to allow the software to stop and start >>> the DMA without any issues. >>> >>> Procedure: >>> 1- STOP: >>> 2- Stop RX core; >>> 3- Set OWN bit of all descriptor of the chain to 1; >>> 4- Stop DMA by writing dma_stop bit to 1 in RX_DMA_Stop register >>> 5- Wait around 100 clock cycles. >>> >>> The pending packets are held until the system will re-start. >>> >>> RE-START: >>> 1- Clear dma_stop bit (note that if at the time of stopping the DMA, >>> the next packet in the FIFO was an UDP packet, when clearing dma_stop, >>> this packet will directly start being written in the DRAM since UDP >>> packets are not controlled by the descriptor mechanism); >>> 2- Program a new chain of descriptor; >>> 3- Re-enable DMA (rx_ctrl register) >>> >>> rx_dma_stop: >>> Software control to stop the Rx DMA. >>> A write to this bit with "1" will gracefully stop the Rx DMA by after >>> transferring the current packet. If more packets are pending they will >>> be held until the software clears this bit. >>> >>> Hmmm, what do you think? This looks promising... >> >> This is only available in the more recent Sigma versions. Although it >> is nicer, I didn't think it was worth the trouble to support both >> methods since the older method should work on all chips. > > Well, from my perspective, the older method does not work on newer > chips :-) It does work on tango4. What does the tango5 do if you flood it with packets faster than the kernel can keep up with? That would make it hit the end of the rx chain, which is apparently what makes it miserable with the current dma stop code. -- Måns Rullgård
Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support
Mason <slash@free.fr> writes: > On 02/08/2017 16:41, Mason wrote: > >> On 01/08/2017 18:32, Mason wrote: >> >>> I need suspend/resume support in the nb8800 driver. >>> On tango platforms, suspend loses all context (MMIO registers). >>> To make the task easy, we just close the device on suspend, >>> and open it again on resume. This requires properly resetting >>> the HW on resume. >>> >>> Patch 1 moves all the HW init to nb8800_init() >>> Patch 2 adds suspend/resume support >> >> I have now confirmed that the "flow control" issue I reported >> in another thread has nothing to do with flow control per se. >> >> The problem is that nb8800_pause_config() calls nb8800_dma_stop() >> and when it does, RX is borked. >> >> On a GigE switch: >> [ 21.444268] ENTER nb8800_pause_config >> [ 21.448604] rxcr=06100a8f pause_rx=1 pause_tx=0 pause=1 asym_pause=1 >> [ 21.455020] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow >> control rx/tx >> >> In this case, pause_tx and RCR_FL match, so we skip the >> silly dance. > > The documentation states: > > Receive Channel Control Register > Description: > The Receive Channel Control Register holds channel enable, mode, endian, > DMA control, and interrupt control information. This register can only > be written when the Receive DMA Channel is idle - the Enable bit in it is "0". > Register Number: 0x200 > Access: read/write > Reset Value: le = AMBA_LE; all other bits = 0x0 > Fields: > > fl: Flow control enable. "1" indicates flow control is enabled. > When flow control is enabled and a Receive FIFO overrun occurs, > the Ethernet 10/100/1000 Subsystem will send PAUSE frames if in > full duplex mode. This continues until the Receive FIFO is emptied. > > en: Receive DMA Channel enable. "1" indicates that the Receive > DMA Channel is being configured from a descriptor, or that the DMA > operation is in progress. The Receive DMA Channel is idle when > this enable bit is "0". Software sets this bit to "1" to start the > configuration from a descriptor and DMA operation. When the DMA > operation is finished, this bit is automatically reset to "0". Here's the problem. Once started, there is no way to forcibly stop rx dma. It keeps going until it hits a descriptor with the end of chain flag set. >> Receive DMA Channel Disabling >> >> When the entire receive frame has been read from the Receive FIFO and >> sent over the AMBA bus, the DMA operation ends, and the Receive DMA >> Channel is automatically disabled. To do this, hardware resets the >> Enable bit in the Receive Channel Control Register to "0" after the >> last data has been read from the Receive FIFO and sent over the AMBA >> bus. >> >> When operating in descriptor mode, upon completion of a receive frame >> DMA operation, if the descriptor chain has not ended when a receive >> frame DMA operation completes, the next receive frame DMA operation >> begins. The last descriptor in a descriptor chain is indicated by >> having its End Of Chain- EOC, flag set to "1". If this EOC flag is >> "0", to begin the next receive frame DMA operation, the next >> descriptor is automatically retrieved and used to configure the >> Receive DMA Channel. The Receive DMA Channel is then automatically >> re-enabled and the next receive frame DMA operation begins. >> >> In descriptor mode, an AMBA bus error can occur when reading receive >> descriptor data. If this happens, receive descriptor processing ends >> and the Receive DMA Channel is turned off. The Descriptor Error bit >> in the Receive Status Register is set to "1". > > Hmmm, I guess this is what Maxime/Mans did... > > Looking at the tango-specific integration, I note this nugget: > > 1.5.4 Stopping & Starting the DMA > > This feature has been added to allow the software to stop and start > the DMA without any issues. > > Procedure: > 1- STOP: > 2- Stop RX core; > 3- Set OWN bit of all descriptor of the chain to 1; > 4- Stop DMA by writing dma_stop bit to 1 in RX_DMA_Stop register > 5- Wait around 100 clock cycles. > > The pending packets are held until the system will re-start. > > RE-START: > 1- Clear dma_stop bit (note that if at the time of stopping the DMA, > the next packet in the FIFO was an UDP packet, when clearing dma_stop, > this packet will directly start being written in the DRAM since UDP > packets are not controlled by the descriptor mechanism); > 2- Program a new chain of descriptor; > 3- Re-enable DMA (rx_ctrl register) > > rx_dma_stop: > Software control to stop the Rx DMA. > A write to this bit with “1” will gracefully stop the Rx DMA by after > transferring the current packet. If more packets are pending they will > be held until the software clears this bit. > > Hmmm, what do you think? This looks promising... This is only available in the more recent Sigma versions. Although it is nicer, I didn't think it was worth the trouble to support both methods since the older method should work on all chips. -- Måns Rullgård
Re: [RFC PATCH v2 1/2] net: ethernet: nb8800: Reset HW block in ndo_open
Mason <slash@free.fr> writes: > On 02/08/2017 13:02, Måns Rullgård wrote: > >> Mason wrote: >> >>> Move all HW initializations to nb8800_init. >>> This provides the basis for suspend/resume support. >>> --- >>> drivers/net/ethernet/aurora/nb8800.c | 50 >>> +--- >>> drivers/net/ethernet/aurora/nb8800.h | 1 + >>> 2 files changed, 25 insertions(+), 26 deletions(-) >> >> You're still not doing anything to preserve flow control and multicast >> configuration, and those are probably not the only ones. > > I did handle flow control (as far as I can tell). > The current settings are stored in: > priv->pause_aneg, priv->pause_rx, priv->pause_tx > and nb8800_pause_config() is called from nb8800_link_reconfigure() I think the whole pause setup needs to be revisited. It's a mess. Maybe I'll find the time one day. > I'll take a closer look at multicast settings. You're currently losing all multicast setup as well as promiscuous mode if that has been enabled. >> Worse, you're now never tearing down dma properly, which means the >> hardware can be left with active pointers to memory no longer allocated >> to it. > > You're making it sound like there is a risk those pointers > might be used somehow. There is no such risk AFAICT. > The PHY is disconnected, NAPI is stopped, RX and TX have > been disabled, and the ISR has been removed. The interrupt handler and NAPI have nothing to do with it since they only get involved *after* the hw has filled the buffers. If you've given the hardware control of a memory buffer, you should damn well take it back before reusing that memory for something else. Otherwise you'll eventually have a very difficult to debug problem and a security risk. > I have considered putting the HW block in reset (clock gated) > at the end of nb8800_stop() to save power, which would make > the issue you point out moot. That's not necessarily possible. It is on Sigma SoCs, but it doesn't have to be. > The reason I removed nb8800_dma_stop() in nb8800_stop() > is because it hangs when called from nb8800_suspend(). > (It requires interrupts to make progress, and interrupts > seem to be disabled when nb8800_suspend() is called.) It shouldn't require interrupts. If it does for some reason, that should be fixed. > Broader question for netdev: > > I've been wondering about costly close operations. > If closing a device is complex (in terms of code), at which > point does it become simpler to just reset the block, and > avoid writing/maintaining/debugging the code performing > said close operation? > >> Finally, you still haven't explained why the hw needs to be reset in >> ndo_open(). Whatever is causing your lockup can almost certainly be >> triggered in some other way too. I will not accept this side-stepping >> of the issue. > > (I was not aware that you were the final authority on which > patches are accepted upstream.) > > FWIW, I have placed a formal request for the HW dev to > investigate, as I could not reproduce on tango4, despite > several days wasted on this issue. > > In the mean time, the driver in its current form does not > support suspend/resume. How would *you* implement it? I'm not saying it's a bad idea for suspend. It might even be the only way. -- Måns Rullgård
Re: [RFC PATCH v2 1/2] net: ethernet: nb8800: Reset HW block in ndo_open
gt;napi, nb8800_poll, NAPI_POLL_WEIGHT);
> @@ -1492,8 +1492,6 @@ static int nb8800_probe(struct platform_device *pdev)
>
> return 0;
>
> -err_free_dma:
> - nb8800_dma_free(dev);
> err_deregister_fixed_link:
> if (of_phy_is_fixed_link(pdev->dev.of_node))
> of_phy_deregister_fixed_link(pdev->dev.of_node);
> diff --git a/drivers/net/ethernet/aurora/nb8800.h
> b/drivers/net/ethernet/aurora/nb8800.h
> index 6ec4a956e1e5..d5f4481a2c7b 100644
> --- a/drivers/net/ethernet/aurora/nb8800.h
> +++ b/drivers/net/ethernet/aurora/nb8800.h
> @@ -305,6 +305,7 @@ struct nb8800_priv {
> dma_addr_t tx_desc_dma;
>
> struct clk *clk;
> + const struct nb8800_ops *ops;
> };
>
> struct nb8800_ops {
> --
> 2.11.0
--
Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Mason <slash@free.fr> writes:
> On 31/07/2017 16:08, Mason wrote:
>
>> Other things make no sense to me, for example in nb8800_dma_stop()
>> there is a polling loop:
>>
>> do {
>> mdelay(100);
>> nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>> wmb();
>> mdelay(100);
>> nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
>>
>> mdelay(5500);
>>
>> err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
>> rxcr, !(rxcr & RCR_EN),
>> 1000, 10);
>> printk("err=%d retry=%d\n", err, retry);
>> } while (err && --retry);
>>
>>
>> (It was me who added the delays.)
>>
>> *Whatever* delays I insert, it always goes 3 times through the loop.
>>
>> [ 29.654492] ++ETH++ gw32 reg=f002610c val=9ecc8000
>> [ 29.759320] ++ETH++ gw32 reg=f0026100 val=005c0aff
>> [ 35.364705] err=-110 retry=5
>> [ 35.467609] ++ETH++ gw32 reg=f002610c val=9ecc8000
>> [ 35.572436] ++ETH++ gw32 reg=f0026100 val=005c0aff
>> [ 41.177822] err=-110 retry=4
>> [ 41.280726] ++ETH++ gw32 reg=f002610c val=9ecc8000
>> [ 41.385553] ++ETH++ gw32 reg=f0026100 val=005c0aff
>> [ 46.890907] err=0 retry=3
>>
>> How is that possible?
>
> First time through the loop, it doesn't matter how long we poll,
> it *always* times out. Second time as well (only on BOARD B).
>
> Third time, it succeeds quickly (first or second poll).
> (This explains why various delays had no impact.)
>
> In fact, requesting the transfer 3 times *before* polling
> makes the polling succeed quickly:
>
> nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> wmb();
> nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
>
> [ 16.464596] ++ETH++ gw32 reg=f002610c val=9ef28000
> [ 16.469414] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 16.474231] ++ETH++ gw32 reg=f002610c val=9ef28000
> [ 16.479048] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 16.483865] ++ETH++ gw32 reg=f002610c val=9ef28000
> [ 16.488682] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 16.493500] ++ETH++ POLL reg=f0026200 val=06100a8f
> [ 16.499317] ++ETH++ POLL reg=f0026200 val=06100a8e
> [ 16.504134] err=0 retry=5
That strengthens my theory that the hardware has an internal queue of
three descriptors that are pre-loaded from memory. Your hardware people
should be able to confirm this.
> With my changes, I get *exactly* the same logs on BOARD A
> and BOARD B (modulo the descriptors addresses).
>
> Yet BOARD A stays functional, but BOARD B is hosed...
What's the difference between board A and board B?
> Depressing. I've run out of ideas.
Get your hardware people involved. Perhaps they can run some test in a
simulator.
--
Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Mason <slash@free.fr> writes:
> On 31/07/2017 13:59, Måns Rullgård wrote:
>
>> Mason writes:
>>
>>> On 29/07/2017 17:18, Florian Fainelli wrote:
>>>
>>>> On 07/29/2017 05:02 AM, Mason wrote:
>>>>
>>>>> I have identified a 100% reproducible flaw.
>>>>> I have proposed a work-around that brings this down to 0
>>>>> (tested 1000 cycles of link up / ping / link down).
>>>>
>>>> Can you also try to get help from your HW resources to eventually help
>>>> you find out what is going on here?
>>>
>>> The patch I proposed /is/ based on the feedback from the HW team :-(
>>> "Just reset the HW block, and everything will work as expected."
>>
>> Nobody is saying a reset won't recover the lockup. The problem is that
>> we don't know what caused it to lock up in the first place. How do we
>> know it can't happen during normal operation? If we knew the cause, it
>> might also be possible to avoid the situation entirely.
>
> How does one prove that something "can't happen during normal operation"?
One figures out what conditions cause the something and ensures they
never arise.
> The "put adapter in loop-back mode so we can send ourselves fake packets"
> shenanigans seems completely insane, if you ask me.
Blame the hardware designers. The *only* way to stop the rx dma is to
have it receive a packet into a descriptor with the end of chain flag
set. Thankfully the loopback mode means this can be made to happen at
will rather than waiting for actual network traffic.
> Other things make no sense to me, for example in nb8800_dma_stop()
> there is a polling loop:
>
> do {
> mdelay(100);
> nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> wmb();
> mdelay(100);
> nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
>
> mdelay(5500);
>
> err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
> rxcr, !(rxcr & RCR_EN),
> 1000, 10);
> printk("err=%d retry=%d\n", err, retry);
> } while (err && --retry);
>
> (It was me who added the delays.)
>
> *Whatever* delays I insert, it always goes 3 times through the loop.
>
> [ 29.654492] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 29.759320] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 35.364705] err=-110 retry=5
> [ 35.467609] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 35.572436] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 41.177822] err=-110 retry=4
> [ 41.280726] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 41.385553] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 46.890907] err=0 retry=3
>
> How is that possible?
One possibility is that the hardware loads three descriptors in advance
and doesn't see the newly set end of chain flag until its internal queue
has been used up.
> I've tried using spinlocks and delays to get parallel execution
> down to a minimum, and have the same logs on both boards.
>
> Regards.
--
Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Mason <slash@free.fr> writes: > On 29/07/2017 17:18, Florian Fainelli wrote: > >> On 07/29/2017 05:02 AM, Mason wrote: >> >>> I have identified a 100% reproducible flaw. >>> I have proposed a work-around that brings this down to 0 >>> (tested 1000 cycles of link up / ping / link down). >> >> Can you also try to get help from your HW resources to eventually help >> you find out what is going on here? > > The patch I proposed /is/ based on the feedback from the HW team :-( > "Just reset the HW block, and everything will work as expected." Nobody is saying a reset won't recover the lockup. The problem is that we don't know what caused it to lock up in the first place. How do we know it can't happen during normal operation? If we knew the cause, it might also be possible to avoid the situation entirely. -- Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Mason <slash@free.fr> writes: > On 29/07/2017 14:05, Måns Rullgård wrote: > >> Mason writes: >> >>> I'll take this opportunity to change flow control to >>> off by default (it breaks several 100 Mbps switches). >> >> I was told to have it on by default. This is what most other drivers >> do too. If you have faulty switches, that's your problem. > > Or maybe the fault is in the HW implementation, and > it is the board's fault that the connection is hosed. > > How many switches did you test the driver on? > > We tested 4 switches, and DHCP failed on 3 of them. > Disabling pause frames "fixed" that. Do those switches work with other NICs? > I could spend weeks testing dozens of switches, but > I have to prioritize. Ethernet flow control is simply > not an important feature in the market the SoC is > intended for. Sure, flow control is rarely needed with well behaved systems. -- Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Mason <slash@free.fr> writes: > On 29/07/2017 13:24, Måns Rullgård wrote: > >> Until you figure out why it's getting stuck, we can't be sure >> it isn't caused by something that could trigger at any time. > Would you take a look at it, if I can reproduce on tango4? > > I have identified a 100% reproducible flaw. > I have proposed a work-around that brings this down to 0 > (tested 1000 cycles of link up / ping / link down). > > In my opinion, upstream should consider this work-around > for inclusion. I'd like to hear David's and Florian's > opinion on the topic. It's always a pain to maintain > out-of-tree patches. I'm not saying it shouldn't be fixed. I am saying we should make sure we make the right fix, not just paper over one instance of a wider issue. >> Yes, but by then you've reset those parameters to the defaults. > > Good catch. There is some non HW-related init in > nb8800_hw_init(). > > I'll take this opportunity to change flow control to > off by default (it breaks several 100 Mbps switches). I was told to have it on by default. This is what most other drivers do too. If you have faulty switches, that's your problem. -- Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Mason <slash@free.fr> writes:
> On 28/07/2017 20:56, Måns Rullgård wrote:
>
>> Marc Gonzalez writes:
>>
>>> On 28/07/2017 18:17, Måns Rullgård wrote:
>>>
>>>> Marc Gonzalez wrote:
>>>>
>>>>> ndo_stop breaks RX in a way that ndo_open is unable to undo.
>>>>
>>>> Please elaborate. Why can't it be fixed in a less heavy-handed way?
>>>
>>> I'm not sure what "elaborate" means. After we've been through
>>> ndo_stop once, the board can send packets, but it doesn't see
>>> any replies from remote systems. RX is wedged.
>>
>> So you say, but you have not explained why this happens. Until we know
>> why, we can't decide on the proper fix.
>
> I'll try adding delays in strategic places, and see if
> I can trigger the same bug on tango4. If I can't, then
> this work around is all we've got.
>
> And I need nb8800_init for resume anyway, so I might
> as well use it in ndo_open.
>
> TODO: test power savings from holding HW in reset.
>
>>> I think ndo_stop is rare enough an event that doing a full
>>> reset is not an issue, in terms of performance.
>>
>> Performance isn't the issue. Doing the right thing is.
>
> I don't have always have time to figure out exactly how
> broken HW is broken. It's already bad enough that disabling
> DMA requires sending a fake packet through the loop back...
Until you figure out why it's getting stuck, we can't be sure it isn't
caused by something that could trigger at any time.
>>>> I'm pretty sure this doesn't preserve everything it should.
>>>
>>> Hmmm, we're supposed to start fresh ("full reset").
>>> What could there be to preserve?
>>> You mentioned flow control and multicast elsewhere.
>>> I will take a closer look. Thanks for the heads up.
>>
>> Yes, those settings are definitely lost with your patch. Now I'm not
>> sure whether the networking core expects these to survive a stop/start
>> cycle, so please check that. There might also be other less obvious
>> things that need to be preserved.
>
> The original code calls nb8800_pause_config() every
> time the link comes up. The proposed patch doesn't
> change that.
Yes, but by then you've reset those parameters to the defaults.
--
Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> On 28/07/2017 18:17, Måns Rullgård wrote:
>
>> Marc Gonzalez wrote:
>>
>>> ndo_stop breaks RX in a way that ndo_open is unable to undo.
>>
>> Please elaborate. Why can't it be fixed in a less heavy-handed way?
>
> I'm not sure what "elaborate" means. After we've been through
> ndo_stop once, the board can send packets, but it doesn't see
> any replies from remote systems. RX is wedged.
So you say, but you have not explained why this happens. Until we know
why, we can't decide on the proper fix.
> I think ndo_stop is rare enough an event that doing a full
> reset is not an issue, in terms of performance.
Performance isn't the issue. Doing the right thing is.
> Also I will need this infrastructure anyway for suspend/resume
> support.
> It might also make sense to put the HW in reset at close
> (to save power). I will try measuring the power savings,
> if any.
>
>>> Work around the issue by resetting the HW in ndo_open.
>>> This will provide the basis for suspend/resume support.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
>>> ---
>>> drivers/net/ethernet/aurora/nb8800.c | 40
>>> +---
>>> drivers/net/ethernet/aurora/nb8800.h | 1 +
>>> 2 files changed, 20 insertions(+), 21 deletions(-)
>>
>> I'm pretty sure this doesn't preserve everything it should.
>
> Hmmm, we're supposed to start fresh ("full reset").
> What could there be to preserve?
> You mentioned flow control and multicast elsewhere.
> I will take a closer look. Thanks for the heads up.
Yes, those settings are definitely lost with your patch. Now I'm not
sure whether the networking core expects these to survive a stop/start
cycle, so please check that. There might also be other less obvious
things that need to be preserved.
--
Måns Rullgård
Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> ndo_stop breaks RX in a way that ndo_open is unable to undo.
Please elaborate. Why can't it be fixed in a less heavy-handed way?
> Work around the issue by resetting the HW in ndo_open.
> This will provide the basis for suspend/resume support.
>
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 40
> +---
> drivers/net/ethernet/aurora/nb8800.h | 1 +
> 2 files changed, 20 insertions(+), 21 deletions(-)
I'm pretty sure this doesn't preserve everything it should.
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index e94159507847..954a28542c3b 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -39,6 +39,7 @@
>
> #include "nb8800.h"
>
> +static void nb8800_init(struct net_device *dev);
> static void nb8800_tx_done(struct net_device *dev);
> static int nb8800_dma_stop(struct net_device *dev);
>
> @@ -957,6 +958,8 @@ static int nb8800_open(struct net_device *dev)
> struct phy_device *phydev;
> int err;
>
> + nb8800_init(dev);
> +
> /* clear any pending interrupts */
> nb8800_writel(priv, NB8800_RXC_SR, 0xf);
> nb8800_writel(priv, NB8800_TXC_SR, 0xf);
> @@ -1350,6 +1353,20 @@ static const struct of_device_id nb8800_dt_ids[] = {
> };
> MODULE_DEVICE_TABLE(of, nb8800_dt_ids);
>
> +static void nb8800_init(struct net_device *dev)
> +{
> + struct nb8800_priv *priv = netdev_priv(dev);
> + const struct nb8800_ops *ops = priv->ops;
> +
> + if (ops && ops->reset)
> + ops->reset(dev);
> + nb8800_hw_init(dev);
> + if (ops && ops->init)
> + ops->init(dev);
> + nb8800_update_mac_addr(dev);
> + priv->speed = 0;
> +}
> +
> static int nb8800_probe(struct platform_device *pdev)
> {
> const struct of_device_id *match;
> @@ -1389,6 +1406,7 @@ static int nb8800_probe(struct platform_device *pdev)
>
> priv = netdev_priv(dev);
> priv->base = base;
> + priv->ops = ops;
>
> priv->phy_mode = of_get_phy_mode(pdev->dev.of_node);
> if (priv->phy_mode < 0)
> @@ -1407,12 +1425,6 @@ static int nb8800_probe(struct platform_device *pdev)
>
> spin_lock_init(>tx_lock);
>
> - if (ops && ops->reset) {
> - ret = ops->reset(dev);
> - if (ret)
> - goto err_disable_clk;
> - }
> -
> bus = devm_mdiobus_alloc(>dev);
> if (!bus) {
> ret = -ENOMEM;
> @@ -1454,16 +1466,6 @@ static int nb8800_probe(struct platform_device *pdev)
>
> priv->mii_bus = bus;
>
> - ret = nb8800_hw_init(dev);
> - if (ret)
> - goto err_deregister_fixed_link;
> -
> - if (ops && ops->init) {
> - ret = ops->init(dev);
> - if (ret)
> - goto err_deregister_fixed_link;
> - }
> -
> dev->netdev_ops = _netdev_ops;
> dev->ethtool_ops = _ethtool_ops;
> dev->flags |= IFF_MULTICAST;
> @@ -1476,14 +1478,12 @@ static int nb8800_probe(struct platform_device *pdev)
> if (!is_valid_ether_addr(dev->dev_addr))
> eth_hw_addr_random(dev);
>
> - nb8800_update_mac_addr(dev);
> -
> netif_carrier_off(dev);
>
> ret = register_netdev(dev);
> if (ret) {
> netdev_err(dev, "failed to register netdev\n");
> - goto err_free_dma;
> + goto err_deregister_fixed_link;
> }
>
> netif_napi_add(dev, >napi, nb8800_poll, NAPI_POLL_WEIGHT);
> @@ -1492,8 +1492,6 @@ static int nb8800_probe(struct platform_device *pdev)
>
> return 0;
>
> -err_free_dma:
> - nb8800_dma_free(dev);
> err_deregister_fixed_link:
> if (of_phy_is_fixed_link(pdev->dev.of_node))
> of_phy_deregister_fixed_link(pdev->dev.of_node);
> diff --git a/drivers/net/ethernet/aurora/nb8800.h
> b/drivers/net/ethernet/aurora/nb8800.h
> index 6ec4a956e1e5..d5f4481a2c7b 100644
> --- a/drivers/net/ethernet/aurora/nb8800.h
> +++ b/drivers/net/ethernet/aurora/nb8800.h
> @@ -305,6 +305,7 @@ struct nb8800_priv {
> dma_addr_t tx_desc_dma;
>
> struct clk *clk;
> + const struct nb8800_ops *ops;
> };
>
> struct nb8800_ops {
> --
> 2.11.0
>
--
Måns Rullgård
Re: [PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> On 21/07/2017 15:47, Måns Rullgård wrote:
>
>> Marc Gonzalez wrote:
>>
>>> On 21/07/2017 15:04, Måns Rullgård wrote:
>>>
>>>> Marc Gonzalez wrote:
>>>>
>>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>>> there are 4 RGMII modes to handle:
>>>>>
>>>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>>>> the MAC should not add the RX or TX delays in this case)
>>>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>>>> the MAC should not add an RX delay in this case)
>>>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>>>> the MAC should not add an TX delay in this case)
>>>>>
>>>>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>>>>
>>>>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
>>>>> ---
>>>>> drivers/net/ethernet/aurora/nb8800.c | 6 --
>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>>>> b/drivers/net/ethernet/aurora/nb8800.c
>>>>> index ded041dbafe7..f3ed320eb4ad 100644
>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device
>>>>> *dev)
>>>>> break;
>>>>>
>>>>> case PHY_INTERFACE_MODE_RGMII:
>>>>> - pad_mode = PAD_MODE_RGMII;
>>>>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>>>>> + pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>> break;
>>>>>
>>>>> + case PHY_INTERFACE_MODE_RGMII_ID:
>>>>> case PHY_INTERFACE_MODE_RGMII_TXID:
>>>>> - pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>> + pad_mode = PAD_MODE_RGMII;
>>>>> break;
>>>>>
>>>>> default:
>>>>
>>>> I still don't like this. Having both the MAC and PHY drivers react to
>>>> the phy-connection-type property is bound to cause trouble somewhere.
>>>>
>>>> The only way out of the current mess is to define new properties for
>>>> both MAC and PHY that override the existing ones if present.
>>>
>>> Do you mean defining 4 new bindings and their corresponding
>>> phy_interface_t enum values? For example:
>>>
>>> "rgmii-v2"
>>> "rgmii-id-v2"
>>> "rgmii-rxid-v2"
>>> "rgmii-txid-v2"
>>>
>>> PHY_INTERFACE_MODE_RGMII_V2,
>>> PHY_INTERFACE_MODE_RGMII_ID_V2,
>>> PHY_INTERFACE_MODE_RGMII_RXID_V2,
>>> PHY_INTERFACE_MODE_RGMII_TXID_V2,
>>>
>>> And then handling these new enums in the at803x and nb8800 drivers?
>>
>> It has already been suggested to add new properties specifying desired
>> delays in picoseconds. If present on the MAC node, the MAC driver
>> should attempt to provide the delay, and if present on the PHY node, the
>> PHY driver is responsible.
>
> Sorry, I had already forgotten about Florian's suggestion:
>> If you introduced PHY and/or MAC specific properties to configure the
>> delays in the appropriate unit of time (say ps), you could use a
>> non-compliant 'phy-mode' just to satisfy the driver/PHY library and
>> still override the delays you need.
>
> So we would need two properties (RX and TX).
> "rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"
>
> but it's not clear to me how the MAC probe function communicates
> the arguments to the phy driver. Looks like we would need to add
> two fields to struct phy_device, and maybe define a new phy-mode
> to instruct the PHY driver to look for the two fields.
There's no need for the drivers to communicate. The location of the
properties in the device tree determines which driver should deal with
it.
> I don't have time to work on that for now, but I do need to
> fix the nb8800 driver now. Can we apply the following patch
> in the interim?
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ded041dbafe7..e94159507847 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
> break;
>
> case PHY_INTERFACE_MODE_RGMII:
> - pad_mode = PAD_MODE_RGMII;
> - break;
> -
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> - pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> + pad_mode = PAD_MODE_RGMII;
> break;
>
> default:
Simply stop reacting to the delay aspect of the phy-connection-type
property? Yes, I'm fine with that.
--
Måns Rullgård
Re: [PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> On 21/07/2017 15:04, Måns Rullgård wrote:
>
>> Marc Gonzalez wrote:
>>
>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>> there are 4 RGMII modes to handle:
>>>
>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>> the MAC should not add the RX or TX delays in this case)
>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>> the MAC should not add an RX delay in this case)
>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>> the MAC should not add an TX delay in this case)
>>>
>>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
>>> ---
>>> drivers/net/ethernet/aurora/nb8800.c | 6 --
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>> b/drivers/net/ethernet/aurora/nb8800.c
>>> index ded041dbafe7..f3ed320eb4ad 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device
>>> *dev)
>>> break;
>>>
>>> case PHY_INTERFACE_MODE_RGMII:
>>> - pad_mode = PAD_MODE_RGMII;
>>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>>> + pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>> break;
>>>
>>> + case PHY_INTERFACE_MODE_RGMII_ID:
>>> case PHY_INTERFACE_MODE_RGMII_TXID:
>>> - pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>> + pad_mode = PAD_MODE_RGMII;
>>> break;
>>>
>>> default:
>>
>> I still don't like this. Having both the MAC and PHY drivers react to
>> the phy-connection-type property is bound to cause trouble somewhere.
>>
>> The only way out of the current mess is to define new properties for
>> both MAC and PHY that override the existing ones if present.
>
> Do you mean defining 4 new bindings and their corresponding
> phy_interface_t enum values? For example:
>
> "rgmii-v2"
> "rgmii-id-v2"
> "rgmii-rxid-v2"
> "rgmii-txid-v2"
>
> PHY_INTERFACE_MODE_RGMII_V2,
> PHY_INTERFACE_MODE_RGMII_ID_V2,
> PHY_INTERFACE_MODE_RGMII_RXID_V2,
> PHY_INTERFACE_MODE_RGMII_TXID_V2,
>
> And then handling these new enums in the at803x and nb8800 drivers?
It has already been suggested to add new properties specifying desired
delays in picoseconds. If present on the MAC node, the MAC driver
should attempt to provide the delay, and if present on the PHY node, the
PHY driver is responsible.
--
Måns Rullgård
Re: [PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
> there are 4 RGMII modes to handle:
>
> "rgmii" (RX and TX delays are added by the MAC when required)
> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
> the MAC should not add the RX or TX delays in this case)
> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
> the MAC should not add an RX delay in this case)
> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
> the MAC should not add an TX delay in this case)
>
> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ded041dbafe7..f3ed320eb4ad 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
> break;
>
> case PHY_INTERFACE_MODE_RGMII:
> - pad_mode = PAD_MODE_RGMII;
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> break;
>
> + case PHY_INTERFACE_MODE_RGMII_ID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> - pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> + pad_mode = PAD_MODE_RGMII;
> break;
>
> default:
I still don't like this. Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.
The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.
--
Måns Rullgård
Re: [PATCH v2 2/4] net: ethernet: nb8800: Set RGMII_MODE for all RGMII modes
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> There are 4 RGMII phy-modes to handle.
>
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 041cfb7952f8..ded041dbafe7 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
> mac_mode |= HALF_DUPLEX;
>
> if (gigabit) {
> - if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
> + if (phy_interface_is_rgmii(dev->phydev))
> mac_mode |= RGMII_MODE;
>
> mac_mode |= GMAC_MODE;
> --
> 2.11.0
>
--
Måns Rullgård
Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup
Mason <slash@free.fr> writes: > I will look for an inter-packet gap knob and FCS error counter. There is an FCS error counter. Use "ethtool -S" and look for rx_bad_fcs_frames. Reading the stats counters automatically resets them to zero. -- Måns Rullgård
Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup
Marc Gonzalez <marc_gonza...@sigmadesigns.com> writes:
> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
> there are 4 RGMII phy-modes to handle:
>
> "rgmii" (RX and TX delays are added by the MAC when required)
> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
> the MAC should not add the RX or TX delays in this case)
> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
> the MAC should not add an RX delay in this case)
> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
> the MAC should not add an TX delay in this case)
>
> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>
> Signed-off-by: Marc Gonzalez <marc_gonza...@sigmadesigns.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 041cfb7952f8..f3ed320eb4ad 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
> mac_mode |= HALF_DUPLEX;
>
> if (gigabit) {
> - if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
> + if (phy_interface_is_rgmii(dev->phydev))
> mac_mode |= RGMII_MODE;
>
> mac_mode |= GMAC_MODE;
This is a separate issue, and the change is obviously correct.
> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
> break;
>
> case PHY_INTERFACE_MODE_RGMII:
> - pad_mode = PAD_MODE_RGMII;
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> break;
>
> + case PHY_INTERFACE_MODE_RGMII_ID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> - pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> + pad_mode = PAD_MODE_RGMII;
> break;
Won't this just make it break in a different set of circumstances?
I think the only sane solution to this mess is to never configure the
MAC delay based on the existing phy-connection-type property. If some
board requires this delay (because the PHY can't do it), a new property
should probably be introduced for that.
--
Måns Rullgård
Re: Setting link down or up in software
Mason <slash@free.fr> writes: > One thing I find confusing... If the 8035 could work with the generic > PHY driver, what's the point of the at803x.ko driver? Almost all Ethernet PHYs support the basic operation defined the by the standard. Many have additional non-standardised features that require a specific driver. Interrupts, for example. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
David Laight <david.lai...@aculab.com> writes: > From: Måns Rullgård >> Sent: 10 December 2016 13:25 > ... >> I solved this problem in an Ethernet driver by copying the initial part >> of the packet to an aligned skb and appending the remainder using >> skb_add_rx_frag(). The kernel network stack only cares about the >> headers, so the alignment of the packet payload doesn't matter. > > That rather depends on where the packet payload ends up. > It is likely that it will be copied to userspace (or maybe > into some aligned kernel buffer). > In which case you get an expensive misaligned copy. There's not much to be done about that. The only other option is to bypass DMA entirely, and that's sure to be even worse. > What do the hardware engineers think the ethernet interface will > be used for! Ticking boxes in marketing material. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
Willy Tarreau <w...@1wt.eu> writes: > Hi Jason, > > On Thu, Dec 08, 2016 at 11:20:04PM +0100, Jason A. Donenfeld wrote: >> Hi David, >> >> On Thu, Dec 8, 2016 at 1:37 AM, David Miller <da...@davemloft.net> wrote: >> > You really have to land the IP header on a proper 4 byte boundary. >> > >> > I would suggest pushing 3 dummy garbage bytes of padding at the front >> > or the end of your header. >> >> Are you sure 3 bytes to get 4 byte alignment is really the best? > > It's always the best. However there's another option which should be > considered : maybe it's difficult but not impossible to move some bits > from the current protocol to remove one byte. That's not always easy, > and sometimes you cannot do it just for one bit. However after you run > through this exercise, if you notice there's really no way to shave > this extra byte, you'll realize there's no room left for future > extensions and you'll more easily accept to add 3 empty bytes for > this, typically protocol version, tags, qos or flagss that you'll be > happy to rely on for future versions of your protocol. Always include some way of extending the protocol in the future. A single bit is often enough. Require a value of zero initially, then if you ever want to change anything, setting it to one can indicate whatever you want, including a complete redesign of the header. Alternatively, a one-bit field can indicate the presence of an extended header yet to be defined. Then old software can still make sense of the basic header. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
Felix Fietkau <n...@nbd.name> writes: > On 2016-12-10 14:25, Måns Rullgård wrote: >> Felix Fietkau <n...@nbd.name> writes: >> >>> On 2016-12-07 19:54, Jason A. Donenfeld wrote: >>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <da...@davemloft.net> wrote: >>>>> It's so much better to analyze properly where the misalignment comes from >>>>> and address it at the source, as we have for various cases that trip up >>>>> Sparc too. >>>> >>>> That's sort of my attitude too, hence starting this thread. Any >>>> pointers you have about this would be most welcome, so as not to >>>> perpetuate what already seems like an issue in other parts of the >>>> stack. >>> Hi Jason, >>> >>> I'm the author of that hackish LEDE/OpenWrt patch that works around the >>> misalignment issues. Here's some context regarding that patch: >>> >>> I intentionally put it in the target specific patches for only one of >>> our MIPS targets. There are a few ar71xx devices where the misalignment >>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment >>> requirement, and does not support inserting 2 bytes of padding to >>> correct the IP header misalignment. >>> >>> With these limitations the choice was between this ugly network stack >>> patch or inserting a very expensive memmove in the data path (which is >>> better than taking the mis-alignment traps, but still hurts routing >>> performance significantly). >> >> I solved this problem in an Ethernet driver by copying the initial part >> of the packet to an aligned skb and appending the remainder using >> skb_add_rx_frag(). The kernel network stack only cares about the >> headers, so the alignment of the packet payload doesn't matter. > > I considered that as well, but it's bad for routing performance if the > ethernet MAC does not support scatter/gather for xmit. > Unfortunately that limitation is quite common on embedded hardware. Yes, I can see that being an issue. However, if you're doing zero-copy routing, the header part of the original buffer should still be there, unused, so you could presumably copy the header of the outgoing packet there and then do dma as usual. Maybe there's something in the network stack that makes this impossible though. -- Måns Rullgård
Re: Misalignment, MIPS, and ip_hdr(skb)->version
Felix Fietkau <n...@nbd.name> writes: > On 2016-12-07 19:54, Jason A. Donenfeld wrote: >> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <da...@davemloft.net> wrote: >>> It's so much better to analyze properly where the misalignment comes from >>> and address it at the source, as we have for various cases that trip up >>> Sparc too. >> >> That's sort of my attitude too, hence starting this thread. Any >> pointers you have about this would be most welcome, so as not to >> perpetuate what already seems like an issue in other parts of the >> stack. > Hi Jason, > > I'm the author of that hackish LEDE/OpenWrt patch that works around the > misalignment issues. Here's some context regarding that patch: > > I intentionally put it in the target specific patches for only one of > our MIPS targets. There are a few ar71xx devices where the misalignment > cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment > requirement, and does not support inserting 2 bytes of padding to > correct the IP header misalignment. > > With these limitations the choice was between this ugly network stack > patch or inserting a very expensive memmove in the data path (which is > better than taking the mis-alignment traps, but still hurts routing > performance significantly). I solved this problem in an Ethernet driver by copying the initial part of the packet to an aligned skb and appending the remainder using skb_add_rx_frag(). The kernel network stack only cares about the headers, so the alignment of the packet payload doesn't matter. -- Måns Rullgård
Re: [PATCH net-next v2 3/4] Documentation: net: phy: Add blurb about RGMII
David Laight <david.lai...@aculab.com> writes: > From: Florian Fainelli >> Sent: 27 November 2016 23:03 >> Le 27/11/2016 14:24, Timur Tabi a crit : >> >> + * PHY device drivers in PHYLIB being reusable by nature, being able to >> >> + configure correctly a specified delay enables more designs with >> >> similar delay >> >> + requirements to be operate correctly >> > >> > Ok, this one I don't know how to fix. I'm not really sure what you're >> > trying to say. >> >> What I am trying to say is that once a PHY driver properly configures a >> delay that you have specified, there is no reason why this is not >> applicable to other platforms using this same PHY driver. > > As has been stated earlier it can depend on the track lengths on the > board itself. > (Although 1ns is about 1 foot - so track delays of that length are unlikely.) There could be a delay element. -- Måns Rullgård
Re: [net-next PATCH v1 0/2] stmmac: dwmac-meson8b: configurable RGMII TX delay
Sebastian Frias <s...@laposte.net> writes: > On 24/11/16 19:55, Florian Fainelli wrote: >> Le 24/11/2016 à 09:05, Martin Blumenstingl a écrit : >>> Based on what I found it seems that rgmii-id, rgmii-txid and >>> rgmii-rxid are supposed to be handled by the PHY. >> >> Correct, the meaning of PHY_INTERFACE_MODE should be from the >> perspective of the PHY device: >> >> - PHY_INTERFACE_MODE_RGMII_TXID means that the PHY is responsible for >> adding a delay when the MAC transmits (TX MAC -> PHY (delay) -> wire) >> - PHY_INTERFACE_MODE_RGMII_RXID means that the PHY is responsible for >> adding a delay when the MAC receives (RX MAC <- (delay) PHY) <- wire) >> > > Thanks for the explanation. > Actually I had thought that the delay was to account for board routing > (wires) between the MAC and the PHY. > From your explanation it appears that the delay is to account for board > routing (wires) between the PHY and the RJ45 socket. The delay pertains to the RGMII link between MAC and PHY. The external connection is self-clocking. -- Måns Rullgård
Re: Debugging Ethernet issues
Florian Fainelli <f.faine...@gmail.com> writes:
> On 11/14/2016 11:00 AM, Måns Rullgård wrote:
>> Florian Fainelli <f.faine...@gmail.com> writes:
>>
>>> On 11/14/2016 10:20 AM, Florian Fainelli wrote:
>>>> On 11/14/2016 09:59 AM, Sebastian Frias wrote:
>>>>> On 11/14/2016 06:32 PM, Florian Fainelli wrote:
>>>>>> On 11/14/2016 07:33 AM, Mason wrote:
>>>>>>> On 14/11/2016 15:58, Mason wrote:
>>>>>>>
>>>>>>>> nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control
>>>>>>>> rx/tx
>>>>>>>> vs
>>>>>>>> nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control
>>>>>>>> off
>>>>>>>>
>>>>>>>> I'm not sure whether "flow control" is relevant...
>>>>>>>
>>>>>>> Based on phy_print_status()
>>>>>>> phydev->pause ? "rx/tx" : "off"
>>>>>>> I added the following patch.
>>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>>>>>> b/drivers/net/ethernet/aurora/nb8800.c
>>>>>>> index defc22a15f67..4e758c1cfa4e 100644
>>>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>>>> @@ -667,6 +667,8 @@ static void nb8800_link_reconfigure(struct
>>>>>>> net_device *dev)
>>>>>>> struct phy_device *phydev = priv->phydev;
>>>>>>> int change = 0;
>>>>>>>
>>>>>>> + printk("%s from %pf\n", __func__, __builtin_return_address(0));
>>>>>>> +
>>>>>>> if (phydev->link) {
>>>>>>> if (phydev->speed != priv->speed) {
>>>>>>> priv->speed = phydev->speed;
>>>>>>> @@ -1274,9 +1276,9 @@ static int nb8800_hw_init(struct net_device *dev)
>>>>>>> nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
>>>>>>>
>>>>>>> /* Auto-negotiate by default */
>>>>>>> - priv->pause_aneg = true;
>>>>>>> - priv->pause_rx = true;
>>>>>>> - priv->pause_tx = true;
>>>>>>> + priv->pause_aneg = false;
>>>>>>> + priv->pause_rx = false;
>>>>>>> + priv->pause_tx = false;
>>>>>>>
>>>>>>> nb8800_mc_init(dev, 0);
>>>>>>>
>>>>>>>
>>
>> [...]
>>
>>>>>> And the time difference is clearly accounted for auto-negotiation time
>>>>>> here, as you can see it takes about 3 seconds for Gigabit Ethernet to
>>>>>> auto-negotiate and that seems completely acceptable and normal to me
>>>>>> since it is a more involved process than lower speeds.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> OK, so now it works (by accident?) even on 100 Mbps switch, but it still
>>>>>>> prints "flow control rx/tx"...
>>>>>>
>>>>>> Because your link partner advertises flow control, and that's what
>>>>>> phydev->pause and phydev->asym_pause report (I know it's confusing, but
>>>>>> that's what it is at the moment).
>>>>>
>>>>> Thanks.
>>>>> Could you confirm that Mason's patch is correct and/or that it does not
>>>>> has negative side-effects?
>>>>
>>>> The patch is not correct nor incorrect per-se, it changes the default
>>>> policy of having pause frames advertised by default to not having them
>>>> advertised by default.
>>
>> I was advised to advertise flow control by default back when I was
>> working on the driver, and I think it makes sense to do so.
>>
>>>> This influences both your Ethernet MAC and the link partner in that
>>>> the result is either flow control is enabled (before) or it is not
>>>> (with the patch). There must be something amiss if you see packet
>>>> loss or some kind of problem like that with an early exchange such as
>>>> DHCP. Flow control
Re: Debugging Ethernet issues
Florian Fainelli <f.faine...@gmail.com> writes:
> On 11/14/2016 10:20 AM, Florian Fainelli wrote:
>> On 11/14/2016 09:59 AM, Sebastian Frias wrote:
>>> On 11/14/2016 06:32 PM, Florian Fainelli wrote:
>>>> On 11/14/2016 07:33 AM, Mason wrote:
>>>>> On 14/11/2016 15:58, Mason wrote:
>>>>>
>>>>>> nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control
>>>>>> rx/tx
>>>>>> vs
>>>>>> nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
>>>>>>
>>>>>> I'm not sure whether "flow control" is relevant...
>>>>>
>>>>> Based on phy_print_status()
>>>>> phydev->pause ? "rx/tx" : "off"
>>>>> I added the following patch.
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>>>> b/drivers/net/ethernet/aurora/nb8800.c
>>>>> index defc22a15f67..4e758c1cfa4e 100644
>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>> @@ -667,6 +667,8 @@ static void nb8800_link_reconfigure(struct net_device
>>>>> *dev)
>>>>> struct phy_device *phydev = priv->phydev;
>>>>> int change = 0;
>>>>>
>>>>> + printk("%s from %pf\n", __func__, __builtin_return_address(0));
>>>>> +
>>>>> if (phydev->link) {
>>>>> if (phydev->speed != priv->speed) {
>>>>> priv->speed = phydev->speed;
>>>>> @@ -1274,9 +1276,9 @@ static int nb8800_hw_init(struct net_device *dev)
>>>>> nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
>>>>>
>>>>> /* Auto-negotiate by default */
>>>>> - priv->pause_aneg = true;
>>>>> - priv->pause_rx = true;
>>>>> - priv->pause_tx = true;
>>>>> + priv->pause_aneg = false;
>>>>> + priv->pause_rx = false;
>>>>> + priv->pause_tx = false;
>>>>>
>>>>> nb8800_mc_init(dev, 0);
>>>>>
>>>>>
[...]
>>>> And the time difference is clearly accounted for auto-negotiation time
>>>> here, as you can see it takes about 3 seconds for Gigabit Ethernet to
>>>> auto-negotiate and that seems completely acceptable and normal to me
>>>> since it is a more involved process than lower speeds.
>>>>
>>>>>
>>>>>
>>>>> OK, so now it works (by accident?) even on 100 Mbps switch, but it still
>>>>> prints "flow control rx/tx"...
>>>>
>>>> Because your link partner advertises flow control, and that's what
>>>> phydev->pause and phydev->asym_pause report (I know it's confusing, but
>>>> that's what it is at the moment).
>>>
>>> Thanks.
>>> Could you confirm that Mason's patch is correct and/or that it does not
>>> has negative side-effects?
>>
>> The patch is not correct nor incorrect per-se, it changes the default
>> policy of having pause frames advertised by default to not having them
>> advertised by default.
I was advised to advertise flow control by default back when I was
working on the driver, and I think it makes sense to do so.
>> This influences both your Ethernet MAC and the link partner in that
>> the result is either flow control is enabled (before) or it is not
>> (with the patch). There must be something amiss if you see packet
>> loss or some kind of problem like that with an early exchange such as
>> DHCP. Flow control tend to kick in under higher packet rates (at
>> least, that's what you expect).
>>
>>>
>>> Right now we know that Mason's patch makes this work, but we do not
>>> understand why nor its implications.
>>
>> You need to understand why, right now, the way this problem is
>> presented, you came up with a workaround, not with the root cause or the
>> solution. What does your link partner (switch?) reports, that is, what
>> is the ethtool output when you have a link up from your nb8800 adapter?
>
> Actually, nb8800_pause_config() seems to be doing a complete MAC/DMA
> reconfiguration when pause frames get auto-negotiated while the link is
> UP,
This is due to a silly hardware limitation. The register containing the
flow control bits can't be written while rx is enabled.
> and it does not differentiate being called from
> ethtool::set_pauseparam or the PHYLIB adjust_link callback (which it
> probably should),
Differentiate how?
> wondering if there is a not a remote chance you can get the reply to
> arrive right when you just got signaled a link UP?
If you're attempting to send or receive things before you get the link
up notification, you shouldn't expect anything to work reliably.
--
Måns Rullgård
Re: [PATCH v3 2/2] net: ethernet: nb8800: handle all RGMII definitions
Sebastian Frias <s...@laposte.net> writes:
> Hi Måns,
>
> On 11/05/2016 01:58 PM, Måns Rullgård wrote:
>>> if (gigabit) {
>>> - if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>>> + if (phy_interface_is_rgmii(phydev))
>>> mac_mode |= RGMII_MODE;
>>>
>>> mac_mode |= GMAC_MODE;
>>
>> As I said before, this part can/should be applied separately, although
>> personally I probably wouldn't have bothered adding a single-use variable.
>
> It was for consistency with other functions that use 'phydev', but I don't
> mind making the changes.
>
> Just to be clear, when you say "can/should be applied separately", do you
> mean that this patch should not be part of a series, and that I should split
> the series into separate patches?
I meant that this change should be made regardless of the others and can
be done separately if desired.
--
Måns Rullgård
Re: [PATCH v3 2/2] net: ethernet: nb8800: handle all RGMII definitions
Sebastian Frias <s...@laposte.net> writes:
> Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
> and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
> delay for tx and rx only") added several RGMII definitions:
> PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
> PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.
>
> Those are all RGMII modes (1Gbit) and must be considered that way when
> setting the MAC mode or the pad mode for the HW to work properly.
>
> Signed-off-by: Sebastian Frias <s...@laposte.net>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index d2855c9..fba2699 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -598,6 +598,7 @@ static irqreturn_t nb8800_irq(int irq, void *dev_id)
> static void nb8800_mac_config(struct net_device *dev)
> {
> struct nb8800_priv *priv = netdev_priv(dev);
> + struct phy_device *phydev = dev->phydev;
> bool gigabit = priv->speed == SPEED_1000;
> u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
> u32 mac_mode = 0;
> @@ -609,7 +610,7 @@ static void nb8800_mac_config(struct net_device *dev)
> mac_mode |= HALF_DUPLEX;
>
> if (gigabit) {
> - if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
> + if (phy_interface_is_rgmii(phydev))
> mac_mode |= RGMII_MODE;
>
> mac_mode |= GMAC_MODE;
As I said before, this part can/should be applied separately, although
personally I probably wouldn't have bothered adding a single-use variable.
> @@ -1278,9 +1279,8 @@ static int nb8800_tangox_init(struct net_device *dev)
> break;
>
> case PHY_INTERFACE_MODE_RGMII:
> - pad_mode = PAD_MODE_RGMII;
> - break;
> -
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> pad_mode = PAD_MODE_RGMII;
> break;
> --
> 1.7.11.2
>
--
Måns Rullgård
Re: [PATCH v2 2/2 ] net: ethernet: nb8800: handle all RGMII definitions
Sebastian Frias <s...@laposte.net> writes:
> Commit a999589ccaae ("phylib: add RGMII-ID interface mode definition")
> and commit 7d400a4c5897 ("phylib: add PHY interface modes for internal
> delay for tx and rx only") added several RGMII definitions:
> PHY_INTERFACE_MODE_RGMII_ID, PHY_INTERFACE_MODE_RGMII_RXID and
> PHY_INTERFACE_MODE_RGMII_TXID to deal with internal delays.
>
> Those are all RGMII modes (1Gbit) and must be considered that way when
> setting the MAC mode or the pad mode for the HW to work properly.
>
> Signed-off-by: Sebastian Frias <s...@laposte.net>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index d2855c9..fba2699 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -598,6 +598,7 @@ static irqreturn_t nb8800_irq(int irq, void *dev_id)
> static void nb8800_mac_config(struct net_device *dev)
> {
> struct nb8800_priv *priv = netdev_priv(dev);
> + struct phy_device *phydev = dev->phydev;
> bool gigabit = priv->speed == SPEED_1000;
> u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
> u32 mac_mode = 0;
> @@ -609,7 +610,7 @@ static void nb8800_mac_config(struct net_device *dev)
> mac_mode |= HALF_DUPLEX;
>
> if (gigabit) {
> - if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
> + if (phy_interface_is_rgmii(phydev))
> mac_mode |= RGMII_MODE;
>
> mac_mode |= GMAC_MODE;
This part is correct regardless of the outcome of the delay setup
discussion.
> @@ -1278,9 +1279,8 @@ static int nb8800_tangox_init(struct net_device *dev)
> break;
>
> case PHY_INTERFACE_MODE_RGMII:
> - pad_mode = PAD_MODE_RGMII;
> - break;
> -
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> pad_mode = PAD_MODE_RGMII;
> break;
> --
> 1.7.11.2
>
--
Måns Rullgård
Re: Ethernet not working on a different SoC with same eth HW
Florian Fainelli <f.faine...@gmail.com> writes: > On 11/04/2016 08:22 AM, Måns Rullgård wrote: >> Andrew Lunn <and...@lunn.ch> writes: >> >>> On Fri, Nov 04, 2016 at 03:05:00PM +, Måns Rullgård wrote: >>>> Andrew Lunn <and...@lunn.ch> writes: >>>> >>>>>>> I agree with you. But fixing it is likely to break boards which >>>>>>> currently have "rgmii", but actually need the delay in order to work. >>>>>> >>>>>> Does the internal delay here refer to the PHY or the MAC? It's a >>>>>> property of the MAC node after all. >>>>> >>>>> It is the PHY which applies the delay. >>>> >>>> Says who? >>> >>> The source code. >> >> There's source code that disagrees with that. The Broadcom GENET >> driver, for instance. > > Correct, and in the case where the MAC adds the delay while transmitting > (because it supports that) the expectation is that the PHY would remove > such a delay internally, conversely, the PHY would introduce a delay > while transmitting back to the PHY, in order to produce the desired 90 > degrees shift on the RGMII signals, and get reproduce the correct clock > and data alignment internally. > >> >>>> Some MACs can do it too. >>> >>> I'm sure they can. But look at the code. Nearly none do, and those >>> that do are potentially broken. >> >> Those few drivers that do anything differently based on these values >> enable clock delay in the MAC. That's why I wrote the NB8800 driver the >> way I did. >> > > I don't really what is wrong with the nb8800 driver at the moment, so > maybe this is just a configuration issue with the Atheros PHY driver, > it's not like it has not given people headache judging by the recent > discussions... We don't even know if the problems Mason is having are caused by incorrect clock skew in the first place. I'd suggest not patching anything at all until he gets it working. -- Måns Rullgård
Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
Florian Fainelli <f.faine...@gmail.com> writes: > On 11/04/2016 08:36 AM, Sebastian Frias wrote: >> Hi Måns, >> >> On 11/04/2016 04:18 PM, Måns Rullgård wrote: >>> Sebastian Frias <s...@laposte.net> writes: >>> >>>> The delay can be applied at PHY or MAC level, but since >>>> PHY drivers will apply the delay at PHY level when using >>>> one of the "internal delay" declinations of RGMII mode >>>> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again >>>> at MAC level causes issues. >>> >>> The Broadcom GENET driver does the same thing. >>> >> >> Well, I don't know who uses that driver, or why they did it that way. > > I do use this driver and it works for me (tm), although I tested mostly > with Broadcom PHYs and Ethernet switches, rarely with third party PHYs, > but had that too, but all of that is in tree though, > drivers/net/phy/broadcom.com, drivers/net/dsa/b53/ so feel free to > "audit" that part of the code too. > > The configuration of the GENET port multiplexer requires us to specify > how we want to align the clock and data, if we don't do that, and the > PHY is also not agreeing with how its own delays should be configured, > mayhem ensues, ranging from occasional transmit success, to high rates > of CRC/FCS errors in best cases. > > I did verify that the settings were correct using a scope FWIW. > >> >> However, with the current code and DT bindings, if one requires >> the delay, phy-connection-type="rgmii-txid" must be set. > > Yes, and we would set it correctly for our Broadcom reference boards > using this driver. > >> >> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers >> will apply the delay. >> >> I think a better way of dealing with this is that both, PHY and MAC >> drivers exchange information so that the delay is applied only once. > > Exchange what information? The PHY device interface (phydev->interface) > conveys the needed information for both entities. There doesn't seem to be any consensus among the drivers regarding where the delay should be applied. Since only a few drivers, MAC or PHY, act on this property, most combinations still work by chance. It is common for boards to set the delay at the PHY using external config pins so no software setup is required (although I have one Sigma based board that gets this wrong). I suspect if drivers/net/ethernet/broadcom/genet were used with one of the four PHY drivers that also set the delay based on this DT property, things would go wrong. -- Måns Rullgård
Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
Andrew Lunn <and...@lunn.ch> writes: > On Fri, Nov 04, 2016 at 04:02:24PM +0100, Sebastian Frias wrote: >> The delay can be applied at PHY or MAC level, but since >> PHY drivers will apply the delay at PHY level when using >> one of the "internal delay" declinations of RGMII mode >> (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again >> at MAC level causes issues. If this is correct, most of the PHY drivers are broken. >> Signed-off-by: Sebastian Frias <s...@laposte.net> >> --- >> drivers/net/ethernet/aurora/nb8800.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/aurora/nb8800.c >> b/drivers/net/ethernet/aurora/nb8800.c >> index b59aa35..d2855c9 100644 >> --- a/drivers/net/ethernet/aurora/nb8800.c >> +++ b/drivers/net/ethernet/aurora/nb8800.c >> @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev) >> break; >> >> case PHY_INTERFACE_MODE_RGMII_TXID: >> -pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY; >> +pad_mode = PAD_MODE_RGMII; >> break; > > How many boards use this Ethernet driver? How many boards are your > potentially breaking, because they need this delay? > > I guess it is a small number, because doesn't it require the PHY is > also broken, not adding a delay when it should? What if the PHY doesn't have that option? -- Måns Rullgård
Re: Ethernet not working on a different SoC with same eth HW
Andrew Lunn <and...@lunn.ch> writes: > On Fri, Nov 04, 2016 at 03:05:00PM +0000, Måns Rullgård wrote: >> Andrew Lunn <and...@lunn.ch> writes: >> >> >> > I agree with you. But fixing it is likely to break boards which >> >> > currently have "rgmii", but actually need the delay in order to work. >> >> >> >> Does the internal delay here refer to the PHY or the MAC? It's a >> >> property of the MAC node after all. >> > >> > It is the PHY which applies the delay. >> >> Says who? > > The source code. There's source code that disagrees with that. The Broadcom GENET driver, for instance. >> Some MACs can do it too. > > I'm sure they can. But look at the code. Nearly none do, and those > that do are potentially broken. Those few drivers that do anything differently based on these values enable clock delay in the MAC. That's why I wrote the NB8800 driver the way I did. -- Måns Rullgård
Re: [PATCH 1/2] net: ethernet: nb8800: Do not apply TX delay at MAC level
Sebastian Frias <s...@laposte.net> writes: > The delay can be applied at PHY or MAC level, but since > PHY drivers will apply the delay at PHY level when using > one of the "internal delay" declinations of RGMII mode > (like PHY_INTERFACE_MODE_RGMII_TXID), applying it again > at MAC level causes issues. The Broadcom GENET driver does the same thing. > Signed-off-by: Sebastian Frias <s...@laposte.net> > --- > drivers/net/ethernet/aurora/nb8800.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/aurora/nb8800.c > b/drivers/net/ethernet/aurora/nb8800.c > index b59aa35..d2855c9 100644 > --- a/drivers/net/ethernet/aurora/nb8800.c > +++ b/drivers/net/ethernet/aurora/nb8800.c > @@ -1282,7 +1282,7 @@ static int nb8800_tangox_init(struct net_device *dev) > break; > > case PHY_INTERFACE_MODE_RGMII_TXID: > - pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY; > + pad_mode = PAD_MODE_RGMII; > break; > > default: > -- > 1.7.11.2 If this change is correct (and I'm not convinced it is), that case should be merged with the one above it and PHY_INTERFACE_MODE_RGMII_RXID added as well. -- Måns Rullgård
Re: Ethernet not working on a different SoC with same eth HW
Andrew Lunn <and...@lunn.ch> writes: >> > I agree with you. But fixing it is likely to break boards which >> > currently have "rgmii", but actually need the delay in order to work. >> >> Does the internal delay here refer to the PHY or the MAC? It's a >> property of the MAC node after all. > > It is the PHY which applies the delay. Says who? Some MACs can do it too. > The phy-mode property is in the MAC part of the device tree binding, > but the delay is not the primary purpose of this property. Its primary > purpose, RGMII, RMII, SGMII, etc, is applicable to both the MAC and > the PHY. It probably would of been better to have the delays as a > separate property, but that is not how it is... The connection type is obviously the same at both ends, so it doesn't really matter where that is specified. The delay can be applied at either end or not at all, and it's anything but clear what the properties are supposed to mean. There is also no way to specify the amount of delay required even though many devices support more than one value. -- Måns Rullgård
Re: Ethernet not working on a different SoC with same eth HW
Andrew Lunn <and...@lunn.ch> writes: >> Considering the ethernet DT bindings: >> >> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet.txt >> >> Specifically, phy-mode values "rgmii", "rgmii-id", "rgmii-rxid", >> "rgmii-txid". >> >> Assuming that "rxid" (rx internal delay) and "rx clock delay" are >> in fact the same concept with different names, do you agree that >> it would be unexpected for "rgmii rx clock delay" to be enabled >> when a DTB specifies "rgmii" or "rgmii-txid" ? > > I agree with you. But fixing it is likely to break boards which > currently have "rgmii", but actually need the delay in order to work. Does the internal delay here refer to the PHY or the MAC? It's a property of the MAC node after all. -- Måns Rullgård
Re: Ethernet not working on a different SoC with same eth HW
Mason <slash@free.fr> writes:
> On 31/10/2016 17:28, Mason wrote:
>
>> On 31/10/2016 16:53, Andrew Lunn wrote:
>>
>>>> I'll add a log for the request_irq call.
>>>
>>> And take a look at /proc/interrupts
>>
>> You're right, there does seem to be something wrong with the interrupts.
>
> Having fixed that, I'm still unable to ping a box on the same
> ethernet segment... Still investigating.
>
> I think I may have spotted a potential issue in the Atheros driver.
>
> if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
> ret = at803x_enable_rx_delay(phydev);
> if (ret < 0)
> return ret;
> }
>
> Looking at this code, one might believe that "rgmii rx clock delay"
> is only enabled when the user requests it (through DT).
>
> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
> cf. PDF page 48
>
> *However* this bit is set to 1 at reset (both HW and SW resets).
> Thus, "rgmii rx clock delay" is always enabled, whether the user
> requests it or not.
>
> Could someone knowledgeable comment on the expected behavior of
> enabling rgmii rx (and tx) clock delay?
Clock delay is sometimes (depending on PCB layout) required to achieve
the correct timing between clock and data signals. The delay can be
applied at the MAC or the PHY. I'd start by finding out what the PCB
design expects or check the signals with a fast oscilloscope if you have
one.
--
Måns Rullgård
Re: [PATCH -next] net: ethernet: nb8800: fix error return code in nb8800_open()
Wei Yongjun <weiyj...@gmail.com> writes:
> From: Wei Yongjun <weiyongj...@huawei.com>
>
> Fix to return error code -ENODEV from the of_phy_connect() error
> handling case instead of 0, as done elsewhere in this function.
>
> Signed-off-by: Wei Yongjun <weiyongj...@huawei.com>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 453dc09..99c4055 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -975,8 +975,10 @@ static int nb8800_open(struct net_device *dev)
> phydev = of_phy_connect(dev, priv->phy_node,
> nb8800_link_reconfigure, 0,
> priv->phy_mode);
> - if (!phydev)
> + if (!phydev) {
> + err = -ENODEV;
> goto err_free_irq;
> + }
>
> nb8800_pause_adv(dev);
>
--
Måns Rullgård
Re: [PATCH 2/7] net: ethernet: nb8800: Fix module autoload
Javier Martinez Canillas <jav...@osg.samsung.com> writes:
> If the driver is built as a module, autoload won't work because the module
> alias information is not filled. So user-space can't match the registered
> device with the corresponding module.
>
> Export the module alias information using the MODULE_DEVICE_TABLE() macro.
>
> Before this patch:
>
> $ $ modinfo drivers/net/ethernet/aurora/nb8800.ko | grep alias
> $
>
> After this patch:
>
> $ modinfo drivers/net/ethernet/aurora/nb8800.ko | grep alias
> alias: of:N*T*Csigma,smp8734-ethernetC*
> alias: of:N*T*Csigma,smp8734-ethernet
> alias: of:N*T*Csigma,smp8642-ethernetC*
> alias: of:N*T*Csigma,smp8642-ethernet
> alias: of:N*T*Caurora,nb8800C*
> alias: of:N*T*Caurora,nb8800
>
> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
>
> drivers/net/ethernet/aurora/nb8800.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 453dc0967125..d5f2ad1a5a30 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1357,6 +1357,7 @@ static const struct of_device_id nb8800_dt_ids[] = {
> },
> { }
> };
> +MODULE_DEVICE_TABLE(of, nb8800_dt_ids);
>
> static int nb8800_probe(struct platform_device *pdev)
> {
> --
--
Måns Rullgård
Re: [PATCH -next] net: ethernet: nb8800: fix error handling of nb8800_probe()
Wei Yongjun <weiyj...@163.com> writes:
> From: Wei Yongjun <yongjun_...@trendmicro.com.cn>
>
> In ops->reset() error handling case, clk_disable_unprepare() is missed
> before return from this function.
>
> Signed-off-by: Wei Yongjun <yongjun_...@trendmicro.com.cn>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 2dcb8c7..0d4ea92 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1419,7 +1419,7 @@ static int nb8800_probe(struct platform_device *pdev)
> if (ops && ops->reset) {
> ret = ops->reset(dev);
> if (ret)
> - goto err_free_dev;
> + goto err_disable_clk;
> }
>
> bus = devm_mdiobus_alloc(>dev);
>
--
Måns Rullgård
Re: [PATCH net] net: nb8800: Fix SKB leak in nb8800_receive()
Florian Fainelli <f.faine...@gmail.com> writes:
> In case nb8800_receive() fails to allocate a fragment, we would leak the
> SKB freshly allocated and just return, instead, free it.
>
> Reported-by: coverity (CID 1341750)
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 08a23e6b60e9..1a3555d03a96 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -259,6 +259,7 @@ static void nb8800_receive(struct net_device *dev,
> unsigned int i,
> if (err) {
> netdev_err(dev, "rx buffer allocation failed\n");
> dev->stats.rx_dropped++;
> + dev_kfree_skb(skb);
> return;
> }
>
> --
> 2.7.4
>
--
Måns Rullgård
Re: [PATCH 1/2] net: ethernet: nb8800: use phydev from struct net_device
device_ops nb8800_netdev_ops
> = {
>
> static int nb8800_get_settings(struct net_device *dev, struct ethtool_cmd
> *cmd)
> {
> - struct nb8800_priv *priv = netdev_priv(dev);
> + struct phy_device *phydev = dev->phydev;
>
> - if (!priv->phydev)
> + if (!phydev)
> return -ENODEV;
>
> - return phy_ethtool_gset(priv->phydev, cmd);
> + return phy_ethtool_gset(phydev, cmd);
> }
>
> static int nb8800_set_settings(struct net_device *dev, struct ethtool_cmd
> *cmd)
> {
> - struct nb8800_priv *priv = netdev_priv(dev);
> + struct phy_device *phydev = dev->phydev;
>
> - if (!priv->phydev)
> + if (!phydev)
> return -ENODEV;
>
> - return phy_ethtool_sset(priv->phydev, cmd);
> + return phy_ethtool_sset(phydev, cmd);
> }
>
> static int nb8800_nway_reset(struct net_device *dev)
> {
> - struct nb8800_priv *priv = netdev_priv(dev);
> + struct phy_device *phydev = dev->phydev;
>
> - if (!priv->phydev)
> + if (!phydev)
> return -ENODEV;
>
> - return genphy_restart_aneg(priv->phydev);
> + return genphy_restart_aneg(phydev);
> }
>
> static void nb8800_get_pauseparam(struct net_device *dev,
> @@ -1079,6 +1079,7 @@ static int nb8800_set_pauseparam(struct net_device *dev,
>struct ethtool_pauseparam *pp)
> {
> struct nb8800_priv *priv = netdev_priv(dev);
> + struct phy_device *phydev = dev->phydev;
>
> priv->pause_aneg = pp->autoneg;
> priv->pause_rx = pp->rx_pause;
> @@ -1088,8 +1089,8 @@ static int nb8800_set_pauseparam(struct net_device *dev,
>
> if (!priv->pause_aneg)
> nb8800_pause_config(dev);
> - else if (priv->phydev)
> - phy_start_aneg(priv->phydev);
> + else if (phydev)
> + phy_start_aneg(phydev);
>
> return 0;
> }
> diff --git a/drivers/net/ethernet/aurora/nb8800.h
> b/drivers/net/ethernet/aurora/nb8800.h
> index e5adbc2..6ec4a95 100644
> --- a/drivers/net/ethernet/aurora/nb8800.h
> +++ b/drivers/net/ethernet/aurora/nb8800.h
> @@ -284,7 +284,6 @@ struct nb8800_priv {
>
> struct mii_bus *mii_bus;
> struct device_node *phy_node;
> - struct phy_device *phydev;
>
> /* PHY connection type from DT */
> int phy_mode;
> --
> 1.7.4.4
>
--
Måns Rullgård
Re: [PATCH 2/2] net: ethernet: nb8800: use phy_ethtool_{get|set}_link_ksettings
Philippe Reynes <trem...@gmail.com> writes:
> There are two generics functions phy_ethtool_{get|set}_link_ksettings,
> so we can use them instead of defining the same code in the driver.
>
> Signed-off-by: Philippe Reynes <trem...@gmail.com>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 24 ++--
> 1 files changed, 2 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 4989d31..dc2c35d 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1035,26 +1035,6 @@ static const struct net_device_ops nb8800_netdev_ops =
> {
> .ndo_validate_addr = eth_validate_addr,
> };
>
> -static int nb8800_get_settings(struct net_device *dev, struct ethtool_cmd
> *cmd)
> -{
> - struct phy_device *phydev = dev->phydev;
> -
> - if (!phydev)
> - return -ENODEV;
> -
> - return phy_ethtool_gset(phydev, cmd);
> -}
> -
> -static int nb8800_set_settings(struct net_device *dev, struct ethtool_cmd
> *cmd)
> -{
> - struct phy_device *phydev = dev->phydev;
> -
> - if (!phydev)
> - return -ENODEV;
> -
> - return phy_ethtool_sset(phydev, cmd);
> -}
> -
> static int nb8800_nway_reset(struct net_device *dev)
> {
> struct phy_device *phydev = dev->phydev;
> @@ -1183,8 +1163,6 @@ static void nb8800_get_ethtool_stats(struct net_device
> *dev,
> }
>
> static const struct ethtool_ops nb8800_ethtool_ops = {
> - .get_settings = nb8800_get_settings,
> - .set_settings = nb8800_set_settings,
> .nway_reset = nb8800_nway_reset,
> .get_link = ethtool_op_get_link,
> .get_pauseparam = nb8800_get_pauseparam,
> @@ -1192,6 +1170,8 @@ static const struct ethtool_ops nb8800_ethtool_ops = {
> .get_sset_count = nb8800_get_sset_count,
> .get_strings= nb8800_get_strings,
> .get_ethtool_stats = nb8800_get_ethtool_stats,
> + .get_link_ksettings = phy_ethtool_get_link_ksettings,
> + .set_link_ksettings = phy_ethtool_set_link_ksettings,
> };
>
> static int nb8800_hw_init(struct net_device *dev)
> --
> 1.7.4.4
>
--
Måns Rullgård
Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
Mason <slash@free.fr> writes: > On 18/03/2016 21:11, Uwe Kleine-König wrote: > >> Hello, >> >> On Fri, Mar 18, 2016 at 08:31:20PM +0100, Mason wrote: >> >>> On 18/03/2016 20:12, Uwe Kleine-König wrote: >>> >>>> On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote: >>>> >>>>> What would you think of making at803x_link_change_notify() print a >>>>> message every time it should do a reset but does not has a way to do it? >>>> >>>> Then this question is obsolete because the device doesn't probe. >>> >>> I don't understand this statement. >>> >>> What does it mean for a question to be obsolete? >> >> If the driver doesn't probe because it cannot control the reset line, >> you don't need to think about how it should behave in >> at803x_link_change_notify without control of the reset line, because >> this code isn't reached then. > > If I understand correctly, it is possible to soft-reset the PHY > by writing to a specific register. The GPIO pin is useful only to > force a hardware-reset when the PHY is wedged by some random event. Yes, and some variants of this phy are broken and require a hard reset in certain situations. At least that's what the comment in the code says. -- Måns Rullgård
Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes: > On 02/08/2016 03:11 PM, Mason wrote: >> On 08/02/2016 14:37, Måns Rullgård wrote: >> >>> Sebastian Frias wrote: >>> >>>> By the way, I know some people like the command line, email, etc. but >>>> there ought to be other tools better suited for patch review... >>> >>> Some kernel subsystems use http://patchwork.ozlabs.org/ to track status >>> of various patches. >> >> There's also a kernel bugzilla, but it may be for actual bugs. >> https://bugzilla.kernel.org/ >> > > Thanks, and what would be the definition of a bug? > I mean, would the issue from this thread qualify? > Should I have created a bug report before submitting the patch? No, that is not necessary. My understanding is that the bugzilla is more of a place to report a bug you've found but don't have a patch for. Patches always go through the mailing lists. -- Måns Rullgård
Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes: >>> By the way, I know some people like the command line, email, etc. but >>> there ought to be other tools better suited for patch review... >> >> Some kernel subsystems use http://patchwork.ozlabs.org/ to track status >> of various patches. >> > > Thanks, I see that netdev is part of it, and that the patches are there: > > https://patchwork.ozlabs.org/patch/580217/ > > seems like a slight layer over plain email and mailinglists; I was > thinking of something more in the line of > https://www.gerritcodereview.com/ > I believe Google uses Gerrit for Android. > I think Gerrit would probably be too big (and being written in Java, > using Prolog and other DSLs, implementing its own Git server in Java, > etc, may make some -or lots?- of kernel developers cry :-) ) > However, in Gerrit it is easier to know where in the "review" process > we are, because people have to explicitly give a score "+/- X" when > commenting on a patch. > Also, the diff can operate between different versions of the patches > themselves to see if the inlined comments were addressed. Gerrit has some merits, but for seasoned developers it's largely a nuisance. It's probably good at keeping junior/undisciplined developers from doing too much damage by strictly enforcing a cumbersome process. -- Måns Rullgård
Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes: > On 02/05/2016 04:26 PM, Måns Rullgård wrote: >> Sebastian Frias <s...@laposte.net> writes: >> >>> On 02/05/2016 04:08 PM, Måns Rullgård wrote: >>>> Sebastian Frias <s...@laposte.net> writes: >>>> >>>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote: >>>>>> Sebastian Frias <s...@laposte.net> writes: >>>>>> >>>>>>> Signed-off-by: Sebastian Frias <s...@laposte.net> >>>>>> >>>>>> Please change the subject to something like "net: ethernet: nb8800: >>>>>> support fixed-link DT node" and add a comment body. >>>>> >>>>> The subject is pretty explicit for such a simple patch, what else >>>>> could I add that wouldn't be unnecessary chat? >>>> >>>> It's customary to include a description body even if it's little more >>>> than a restatement of the subject. Also, while the subject usually only >>>> says _what_ the patch does, the body should additionally state _why_ it >>>> is needed. >>> >>> I understand, but _why_ it is needed is also obvious in this case; I >>> mean, without the patch "fixed-link" cannot be used. >> >> Then say so. >> >>> Other patches may not be as obvious/simple and thus justify and >>> require more details. >>> >>> Anyway, I added "Properly handles the case where the PHY is not connected >>> to the real MDIO bus" would that be ok? >> >> Have you read Documentation/SubmittingPatches? Do so (again) and pay >> special attention to section 2 "Describe your changes." > > I just sent v5. Thanks for your patience. > If for whatever reason, you or anybody else think that the comment is > not good, would you mind proposing a comment that would make everybody > happy so that the patch goes thru? > And if you or anybody else does not want the patch, could you please > say so as well? > > I have to admit this process (sending patches and getting it reviewed) > could benefit from more clarifications. > For example, the process could say that at least 2 reviewers must > agree on it (on the comments made to the patch and on the patch > itself). > I could also say that reviewers are to express not only their opinion > but to clearly and unequivocally accept or reject. > > For instance, right now, it is not clear to me if your comments are > "nice to have" or "blocking" the patch. > I don't know if the patch is welcome or not, etc. > So I submitted v5, but maybe it was not even necessary, it's hard to > know where in the submission process we are. In this case, it's ultimately up to Dave Miller. He'll take into account whatever comments others have made and decide whether he wants to accept it. > By the way, I know some people like the command line, email, etc. but > there ought to be other tools better suited for patch review... Some kernel subsystems use http://patchwork.ozlabs.org/ to track status of various patches. -- Måns Rullgård
Re: [PATCH v5] net: ethernet: nb8800: support fixed-link DT node
Sebastian Frias <s...@laposte.net> writes:
> Under some circumstances, for example when connecting
> to a switch:
>
> https://stackoverflow.com/questions/31046172/device-tree-for-phy-less-connection-to-a-dsa-switch
>
> the ethernet port will not be connected to a PHY.
> In that case a "fixed-link" DT node can be used to replace it.
>
> This patch adds support for the "fixed-link" node to the
> nb8800 driver.
>
> Signed-off-by: Sebastian Frias <s...@laposte.net>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a33..e1fb071 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev)
> goto err_disable_clk;
> }
>
> - priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> + if (of_phy_is_fixed_link(pdev->dev.of_node)) {
> + ret = of_phy_register_fixed_link(pdev->dev.of_node);
> + if (ret < 0) {
> + dev_err(>dev, "bad fixed-link spec\n");
> + goto err_free_bus;
> + }
> + priv->phy_node = of_node_get(pdev->dev.of_node);
> + }
> +
> + if (!priv->phy_node)
> + priv->phy_node = of_parse_phandle(pdev->dev.of_node,
> + "phy-handle", 0);
> +
> if (!priv->phy_node) {
> dev_err(>dev, "no PHY specified\n");
> ret = -ENODEV;
> --
> 2.1.4
>
--
Måns Rullgård
Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes:
> On 02/05/2016 04:08 PM, Måns Rullgård wrote:
>> Sebastian Frias <s...@laposte.net> writes:
>>
>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>>>> Sebastian Frias <s...@laposte.net> writes:
>>>>
>>>>> Signed-off-by: Sebastian Frias <s...@laposte.net>
>>>>
>>>> Please change the subject to something like "net: ethernet: nb8800:
>>>> support fixed-link DT node" and add a comment body.
>>>
>>> The subject is pretty explicit for such a simple patch, what else
>>> could I add that wouldn't be unnecessary chat?
>>
>> It's customary to include a description body even if it's little more
>> than a restatement of the subject. Also, while the subject usually only
>> says _what_ the patch does, the body should additionally state _why_ it
>> is needed.
>
> I understand, but _why_ it is needed is also obvious in this case; I
> mean, without the patch "fixed-link" cannot be used.
Then say so.
> Other patches may not be as obvious/simple and thus justify and
> require more details.
>
> Anyway, I added "Properly handles the case where the PHY is not connected
> to the real MDIO bus" would that be ok?
Have you read Documentation/SubmittingPatches? Do so (again) and pay
special attention to section 2 "Describe your changes."
>>>>> ---
>>>>>drivers/net/ethernet/aurora/nb8800.c | 14 +-
>>>>>1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>>>> b/drivers/net/ethernet/aurora/nb8800.c
>>>>> index ecc4a33..e1fb071 100644
>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>> @@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device
>>>>> *pdev)
>>>>> goto err_disable_clk;
>>>>> }
>>>>>
>>>>> - priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
>>>>> + if (of_phy_is_fixed_link(pdev->dev.of_node)) {
>>>>> + ret = of_phy_register_fixed_link(pdev->dev.of_node);
>>>>> + if (ret < 0) {
>>>>> + dev_err(>dev, "bad fixed-link spec\n");
>>>>> + goto err_free_bus;
>>>>> + }
>>>>> + priv->phy_node = of_node_get(pdev->dev.of_node);
>>>>> + }
>>>>> +
>>>>> + if (!priv->phy_node)
>>>>> + priv->phy_node = of_parse_phandle(pdev->dev.of_node,
>>>>> + "phy-handle", 0);
>>>>> +
>>>>> if (!priv->phy_node) {
>>>>> dev_err(>dev, "no PHY specified\n");
>>>>> ret = -ENODEV;
>>>>> --
>>>>> 2.1.4
>>>>
>>
--
Måns Rullgård
Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes:
> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>> Sebastian Frias <s...@laposte.net> writes:
>>
>>> Signed-off-by: Sebastian Frias <s...@laposte.net>
>>
>> Please change the subject to something like "net: ethernet: nb8800:
>> support fixed-link DT node" and add a comment body.
>
> The subject is pretty explicit for such a simple patch, what else
> could I add that wouldn't be unnecessary chat?
It's customary to include a description body even if it's little more
than a restatement of the subject. Also, while the subject usually only
says _what_ the patch does, the body should additionally state _why_ it
is needed.
>>> ---
>>> drivers/net/ethernet/aurora/nb8800.c | 14 +-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>> b/drivers/net/ethernet/aurora/nb8800.c
>>> index ecc4a33..e1fb071 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -1460,7 +1460,19 @@ static int nb8800_probe(struct platform_device *pdev)
>>> goto err_disable_clk;
>>> }
>>>
>>> - priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
>>> + if (of_phy_is_fixed_link(pdev->dev.of_node)) {
>>> + ret = of_phy_register_fixed_link(pdev->dev.of_node);
>>> + if (ret < 0) {
>>> + dev_err(>dev, "bad fixed-link spec\n");
>>> + goto err_free_bus;
>>> + }
>>> + priv->phy_node = of_node_get(pdev->dev.of_node);
>>> + }
>>> +
>>> + if (!priv->phy_node)
>>> + priv->phy_node = of_parse_phandle(pdev->dev.of_node,
>>> + "phy-handle", 0);
>>> +
>>> if (!priv->phy_node) {
>>> dev_err(>dev, "no PHY specified\n");
>>> ret = -ENODEV;
>>> --
>>> 2.1.4
>>
--
Måns Rullgård
Re: [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver
Andy Shevchenko <andy.shevche...@gmail.com> writes:
> On Fri, Feb 5, 2016 at 3:39 PM, Måns Rullgård <m...@mansr.com> wrote:
>>> + if (ret < 0) {
>>> + dev_err(>dev, "broken fixed-link
>>> specification\n");
>>
>> Line is longer than 80 chars.
>
> This is actually okay, though I would recommend to move long string
> literal to the next line.
I only mentioned it because fixing it was trivial.
--
Måns Rullgård
Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes:
> Signed-off-by: Sebastian Frias <s...@laposte.net>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a33..dd7bedc 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1462,9 +1462,18 @@ static int nb8800_probe(struct platform_device *pdev)
>
> priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> if (!priv->phy_node) {
> - dev_err(>dev, "no PHY specified\n");
> - ret = -ENODEV;
> - goto err_free_bus;
> + if (of_phy_is_fixed_link(pdev->dev.of_node)) {
> + ret = of_phy_register_fixed_link(pdev->dev.of_node);
> + if (ret < 0) {
> + dev_err(>dev, "bad fixed-link spec\n");
> + goto err_free_bus;
> + }
> + priv->phy_node = of_node_get(pdev->dev.of_node);
> + } else {
> + dev_err(>dev, "no PHY specified\n");
> + ret = -ENODEV;
> + goto err_free_bus;
> + }
> }
Maybe it would be clearer to reduce the if() nesting a bit, like this
for instance:
if (of_phy_is_fixed_link(pdev->dev.of_node)) {
ret = of_phy_register_fixed_link(pdev->dev.of_node);
if (ret < 0) {
dev_err(>dev, "bad fixed-link spec\n");
goto err_free_bus;
}
priv->phy_node = of_node_get(pdev->dev.of_node);
}
if (!priv->phy_node)
priv->phy_node = of_parse_phandle(pdev->dev.of_node,
"phy-handle", 0);
if (!priv->phy_node) {
dev_err(>dev, "no PHY specified\n");
ret = -ENODEV;
goto err_free_bus;
}
--
Måns Rullgård
Re: [PATCH v2] net: ethernet: support "fixed-link" DT key/node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes:
> On 02/05/2016 02:58 PM, Måns Rullgård wrote:
>> Sebastian Frias <s...@laposte.net> writes:
>>
>>> Signed-off-by: Sebastian Frias <s...@laposte.net>
>>> ---
>>> drivers/net/ethernet/aurora/nb8800.c | 15 ---
>>> 1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c
>>> b/drivers/net/ethernet/aurora/nb8800.c
>>> index ecc4a33..dd7bedc 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -1462,9 +1462,18 @@ static int nb8800_probe(struct platform_device *pdev)
>>>
>>> priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
>>> if (!priv->phy_node) {
>>> - dev_err(>dev, "no PHY specified\n");
>>> - ret = -ENODEV;
>>> - goto err_free_bus;
>>> + if (of_phy_is_fixed_link(pdev->dev.of_node)) {
>>> + ret = of_phy_register_fixed_link(pdev->dev.of_node);
>>> + if (ret < 0) {
>>> + dev_err(>dev, "bad fixed-link spec\n");
>>> + goto err_free_bus;
>>> + }
>>> + priv->phy_node = of_node_get(pdev->dev.of_node);
>>> + } else {
>>> + dev_err(>dev, "no PHY specified\n");
>>> + ret = -ENODEV;
>>> + goto err_free_bus;
>>> + }
>>> }
>>
>> Maybe it would be clearer to reduce the if() nesting a bit, like this
>> for instance:
>>
>> if (of_phy_is_fixed_link(pdev->dev.of_node)) {
>> ret = of_phy_register_fixed_link(pdev->dev.of_node);
>> if (ret < 0) {
>> dev_err(>dev, "bad fixed-link spec\n");
>> goto err_free_bus;
>> }
>> priv->phy_node = of_node_get(pdev->dev.of_node);
>> }
>>
>> if (!priv->phy_node)
>> priv->phy_node = of_parse_phandle(pdev->dev.of_node,
>>"phy-handle", 0);
>>
>> if (!priv->phy_node) {
>> dev_err(>dev, "no PHY specified\n");
>> ret = -ENODEV;
>> goto err_free_bus;
>> }
>>
>>
>
> Thanks Måns for your comments.
> With old code + my patch, we only hit 1 comparison in the general
> case, and a 2nd one in "fixed-link" case.
> With your suggestion above, it would mean that we hit 3 comparisons
> all the time.
> If you are ok with the 3 comparisons, I can post a v3.
This is code that runs once so IMO clarity is more important than a
minuscule speed difference.
--
Måns Rullgård
Re: [PATCH] net: ethernet: support "fixed-link" DT node on nb8800 driver
Sebastian Frias <s...@laposte.net> writes:
> Signed-off-by: Sebastian Frias <s...@laposte.net>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 19 +++
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a33..1353fee 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1461,10 +1461,21 @@ static int nb8800_probe(struct platform_device
> *pdev)
> }
>
> priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> - if (!priv->phy_node) {
> - dev_err(>dev, "no PHY specified\n");
> - ret = -ENODEV;
> - goto err_free_bus;
> + if (!priv->phy_node)
> + {
> + if (of_phy_is_fixed_link(pdev->dev.of_node)) {
> + ret = of_phy_register_fixed_link(pdev->dev.of_node);
> + if (ret < 0) {
> + dev_err(>dev, "broken fixed-link
> specification\n");
Line is longer than 80 chars.
> + goto err_free_bus;
> + }
> + priv->phy_node = of_node_get(pdev->dev.of_node);
> + }
> + else {
Wrong brace placement.
> + dev_err(>dev, "no PHY specified\n");
> + ret = -ENODEV;
> + goto err_free_bus;
> + }
> }
>
> priv->mii_bus = bus;
> --
> 2.1.4
>
--
Måns Rullgård
Re: [PATCH 8/9] net: nb8800: avoid uninitialized variable warning
Arnd Bergmann <a...@arndb.de> writes:
> The nb8800_poll() function initializes the 'next' variable in the
> loop looking for new input data. We know this will be called at
> least once because 'budget' is a guaranteed to be a positive number
> when we enter the function, but the compiler doesn't know that
> and warns when the variable is used later:
>
> drivers/net/ethernet/aurora/nb8800.c: In function 'nb8800_poll':
> drivers/net/ethernet/aurora/nb8800.c:350:21: warning: 'next' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
Which gcc version is this? 4.9 doesn't warn here, presumably because
it's clever enough to notice that the offending use of 'next' is under a
condition that can only be true if the first one was. Of course fixing
the code so older compilers don't warn is a good idea.
> Changing the 'while() {}' loop to 'do {} while()' makes it obvious
> to the compiler what is going on so it no longer warns.
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
Acked-by: Mans Rullgard <m...@mansr.com>
> ---
> drivers/net/ethernet/aurora/nb8800.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a334c507..f71ab2647a3b 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -302,7 +302,7 @@ static int nb8800_poll(struct napi_struct *napi, int
> budget)
> nb8800_tx_done(dev);
>
> again:
> - while (work < budget) {
> + do {
> struct nb8800_rx_buf *rxb;
> unsigned int len;
>
> @@ -330,7 +330,7 @@ again:
> rxd->report = 0;
> last = next;
> work++;
> - }
> + } while (work < budget);
>
> if (work) {
> priv->rx_descs[last].desc.config |= DESC_EOC;
> --
> 2.7.0
>
--
Måns Rullgård
Re: [PATCH] ethernet: aurora: AURORA_NB8800 should depend on HAS_DMA
Geert Uytterhoeven <ge...@linux-m68k.org> writes: > If NO_DMA=y: > > ERROR: "dma_map_single" [drivers/net/ethernet/aurora/nb8800.ko] undefined! > ERROR: "dma_unmap_page" [drivers/net/ethernet/aurora/nb8800.ko] undefined! > ERROR: "dma_sync_single_for_cpu" [drivers/net/ethernet/aurora/nb8800.ko] > undefined! > ERROR: "dma_unmap_single" [drivers/net/ethernet/aurora/nb8800.ko] > undefined! > ERROR: "dma_alloc_coherent" [drivers/net/ethernet/aurora/nb8800.ko] > undefined! > ERROR: "dma_mapping_error" [drivers/net/ethernet/aurora/nb8800.ko] > undefined! > ERROR: "dma_map_page" [drivers/net/ethernet/aurora/nb8800.ko] undefined! > ERROR: "dma_free_coherent" [drivers/net/ethernet/aurora/nb8800.ko] > undefined! > > Signed-off-by: Geert Uytterhoeven <ge...@linux-m68k.org> Acked-by: Mans Rullgard <m...@mansr.com> > drivers/net/ethernet/aurora/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/aurora/Kconfig > b/drivers/net/ethernet/aurora/Kconfig > index a3c7106fdf85e78f..8ba7f8ff3434000f 100644 > --- a/drivers/net/ethernet/aurora/Kconfig > +++ b/drivers/net/ethernet/aurora/Kconfig > @@ -13,6 +13,7 @@ if NET_VENDOR_AURORA > > config AURORA_NB8800 > tristate "Aurora AU-NB8800 support" > + depends on HAS_DMA > select PHYLIB > help >Support for the AU-NB8800 gigabit Ethernet controller. > -- > 1.9.1 > -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Alexander Duyck <alexander.du...@gmail.com> writes:
> On Wed, Nov 25, 2015 at 5:04 AM, Måns Rullgård <m...@mansr.com> wrote:
>> Mason <slash@free.fr> writes:
>>
>>> On 25/11/2015 13:45, Måns Rullgård wrote:
>>>
>>>> Mason wrote:
>>>>
>>>>> On 19/11/2015 14:02, Mans Rullgard wrote:
>>>>>
>>>>>> + if (dma_mapping_error(>dev, dma_addr)) {
>>>>>> + skb_free_frag(data);
>>>>>> + return -ENOMEM;
>>>>>> + }
>>>>>
>>>>> I'm back-porting this driver to 4.1
>>>>>
>>>>> skb_free_frag() was introduced in 4.2 by 181edb2bfa22b IIUC.
>>>>>
>>>>> +static inline void skb_free_frag(void *addr)
>>>>> +{
>>>>> + __free_page_frag(addr);
>>>>> +}
>>>>>
>>>>> Should I just copy the definition of __free_page_frag() ?
>>>>
>>>> Looks like it ought to work. Try and find out. Not that you'll ever
>>>> hit that error condition unless you fake it.
>>>
>>> Turns out __free_pages_ok() is static and I'd rather not touch
>>> mm/page_alloc.c in my back-port.
>>>
>>> Since you say the error condition is rare, I think I'll go with
>>> the code that 181edb2bfa22b replaced (put_page, IIUC).
>>>
>>> #include
>>> #if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
>>> #define skb_free_frag(data) put_page(virt_to_head_page(data))
>>> #else
>>> #error DELETE ME NOW (see commit 181edb2bfa22b)
>>> #endif
>>
>> You can simply put_page(page) instead since we already have the
>> virt_to_head_page() a few lines up.
>
> What you could do is use __free_pages instead of __free_pages_ok.
> Generally you will want to use __free_pages instead of put_page just
> to avoid a bunch of unnecessary tests and function pointer accesses.
Note that this is on a should-never-happen error path, so there's no
need to be counting cycles.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mason <slash@free.fr> writes:
> On 25/11/2015 17:16, Måns Rullgård wrote:
>
>> Alexander Duyck writes:
>>
>>> On Wed, Nov 25, 2015 at 5:04 AM, Måns Rullgård wrote:
>>>
>>>> Mason writes:
>>>>
>>>>> On 25/11/2015 13:45, Måns Rullgård wrote:
>>>>>
>>>>>> Mason wrote:
>>>>>>
>>>>>>> On 19/11/2015 14:02, Mans Rullgard wrote:
>>>>>>>
>>>>>>>> + if (dma_mapping_error(>dev, dma_addr)) {
>>>>>>>> + skb_free_frag(data);
>>>>>>>> + return -ENOMEM;
>>>>>>>> + }
>>>>>>>
>>>>>>> I'm back-porting this driver to 4.1
>>>>>>>
>>>>>>> skb_free_frag() was introduced in 4.2 by 181edb2bfa22b IIUC.
>>>>>>>
>>>>>>> +static inline void skb_free_frag(void *addr)
>>>>>>> +{
>>>>>>> + __free_page_frag(addr);
>>>>>>> +}
>>>>>>>
>>>>>>> Should I just copy the definition of __free_page_frag() ?
>>>>>>
>>>>>> Looks like it ought to work. Try and find out. Not that you'll ever
>>>>>> hit that error condition unless you fake it.
>>>>>
>>>>> Turns out __free_pages_ok() is static and I'd rather not touch
>>>>> mm/page_alloc.c in my back-port.
>>>>>
>>>>> Since you say the error condition is rare, I think I'll go with
>>>>> the code that 181edb2bfa22b replaced (put_page, IIUC).
>>>>>
>>>>> #include
>>>>> #if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
>>>>> #define skb_free_frag(data) put_page(virt_to_head_page(data))
>>>>> #else
>>>>> #error DELETE ME NOW (see commit 181edb2bfa22b)
>>>>> #endif
>>>>
>>>> You can simply put_page(page) instead since we already have the
>>>> virt_to_head_page() a few lines up.
>>>
>>> What you could do is use __free_pages instead of __free_pages_ok.
>>> Generally you will want to use __free_pages instead of put_page just
>>> to avoid a bunch of unnecessary tests and function pointer accesses.
>>
>> Note that this is on a should-never-happen error path, so there's no
>> need to be counting cycles.
>
> Per Mans' advice, I have locally committed this patch:
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index ecc4a334c507..9f929be96b74 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -39,6 +39,13 @@
>
> #include "nb8800.h"
>
> +#include
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
> +#define skb_free_frag(data) put_page(page)
> +#else
> +#error DELETE ME NOW (see commit 181edb2bfa22)
> +#endif
> +
> static void nb8800_tx_done(struct net_device *dev);
> static int nb8800_dma_stop(struct net_device *dev);
That works only by accident due to how local variables are named. If
you want a macro to emulate the new function, your first suggestion is
better. I thought you were going to change the actual function.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mason <slash@free.fr> writes:
> On 19/11/2015 14:02, Mans Rullgard wrote:
>
>> +if (dma_mapping_error(>dev, dma_addr)) {
>> +skb_free_frag(data);
>> +return -ENOMEM;
>> +}
>
> I'm back-porting this driver to 4.1
>
> skb_free_frag() was introduced in 4.2 by 181edb2bfa22b IIUC.
>
> +static inline void skb_free_frag(void *addr)
> +{
> + __free_page_frag(addr);
> +}
>
> Should I just copy the definition of __free_page_frag() ?
Looks like it ought to work. Try and find out. Not that you'll ever
hit that error condition unless you fake it.
> /*
> * Frees a page fragment allocated out of either a compound or order 0 page.
> */
> void __free_page_frag(void *addr)
> {
> struct page *page = virt_to_head_page(addr);
>
> if (unlikely(put_page_testzero(page)))
> __free_pages_ok(page, compound_order(page));
> }
>
> Regards.
>
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mason <slash@free.fr> writes:
> On 25/11/2015 13:45, Måns Rullgård wrote:
>
>> Mason wrote:
>>
>>> On 19/11/2015 14:02, Mans Rullgard wrote:
>>>
>>>> + if (dma_mapping_error(>dev, dma_addr)) {
>>>> + skb_free_frag(data);
>>>> + return -ENOMEM;
>>>> + }
>>>
>>> I'm back-porting this driver to 4.1
>>>
>>> skb_free_frag() was introduced in 4.2 by 181edb2bfa22b IIUC.
>>>
>>> +static inline void skb_free_frag(void *addr)
>>> +{
>>> + __free_page_frag(addr);
>>> +}
>>>
>>> Should I just copy the definition of __free_page_frag() ?
>>
>> Looks like it ought to work. Try and find out. Not that you'll ever
>> hit that error condition unless you fake it.
>
> Turns out __free_pages_ok() is static and I'd rather not touch
> mm/page_alloc.c in my back-port.
>
> Since you say the error condition is rare, I think I'll go with
> the code that 181edb2bfa22b replaced (put_page, IIUC).
>
> #include
> #if LINUX_VERSION_CODE < KERNEL_VERSION(4,2,0)
> #define skb_free_frag(data) put_page(virt_to_head_page(data))
> #else
> #error DELETE ME NOW (see commit 181edb2bfa22b)
> #endif
You can simply put_page(page) instead since we already have the
virt_to_head_page() a few lines up.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Mon, 16 Nov 2015 20:59:18 + > >> Anything else? > > Sorry, when I find one problem I give you the feedback for that > and move on to the 100s of other patches I have in my queue. Some people prefer to comment on more than one issue, if present, per patch iteration. Apparently you're not one of them. > Or would you like me to devote all of my time to just your driver > instead of taking everyone's submissions into consideration? Of course not. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Mans Rullgard <m...@mansr.com> > Date: Mon, 16 Nov 2015 18:23:35 + > >> +static int nb8800_alloc_rx(struct net_device *dev, int i, bool napi) > > "i" is passed in as a signed int here, but: > >> +static void nb8800_receive(struct net_device *dev, unsigned i, unsigned len) > ... >> +err = nb8800_alloc_rx(dev, i, true); > > It comes from an 'unsigned' value. > > Please pick one type and use it consistently. Darn, missed one. > Also, always fully spell out "unsigned int" rather than use "unsigned" > as a shorthand. OK Anything else? -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: vitesse: add support for VSC8601
Mason <slash@free.fr> writes: > On 12/11/2015 19:41, Mans Rullgard wrote: > >> +.phy_id = PHY_ID_VSC8601, >> +.name = "Vitesse VSC8601", >> +.phy_id_mask= 0x0000, >> +.features = PHY_GBIT_FEATURES, >> +.flags = PHY_HAS_INTERRUPT, >> +.config_init= _config_init, >> +.config_aneg= _config_aneg, >> +.read_status= _read_status, >> +.ack_interrupt = _ack_interrupt, >> +.config_intr= _config_intr, > > I expected Documentation/CodingStyle to forbid taking the address > of functions. I can't find anything to that effect. That said, it's not something I would normally do, but all the other phy_driver entries in that file look like that. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: at803x: support interrupt on 8030 and 8035
Mason <slash@free.fr> writes:
> On 12/11/2015 20:14, Florian Fainelli wrote:
>> On 12/11/15 11:09, Måns Rullgård wrote:
>>> On 12 November 2015 19:06:23 GMT+00:00, Mason wrote:
>>>> On 12/11/2015 18:40, Mans Rullgard wrote:
>>>>> Commit 77a993942 "phy/at8031: enable at8031 to work on interrupt mode"
>>>>> added interrupt support for the 8031 PHY but left out the other two
>>>>> chips supported by this driver.
>>>>>
>>>>> This patch sets the .ack_interrupt and .config_intr functions for the
>>>>> 8030 and 8035 drivers as well.
>>>>>
>>>>> Signed-off-by: Mans Rullgard <m...@mansr.com>
>>>>> ---
>>>>> I have only tested this with an 8035. I can't find a datasheet for
>>>>> the 8030, but since 8031, 8032, and 8035 all have the same register
>>>>> layout, there's a good chance 8030 does as well.
>>>>> ---
>>>>> drivers/net/phy/at803x.c | 4
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>>>>> index fabf11d..2d020a3 100644
>>>>> --- a/drivers/net/phy/at803x.c
>>>>> +++ b/drivers/net/phy/at803x.c
>>>>> @@ -308,6 +308,8 @@ static struct phy_driver at803x_driver[] = {
>>>>> .flags = PHY_HAS_INTERRUPT,
>>>>> .config_aneg= genphy_config_aneg,
>>>>> .read_status= genphy_read_status,
>>>>> + .ack_interrupt = at803x_ack_interrupt,
>>>>> + .config_intr= at803x_config_intr,
>>>>> .driver = {
>>>>> .owner = THIS_MODULE,
>>>>> },
>>>>> @@ -327,6 +329,8 @@ static struct phy_driver at803x_driver[] = {
>>>>> .flags = PHY_HAS_INTERRUPT,
>>>>> .config_aneg= genphy_config_aneg,
>>>>> .read_status= genphy_read_status,
>>>>> + .ack_interrupt = at803x_ack_interrupt,
>>>>> + .config_intr= at803x_config_intr,
>>>>> .driver = {
>>>>> .owner = THIS_MODULE,
>>>>> },
>>>>
>>>> Shouldn't we take the opportunity to clean up the duplicated register
>>>> definitions? (I'll send an informal patch to spur discussion.)
>>>>
>>>> Regards.
>>>
>>> That can be done independently. Feel free to send a patch.
>>
>> Agreed, that deserve a separate patch.
>
> Isn't there a problem when at803x_set_wol() sets the AT803X_WOL_ENABLE
> bit, but a DISABLE/ENABLE cycle through at803x_config_intr() will
> discard that bit?
Possibly, but fixing that should be yet another patch.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mason <slash@free.fr> writes: > On 10/11/2015 20:25, Måns Rullgård wrote: > >> Mason writes: >> >>> On 10/11/2015 17:14, Mans Rullgard wrote: >>> >>>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller. >>>> It is an almost complete rewrite of a driver originally found in >>>> a Sigma Designs 2.6.22 tree. >>>> >>>> Signed-off-by: Mans Rullgard <m...@mansr.com> >>>> --- >>>> Changes: >>>> - Refactored mdio access functions >>>> - Refactored register access helpers >>>> - Improved error handling in rx buffer allocation >>>> - Optimised some fifo parameters >>>> - Overhauled tx dma. Multiple packets are now chained in a single dma >>>> operation if xmit_more is set, improving performance. >>>> - Improved rx irq handling. It's not possible to disable interrupts >>>> entirely for napi poll, but they can be slowed down a little. >>>> - Use readx_poll_timeout in various places >>>> - Improved error detection >>>> - Improved statistics >>>> - Report hardware statistics counters through ethtool >>>> - Improved tangox-specific setup >>>> - Support for flow control using pause frames >>>> - Explanatory comments added >>>> - Various minor stylistic changes >>>> --- >>>> drivers/net/ethernet/Kconfig |1 + >>>> drivers/net/ethernet/Makefile|1 + >>>> drivers/net/ethernet/aurora/Kconfig | 20 + >>>> drivers/net/ethernet/aurora/Makefile |1 + >>>> drivers/net/ethernet/aurora/nb8800.c | 1530 >>>> ++ >>>> drivers/net/ethernet/aurora/nb8800.h | 314 +++ >>>> 6 files changed, 1867 insertions(+) >>> >>> The code has grown much since the previous patch, despite some >>> refactoring. Is this mostly due to ethtool_ops support? >>> >>> drivers/net/ethernet/aurora/nb8800.c | 1146 >>> ++ >>> drivers/net/ethernet/aurora/nb8800.h | 230 +++ >> >> Some of the increase is from new features, some from improvements, and >> then there are a bunch of new comments. > > Sweet. > > With this version, my kernel boots faster than before > (I had been using a 5 month-old version.) > > Before: > > [0.613623] tangox-enet 26000.ethernet: SMP86xx internal Ethernet at > 0x26000 > [0.623638] libphy: tangox-mii: probed > [0.686527] tangox-enet 26000.ethernet: PHY: found Atheros 8035 ethernet > at 0x4 > [0.697169] tangox-enet 26000.ethernet eth0: MAC address 00:16:e8:02:08:42 > ... > [1.306360] Sending DHCP requests .. > [4.699969] tangox-enet 26000.ethernet eth0: Link is Up - 1Gbps/Full - > flow control rx/tx > [8.899671] ., OK > [8.926343] IP-Config: Got DHCP answer from 172.27.200.1, my address is > 172.27.64.49 > ... > [8.987327] Freeing unused kernel memory: 168K (c039e000 - c03c8000) > > After: > > [0.623526] libphy: nb8800-mii: probed > [0.628092] nb8800 26000.ethernet eth0: MAC address 00:16:e8:02:08:42 > ... > [4.732948] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow > control rx/tx > [4.752655] Sending DHCP requests ., OK > [4.782644] IP-Config: Got DHCP answer from 172.27.200.1, my address is > 172.27.64.49 > ... > [4.849298] Freeing unused kernel memory: 164K (c039f000 - c03c8000) > > The DHCP request is sent later, but the kernel doesn't twiddle its thumbs > for 4 seconds after the link comes up. Does this come from not probing the > PHY anymore? No, that's from properly setting the link state initially down. > BTW, you're not using the PHY IRQ, right? I think I remember you saying > it didn't work reliably? It doesn't seem to be wired up on any of my boards, or there's some magic required to activate it that I'm unaware of. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mason <slash@free.fr> writes: > [ CCing a few knowledgeable people ] > > Despite the subject, this is about an Atheros 8035 PHY :-) > > On 12/11/2015 15:04, Måns Rullgård wrote: > >> Mason wrote: >> >>> BTW, you're not using the PHY IRQ, right? I think I remember you saying >>> it didn't work reliably? >> >> It doesn't seem to be wired up on any of my boards, or there's some >> magic required to activate it that I'm unaware of. > > Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin > properly connected to AR8035 INT pin (pin 20). I have a different board. > > > http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf > > INT pin 20 > I/O, D, PD > Interrupt Signal to System; default OD-gate, needs an external > 10Kohm pull-up, active low; can be configured to I/O by register, > active high. > > 4.1.17 Interrupt Enable > Offset: 0x12 > Mode: Read/Write > Hardware Reset: 0 > > Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to > the same "Interrupt Enable" register? Seems like someone missed that it was already defined. > In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12: > link success/fail, speed/duplex changed, autoneg error > > Looks like at803x_config_intr() is used for 8031, but not for 8035... > > Relevant commit: > 77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode" > > If I add .config_intr and .ack_interrupt to the 8035 struct, then I get > (also added some traces) I tried that just now, and I get nothing. What interrupt did you specify in your device tree? > Questions: > > Can't at803x_ack_interrupt() just return phy_read(phydev, AT803X_INSR); No, that would return the actual value of the register. The caller doesn't care about the value, but should be notified if there was an error. > Can at803x_config_intr() be used with the 8035 Probably. The person who sent the patch for 8031 probably happened to have that model. > What about AT803X_INER/AT803X_INTR_ENABLE and AT803X_INSR/AT803X_INTR_STATUS Accidental duplicates. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Måns Rullgård <m...@mansr.com> writes: > Mason <slash@free.fr> writes: > >> [ CCing a few knowledgeable people ] >> >> Despite the subject, this is about an Atheros 8035 PHY :-) >> >> On 12/11/2015 15:04, Måns Rullgård wrote: >> >>> Mason wrote: >>> >>>> BTW, you're not using the PHY IRQ, right? I think I remember you saying >>>> it didn't work reliably? >>> >>> It doesn't seem to be wired up on any of my boards, or there's some >>> magic required to activate it that I'm unaware of. >> >> Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin >> properly connected to AR8035 INT pin (pin 20). > > I have a different board. > >> >> >> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf >> >> INT pin 20 >> I/O, D, PD >> Interrupt Signal to System; default OD-gate, needs an external >> 10Kohm pull-up, active low; can be configured to I/O by register, >> active high. >> >> 4.1.17 Interrupt Enable >> Offset: 0x12 >> Mode: Read/Write >> Hardware Reset: 0 >> >> Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to >> the same "Interrupt Enable" register? > > Seems like someone missed that it was already defined. > >> In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12: >> link success/fail, speed/duplex changed, autoneg error >> >> Looks like at803x_config_intr() is used for 8031, but not for 8035... >> >> Relevant commit: >> 77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode" >> >> If I add .config_intr and .ack_interrupt to the 8035 struct, then I get >> (also added some traces) > > I tried that just now, and I get nothing. What interrupt did you > specify in your device tree? It works with the interrupt set to trigger on rising edge. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net: phy: at803x: support interrupt on 8030 and 8035
On 12 November 2015 19:06:23 GMT+00:00, Mason <slash@free.fr> wrote:
>On 12/11/2015 18:40, Mans Rullgard wrote:
>> Commit 77a993942 "phy/at8031: enable at8031 to work on interrupt
>mode"
>> added interrupt support for the 8031 PHY but left out the other two
>> chips supported by this driver.
>>
>> This patch sets the .ack_interrupt and .config_intr functions for the
>> 8030 and 8035 drivers as well.
>>
>> Signed-off-by: Mans Rullgard <m...@mansr.com>
>> ---
>> I have only tested this with an 8035. I can't find a datasheet for
>> the 8030, but since 8031, 8032, and 8035 all have the same register
>> layout, there's a good chance 8030 does as well.
>> ---
>> drivers/net/phy/at803x.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index fabf11d..2d020a3 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -308,6 +308,8 @@ static struct phy_driver at803x_driver[] = {
>> .flags = PHY_HAS_INTERRUPT,
>> .config_aneg= genphy_config_aneg,
>> .read_status= genphy_read_status,
>> +.ack_interrupt = at803x_ack_interrupt,
>> +.config_intr= at803x_config_intr,
>> .driver = {
>> .owner = THIS_MODULE,
>> },
>> @@ -327,6 +329,8 @@ static struct phy_driver at803x_driver[] = {
>> .flags = PHY_HAS_INTERRUPT,
>> .config_aneg= genphy_config_aneg,
>> .read_status= genphy_read_status,
>> +.ack_interrupt = at803x_ack_interrupt,
>> +.config_intr= at803x_config_intr,
>> .driver = {
>> .owner = THIS_MODULE,
>> },
>
>Shouldn't we take the opportunity to clean up the duplicated register
>definitions? (I'll send an informal patch to spur discussion.)
>
>Regards.
That can be done independently. Feel free to send a patch.
--
Måns Rullgård
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Wed, 11 Nov 2015 13:04:07 + > >> Måns Rullgård <m...@mansr.com> writes: >> >>> David Miller <da...@davemloft.net> writes: >>> >>>> From: Måns Rullgård <m...@mansr.com> >>>> Date: Wed, 11 Nov 2015 00:40:09 + >>>> >>>>> When the DMA complete interrupt arrives, the next chain should be >>>>> kicked off as quickly as possible, and I don't see why that would >>>>> benefit from being done in napi context. >>>> >>>> NAPI isn't about low latency, it's about fairness and interrupt >>>> mitigation. >>>> >>>> You probably don't even realize that all of the TX SKB freeing you do >>>> in the hardware interrupt handler end up being actually processed by a >>>> scheduled software interrupt anyways. >>>> >>>> So you are gaining almost nothing by not doing TX completion in NAPI >>>> context, whereas by doing so you would be gaining a lot including >>>> more simplified locking or even the ability to do no locking at all. >>> >>> TX completion is separate from restarting the DMA, and moving that to >>> NAPI may well be a good idea. Should I simply napi_schedule() if the >>> hardware indicates TX is complete and do the cleanup in the NAPI poll >>> function? >> >> I tried that, and throughput (as measured by iperf3) dropped by 2%. >> Maybe I did something wrong. > > Did you fix all the locking in that change? > > Since all of your TX handling runs in software interrupt context, you > can stop using IRQ locking and use BH locking driver-wide instead. > > And actually, no locking is really needed for TX processing. With > proper memory barriers and properly crafter queue state tests, you > can run completely lockless. > > Again, look at example drivers. I know, for example, that > drivers/net/ethernet/broadcom/tg3.c runs TX lockless. You'll > see that tg3_tx() takes no locks at all. The way the hardware works, once a DMA operation has been started, adding more packets to the active chain can't be done reliably. For that reason, if start_xmit is called (with xmit_more zero) while a DMA operation is in progress, the new packet(s) must be queued until the hardware raises the DMA complete interrupt. At that time, the next pending DMA chain, if any, can be kicked off. If the TX DMA channel is idle when start_xmit is called, it can be started immediately. Checking the DMA status and starting it if idle has to be done atomically somehow. There is a separate indication for actual TX completion, and the interrupt for that can be set to only fire every 7 frames or when a timeout expires. When this happens, the TX cleanup needs to run, and that can obviously be done from NAPI without using any locks. Bear in mind that this hardware is quite primitive compared to modern high-performance Ethernet controllers from the likes of Intel and Broadcom. The documentation I have is dated 2003. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mason <slash@free.fr> writes: > On 10/11/2015 22:51, Eric Dumazet wrote: > >> On Tue, 2015-11-10 at 21:21 +0000, Måns Rullgård wrote: >> >>> Even ixgbe uses napi_complete() while netdevice.h says one should >>> "consider using napi_complete_done() instead." Did the author consider >>> it and decide not to, or has the driver simply not been updated? >> >> napi_complete_done() is quite new, very few drivers use it. > > I was hoping to back-port this driver for my platform. Are the recent > features used in the driver available in 4.1? What about 3.14? xmit_more was added in 3.18, napi_complete_done() in 3.19. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Eric Dumazet <eric.duma...@gmail.com> writes: > On Wed, 2015-11-11 at 13:48 +0000, Måns Rullgård wrote: >> Eric Dumazet <eric.duma...@gmail.com> writes: >> >> > On Wed, 2015-11-11 at 13:04 +, Måns Rullgård wrote: >> > >> >> I tried that, and throughput (as measured by iperf3) dropped by 2%. >> >> Maybe I did something wrong. >> > >> > What link speed have you used, what was the throughput you got, >> > and is the receiver using the same NIC ? >> >> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC. >> Why does it matter what NIC the receiver has? > > Because at 1Gb line rate, you better get GRO properly implemented in the > receiver, so that TCP stack does not send one ACK every 2 MSS. > > Send speed is also dependent on the number of ACK packets the sender has > to process. > > This is why I suggested you use napi_gro_receive() in your driver. FWIW, with UDP I get 650 Mbps. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Måns Rullgård <m...@mansr.com> writes: > Eric Dumazet <eric.duma...@gmail.com> writes: > >> On Wed, 2015-11-11 at 13:48 +, Måns Rullgård wrote: >>> Eric Dumazet <eric.duma...@gmail.com> writes: >>> >>> > On Wed, 2015-11-11 at 13:04 +, Måns Rullgård wrote: >>> > >>> >> I tried that, and throughput (as measured by iperf3) dropped by 2%. >>> >> Maybe I did something wrong. >>> > >>> > What link speed have you used, what was the throughput you got, >>> > and is the receiver using the same NIC ? >>> >>> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC. >>> Why does it matter what NIC the receiver has? >> >> Because at 1Gb line rate, you better get GRO properly implemented in the >> receiver, so that TCP stack does not send one ACK every 2 MSS. >> >> Send speed is also dependent on the number of ACK packets the sender has >> to process. >> >> This is why I suggested you use napi_gro_receive() in your driver. > > FWIW, with UDP I get 650 Mbps. It seems the on-chip interconnect is limiting memory bandwidth available to the Ethernet DMA. If I increase the limits, I get 800 Mbps over TCP and 850 Mbps over UDP with the driver posted here, the TCP case being CPU bound in csum_partial_copy_from_user. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Wed, 11 Nov 2015 18:25:05 + > >> If the TX DMA channel is idle when start_xmit is called, it can be >> started immediately. Checking the DMA status and starting it if >> idle has to be done atomically somehow. > > ->ndo_start_xmit() is guaranteed to be invoked atomically, protected > by the TX queue spinlock. Yes, but the DMA needs to be restarted from some other context if it was busy when start_xmit checked. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Wed, 11 Nov 2015 19:09:19 + > >> David Miller <da...@davemloft.net> writes: >> >>> From: Måns Rullgård <m...@mansr.com> >>> Date: Wed, 11 Nov 2015 18:25:05 + >>> >>>> If the TX DMA channel is idle when start_xmit is called, it can be >>>> started immediately. Checking the DMA status and starting it if >>>> idle has to be done atomically somehow. >>> >>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected >>> by the TX queue spinlock. >> >> Yes, but the DMA needs to be restarted from some other context if it was >> busy when start_xmit checked. > > Then you can probably use the TXQ lock in the interrupt handler just for > that. That seems a bit heavy-handed when the critical section for this is only a tiny part of the start_xmit function. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Wed, 11 Nov 2015 19:17:07 + > >> David Miller <da...@davemloft.net> writes: >> >>> From: Måns Rullgård <m...@mansr.com> >>> Date: Wed, 11 Nov 2015 19:09:19 + >>> >>>> David Miller <da...@davemloft.net> writes: >>>> >>>>> From: Måns Rullgård <m...@mansr.com> >>>>> Date: Wed, 11 Nov 2015 18:25:05 + >>>>> >>>>>> If the TX DMA channel is idle when start_xmit is called, it can be >>>>>> started immediately. Checking the DMA status and starting it if >>>>>> idle has to be done atomically somehow. >>>>> >>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected >>>>> by the TX queue spinlock. >>>> >>>> Yes, but the DMA needs to be restarted from some other context if it was >>>> busy when start_xmit checked. >>> >>> Then you can probably use the TXQ lock in the interrupt handler just for >>> that. >> >> That seems a bit heavy-handed when the critical section for this is only >> a tiny part of the start_xmit function. > > Then what synchornization primitive other than spin locks are you going > to use for this? > > My point is that there is a spinlock the core code is _already_ taking, > unconditionally, when ->ndo_start_xmit() executes. And you can therefore > take advantage of that rather than using another lock of your own. I get that. But that remains locked for the duration of ndo_start_xmit() whereas the part that needs to be synchronised with the DMA completion IRQ handler is tiny. Having the IRQ handler spin for the duration of ndo_start_xmit() seemed silly to me. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Wed, 11 Nov 2015 19:35:05 + > >>> I don't think it's silly at all. >> >> I'm sure I read somewhere that the time spent spinning on a lock should >> be kept as small as possible. >> >>> And unless you can measure it making a difference, don't knock the idea. >> >> I tried using netif_tx_lock() in the IRQ handler instead, and it locked >> up solid. Clearly that was the wrong thing to do. > > Oh that's right, it's a BH lock not an IRQ one. > > Yet another argument for doing everything in ->poll(), thus making all > operations outside of NAPI scheduling run in software interrupt > context, and therefore being able to make use of the TXQ lock for > this. Well, I tried calling the DMA restart function from NAPI poll under netif_tx_lock(). Now it works only as long as there is incoming traffic. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Eric Dumazet <eric.duma...@gmail.com> writes:
> On Tue, 2015-11-10 at 16:14 +, Mans Rullgard wrote:
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>
> ...
>
>> +
>> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +struct nb8800_priv *priv = netdev_priv(dev);
>> +struct nb8800_tx_desc *txd;
>> +struct nb8800_tx_buf *txb;
>> +struct nb8800_dma_desc *dma;
>> +dma_addr_t dma_addr;
>> +unsigned int dma_len;
>> +unsigned long flags;
>> +int align;
>> +int next;
>> +
>> +if (atomic_read(>tx_free) <= NB8800_DESC_LOW) {
>> +netif_stop_queue(dev);
>> +return NETDEV_TX_BUSY;
>> +}
>> +
>> +align = (8 - (uintptr_t)skb->data) & 7;
>> +
>> +dma_len = skb->len - align;
>> +dma_addr = dma_map_single(>dev, skb->data + align,
>> + dma_len, DMA_TO_DEVICE);
>> +
>> +if (dma_mapping_error(>dev, dma_addr)) {
>> +netdev_err(dev, "tx dma mapping error\n");
>> +kfree_skb(skb);
>> +dev->stats.tx_dropped++;
>> + return NETDEV_TX_OK;
>> +}
>> +
>> +if (atomic_dec_return(>tx_free) <= NB8800_DESC_LOW)
>> +netif_stop_queue(dev);
>
> You probably also want to clear skb->xmit_more or risk a stall
Very true, will fix.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Eric Dumazet <eric.duma...@gmail.com> writes:
> On Tue, 2015-11-10 at 16:14 +, Mans Rullgard wrote:
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>
> ...
>
>> +
>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>> +{
>> +struct nb8800_priv *priv = netdev_priv(dev);
>> +struct nb8800_rx_desc *rxd = >rx_descs[i];
>> +struct page *page = priv->rx_bufs[i].page;
>> +int offset = priv->rx_bufs[i].offset;
>> +void *data = page_address(page) + offset;
>> +dma_addr_t dma = rxd->desc.s_addr;
>> +struct sk_buff *skb;
>> +int size;
>> +int err;
>> +
>> +size = len <= RX_COPYBREAK ? len : RX_COPYHDR;
>> +
>> +skb = napi_alloc_skb(>napi, size);
>> +if (!skb) {
>> +netdev_err(dev, "rx skb allocation failed\n");
>> +dev->stats.rx_dropped++;
>> +return;
>> +}
>> +
>> +if (len <= RX_COPYBREAK) {
>> +dma_sync_single_for_cpu(>dev, dma, len, DMA_FROM_DEVICE);
>> +memcpy(skb_put(skb, len), data, len);
>> +dma_sync_single_for_device(>dev, dma, len,
>> + DMA_FROM_DEVICE);
>> +} else {
>> +err = nb8800_alloc_rx(dev, i, true);
>> +if (err) {
>> +netdev_err(dev, "rx buffer allocation failed\n");
>> +dev->stats.rx_dropped++;
>> +return;
>> +}
>> +
>> +dma_unmap_page(>dev, dma, RX_BUF_SIZE, DMA_FROM_DEVICE);
>> +memcpy(skb_put(skb, RX_COPYHDR), data, RX_COPYHDR);
>> +skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>> +offset + RX_COPYHDR, len - RX_COPYHDR,
>> +RX_BUF_SIZE);
>> +}
>> +
>> + skb->protocol = eth_type_trans(skb, dev);
>> +netif_receive_skb(skb);
>> +}
>> +
>
> Any reason you do not use napi_gro_receive(>napi, skb) instead of
> netif_receive_skb() ?
Because I haven't been following the netdev list closely for the last
five years, and no documentation I read mentioned this function. I can
certainly change it.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Mason <slash@free.fr> writes: > On 10/11/2015 17:14, Mans Rullgard wrote: > >> This adds a driver for the Aurora VLSI NB8800 Ethernet controller. >> It is an almost complete rewrite of a driver originally found in >> a Sigma Designs 2.6.22 tree. >> >> Signed-off-by: Mans Rullgard <m...@mansr.com> >> --- >> Changes: >> - Refactored mdio access functions >> - Refactored register access helpers >> - Improved error handling in rx buffer allocation >> - Optimised some fifo parameters >> - Overhauled tx dma. Multiple packets are now chained in a single dma >> operation if xmit_more is set, improving performance. >> - Improved rx irq handling. It's not possible to disable interrupts >> entirely for napi poll, but they can be slowed down a little. >> - Use readx_poll_timeout in various places >> - Improved error detection >> - Improved statistics >> - Report hardware statistics counters through ethtool >> - Improved tangox-specific setup >> - Support for flow control using pause frames >> - Explanatory comments added >> - Various minor stylistic changes >> --- >> drivers/net/ethernet/Kconfig |1 + >> drivers/net/ethernet/Makefile|1 + >> drivers/net/ethernet/aurora/Kconfig | 20 + >> drivers/net/ethernet/aurora/Makefile |1 + >> drivers/net/ethernet/aurora/nb8800.c | 1530 >> ++ >> drivers/net/ethernet/aurora/nb8800.h | 314 +++ >> 6 files changed, 1867 insertions(+) > > The code has grown much since the previous patch, despite some > refactoring. Is this mostly due to ethtool_ops support? > > drivers/net/ethernet/aurora/nb8800.c | 1146 > ++ > drivers/net/ethernet/aurora/nb8800.h | 230 +++ Some of the increase is from new features, some from improvements, and then there are a bunch of new comments. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Tue, 10 Nov 2015 18:05:15 + > >> Because I haven't been following the netdev list closely for the last >> five years, and no documentation I read mentioned this function. I can >> certainly change it. > > It is always advisable to mimick what other drivers do and use them as > a reference, rather than depend upon documentation which by definition > is always going to be out of sync with the source tree. Sure. The trick is to pick the right driver(s) to use as reference. Quite a few of them don't use that function. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
David Miller <da...@davemloft.net> writes: > From: Måns Rullgård <m...@mansr.com> > Date: Tue, 10 Nov 2015 20:53:19 + > >> David Miller <da...@davemloft.net> writes: >> >>> From: Måns Rullgård <m...@mansr.com> >>> Date: Tue, 10 Nov 2015 18:05:15 + >>> >>>> Because I haven't been following the netdev list closely for the last >>>> five years, and no documentation I read mentioned this function. I can >>>> certainly change it. >>> >>> It is always advisable to mimick what other drivers do and use them as >>> a reference, rather than depend upon documentation which by definition >>> is always going to be out of sync with the source tree. >> >> Sure. The trick is to pick the right driver(s) to use as reference. >> Quite a few of them don't use that function. > > If you really are stumped on this matter, start at least with the > ixgbe driver. In fact pretty much every Intel ethernet driver is > a reasonable reference. Others to check out are bnx2x and mlx5. Even ixgbe uses napi_complete() while netdevice.h says one should "consider using napi_complete_done() instead." Did the author consider it and decide not to, or has the driver simply not been updated? As for the napi_gro_receive() function, calling that instead of netif_receive_skb() is easy enough, or are there other things I should be doing in addition? -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Andy Shevchenko <andy.shevche...@gmail.com> writes:
> On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård <m...@mansr.com> wrote:
>> Andy Shevchenko <andy.shevche...@gmail.com> writes:
>>
>>>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>>>> + u32 mask, u32 val)
>>>> +{
>>>> + u32 old = nb8800_readb(priv, reg);
>>>> + u32 new = (old & ~mask) | val;
>>>
>>> Shoudn't be "… | (val & mask);" ?
>>
>> No, it's meant to replace the bits in mask with the corresponding bits
>> from val.
>
> But you unconditionally use entire val value which might bring bits
> outside of mask.
Very well, I'll apply the mask to both then.
>>>> + nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>>> + wmb(); /* ensure desc addr is written before starting DMA
>>>> */
>>>
>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps
>>> mmiowb() ?
>>
>> Possibly.
>
> Standalone wmb() doesn't make sense.
It does if you need to enforce ordering between normal and I/O memory.
In fact, since the descriptor is filled in using normal memory accesses,
my understanding is that mmiowb() would be insufficient here. The
comment could be improved, however.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Andy Shevchenko <andy.shevche...@gmail.com> writes:
>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>> + u32 mask, u32 val)
>> +{
>> + u32 old = nb8800_readb(priv, reg);
>> + u32 new = (old & ~mask) | val;
>
> Shoudn't be "… | (val & mask);" ?
No, it's meant to replace the bits in mask with the corresponding bits
from val.
> + empty line?
>
>> + if (new != old)
>> + nb8800_writeb(priv, reg, new);
>> +}
>> +
[...]
>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>
> unsigned int i ?
> len as well?
Does it matter? The values are nowhere near overflowing signed int.
[...]
>> + /* If a packet arrived after we last checked but
>> +* before writing RX_ITR, the interrupt will be
>> +* delayed, so we retrieve it now. */
>
> Block comments usually
> /*
> * text
> */
Documentation/CodingStyle says net/ and drivers/net/ are special, though
currently a mix of styles can be found. Personally, I don't
particularly care.
> Can be longer lines?
Still won't fit on two lines.
>> + if (priv->rx_descs[next].report)
>> + goto again;
>> +
>> + napi_complete_done(napi, work);
>> + }
>> +
>> + return work;
>> +}
>> +
>> +static void nb8800_tx_dma_start(struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + struct nb8800_tx_buf *txb;
>> + u32 txc_cr;
>> +
>> + txb = >tx_bufs[priv->tx_queue];
>> + if (!txb->ready)
>> + return;
>> +
>> + txc_cr = nb8800_readl(priv, NB8800_TXC_CR);
>> + if (txc_cr & TCR_EN)
>> + return;
>> +
>> + nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>> + wmb(); /* ensure desc addr is written before starting DMA */
>
> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
Possibly.
>> + nb8800_writel(priv, NB8800_TXC_CR, txc_cr | TCR_EN);
>> +
>> + priv->tx_queue = (priv->tx_queue + txb->chain_len) % TX_DESC_COUNT;
>> +}
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
