LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Francois Romieu <romieu@fr.zoreil.com>
To: =?unknown-8bit?B?6Kix5oGG5ZiJ?= <edward_hsu@realtek.com.tw>
Cc: jeff@garzik.org, linux-kernel@vger.kernel.org, hiwu@realtek.com.tw
Subject: Re: [PATCH 2.6.19.2] r8169: support RTL8169SC/8110SC
Date: Tue, 6 Feb 2007 01:31:30 +0100	[thread overview]
Message-ID: <20070206003130.GA18236@electric-eye.fr.zoreil.com> (raw)
In-Reply-To: <45C6A8EC.5090201@realtek.com.tw>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 3904 bytes --]

許恆嘉 <edward_hsu@realtek.com.tw> :
[...]
> >>static struct pci_device_id rtl8169_pci_tbl[] = {
> >>- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8129), 0, 0, RTL_CFG_0 },
> >>- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8136), 0, 0, RTL_CFG_2 },
> >>- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8167), 0, 0, RTL_CFG_0 },
> >>- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8168), 0, 0, RTL_CFG_2 },
> >>- { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8169), 0, 0, RTL_CFG_0 },
> >>- { PCI_DEVICE(PCI_VENDOR_ID_DLINK, 0x4300), 0, 0, RTL_CFG_0 },
> >>- { PCI_DEVICE(0x1259, 0xc107), 0, 0, RTL_CFG_0 },
> >>- { PCI_DEVICE(0x16ec, 0x0116), 0, 0, RTL_CFG_0 },
> >>- { PCI_VENDOR_ID_LINKSYS, 0x1032,
> >>- PCI_ANY_ID, 0x0024, 0, 0, RTL_CFG_0 },
> >>+ { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8169), },
> >>+ { PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8167), },
> >>   
> >>
> >
> >The current driver is intended to handle the whole set of PCI IDs
> >which would be removed by the patch. Thoug some combination of
> >chipset and motherboard do not work as expected, the gigabit
> >chipsets have been reported to work.
> >
> >Please elaborate if there is a good reason to remove any ID.
> > 
> >
> ANS:
> I have explained my point about this in last question. This driver is 
> developed for Realtek PCI gigabit ethernet controllers. Although some 
> vendors may use Realtek solutions with their own PCI DIDs and VIDs, they 
> should customize this driver and maintained the customized driver on 
> their own.

Vendors will change the PCI ID because they like to see their name in
the nifty hardware GUI under Windows. The brave ones will mess with the
VPD (before or after they vandalize the ACPI tables, at their option).
Eventually they will copy an outdated version of your driver on their site
with the updated ID. Given the tight margin on these kind of mass-volume
product, I would not expect vendors to do more for their customers who
use Linux.

If your hardware changes significantly, it may make sense to use a new,
different driver. Otherwise, as long as the changes are minor, a single
in-kernel driver which works out of the box for most users/vendors offers
the best coverage. It should not be neglected.

So far, I have only seen minor differences between r8101-1.001.00,
r8168-8.001.00 and r8169-6.001.00. The mac init sequence is not pretty
to unify but the drivers stay mostly the same. There even is a floating
patch for MSI support which seems manageable within a single driver.

Of course it depends a lot on the kind of changes that you envision for
the driver(s).

[...]
> >>RxMaxSize = 0xDA,
> >>CPlusCmd = 0xE0,
> >>IntrMitigate = 0xE2,
> >>@@ -287,11 +291,10 @@ enum RTL8169_register_content {
> >>RxOK = 0x01,
> >>
> >>/* RxStatusDesc */
> >>- RxFOVF = (1 << 23),
> >>- RxRWT = (1 << 22),
> >>- RxRES = (1 << 21),
> >>- RxRUNT = (1 << 20),
> >>- RxCRC = (1 << 19),
> >>+ RxRES = 0x00200000,
> >>+ RxCRC = 0x00080000,
> >>+ RxRUNT = 0x00100000,
> >>+ RxRWT = 0x00400000,
> >>   
> >>
> >
> >This part removes RxFOVF. Please elaborate if there is a reason
> >to do so.
> > 
> >
> 
> ANS:
> According to the spec of RTL8110SC, bit 23 and bit 24 are reserved in 
> the 1st double word of rx status descriptor. Therefore, I delete it.

The bits are fine for the old Gigabit 8169 though, aren't they ?

[...]
> >Two points here:
> >1. the driver uniformly uses u{8/16/32} types. Please follow the current
> >  style and avoid to add uint{8/16/32}_t things.
> >2. does this new field bring something that struct net_device.dev_addr
> >  does not ?
> >
> > 
> >
> ANS:
> I reference the source code of e1000.c, which is currently existing in 
> Linux kernel. I think it uses u{8/16/32} and the new field.

The e1000 driver has an interesting history. It is strongly suggested
to ponder several drivers to figure some good practices. Please see for
instance tg3.c, b44.c, sky2.c or any recent driver in drivers/net.

-- 
Ueimor

  reply	other threads:[~2007-02-06  0:33 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-02  8:17 許恆嘉
2007-02-03  0:39 ` Francois Romieu
2007-02-05  3:47   ` 許恆嘉
2007-02-06  0:31     ` Francois Romieu [this message]
2007-02-06  2:44       ` 許恆嘉
2007-02-06 21:38         ` Francois Romieu
2007-02-07  2:56           ` 許恆嘉
2007-02-07  3:25           ` 許恆嘉
2007-02-07 23:49             ` Francois Romieu
2007-02-08  1:18               ` 許恆嘉
  -- strict thread matches above, loose matches on Subject: below --
2007-02-02  7:08 許恆嘉
2007-02-02  8:03 ` Andrey Panin
2007-02-02  8:06 ` Andrew Morton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070206003130.GA18236@electric-eye.fr.zoreil.com \
    --to=romieu@fr.zoreil.com \
    --cc=edward_hsu@realtek.com.tw \
    --cc=hiwu@realtek.com.tw \
    --cc=jeff@garzik.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 2.6.19.2] r8169: support RTL8169SC/8110SC' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).