RE: [PATCH v1 net] lan743x: fix return value for lan743x_tx_napi_poll

[Bryan.Whitehead]

> I notice 2 problems in the driver:
> 
> 1. netif_napi_add is used instead of netif_tx_napi_add.
> 2. In all other drivers that use netif_tx_napi_add most do not call
> napi_complete_done.
> They all call napi_complete directly and return 0.
> 
> freescale/gianfar.c
> rocker/rocker_main.c
> ti/cpsw.c
> 
> virtio_net.c does use napi_complete_done but it also passes 0 as a
> parameter.
> 
Tristram,

Thanks for the tips. I will make a new version which uses
  netif_tx_napi_add, and napi_complete

Regards,
Bryan

RE: [PATCH v1 net] lan743x: fix return value for lan743x_tx_napi_poll

[Bryan.Whitehead]

> Did you look at the output of "perf top" or something along those lines to
> figure out if your lan743x driver is indeed responsible for that by not being
> scheduler friendly? What is likely happening is that you do not reclaim
> "weight" packets and instead keep looping into NAPI context, which
> prevents the system from making further progress.
> 
I'm having trouble installing "perf" for my kernel. But I have used GPIO's and 
a scope
To make sure my poll routine is called and returns in a timely manner. I've 
never seen
A problem with it "not being scheduler friendly"

> Calling napi_complete_done() for the TX path is not necessary AFAICT, what
> you really want to do is call napi_complete() and make sure you:
> 
> - reclaim/free as many TX buffers as possible, without looking at the NAPI
> weight which becomes irrelevant
> - if you have been able to reclaim enough descriptors, wake-up the transmit
> queue
> 
> So ignoring the NAPI weight like you do is correct, but calling
> napi_complete_done() with a 0 argument does not sound correct to me.
> --
> Florian

I see that napi_complete just maps to napi_complete_done with a 0 argument 
anyway.
So they are currently functionally identical, but I will make a new version 
that uses
napi_complete as you suggested.

But it seems you would definitely agree that the previous version of lan743x 
driver
Which called napi_complete_done with 'weight' as the argument is wrong,
And therefor this fix is correct.

RE: [PATCH v1 net] lan743x: fix return value for lan743x_tx_napi_poll

[Bryan.Whitehead]

> -Original Message-
> From: Andrew Lunn 
> Sent: Tuesday, November 20, 2018 2:31 PM
> To: Bryan Whitehead - C21958 
> Cc: da...@davemloft.net; netdev@vger.kernel.org; UNGLinuxDriver
> 
> Subject: Re: [PATCH v1 net] lan743x: fix return value for
> lan743x_tx_napi_poll
> 
> On Tue, Nov 20, 2018 at 01:26:43PM -0500, Bryan Whitehead wrote:
> > It has been noticed that under stress the lan743x driver will
> > sometimes hang or cause a kernel panic. It has been noticed that
> > returning '0' instead of 'weight' fixes this issue.
> >
> > fixes: rare kernel panic under heavy traffic load.
> > Signed-off-by: Bryan Whitehead 
> 
> Hi Bryan
> 
> This sounds like a band aid over something which is broken, not a real fix.
> 
> Can you show us the stack trace from the panic?
> 
> Andrew

Andrew,

Admittedly, my knowledge of what the kernel is doing behind the scenes is 
limited.

But according to documentation found on 
https://wiki.linuxfoundation.org/networking/napi

It states the following
"The poll() function may also process TX completions, in which case if it 
processes
the entire TX ring then it should count that work as the rest of the budget.
Otherwise, TX completions are not counted."

So based on that, the original driver was returning the full budget. But I was 
having
Issues with it. And the above documentation seems to suggest that I could 
return 0
As in "not counted" from above.

I tried it, and my lock up issues disappeared.

Regarding the kernel panic stack trace. So far its very hard to replicate that 
on the 
latest kernel. I've seen it more frequently when back porting to older kernels 
such
as 4.14, and 4.9. This same fix caused those kernel panics to disappear.
Are you interested in seeing a stack dump from older kernels?

In the latest kernel the issue manifests as a kernel message which states
"[  945.021101] enp48s0: Budget exhausted after napi rescheduled"

I'm not sure what that means. But it does not lock up immediately after seeing 
that
Message. But it usually locks up with in a minute of seeing that message.

And the sometimes I get the following warning
[ 1240.425020] [ cut here ]
[ 1240.426014] NETDEV WATCHDOG: enp0s25 (e1000e): transmit queue 0 timed out
[ 1240.430027] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:461 
dev_watchdog+0x1ef/0x200
[ 1240.430027] Modules linked in: lan743x
[ 1240.430027] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G  I   4.19.2 
#1
[ 1240.430027] Hardware name: Hewlett-Packard HP Compaq dc7900 Convertible 
Minitower/3032h, BIOS 786G1 v01.16 03/05/2009
[ 1240.430027] RIP: 0010:dev_watchdog+0x1ef/0x200
[ 1240.430027] Code: 00 48 63 4d e0 eb 93 4c 89 e7 c6 05 68 30 b3 00 01 e8 25 
3d fd ff 89 d9 48 89 c2 4c 89 e6 48 c7 c7 98 92 48 ab e8 f1 28 87 ff <0f> 0b eb 
c0 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 48 c7 47 08 00
[ 1240.430027] RSP: 0018:98490be03e90 EFLAGS: 00010282
[ 1240.430027] RAX:  RBX:  RCX: 
[ 1240.497168] RDX: 00040400 RSI: 00f6 RDI: 0300
[ 1240.497168] RBP: 984908574440 R08:  R09: 03a4
[ 1240.497168] R10: 0020 R11: abc928ed R12: 984908574000
[ 1240.497168] R13:  R14:  R15: 98490be195b0
[ 1240.497168] FS:  () GS:98490be0() 
knlGS:
[ 1240.497168] CS:  0010 DS:  ES:  CR0: 80050033
[ 1240.497168] CR2: 7f31cd4c CR3: 000109bca000 CR4: 000406f0
[ 1240.497168] Call Trace:
[ 1240.497168]  
[ 1240.497168]  ? qdisc_reset+0xe0/0xe0
[ 1240.497168]  call_timer_fn+0x26/0x130
[ 1240.497168]  run_timer_softirq+0x1cd/0x400
[ 1240.497168]  ? hpet_interrupt_handler+0x10/0x30
[ 1240.497168]  __do_softirq+0xed/0x2aa
[ 1240.497168]  irq_exit+0xb7/0xc0
[ 1240.497168]  do_IRQ+0x45/0xd0
[ 1240.497168]  common_interrupt+0xf/0xf
[ 1240.497168]  
[ 1240.497168] RIP: 0010:cpuidle_enter_state+0xa6/0x330
[ 1240.497168] Code: 65 8b 3d 1d b0 4d 55 e8 58 6a 95 ff 48 89 c3 66 66 66 66 
90 31 ff e8 59 73 95 ff 80 7c 24 0b 00 0f 85 25 02 00 00 fb 4c 29 eb <48> ba cf 
f7 53 e3 a5 9b c4 20 48 89 d8 48 c1 fb 3f 48 f7 ea b8 ff
[ 1240.497168] RSP: 0018:ab603e60 EFLAGS: 0216 ORIG_RAX: 
ffde
[ 1240.497168] RAX: 98490be20a80 RBX: 0081035c RCX: 0120cf178c49
[ 1240.497168] RDX: 0120cf178ca0 RSI: 0120cf178ca0 RDI: 
[ 1240.497168] RBP: 984908fbd000 R08: fffb58ea5f9e R09: 01208e0b48df
[ 1240.497168] R10: 18c4 R11: 2468 R12: 0002
[ 1240.497168] R13: 0120ce968944 R14: ab6a68a0 R15: ab611740
[ 1240.497168]  do_idle+0x1da/0x230
[ 1240.497168]  cpu_startup_entry+0x6a/0x70
[ 1240.497168]  start_kernel+0x4a2/0x4c2
[ 1240.497168]  secondary_startup_64+0xa4/0xb0
[ 1240.497168] ---[ end trace c6f3be34c214db4e ]---

Notice the warning is referring to a different adapter. So I 

RE: [PATCH net-next] lan743x: Make symbol lan743x_pm_ops static

[Bryan.Whitehead]

> Subject: [PATCH net-next] lan743x: Make symbol lan743x_pm_ops static
> 
> Fixes the following sparse warning:
> 
> drivers/net/ethernet/microchip/lan743x_main.c:2944:25: warning:
>  symbol 'lan743x_pm_ops' was not declared. Should it be static?
> 
> Signed-off-by: Wei Yongjun 

Acked-by: Bryan Whitehead 

RE: [PATCH v3 net-next 6/8] lan743x: Add power management support

[Bryan.Whitehead]

> It depends on the hardware designs, but in some designs, the PHY can do
> WOL, without the MAC involved. When a WoL condition happens, it triggers
> an interrupt, or blinks an LED which is also connected to the power supply
> etc. And if the PHY is doing WoL, you can shut down the MAC, save more
> power.
> 
> Look at some of the PHY drivers, at803x, dp83822, dp83tc811, marvell, and
> mscc all support some sort of WoL.
> 
> Ideally, you want to offer the superset of both PHY WoL and MAC WoL.
> 
> Andrew

OK thanks Andrew

RE: [PATCH v3 net-next 6/8] lan743x: Add power management support

[Bryan.Whitehead]

> From: David Miller 
> Date: Thu, 19 Jul 2018 07:15:52 +0900 (KST)
> 
> > Please remove these "#endif FOO, #ifdef FOO" sequences, and instead
> > just have one large continuous "ifdef FOO, endif FOO" section.
> 
> BTW, there were other patches that had this problem too, so please go
> through your entire submission correcting this.
> 
> Thank you.

OK will do.

RE: [PATCH v3 net-next 6/8] lan743x: Add power management support

[Bryan.Whitehead]

> > +#ifdef CONFIG_PM
> > +static void lan743x_ethtool_get_wol(struct net_device *netdev,
> > +   struct ethtool_wolinfo *wol)
> > +{
> > +   struct lan743x_adapter *adapter = netdev_priv(netdev);
> > +
> > +   wol->supported = 0;
> > +   wol->wolopts = 0;
> > +   phy_ethtool_get_wol(netdev->phydev, wol);
> > +
> > +   wol->supported &= WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> > +   WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> 
> Hi Bryan
> 
> Say the PHY set WAKE_MAGICSECURE, because it supports that. This AND
> then wipes it out, making the call to phy_ethtool_get_wol() pointless.
> In fact, should this AND be an OR?

Hi Andrew,
I assumed that "supported" means that it is supported by both the phy and mac 
driver, which is why I used the AND operator.
Am I mistaken? Is WAKE_MAGICSECURE a special case not requiring mac driver 
support?


> 
> > +
> > +#ifdef CONFIG_PM
> > +static int lan743x_ethtool_set_wol(struct net_device *netdev,
> > +  struct ethtool_wolinfo *wol)
> > +{
> > +   struct lan743x_adapter *adapter = netdev_priv(netdev);
> > +
> > +   if (wol->wolopts & WAKE_MAGICSECURE)
> > +   return -EOPNOTSUPP;
> 
> The PHY might support this. Since you call phy_ethtool_set_wol(), you
> should give it the chance.
> 
Again, does WAKE_MAGICSECURE require mac driver support?
If so then it does not matter if the phy driver supports it.
If I misunderstand, can you explain, or direct me to documents.

Thanks,
Bryan

RE: [PATCH v2 net-next 9/9] lan743x: Add PTP support

[Bryan.Whitehead]

> > Can you clarify what is poor and what would be better?
> > For example, should I change "X != 0" to "X ? true : false".
> 
> Look at this:
>   lan743x_ptp_tx_timestamp_skb(tx->adapter,
>buffer_info->skb,
>(buffer_info->flags &
> 
> TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
>!= 0);
> 
> Can't you reduce
> 
>   (buffer_info->flags & TX_BUFFER_INFO_FLAG_IGNORE_SYNC) != 0
> 
> into a local variable:
> 
>   lan743x_ptp_tx_timestamp_skb(tx->adapter, buffer_info-
> >skb, xyz); ?
> 
> > So you mean PPS is not intended to generate a physical signal?
> 
> Yes.
> 
> > It is only intended to call ptp_clock_event?
> 
> Yes.
> 
> > I can configure the hardware to generate an interrupt each second and
> > then call ptp_clock_event. Would that satisfy the pps requirements?
> 
> Yes.
> 
> > Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals?
> 
> Yes.
> 
> Thanks,
> Richard

Richard,
Thank you for your quick answers.
Bryan

RE: [PATCH v2 net-next 6/9] lan743x: Add power management support

[Bryan.Whitehead]

Hi Andrew,

Thanks for your reviews, and corrections.
I will submit a new patch series soon.

Bryan

RE: [PATCH v2 net-next 9/9] lan743x: Add PTP support

[Bryan.Whitehead]

Hi Richard,

Thank you for your detailed feedback. I'm working on it now, but I feel it will 
take a little extra time to complete. Therefor I'm planning to remove PTP 
support from this patch series, and resubmit it in a new patch later.

I also have a few questions below.

> -Original Message-
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Thursday, July 12, 2018 11:32 PM
> To: Bryan Whitehead - C21958 
> Cc: da...@davemloft.net; netdev@vger.kernel.org; UNGLinuxDriver
> 
> Subject: Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support
> 
...
> > +   if (cleanup) {
> > +   lan743x_ptp_unrequest_tx_timestamp(tx->adapter);
> > +   dev_kfree_skb(buffer_info->skb);
> > +   } else {
> > +   lan743x_ptp_tx_timestamp_skb(tx->adapter,
> > +buffer_info->skb,
> > +(buffer_info->flags &
> > +
> TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
> > +!= 0);
> 
> This is poor coding style.  Please find a better way.

Can you clarify what is poor and what would be better?
For example, should I change "X != 0" to "X ? true : false".

> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter) {
> > +   struct lan743x_ptp *ptp = >ptp;
> > +   u32 current_seconds = 0;
> > +   u32 target_seconds = 0;
> > +   u32 general_config = 0;
> > +   int result = -ENODEV;
> > +   int pps_bit = 0;
> 
> So this function is really *not* implementing the PTP_CLK_REQ_PPS feature
> but rather the PTP_CLK_REQ_PEROUT with a period of once per second.
> 
> PTP_CLK_REQ_PPS means placing a PPS event into the kernel's "hardpps"
> subsystem by calling ptp_clock_event().
> 
> I'm sorry this isn't really documented.  I should fix that.
> 
> If you HW can output arbitrary signals, then you should implement
> PTP_CLK_REQ_PEROUT.  In any case, you shouldn't advertise the
> ptp_clock_info.pps capability.

So you mean PPS is not intended to generate a physical signal?
It is only intended to call ptp_clock_event?
I can configure the hardware to generate an interrupt each second and then call 
ptp_clock_event. Would that satisfy the pps requirements?

Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals?

Thanks,
Bryan

RE: [PATCH v2 net-next 7/9] lan743x: Add EEE support

[Bryan.Whitehead]

> > +static int lan743x_ethtool_set_eee(struct net_device *netdev,
> > +  struct ethtool_eee *eee)
> > +{

> 
> Hi Bryan
> 
> You should call phy_ethtool_set_eee() in both cases, so that it gets disabled
> in the PHY as well. It needs to stop advertising it.
> 
>Andrew

OK, thanks Andrew, will do.

RE: [PATCH v1 net-next 9/9] lan743x: Add PTP support

[Bryan.Whitehead]

Thanks Richard,
I'll add it in my next revision.

> -Original Message-
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Friday, July 6, 2018 5:25 PM
> To: Bryan Whitehead - C21958 
> Cc: da...@davemloft.net; netdev@vger.kernel.org; UNGLinuxDriver
> 
> Subject: Re: [PATCH v1 net-next 9/9] lan743x: Add PTP support
> 
> On Thu, Jul 05, 2018 at 12:39:26PM -0400, Bryan Whitehead wrote:
> > Signed-off-by: Bryan Whitehead 
> 
> 1. You forgot the commit message.
> 2. You forgot to add the PTP maintainer on CC.
> 
> Thanks,
> Richard

RE: [PATCH v1 net-next 6/9] lan743x: Add power management support

[Bryan.Whitehead]

> > +   data = lan743x_csr_read(adapter, PMT_CTL);
> 
> Hi Bryan
> 
> Why do you do this read? You do not use the result.
> 
Good catch, I'll remove it.

> > +
> > +   wol->supported = WAKE_BCAST | WAKE_UCAST | WAKE_MCAST |
> > +   WAKE_MAGIC | WAKE_PHY | WAKE_ARP;
> > +
> > +   wol->wolopts = adapter->wolopts;
> > +}
> > +#endif /* CONFIG_PM */
> > +
> > +static int lan743x_pm_wakeframe_crc16(const u8 *buf, int len) {
> > +   const u16 crc16poly = 0x8005;
> > +   u16 bit, crc, msb;
> > +   u8 data;
> > +   int i;
> > +
> > +   crc = 0x;
> > +   for (i = 0; i < len; i++) {
> > +   data = *buf++;
> > +   for (bit = 0; bit < 8; bit++) {
> > +   msb = crc >> 15;
> > +   crc <<= 1;
> > +
> > +   if (msb ^ (u16)(data & 1)) {
> > +   crc ^= crc16poly;
> > +   crc |= (u16)0x0001U;
> > +   }
> > +   data >>= 1;
> > +   }
> > +   }
> > +
> 
> There are a few different crc algorithms in lib. Can you use one of them,
> rather than implementing it yourself?

OK I'll check.

> 
> > +#if CONFIG_PM
> > +static int lan743x_pm_suspend(struct device *dev) {
> > +   struct pci_dev *pdev = to_pci_dev(dev);
> > +   struct net_device *netdev = pci_get_drvdata(pdev);
> > +   struct lan743x_adapter *adapter = netdev_priv(netdev);
> > +   u16 phydata;
> > +   int ret;
> > +
> > +   if (adapter->wolopts & WAKE_PHY) {
> > +   phydata = phy_read(netdev->phydev, 27);
> > +   phydata |= 0x0500;
> > +   phy_write(netdev->phydev, 27, phydata);
> > +   }
> 
> Shouldn't the PHY driver do this?

Perhaps so. I'll check with the PM writer.

Thanks Andrew

RE: [PATCH v1 net-next 5/9] lan743x: Add support for ethtool eeprom access

[Bryan.Whitehead]

> 
> MAX_EEPROM_SIZE ?
> 
... snip ...
> 
>   Andrew

Thanks Andrew, I'll change it.

RE: [PATCH v1 net-next 3/9] lan743x: Add support for ethtool statistics

[Bryan.Whitehead]

> ARRAY_SIZE(lan743x_set0_hw_cnt_addr) ?
> 
...snip...
> 
>   Andrew

Will do, I will resubmit with these changes.

RE: [PATCH v1 net-next 1/9] lan743x: Add support for ethtool get_drvinfo

[Bryan.Whitehead]

> Hi Bryan
> 
> It is normal to put something in the commit message, even if it is the Subject
> line said in a different way.
> 
> Otherwise, this looks O.K.
> 
>   Andrew

OK, thanks Andrew

RE: [PATCH v4 net-next 0/2] lan743x: Add new lan743x driver

[Bryan.Whitehead]

> From: Bryan Whitehead 
> Date: Mon, 5 Mar 2018 14:23:29 -0500
> 
> > Add new lan743x driver.
> >
> > The lan743x from Microchip Technologies Inc, is a PCIe to Gigabit
> > Ethernet Controller.
> 
> Series applied, thank you.

Thanks David

RE: [PATCH v4 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

> On Mon, Mar 05, 2018 at 02:23:30PM -0500, Bryan Whitehead wrote:
> > Add main source files for new lan743x driver
> >
> > Signed-off-by: Bryan Whitehead 
> 
> For MDIO & PHY handling etc
> 
> Reviewed-by: Andrew Lunn 
> 
> But i have not look at any packet handling, DMA, NAPI, etc.
> 
> Andrew

Thanks Andrew

RE: [PATCH v3 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

> > > > +static int lan743x_phy_reset(struct lan743x_adapter *adapter) {
> > > > +   u32 data;
> > > > +
> > > > +   data = lan743x_csr_read(adapter, PMT_CTL);
> > > > +   data |= PMT_CTL_ETH_PHY_RST_;
> > > > +   lan743x_csr_write(adapter, PMT_CTL, data);
> > > > +
> > > > +   return readx_poll_timeout(LAN743X_CSR_READ_OP, PMT_CTL,
> > > data,
> > > > + (!(data & PMT_CTL_ETH_PHY_RST_) &&
> > > > + (data & PMT_CTL_READY_)),
> > > > + 5, 100);
> > > > +}
> > >
> > > Hi Bryan
> > >
> > > Could you explain this a bit more. What exactly is it resetting? Do
> > > we need to tell the phylib that the PHY has been reset and that it needs
> to re-program it?
> > > Or by phy do you mean a SERDES interface?
> >
> > Hi Andrew,
> >
> > This function resets the Ethernet phy. But it is called only in probe
> > and before mdiobus_register. So I don't believe it is necessary to
> > tell phylib.
> 
> Hi Bryan
> 
> So the PHY is built in? It should be safe. Normally we have the PHY driver, or
> the generic layers handle any such reset via a GPIO. But built in is 
> different.

Hi Andrew,

The phy is built in for LAN7430, and external for LAN7431. But the same reset 
should work in both cases because it asserts the normal phy reset line. 

> 
> > [snip]
> > > > +
> > > > +   /* PHY interrupt enabled here */
> > > > +   phy_start(phydev);
> > > > +   phy_start_aneg(phydev);
> > > > +   return 0;
> > >
> > > Are phy interrupts really enabled here? I could of missed it, but i
> > > don't see anywhere PHY interrupts are configured. This is either
> > > done via device tree, you set phydev->irq, or mdiobus->irq[X].
> >
> 
> > Sorry that is an obsolete comment, I will remove it. It is not using
> > phy interrupts. It's using polling.
> 
> Assuming it is built in, does the MAC get the interrupt?
> phy_mac_interrupt() can be called if so.

I believe the MAC does support a phy interrupt. But right now it's
working well using phy polling. Is it ok if I post pone supporting phy
interrupts until a future patch? I am planning a series of patches, and I
can add support for phy interrupts to the list.

Bryan

RE: [PATCH v3 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

> > +static int lan743x_phy_reset(struct lan743x_adapter *adapter) {
> > +   u32 data;
> > +
> > +   data = lan743x_csr_read(adapter, PMT_CTL);
> > +   data |= PMT_CTL_ETH_PHY_RST_;
> > +   lan743x_csr_write(adapter, PMT_CTL, data);
> > +
> > +   return readx_poll_timeout(LAN743X_CSR_READ_OP, PMT_CTL,
> data,
> > + (!(data & PMT_CTL_ETH_PHY_RST_) &&
> > + (data & PMT_CTL_READY_)),
> > + 5, 100);
> > +}
> 
> Hi Bryan
> 
> Could you explain this a bit more. What exactly is it resetting? Do we need to
> tell the phylib that the PHY has been reset and that it needs to re-program 
> it?
> Or by phy do you mean a SERDES interface?

Hi Andrew,

This function resets the Ethernet phy. But it is called only in probe and 
before mdiobus_register. So I don't believe it is necessary to tell phylib.

[snip]
> > +
> > +   /* PHY interrupt enabled here */
> > +   phy_start(phydev);
> > +   phy_start_aneg(phydev);
> > +   return 0;
> 
> Are phy interrupts really enabled here? I could of missed it, but i don't see
> anywhere PHY interrupts are configured. This is either done via device tree,
> you set phydev->irq, or mdiobus->irq[X].

Sorry that is an obsolete comment, I will remove it. It is not using phy 
interrupts. It's using polling.

> 
> > +static int lan743x_tx_open(struct lan743x_tx *tx) {
> > +   struct lan743x_adapter *adapter = NULL;
> > +   u32 data = 0;
> > +   int ret;
> > +
> > +   adapter = tx->adapter;
> > +   ret = lan743x_tx_ring_init(tx);
> > +   if (ret)
> > +   goto return_error;
> 
> You could just return here. You don't do anything useful at
> return_error:

Yes, I'll fix it.

[snip]
 
> This is much nicer without all the flags. Thanks.

No problem, thanks for your patients with me.

> 
> > +static struct pci_driver lan743x_pcidev_driver = {
> > +   .name = DRIVER_NAME,
> > +   .id_table = lan743x_pcidev_tbl,
> > +   .probe= lan743x_pcidev_probe,
> > +   .remove   = lan743x_pcidev_remove,
> > +   .shutdown = lan743x_pcidev_shutdown, };
> > +
> > +static int __init lan743x_module_init(void) {
> > +   int result = -EINVAL;
> > +
> > +   pr_info(DRIVER_DESC "\n");
> > +   result = pci_register_driver(_pcidev_driver);
> > +   if (result)
> > +   pr_warn("pci_register_driver returned error code, %d\n",
> > +   result);
> > +   return result;
> > +}
> > +
> > +module_init(lan743x_module_init);
> > +
> > +static void __exit lan743x_module_exit(void) {
> > +   pci_unregister_driver(_pcidev_driver);
> > +}
> >
> > +module_exit(lan743x_module_exit);
> 
> You can replace this boilerplate code with module_pci_driver().
> You don't do anything special here.

OK

Thanks,
Bryan

RE: [PATCH v2 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

> Hi Bryan
> 
> It is good to look at other drivers and see how they work. Ideally, your 
> driver
> wants to look similar to all other drivers, so making the maintenance easier.
> 
> You will find that most drivers have a set of goto statements for error
> handling, which jump to the end of the function, and do cleanup.
> No flags are maintained.
> 
>   Andrew

Thanks Andrew,

I understand now, I'll work on it.

Bryan

RE: [PATCH v2 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

> > Ok, but it seems to me that what I have is an example of "specific
> > book keeping private information". Can you clarify the style you prefer?
> >
> > In cases of allocation where I can just compare a pointer to null, I
> > can easily remove the flags. But in other cases I need a record of
> > which steps completed in order to clean up properly. In cases where I
> > need some sort of a flag would you prefer I avoid a bit mask, and have a
> standalone variable for each flag?
> 
> Hi Bryan
> 
> Often you know some thing has been done, because if it had not been
> done, you would of bombed out with an error. In the release function you
> can assume everything done in probe has been done, otherwise the probe
> would not be successful. In close, you can assume everything done in open
> was successful, otherwise the open would of failed
> 
> So probe does not need any flags. open does not need any flags.
> 
>Andrew

Hi Andrew,

OK, so there are two cases where clean-up is necessary. One is through call to 
remove or ndo_stop. For these cases I agree with you. Everything must have been 
set up correctly, so everything should be cleaned up, no flags required.

But the other case is when things fail anywhere during probe, or open. In these 
cases things are partially initialized and I assumed I needed to clean up 
whatever was initialized successfully before returning an error. I used flags 
so I could share a common clean up function for all possible error cases as 
well as the fully successful case. Without flags I would need to customize a 
clean-up sequence for each possible error point.

Or are you suggesting that I don't need to worry about clean up if an error 
happens during probe or open?

Bryan

RE: [PATCH v2 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

Hi Florian,
Thanks for your review. I have the following questions/comments.

> On 02/21/2018 11:06 AM, Bryan Whitehead wrote:
> > Add main source files for new lan743x driver.
> >
> > Signed-off-by: Bryan Whitehead 
> > ---
> 
> > +lan743x-objs := lan743x_main.o
> 
> Should we assume that you have additional object files you would like to
> contribute at a later point? If that is the case, this is fine, if this is 
> going to be
> only file of this driver, consider renaming it so you don't even have to have
> this lan743x-objs line at all.

I am planning to add additional source files later. At the very least there will
be a lan743x_ethtool.o, and a lan743x_ptp.o. I didn't want to have to change
the name of the main file later, so I gave it the expected name now.

> > +
> > +static void lan743x_pci_cleanup(struct lan743x_adapter *adapter) {
> > +   struct lan743x_pci *pci = >pci;
> > +
> > +   if (pci->init_flags & INIT_FLAG_PCI_REGIONS_REQUESTED) {
> 
> There is a pattern throughout the driver of maintaining flags to track what
> was initialized and what was not, do you really need that, or can you check
> for specific book keeping private information. Maintaining flags is error 
> prone
> and requires you to keep adding new ones, that does not really scale.

Ok, but it seems to me that what I have is an example of "specific book keeping
private information". Can you clarify the style you prefer?

In cases of allocation where I can just compare a pointer to null, I can easily 
remove
the flags. But in other cases I need a record of which steps completed in order 
to
clean up properly. In cases where I need some sort of a flag would you prefer
I avoid a bit mask, and have a standalone variable for each flag?

[snip]

> > +   dmac_int_en &= ioc_bit;
> > +   dmac_int_sts &= dmac_int_en;
> > +   if (dmac_int_sts & ioc_bit) {
> > +   tasklet_schedule(>tx_isr_bottom_half);
> > +   enable_flag = 0;/* tasklet will re-enable later */
> > +   }
> 
> Consider migrating your TX buffer reclamation to a NAPI context. If you have
> one TX queue and one RX, the same NAPI context can be re-used, if you
> have separate RX/TX queues, you may create a NAPI context per RX/TX pair,
> or you may create separate NAPI contexts per TX queues and RX queues.

This may take some time to refactor, But I will see what I can do.

[snip]


> > +static int lan743x_phy_open(struct lan743x_adapter *adapter) {
> > +   struct lan743x_phy *phy = >phy;
> > +   struct phy_device *phydev;
> > +   struct net_device *netdev;
> > +   int ret = -EIO;
> > +   u32 mii_adv;
> > +
> > +   netdev = adapter->netdev;
> > +   phydev = phy_find_first(adapter->mdiobus);
> > +   if (!phydev) {
> > +   ret = -EIO;
> > +   goto clean_up;
> > +   }
> > +   ret = phy_connect_direct(netdev, phydev,
> > +lan743x_phy_link_status_change,
> > +PHY_INTERFACE_MODE_GMII);
> > +   if (ret)
> > +   goto clean_up;
> > +   phy->flags |= PHY_FLAG_ATTACHED;
> > +
> > +   /* MAC doesn't support 1000T Half */
> > +   phydev->supported &= ~SUPPORTED_1000baseT_Half;
> > +
> > +   /* support both flow controls */
> > +   phy->fc_request_control = (FLOW_CTRL_RX | FLOW_CTRL_TX);
> > +   phydev->advertising &= ~(ADVERTISED_Pause |
> ADVERTISED_Asym_Pause);
> > +   mii_adv = (u32)mii_advertise_flowctrl(phy->fc_request_control);
> > +   phydev->advertising |= mii_adv_to_ethtool_adv_t(mii_adv);
> > +   phy->fc_autoneg = phydev->autoneg;
> 
> No driver does things like that, that appears to be really wrong.
Can you clarify? What is wrong and how should it be?

[snip]

> > +static int lan743x_dmac_init(struct lan743x_adapter *adapter) {
> > +   struct lan743x_dmac *dmac = >dmac;
> > +   u32 data = 0;
> > +
> > +   dmac->flags = 0;
> > +   dmac->descriptor_spacing = DEFAULT_DMA_DESCRIPTOR_SPACING;
> > +   lan743x_csr_write(adapter, DMAC_CMD, DMAC_CMD_SWR_);
> > +   lan743x_csr_wait_for_bit(adapter, DMAC_CMD,
> DMAC_CMD_SWR_,
> > +0, 1000, 2, 100);
> > +   switch (dmac->descriptor_spacing) {
> > +   case DMA_DESCRIPTOR_SPACING_16:
> > +   data = DMAC_CFG_MAX_DSPACE_16_;
> > +   break;
> > +   case DMA_DESCRIPTOR_SPACING_32:
> > +   data = DMAC_CFG_MAX_DSPACE_32_;
> > +   break;
> > +   case DMA_DESCRIPTOR_SPACING_64:
> > +   data = DMAC_CFG_MAX_DSPACE_64_;
> > +   break;
> > +   case DMA_DESCRIPTOR_SPACING_128:
> > +   data = DMAC_CFG_MAX_DSPACE_128_;
> > +   break;
> > +   default:
> > +   return -EPERM;
> 
> -EINVAL maybe?
I think -EPERM is more appropriate because it reflects an unresolvable error. 
In other words the platform is in compatible with the device.
Would you prefer I use a preprocessor error instead, such as

#if invalid_setting
#error incompatible setting
#endif

> 
> [snip]
> 
> > +
> > +done:
> > +   

RE: [PATCH v2 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

> On Thu, Feb 22, 2018 at 10:45:34PM +0100, Andrew Lunn wrote:
> > > Also I'm allocating interrupt resources on interface up, and freeing
> > > resources on interface down. So if there is an up, down, up sequence
> > > then the driver will allocate resources twice. In order for devm to
> > > work properly, should I move all resource allocation into the probe
> function?
> >
> > Hi Bryan
> >
> > It is better to fail early if the resource is not available, so yes, i
> > would register the interrupt handler in probe.
> 
> And we maintainers don't always agree with each other :-)
> 
> Doing irq handling in open/close without devm_ is also O.K.
> 
>Andrew

Thanks Andrew, and Florian,

Moving irq allocation and free, to probe and remove, will require a bit of 
refactoring and possibly introduce new issues. For now I will keep IRQ handling 
in open/close without devm_.

Other resource allocations are already in probe/remove so I will apply your 
suggestions in the next patch revision.

Thanks,
Bryan

RE: [PATCH v2 net-next 1/2] lan743x: Add main source files for new lan743x driver

[Bryan.Whitehead]

> > +static void lan743x_intr_unregister_isr(struct lan743x_adapter *adapter,
> > +   int vector_index)
> > +{
> > +   struct lan743x_vector *vector = >intr.vector_list
> > +   [vector_index];
> > +
> > +   devm_free_irq(>pci.pdev->dev, vector->irq, vector);
> 
> Hu Bryan
> 
> The point of devm_ is that you don't need to free resources you have
> allocated using devm_. The core will release them when the device is
> removed.

Hi Andrew,

When I remove the call devm_free_irq, I get a segmentation fault on close
in pci_disable_msix. Did I do something else wrong?

Also I'm allocating interrupt resources on interface up, and freeing resources
on interface down. So if there is an up, down, up sequence then the driver
will allocate resources twice. In order for devm to work properly, should I
move all resource allocation into the probe function?

Bryan

RE: [PATCH net-next 2/3] Add LAN743X to Kconfig and Makefile (fwd)

[Bryan.Whitehead]

> I don't think netdev_priv can return NULL, so lines 6641 to 6646 could just be
> dropped.
> 
> julia
> 

Julia,

Thanks for the feedback.
I will make changes and submit again later.

Bryan

RE: [PATCH net-next 1/3] Add LAN743X driver

[Bryan.Whitehead]

> 
> The statistics code here is confused.
> You are already counting rx_packets in software in napi_poll Then you get
> values from MAC. One or the other?
> There are two copies of stats, one in netdev and other in your mac structure.
> 
> Also what about byte and error counts?
> 
> If possible implement 64 bit get_stats64 instead.

Stephen,

Thanks for the feedback.

I will work on your suggestions, and submit again later.

Bryan

RE: [PATCH net-next 1/3] Add LAN743X driver

[Bryan.Whitehead]

> Could you split it up a bit. Take the PTP support out for the moment, and
> submit it later once the core driver is accepted. The same for any other
> optional bits.
> 
> Andrew

Andrew, 

thanks for the feedback.
I will work on them and submit again later.

Bryan

RE: [PATCH net-next 1/3] Add LAN743X driver

[Bryan.Whitehead]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Friday, August 11, 2017 6:12 PM
> To: Bryan Whitehead - C21958
> Cc: netdev@vger.kernel.org; UNGLinuxDriver
> Subject: Re: [PATCH net-next 1/3] Add LAN743X driver
> 
> From: 
> Date: Fri, 11 Aug 2017 19:47:57 +
> 
> > +static int lan743x_pci_init(struct lan743x_adapter *adapter,
> > +   struct pci_dev *pdev)
> > +{
> > +   int ret = -ENODEV;
> > +   int bars = 0;
> > +   struct lan743x_pci *pci = >pci;
> 
> Please always order local variable declarations from longest to shortest line
> (reverse christmas tree format).
> 
> > +   pci_set_master(pdev);
> > +
> > +clean_up:
> > +   if (ret) {
> 
> It is more intuitive to structure this like:
> 
>   return 0;
> 
> clean_up:
>  ...
> 
> > +static u8 __iomem *lan743x_pci_get_bar_address(struct lan743x_adapter
> *adapter,
> > +  int bar_index)
> > +{
> > +   u8 __iomem *result = NULL;
> > +   struct lan743x_pci *pci = >pci;
> 
> Reverse christmas tree ordering please.
> 
> > +static int lan743x_csr_light_reset(struct lan743x_adapter *adapter) {
> > +   int result = -EIO;
> > +   u32 data;
> > +   unsigned long timeout;
> 
> Likewise.
> 
> > +static inline void lan743x_csr_write(
> > +   struct lan743x_adapter *adapter, int offset, u32 data)
> 
> Don't do the argument formatting like this please, it looks terrible.
> 
> This:
> 
> static inline void lan743x_csr_write(struct lan743x_adapter *adapter,
>int offset, u32 data)
> 
> works much better.
> 
> > +static void lan743x_intr_union_isr(void *context, u32 int_sts) {
> > +   struct lan743x_adapter *adapter = (struct lan743x_adapter
> *)context;
> 
> Casts from void pointers are never necessary, that's the whole point of void
> pointers.  Please remove this cast.
> 
> > +static irqreturn_t lan743x_vector_isr(int irq, void *ptr) {
> > +   irqreturn_t result = IRQ_NONE;
> > +   struct lan743x_vector *vector = (struct lan743x_vector *)ptr;
> > +   struct lan743x_adapter *adapter = NULL;
> > +   u32 int_sts;
> > +   u32 mask;
> 
> Reverse christmas tree ordering please.
> 
> > +static int lan743x_intr_open(struct lan743x_adapter *adapter) {
> > +   int ret = -ENODEV;
> > +   struct lan743x_intr *intr = >intr;
> > +   int index = 0;
> 
> Likewise.
> 
> > +static int lan743x_dp_wait_till_not_busy(struct lan743x_adapter
> > +*adapter) {
> > +   int i;
> > +   u32 dp_sel = 0;
> 
> Likewise.
> 
> > +static int lan743x_dp_write(struct lan743x_adapter *adapter,
> > +   u32 select, u32 addr, u32 length, u32 *buf) {
> > +   struct lan743x_dp *dp = >dp;
> > +   int ret = -EIO;
> > +   int i;
> > +   u32 dp_sel;
> 
> Likewise.
> 
> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_gpio_reserve_ptp_output(struct lan743x_adapter
> *adapter,
> > +  int bit, int ptp_channel)
> > +{
> > +   struct lan743x_gpio *gpio = >gpio;
> > +   int ret = -EBUSY;
> > +   unsigned long irq_flags = 0;
> > +   int bit_mask = BIT(bit);
> 
> Likewise.
> 
> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptpci_adjfreq(struct ptp_clock_info *ptpci, s32
> > +delta_ppb) {
> > +   u32 u32_delta = 0;
> > +   u64 u64_delta = 0;
> > +   u32 lan743x_rate_adj = 0;
> > +   bool positive = true;
> > +   struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
> > +   struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;
> 
> Likewise.
> 
> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptpci_adjtime(struct ptp_clock_info *ptpci, s64
> > +delta) {
> > +   struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
> > +   struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;
> 
> Likewise.
> 
> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptpci_gettime64(struct ptp_clock_info *ptpci,
> > +  struct timespec64 *ts)
> > +{
> > +   struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
> > +   struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;
> 
> Likewise.
> 
> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptpci_settime64(struct ptp_clock_info *ptpci,
> > +  const struct timespec64 *ts)
> > +{
> > +   struct lan743x_ptp *ptp = LAN743X_PTPCI_TO_PTP;
> > +   struct lan743x_adapter *adapter = LAN743X_PTP_TO_ADAPTER;
> 
> Likewise.
> 
> Also, these X_TO_Y macros are terrible.  If you aren an accessor like that,
> make it a nice inline function declared in a header file with lowercase letter
> which actually does type checking on the pointer variable which is
> dereferenced.
> 
> > +   if (ts) {
> > +   u32 seconds = 0;
> > +   u32 nano_seconds = 0;
> 
> Reverse christmas tree ordering please.
> 
> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter) {
> > +   struct lan743x_ptp *ptp = >ptp;
> > +   int result = -ENODEV;
> > +   u32 

[PATCH net-next 3/3] Add LAN743X to MAINTAINER list

[Bryan.Whitehead]

From: Bryan Whitehead 

Add LAN743X to MAINTAINER list

Signed-off-by: Bryan Whitehead 
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2db0f8c..3216348 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8749,6 +8749,13 @@ F:   drivers/net/dsa/microchip/*
 F: include/linux/platform_data/microchip-ksz.h
 F: Documentation/devicetree/bindings/net/dsa/ksz.txt
 
+MICROCHIP LAN743X ETHERNET DRIVER
+M: Bryan Whitehead 
+M: Microchip Linux Driver Support 
+L: netdev@vger.kernel.org
+S: Maintained
+F: drivers/net/ethernet/microchip/lan743x.*
+
 MICROCHIP USB251XB DRIVER
 M: Richard Leitner 
 L: linux-...@vger.kernel.org
-- 
2.7.4

[PATCH net-next 2/3] Add LAN743X to Kconfig and Makefile

[Bryan.Whitehead]

From: Bryan Whitehead 

Add LAN743X Driver to Kconfig and Makefile

Removed "depends on SPI" from NET_VENDOR_MICROCHIP group
Because all existing drivers already specify SPI dependency, and
New driver does not have SPI dependency.

Signed-off-by: Bryan Whitehead 
---
 drivers/net/ethernet/microchip/Kconfig  | 10 +-
 drivers/net/ethernet/microchip/Makefile |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig 
b/drivers/net/ethernet/microchip/Kconfig
index 36a09d9..5796718 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -5,7 +5,6 @@
 config NET_VENDOR_MICROCHIP
bool "Microchip devices"
default y
-   depends on SPI
---help---
  If you have a network (Ethernet) card belonging to this class, say Y.
 
@@ -42,4 +41,13 @@ config ENCX24J600
   To compile this driver as a module, choose M here. The module will be
   called encx24j600.
 
+config LAN743X
+   tristate "LAN7430 support"
+   depends on PCI
+   ---help---
+ Support for the Microchip LAN7430 PCI Express Gigabit Ethernet chip
+
+ To compile this driver as a module, choose M here. The module will be
+ called lan743x.
+
 endif # NET_VENDOR_MICROCHIP
diff --git a/drivers/net/ethernet/microchip/Makefile 
b/drivers/net/ethernet/microchip/Makefile
index ff78f62..f7215dd 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_ENC28J60) += enc28j60.o
 obj-$(CONFIG_ENCX24J600) += encx24j600.o encx24j600-regmap.o
+obj-$(CONFIG_LAN743X) += lan743x.o
-- 
2.7.4

[PATCH net-next 0/3] Add LAN743X driver

[Bryan.Whitehead]

From: Bryan Whitehead 

Add Microchip LAN743X PCIe 3.1 Gigabit Ethernet Controller Driver

Bryan Whitehead (3):
  Add LAN743X driver
  Add LAN743X to Kconfig and Makefile
  Add LAN743X to MAINTAINER list

 MAINTAINERS  |7 +
 drivers/net/ethernet/microchip/Kconfig   |   10 +-
 drivers/net/ethernet/microchip/Makefile  |1 +
 drivers/net/ethernet/microchip/lan743x.c | 6842 ++
 drivers/net/ethernet/microchip/lan743x.h | 1417 +++
 5 files changed, 8276 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/microchip/lan743x.c
 create mode 100644 drivers/net/ethernet/microchip/lan743x.h

-- 
2.7.4

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Friday, February 19, 2016 3:14 PM 

> > I'm not yet sure how it attaches to the underlying Ethernet driver.
> 
> The DSA core does that for you. If you look at the device tree
> binding:
> 
> dsa@0 {
> compatible = "marvell,dsa";
> #address-cells = <2>;
> #size-cells = <0>;
> 
> interrupts = <10>;
> dsa,ethernet = <>;
> 
> So this says which Ethernet interface to use. net/dsa/dsa.c will create a 
> slave
> interface per external port of your switch. This slave is a netdev. Frames
> transmitted by this slave are fed through the tag code to add additional
> headers/trailers, and then passed to this ethernet device. Frames received
> by the ethernet again through the tag code, stripping off any headers/trails
> and demuxing to the correct slave.

Hi Andrew,

Sorry to bother you with this. But I'm having major difficulty with getting my 
dsa driver entry points called.

Here is what I've done so far.

I copied drivers/net/dsa/mv88x6060.c into drivers/net/dsa/mchp9352_dsa.c
I then modified mchp9352_dsa.c as follows
I emptied out the function bodies, and replaced them with a printk("Not 
Implemented\n");

I did the same thing with net/dsa/tag_trailer.c which was copied into 
net/dsa/tag_mchp9352.c
And function bodies were replaced with printk("Not Implemented\n");

I also modified net/dsa/dsa.c, and net/dsa/slave.c, to include the hooks into 
my new tag files.

My intent was to just see one of my "Not Implemented" functions called, and 
then I would focus on implementing it.

But so far I have not seen any of my "Not Implemented" functions called.

Here is what I know so far.

It appears the dsa.c is not able to attach my underlying net device. And that 
seems to be due to it is unable to find the mdio_bus. 
So I modified my net device driver so that in probe it calls 
of_mdiobus_register instead of mdiobus_register.
And of_mdiobus_register seems to be looking for some kind of phy definitions in 
the device tree, which it does not find. And so it does not appear to register 
the bus in such a way that dsa.c can connect to it.

So the problem appears to be an issue with my device tree, which is partially 
shown below.

 {
status = "okay";
ranges = <0 0 0x1000 0x0800>;   // CS0: 128M 
pinctrl-names = "default";
pinctrl-0 = <_pins>;
lan9352: ethernet@gpmc {
compatible = "microchip,lan9352";
interrupt-parent = <>;
interrupts = <7 8>;//7==GPIO bit 7, 8 = Active low level 
triggered.

bank-width = <2>;

phy-mode = "mii";

reg = <0 0 0x1>;

reg-io-width = <4>;
microchip,save-mac-address;
microchip,irq-push-pull;

};
};

/ {
dsa@0 {
compatible = "microchip,dsa";
#address-cells = <2>;
#size-cells = <0>;
dsa,ethernet = <>;
dsa,mii-bus = <>;
switch@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0 0>;/* MDIO address 0, switch 0 in tree */
port@0 {
reg = <0>;
label = "cpu";
};
port@1 {
reg = <1>;
label = "lan1";
};
port@2 {
reg = <2>;
label = "lan2";
};
};
};
};

I modeled this from what I found in 
arch/arm/boot/dts/kirkwood-mv88f6281gtw-ge.dts 

So my question is. Can you see anything that I'm doing wrong?
Given the issue seems to be about the lan9352 node not having child phy nodes, 
then I wonder if I'm not looking at the best example since my example does not 
use child phy nodes either. If so, can you point me to a better example?

Anyway I'm really hitting a road block here, so any and all guidance is greatly 
appreciated.

Thanks,
Bryan

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Thanks Andrew,

Your tips are much appreciated.

Bryan

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Tuesday, February 16, 2016 5:16 PM
> 
> You are already 1/2 way to a DSA driver, since you have a MAC driver. So i
> agree with David, do it right and add a simple DSA driver.
> 
Andrew,

I've done a little research on DSA.
I've read Documentation/networking/dsa/dsa.txt
And I've looked over some examples in drivers/net/dsa/

It appears there are about 40 functions to implement.
I see examples from only 2 manufacturers, and they average more than 2000 lines 
of code.
I'm not yet sure how it attaches to the underlying Ethernet driver.
And I have no idea how I would test it at the end. 

Given these issues, it will be hard to sell it to my manager.
Since you claim it is simple. Can you explain your reasons?

I've never done a DSA driver before. Given that, can you generally outline the 
steps.
If it can be partly implemented, which parts should be implemented first?

I appreciate any pointers you can provide.

Bryan

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Tuesday, February 16, 2016 3:58 PM 
> > I believe I can make the physical phys accessible through ethertool.
> > Would that be reasonable?
> 
> How, you have no netdev to attach them to?
> 
> There was a nice presentation at netdev last week by Mellanox about their
> switches. I don't know if the video is available yet, but that shows the 
> correct
> way to do this.
> 

You are right. I misunderstood how ethtool works.

Again, I'd like to change our target device to LAN9250, which is just an 
Ethernet controller with no switch. 

Will a driver for that device be more easily accepted?

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Tuesday, February 16, 2016 3:53 PM
> >>
> >> I do not think, with all the infrastructure we have, that we should
> >> accept pure ethernet drivers for such devices any more.
> >>
> >> About year ago I would have responded differently, but these days all
> >> of the necessary support and infrastructure is there, and reasonable
> >> easy for driver authors to use.
> >
> > I believe I can make the physical phys accessible through ethertool. Would
> that be reasonable?
> 
> No, we are asking you to implement DSA or switchdev support.

I just spoke with my manager, and we would like to change the target device 
from LAN9352 to LAN9250. The LAN9250 is the same as the LAN9352 but without the 
switch. It has one mac and one phy. 

Since there is no switch in that product, will that make a pure Ethernet driver 
easier to accept?

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Tuesday, February 16, 2016 3:44 PM
> To: and...@lunn.ch
> Cc: Bryan Whitehead - C21958; f.faine...@gmail.com; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver
> 
> From: Andrew Lunn 
> Date: Tue, 16 Feb 2016 21:36:25 +0100
> 
> > So this is the discussion we need to have.
> >
> > The interface to the outside work is the two switch ports with real
> > PHYs. What you are implementing is an Ethernet driver for an internal
> > port connected to the switch. This port does not go to the outside
> > world. This driver provides no way to control the ports to the outside
> > world and you have no short term plan to actually implement control of
> > the ports connected to the outside world.
> >
> > Should the Linux community accept this driver in this state?
> >
> > I would prefer to see a simple switchdev or DSA driver which exposes
> > the two external ports.
> 
> I do not think, with all the infrastructure we have, that we should accept 
> pure
> ethernet drivers for such devices any more.
> 
> About year ago I would have responded differently, but these days all of the
> necessary support and infrastructure is there, and reasonable easy for driver
> authors to use.

I believe I can make the physical phys accessible through ethertool. Would that 
be reasonable?

Bryan

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Andrew,

At this point, I am not tasked with implementing switch features, which would 
likely take a long time to complete.

If it is too difficult to add switch features later, then they may come as an 
entirely new driver at that time.

So this driver should only be thought of as operating a single port ethernet 
controller.

Bryan

-Original Message-
From: Andrew Lunn [mailto:and...@lunn.ch] 
Sent: Friday, February 12, 2016 12:18 PM
To: Bryan Whitehead - C21958
Cc: f.faine...@gmail.com; da...@davemloft.net; netdev@vger.kernel.org
Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

> At this point my plan is to just get a basic Ethernet controller 
> driver submitted, and later work on switch features.

They are not really separable. 

The linux way of dealing with switches is to model each port as a net device. 
So you should have a lan0 device and a lan1 device for this two port switch. 
You can put IP addresses on the ports, and use them as separate ports. If you 
want to bridge the two ports, you create a linux bridge and add the ports to 
the bridge. Using netdev or DSA, you can then push this configuration down to 
the hardware, so it performs the actual bridging of packets between ports.

Andrew

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Hi Andrew,

You can find my comments below wrapped in  ... 

-Original Message-
From: Andrew Lunn [mailto:and...@lunn.ch] 
Sent: Friday, February 12, 2016 12:11 PM
To: Bryan Whitehead - C21958
Cc: da...@davemloft.net; netdev@vger.kernel.org
Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

On Fri, Feb 12, 2016 at 04:51:49PM +, bryan.whiteh...@microchip.com wrote:
> Hi Andrew,
> 
> Sorry I still did not make this clear. And I'm not sure I understand your 
> question so I'll try to explain again, but please give me feedback if it's 
> still not clear.
> 
> Also you can reference Figure 2-1 for an Internal Block Diagram on 
> page 9 of 
> http://ww1.microchip.com/downloads/en/DeviceDoc/1923A.pdf
> 
> Conceptually I think it's easier to ignore the switch all together, since the 
> driver really doesn't touch it.
> 
> Imagine we have two separate components
>   1) LAN9218 (which is a 10/100 Ethernet Controller)
>   2) An external 3 port switch (which is actually embedded)
> 
> This driver only operates on the Ethernet Controller, whose phy is in reality 
> just a virtual phy.

> That virtual phy is connected directly to the embedded switch fabric, 
> which has the two physical phys that you are asking about. Since this 
> driver only operates on the Ethernet controller and its virtual phy. I 
> makes no sense to talk about phy-modes for the physical phys on the 
> switch.

So the code implements an MDIO bus and registers it with the MDIO framework. 
 Yes 

It then finds the first phy and connects it to the netdev.
 Yes 

When doing this, it passes the phy-mode.
 Yes 

My assumption is, the first PHY on the MDIO bus is the PHY connected to port 0 
of the switch. 
 Yes, that is the correct assumption 

So you are setting the phy-mode of this port.
 Yes 

This is a real phy, not a virtual phy.
 
>From the software point of view it is a real phy, in that there is a real phy 
>register set that can be read and written. But from the hardware point of 
>view, it is virtual because it does not control a physical phy. 

The following is a clip from the spec.
"The Virtual PHY provides a basic MII management interface (MDIO) per EEE 802.3 
(clause 22) so
that a MAC with an unmodified driver can be supported as if it was attached to 
a single port PHY. This
functionality is designed to allow easy and quick integration of the device 
into designs with minimal
driver modifications. The Virtual PHY provides a full bank of registers which 
comply with the IEEE
802.3 specification. This enables the Virtual PHY to provide various status and 
control bits similar to
those provided by a real PHY. These include the output of speed selection, 
duplex, loopback, isolate,
collision test, and Auto-Negotiation status."


 Andrew

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Hi Andrew,

Sorry I still did not make this clear. And I'm not sure I understand your 
question so I'll try to explain again, but please give me feedback if it's 
still not clear.

Also you can reference Figure 2-1 for an Internal Block Diagram on page 9 of
http://ww1.microchip.com/downloads/en/DeviceDoc/1923A.pdf

Conceptually I think it's easier to ignore the switch all together, since the 
driver really doesn't touch it.

Imagine we have two separate components
1) LAN9218 (which is a 10/100 Ethernet Controller)
2) An external 3 port switch (which is actually embedded)

This driver only operates on the Ethernet Controller, whose phy is in reality 
just a virtual phy.
That virtual phy is connected directly to the embedded switch fabric, which has 
the two physical phys that you are asking about. Since this driver only 
operates on the Ethernet controller and its virtual phy. I makes no sense to 
talk about phy-modes for the physical phys on the switch.

If this is still not clear please let me know. If you think you understand but 
my explanation could be better, then please advise me on how best to explain 
this.

Regards,
Bryan


-Original Message-
From: Andrew Lunn [mailto:and...@lunn.ch] 
Sent: Thursday, February 11, 2016 4:55 PM
To: Bryan Whitehead - C21958
Cc: da...@davemloft.net; netdev@vger.kernel.org
Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

On Thu, Feb 11, 2016 at 06:58:52PM +, bryan.whiteh...@microchip.com wrote:
> This is the initial submission of an ethernet driver for the Microchip 
> LAN9352.
> 
> The LAN9352 is a 2-Port 10/100 Managed Ethernet Switch with 16-Bit 
> Non-PCI CPU Interface. The CPU interface includes a basic ethernet 
> controller interface whose virtual phy is connected internally to a 
> 3rd port on the embedded switch.
> 
> This driver only operates as a simple ethernet controller on the CPU 
> interface. Since this interface is connected directly to the embedded 
> switch, the result is that traffic can be sent and received on both 
> physical ports.
> 
> Signed-off-by: Bryan Whitehead 
> ---
>  Documentation/devicetree/bindings/net/mchp9352.txt |   31 +
>  MAINTAINERS|9 +
>  drivers/net/ethernet/microchip/Kconfig |   32 +-
>  drivers/net/ethernet/microchip/Makefile|1 +
>  drivers/net/ethernet/microchip/mchp9352.c  | 2587 
> 
>  drivers/net/ethernet/microchip/mchp9352.h  |  443 
>  6 files changed, 3102 insertions(+), 1 deletion(-)  create mode 
> 100644 Documentation/devicetree/bindings/net/mchp9352.txt
>  create mode 100644 drivers/net/ethernet/microchip/mchp9352.c
>  create mode 100644 drivers/net/ethernet/microchip/mchp9352.h
> 
> diff --git a/Documentation/devicetree/bindings/net/mchp9352.txt 
> b/Documentation/devicetree/bindings/net/mchp9352.txt
> new file mode 100644
> index 000..5b22e73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mchp9352.txt
> @@ -0,0 +1,31 @@
> +* Microchip LAN9352 Controller
> +
> +Required properties:
> +- compatible : Should be "microchip,lan9352"
> +- reg : Address and length of the io space for Microchip LAN
> +- interrupts : Should contain Microchip LAN interrupt line
> +- interrupt-parent : Should be the phandle for the interrupt 
> +controller
> +  that services interrupts for this device
> +- phy-mode : See ethernet.txt file in the same directory

Hi Bryan

You still have not explained which of the two phys this phy-mode applies to? 
What if i want phy #0 to be rgmii and phy #1 to be rgmii-txid?

Andrew

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Florian,
I'm not familiar with the DSA subsystem. Can you refer me to documents about it?

Andrew,
I'm also not familiar with switchdev, or which is better in this case. Can you 
refer me to documents about it?

At this point my plan is to just get a basic Ethernet controller driver 
submitted, and later work on switch features.

Regards,
Bryan


-Original Message-
From: Andrew Lunn [mailto:and...@lunn.ch] 
Sent: Friday, February 12, 2016 2:21 AM
To: Florian Fainelli
Cc: Bryan Whitehead - C21958; da...@davemloft.net; netdev@vger.kernel.org
Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

On Thu, Feb 11, 2016 at 06:18:25PM -0800, Florian Fainelli wrote:
> On 11/02/16 10:58, bryan.whiteh...@microchip.com wrote:
> > This is the initial submission of an ethernet driver for the 
> > Microchip LAN9352.
> > 
> > The LAN9352 is a 2-Port 10/100 Managed Ethernet Switch with 16-Bit 
> > Non-PCI CPU Interface. The CPU interface includes a basic ethernet 
> > controller interface whose virtual phy is connected internally to a 
> > 3rd port on the embedded switch.
> > 
> > This driver only operates as a simple ethernet controller on the CPU 
> > interface. Since this interface is connected directly to the 
> > embedded switch, the result is that traffic can be sent and received 
> > on both physical ports.
> 
> If this is an integrated switch, have you considered using the DSA 
> subsystem and having this driver just be the Ethernet driver that 
> interfaces to your CPU interface, but does not attempt to configure 
> the switch in any way?

I actually think switchdev might be the better model. Hard to say. The 
datasheet suggests it could be modelled as a three port switch, so DSA might be 
appropriate...

  Andrew

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Lino,

Regarding "a matching smp_rmb() in the irq handler"
There is a smp_wmb() in the irq handler, since in both cases we are forcing a 
write operation on software_irq_signal.

I suppose using atomic operations on software_irq_signal would also work, but 
this driver was based on 
drivers/net/ethernet/smsc/smsc911x.c
And if possible I'd prefer to keep logical changes to a minimum.
Plus this is not a "read modify write" scenario so I think the memory barrier 
is sufficient.
Do you agree?

Regarding register_netdev.
I'll move register_netdev till after the mac address is set.

Thanks,
Bryan

-Original Message-
From: Lino Sanfilippo [mailto:linosanfili...@gmx.de] 
Sent: Thursday, February 11, 2016 7:14 PM
To: Bryan Whitehead - C21958; da...@davemloft.net
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

Hi,

> +static int mchp9352_open(struct net_device *dev) {

> +
> + MCHP_TRACE(pdata, ifup, "Testing irq handler using IRQ %d", dev->irq);
> + pdata->software_irq_signal = 0;
> +
> + /* Testing irq handler */
> + smp_wmb();

Should not there at least be a matching smp_rmb() in the irq handler?
Maybe an atomic_t would be a better choice for a flag like software_irq_signal 
here.

> +
> +static int mchp9352_drv_probe(struct platform_device *pdev)

> +
> + netif_carrier_off(dev);
> +
> + retval = register_netdev(dev);
> + if (retval) {
> + MCHP_WARN(pdata, probe, "Error %i registering device", retval);
> + goto out_free_irq;
> + } else {
> + MCHP_TRACE(pdata, probe,
> +"Network interface: \"%s\"", dev->name);
> + }

Note that as soon as the network device is registered "open" may be called so 
everything should be set up already. In this case the mac address 
(dev->dev_addr) is accessed in open but may not yet contain valid data when 
register_netdev is called.

Regards,
Lino

RE: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Thanks Florian,

Lots of good comments here. I'm working on fixes and I'll respond when I get 
through it all.

Bryan

-Original Message-
From: Florian Fainelli [mailto:f.faine...@gmail.com] 
Sent: Thursday, February 11, 2016 9:18 PM
To: Bryan Whitehead - C21958; da...@davemloft.net
Cc: netdev@vger.kernel.org; Andrew Lunn
Subject: Re: [PATCH net-next,V2] Add LAN9352 Ethernet Driver

On 11/02/16 10:58, bryan.whiteh...@microchip.com wrote:
> This is the initial submission of an ethernet driver for the Microchip 
> LAN9352.
> 
> The LAN9352 is a 2-Port 10/100 Managed Ethernet Switch with 16-Bit 
> Non-PCI CPU Interface. The CPU interface includes a basic ethernet 
> controller interface whose virtual phy is connected internally to a 
> 3rd port on the embedded switch.
> 
> This driver only operates as a simple ethernet controller on the CPU 
> interface. Since this interface is connected directly to the embedded 
> switch, the result is that traffic can be sent and received on both 
> physical ports.

If this is an integrated switch, have you considered using the DSA subsystem 
and having this driver just be the Ethernet driver that interfaces to your CPU 
interface, but does not attempt to configure the switch in any way?

It would also help if you clarified the references to the SMSC911x controllers 
and explain whether this was used for the Ethernet MAC, or how the things are 
pieced together, as it seems like there could be different strategies to 
support your hardware here.

> 
> Signed-off-by: Bryan Whitehead 
> ---
>  Documentation/devicetree/bindings/net/mchp9352.txt |   31 +
>  MAINTAINERS|9 +
>  drivers/net/ethernet/microchip/Kconfig |   32 +-
>  drivers/net/ethernet/microchip/Makefile|1 +
>  drivers/net/ethernet/microchip/mchp9352.c  | 2587 
> 
>  drivers/net/ethernet/microchip/mchp9352.h  |  443 
>  6 files changed, 3102 insertions(+), 1 deletion(-)  create mode 
> 100644 Documentation/devicetree/bindings/net/mchp9352.txt
>  create mode 100644 drivers/net/ethernet/microchip/mchp9352.c
>  create mode 100644 drivers/net/ethernet/microchip/mchp9352.h
> 
> diff --git a/Documentation/devicetree/bindings/net/mchp9352.txt 
> b/Documentation/devicetree/bindings/net/mchp9352.txt
> new file mode 100644
> index 000..5b22e73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mchp9352.txt
> @@ -0,0 +1,31 @@
> +* Microchip LAN9352 Controller
> +
> +Required properties:
> +- compatible : Should be "microchip,lan9352"
> +- reg : Address and length of the io space for Microchip LAN
> +- interrupts : Should contain Microchip LAN interrupt line
> +- interrupt-parent : Should be the phandle for the interrupt 
> +controller

This is more like an optional property, in general there is a default parent 
configured in your Device Tree.

> +  that services interrupts for this device
> +- phy-mode : See ethernet.txt file in the same directory

Same question as Andrew, if this is a 2-port switch, should not we have one 
phy-mode per port of the switch?

> +
> +Optional properties:
> +- reg-shift : Specify the quantity to shift the register offsets by
> +- reg-io-width : Specify the size (in bytes) of the IO accesses that
> +  should be performed on the device.  Valid value for Microchip LAN 
> +is
> +  2 or 4.  If it's omitted or invalid, the size would be 2.
> +- microchip,irq-active-high : Indicates the IRQ polarity is 
> +active-high

Should not that be indicated in the interrupts property as a specific cell?

> +- microchip,irq-push-pull : Indicates the IRQ type is push-pull

Same here maybe?

> +- microchip,save-mac-address : Indicates that mac address needs to be 
> +saved
> +  before resetting the controller

That's a software construct here, no?

> +
> +Examples:
> +
> +lan9220@f400 {
> + compatible = "microchip,lan9352";
> + reg = <0xf400 0x200>;
> + phy-mode = "mii";
> + interrupt-parent = <>;
> + interrupts = <31>;
> + reg-io-width = <4>;
> + microchip,irq-push-pull;
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS index f678c37..c39edef 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7136,6 +7136,15 @@ T: git git://git.monstr.eu/linux-2.6-microblaze.git
>  S:   Supported
>  F:   arch/microblaze/
>  
> +MICROCHIP LAN9352 ETHERNET DRIVER
> +M:   Microchip Linux Driver Support 
> +M:   Bryan Whitehead 
> +L:   netdev@vger.kernel.org
> +S:   Maintained
> +F:   Documentation/devicetree/bindings/net/mchp9352.txt
> +F:   drivers/net/ethernet/microchip/mchp9352.h
> +F:   drivers/net/ethernet/microchip/mchp9352.c
> +
>  MICROSOFT SURFACE PRO 3 BUTTON DRIVER
>  M:   Chen Yu 
>  L:   platform-driver-...@vger.kernel.org
> diff --git a/drivers/net/ethernet/microchip/Kconfig 
> b/drivers/net/ethernet/microchip/Kconfig
> index 

[PATCH net-next,V2] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

This is the initial submission of an ethernet driver for
the Microchip LAN9352.

The LAN9352 is a 2-Port 10/100 Managed Ethernet Switch with
16-Bit Non-PCI CPU Interface. The CPU interface includes a basic
ethernet controller interface whose virtual phy is connected
internally to a 3rd port on the embedded switch.

This driver only operates as a simple ethernet controller
on the CPU interface. Since this interface is connected directly
to the embedded switch, the result is that traffic can be sent and
received on both physical ports.

Signed-off-by: Bryan Whitehead 
---
 Documentation/devicetree/bindings/net/mchp9352.txt |   31 +
 MAINTAINERS|9 +
 drivers/net/ethernet/microchip/Kconfig |   32 +-
 drivers/net/ethernet/microchip/Makefile|1 +
 drivers/net/ethernet/microchip/mchp9352.c  | 2587 
 drivers/net/ethernet/microchip/mchp9352.h  |  443 
 6 files changed, 3102 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/mchp9352.txt
 create mode 100644 drivers/net/ethernet/microchip/mchp9352.c
 create mode 100644 drivers/net/ethernet/microchip/mchp9352.h

diff --git a/Documentation/devicetree/bindings/net/mchp9352.txt 
b/Documentation/devicetree/bindings/net/mchp9352.txt
new file mode 100644
index 000..5b22e73
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mchp9352.txt
@@ -0,0 +1,31 @@
+* Microchip LAN9352 Controller
+
+Required properties:
+- compatible : Should be "microchip,lan9352"
+- reg : Address and length of the io space for Microchip LAN
+- interrupts : Should contain Microchip LAN interrupt line
+- interrupt-parent : Should be the phandle for the interrupt controller
+  that services interrupts for this device
+- phy-mode : See ethernet.txt file in the same directory
+
+Optional properties:
+- reg-shift : Specify the quantity to shift the register offsets by
+- reg-io-width : Specify the size (in bytes) of the IO accesses that
+  should be performed on the device.  Valid value for Microchip LAN is
+  2 or 4.  If it's omitted or invalid, the size would be 2.
+- microchip,irq-active-high : Indicates the IRQ polarity is active-high
+- microchip,irq-push-pull : Indicates the IRQ type is push-pull
+- microchip,save-mac-address : Indicates that mac address needs to be saved
+  before resetting the controller
+
+Examples:
+
+lan9220@f400 {
+   compatible = "microchip,lan9352";
+   reg = <0xf400 0x200>;
+   phy-mode = "mii";
+   interrupt-parent = <>;
+   interrupts = <31>;
+   reg-io-width = <4>;
+   microchip,irq-push-pull;
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index f678c37..c39edef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7136,6 +7136,15 @@ T:   git git://git.monstr.eu/linux-2.6-microblaze.git
 S: Supported
 F: arch/microblaze/
 
+MICROCHIP LAN9352 ETHERNET DRIVER
+M: Microchip Linux Driver Support 
+M: Bryan Whitehead 
+L: netdev@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/net/mchp9352.txt
+F: drivers/net/ethernet/microchip/mchp9352.h
+F: drivers/net/ethernet/microchip/mchp9352.c
+
 MICROSOFT SURFACE PRO 3 BUTTON DRIVER
 M: Chen Yu 
 L: platform-driver-...@vger.kernel.org
diff --git a/drivers/net/ethernet/microchip/Kconfig 
b/drivers/net/ethernet/microchip/Kconfig
index 36a09d9..1f346a7 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -5,7 +5,6 @@
 config NET_VENDOR_MICROCHIP
bool "Microchip devices"
default y
-   depends on SPI
---help---
  If you have a network (Ethernet) card belonging to this class, say Y.
 
@@ -28,6 +27,7 @@ config ENC28J60
 
 config ENC28J60_WRITEVERIFY
bool "Enable write verify"
+   depends on SPI
depends on ENC28J60
---help---
  Enable the verify after the buffer write useful for debugging purpose.
@@ -42,4 +42,34 @@ config ENCX24J600
   To compile this driver as a module, choose M here. The module will be
   called encx24j600.
 
+config MCHP9352
+   tristate "LAN9352 embedded ethernet support"
+   depends on HAS_IOMEM
+   select CRC32
+   select MII
+   select PHYLIB
+   ---help---
+ The LAN9352 is a 2-Port 10/100 Managed Ethernet Switch with
+ 16-Bit Non-PCI CPU Interface. The CPU interface includes a basic
+ ethernet controller interface whose virtual phy is connected
+ internally to a 3rd port on the embedded switch.
+
+ This driver only operates as a simple ethernet controller
+ on the CPU interface. Since this interface is connected directly
+ to the embedded switch, the result is that traffic can be sent and
+ received on both physical ports.
+
+ No managed 

RE: [PATCH net-next] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Thanks David,

I'll submit a revised patch soon.

-Original Message-
From: David Miller [mailto:da...@davemloft.net] 
Sent: Wednesday, February 10, 2016 5:34 AM
To: Bryan Whitehead - C21958
Cc: netdev@vger.kernel.org; cor...@lwn.net
Subject: Re: [PATCH net-next] Add LAN9352 Ethernet Driver

From: 
Date: Tue, 9 Feb 2016 20:40:30 +

> +#ifdef USE_PHY_WORK_AROUND
> +#define MIN_PACKET_SIZE (64)
> + char loopback_tx_pkt[MIN_PACKET_SIZE];
> + char loopback_rx_pkt[MIN_PACKET_SIZE];
> + unsigned int resetcount;
> +#endif
 ...
> +#define USE_PHY_WORK_AROUND

If you're going to unconditionally define this, and not provide a Kconfig 
mechanism to change it, just get rid of this and the CPP tests upon it.

Thanks.

RE: [PATCH net-next] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

Hi Andrew,

Thanks for your comment.

The LAN9352 actually has 2 physical ports, and one virtual port which is tied 
internally to the 16-bit Non-PCI CPU Interface. This driver acts as a normal 
Ethernet controller on the virtual port, which itself is an input to the 
embedded switch. The switch directs traffic as a normal switch and requires no 
software support. So the result is that this single ethernet driver can send 
and receive traffic from both physical ports. 

I will submit a revised patch to apply David's suggestion.
And I'll make sure the revised patch has a better description of how this part 
works.

Thanks,
Bryan

-Original Message-
From: Andrew Lunn [mailto:and...@lunn.ch] 
Sent: Wednesday, February 10, 2016 1:14 PM
To: Bryan Whitehead - C21958
Cc: da...@davemloft.net; netdev@vger.kernel.org; cor...@lwn.net
Subject: Re: [PATCH net-next] Add LAN9352 Ethernet Driver

On Tue, Feb 09, 2016 at 08:40:30PM +, bryan.whiteh...@microchip.com wrote:
> This is the initial submission of an ethernet driver for the Microchip 
> LAN9352.
> 
> The LAN9352 is a 2-Port 10/100 Managed Ethernet Switch with 16-Bit 
> Non-PCI CPU Interface.
> 
> While the LAN9352 is a Managed Ethernet Switch, this driver only 
> supports a simple ethernet controller interface.
> 
> Signed-off-by: Bryan Whitehead 
> ---
>  Documentation/devicetree/bindings/net/mchp9352.txt |   31 +
>  MAINTAINERS|9 +
>  drivers/net/ethernet/microchip/Kconfig |   23 +-
>  drivers/net/ethernet/microchip/Makefile|1 +
>  drivers/net/ethernet/microchip/mchp9352.c  | 2593 
> 
>  drivers/net/ethernet/microchip/mchp9352.h  |  448 
>  6 files changed, 3104 insertions(+), 1 deletion(-)  create mode 
> 100644 Documentation/devicetree/bindings/net/mchp9352.txt
>  create mode 100644 drivers/net/ethernet/microchip/mchp9352.c
>  create mode 100644 drivers/net/ethernet/microchip/mchp9352.h
> 
> diff --git a/Documentation/devicetree/bindings/net/mchp9352.txt 
> b/Documentation/devicetree/bindings/net/mchp9352.txt
> new file mode 100644
> index 000..5b22e73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mchp9352.txt
> @@ -0,0 +1,31 @@
> +* Microchip LAN9352 Controller
> +
> +Required properties:
> +- compatible : Should be "microchip,lan9352"
> +- reg : Address and length of the io space for Microchip LAN
> +- interrupts : Should contain Microchip LAN interrupt line
> +- interrupt-parent : Should be the phandle for the interrupt 
> +controller
> +  that services interrupts for this device
> +- phy-mode : See ethernet.txt file in the same directory

Hi Bryan

You say this is a 2 port switch. So which port does this phy-mode apply to? 
Should this be a per port property?

  Andrew

[PATCH net-next] Add LAN9352 Ethernet Driver

[Bryan.Whitehead]

This is the initial submission of an ethernet driver for
the Microchip LAN9352. 

The LAN9352 is a 2-Port 10/100 Managed Ethernet Switch 
with 16-Bit Non-PCI CPU Interface.

While the LAN9352 is a Managed Ethernet Switch, this driver
only supports a simple ethernet controller interface.

Signed-off-by: Bryan Whitehead 
---
 Documentation/devicetree/bindings/net/mchp9352.txt |   31 +
 MAINTAINERS|9 +
 drivers/net/ethernet/microchip/Kconfig |   23 +-
 drivers/net/ethernet/microchip/Makefile|1 +
 drivers/net/ethernet/microchip/mchp9352.c  | 2593 
 drivers/net/ethernet/microchip/mchp9352.h  |  448 
 6 files changed, 3104 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/mchp9352.txt
 create mode 100644 drivers/net/ethernet/microchip/mchp9352.c
 create mode 100644 drivers/net/ethernet/microchip/mchp9352.h

diff --git a/Documentation/devicetree/bindings/net/mchp9352.txt 
b/Documentation/devicetree/bindings/net/mchp9352.txt
new file mode 100644
index 000..5b22e73
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mchp9352.txt
@@ -0,0 +1,31 @@
+* Microchip LAN9352 Controller
+
+Required properties:
+- compatible : Should be "microchip,lan9352"
+- reg : Address and length of the io space for Microchip LAN
+- interrupts : Should contain Microchip LAN interrupt line
+- interrupt-parent : Should be the phandle for the interrupt controller
+  that services interrupts for this device
+- phy-mode : See ethernet.txt file in the same directory
+
+Optional properties:
+- reg-shift : Specify the quantity to shift the register offsets by
+- reg-io-width : Specify the size (in bytes) of the IO accesses that
+  should be performed on the device.  Valid value for Microchip LAN is
+  2 or 4.  If it's omitted or invalid, the size would be 2.
+- microchip,irq-active-high : Indicates the IRQ polarity is active-high
+- microchip,irq-push-pull : Indicates the IRQ type is push-pull
+- microchip,save-mac-address : Indicates that mac address needs to be saved
+  before resetting the controller
+
+Examples:
+
+lan9220@f400 {
+   compatible = "microchip,lan9352";
+   reg = <0xf400 0x200>;
+   phy-mode = "mii";
+   interrupt-parent = <>;
+   interrupts = <31>;
+   reg-io-width = <4>;
+   microchip,irq-push-pull;
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index f678c37..c39edef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7136,6 +7136,15 @@ T:   git git://git.monstr.eu/linux-2.6-microblaze.git
 S: Supported
 F: arch/microblaze/
 
+MICROCHIP LAN9352 ETHERNET DRIVER
+M: Microchip Linux Driver Support 
+M: Bryan Whitehead 
+L: netdev@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/net/mchp9352.txt
+F: drivers/net/ethernet/microchip/mchp9352.h
+F: drivers/net/ethernet/microchip/mchp9352.c
+
 MICROSOFT SURFACE PRO 3 BUTTON DRIVER
 M: Chen Yu 
 L: platform-driver-...@vger.kernel.org
diff --git a/drivers/net/ethernet/microchip/Kconfig 
b/drivers/net/ethernet/microchip/Kconfig
index 36a09d9..caf4011 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -5,7 +5,6 @@
 config NET_VENDOR_MICROCHIP
bool "Microchip devices"
default y
-   depends on SPI
---help---
  If you have a network (Ethernet) card belonging to this class, say Y.
 
@@ -28,6 +27,7 @@ config ENC28J60
 
 config ENC28J60_WRITEVERIFY
bool "Enable write verify"
+   depends on SPI
depends on ENC28J60
---help---
  Enable the verify after the buffer write useful for debugging purpose.
@@ -42,4 +42,25 @@ config ENCX24J600
   To compile this driver as a module, choose M here. The module will be
   called encx24j600.
 
+config MCHP9352
+   tristate "LAN9352 embedded ethernet support"
+   depends on HAS_IOMEM
+   select CRC32
+   select MII
+   select PHYLIB
+   ---help---
+ LAN9352 is a switch product but this driver only support
+ it as a simple ethernet controller. i.e. No switch features
+ are supported yet.
+
+ May support LAN9250, LAN9311, and LAN9312, but these products
+ were not tested.
+
+ Say Y here if you want support for MICROCHIP LAN9352 as a simple
+ ethernet controller.
+
+ To compile this driver as a module, choose M here. The module
+ will be called mchp9352.
+
+
 endif # NET_VENDOR_MICROCHIP
diff --git a/drivers/net/ethernet/microchip/Makefile 
b/drivers/net/ethernet/microchip/Makefile
index ff78f62..6ce5296 100644
--- a/drivers/net/ethernet/microchip/Makefile
+++ b/drivers/net/ethernet/microchip/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_ENC28J60) += enc28j60.o