Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
> > > > Please you should not be using netdev_info(). netdev_dbg() please. > > > > I changed most netif_msg_*()+netdev_*() to netif_*(), including > netif_dbg() in several places. However, after reading other drivers I > decided to leave this at INFO level. I think this is the way to go, > because this is what user asks for and with dynamic debug enabled users > would have to ask for these in two different places. dynamic_debug_exec_queries is now exported, allowing module authors full >control of their debug
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Tue, Sep 08, 2020 at 07:49:20PM +0200, Lukasz Stelmach wrote:
> It was <2020-09-07 pon 20:18>, when Andrew Lunn wrote:
> >> > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
> >> >> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
> >> >
> >> > This is an odd filename. The ioctl code is wrong anyway, but there is
> >> > a lot more than ioctl in here. I suggest you give it a new name.
> >> >
> >>
> >> Sure, any suggestions?
> >
> > Sorry, i have forgotten what is actually contained.
>
> IOCTL handler (.ndo_do_ioctl), ethtool ops, and a bunch of hw control
> functions.
>
> > Does it even need to be a separate file?
>
> It doesn't need, but I think it makes sense to keep ioctl and ethtool
> stuff in a separate file. Some of the hw control function look like they
> might change after using phylib.
_ethtool.c is a common file name.
> Good point. I need to figure out how to do it. Can you point (from the
> top fou your head) a driver which does it for a simmilarly integrated
> device?
Being integrated or not makes no difference. The API usage is the
same. drivers/net/usb/smsc95xx.c has an integrated PHY i think.
> I am not arguing to keep the parameter at any cost, but I would really
> like to know if there is a viable alternative for DT and ACPI. This chip
> is for smaller systems which not necessarily implement advanced
> bootloaders (and DT).
What bootload is being used, if not uboot or bearbox?
> According to module.h
>
> /*
> * Author(s), use "Name " or just "Name", for multiple
> * authors use multiple MODULE_AUTHOR() statements/lines.
> */
Thanks, did not know that.
> >> >> + struct net_device *ndev = ax_local->ndev;
> >> >> + int status;
> >> >> +
> >> >> + do {
> >> >> + if (!(ax_local->checksum & AX_RX_CHECKSUM))
> >> >> + break;
> >> >> +
> >> >> + /* checksum error bit is set */
> >> >> + if ((rxhdr->flags & RX_HDR3_L3_ERR) ||
> >> >> + (rxhdr->flags & RX_HDR3_L4_ERR))
> >> >> + break;
> >> >> +
> >> >> + if ((rxhdr->flags & RX_HDR3_L4_TYPE_TCP) ||
> >> >> + (rxhdr->flags & RX_HDR3_L4_TYPE_UDP)) {
> >> >> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> >> >> + }
> >> >> + } while (0);
> >> >
> >> >
> >> > ??
> >> >
> >>
> >> if() break; Should I use goto?
> >
> > Sorry, i was too ambiguous. Why:
> >
> > do {
> > } while (0);
> >
> > It is an odd construct.
>
> As to "why" — you have correctly spotted, this is a vendor driver I am
> porting. Although it's not like I am trying to avoid any changes, but
> because this driver worked for us on older kernels (v3.10.9) I am trying
> not to touch pieces which IMHO are good enough.
My experience with vendor drivers is you nearly end up rewriting it to
bring it up to mainline standards. I would suggest first setting up
some automated tests, and then make lots of small changes, committed
to git. You can then go backwards and find where regressions have been
introduced and found by the automated tests. And then every so often
squash it all together, ready for submission.
> To avoid using do{}while(0) it requires either goto (instead of breaks),
> nesting those if()s in one another or a humongous single if(). Neither
> looks pretty and the last one is even less readable than
> do()while.
I removed too much context. Does anything useful happen afterwards?
Maybe you can just use return? Or put that code into a helper which
can use return rather than break?
Andrew
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-09-07 pon 20:18>, when Andrew Lunn wrote:
>> > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
>> >> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
>> >
>> > This is an odd filename. The ioctl code is wrong anyway, but there is
>> > a lot more than ioctl in here. I suggest you give it a new name.
>> >
>>
>> Sure, any suggestions?
>
> Sorry, i have forgotten what is actually contained.
IOCTL handler (.ndo_do_ioctl), ethtool ops, and a bunch of hw control
functions.
> Does it even need to be a separate file?
It doesn't need, but I think it makes sense to keep ioctl and ethtool
stuff in a separate file. Some of the hw control function look like they
might change after using phylib.
>> >> +u8 ax88796c_check_power(struct ax88796c_device *ax_local)
>> >
>> > bool ?
>>
>> OK.
>>
>> It appears, however, that 0 means OK and 1 !OK. Do you think changing to
>> TRUE and FALSE (or FALSE and TRUE) is required?
>
> Or change the name, ax88796c_check_power_off()? I don't really care,
> so long as it is logical and not surprising.
>
Good idea, thanks.
>> >> + AX_READ_STATUS(_local->ax_spi, _status);
>> >> + if (!(ax_status.status & AX_STATUS_READY)) {
>> >> +
>> >> + /* AX88796C in power saving mode */
>> >> + AX_WAKEUP(_local->ax_spi);
>> >> +
>> >> + /* Check status */
>> >> + start_time = jiffies;
>> >> + do {
>> >> + if (time_after(jiffies, start_time + HZ/2)) {
>> >> + netdev_err(ax_local->ndev,
>> >> + "timeout waiting for wakeup"
>> >> + " from power saving\n");
>> >> + break;
>> >> + }
>> >> +
>> >> + AX_READ_STATUS(_local->ax_spi, _status);
>> >> +
>> >> + } while (!(ax_status.status & AX_STATUS_READY));
>> >
>> > include/linux/iopoll.h
>> >
>>
>> Done. The result seems only slightly more elegant since the generic
>> read_poll_timeout() needs to be employed.
>
> Often code like this has bugs in it, not correctly handling the
> scheduler sleeping longer than expected. That is why i point people at
> iopoll, no bugs, not elegance.
>
>> The manufacturer says
>>
>> The AX88796C integrates on-chip Fast Ethernet MAC and PHY, […]
>>
>> There is a single integrated PHY in this chip and no possiblity to
>> connect external one. Do you think it makes sense in such case to
>> introduce the additional layer of abstraction?
>
> Yes it does, because it then uses all the standard phylib code to
> drive the PHY which many people understand, is well tested, etc. It
> will make the MAC driver smaller and probably less buggy.
>
Good point. I need to figure out how to do it. Can you point (from the
top fou your head) a driver which does it for a simmilarly integrated
device?
>> >> +static char *macaddr;
>> >> +module_param(macaddr, charp, 0);
>> >> +MODULE_PARM_DESC(macaddr, "MAC address");
>> >
>> > No Module parameters. You can get the MAC address from DT.
>>
>> What about systems without DT? Not every bootloader is sophisicated
>> enough to edit DT before starting kernel. AX88786C is a chip that can be
>> used in a variety of systems and I'd like to avoid too strong
>> assumptions.
>
> There is also a standardised way to read it from ACPI. And you can set
> it using ip link set. DaveM will likely NACK a module parameter.
>
I am not arguing to keep the parameter at any cost, but I would really
like to know if there is a viable alternative for DT and ACPI. This chip
is for smaller systems which not necessarily implement advanced
bootloaders (and DT).
>> >> +MODULE_AUTHOR("ASIX");
>> >
>> > Do you expect ASIX to support this?
>>
>> No.
>>
>> > You probably want to put your name here.
>>
>> I don't want to be considered as the only author and as far as I can
>> tell being mentioned as an author does not imply being a
>> maintainer. Do you think two MODULE_AUTHOR()s be OK?
>
> Can you have two? One with two names listed is O.K.
>
According to module.h
/*
* Author(s), use "Name " or just "Name", for multiple
* authors use multiple MODULE_AUTHOR() statements/lines.
*/
>> >> +
>> >> + phy_status = AX_READ(_local->ax_spi, P0_PSCR);
>> >> + if (phy_status & PSCR_PHYLINK) {
>> >> +
>> >> + ax_local->w_state = ax_nop;
>> >> + time_to_chk = 0;
>> >> +
>> >> + } else if (!(phy_status & PSCR_PHYCOFF)) {
>> >> + /* The ethernet cable has been plugged */
>> >> + if (ax_local->w_state == chk_cable) {
>> >> + if (netif_msg_timer(ax_local))
>> >> + netdev_info(ndev, "Cable connected\n");
>> >> +
>> >> + ax_local->w_state = chk_link;
>> >> + ax_local->w_ticks = 0;
>> >> + } else {
>> >> + if (netif_msg_timer(ax_local))
>> >> + netdev_info(ndev, "Check media status\n");
>> >> +
>> >> + if (++ax_local->w_ticks ==
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
> > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
> >> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
> >
> > This is an odd filename. The ioctl code is wrong anyway, but there is
> > a lot more than ioctl in here. I suggest you give it a new name.
> >
>
> Sure, any suggestions?
Sorry, i have forgotten what is actually contained. Does it even need
to be a separate file?
> >> +u8 ax88796c_check_power(struct ax88796c_device *ax_local)
> >
> > bool ?
>
> OK.
>
> It appears, however, that 0 means OK and 1 !OK. Do you think changing to
> TRUE and FALSE (or FALSE and TRUE) is required?
Or change the name, ax88796c_check_power_off()? I don't really care,
so long as it is logical and not surprising.
> >> + AX_READ_STATUS(_local->ax_spi, _status);
> >> + if (!(ax_status.status & AX_STATUS_READY)) {
> >> +
> >> + /* AX88796C in power saving mode */
> >> + AX_WAKEUP(_local->ax_spi);
> >> +
> >> + /* Check status */
> >> + start_time = jiffies;
> >> + do {
> >> + if (time_after(jiffies, start_time + HZ/2)) {
> >> + netdev_err(ax_local->ndev,
> >> + "timeout waiting for wakeup"
> >> + " from power saving\n");
> >> + break;
> >> + }
> >> +
> >> + AX_READ_STATUS(_local->ax_spi, _status);
> >> +
> >> + } while (!(ax_status.status & AX_STATUS_READY));
> >
> > include/linux/iopoll.h
> >
>
> Done. The result seems only slightly more elegant since the generic
> read_poll_timeout() needs to be employed.
Often code like this has bugs in it, not correctly handling the
scheduler sleeping longer than expected. That is why i point people at
iopoll, no bugs, not elegance.
> The manufacturer says
>
> The AX88796C integrates on-chip Fast Ethernet MAC and PHY, […]
>
> There is a single integrated PHY in this chip and no possiblity to
> connect external one. Do you think it makes sense in such case to
> introduce the additional layer of abstraction?
Yes it does, because it then uses all the standard phylib code to
drive the PHY which many people understand, is well tested, etc. It
will make the MAC driver smaller and probably less buggy.
> >> +static char *macaddr;
> >> +module_param(macaddr, charp, 0);
> >> +MODULE_PARM_DESC(macaddr, "MAC address");
> >
> > No Module parameters. You can get the MAC address from DT.
>
> What about systems without DT? Not every bootloader is sophisicated
> enough to edit DT before starting kernel. AX88786C is a chip that can be
> used in a variety of systems and I'd like to avoid too strong
> assumptions.
There is also a standardised way to read it from ACPI. And you can set
it using ip link set. DaveM will likely NACK a module parameter.
> >> +MODULE_AUTHOR("ASIX");
> >
> > Do you expect ASIX to support this?
>
> No.
>
> > You probably want to put your name here.
>
> I don't want to be considered as the only author and as far as I can
> tell being mentioned as an author does not imply being a
> maintainer. Do you think two MODULE_AUTHOR()s be OK?
Can you have two? One with two names listed is O.K.
> >> +
> >> + phy_status = AX_READ(_local->ax_spi, P0_PSCR);
> >> + if (phy_status & PSCR_PHYLINK) {
> >> +
> >> + ax_local->w_state = ax_nop;
> >> + time_to_chk = 0;
> >> +
> >> + } else if (!(phy_status & PSCR_PHYCOFF)) {
> >> + /* The ethernet cable has been plugged */
> >> + if (ax_local->w_state == chk_cable) {
> >> + if (netif_msg_timer(ax_local))
> >> + netdev_info(ndev, "Cable connected\n");
> >> +
> >> + ax_local->w_state = chk_link;
> >> + ax_local->w_ticks = 0;
> >> + } else {
> >> + if (netif_msg_timer(ax_local))
> >> + netdev_info(ndev, "Check media status\n");
> >> +
> >> + if (++ax_local->w_ticks == AX88796C_WATCHDOG_RESTART) {
> >> + if (netif_msg_timer(ax_local))
> >> + netdev_info(ndev, "Restart autoneg\n");
> >> + ax88796c_mdio_write(ndev,
> >> + ax_local->mii.phy_id, MII_BMCR,
> >> + (BMCR_SPEED100 | BMCR_ANENABLE |
> >> + BMCR_ANRESTART));
> >> +
> >> + if (netif_msg_hw(ax_local))
> >> + ax88796c_dump_phy_regs(ax_local);
> >> + ax_local->w_ticks = 0;
> >> + }
> >> + }
> >> + } else {
> >> + if (netif_msg_timer(ax_local))
> >> + netdev_info(ndev, "Check cable status\n");
> >> +
> >> + ax_local->w_state = chk_cable;
> >> + }
> >> +
> >> + ax88796c_set_power_saving(ax_local, ax_local->ps_level);
> >> +
> >> + if
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-08-26 śro 15:06>, when David Laight wrote: > From: Lukasz Stelmach >> Sent: 26 August 2020 15:59 >> >> It was <2020-08-25 wto 20:44>, when Krzysztof Kozlowski wrote: >> > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote: >> >> ASIX AX88796[1] is a versatile ethernet adapter chip, that can be >> >> connected to a CPU with a 8/16-bit bus or with an SPI. This driver >> >> supports SPI connection. > ... >> >> +++ b/drivers/net/ethernet/asix/Kconfig >> >> @@ -0,0 +1,20 @@ >> >> +# >> >> +# Asix network device configuration >> >> +# >> >> + >> >> +config NET_VENDOR_ASIX >> >> + bool "Asix devices" >> >> + depends on SPI >> >> + help >> >> + If you have a network (Ethernet) interface based on a chip from ASIX, >> >> say Y >> > >> > Looks like too long, did it pass checkpatch? >> >> Yes? Let me try again. Yes, this one passed, but I missed a few other >> problems. Thank you. >> >> >> + >> >> +if NET_VENDOR_ASIX >> >> + >> >> +config SPI_AX88796C >> >> + tristate "Asix AX88796C-SPI support" >> >> + depends on SPI >> >> + depends on GPIOLIB >> >> + help >> >> + Say Y here if you intend to attach a Asix AX88796C as SPI mode >> >> + >> >> +endif # NET_VENDOR_ASIX > > There are plenty of other ethernet devices made by ASIX (eg USB ones) > that have nothing at all to do with this driver. > So those questions are too broad. > > The first one should probable be for ASIX SPI network devices. > On the other hand there is only one ASIX SPI network device and there are four other Non-PCI AX88* chips (and that is all I know about them). > (I can't imagine SPI being fast enough to be useful for ethernet...) Not for a file server, sure. It handles clock up to 40MHz and it's meant for systems that cannot handle more than a few MB/s anyway. -- Łukasz Stelmach Samsung R Institute Poland Samsung Electronics signature.asc Description: PGP signature
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-08-26 śro 09:13>, when Geert Uytterhoeven wrote:
> On Tue, Aug 25, 2020 at 8:02 PM Andrew Lunn wrote:
>> On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
>> > + if (netif_msg_pktdata(ax_local)) {
>> > + int loop;
>> > + netdev_info(ndev, "TX packet len %d, total len %d, seq %d\n",
>> > + pkt_len, tx_skb->len, seq_num);
>> > +
>> > + netdev_info(ndev, " Dump SPI Header:\n");
>> > + for (loop = 0; loop < 4; loop++)
>> > + netdev_info(ndev, "%02x ", *(tx_skb->data + loop));
>> > +
>> > + netdev_info(ndev, "\n");
>>
>> This no longer works as far as i remember. Lines are terminate by
>> default even if they don't have a \n.
>>
>> Please you should not be using netdev_info(). netdev_dbg() please.
>
> We have a nice helper for this: print_hex_dump_debug().
It is good to know.
Actually I think printe_hex_dump(KERN_INFO) is here more
appropriate. With *_debug() functions and dynamic debug enabled users
need to flip two switches to see messages. I think that if msglvl
(pktdata in this case) is not turned on by default and users need to use
ethtool to switch it, they shouldn't be required to fiddle with dynamic
debug too.
--
Łukasz Stelmach
Samsung R Institute Poland
Samsung Electronics
signature.asc
Description: PGP signature
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-08-25 wto 20:01>, when Andrew Lunn wrote:
> Hi Łukasz
>
> It is pretty clear this is a "vendor crap driver".
I can't deny.
> It needs quite a bit more work on it.
I'll be more than happy to take your suggestions.
> On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
>> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
>
> This is an odd filename. The ioctl code is wrong anyway, but there is
> a lot more than ioctl in here. I suggest you give it a new name.
>
Sure, any suggestions?
>> @@ -0,0 +1,293 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2010 ASIX Electronics Corporation
>> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
>> + *
>> + * ASIX AX88796C SPI Fast Ethernet Linux driver
>> + */
>> +
>> +#include "ax88796c_main.h"
>> +#include "ax88796c_spi.h"
>> +#include "ax88796c_ioctl.h"
>> +
>> +u8 ax88796c_check_power(struct ax88796c_device *ax_local)
>
> bool ?
OK.
It appears, however, that 0 means OK and 1 !OK. Do you think changing to
TRUE and FALSE (or FALSE and TRUE) is required?
>> +{
>> +struct spi_status ax_status;
>> +
>> +/* Check media link status first */
>> +if (netif_carrier_ok(ax_local->ndev) ||
>> +(ax_local->ps_level == AX_PS_D0) ||
>> +(ax_local->ps_level == AX_PS_D1)) {
>> +return 0;
>> +}
>> +
>> +AX_READ_STATUS(_local->ax_spi, _status);
>> +if (!(ax_status.status & AX_STATUS_READY))
>> +return 1;
>> +
>> +return 0;
>> +}
>> +
>> +u8 ax88796c_check_power_and_wake(struct ax88796c_device *ax_local)
>> +{
>> +struct spi_status ax_status;
>> +unsigned long start_time;
>> +
>> +/* Check media link status first */
>> +if (netif_carrier_ok(ax_local->ndev) ||
>> +(ax_local->ps_level == AX_PS_D0) ||
>> +(ax_local->ps_level == AX_PS_D1)) {
>> +return 0;
>> +}
>> +
>> +AX_READ_STATUS(_local->ax_spi, _status);
>> +if (!(ax_status.status & AX_STATUS_READY)) {
>> +
>> +/* AX88796C in power saving mode */
>> +AX_WAKEUP(_local->ax_spi);
>> +
>> +/* Check status */
>> +start_time = jiffies;
>> +do {
>> +if (time_after(jiffies, start_time + HZ/2)) {
>> +netdev_err(ax_local->ndev,
>> +"timeout waiting for wakeup"
>> +" from power saving\n");
>> +break;
>> +}
>> +
>> +AX_READ_STATUS(_local->ax_spi, _status);
>> +
>> +} while (!(ax_status.status & AX_STATUS_READY));
>
> include/linux/iopoll.h
>
Done. The result seems only slightly more elegant since the generic
read_poll_timeout() needs to be employed.
>
> Can the device itself put itself to sleep? If not, maybe just track
> the power saving state in struct ax88796c_device?
>
Yes, it can. When the cable is unplugged, parts of of the chip enter
power saving mode and the values in registers change.
>> +int ax88796c_mdio_read(struct net_device *ndev, int phy_id, int loc)
>> +{
>> +struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +unsigned long start_time;
>> +
>> +AX_WRITE(_local->ax_spi, MDIOCR_RADDR(loc)
>> +| MDIOCR_FADDR(phy_id) | MDIOCR_READ, P2_MDIOCR);
>> +
>> +start_time = jiffies;
>> +while ((AX_READ(_local->ax_spi, P2_MDIOCR) & MDIOCR_VALID) == 0) {
>> +if (time_after(jiffies, start_time + HZ/100))
>> +return -EBUSY;
>> +}
>
> Another use case of iopoll.h
>
Done.
>> +return AX_READ(_local->ax_spi, P2_MDIODR);
>> +}
>> +
>> +void
>> +ax88796c_mdio_write(struct net_device *ndev, int phy_id, int loc, int val)
>> +{
>> +struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> +unsigned long start_time;
>> +
>> +AX_WRITE(_local->ax_spi, val, P2_MDIODR);
>> +
>> +AX_WRITE(_local->ax_spi,
>> +MDIOCR_RADDR(loc) | MDIOCR_FADDR(phy_id)
>> +| MDIOCR_WRITE, P2_MDIOCR);
>> +
>> +start_time = jiffies;
>> +while ((AX_READ(_local->ax_spi, P2_MDIOCR) & MDIOCR_VALID) == 0) {
>> +if (time_after(jiffies, start_time + HZ/100))
>> +return;
>> +}
>> +
>> +if (loc == MII_ADVERTISE) {
>> +AX_WRITE(_local->ax_spi, (BMCR_FULLDPLX | BMCR_ANRESTART |
>> + BMCR_ANENABLE | BMCR_SPEED100), P2_MDIODR);
>> +AX_WRITE(_local->ax_spi, (MDIOCR_RADDR(MII_BMCR) |
>> + MDIOCR_FADDR(phy_id) | MDIOCR_WRITE),
>> + P2_MDIOCR);
>
> Odd. An mdio bus driver should not need to do anything like this.
>
> Humm, please make this is a plain MDIO bus driver, using
> mdiobus_register().
>
The manufacturer says
The AX88796C integrates on-chip Fast Ethernet MAC and PHY, […]
There is a single integrated PHY in this chip and no possiblity to
connect external
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Wed, Aug 26, 2020 at 06:45:33PM +0200, Krzysztof Kozlowski wrote: > On Wed, Aug 26, 2020 at 04:59:09PM +0200, Lukasz Stelmach wrote: > >> +#include > > >> +#endif > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > >> +#include > > > > > > All of these should be removed except the headers used directly in this > > > header. > > > > > > > This is "private" header file included in all ax88796c_*.c files and > > these are headers required in them. It seems more conveninet to have > > them all listed in one place. What is the reason to do otherwise? > > Because: > 1. The header is included in other files (more than one) so each other > compilation unit will include all these headers, while not all of them > need. This has a performance penalty during preprocessing. > > 2. You will loose the track which headers are needed, which are not. We > tend to keep it local, which means each compilation unit includes stuff > it needs. This helps removing obsolete includes later. > > 3. Otherwise you could make one header, including all headers of Linux, > and then include this one header in each of C files. One to rule them > all. ... and I got one more: 4. Drivers sometimes get reused, extended or they parts got reused. If a header includes more stuff, it simply will pollute all other units trying to reuse it... making the re-usage difficult. This is less likely reason, I mean, quite imaginary for this particular driver. I don't expect pieces of this driver to be reused... but who knows. Many times in the past in the kernel there was a huge work rewriting headers in many files, because something was including something else and we wanted to decouple these things. Therefore following the pattern - include stuff you explicitly use - helps in every case. Best regards, Krzysztof
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Wed, Aug 26, 2020 at 04:59:09PM +0200, Lukasz Stelmach wrote: > It was <2020-08-25 wto 20:44>, when Krzysztof Kozlowski wrote: > > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote: > >> ASIX AX88796[1] is a versatile ethernet adapter chip, that can be > >> connected to a CPU with a 8/16-bit bus or with an SPI. This driver > >> supports SPI connection. > >> > >> The driver has been ported from the vendor kernel for ARTIK5[2] > >> boards. Several changes were made to adapt it to the current kernel > >> which include: > >> > >> + updated DT configuration, > >> + clock configuration moved to DT, > >> + new timer, ethtool and gpio APIs > >> + dev_* instead of pr_* and custom printk() wrappers. > >> > >> [1] > >> https://protect2.fireeye.com/v1/url?k=074e9e9d-5a9dc212-074f15d2-0cc47a31ce52-0f896a3d08738907=1=bcaebfa2-4f00-46b6-a35d-096f39710f47=https%3A%2F%2Fwww.asix.com.tw%2Fproducts.php%3Fop%3DpItemdetail%26PItemID%3D104%3B65%3B86%26PLine%3D65 > >> [2] > >> https://protect2.fireeye.com/v1/url?k=553869ec-08eb3563-5539e2a3-0cc47a31ce52-fc42424019c6fd8f=1=bcaebfa2-4f00-46b6-a35d-096f39710f47=https%3A%2F%2Fgit.tizen.org%2Fcgit%2Fprofile%2Fcommon%2Fplatform%2Fkernel%2Flinux-3.10-artik%2F > >> > >> The other ax88796 driver is for NE2000 compatible AX88796L chip. These > >> chips are not compatible. Hence, two separate drivers are required. > > > > Hi, > > > > Thanks for the driver, nice work. Few comments below. > > > > Thank you. I fixed most problems and asked some question where I didn't > understand. > > >> > >> Signed-off-by: Łukasz Stelmach > >> --- > >> drivers/net/ethernet/Kconfig |1 + > >> drivers/net/ethernet/Makefile |1 + > >> drivers/net/ethernet/asix/Kconfig | 20 + > >> drivers/net/ethernet/asix/Makefile |6 + > >> drivers/net/ethernet/asix/ax88796c_ioctl.c | 293 + > >> drivers/net/ethernet/asix/ax88796c_ioctl.h | 21 + > >> drivers/net/ethernet/asix/ax88796c_main.c | 1373 > >> drivers/net/ethernet/asix/ax88796c_main.h | 596 + > >> drivers/net/ethernet/asix/ax88796c_spi.c | 103 ++ > >> drivers/net/ethernet/asix/ax88796c_spi.h | 67 + > >> 10 files changed, 2481 insertions(+) > >> create mode 100644 drivers/net/ethernet/asix/Kconfig > >> create mode 100644 drivers/net/ethernet/asix/Makefile > >> create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.c > >> create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.h > >> create mode 100644 drivers/net/ethernet/asix/ax88796c_main.c > >> create mode 100644 drivers/net/ethernet/asix/ax88796c_main.h > >> create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.c > >> create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.h > >> > >> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig > >> index de50e8b9e656..f3b218e45ea5 100644 > >> --- a/drivers/net/ethernet/Kconfig > >> +++ b/drivers/net/ethernet/Kconfig > >> @@ -32,6 +32,7 @@ source "drivers/net/ethernet/apm/Kconfig" > >> source "drivers/net/ethernet/apple/Kconfig" > >> source "drivers/net/ethernet/aquantia/Kconfig" > >> source "drivers/net/ethernet/arc/Kconfig" > >> +source "drivers/net/ethernet/asix/Kconfig" > >> source "drivers/net/ethernet/atheros/Kconfig" > >> source "drivers/net/ethernet/aurora/Kconfig" > >> source "drivers/net/ethernet/broadcom/Kconfig" > >> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile > >> index f8f38dcb5f8a..9eb368d93607 100644 > >> --- a/drivers/net/ethernet/Makefile > >> +++ b/drivers/net/ethernet/Makefile > >> @@ -18,6 +18,7 @@ obj-$(CONFIG_NET_XGENE) += apm/ > >> obj-$(CONFIG_NET_VENDOR_APPLE) += apple/ > >> obj-$(CONFIG_NET_VENDOR_AQUANTIA) += aquantia/ > >> obj-$(CONFIG_NET_VENDOR_ARC) += arc/ > >> +obj-$(CONFIG_NET_VENDOR_ASIX) += asix/ > >> obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/ > >> obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/ > >> obj-$(CONFIG_NET_VENDOR_CADENCE) += cadence/ > >> diff --git a/drivers/net/ethernet/asix/Kconfig > >> b/drivers/net/ethernet/asix/Kconfig > >> new file mode 100644 > >> index ..4b127a4a659a > >> --- /dev/null > >> +++ b/drivers/net/ethernet/asix/Kconfig > >> @@ -0,0 +1,20 @@ > >> +# > >> +# Asix network device configuration > >> +# > >> + > >> +config NET_VENDOR_ASIX > >> + bool "Asix devices" > >> + depends on SPI > >> + help > >> +If you have a network (Ethernet) interface based on a chip from ASIX, > >> say Y > > > > Looks like too long, did it pass checkpatch? > > Yes? Let me try again. Yes, this one passed, but I missed a few other > problems. Thank you. I noticed that now the limit is 100 when improves readability, so this one is good. (...) > >> + > >> +u8 ax88796c_check_power(struct ax88796c_device *ax_local) > > > > Looks here like pointer to const. Unless it is because of > > AX_READ_STATUS() which cannot take const? > > It can. I changed other stuff in ax88796c_spi.[hc] to const too.
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
> (I can't imagine SPI being fast enough to be useful for ethernet...) There are plenty of IoT things which only need a few kbit/s. A VoIP phone can probably get by with 128Kbps, which a 50Mbps SPI bus has no problem to provide, etc. Andrew
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
> >> +module_param(comp, int, 0); > >> +MODULE_PARM_DESC(comp, "0=Non-Compression Mode, 1=Compression Mode"); > >> + > >> +module_param(ps_level, int, 0); > >> +MODULE_PARM_DESC(ps_level, > >> + "Power Saving Level (0:disable 1:level 1 2:level 2)"); > >> + > >> +module_param(msg_enable, int, 0); > >> +MODULE_PARM_DESC(msg_enable, "Message mask (see linux/netdevice.h for > >> bitmap)"); > >> + > >> +static char *macaddr; > >> +module_param(macaddr, charp, 0); > >> +MODULE_PARM_DESC(macaddr, "MAC address"); > > > > I think MAC address as param is not accepted in mainline... > > > > $ git grep MODULE_PARM_DESC\(macaddr -- drivers/net | wc -l > 6 Yes, they exist. But still it is considered back practice, don't do it, use device tree for MAC addresses. Andrew
RE: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
From: Lukasz Stelmach > Sent: 26 August 2020 15:59 > > It was <2020-08-25 wto 20:44>, when Krzysztof Kozlowski wrote: > > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote: > >> ASIX AX88796[1] is a versatile ethernet adapter chip, that can be > >> connected to a CPU with a 8/16-bit bus or with an SPI. This driver > >> supports SPI connection. ... > >> +++ b/drivers/net/ethernet/asix/Kconfig > >> @@ -0,0 +1,20 @@ > >> +# > >> +# Asix network device configuration > >> +# > >> + > >> +config NET_VENDOR_ASIX > >> + bool "Asix devices" > >> + depends on SPI > >> + help > >> +If you have a network (Ethernet) interface based on a chip from ASIX, > >> say Y > > > > Looks like too long, did it pass checkpatch? > > Yes? Let me try again. Yes, this one passed, but I missed a few other > problems. Thank you. > > >> + > >> +if NET_VENDOR_ASIX > >> + > >> +config SPI_AX88796C > >> + tristate "Asix AX88796C-SPI support" > >> + depends on SPI > >> + depends on GPIOLIB > >> + help > >> +Say Y here if you intend to attach a Asix AX88796C as SPI mode > >> + > >> +endif # NET_VENDOR_ASIX There are plenty of other ethernet devices made by ASIX (eg USB ones) that have nothing at all to do with this driver. So those questions are too broad. The first one should probable be for ASIX SPI network devices. (I can't imagine SPI being fast enough to be useful for ethernet...) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-08-25 wto 20:44>, when Krzysztof Kozlowski wrote: > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote: >> ASIX AX88796[1] is a versatile ethernet adapter chip, that can be >> connected to a CPU with a 8/16-bit bus or with an SPI. This driver >> supports SPI connection. >> >> The driver has been ported from the vendor kernel for ARTIK5[2] >> boards. Several changes were made to adapt it to the current kernel >> which include: >> >> + updated DT configuration, >> + clock configuration moved to DT, >> + new timer, ethtool and gpio APIs >> + dev_* instead of pr_* and custom printk() wrappers. >> >> [1] >> https://protect2.fireeye.com/v1/url?k=074e9e9d-5a9dc212-074f15d2-0cc47a31ce52-0f896a3d08738907=1=bcaebfa2-4f00-46b6-a35d-096f39710f47=https%3A%2F%2Fwww.asix.com.tw%2Fproducts.php%3Fop%3DpItemdetail%26PItemID%3D104%3B65%3B86%26PLine%3D65 >> [2] >> https://protect2.fireeye.com/v1/url?k=553869ec-08eb3563-5539e2a3-0cc47a31ce52-fc42424019c6fd8f=1=bcaebfa2-4f00-46b6-a35d-096f39710f47=https%3A%2F%2Fgit.tizen.org%2Fcgit%2Fprofile%2Fcommon%2Fplatform%2Fkernel%2Flinux-3.10-artik%2F >> >> The other ax88796 driver is for NE2000 compatible AX88796L chip. These >> chips are not compatible. Hence, two separate drivers are required. > > Hi, > > Thanks for the driver, nice work. Few comments below. > Thank you. I fixed most problems and asked some question where I didn't understand. >> >> Signed-off-by: Łukasz Stelmach >> --- >> drivers/net/ethernet/Kconfig |1 + >> drivers/net/ethernet/Makefile |1 + >> drivers/net/ethernet/asix/Kconfig | 20 + >> drivers/net/ethernet/asix/Makefile |6 + >> drivers/net/ethernet/asix/ax88796c_ioctl.c | 293 + >> drivers/net/ethernet/asix/ax88796c_ioctl.h | 21 + >> drivers/net/ethernet/asix/ax88796c_main.c | 1373 >> drivers/net/ethernet/asix/ax88796c_main.h | 596 + >> drivers/net/ethernet/asix/ax88796c_spi.c | 103 ++ >> drivers/net/ethernet/asix/ax88796c_spi.h | 67 + >> 10 files changed, 2481 insertions(+) >> create mode 100644 drivers/net/ethernet/asix/Kconfig >> create mode 100644 drivers/net/ethernet/asix/Makefile >> create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.c >> create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.h >> create mode 100644 drivers/net/ethernet/asix/ax88796c_main.c >> create mode 100644 drivers/net/ethernet/asix/ax88796c_main.h >> create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.c >> create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.h >> >> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig >> index de50e8b9e656..f3b218e45ea5 100644 >> --- a/drivers/net/ethernet/Kconfig >> +++ b/drivers/net/ethernet/Kconfig >> @@ -32,6 +32,7 @@ source "drivers/net/ethernet/apm/Kconfig" >> source "drivers/net/ethernet/apple/Kconfig" >> source "drivers/net/ethernet/aquantia/Kconfig" >> source "drivers/net/ethernet/arc/Kconfig" >> +source "drivers/net/ethernet/asix/Kconfig" >> source "drivers/net/ethernet/atheros/Kconfig" >> source "drivers/net/ethernet/aurora/Kconfig" >> source "drivers/net/ethernet/broadcom/Kconfig" >> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile >> index f8f38dcb5f8a..9eb368d93607 100644 >> --- a/drivers/net/ethernet/Makefile >> +++ b/drivers/net/ethernet/Makefile >> @@ -18,6 +18,7 @@ obj-$(CONFIG_NET_XGENE) += apm/ >> obj-$(CONFIG_NET_VENDOR_APPLE) += apple/ >> obj-$(CONFIG_NET_VENDOR_AQUANTIA) += aquantia/ >> obj-$(CONFIG_NET_VENDOR_ARC) += arc/ >> +obj-$(CONFIG_NET_VENDOR_ASIX) += asix/ >> obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/ >> obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/ >> obj-$(CONFIG_NET_VENDOR_CADENCE) += cadence/ >> diff --git a/drivers/net/ethernet/asix/Kconfig >> b/drivers/net/ethernet/asix/Kconfig >> new file mode 100644 >> index ..4b127a4a659a >> --- /dev/null >> +++ b/drivers/net/ethernet/asix/Kconfig >> @@ -0,0 +1,20 @@ >> +# >> +# Asix network device configuration >> +# >> + >> +config NET_VENDOR_ASIX >> +bool "Asix devices" >> +depends on SPI >> +help >> + If you have a network (Ethernet) interface based on a chip from ASIX, >> say Y > > Looks like too long, did it pass checkpatch? Yes? Let me try again. Yes, this one passed, but I missed a few other problems. Thank you. >> + >> +if NET_VENDOR_ASIX >> + >> +config SPI_AX88796C >> +tristate "Asix AX88796C-SPI support" >> +depends on SPI >> +depends on GPIOLIB >> +help >> + Say Y here if you intend to attach a Asix AX88796C as SPI mode >> + >> +endif # NET_VENDOR_ASIX >> diff --git a/drivers/net/ethernet/asix/Makefile >> b/drivers/net/ethernet/asix/Makefile >> new file mode 100644 >> index ..0bfbbb042634 >> --- /dev/null >> +++ b/drivers/net/ethernet/asix/Makefile >> @@ -0,0 +1,6 @@ >> +# >> +# Makefile for the Asix network device drivers. >> +# >> + >> +obj-$(CONFIG_SPI_AX88796C)
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Tue, Aug 25, 2020 at 8:02 PM Andrew Lunn wrote:
> On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
> > + if (netif_msg_pktdata(ax_local)) {
> > + int loop;
> > + netdev_info(ndev, "TX packet len %d, total len %d, seq %d\n",
> > + pkt_len, tx_skb->len, seq_num);
> > +
> > + netdev_info(ndev, " Dump SPI Header:\n");
> > + for (loop = 0; loop < 4; loop++)
> > + netdev_info(ndev, "%02x ", *(tx_skb->data + loop));
> > +
> > + netdev_info(ndev, "\n");
>
> This no longer works as far as i remember. Lines are terminate by
> default even if they don't have a \n.
>
> Please you should not be using netdev_info(). netdev_dbg() please.
We have a nice helper for this: print_hex_dump_debug().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
Hi "Łukasz,
I love your patch! Perhaps something to improve:
[auto build test WARNING on arm/for-next]
[also build test WARNING on net-next/master net/master linus/master
sparc-next/master v5.9-rc2 next-20200825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/ukasz-Stelmach/net-ax88796c-ASIX-AX88796C-SPI-Ethernet-Adapter-Driver/20200826-010937
base: git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
In file included from arch/m68k/include/asm/io_mm.h:25,
from arch/m68k/include/asm/io.h:8,
from include/linux/scatterlist.h:9,
from include/linux/dma-mapping.h:11,
from include/linux/skbuff.h:31,
from include/linux/if_ether.h:19,
from include/linux/etherdevice.h:20,
from drivers/net/ethernet/asix/ax88796c_main.h:19,
from drivers/net/ethernet/asix/ax88796c_main.c:9:
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsb':
arch/m68k/include/asm/raw_io.h:83:7: warning: variable '__w' set but not
used [-Wunused-but-set-variable]
83 | ({u8 __w, __v = (b); u32 _addr = ((u32) (addr)); \
| ^~~
arch/m68k/include/asm/raw_io.h:430:3: note: in expansion of macro 'rom_out_8'
430 | rom_out_8(port, *buf++);
| ^
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw':
arch/m68k/include/asm/raw_io.h:86:8: warning: variable '__w' set but not
used [-Wunused-but-set-variable]
86 | ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
|^~~
arch/m68k/include/asm/raw_io.h:448:3: note: in expansion of macro
'rom_out_be16'
448 | rom_out_be16(port, *buf++);
| ^~~~
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw_swapw':
arch/m68k/include/asm/raw_io.h:90:8: warning: variable '__w' set but not
used [-Wunused-but-set-variable]
90 | ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
|^~~
arch/m68k/include/asm/raw_io.h:466:3: note: in expansion of macro
'rom_out_le16'
466 | rom_out_le16(port, *buf++);
| ^~~~
In file included from include/linux/kernel.h:11,
from include/linux/skbuff.h:13,
from include/linux/if_ether.h:19,
from include/linux/etherdevice.h:20,
from drivers/net/ethernet/asix/ax88796c_main.h:19,
from drivers/net/ethernet/asix/ax88796c_main.c:9:
include/linux/scatterlist.h: In function 'sg_set_buf':
arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of
pointer with null pointer [-Wextra]
169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void
*)PAGE_OFFSET && (void *)(kaddr) < high_memory)
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~
include/linux/scatterlist.h:143:10: note: in expansion of macro
'virt_addr_valid'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~~
In file included from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/skbuff.h:15,
from include/linux/if_ether.h:19,
from include/linux/etherdevice.h:20,
from drivers/net/ethernet/asix/ax88796c_main.h:19,
from drivers/net/ethernet/asix/ax88796c_main.c:9:
include/linux/dma-mapping.h: In function 'dma_map_resource':
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote: > ASIX AX88796[1] is a versatile ethernet adapter chip, that can be > connected to a CPU with a 8/16-bit bus or with an SPI. This driver > supports SPI connection. > > The driver has been ported from the vendor kernel for ARTIK5[2] > boards. Several changes were made to adapt it to the current kernel > which include: > > + updated DT configuration, > + clock configuration moved to DT, > + new timer, ethtool and gpio APIs > + dev_* instead of pr_* and custom printk() wrappers. > > [1] > https://www.asix.com.tw/products.php?op=pItemdetail=104;65;86=65 > [2] > https://git.tizen.org/cgit/profile/common/platform/kernel/linux-3.10-artik/ > > The other ax88796 driver is for NE2000 compatible AX88796L chip. These > chips are not compatible. Hence, two separate drivers are required. Hi, Thanks for the driver, nice work. Few comments below. > > Signed-off-by: Łukasz Stelmach > --- > drivers/net/ethernet/Kconfig |1 + > drivers/net/ethernet/Makefile |1 + > drivers/net/ethernet/asix/Kconfig | 20 + > drivers/net/ethernet/asix/Makefile |6 + > drivers/net/ethernet/asix/ax88796c_ioctl.c | 293 + > drivers/net/ethernet/asix/ax88796c_ioctl.h | 21 + > drivers/net/ethernet/asix/ax88796c_main.c | 1373 > drivers/net/ethernet/asix/ax88796c_main.h | 596 + > drivers/net/ethernet/asix/ax88796c_spi.c | 103 ++ > drivers/net/ethernet/asix/ax88796c_spi.h | 67 + > 10 files changed, 2481 insertions(+) > create mode 100644 drivers/net/ethernet/asix/Kconfig > create mode 100644 drivers/net/ethernet/asix/Makefile > create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.c > create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.h > create mode 100644 drivers/net/ethernet/asix/ax88796c_main.c > create mode 100644 drivers/net/ethernet/asix/ax88796c_main.h > create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.c > create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.h > > diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig > index de50e8b9e656..f3b218e45ea5 100644 > --- a/drivers/net/ethernet/Kconfig > +++ b/drivers/net/ethernet/Kconfig > @@ -32,6 +32,7 @@ source "drivers/net/ethernet/apm/Kconfig" > source "drivers/net/ethernet/apple/Kconfig" > source "drivers/net/ethernet/aquantia/Kconfig" > source "drivers/net/ethernet/arc/Kconfig" > +source "drivers/net/ethernet/asix/Kconfig" > source "drivers/net/ethernet/atheros/Kconfig" > source "drivers/net/ethernet/aurora/Kconfig" > source "drivers/net/ethernet/broadcom/Kconfig" > diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile > index f8f38dcb5f8a..9eb368d93607 100644 > --- a/drivers/net/ethernet/Makefile > +++ b/drivers/net/ethernet/Makefile > @@ -18,6 +18,7 @@ obj-$(CONFIG_NET_XGENE) += apm/ > obj-$(CONFIG_NET_VENDOR_APPLE) += apple/ > obj-$(CONFIG_NET_VENDOR_AQUANTIA) += aquantia/ > obj-$(CONFIG_NET_VENDOR_ARC) += arc/ > +obj-$(CONFIG_NET_VENDOR_ASIX) += asix/ > obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/ > obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/ > obj-$(CONFIG_NET_VENDOR_CADENCE) += cadence/ > diff --git a/drivers/net/ethernet/asix/Kconfig > b/drivers/net/ethernet/asix/Kconfig > new file mode 100644 > index ..4b127a4a659a > --- /dev/null > +++ b/drivers/net/ethernet/asix/Kconfig > @@ -0,0 +1,20 @@ > +# > +# Asix network device configuration > +# > + > +config NET_VENDOR_ASIX > + bool "Asix devices" > + depends on SPI > + help > + If you have a network (Ethernet) interface based on a chip from ASIX, > say Y Looks like too long, did it pass checkpatch? > + > +if NET_VENDOR_ASIX > + > +config SPI_AX88796C > + tristate "Asix AX88796C-SPI support" > + depends on SPI > + depends on GPIOLIB > + help > + Say Y here if you intend to attach a Asix AX88796C as SPI mode > + > +endif # NET_VENDOR_ASIX > diff --git a/drivers/net/ethernet/asix/Makefile > b/drivers/net/ethernet/asix/Makefile > new file mode 100644 > index ..0bfbbb042634 > --- /dev/null > +++ b/drivers/net/ethernet/asix/Makefile > @@ -0,0 +1,6 @@ > +# > +# Makefile for the Asix network device drivers. > +# > + > +obj-$(CONFIG_SPI_AX88796C) += ax88796c.o > +ax88796c-y := ax88796c_main.o ax88796c_ioctl.o ax88796c_spi.o > diff --git a/drivers/net/ethernet/asix/ax88796c_ioctl.c > b/drivers/net/ethernet/asix/ax88796c_ioctl.c > new file mode 100644 > index ..eba361e2a8b7 > --- /dev/null > +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c > @@ -0,0 +1,293 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2010 ASIX Electronics Corporation > + * Copyright (c) 2020 Samsung Electronics Co., Ltd. > + * > + * ASIX AX88796C SPI Fast Ethernet Linux driver > + */ > + > +#include "ax88796c_main.h" > +#include "ax88796c_spi.h" > +#include "ax88796c_ioctl.h" > + > +u8
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
Hi Łukasz
It is pretty clear this is a "vendor crap driver". It needs quite a
bit more work on it.
On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
> +++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
This is an odd filename. The ioctl code is wrong anyway, but there is
a lot more than ioctl in here. I suggest you give it a new name.
> @@ -0,0 +1,293 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2010 ASIX Electronics Corporation
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
> + *
> + * ASIX AX88796C SPI Fast Ethernet Linux driver
> + */
> +
> +#include "ax88796c_main.h"
> +#include "ax88796c_spi.h"
> +#include "ax88796c_ioctl.h"
> +
> +u8 ax88796c_check_power(struct ax88796c_device *ax_local)
bool ?
> +{
> + struct spi_status ax_status;
> +
> + /* Check media link status first */
> + if (netif_carrier_ok(ax_local->ndev) ||
> + (ax_local->ps_level == AX_PS_D0) ||
> + (ax_local->ps_level == AX_PS_D1)) {
> + return 0;
> + }
> +
> + AX_READ_STATUS(_local->ax_spi, _status);
> + if (!(ax_status.status & AX_STATUS_READY))
> + return 1;
> +
> + return 0;
> +}
> +
> +u8 ax88796c_check_power_and_wake(struct ax88796c_device *ax_local)
> +{
> + struct spi_status ax_status;
> + unsigned long start_time;
> +
> + /* Check media link status first */
> + if (netif_carrier_ok(ax_local->ndev) ||
> + (ax_local->ps_level == AX_PS_D0) ||
> + (ax_local->ps_level == AX_PS_D1)) {
> + return 0;
> + }
> +
> + AX_READ_STATUS(_local->ax_spi, _status);
> + if (!(ax_status.status & AX_STATUS_READY)) {
> +
> + /* AX88796C in power saving mode */
> + AX_WAKEUP(_local->ax_spi);
> +
> + /* Check status */
> + start_time = jiffies;
> + do {
> + if (time_after(jiffies, start_time + HZ/2)) {
> + netdev_err(ax_local->ndev,
> + "timeout waiting for wakeup"
> + " from power saving\n");
> + break;
> + }
> +
> + AX_READ_STATUS(_local->ax_spi, _status);
> +
> + } while (!(ax_status.status & AX_STATUS_READY));
include/linux/iopoll.h
Can the device itself put itself to sleep? If not, maybe just track
the power saving state in struct ax88796c_device?
> +int ax88796c_mdio_read(struct net_device *ndev, int phy_id, int loc)
> +{
> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> + unsigned long start_time;
> +
> + AX_WRITE(_local->ax_spi, MDIOCR_RADDR(loc)
> + | MDIOCR_FADDR(phy_id) | MDIOCR_READ, P2_MDIOCR);
> +
> + start_time = jiffies;
> + while ((AX_READ(_local->ax_spi, P2_MDIOCR) & MDIOCR_VALID) == 0) {
> + if (time_after(jiffies, start_time + HZ/100))
> + return -EBUSY;
> + }
Another use case of iopoll.h
> + return AX_READ(_local->ax_spi, P2_MDIODR);
> +}
> +
> +void
> +ax88796c_mdio_write(struct net_device *ndev, int phy_id, int loc, int val)
> +{
> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
> + unsigned long start_time;
> +
> + AX_WRITE(_local->ax_spi, val, P2_MDIODR);
> +
> + AX_WRITE(_local->ax_spi,
> + MDIOCR_RADDR(loc) | MDIOCR_FADDR(phy_id)
> + | MDIOCR_WRITE, P2_MDIOCR);
> +
> + start_time = jiffies;
> + while ((AX_READ(_local->ax_spi, P2_MDIOCR) & MDIOCR_VALID) == 0) {
> + if (time_after(jiffies, start_time + HZ/100))
> + return;
> + }
> +
> + if (loc == MII_ADVERTISE) {
> + AX_WRITE(_local->ax_spi, (BMCR_FULLDPLX | BMCR_ANRESTART |
> + BMCR_ANENABLE | BMCR_SPEED100), P2_MDIODR);
> + AX_WRITE(_local->ax_spi, (MDIOCR_RADDR(MII_BMCR) |
> + MDIOCR_FADDR(phy_id) | MDIOCR_WRITE),
> + P2_MDIOCR);
Odd. An mdio bus driver should not need to do anything like this.
Humm, please make this is a plain MDIO bus driver, using
mdiobus_register().
> +
> + start_time = jiffies;
> + while ((AX_READ(_local->ax_spi, P2_MDIOCR)
> + & MDIOCR_VALID) == 0) {
> + if (time_after(jiffies, start_time + HZ/100))
> + return;
> + }
> + }
> +}
> +
> +static void ax88796c_get_drvinfo(struct net_device *ndev,
> + struct ethtool_drvinfo *info)
> +{
> + /* Inherit standard device info */
> + strncpy(info->driver, DRV_NAME, sizeof(info->driver));
> + strncpy(info->version, DRV_VERSION, sizeof(info->version));
verion is pretty much not wanted any more.
> +static u32 ax88796c_get_link(struct net_device *ndev)
> +{
> + u32 link;
> + struct ax88796c_device
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
>>> +if NET_VENDOR_ASIX >>> + >>> +config SPI_AX88796C >>> + tristate "Asix AX88796C-SPI support" >>> + depends on SPI >> >> That line is redundant (but not harmful). > > Why? Is it because NET_VENDOR_ASIX depends on SPI? Probably it > shouldn't. Thanks for spotting. Yes, that. -- ~Randy
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
It was <2020-08-25 wto 10:19>, when Randy Dunlap wrote: > On 8/25/20 10:03 AM, Łukasz Stelmach wrote: >> diff --git a/drivers/net/ethernet/asix/Kconfig >> b/drivers/net/ethernet/asix/Kconfig >> new file mode 100644 >> index ..4b127a4a659a >> --- /dev/null >> +++ b/drivers/net/ethernet/asix/Kconfig >> @@ -0,0 +1,20 @@ >> +# >> +# Asix network device configuration >> +# >> + >> +config NET_VENDOR_ASIX >> +bool "Asix devices" > > Most vendor entries also have: > default y > so that they will be displayed in the config menu. OK. >> +depends on SPI >> +help >> + If you have a network (Ethernet) interface based on a chip from ASIX, >> say Y >> + >> +if NET_VENDOR_ASIX >> + >> +config SPI_AX88796C >> +tristate "Asix AX88796C-SPI support" >> +depends on SPI > > That line is redundant (but not harmful). Why? Is it because NET_VENDOR_ASIX depends on SPI? Probably it shouldn't. Thanks for spotting. >> +depends on GPIOLIB >> +help >> + Say Y here if you intend to attach a Asix AX88796C as SPI mode >> + >> +endif # NET_VENDOR_ASIX -- Łukasz Stelmach Samsung R Institute Poland Samsung Electronics signature.asc Description: PGP signature
Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
On 8/25/20 10:03 AM, Łukasz Stelmach wrote: > diff --git a/drivers/net/ethernet/asix/Kconfig > b/drivers/net/ethernet/asix/Kconfig > new file mode 100644 > index ..4b127a4a659a > --- /dev/null > +++ b/drivers/net/ethernet/asix/Kconfig > @@ -0,0 +1,20 @@ > +# > +# Asix network device configuration > +# > + > +config NET_VENDOR_ASIX > + bool "Asix devices" Most vendor entries also have: default y so that they will be displayed in the config menu. > + depends on SPI > + help > + If you have a network (Ethernet) interface based on a chip from ASIX, > say Y > + > +if NET_VENDOR_ASIX > + > +config SPI_AX88796C > + tristate "Asix AX88796C-SPI support" > + depends on SPI That line is redundant (but not harmful). > + depends on GPIOLIB > + help > + Say Y here if you intend to attach a Asix AX88796C as SPI mode > + > +endif # NET_VENDOR_ASIX -- ~Randy
[PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
ASIX AX88796[1] is a versatile ethernet adapter chip, that can be
connected to a CPU with a 8/16-bit bus or with an SPI. This driver
supports SPI connection.
The driver has been ported from the vendor kernel for ARTIK5[2]
boards. Several changes were made to adapt it to the current kernel
which include:
+ updated DT configuration,
+ clock configuration moved to DT,
+ new timer, ethtool and gpio APIs
+ dev_* instead of pr_* and custom printk() wrappers.
[1]
https://www.asix.com.tw/products.php?op=pItemdetail=104;65;86=65
[2] https://git.tizen.org/cgit/profile/common/platform/kernel/linux-3.10-artik/
The other ax88796 driver is for NE2000 compatible AX88796L chip. These
chips are not compatible. Hence, two separate drivers are required.
Signed-off-by: Łukasz Stelmach
---
drivers/net/ethernet/Kconfig |1 +
drivers/net/ethernet/Makefile |1 +
drivers/net/ethernet/asix/Kconfig | 20 +
drivers/net/ethernet/asix/Makefile |6 +
drivers/net/ethernet/asix/ax88796c_ioctl.c | 293 +
drivers/net/ethernet/asix/ax88796c_ioctl.h | 21 +
drivers/net/ethernet/asix/ax88796c_main.c | 1373
drivers/net/ethernet/asix/ax88796c_main.h | 596 +
drivers/net/ethernet/asix/ax88796c_spi.c | 103 ++
drivers/net/ethernet/asix/ax88796c_spi.h | 67 +
10 files changed, 2481 insertions(+)
create mode 100644 drivers/net/ethernet/asix/Kconfig
create mode 100644 drivers/net/ethernet/asix/Makefile
create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.c
create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.h
create mode 100644 drivers/net/ethernet/asix/ax88796c_main.c
create mode 100644 drivers/net/ethernet/asix/ax88796c_main.h
create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.c
create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.h
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index de50e8b9e656..f3b218e45ea5 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -32,6 +32,7 @@ source "drivers/net/ethernet/apm/Kconfig"
source "drivers/net/ethernet/apple/Kconfig"
source "drivers/net/ethernet/aquantia/Kconfig"
source "drivers/net/ethernet/arc/Kconfig"
+source "drivers/net/ethernet/asix/Kconfig"
source "drivers/net/ethernet/atheros/Kconfig"
source "drivers/net/ethernet/aurora/Kconfig"
source "drivers/net/ethernet/broadcom/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index f8f38dcb5f8a..9eb368d93607 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_NET_XGENE) += apm/
obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
obj-$(CONFIG_NET_VENDOR_AQUANTIA) += aquantia/
obj-$(CONFIG_NET_VENDOR_ARC) += arc/
+obj-$(CONFIG_NET_VENDOR_ASIX) += asix/
obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/
obj-$(CONFIG_NET_VENDOR_CADENCE) += cadence/
diff --git a/drivers/net/ethernet/asix/Kconfig
b/drivers/net/ethernet/asix/Kconfig
new file mode 100644
index ..4b127a4a659a
--- /dev/null
+++ b/drivers/net/ethernet/asix/Kconfig
@@ -0,0 +1,20 @@
+#
+# Asix network device configuration
+#
+
+config NET_VENDOR_ASIX
+ bool "Asix devices"
+ depends on SPI
+ help
+ If you have a network (Ethernet) interface based on a chip from ASIX,
say Y
+
+if NET_VENDOR_ASIX
+
+config SPI_AX88796C
+ tristate "Asix AX88796C-SPI support"
+ depends on SPI
+ depends on GPIOLIB
+ help
+ Say Y here if you intend to attach a Asix AX88796C as SPI mode
+
+endif # NET_VENDOR_ASIX
diff --git a/drivers/net/ethernet/asix/Makefile
b/drivers/net/ethernet/asix/Makefile
new file mode 100644
index ..0bfbbb042634
--- /dev/null
+++ b/drivers/net/ethernet/asix/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for the Asix network device drivers.
+#
+
+obj-$(CONFIG_SPI_AX88796C) += ax88796c.o
+ax88796c-y := ax88796c_main.o ax88796c_ioctl.o ax88796c_spi.o
diff --git a/drivers/net/ethernet/asix/ax88796c_ioctl.c
b/drivers/net/ethernet/asix/ax88796c_ioctl.c
new file mode 100644
index ..eba361e2a8b7
--- /dev/null
+++ b/drivers/net/ethernet/asix/ax88796c_ioctl.c
@@ -0,0 +1,293 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2010 ASIX Electronics Corporation
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd.
+ *
+ * ASIX AX88796C SPI Fast Ethernet Linux driver
+ */
+
+#include "ax88796c_main.h"
+#include "ax88796c_spi.h"
+#include "ax88796c_ioctl.h"
+
+u8 ax88796c_check_power(struct ax88796c_device *ax_local)
+{
+ struct spi_status ax_status;
+
+ /* Check media link status first */
+ if (netif_carrier_ok(ax_local->ndev) ||
+ (ax_local->ps_level == AX_PS_D0) ||
+ (ax_local->ps_level == AX_PS_D1)) {
+ return 0;
+ }
+
+ AX_READ_STATUS(_local->ax_spi, _status);
+ if (!(ax_status.status & AX_STATUS_READY))
+
