Re: [PATCH] Revert "net/ibm/emac: wrong bit is used for STA control"

[Benjamin Herrenschmidt]

On Fri, 2018-12-07 at 14:20 +1100, Benjamin Herrenschmidt wrote:
>

Apologies for the empty email, not too sure what happened, I did a
resend and the second one worked.

Cheers
Ben.

[PATCH] Revert "net/ibm/emac: wrong bit is used for STA control"

[Benjamin Herrenschmidt]

This reverts commit 624ca9c33c8a853a4a589836e310d776620f4ab9.

This commit is completely bogus. The STACR register has two formats, old
and new, depending on the version of the IP block used. There's a pair of
device-tree properties that can be used to specify the format used:

has-inverted-stacr-oc
has-new-stacr-staopc

What this commit did was to change the bit definition used with the old
parts to match the new parts. This of course breaks the driver on all
the old ones.

Instead, the author should have set the appropriate properties in the
device-tree for the variant used on his board.

Signed-off-by: Benjamin Herrenschmidt 
---

Found while setting up some old ppc440 boxes for test/CI

 drivers/net/ethernet/ibm/emac/emac.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/emac/emac.h 
b/drivers/net/ethernet/ibm/emac/emac.h
index e2f80cc..0d2de6f 100644
--- a/drivers/net/ethernet/ibm/emac/emac.h
+++ b/drivers/net/ethernet/ibm/emac/emac.h
@@ -231,7 +231,7 @@ struct emac_regs {
 #define EMAC_STACR_PHYE0x4000
 #define EMAC_STACR_STAC_MASK   0x3000
 #define EMAC_STACR_STAC_READ   0x1000
-#define EMAC_STACR_STAC_WRITE  0x0800
+#define EMAC_STACR_STAC_WRITE  0x2000
 #define EMAC_STACR_OPBC_MASK   0x0C00
 #define EMAC_STACR_OPBC_50 0x
 #define EMAC_STACR_OPBC_66 0x0400
-- 
2.7.4

[PATCH] Revert "net/ibm/emac: wrong bit is used for STA control"

[Benjamin Herrenschmidt]

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Thu, 2018-03-29 at 09:56 -0400, Sinan Kaya wrote:
> On 3/28/2018 11:55 AM, David Miller wrote:
> > From: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> > Date: Thu, 29 Mar 2018 02:13:16 +1100
> > 
> > > Let's fix all archs, it's way easier than fixing all drivers. Half of
> > > the archs are unused or dead anyway.
> > 
> > Agreed.
> > 
> 
> I pinged most of the maintainers yesterday.
> Which arches do we care about these days?
> I have not been paying attention any other architecture besides arm64.

Thanks for going through that exercise !

Once sparc, s390, microblaze and mips reply, I think we'll have a good
coverage, maybe riscv is to put in that lot too.
 
Cheers,
Ben.

> 
> arch  status  detail
> ---   
> 
> alpha question sent
> arc   question sent   ys...@users.sourceforge.jp will fix it.
> arm   no issues
> arm64 no issues
> blackfin  question sent   about to be removed
> c6x   question sent
> cris  question sent
> frv
> h8300 question sent
> hexagon   question sent
> ia64  no issues   confirmed by Tony Luck
> m32r
> m68k  question sent
> metag
> microblazequestion sent
> mips  question sent
> mn10300   question sent
> nios2 question sent
> openrisc  no issues   sho...@gmail.com says should no issues
> pariscno issues   grantgrund...@gmail.com says 
> most probably no problem but still looking
> powerpc   no issues
> riscv question sent
> s390  question sent
> score question sent
> shquestion sent
> sparc question sent
> tile  question sent
> unicore32 question sent
> x86   no issues
> xtensaquestion sent
> 
> 

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Thu, 2018-03-29 at 02:23 +1000, Nicholas Piggin wrote:
> On Wed, 28 Mar 2018 11:55:09 -0400 (EDT)
> David Miller <da...@davemloft.net> wrote:
> 
> > From: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> > Date: Thu, 29 Mar 2018 02:13:16 +1100
> > 
> > > Let's fix all archs, it's way easier than fixing all drivers. Half of
> > > the archs are unused or dead anyway.  
> > 
> > Agreed.
> 
> While we're making decrees here, can we do something about mmiowb?
> The semantics are basically indecipherable.

I was going to tackle that next :-)

>   This is a variation on the mandatory write barrier that causes writes to 
> weakly
>   ordered I/O regions to be partially ordered.  Its effects may go beyond the
>   CPU->Hardware interface and actually affect the hardware at some level.
> 
> How can a driver writer possibly get that right?
> 
> IIRC it was added for some big ia64 system that was really expensive
> to implement the proper wmb() semantics on. So wmb() semantics were
> quietly downgraded, then the subsequently broken drivers they cared
> about were fixed by adding the stronger mmiowb().
> 
> What should have happened was wmb and writel remained correct, sane, and
> expensive, and they add an mmio_wmb() to order MMIO stores made by the
> writel_relaxed accessors, then use that to speed up the few drivers they
> care about.
> 
> Now that ia64 doesn't matter too much, can we deprecate mmiowb and just
> make wmb ordering talk about stores to the device, not to some
> intermediate stage of the interconnect where it can be subsequently
> reordered wrt the device? Drivers can be converted back to using wmb
> or writel gradually.

I was under the impression that mmiowb was specifically about ordering
writel's with a subsequent spin_unlock, without it, MMIOs from
different CPUs (within the same lock) would still arrive OO.

If that's indeed the case, I would suggest ia64 switches to a similar
per-cpu flag trick powerpc uses.

Cheers,
Ben.

> Thanks,
> Nick

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Wed, 2018-03-28 at 07:41 -0400, ok...@codeaurora.org wrote:
> Yes, we want to get there indeed. It is because of some arch not 
> implementing writel properly. Maintainers want to play safe.
> 
> That is why I asked if IA64 and other well known archs follow the 
> strongly ordered rule at this moment like PPC and ARM.
> 
> Or should we go and inform every arch about this before yanking wmb()?
> 
> Maintainers are afraid of introducing a regression.

Let's fix all archs, it's way easier than fixing all drivers. Half of
the archs are unused or dead anyway.

> > 
> > The above code makes no sense, and just looks stupid to me. It also
> > generates pointlessly bad code on x86, so it's bad there too.
> > 
> > Linus

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Wed, 2018-03-28 at 11:30 +, David Laight wrote:
> From: Benjamin Herrenschmidt
> > Sent: 28 March 2018 10:56
> 
> ...
> > For example, let's say I have a device with a reset bit and the spec
> > says the reset bit needs to be set for at least 10us.
> > 
> > This is wrong:
> > 
> > writel(1, RESET_REG);
> > usleep(10);
> > writel(0, RESET_REG);
> > 
> > Because of write posting, the first write might arrive to the device
> > right before the second one.
> > 
> > The typical "fix" is to turn that into:
> > 
> > writel(1, RESET_REG);
> > readl(RESET_REG); /* Flush posted writes */
> 
> Would a writel(1, RESET_REG) here provide enough synchronsiation?

Probably yes. It's one of those things where you try to deal with the
fact that 90% of driver writers barely understand the basic stuff and
so you need the "default" accessors to be hardened as much as possible.

We still need to get a reasonably definition of the semantics of the
relaxed ones vs. WC memory but let's get through that exercise first
and hopefully for the last time.
 
> > usleep(10);
> > writel(0, RESET_REG);
> > 
> > *However* the issue here, at least on power, is that the CPU can issue
> > that readl but doesn't necessarily wait for it to complete (ie, the
> > data to return), before proceeding to the usleep. Now a usleep contains
> > a bunch of loads and stores and is probably fine, but a udelay which
> > just loops on the timebase may not be.
> > 
> > Thus we may still violate the timing requirement.
> 
> I've seem that sort of code (with udelay() and no read back) quite often.
> How many were in linux I don't know.
> 
> For small delays I usually fix it by repeated writes (of the same value)
> to the device register. That can guarantee very short intervals.

As long as you know the bus frequency...

> The only time I've actually seen buffered writes break timing was
> between a 286 and an 8859 interrupt controller.

:-)

The problem for me is not so much what I've seen, I realize that most
of the issues we are talking about are the kind that will hit once in a
thousand times or less.

But we *can* reason about them in a way that can effectively prevent
the problem completely and when your cluster has 1 machine, 1/1000
starts becoming significant.

These days the vast majority of IO devices either are 99% DMA driven so
that a bit of overhead on MMIOs is irrelevant, or have one fast path
(low latency IB etc...) that needs some finer control, and the rest is
all setup which can be paranoid at will.

So I think we should aim for the "default" accessors most people use to
be as hadened as we can think of. I favor correctness over performance
in all cases. But then we also define a reasonable semantic for the
relaxed ones (well, we sort-of do have one, we might have to make it a
bit more precise in some areas) that allows the few MMIO fast path that
care to be optimized.

> If you wrote to the mask then enabled interrupts the first IACK cycle
> could be too close to write and break the cycle recovery time.
> That clobbered most of the interrupt controller registers.
> That probably affected every 286 board ever built!
> Not sure how much software added the required extra bus cycle.
> 
> > What we did inside readl, with the twi;isync sequence (which basically
> > means, trap on return value with "trap never" as a condition, followed
> > by isync that ensures all excpetion conditions are resolved), is force
> > the CPU to "consume" the data from the read before moving on.
> > 
> > This effectively makes readl fully synchronous (we would probably avoid
> > that if we were to implement a readl_relaxed).
> 
> I've always wondered exactly what the twi;isync were for - always seemed
> very heavy handed for most mmio reads.
> Particularly if you are doing mmio reads from a data fifo.

If you do that you should use the "s" version of the accessors. Those
will only do the above trick at the end of the access series. Also a
FIFO needs special care about endianness anyway, so you should use
those accessors regardless. (Hint: you never endian swap a FIFO even on
BE on a LE device, unless something's been wired very badly in HW).

> Perhaps there should be a writel_wait() that is allowed to do a read back
> for such code paths?

I think what we have is fine, we just define that the standard
writel/readl as fairly simple and hardened, and we look at providing a
somewhat reasonable set of relaxed variants for optimizing fast path.
We pretty much already are there, we just need to be better at defining
the semantics.

And for the super high perf case, which thankfully is either seldom
(server high perf network stuff) or very arch specific (ARM SoC stuff),
then arch specific driver hacks will always remain the norm.

Cheers,
Ben. 

>   David
> 

Re: Aw: Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Wed, 2018-03-28 at 12:13 +0200, Lino Sanfilippo wrote:
> Hi,
> 
> 
> > 
> > Yeah so that other trick I'm talking about is also used for timing
> > accuracy.
> > 
> > For example, let's say I have a device with a reset bit and the spec
> > says the reset bit needs to be set for at least 10us.
> > 
> > This is wrong:
> > 
> > writel(1, RESET_REG);
> > usleep(10);
> > writel(0, RESET_REG);
> > 
> > Because of write posting, the first write might arrive to the device
> > right before the second one.
> > 
> 
> Does not write posting only concern PCI? This seems to be a different topic. 
> Furthermore
> write posting should not include write reordering...

Nobody's talking about re-ordering and no, write posting is rather
common practice on a whole lot of different busses, not just PCI(e).

Cheers,
Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Wed, 2018-03-28 at 10:07 +0100, Will Deacon wrote:
> 
> For arm/arm64 we guarantee ordering for (1) but not for (2) -- you'd need to
> add an mb() to make it work.
> 
> Do both of these work on power? 

Yes. There's even another quirk, see further down ;-)

> If so, I guess I can make readl even more
> expensive :/ Feels a bit like the tail wagging the dog, though.

Maybe, but then readl is always horribly slow anyway so you may not
necessarily be losing that much.

> Another thing I just realised is that we restrict the barriers we use in
> readl/writel on arm64 so that they don't necessary apply to both loads and
> stores. To be specific:
> 
>writel is ordered against prior writes to memory, but not reads

That could be tricky... You may end up with something that reads before
triggering a DMA and ends up with the post-DMA value ... ugh.

>readl is ordered against subsequent reads of memory, but not writes (but
>note that in example (1) above, the control dependency ensures that).
> 
> If necessary, I could move the barrier in our readl implementation to be
> before the read, then play the control-dependency + instruction-sync (ISB)
> trick that you do on power.

Yeah so that other trick I'm talking about is also used for timing
accuracy.

For example, let's say I have a device with a reset bit and the spec
says the reset bit needs to be set for at least 10us.

This is wrong:

writel(1, RESET_REG);
usleep(10);
writel(0, RESET_REG);

Because of write posting, the first write might arrive to the device
right before the second one.

The typical "fix" is to turn that into:

writel(1, RESET_REG);
readl(RESET_REG); /* Flush posted writes */
usleep(10);
writel(0, RESET_REG);

*However* the issue here, at least on power, is that the CPU can issue
that readl but doesn't necessarily wait for it to complete (ie, the
data to return), before proceeding to the usleep. Now a usleep contains
a bunch of loads and stores and is probably fine, but a udelay which
just loops on the timebase may not be.

Thus we may still violate the timing requirement.

What we did inside readl, with the twi;isync sequence (which basically
means, trap on return value with "trap never" as a condition, followed
by isync that ensures all excpetion conditions are resolved), is force
the CPU to "consume" the data from the read before moving on.

This effectively makes readl fully synchronous (we would probably avoid
that if we were to implement a readl_relaxed).

Cheers,
Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Wed, 2018-03-28 at 09:11 +0200, Arnd Bergmann wrote:
> On Wed, Mar 28, 2018 at 8:56 AM, Benjamin Herrenschmidt
> <b...@kernel.crashing.org> wrote:
> > On Wed, 2018-03-28 at 06:53 +, Linus Torvalds wrote:
> > > On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt 
> > > <b...@kernel.crashing.org> wrote:
> > > That's why in/out were *so* slow, and why nobody uses them any more
> > > (well, the address size limitations and the lack of any remapping of
> > > the address obviously also are a reason).
> > 
> > All true indeed, though a lot of other archs never quite made them
> > fully synchronous, which was another can of worms ... oh well.
> 
> Many architectures have no way of providing PCI compliant semantics
> for outb, as their instruction set and/or bus interconnect lacks a
> method of waiting for completion of an outb.

Yup, that includes powerpc. Note that since POWER8 we don't even
genetate IO space anymore :-)

> In practice, it doesn't seem to matter for any of the devices one would
> encounter these days: very few use I/O space, and those that do don't
> actually rely on the strict ordering. Some architectures (in particular
> s390, but I remember seeing the same thing elsewhere) explicitly
> disallow I/O space access on PCI because of this. On ARM, the typical
> PCI implementations have other problems that are worse than this
> one, so most drivers are fine with the almost-working semantics.

/me cries...

Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Wed, 2018-03-28 at 06:53 +, Linus Torvalds wrote:
> 
> 
> On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crash
> ing.org> wrote:
> > >
> > > Of course, you'd have to be pretty odd to want to start a DMA
> > with a
> > > read anyway - partly exactly because it's bad for performance
> > since
> > > reads will be synchronous and not buffered like a write).
> > 
> > I have bad memories of old adaptec controllers ...
> 
> *Old* adaptec controllers were likely to use the in/out instructions
> for status and command data.
> 
> Those are actually even more ordered than UC reads and writes: the
> in/out instructions are not just fully ordered, but are fully
> *synchronous* on x86. 
> 
> So not just doing accesses in order, but actually waiting for
> everything to drain before they start executing, but they also wait
> for the operation itself to complete (ie "out" will not just queue
> the write, it will then wait for the queue to empty and the write
> data to hit the line).
> 
> That's why in/out were *so* slow, and why nobody uses them any more
> (well, the address size limitations and the lack of any remapping of
> the address obviously also are a reason).

All true indeed, though a lot of other archs never quite made them
fully synchronous, which was another can of worms ... oh well.

As for Adaptec, you might be right, I do remember having cases of old
stuff triggering DMA on reads, it might have been "Mac" variants of
Adaptec using MMIO or something...

Cheers,
Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 20:26 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 6:33 PM, Benjamin Herrenschmidt
> <b...@kernel.crashing.org> wrote:
> > 
> > This is why, I want (with your agreement) to define clearly and once
> > and for all, that the Linux semantics of writel are that it is ordered
> > with previous writes to coherent memory (*)
> 
> Honestly, I think those are the sane semantics. In fact, make it
> "ordered with previous writes" full stop, since it's not only ordered
> wrt previous writes to memory, but also previous writel's.

Of course. It was somewhat a given that it's ordered vs. any previous
MMIO actually, but it doesn't hurt to spell it out once more.

> > Also, can I assume the above ordering with writel() equally applies to
> > readl() or not ?
> > 
> > IE:
> > dma_buf->foo = 1;
> > readl(STUPID_DEVICE_DMA_KICK_ON_READ);
> 
> If that KICK_ON_READ is UC, then that's definitely the case. And
> honestly, status registers like that really should always be UC.
> 
> But if somebody sets the area WC (which is crazy), then I think it
> might be at least debatable. x86 semantics does allow reads to be done
> before previous writes (or, put another way, writes to be buffered -
> the buffers are ordered so writes don't get re-ordered, but reads can
> happen during the buffering).

Right, for now I worry about UC semantics. Once we have nailed that, we
can look at WC, which is a lot more tricky as archs differs more
widely, but one thing at a time.

> But UC accesses are always  done entirely ordered, and honestly, any
> status register that starts a DMA would not make sense any other way.
> 
> Of course, you'd have to be pretty odd to want to start a DMA with a
> read anyway - partly exactly because it's bad for performance since
> reads will be synchronous and not buffered like a write).

I have bad memories of old adaptec controllers ...

That said, I think the above might not be right on ARM if we want to
make it the rule, Will, what do you reckon ?

Cheers,
Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 23:24 -0400, Sinan Kaya wrote:
> On 3/27/2018 10:51 PM, Linus Torvalds wrote:
> > > The discussion at hand is about
> > > 
> > > dma_buffer->foo = 1;/* WB */
> > > writel(KICK, DMA_KICK_REGISTER);/* UC */
> > 
> > Yes. That certainly is ordered on x86. In fact, afaik it's ordered
> > even if that writel() might be of type WC, because that only delays
> > writes, it doesn't move them earlier.
> 
> Now that we clarified x86 myth, Is this guaranteed on all architectures?

If not we need to fix it. It's guaranteed on the "main" ones (arm,
arm64, powerpc, i386, x86_64). We might need to check with other arch
maintainers for the rest.

We really want Linux to provide well defined "sane" semantics for the
basic writel accessors.

Note: We still have open questions about how readl() relates to
surrounding memory accesses. It looks like ARM and powerpc do different
things here.

> We keep getting IA64 exception example. Maybe, this is also corrected since
> then.

I would think ia64 fixed it back when it was all discussed. I was under
the impression all ia64 had "special" was the writel vs. spin_unlock
which requires mmiowb, but maybe that was never completely fixed ?

> Jose Abreu says "I don't know about x86 but arc architecture doesn't
> have a wmb() in the writel() function (in some configs)".

Well, it probably should then.

> As long as we have these exceptions, these wmb() in common drivers is not
> going anywhere and relaxed-arches will continue paying performance penalty.

Well, let's fix them or leave them broken, at this point, it doesn't
matter. We can give all arch maintainers a wakeup call and start making
drivers work based on the documented assumptions.

> I see 15% performance loss on ARM64 servers using Intel i40e network
> drivers and an XL710 adapter due to CPU keeping itself busy doing barriers
> most of the time rather than real work because of sequences like this all over
> the place.
> 
>  dma_buffer->foo = 1;/* WB */
>wmb()
>  writel(KICK, DMA_KICK_REGISTER);/* UC */
>
> I posted several patches last week to remove duplicate barriers on ARM while
> trying to make the code friendly with other architectures.
> 
> Basically changing it to
> 
> dma_buffer->foo = 1;/* WB */
> wmb()
> writel_relaxed(KICK, DMA_KICK_REGISTER);/* UC */
> mmiowb()
> 
> This is a small step in the performance direction until we remove all 
> exceptions.
> 
> https://www.spinics.net/lists/netdev/msg491842.html
> https://www.spinics.net/lists/linux-rdma/msg62434.html
> https://www.spinics.net/lists/arm-kernel/msg642336.html
> 
> Discussion started to move around the need for relaxed API on PPC and then
> why wmb() question came up.

I'm working on the problem of relaxed APIs for powerpc, but we can keep
that orthogonal. As is, today, a wmb() + writel() and a wmb() +
writel_relaxed() on powerpc are identical. So changing them will not
break us.

But I don't see the point of doing that transformation if we can just
get the straying archs fixed. It's not like any of them has a
significant market presence these days anyway.

Cheers,
Ben.

> Sinan
> 

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 16:51 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 3:03 PM, Benjamin Herrenschmidt
> <b...@kernel.crashing.org> wrote:
> > 
> > The discussion at hand is about
> > 
> > dma_buffer->foo = 1;/* WB */
> > writel(KICK, DMA_KICK_REGISTER);/* UC */
> 
> Yes. That certainly is ordered on x86. In fact, afaik it's ordered
> even if that writel() might be of type WC, because that only delays
> writes, it doesn't move them earlier.

Ok so this is our answer ...

 ... snip ... (thanks for the background info !)

> Oh, the above UC case is absoutely guaranteed.

Good.

Then

> The only issue really is that 99.9% of all testing gets done on x86
> unless you look at specific SoC drivers.
> 
> On ARM, for example, there is likely little reason to care about x86
> memory ordering, because there is almost zero driver overlap between
> x86 and ARM.
> 
> *Historically*, the reason for following the x86 IO ordering was
> simply that a lot of architectures used the drivers that were
> developed on x86. The alpha and powerpc workstations were *designed*
> with the x86 IO bus (PCI, then PCIe) and to work with the devices that
> came with it.
> 
> ARM? PCIe is almost irrelevant. For ARM servers, if they ever take
> off, sure. But 99.99% of ARM is about their own SoC's, and so "x86
> test coverage" is simply not an issue.
> 
> How much of an issue is it for Power? Maybe you decide it's not a big deal.
> 
> Then all the above is almost irrelevant.

So the overlap may not be that NIL in practice :-) But even then that
doesn't matter as ARM has been happily implementing the same semantic
you describe above for years, as do we powerpc.

This is why, I want (with your agreement) to define clearly and once
and for all, that the Linux semantics of writel are that it is ordered
with previous writes to coherent memory (*)

This is already what ARM and powerpc provide, from what you say, what
x86 provides, I don't see any reason to keep that badly documented and
have drivers randomly growing useless wmb()'s because they don't think
it works on x86 without them !

Once that's sorted, let's tackle the problem of mmiowb vs. spin_unlock
and the problem of writel_relaxed semantics but as separate issues :-)

Also, can I assume the above ordering with writel() equally applies to
readl() or not ?

IE:
dma_buf->foo = 1;
readl(STUPID_DEVICE_DMA_KICK_ON_READ);

Also works on x86 ? (It does on power, maybe not on ARM).

Cheers,
Ben.

(*) From an Linux API perspective, all of this is only valid if the
memory was allocated by dma_alloc_coherent(). Anything obtained by
dma_map_something() might have been bounced bufferred or might require
extra cache flushes on some architectures, and thus needs
dma_sync_for_{cpu,device} calls.

Cheers,
Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 16:10 +0100, Will Deacon wrote:
> To clarify: are you saying that on x86 you need a wmb() prior to a writel
> if you want that writel to be ordered after prior writes to memory? Is this
> specific to WC memory or some other non-standard attribute?
> 
> The only reason we have wmb() inside writel() on arm, arm64 and power is for
> parity with x86 because Linus (CC'd) wanted architectures to order I/O vs
> memory by default so that it was easier to write portable drivers. The
> performance impact of that implicit barrier is non-trivial, but we want the
> driver portability and I went as far as adding generic _relaxed versions for
> the cases where ordering isn't required. You seem to be suggesting that none
> of this is necessary and drivers would already run into problems on x86 if
> they didn't use wmb() explicitly in conjunction with writel, which I find
> hard to believe and is in direct contradiction with the current Linux I/O
> memory model (modulo the broken example in the dma_*mb section of
> memory-barriers.txt).

Another clarification while we are at it 

All of this only applies to concurrent access by the CPU and the device
to memory allocate with dma_alloc_coherent().

For memory "mapped" into the DMA domain via dma_map_* then an extra
dma_sync_for_* is needed.

In most useful server cases etc... these latter are NOPs, but
architecture without full DMA cache coherency or using swiotlb,
dma_map_* might maintain bounce buffers or play additional cache
flushing tricks.

Cheers,
Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 14:39 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 11:33 AM, Benjamin Herrenschmidt
> <b...@kernel.crashing.org> wrote:
> > 
> > Well, we need to clarify that once and for all, because as I wrote
> > earlier, it was decreed by Linus more than a decade ago that writel
> > would be fully ordered by itself vs. previous memory stores (at least
> > on UC memory).
> 
> Yes.
> 
> So "writel()" needs to be ordered with respect to other writel() uses
> on the same thread. Anything else *will* break drivers. Obviously, the
> drivers may then do magic to say "do write combining etc", but that
> magic will be architecture-specific.
> 
> The other issue is that "writel()" needs to be ordered wrt other CPU's
> doing "writel()" if those writel's are in a spinlocked region.

 .../...

The discussion at hand is about

dma_buffer->foo = 1;/* WB */
writel(KICK, DMA_KICK_REGISTER);/* UC */

(The WC case is something else, let's not mix things up just yet)

IE, a store to normal WB cache memory followed by a writel to a device
which will then DMA from that buffer.

Back in the days, we did require on powerpc a wmb() between these, but
you made the point that x86 didn't and driver writers would never get
that right.

We decided to go conservative, added the necessary barrier inside
writel, so did ARM and it became the norm that writel is also fully
ordered vs. previous stores to memory *by the same CPU* of course (or
protected by the same spinlock).

Now it appears that this wasn't fully understood back then, and some
people are now saying that x86 might not even provide that semantic
always.

So a number (fairly large) of drivers have been adding wmb() in those
case, while others haven't, and it's a mess.

The mess is compounded by the fact that if writel is now defined to
*not* provide that ordering guarantee, then writel_relaxed() is
pointless since all it is defined to relax is precisely the above
ordering guarantee.

So I want to get to the bottom of this once and for all so we can have
well defined and documented semantics and stop having drivers do random
things that may or may not work on some or all architectures (including
x86 !).

Quite note about the spinlock case... In fact this is the only case you
did allow back then to be relaxed. In theory a writel followed by a
spin_unlock requires an mmiowb (which is the only point of that barrier
in fact). This was done because an arch (I think ia64) had a hard time
getting MMIOs from multiple CPUs get in order vs. a lock and required
an expensive access to the PCI host bridge to do so.

Back then, on powerpc, we chose not to allow that relaxing and instead
added code to our writel to set a per-cpu flag which would cause the
next spin_unlock to use a stronger barrier than usual.

We do need to clarify this as well, but let's start with the most basic
one first, there is enough confusion already.

Cheers,
Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 14:54 -0700, Alexander Duyck wrote:
> On Tue, Mar 27, 2018 at 2:35 PM, Benjamin Herrenschmidt
> <b...@kernel.crashing.org> wrote:
> > On Tue, 2018-03-27 at 10:46 -0400, Sinan Kaya wrote:
> > >  combined buffers.
> > > 
> > > Alex:
> > > "Don't bother. I can tell you right now that for x86 you have to have a
> > > wmb() before the writel().
> > 
> > No, this isn't the semantics of writel. You shouldn't need it unless
> > something changed and we need to revisit our complete understanding of
> > *all* MMIO accessor semantics.
> 
> The issue seems to be that there have been two different ways of
> dealing with this. There has historically been a number of different
> drivers that have been carrying this wmb() workaround since something
> like 2002. I get that the semantics for writel might have changed
> since then, but those of us who already have the wmb() in our drivers
> will be very wary of anyone wanting to go through and remove them
> since writel is supposed to be "good enough". I would much rather err
> on the side of caution here.
> 
> I view the wmb() + writel_relaxed() as more of a driver owning and
> handling this itself. Besides in the Intel Ethernet driver case it is
> better performance as our wmb() placement for us also provides a
> secondary barrier so we don't need to add a separate smp_wmb() to deal
> with a potential race we have with the Tx cleanup.
> 
> > At least for UC space, it has always been accepted (and enforced) that
> > writel would not require any other barrier to order vs. previous stores
> > to memory.
> 
> So the one thing I would question here is if this is UC vs UC or if
> this extends to other types as well? So for x86 we could find
> references to Write Combining being flushed by a write to UC memory,
> however I have yet to find a clear explanation of what a write to UC
> does to WB. 

Well, this is the standard write memory + trigger DMA case, the one
specific case for which Linus was adamant we don't need another barrier
 back then ...

> My personal inclination would be to err on the side of
> caution.

Which means writel_relaxed is now pointless ?

We need clear semantics here. In this case the "side of caution" means
we are randomly doing things not understanding what really happens and
that makes me *more* nervous.

>  I just don't want us going through and removing the wmb()
> calls because it "should" work. I would want to know for certain it
> will work.

We need to know for certain anyway. Otherwise, all the drivers that do
not have wmb's are potentially broken.

So I dont agree with the status quo.

We need to establish precisely what x86 does, decide what we want the
semantic of writel to be, and implement things accordingly.

Ben.

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 10:46 -0400, Sinan Kaya wrote:
>  combined buffers.
> 
> Alex:
> "Don't bother. I can tell you right now that for x86 you have to have a
> wmb() before the writel().

No, this isn't the semantics of writel. You shouldn't need it unless
something changed and we need to revisit our complete understanding of
*all* MMIO accessor semantics.

At least for UC space, it has always been accepted (and enforced) that
writel would not require any other barrier to order vs. previous stores
to memory.

> Based on the comment in
> (https://www.spinics.net/lists/linux-rdma/msg62666.html):
> Replacing wmb() + writel() with wmb() + writel_relaxed() will work on
> PPC, it will just not give you a benefit today.
> 
> I say the patch set stays. This gives benefit on ARM, and has no
> effect on x86 and PowerPC. If you want to look at trying to optimize
> things further on PowerPC and such then go for it in terms of trying
> to implement the writel_relaxed(). Otherwise I say we call the ARM
> goodness a win and don't get ourselves too wrapped up in trying to fix
> this for all architectures."

Re: RFC on writel and writel_relaxed

[Benjamin Herrenschmidt]

On Tue, 2018-03-27 at 11:54 -0700, Alexander Duyck wrote:
> As far as I know the code has been this way for a while, something
> like 2002, when the barrier was already present in e1000. However
> there it was calling out weakly ordered models "such as IA-64". Since
> then pretty much all the hardware based network drivers at this point
> have similar code floating around with wmb() in place to prevent
> issues on weak ordered memory systems.
> 
> So in any case we still need to be careful as there are architectures
> that are depending on this even if they might not be x86. :-/

Well, we need to clarify that once and for all, because as I wrote
earlier, it was decreed by Linus more than a decade ago that writel
would be fully ordered by itself vs. previous memory stores (at least
on UC memory).

This is why we added sync's to writel on powerpc and later ARM added
similar barriers to theirs.

This is also why writel_relaxed was added (though much later), since
what writel_relaxed does is to life that specific requirement.

IE. If what you say is true and wmb() is needed on x86, then
writel_relaxed is now completely useless...

Ben.

Re: [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[Benjamin Herrenschmidt]

On Wed, 2017-12-20 at 22:07 +0100, Christian Lamparter wrote:
> 
> > will read this and say "Oh the function tests against these weird
> > PHY_MODE_* aliases, but the helper function phy_interface_mode_is_rgmii()
> > tests against PHY_INTERFACE_MODE_*, what is going on?"
> > 
> > I hate to do this to you, but please get rid of these confusing and
> > completely unnecessary PHY_MODE_* CPP aliases first, and just use the
> > proper PHY_INTERFACE_MODE_* values consistently.
> 
> Yeah, I can do that. no problem. 
> 
> Question is, should I also replace the rgmii_mode_name() with phy_modes() too?
> 
> The only user of rgmii_mode_name() is this notice printk in rgmii_attach():
> 

Yup, this is all pre-hisorical gunk, feel free to replace it.

Cheers,
Ben.

Re: [PATCH] ibmveth: Kernel crash LSO offload flag toggle

[Benjamin Herrenschmidt]

On Wed, 2017-11-15 at 10:45 -0600, Bryant G. Ly wrote:
> This patch just closes the window, bad things can still happen. I wanted to 
> leave it
> up to the people who actively develop in ibmveth to close the window, since 
> introducing
> a lock can be expensive in tx. 

You don't need to instroduce a lock. The network stack already have a
per-queue lock, you just use the existing one.

Look at what I did in sungem or ftgmac100 with the reset task, those
are fairly simple drivers and should illustrate the technique.

Cheers,
Ben.

Re: [PATCH] ibmveth: Kernel crash LSO offload flag toggle

[Benjamin Herrenschmidt]

On Wed, 2017-11-15 at 13:47 +1100, Daniel Axtens wrote:
> Hi Bryant,
> 
> This looks a bit better, but...
> 
> > The following patch ensures that the bounce_buffer is not null
> > prior to using it within skb_copy_from_linear_data.
> 
> How would this occur?
> 
> Looking at ibmveth.c, I see bounce_buffer being freed in ibmveth_close()
> and allocated in ibmveth_open() only. If allocation fails, the whole
> opening of the device fails with -ENOMEM.
> 
> It seems your test case - changing TSO - causes ibmveth_set_tso() to
> cause an adaptor restart - an ibmveth_close(dev) and then an
> ibmveth_open(dev). Is this happening in parallel with an out of memory
> condition - is the memory allocation failing?
> 
> Alternatively, could it be the case that you're closing the device while
> packets are in flight, and then trying to use a bounce_buffer that's
> been freed elsewhere? Do you need to decouple memory freeing from
> ibmveth_close?

Hrm, you should at least stop the tx queue and NAPI (and synchronize)
while doing a reset. A lot of drivers, rather than doing close/open
(which does subtly different things) tend to instead fire a work queue
(often called reset_task) which does the job (and uses the same lower
level helpers as open/close to free/allocate the rings etc...).


> > The problem can be recreated toggling on/off Large send offload.
> > 
> > The following script when run (along with some iperf traffic recreates the
> > crash within 5-10 mins or so).
> > 
> > while true
> > do
> > ethtool -k ibmveth0 | grep tcp-segmentation-offload
> > ethtool -K ibmveth0 tso off
> > ethtool -k ibmveth0 | grep tcp-segmentation-offload
> > ethtool -K ibmveth0 tso on
> > done
> > 
> > Note: This issue happens the very first time largsesend offload is
> > turned off too (but the above script recreates the issue all the times)
> > 
> > [76563.914173] Unable to handle kernel paging request for data at address 
> > 0x
> > [76563.914197] Faulting instruction address: 0xc0063940
> > [76563.914205] Oops: Kernel access of bad area, sig: 11 [#1]
> > [76563.914210] SMP NR_CPUS=2048 NUMA pSeries
> > [76563.914217] Modules linked in: rpadlpar_io rpaphp dccp_diag dccp 
> > tcp_diag udp_diag inet_diag unix_diag af_packet_diag netlink_diag nls_utf8 
> > isofs binfmt_misc pseries_rng rtc_generic autofs4 ibmvfc scsi_transport_fc 
> > ibmvscsi ibmveth
> > [76563.914251] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.4.0-34-generic 
> > #53-Ubuntu
> 
> ^--- yikes!
> 
> There are relevant changes to this area since 4.4:
> 2c42bf4b4317 ("ibmveth: check return of skb_linearize in ibmveth_start_xmit")
> 66aa0678efc2 ("ibmveth: Support to enable LSO/CSO for Trunk VEA.")
> 
> Does this crash occurs on a more recent kernel?
> 
> > diff --git a/drivers/net/ethernet/ibm/ibmveth.c 
> > b/drivers/net/ethernet/ibm/ibmveth.c
> > index f210398200ece..1d29b1649118d 100644
> > --- a/drivers/net/ethernet/ibm/ibmveth.c
> > +++ b/drivers/net/ethernet/ibm/ibmveth.c
> > @@ -1092,8 +1092,14 @@ static netdev_tx_t ibmveth_start_xmit(struct sk_buff 
> > *skb,
> >  */
> > if (force_bounce || (!skb_is_nonlinear(skb) &&
> > (skb->len < tx_copybreak))) {
> > -   skb_copy_from_linear_data(skb, adapter->bounce_buffer,
> > - skb->len);
> > +   if (adapter->bounce_buffer) {
> > +   skb_copy_from_linear_data(skb, adapter->bounce_buffer,
> > + skb->len);
> > +   } else {
> > +   adapter->tx_send_failed++;
> > +   netdev->stats.tx_dropped++;
> > +   goto out;
> 
> Finally, as I alluded to at the top of this message, isn't the
> disappearance of the bounce-buffer a pretty serious issue? As I
> understand it, it means your data structures are now inconsistent. Do
> you need to - at the least - be more chatty here?
> 
> Regards,
> Daniel
> > +   }
> >  
> > descs[0].fields.flags_len = desc_flags | skb->len;
> > descs[0].fields.address = adapter->bounce_buffer_dma;
> > -- 
> > 2.13.6 (Apple Git-96)

Re: [PATCH v2] net: ftgmac100: Request clock and set speed

[Benjamin Herrenschmidt]

On Thu, 2017-10-12 at 11:32 +0800, Joel Stanley wrote:
> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher. This patch
> configures a 100MHz clock if the system has a direct-attached
> PHY, or 25MHz if the system is running NC-SI which is limited to 100MHz.
> 
> There appear to be no other upstream users of the FTGMAC100 driver so it
> is hard to know the clocking requirements of other platforms. Therefore
> a conservative approach was taken with enabling clocks. If the platform
> is not ASPEED, both requesting the clock and configuring the speed is
> skipped.

We might still be able to check the PHY capabilities and it might also
be possible to do the live change as you were doing previously but it
needs testing. So I'm ok with this patch for now, and later I might be
able to try the live change option on the eval board (provided I still
have one) when I come back to Australia.

> Signed-off-by: Joel Stanley 
> ---
> Andrew, as I'm travelling can you please test this on the evb and a
> palmetto? Use my wip/aspeed-v4.14-clk branch, or OpenBMC's dev-4.13.
> 
> David, please wait for Andrew's tested-by before applying.
> 
> Cheers!
> 
> v2:
>  - only touch the clocks on Aspeed platforms
>  - unconditionally call clk_unprepare_disable
> 
>  drivers/net/ethernet/faraday/ftgmac100.c | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 9ed8e4b81530..cd352bf41da1 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -21,6 +21,7 @@
>  
>  #define pr_fmt(fmt)  KBUILD_MODNAME ": " fmt
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -59,6 +60,9 @@
>  /* Min number of tx ring entries before stopping queue */
>  #define TX_THRESHOLD (MAX_SKB_FRAGS + 1)
>  
> +#define FTGMAC_100MHZ1
> +#define FTGMAC_25MHZ 2500
> +
>  struct ftgmac100 {
>   /* Registers */
>   struct resource *res;
> @@ -96,6 +100,7 @@ struct ftgmac100 {
>   struct napi_struct napi;
>   struct work_struct reset_task;
>   struct mii_bus *mii_bus;
> + struct clk *clk;
>  
>   /* Link management */
>   int cur_speed;
> @@ -1734,6 +1739,22 @@ static void ftgmac100_ncsi_handler(struct ncsi_dev *nd)
>   nd->link_up ? "up" : "down");
>  }
>  
> +static void ftgmac100_setup_clk(struct ftgmac100_priv *priv)
> +{
> + priv->clk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(priv->clk))
> + return;
> +
> + clk_prepare_enable(priv->clk);
> +
> + /* Aspeed specifies a 100MHz clock is required for up to
> +  * 1000Mbit link speeds. As NCSI is limited to 100Mbit, 25MHz
> +  * is sufficient
> +  */
> + clk_set_rate(priv->clk, priv->is_ncsi ? FTGMAC_25MHZ :
> + FTGMAC_100MHZ);
> +}
> +
>  static int ftgmac100_probe(struct platform_device *pdev)
>  {
>   struct resource *res;
> @@ -1830,6 +1851,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
>   goto err_setup_mdio;
>   }
>  
> + if (priv->is_aspeed)
> + ftgmac100_setup_clk(priv);
> +
>   /* Default ring sizes */
>   priv->rx_q_entries = priv->new_rx_q_entries = DEF_RX_QUEUE_ENTRIES;
>   priv->tx_q_entries = priv->new_tx_q_entries = DEF_TX_QUEUE_ENTRIES;
> @@ -1883,6 +1907,8 @@ static int ftgmac100_remove(struct platform_device 
> *pdev)
>  
>   unregister_netdev(netdev);
>  
> + clk_disable_unprepare(priv->clk);
> +
>   /* There's a small chance the reset task will have been re-queued,
>* during stop, make sure it's gone before we free the structure.
>*/

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Benjamin Herrenschmidt]

On Tue, 2017-10-10 at 15:19 +1030, Joel Stanley wrote:
> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher.

Did you try "live" changing by either using ethtool or plugging into
switches/hubs at different speed ?

Also this is aspeed'isms, we should probably keep that under an
is_aspeed test.

My assumption is that we wouldn't bother, and just leave the freq
set based on whether there's a physical gigabit capable connection or
not (ie, real gigabit PHY vs. NC-SI really). But if it can help save a
few milliwatts..

> Signed-off-by: Joel Stanley 
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 9ed8e4b81530..870ebd857978 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -21,6 +21,7 @@
>  
>  #define pr_fmt(fmt)  KBUILD_MODNAME ": " fmt
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -59,6 +60,9 @@
>  /* Min number of tx ring entries before stopping queue */
>  #define TX_THRESHOLD (MAX_SKB_FRAGS + 1)
>  
> +#define FTGMAC_100MHZ1
> +#define FTGMAC_25MHZ 2500
> +
>  struct ftgmac100 {
>   /* Registers */
>   struct resource *res;
> @@ -96,6 +100,7 @@ struct ftgmac100 {
>   struct napi_struct napi;
>   struct work_struct reset_task;
>   struct mii_bus *mii_bus;
> + struct clk *clk;
>  
>   /* Link management */
>   int cur_speed;
> @@ -142,18 +147,22 @@ static int ftgmac100_reset_mac(struct ftgmac100 *priv, 
> u32 maccr)
>  static int ftgmac100_reset_and_config_mac(struct ftgmac100 *priv)
>  {
>   u32 maccr = 0;
> + int freq = 0;
>  
>   switch (priv->cur_speed) {
>   case SPEED_10:
>   case 0: /* no link */
> + freq = FTGMAC_25MHZ;
>   break;
>  
>   case SPEED_100:
>   maccr |= FTGMAC100_MACCR_FAST_MODE;
> + freq = FTGMAC_25MHZ;
>   break;
>  
>   case SPEED_1000:
>   maccr |= FTGMAC100_MACCR_GIGA_MODE;
> + freq = FTGMAC_100MHZ;
>   break;
>   default:
>   netdev_err(priv->netdev, "Unknown speed %d !\n",
> @@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct 
> ftgmac100 *priv)
>   break;
>   }
>  
> + if (freq && priv->clk)
> + clk_set_rate(priv->clk, freq);
> +
>   /* (Re)initialize the queue pointers */
>   priv->rx_pointer = 0;
>   priv->tx_clean_pointer = 0;
> @@ -1775,6 +1787,13 @@ static int ftgmac100_probe(struct platform_device 
> *pdev)
>   priv->dev = >dev;
>   INIT_WORK(>reset_task, ftgmac100_reset_task);
>  
> + /* Enable clock if present */
> + priv->clk = devm_clk_get(>dev, NULL);
> + if (!IS_ERR(priv->clk))
> + clk_prepare_enable(priv->clk);
> + else
> + priv->clk = NULL;
> +
>   /* map io memory */
>   priv->res = request_mem_region(res->start, resource_size(res),
>  dev_name(>dev));
> @@ -1883,6 +1902,9 @@ static int ftgmac100_remove(struct platform_device 
> *pdev)
>  
>   unregister_netdev(netdev);
>  
> + if (priv->clk)
> + clk_disable_unprepare(priv->clk);
> +
>   /* There's a small chance the reset task will have been re-queued,
>* during stop, make sure it's gone before we free the structure.
>*/

Re: [PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for OpenBlocks 600

[Benjamin Herrenschmidt]

On Fri, 2017-09-01 at 17:35 -0700, Florian Fainelli wrote:
> On 08/31/2017 09:44 PM, Benjamin Herrenschmidt wrote:
> > The vendor patches initialize those registers to get the
> > PHY working properly.
> > 
> > Sadly I don't have that PHY spec and whatever Broadcom PHY
> > code we already have don't seem to document these two shadow
> > registers (unless I miscalculated the address) so I'm keeping
> > this as "vendor magic for that board". The vendor has long
> > abandoned that product, but I find it handy to test ppc405
> > kernels and so would like to keep it alive upstream :-)
> > 
> > Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> > ---
> > 
> > Note: Ideally, the whole driver should switch over to the
> > generic PHY layer. However this is a much bigger undertaking
> > which requires access to a bunch of HW to test, and for which
> > I have neither the time nor the HW available these days.
> 
> Yes it sure does and the function names are so close, it is almost
> irresistible not to do it.

I think there's some common ancestry :-)

That said, I'm weary of doing it without proper testing, especially
those old cell blades which I'm not sure I still have a functional
one, and whatever is using gpcs...

Cheers,
Ben.

> 
> > 
> > (Some of the HW could prove hard to find ...)
> > ---
> >  drivers/net/ethernet/ibm/emac/phy.c | 30 ++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/ibm/emac/phy.c 
> > b/drivers/net/ethernet/ibm/emac/phy.c
> > index 35865d05fccd..daa10de542fb 100644
> > --- a/drivers/net/ethernet/ibm/emac/phy.c
> > +++ b/drivers/net/ethernet/ibm/emac/phy.c
> > @@ -24,6 +24,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include "emac.h"
> >  #include "phy.h"
> > @@ -363,6 +364,34 @@ static struct mii_phy_def bcm5248_phy_def = {
> > .ops= _phy_ops
> >  };
> >  
> > +static int bcm5482_init(struct mii_phy *phy)
> > +{
> > +   if (!of_machine_is_compatible("plathome,obs600"))
> > +   return 0;
> 
> You can probably include brcmphy.h and pull the definition for at least
> 0x1c: MII_BCM54XX_SHD

Yup.

> > +
> > +   /* Magic inits from vendor original patches */
> > +   phy_write(phy, 0x1c, 0xa410);
> 
> What you are doing here is write to shadow register 9 (9 << 10) which is
> the LED control register, and making the activity LED be driven on
> activity/link as opposed to just activity. So this can probably be
> written as:

Ok so I really don't *need* that in fact.


>   phy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |
> MII_BCM54XX_SHD_VAL(9) | MII_BCM54XX_SHD_DATA(BIT(4));
> 
> > +   phy_write(phy, 0x1c, 0x8804);
> 
> And here you are writing to the spare control 1 register and setting bit
> 2 (which appears reserved but this is not clear) which would be enabling
> the activity LED for 10BaseT or no link which can be written as:
> 
>   phy_write(phy, MII_BCM54XX_SHD, MII_BCM54XX_SHD_WRITE |
> MII_BCM54XX_SHD_VAL(2) | MII_BCM4XX_SHD_DATA(BIT(2));
> 
> So basically you are touching registers that only affect LED
> configuration and should not be doing anything else...

I wonder if I need to bother at all then. I was worried it was related
to actual function of the device, but if it's just LEDs, I think I may
as well just drop it.

> > +
> > +   return 0;
> > +}
> > +
> > +static const struct mii_phy_ops bcm5482_phy_ops = {
> > +   .init   = bcm5482_init,
> > +   .setup_aneg = genmii_setup_aneg,
> > +   .setup_forced   = genmii_setup_forced,
> > +   .poll_link  = genmii_poll_link,
> > +   .read_link  = genmii_read_link
> > +};
> > +
> > +static struct mii_phy_def bcm5482_phy_def = {
> > +
> > +   .phy_id = 0x0143bcb0,
> > +   .phy_id_mask= 0x0ff0,
> > +   .name   = "BCM5482 Gigabit Ethernet",
> > +   .ops= _phy_ops
> > +};
> > +
> >  static int m88e_init(struct mii_phy *phy)
> >  {
> > pr_debug("%s: Marvell 88E Ethernet\n", __func__);
> > @@ -499,6 +528,7 @@ static struct mii_phy_def *mii_phy_table[] = {
> > _phy_def,
> > _phy_def,
> > _phy_def,
> > +   _phy_def,
> > _phy_def,
> > _phy_def,
> > _phy_def,
> > 
> 
> 

[PATCH] net: ethernet: ibm-emac: Add 5482 PHY init for OpenBlocks 600

[Benjamin Herrenschmidt]

The vendor patches initialize those registers to get the
PHY working properly.

Sadly I don't have that PHY spec and whatever Broadcom PHY
code we already have don't seem to document these two shadow
registers (unless I miscalculated the address) so I'm keeping
this as "vendor magic for that board". The vendor has long
abandoned that product, but I find it handy to test ppc405
kernels and so would like to keep it alive upstream :-)

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---

Note: Ideally, the whole driver should switch over to the
generic PHY layer. However this is a much bigger undertaking
which requires access to a bunch of HW to test, and for which
I have neither the time nor the HW available these days.

(Some of the HW could prove hard to find ...)
---
 drivers/net/ethernet/ibm/emac/phy.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/ibm/emac/phy.c 
b/drivers/net/ethernet/ibm/emac/phy.c
index 35865d05fccd..daa10de542fb 100644
--- a/drivers/net/ethernet/ibm/emac/phy.c
+++ b/drivers/net/ethernet/ibm/emac/phy.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "emac.h"
 #include "phy.h"
@@ -363,6 +364,34 @@ static struct mii_phy_def bcm5248_phy_def = {
.ops= _phy_ops
 };
 
+static int bcm5482_init(struct mii_phy *phy)
+{
+   if (!of_machine_is_compatible("plathome,obs600"))
+   return 0;
+
+   /* Magic inits from vendor original patches */
+   phy_write(phy, 0x1c, 0xa410);
+   phy_write(phy, 0x1c, 0x8804);
+
+   return 0;
+}
+
+static const struct mii_phy_ops bcm5482_phy_ops = {
+   .init   = bcm5482_init,
+   .setup_aneg = genmii_setup_aneg,
+   .setup_forced   = genmii_setup_forced,
+   .poll_link  = genmii_poll_link,
+   .read_link  = genmii_read_link
+};
+
+static struct mii_phy_def bcm5482_phy_def = {
+
+   .phy_id = 0x0143bcb0,
+   .phy_id_mask= 0x0ff0,
+   .name   = "BCM5482 Gigabit Ethernet",
+   .ops= _phy_ops
+};
+
 static int m88e_init(struct mii_phy *phy)
 {
pr_debug("%s: Marvell 88E Ethernet\n", __func__);
@@ -499,6 +528,7 @@ static struct mii_phy_def *mii_phy_table[] = {
_phy_def,
_phy_def,
_phy_def,
+   _phy_def,
_phy_def,
_phy_def,
_phy_def,
-- 
2.13.5

Re: [PATCH net-next] net/ncsi: Define {add, kill}_vid callbacks for !CONFIG_NET_NCSI

[Benjamin Herrenschmidt]

On Thu, 2017-08-31 at 08:24 -0700, Vernon Mauery wrote:
>  +int ncsi_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
> > +{
> > + return -ENOTTY;
> > +}
> > +int ncsi_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
> > +{
> > + return -ENOTTY;
> > +}
> 
> These should be static functions because they are defined in the header 
> file or you will get multiple symbol definitions.

static inline even or you'll get warning about them being unused iirc.

Cheers,
Ben.

Re: [PATCH] net: ftgmac100: Fix oops in probe on failure to find associated PHY

[Benjamin Herrenschmidt]

;] 
> (bus_add_driver+0x14c/0x268)
>[ 2.81] [<80240ba8>] (bus_add_driver) from [<80243068>] 
> (driver_register+0x88/0x104)
>[ 2.81] [<80243068>] (driver_register) from [<80244a38>] 
> (__platform_driver_register+0x40/0x54)
>[ 2.81] [<80244a38>] (__platform_driver_register) from 
> [<805ab264>] (ftgmac100_driver_init+0x18/0x20)
>[ 2.81] [<805ab264>] (ftgmac100_driver_init) from [<8058ae70>] 
> (do_one_initcall+0xac/0x168)
>[ 2.81] [<8058ae70>] (do_one_initcall) from [<8058b040>] 
> (kernel_init_freeable+0x114/0x1cc)
>[ 2.81] [<8058b040>] (kernel_init_freeable) from [<803fd170>] 
> (kernel_init+0x18/0x104)
>[ 2.81] [<803fd170>] (kernel_init) from [<8000a5e8>] 
> (ret_from_fork+0x14/0x2c)
>[ 2.81] Code: e594205c e5941058 e2843058 e3a05000 (e5812004)
>[ 3.21] ---[ end trace f32811052fd3860c ]---
> 
> Signed-off-by: Andrew Jeffery <and...@aj.id.au>

Good catch, thanks. Ack below.

I think there is one more instance of that bug in ftgmac100_remove()
that should probably go away too. (travelling, can't really do/test
a patch this week).

Acked-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>

> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 34dae51effd4..59da7ac3c108 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1863,7 +1863,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  err_ioremap:
>   release_resource(priv->res);
>  err_req_mem:
> - netif_napi_del(>napi);
>   free_netdev(netdev);
>  err_alloc_etherdev:
>   return err;

[PATCH 2/2] ftgmac100: Make the MDIO bus a child of the ethernet device

[Benjamin Herrenschmidt]

Populate mii_bus->parent with our own platform device before
registering, which makes it easier to locate the MDIO bus
in sysfs when trying to diagnose problems.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 2b17f7023d91..fce0aa4f78b7 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1682,6 +1682,7 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
priv->mii_bus->name = "ftgmac100_mdio";
snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%d",
 pdev->name, pdev->id);
+   priv->mii_bus->parent = priv->dev;
priv->mii_bus->priv = priv->netdev;
priv->mii_bus->read = ftgmac100_mdiobus_read;
priv->mii_bus->write = ftgmac100_mdiobus_write;

[PATCH 1/2] ftgmac100: Increase reset timeout

[Benjamin Herrenschmidt]

We had reports of 50us not being sufficient to reset the MAC,
thus hitting the "Hardware reset failed" error bringing the
interface up on some AST2400 based machines.

This bumps the timeout to 200us.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index b16128d3..2b17f7023d91 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -125,7 +125,7 @@ static int ftgmac100_reset_mac(struct ftgmac100 *priv, u32 
maccr)
iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
iowrite32(maccr | FTGMAC100_MACCR_SW_RST,
  priv->base + FTGMAC100_OFFSET_MACCR);
-   for (i = 0; i < 50; i++) {
+   for (i = 0; i < 200; i++) {
unsigned int maccr;
 
maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);

Re: clean up and modularize arch dma_mapping interface V2

[Benjamin Herrenschmidt]

On Sat, 2017-06-24 at 09:18 +0200, Christoph Hellwig wrote:
> On Wed, Jun 21, 2017 at 12:24:28PM -0700, tndave wrote:
> > Thanks for doing this.
> > So archs can still have their own definition for dma_set_mask() if 
> > HAVE_ARCH_DMA_SET_MASK is y?
> > (and similarly for dma_set_coherent_mask() when 
> > CONFIG_ARCH_HAS_DMA_SET_COHERENT_MASK is y)
> > Any plan to change these?
> 
> Yes, those should go away, but I'm not entirely sure how yet.  We'll
> need some hook for switching between an IOMMU and a direct mapping
> (I guess that's what you want to do for sparc as well?), and I need
> to find the best way to do that.  Reimplementing all of dma_set_mask
> and dma_set_coherent_mask is something that I want to move away from.

I think we still need to do it. For example we have a bunch new "funky"
cases.

We already have the case where we mix the direct and iommu mappings,
on some powerpc platforms that translates in an iommu mapping down at
0 for the 32-bit space and a direct mapping high up in the PCI address
space (which crops the top bits and thus hits memory at 0 onwards).

This type of hybrid layout is needed by some adapters, typically
storage, which want to keep the "coherent" mask at 32-bit but support
64-bit for streaming masks.

Another one we are trying to deal better with at the moment is devices
with DMA addressing limitations. Some GPUs typically (but not only)
have limits that go all accross the gamut, typically I've seen 40 bits,
44 bits and 47 bits... And of course those are "high peformance"
adapters so we don't want to limit them to the comparatively small
iommu mapping with extra overhead.

At the moment, we're looking at a dma_set_mask() hook that will, for
these guys, re-configure the iommu mapping to create a "compressed"
linear mapping of system memory (ie, skipping the holes we have between
chip on P9 for example) using the largest possible iommu page size
(256M on P8, 1G on P9).

This is made tricky of course because several devices can potentially
share a DMA domain based on various platform specific reasons. And of
course we have no way to figure out what's the "common denominator" of
all those devices before they start doing DMA. A driver can start
before the neighbour is probed and a driver can start doing DMAs using
the standard 32-bit mapping without doing dma_set_mask().

So heuristics ... ugh. Better ideas welcome :-) All that to say that we
are far from being able to get rid of dma_set_mask() custom
implementations (and coherent mask too).

I was tempted at some point retiring the 32-bit iommu mapping
completely, just doing that "linear" thing I mentioned above and
swiotlb for the rest, along with introducing ZONE_DMA32 on powerpc
(with the real 64-bit bypass still around for non-limited devices but
that's then just extra speed by bypassing the iommu xlate & cache).

But I worry of the impact on those silly adapters that set the coherent
mask to 32-bits to keep their microcode & descriptor ring down in 32-
bit space. I'm not sure how well ZONE_DMA32 behaves in those cases.

Cheers,
Ben.

Re: [PATCH 42/44] powerpc/cell: use the dma_supported method for ops switching

[Benjamin Herrenschmidt]

On Sun, 2017-06-18 at 00:13 -0700, Christoph Hellwig wrote:
> On Sun, Jun 18, 2017 at 06:50:27AM +1000, Benjamin Herrenschmidt wrote:
> > What is your rationale here ? (I have missed patch 0 it seems).
> 
> Less code duplication, more modular dma_map_ops insteance.
> 
> > dma_supported() was supposed to be pretty much a "const" function
> > simply informing whether a given setup is possible. Having it perform
> > an actual switch of ops seems to be pushing it...
> 
> dma_supported() is already gone from the public DMA API as it doesn't
> make sense to be called separately from set_dma_mask.  It will be
> entirely gone in the next series after this one.

Ah ok, in that case it makes much more sense, we can rename it then.

> > What if a driver wants to test various dma masks and then pick one ?
> > 
> > Where does the API documents that if a driver calls dma_supported() it
> > then *must* set the corresponding mask and use that ?
> 
> Where is the API document for _any_ of the dma routines? (A: work in
> progress by me, but I need to clean up the mess of arch hooks before
> it can make any sense)

Heh fair enough.

> > I don't like a function that is a "boolean query" like this one to have
> > such a major side effect.
> > 
> > > From an API standpoint, dma_set_mask() is when the mask is established,
> > 
> > and thus when the ops switch should happen.
> 
> And that's exactly what happens at the driver API level.  It just turns
> out the dma_capable method is they way better place to actually
> implement it, as the ->set_dma_mask method requires lots of code
> duplication while not offering any actual benefit over ->dma_capable.
> And because of that it's gone after this series.
> 
> In theory we could rename ->dma_capable now, but it would require a
> _lot_ of churn.  Give me another merge window or two and we should
> be down to be about 2 handful of dma_map_ops instance, at which point
> we could do all this gratious renaming a lot more easily :)

Sure, I get it now, as long as it's not publicly exposed to drivers
we are fine.

Cheers,
Ben.

Re: [PATCH 42/44] powerpc/cell: use the dma_supported method for ops switching

[Benjamin Herrenschmidt]

On Fri, 2017-06-16 at 20:10 +0200, Christoph Hellwig wrote:
> Besides removing the last instance of the set_dma_mask method this also
> reduced the code duplication.

What is your rationale here ? (I have missed patch 0 it seems).

dma_supported() was supposed to be pretty much a "const" function
simply informing whether a given setup is possible. Having it perform
an actual switch of ops seems to be pushing it...

What if a driver wants to test various dma masks and then pick one ?

Where does the API documents that if a driver calls dma_supported() it
then *must* set the corresponding mask and use that ?

I don't like a function that is a "boolean query" like this one to have
such a major side effect.

>From an API standpoint, dma_set_mask() is when the mask is established,
and thus when the ops switch should happen.

Ben.

> Signed-off-by: Christoph Hellwig 
> ---
>  arch/powerpc/platforms/cell/iommu.c | 25 +
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/cell/iommu.c 
> b/arch/powerpc/platforms/cell/iommu.c
> index 497bfbdbd967..29d4f96ed33e 100644
> --- a/arch/powerpc/platforms/cell/iommu.c
> +++ b/arch/powerpc/platforms/cell/iommu.c
> @@ -644,20 +644,14 @@ static void dma_fixed_unmap_sg(struct device *dev, 
> struct scatterlist *sg,
>  direction, attrs);
>  }
>  
> -static int dma_fixed_dma_supported(struct device *dev, u64 mask)
> -{
> - return mask == DMA_BIT_MASK(64);
> -}
> -
> -static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask);
> +static int dma_suported_and_switch(struct device *dev, u64 dma_mask);
>  
>  static const struct dma_map_ops dma_iommu_fixed_ops = {
>   .alloc  = dma_fixed_alloc_coherent,
>   .free   = dma_fixed_free_coherent,
>   .map_sg = dma_fixed_map_sg,
>   .unmap_sg   = dma_fixed_unmap_sg,
> - .dma_supported  = dma_fixed_dma_supported,
> - .set_dma_mask   = dma_set_mask_and_switch,
> + .dma_supported  = dma_suported_and_switch,
>   .map_page   = dma_fixed_map_page,
>   .unmap_page = dma_fixed_unmap_page,
>   .mapping_error  = dma_iommu_mapping_error,
> @@ -952,11 +946,8 @@ static u64 cell_iommu_get_fixed_address(struct device 
> *dev)
>   return dev_addr;
>  }
>  
> -static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask)
> +static int dma_suported_and_switch(struct device *dev, u64 dma_mask)
>  {
> - if (!dev->dma_mask || !dma_supported(dev, dma_mask))
> - return -EIO;
> -
>   if (dma_mask == DMA_BIT_MASK(64) &&
>   cell_iommu_get_fixed_address(dev) != OF_BAD_ADDR) {
>   u64 addr = cell_iommu_get_fixed_address(dev) +
> @@ -965,14 +956,16 @@ static int dma_set_mask_and_switch(struct device *dev, 
> u64 dma_mask)
>   dev_dbg(dev, "iommu: fixed addr = %llx\n", addr);
>   set_dma_ops(dev, _iommu_fixed_ops);
>   set_dma_offset(dev, addr);
> - } else {
> + return 1;
> + }
> +
> + if (dma_iommu_dma_supported(dev, dma_mask)) {
>   dev_dbg(dev, "iommu: not 64-bit, using default ops\n");
>   set_dma_ops(dev, get_pci_dma_ops());
>   cell_dma_dev_setup(dev);
> + return 1;
>   }
>  
> - *dev->dma_mask = dma_mask;
> -
>   return 0;
>  }
>  
> @@ -1127,7 +1120,7 @@ static int __init cell_iommu_fixed_mapping_init(void)
>   cell_iommu_setup_window(iommu, np, dbase, dsize, 0);
>   }
>  
> - dma_iommu_ops.set_dma_mask = dma_set_mask_and_switch;
> + dma_iommu_ops.dma_supported = dma_suported_and_switch;
>   set_pci_dma_ops(_iommu_ops);
>  
>   return 0;

[PATCH v3 6/9] ftgmac100: Allow configuration of phy interface via device-tree

[Benjamin Herrenschmidt]

This uses the standard phy-mode property

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 42 +---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index f76765e..7721c2a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -1051,7 +1052,7 @@ static void ftgmac100_adjust_link(struct net_device 
*netdev)
schedule_work(>reset_task);
 }
 
-static int ftgmac100_mii_probe(struct ftgmac100 *priv)
+static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t intf)
 {
struct net_device *netdev = priv->netdev;
struct phy_device *phydev;
@@ -1063,7 +1064,7 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv)
}
 
phydev = phy_connect(netdev, phydev_name(phydev),
-_adjust_link, PHY_INTERFACE_MODE_GMII);
+_adjust_link, intf);
 
if (IS_ERR(phydev)) {
netdev_err(netdev, "%s: Could not attach to PHY\n", 
netdev->name);
@@ -1618,6 +1619,8 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
 {
struct ftgmac100 *priv = netdev_priv(netdev);
struct platform_device *pdev = to_platform_device(priv->dev);
+   int phy_intf = PHY_INTERFACE_MODE_RGMII;
+   struct device_node *np = pdev->dev.of_node;
int i, err = 0;
u32 reg;
 
@@ -1633,6 +1636,39 @@ static int ftgmac100_setup_mdio(struct net_device 
*netdev)
iowrite32(reg, priv->base + FTGMAC100_OFFSET_REVR);
};
 
+   /* Get PHY mode from device-tree */
+   if (np) {
+   /* Default to RGMII. It's a gigabit part after all */
+   phy_intf = of_get_phy_mode(np);
+   if (phy_intf < 0)
+   phy_intf = PHY_INTERFACE_MODE_RGMII;
+
+   /* Aspeed only supports these. I don't know about other IP
+* block vendors so I'm going to just let them through for
+* now. Note that this is only a warning if for some obscure
+* reason the DT really means to lie about it or it's a newer
+* part we don't know about.
+*
+* On the Aspeed SoC there are additionally straps and SCU
+* control bits that could tell us what the interface is
+* (or allow us to configure it while the IP block is held
+* in reset). For now I chose to keep this driver away from
+* those SoC specific bits and assume the device-tree is
+* right and the SCU has been configured properly by pinmux
+* or the firmware.
+*/
+   if (priv->is_aspeed &&
+   phy_intf != PHY_INTERFACE_MODE_RMII &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_ID &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_RXID &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) {
+   netdev_warn(netdev,
+  "Unsupported PHY mode %s !\n",
+  phy_modes(phy_intf));
+   }
+   }
+
priv->mii_bus->name = "ftgmac100_mdio";
snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%d",
 pdev->name, pdev->id);
@@ -1649,7 +1685,7 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
goto err_register_mdiobus;
}
 
-   err = ftgmac100_mii_probe(priv);
+   err = ftgmac100_mii_probe(priv, phy_intf);
if (err) {
dev_err(priv->dev, "MII Probe failed!\n");
goto err_mii_probe;
-- 
2.9.3

[PATCH v3 8/9] ftgmac100: Fix potential ordering issue in NAPI poll

[Benjamin Herrenschmidt]

We need to ensure the loads from the descriptor are done after the
MMIO store clearing the interrupts has completed, otherwise we
might still miss work.

A read back from the MMIO register will "push" the posted store and
ioread32 has a barrier on weakly aordered architectures that will
order subsequent accesses.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 45b8267..95bf5e8 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1349,6 +1349,13 @@ static int ftgmac100_poll(struct napi_struct *napi, int 
budget)
 */
iowrite32(FTGMAC100_INT_RXTX,
  priv->base + FTGMAC100_OFFSET_ISR);
+
+   /* Push the above (and provides a barrier vs. subsequent
+* reads of the descriptor).
+*/
+   ioread32(priv->base + FTGMAC100_OFFSET_ISR);
+
+   /* Check RX and TX descriptors for more work to do */
if (ftgmac100_check_rx(priv) ||
ftgmac100_tx_buf_cleanable(priv))
return budget;
-- 
2.9.3

[PATCH v3 7/9] ftgmac100: Display the discovered PHY device info

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 7721c2a..45b8267 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1077,6 +1077,9 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv, 
phy_interface_t intf)
phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
phydev->advertising = phydev->supported;
 
+   /* Display what we found */
+   phy_attached_info(phydev);
+
return 0;
 }
 
-- 
2.9.3

[PATCH v3 9/9] ftgmac100: Document device-tree binding

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
--

v3. - Update supported values for phy-mode
---
 .../devicetree/bindings/net/ftgmac100.txt  | 37 ++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ftgmac100.txt

diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt 
b/Documentation/devicetree/bindings/net/ftgmac100.txt
new file mode 100644
index 000..fceeede
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
@@ -0,0 +1,37 @@
+* Faraday Technology FTGMAC100 gigabit ethernet controller
+
+Required properties:
+- compatible: "faraday,ftgmac100"
+
+  Must also contain one of these if used as part of an Aspeed AST2400
+  or 2500 family SoC as they have some subtle tweaks to the
+  implementation:
+
+ - "aspeed,ast2400-mac"
+ - "aspeed,ast2500-mac"
+
+- reg: Address and length of the register set for the device
+- interrupts: Should contain ethernet controller interrupt
+
+Optional properties:
+- phy-mode: See ethernet.txt file in the same directory. If the property is
+  absent, "rgmii" is assumed. Supported values are "rgmii*" and "rmii" for
+  aspeed parts. Other (unknown) parts will accept any value.
+- use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently assumes
+  rmii (100bT) but kept as a separate property in case NC-SI grows support
+  for a gigabit link.
+- no-hw-checksum: Used to disable HW checksum support. Here for backward
+  compatibility as the driver now should have correct defaults based on
+  the SoC.
+
+Example:
+
+   mac0: ethernet@1e66 {
+   compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
+   reg = <0x1e66 0x180>;
+   interrupts = <2>;
+   status = "okay";
+   use-ncsi;
+   };
+
+
-- 
2.9.3

[PATCH v3 5/9] ftgmac100: Add netpoll support

[Benjamin Herrenschmidt]

Just call the interrupt handler with interrupts locally disabled

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 40a03d5..f76765e 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1588,6 +1588,17 @@ static int ftgmac100_set_features(struct net_device 
*netdev,
return 0;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void ftgmac100_poll_controller(struct net_device *netdev)
+{
+   unsigned long flags;
+
+   local_irq_save(flags);
+   ftgmac100_interrupt(netdev->irq, netdev);
+   local_irq_restore(flags);
+}
+#endif
+
 static const struct net_device_ops ftgmac100_netdev_ops = {
.ndo_open   = ftgmac100_open,
.ndo_stop   = ftgmac100_stop,
@@ -1598,6 +1609,9 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_tx_timeout = ftgmac100_tx_timeout,
.ndo_set_rx_mode= ftgmac100_set_rx_mode,
.ndo_set_features   = ftgmac100_set_features,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+   .ndo_poll_controller= ftgmac100_poll_controller,
+#endif
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
-- 
2.9.3

[PATCH v3 3/9] ftgmac100: Add ndo_set_rx_mode() and support for multicast & promisc

[Benjamin Herrenschmidt]

This adds the ndo_set_rx_mode() callback to configure the
multicast filters, promisc and allmulti options.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
--

v3. - Rebase to fix conflict with #include changes
---
 drivers/net/ethernet/faraday/ftgmac100.c | 52 
 1 file changed, 52 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 949b48c..f4db6e2 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -99,6 +100,10 @@ struct ftgmac100 {
int cur_duplex;
bool use_ncsi;
 
+   /* Multicast filter settings */
+   u32 maht0;
+   u32 maht1;
+
/* Flow control settings */
bool tx_pause;
bool rx_pause;
@@ -266,6 +271,10 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
/* Write MAC address */
ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
 
+   /* Write multicast filter */
+   iowrite32(priv->maht0, priv->base + FTGMAC100_OFFSET_MAHT0);
+   iowrite32(priv->maht1, priv->base + FTGMAC100_OFFSET_MAHT1);
+
/* Configure descriptor sizes and increase burst sizes according
 * to values in Aspeed SDK. The FIFO arbitration is enabled and
 * the thresholds set based on the recommended values in the
@@ -319,6 +328,12 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
/* Add other bits as needed */
if (priv->cur_duplex == DUPLEX_FULL)
maccr |= FTGMAC100_MACCR_FULLDUP;
+   if (priv->netdev->flags & IFF_PROMISC)
+   maccr |= FTGMAC100_MACCR_RX_ALL;
+   if (priv->netdev->flags & IFF_ALLMULTI)
+   maccr |= FTGMAC100_MACCR_RX_MULTIPKT;
+   else if (netdev_mc_count(priv->netdev))
+   maccr |= FTGMAC100_MACCR_HT_MULTI_EN;
 
/* Hit the HW */
iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
@@ -329,6 +344,42 @@ static void ftgmac100_stop_hw(struct ftgmac100 *priv)
iowrite32(0, priv->base + FTGMAC100_OFFSET_MACCR);
 }
 
+static void ftgmac100_calc_mc_hash(struct ftgmac100 *priv)
+{
+   struct netdev_hw_addr *ha;
+
+   priv->maht1 = 0;
+   priv->maht0 = 0;
+   netdev_for_each_mc_addr(ha, priv->netdev) {
+   u32 crc_val = ether_crc_le(ETH_ALEN, ha->addr);
+
+   crc_val = (~(crc_val >> 2)) & 0x3f;
+   if (crc_val >= 32)
+   priv->maht1 |= 1ul << (crc_val - 32);
+   else
+   priv->maht0 |= 1ul << (crc_val);
+   }
+}
+
+static void ftgmac100_set_rx_mode(struct net_device *netdev)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+
+   /* Setup the hash filter */
+   ftgmac100_calc_mc_hash(priv);
+
+   /* Interface down ? that's all there is to do */
+   if (!netif_running(netdev))
+   return;
+
+   /* Update the HW */
+   iowrite32(priv->maht0, priv->base + FTGMAC100_OFFSET_MAHT0);
+   iowrite32(priv->maht1, priv->base + FTGMAC100_OFFSET_MAHT1);
+
+   /* Reconfigure MACCR */
+   ftgmac100_start_hw(priv);
+}
+
 static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, unsigned int entry,
  struct ftgmac100_rxdes *rxdes, gfp_t gfp)
 {
@@ -1503,6 +1554,7 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_validate_addr  = eth_validate_addr,
.ndo_do_ioctl   = ftgmac100_do_ioctl,
.ndo_tx_timeout = ftgmac100_tx_timeout,
+   .ndo_set_rx_mode= ftgmac100_set_rx_mode,
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
-- 
2.9.3

[PATCH v3 4/9] ftgmac100: Add vlan HW offload

[Benjamin Herrenschmidt]

The chip supports HW vlan tag insertion and extraction. Add support
for it.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 46 +++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index f4db6e2..40a03d5 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -335,6 +336,10 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
else if (netdev_mc_count(priv->netdev))
maccr |= FTGMAC100_MACCR_HT_MULTI_EN;
 
+   /* Vlan filtering enabled */
+   if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
+   maccr |= FTGMAC100_MACCR_RM_VLAN;
+
/* Hit the HW */
iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
 }
@@ -530,6 +535,12 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, 
int *processed)
/* Transfer received size to skb */
skb_put(skb, size);
 
+   /* Extract vlan tag */
+   if ((netdev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+   (csum_vlan & FTGMAC100_RXDES1_VLANTAG_AVAIL))
+   __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+  csum_vlan & 0x);
+
/* Tear down DMA mapping, do necessary cache management */
map = le32_to_cpu(rxdes->rxdes3);
 
@@ -754,6 +765,13 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
if (skb->ip_summed == CHECKSUM_PARTIAL &&
!ftgmac100_prep_tx_csum(skb, _vlan))
goto drop;
+
+   /* Add VLAN tag */
+   if (skb_vlan_tag_present(skb)) {
+   csum_vlan |= FTGMAC100_TXDES1_INS_VLANTAG;
+   csum_vlan |= skb_vlan_tag_get(skb) & 0x;
+   }
+
txdes->txdes1 = cpu_to_le32(csum_vlan);
 
/* Next descriptor */
@@ -1546,6 +1564,30 @@ static void ftgmac100_tx_timeout(struct net_device 
*netdev)
schedule_work(>reset_task);
 }
 
+static int ftgmac100_set_features(struct net_device *netdev,
+ netdev_features_t features)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   netdev_features_t changed = netdev->features ^ features;
+
+   if (!netif_running(netdev))
+   return 0;
+
+   /* Update the vlan filtering bit */
+   if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
+   u32 maccr;
+
+   maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
+   if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
+   maccr |= FTGMAC100_MACCR_RM_VLAN;
+   else
+   maccr &= ~FTGMAC100_MACCR_RM_VLAN;
+   iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
+   }
+
+   return 0;
+}
+
 static const struct net_device_ops ftgmac100_netdev_ops = {
.ndo_open   = ftgmac100_open,
.ndo_stop   = ftgmac100_stop,
@@ -1555,6 +1597,7 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_do_ioctl   = ftgmac100_do_ioctl,
.ndo_tx_timeout = ftgmac100_tx_timeout,
.ndo_set_rx_mode= ftgmac100_set_rx_mode,
+   .ndo_set_features   = ftgmac100_set_features,
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
@@ -1730,7 +1773,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
 
/* Base feature set */
netdev->hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
-   NETIF_F_GRO | NETIF_F_SG;
+   NETIF_F_GRO | NETIF_F_SG | NETIF_F_HW_VLAN_CTAG_RX |
+   NETIF_F_HW_VLAN_CTAG_TX;
 
/* AST2400  doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
-- 
2.9.3

[PATCH v3 1/9] ftgmac100: Add ethtool n-way reset call

[Benjamin Herrenschmidt]

A non-wired up implementation accidentally made its way in
a previous patch (Make ring sizes configurable via ethtool).

This removes it and wires up the generic phy_ethtool_nway_reset
instead.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
--

v2. - Use phy_ethtool_nway_reset() instead of custom implementation
---
 drivers/net/ethernet/faraday/ftgmac100.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 2153c5b..4cdd25a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1045,13 +1045,6 @@ static void ftgmac100_get_drvinfo(struct net_device 
*netdev,
strlcpy(info->bus_info, dev_name(>dev), sizeof(info->bus_info));
 }
 
-static int ftgmac100_nway_reset(struct net_device *ndev)
-{
-   if (!ndev->phydev)
-   return -ENXIO;
-   return phy_start_aneg(ndev->phydev);
-}
-
 static void ftgmac100_get_ringparam(struct net_device *netdev,
struct ethtool_ringparam *ering)
 {
@@ -1090,6 +1083,7 @@ static const struct ethtool_ops ftgmac100_ethtool_ops = {
.get_link   = ethtool_op_get_link,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
+   .nway_reset = phy_ethtool_nway_reset,
.get_ringparam  = ftgmac100_get_ringparam,
.set_ringparam  = ftgmac100_set_ringparam,
 };
-- 
2.9.3

[PATCH v3 2/9] ftgmac100: Add pause frames configuration and support

[Benjamin Herrenschmidt]

Hopefully my understanding of how the hardware works is correct,
as the documentation isn't completely clear. So far I have seen
no obvious issue. Pause seem to also work with NC-SI.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 96 +++-
 drivers/net/ethernet/faraday/ftgmac100.h |  7 +++
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 4cdd25a..949b48c 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -99,6 +99,11 @@ struct ftgmac100 {
int cur_duplex;
bool use_ncsi;
 
+   /* Flow control settings */
+   bool tx_pause;
+   bool rx_pause;
+   bool aneg_pause;
+
/* Misc */
bool need_mac_restart;
bool is_aspeed;
@@ -219,6 +224,23 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
return 0;
 }
 
+static void ftgmac100_config_pause(struct ftgmac100 *priv)
+{
+   u32 fcr = FTGMAC100_FCR_PAUSE_TIME(16);
+
+   /* Throttle tx queue when receiving pause frames */
+   if (priv->rx_pause)
+   fcr |= FTGMAC100_FCR_FC_EN;
+
+   /* Enables sending pause frames when the RX queue is past a
+* certain threshold.
+*/
+   if (priv->tx_pause)
+   fcr |= FTGMAC100_FCR_FCTHR_EN;
+
+   iowrite32(fcr, priv->base + FTGMAC100_OFFSET_FCR);
+}
+
 static void ftgmac100_init_hw(struct ftgmac100 *priv)
 {
u32 reg, rfifo_sz, tfifo_sz;
@@ -912,6 +934,7 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 {
struct ftgmac100 *priv = netdev_priv(netdev);
struct phy_device *phydev = netdev->phydev;
+   bool tx_pause, rx_pause;
int new_speed;
 
/* We store "no link" as speed 0 */
@@ -920,8 +943,21 @@ static void ftgmac100_adjust_link(struct net_device 
*netdev)
else
new_speed = phydev->speed;
 
+   /* Grab pause settings from PHY if configured to do so */
+   if (priv->aneg_pause) {
+   rx_pause = tx_pause = phydev->pause;
+   if (phydev->asym_pause)
+   tx_pause = !rx_pause;
+   } else {
+   rx_pause = priv->rx_pause;
+   tx_pause = priv->tx_pause;
+   }
+
+   /* Link hasn't changed, do nothing */
if (phydev->speed == priv->cur_speed &&
-   phydev->duplex == priv->cur_duplex)
+   phydev->duplex == priv->cur_duplex &&
+   rx_pause == priv->rx_pause &&
+   tx_pause == priv->tx_pause)
return;
 
/* Print status if we have a link or we had one and just lost it,
@@ -932,6 +968,8 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 
priv->cur_speed = new_speed;
priv->cur_duplex = phydev->duplex;
+   priv->rx_pause = rx_pause;
+   priv->tx_pause = tx_pause;
 
/* Link is down, do nothing else */
if (!new_speed)
@@ -963,6 +1001,12 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv)
return PTR_ERR(phydev);
}
 
+   /* Indicate that we support PAUSE frames (see comment in
+* Documentation/networking/phy.txt)
+*/
+   phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+   phydev->advertising = phydev->supported;
+
return 0;
 }
 
@@ -1078,6 +1122,48 @@ static int ftgmac100_set_ringparam(struct net_device 
*netdev,
return 0;
 }
 
+static void ftgmac100_get_pauseparam(struct net_device *netdev,
+struct ethtool_pauseparam *pause)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+
+   pause->autoneg = priv->aneg_pause;
+   pause->tx_pause = priv->tx_pause;
+   pause->rx_pause = priv->rx_pause;
+}
+
+static int ftgmac100_set_pauseparam(struct net_device *netdev,
+   struct ethtool_pauseparam *pause)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   struct phy_device *phydev = netdev->phydev;
+
+   priv->aneg_pause = pause->autoneg;
+   priv->tx_pause = pause->tx_pause;
+   priv->rx_pause = pause->rx_pause;
+
+   if (phydev) {
+   phydev->advertising &= ~ADVERTISED_Pause;
+   phydev->advertising &= ~ADVERTISED_Asym_Pause;
+
+   if (pause->rx_pause) {
+   phydev->advertising |= ADVERTISED_Pause;
+   phydev->advertising |= ADVERTISED_Asym_Pause;
+   }
+
+   if (pause->tx_pause)
+   phydev->advertising ^= ADVERTISED_Asym_Pause;
+   }
+   if (netif_running(netdev)) {
+

[PATCH v3 0/9] ftgmac100: Rework batch 5 - Features

[Benjamin Herrenschmidt]

This is the third spin of the fifth and last batch of
updates to the ftgmac100 driver.

This contains a few additional "features" such as:

 - Support for ethtool n-way reset
 - Multicast filtering & promisc support
 - Vlan offload
 - netpoll

And a couple of misc bits. This also adds the device-tree binding
documentation.

v2. - Addresses review comments and adds a new patch fixing a
  theorical ordering issue in my new NAPI poll implementation
- Add a bug fix (Patch 8/9) for a potential ordering issue
  in the new NAPI poll code.

v3. - Rebase on net-next (fix conflict with an unrelated #include
  change series)
- Update DT bindings better describing accepted phy-mode values

Re: [PATCH v2 0/9] ftgmac100: Rework batch 5 - Features

[Benjamin Herrenschmidt]

On Mon, 2017-04-17 at 11:11 -0400, David Miller wrote:
> From: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> Date: Thu, 13 Apr 2017 14:39:07 +1000
> 
> > This is the second spin of the fifth and last batch of
> > updates to the ftgmac100 driver.
> 
> This series doesn't apply cleanly to net-next, please respin.

Sure, I'll do this today. Thanks.

Cheers,
Ben.

Re: [PATCH 8/8] ftgmac100: Document device-tree binding

[Benjamin Herrenschmidt]

On Thu, 2017-04-13 at 15:42 +0200, Andrew Lunn wrote:
> > +- phy-mode: See ethernet.txt file in the same directory. If the
> > property is
> > +  absent, "rgmii" is assumed. Supported values are "rgmii" and
> > "rmii"
> 
> You might want to say rgmii*, or similar, it indicate the delayed
> versions are accepted as well.

You are right. I originally didn't accept them, then fixed that
up in the code but forgot to update the binding.

Dave, if that's the only issue, I'd rather send a separate later
tomorrow or so patch to amend the binding rather than send the whole
series all over again as this is rather a minor detail.

Cheers,
Ben.

[PATCH v2 6/9] ftgmac100: Allow configuration of phy interface via device-tree

[Benjamin Herrenschmidt]

This uses the standard phy-mode property

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 42 +---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index b1fb729..7c607eb 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -1049,7 +1050,7 @@ static void ftgmac100_adjust_link(struct net_device 
*netdev)
schedule_work(>reset_task);
 }
 
-static int ftgmac100_mii_probe(struct ftgmac100 *priv)
+static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t intf)
 {
struct net_device *netdev = priv->netdev;
struct phy_device *phydev;
@@ -1061,7 +1062,7 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv)
}
 
phydev = phy_connect(netdev, phydev_name(phydev),
-_adjust_link, PHY_INTERFACE_MODE_GMII);
+_adjust_link, intf);
 
if (IS_ERR(phydev)) {
netdev_err(netdev, "%s: Could not attach to PHY\n", 
netdev->name);
@@ -1616,6 +1617,8 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
 {
struct ftgmac100 *priv = netdev_priv(netdev);
struct platform_device *pdev = to_platform_device(priv->dev);
+   int phy_intf = PHY_INTERFACE_MODE_RGMII;
+   struct device_node *np = pdev->dev.of_node;
int i, err = 0;
u32 reg;
 
@@ -1631,6 +1634,39 @@ static int ftgmac100_setup_mdio(struct net_device 
*netdev)
iowrite32(reg, priv->base + FTGMAC100_OFFSET_REVR);
};
 
+   /* Get PHY mode from device-tree */
+   if (np) {
+   /* Default to RGMII. It's a gigabit part after all */
+   phy_intf = of_get_phy_mode(np);
+   if (phy_intf < 0)
+   phy_intf = PHY_INTERFACE_MODE_RGMII;
+
+   /* Aspeed only supports these. I don't know about other IP
+* block vendors so I'm going to just let them through for
+* now. Note that this is only a warning if for some obscure
+* reason the DT really means to lie about it or it's a newer
+* part we don't know about.
+*
+* On the Aspeed SoC there are additionally straps and SCU
+* control bits that could tell us what the interface is
+* (or allow us to configure it while the IP block is held
+* in reset). For now I chose to keep this driver away from
+* those SoC specific bits and assume the device-tree is
+* right and the SCU has been configured properly by pinmux
+* or the firmware.
+*/
+   if (priv->is_aspeed &&
+   phy_intf != PHY_INTERFACE_MODE_RMII &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_ID &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_RXID &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) {
+   netdev_warn(netdev,
+  "Unsupported PHY mode %s !\n",
+  phy_modes(phy_intf));
+   }
+   }
+
priv->mii_bus->name = "ftgmac100_mdio";
snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%d",
 pdev->name, pdev->id);
@@ -1647,7 +1683,7 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
goto err_register_mdiobus;
}
 
-   err = ftgmac100_mii_probe(priv);
+   err = ftgmac100_mii_probe(priv, phy_intf);
if (err) {
dev_err(priv->dev, "MII Probe failed!\n");
goto err_mii_probe;
-- 
2.9.3

[PATCH v2 9/9] ftgmac100: Document device-tree binding

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 .../devicetree/bindings/net/ftgmac100.txt  | 36 ++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ftgmac100.txt

diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt 
b/Documentation/devicetree/bindings/net/ftgmac100.txt
new file mode 100644
index 000..68a694a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
@@ -0,0 +1,36 @@
+* Faraday Technology FTGMAC100 gigabit ethernet controller
+
+Required properties:
+- compatible: "faraday,ftgmac100"
+
+  Must also contain one of these if used as part of an Aspeed AST2400
+  or 2500 family SoC as they have some subtle tweaks to the
+  implementation:
+
+ - "aspeed,ast2400-mac"
+ - "aspeed,ast2500-mac"
+
+- reg: Address and length of the register set for the device
+- interrupts: Should contain ethernet controller interrupt
+
+Optional properties:
+- phy-mode: See ethernet.txt file in the same directory. If the property is
+  absent, "rgmii" is assumed. Supported values are "rgmii" and "rmii"
+- use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently assumes
+  rmii (100bT) but kept as a separate property in case NC-SI grows support
+  for a gigabit link.
+- no-hw-checksum: Used to disable HW checksum support. Here for backward
+  compatibility as the driver now should have correct defaults based on
+  the SoC.
+
+Example:
+
+   mac0: ethernet@1e66 {
+   compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
+   reg = <0x1e66 0x180>;
+   interrupts = <2>;
+   status = "okay";
+   use-ncsi;
+   };
+
+
-- 
2.9.3

[PATCH v2 8/9] ftgmac100: Fix potential ordering issue in NAPI poll

[Benjamin Herrenschmidt]

We need to ensure the loads from the descriptor are done after the
MMIO store clearing the interrupts has completed, otherwise we
might still miss work.

A read back from the MMIO register will "push" the posted store and
ioread32 has a barrier on weakly aordered architectures that will
order subsequent accesses.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 71763e4..9b7a24e 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1347,6 +1347,13 @@ static int ftgmac100_poll(struct napi_struct *napi, int 
budget)
 */
iowrite32(FTGMAC100_INT_RXTX,
  priv->base + FTGMAC100_OFFSET_ISR);
+
+   /* Push the above (and provides a barrier vs. subsequent
+* reads of the descriptor).
+*/
+   ioread32(priv->base + FTGMAC100_OFFSET_ISR);
+
+   /* Check RX and TX descriptors for more work to do */
if (ftgmac100_check_rx(priv) ||
ftgmac100_tx_buf_cleanable(priv))
return budget;
-- 
2.9.3

[PATCH v2 3/9] ftgmac100: Add ndo_set_rx_mode() and support for multicast & promisc

[Benjamin Herrenschmidt]

This adds the ndo_set_rx_mode() callback to configure the
multicast filters, promisc and allmulti options.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 52 
 1 file changed, 52 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 4be8bf9..551ab3e 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -97,6 +98,10 @@ struct ftgmac100 {
int cur_duplex;
bool use_ncsi;
 
+   /* Multicast filter settings */
+   u32 maht0;
+   u32 maht1;
+
/* Flow control settings */
bool tx_pause;
bool rx_pause;
@@ -264,6 +269,10 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
/* Write MAC address */
ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
 
+   /* Write multicast filter */
+   iowrite32(priv->maht0, priv->base + FTGMAC100_OFFSET_MAHT0);
+   iowrite32(priv->maht1, priv->base + FTGMAC100_OFFSET_MAHT1);
+
/* Configure descriptor sizes and increase burst sizes according
 * to values in Aspeed SDK. The FIFO arbitration is enabled and
 * the thresholds set based on the recommended values in the
@@ -317,6 +326,12 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
/* Add other bits as needed */
if (priv->cur_duplex == DUPLEX_FULL)
maccr |= FTGMAC100_MACCR_FULLDUP;
+   if (priv->netdev->flags & IFF_PROMISC)
+   maccr |= FTGMAC100_MACCR_RX_ALL;
+   if (priv->netdev->flags & IFF_ALLMULTI)
+   maccr |= FTGMAC100_MACCR_RX_MULTIPKT;
+   else if (netdev_mc_count(priv->netdev))
+   maccr |= FTGMAC100_MACCR_HT_MULTI_EN;
 
/* Hit the HW */
iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
@@ -327,6 +342,42 @@ static void ftgmac100_stop_hw(struct ftgmac100 *priv)
iowrite32(0, priv->base + FTGMAC100_OFFSET_MACCR);
 }
 
+static void ftgmac100_calc_mc_hash(struct ftgmac100 *priv)
+{
+   struct netdev_hw_addr *ha;
+
+   priv->maht1 = 0;
+   priv->maht0 = 0;
+   netdev_for_each_mc_addr(ha, priv->netdev) {
+   u32 crc_val = ether_crc_le(ETH_ALEN, ha->addr);
+
+   crc_val = (~(crc_val >> 2)) & 0x3f;
+   if (crc_val >= 32)
+   priv->maht1 |= 1ul << (crc_val - 32);
+   else
+   priv->maht0 |= 1ul << (crc_val);
+   }
+}
+
+static void ftgmac100_set_rx_mode(struct net_device *netdev)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+
+   /* Setup the hash filter */
+   ftgmac100_calc_mc_hash(priv);
+
+   /* Interface down ? that's all there is to do */
+   if (!netif_running(netdev))
+   return;
+
+   /* Update the HW */
+   iowrite32(priv->maht0, priv->base + FTGMAC100_OFFSET_MAHT0);
+   iowrite32(priv->maht1, priv->base + FTGMAC100_OFFSET_MAHT1);
+
+   /* Reconfigure MACCR */
+   ftgmac100_start_hw(priv);
+}
+
 static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, unsigned int entry,
  struct ftgmac100_rxdes *rxdes, gfp_t gfp)
 {
@@ -1501,6 +1552,7 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_validate_addr  = eth_validate_addr,
.ndo_do_ioctl   = ftgmac100_do_ioctl,
.ndo_tx_timeout = ftgmac100_tx_timeout,
+   .ndo_set_rx_mode= ftgmac100_set_rx_mode,
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
-- 
2.9.3

[PATCH v2 5/9] ftgmac100: Add netpoll support

[Benjamin Herrenschmidt]

Just call the interrupt handler with interrupts locally disabled

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 08fe228..b1fb729 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1586,6 +1586,17 @@ static int ftgmac100_set_features(struct net_device 
*netdev,
return 0;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void ftgmac100_poll_controller(struct net_device *netdev)
+{
+   unsigned long flags;
+
+   local_irq_save(flags);
+   ftgmac100_interrupt(netdev->irq, netdev);
+   local_irq_restore(flags);
+}
+#endif
+
 static const struct net_device_ops ftgmac100_netdev_ops = {
.ndo_open   = ftgmac100_open,
.ndo_stop   = ftgmac100_stop,
@@ -1596,6 +1607,9 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_tx_timeout = ftgmac100_tx_timeout,
.ndo_set_rx_mode= ftgmac100_set_rx_mode,
.ndo_set_features   = ftgmac100_set_features,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+   .ndo_poll_controller= ftgmac100_poll_controller,
+#endif
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
-- 
2.9.3

[PATCH v2 7/9] ftgmac100: Display the discovered PHY device info

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 7c607eb..71763e4 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1075,6 +1075,9 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv, 
phy_interface_t intf)
phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
phydev->advertising = phydev->supported;
 
+   /* Display what we found */
+   phy_attached_info(phydev);
+
return 0;
 }
 
-- 
2.9.3

[PATCH v2 4/9] ftgmac100: Add vlan HW offload

[Benjamin Herrenschmidt]

The chip supports HW vlan tag insertion and extraction. Add support
for it.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 46 +++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 551ab3e..08fe228 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -333,6 +334,10 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
else if (netdev_mc_count(priv->netdev))
maccr |= FTGMAC100_MACCR_HT_MULTI_EN;
 
+   /* Vlan filtering enabled */
+   if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
+   maccr |= FTGMAC100_MACCR_RM_VLAN;
+
/* Hit the HW */
iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
 }
@@ -528,6 +533,12 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, 
int *processed)
/* Transfer received size to skb */
skb_put(skb, size);
 
+   /* Extract vlan tag */
+   if ((netdev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+   (csum_vlan & FTGMAC100_RXDES1_VLANTAG_AVAIL))
+   __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+  csum_vlan & 0x);
+
/* Tear down DMA mapping, do necessary cache management */
map = le32_to_cpu(rxdes->rxdes3);
 
@@ -752,6 +763,13 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
if (skb->ip_summed == CHECKSUM_PARTIAL &&
!ftgmac100_prep_tx_csum(skb, _vlan))
goto drop;
+
+   /* Add VLAN tag */
+   if (skb_vlan_tag_present(skb)) {
+   csum_vlan |= FTGMAC100_TXDES1_INS_VLANTAG;
+   csum_vlan |= skb_vlan_tag_get(skb) & 0x;
+   }
+
txdes->txdes1 = cpu_to_le32(csum_vlan);
 
/* Next descriptor */
@@ -1544,6 +1562,30 @@ static void ftgmac100_tx_timeout(struct net_device 
*netdev)
schedule_work(>reset_task);
 }
 
+static int ftgmac100_set_features(struct net_device *netdev,
+ netdev_features_t features)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   netdev_features_t changed = netdev->features ^ features;
+
+   if (!netif_running(netdev))
+   return 0;
+
+   /* Update the vlan filtering bit */
+   if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
+   u32 maccr;
+
+   maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
+   if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
+   maccr |= FTGMAC100_MACCR_RM_VLAN;
+   else
+   maccr &= ~FTGMAC100_MACCR_RM_VLAN;
+   iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
+   }
+
+   return 0;
+}
+
 static const struct net_device_ops ftgmac100_netdev_ops = {
.ndo_open   = ftgmac100_open,
.ndo_stop   = ftgmac100_stop,
@@ -1553,6 +1595,7 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_do_ioctl   = ftgmac100_do_ioctl,
.ndo_tx_timeout = ftgmac100_tx_timeout,
.ndo_set_rx_mode= ftgmac100_set_rx_mode,
+   .ndo_set_features   = ftgmac100_set_features,
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
@@ -1728,7 +1771,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
 
/* Base feature set */
netdev->hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
-   NETIF_F_GRO | NETIF_F_SG;
+   NETIF_F_GRO | NETIF_F_SG | NETIF_F_HW_VLAN_CTAG_RX |
+   NETIF_F_HW_VLAN_CTAG_TX;
 
/* AST2400  doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
-- 
2.9.3

[PATCH v2 2/9] ftgmac100: Add pause frames configuration and support

[Benjamin Herrenschmidt]

Hopefully my understanding of how the hardware works is correct,
as the documentation isn't completely clear. So far I have seen
no obvious issue. Pause seem to also work with NC-SI.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 96 +++-
 drivers/net/ethernet/faraday/ftgmac100.h |  7 +++
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 66a5065..4be8bf9 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -97,6 +97,11 @@ struct ftgmac100 {
int cur_duplex;
bool use_ncsi;
 
+   /* Flow control settings */
+   bool tx_pause;
+   bool rx_pause;
+   bool aneg_pause;
+
/* Misc */
bool need_mac_restart;
bool is_aspeed;
@@ -217,6 +222,23 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
return 0;
 }
 
+static void ftgmac100_config_pause(struct ftgmac100 *priv)
+{
+   u32 fcr = FTGMAC100_FCR_PAUSE_TIME(16);
+
+   /* Throttle tx queue when receiving pause frames */
+   if (priv->rx_pause)
+   fcr |= FTGMAC100_FCR_FC_EN;
+
+   /* Enables sending pause frames when the RX queue is past a
+* certain threshold.
+*/
+   if (priv->tx_pause)
+   fcr |= FTGMAC100_FCR_FCTHR_EN;
+
+   iowrite32(fcr, priv->base + FTGMAC100_OFFSET_FCR);
+}
+
 static void ftgmac100_init_hw(struct ftgmac100 *priv)
 {
u32 reg, rfifo_sz, tfifo_sz;
@@ -910,6 +932,7 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 {
struct ftgmac100 *priv = netdev_priv(netdev);
struct phy_device *phydev = netdev->phydev;
+   bool tx_pause, rx_pause;
int new_speed;
 
/* We store "no link" as speed 0 */
@@ -918,8 +941,21 @@ static void ftgmac100_adjust_link(struct net_device 
*netdev)
else
new_speed = phydev->speed;
 
+   /* Grab pause settings from PHY if configured to do so */
+   if (priv->aneg_pause) {
+   rx_pause = tx_pause = phydev->pause;
+   if (phydev->asym_pause)
+   tx_pause = !rx_pause;
+   } else {
+   rx_pause = priv->rx_pause;
+   tx_pause = priv->tx_pause;
+   }
+
+   /* Link hasn't changed, do nothing */
if (phydev->speed == priv->cur_speed &&
-   phydev->duplex == priv->cur_duplex)
+   phydev->duplex == priv->cur_duplex &&
+   rx_pause == priv->rx_pause &&
+   tx_pause == priv->tx_pause)
return;
 
/* Print status if we have a link or we had one and just lost it,
@@ -930,6 +966,8 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 
priv->cur_speed = new_speed;
priv->cur_duplex = phydev->duplex;
+   priv->rx_pause = rx_pause;
+   priv->tx_pause = tx_pause;
 
/* Link is down, do nothing else */
if (!new_speed)
@@ -961,6 +999,12 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv)
return PTR_ERR(phydev);
}
 
+   /* Indicate that we support PAUSE frames (see comment in
+* Documentation/networking/phy.txt)
+*/
+   phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+   phydev->advertising = phydev->supported;
+
return 0;
 }
 
@@ -1076,6 +1120,48 @@ static int ftgmac100_set_ringparam(struct net_device 
*netdev,
return 0;
 }
 
+static void ftgmac100_get_pauseparam(struct net_device *netdev,
+struct ethtool_pauseparam *pause)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+
+   pause->autoneg = priv->aneg_pause;
+   pause->tx_pause = priv->tx_pause;
+   pause->rx_pause = priv->rx_pause;
+}
+
+static int ftgmac100_set_pauseparam(struct net_device *netdev,
+   struct ethtool_pauseparam *pause)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   struct phy_device *phydev = netdev->phydev;
+
+   priv->aneg_pause = pause->autoneg;
+   priv->tx_pause = pause->tx_pause;
+   priv->rx_pause = pause->rx_pause;
+
+   if (phydev) {
+   phydev->advertising &= ~ADVERTISED_Pause;
+   phydev->advertising &= ~ADVERTISED_Asym_Pause;
+
+   if (pause->rx_pause) {
+   phydev->advertising |= ADVERTISED_Pause;
+   phydev->advertising |= ADVERTISED_Asym_Pause;
+   }
+
+   if (pause->tx_pause)
+   phydev->advertising ^= ADVERTISED_Asym_Pause;
+   }
+   if (netif_running(netdev)) {
+

[PATCH v2 0/9] ftgmac100: Rework batch 5 - Features

[Benjamin Herrenschmidt]

This is the second spin of the fifth and last batch of
updates to the ftgmac100 driver.

This contains a few additional "features" such as:

 - Support for ethtool n-way reset
 - Multicast filtering & promisc support
 - Vlan offload
 - netpoll

And a couple of misc bits. This also adds the device-tree binding
documentation.

v2. - Addresses review comments and adds a new patch fixing a
  theorical ordering issue in my new NAPI poll implementation
- Add a bug fix (Patch 8/9) for a potential ordering issue
  in the new NAPI poll code.

[PATCH v2 1/9] ftgmac100: Add ethtool n-way reset call

[Benjamin Herrenschmidt]

A non-wired up implementation accidentally made its way in
a previous patch (Make ring sizes configurable via ethtool).

This removes it and wires up the generic phy_ethtool_nway_reset
instead.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
--

v2. - Use phy_ethtool_nway_reset() instead of custom implementation
---
 drivers/net/ethernet/faraday/ftgmac100.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 796b37e..66a5065 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1043,13 +1043,6 @@ static void ftgmac100_get_drvinfo(struct net_device 
*netdev,
strlcpy(info->bus_info, dev_name(>dev), sizeof(info->bus_info));
 }
 
-static int ftgmac100_nway_reset(struct net_device *ndev)
-{
-   if (!ndev->phydev)
-   return -ENXIO;
-   return phy_start_aneg(ndev->phydev);
-}
-
 static void ftgmac100_get_ringparam(struct net_device *netdev,
struct ethtool_ringparam *ering)
 {
@@ -1088,6 +1081,7 @@ static const struct ethtool_ops ftgmac100_ethtool_ops = {
.get_link   = ethtool_op_get_link,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
+   .nway_reset = phy_ethtool_nway_reset,
.get_ringparam  = ftgmac100_get_ringparam,
.set_ringparam  = ftgmac100_set_ringparam,
 };
-- 
2.9.3

Re: [PATCH 1/8] ftgmac100: Add ethtool n-way reset call

[Benjamin Herrenschmidt]

On Wed, 2017-04-12 at 17:00 -0700, Florian Fainelli wrote:
> > -static int ftgmac100_nway_reset(struct net_device *ndev)
> > +static int ftgmac100_nway_reset(struct net_device *netdev)
> >   {
> > - if (!ndev->phydev)
> > + if (!netdev->phydev)
> >    return -ENXIO;
> > - return phy_start_aneg(ndev->phydev);
> > + return phy_start_aneg(netdev->phydev);
> 
> Can you use phy_ethtool_nway_reset() which does that (and also checks
> if phydev->drv is NULL which would be the case after an unbind).

Ah sure, I didn't notice that one, grepped the wrong driver :-)

I'll respin later today.

Thanks !

Cheers,
Ben.

[PATCH 8/8] ftgmac100: Document device-tree binding

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 .../devicetree/bindings/net/ftgmac100.txt  | 36 ++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ftgmac100.txt

diff --git a/Documentation/devicetree/bindings/net/ftgmac100.txt 
b/Documentation/devicetree/bindings/net/ftgmac100.txt
new file mode 100644
index 000..68a694a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ftgmac100.txt
@@ -0,0 +1,36 @@
+* Faraday Technology FTGMAC100 gigabit ethernet controller
+
+Required properties:
+- compatible: "faraday,ftgmac100"
+
+  Must also contain one of these if used as part of an Aspeed AST2400
+  or 2500 family SoC as they have some subtle tweaks to the
+  implementation:
+
+ - "aspeed,ast2400-mac"
+ - "aspeed,ast2500-mac"
+
+- reg: Address and length of the register set for the device
+- interrupts: Should contain ethernet controller interrupt
+
+Optional properties:
+- phy-mode: See ethernet.txt file in the same directory. If the property is
+  absent, "rgmii" is assumed. Supported values are "rgmii" and "rmii"
+- use-ncsi: Use the NC-SI stack instead of an MDIO PHY. Currently assumes
+  rmii (100bT) but kept as a separate property in case NC-SI grows support
+  for a gigabit link.
+- no-hw-checksum: Used to disable HW checksum support. Here for backward
+  compatibility as the driver now should have correct defaults based on
+  the SoC.
+
+Example:
+
+   mac0: ethernet@1e66 {
+   compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
+   reg = <0x1e66 0x180>;
+   interrupts = <2>;
+   status = "okay";
+   use-ncsi;
+   };
+
+
-- 
2.9.3

[PATCH 5/8] ftgmac100: Add netpoll support

[Benjamin Herrenschmidt]

Just call the interrupt handler with interrupts locally disabled

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index ded3447..e71b9c4 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1593,6 +1593,17 @@ static int ftgmac100_set_features(struct net_device 
*netdev,
return 0;
 }
 
+#ifdef CONFIG_NET_POLL_CONTROLLER
+static void ftgmac100_poll_controller(struct net_device *netdev)
+{
+   unsigned long flags;
+
+   local_irq_save(flags);
+   ftgmac100_interrupt(netdev->irq, netdev);
+   local_irq_restore(flags);
+}
+#endif
+
 static const struct net_device_ops ftgmac100_netdev_ops = {
.ndo_open   = ftgmac100_open,
.ndo_stop   = ftgmac100_stop,
@@ -1603,6 +1614,9 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_tx_timeout = ftgmac100_tx_timeout,
.ndo_set_rx_mode= ftgmac100_set_rx_mode,
.ndo_set_features   = ftgmac100_set_features,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+   .ndo_poll_controller= ftgmac100_poll_controller,
+#endif
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
-- 
2.9.3

[PATCH 6/8] ftgmac100: Allow configuration of phy interface via device-tree

[Benjamin Herrenschmidt]

This uses the standard phy-mode property

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 42 +---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index e71b9c4..c1afda8 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -1049,7 +1050,7 @@ static void ftgmac100_adjust_link(struct net_device 
*netdev)
schedule_work(>reset_task);
 }
 
-static int ftgmac100_mii_probe(struct ftgmac100 *priv)
+static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t intf)
 {
struct net_device *netdev = priv->netdev;
struct phy_device *phydev;
@@ -1061,7 +1062,7 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv)
}
 
phydev = phy_connect(netdev, phydev_name(phydev),
-_adjust_link, PHY_INTERFACE_MODE_GMII);
+_adjust_link, intf);
 
if (IS_ERR(phydev)) {
netdev_err(netdev, "%s: Could not attach to PHY\n", 
netdev->name);
@@ -1623,6 +1624,8 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
 {
struct ftgmac100 *priv = netdev_priv(netdev);
struct platform_device *pdev = to_platform_device(priv->dev);
+   int phy_intf = PHY_INTERFACE_MODE_RGMII;
+   struct device_node *np = pdev->dev.of_node;
int i, err = 0;
u32 reg;
 
@@ -1638,6 +1641,39 @@ static int ftgmac100_setup_mdio(struct net_device 
*netdev)
iowrite32(reg, priv->base + FTGMAC100_OFFSET_REVR);
};
 
+   /* Get PHY mode from device-tree */
+   if (np) {
+   /* Default to RGMII. It's a gigabit part after all */
+   phy_intf = of_get_phy_mode(np);
+   if (phy_intf < 0)
+   phy_intf = PHY_INTERFACE_MODE_RGMII;
+
+   /* Aspeed only supports these. I don't know about other IP
+* block vendors so I'm going to just let them through for
+* now. Note that this is only a warning if for some obscure
+* reason the DT really means to lie about it or it's a newer
+* part we don't know about.
+*
+* On the Aspeed SoC there are additionally straps and SCU
+* control bits that could tell us what the interface is
+* (or allow us to configure it while the IP block is held
+* in reset). For now I chose to keep this driver away from
+* those SoC specific bits and assume the device-tree is
+* right and the SCU has been configured properly by pinmux
+* or the firmware.
+*/
+   if (priv->is_aspeed &&
+   phy_intf != PHY_INTERFACE_MODE_RMII &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_ID &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_RXID &&
+   phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) {
+   netdev_warn(netdev,
+  "Unsupported PHY mode %s !\n",
+  phy_modes(phy_intf));
+   }
+   }
+
priv->mii_bus->name = "ftgmac100_mdio";
snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%d",
 pdev->name, pdev->id);
@@ -1654,7 +1690,7 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
goto err_register_mdiobus;
}
 
-   err = ftgmac100_mii_probe(priv);
+   err = ftgmac100_mii_probe(priv, phy_intf);
if (err) {
dev_err(priv->dev, "MII Probe failed!\n");
goto err_mii_probe;
-- 
2.9.3

[PATCH 4/8] ftgmac100: Add vlan HW offload

[Benjamin Herrenschmidt]

The chip supports HW vlan tag insertion and extraction. Add support
for it.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 46 +++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index d86b27f..ded3447 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -333,6 +334,10 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
else if (netdev_mc_count(priv->netdev))
maccr |= FTGMAC100_MACCR_HT_MULTI_EN;
 
+   /* Vlan filtering enabled */
+   if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
+   maccr |= FTGMAC100_MACCR_RM_VLAN;
+
/* Hit the HW */
iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
 }
@@ -528,6 +533,12 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, 
int *processed)
/* Transfer received size to skb */
skb_put(skb, size);
 
+   /* Extract vlan tag */
+   if ((netdev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
+   (csum_vlan & FTGMAC100_RXDES1_VLANTAG_AVAIL))
+   __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+  csum_vlan & 0x);
+
/* Tear down DMA mapping, do necessary cache management */
map = le32_to_cpu(rxdes->rxdes3);
 
@@ -752,6 +763,13 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
if (skb->ip_summed == CHECKSUM_PARTIAL &&
!ftgmac100_prep_tx_csum(skb, _vlan))
goto drop;
+
+   /* Add VLAN tag */
+   if (skb_vlan_tag_present(skb)) {
+   csum_vlan |= FTGMAC100_TXDES1_INS_VLANTAG;
+   csum_vlan |= skb_vlan_tag_get(skb) & 0x;
+   }
+
txdes->txdes1 = cpu_to_le32(csum_vlan);
 
/* Next descriptor */
@@ -1551,6 +1569,30 @@ static void ftgmac100_tx_timeout(struct net_device 
*netdev)
schedule_work(>reset_task);
 }
 
+static int ftgmac100_set_features(struct net_device *netdev,
+ netdev_features_t features)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   netdev_features_t changed = netdev->features ^ features;
+
+   if (!netif_running(netdev))
+   return 0;
+
+   /* Update the vlan filtering bit */
+   if (changed & NETIF_F_HW_VLAN_CTAG_RX) {
+   u32 maccr;
+
+   maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
+   if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
+   maccr |= FTGMAC100_MACCR_RM_VLAN;
+   else
+   maccr &= ~FTGMAC100_MACCR_RM_VLAN;
+   iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
+   }
+
+   return 0;
+}
+
 static const struct net_device_ops ftgmac100_netdev_ops = {
.ndo_open   = ftgmac100_open,
.ndo_stop   = ftgmac100_stop,
@@ -1560,6 +1602,7 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_do_ioctl   = ftgmac100_do_ioctl,
.ndo_tx_timeout = ftgmac100_tx_timeout,
.ndo_set_rx_mode= ftgmac100_set_rx_mode,
+   .ndo_set_features   = ftgmac100_set_features,
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
@@ -1735,7 +1778,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
 
/* Base feature set */
netdev->hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
-   NETIF_F_GRO | NETIF_F_SG;
+   NETIF_F_GRO | NETIF_F_SG | NETIF_F_HW_VLAN_CTAG_RX |
+   NETIF_F_HW_VLAN_CTAG_TX;
 
/* AST2400  doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
-- 
2.9.3

[PATCH 7/8] ftgmac100: Display the discovered PHY device info

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index c1afda8..d04ad31 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1075,6 +1075,9 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv, 
phy_interface_t intf)
phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
phydev->advertising = phydev->supported;
 
+   /* Display what we found */
+   phy_attached_info(phydev);
+
return 0;
 }
 
-- 
2.9.3

[PATCH 2/8] ftgmac100: Add pause frames configuration and support

[Benjamin Herrenschmidt]

Hopefully my understanding of how the hardware works is correct,
as the documentation isn't completely clear. So far I have seen
no obvious issue. Pause seem to also work with NC-SI.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 96 +++-
 drivers/net/ethernet/faraday/ftgmac100.h |  7 +++
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index bbeb8e7..4f3ec2c 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -97,6 +97,11 @@ struct ftgmac100 {
int cur_duplex;
bool use_ncsi;
 
+   /* Flow control settings */
+   bool tx_pause;
+   bool rx_pause;
+   bool aneg_pause;
+
/* Misc */
bool need_mac_restart;
bool is_aspeed;
@@ -217,6 +222,23 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
return 0;
 }
 
+static void ftgmac100_config_pause(struct ftgmac100 *priv)
+{
+   u32 fcr = FTGMAC100_FCR_PAUSE_TIME(16);
+
+   /* Throttle tx queue when receiving pause frames */
+   if (priv->rx_pause)
+   fcr |= FTGMAC100_FCR_FC_EN;
+
+   /* Enables sending pause frames when the RX queue is past a
+* certain threshold.
+*/
+   if (priv->tx_pause)
+   fcr |= FTGMAC100_FCR_FCTHR_EN;
+
+   iowrite32(fcr, priv->base + FTGMAC100_OFFSET_FCR);
+}
+
 static void ftgmac100_init_hw(struct ftgmac100 *priv)
 {
u32 reg, rfifo_sz, tfifo_sz;
@@ -910,6 +932,7 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 {
struct ftgmac100 *priv = netdev_priv(netdev);
struct phy_device *phydev = netdev->phydev;
+   bool tx_pause, rx_pause;
int new_speed;
 
/* We store "no link" as speed 0 */
@@ -918,8 +941,21 @@ static void ftgmac100_adjust_link(struct net_device 
*netdev)
else
new_speed = phydev->speed;
 
+   /* Grab pause settings from PHY if configured to do so */
+   if (priv->aneg_pause) {
+   rx_pause = tx_pause = phydev->pause;
+   if (phydev->asym_pause)
+   tx_pause = !rx_pause;
+   } else {
+   rx_pause = priv->rx_pause;
+   tx_pause = priv->tx_pause;
+   }
+
+   /* Link hasn't changed, do nothing */
if (phydev->speed == priv->cur_speed &&
-   phydev->duplex == priv->cur_duplex)
+   phydev->duplex == priv->cur_duplex &&
+   rx_pause == priv->rx_pause &&
+   tx_pause == priv->tx_pause)
return;
 
/* Print status if we have a link or we had one and just lost it,
@@ -930,6 +966,8 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 
priv->cur_speed = new_speed;
priv->cur_duplex = phydev->duplex;
+   priv->rx_pause = rx_pause;
+   priv->tx_pause = tx_pause;
 
/* Link is down, do nothing else */
if (!new_speed)
@@ -961,6 +999,12 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv)
return PTR_ERR(phydev);
}
 
+   /* Indicate that we support PAUSE frames (see comment in
+* Documentation/networking/phy.txt)
+*/
+   phydev->supported |= SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+   phydev->advertising = phydev->supported;
+
return 0;
 }
 
@@ -1083,6 +1127,48 @@ static int ftgmac100_set_ringparam(struct net_device 
*netdev,
return 0;
 }
 
+static void ftgmac100_get_pauseparam(struct net_device *netdev,
+struct ethtool_pauseparam *pause)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+
+   pause->autoneg = priv->aneg_pause;
+   pause->tx_pause = priv->tx_pause;
+   pause->rx_pause = priv->rx_pause;
+}
+
+static int ftgmac100_set_pauseparam(struct net_device *netdev,
+   struct ethtool_pauseparam *pause)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   struct phy_device *phydev = netdev->phydev;
+
+   priv->aneg_pause = pause->autoneg;
+   priv->tx_pause = pause->tx_pause;
+   priv->rx_pause = pause->rx_pause;
+
+   if (phydev) {
+   phydev->advertising &= ~ADVERTISED_Pause;
+   phydev->advertising &= ~ADVERTISED_Asym_Pause;
+
+   if (pause->rx_pause) {
+   phydev->advertising |= ADVERTISED_Pause;
+   phydev->advertising |= ADVERTISED_Asym_Pause;
+   }
+
+   if (pause->tx_pause)
+   phydev->advertising ^= ADVERTISED_Asym_Pause;
+   }
+   if (netif_running(netdev)) {
+

[PATCH 3/8] ftgmac100: Add ndo_set_rx_mode() and support for multicast & promisc

[Benjamin Herrenschmidt]

This adds the ndo_set_rx_mode() callback to configure the
multicast filters, promisc and allmulti options.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 52 
 1 file changed, 52 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 4f3ec2c..d86b27f 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -97,6 +98,10 @@ struct ftgmac100 {
int cur_duplex;
bool use_ncsi;
 
+   /* Multicast filter settings */
+   u32 maht0;
+   u32 maht1;
+
/* Flow control settings */
bool tx_pause;
bool rx_pause;
@@ -264,6 +269,10 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
/* Write MAC address */
ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
 
+   /* Write multicast filter */
+   iowrite32(priv->maht0, priv->base + FTGMAC100_OFFSET_MAHT0);
+   iowrite32(priv->maht1, priv->base + FTGMAC100_OFFSET_MAHT1);
+
/* Configure descriptor sizes and increase burst sizes according
 * to values in Aspeed SDK. The FIFO arbitration is enabled and
 * the thresholds set based on the recommended values in the
@@ -317,6 +326,12 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
/* Add other bits as needed */
if (priv->cur_duplex == DUPLEX_FULL)
maccr |= FTGMAC100_MACCR_FULLDUP;
+   if (priv->netdev->flags & IFF_PROMISC)
+   maccr |= FTGMAC100_MACCR_RX_ALL;
+   if (priv->netdev->flags & IFF_ALLMULTI)
+   maccr |= FTGMAC100_MACCR_RX_MULTIPKT;
+   else if (netdev_mc_count(priv->netdev))
+   maccr |= FTGMAC100_MACCR_HT_MULTI_EN;
 
/* Hit the HW */
iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
@@ -327,6 +342,42 @@ static void ftgmac100_stop_hw(struct ftgmac100 *priv)
iowrite32(0, priv->base + FTGMAC100_OFFSET_MACCR);
 }
 
+static void ftgmac100_calc_mc_hash(struct ftgmac100 *priv)
+{
+   struct netdev_hw_addr *ha;
+
+   priv->maht1 = 0;
+   priv->maht0 = 0;
+   netdev_for_each_mc_addr(ha, priv->netdev) {
+   u32 crc_val = ether_crc_le(ETH_ALEN, ha->addr);
+
+   crc_val = (~(crc_val >> 2)) & 0x3f;
+   if (crc_val >= 32)
+   priv->maht1 |= 1ul << (crc_val - 32);
+   else
+   priv->maht0 |= 1ul << (crc_val);
+   }
+}
+
+static void ftgmac100_set_rx_mode(struct net_device *netdev)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+
+   /* Setup the hash filter */
+   ftgmac100_calc_mc_hash(priv);
+
+   /* Interface down ? that's all there is to do */
+   if (!netif_running(netdev))
+   return;
+
+   /* Update the HW */
+   iowrite32(priv->maht0, priv->base + FTGMAC100_OFFSET_MAHT0);
+   iowrite32(priv->maht1, priv->base + FTGMAC100_OFFSET_MAHT1);
+
+   /* Reconfigure MACCR */
+   ftgmac100_start_hw(priv);
+}
+
 static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, unsigned int entry,
  struct ftgmac100_rxdes *rxdes, gfp_t gfp)
 {
@@ -1508,6 +1559,7 @@ static const struct net_device_ops ftgmac100_netdev_ops = 
{
.ndo_validate_addr  = eth_validate_addr,
.ndo_do_ioctl   = ftgmac100_do_ioctl,
.ndo_tx_timeout = ftgmac100_tx_timeout,
+   .ndo_set_rx_mode= ftgmac100_set_rx_mode,
 };
 
 static int ftgmac100_setup_mdio(struct net_device *netdev)
-- 
2.9.3

[PATCH 1/8] ftgmac100: Add ethtool n-way reset call

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 796b37e..bbeb8e7 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1043,11 +1043,11 @@ static void ftgmac100_get_drvinfo(struct net_device 
*netdev,
strlcpy(info->bus_info, dev_name(>dev), sizeof(info->bus_info));
 }
 
-static int ftgmac100_nway_reset(struct net_device *ndev)
+static int ftgmac100_nway_reset(struct net_device *netdev)
 {
-   if (!ndev->phydev)
+   if (!netdev->phydev)
return -ENXIO;
-   return phy_start_aneg(ndev->phydev);
+   return phy_start_aneg(netdev->phydev);
 }
 
 static void ftgmac100_get_ringparam(struct net_device *netdev,
@@ -1088,6 +1088,7 @@ static const struct ethtool_ops ftgmac100_ethtool_ops = {
.get_link   = ethtool_op_get_link,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
+   .nway_reset = ftgmac100_nway_reset,
.get_ringparam  = ftgmac100_get_ringparam,
.set_ringparam  = ftgmac100_set_ringparam,
 };
-- 
2.9.3

[PATCH 0/8] ftgmac100: Rework batch 5 - Features

[Benjamin Herrenschmidt]

This is fifth and last batch of updates to the ftgmac100 driver.

This contains a few additional "features" such as:

 - Support for ethtool n-way reset
 - Multicast filtering & promisc support
 - Vlan offload
 - netpoll

And a couple of misc bits. This also adds the device-tree binding
documentation.

Re: [PATCH v2 00/10] ftgmac100: Rework batch 4 - Misc

[Benjamin Herrenschmidt]

On Wed, 2017-04-12 at 10:19 -0400, David Miller wrote:
> 
> > v2 Fixes patch 1/10 (NETIF_F_HW_CSUM conversion)
> > 
> > The next (and last) batch will add a few more "features" such
> > as netpoll, multicast/promist, vlan offload...
> > 
> 
> Series applied, thanks Benjamin.
> 
> I really like how you use the reset task to implement ring resizing.

Thanks :-)

I didn't see the point of doing it synchronously as it's not "urgent"
and this keeps the code a lot simpler.

Cheers,
Ben.

Re: Network driver "test suite"

[Benjamin Herrenschmidt]

On Tue, 2017-04-11 at 17:36 -0700, Florian Fainelli wrote:
> 
> You could start with using LNST:
> 
> https://github.com/jpirko/lnst
> 
> and there is also Ostinato which is a great way to get access to
> something IXIA-like, but all configurable in software through python
> bindings. Andrew's dsa-tests make use of it, but they would not be
> directly portable here [1].
> 
> [1]: https://github.com/lunn/dsa-tests

Thanks. I'll play with these when I have a bit of spare time !

Cheers,
Ben.

[PATCH v2 06/10] ftgmac100: Rename ftgmac100_setup_mac to ftgmac100_initial_mac

[Benjamin Herrenschmidt]

To remove more confusion. This function is about obtaining the
initial MAC address at driver probe time.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 4bcd22b..35c54e8 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -182,7 +182,7 @@ static void ftgmac100_write_mac_addr(struct ftgmac100 
*priv, const u8 *mac)
iowrite32(laddr, priv->base + FTGMAC100_OFFSET_MAC_LADR);
 }
 
-static void ftgmac100_setup_mac(struct ftgmac100 *priv)
+static void ftgmac100_initial_mac(struct ftgmac100 *priv)
 {
u8 mac[ETH_ALEN];
unsigned int m;
@@ -1440,7 +1440,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
netdev->irq = irq;
 
/* MAC address from chip or random one */
-   ftgmac100_setup_mac(priv);
+   ftgmac100_initial_mac(priv);
 
np = pdev->dev.of_node;
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
-- 
2.9.3

[PATCH v2 05/10] ftgmac100: Rename ftgmac100_set_mac to ftgmac100_write_mac_addr

[Benjamin Herrenschmidt]

To avoid confusion with the ndo callback and generally be
clearer about the purpose of that function

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index cef5909..4bcd22b 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -173,7 +173,7 @@ static int ftgmac100_reset_and_config_mac(struct ftgmac100 
*priv)
return ftgmac100_reset_mac(priv, maccr);
 }
 
-static void ftgmac100_set_mac(struct ftgmac100 *priv, const unsigned char *mac)
+static void ftgmac100_write_mac_addr(struct ftgmac100 *priv, const u8 *mac)
 {
unsigned int maddr = mac[0] << 8 | mac[1];
unsigned int laddr = mac[2] << 24 | mac[3] << 16 | mac[4] << 8 | mac[5];
@@ -226,7 +226,7 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
return ret;
 
eth_commit_mac_addr_change(dev, p);
-   ftgmac100_set_mac(netdev_priv(dev), dev->dev_addr);
+   ftgmac100_write_mac_addr(netdev_priv(dev), dev->dev_addr);
 
return 0;
 }
@@ -245,7 +245,7 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
 
iowrite32(FTGMAC100_APTC_RXPOLL_CNT(1), priv->base + 
FTGMAC100_OFFSET_APTC);
 
-   ftgmac100_set_mac(priv, priv->netdev->dev_addr);
+   ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
 }
 
 static void ftgmac100_start_hw(struct ftgmac100 *priv)
-- 
2.9.3

[PATCH v2 09/10] ftgmac100: Make ring sizes configurable via ethtool

[Benjamin Herrenschmidt]

We set an arbitrary max at 1024 since we pre-allocate the actual
descriptor arrays and skb arrays to the full size to keep the
code a bit simpler and avoid allocation failures in the reset
task.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 204 ++-
 1 file changed, 148 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 96153f8..0b92bde 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -38,8 +38,15 @@
 #define DRV_NAME   "ftgmac100"
 #define DRV_VERSION"0.7"
 
-#define RX_QUEUE_ENTRIES   256 /* must be power of 2 */
-#define TX_QUEUE_ENTRIES   512 /* must be power of 2 */
+/* Arbitrary values, I am not sure the HW has limits */
+#define MAX_RX_QUEUE_ENTRIES   1024
+#define MAX_TX_QUEUE_ENTRIES   1024
+#define MIN_RX_QUEUE_ENTRIES   32
+#define MIN_TX_QUEUE_ENTRIES   32
+
+/* Defaults */
+#define DEF_RX_QUEUE_ENTRIES   256
+#define DEF_TX_QUEUE_ENTRIES   512
 
 #define MAX_PKT_SIZE   1536
 #define RX_BUF_SIZEMAX_PKT_SIZE/* must be smaller than 0x3fff 
*/
@@ -47,30 +54,32 @@
 /* Min number of tx ring entries before stopping queue */
 #define TX_THRESHOLD   (MAX_SKB_FRAGS + 1)
 
-struct ftgmac100_descs {
-   struct ftgmac100_rxdes rxdes[RX_QUEUE_ENTRIES];
-   struct ftgmac100_txdes txdes[TX_QUEUE_ENTRIES];
-};
-
 struct ftgmac100 {
/* Registers */
struct resource *res;
void __iomem *base;
 
-   struct ftgmac100_descs *descs;
-   dma_addr_t descs_dma_addr;
-
/* Rx ring */
-   struct sk_buff *rx_skbs[RX_QUEUE_ENTRIES];
+   unsigned int rx_q_entries;
+   struct ftgmac100_rxdes *rxdes;
+   dma_addr_t rxdes_dma;
+   struct sk_buff **rx_skbs;
unsigned int rx_pointer;
u32 rxdes0_edorr_mask;
 
/* Tx ring */
-   struct sk_buff *tx_skbs[TX_QUEUE_ENTRIES];
+   unsigned int tx_q_entries;
+   struct ftgmac100_txdes *txdes;
+   dma_addr_t txdes_dma;
+   struct sk_buff **tx_skbs;
unsigned int tx_clean_pointer;
unsigned int tx_pointer;
u32 txdes0_edotr_mask;
 
+   /* Used to signal the reset task of ring change request */
+   unsigned int new_rx_q_entries;
+   unsigned int new_tx_q_entries;
+
/* Scratch page to use when rx skb alloc fails */
void *rx_scratch;
dma_addr_t rx_scratch_dma;
@@ -217,14 +226,10 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
iowrite32(reg, priv->base + FTGMAC100_OFFSET_ISR);
 
/* Setup RX ring buffer base */
-   iowrite32(priv->descs_dma_addr +
- offsetof(struct ftgmac100_descs, rxdes),
- priv->base + FTGMAC100_OFFSET_RXR_BADR);
+   iowrite32(priv->rxdes_dma, priv->base + FTGMAC100_OFFSET_RXR_BADR);
 
/* Setup TX ring buffer base */
-   iowrite32(priv->descs_dma_addr +
- offsetof(struct ftgmac100_descs, txdes),
- priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
+   iowrite32(priv->txdes_dma, priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
 
/* Configure RX buffer size */
iowrite32(FTGMAC100_RBSR_SIZE(RX_BUF_SIZE),
@@ -337,7 +342,7 @@ static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, 
unsigned int entry,
dma_wmb();
 
/* Clean status (which resets own bit) */
-   if (entry == (RX_QUEUE_ENTRIES - 1))
+   if (entry == (priv->rx_q_entries - 1))
rxdes->rxdes0 = cpu_to_le32(priv->rxdes0_edorr_mask);
else
rxdes->rxdes0 = 0;
@@ -345,9 +350,10 @@ static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, 
unsigned int entry,
return 0;
 }
 
-static int ftgmac100_next_rx_pointer(int pointer)
+static unsigned int ftgmac100_next_rx_pointer(struct ftgmac100 *priv,
+ unsigned int pointer)
 {
-   return (pointer + 1) & (RX_QUEUE_ENTRIES - 1);
+   return (pointer + 1) & (priv->rx_q_entries - 1);
 }
 
 static void ftgmac100_rx_packet_error(struct ftgmac100 *priv, u32 status)
@@ -377,7 +383,7 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int 
*processed)
 
/* Grab next RX descriptor */
pointer = priv->rx_pointer;
-   rxdes = >descs->rxdes[pointer];
+   rxdes = >rxdes[pointer];
 
/* Grab descriptor status */
status = le32_to_cpu(rxdes->rxdes0);
@@ -465,7 +471,7 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int 
*processed)
 
/* Resplenish rx ring */
ftgmac100_alloc_rx_buf(priv, pointer, rxdes, GFP_ATOMIC);
-   priv->rx_pointer = ftgmac100_next_rx_pointer(pointer);
+   priv->rx_pointer = ftgmac100_next_rx_pointer(priv, p

[PATCH v2 01/10] ftgmac100: Upgrade to NETIF_F_HW_CSUM

[Benjamin Herrenschmidt]

The documentation describes NETIF_F_IP_CSUM as deprecated
so let's switch to NETIF_F_HW_CSUM and use the helper to
handle unhandled protocols.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
--

v2. - Properly fallback on unknown IP protocols (side effect
  of fixing a reported coding style issue as well).
---
 drivers/net/ethernet/faraday/ftgmac100.c | 40 
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 98b8956..bfa7d0a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -570,6 +570,26 @@ static void ftgmac100_tx_complete(struct ftgmac100 *priv)
}
 }
 
+static bool ftgmac100_prep_tx_csum(struct sk_buff *skb, u32 *csum_vlan)
+{
+   if (skb->protocol == cpu_to_be16(ETH_P_IP)) {
+   u8 ip_proto = ip_hdr(skb)->protocol;
+
+   *csum_vlan |= FTGMAC100_TXDES1_IP_CHKSUM;
+   switch(ip_proto) {
+   case IPPROTO_TCP:
+   *csum_vlan |= FTGMAC100_TXDES1_TCP_CHKSUM;
+   return true;
+   case IPPROTO_UDP:
+   *csum_vlan |= FTGMAC100_TXDES1_UDP_CHKSUM;
+   return true;
+   case IPPROTO_IP:
+   return true;
+   }
+   }
+   return skb_checksum_help(skb) == 0;
+}
+
 static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
 struct net_device *netdev)
 {
@@ -626,19 +646,9 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
 
/* Setup HW checksumming */
csum_vlan = 0;
-   if (skb->ip_summed == CHECKSUM_PARTIAL) {
-   __be16 protocol = skb->protocol;
-
-   if (protocol == cpu_to_be16(ETH_P_IP)) {
-   u8 ip_proto = ip_hdr(skb)->protocol;
-
-   csum_vlan |= FTGMAC100_TXDES1_IP_CHKSUM;
-   if (ip_proto == IPPROTO_TCP)
-   csum_vlan |= FTGMAC100_TXDES1_TCP_CHKSUM;
-   else if (ip_proto == IPPROTO_UDP)
-   csum_vlan |= FTGMAC100_TXDES1_UDP_CHKSUM;
-   }
-   }
+   if (skb->ip_summed == CHECKSUM_PARTIAL &&
+   !ftgmac100_prep_tx_csum(skb, _vlan))
+   goto drop;
txdes->txdes1 = cpu_to_le32(csum_vlan);
 
/* Next descriptor */
@@ -1463,11 +1473,11 @@ static int ftgmac100_probe(struct platform_device *pdev)
 * when NCSI is enabled on the interface. It doesn't work
 * in that case.
 */
-   netdev->features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM |
+   netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
if (priv->use_ncsi &&
of_get_property(pdev->dev.of_node, "no-hw-checksum", NULL))
-   netdev->features &= ~NETIF_F_IP_CSUM;
+   netdev->features &= ~NETIF_F_HW_CSUM;
 
/* register network device */
err = register_netdev(netdev);
-- 
2.9.3

[PATCH v2 10/10] ftgmac100: Set default ring sizes to 128 entries

[Benjamin Herrenschmidt]

I haven't seen any improvement above that size on the machines
I've tested with.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 0b92bde..796b37e 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -45,8 +45,8 @@
 #define MIN_TX_QUEUE_ENTRIES   32
 
 /* Defaults */
-#define DEF_RX_QUEUE_ENTRIES   256
-#define DEF_TX_QUEUE_ENTRIES   512
+#define DEF_RX_QUEUE_ENTRIES   128
+#define DEF_TX_QUEUE_ENTRIES   128
 
 #define MAX_PKT_SIZE   1536
 #define RX_BUF_SIZEMAX_PKT_SIZE/* must be smaller than 0x3fff 
*/
-- 
2.9.3

[PATCH v2 08/10] ftgmac100: Add more register inits in ftgmac100_init_hw()

[Benjamin Herrenschmidt]

Clear stale interrupts on entry, configure FIFO sizes, set FIFO
thresholds, configure interrupt mitigation.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index c679a09..96153f8 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -210,7 +210,11 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
 
 static void ftgmac100_init_hw(struct ftgmac100 *priv)
 {
+   u32 reg, rfifo_sz, tfifo_sz;
 
+   /* Clear stale interrupts */
+   reg = ioread32(priv->base + FTGMAC100_OFFSET_ISR);
+   iowrite32(reg, priv->base + FTGMAC100_OFFSET_ISR);
 
/* Setup RX ring buffer base */
iowrite32(priv->descs_dma_addr +
@@ -232,6 +236,38 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
 
/* Write MAC address */
ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
+
+   /* Configure descriptor sizes and increase burst sizes according
+* to values in Aspeed SDK. The FIFO arbitration is enabled and
+* the thresholds set based on the recommended values in the
+* AST2400 specification.
+*/
+   iowrite32(FTGMAC100_DBLAC_RXDES_SIZE(2) |   /* 2*8 bytes RX descs */
+ FTGMAC100_DBLAC_TXDES_SIZE(2) |   /* 2*8 bytes TX descs */
+ FTGMAC100_DBLAC_RXBURST_SIZE(3) | /* 512 bytes max RX bursts 
*/
+ FTGMAC100_DBLAC_TXBURST_SIZE(3) | /* 512 bytes max TX bursts 
*/
+ FTGMAC100_DBLAC_RX_THR_EN |   /* Enable fifo threshold 
arb */
+ FTGMAC100_DBLAC_RXFIFO_HTHR(6) |  /* 6/8 of FIFO high 
threshold */
+ FTGMAC100_DBLAC_RXFIFO_LTHR(2),   /* 2/8 of FIFO low 
threshold */
+ priv->base + FTGMAC100_OFFSET_DBLAC);
+
+   /* Interrupt mitigation configured for 1 interrupt/packet. HW interrupt
+* mitigation doesn't seem to provide any benefit with NAPI so leave
+* it at that.
+*/
+   iowrite32(FTGMAC100_ITC_RXINT_THR(1) |
+ FTGMAC100_ITC_TXINT_THR(1),
+ priv->base + FTGMAC100_OFFSET_ITC);
+
+   /* Configure FIFO sizes in the TPAFCR register */
+   reg = ioread32(priv->base + FTGMAC100_OFFSET_FEAR);
+   rfifo_sz = reg & 0x0007;
+   tfifo_sz = (reg >> 3) & 0x0007;
+   reg = ioread32(priv->base + FTGMAC100_OFFSET_TPAFCR);
+   reg &= ~0x3f00;
+   reg |= (tfifo_sz << 27);
+   reg |= (rfifo_sz << 24);
+   iowrite32(reg, priv->base + FTGMAC100_OFFSET_TPAFCR);
 }
 
 static void ftgmac100_start_hw(struct ftgmac100 *priv)
-- 
2.9.3

[PATCH v2 02/10] ftgmac100: Use device "compatible" property, not machine.

[Benjamin Herrenschmidt]

We test for aspeed chips to handle a couple of special cases,
but we do that by checking the machine type which isn't right.

Instead check the actual device compatible property. This also
updates the dtsi files for the aspeed SoC to match.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 arch/arm/boot/dts/aspeed-g4.dtsi |  4 ++--
 arch/arm/boot/dts/aspeed-g5.dtsi |  4 ++--
 drivers/net/ethernet/faraday/ftgmac100.c | 16 +---
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 0b4932c..6068e79 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -42,7 +42,7 @@
};
 
mac0: ethernet@1e66 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
no-hw-checksum;
@@ -50,7 +50,7 @@
};
 
mac1: ethernet@1e68 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
no-hw-checksum;
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index b664fe3..4dbe91a 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -33,7 +33,7 @@
};
 
mac0: ethernet@1e66 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
no-hw-checksum;
@@ -41,7 +41,7 @@
};
 
mac1: ethernet@1e68 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
no-hw-checksum;
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index bfa7d0a..6a4fed6 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -90,6 +90,7 @@ struct ftgmac100 {
 
/* Misc */
bool need_mac_restart;
+   bool is_aspeed;
 };
 
 static void ftgmac100_set_rx_ring_base(struct ftgmac100 *priv, dma_addr_t addr)
@@ -1320,8 +1321,7 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
if (!priv->mii_bus)
return -EIO;
 
-   if (of_machine_is_compatible("aspeed,ast2400") ||
-   of_machine_is_compatible("aspeed,ast2500")) {
+   if (priv->is_aspeed) {
/* This driver supports the old MDIO interface */
reg = ioread32(priv->base + FTGMAC100_OFFSET_REVR);
reg &= ~FTGMAC100_REVR_NEW_MDIO_INTERFACE;
@@ -1386,6 +1386,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
int irq;
struct net_device *netdev;
struct ftgmac100 *priv;
+   struct device_node *np;
int err = 0;
 
if (!pdev)
@@ -1441,17 +1442,18 @@ static int ftgmac100_probe(struct platform_device *pdev)
/* MAC address from chip or random one */
ftgmac100_setup_mac(priv);
 
-   if (of_machine_is_compatible("aspeed,ast2400") ||
-   of_machine_is_compatible("aspeed,ast2500")) {
+   np = pdev->dev.of_node;
+   if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
+  of_device_is_compatible(np, "aspeed,ast2500-mac"))) {
priv->rxdes0_edorr_mask = BIT(30);
priv->txdes0_edotr_mask = BIT(30);
+   priv->is_aspeed = true;
} else {
priv->rxdes0_edorr_mask = BIT(15);
priv->txdes0_edotr_mask = BIT(15);
}
 
-   if (pdev->dev.of_node &&
-   of_get_property(pdev->dev.of_node, "use-ncsi", NULL)) {
+   if (np && of_get_property(np, "use-ncsi", NULL)) {
if (!IS_ENABLED(CONFIG_NET_NCSI)) {
dev_err(>dev, "NCSI stack not enabled\n");
goto err_ncsi_dev;
@@ -1476,7 +1478,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
if (priv->use_ncsi &&
-   of_get_property(pdev->dev.of_node, "no-hw-checksum", NULL))
+   of_get_property(np, "no-hw-checksum", NULL))
netdev->features &= ~NETIF_F_HW_CSUM;
 
/* register network device */
-- 
2.9.3

[PATCH v2 03/10] ftgmac100: Disable HW checksum generation on AST2400, enable on others

[Benjamin Herrenschmidt]

We found out that HW checksum generation only works from AST2500
onward. This disables it on AST2400 and removes the "no-hw-checksum"
properties in the device-trees. The problem we had wasn't related
to NC-SI.

Also rework the logic testing for that property so it can be used
to disable HW checksum generation and checking regardless of whether
NC-SI is used or not in case other variants out there need this.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 arch/arm/boot/dts/aspeed-g4.dtsi |  2 --
 arch/arm/boot/dts/aspeed-g5.dtsi |  2 --
 drivers/net/ethernet/faraday/ftgmac100.c | 12 ++--
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 6068e79..c79c937 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -45,7 +45,6 @@
compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
-   no-hw-checksum;
status = "disabled";
};
 
@@ -53,7 +52,6 @@
compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
-   no-hw-checksum;
status = "disabled";
};
 
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 4dbe91a..b659663 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -36,7 +36,6 @@
compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
-   no-hw-checksum;
status = "disabled";
};
 
@@ -44,7 +43,6 @@
compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
-   no-hw-checksum;
status = "disabled";
};
 
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 6a4fed6..bd25b6b 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1471,15 +1471,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
goto err_setup_mdio;
}
 
-   /* We have to disable on-chip IP checksum functionality
-* when NCSI is enabled on the interface. It doesn't work
-* in that case.
-*/
+   /* Base feature set */
netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
-   if (priv->use_ncsi &&
-   of_get_property(np, "no-hw-checksum", NULL))
+
+   /* AST2400  doesn't have working HW checksum generation */
+   if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
netdev->features &= ~NETIF_F_HW_CSUM;
+   if (np && of_get_property(np, "no-hw-checksum", NULL))
+   netdev->features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
 
/* register network device */
err = register_netdev(netdev);
-- 
2.9.3

[PATCH v2 07/10] ftgmac100: Open code remaining register writes

[Benjamin Herrenschmidt]

The helpers just take space but don't provide much value. Simple
one line comments are more explanatory.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 53 
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 35c54e8..c679a09 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -93,29 +93,6 @@ struct ftgmac100 {
bool is_aspeed;
 };
 
-static void ftgmac100_set_rx_ring_base(struct ftgmac100 *priv, dma_addr_t addr)
-{
-   iowrite32(addr, priv->base + FTGMAC100_OFFSET_RXR_BADR);
-}
-
-static void ftgmac100_set_rx_buffer_size(struct ftgmac100 *priv,
-   unsigned int size)
-{
-   size = FTGMAC100_RBSR_SIZE(size);
-   iowrite32(size, priv->base + FTGMAC100_OFFSET_RBSR);
-}
-
-static void ftgmac100_set_normal_prio_tx_ring_base(struct ftgmac100 *priv,
-  dma_addr_t addr)
-{
-   iowrite32(addr, priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
-}
-
-static void ftgmac100_txdma_normal_prio_start_polling(struct ftgmac100 *priv)
-{
-   iowrite32(1, priv->base + FTGMAC100_OFFSET_NPTXPD);
-}
-
 static int ftgmac100_reset_mac(struct ftgmac100 *priv, u32 maccr)
 {
struct net_device *netdev = priv->netdev;
@@ -233,18 +210,27 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
 
 static void ftgmac100_init_hw(struct ftgmac100 *priv)
 {
-   /* setup ring buffer base registers */
-   ftgmac100_set_rx_ring_base(priv,
-  priv->descs_dma_addr +
-  offsetof(struct ftgmac100_descs, rxdes));
-   ftgmac100_set_normal_prio_tx_ring_base(priv,
-  priv->descs_dma_addr +
-  offsetof(struct ftgmac100_descs, 
txdes));
 
-   ftgmac100_set_rx_buffer_size(priv, RX_BUF_SIZE);
 
-   iowrite32(FTGMAC100_APTC_RXPOLL_CNT(1), priv->base + 
FTGMAC100_OFFSET_APTC);
+   /* Setup RX ring buffer base */
+   iowrite32(priv->descs_dma_addr +
+ offsetof(struct ftgmac100_descs, rxdes),
+ priv->base + FTGMAC100_OFFSET_RXR_BADR);
 
+   /* Setup TX ring buffer base */
+   iowrite32(priv->descs_dma_addr +
+ offsetof(struct ftgmac100_descs, txdes),
+ priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
+
+   /* Configure RX buffer size */
+   iowrite32(FTGMAC100_RBSR_SIZE(RX_BUF_SIZE),
+ priv->base + FTGMAC100_OFFSET_RBSR);
+
+   /* Set RX descriptor autopoll */
+   iowrite32(FTGMAC100_APTC_RXPOLL_CNT(1),
+ priv->base + FTGMAC100_OFFSET_APTC);
+
+   /* Write MAC address */
ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
 }
 
@@ -704,7 +690,8 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
netif_wake_queue(netdev);
}
 
-   ftgmac100_txdma_normal_prio_start_polling(priv);
+   /* Poke transmitter to read the updated TX descriptors */
+   iowrite32(1, priv->base + FTGMAC100_OFFSET_NPTXPD);
 
return NETDEV_TX_OK;
 
-- 
2.9.3

[PATCH v2 04/10] ftgmac100: Set netdev->hw_features

[Benjamin Herrenschmidt]

So features can be turned on/off via ethtool

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index bd25b6b..cef5909 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1472,14 +1472,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
}
 
/* Base feature set */
-   netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
+   netdev->hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
 
/* AST2400  doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
-   netdev->features &= ~NETIF_F_HW_CSUM;
+   netdev->hw_features &= ~NETIF_F_HW_CSUM;
if (np && of_get_property(np, "no-hw-checksum", NULL))
-   netdev->features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+   netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+   netdev->features |= netdev->hw_features;
 
/* register network device */
err = register_netdev(netdev);
-- 
2.9.3

[PATCH v2 00/10] ftgmac100: Rework batch 4 - Misc

[Benjamin Herrenschmidt]

This is v2 of the fourth batch of updates to the ftgmac100 driver.

This is a bunch of misc cleanups and fixes, such as properly
disabling HW checksum generation on AST2400 where it's known
to be broken and some chip init updates.

This also adds the ability to turn HW checksum on/off and
configure the ring sizes via ethtool.

v2 Fixes patch 1/10 (NETIF_F_HW_CSUM conversion)

The next (and last) batch will add a few more "features" such
as netpoll, multicast/promist, vlan offload...

Network driver "test suite"

[Benjamin Herrenschmidt]

Hi folks !

Does anybody knows of an existing kind of automated "test suite" for a
network/ethernet driver ?

IE. Something we could run both on the "tested" driver and a cross-over 
"known good" peer (possibly the latter set to promisc & no offload for
proper analysis), that would out the driver through a whole bunch of
tests, such as verifying the checksum offload on a various combinations
of headers lenghts and encapsulation, vlan stuff, multicast filters,
etc... ?

I've hacking on a driver recently and ended up "manually" testing a
bunch of these things using a palette of tools (iperf, nuttcp, some
multicast hack I have around, etc... along with tcpdump) but it feels
like this is the kind of things that could be greatly automated.

Cheers,
Ben.

Re: [PATCH 01/10] ftgmac100: Upgrade to NETIF_F_HW_CSUM

[Benjamin Herrenschmidt]

On Tue, 2017-04-11 at 20:03 -0400, David Miller wrote:
> > From: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> Date: Wed, 12 Apr 2017 09:36:05 +1000
> 
> > I should call the helper when I don't recognize the protocol type in
> > the IP header, not just when the main skb protocol type is not IP.
> 
> That's correct.
> 
> > BTW. I'm not too familiar with how encapsulation works these days. I
> > wouldn't throw at that HW anything other than unencapsulated packets
> > for HW checksumming. Is checking skb->protocol to be IP and
> > ip_hdr(skb)->protocol to be IP, UDP or TCP enough ? IE. Will the latter
> > especially return the outer header ?
> 
> If skb->protocol is IP then yes ip_hdr() will point at the outermost
> header.

Great thanks. I'll repost later today in case some other comments still
come in.

Cheers,
Ben.

Re: [PATCH 01/10] ftgmac100: Upgrade to NETIF_F_HW_CSUM

[Benjamin Herrenschmidt]

On Wed, 2017-04-12 at 08:06 +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2017-04-11 at 11:27 -0400, David Miller wrote:
> > > I'll fix it in a next spin if Dave wants it that way but
> > > otherwise
> > > I'm keen to leave it as it is.
> > 
> > Please fix this and respin.
> > 
> > Meanwhile get the coding style rules changed if you disagree with
> > them.  A patch series review is not the place to argue about your
> > disagreement with the coding style rules.
> 
> I will fix.

Funny thing is, I think the code is wrong :)

I should call the helper when I don't recognize the protocol type
in the IP header, not just when the main skb protocol type is not IP.

BTW. I'm not too familiar with how encapsulation works these days. I
wouldn't throw at that HW anything other than unencapsulated packets
for HW checksumming. Is checking skb->protocol to be IP and
ip_hdr(skb)->protocol to be IP, UDP or TCP enough ? IE. Will the latter
especially return the outer header ?

Cheers,
Ben.

Re: [PATCH 01/10] ftgmac100: Upgrade to NETIF_F_HW_CSUM

[Benjamin Herrenschmidt]

On Tue, 2017-04-11 at 11:27 -0400, David Miller wrote:
> > I'll fix it in a next spin if Dave wants it that way but otherwise
> > I'm keen to leave it as it is.
> 
> Please fix this and respin.
> 
> Meanwhile get the coding style rules changed if you disagree with
> them.  A patch series review is not the place to argue about your
> disagreement with the coding style rules.

I will fix.

Note that I don't disagree with the rule as stated.

However I'd like to point out that the rule doesn't precisely match
the construct here as it's for a dangling single else while what I
had here is an else if ... so it's open to intepretation :-)

I also tend to disagree that coding style rules should be firm laws,
imho they should be considered in context and broken if they render
a given piece of code less clear.

That said, I will respin.

Cheers,
Ben.

Re: [PATCH 01/10] ftgmac100: Upgrade to NETIF_F_HW_CSUM

[Benjamin Herrenschmidt]

On Tue, 2017-04-11 at 21:13 +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2017-04-11 at 13:57 +0300, Sergei Shtylyov wrote:
> >     Need {} here as well since the 1st branch has it -- see 
> > Documentation/process/coding-style.rst (the end of the section 3).
> 
> Adding {} in that specific statements just makes things more
> cluttered and less readable.
> 
> I can find a ton of examples of 
> 
>   if (...) {
>   multi lines
>   ...
>   } else if (...)
>   single_line()
> 
> In existing kernel code.
> 
> I'll fix it in a next spin if Dave wants it that way but otherwise
> I'm keen to leave it as it is.

I actually took the time to read the coding-style.txt statement,
and I would argue that it is about

if (...) {
...
} else {
...
}

Which is a different can of worms. I tend to agree that the else by
itself followed by a single tabulated statement is a bit "odd" and
I tend to use braces in that case as well.

However the "} else if (...)" case is subtly different and from a code
clarity point of view I prefer what I wrote.

This isn't an accident, I specifically wrote that statement that way
because I preferred how the code looked like that way.

This tend to be my problem with coding style rules (and people who
comment on patches solely on coding style issues, especially so
marginal ones ;-) ... this needs to be applied along with a bit of
common sense and taste.

Those rules should be "general guidelines" not absolute laws. A
specific piece of code might benefit from violating some of them
occasionally for all sort of reasons provided the end result is both
clear and more concise for example.

Anyway, way too much internet bandwidth wasted today on that subject.

Dave, just let me know how you want it to look like :-)

Cheers,
Ben.
  

Re: [PATCH 01/10] ftgmac100: Upgrade to NETIF_F_HW_CSUM

[Benjamin Herrenschmidt]

On Tue, 2017-04-11 at 13:57 +0300, Sergei Shtylyov wrote:
>     Need {} here as well since the 1st branch has it -- see 
> Documentation/process/coding-style.rst (the end of the section 3).

Adding {} in that specific statements just makes things more
cluttered and less readable.

I can find a ton of examples of 

if (...) {
multi lines
...
} else if (...)
single_line()

In existing kernel code.

I'll fix it in a next spin if Dave wants it that way but otherwise
I'm keen to leave it as it is.

Ben.

[PATCH 02/10] ftgmac100: Use device "compatible" property, not machine.

[Benjamin Herrenschmidt]

We test for aspeed chips to handle a couple of special cases,
but we do that by checking the machine type which isn't right.

Instead check the actual device compatible property. This also
updates the dtsi files for the aspeed SoC to match.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 arch/arm/boot/dts/aspeed-g4.dtsi |  4 ++--
 arch/arm/boot/dts/aspeed-g5.dtsi |  4 ++--
 drivers/net/ethernet/faraday/ftgmac100.c | 16 +---
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 0b4932c..6068e79 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -42,7 +42,7 @@
};
 
mac0: ethernet@1e66 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
no-hw-checksum;
@@ -50,7 +50,7 @@
};
 
mac1: ethernet@1e68 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
no-hw-checksum;
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index b664fe3..4dbe91a 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -33,7 +33,7 @@
};
 
mac0: ethernet@1e66 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
no-hw-checksum;
@@ -41,7 +41,7 @@
};
 
mac1: ethernet@1e68 {
-   compatible = "faraday,ftgmac100";
+   compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
no-hw-checksum;
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 85b650a..5e04717 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -90,6 +90,7 @@ struct ftgmac100 {
 
/* Misc */
bool need_mac_restart;
+   bool is_aspeed;
 };
 
 static void ftgmac100_set_rx_ring_base(struct ftgmac100 *priv, dma_addr_t addr)
@@ -1311,8 +1312,7 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
if (!priv->mii_bus)
return -EIO;
 
-   if (of_machine_is_compatible("aspeed,ast2400") ||
-   of_machine_is_compatible("aspeed,ast2500")) {
+   if (priv->is_aspeed) {
/* This driver supports the old MDIO interface */
reg = ioread32(priv->base + FTGMAC100_OFFSET_REVR);
reg &= ~FTGMAC100_REVR_NEW_MDIO_INTERFACE;
@@ -1377,6 +1377,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
int irq;
struct net_device *netdev;
struct ftgmac100 *priv;
+   struct device_node *np;
int err = 0;
 
if (!pdev)
@@ -1432,17 +1433,18 @@ static int ftgmac100_probe(struct platform_device *pdev)
/* MAC address from chip or random one */
ftgmac100_setup_mac(priv);
 
-   if (of_machine_is_compatible("aspeed,ast2400") ||
-   of_machine_is_compatible("aspeed,ast2500")) {
+   np = pdev->dev.of_node;
+   if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
+  of_device_is_compatible(np, "aspeed,ast2500-mac"))) {
priv->rxdes0_edorr_mask = BIT(30);
priv->txdes0_edotr_mask = BIT(30);
+   priv->is_aspeed = true;
} else {
priv->rxdes0_edorr_mask = BIT(15);
priv->txdes0_edotr_mask = BIT(15);
}
 
-   if (pdev->dev.of_node &&
-   of_get_property(pdev->dev.of_node, "use-ncsi", NULL)) {
+   if (np && of_get_property(np, "use-ncsi", NULL)) {
if (!IS_ENABLED(CONFIG_NET_NCSI)) {
dev_err(>dev, "NCSI stack not enabled\n");
goto err_ncsi_dev;
@@ -1467,7 +1469,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
if (priv->use_ncsi &&
-   of_get_property(pdev->dev.of_node, "no-hw-checksum", NULL))
+   of_get_property(np, "no-hw-checksum", NULL))
netdev->features &= ~NETIF_F_HW_CSUM;
 
/* register network device */
-- 
2.9.3

Re: [PATCH 00/10] ftgmac100: Rework batch 4 - Misc

[Benjamin Herrenschmidt]

On Tue, 2017-04-11 at 11:04 +1000, Benjamin Herrenschmidt wrote:
> This is the fourth batch of updates to the ftgmac100 driver.
> 
> This is a bunch of misc cleanups and fixes, such as properly
> disabling HW checksum generation on AST2400 where it's known
> to be broken and some chip init updates.
> 
> This also adds the ability to turn HW checksum on/off and
> configure the ring sizes via ethtool.
> 
> The next (and last) batch will add a few more "features" such
> as netpoll, multicast/promist, vlan offload...

FYI. This series plays a bit with the device-tree properties for
this driver. There is more in the last batch, which will also
include an patch adding 

Documentation/devicetree/bindings/net/ftgmac100.txt

documenting all this.

Cheers,
Ben.

[PATCH 10/10] ftgmac100: Set default ring sizes to 128 entries

[Benjamin Herrenschmidt]

I haven't seen any improvement above that size on the machines
I've tested with.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 2f0b73b..af11e2b 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -45,8 +45,8 @@
 #define MIN_TX_QUEUE_ENTRIES   32
 
 /* Defaults */
-#define DEF_RX_QUEUE_ENTRIES   256
-#define DEF_TX_QUEUE_ENTRIES   512
+#define DEF_RX_QUEUE_ENTRIES   128
+#define DEF_TX_QUEUE_ENTRIES   128
 
 #define MAX_PKT_SIZE   1536
 #define RX_BUF_SIZEMAX_PKT_SIZE/* must be smaller than 0x3fff 
*/
-- 
2.9.3

[PATCH 09/10] ftgmac100: Make ring sizes configurable via ethtool

[Benjamin Herrenschmidt]

We set an arbitrary max at 1024 since we pre-allocate the actual
descriptor arrays and skb arrays to the full size to keep the
code a bit simpler and avoid allocation failures in the reset
task.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 204 ++-
 1 file changed, 148 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index d4fe599..2f0b73b 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -38,8 +38,15 @@
 #define DRV_NAME   "ftgmac100"
 #define DRV_VERSION"0.7"
 
-#define RX_QUEUE_ENTRIES   256 /* must be power of 2 */
-#define TX_QUEUE_ENTRIES   512 /* must be power of 2 */
+/* Arbitrary values, I am not sure the HW has limits */
+#define MAX_RX_QUEUE_ENTRIES   1024
+#define MAX_TX_QUEUE_ENTRIES   1024
+#define MIN_RX_QUEUE_ENTRIES   32
+#define MIN_TX_QUEUE_ENTRIES   32
+
+/* Defaults */
+#define DEF_RX_QUEUE_ENTRIES   256
+#define DEF_TX_QUEUE_ENTRIES   512
 
 #define MAX_PKT_SIZE   1536
 #define RX_BUF_SIZEMAX_PKT_SIZE/* must be smaller than 0x3fff 
*/
@@ -47,30 +54,32 @@
 /* Min number of tx ring entries before stopping queue */
 #define TX_THRESHOLD   (MAX_SKB_FRAGS + 1)
 
-struct ftgmac100_descs {
-   struct ftgmac100_rxdes rxdes[RX_QUEUE_ENTRIES];
-   struct ftgmac100_txdes txdes[TX_QUEUE_ENTRIES];
-};
-
 struct ftgmac100 {
/* Registers */
struct resource *res;
void __iomem *base;
 
-   struct ftgmac100_descs *descs;
-   dma_addr_t descs_dma_addr;
-
/* Rx ring */
-   struct sk_buff *rx_skbs[RX_QUEUE_ENTRIES];
+   unsigned int rx_q_entries;
+   struct ftgmac100_rxdes *rxdes;
+   dma_addr_t rxdes_dma;
+   struct sk_buff **rx_skbs;
unsigned int rx_pointer;
u32 rxdes0_edorr_mask;
 
/* Tx ring */
-   struct sk_buff *tx_skbs[TX_QUEUE_ENTRIES];
+   unsigned int tx_q_entries;
+   struct ftgmac100_txdes *txdes;
+   dma_addr_t txdes_dma;
+   struct sk_buff **tx_skbs;
unsigned int tx_clean_pointer;
unsigned int tx_pointer;
u32 txdes0_edotr_mask;
 
+   /* Used to signal the reset task of ring change request */
+   unsigned int new_rx_q_entries;
+   unsigned int new_tx_q_entries;
+
/* Scratch page to use when rx skb alloc fails */
void *rx_scratch;
dma_addr_t rx_scratch_dma;
@@ -217,14 +226,10 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
iowrite32(reg, priv->base + FTGMAC100_OFFSET_ISR);
 
/* Setup RX ring buffer base */
-   iowrite32(priv->descs_dma_addr +
- offsetof(struct ftgmac100_descs, rxdes),
- priv->base + FTGMAC100_OFFSET_RXR_BADR);
+   iowrite32(priv->rxdes_dma, priv->base + FTGMAC100_OFFSET_RXR_BADR);
 
/* Setup TX ring buffer base */
-   iowrite32(priv->descs_dma_addr +
- offsetof(struct ftgmac100_descs, txdes),
- priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
+   iowrite32(priv->txdes_dma, priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
 
/* Configure RX buffer size */
iowrite32(FTGMAC100_RBSR_SIZE(RX_BUF_SIZE),
@@ -337,7 +342,7 @@ static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, 
unsigned int entry,
dma_wmb();
 
/* Clean status (which resets own bit) */
-   if (entry == (RX_QUEUE_ENTRIES - 1))
+   if (entry == (priv->rx_q_entries - 1))
rxdes->rxdes0 = cpu_to_le32(priv->rxdes0_edorr_mask);
else
rxdes->rxdes0 = 0;
@@ -345,9 +350,10 @@ static int ftgmac100_alloc_rx_buf(struct ftgmac100 *priv, 
unsigned int entry,
return 0;
 }
 
-static int ftgmac100_next_rx_pointer(int pointer)
+static unsigned int ftgmac100_next_rx_pointer(struct ftgmac100 *priv,
+ unsigned int pointer)
 {
-   return (pointer + 1) & (RX_QUEUE_ENTRIES - 1);
+   return (pointer + 1) & (priv->rx_q_entries - 1);
 }
 
 static void ftgmac100_rx_packet_error(struct ftgmac100 *priv, u32 status)
@@ -377,7 +383,7 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int 
*processed)
 
/* Grab next RX descriptor */
pointer = priv->rx_pointer;
-   rxdes = >descs->rxdes[pointer];
+   rxdes = >rxdes[pointer];
 
/* Grab descriptor status */
status = le32_to_cpu(rxdes->rxdes0);
@@ -465,7 +471,7 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int 
*processed)
 
/* Resplenish rx ring */
ftgmac100_alloc_rx_buf(priv, pointer, rxdes, GFP_ATOMIC);
-   priv->rx_pointer = ftgmac100_next_rx_pointer(pointer);
+   priv->rx_pointer = ftgmac100_next_rx_pointer(priv, p

[PATCH 08/10] ftgmac100: Add more register inits in ftgmac100_init_hw()

[Benjamin Herrenschmidt]

Clear stale interrupts on entry, configure FIFO sizes, set FIFO
thresholds, configure interrupt mitigation.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 3c7a1e4..d4fe599 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -210,7 +210,11 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
 
 static void ftgmac100_init_hw(struct ftgmac100 *priv)
 {
+   u32 reg, rfifo_sz, tfifo_sz;
 
+   /* Clear stale interrupts */
+   reg = ioread32(priv->base + FTGMAC100_OFFSET_ISR);
+   iowrite32(reg, priv->base + FTGMAC100_OFFSET_ISR);
 
/* Setup RX ring buffer base */
iowrite32(priv->descs_dma_addr +
@@ -232,6 +236,38 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
 
/* Write MAC address */
ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
+
+   /* Configure descriptor sizes and increase burst sizes according
+* to values in Aspeed SDK. The FIFO arbitration is enabled and
+* the thresholds set based on the recommended values in the
+* AST2400 specification.
+*/
+   iowrite32(FTGMAC100_DBLAC_RXDES_SIZE(2) |   /* 2*8 bytes RX descs */
+ FTGMAC100_DBLAC_TXDES_SIZE(2) |   /* 2*8 bytes TX descs */
+ FTGMAC100_DBLAC_RXBURST_SIZE(3) | /* 512 bytes max RX bursts 
*/
+ FTGMAC100_DBLAC_TXBURST_SIZE(3) | /* 512 bytes max TX bursts 
*/
+ FTGMAC100_DBLAC_RX_THR_EN |   /* Enable fifo threshold 
arb */
+ FTGMAC100_DBLAC_RXFIFO_HTHR(6) |  /* 6/8 of FIFO high 
threshold */
+ FTGMAC100_DBLAC_RXFIFO_LTHR(2),   /* 2/8 of FIFO low 
threshold */
+ priv->base + FTGMAC100_OFFSET_DBLAC);
+
+   /* Interrupt mitigation configured for 1 interrupt/packet. HW interrupt
+* mitigation doesn't seem to provide any benefit with NAPI so leave
+* it at that.
+*/
+   iowrite32(FTGMAC100_ITC_RXINT_THR(1) |
+ FTGMAC100_ITC_TXINT_THR(1),
+ priv->base + FTGMAC100_OFFSET_ITC);
+
+   /* Configure FIFO sizes in the TPAFCR register */
+   reg = ioread32(priv->base + FTGMAC100_OFFSET_FEAR);
+   rfifo_sz = reg & 0x0007;
+   tfifo_sz = (reg >> 3) & 0x0007;
+   reg = ioread32(priv->base + FTGMAC100_OFFSET_TPAFCR);
+   reg &= ~0x3f00;
+   reg |= (tfifo_sz << 27);
+   reg |= (rfifo_sz << 24);
+   iowrite32(reg, priv->base + FTGMAC100_OFFSET_TPAFCR);
 }
 
 static void ftgmac100_start_hw(struct ftgmac100 *priv)
-- 
2.9.3

[PATCH 05/10] ftgmac100: Rename ftgmac100_set_mac to ftgmac100_write_mac_addr

[Benjamin Herrenschmidt]

To avoid confusion with the ndo callback and generally be
clearer about the purpose of that function

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 257243a..c2053e5 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -173,7 +173,7 @@ static int ftgmac100_reset_and_config_mac(struct ftgmac100 
*priv)
return ftgmac100_reset_mac(priv, maccr);
 }
 
-static void ftgmac100_set_mac(struct ftgmac100 *priv, const unsigned char *mac)
+static void ftgmac100_write_mac_addr(struct ftgmac100 *priv, const u8 *mac)
 {
unsigned int maddr = mac[0] << 8 | mac[1];
unsigned int laddr = mac[2] << 24 | mac[3] << 16 | mac[4] << 8 | mac[5];
@@ -226,7 +226,7 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
return ret;
 
eth_commit_mac_addr_change(dev, p);
-   ftgmac100_set_mac(netdev_priv(dev), dev->dev_addr);
+   ftgmac100_write_mac_addr(netdev_priv(dev), dev->dev_addr);
 
return 0;
 }
@@ -245,7 +245,7 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
 
iowrite32(FTGMAC100_APTC_RXPOLL_CNT(1), priv->base + 
FTGMAC100_OFFSET_APTC);
 
-   ftgmac100_set_mac(priv, priv->netdev->dev_addr);
+   ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
 }
 
 static void ftgmac100_start_hw(struct ftgmac100 *priv)
-- 
2.9.3

[PATCH 04/10] ftgmac100: Set netdev->hw_features

[Benjamin Herrenschmidt]

So features can be turned on/off via ethtool

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 3c68823..257243a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1463,14 +1463,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
}
 
/* Base feature set */
-   netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
+   netdev->hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
 
/* AST2400  doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
-   netdev->features &= ~NETIF_F_HW_CSUM;
+   netdev->hw_features &= ~NETIF_F_HW_CSUM;
if (np && of_get_property(np, "no-hw-checksum", NULL))
-   netdev->features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+   netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+   netdev->features |= netdev->hw_features;
 
/* register network device */
err = register_netdev(netdev);
-- 
2.9.3

[PATCH 07/10] ftgmac100: Open code remaining register writes

[Benjamin Herrenschmidt]

The helpers just take space but don't provide much value. Simple
one line comments are more explanatory.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 53 
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index e92f382..3c7a1e4 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -93,29 +93,6 @@ struct ftgmac100 {
bool is_aspeed;
 };
 
-static void ftgmac100_set_rx_ring_base(struct ftgmac100 *priv, dma_addr_t addr)
-{
-   iowrite32(addr, priv->base + FTGMAC100_OFFSET_RXR_BADR);
-}
-
-static void ftgmac100_set_rx_buffer_size(struct ftgmac100 *priv,
-   unsigned int size)
-{
-   size = FTGMAC100_RBSR_SIZE(size);
-   iowrite32(size, priv->base + FTGMAC100_OFFSET_RBSR);
-}
-
-static void ftgmac100_set_normal_prio_tx_ring_base(struct ftgmac100 *priv,
-  dma_addr_t addr)
-{
-   iowrite32(addr, priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
-}
-
-static void ftgmac100_txdma_normal_prio_start_polling(struct ftgmac100 *priv)
-{
-   iowrite32(1, priv->base + FTGMAC100_OFFSET_NPTXPD);
-}
-
 static int ftgmac100_reset_mac(struct ftgmac100 *priv, u32 maccr)
 {
struct net_device *netdev = priv->netdev;
@@ -233,18 +210,27 @@ static int ftgmac100_set_mac_addr(struct net_device *dev, 
void *p)
 
 static void ftgmac100_init_hw(struct ftgmac100 *priv)
 {
-   /* setup ring buffer base registers */
-   ftgmac100_set_rx_ring_base(priv,
-  priv->descs_dma_addr +
-  offsetof(struct ftgmac100_descs, rxdes));
-   ftgmac100_set_normal_prio_tx_ring_base(priv,
-  priv->descs_dma_addr +
-  offsetof(struct ftgmac100_descs, 
txdes));
 
-   ftgmac100_set_rx_buffer_size(priv, RX_BUF_SIZE);
 
-   iowrite32(FTGMAC100_APTC_RXPOLL_CNT(1), priv->base + 
FTGMAC100_OFFSET_APTC);
+   /* Setup RX ring buffer base */
+   iowrite32(priv->descs_dma_addr +
+ offsetof(struct ftgmac100_descs, rxdes),
+ priv->base + FTGMAC100_OFFSET_RXR_BADR);
 
+   /* Setup TX ring buffer base */
+   iowrite32(priv->descs_dma_addr +
+ offsetof(struct ftgmac100_descs, txdes),
+ priv->base + FTGMAC100_OFFSET_NPTXR_BADR);
+
+   /* Configure RX buffer size */
+   iowrite32(FTGMAC100_RBSR_SIZE(RX_BUF_SIZE),
+ priv->base + FTGMAC100_OFFSET_RBSR);
+
+   /* Set RX descriptor autopoll */
+   iowrite32(FTGMAC100_APTC_RXPOLL_CNT(1),
+ priv->base + FTGMAC100_OFFSET_APTC);
+
+   /* Write MAC address */
ftgmac100_write_mac_addr(priv, priv->netdev->dev_addr);
 }
 
@@ -695,7 +681,8 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
netif_wake_queue(netdev);
}
 
-   ftgmac100_txdma_normal_prio_start_polling(priv);
+   /* Poke transmitter to read the updated TX descriptors */
+   iowrite32(1, priv->base + FTGMAC100_OFFSET_NPTXPD);
 
return NETDEV_TX_OK;
 
-- 
2.9.3

[PATCH 06/10] ftgmac100: Rename ftgmac100_setup_mac to ftgmac100_initial_mac

[Benjamin Herrenschmidt]

To remove more confusion. This function is about obtaining the
initial MAC address at driver probe time.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index c2053e5..e92f382 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -182,7 +182,7 @@ static void ftgmac100_write_mac_addr(struct ftgmac100 
*priv, const u8 *mac)
iowrite32(laddr, priv->base + FTGMAC100_OFFSET_MAC_LADR);
 }
 
-static void ftgmac100_setup_mac(struct ftgmac100 *priv)
+static void ftgmac100_initial_mac(struct ftgmac100 *priv)
 {
u8 mac[ETH_ALEN];
unsigned int m;
@@ -1431,7 +1431,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
netdev->irq = irq;
 
/* MAC address from chip or random one */
-   ftgmac100_setup_mac(priv);
+   ftgmac100_initial_mac(priv);
 
np = pdev->dev.of_node;
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
-- 
2.9.3

[PATCH 03/10] ftgmac100: Disable HW checksum generation on AST2400, enable on others

[Benjamin Herrenschmidt]

We found out that HW checksum generation only works from AST2500
onward. This disables it on AST2400 and removes the "no-hw-checksum"
properties in the device-trees. The problem we had wasn't related
to NC-SI.

Also rework the logic testing for that property so it can be used
to disable HW checksum generation and checking regardless of whether
NC-SI is used or not in case other variants out there need this.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 arch/arm/boot/dts/aspeed-g4.dtsi |  2 --
 arch/arm/boot/dts/aspeed-g5.dtsi |  2 --
 drivers/net/ethernet/faraday/ftgmac100.c | 12 ++--
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 6068e79..c79c937 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -45,7 +45,6 @@
compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
-   no-hw-checksum;
status = "disabled";
};
 
@@ -53,7 +52,6 @@
compatible = "aspeed,ast2400-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
-   no-hw-checksum;
status = "disabled";
};
 
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 4dbe91a..b659663 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -36,7 +36,6 @@
compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e66 0x180>;
interrupts = <2>;
-   no-hw-checksum;
status = "disabled";
};
 
@@ -44,7 +43,6 @@
compatible = "aspeed,ast2500-mac", "faraday,ftgmac100";
reg = <0x1e68 0x180>;
interrupts = <3>;
-   no-hw-checksum;
status = "disabled";
};
 
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 5e04717..3c68823 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1462,15 +1462,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
goto err_setup_mdio;
}
 
-   /* We have to disable on-chip IP checksum functionality
-* when NCSI is enabled on the interface. It doesn't work
-* in that case.
-*/
+   /* Base feature set */
netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
-   if (priv->use_ncsi &&
-   of_get_property(np, "no-hw-checksum", NULL))
+
+   /* AST2400  doesn't have working HW checksum generation */
+   if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
netdev->features &= ~NETIF_F_HW_CSUM;
+   if (np && of_get_property(np, "no-hw-checksum", NULL))
+   netdev->features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
 
/* register network device */
err = register_netdev(netdev);
-- 
2.9.3

[PATCH 01/10] ftgmac100: Upgrade to NETIF_F_HW_CSUM

[Benjamin Herrenschmidt]

The documentation describes NETIF_F_IP_CSUM as deprecated
so let's switch to NETIF_F_HW_CSUM and use the helper to
handle unhandled protocols.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 98b8956..85b650a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -637,7 +637,8 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
csum_vlan |= FTGMAC100_TXDES1_TCP_CHKSUM;
else if (ip_proto == IPPROTO_UDP)
csum_vlan |= FTGMAC100_TXDES1_UDP_CHKSUM;
-   }
+   } else if (skb_checksum_help(skb))
+   goto drop;
}
txdes->txdes1 = cpu_to_le32(csum_vlan);
 
@@ -1463,11 +1464,11 @@ static int ftgmac100_probe(struct platform_device *pdev)
 * when NCSI is enabled on the interface. It doesn't work
 * in that case.
 */
-   netdev->features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM |
+   netdev->features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM |
NETIF_F_GRO | NETIF_F_SG;
if (priv->use_ncsi &&
of_get_property(pdev->dev.of_node, "no-hw-checksum", NULL))
-   netdev->features &= ~NETIF_F_IP_CSUM;
+   netdev->features &= ~NETIF_F_HW_CSUM;
 
/* register network device */
err = register_netdev(netdev);
-- 
2.9.3

[PATCH 00/10] ftgmac100: Rework batch 4 - Misc

[Benjamin Herrenschmidt]

This is the fourth batch of updates to the ftgmac100 driver.

This is a bunch of misc cleanups and fixes, such as properly
disabling HW checksum generation on AST2400 where it's known
to be broken and some chip init updates.

This also adds the ability to turn HW checksum on/off and
configure the ring sizes via ethtool.

The next (and last) batch will add a few more "features" such
as netpoll, multicast/promist, vlan offload...

[PATCH v2 02/12] ftgmac100: Move ftgmac100_hard_start_xmit() around

[Benjamin Herrenschmidt]

Move it below ftgmac100_xmit() and the rest of the tx path

No code change.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---

v2. - Fix patch splitting mistake
---
 drivers/net/ethernet/faraday/ftgmac100.c | 58 
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index f4b719214..21df322 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -673,6 +673,35 @@ static int ftgmac100_xmit(struct ftgmac100 *priv, struct 
sk_buff *skb,
return NETDEV_TX_OK;
 }
 
+static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
+struct net_device *netdev)
+{
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   dma_addr_t map;
+
+   if (unlikely(skb->len > MAX_PKT_SIZE)) {
+   if (net_ratelimit())
+   netdev_dbg(netdev, "tx packet too big\n");
+
+   netdev->stats.tx_dropped++;
+   kfree_skb(skb);
+   return NETDEV_TX_OK;
+   }
+
+   map = dma_map_single(priv->dev, skb->data, skb_headlen(skb), 
DMA_TO_DEVICE);
+   if (unlikely(dma_mapping_error(priv->dev, map))) {
+   /* drop packet */
+   if (net_ratelimit())
+   netdev_err(netdev, "map socket buffer failed\n");
+
+   netdev->stats.tx_dropped++;
+   kfree_skb(skb);
+   return NETDEV_TX_OK;
+   }
+
+   return ftgmac100_xmit(priv, skb, map);
+}
+
 static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 {
int i;
@@ -1210,35 +1239,6 @@ static int ftgmac100_stop(struct net_device *netdev)
return 0;
 }
 
-static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
-struct net_device *netdev)
-{
-   struct ftgmac100 *priv = netdev_priv(netdev);
-   dma_addr_t map;
-
-   if (unlikely(skb->len > MAX_PKT_SIZE)) {
-   if (net_ratelimit())
-   netdev_dbg(netdev, "tx packet too big\n");
-
-   netdev->stats.tx_dropped++;
-   kfree_skb(skb);
-   return NETDEV_TX_OK;
-   }
-
-   map = dma_map_single(priv->dev, skb->data, skb_headlen(skb), 
DMA_TO_DEVICE);
-   if (unlikely(dma_mapping_error(priv->dev, map))) {
-   /* drop packet */
-   if (net_ratelimit())
-   netdev_err(netdev, "map socket buffer failed\n");
-
-   netdev->stats.tx_dropped++;
-   kfree_skb(skb);
-   return NETDEV_TX_OK;
-   }
-
-   return ftgmac100_xmit(priv, skb, map);
-}
-
 /* optional */
 static int ftgmac100_do_ioctl(struct net_device *netdev, struct ifreq *ifr, 
int cmd)
 {
-- 
2.9.3

[PATCH v2 10/12] ftgmac100: Don't clear tx desc fields unnecessarily

[Benjamin Herrenschmidt]

Those are non-cachable stores, let's avoid those we don't need. Remove
the helper, it's not particularly helpful and since it uses "priv"
I can't move it to the header file.

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index b33de5c..8a44c22 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -466,16 +466,6 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, 
int *processed)
return true;
 }
 
-static void ftgmac100_txdes_reset(const struct ftgmac100 *priv,
- struct ftgmac100_txdes *txdes)
-{
-   /* clear all except end of ring bit */
-   txdes->txdes0 &= cpu_to_le32(priv->txdes0_edotr_mask);
-   txdes->txdes1 = 0;
-   txdes->txdes2 = 0;
-   txdes->txdes3 = 0;
-}
-
 static bool ftgmac100_txdes_owned_by_dma(struct ftgmac100_txdes *txdes)
 {
return txdes->txdes0 & cpu_to_le32(FTGMAC100_TXDES0_TXDMA_OWN);
@@ -575,7 +565,12 @@ static void ftgmac100_free_tx_packet(struct ftgmac100 
*priv,
dev_kfree_skb(skb);
priv->tx_skbs[pointer] = NULL;
 
-   ftgmac100_txdes_reset(priv, txdes);
+   /* Clear txdes0 except end of ring bit, clear txdes1 as we
+* only "OR" into it, leave 2 and 3 alone as 2 is unused
+* and 3 will be overwritten entirely
+*/
+   txdes->txdes0 &= cpu_to_le32(priv->txdes0_edotr_mask);
+   txdes->txdes1 = 0;
 }
 
 static bool ftgmac100_tx_complete_packet(struct ftgmac100 *priv)
-- 
2.9.3

[PATCH v2 03/12] ftgmac100: Merge ftgmac100_xmit() into ftgmac100_hard_start_xmit()

[Benjamin Herrenschmidt]

This will make subsequent rework of the tx path simpler

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 58 ++--
 1 file changed, 25 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 21df322..a5bab0d 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -627,12 +627,33 @@ static void ftgmac100_tx_complete(struct ftgmac100 *priv)
;
 }
 
-static int ftgmac100_xmit(struct ftgmac100 *priv, struct sk_buff *skb,
- dma_addr_t map)
+static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
+struct net_device *netdev)
 {
-   struct net_device *netdev = priv->netdev;
-   struct ftgmac100_txdes *txdes;
unsigned int len = (skb->len < ETH_ZLEN) ? ETH_ZLEN : skb->len;
+   struct ftgmac100 *priv = netdev_priv(netdev);
+   struct ftgmac100_txdes *txdes;
+   dma_addr_t map;
+
+   if (unlikely(skb->len > MAX_PKT_SIZE)) {
+   if (net_ratelimit())
+   netdev_dbg(netdev, "tx packet too big\n");
+
+   netdev->stats.tx_dropped++;
+   kfree_skb(skb);
+   return NETDEV_TX_OK;
+   }
+
+   map = dma_map_single(priv->dev, skb->data, skb_headlen(skb), 
DMA_TO_DEVICE);
+   if (unlikely(dma_mapping_error(priv->dev, map))) {
+   /* drop packet */
+   if (net_ratelimit())
+   netdev_err(netdev, "map socket buffer failed\n");
+
+   netdev->stats.tx_dropped++;
+   kfree_skb(skb);
+   return NETDEV_TX_OK;
+   }
 
txdes = ftgmac100_current_txdes(priv);
ftgmac100_tx_pointer_advance(priv);
@@ -673,35 +694,6 @@ static int ftgmac100_xmit(struct ftgmac100 *priv, struct 
sk_buff *skb,
return NETDEV_TX_OK;
 }
 
-static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
-struct net_device *netdev)
-{
-   struct ftgmac100 *priv = netdev_priv(netdev);
-   dma_addr_t map;
-
-   if (unlikely(skb->len > MAX_PKT_SIZE)) {
-   if (net_ratelimit())
-   netdev_dbg(netdev, "tx packet too big\n");
-
-   netdev->stats.tx_dropped++;
-   kfree_skb(skb);
-   return NETDEV_TX_OK;
-   }
-
-   map = dma_map_single(priv->dev, skb->data, skb_headlen(skb), 
DMA_TO_DEVICE);
-   if (unlikely(dma_mapping_error(priv->dev, map))) {
-   /* drop packet */
-   if (net_ratelimit())
-   netdev_err(netdev, "map socket buffer failed\n");
-
-   netdev->stats.tx_dropped++;
-   kfree_skb(skb);
-   return NETDEV_TX_OK;
-   }
-
-   return ftgmac100_xmit(priv, skb, map);
-}
-
 static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 {
int i;
-- 
2.9.3

[PATCH v2 04/12] ftgmac100: Factor tx packet dropping path

[Benjamin Herrenschmidt]

Use a simple goto to a drop path at the tail of the function,
it will be used in a few more cases soon

Signed-off-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index a5bab0d..84ae800 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -638,10 +638,7 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
if (unlikely(skb->len > MAX_PKT_SIZE)) {
if (net_ratelimit())
netdev_dbg(netdev, "tx packet too big\n");
-
-   netdev->stats.tx_dropped++;
-   kfree_skb(skb);
-   return NETDEV_TX_OK;
+   goto drop;
}
 
map = dma_map_single(priv->dev, skb->data, skb_headlen(skb), 
DMA_TO_DEVICE);
@@ -649,10 +646,7 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
/* drop packet */
if (net_ratelimit())
netdev_err(netdev, "map socket buffer failed\n");
-
-   netdev->stats.tx_dropped++;
-   kfree_skb(skb);
-   return NETDEV_TX_OK;
+   goto drop;
}
 
txdes = ftgmac100_current_txdes(priv);
@@ -692,6 +686,13 @@ static int ftgmac100_hard_start_xmit(struct sk_buff *skb,
ftgmac100_txdma_normal_prio_start_polling(priv);
 
return NETDEV_TX_OK;
+
+ drop:
+   /* Drop the packet */
+   dev_kfree_skb_any(skb);
+   netdev->stats.tx_dropped++;
+
+   return NETDEV_TX_OK;
 }
 
 static void ftgmac100_free_buffers(struct ftgmac100 *priv)
-- 
2.9.3