Re: [PATCH net-next] modules: allow modprobe load regular elf binaries

[Chris Mason]

On 6 Mar 2018, at 11:12, Linus Torvalds wrote:

On Mon, Mar 5, 2018 at 5:34 PM, Alexei Starovoitov  
wrote:
As the first step in development of bpfilter project [1] the 
request_module()
code is extended to allow user mode helpers to be invoked. Idea is 
that
user mode helpers are built as part of the kernel build and installed 
as
traditional kernel modules with .ko file extension into distro 
specified
location, such that from a distribution point of view, they are no 
different
than regular kernel modules. Thus, allow request_module() logic to 
load such

user mode helper (umh) modules via:

[,,]

I like this, but I have one request: can we make sure that this action
is visible in the system messages?

When we load a regular module, at least it shows in lsmod afterwards,
although I have a few times wanted to really see module load as an
event in the logs too.

When we load a module that just executes a user program, and there is
no sign of it in the module list, I think we *really* need to make
that event show to the admin some way.

.. and yes, maybe we'll need to rate-limit the messages, and maybe it
turns out that I'm entirely wrong and people will hate the messages
after they get used to the concept of these pseudo-modules, but
particularly for the early implementation when this is a new thing, I
really want a message like

 executed user process xyz-abc as a pseudo-module

or something in dmesg.

I do *not* want this to be a magical way to hide things.


Especially early on, this makes a lot of sense.  But I wanted to plug 
bps and the hopefully growing set of bpf introspection tools:


https://github.com/iovisor/bcc/blob/master/introspection/bps_example.txt

Long term these are probably a good place to tell the admin what's going 
on.


-chris

Re: Waiting for the PHY to complete auto-negotiation

[Mason]

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.

Regards.

Re: Waiting for the PHY to complete auto-negotiation

[Mason]

+ 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*.
> 
> 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.

Regards.

Re: Waiting for the PHY to complete auto-negotiation

[Mason]

On 07/12/2017 00:00, Florian Fainelli wrote:

> On 12/06/2017 11:25 AM, Mason wrote:
>
>> When we detect link down, we put the ethernet HW block in reset,
>> and repeat initialization when the link comes back up.
>>
>> Hmmm, however, at the moment, I only reset on an administrative
>> (user-requested) link down, i.e. through ndo_stop. I would probably
>> have to handle cable unplug/replug events as well.
>>
>> Or just consider the quirk to make flow control too complicated
>> to implement correctly...
> 
> I suppose your procedure is fine, but don't you have a better way to
> resolve that by trying to place a special RX DMA ring entry that allows
> your RX DMA not to be entirely stopped, but intentionally looped through
> a buffer that you control? As long as you can stop the Ethernet MAC RX,
> working with such a limitation is probably fine, but this really sounds
> like a huge pain in the butt and a major HW flaw.

Could you elaborate a bit on your suggestion?
(Special ring entry, looped through a buffer under my control)
Is this a typical thing to do to stop DMA?

Currently the driver tries to stop DMA in nb8800_dma_stop(),
which does the following:

http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/aurora/nb8800.c#L881

1) poll until TX finishes (I assume the system no longer accepts new
   frames to send at this point)
2) set the EOC (end of chain) bit on all descriptors (could there be
   a problem if we receive a frame at that moment? Don't we need some
   kind of lock?)
3) disable address filtering (need to check what this does)
4) enable loop-back mode
5) send up to 5 "fake" packets in order to hit an EOC descriptor

The reason I'm trying to move away from this method is that it doesn't
work on our new SoC; and when pressed, the HW dev said it had never been
supported. (Also I find it somewhat hackish, but that's a matter of taste.)

Regards.

Re: Waiting for the PHY to complete auto-negotiation

[Mason]

On 06/12/2017 20:07, Andrew Lunn wrote:

> By chip, you mean the Ethernet controller? Not the whole SoC?

Doh! Yes. Let me rephrase.

When we detect link down, we put the ethernet HW block in reset,
and repeat initialization when the link comes back up.

Hmmm, however, at the moment, I only reset on an administrative
(user-requested) link down, i.e. through ndo_stop. I would probably
have to handle cable unplug/replug events as well.

Or just consider the quirk to make flow control too complicated
to implement correctly...

Regards.

Re: Waiting for the PHY to complete auto-negotiation

[Mason]

On 06/12/2017 19:26, Andrew Lunn wrote:

> On 06/12/2017 19:03, Mason wrote:
>
>> The problem with this is the following mind-boggling quirk of
>> the hardware: once RX DMA is enabled, there is no supported
>> way to disable it! Thus, I'm trying to find a clean way to set
>> the control flow parameter BEFORE enabling the MAC.
> 
> There is no solution. I can pull the cable out, and plug it into a
> different computer. The first could use flow control, the second not.
> 
> If you cannot disable RX DMA, you should probably disable unloading of
> the module. Also kexec.

It is not possible to /selectively/ disable RX DMA, but after a reset,
DMA is disabled. So the solution I propose is this:

When we detect link down, we put the chip in reset, so that we will
repeat initialization when the link comes back up.

Mans has been reluctant to adopt this approach, but this is what
I implemented in my forked version. I'm trying to push my changes
upstream, to minimize the differences.

Regards.

Re: Waiting for the PHY to complete auto-negotiation

[Mason]

On 06/12/2017 17:59, Andrew Lunn wrote:

> On Wed, Dec 06, 2017 at 05:39:00PM +0100, Mason wrote:
>
>> I've been trying to wrap my head around Ethernet auto-negotiation,
>> vs actual / real packets seen at the MAC layer. I found the relevant
>> Wikipedia article to be fairly informative:
>>
>> https://en.wikipedia.org/wiki/Autonegotiation
>>
>> The reason I care is that my Ethernet HW does not allow changing the
>> flow control setting once the MAC has started (more specifically, once
>> RX DMA has been enabled).
>>
>> In nb8800_open(), the code currently works in this order:
>>
>>  nb8800_start_rx(dev);
>>  phy_start(phydev);
>>
>> The first line enables the MAC (and DMA).
>> The second enables the PHY and starts auto-negotiation.
>>
>> This is a problem: I would like for PHY auto-negotiation to be
>> /complete/ before I enable the MAC.
>>
>> What is the recommended way to wait for the PHY?
> 
>> AFAICT, the PHY layer calls back into the eth driver through the
>> adjust_link() callback registered through of_phy_connect().
>> It seems like this might be a good place to enable the MAC?
> 
> That probably works, but you might have a few corner cases to handle.
> I'm not sure changes at the PHY always transition through down. So you
> could for example get a callback saying the link is up, 1Gbps, then a
> second call saying it has dropped to 100Mbps, if your
> cables/connectors are bad.
> 
> If your hardware has problems, it might be safest to stop everything
> in the callback, make configuration changes, and they start everything
> back up. A link negotiation change is not something you expect to
> happen often. So making it slow but robust is O.K.

What you've described is, in fact, the existing implementation! ;-)

nb8800_pause_config() checks for netif_running() and, when true,
tries to disable everything, make the change, then re-enable
everything.

The problem with this is the following mind-boggling quirk of
the hardware: once RX DMA is enabled, there is no supported
way to disable it! Thus, I'm trying to find a clean way to set
the control flow parameter BEFORE enabling the MAC.

Regards.

Waiting for the PHY to complete auto-negotiation

[Mason]

Hello,

I've been trying to wrap my head around Ethernet auto-negotiation,
vs actual / real packets seen at the MAC layer. I found the relevant
Wikipedia article to be fairly informative:

https://en.wikipedia.org/wiki/Autonegotiation

The reason I care is that my Ethernet HW does not allow changing the
flow control setting once the MAC has started (more specifically, once
RX DMA has been enabled).

In nb8800_open(), the code currently works in this order:

nb8800_start_rx(dev);
phy_start(phydev);

The first line enables the MAC (and DMA).
The second enables the PHY and starts auto-negotiation.

This is a problem: I would like for PHY auto-negotiation to be
/complete/ before I enable the MAC.

What is the recommended way to wait for the PHY?

AFAICT, the PHY layer calls back into the eth driver through the
adjust_link() callback registered through of_phy_connect().
It seems like this might be a good place to enable the MAC?
(When some other conditions are true.)

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 06/09/2017 20:00, David Daney wrote:
> On 08/31/2017 11:29 AM, Florian Fainelli wrote:
>> On 08/31/2017 11:12 AM, Mason wrote:
>>> On 31/08/2017 19:53, Florian Fainelli wrote:
>>>> On 08/31/2017 10:49 AM, Mason wrote:
>>>>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>>>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>>>>> and phy_state_machine() running in PHY_HALTED state and calling
>>>>>> netif_carrier_off().
>>>>>
>>>>> I must be missing something.
>>>>> (Since a thread cannot race against itself.)
>>>>>
>>>>> phy_disconnect calls phy_stop_machine which
>>>>> 1) stops the work queue from running in a separate thread
>>>>> 2) calls phy_state_machine *synchronously*
>>>>>   which runs the PHY_HALTED case with everything well-defined
>>>>> end of phy_stop_machine
>>>>>
>>>>> phy_disconnect only then calls phy_detach()
>>>>> which makes future calls of phy_state_machine perilous.
>>>>>
>>>>> This all happens in the same thread, so I'm not yet
>>>>> seeing where the race happens?
>>>>
>>>> The race is as described in David's earlier email, so let's recap:
>>>>
>>>> Thread 1   Thread 2
>>>> phy_disconnect()
>>>> phy_stop_interrupts()
>>>> phy_stop_machine()
>>>> phy_state_machine()
>>>>   -> queue_delayed_work()
>>>> phy_detach()
>>>>phy_state_machine()
>>>>-> netif_carrier_off()
>>>>
>>>> If phy_detach() finishes earlier than the workqueue had a chance to be
>>>> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
>>>> de-reference.
>>>>
>>>> workqueues are not tasklets, the CPU scheduling them gets no guarantee
>>>> they will run on the same CPU.
>>>
>>> Something does not add up.
>>>
>>> The synchronous call to phy_state_machine() does:
>>>
>>> case PHY_HALTED:
>>> if (phydev->link) {
>>> phydev->link = 0;
>>> netif_carrier_off(phydev->attached_dev);
>>> phy_adjust_link(phydev);
>>> do_suspend = true;
>>> }
>>>
>>> then sets phydev->link = 0; therefore subsequent calls to
>>> phy_state_machin() will be no-op.
>>
>> Actually you are right, once phydev->link is set to 0 these would become
>> no-ops. Still scratching my head as to what happens for David then...
>>
>>>
>>> Also, queue_delayed_work() is only called in polling mode.
>>> David stated that he's using interrupt mode.
> 
> Did you see what I wrote?
> 
> phy_disconnect() calls phy_stop_interrupts() which puts it into polling 
> mode.  So the polling work gets queued unconditionally.

I did address that remark in
https://www.mail-archive.com/netdev@vger.kernel.org/msg186336.html

int phy_stop_interrupts(struct phy_device *phydev)
{
int err = phy_disable_interrupts(phydev);

if (err)
phy_error(phydev);

free_irq(phydev->irq, phydev);

/* If work indeed has been cancelled, disable_irq() will have
 * been left unbalanced from phy_interrupt() and enable_irq()
 * has to be called so that other devices on the line work.
 */
while (atomic_dec_return(>irq_disable) >= 0)
enable_irq(phydev->irq);

return err;
}

Which part of this function changes phydev->irq to PHY_POLL?

Perhaps phydev->drv->config_intr?

What PHY are you using?

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 31/08/2017 21:18, Florian Fainelli wrote:


On 08/31/2017 12:09 PM, Mason wrote:


On 31/08/2017 19:03, Florian Fainelli wrote:


On 08/31/2017 05:29 AM, Marc Gonzalez wrote:


On 31/08/2017 02:49, Florian Fainelli wrote:


The original motivation for this change originated from Marc Gonzalez
indicating that his network driver did not have its adjust_link callback
executing with phydev->link = 0 while he was expecting it.


I expect the core to call phy_adjust_link() for link changes.
This used to work back in 3.4 and was broken somewhere along
the way.


If that was working correctly in 3.4 surely we can look at the diff and
figure out what changed, even maybe find the offending commit, can you
do that?


Bisecting would a be a huge pain because my platform was
not upstream until v4.4


Then just diff the file and try to pinpoint which commit may have
changed that?


Running 'ip link set eth0 down' on the command-line.

In v3.4 => adjust_link() callback is called
In v4.5 => adjust_link() callback is NOT called

$ git log --oneline --no-merges v3.4..v4.5 drivers/net/phy/phy.c | wc -l
59

I'm not sure what "just diff the file" entails.
I can't move 3.4 up, nor move 4.5 down.
I'm not even sure the problem comes from drivers/net/phy/phy.c
to be honest.

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 31/08/2017 21:18, Florian Fainelli wrote:


On 08/31/2017 12:09 PM, Mason wrote:


1) nb8800_link_reconfigure() calls phy_print_status()
which prints the "Link down" and "Link up" messages
to the console. With the patch reverted, nothing is
printed when the link goes down, and the result is
random when the link comes up. Sometimes, we get
down + up, sometimes just up.


Nothing printed when you bring down the network interface as a result of
not signaling the link down, there is a small nuance here.


Let me first focus on the "Link down" message.

Do you agree that such a message should be printed when the
link goes down, not when the link comes up?

Perhaps the issue is that the 2 following cases need to be
handled differently:
A) operator sets link down on the command-line
B) asynchronous event makes link go down (peer is dead, cable is cut, etc)

In B) the PHY state machine keeps on running, and eventually
calls adjust_link()

In A) the driver calls phy_stop() and phy_disconnect() and
therefore adjust_link() will not be called?

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 31/08/2017 20:29, Florian Fainelli wrote:

On 08/31/2017 11:12 AM, Mason wrote:

On 31/08/2017 19:53, Florian Fainelli wrote:

On 08/31/2017 10:49 AM, Mason wrote:

On 31/08/2017 18:57, Florian Fainelli wrote:

And the race is between phy_detach() setting phydev->attached_dev = NULL
and phy_state_machine() running in PHY_HALTED state and calling
netif_carrier_off().


I must be missing something.
(Since a thread cannot race against itself.)

phy_disconnect calls phy_stop_machine which
1) stops the work queue from running in a separate thread
2) calls phy_state_machine *synchronously*
  which runs the PHY_HALTED case with everything well-defined
end of phy_stop_machine

phy_disconnect only then calls phy_detach()
which makes future calls of phy_state_machine perilous.

This all happens in the same thread, so I'm not yet
seeing where the race happens?


The race is as described in David's earlier email, so let's recap:

Thread 1Thread 2
phy_disconnect()
phy_stop_interrupts()
phy_stop_machine()
phy_state_machine()
  -> queue_delayed_work()
phy_detach()
phy_state_machine()
-> netif_carrier_off()

If phy_detach() finishes earlier than the workqueue had a chance to be
scheduled and process PHY_HALTED again, then we trigger the NULL pointer
de-reference.

workqueues are not tasklets, the CPU scheduling them gets no guarantee
they will run on the same CPU.


Something does not add up.

The synchronous call to phy_state_machine() does:

case PHY_HALTED:
if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
phy_adjust_link(phydev);
do_suspend = true;
}

then sets phydev->link = 0; therefore subsequent calls to
phy_state_machin() will be no-op.


Actually you are right, once phydev->link is set to 0 these would become
no-ops. Still scratching my head as to what happens for David then...



Also, queue_delayed_work() is only called in polling mode.
David stated that he's using interrupt mode.


Right that's confusing too now. David can you check if you tree has:

49d52e8108a21749dc2114b924c907db43358984 ("net: phy: handle state
correctly in phy_stop_machine")


Hello David,

A week ago, you wrote about my patch:
"This is broken.  Please revert."

I assume you tested the revert locally, and that reverting did make
the crash disappear. Is that correct?

The reason I ask is because the analysis you provided contains some
flaws, as noted above. But, if reverting my patch did fix your issue,
then perhaps understanding *why* is unimportant.

I'm a bit baffled that it took less than 90 minutes for your request
to be approved, and the patch reverted in all branches, before I even
had a chance to comment.

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 31/08/2017 19:03, Florian Fainelli wrote:

> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>
>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>
>>> The original motivation for this change originated from Marc Gonzalez
>>> indicating that his network driver did not have its adjust_link callback
>>> executing with phydev->link = 0 while he was expecting it.
>>
>> I expect the core to call phy_adjust_link() for link changes.
>> This used to work back in 3.4 and was broken somewhere along
>> the way.
> 
> If that was working correctly in 3.4 surely we can look at the diff and
> figure out what changed, even maybe find the offending commit, can you
> do that?

Bisecting would a be a huge pain because my platform was
not upstream until v4.4

You mentioned the guarantees made by PHYLIB.
When is the adjust_link callback guaranteed to be called?

>>> PHYLIB has never made any such guarantees ever because phy_stop() merely
>>> just tells the workqueue to move into PHY_HALTED state which will happen
>>> asynchronously.
>>
>> My original proposal was to fix the issue in the driver.
>> I'll try locating it in my archives.
> 
> Yes I remember you telling that, by the way I don't think you ever
> provided a clear explanation why this is absolutely necessary for your
> driver though?

1) nb8800_link_reconfigure() calls phy_print_status()
which prints the "Link down" and "Link up" messages
to the console. With the patch reverted, nothing is
printed when the link goes down, and the result is
random when the link comes up. Sometimes, we get
down + up, sometimes just up.

2) nb8800_link_reconfigure() does some HW init when
the link state changes. If we miss some notifications,
we might not perform some HW init, and stuff breaks.

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 31/08/2017 19:53, Florian Fainelli wrote:
> On 08/31/2017 10:49 AM, Mason wrote:
>> On 31/08/2017 18:57, Florian Fainelli wrote:
>>> And the race is between phy_detach() setting phydev->attached_dev = NULL
>>> and phy_state_machine() running in PHY_HALTED state and calling
>>> netif_carrier_off().
>>
>> I must be missing something.
>> (Since a thread cannot race against itself.)
>>
>> phy_disconnect calls phy_stop_machine which
>> 1) stops the work queue from running in a separate thread
>> 2) calls phy_state_machine *synchronously*
>>  which runs the PHY_HALTED case with everything well-defined
>> end of phy_stop_machine
>>
>> phy_disconnect only then calls phy_detach()
>> which makes future calls of phy_state_machine perilous.
>>
>> This all happens in the same thread, so I'm not yet
>> seeing where the race happens?
> 
> The race is as described in David's earlier email, so let's recap:
> 
> Thread 1  Thread 2
> phy_disconnect()
> phy_stop_interrupts()
> phy_stop_machine()
> phy_state_machine()
>  -> queue_delayed_work()
> phy_detach()
>   phy_state_machine()
>   -> netif_carrier_off()
> 
> If phy_detach() finishes earlier than the workqueue had a chance to be
> scheduled and process PHY_HALTED again, then we trigger the NULL pointer
> de-reference.
> 
> workqueues are not tasklets, the CPU scheduling them gets no guarantee
> they will run on the same CPU.

Something does not add up.

The synchronous call to phy_state_machine() does:

case PHY_HALTED:
if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
phy_adjust_link(phydev);
do_suspend = true;
}

then sets phydev->link = 0; therefore subsequent calls to
phy_state_machin() will be no-op.

Also, queue_delayed_work() is only called in polling mode.
David stated that he's using interrupt mode.

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 31/08/2017 18:57, Florian Fainelli wrote:
> And the race is between phy_detach() setting phydev->attached_dev = NULL
> and phy_state_machine() running in PHY_HALTED state and calling
> netif_carrier_off().

I must be missing something.
(Since a thread cannot race against itself.)

phy_disconnect calls phy_stop_machine which
1) stops the work queue from running in a separate thread
2) calls phy_state_machine *synchronously*
 which runs the PHY_HALTED case with everything well-defined
end of phy_stop_machine

phy_disconnect only then calls phy_detach()
which makes future calls of phy_state_machine perilous.

This all happens in the same thread, so I'm not yet
seeing where the race happens?

Regards.

Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"

[Mason]

On 31/08/2017 18:36, David Daney wrote:
> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>
>>> This reverts commit 7ad813f208533cebfcc32d3d7474dc1677d1b09a ("net: phy:
>>> Correctly process PHY_HALTED in phy_stop_machine()") because it is
>>> creating the possibility for a NULL pointer dereference.
>>>
>>> David Daney provide the following call trace and diagram of events:
>>>
>>> When ndo_stop() is called we call:
>>>
>>>   phy_disconnect()
>>>  +---> phy_stop_interrupts() implies: phydev->irq = PHY_POLL;
>>
>> What does this mean?
> 
> I meant that after the call to phy_stop_interrupts(), phydev->irq = 
> PHY_POLL;

I must be missing something.

http://elixir.free-electrons.com/linux/latest/source/drivers/net/phy/phy.c#L868

phy_stop_interrupts() doesn't change phydev->irq right?

Only phy_start_interrupts() sets phydev->irq to
PHY_POLL if it cannot set up interrupt mode.

Regards.

Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

On 02/08/2017 22:02, Mason wrote:

> I need to run the test slightly slower, to prevent packet loss
> at the sender.

# iperf3 -c 172.27.64.45 -u -b 0 -l 1000
Connecting to host 172.27.64.45, port 5201
[  4] local 172.27.64.1 port 42607 connected to 172.27.64.45 port 5201
[ ID] Interval   Transfer Bandwidth   Total Datagrams
[  4]   0.00-1.00   sec   111 MBytes   931 Mbits/sec  116420  
[  4]   1.00-2.00   sec   111 MBytes   931 Mbits/sec  116390  
[  4]   2.00-3.00   sec   111 MBytes   930 Mbits/sec  116220  
[  4]   3.00-4.00   sec   111 MBytes   930 Mbits/sec  116310  
[  4]   4.00-5.00   sec   111 MBytes   931 Mbits/sec  116380  
[  4]   5.00-6.00   sec   111 MBytes   930 Mbits/sec  116280  
[  4]   6.00-7.00   sec   111 MBytes   931 Mbits/sec  116390  
[  4]   7.00-8.00   sec   111 MBytes   931 Mbits/sec  116370  
[  4]   8.00-9.00   sec   111 MBytes   931 Mbits/sec  116340  
[  4]   9.00-10.00  sec   111 MBytes   930 Mbits/sec  116310  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  4]   0.00-10.00  sec  1.08 GBytes   931 Mbits/sec  0.009 ms  278644/1163363 
(24%)  
[  4] Sent 1163363 datagrams

iperf Done.


# iperf3 -s
Accepted connection from 172.27.64.1, port 42966
[  5] local 172.27.64.45 port 5201 connected to 172.27.64.1 port 42607
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  5]   0.00-1.00   sec  81.1 MBytes   681 Mbits/sec  0.017 ms  26834/111909 
(24%)  
[  5]   1.00-2.00   sec  84.2 MBytes   706 Mbits/sec  0.019 ms  28127/116384 
(24%)  
[  5]   2.00-3.00   sec  84.2 MBytes   706 Mbits/sec  0.013 ms  27946/116204 
(24%)  
[  5]   3.00-4.00   sec  84.5 MBytes   709 Mbits/sec  0.013 ms  27674/116311 
(24%)  
[  5]   4.00-5.00   sec  84.6 MBytes   709 Mbits/sec  0.015 ms  27712/116387 
(24%)  
[  5]   5.00-6.00   sec  84.5 MBytes   709 Mbits/sec  0.010 ms  27649/116265 
(24%)  
[  5]   6.00-7.00   sec  84.3 MBytes   707 Mbits/sec  0.011 ms  27995/116382 
(24%)  
[  5]   7.00-8.00   sec  84.3 MBytes   707 Mbits/sec  0.013 ms  27972/116387 
(24%)  
[  5]   8.00-9.00   sec  84.3 MBytes   708 Mbits/sec  0.020 ms  27899/116343 
(24%)  
[  5]   9.00-10.00  sec  84.4 MBytes   708 Mbits/sec  0.014 ms  27759/116305 
(24%)  
[  5]  10.00-10.04  sec  3.25 MBytes   710 Mbits/sec  0.009 ms  1077/4486 (24%) 
 
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  5]   0.00-10.04  sec  0.00 Bytes  0.00 bits/sec  0.009 ms  278644/1163363 
(24%)  


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.

Regards.

Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

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??
In the nb8800 driver, RX_BUF_SIZE is only 1552,
how would it deal with jumbo frames... truncate?

> # iperf3 -c 172.27.64.45 -u -b 950M -l 800
> Connecting to host 172.27.64.45, port 5201
> [  4] local 172.27.64.1 port 35197 connected to 172.27.64.45 port 5201
> [ ID] Interval   Transfer Bandwidth   Total Datagrams
> [  4]   0.00-1.00   sec  90.6 MBytes   760 Mbits/sec  118724  
> [  4]   1.00-2.00   sec   107 MBytes   894 Mbits/sec  139718  

107 MB in 139718 packets => 766 bytes per packet
800 - 8 (UDP) - 20 (IPv4) = 772 bytes per packet
I suppose that's close enough...
772 * 139718 = 107.86 MB

I need to run the test slightly slower, to prevent
packet loss at the sender.

Perhaps -b 0 or -b 800M

Regards.

Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

On 02/08/2017 18:10, Måns Rullgård wrote:

> ping -f is limited to 100 packets per second.
> Use something like iperf in UDP mode instead.

CLIENT: DESKTOP
# 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  
[  4]   2.00-3.00   sec   114 MBytes   953 Mbits/sec  14542  
[  4]   3.00-4.00   sec   114 MBytes   953 Mbits/sec  14541  
[  4]   4.00-5.00   sec   114 MBytes   953 Mbits/sec  14542  
[  4]   5.00-6.00   sec   114 MBytes   953 Mbits/sec  14541  
[  4]   6.00-7.00   sec   114 MBytes   953 Mbits/sec  14535  
[  4]   7.00-8.00   sec   114 MBytes   953 Mbits/sec  14536  
[  4]   8.00-9.00   sec   114 MBytes   953 Mbits/sec  14543  
[  4]   9.00-10.00  sec   114 MBytes   953 Mbits/sec  14541  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  4]   0.00-10.00  sec  1.10 GBytes   943 Mbits/sec  0.247 ms  132176/143788 
(92%)  
[  4] Sent 143788 datagrams

iperf Done.


SERVER: TANGO BOARD
# iperf3 -s
Accepted connection from 172.27.64.1, port 44995
[  5] local 172.27.64.45 port 5201 connected to 172.27.64.1 port 55533
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  5]   0.00-1.00   sec  7.90 MBytes  66.2 Mbits/sec  0.218 ms  11365/12376 
(92%)  
[  5]   1.00-2.00   sec  9.13 MBytes  76.6 Mbits/sec  0.230 ms  13381/14550 
(92%)  
[  5]   2.00-3.00   sec  9.12 MBytes  76.5 Mbits/sec  0.290 ms  13372/14540 
(92%)  
[  5]   3.00-4.00   sec  9.11 MBytes  76.4 Mbits/sec  0.292 ms  13369/14535 
(92%)  
[  5]   4.00-5.00   sec  9.18 MBytes  77.0 Mbits/sec  0.178 ms  13374/14549 
(92%)  
[  5]   5.00-6.00   sec  9.10 MBytes  76.3 Mbits/sec  0.228 ms  13367/14532 
(92%)  
[  5]   6.00-7.00   sec  9.26 MBytes  77.7 Mbits/sec  0.607 ms  13356/14541 
(92%)  
[  5]   7.00-8.00   sec  9.23 MBytes  77.4 Mbits/sec  0.507 ms  13364/14545 
(92%)  
[  5]   8.00-9.00   sec  9.20 MBytes  77.1 Mbits/sec  0.215 ms  13351/14528 
(92%)  
[  5]   9.00-10.00  sec  9.16 MBytes  76.9 Mbits/sec  0.188 ms  13356/14529 
(92%)  
[  5]  10.00-10.04  sec   336 KBytes  72.2 Mbits/sec  0.247 ms  521/563 (93%)  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  5]   0.00-10.04  sec  0.00 Bytes  0.00 bits/sec  0.247 ms  132176/143788 
(92%)  


There is some packet loss, but the board doesn't lose connectivity.

With smaller packets...

# iperf3 -c 172.27.64.45 -u -b 950M -l 800
Connecting to host 172.27.64.45, port 5201
[  4] local 172.27.64.1 port 35197 connected to 172.27.64.45 port 5201
[ ID] Interval   Transfer Bandwidth   Total Datagrams
[  4]   0.00-1.00   sec  90.6 MBytes   760 Mbits/sec  118724  
[  4]   1.00-2.00   sec   107 MBytes   894 Mbits/sec  139718  
[  4]   2.00-3.00   sec   106 MBytes   889 Mbits/sec  138918  
[  4]   3.00-4.00   sec   107 MBytes   895 Mbits/sec  139768  
[  4]   4.00-5.00   sec   106 MBytes   891 Mbits/sec  139275  
[  4]   5.00-6.00   sec   107 MBytes   895 Mbits/sec  139862  
[  4]   6.00-7.00   sec   107 MBytes   895 Mbits/sec  139825  
[  4]   7.00-8.00   sec   107 MBytes   895 Mbits/sec  139775  
[  4]   8.00-9.00   sec   107 MBytes   895 Mbits/sec  139849  
[  4]   9.00-10.00  sec   107 MBytes   895 Mbits/sec  139835  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  4]   0.00-10.00  sec  1.02 GBytes   880 Mbits/sec  0.009 ms  564117/1375520 
(41%)  
[  4] Sent 1375520 datagrams

iperf Done.


Accepted connection from 172.27.64.1, port 46151
[  5] local 172.27.64.45 port 5201 connected to 172.27.64.1 port 35197
[ ID] Interval   Transfer Bandwidth   JitterLost/Total 
Datagrams
[  5]   0.00-1.00   sec  60.8 MBytes   510 Mbits/sec  0.004 ms  33508/113252 
(30%)  
iperf3: OUT OF ORDER - incoming packet = 147146 and received packet = 0 AND SP 
= 147497
iperf3: OUT OF ORDER - incoming packet = 146128 and received packet = 0 AND SP 
= 147690
iperf3: OUT OF ORDER - incoming packet = 146067 and received packet = 0 AND SP 
= 147863
iperf3: OUT OF ORDER - incoming packet = 147242 and received packet = 0 AND SP 
= 148039
iperf3: OUT OF ORDER - incoming packet = 163837 and received packet = 0 AND SP 
= 164363
[  5]   1.00-2.00   sec  61.0 MBytes   511 Mbits/sec  0.198 ms  59635/139649 
(43%)  
iperf3: OUT OF ORDER - incoming packet = 286437 and received packet = 0 AND SP 
= 287226
iperf3: OUT OF ORDER - incoming packet = 302990 and received packet = 0 AND SP 
= 305710
[  5]   2.00-3.00   sec  61.5 MBytes   517 Mbits/sec  0.005 ms  58369/138944 
(42%)  
iperf3: OUT OF ORDER - incoming packet 

Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

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.

Regards.

Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

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?

Regards.

Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

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 :-)

The "new" method is available on the SMP8670 onward
(released Jan 2011). I might try implementing it for
"sigma,smp8734-ethernet" boards.

Regards.

Re: [RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

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.

On a FastE switch:
[   11.611613] ENTER nb8800_pause_config
[   11.615942] rxcr=06100a8f pause_rx=1 pause_tx=1 pause=1 asym_pause=0
[   11.825535] nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow 
control rx/tx

In this case pause_tx and RCR_FL don't match, so we hit
nb8800_dma_stop() and the HW block becomes deaf.

In conclusion, two issues I've been discussing:
A) RX wedged after ndo_close
B) flow control breaks on FastE switches
are in fact two symptoms of the same root issue.

Regards.

Re: [RFC PATCH v2 1/2] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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'll take a closer look at multicast settings.

> 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.

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.

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.)


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?
(IMO, my way is great in its simplicity and code reuse.)

Regards.

[RFC PATCH v2 2/2] net: ethernet: nb8800: Add suspend/resume support

[Mason]

Wrappers around nb8800_stop and nb8800_open.
---
 drivers/net/ethernet/aurora/nb8800.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
index aa18ea25d91f..607064a6d7a1 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1012,7 +1012,6 @@ static int nb8800_stop(struct net_device *dev)
netif_stop_queue(dev);
napi_disable(>napi);
 
-   nb8800_dma_stop(dev);
nb8800_mac_rx(dev, false);
nb8800_mac_tx(dev, false);
 
@@ -1526,6 +1525,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))
+   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))
+   nb8800_open(dev);
+
+   return 0;
+}
+
 static struct platform_driver nb8800_driver = {
.driver = {
.name   = "nb8800",
@@ -1533,6 +1552,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.11.0

[RFC PATCH v2 1/2] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
index e94159507847..aa18ea25d91f 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);
@@ -1246,11 +1249,6 @@ static int nb8800_hw_init(struct net_device *dev)
nb8800_writeb(priv, NB8800_PQ1, val >> 8);
nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
 
-   /* Auto-negotiate by default */
-   priv->pause_aneg = true;
-   priv->pause_rx = true;
-   priv->pause_tx = true;
-
nb8800_mc_init(dev, 0);
 
return 0;
@@ -1350,6 +1348,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 +1401,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 +1420,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,21 +1461,16 @@ 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;
dev->irq = irq;
 
+   /* Auto-negotiate by default */
+   priv->pause_aneg = true;
+   priv->pause_rx = true;
+   priv->pause_tx = true;
+
mac = of_get_mac_address(pdev->dev.of_node);
if (mac)
ether_addr_copy(dev->dev_addr, mac);
@@ -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

[RFC PATCH v2 0/2] nb8800 suspend/resume support

[Mason]

Hello,

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

Regards.

Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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


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...

Depressing. I've run out of ideas.


BOARD A LOGS:

# test_eth.sh 
[   18.037782] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
172.27.64.1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.39/0.39/0.39
[   20.617917] nb8800_stop from __dev_close_many
[   20.622314] ++ETH++ gw32 reg=f0026020 val=0092
[   20.627135] ++ETH++ gw32 reg=f0026020 val=8092
[   20.631973] ++ETH++ gr32 reg=f0026024 val=ec00
[   20.636782] ++ETH++ gw32 reg=f0026020 val=0492
[   20.641601] ++ETH++ gw32 reg=f0026020 val=8492
[   20.646440] ++ETH++ gw32 reg=f0026020 val=0093
[   20.651258] ++ETH++ gw32 reg=f0026020 val=8093
[   20.656095] ++ETH++ gr32 reg=f0026024 val=
[   20.660909] ++ETH++ POLL reg=f0026100 val=005c0afe
[   20.665743] ++ETH++ gr8  reg=f0026004 val=2b
[   20.670028] ++ETH++ gw8  reg=f0026004 val=0b
[   20.674313] ++ETH++ gr8  reg=f0026044 val=81
[   20.678598] ++ETH++ gw8  reg=f0026044 val=85
[   20.682883] ++ETH++ gw32 reg=f002610c val=9de0c000
[   20.687693] ++ETH++ gw32 reg=f0026100 val=005c0aff
[   20.692503] ++ETH++ gw32 reg=f002610c val=9de0c000
[   20.697312] ++ETH++ gw32 reg=f0026100 val=005c0aff
[   20.702121] ++ETH++ gw32 reg=f002610c val=9de0c000
[   20.706929] ++ETH++ gw32 reg=f0026100 val=005c0aff
[   20.712738] ++ETH++ POLL reg=f0026200 val=06100a8e
[   20.717547] err=0 retry=5
[   20.720172] ++ETH++ gr8  reg=f0026004 val=0b
[   20.724456] ++ETH++ gw8  reg=f0026004 val=2b
[   20.728742] ++ETH++ gr8  reg=f0026044 val=85
[   20.733026] ++ETH++ gw8  reg=f0026044 val=81
[   20.737349] ++ETH++ gw32 reg=f002620c val=9de04000
[   20.742158] nb8800_dma_stop=0
[   21.845224] ENTER nb8800_irq
[   21.848116] ++ETH++ gr32 reg=f0026104 val=0005
[   21.852927] ++ETH++ gw32 reg=f0026104 val=0005
[   21.857738] ++ETH++ gr32 reg=f0026204 val=0004
[   21.862547] ++ETH++ gw32 reg=f0026204 val=0004
[   21.867356] ++ETH++ gw32 reg=f0026218 val=003cc4a4
[   21.872164]  EXIT nb8800_irq
[   24.373448] ++ETH++ gr8  reg=f0026004 val=2b
[   24.377755] ++ETH++ gw8  reg=f0026004 val=2a
[   24.382054] ++ETH++ gr32 reg=f0026100 val=00080afe
[   24.386876] ++ETH++ gr8  reg=f0026000 val=1d
[   24.391192] ++ETH++ gw8  reg=f0026000 val=1c
[   25.706747] ++ETH++ gw32 reg=f0026020 val=0092
[   25.711578] ++ETH++ gw32 reg=f0026020 val=8092
[   25.716427] ++ETH++ gr32 reg=f0026024 val=
[   25.721248] ++ETH++ gw32 reg=f0026020 val=0492
[   25.726091] ++ETH++ gw32 reg=f0026020 val=8492

Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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"?

The "put adapter in loop-back mode so we can send ourselves fake packets"
shenanigans seems completely insane, if you ask me.

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?

I've tried using spinlocks and delays to get parallel execution
down to a minimum, and have the same logs on both boards.

Regards.

Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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."

>> 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 have to agree with Mans here that the commit message explanation is
> not good enough to understand how the RX path is hosed after a call to
> ndo_stop() it would be good, both for you and for the people maintaining
> this driver to understand what happens exactly so the fix is correct,
> understood and maintainable. The patch itself looks reasonable with the
> limited description given, but it's the description itself that needs
> changing.

I have logged all register reads/writes occurring while
nb8800_stop() is executing.



1) BOARD A -- EVERYTHING WORKS AS EXPECTED

# test_eth.sh 
[   13.293669] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
172.27.64.1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.41/0.41/0.41
[   15.874627] nb8800_stop from __dev_close_many
[   15.879044] ++ETH++ gw32 reg=f0026020 val=0092
[   15.883900] ++ETH++ gw32 reg=f0026020 val=8092
[   15.888969] ++ETH++ gr32 reg=f0026024 val=ec00
[   15.893809] ++ETH++ gw32 reg=f0026020 val=0492
[   15.898697] ++ETH++ gw32 reg=f0026020 val=8492
[   15.903582] ++ETH++ gw32 reg=f0026020 val=0093
[   15.908423] ++ETH++ gw32 reg=f0026020 val=8093
[   15.913272] ++ETH++ gr32 reg=f0026024 val=
[   15.918160] ++ETH++ gr8  reg=f0026004 val=2b
[   15.922459] ++ETH++ gw8  reg=f0026004 val=0b
[   15.926782] ++ETH++ gr8  reg=f0026044 val=81
[   15.931123] ++ETH++ gw8  reg=f0026044 val=85
[   15.935457] ++ETH++ gw32 reg=f002610c val=9de74000
[   15.940317] ++ETH++ gw32 reg=f0026100 val=005c0aff
[   15.945187] ENTER nb8800_irq
[   15.948077] ++ETH++ gr32 reg=f0026104 val=0004
[   15.952887] ++ETH++ gw32 reg=f0026104 val=0004
[   15.957697] ++ETH++ gr32 reg=f0026204 val=0004
[   15.962507] ++ETH++ gw32 reg=f0026204 val=0004
[   15.967316] ++ETH++ gw32 reg=f0026218 val=003cc4a4
[   15.972149] ENTER nb8800_irq
[   15.975039] ++ETH++ gr32 reg=f0026104 val=0001
[   15.979848] ++ETH++ gw32 reg=f0026104 val=0001
[   15.984658] ++ETH++ gr32 reg=f0026204 val=
[   16.045509] ++ETH++ gw32 reg=f002610c val=9de74000
[   16.050329] ++ETH++ gw32 reg=f0026100 val=005c0aff
[   16.055150] ENTER nb8800_irq
[   16.058042] ++ETH++ gr32 reg=f0026104 val=0004
[   16.062852] ++ETH++ gw32 reg=f0026104 val=0004
[   16.067662] ++ETH++ gr32 reg=f0026204 val=0004
[   16.072470] ++ETH++ gw32 reg=f0026204 val=0004
[   16.077279] ++ETH++ gw32 reg=f0026218 val=003cc4a4
[   16.082100] ENTER nb8800_irq
[   16.084993] ++ETH++ gr32 reg=f0026104 val=0001
[   16.089802] ++ETH++ gw32 reg=f0026104 val=0001
[   16.094611] ++ETH++ gr32 reg=f0026204 val=
[   16.099454] ++ETH++ gr8  reg=f0026004 val=0b
[   16.103752] ++ETH++ gw8  reg=f0026004 val=2b
[   16.108057] ++ETH++ gr8  reg=f0026044 val=85
[   16.112353] ++ETH++ gw8  reg=f0026044 val=81
[   16.116699] ++ETH++ gw32 reg=f002620c val=9f7b2000
[   16.121528] ++ETH++ gr8  reg=f0026004 val=2b
[   16.125827] ++ETH++ gw8  reg=f0026004 val=2a
[   16.130126] ++ETH++ gr32 reg=f0026100 val=00080afe
[   16.134945] ++ETH++ gr8  reg=f0026000 val=1d
[   16.139238] ++ETH++ gw8  reg=f0026000 val=1c
[   16.143534] ++ETH++ gw32 reg=f0026020 val=0092
[   16.148363] ++ETH++ gw32 reg=f0026020 val=8092
[   16.153209] ++ETH++ gr32 reg=f0026024 val=
[   16.158027] ++ETH++ gw32 reg=f0026020 val=0492
[   16.162856] ++ETH++ gw32 reg=f0026020 val=8492
[   16.167702] ++ETH++ gw32 reg=f0026020 val=0093
[   16.172531] ++ETH++ gw32 reg=f0026020 val=8093
[   16.177377] ++ETH++ gr32 reg=f0026024 val=
[   16.182338] nb8800 26000.ethernet eth0: Link is Down
[   16.187361] ++ETH++ gw32 reg=f0026020 val=0092
[   16.192194] ++ETH++ gw32 reg=f0026020 val=8092
[   16.197052] ++ETH++ gr32 reg=f0026024 val=
[   16.201887] ++ETH++ gw32 reg=f0026020 val=0092
[   16.206717] ++ETH++ gw32 reg=f0026020 val=8092
[   16.211575] ++ETH++ gr32 reg=f0026024 val=
[   16.216394] ++ETH++ gw32 reg=f0026020 val=0080
[   16.221235] ++ETH++ gw32 reg=f0026020 val=8080
[   16.226084] ++ETH++ gr32 reg=f0026024 val=1000
[   16.230913] ++ETH++ gw32 reg=f0026020 val=04801800
[   16.235742] ++ETH++ gw32 reg=f0026020 val=848

Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

On 29/07/2017 22:15, Florian Fainelli wrote:

> On 07/29/2017 05:44 AM, Mason wrote:
>
>> We tested 4 switches, and DHCP failed on 3 of them.
>> Disabling pause frames "fixed" that.
> 
> OK, so it is this problem that you reported about before?

The "Ethernet flow control / pause frames" issue
is separate from the "link down wedges RX" issue.

We discussed the former back in November 2016:

https://www.mail-archive.com/netdev@vger.kernel.org/msg137094.html
https://patchwork.ozlabs.org/patch/694577/

Wait a second... I see that you and Mans had the
following exchange:

https://www.mail-archive.com/netdev@vger.kernel.org/msg138175.html

Mans mentions disabling DMA to be able to change
the flow control bits. The current theory is that
it is disabling DMA in ndo_stop that wedges RX.

So maybe the two issues are related after all...

I hate all these hardware quirks. Why can't HW
engineers make stuff that "just works"...

> Pause frames are tricky in that receiving pause frames means you
> should backpressure your transmitter and sending pause frames happens
> when your receiver cannot keep up. It is somewhat conceivable that
> your HW implementation is bogus and that you can get the HW in a
> state where it gets permanently backpressured for instance? And then
> only a full re-init would get you out of this stuck state presumably?
> Are there significant differences at the DMA/Ethernet controller
> level between Tango 3 (is that the one Mans worked on?) and Tango 4
> for instance that could explain a behavioral difference?

I'll have to take a look at the issue in light of
the new information. FWIW, Mans has tango3&4 boards.
I work on newer boards. The HW dev *swears* there
have been no functional differences in the eth block
"forever". However, bus accesses are faster in recent
chips, which could change who wins specific races.

Regards.

Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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.

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.

Regards.

Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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.

> 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'll take a closer look at priv assignments on Monday.

Regards.

Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open

[Mason]

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...


>>> 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.

Regards.

Re: Problem with PHY state machine when using interrupts

[Mason]

On 25/07/2017 12:51, Mason wrote:

> Moving the call to phy_stop() down after all the MAC tear down
> avoids the hang.
> 
> As far as I understand, when we are shutting everything down,
> we don't need the phy_state_machine to run asynchronously.
> We can run it synchronously one last time after the delayed
> stuff has been disabled.

Below is my current WIP diff. (It conflates the two issues
I've been discussing. Splitting the diff required.)

Tested in interrupt mode:

# ip link set eth0 up
[   11.107547] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change UP 
-> AN
[   14.530329] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[   14.538136] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change AN 
-> RUNNING
# ip link set eth0 down
[   23.801018] nb8800 26000.ethernet eth0: Link is Down
# ip link set eth0 up
[   28.740870] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change UP 
-> AN
[   31.431528] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[   31.439350] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change AN 
-> RUNNING

Works as expected.

Tested in polling mode:

# ip link set eth0 up
[   23.001199] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change UP 
-> AN
[   24.024315] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change AN 
-> NOLINK
[   27.064355] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[   27.072156] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change 
NOLINK -> RUNNING
# ip link set eth0 down
[   42.134617] nb8800 26000.ethernet eth0: Link is Down
# ip link set eth0 up
[   48.381185] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change UP 
-> AN
[   49.410976] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change AN 
-> NOLINK
[   51.437686] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[   51.445486] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change 
NOLINK -> RUNNING

Works as expected.

Also tested on my old board, no regression seen.

Can you confirm that the changes to drivers/net/phy/phy.c
look reasonable?

Regards.


diff --git a/drivers/net/ethernet/aurora/nb8800.c 
b/drivers/net/ethernet/aurora/nb8800.c
index e94159507847..b5eba7958871 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -41,6 +41,7 @@
 
 static void nb8800_tx_done(struct net_device *dev);
 static int nb8800_dma_stop(struct net_device *dev);
+static int nb8800_init(struct net_device *dev);
 
 static inline u8 nb8800_readb(struct nb8800_priv *priv, int reg)
 {
@@ -957,6 +958,9 @@ static int nb8800_open(struct net_device *dev)
struct phy_device *phydev;
int err;
 
+   nb8800_init(dev);
+   priv->speed = 0;
+
/* clear any pending interrupts */
nb8800_writel(priv, NB8800_RXC_SR, 0xf);
nb8800_writel(priv, NB8800_TXC_SR, 0xf);
@@ -1004,8 +1008,6 @@ static int nb8800_stop(struct net_device *dev)
struct nb8800_priv *priv = netdev_priv(dev);
struct phy_device *phydev = dev->phydev;
 
-   phy_stop(phydev);
-
netif_stop_queue(dev);
napi_disable(>napi);
 
@@ -1013,6 +1015,7 @@ static int nb8800_stop(struct net_device *dev)
nb8800_mac_rx(dev, false);
nb8800_mac_tx(dev, false);
 
+   phy_stop(phydev);
phy_disconnect(phydev);
 
free_irq(dev->irq, dev);
@@ -1350,6 +1353,21 @@ static int nb8800_tango4_init(struct net_device *dev)
 };
 MODULE_DEVICE_TABLE(of, nb8800_dt_ids);
 
+static int nb8800_init(struct net_device *dev)
+{
+#ifndef RESET_IN_PROBE
+   struct nb8800_priv *priv = netdev_priv(dev);
+   const struct nb8800_ops *ops = priv->ops;
+
+   ops->reset(dev);
+   nb8800_hw_init(dev);
+   ops->init(dev);
+   nb8800_update_mac_addr(dev);
+#endif
+
+   return 0;
+}
+
 static int nb8800_probe(struct platform_device *pdev)
 {
const struct of_device_id *match;
@@ -1389,6 +1407,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,11 +1426,13 @@ static int nb8800_probe(struct platform_device *pdev)
 
spin_lock_init(>tx_lock);
 
+#ifdef RESET_IN_PROBE
if (ops && ops->reset) {
ret = ops->reset(dev);
if (ret)
goto err_disable_clk;
}
+#endif
 
bus = devm_mdiobus_alloc(>dev);
if (!bus) {
@@ -1454,6 +1475,7 @@ static int nb8800_probe(struct platform_device *pdev)
 
priv->mii_bus = bus;
 
+#ifdef RESET_IN_PROBE
ret = nb8800_hw_init(dev);
if (ret)
goto err_deregister_fixed_link;
@@ -1463,6 +1485

Re: Problem with PHY state machine when using interrupts

[Mason]

On 25/07/2017 00:39, Florian Fainelli wrote:

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d0626bf5c540..652e24b53f3f 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -968,6 +968,8 @@ void phy_stop(struct phy_device *phydev)
>  * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
>  * will not reenable interrupts.
>  */
> +   if (phy_interrupt_is_valid(phydev))
> +   phy_change(phydev);
>  }
>  EXPORT_SYMBOL(phy_stop);
> 

When setting link down:
- kernel logs Unbalanced enable for IRQ 30 (PHY interrupt)
- serial console hangs

# ip link set eth0 up
[  125.800057] ENTER nb8800_tangox_reset
[  125.803790] ++ETH++ gw8  reg=f0026424 val=00
[  125.817446] ++ETH++ gw8  reg=f0026424 val=01
[  125.821768] ++ETH++ gw16 reg=f0026420 val=0050
[  125.826241] ENTER nb8800_hw_init
[  125.829507] ++ETH++ gw8  reg=f0026000 val=1c
[  125.833808] ++ETH++ gw8  reg=f0026001 val=05
[  125.838122] ++ETH++ gw8  reg=f0026004 val=22
[  125.842421] ++ETH++ gw8  reg=f0026008 val=04
[  125.846717] ++ETH++ gw8  reg=f0026014 val=0c
[  125.851015] ++ETH++ gw8  reg=f0026051 val=00
[  125.855310] ++ETH++ gw8  reg=f0026052 val=ff
[  125.859607] ++ETH++ gw8  reg=f0026054 val=40
[  125.863903] ++ETH++ gw8  reg=f0026060 val=00
[  125.868226] ++ETH++ gw8  reg=f0026061 val=c3
[  125.872534] ENTER nb8800_mc_init
[  125.875800] ++ETH++ gw8  reg=f002602e val=00
[  125.880096] ENTER nb8800_tangox_init
[  125.883697] ++ETH++ gw8  reg=f0026400 val=01
[  125.887993] ENTER nb8800_tango4_init
[  125.891592] ENTER nb8800_update_mac_addr
[  125.895538] ++ETH++ gw8  reg=f002606a val=00
[  125.899835] ++ETH++ gw8  reg=f002606b val=16
[  125.904132] ++ETH++ gw8  reg=f002606c val=e8
[  125.908429] ++ETH++ gw8  reg=f002606d val=5e
[  125.912725] ++ETH++ gw8  reg=f002606e val=65
[  125.917022] ++ETH++ gw8  reg=f002606f val=bc
[  125.921319] ++ETH++ gw8  reg=f002603c val=00
[  125.925618] ++ETH++ gw8  reg=f002603d val=16
[  125.929912] ++ETH++ gw8  reg=f002603e val=e8
[  125.934210] ++ETH++ gw8  reg=f002603f val=5e
[  125.938505] ++ETH++ gw8  reg=f0026040 val=65
[  125.942802] ++ETH++ gw8  reg=f0026041 val=bc
[  125.947095] ENTER nb8800_open
[  125.950082] ENTER nb8800_dma_init
[  125.954025] ++ETH++ gw8  reg=f0026004 val=23
[  125.958331] ++ETH++ gw8  reg=f0026000 val=1d
[  126.027848] ENTER nb8800_set_rx_mode
[  126.031451] ENTER nb8800_mc_init
[  126.034702] ++ETH++ gw8  reg=f002602e val=00
[  126.039334] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change UP 
-> AN
[  126.046838] ENTER nb8800_set_rx_mode
[  126.050438] ENTER nb8800_mc_init
[  126.053688] ++ETH++ gw8  reg=f002602e val=00
[  126.057983] ++ETH++ gw8  reg=f0026028 val=01
[  126.062279] ++ETH++ gw8  reg=f0026029 val=00
[  126.066573] ++ETH++ gw8  reg=f002602a val=5e
[  126.070868] ++ETH++ gw8  reg=f002602b val=00
[  126.075162] ++ETH++ gw8  reg=f002602c val=00
[  126.079457] ++ETH++ gw8  reg=f002602d val=01
[  126.083751] ENTER nb8800_mc_init
[  126.086997] ++ETH++ gw8  reg=f002602e val=ff
[  129.426741] ENTER nb8800_link_reconfigure
[  129.430800] PRIV link=0 speed=0 duplex=0
[  129.434753] PHYDEV link=1 speed=1000 duplex=1
[  129.439141] ENTER nb8800_mac_config
[  129.442662] ++ETH++ gw8  reg=f0026050 val=01
[  129.446962] ++ETH++ gw8  reg=f002601c val=ff
[  129.451262] ++ETH++ gw8  reg=f0026044 val=81
[  129.455563] ENTER nb8800_pause_config
[  129.459252] ++ETH++ gw8  reg=f0026004 val=2b
[  129.463558] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow 
control rx/tx
[  129.471355] Atheros 8035 ethernet 26000.nb8800-mii:04: PHY state change AN 
-> RUNNING


# ip link set eth0 down
[  170.933454] ENTER nb8800_set_rx_mode
[  170.937067] ENTER nb8800_mc_init
[  170.940318] ++ETH++ gw8  reg=f002602e val=00
[  170.944613] ++ETH++ gw8  reg=f0026028 val=01
[  170.948907] ++ETH++ gw8  reg=f0026029 val=00
[  170.953199] ++ETH++ gw8  reg=f002602a val=5e
[  170.957494] ++ETH++ gw8  reg=f002602b val=00
[  170.961786] ++ETH++ gw8  reg=f002602c val=00
[  170.966079] ++ETH++ gw8  reg=f002602d val=01
[  170.970371] ENTER nb8800_mc_init
[  170.973616] ++ETH++ gw8  reg=f002602e val=ff
[  170.977962] ENTER nb8800_stop
[  170.981210] [ cut here ]
[  170.985860] WARNING: CPU: 0 PID: 964 at kernel/irq/manage.c:498 
__enable_irq+0x44/0x68
[  170.993815] Unbalanced enable for IRQ 30
[  170.997751] Modules linked in:
[  171.000821] CPU: 0 PID: 964 Comm: ip Not tainted 4.13.0-rc1 #154
[  171.006854] Hardware name: Sigma Tango DT
[  171.010896] [] (unwind_backtrace) from [] 
(show_stack+0x10/0x14)
[  171.018686] [] (show_stack) from [] 
(dump_stack+0x78/0x8c)
[  171.025950] [] (dump_stack) from [] (__warn+0xe8/0x100)
[  171.032948] [] (__warn) from [] 
(warn_slowpath_fmt+0x38/0x48)
[  171.040471] [] (warn_slowpath_fmt) from [] 
(__enable_irq+0x44/0x68)
[  171.048519] [] (__enable_irq) from [] 
(enable_irq+0x34/0x6c)
[  171.055956] [] (enable_irq) from [] 
(phy_change+0x98/0x13c)  

Re: Problem with PHY state machine when using interrupts

[Mason]

On 25/07/2017 00:36, Florian Fainelli wrote:
> On 07/24/2017 02:20 PM, Mason wrote:
>> On 24/07/2017 21:53, Florian Fainelli wrote:
>>
>>> Well now that I see the possible interrupts generated, I indeed don't
>>> see how you can get a link down notification unless you somehow force
>>> the link down yourself, which would certainly happen in phy_suspend()
>>> when we set BMCR.pwrdwn, but that may be too late.
>>>
>>> You should still expect the adjust_link() function to be called though
>>> with PHY_HALTED being set and that takes care of doing phydev->link = 0
>>> and netif_carrier_off(). If that still does not work, then see whether
>>> removing the call to phy_stop() does help (it really should).
>>
>> The only functions setting phydev->state to PHY_HALTED
>> are phy_error() and phy_stop() AFAICT.
>>
>> I am aware that when phy_state_machine() handles the
>> PHY_HALTED state, it will set phydev->link = 0;
>> and call netif_carrier_off() -- because that's where
>> I copied that code from.
>>
>> My issue is that phy_state_machine() does not run when
>> I run 'ip set link dev eth0 down' from the command line.
> 
> Yes, that much is clear, which is why I suggested earlier you try the
> patch at the end now.
> 
>>
>> If I'm reading the code right, phy_disconnect() actually
>> stops the state machine.
>>
>> In interrupt mode, phy_state_machine() doesn't run
>> because no interrupt is generated.
>>
>> In polling mode, phy_state_machine() doesn't run
>> because phy_disconnect() stops the state machine.
>>
>> Introducing a sleep before phy_disconnect() gives
>> the state machine a chance to run in polling mode,
>> but it doesn't feel right, and doesn't fix the
>> other mode, which I'm using.
> 
> There are several problems it seems:
> 
> - the PHY state machine cannot move solely based on the PHY generating
> interrupts during phy_stop() if none of the changing conditions for the
> HW have changed, come to think about it, I doubt any PHY would be
> capable of doing something like that
> 
> - there is an expectation from your driver to have adjust_link() run
> sometime during ndo_stop() to do something, but why?
> 
> What is special about nb8800 that interrupts should be generated during
> ndo_stop(), and why do you think this is going to solve your problem?
> 
>>
>> Looking at bcm_enet_stop() it calls phy_stop() and
>> phy_disconnect() just like the nb8800 driver...
>>
>> I'm stumped.
> 
> My suggestion of not using phy_stop() was not a good one, but
> functionally there is little difference in doing phy_stop() +
> phy_disconnect() or just phy_disconnect() alone. What matters is that we
> restart the PHY properly when phy_connect() or phy_start() is called.
> 
> What I understand now from your "bug report" is that you want
> adjust_link to run with phydev->link = 0 to do something during
> ndo_close() but you have not explained why, but I suspect such that upon
> ndo_open() things work again.
> 
> What I suspect your bug is, is that the really was no change in link
> status, no interrupt was generated because there should not be one, yet
> some of what nb8800_stop() does is not properly balanced by
> nb8800_open(). How about the following patch:
> 
> diff --git a/drivers/net/ethernet/aurora/nb8800.c
> b/drivers/net/ethernet/aurora/nb8800.c
> index 041cfb7952f8..b07dea3ab019 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -972,6 +972,10 @@ static int nb8800_open(struct net_device *dev)
> nb8800_mac_rx(dev, true);
> nb8800_mac_tx(dev, true);
> 
> +   priv->speed = -1;
> +   priv->link = -1;
> +   priv->duplex = -1;
> +
> phydev = of_phy_connect(dev, priv->phy_node,
> nb8800_link_reconfigure, 0,
> priv->phy_mode);
> 

I will test your two patches ASAP.

For the record, I have two (maybe related) issues:

1) After a sequence of
ip set link dev eth0 up
ip set link dev eth0 down
ip set link dev eth0 up
RX engine is hosed, thus network connectivity is borked.
The work-around is resetting the MAC in ndo_open
=> mac_init in my proposed patch.
This is by far the biggest issue.
Also, resetting the MAC in ndo_open will make it easy
to implement suspend/resume, as I can just ndo_stop
in suspend, and ndo_open in resume.

2) The system does not print "Link down" when I run
'ip set link dev eth0 down'
This is a symptom of nb8800_link_reconfigure()
not being called at all (or being called
with phydev->link == priv->link when I tried
skipping phy_stop)

Again, thanks for your patches and suggestions,
which I will test in the morning.

I will also try to understand why I didn't have
these issues on the other board...

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 24/07/2017 23:49, Florian Fainelli wrote:
> On 07/24/2017 02:21 PM, Mason wrote:
>> On 20/07/2017 14:33, Mason wrote:
>>
>>> As [Florian] pointed out, the spec states that the
>>> "Data to Clock input Skew (at Receiver)"
>>> must be within [ 1.0, 2.6 ] ns.
>>>
>>> I understand that 2 ns is 1/4 of a 125 MHz period,
>>> but it's not clear to me why the above interval is
>>> centered at 1.8 instead of 2.0 ns.
>>>
>>> Also, the AR8035 PHY offers 4 possible TX clock delays:
>>> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
>>> The two extremes are outside the interval, when would
>>> they be useful? In case the transmitter adds "bad" skew?
>>>
>>> Why doesn't the PHY support 1.8/2.0? Is it perhaps
>>> unable to, because of PLL limitations?
>>
>> I haven't yet found answers for these questions.
>>
>> - Why is the interval centered at 1.8 instead of 2.0 ns?
> 
> Presumably because this is almost the middle of the available range and
> it still provides a value that is within the specification...

I was talking about the RGMII spec.
If - theoretically - the best results are achieved
by having a 2 ns skew between clock and data,
it seems odd for the RGMII spec to define an
interval of [ 1.0, 2.6 ] ns for acceptable values.
I would have expected [ 1.2, 2.8 ] ns.

>> - What use are 0.25 ns and 3.4 ns skew?
> 
> Accounting for extreme PCB traces lengths possibly, or just exposing the
> raw values that the HW supports by increments of 0.25 ns, just because
> the HW supports it.

The AR8035 doesn't support increments of 0.25 ns,
it supports just 4 values: 0.25, 1.3, 2.4, 3.4
Two of which are outside the acceptable range
defined in the RGMII spec. Odd.
Giving it more thought, I don't think trace length
factors in, unless the data and clock lines have
very different length (signal propagation).

>> - Why doesn't the PHY support a "recommended" value like 1.8 ns?
>>
>> Does anyone have pointers to good resources?
> 
> The PHY datasheet and the RGMII specification really ought to be the
> starting points, there is not much more to it. Maybe go ask your support
> person at Qualcomm/Atheros about their PHY design?

Sadly, I rarely have access to support for the blocks
we use. I had to download the datasheet off the internet.
But I was only asking out of personal curiosity, since
this is outside my field. I don't think any customer
has complained about the default settings.

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 20/07/2017 14:33, Mason wrote:

> As [Florian] pointed out, the spec states that the
> "Data to Clock input Skew (at Receiver)"
> must be within [ 1.0, 2.6 ] ns.
> 
> I understand that 2 ns is 1/4 of a 125 MHz period,
> but it's not clear to me why the above interval is
> centered at 1.8 instead of 2.0 ns.
> 
> Also, the AR8035 PHY offers 4 possible TX clock delays:
> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
> The two extremes are outside the interval, when would
> they be useful? In case the transmitter adds "bad" skew?
> 
> Why doesn't the PHY support 1.8/2.0? Is it perhaps
> unable to, because of PLL limitations?

I haven't yet found answers for these questions.

- Why is the interval centered at 1.8 instead of 2.0 ns?
- What use are 0.25 ns and 3.4 ns skew?
- Why doesn't the PHY support a "recommended" value like 1.8 ns?

Does anyone have pointers to good resources?

Regards.

Re: Problem with PHY state machine when using interrupts

[Mason]

On 24/07/2017 21:53, Florian Fainelli wrote:

> Well now that I see the possible interrupts generated, I indeed don't
> see how you can get a link down notification unless you somehow force
> the link down yourself, which would certainly happen in phy_suspend()
> when we set BMCR.pwrdwn, but that may be too late.
> 
> You should still expect the adjust_link() function to be called though
> with PHY_HALTED being set and that takes care of doing phydev->link = 0
> and netif_carrier_off(). If that still does not work, then see whether
> removing the call to phy_stop() does help (it really should).

The only functions setting phydev->state to PHY_HALTED
are phy_error() and phy_stop() AFAICT.

I am aware that when phy_state_machine() handles the
PHY_HALTED state, it will set phydev->link = 0;
and call netif_carrier_off() -- because that's where
I copied that code from.

My issue is that phy_state_machine() does not run when
I run 'ip set link dev eth0 down' from the command line.

If I'm reading the code right, phy_disconnect() actually
stops the state machine.

In interrupt mode, phy_state_machine() doesn't run
because no interrupt is generated.

In polling mode, phy_state_machine() doesn't run
because phy_disconnect() stops the state machine.

Introducing a sleep before phy_disconnect() gives
the state machine a chance to run in polling mode,
but it doesn't feel right, and doesn't fix the
other mode, which I'm using.

Looking at bcm_enet_stop() it calls phy_stop() and
phy_disconnect() just like the nb8800 driver...

I'm stumped.

Regards.

Re: Problem with PHY state machine when using interrupts

[Mason]

On 24/07/2017 18:49, Florian Fainelli wrote:

> On 07/24/2017 08:01 AM, Mason wrote:
>
>>> When I set the link down via 'ip link set eth0 down'
>>> (as opposed to pulling the Ethernet cable) things don't happen as expected:
>>>
>>> The driver's adjust_link() callback is never called, and doesn't
>>> get a chance make some required changes. And when I set the link
>>> up again, there is no network connectivity.
>>>
>>> I get this problem only if I enable interrupts on my PHY.
>>> If I use polling, things work as expected.
>>>
>>>
>>> When I set the link down, devinet_ioctl() eventually calls
>>> ndo_set_rx_mode() and ndo_stop()
>>>
>>> In ndo_stop() the driver calls
>>> phy_stop(phydev);
>>> which disables interrupts and sets the state to HALTED.
>>>
>>> In phy_state_machine()
>>> the PHY_HALTED case does call the adjust_link() callback:
>>>
>>> if (phydev->link) {
>>> phydev->link = 0;
>>> netif_carrier_off(phydev->attached_dev);
>>> phy_adjust_link(phydev);
>>> do_suspend = true;
>>> }
>>>
>>> But it's not called when I use interrupts...
>>>
>>> Perhaps because there are no interrupts generated?
>>> Or even if there were, they have been turned off by phy_stop?
>>>
>>> Basically, it seems like when I use interrupts,
>>> the phy_state_machine() is not called on link down,
>>> which breaks the MAC driver's expectations.
>>>
>>> Am I barking up the wrong tree?
>>
>> FWIW, the patch below solves my issue.
>> Basically, we reset the MAC in open(), instead of probe().
>>
>> I also had to solve the issue of adjust_link() not being
>> called by calling it explicitly in stop() instead of
>> relying on phy_stop() to do it indirectly.
> 
> Which is of course absolutely not how it is intended to be used.
> phy_stop() does the following:
> 
> - if the PHY was already HALTED do nothing and exit
> - if it was not and an interrupt is valid for this PHY:  disable and
> clear these interrupts
> - set state to PHY_HALTED
> 
> somehow an interrupt should be generated from doing this such that
> phy_change(), invoked from phy_interrupt() should have a chance to run
> and make the PHY state machine transition properly to PHY_HALTED.

I'm totally confused. Are you saying that phy_stop itself
should trigger an interrupt, or that the process of setting
the link down should generate an interrupt *before* we reach
phy_stop?

I'm also perplex over this synchronous IRQ business.
Should I be looking for a way to trigger an IRQ in
software in the Atheros PHY?

Before I forget: there is also an issue when using the PHY
in polling mode. The ndo_stop callback runs through phy_stop
and phy_disconnect too fast for the adjust_link() callback
to be called. My patch fixed that too, by calling
nb8800_link_reconfigure() explicitly.


> So from there can you check a few things:
> 
> - is such an interrupt actually generated?
> - if you turn on dynamic debug prints for drivers/net/phy/phy.c where do
> we leave the PHY state machine and what state is it in when you call
> ifconfig up again?

The only interrupts I've ever seen the PHY generate are
on plugging/unplugging the Ethernet cable.

Looking at the driver and datasheet...
http://elixir.free-electrons.com/linux/v4.13-rc2/source/drivers/net/phy/at803x.c#L312
value |= AT803X_INTR_ENABLE_AUTONEG_ERR;
value |= AT803X_INTR_ENABLE_SPEED_CHANGED;
value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
value |= AT803X_INTR_ENABLE_LINK_FAIL;
value |= AT803X_INTR_ENABLE_LINK_SUCCESS;

And the interrupts reasons supported by the PHY are:
#define AT803X_INTR_ENABLE_AUTONEG_ERR  BIT(15)
#define AT803X_INTR_ENABLE_SPEED_CHANGEDBIT(14)
#define AT803X_INTR_ENABLE_DUPLEX_CHANGED   BIT(13)
#define AT803X_INTR_ENABLE_PAGE_RECEIVEDBIT(12)
#define AT803X_INTR_ENABLE_LINK_FAILBIT(11)
#define AT803X_INTR_ENABLE_LINK_SUCCESS BIT(10)
#define AT803X_INTR_ENABLE_WIRESPEED_DOWNGRADE  BIT(5)
#define AT803X_INTR_ENABLE_POLARITY_CHANGED BIT(1)
#define AT803X_INTR_ENABLE_WOL  BIT(0)

These all seem to be external reasons (from the peer).

I did enable debug logs in drivers/net/phy/phy.c
to trace the state machine, and it is not called
at all on set link down, so it remains in state
RUNNING (both in polling and interrupt modes).

IIRC, this used to work on the 2-core board, but breaks
on the 4-core board. Could this be some kind of race?

Regards.

Re: Problem with PHY state machine when using interrupts

[Mason]

On 24/07/2017 13:07, Mason wrote:

> When I set the link down via 'ip link set eth0 down'
> (as opposed to pulling the Ethernet cable) things don't happen as expected:
> 
> The driver's adjust_link() callback is never called, and doesn't
> get a chance make some required changes. And when I set the link
> up again, there is no network connectivity.
> 
> I get this problem only if I enable interrupts on my PHY.
> If I use polling, things work as expected.
> 
> 
> When I set the link down, devinet_ioctl() eventually calls
> ndo_set_rx_mode() and ndo_stop()
> 
> In ndo_stop() the driver calls
> phy_stop(phydev);
> which disables interrupts and sets the state to HALTED.
> 
> In phy_state_machine()
> the PHY_HALTED case does call the adjust_link() callback:
> 
>   if (phydev->link) {
>   phydev->link = 0;
>   netif_carrier_off(phydev->attached_dev);
>   phy_adjust_link(phydev);
>   do_suspend = true;
>   }
> 
> But it's not called when I use interrupts...
> 
> Perhaps because there are no interrupts generated?
> Or even if there were, they have been turned off by phy_stop?
> 
> Basically, it seems like when I use interrupts,
> the phy_state_machine() is not called on link down,
> which breaks the MAC driver's expectations.
> 
> Am I barking up the wrong tree?

FWIW, the patch below solves my issue.
Basically, we reset the MAC in open(), instead of probe().

I also had to solve the issue of adjust_link() not being
called by calling it explicitly in stop() instead of
relying on phy_stop() to do it indirectly.

With this code, I think it is easy to handle suspend/resume:
on suspend, I will stop() and on resume, I will start(),
and everything should work as expected.

I'd like to hear comments on the patch, so I can turn it
into a formal submission.

Regards.



For the record, here is the debug output printed:

# ip addr add 172.27.64.45/18 brd 172.27.127.255 dev eth0
# ip link set eth0 up
[   10.460952] ENTER nb8800_tangox_reset
[   10.464680] ++ETH++ gw8  reg=f0026424 val=00
[   10.478521] ++ETH++ gw8  reg=f0026424 val=01
[   10.482837] ++ETH++ gw16 reg=f0026420 val=0050
[   10.487325] ENTER nb8800_hw_init
[   10.490571] ++ETH++ gw8  reg=f0026000 val=1c
[   10.494878] ++ETH++ gw8  reg=f0026001 val=05
[   10.499176] ++ETH++ gw8  reg=f0026004 val=22
[   10.503481] ++ETH++ gw8  reg=f0026008 val=04
[   10.50] ++ETH++ gw8  reg=f0026014 val=0c
[   10.512082] ++ETH++ gw8  reg=f0026051 val=00
[   10.516377] ++ETH++ gw8  reg=f0026052 val=ff
[   10.520672] ++ETH++ gw8  reg=f0026054 val=40
[   10.524967] ++ETH++ gw8  reg=f0026060 val=00
[   10.529261] ++ETH++ gw8  reg=f0026061 val=c3
[   10.533555] ENTER nb8800_mc_init
[   10.536801] ++ETH++ gw8  reg=f002602e val=00
[   10.541094] ENTER nb8800_tangox_init
[   10.544690] ++ETH++ gw8  reg=f0026400 val=01
[   10.548985] ENTER nb8800_tango4_init
[   10.552580] ENTER nb8800_update_mac_addr
[   10.556523] ++ETH++ gw8  reg=f002606a val=00
[   10.560818] ++ETH++ gw8  reg=f002606b val=16
[   10.565112] ++ETH++ gw8  reg=f002606c val=e8
[   10.569407] ++ETH++ gw8  reg=f002606d val=5e
[   10.573700] ++ETH++ gw8  reg=f002606e val=65
[   10.577994] ++ETH++ gw8  reg=f002606f val=bc
[   10.582288] ++ETH++ gw8  reg=f002603c val=00
[   10.586582] ++ETH++ gw8  reg=f002603d val=16
[   10.590876] ++ETH++ gw8  reg=f002603e val=e8
[   10.595171] ++ETH++ gw8  reg=f002603f val=5e
[   10.599465] ++ETH++ gw8  reg=f0026040 val=65
[   10.603759] ++ETH++ gw8  reg=f0026041 val=bc
[   10.608051] ENTER nb8800_open
[   10.611034] ENTER nb8800_dma_init
[   10.614951] ++ETH++ gw8  reg=f0026004 val=23
[   10.619255] ++ETH++ gw8  reg=f0026000 val=1d
[   10.688912] ENTER nb8800_set_rx_mode
[   10.692515] ENTER nb8800_mc_init
[   10.695762] ++ETH++ gw8  reg=f002602e val=00
[   10.700384] PHY state change UP -> AN
[   10.704118] ENTER nb8800_set_rx_mode
[   10.707717] ENTER nb8800_mc_init
[   10.710963] ++ETH++ gw8  reg=f002602e val=00
[   10.715257] ++ETH++ gw8  reg=f0026028 val=01
[   10.719550] ++ETH++ gw8  reg=f0026029 val=00
[   10.723843] ++ETH++ gw8  reg=f002602a val=5e
[   10.728135] ++ETH++ gw8  reg=f002602b val=00
[   10.732428] ++ETH++ gw8  reg=f002602c val=00
[   10.736721] ++ETH++ gw8  reg=f002602d val=01
[   10.741013] ENTER nb8800_mc_init
[   10.744258] ++ETH++ gw8  reg=f002602e val=ff

[   14.141948] ENTER nb8800_link_reconfigure
[   14.145988] PRIV link=0 speed=0 duplex=0
[   14.150121] PHYDEV link=1 speed=1000 duplex=1
[   14.154589] ENTER nb8800_mac_config
[   14.158164] ++ETH++ gw8  reg=f0026050 val=01
[   14.162527] ++ETH++ gw8  reg=f002601c val=ff
[   14.166882] ++ETH++ gw8  reg=f0026044 val=81
[   14.171233] ENTER nb8800_pause_config
[   14.174981] ++ETH++ gw8  reg=f0026004 val=2b
[   14.179342] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow

Problem with PHY state machine when using interrupts

[Mason]

Hello,

When I set the link down via 'ip link set eth0 down'
(as opposed to pulling the Ethernet cable) things don't happen as expected:

The driver's adjust_link() callback is never called, and doesn't
get a chance make some required changes. And when I set the link
up again, there is no network connectivity.

I get this problem only if I enable interrupts on my PHY.
If I use polling, things work as expected.


When I set the link down, devinet_ioctl() eventually calls
ndo_set_rx_mode() and ndo_stop()

In ndo_stop() the driver calls
phy_stop(phydev);
which disables interrupts and sets the state to HALTED.

In phy_state_machine()
the PHY_HALTED case does call the adjust_link() callback:

if (phydev->link) {
phydev->link = 0;
netif_carrier_off(phydev->attached_dev);
phy_adjust_link(phydev);
do_suspend = true;
}

But it's not called when I use interrupts...

Perhaps because there are no interrupts generated?
Or even if there were, they have been turned off by phy_stop?

Basically, it seems like when I use interrupts,
the phy_state_machine() is not called on link down,
which breaks the MAC driver's expectations.

Am I barking up the wrong tree?

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 23:34, Florian Fainelli wrote:

> How about you start reading the RGMII specification so we can at least,
> if nothing else agree on the terminology? It's public:
> 
> http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf

Thanks for linking the spec. Having no EE training,
I am ill-equipped to interpret the timings table.

As you pointed out, the spec states that the
"Data to Clock input Skew (at Receiver)"
must be within [ 1.0, 2.6 ] ns.

I understand that 2 ns is 1/4 of a 125 MHz period,
but it's not clear to me why the above interval is
centered at 1.8 instead of 2.0 ns.

Also, the AR8035 PHY offers 4 possible TX clock delays:
{ 0.25, 1.3, 2.4, 3.4 } according to their doc.
The two extremes are outside the interval, when would
they be useful? In case the transmitter adds "bad" skew?

Why doesn't the PHY support 1.8/2.0? Is it perhaps
unable to, because of PLL limitations?

It's also not clear to me if wire length has any
influence on the required skew. I would say "no".
I think signal propagation time has nothing to do
with clock skew (as long as both wires are roughly
the same length).

> Some Ethernet controllers let you change it, some don't, if nb8800
> allows it, it's good for testing in that it packs more frames per
> quantum of time. If not, do you have at least a FCS error counter?

I'll have a closer look, and test with iPerf3.
Or is there a better benchmark? I will look for
an inter-packet gap knob and FCS error counter.

> I completely understand what you want to solve but I suspect you will
> have to do it in a way where you either accept that you may not be fully
> compliant with the now clarified "phy-mode" description, in order not to
> break other people's set up that were already non-compliant (can't blame
> them, they did not know back then), or you will have to use additional
> MAC properties to override the delay settings on the MAC or PHY side.

I think I need to give up the notion of "fixing"
the at803x driver. Some boards rely on the fact
that RX clock delay is enabled by default, like
am335x-evm using "rgmii-txid" instead of "rgmii-id".

My board needs to enable both internal delays,
so I don't need the PHY patch. I will only fix
the MAC driver and the DTS.

Regards.

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Mason]

On 19/07/2017 21:30, Florian Fainelli wrote:
> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>> Hi
>>
>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>> The current code supports enabling RGMII RX and TX clock delays.
>>> The unstated assumption is that these settings are disabled by
>>> default at reset, which is not the case.
>>>
>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>> it will remain enabled at reset in Linux.
>>>
>>> Provide disable functions to configure the RGMII clock delays
>>> exactly as specified in the fwspec.
>>>
>>> Signed-off-by: Marc Gonzalez 
>>> ---
>>>   drivers/net/phy/at803x.c | 32 
>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>> This patch breaks am335x-evm networking.
>>
>> To restore it I've had to apply below diff:
>> diff --git a/arch/arm/boot/dts/am335x-evm.dts 
>> b/arch/arm/boot/dts/am335x-evm.dts
>> index 200d6ab..9578bdf 100644
>> --- a/arch/arm/boot/dts/am335x-evm.dts
>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>> @@ -724,12 +724,12 @@
>>  
>>  _emac0 {
>> phy_id = <_mdio>, <0>;
>> -   phy-mode = "rgmii-txid";
>> +   phy-mode = "rgmii-id";
>>  };
>>  
>>  _emac1 {
>> phy_id = <_mdio>, <1>;
>> -   phy-mode = "rgmii-txid";
>> +   phy-mode = "rgmii-id";
>>  };
>>  
>>   {
>>
>> Sry, can't comment here to much - not E-PHY expert.
> 
> It's useful feedback, since we had poorly defined "phy-mode" semantics
> for too long, this is totally expected, Marc this is exactly why Mans is
> suggesting additional MAC-specific properties to define delays.

In the current situation, it is impossible to configure
the at803x to disable RX clock delay or TX clock delay
(in case the boot loader enabled it).

Are you saying that, because no one has had a problem
so far, it is not possible to fix it now, as it would
break boards like am335x-evm.dts which didn't request
RX clock delay, but got one anyway?

Does that mean we cannot support boards using AR8035
that need the RX and TX clock delays disabled?

I'm not sure how the MAC-specific properties can save
the day?

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 20:30, Florian Fainelli wrote:
> On 07/19/2017 10:36 AM, Mason wrote:
>> On 19/07/2017 19:17, Måns Rullgård wrote:
>>
>>> Marc Gonzalez 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 don't think so, and here's my reasoning:
>>
>> AFAIU, the HW block always requires a TX clock delay
>> (I don't know what the "safe" interval is. PHY adds
>> 2.4 ns, MAC adds ~1 ns, both work.)
> 
> The nominal delay should be 2ns because that's exactly what a 90 degrees
> shift at a 125Mhz would be. The RGMII specification defines the following:
> 
> TskewT - Data to Clock output Skew (At Transmitter) Min: -500ns, Nom: 0,
> Max: + 500 ns
> TskewR - Data to Clock input Skew (At Receiver) Min: 1ns, Nom: 0, Max:
> 2.6ns (see note 1)
> 
> note 1: This implies that PC board design will require clocks to be
> routed such that an additional trace delay of greater than 1.5ns and
> less than 2.0ns will be added to the associated clock signal. For 10/100
> the Max value is unspecified.
> 
> So it seems to me like you are borderline spec in both delays you gave
> here and the "HW block always requires a TX clock delay" statement is
> true for a given board design only.

I must confess that my understanding of clock delays,
clock skew, routing, traces, etc is nil.

Is TskewT the TX clock delay?
And TskewR the RX clock delay?

Doesn't wire delay factor in too?
(So longer wires require more delay.)

>> RX clock delay seems to be "Don't Care" (tested both
>> enabled and disabled by PHY)
>> By "tested", I mean ability to ping remote system.
> 
> Can you do something a bit more stressful than just a ping, also if you
> have the ability to change the inter-packet gap, do it, and see if you
> start seeing FCS or any other decoding errors.

Errr... "Inter-packet gap"?
Is there supposed to be a HW knob to tweak how long
the HW waits between sending two frames?

>> If phy-mode is RGMII or RGMII_RXID, then don't add
>> TX clock delay from PHY, therefore add it from MAC.
>>
>> If phy_mode is RGMII_ID or RGMII_TXID, then do add
>> TX clock delay from PHY, therefore don't add it from MAC.
>>
>> What set of circumstances would create an issue?
> 
> Existing Device Tree sources that do not correspond to that description
> you just did, I suppose they are all out of tree?

The problem with PHY drivers is that there is no
simple compatible string to grep for.

The tango boards use "ethernet-phy-id004d.d072"
but not a single other DT uses that string.
For example, am335x-evm.dts doesn't seem to name the PHY.
Hmmm, how does the at803x probe function match for that
board?

How does one estimate the impact of driver changes in
the eth PHY layer?

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 19:17, Måns Rullgård wrote:

> Marc Gonzalez 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 
>> ---
>>  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 don't think so, and here's my reasoning:

AFAIU, the HW block always requires a TX clock delay
(I don't know what the "safe" interval is. PHY adds
2.4 ns, MAC adds ~1 ns, both work.)
RX clock delay seems to be "Don't Care" (tested both
enabled and disabled by PHY)
By "tested", I mean ability to ping remote system.

If phy-mode is RGMII or RGMII_RXID, then don't add
TX clock delay from PHY, therefore add it from MAC.

If phy_mode is RGMII_ID or RGMII_TXID, then do add
TX clock delay from PHY, therefore don't add it from MAC.

What set of circumstances would create an issue?

Regards.

Re: [PATCH 2/3] net: ethernet: bgmac: Make IDM register space optional

[Jon Mason]

On Mon, Jul 17, 2017 at 2:37 PM, Abhishek Shah
 wrote:
> Hi Jon,
>
>> This also will need to be added to
>> drivers/net/ethernet/broadcom/bgmac-bcma.c.  Otherwise, the driver
>> will no longer work for those platforms.  Since this was already
>> accepted, please push out a fix ASAP.
>>
> I do not see how the bcma driver [bgmac-bcma.c] will no longer work.
> The whole idea behind introducing this  BGMAC_FEAT_IDM_MASK bit
> was to keep bcma driver's functionality intact.
>
> The BGMAC_FEAT_IDM_MASK bit is explicitly set by the bgmac-platform driver
> and is unset inside the same if the idm register base is provided.
>
> For BCMA driver case, I do not touch BGMAC_FEAT_IDM_MASK bit, i.e. it
> is *not* set.
> And if it is not set, each idm operation done in bgmac.c will take place.
> Please explain if I am missing something here.

Sorry, I had it flipped in my mind.  Please disregard.

>
>
> Regards,
> Abhishek

Re: [PATCH 2/3] net: ethernet: bgmac: Make IDM register space optional

[Jon Mason]

On Thu, Jul 13, 2017 at 3:04 PM, Abhishek Shah
 wrote:
> IDM operations are usually one time ops and should be done in
> firmware itself. Driver is not supposed to touch IDM registers.
>
> However, for some SoCs', driver is performing IDM read/writes.
> So this patch masks IDM operations in case firmware is taking
> care of IDM operations.
>
> Signed-off-by: Abhishek Shah 
> Reviewed-by: Oza Oza 
> Reviewed-by: Ray Jui 
> Reviewed-by: Scott Branden 
> ---
>  drivers/net/ethernet/broadcom/bgmac-platform.c | 19 ---
>  drivers/net/ethernet/broadcom/bgmac.c  | 70 
> +++---
>  drivers/net/ethernet/broadcom/bgmac.h  |  1 +
>  3 files changed, 55 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
> b/drivers/net/ethernet/broadcom/bgmac-platform.c
> index 1ca75de..d937083d 100644
> --- a/drivers/net/ethernet/broadcom/bgmac-platform.c
> +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
> @@ -55,6 +55,9 @@ static void platform_bgmac_idm_write(struct bgmac *bgmac, 
> u16 offset, u32 value)
>
>  static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
>  {
> +   if (!bgmac->plat.idm_base)
> +   return true;
> +
> if ((bgmac_idm_read(bgmac, BCMA_IOCTL) & BGMAC_CLK_EN) != 
> BGMAC_CLK_EN)
> return false;
> if (bgmac_idm_read(bgmac, BCMA_RESET_CTL) & BCMA_RESET_CTL_RESET)
> @@ -66,6 +69,9 @@ static void platform_bgmac_clk_enable(struct bgmac *bgmac, 
> u32 flags)
>  {
> u32 val;
>
> +   if (!bgmac->plat.idm_base)
> +   return;
> +
> /* The Reset Control register only contains a single bit to show if 
> the
>  * controller is currently in reset.  Do a sanity check here, just in
>  * case the bootloader happened to leave the device in reset.
> @@ -180,6 +186,7 @@ static int bgmac_probe(struct platform_device *pdev)
> bgmac->feature_flags |= BGMAC_FEAT_CMDCFG_SR_REV4;
> bgmac->feature_flags |= BGMAC_FEAT_TX_MASK_SETUP;
> bgmac->feature_flags |= BGMAC_FEAT_RX_MASK_SETUP;
> +   bgmac->feature_flags |= BGMAC_FEAT_IDM_MASK;
>
> bgmac->dev = >dev;
> bgmac->dma_dev = >dev;
> @@ -207,15 +214,13 @@ static int bgmac_probe(struct platform_device *pdev)
> return PTR_ERR(bgmac->plat.base);
>
> regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "idm_base");
> -   if (!regs) {
> -   dev_err(>dev, "Unable to obtain idm resource\n");
> -   return -EINVAL;
> +   if (regs) {
> +   bgmac->plat.idm_base = devm_ioremap_resource(>dev, 
> regs);
> +   if (IS_ERR(bgmac->plat.idm_base))
> +   return PTR_ERR(bgmac->plat.idm_base);
> +   bgmac->feature_flags &= ~BGMAC_FEAT_IDM_MASK;
> }
>
> -   bgmac->plat.idm_base = devm_ioremap_resource(>dev, regs);
> -   if (IS_ERR(bgmac->plat.idm_base))
> -   return PTR_ERR(bgmac->plat.idm_base);
> -
> regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, 
> "nicpm_base");
> if (regs) {
> bgmac->plat.nicpm_base = devm_ioremap_resource(>dev,
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
> b/drivers/net/ethernet/broadcom/bgmac.c
> index ba4d2e1..48d672b 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -622,9 +622,11 @@ static int bgmac_dma_alloc(struct bgmac *bgmac)
> BUILD_BUG_ON(BGMAC_MAX_TX_RINGS > ARRAY_SIZE(ring_base));
> BUILD_BUG_ON(BGMAC_MAX_RX_RINGS > ARRAY_SIZE(ring_base));
>
> -   if (!(bgmac_idm_read(bgmac, BCMA_IOST) & BCMA_IOST_DMA64)) {
> -   dev_err(bgmac->dev, "Core does not report 64-bit DMA\n");
> -   return -ENOTSUPP;
> +   if (!(bgmac->feature_flags & BGMAC_FEAT_IDM_MASK)) {
> +   if (!(bgmac_idm_read(bgmac, BCMA_IOST) & BCMA_IOST_DMA64)) {
> +   dev_err(bgmac->dev, "Core does not report 64-bit 
> DMA\n");
> +   return -ENOTSUPP;
> +   }
> }
>
> for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
> @@ -855,9 +857,11 @@ static void bgmac_mac_speed(struct bgmac *bgmac)
>  static void bgmac_miiconfig(struct bgmac *bgmac)
>  {
> if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500) {
> -   bgmac_idm_write(bgmac, BCMA_IOCTL,
> -   bgmac_idm_read(bgmac, BCMA_IOCTL) | 0x40 |
> -   BGMAC_BCMA_IOCTL_SW_CLKEN);
> +   if (!(bgmac->feature_flags & BGMAC_FEAT_IDM_MASK)) {
> +   bgmac_idm_write(bgmac, BCMA_IOCTL,
> +   bgmac_idm_read(bgmac, BCMA_IOCTL) |
> +   0x40 | BGMAC_BCMA_IOCTL_SW_CLKEN);
> +   }
>

Re: Quirks of the Atheros 8035 PHY

[Mason]

On 14/07/2017 23:28, Florian Fainelli wrote:

> On 07/14/2017 02:08 PM, Mason wrote:
>
>> I've discussed this subject in the past, but we never reached
>> a conclusion, AFAIR.
>>
>> The Atheros 8035 GigE PHY has IMO 2 quirks wrt to clock delays.
>>
>> https://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
>>
>>
>> 1) RX clock delay
>>
>> Commit 2e5f9f281ee8369f56d403b4a52942f19b6978f8
>>
>> In fact, RX clock delay is *ENABLED* by default at
>> reset. So if a board actually required it *disabled*
>> then we actually need to set the bit to 0.
>>
>> Reference:
>> 4.2.25 rgmii rx clock delay control
>>
>>
>> 2) TX clock delay
>>
>> Commit 1ca6d1b1aef4628ff0fe458135ddb008d134ad8f
>>
>> TX clock delay is DISABLED by default, so no surprises
>> there... except that it is only DISABLED after *HW reset*.
>>
>> After a SW reset, such as that performed in Linux IIUC,
>> the PHY retains the value previously set.
>>
>> So if a bootloader such a Uboot enabled TX delay, then
>> Linux would "inherit" the setting, even if it performs
>> a reset. If the PHY setting calls for no TX clock delay,
>> the Linux driver would have to actively disable it.
>>
>> Reference:
>> 4.2.26 rgmii tx clock delay control
>>
>>
>> I can (of course) send a patch fixing both issues, but
>> what was said last time was that "it's too late to
>> fix it now, since the fix risks breaking working
>> setups". Is that an accurate paraphrase?
> 
> More or less, this particular problematic PHY has come up with some many
> different platforms, and people and typically no one being able to have
> insights on its internal design that it is really hard to comment on
> what would break, it's already apparently pretty broken.

When you say broken, do you mean the situation,
or the HW? I don't think the HW is broken.

The API is confusing, and violates the principle of
least astonishment, which led several SW devs to make
implementation errors, but the documentation is
pretty clear and well-written, as far as docs go.

The fix is straight-forward:

if (PHY_INTERFACE_MODE_RGMII)
disable_rx_delay;
disable_tx_delay;

if (PHY_INTERFACE_MODE_RGMII_RXID)
enable_rx_delay;
disable_tx_delay;

if (PHY_INTERFACE_MODE_RGMII_TXID)
disable_rx_delay;
enable_tx_delay;

if (PHY_INTERFACE_MODE_RGMII_ID)
enable_rx_delay;
enable_tx_delay;

Then the setting will be as requested in the DT.

>> 3) There's also a RGMII GTX_CLK documented as
>> "RGMII transmit clock, 125 MHz digital. Adding a 22 ohm
>> damping  resistor is recommended for EMI design near MAC side"
>> => Is that TX_CLK?
>> There's a 2-bit related field called Gtx_dly_val
>> which allows tweaking the delay
>>
>> Select the delay of gtx_clk.
>> 00: 0.25ns
>> 01: 1.3ns
>> 10: 2.4ns
>> 11: 3.4ns
>> (DEFAULT 10b = 2.4 ns, BUT Retain val on SW reset,
>> so inherit bootloader value)
>> I'm not sure the current DT allows such fine-grained
>> tweaking? Should we enhance it?
> 
> What is "the current DT" in that context? There is no binding for
> at8033x defined and there is not one either for nb8800.

I meant
Documentation/devicetree/bindings/net/ethernet.txt
Documentation/devicetree/bindings/net/phy.txt

There is no prop to define the required delay,
in case the specific PHY supports multiple
delays, as the AR8035 seems to do.

This is the current DT
http://elixir.free-electrons.com/linux/latest/source/arch/arm/boot/dts/tango4-vantage-1172.dts

 {
phy-connection-type = "rgmii";
phy-handle = <_phy>;
#address-cells = <1>;
#size-cells = <0>;

/* Atheros AR8035 */
eth0_phy: ethernet-phy@4 {
compatible = "ethernet-phy-id004d.d072",
 "ethernet-phy-ieee802.3-c22";
interrupts = <37 IRQ_TYPE_EDGE_RISING>;
reg = <4>;
};
};

phy-connection-type is wrong, since I need both delays.
So I will change to rgmii-id, and fix the MAC driver to
honor the doc blurb:

  * "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)

> Some bindings do define RGM

Re: Quirks of the Atheros 8035 PHY

[Mason]

Mugunthan's address bounces. Removing it.
Adding Grygorii's address instead.

On 14/07/2017 23:08, Mason wrote:

> Hello,
> 
> I've discussed this subject in the past, but we never reached
> a conclusion, AFAIR.
> 
> The Atheros 8035 GigE PHY has IMO 2 quirks wrt to clock delays.
> 
> https://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
> 
> 
> 1) RX clock delay
> 
> Commit 2e5f9f281ee8369f56d403b4a52942f19b6978f8
> 
> In fact, RX clock delay is *ENABLED* by default at
> reset. So if a board actually required it *disabled*
> then we actually need to set the bit to 0.
> 
> Reference:
> 4.2.25 rgmii rx clock delay control
> 
> 
> 2) TX clock delay
> 
> Commit 1ca6d1b1aef4628ff0fe458135ddb008d134ad8f
> 
> TX clock delay is DISABLED by default, so no surprises
> there... except that it is only DISABLED after *HW reset*.
> 
> After a SW reset, such as that performed in Linux IIUC,
> the PHY retains the value previously set.
> 
> So if a bootloader such a Uboot enabled TX delay, then
> Linux would "inherit" the setting, even if it performs
> a reset. If the PHY setting calls for no TX clock delay,
> the Linux driver would have to actively disable it.
> 
> Reference:
> 4.2.26 rgmii tx clock delay control
> 
> 
> I can (of course) send a patch fixing both issues, but
> what was said last time was that "it's too late to
> fix it now, since the fix risks breaking working
> setups". Is that an accurate paraphrase?
> 
> 
> 3) There's also a RGMII GTX_CLK documented as
> "RGMII transmit clock, 125 MHz digital. Adding a 22 ohm
> damping  resistor is recommended for EMI design near MAC side"
> => Is that TX_CLK?
> There's a 2-bit related field called Gtx_dly_val
> which allows tweaking the delay
> 
> Select the delay of gtx_clk.
> 00: 0.25ns
> 01: 1.3ns
> 10: 2.4ns
> 11: 3.4ns
> (DEFAULT 10b = 2.4 ns, BUT Retain val on SW reset,
> so inherit bootloader value)
> I'm not sure the current DT allows such fine-grained
> tweaking? Should we enhance it?
> 
> 
> 4) There's the whole mess of having clock delays
> supported both by the PHY *AND* the MAC. If both
> add a delay, things won't work as expected.
> What's the recommended approach there?
> 
> 
> Regards.

Quirks of the Atheros 8035 PHY

[Mason]

Hello,

I've discussed this subject in the past, but we never reached
a conclusion, AFAIR.

The Atheros 8035 GigE PHY has IMO 2 quirks wrt to clock delays.

https://www.redeszone.net/app/uploads/2014/04/AR8035.pdf


1) RX clock delay

Commit 2e5f9f281ee8369f56d403b4a52942f19b6978f8

In fact, RX clock delay is *ENABLED* by default at
reset. So if a board actually required it *disabled*
then we actually need to set the bit to 0.

Reference:
4.2.25 rgmii rx clock delay control


2) TX clock delay

Commit 1ca6d1b1aef4628ff0fe458135ddb008d134ad8f

TX clock delay is DISABLED by default, so no surprises
there... except that it is only DISABLED after *HW reset*.

After a SW reset, such as that performed in Linux IIUC,
the PHY retains the value previously set.

So if a bootloader such a Uboot enabled TX delay, then
Linux would "inherit" the setting, even if it performs
a reset. If the PHY setting calls for no TX clock delay,
the Linux driver would have to actively disable it.

Reference:
4.2.26 rgmii tx clock delay control


I can (of course) send a patch fixing both issues, but
what was said last time was that "it's too late to
fix it now, since the fix risks breaking working
setups". Is that an accurate paraphrase?


3) There's also a RGMII GTX_CLK documented as
"RGMII transmit clock, 125 MHz digital. Adding a 22 ohm
damping  resistor is recommended for EMI design near MAC side"
=> Is that TX_CLK?
There's a 2-bit related field called Gtx_dly_val
which allows tweaking the delay

Select the delay of gtx_clk.
00: 0.25ns
01: 1.3ns
10: 2.4ns
11: 3.4ns
(DEFAULT 10b = 2.4 ns, BUT Retain val on SW reset,
so inherit bootloader value)
I'm not sure the current DT allows such fine-grained
tweaking? Should we enhance it?


4) There's the whole mess of having clock delays
supported both by the PHY *AND* the MAC. If both
add a delay, things won't work as expected.
What's the recommended approach there?


Regards.

Re: [PATCH net] mdio: mux: fix parsing mux registers outside of the PHY address range

[Jon Mason]

On Mon, Jul 10, 2017 at 8:56 AM, Andrew Lunn  wrote:
> On Mon, Jul 10, 2017 at 02:35:23PM +0200, Martin Blumenstingl wrote:
>> mdio_mux_init parses the child nodes of the MDIO mux. When using
>> "mdio-mux-mmioreg" the child nodes are describing the register value
>> that is written to switch between the MDIO busses.
>>
>> The change which makes the error messages more verbose changed the
>> parsing of the "reg" property from a simple of_property_read_u32 call
>> to of_mdio_parse_addr. On a Khadas VIM (based on the Meson GXL SoC,
>> which uses mdio-mux-mmioreg) this prevents registering the MDIO mux
>> (because the "reg" values on the MDIO mux child nodes are 0x2009087f
>> and 0xe40908ff) and leads to the following errors:
>>   mdio-mux-mmioreg c883455c.eth-phy-mux: 
>> /soc/periphs@c8834000/eth-phy-mux/mdio@e40908ff PHY address -469169921 is 
>> too large
>>   mdio-mux-mmioreg c883455c.eth-phy-mux: Error: Failed to find reg for child 
>> /soc/periphs@c8834000/eth-phy-mux/mdio@e40908ff
>>   mdio-mux-mmioreg c883455c.eth-phy-mux: 
>> /soc/periphs@c8834000/eth-phy-mux/mdio@2009087f PHY address 537462911 is too 
>> large
>>   mdio-mux-mmioreg c883455c.eth-phy-mux: Error: Failed to find reg for child 
>> /soc/periphs@c8834000/eth-phy-mux/mdio@2009087f
>>   mdio-mux-mmioreg c883455c.eth-phy-mux: Error: No acceptable child buses 
>> found
>>   mdio-mux-mmioreg c883455c.eth-phy-mux: failed to register mdio-mux bus 
>> /soc/periphs@c8834000/eth-phy-mux
>> (as a result of that ethernet is not working, because the PHY which is
>> connected through the mux' child MDIO bus, which is not being
>> registered).
>>
>> Fix this by reverting the change from of_mdio_parse_addr to
>> of_mdio_parse_addr.
>
> Reviewed-by: Andrew Lunn 
>
> Yes, validating the reg property needs to be done separately in each
> user of the generic mdio-mix code. The reg for the gpio mux must be <=
> number of gpios, mmioreg must be somewhere within the address space,
> bcm-iproc < 1024?
>
> Jon, please feel free to add such code.

To be clear, are you suggesting that we add an additional property to
of_mdio_parse_addr() that specifies the limit to check against, or
remove the check and add it to each additional caller?

Thanks,
Jon

>
> Andrew

Re: Toggling link state breaks network connectivity

[Mason]

On 15/06/2017 12:19, Måns Rullgård wrote:

> Now I did notice one thing.  When the interrupts from the loopback
> frames are handled, the rx interrupt is all but disabled for NAPI poll
> mode.  Of course, NAPI isn't active, so the rx interrupt never gets
> re-enabled.  We should probably do this in ndo_open just to be sure.

I have tried everything I could think of (whether reasonable,
or just random). Setting the link down always hoses the board's
network connectivity, 100% reproducible on Vantage 1218.

The single HW difference that I'm aware of is that the Atheros
PHY used to be clocked at 25 MHz (I think) on 1172 and 1191
boards, but it is clocked at 100 MHz on this 1218 board. 
I'll try tweaking this setting.

For the time being, my test script is:

ip addr add 172.27.64.77/18 brd 172.27.127.255 dev eth0
ip link set eth0 up
sleep 6  ## autoneg should be complete
ntpdate 172.27.64.1
sleep 1
date +%T && ping -c 3 172.27.64.1 > /tmp/ping_before
sleep 1
ip link set eth0 down
sleep 2
ip link set eth0 up
sleep 5
date +%T && ping -c 3 172.27.64.1 > /tmp/ping_after

# cat /tmp/ping_before

PING 172.27.64.1 (172.27.64.1): 56 data bytes
64 bytes from 172.27.64.1: seq=0 ttl=64 time=31.442 ms
64 bytes from 172.27.64.1: seq=1 ttl=64 time=31.359 ms
64 bytes from 172.27.64.1: seq=2 ttl=64 time=31.379 ms

--- 172.27.64.1 ping statistics ---
3 packets transmitted, 3 packets received, 0% packet loss
round-trip min/avg/max = 31.359/31.393/31.442 ms

=> These RTT numbers are crazy high for a LAN connection
through a switch. (I get 0.250 ms from my desktop.)

=> What the server saw was:
15:23:30.400066 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41987, seq 
0, length 64
15:23:30.400094 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41987, seq 
0, length 64
15:23:31.431479 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41987, seq 
1, length 64
15:23:31.431500 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41987, seq 
1, length 64
15:23:32.462844 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41987, seq 
2, length 64
15:23:32.462867 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41987, seq 
2, length 64


# cat /tmp/ping_after  
PING 172.27.64.1 (172.27.64.1): 56 data bytes

--- 172.27.64.1 ping statistics ---
3 packets transmitted, 0 packets received, 100% packet loss

=> What the server saw was:
15:23:41.180068 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
15:23:41.180081 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
15:23:42.194409 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
15:23:42.194419 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
15:23:43.207700 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
15:23:43.207710 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28



Below are the "raw" logs (of what I thought might be useful)

Regards.


[0.898538] libphy: Fixed MDIO Bus: probed
[0.902734] ENTER nb8800_probe
[0.905863] ++ETH++ gw8  0xf0026424 0x00
[0.919601] ++ETH++ gw8  0xf0026424 0x01
[0.923550] ++ETH++ gw16 0xf0026420 0x0050
[0.927749] libphy: nb8800-mii: probed
[0.933736] ENTER nb8800_hw_init
[0.936991] ++ETH++ gw8  0xf0026000 0x1c
[0.940952] ++ETH++ gw8  0xf0026001 0x05
[0.944897] ++ETH++ gw8  0xf0026004 0x22
[0.948839] ++ETH++ gw8  0xf0026008 0x04
[0.952792] ++ETH++ gw8  0xf0026014 0x0c
[0.956740] ++ETH++ gw8  0xf0026051 0x00
[0.960693] ++ETH++ gw8  0xf0026052 0xff
[0.964637] ++ETH++ gw8  0xf0026054 0x40
[0.968579] ++ETH++ gw32 0xf0026100 0x02fe
[0.973058] ++ETH++ gw32 0xf0026118 0x003cc4a4
[0.977527] ++ETH++ gw32 0xf0026200 0x02fe
[0.981994] ++ETH++ gw32 0xf0026218 0x4dc8
[0.986473] ++ETH++ gw8  0xf002602e 0x00
[0.990417] EXIT nb8800_hw_init
[0.993576] ++ETH++ gw8  0xf0026400 0x01
[0.997518] ++ETH++ gw32 0xf0026200 0x028e
[1.001988] ++ETH++ gw8  0xf002606a 0x00
[1.005930] ++ETH++ gw8  0xf002606b 0x16
[1.009873] ++ETH++ gw8  0xf002606c 0xe8
[1.013815] ++ETH++ gw8  0xf002606d 0x4d
[1.017757] ++ETH++ gw8  0xf002606e 0x7f
[1.021699] ++ETH++ gw8  0xf002606f 0xc4
[1.025641] ++ETH++ gw8  0xf002603c 0x00
[1.029584] ++ETH++ gw8  0xf002603d 0x16
[1.033526] ++ETH++ gw8  0xf002603e 0xe8
[1.037468] ++ETH++ gw8  0xf002603f 0x4d
[1.041411] ++ETH++ gw8  0xf0026040 0x7f
[1.045354] ++ETH++ gw8  0xf0026041 0xc4
[1.049772] nb8800 26000.ethernet eth0: MAC address 00:16:e8:4d:7f:c4
[1.056259] EXIT nb8800_probe

# ip addr add 172.27.64.77/18 brd 172.27.127.255 dev eth0
# ip link set eth0 up
[   16.172093] ENTER nb8800_open
[   16.175096] ++ETH++ gw32 0xf0026204 0x000f
[   16.179574] ++ETH++ gw32 0xf0026104 0x000f
[   16.184063] ENTER nb8800_dma_init
[   16.188000] ++ETH++ gw32 0xf002620c 0x9dc4c000
[   16.192478] EXIT  nb8800_dma_init
[   16.195851] ++ETH++ gw32 0xf0026218 0x4dc8
[   16.200334] ++ETH++ gw8  0xf0026004 0x23
[   16.204280] ++ETH++ gw8  

Re: Toggling link state breaks network connectivity

[Mason]

On 13/06/2017 17:50, Florian Fainelli wrote:

> On 06/13/2017 08:47 AM, Mason wrote:
>
>> If I unplug/replug the cable, ping still works afterward.
>> (Packet RX appears to be *not* wedged)
>>
>> If I toggle the link state in SW (set link down/set link up),
>> I can no longer ping afterward.
>> (Packet RX appears to be wedged, so ARP replies are not seen)
>>
>> Maybe the two experiments are too unrelated to consider
>> the different results relevant in any way?
> 
> No, they are not, this really tells you that whatever your ndo_open()
> and ndo_stop() functions do, they are currently broken with your
> particular HW. I have seen numerous problems on some of our older
> platforms using bcmgenet where the PHY needs to be reset *before* any
> MAC activity occurs, and any MAC activity even qualifies as a reset of
> the MAC itself, we can perform a PHY reset there because the PHY is
> internal, that may not be an option, and your HW is different anyway.

I have logged *all* register writes, to compare state after probe,
after open, and after close. I do see a few odd values...
I'm still deep in the maze, but I may find an exit eventually.

What state is probe supposed to leave the HW in?
What state is open  supposed to leave the HW in?

I'm thinking that I could reset the entire block in close.

Regards.


Found ethernet phy : AR8035
[0.852746] libphy: Fixed MDIO Bus: probed
[0.856975] ++ETH++ gw8  0xf0026424 0x00 # ETH_SW_RESET
[0.870942] ++ETH++ gw8  0xf0026424 0x01 # ETH_SW_RESET
[0.874894] ++ETH++ gw16 0xf0026420 0x0050   # ETH_MDIO_CLK_DIV
[0.879101] libphy: nb8800-mii: probed
[0.885005] ++ETH++ gw8  0xf0026000 0x1c # 
ETH_MAC_CORE_TRANSMIT_CONTROL
[0.888967] ++ETH++ gw8  0xf0026001 0x05 # 
ETH_MAC_CORE_TRANSMIT_CONTROL
[0.892911] ++ETH++ gw8  0xf0026004 0x22 # 
ETH_MAC_CORE_RECEIVE_CONTROL
[0.896855] ++ETH++ gw8  0xf0026008 0x04 # 
ETH_MAC_CORE_RANDOM_SEED
[0.900815] ++ETH++ gw8  0xf0026014 0x0c # 
ETH_MAC_CORE_TRANSMIT_SINGLE_DEFERRAL
[0.904758] ++ETH++ gw8  0xf0026051 0x00 # 
ETH_MAC_CORE_THRESHOLDS
[0.908700] ++ETH++ gw8  0xf0026052 0xff # 
ETH_MAC_CORE_THRESHOLDS
[0.912643] ++ETH++ gw8  0xf0026054 0x40 # 
ETH_MAC_CORE_BUFFER_SIZE_TX_AND_ETH_MAC_CORE_FIFO_CONTROL
[0.916586] ++ETH++ gw32 0xf0026100 0x02fe   # 
TRANSMIT_CHANNEL_CONTROL
[0.921054] ++ETH++ gw32 0xf0026118 0x003cc4a4   # 
TRANSMIT_INTERRUPT_TIME
[0.925521] ++ETH++ gw32 0xf0026200 0x02fe   # 
RECEIVE_CHANNEL_CONTROL
[0.929990] ++ETH++ gw32 0xf0026218 0x4dc8   # RECEIVE_INTERRUPT_TIME
[0.934456] ++ETH++ gw8  0xf0026060 0x00 # 
ETH_MAC_CORE_PAUSE_QUANTA
[0.938398] ++ETH++ gw8  0xf0026061 0xc3 # 
ETH_MAC_CORE_PAUSE_QUANTA
[0.942338] ++ETH++ gw8  0xf002602e 0x00 # 
ETH_MAC_CORE_MULTICAST_ADDRESS1
[0.946283] ++ETH++ gw8  0xf0026400 0x01 # ETH_GPIO_PAD_MODE
[0.950226] ++ETH++ gw32 0xf0026200 0x028e   # 
RECEIVE_CHANNEL_CONTROL
[0.954695] ++ETH++ gw8  0xf002606a 0x00 # 
ETH_MAC_CORE_SOURCE_ADDRESS0
[0.958638] ++ETH++ gw8  0xf002606b 0x16 # 
ETH_MAC_CORE_SOURCE_ADDRESS0
[0.962581] ++ETH++ gw8  0xf002606c 0xe8 # 
ETH_MAC_CORE_SOURCE_ADDRESS1
[0.966523] ++ETH++ gw8  0xf002606d 0x4d # 
ETH_MAC_CORE_SOURCE_ADDRESS1
[0.970465] ++ETH++ gw8  0xf002606e 0x7f # 
ETH_MAC_CORE_SOURCE_ADDRESS1
[0.974408] ++ETH++ gw8  0xf002606f 0xc4 # 
ETH_MAC_CORE_SOURCE_ADDRESS1
[0.978349] ++ETH++ gw8  0xf002603c 0x00 # 
ETH_MAC_CORE_UNICAST_ADDRESS0
[0.982291] ++ETH++ gw8  0xf002603d 0x16 # 
ETH_MAC_CORE_UNICAST_ADDRESS0
[0.986234] ++ETH++ gw8  0xf002603e 0xe8 # 
ETH_MAC_CORE_UNICAST_ADDRESS0
[0.990176] ++ETH++ gw8  0xf002603f 0x4d # 
ETH_MAC_CORE_UNICAST_ADDRESS0
[0.994118] ++ETH++ gw8  0xf0026040 0x7f # 
ETH_MAC_CORE_UNICAST_ADDRESS1
[0.998060] ++ETH++ gw8  0xf0026041 0xc4 # 
ETH_MAC_CORE_UNICAST_ADDRESS1
[1.002435] nb8800 26000.ethernet eth0: MAC address 00:16:e8:4d:7f:c4

# ip link set eth0 up
[   33.202058] ENTER nb8800_open
[   33.205134] ++ETH++ gw32 0xf0026204 0x000f   # RECEIVE_CHANNEL_STATUS
[   33.209610] ++ETH++ gw32 0xf0026104 0x000f   # 
TRANSMIT_CHANNEL_STATUS
[   33.214626] ++ETH++ gw32 0xf002620c 0x9de36000   # 
RECEIVE_DESCRIPTOR_ADDRESS
[   33.219138] ++ETH++ gw8  0xf0026004 0x23 # 
ETH_MAC_CORE_RECEIVE_CONTROL
[   33.223097] ++ETH++ gw8  0xf0026000 0x1d # 
ETH_MAC_CORE_TRANSMIT_CONTROL
[   33.227054] nb8800_mdio_write: reg=0 val=8000# by genphy_soft_reset()
[   33.339579] nb8800_mdio_write: reg=0 val=# by at803x_resum

Re: Toggling link state breaks network connectivity

[Mason]

On 13/06/2017 17:36, Florian Fainelli wrote:

> On 06/13/2017 08:07 AM, Mason wrote:
>
>> I did note something that seems important.
>> If I toggle the link state in software, then connectivity breaks.
>> If I unplug the ethernet cable, and replug, connectivity remains.
> 
> What does that actually mean? If you disconnect the cable a link state
> should be notified and another link state should be notified, even if
> that happens faster than the PHYLIB polling time (1s).

Sorry for being unclear.

If I unplug/replug the cable, ping still works afterward.
(Packet RX appears to be *not* wedged)

If I toggle the link state in SW (set link down/set link up),
I can no longer ping afterward.
(Packet RX appears to be wedged, so ARP replies are not seen)

Maybe the two experiments are too unrelated to consider
the different results relevant in any way?

Regards.

Re: Toggling link state breaks network connectivity

[Mason]

On 13/06/2017 17:07, Mason wrote:

> I did note something that seems important.
> 
> If I toggle the link state in software, then connectivity breaks.
> 
> If I unplug the ethernet cable, and replug, connectivity remains.
> 
> The difference is that plugging/unplugging doesn't call the
> .ndo_stop callback. But 'ip link set eth0 down' does call it.
> 
> Should the .ndo_stop callback be symmetric to the .ndo_open callback?
> In other words, should .ndo_open(); .ndo_stop(); be a NOP?

I changed the ndo_open callback to a wrapper that calls:

nb8800_open(dev);
nb8800_stop(dev);
nb8800_open(dev);

With this change, connectivity is broken from the start.
Valid ARP requests are correctly *sent* but the corresponding
ARP replies are not received.

I'm hoping this limits the scope of what needs to be investigated.

Regards.

Re: Toggling link state breaks network connectivity

[Mason]

On 12/06/2017 18:38, Florian Fainelli wrote:

> On 06/12/2017 06:22 AM, Mason wrote:
>
>> I am using the following drivers for Ethernet connectivity.
>> drivers/net/ethernet/aurora/nb8800.c
>> drivers/net/phy/at803x.c
>>
>> Pulling the cable and plugging it back works as expected.
>> (I can ping both before and after.)
>>
>> However, if I toggle the link state in software (using ip link set),
>> the board loses network connectivity.
>>
>> # Statically assign IP address
>> ip addr add 172.27.64.77/18 brd 172.27.127.255 dev eth0
>> # Set link state to "up"
>> ip link set eth0 up
>> # ping -c 3 172.27.64.1 > /tmp/v1
>>
>> PING 172.27.64.1 (172.27.64.1): 56 data bytes
>> 64 bytes from 172.27.64.1: seq=0 ttl=64 time=18.321 ms
> 
> This delay seems abnormally long unless you are purposely introducing
> delay (e.g: with cls_netem) or this is a really remote host, does not
> seem to be based on your traces later on.

I think the delay is due to calling ping before the link
is actually up. For example, if I ping immediately after
setting the link up, the first 4 packets are lost.

PING 172.27.64.1 (172.27.64.1): 56 data bytes
64 bytes from 172.27.64.1: seq=4 ttl=64 time=0.235 ms
64 bytes from 172.27.64.1: seq=5 ttl=64 time=0.142 ms
64 bytes from 172.27.64.1: seq=6 ttl=64 time=0.110 ms
64 bytes from 172.27.64.1: seq=7 ttl=64 time=0.095 ms
64 bytes from 172.27.64.1: seq=8 ttl=64 time=0.139 ms
64 bytes from 172.27.64.1: seq=9 ttl=64 time=0.120 ms

--- 172.27.64.1 ping statistics ---
10 packets transmitted, 6 packets received, 40% packet loss
round-trip min/avg/max = 0.095/0.140/0.235 ms


>> So basically, the board is asking the desktop for its MAC address,
>> and the desktop is answering immediately. But the board doesn't seem
>> to be getting the replies... Any ideas, or words of wisdom, as they say?
> 
> - check the Ethernet MAC counters to see if there is packet loss, or
> error, or both
> 
> - consult with your HW engineers for possible flaws in your
> ndo_open/ndo_close paths and possible interactions with the MAC/PHY
> clocks, or reset etc.
> 
> - see if your PHY needs a complete re-init after an up/down sequence and
> if you are doing this properly

I'm using the following test script:

ip addr add 172.27.64.77/18 brd 172.27.127.255 dev eth0
ip link set eth0 up
sleep 3  ## hopefully autoneg is complete

ethtool -S eth0 > /tmp/s0
ping -c 10 172.27.64.1 > /tmp/v1
ethtool -S eth0 > /tmp/s1
diff -U0 /tmp/s0 /tmp/s1

ip link set eth0 down
sleep 1
ip link set eth0 up
sleep 1

ethtool -S eth0 > /tmp/s0
ping -c 10 172.27.64.1 > /tmp/v2
ethtool -S eth0 > /tmp/s1
diff -U0 /tmp/s0 /tmp/s1

Testing with a generic PHY driver (no Atheros 8035 support built).
Apparently, ethtool doesn't report any packet loss or error.

First time:

# diff -U0 /tmp/s0 /tmp/s1
--- /tmp/s0
+++ /tmp/s1
@@ -2,2 +2,2 @@
- rx_bytes_ok: 0
- rx_frames_ok: 0
+ rx_bytes_ok: 1084
+ rx_frames_ok: 11
@@ -6,2 +6,2 @@
- rx_64_byte_frames: 0
- rx_127_byte_frames: 0
+ rx_64_byte_frames: 1
+ rx_127_byte_frames: 10
@@ -22,6 +22,6 @@
- rx_bytes: 0
- rx_frames: 0
- tx_bytes_ok: 0
- tx_frames_ok: 0
- tx_64_byte_frames: 0
- tx_127_byte_frames: 0
+ rx_bytes: 1084
+ rx_frames: 11
+ tx_bytes_ok: 1084
+ tx_frames_ok: 11
+ tx_64_byte_frames: 1
+ tx_127_byte_frames: 10
@@ -33 +33 @@
- tx_broadcast_frames: 0
+ tx_broadcast_frames: 1
@@ -43,2 +43,2 @@
- tx_bytes: 0
- tx_frames: 0
+ tx_bytes: 1084
+ tx_frames: 11


Second time:

# diff -U0 /tmp/s0 /tmp/s1
--- /tmp/s0
+++ /tmp/s1
@@ -2,2 +2,2 @@
- rx_bytes_ok: 1276
- rx_frames_ok: 14
+ rx_bytes_ok: 1779
+ rx_frames_ok: 19
@@ -6 +6 @@
- rx_64_byte_frames: 4
+ rx_64_byte_frames: 8
@@ -8 +8 @@
- rx_255_byte_frames: 0
+ rx_255_byte_frames: 1
@@ -14 +14 @@
- rx_broadcast_frames: 0
+ rx_broadcast_frames: 1
@@ -22,5 +22,5 @@
- rx_bytes: 1276
- rx_frames: 14
- tx_bytes_ok: 1276
- tx_frames_ok: 14
- tx_64_byte_frames: 4
+ rx_bytes: 1779
+ rx_frames: 19
+ tx_bytes_ok: 1724
+ tx_frames_ok: 21
+ tx_64_byte_frames: 11
@@ -33 +33 @@
- tx_broadcast_frames: 1
+ tx_broadcast_frames: 8
@@ -43,2 +43,2 @@
- tx_bytes: 1276
- tx_frames: 14
+ tx_bytes: 1724
+ tx_frames: 21


I did note something that seems important.

If I toggle the link state in software, then connectivity breaks.

If I unplug the ethernet cable, and replug, connectivity remains.

The difference is that plugging/unplugging doesn't call the
.ndo_stop callback. But 'ip link set eth0 down' does call it.

Should the .ndo_stop callback be symmetric to the .ndo_open callback?
In other words, should .ndo_open(); .ndo_stop(); be a NOP?

Regards.

[PATCH net-next] of_mdio: move of_mdio_parse_addr to header file

[Jon Mason]

The of_mdio_parse_addr() helper function is useful to other code, but
the module dependency chain causes issues.  To work around this, we can
move of_mdio_parse_addr() to be an inline function in the header file.
This gets rid of the dependencies and still allows for the reuse of
code.

Reported-by: Liviu Dudau <li...@dudau.co.uk>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: 342fa1964439 ("mdio: mux: make child bus walking more permissive and 
errors more verbose")
---
 drivers/of/of_mdio.c| 22 --
 include/linux/of_mdio.h | 24 +++-
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 7e4c80f9b6cd..057963f2b74f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -119,28 +119,6 @@ static void of_mdiobus_register_device(struct mii_bus 
*mdio,
child->name, addr);
 }
 
-int of_mdio_parse_addr(struct device *dev, const struct device_node *np)
-{
-   u32 addr;
-   int ret;
-
-   ret = of_property_read_u32(np, "reg", );
-   if (ret < 0) {
-   dev_err(dev, "%s has invalid PHY address\n", np->full_name);
-   return ret;
-   }
-
-   /* A PHY must have a reg property in the range [0-31] */
-   if (addr >= PHY_MAX_ADDR) {
-   dev_err(dev, "%s PHY address %i is too large\n",
-   np->full_name, addr);
-   return -EINVAL;
-   }
-
-   return addr;
-}
-EXPORT_SYMBOL(of_mdio_parse_addr);
-
 /* The following is a list of PHY compatible strings which appear in
  * some DTBs. The compatible string is never matched against a PHY
  * driver, so is pointless. We only expect devices which are not PHYs
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index ba35ba520487..f5db93bcd069 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -27,11 +27,33 @@ struct phy_device *of_phy_attach(struct net_device *dev,
 phy_interface_t iface);
 
 extern struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
-extern int of_mdio_parse_addr(struct device *dev, const struct device_node 
*np);
 extern int of_phy_register_fixed_link(struct device_node *np);
 extern void of_phy_deregister_fixed_link(struct device_node *np);
 extern bool of_phy_is_fixed_link(struct device_node *np);
 
+
+static inline int of_mdio_parse_addr(struct device *dev,
+const struct device_node *np)
+{
+   u32 addr;
+   int ret;
+
+   ret = of_property_read_u32(np, "reg", );
+   if (ret < 0) {
+   dev_err(dev, "%s has invalid PHY address\n", np->full_name);
+   return ret;
+   }
+
+   /* A PHY must have a reg property in the range [0-31] */
+   if (addr >= PHY_MAX_ADDR) {
+   dev_err(dev, "%s PHY address %i is too large\n",
+   np->full_name, addr);
+   return -EINVAL;
+   }
+
+   return addr;
+}
+
 #else /* CONFIG_OF_MDIO */
 static inline int of_mdiobus_register(struct mii_bus *mdio, struct device_node 
*np)
 {
-- 
2.7.4

Re: Toggling link state breaks network connectivity

[Mason]

On 13/06/2017 11:39, Matthias May wrote:
> On 12/06/17 15:22, Mason wrote:
>> Hello,
>>
>> I am using the following drivers for Ethernet connectivity.
>> drivers/net/ethernet/aurora/nb8800.c
>> drivers/net/phy/at803x.c
>>
>> Pulling the cable and plugging it back works as expected.
>> (I can ping both before and after.)
>>
>> However, if I toggle the link state in software (using ip link set),
>> the board loses network connectivity.
>>
>> # Statically assign IP address
>> ip addr add 172.27.64.77/18 brd 172.27.127.255 dev eth0
>> # Set link state to "up"
>> ip link set eth0 up
>> # ping -c 3 172.27.64.1 > /tmp/v1
>>
>> PING 172.27.64.1 (172.27.64.1): 56 data bytes
>> 64 bytes from 172.27.64.1: seq=0 ttl=64 time=18.321 ms
>>
>> --- 172.27.64.1 ping statistics ---
>> 3 packets transmitted, 1 packets received, 66% packet loss
>> round-trip min/avg/max = 18.321/18.321/18.321 ms
>>
>>
>> 172.27.64.1 is a desktop system.
>> Running
>> % tcpdump -n -i eth1-boards ether host 00:16:e8:4d:7f:c4
>> on the desktop, I get:
>>
>> 15:01:45.187346 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:01:45.187359 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>> 15:01:45.194633 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, 
>> seq 0, length 64
>> 15:01:45.194662 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, 
>> seq 0, length 64
>> 15:01:50.198564 ARP, Request who-has 172.27.64.77 tell 172.27.64.1, length 28
>> 15:01:50.205929 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, 
>> seq 1, length 64
>> 15:01:50.205951 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, 
>> seq 1, length 64
>> 15:01:50.213217 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, 
>> seq 2, length 64
>> 15:01:50.213232 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, 
>> seq 2, length 64
>> 15:01:51.198563 ARP, Request who-has 172.27.64.77 tell 172.27.64.1, length 28
>> 15:01:51.209586 ARP, Reply 172.27.64.77 is-at 00:16:e8:4d:7f:c4, length 46
>> 15:01:51.209598 ARP, Reply 172.27.64.77 is-at 00:16:e8:4d:7f:c4, length 46
>>
>> Packet #1: the board asks for the desktop's MAC address
>> Packet #2: the desktop replies instantly
>> Packet #3: the board sends the first ping
>> Packet #4: the desktop replies instantly
>> Then the board goes quiet for a long time (why???)
>> Packet #5: the desktop asks for the board's MAC address (doesn't it have it 
>> already?)
>> Packet #6: this seems to unwedge the board, which sends the second ping
>> Packet #7: the desktop replies instantly
>> Packet #8: the board sends the third ping
>> Packet #9: the desktop replies instantly
>> Packet #10: the desktop asks again for the board's MAC address
>> Packet #11 and #12: the board answers twice (for the old and new requests?)
>>
>> Some oddities, but it seems to work.
>>
>> Now toggle the link state:
>>
>> % ip link set eth0 down
>> % ip link set eth0 up
>> % ping -c 3 172.27.64.1 > /tmp/v2
>>
>> PING 172.27.64.1 (172.27.64.1): 56 data bytes
>>
>> --- 172.27.64.1 ping statistics ---
>> 3 packets transmitted, 0 packets received, 100% packet loss
>>
>>
>> On the desktop, I see
>>
>> 15:14:03.900162 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:14:03.900175 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>> 15:14:05.017189 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:14:05.017200 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>> 15:14:06.030531 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:14:06.030541 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>>
>> So basically, the board is asking the desktop for its MAC address,
>> and the desktop is answering immediately. But the board doesn't seem
>> to be getting the replies... Any ideas, or words of wisdom, as they say?
> 
> You might want to read this thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg133896.html
> The symptoms you describe are pretty much what we had at one point.

Hello Matthias,

Yes, I remember discussing the issue with Zefir.
https://www.mail-archive.com/netdev@vger.kernel.org/msg147369.html

Do you think I should apply Zefir's patch?

In an attempt to remove the Atheros driver from the equation,
I used the generic PHY driver (I didn't build the Atheros
driver at all). But this had no impact on the issue.

Regards.

Re: Toggling link state breaks network connectivity

[Mason]

Hello Florian,

On 12/06/2017 18:38, Florian Fainelli wrote:

> On 06/12/2017 06:22 AM, Mason wrote:
>
>> I am using the following drivers for Ethernet connectivity.
>> drivers/net/ethernet/aurora/nb8800.c
>> drivers/net/phy/at803x.c
>>
>> Pulling the cable and plugging it back works as expected.
>> (I can ping both before and after.)
>>
>> However, if I toggle the link state in software (using ip link set),
>> the board loses network connectivity.
>>
>> # Statically assign IP address
>> ip addr add 172.27.64.77/18 brd 172.27.127.255 dev eth0
>> # Set link state to "up"
>> ip link set eth0 up
>> # ping -c 3 172.27.64.1 > /tmp/v1
>>
>> PING 172.27.64.1 (172.27.64.1): 56 data bytes
>> 64 bytes from 172.27.64.1: seq=0 ttl=64 time=18.321 ms
> 
> This delay seems abnormally long unless you are purposely introducing
> delay (e.g: with cls_netem) or this is a really remote host, does not
> seem to be based on your traces later on.

172.27.64.1 and 172.27.64.77 are connected to the
same switch. Purely local traffic. It seems to me
that the ARP request/reply could explain the delay.

Start op at 45.187346
Receive ICMP echo reply at 45.194662
Hmmm, that's only 7 ms


>> 172.27.64.1 is a desktop system.
>> Running
>> % tcpdump -n -i eth1-boards ether host 00:16:e8:4d:7f:c4
>> on the desktop, I get:
>>
>> 15:01:45.187346 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:01:45.187359 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>> 15:01:45.194633 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, 
>> seq 0, length 64
>> 15:01:45.194662 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, 
>> seq 0, length 64
>> 15:01:50.198564 ARP, Request who-has 172.27.64.77 tell 172.27.64.1, length 28
>> 15:01:50.205929 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, 
>> seq 1, length 64
>> 15:01:50.205951 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, 
>> seq 1, length 64
>> 15:01:50.213217 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, 
>> seq 2, length 64
>> 15:01:50.213232 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, 
>> seq 2, length 64
>> 15:01:51.198563 ARP, Request who-has 172.27.64.77 tell 172.27.64.1, length 28
>> 15:01:51.209586 ARP, Reply 172.27.64.77 is-at 00:16:e8:4d:7f:c4, length 46
>> 15:01:51.209598 ARP, Reply 172.27.64.77 is-at 00:16:e8:4d:7f:c4, length 46
>>
>> Packet #1: the board asks for the desktop's MAC address
>> Packet #2: the desktop replies instantly
>> Packet #3: the board sends the first ping
>> Packet #4: the desktop replies instantly
>> Then the board goes quiet for a long time (why???)
>> Packet #5: the desktop asks for the board's MAC address (doesn't it have it 
>> already?)
>> Packet #6: this seems to unwedge the board, which sends the second ping
>> Packet #7: the desktop replies instantly
>> Packet #8: the board sends the third ping
>> Packet #9: the desktop replies instantly
>> Packet #10: the desktop asks again for the board's MAC address
>> Packet #11 and #12: the board answers twice (for the old and new requests?)
>>
>> Some oddities, but it seems to work.
>>
>> Now toggle the link state:
>>
>> % ip link set eth0 down
>> % ip link set eth0 up
>> % ping -c 3 172.27.64.1 > /tmp/v2
>>
>> PING 172.27.64.1 (172.27.64.1): 56 data bytes
>>
>> --- 172.27.64.1 ping statistics ---
>> 3 packets transmitted, 0 packets received, 100% packet loss
>>
>>
>> On the desktop, I see
>>
>> 15:14:03.900162 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:14:03.900175 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>> 15:14:05.017189 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:14:05.017200 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>> 15:14:06.030531 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
>> 15:14:06.030541 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
>>
>> So basically, the board is asking the desktop for its MAC address,
>> and the desktop is answering immediately. But the board doesn't seem
>> to be getting the replies... Any ideas, or words of wisdom, as they say?
> 
> - check the Ethernet MAC counters to see if there is packet loss, or
> error, or both

I'll take a look, but I don't expect any packet loss
(LAN traffic on an idle switch).

> - consult with your HW engineers for possible flaws in your
> ndo_open/ndo_close paths and possible interactions with the MAC/PHY
> clocks, or reset etc.

(The HW engineers have no knowledge of Linux use-cases.)
The crazy thing is that I can use the same driver on the
previous chip, and I don't see this behavior... Will
retest tomorrow to be sure. What does change between
the two chips are a few clock frequencies though.
So maybe some race is now consistently lost on the
new chip...

> - see if your PHY needs a complete re-init after an up/down sequence and
> if you are doing this properly

Thanks for these suggestions. I'll take a closer look
tomorrow.

Regards.

Toggling link state breaks network connectivity

[Mason]

Hello,

I am using the following drivers for Ethernet connectivity.
drivers/net/ethernet/aurora/nb8800.c
drivers/net/phy/at803x.c

Pulling the cable and plugging it back works as expected.
(I can ping both before and after.)

However, if I toggle the link state in software (using ip link set),
the board loses network connectivity.

# Statically assign IP address
ip addr add 172.27.64.77/18 brd 172.27.127.255 dev eth0
# Set link state to "up"
ip link set eth0 up
# ping -c 3 172.27.64.1 > /tmp/v1

PING 172.27.64.1 (172.27.64.1): 56 data bytes
64 bytes from 172.27.64.1: seq=0 ttl=64 time=18.321 ms

--- 172.27.64.1 ping statistics ---
3 packets transmitted, 1 packets received, 66% packet loss
round-trip min/avg/max = 18.321/18.321/18.321 ms


172.27.64.1 is a desktop system.
Running
% tcpdump -n -i eth1-boards ether host 00:16:e8:4d:7f:c4
on the desktop, I get:

15:01:45.187346 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
15:01:45.187359 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
15:01:45.194633 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, seq 
0, length 64
15:01:45.194662 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, seq 
0, length 64
15:01:50.198564 ARP, Request who-has 172.27.64.77 tell 172.27.64.1, length 28
15:01:50.205929 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, seq 
1, length 64
15:01:50.205951 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, seq 
1, length 64
15:01:50.213217 IP 172.27.64.77 > 172.27.64.1: ICMP echo request, id 41219, seq 
2, length 64
15:01:50.213232 IP 172.27.64.1 > 172.27.64.77: ICMP echo reply, id 41219, seq 
2, length 64
15:01:51.198563 ARP, Request who-has 172.27.64.77 tell 172.27.64.1, length 28
15:01:51.209586 ARP, Reply 172.27.64.77 is-at 00:16:e8:4d:7f:c4, length 46
15:01:51.209598 ARP, Reply 172.27.64.77 is-at 00:16:e8:4d:7f:c4, length 46

Packet #1: the board asks for the desktop's MAC address
Packet #2: the desktop replies instantly
Packet #3: the board sends the first ping
Packet #4: the desktop replies instantly
Then the board goes quiet for a long time (why???)
Packet #5: the desktop asks for the board's MAC address (doesn't it have it 
already?)
Packet #6: this seems to unwedge the board, which sends the second ping
Packet #7: the desktop replies instantly
Packet #8: the board sends the third ping
Packet #9: the desktop replies instantly
Packet #10: the desktop asks again for the board's MAC address
Packet #11 and #12: the board answers twice (for the old and new requests?)

Some oddities, but it seems to work.

Now toggle the link state:

% ip link set eth0 down
% ip link set eth0 up
% ping -c 3 172.27.64.1 > /tmp/v2

PING 172.27.64.1 (172.27.64.1): 56 data bytes

--- 172.27.64.1 ping statistics ---
3 packets transmitted, 0 packets received, 100% packet loss


On the desktop, I see

15:14:03.900162 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
15:14:03.900175 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
15:14:05.017189 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
15:14:05.017200 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28
15:14:06.030531 ARP, Request who-has 172.27.64.1 tell 172.27.64.77, length 46
15:14:06.030541 ARP, Reply 172.27.64.1 is-at 00:15:17:24:e0:81, length 28

So basically, the board is asking the desktop for its MAC address,
and the desktop is answering immediately. But the board doesn't seem
to be getting the replies... Any ideas, or words of wisdom, as they say?

Regards.

Re: [PATCH net-next] net: phy: use of_mdio_parse_addr

[Jon Mason]

On Wed, Jun 7, 2017 at 12:18 PM, Liviu Dudau <li...@dudau.co.uk> wrote:
> On Fri, Jun 02, 2017 at 02:22:51PM -0400, David Miller wrote:
>> From: Jon Mason <jon.ma...@broadcom.com>
>> Date: Wed, 31 May 2017 15:43:30 -0400
>>
>> > use of_mdio_parse_addr() in place of an OF read of reg and a bounds
>> > check (which is litterally the exact same thing that
>> > of_mdio_parse_addr() does)
>> >
>> > Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
>>
>> Applied, thanks Jon.
>
> This makes linux-next fail the modules_install target as depmod detects 2 
> circular
> dependencies. Reverting this patch fixes the issue.
>
> depmod: ERROR: Cycle detected: libphy -> of_mdio -> fixed_phy -> libphy
> depmod: ERROR: Cycle detected: libphy -> of_mdio -> libphy
> depmod: ERROR: Found 3 modules in dependency cycles!
> make[1]: *** [/home/dliviu/devel/kernel/Makefile:1245: _modinst_post] Error 1

I did not test this as modules.  Sorry.

It would be ugly to duplicate the code in both place, and the code in
question does not seem to really need to be in a C file.  Perhaps it
can be moved to a header file as an inline function, which would solve
this dependency.  Would this be acceptable?

Thanks,
Jon

>
> This is on an ARCH=arm build, build I doubt it makes a difference. Let me 
> know if
> you need some .config values in order to reproduce.
>
> Best regards,
> Liviu
>

[PATCH net-next] mdio: mux: make child bus walking more permissive and errors more verbose

[Jon Mason]

If any errors are encountered while walking the device tree structure of
the MDIO bus for children, the code may silently continue, silently
exit, or throw an error and exit.  This make it difficult for device
tree writers to know there is an error.  Also, it makes any error in a
child entry of the MDIO bus be fatal for all entries.  Instead, we
should provide verbose errors describing the error and then attempt to
continue if it all possible.  Also, use of_mdio_parse_addr()

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/phy/mdio-mux.c | 24 +---
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
index 599ce24c514f..47ded3904050 100644
--- a/drivers/net/phy/mdio-mux.c
+++ b/drivers/net/phy/mdio-mux.c
@@ -135,27 +135,33 @@ int mdio_mux_init(struct device *dev,
for_each_available_child_of_node(dev->of_node, child_bus_node) {
u32 v;
 
-   r = of_property_read_u32(child_bus_node, "reg", );
-   if (r)
+   v = of_mdio_parse_addr(dev, child_bus_node);
+   if (v < 0) {
+   dev_err(dev,
+   "Error: Failed to find reg for child %s\n",
+   of_node_full_name(child_bus_node));
continue;
+   }
 
cb = devm_kzalloc(dev, sizeof(*cb), GFP_KERNEL);
if (cb == NULL) {
dev_err(dev,
-   "Error: Failed to allocate memory for child\n");
+   "Error: Failed to allocate memory for child 
%s\n",
+   of_node_full_name(child_bus_node));
ret_val = -ENOMEM;
-   of_node_put(child_bus_node);
-   break;
+   continue;
}
cb->bus_number = v;
cb->parent = pb;
 
cb->mii_bus = mdiobus_alloc();
if (!cb->mii_bus) {
+   dev_err(dev,
+   "Error: Failed to allocate MDIO bus for child 
%s\n",
+   of_node_full_name(child_bus_node));
ret_val = -ENOMEM;
devm_kfree(dev, cb);
-   of_node_put(child_bus_node);
-   break;
+   continue;
}
cb->mii_bus->priv = cb;
 
@@ -167,6 +173,9 @@ int mdio_mux_init(struct device *dev,
cb->mii_bus->write = mdio_mux_write;
r = of_mdiobus_register(cb->mii_bus, child_bus_node);
if (r) {
+   dev_err(dev,
+   "Error: Failed to register MDIO bus for child 
%s\n",
+   of_node_full_name(child_bus_node));
mdiobus_free(cb->mii_bus);
devm_kfree(dev, cb);
} else {
@@ -180,6 +189,7 @@ int mdio_mux_init(struct device *dev,
return 0;
}
 
+   dev_err(dev, "Error: No acceptable child buses found\n");
devm_kfree(dev, pb);
 err_pb_kz:
/* balance the reference of_mdio_find_bus() took */
-- 
2.7.4

[PATCH net-next] net: phy: use of_mdio_parse_addr

[Jon Mason]

use of_mdio_parse_addr() in place of an OF read of reg and a bounds
check (which is litterally the exact same thing that
of_mdio_parse_addr() does)

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/phy/mdio_bus.c | 15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 8e73f5f36e71..d4782e902e2e 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -263,21 +263,10 @@ static void of_mdiobus_link_mdiodev(struct mii_bus *bus,
 
for_each_available_child_of_node(bus->dev.of_node, child) {
int addr;
-   int ret;
 
-   ret = of_property_read_u32(child, "reg", );
-   if (ret < 0) {
-   dev_err(dev, "%s has invalid MDIO address\n",
-   child->full_name);
+   addr = of_mdio_parse_addr(dev, child);
+   if (addr < 0)
continue;
-   }
-
-   /* A MDIO device must have a reg property in the range [0-31] */
-   if (addr >= PHY_MAX_ADDR) {
-   dev_err(dev, "%s MDIO address %i is too large\n",
-   child->full_name, addr);
-   continue;
-   }
 
if (addr == mdiodev->addr) {
dev->of_node = child;
-- 
2.7.4

Re: [PATCH] mdio: mux: fix device_node_continue.cocci warnings

[Jon Mason]

On Fri, May 12, 2017 at 6:52 PM, Florian Fainelli  wrote:
> On 05/12/2017 09:22 AM, David Miller wrote:
>> From: Julia Lawall 
>> Date: Fri, 12 May 2017 22:54:23 +0800 (SGT)
>>
>>> Device node iterators put the previous value of the index variable, so an
>>> explicit put causes a double put.
>>  ...
>>> @@ -169,7 +169,6 @@ int mdio_mux_init(struct device *dev,
>>>  if (r) {
>>>  mdiobus_free(cb->mii_bus);
>>>  devm_kfree(dev, cb);
>>> -of_node_put(child_bus_node);
>>>  } else {
>>
>> I think we're instead simply missing a break; statement here.
>
> It's kind of questionable, if we have an error initializing one of our
> child MDIO bus controller (child from the perspective of the MDIO mux,
> boy this is getting complicated...), should we keep on going, or should
> we abort entirely and rollback what we have successfully registered?
>
> I don't think Julia's patch makes thing worse, in that if we had to
> rollback, we would not be doing this correctly now anyway.
>
> Jon, what do you think?

If every other case is fatal, then it is odd that this one is
permissive.  I think we should go 100% one way or the other.  So, the
options here are to:
1.  Encounter an error, unroll any mallocs, etc created by this entry,
but continue on to the next entry and return success if any are
created
2.  Encounter an error, unroll any mallocs, etc created by this entry
and any others that were created, and return an error
3.  Encounter an error, unroll any mallocs, etc created by this entry,
exit and return success if any are created

#1 would be the most accepting of any errors encountered
#2 would identify any poorly written DTs by breaking their currently
working functionality (though we should add some error messages to let
them know why)
#3 matches the suggestion by David Miller, and would be a hybrid of #1
and #2 in outcome

I would prefer #1, as I would not want to break something that was
currently working.  However, I think we should add much error logging
here to let people know their DT is hosed (instead of silently
working).  So, this would mean applying Julia's patch, and I'll do a
follow-on to change the breaks to continues and add the error logging
(assuming others agree with me).

Thanks,
Jon

[PATCH] mdio: mux: Correct mdio_mux_init error path issues

[Jon Mason]

There is a potential unnecessary refcount decriment on error path of
put_device(>mii_bus->dev), as it is possible to avoid the
of_mdio_find_bus() call if mux_bus is specified by the calling function.

The same put_device() is not called in the error path if the
devm_kzalloc of pb fails.  This caused the variable used in the
put_device() to be changed, as the pb pointer was obviously not set up.

There is an unnecessary of_node_get() on child_bus_node if the
of_mdiobus_register() is successful, as the
for_each_available_child_of_node() automatically increments this.
Thus the refcount on this node will always be +1 more than it should be.

There is no of_node_put() on child_bus_node if the of_mdiobus_register()
call fails.

Finally, it is lacking devm_kfree() of pb in the error path.  While this
might not be technically necessary, it was present in other parts of the
function.  So, I am adding it where necessary to make it uniform.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: f20e6657a875 ("mdio: mux: Enhanced MDIO mux framework for integrated 
multiplexers")
Fixes: 0ca2997d1452 ("netdev/of/phy: Add MDIO bus multiplexer support.")
---
 drivers/net/phy/mdio-mux.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
index 963838d4fac1..6943c5ece44a 100644
--- a/drivers/net/phy/mdio-mux.c
+++ b/drivers/net/phy/mdio-mux.c
@@ -122,10 +122,9 @@ int mdio_mux_init(struct device *dev,
pb = devm_kzalloc(dev, sizeof(*pb), GFP_KERNEL);
if (pb == NULL) {
ret_val = -ENOMEM;
-   goto err_parent_bus;
+   goto err_pb_kz;
}
 
-
pb->switch_data = data;
pb->switch_fn = switch_fn;
pb->current_child = -1;
@@ -154,6 +153,7 @@ int mdio_mux_init(struct device *dev,
cb->mii_bus = mdiobus_alloc();
if (!cb->mii_bus) {
ret_val = -ENOMEM;
+   devm_kfree(dev, cb);
of_node_put(child_bus_node);
break;
}
@@ -169,8 +169,8 @@ int mdio_mux_init(struct device *dev,
if (r) {
mdiobus_free(cb->mii_bus);
devm_kfree(dev, cb);
+   of_node_put(child_bus_node);
} else {
-   of_node_get(child_bus_node);
cb->next = pb->children;
pb->children = cb;
}
@@ -181,9 +181,11 @@ int mdio_mux_init(struct device *dev,
return 0;
}
 
+   devm_kfree(dev, pb);
+err_pb_kz:
/* balance the reference of_mdio_find_bus() took */
-   put_device(>mii_bus->dev);
-
+   if (!mux_bus)
+   put_device(_bus->dev);
 err_parent_bus:
of_node_put(parent_bus_node);
return ret_val;
-- 
2.7.4

[PATCH] net: mdio-mux: bcm-iproc: call mdiobus_free() in error path

[Jon Mason]

If an error is encountered in mdio_mux_init(), the error path will call
mdiobus_free().  Since mdiobus_register() has been called prior to
mdio_mux_init(), the bus->state will not be MDIOBUS_UNREGISTERED.  This
causes a BUG_ON() in mdiobus_free().  To correct this issue, add an
error path for mdio_mux_init() which calls mdiobus_unregister() prior to
mdiobus_free().

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: 98bc865a1ec8 ("net: mdio-mux: Add MDIO mux driver for iProc SoCs")
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c 
b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 0a0412524cec..0a5f62e0efcc 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -203,11 +203,14 @@ static int mdio_mux_iproc_probe(struct platform_device 
*pdev)
   >mux_handle, md, md->mii_bus);
if (rc) {
dev_info(md->dev, "mdiomux initialization failed\n");
-   goto out;
+   goto out_register;
}
 
dev_info(md->dev, "iProc mdiomux registered\n");
return 0;
+
+out_register:
+   mdiobus_unregister(bus);
 out:
mdiobus_free(bus);
return rc;
-- 
2.7.4

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Jon Mason]

On Tue, Apr 25, 2017 at 11:23 AM, Sergei Shtylyov
<sergei.shtyl...@cogentembedded.com> wrote:
> Hello!
>
> On 04/25/2017 06:15 PM, Jon Mason wrote:
>
>>>> Cygnus has a single amac controller connected to the B53 switch with 2
>>>> PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
>>>> the external ethernet jacks.
>
>
> [...]
>
>>>> Signed-off-by: Eric Anholt <e...@anholt.net>
>>>> ---
>>>>  arch/arm/boot/dts/bcm-cygnus.dtsi  | 60
>>>> ++
>>>>  arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
>>>>  2 files changed, 68 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> index 009f1346b817..318899df9972 100644
>>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>
> [...]
>>>>
>>>> @@ -295,6 +345,16 @@
>>>> status = "disabled";
>>>> };
>>>>
>>>> +   eth0: enet@18042000 {
>>>> +   compatible = "brcm,amac";
>>>> +   reg = <0x18042000 0x1000>,
>>>> + <0x1811 0x1000>;
>>>> +   reg-names = "amac_base", "idm_base";
>>>
>>>
>>>
>>>I don't think "_base" suffixes are necessary here.
>>
>>
>> 100% necessary, per the driver.  See
>> drivers/net/ethernet/broadcom/bgmac-platform.c
>
>
>I'd recommend to fix the driver/bindings then...

They're already in use in other device trees.  So, we'd need to
support backward compatibility on them, thus removing any real benefit
to changing them.


>
> MBR, Sergei
>

Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.

[Jon Mason]

On Tue, Apr 25, 2017 at 5:40 AM, Sergei Shtylyov
 wrote:
> Hello.
>
> On 4/25/2017 12:50 AM, Eric Anholt wrote:
>
>> Cygnus has a single amac controller connected to the B53 switch with 2
>> PHYs.  On the BCM911360_EP platform, those two PHYs are connected to
>> the external ethernet jacks.
>
>
>My spell checker trips on "amac" and "ethernet" -- perhaps they need
> capitalization?
>
>> Signed-off-by: Eric Anholt 
>> ---
>>  arch/arm/boot/dts/bcm-cygnus.dtsi  | 60
>> ++
>>  arch/arm/boot/dts/bcm911360_entphn.dts |  8 +
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> index 009f1346b817..318899df9972 100644
>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>> @@ -142,6 +142,56 @@
>> interrupts = <0>;
>> };
>>
>> +   mdio: mdio@18002000 {
>> +   compatible = "brcm,iproc-mdio";
>> +   reg = <0x18002000 0x8>;
>> +   #size-cells = <1>;
>> +   #address-cells = <0>;
>> +
>> +   gphy0: eth-gphy@0 {
>
>
>The node anmes must be generic, the DT spec has standardized
> "ethernet-phy" name for this case.
>
>> +   reg = <0>;
>> +   max-speed = <1000>;
>> +   };
>> +
>> +   gphy1: eth-gphy@1 {
>> +   reg = <1>;
>> +   max-speed = <1000>;
>> +   };
>> +   };
>
> [...]
>>
>> @@ -295,6 +345,16 @@
>> status = "disabled";
>> };
>>
>> +   eth0: enet@18042000 {
>> +   compatible = "brcm,amac";
>> +   reg = <0x18042000 0x1000>,
>> + <0x1811 0x1000>;
>> +   reg-names = "amac_base", "idm_base";
>
>
>I don't think "_base" suffixes are necessary here.

100% necessary, per the driver.  See
drivers/net/ethernet/broadcom/bgmac-platform.c


>
> [...]
>
> MBR, Sergei
>

Re: [PATCH] net: ethernet: bgmac: Allow MAC address to be specified in DTB

[Jon Mason]

On Thu, Mar 16, 2017 at 12:39 PM, Florian Fainelli <f.faine...@gmail.com> wrote:
> On 03/16/2017 08:48 AM, Steve Lin wrote:
>> Allows the BCMA version of the bgmac driver to obtain MAC address
>> from the device tree.  If no MAC address is specified there, then
>> the previous behavior (obtaining MAC address from SPROM) is
>> used.
>>
>> Signed-off-by: Steve Lin <steven.l...@broadcom.com>
>
> Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
>
> PS: you might want to specify which tree this applies to by using [PATCH
> net-next] or [PATCH net] in the subject, see the netdev-FAQ.txt for
> details:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.txt

I believe he wants this in net-next

Acked-by: Jon Mason <jon.ma...@broadcom.com>

>> ---
>>  drivers/net/ethernet/broadcom/bgmac-bcma.c | 39 
>> ++
>>  1 file changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
>> b/drivers/net/ethernet/broadcom/bgmac-bcma.c
>> index cf15b7e..6322594 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
>> @@ -11,6 +11,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include "bgmac.h"
>>
>>  static inline bool bgmac_is_bcm4707_family(struct bcma_device *core)
>> @@ -114,7 +115,7 @@ static int bgmac_probe(struct bcma_device *core)
>>   struct ssb_sprom *sprom = >bus->sprom;
>>   struct mii_bus *mii_bus;
>>   struct bgmac *bgmac;
>> - u8 *mac;
>> + const u8 *mac = NULL;
>>   int err;
>>
>>   bgmac = bgmac_alloc(>dev);
>> @@ -127,21 +128,27 @@ static int bgmac_probe(struct bcma_device *core)
>>
>>   bcma_set_drvdata(core, bgmac);
>>
>> - switch (core->core_unit) {
>> - case 0:
>> - mac = sprom->et0mac;
>> - break;
>> - case 1:
>> - mac = sprom->et1mac;
>> - break;
>> - case 2:
>> - mac = sprom->et2mac;
>> - break;
>> - default:
>> - dev_err(bgmac->dev, "Unsupported core_unit %d\n",
>> - core->core_unit);
>> - err = -ENOTSUPP;
>> - goto err;
>> + if (bgmac->dev->of_node)
>> + mac = of_get_mac_address(bgmac->dev->of_node);
>> +
>> + /* If no MAC address assigned via device tree, check SPROM */
>> + if (!mac) {
>> + switch (core->core_unit) {
>> + case 0:
>> + mac = sprom->et0mac;
>> + break;
>> + case 1:
>> + mac = sprom->et1mac;
>> + break;
>> + case 2:
>> + mac = sprom->et2mac;
>> + break;
>> + default:
>> + dev_err(bgmac->dev, "Unsupported core_unit %d\n",
>> + core->core_unit);
>> + err = -ENOTSUPP;
>> + goto err;
>> + }
>>   }
>>
>>   ether_addr_copy(bgmac->net_dev->dev_addr, mac);
>>
>
>
> --
> Florian

Re: Legacy features in PCI Express devices

[Mason]

On 13/03/2017 18:12, Robin Murphy wrote:

> On 13/03/17 16:10, Mason wrote:
>
>> There are two revisions of our PCI Express controller.
>>
>> Rev 1 did not support the following features:
>>
>>   1) legacy PCI interrupt delivery (INTx signals)
>>   2) I/O address space
>>
>> Internally, someone stated that such missing support would prevent
>> some PCIe cards from working with our controller.
>>
>> Are there really modern PCIe cards that require 1) and/or 2)
>> to function?
>>
>> Can someone provide examples of such cards, so that I may test them
>> on both revisions?
>>
>> I was told to check ath9k-based cards. Any other examples?
>>
>> Looking around, I came across this thread:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/418254.html
>> "i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity"
>>
>> IIUC, although some PCIe boards do support MSI, the driver might not
>> put in the work to use that infrastructure, and instead reverts to
>> legacy interrupts. (So it is a SW issue, in a sense.)
> 
> Secondary to that category is endpoints which nominally support MSI, but
> in a way which is unreliable or otherwise broken. My experience shows
> that the Silicon Image SiI 3132 (as integrated on ARM Juno boards, but
> seemingly also relatively common on 'generic' 2-port SATA cards) falls
> into that category - using the command-line parameter to force MSIs
> instead of legacy interrupts leads to the the machine barely reaching
> userspace before something goes horribly wrong:

Do drivers typically support *both* MSI and INTx?

Specifically, would the xhci driver support both?

If I remove MSI support from my kernel, I might be able to test
legacy interrupt support that way, right?

Regards.

Legacy features in PCI Express devices

[Mason]

Hello,

There are two revisions of our PCI Express controller.

Rev 1 did not support the following features:

  1) legacy PCI interrupt delivery (INTx signals)
  2) I/O address space

Internally, someone stated that such missing support would prevent
some PCIe cards from working with our controller.

Are there really modern PCIe cards that require 1) and/or 2)
to function?

Can someone provide examples of such cards, so that I may test them
on both revisions?

I was told to check ath9k-based cards. Any other examples?

Looking around, I came across this thread:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/418254.html
"i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity"

IIUC, although some PCIe boards do support MSI, the driver might not
put in the work to use that infrastructure, and instead reverts to
legacy interrupts. (So it is a SW issue, in a sense.)

Regards.

Re: [PATCH net v4 0/2] net: ethernet: bgmac: bug fixes

[Jon Mason]

On Thu, Mar 02, 2017 at 12:56:05PM -0800, David Miller wrote:
> From: David Miller <da...@davemloft.net>
> Date: Thu, 02 Mar 2017 12:50:15 -0800 (PST)
> 
> > From: Jon Mason <jon.ma...@broadcom.com>
> > Date: Tue, 28 Feb 2017 13:41:49 -0500
> > 
> >> Changes in v4:
> >> * Added the udelays from the previous code (per David Miller)
> >> 
> >> Changes in v3:
> >> * Reworked the init sequence patch to only remove the device reset if
> >>   the device is actually in reset.  Given that this code doesn't bear
> >>   much resemblance to the original code, I'm changing the author of the
> >>   patch.  This was tested on NS2 SVK.
> >> 
> >> Changes in v2:
> >> * Reworked the first match to make it more obvious what portions of the
> >>   register were being preserved (Per Rafal Mileki)
> >> * Style change to reorder the function variables in patch 2 (per Sergei
> >>   Shtylyov)
> >> 
> >> Bug fixes for bgmac driver
> > 
> > Series applied.
> 
> Actually, this doesn't even compile.  Reverted...
> 
> [davem@kkuri net]$ make -s -j4
> drivers/net/ethernet/broadcom/bgmac.c: In function ‘bgmac_set_mac_address’:
> drivers/net/ethernet/broadcom/bgmac.c:1233:23: error: ‘struct bgmac’ has no 
> member named ‘mac_addr’; did you mean ‘phyaddr’?
>   ether_addr_copy(bgmac->mac_addr, sa->sa_data);
>^~
> drivers/net/ethernet/broadcom/bgmac.c:1234:38: error: ‘struct bgmac’ has no 
> member named ‘mac_addr’; did you mean ‘phyaddr’?
>   bgmac_write_mac_address(bgmac, bgmac->mac_addr);
>   ^~

Well this is embarrassing.  I didn't rebase, even though I acked the
patch which changed it out from under me.  Sorry, I should've known
better.

Rebased, compiled, and tested patch coming shortly.  I appreciate your
patience.

Thanks,
Jon

[PATCH net v5 1/2] net: ethernet: bgmac: init sequence bug

[Jon Mason]

Fix a bug in the 'bgmac' driver init sequence that blind writes for init
sequence where it should preserve most bits other than the ones it is
deliberately manipulating.

The code now checks to see if the adapter needs to be brought out of
reset (where as before it was doing an IDM write to bring it out of
reset regardless of whether it was in reset or not).  Also, removed
unnecessary usleeps (as there is already a read present to flush the
IDM writes).

Signed-off-by: Zac Schroff <zschr...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 27 +-
 drivers/net/ethernet/broadcom/bgmac.h  | 16 +++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 7b1af95..da1b8b2 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -51,8 +51,7 @@ static void platform_bgmac_idm_write(struct bgmac *bgmac, u16 
offset, u32 value)
 
 static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 {
-   if ((bgmac_idm_read(bgmac, BCMA_IOCTL) &
-(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC)) != BCMA_IOCTL_CLK)
+   if ((bgmac_idm_read(bgmac, BCMA_IOCTL) & BGMAC_CLK_EN) != BGMAC_CLK_EN)
return false;
if (bgmac_idm_read(bgmac, BCMA_RESET_CTL) & BCMA_RESET_CTL_RESET)
return false;
@@ -61,15 +60,25 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 
 static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
 {
-   bgmac_idm_write(bgmac, BCMA_IOCTL,
-   (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
-   bgmac_idm_read(bgmac, BCMA_IOCTL);
+   u32 val;
 
-   bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
-   bgmac_idm_read(bgmac, BCMA_RESET_CTL);
-   udelay(1);
+   /* The Reset Control register only contains a single bit to show if the
+* controller is currently in reset.  Do a sanity check here, just in
+* case the bootloader happened to leave the device in reset.
+*/
+   val = bgmac_idm_read(bgmac, BCMA_RESET_CTL);
+   if (val) {
+   bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
+   bgmac_idm_read(bgmac, BCMA_RESET_CTL);
+   udelay(1);
+   }
 
-   bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
+   val = bgmac_idm_read(bgmac, BCMA_IOCTL);
+   /* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
+   val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
+BGMAC_ARUSER);
+   val |= BGMAC_CLK_EN;
+   bgmac_idm_write(bgmac, BCMA_IOCTL, val);
bgmac_idm_read(bgmac, BCMA_IOCTL);
udelay(1);
 }
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 248727d..6d1c6ff 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -213,6 +213,22 @@
 /* BCMA GMAC core specific IO Control (BCMA_IOCTL) flags */
 #define BGMAC_BCMA_IOCTL_SW_CLKEN  0x0004  /* PHY Clock 
Enable */
 #define BGMAC_BCMA_IOCTL_SW_RESET  0x0008  /* PHY Reset */
+/* The IOCTL values appear to be different in NS, NSP, and NS2, and do not 
match
+ * the values directly above
+ */
+#define BGMAC_CLK_EN   BIT(0)
+#define BGMAC_RESERVED_0   BIT(1)
+#define BGMAC_SOURCE_SYNC_MODE_EN  BIT(2)
+#define BGMAC_DEST_SYNC_MODE_ENBIT(3)
+#define BGMAC_TX_CLK_OUT_INVERT_EN BIT(4)
+#define BGMAC_DIRECT_GMII_MODE BIT(5)
+#define BGMAC_CLK_250_SEL  BIT(6)
+#define BGMAC_AWCACHE  (0xf << 7)
+#define BGMAC_RESERVED_1   (0x1f << 11)
+#define BGMAC_ARCACHE  (0xf << 16)
+#define BGMAC_AWUSER   (0x3f << 20)
+#define BGMAC_ARUSER   (0x3f << 26)
+#define BGMAC_RESERVED BIT(31)
 
 /* BCMA GMAC core specific IO status (BCMA_IOST) flags */
 #define BGMAC_BCMA_IOST_ATTACHED   0x0800
-- 
2.7.4

[PATCH net v5 2/2] net: ethernet: bgmac: mac address change bug

[Jon Mason]

From: Hari Vyas <ha...@broadcom.com>

ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
chopping and the wrong mac address was configured.

Signed-off-by: Hari Vyas <ha...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: 4e209001b86 ("bgmac: write mac address to hardware in 
ndo_set_mac_address")
---
 drivers/net/ethernet/broadcom/bgmac.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 4150467..fd66fca 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1223,12 +1223,16 @@ static netdev_tx_t bgmac_start_xmit(struct sk_buff *skb,
 static int bgmac_set_mac_address(struct net_device *net_dev, void *addr)
 {
struct bgmac *bgmac = netdev_priv(net_dev);
+   struct sockaddr *sa = addr;
int ret;
 
ret = eth_prepare_mac_addr_change(net_dev, addr);
if (ret < 0)
return ret;
-   bgmac_write_mac_address(bgmac, (u8 *)addr);
+
+   ether_addr_copy(net_dev->dev_addr, sa->sa_data);
+   bgmac_write_mac_address(bgmac, net_dev->dev_addr);
+
eth_commit_mac_addr_change(net_dev, addr);
return 0;
 }
-- 
2.7.4

[PATCH net v5 0/2] net: ethernet: bgmac: bug fixes

[Jon Mason]

Changes in v5:
* Rebased to the latest code and fixed up a compile error due to the
  mac_addr struct going away (found by David Miller)

Changes in v4:
* Added the udelays from the previous code (per David Miller)

Changes in v3:
* Reworked the init sequence patch to only remove the device reset if
  the device is actually in reset.  Given that this code doesn't bear
  much resemblance to the original code, I'm changing the author of the
  patch.  This was tested on NS2 SVK.

Changes in v2:
* Reworked the first match to make it more obvious what portions of the
  register were being preserved (Per Rafal Mileki)
* Style change to reorder the function variables in patch 2 (per Sergei
  Shtylyov)


Bug fixes for bgmac driver


Hari Vyas (1):
  net: ethernet: bgmac: mac address change bug

Jon Mason (1):
  net: ethernet: bgmac: init sequence bug

 drivers/net/ethernet/broadcom/bgmac-platform.c | 27 +-
 drivers/net/ethernet/broadcom/bgmac.c  |  6 +-
 drivers/net/ethernet/broadcom/bgmac.h  | 16 +++
 3 files changed, 39 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH net-next v3 0/3] net: ethernet: bgmac: PM support and clean-ups

[Jon Mason]

Changes in v3:
* Corrected a bug Florian found and added his Reviewed-by

Changes in v2:
* Reworked the PM patch with Florian's suggestions


Add code to support Power Management (only tested on NS2), and add some
code clean-ups


Joey Zhong (1):
  net: ethernet: bgmac: driver power manangement

Jon Mason (2):
  net: ethernet: bgmac: use #defines for MAX size
  net: ethernet: bgmac: unify code of the same family

 drivers/net/ethernet/broadcom/bgmac-bcma.c | 64 +++---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 ++
 drivers/net/ethernet/broadcom/bgmac.c  | 51 
 drivers/net/ethernet/broadcom/bgmac.h  |  4 +-
 4 files changed, 116 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH net-next v3 1/3] net: ethernet: bgmac: use #defines for MAX size

[Jon Mason]

The maximum frame size is really just the standard ethernet frame size
and FCS.  So use those existing defines to make the code a little more
beautiful.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 6d1c6ff..a75ed35 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -402,7 +402,7 @@
 
 #define BGMAC_WEIGHT   64
 
-#define ETHER_MAX_LEN   1518
+#define ETHER_MAX_LEN  (ETH_FRAME_LEN + ETH_FCS_LEN)
 
 /* Feature Flags */
 #define BGMAC_FEAT_TX_MASK_SETUP   BIT(0)
-- 
2.7.4

[PATCH net-next v3 3/3] net: ethernet: bgmac: driver power manangement

[Jon Mason]

From: Joey Zhong <zho...@broadcom.com>

Implement suspend/resume callbacks in the bgmac driver. This makes sure
that we de-initialize and re-initialize the hardware correctly before
entering suspend and when resuming.

Signed-off-by: Joey Zhong <zho...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 +
 drivers/net/ethernet/broadcom/bgmac.c  | 51 ++
 drivers/net/ethernet/broadcom/bgmac.h  |  2 +
 3 files changed, 87 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 5a3d0b7..ce47728 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -21,8 +21,12 @@
 #include 
 #include "bgmac.h"
 
+#define NICPM_PADRING_CFG  0x0004
 #define NICPM_IOMUX_CTRL   0x0008
 
+#define NICPM_PADRING_CFG_INIT_VAL 0x7400
+#define NICPM_IOMUX_CTRL_INIT_VAL_AX   0x2188
+
 #define NICPM_IOMUX_CTRL_INIT_VAL  0x3196e000
 #define NICPM_IOMUX_CTRL_SPD_SHIFT 10
 #define NICPM_IOMUX_CTRL_SPD_10M   0
@@ -108,6 +112,10 @@ static void bgmac_nicpm_speed_set(struct net_device 
*net_dev)
if (!bgmac->plat.nicpm_base)
return;
 
+   /* SET RGMII IO CONFIG */
+   writel(NICPM_PADRING_CFG_INIT_VAL,
+  bgmac->plat.nicpm_base + NICPM_PADRING_CFG);
+
val = NICPM_IOMUX_CTRL_INIT_VAL;
switch (bgmac->net_dev->phydev->speed) {
default:
@@ -239,6 +247,31 @@ static int bgmac_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM
+static int bgmac_suspend(struct device *dev)
+{
+   struct bgmac *bgmac = dev_get_drvdata(dev);
+
+   return bgmac_enet_suspend(bgmac);
+}
+
+static int bgmac_resume(struct device *dev)
+{
+   struct bgmac *bgmac = dev_get_drvdata(dev);
+
+   return bgmac_enet_resume(bgmac);
+}
+
+static const struct dev_pm_ops bgmac_pm_ops = {
+   .suspend = bgmac_suspend,
+   .resume = bgmac_resume
+};
+
+#define BGMAC_PM_OPS (_pm_ops)
+#else
+#define BGMAC_PM_OPS NULL
+#endif /* CONFIG_PM */
+
 static const struct of_device_id bgmac_of_enet_match[] = {
{.compatible = "brcm,amac",},
{.compatible = "brcm,nsp-amac",},
@@ -252,6 +285,7 @@ static struct platform_driver bgmac_enet_driver = {
.driver = {
.name  = "bgmac-enet",
.of_match_table = bgmac_of_enet_match,
+   .pm = BGMAC_PM_OPS
},
.probe = bgmac_probe,
.remove = bgmac_remove,
diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 6b7782f..7f516a2 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1480,6 +1480,7 @@ int bgmac_enet_probe(struct bgmac *bgmac)
 
net_dev->irq = bgmac->irq;
SET_NETDEV_DEV(net_dev, bgmac->dev);
+   dev_set_drvdata(bgmac->dev, bgmac);
 
if (!is_valid_ether_addr(net_dev->dev_addr)) {
dev_err(bgmac->dev, "Invalid MAC addr: %pM\n",
@@ -1552,5 +1553,55 @@ void bgmac_enet_remove(struct bgmac *bgmac)
 }
 EXPORT_SYMBOL_GPL(bgmac_enet_remove);
 
+int bgmac_enet_suspend(struct bgmac *bgmac)
+{
+   if (!netif_running(bgmac->net_dev))
+   return 0;
+
+   phy_stop(bgmac->net_dev->phydev);
+
+   netif_stop_queue(bgmac->net_dev);
+
+   napi_disable(>napi);
+
+   netif_tx_lock(bgmac->net_dev);
+   netif_device_detach(bgmac->net_dev);
+   netif_tx_unlock(bgmac->net_dev);
+
+   bgmac_chip_intrs_off(bgmac);
+   bgmac_chip_reset(bgmac);
+   bgmac_dma_cleanup(bgmac);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_suspend);
+
+int bgmac_enet_resume(struct bgmac *bgmac)
+{
+   int rc;
+
+   if (!netif_running(bgmac->net_dev))
+   return 0;
+
+   rc = bgmac_dma_init(bgmac);
+   if (rc)
+   return rc;
+
+   bgmac_chip_init(bgmac);
+
+   napi_enable(>napi);
+
+   netif_tx_lock(bgmac->net_dev);
+   netif_device_attach(bgmac->net_dev);
+   netif_tx_unlock(bgmac->net_dev);
+
+   netif_start_queue(bgmac->net_dev);
+
+   phy_start(bgmac->net_dev->phydev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_resume);
+
 MODULE_AUTHOR("Rafał Miłecki");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index a75ed35..c181876 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -537,6 +537,8 @@ int bgmac_enet_probe(struct bgmac *bgmac);
 void bgmac_enet_remove(struct bgmac *bgmac);
 void

[PATCH net-next v3 2/3] net: ethernet: bgmac: unify code of the same family

[Jon Mason]

BCM471X and BCM535X are of the same family (from what I can derive from
internal documents).  Group them into the case statement together, which
results in more code reuse.

Also, use existing helper variables to make the code a little more
readable too.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 64 +-
 1 file changed, 28 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index d59cfcc4..cf15b7e 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -192,36 +192,50 @@ static int bgmac_probe(struct bcma_device *core)
goto err1;
}
 
-   bgmac->has_robosw = !!(core->bus->sprom.boardflags_lo &
-  BGMAC_BFL_ENETROBO);
+   bgmac->has_robosw = !!(sprom->boardflags_lo & BGMAC_BFL_ENETROBO);
if (bgmac->has_robosw)
dev_warn(bgmac->dev, "Support for Roboswitch not 
implemented\n");
 
-   if (core->bus->sprom.boardflags_lo & BGMAC_BFL_ENETADM)
+   if (sprom->boardflags_lo & BGMAC_BFL_ENETADM)
dev_warn(bgmac->dev, "Support for ADMtek ethernet switch not 
implemented\n");
 
/* Feature Flags */
-   switch (core->bus->chipinfo.id) {
+   switch (ci->id) {
+   /* BCM 471X/535X family */
+   case BCMA_CHIP_ID_BCM4716:
+   bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
+   /* fallthrough */
+   case BCMA_CHIP_ID_BCM47162:
+   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
+   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   break;
case BCMA_CHIP_ID_BCM5357:
+   case BCMA_CHIP_ID_BCM53572:
bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47186) {
-   bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+   if (ci->pkg == BCMA_PKG_ID_BCM47188 ||
+   ci->pkg == BCMA_PKG_ID_BCM47186) {
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+   bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
}
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM5358)
+   if (ci->pkg == BCMA_PKG_ID_BCM5358)
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_EPHYRMII;
break;
-   case BCMA_CHIP_ID_BCM53572:
-   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   case BCMA_CHIP_ID_BCM53573:
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
-   bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47188) {
-   bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+   if (core->core_unit == 0) {
+   bgmac->feature_flags |= BGMAC_FEAT_CC4_IF_SW_TYPE;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
+   bgmac->feature_flags |=
+   BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII;
+   } else if (core->core_unit == 1) {
+   bgmac->feature_flags |= BGMAC_FEAT_IRQ_ID_OOB_6;
+   bgmac->feature_flags |= BGMAC_FEAT_CC7_IF_TYPE_RGMII;
}
break;
case BCMA_CHIP_ID_BCM4749:
@@ -229,18 +243,11 @@ static int bgmac_probe(struct bcma_device *core)
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == 10) {
+   if (ci->pkg == 10) {
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
}
break;
-   case BCMA_CHIP_ID_BCM4716:
-   bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-   /* fallthrough */
-   case BCMA_CHIP_ID_BCM47162:
-   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
-   bgmac->feature_fl

[PATCH net v4 1/2] net: ethernet: bgmac: init sequence bug

[Jon Mason]

Fix a bug in the 'bgmac' driver init sequence that blind writes for init
sequence where it should preserve most bits other than the ones it is
deliberately manipulating.

The code now checks to see if the adapter needs to be brought out of
reset (where as before it was doing an IDM write to bring it out of
reset regardless of whether it was in reset or not).  Also, removed
unnecessary usleeps (as there is already a read present to flush the
IDM writes).

Signed-off-by: Zac Schroff <zschr...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 27 +-
 drivers/net/ethernet/broadcom/bgmac.h  | 16 +++
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 7b1af95..da1b8b2 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -51,8 +51,7 @@ static void platform_bgmac_idm_write(struct bgmac *bgmac, u16 
offset, u32 value)
 
 static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 {
-   if ((bgmac_idm_read(bgmac, BCMA_IOCTL) &
-(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC)) != BCMA_IOCTL_CLK)
+   if ((bgmac_idm_read(bgmac, BCMA_IOCTL) & BGMAC_CLK_EN) != BGMAC_CLK_EN)
return false;
if (bgmac_idm_read(bgmac, BCMA_RESET_CTL) & BCMA_RESET_CTL_RESET)
return false;
@@ -61,15 +60,25 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 
 static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
 {
-   bgmac_idm_write(bgmac, BCMA_IOCTL,
-   (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
-   bgmac_idm_read(bgmac, BCMA_IOCTL);
+   u32 val;
 
-   bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
-   bgmac_idm_read(bgmac, BCMA_RESET_CTL);
-   udelay(1);
+   /* The Reset Control register only contains a single bit to show if the
+* controller is currently in reset.  Do a sanity check here, just in
+* case the bootloader happened to leave the device in reset.
+*/
+   val = bgmac_idm_read(bgmac, BCMA_RESET_CTL);
+   if (val) {
+   bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
+   bgmac_idm_read(bgmac, BCMA_RESET_CTL);
+   udelay(1);
+   }
 
-   bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
+   val = bgmac_idm_read(bgmac, BCMA_IOCTL);
+   /* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
+   val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
+BGMAC_ARUSER);
+   val |= BGMAC_CLK_EN;
+   bgmac_idm_write(bgmac, BCMA_IOCTL, val);
bgmac_idm_read(bgmac, BCMA_IOCTL);
udelay(1);
 }
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 248727d..6d1c6ff 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -213,6 +213,22 @@
 /* BCMA GMAC core specific IO Control (BCMA_IOCTL) flags */
 #define BGMAC_BCMA_IOCTL_SW_CLKEN  0x0004  /* PHY Clock 
Enable */
 #define BGMAC_BCMA_IOCTL_SW_RESET  0x0008  /* PHY Reset */
+/* The IOCTL values appear to be different in NS, NSP, and NS2, and do not 
match
+ * the values directly above
+ */
+#define BGMAC_CLK_EN   BIT(0)
+#define BGMAC_RESERVED_0   BIT(1)
+#define BGMAC_SOURCE_SYNC_MODE_EN  BIT(2)
+#define BGMAC_DEST_SYNC_MODE_ENBIT(3)
+#define BGMAC_TX_CLK_OUT_INVERT_EN BIT(4)
+#define BGMAC_DIRECT_GMII_MODE BIT(5)
+#define BGMAC_CLK_250_SEL  BIT(6)
+#define BGMAC_AWCACHE  (0xf << 7)
+#define BGMAC_RESERVED_1   (0x1f << 11)
+#define BGMAC_ARCACHE  (0xf << 16)
+#define BGMAC_AWUSER   (0x3f << 20)
+#define BGMAC_ARUSER   (0x3f << 26)
+#define BGMAC_RESERVED BIT(31)
 
 /* BCMA GMAC core specific IO status (BCMA_IOST) flags */
 #define BGMAC_BCMA_IOST_ATTACHED   0x0800
-- 
2.7.4

[PATCH net v4 0/2] net: ethernet: bgmac: bug fixes

[Jon Mason]

Changes in v4:
* Added the udelays from the previous code (per David Miller)

Changes in v3:
* Reworked the init sequence patch to only remove the device reset if
  the device is actually in reset.  Given that this code doesn't bear
  much resemblance to the original code, I'm changing the author of the
  patch.  This was tested on NS2 SVK.

Changes in v2:
* Reworked the first match to make it more obvious what portions of the
  register were being preserved (Per Rafal Mileki)
* Style change to reorder the function variables in patch 2 (per Sergei
  Shtylyov)


Bug fixes for bgmac driver


Hari Vyas (1):
  net: ethernet: bgmac: mac address change bug

Jon Mason (1):
  net: ethernet: bgmac: init sequence bug

 drivers/net/ethernet/broadcom/bgmac-platform.c | 27 +-
 drivers/net/ethernet/broadcom/bgmac.c  |  6 +-
 drivers/net/ethernet/broadcom/bgmac.h  | 16 +++
 3 files changed, 39 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH net v4 2/2] net: ethernet: bgmac: mac address change bug

[Jon Mason]

From: Hari Vyas <ha...@broadcom.com>

ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
chopping and the wrong mac address was configured.

Signed-off-by: Hari Vyas <ha...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: 4e209001b86 ("bgmac: write mac address to hardware in 
ndo_set_mac_address")
---
 drivers/net/ethernet/broadcom/bgmac.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index 4150467..6b7782f 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1223,12 +1223,16 @@ static netdev_tx_t bgmac_start_xmit(struct sk_buff *skb,
 static int bgmac_set_mac_address(struct net_device *net_dev, void *addr)
 {
struct bgmac *bgmac = netdev_priv(net_dev);
+   struct sockaddr *sa = addr;
int ret;
 
ret = eth_prepare_mac_addr_change(net_dev, addr);
if (ret < 0)
return ret;
-   bgmac_write_mac_address(bgmac, (u8 *)addr);
+
+   ether_addr_copy(bgmac->mac_addr, sa->sa_data);
+   bgmac_write_mac_address(bgmac, bgmac->mac_addr);
+
eth_commit_mac_addr_change(net_dev, addr);
return 0;
 }
-- 
2.7.4

Re: [PATCH] net: bgmac: store MAC address directly in netdev->dev_addr

[Jon Mason]

On Thu, Feb 16, 2017 at 9:11 AM, Tobias Klauser <tklau...@distanz.ch> wrote:
> After commit 34a5102c3235 ("net: bgmac: allocate struct bgmac just once
> & don't copy it") the mac_addr member of struct bgmac is no longer
> necessary to pass the MAC address to bgmac_enet_probe(). Instead it can
> directly be stored in netdev->dev_addr.
>
> Also use eth_hw_addr_random() instead of eth_random_addr() in case a
> random MAC is nedded. This will make sure netdev->addr_assign_type will
> be properly set.
>
> Signed-off-by: Tobias Klauser <tklau...@distanz.ch>

Looks sane to me

Acked-by: Jon Mason <jon.ma...@broadcom.com>

> ---
>  drivers/net/ethernet/broadcom/bgmac-bcma.c | 2 +-
>  drivers/net/ethernet/broadcom/bgmac-platform.c | 2 +-
>  drivers/net/ethernet/broadcom/bgmac.c  | 9 -
>  drivers/net/ethernet/broadcom/bgmac.h  | 1 -
>  4 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
> b/drivers/net/ethernet/broadcom/bgmac-bcma.c
> index 5ef60d4f12b4..d59cfcc4c4d5 100644
> --- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
> +++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
> @@ -144,7 +144,7 @@ static int bgmac_probe(struct bcma_device *core)
> goto err;
> }
>
> -   ether_addr_copy(bgmac->mac_addr, mac);
> +   ether_addr_copy(bgmac->net_dev->dev_addr, mac);
>
> /* On BCM4706 we need common core to access PHY */
> if (core->id.id == BCMA_CORE_4706_MAC_GBIT &&
> diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
> b/drivers/net/ethernet/broadcom/bgmac-platform.c
> index 805e6ed6c390..7b1af950f312 100644
> --- a/drivers/net/ethernet/broadcom/bgmac-platform.c
> +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
> @@ -169,7 +169,7 @@ static int bgmac_probe(struct platform_device *pdev)
>
> mac_addr = of_get_mac_address(np);
> if (mac_addr)
> -   ether_addr_copy(bgmac->mac_addr, mac_addr);
> +   ether_addr_copy(bgmac->net_dev->dev_addr, mac_addr);
> else
> dev_warn(>dev, "MAC address not present in device 
> tree\n");
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
> b/drivers/net/ethernet/broadcom/bgmac.c
> index 20fe2520da42..415046750bb4 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1477,14 +1477,13 @@ int bgmac_enet_probe(struct bgmac *bgmac)
> net_dev->irq = bgmac->irq;
> SET_NETDEV_DEV(net_dev, bgmac->dev);
>
> -   if (!is_valid_ether_addr(bgmac->mac_addr)) {
> +   if (!is_valid_ether_addr(net_dev->dev_addr)) {
> dev_err(bgmac->dev, "Invalid MAC addr: %pM\n",
> -   bgmac->mac_addr);
> -   eth_random_addr(bgmac->mac_addr);
> +   net_dev->dev_addr);
> +   eth_hw_addr_random(net_dev);
> dev_warn(bgmac->dev, "Using random MAC: %pM\n",
> -bgmac->mac_addr);
> +net_dev->dev_addr);
> }
> -   ether_addr_copy(net_dev->dev_addr, bgmac->mac_addr);
>
> /* This (reset &) enable is not preset in specs or reference driver 
> but
>  * Broadcom does it in arch PCI code when enabling fake PCI device.
> diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
> b/drivers/net/ethernet/broadcom/bgmac.h
> index ab2db76e4fb8..248727dc62f2 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.h
> +++ b/drivers/net/ethernet/broadcom/bgmac.h
> @@ -474,7 +474,6 @@ struct bgmac {
>
> struct device *dev;
> struct device *dma_dev;
> -   unsigned char mac_addr[ETH_ALEN];
> u32 feature_flags;
>
> struct net_device *net_dev;
> --
> 2.11.0
>
>

[PATCH net] net: phy: Initialize mdio clock at probe function

[Jon Mason]

From: Yendapally Reddy Dhananjaya Reddy <yendapally.re...@broadcom.com>

USB PHYs need the MDIO clock divisor enabled earlier to work.
Initialize mdio clock divisor in probe function. The ext bus
bit available in the same register will be used by mdio mux
to enable external mdio.

Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.re...@broadcom.com>
Fixes: ddc24ae1 ("net: phy: Broadcom iProc MDIO bus driver")
Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/phy/mdio-bcm-iproc.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mdio-bcm-iproc.c b/drivers/net/phy/mdio-bcm-iproc.c
index c0b4e65..46fe1ae 100644
--- a/drivers/net/phy/mdio-bcm-iproc.c
+++ b/drivers/net/phy/mdio-bcm-iproc.c
@@ -81,8 +81,6 @@ static int iproc_mdio_read(struct mii_bus *bus, int phy_id, 
int reg)
if (rc)
return rc;
 
-   iproc_mdio_config_clk(priv->base);
-
/* Prepare the read operation */
cmd = (MII_DATA_TA_VAL << MII_DATA_TA_SHIFT) |
(reg << MII_DATA_RA_SHIFT) |
@@ -112,8 +110,6 @@ static int iproc_mdio_write(struct mii_bus *bus, int phy_id,
if (rc)
return rc;
 
-   iproc_mdio_config_clk(priv->base);
-
/* Prepare the write operation */
cmd = (MII_DATA_TA_VAL << MII_DATA_TA_SHIFT) |
(reg << MII_DATA_RA_SHIFT) |
@@ -163,6 +159,8 @@ static int iproc_mdio_probe(struct platform_device *pdev)
bus->read = iproc_mdio_read;
bus->write = iproc_mdio_write;
 
+   iproc_mdio_config_clk(priv->base);
+
rc = of_mdiobus_register(bus, pdev->dev.of_node);
if (rc) {
dev_err(>dev, "MDIO bus registration failed\n");
-- 
2.7.4

[PATCH net v3 0/2] net: ethernet: bgmac: bug fixes

[Jon Mason]

Changes in v3:
* Reworked the init sequence patch to only remove the device reset if
  the device is actually in reset.  Given that this code doesn't bear
  much resemblance to the original code, I'm changing the author of the
  patch.  This was tested on NS2 SVK.

Changes in v2:
* Reworked the first match to make it more obvious what portions of the
  register were being preserved (Per Rafal Mileki)
* Style change to reorder the function variables in patch 2 (per Sergei
  Shtylyov)


Bug fixes for bgmac driver


Hari Vyas (1):
  net: ethernet: bgmac: mac address change bug

Jon Mason (1):
  net: ethernet: bgmac: init sequence bug

 drivers/net/ethernet/broadcom/bgmac-platform.c | 27 --
 drivers/net/ethernet/broadcom/bgmac.c  |  6 +-
 drivers/net/ethernet/broadcom/bgmac.h  | 16 +++
 3 files changed, 38 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH net-next v2 1/3] net: ethernet: bgmac: use #defines for MAX size

[Jon Mason]

The maximum frame size is really just the standard ethernet frame size
and FCS.  So use those existing defines to make the code a little more
beautiful.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 6d0b5b3..5a518fe 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -402,7 +402,7 @@
 
 #define BGMAC_WEIGHT   64
 
-#define ETHER_MAX_LEN   1518
+#define ETHER_MAX_LEN  (ETH_FRAME_LEN + ETH_FCS_LEN)
 
 /* Feature Flags */
 #define BGMAC_FEAT_TX_MASK_SETUP   BIT(0)
-- 
2.7.4

[PATCH net-next v2 0/3] net: ethernet: bgmac: PM support and clean-ups

[Jon Mason]

Changes in v2:
* Reworked the PM patch with Florian's suggestions


Add code to support Power Management (only tested on NS2), and add some
code clean-ups

Joey Zhong (1):
  net: ethernet: bgmac: driver power manangement

Jon Mason (2):
  net: ethernet: bgmac: use #defines for MAX size
  net: ethernet: bgmac: unify code of the same family

 drivers/net/ethernet/broadcom/bgmac-bcma.c | 64 +++---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 ++
 drivers/net/ethernet/broadcom/bgmac.c  | 51 
 drivers/net/ethernet/broadcom/bgmac.h  |  4 +-
 4 files changed, 116 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH net-next v2 3/3] net: ethernet: bgmac: driver power manangement

[Jon Mason]

From: Joey Zhong <zho...@broadcom.com>

Implement suspend/resume callbacks in the bgmac driver. This makes sure
that we de-initialize and re-initialize the hardware correctly before
entering suspend and when resuming.

Signed-off-by: Joey Zhong <zho...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 +
 drivers/net/ethernet/broadcom/bgmac.c  | 51 ++
 drivers/net/ethernet/broadcom/bgmac.h  |  2 +
 3 files changed, 87 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 2d153f7..3df91e7 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -21,8 +21,12 @@
 #include 
 #include "bgmac.h"
 
+#define NICPM_PADRING_CFG  0x0004
 #define NICPM_IOMUX_CTRL   0x0008
 
+#define NICPM_PADRING_CFG_INIT_VAL 0x7400
+#define NICPM_IOMUX_CTRL_INIT_VAL_AX   0x2188
+
 #define NICPM_IOMUX_CTRL_INIT_VAL  0x3196e000
 #define NICPM_IOMUX_CTRL_SPD_SHIFT 10
 #define NICPM_IOMUX_CTRL_SPD_10M   0
@@ -108,6 +112,10 @@ static void bgmac_nicpm_speed_set(struct net_device 
*net_dev)
if (!bgmac->plat.nicpm_base)
return;
 
+   /* SET RGMII IO CONFIG */
+   writel(NICPM_PADRING_CFG_INIT_VAL,
+  bgmac->plat.nicpm_base + NICPM_PADRING_CFG);
+
val = NICPM_IOMUX_CTRL_INIT_VAL;
switch (bgmac->net_dev->phydev->speed) {
default:
@@ -239,6 +247,31 @@ static int bgmac_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM
+static int bgmac_suspend(struct device *dev)
+{
+   struct bgmac *bgmac = dev_get_drvdata(dev);
+
+   return bgmac_enet_suspend(bgmac);
+}
+
+static int bgmac_resume(struct device *dev)
+{
+   struct bgmac *bgmac = dev_get_drvdata(dev);
+
+   return bgmac_enet_resume(bgmac);
+}
+
+static const struct dev_pm_ops bgmac_pm_ops = {
+   .suspend = bgmac_suspend,
+   .resume = bgmac_resume
+};
+
+#define BGMAC_PM_OPS (_pm_ops)
+#else
+#define BGMAC_PM_OPS NULL
+#endif /* CONFIG_PM */
+
 static const struct of_device_id bgmac_of_enet_match[] = {
{.compatible = "brcm,amac",},
{.compatible = "brcm,nsp-amac",},
@@ -252,6 +285,7 @@ static struct platform_driver bgmac_enet_driver = {
.driver = {
.name  = "bgmac-enet",
.of_match_table = bgmac_of_enet_match,
+   .pm = BGMAC_PM_OPS
},
.probe = bgmac_probe,
.remove = bgmac_remove,
diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index bd549f8..e78c91d 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1478,6 +1478,7 @@ int bgmac_enet_probe(struct bgmac *bgmac)
 
net_dev->irq = bgmac->irq;
SET_NETDEV_DEV(net_dev, bgmac->dev);
+   dev_set_drvdata(bgmac->dev, bgmac);
 
if (!is_valid_ether_addr(bgmac->mac_addr)) {
dev_err(bgmac->dev, "Invalid MAC addr: %pM\n",
@@ -1551,5 +1552,55 @@ void bgmac_enet_remove(struct bgmac *bgmac)
 }
 EXPORT_SYMBOL_GPL(bgmac_enet_remove);
 
+int bgmac_enet_suspend(struct bgmac *bgmac)
+{
+   if (!netif_running(bgmac->net_dev))
+   return 0;
+
+   phy_stop(bgmac->net_dev->phydev);
+
+   netif_stop_queue(bgmac->net_dev);
+
+   napi_disable(>napi);
+
+   netif_tx_lock(bgmac->net_dev);
+   netif_device_detach(bgmac->net_dev);
+   netif_tx_unlock(bgmac->net_dev);
+
+   bgmac_chip_intrs_off(bgmac);
+   bgmac_chip_reset(bgmac);
+   bgmac_dma_cleanup(bgmac);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_suspend);
+
+int bgmac_enet_resume(struct bgmac *bgmac)
+{
+   int rc;
+
+   if (netif_running(bgmac->net_dev))
+   return 0;
+
+   rc = bgmac_dma_init(bgmac);
+   if (rc)
+   return rc;
+
+   bgmac_chip_init(bgmac);
+
+   napi_enable(>napi);
+
+   netif_tx_lock(bgmac->net_dev);
+   netif_device_attach(bgmac->net_dev);
+   netif_tx_unlock(bgmac->net_dev);
+
+   netif_start_queue(bgmac->net_dev);
+
+   phy_start(bgmac->net_dev->phydev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_resume);
+
 MODULE_AUTHOR("Rafał Miłecki");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 5a518fe..741ca27 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -538,6 +538,8 @@ int bgmac_enet_probe(struct bgmac *bgmac);
 void bgmac_enet_remove(struct bgmac *bgmac);
 void bgmac_adjust_link(struct net_device *net_dev);
 int bgmac_phy_

[PATCH net-next v2 2/3] net: ethernet: bgmac: unify code of the same family

[Jon Mason]

BCM471X and BCM535X are of the same family (from what I can derive from
internal documents).  Group them into the case statement together, which
results in more code reuse.

Also, use existing helper variables to make the code a little more
readable too.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 64 +-
 1 file changed, 28 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 5ef60d4..f5c27f4 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -192,36 +192,50 @@ static int bgmac_probe(struct bcma_device *core)
goto err1;
}
 
-   bgmac->has_robosw = !!(core->bus->sprom.boardflags_lo &
-  BGMAC_BFL_ENETROBO);
+   bgmac->has_robosw = !!(sprom->boardflags_lo & BGMAC_BFL_ENETROBO);
if (bgmac->has_robosw)
dev_warn(bgmac->dev, "Support for Roboswitch not 
implemented\n");
 
-   if (core->bus->sprom.boardflags_lo & BGMAC_BFL_ENETADM)
+   if (sprom->boardflags_lo & BGMAC_BFL_ENETADM)
dev_warn(bgmac->dev, "Support for ADMtek ethernet switch not 
implemented\n");
 
/* Feature Flags */
-   switch (core->bus->chipinfo.id) {
+   switch (ci->id) {
+   /* BCM 471X/535X family */
+   case BCMA_CHIP_ID_BCM4716:
+   bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
+   /* fallthrough */
+   case BCMA_CHIP_ID_BCM47162:
+   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
+   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   break;
case BCMA_CHIP_ID_BCM5357:
+   case BCMA_CHIP_ID_BCM53572:
bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47186) {
-   bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+   if (ci->pkg == BCMA_PKG_ID_BCM47188 ||
+   ci->pkg == BCMA_PKG_ID_BCM47186) {
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+   bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
}
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM5358)
+   if (ci->pkg == BCMA_PKG_ID_BCM5358)
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_EPHYRMII;
break;
-   case BCMA_CHIP_ID_BCM53572:
-   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   case BCMA_CHIP_ID_BCM53573:
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
-   bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47188) {
-   bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+   if (core->core_unit == 0) {
+   bgmac->feature_flags |= BGMAC_FEAT_CC4_IF_SW_TYPE;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
+   bgmac->feature_flags |=
+   BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII;
+   } else if (core->core_unit == 1) {
+   bgmac->feature_flags |= BGMAC_FEAT_IRQ_ID_OOB_6;
+   bgmac->feature_flags |= BGMAC_FEAT_CC7_IF_TYPE_RGMII;
}
break;
case BCMA_CHIP_ID_BCM4749:
@@ -229,18 +243,11 @@ static int bgmac_probe(struct bcma_device *core)
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == 10) {
+   if (ci->pkg == 10) {
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
}
break;
-   case BCMA_CHIP_ID_BCM4716:
-   bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-   /* fallthrough */
-   case BCMA_CHIP_ID_BCM47162:
-   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
-   bgmac->feature_fl

[PATCH net v3 2/2] net: ethernet: bgmac: mac address change bug

[Jon Mason]

From: Hari Vyas <ha...@broadcom.com>

ndo_set_mac_address() passes struct sockaddr * as 2nd parameter to
bgmac_set_mac_address() but code assumed u8 *.  This caused two bytes
chopping and the wrong mac address was configured.

Signed-off-by: Hari Vyas <ha...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: 4e209001b86 ("bgmac: write mac address to hardware in 
ndo_set_mac_address")
---
 drivers/net/ethernet/broadcom/bgmac.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index fe88126..bd549f8 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1221,12 +1221,16 @@ static netdev_tx_t bgmac_start_xmit(struct sk_buff *skb,
 static int bgmac_set_mac_address(struct net_device *net_dev, void *addr)
 {
struct bgmac *bgmac = netdev_priv(net_dev);
+   struct sockaddr *sa = addr;
int ret;
 
ret = eth_prepare_mac_addr_change(net_dev, addr);
if (ret < 0)
return ret;
-   bgmac_write_mac_address(bgmac, (u8 *)addr);
+
+   ether_addr_copy(bgmac->mac_addr, sa->sa_data);
+   bgmac_write_mac_address(bgmac, bgmac->mac_addr);
+
eth_commit_mac_addr_change(net_dev, addr);
return 0;
 }
-- 
2.7.4

[PATCH net v3 1/2] net: ethernet: bgmac: init sequence bug

[Jon Mason]

Fix a bug in the 'bgmac' driver init sequence that blind writes for init
sequence where it should preserve most bits other than the ones it is
deliberately manipulating.

The code now checks to see if the adapter needs to be brought out of
reset (where as before it was doing an IDM write to bring it out of
reset regardless of whether it was in reset or not).  Also, removed
unnecessary usleeps (as there is already a read present to flush the
IDM writes).

Signed-off-by: Zac Schroff <zschr...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
Fixes: f6a95a24957 ("net: ethernet: bgmac: Add platform device support")
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 27 --
 drivers/net/ethernet/broadcom/bgmac.h  | 16 +++
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 805e6ed..3aba785 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -51,8 +51,7 @@ static void platform_bgmac_idm_write(struct bgmac *bgmac, u16 
offset, u32 value)
 
 static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 {
-   if ((bgmac_idm_read(bgmac, BCMA_IOCTL) &
-(BCMA_IOCTL_CLK | BCMA_IOCTL_FGC)) != BCMA_IOCTL_CLK)
+   if ((bgmac_idm_read(bgmac, BCMA_IOCTL) & BGMAC_CLK_EN) != BGMAC_CLK_EN)
return false;
if (bgmac_idm_read(bgmac, BCMA_RESET_CTL) & BCMA_RESET_CTL_RESET)
return false;
@@ -61,17 +60,25 @@ static bool platform_bgmac_clk_enabled(struct bgmac *bgmac)
 
 static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
 {
-   bgmac_idm_write(bgmac, BCMA_IOCTL,
-   (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
-   bgmac_idm_read(bgmac, BCMA_IOCTL);
+   u32 val;
 
-   bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
-   bgmac_idm_read(bgmac, BCMA_RESET_CTL);
-   udelay(1);
+   /* The Reset Control register only contains a single bit to show if the
+* controller is currently in reset.  Do a sanity check here, just in
+* case the bootloader happened to leave the device in reset.
+*/
+   val = bgmac_idm_read(bgmac, BCMA_RESET_CTL);
+   if (val) {
+   bgmac_idm_write(bgmac, BCMA_RESET_CTL, 0);
+   bgmac_idm_read(bgmac, BCMA_RESET_CTL);
+   }
 
-   bgmac_idm_write(bgmac, BCMA_IOCTL, (BCMA_IOCTL_CLK | flags));
+   val = bgmac_idm_read(bgmac, BCMA_IOCTL);
+   /* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
+   val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
+BGMAC_ARUSER);
+   val |= BGMAC_CLK_EN;
+   bgmac_idm_write(bgmac, BCMA_IOCTL, val);
bgmac_idm_read(bgmac, BCMA_IOCTL);
-   udelay(1);
 }
 
 static void platform_bgmac_cco_ctl_maskset(struct bgmac *bgmac, u32 offset,
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index ab2db76..6d0b5b3 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -213,6 +213,22 @@
 /* BCMA GMAC core specific IO Control (BCMA_IOCTL) flags */
 #define BGMAC_BCMA_IOCTL_SW_CLKEN  0x0004  /* PHY Clock 
Enable */
 #define BGMAC_BCMA_IOCTL_SW_RESET  0x0008  /* PHY Reset */
+/* The IOCTL values appear to be different in NS, NSP, and NS2, and do not 
match
+ * the values directly above
+ */
+#define BGMAC_CLK_EN   BIT(0)
+#define BGMAC_RESERVED_0   BIT(1)
+#define BGMAC_SOURCE_SYNC_MODE_EN  BIT(2)
+#define BGMAC_DEST_SYNC_MODE_ENBIT(3)
+#define BGMAC_TX_CLK_OUT_INVERT_EN BIT(4)
+#define BGMAC_DIRECT_GMII_MODE BIT(5)
+#define BGMAC_CLK_250_SEL  BIT(6)
+#define BGMAC_AWCACHE  (0xf << 7)
+#define BGMAC_RESERVED_1   (0x1f << 11)
+#define BGMAC_ARCACHE  (0xf << 16)
+#define BGMAC_AWUSER   (0x3f << 20)
+#define BGMAC_ARUSER   (0x3f << 26)
+#define BGMAC_RESERVED BIT(31)
 
 /* BCMA GMAC core specific IO status (BCMA_IOST) flags */
 #define BGMAC_BCMA_IOST_ATTACHED   0x0800
-- 
2.7.4

Re: [PATCH 3/3] net: ethernet: bgmac: driver power manangement

[Jon Mason]

On Fri, Feb 3, 2017 at 9:16 PM, Florian Fainelli <f.faine...@gmail.com> wrote:
> On 02/03/2017 01:39 PM, Jon Mason wrote:
>> From: Joey Zhong <zho...@broadcom.com>
>>
>> Implements suspend/resume, external phy 54810 is assumed
>> to remain powered up during deep-sleep for wake-on-lane.
>
> s/wake-on-lane/Wake-on-LAN, are you positive phy_stop() is not
> suspending the PHY and issuing BMCR_PWRDOWN write?
>
> This also seems incomplete in that, if the device is really configured
> for Wake-on-LAN (through ethtool) you should call
> device_set_wakeup_capable() and then check for device_may_wakeup()
> during suspend or resume to know which part of the suspend/resume
> portion should be done. You could refer to bcmgenet for an example.

After some internal discussion, WOL is not supported by our SVK.  So,
we have no way of testing it.  Given this limitation, I'm removing the
WOL comment until such time as we can actually test the logic.

>>
>> +int bgmac_enet_suspend(struct bgmac *bgmac)
>> +{
>> + netdev_info(bgmac->net_dev, "Suspending\n");
>
> remove that message
>
>> +
>> + if (netif_running(bgmac->net_dev)) {
>> + netif_stop_queue(bgmac->net_dev);
>> +
>> + napi_disable(>napi);
>> +
>> + netif_tx_lock(bgmac->net_dev);
>> + netif_device_detach(bgmac->net_dev);
>> + netif_tx_unlock(bgmac->net_dev);
>> +
>> + bgmac_chip_intrs_off(bgmac);
>> + bgmac_chip_reset(bgmac);
>> + bgmac_dma_cleanup(bgmac);
>> + }
>
> Can you change the indentation to test for netiff_running() first and
> return 0 in that case?
>
>> +
>> + phy_stop(bgmac->net_dev->phydev);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(bgmac_enet_suspend);
>> +
>> +int bgmac_enet_resume(struct bgmac *bgmac)
>> +{
>> + int rc;
>> +
>> + netdev_info(bgmac->net_dev, "Resuming\n");
>
> Same here, this needs to be removed.

Will do this and above.

Thanks,
Jon

>
>> +
>> + phy_start(bgmac->net_dev->phydev);
>> +
>> + if (netif_running(bgmac->net_dev)) {
>> + rc = bgmac_dma_init(bgmac);
>> + if (rc)
>> + return rc;
>> +
>> + bgmac_chip_init(bgmac);
>> +
>> + napi_enable(>napi);
>> +
>> + netif_tx_lock(bgmac->net_dev);
>> + netif_device_attach(bgmac->net_dev);
>> + netif_tx_unlock(bgmac->net_dev);
>> +
>> + netif_start_queue(bgmac->net_dev);
>> + }
> --
> Florian

Re: [PATCH v2 1/2] net: ethernet: bgmac: init sequence bug

[Jon Mason]

On Fri, Feb 3, 2017 at 4:41 PM, Rafał Miłecki <ra...@milecki.pl> wrote:
> On 02/03/2017 10:08 PM, Jon Mason wrote:
>>
>> @@ -61,15 +60,20 @@ static bool platform_bgmac_clk_enabled(struct bgmac
>> *bgmac)
>>
>>  static void platform_bgmac_clk_enable(struct bgmac *bgmac, u32 flags)
>>  {
>> -   bgmac_idm_write(bgmac, BCMA_IOCTL,
>> -   (BCMA_IOCTL_CLK | BCMA_IOCTL_FGC | flags));
>> +   u32 val;
>> +
>> +   val = bgmac_idm_read(bgmac, BCMA_IOCTL);
>> +   /* Some bits of BCMA_IOCTL set by HW/ATF and should not change */
>> +   val |= flags & ~(BGMAC_AWCACHE | BGMAC_ARCACHE | BGMAC_AWUSER |
>> +BGMAC_ARUSER);
>> +   val |= BGMAC_CLK_EN;
>> bgmac_idm_read(bgmac, BCMA_IOCTL);
>
>
> This read was previously following write op most likely to flush it or
> something. I don't think it makes any sense to read after read.

Actually, that is sloppy coding on my part.  It should have a write
prior to the read to match what was there before.

I find it odd that it worked when I tested this patch.  It makes me
wonder if this "modify, reset, modify" series is really necessary
after all.  The docs indicate that writing a 0 to the reset brings it
out of reset.  I do not see any code that puts the HW in reset.  So,
unless the bootloader puts the HW in reset or it is in the reset state
by default, this seems like unnecessary code.  I can add some CYA
logic to read and see if it is in reset, toggle the bit, and then just
do the CLK enable.  Thoughts?

Thanks,
Jon

Re: [PATCH 2/3] net: ethernet: bgmac: unify code of the same family

[Jon Mason]

On Fri, Feb 3, 2017 at 4:48 PM, Rafał Miłecki <ra...@milecki.pl> wrote:
> On 2017-02-03 22:39, Jon Mason wrote:
>>
>> BCM471X and BCM535X are of the same family (from what I can derive from
>> internal documents).  Group them into the case statement together, which
>> results in more code reuse.
>>
>> Also, use existing helper variables to make the code a little more
>> readable too.
>>
>> Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
>
>
> I'd like to review it / test it on few devices. Please give me weekend for
> that.

Yes, please test this as much as you can.  The code move was pretty
innocuous, and those are always the times when it comes back to bite
me.

Thanks,
Jon

[PATCH net-next 0/3] net: ethernet: bgmac: PM support and clean-ups

[Jon Mason]

Add code to support Power Management (only tested on NS2), and add some
code clean-ups

Joey Zhong (1):
  net: ethernet: bgmac: driver power manangement

Jon Mason (2):
  net: ethernet: bgmac: use #defines for MAX size
  net: ethernet: bgmac: unify code of the same family

 drivers/net/ethernet/broadcom/bgmac-bcma.c | 64 +++---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 ++
 drivers/net/ethernet/broadcom/bgmac.c  | 53 +
 drivers/net/ethernet/broadcom/bgmac.h  |  4 +-
 4 files changed, 118 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH 2/3] net: ethernet: bgmac: unify code of the same family

[Jon Mason]

BCM471X and BCM535X are of the same family (from what I can derive from
internal documents).  Group them into the case statement together, which
results in more code reuse.

Also, use existing helper variables to make the code a little more
readable too.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 64 +-
 1 file changed, 28 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c 
b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 5ef60d4..f5c27f4 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -192,36 +192,50 @@ static int bgmac_probe(struct bcma_device *core)
goto err1;
}
 
-   bgmac->has_robosw = !!(core->bus->sprom.boardflags_lo &
-  BGMAC_BFL_ENETROBO);
+   bgmac->has_robosw = !!(sprom->boardflags_lo & BGMAC_BFL_ENETROBO);
if (bgmac->has_robosw)
dev_warn(bgmac->dev, "Support for Roboswitch not 
implemented\n");
 
-   if (core->bus->sprom.boardflags_lo & BGMAC_BFL_ENETADM)
+   if (sprom->boardflags_lo & BGMAC_BFL_ENETADM)
dev_warn(bgmac->dev, "Support for ADMtek ethernet switch not 
implemented\n");
 
/* Feature Flags */
-   switch (core->bus->chipinfo.id) {
+   switch (ci->id) {
+   /* BCM 471X/535X family */
+   case BCMA_CHIP_ID_BCM4716:
+   bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
+   /* fallthrough */
+   case BCMA_CHIP_ID_BCM47162:
+   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
+   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   break;
case BCMA_CHIP_ID_BCM5357:
+   case BCMA_CHIP_ID_BCM53572:
bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47186) {
-   bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+   if (ci->pkg == BCMA_PKG_ID_BCM47188 ||
+   ci->pkg == BCMA_PKG_ID_BCM47186) {
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+   bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
}
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM5358)
+   if (ci->pkg == BCMA_PKG_ID_BCM5358)
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_EPHYRMII;
break;
-   case BCMA_CHIP_ID_BCM53572:
-   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   case BCMA_CHIP_ID_BCM53573:
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
-   bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47188) {
-   bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+   bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+   if (core->core_unit == 0) {
+   bgmac->feature_flags |= BGMAC_FEAT_CC4_IF_SW_TYPE;
+   if (ci->pkg == BCMA_PKG_ID_BCM47189)
+   bgmac->feature_flags |=
+   BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII;
+   } else if (core->core_unit == 1) {
+   bgmac->feature_flags |= BGMAC_FEAT_IRQ_ID_OOB_6;
+   bgmac->feature_flags |= BGMAC_FEAT_CC7_IF_TYPE_RGMII;
}
break;
case BCMA_CHIP_ID_BCM4749:
@@ -229,18 +243,11 @@ static int bgmac_probe(struct bcma_device *core)
bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-   if (core->bus->chipinfo.pkg == 10) {
+   if (ci->pkg == 10) {
bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
}
break;
-   case BCMA_CHIP_ID_BCM4716:
-   bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-   /* fallthrough */
-   case BCMA_CHIP_ID_BCM47162:
-   bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
-   bgmac->feature_fl

[PATCH 1/3] net: ethernet: bgmac: use #defines for MAX size

[Jon Mason]

The maximum frame size is really just the standard ethernet frame size
and FCS.  So use those existing defines to make the code a little more
beautiful.

Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 6d0b5b3..5a518fe 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -402,7 +402,7 @@
 
 #define BGMAC_WEIGHT   64
 
-#define ETHER_MAX_LEN   1518
+#define ETHER_MAX_LEN  (ETH_FRAME_LEN + ETH_FCS_LEN)
 
 /* Feature Flags */
 #define BGMAC_FEAT_TX_MASK_SETUP   BIT(0)
-- 
2.7.4

[PATCH 3/3] net: ethernet: bgmac: driver power manangement

[Jon Mason]

From: Joey Zhong <zho...@broadcom.com>

Implements suspend/resume, external phy 54810 is assumed
to remain powered up during deep-sleep for wake-on-lane.

Signed-off-by: Joey Zhong <zho...@broadcom.com>
Signed-off-by: Jon Mason <jon.ma...@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 +
 drivers/net/ethernet/broadcom/bgmac.c  | 53 ++
 drivers/net/ethernet/broadcom/bgmac.h  |  2 +
 3 files changed, 89 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c 
b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 2d153f7..3df91e7 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -21,8 +21,12 @@
 #include 
 #include "bgmac.h"
 
+#define NICPM_PADRING_CFG  0x0004
 #define NICPM_IOMUX_CTRL   0x0008
 
+#define NICPM_PADRING_CFG_INIT_VAL 0x7400
+#define NICPM_IOMUX_CTRL_INIT_VAL_AX   0x2188
+
 #define NICPM_IOMUX_CTRL_INIT_VAL  0x3196e000
 #define NICPM_IOMUX_CTRL_SPD_SHIFT 10
 #define NICPM_IOMUX_CTRL_SPD_10M   0
@@ -108,6 +112,10 @@ static void bgmac_nicpm_speed_set(struct net_device 
*net_dev)
if (!bgmac->plat.nicpm_base)
return;
 
+   /* SET RGMII IO CONFIG */
+   writel(NICPM_PADRING_CFG_INIT_VAL,
+  bgmac->plat.nicpm_base + NICPM_PADRING_CFG);
+
val = NICPM_IOMUX_CTRL_INIT_VAL;
switch (bgmac->net_dev->phydev->speed) {
default:
@@ -239,6 +247,31 @@ static int bgmac_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM
+static int bgmac_suspend(struct device *dev)
+{
+   struct bgmac *bgmac = dev_get_drvdata(dev);
+
+   return bgmac_enet_suspend(bgmac);
+}
+
+static int bgmac_resume(struct device *dev)
+{
+   struct bgmac *bgmac = dev_get_drvdata(dev);
+
+   return bgmac_enet_resume(bgmac);
+}
+
+static const struct dev_pm_ops bgmac_pm_ops = {
+   .suspend = bgmac_suspend,
+   .resume = bgmac_resume
+};
+
+#define BGMAC_PM_OPS (_pm_ops)
+#else
+#define BGMAC_PM_OPS NULL
+#endif /* CONFIG_PM */
+
 static const struct of_device_id bgmac_of_enet_match[] = {
{.compatible = "brcm,amac",},
{.compatible = "brcm,nsp-amac",},
@@ -252,6 +285,7 @@ static struct platform_driver bgmac_enet_driver = {
.driver = {
.name  = "bgmac-enet",
.of_match_table = bgmac_of_enet_match,
+   .pm = BGMAC_PM_OPS
},
.probe = bgmac_probe,
.remove = bgmac_remove,
diff --git a/drivers/net/ethernet/broadcom/bgmac.c 
b/drivers/net/ethernet/broadcom/bgmac.c
index bd549f8..8d3aada 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1478,6 +1478,7 @@ int bgmac_enet_probe(struct bgmac *bgmac)
 
net_dev->irq = bgmac->irq;
SET_NETDEV_DEV(net_dev, bgmac->dev);
+   dev_set_drvdata(bgmac->dev, bgmac);
 
if (!is_valid_ether_addr(bgmac->mac_addr)) {
dev_err(bgmac->dev, "Invalid MAC addr: %pM\n",
@@ -1551,5 +1552,57 @@ void bgmac_enet_remove(struct bgmac *bgmac)
 }
 EXPORT_SYMBOL_GPL(bgmac_enet_remove);
 
+int bgmac_enet_suspend(struct bgmac *bgmac)
+{
+   netdev_info(bgmac->net_dev, "Suspending\n");
+
+   if (netif_running(bgmac->net_dev)) {
+   netif_stop_queue(bgmac->net_dev);
+
+   napi_disable(>napi);
+
+   netif_tx_lock(bgmac->net_dev);
+   netif_device_detach(bgmac->net_dev);
+   netif_tx_unlock(bgmac->net_dev);
+
+   bgmac_chip_intrs_off(bgmac);
+   bgmac_chip_reset(bgmac);
+   bgmac_dma_cleanup(bgmac);
+   }
+
+   phy_stop(bgmac->net_dev->phydev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_suspend);
+
+int bgmac_enet_resume(struct bgmac *bgmac)
+{
+   int rc;
+
+   netdev_info(bgmac->net_dev, "Resuming\n");
+
+   phy_start(bgmac->net_dev->phydev);
+
+   if (netif_running(bgmac->net_dev)) {
+   rc = bgmac_dma_init(bgmac);
+   if (rc)
+   return rc;
+
+   bgmac_chip_init(bgmac);
+
+   napi_enable(>napi);
+
+   netif_tx_lock(bgmac->net_dev);
+   netif_device_attach(bgmac->net_dev);
+   netif_tx_unlock(bgmac->net_dev);
+
+   netif_start_queue(bgmac->net_dev);
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_resume);
+
 MODULE_AUTHOR("Rafał Miłecki");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/broadcom/bgmac.h 
b/drivers/net/ethernet/broadcom/bgmac.h
index 5a518fe..741ca27 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@