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

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

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

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

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

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

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

> 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

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

> 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

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

> 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

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

> > +#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; > > +

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

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

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

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

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

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

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

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

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

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

> > + 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; > > + > > +

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

> > MAX_EEPROM_SIZE ? > ... snip ... > > Andrew Thanks Andrew, I'll change it.

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

> 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

> 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

> 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

> 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

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

> > > > +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); > > > > + > > > > +

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

> > +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,

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

> 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

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

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

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

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 >

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

> 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

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

> > +static void lan743x_intr_unregister_isr(struct lan743x_adapter *adapter, > > + int vector_index) > > +{ > > + struct lan743x_vector *vector = >intr.vector_list > > + [vector_index]; > > + > > +

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

> 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

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

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

> 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

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

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

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

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

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

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

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 +

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

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

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

Thanks Andrew, Your tips are much appreciated. Bryan

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

> 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

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

> 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

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

> 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

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

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

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

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

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

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

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

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

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

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,

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

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

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

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:

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

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

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

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

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

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

[PATCH net-next] Add LAN9352 Ethernet Driver

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.