Re: [9fans] pc/ether8169.c vs Realtek released driver

[tlaronde]

Hello Aidan,

I have started to look more closely at the various implementations
(Realtek provided, NetBSD --- indirectly from OpenBSD --- and Linux)
and your patch and I think we are on the same tracks.

Realtek uses the two masks for the macver: 0xFCF0 to do what is
done (limitedly) with the macver on the current driver, and to set the
MACFG (MAC firmware conFiG?) type. The second mask 0x7C80 is used
to refine with subconfiguration. I think we should do the same: the
general matching first; and handling the special cases, apart, after
without looping again.

One finds this for debugging in the Realtek driver:

sc->re_8169_MacVersion = (CSR_READ_4(sc, RE_TXCFG)&0x7c80)>>25;
/* Get bit 26~30*/  
sc->re_8169_MacVersion |= ((CSR_READ_4(sc, RE_TXCFG)&0x0080)!=0 ? 1: 0);
 /* Get bit 23   */  
DBGPRINT1(sc->re_unit,"8169 Mac Version %d",sc->re_8169_MacVersion);  

So all are considered variants of the 8169, and I don't think we
should bother to continue trying to spot a version with enumerations
like:

Macv01  = 0x,   /* RTL8169 */
Macv02  = 0x0080,   /* RTL8169S/8110S */
Macv03  = 0x0400,   /* RTL8169S/8110S */
...

but instead use, in a header, the MACFG_[0-9][0-9] used in the Realtek
driver and to let the hardcoded macver as is in a switch, simply
setting a type member in the Ctlr structure. This doesn't add any
complexity and this push the code nearer to what Realtek release, so
it will be more easy to read for people coming after us on the driver.

I think we agree that all the specific "black box" instructions for
the 8125 should be severed from the code. It is not binary firmware
(but in the spirit, it is), so we could put this in the header to keep the
C file humanly understandable (the Realtek if_re.c is almost 2MB in size!).

Since I'm on it, I will add also the 8126, even if I
don't have one, and add the variants:

enum {  /* Variants */
Rtl8100e= (0x8136<<16)|0x10EC,  /* RTL810[01]E: pci -e */
Rtl8169c= (0x0116<<16)|0x16EC,  /* RTL8169C+ (USR997902) */
Rtl8125 = (0x8125<<16)|0x10EC,  /* RTL8125 pci-e */
Rtl8126 = (0x8126<<16)|0x10EC,  /* RTL8126 pci-e */
Rtl8129 = (0x8129<<16)|0x10EC,  /* RTL8129 */
Rtl8139 = (0x8139<<16)|0x10EC,  /* RTL8139 */
Rtl8111b= (0x8161<<16)|0x10EC,  /* RTL8111/8168/8411: pci-e */
Rtl8168kb   = (0x8162<<16)|0x10EC,  /* RTL8168KB */
Rtl8169sc   = (0x8167<<16)|0x10EC,  /* RTL8169SC */
Rtl8168b= (0x8168<<16)|0x10EC,  /* RTL8168B: pci-e */
Rtl8169 = (0x8169<<16)|0x10EC,  /* RTL8169 */
};
 

What I'm unclear about at the moment is the Wifi support. 

More generally, wouldn't it be more clear to use PCI_VENDOR_ID, PCI_DEVICE_ID,
etc. from a header instead of the hardcoded values? (that may be hard
to grep to find if support is there or not---I missed at first that
there is a U.S. Robotics card with a Realtek chip in the list since
the vendor id is 0x16EC, to be compared with Realtek 0x10EC.)

What do you, and what do other think about this?

I think I will have to "steal" a couple of hours this week every day
to come with something testable at the end of the week.

Best,

T. Laronde


On Fri, Mar 07, 2025 at 09:08:45PM -0800, Aidan K. Wiggins via 9fans wrote:
> Hi Thierry,
> 
> > Thank you for the link. I will give it a look this week-end and will
> > go back to you during the next week. Hopefully, joining our efforts,
> > we should manage to get the cards wortking.
> 
> Excellent! It will be good to have more work/hardware when it comes to
> these Realtek chips. Overtime, I will be updating the linked file of
> any bugs/cleanups I find, which I have also now moved to
> http://oneiri.one/9front/ether8169.patch for the time being. (One
> problem I have encountered is losing Rx after unplugging the cable for
> a couple of seconds, but this looks well evidenced in commits relating
> to the setting of something called "aldps", which I'll try to get
> going tonight.)
> 
> >  From a cursory look, there are two distinct uses: one mask to match a
> > macver, and another mask to select a revision, with both setting
> > divergent things. In our present drivers, it seems the two cases are
> > more or less mixed---once again: I'm just starting to discover the
> > whole thing, so I may be wrong.
> 
> Yes this is right, and we do mix them. We (like Linux), are just
> making it so that the Mac version _and_ the revision ID identify the
> card (Macv |= RevID); thus having the mask of 0x[7F]CF00 lets us
> differentiate between tested cards by switching on Macv[0-9]+, like
> the two RTL8125D versions (as opposed to sub-switching elsewhere in
> the code for every time we test for a 8125D).
> 
> > Yes, the 8125 specially has a bunch of hard coded commands, given by
> > Realtek, and no explanations. Th

Re: [9fans] pc/ether8169.c vs Realtek released driver

[ori]

Quoth tlaro...@kergis.com:
> 
> More generally, wouldn't it be more clear to use PCI_VENDOR_ID, PCI_DEVICE_ID,
> etc. from a header instead of the hardcoded values? (that may be hard
> to grep to find if support is there or not---I missed at first that
> there is a U.S. Robotics card with a Realtek chip in the list since
> the vendor id is 0x16EC, to be compared with Realtek 0x10EC.)
> 
> What do you, and what do other think about this?

It's far clearer, in my books, to have:

switch(did){
case 0x8169:/* Rtl8169 */
...
}


in the one place that it's used, than to have it hidden in
a header somewhere. All of our drivers are currently self
contained, and use this pattern. Let's not break that.



--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T832e366730c74bfa-M45b2fce46acb2a1c83d73a14
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

[9fans] IWP9 trip

[Shawn Rutledge]

So who’s planning to go?  I think we should get started coordinating the 
lodging so we can get the flights ahead of time too. https://iwp9.org/#loc says 
“To be provided”.  If we’re just going to get Airbnb’s and such on our own, 
that’s fine with me, and I could share with someone.

Sorry to bother the list with this; we could maybe coordinate amongst those who 
are going, to avoid bothering everyone else.


--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/Tb0a06806516f2a40-M2153b1e75a062152f71dd802
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] NIX this morning: move to GitHub.com/rminnich/9front/heads/nix

[Christopher Nielsen]

I am still interested in helping, but life has been throwing curve balls.
So I've been a bit preoccupied.

I'll see if I can have a look at some of the simpler stuff this weekend. At
least setup a test environment.

*--*
*"The best programs are the ones written when the programmer is supposed to
be working on something else." -- Melinda Varian*

On Sat, Mar 8, 2025, 09:18 ron minnich  wrote:

> github.com/rminnich/9front/nix
> is all nix changes to date, squashed to one commit, with some unsightly
> language removed from pc/squidboy.c
>
> I'll try to get a list of TODOs out later today. Some of them are easy
> (format code), less easy (carefully remove debug prints), harder (reduce
> the number of things we need to change), and hard (fix trap handling).
>
> Something for everyone.
> *9fans * / 9fans / see discussions
>  + participants
>  + delivery options
>  Permalink
> 
>

--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T6478824d1072b1d0-M120f63f34f68ad4579c22c31
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pc/ether8169.c vs Realtek released driver

[tlaronde]

On Sun, Mar 09, 2025 at 03:00:46PM -0400, o...@eigenstate.org wrote:
> Quoth tlaro...@kergis.com:
> > 
> > More generally, wouldn't it be more clear to use PCI_VENDOR_ID, 
> > PCI_DEVICE_ID,
> > etc. from a header instead of the hardcoded values? (that may be hard
> > to grep to find if support is there or not---I missed at first that
> > there is a U.S. Robotics card with a Realtek chip in the list since
> > the vendor id is 0x16EC, to be compared with Realtek 0x10EC.)
> > 
> > What do you, and what do other think about this?
> 
> It's far clearer, in my books, to have:
> 
> switch(did){
> case 0x8169:/* Rtl8169 */
> ...
> }
> 
> 
> in the one place that it's used, than to have it hidden in
> a header somewhere. All of our drivers are currently self
> contained, and use this pattern. Let's not break that.
> 

Would you accept at least, in the very file, defines:

#define REALTEK_VID 0x10EC
#define USROBOTICS_VID 0x16EC

so that one can write:

Rtl8100e= (0x8136<<16)|REALTEK_VID, /* RTL810[01]E: pci -e 
*/
Rtl8169c= (0x0116<<16)|USROBOTICS_VID,  /* RTL8169C+ 
(USR997902) */

that is far easier to catch instead of 0x10EC vs 0x16EC, one above the
other (I thought it was a typo before consulting the PCI databases).

-- 
Thierry Laronde 
 http://www.kergis.com/
http://kertex.kergis.com/
Key fingerprint = 0FF7 E906 FBAF FE95 FD89  250D 52B1 AE95 6006 F40C

--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T832e366730c74bfa-M4e199640c27164a0b32e7411
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pc/ether8169.c vs Realtek released driver

[tlaronde]

On Sun, Mar 09, 2025 at 12:46:55PM -0700, Aidan K. Wiggins via 9fans wrote:
> Hi Thierry,
> 
> > Realtek uses the two masks for the macver: 0xFCF0 to do what is
> > done (limitedly) with the macver on the current driver, and to set the
> > MACFG (MAC firmware conFiG?) type. The second mask 0x7C80 is used
> > to refine with subconfiguration. I think we should do the same: the
> > general matching first; and handling the special cases, apart, after
> > without looping again.
> 
> I think I understand what you're getting at here: instead of having an
> enum constant for every little card, we should be using the MACFG
> values as indicated by the result of ANDing with 0xFCF00, as seen
> in re_check_mac_version() of Realtek's BSD source. I think there is
> merit in this approach, but it would require changing more of the code
> than I thought immediately necessary, which is partially why I just
> decided to follow suit in borrowing values from Linux (esp. with it's
> large userbase included). With looping in our vetmacv(), I was
> attempting to keep some level of guaranteed backwards compatibility,
> since I wasn't sure if a more specific (0xFCF) mask would tease out a
> new Macv value from an older card that was otherwise previously
> supported under the mask 0x7C80.
> 

The 8125 will add so much code that I think we can depart for some
things from the current state. Since the first test is about the PCI
device ID, before verifying the macver, the problems should be unlikely.

But my mind is not set for now about the style. What I want to find is
something both small and clear enough for anyone having to work with
the driver to understand rapidly what is done. The Linux team seems to
be working to some extend with the hardware producers, so I will, too,
keep also the Linux driver as a guide---but I give a kind of priority
to the Realtek provided one because they are supposed to know what
they do produce ;-)

One could also define flags and have a struct with the hardware
version and a definition of the corresponding flags, with the
initialization simply done if the flag if set.

> > What I'm unclear about at the moment is the Wifi support. 
> 
> Can you expand on this?
> 

My card at least, has a RJ45 but provides wireless connection too. I have
to find if the distinction is the device ID, or the function (or a 
revision), and if the Realtek driver has some parts dedicated to wireless
configuration.

So the question: are the wireless features mixed with this and should
be done "here", or can be postpone being supposedly dealt with by
another driver?

> > More generally, wouldn't it be more clear to use PCI_VENDOR_ID, 
> > PCI_DEVICE_ID,
> > etc. from a header instead of the hardcoded values? (that may be hard
> > to grep to find if support is there or not---I missed at first that
> > there is a U.S. Robotics card with a Realtek chip in the list since
> > the vendor id is 0x16EC, to be compared with Realtek 0x10EC.)
> 
> I agree with Ori's approach here, the device IDs are so specific that
> they really only need inclusion in their respective drivers. We do,
> however, have some vendor IDs in pci.h, so you could have:
> 
> #define PCIdev(vid, did)((vid)<<16|(did))
> enum{
> Rtl8169 = PCIdev(Vrealtek, 0x8169),
> Rtl8169c= PCIdev(0x16ec, 0x0116)
> ...
> }
> 
> switch(PCIdev(p->vid, p->did)){
> ...
> }

Yep, I like it more this way!

> 
> If it weren't for these non-realtek chips, I think we would just use
> switch(did){}.
> 

The problem is that there is two "alien" cards: the US ROBOTICS, set
in the enum, and one hard coded in the switch (to fall back to the
Rtl8169 case):

case (0xC107<<16)|0x1259:   /* Corega CG-LAPCIGT */

(These are the exceptions that we will have to verify we are not dropping
by inadvertence.)

-- 
Thierry Laronde 
 http://www.kergis.com/
http://kertex.kergis.com/
Key fingerprint = 0FF7 E906 FBAF FE95 FD89  250D 52B1 AE95 6006 F40C

--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T832e366730c74bfa-M7c40e88b71cc8ae8dd406a0a
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pc/ether8169.c vs Realtek released driver

[ori]

Quoth Aidan K. Wiggins via 9fans <9fans@9fans.net>:
> If it weren't for these non-realtek chips, I think we would just use
> switch(did){}.

Ugh. yes. I figure it'd be fine to add

enum {
/*
 * Some realtek chips are sold with
 * the US robotics vendor id.
 */
VRtl = 0x10EC,
VUsr = 0x16EC,
};

Usually, we'd just check the vendor and
switch on the device id.

(these realtek cards are a pain; a ton of
odd special cases...)


--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T832e366730c74bfa-M8f1600a2bbcbb03367923327
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pc/ether8169.c vs Realtek released driver

[Aidan K. Wiggins via 9fans]

Hi Thierry,

> Realtek uses the two masks for the macver: 0xFCF0 to do what is
> done (limitedly) with the macver on the current driver, and to set the
> MACFG (MAC firmware conFiG?) type. The second mask 0x7C80 is used
> to refine with subconfiguration. I think we should do the same: the
> general matching first; and handling the special cases, apart, after
> without looping again.

I think I understand what you're getting at here: instead of having an
enum constant for every little card, we should be using the MACFG
values as indicated by the result of ANDing with 0xFCF00, as seen
in re_check_mac_version() of Realtek's BSD source. I think there is
merit in this approach, but it would require changing more of the code
than I thought immediately necessary, which is partially why I just
decided to follow suit in borrowing values from Linux (esp. with it's
large userbase included). With looping in our vetmacv(), I was
attempting to keep some level of guaranteed backwards compatibility,
since I wasn't sure if a more specific (0xFCF) mask would tease out a
new Macv value from an older card that was otherwise previously
supported under the mask 0x7C80.

> What I'm unclear about at the moment is the Wifi support. 

Can you expand on this?

> More generally, wouldn't it be more clear to use PCI_VENDOR_ID, PCI_DEVICE_ID,
> etc. from a header instead of the hardcoded values? (that may be hard
> to grep to find if support is there or not---I missed at first that
> there is a U.S. Robotics card with a Realtek chip in the list since
> the vendor id is 0x16EC, to be compared with Realtek 0x10EC.)

I agree with Ori's approach here, the device IDs are so specific that
they really only need inclusion in their respective drivers. We do,
however, have some vendor IDs in pci.h, so you could have:

#define PCIdev(vid, did)((vid)<<16|(did))
enum{
Rtl8169 = PCIdev(Vrealtek, 0x8169),
Rtl8169c= PCIdev(0x16ec, 0x0116)
...
}

switch(PCIdev(p->vid, p->did)){
...
}

If it weren't for these non-realtek chips, I think we would just use
switch(did){}.

> I think I will have to "steal" a couple of hours this week every day
> to come with something testable at the end of the week.

Sounds good :)

Aidan

--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T832e366730c74bfa-M1845e8001ee31b4a83f624ab
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] Re: pc/ether8169.c vs Realtek released driver

[Aidan K. Wiggins via 9fans]

Hrm,

I see what you mean, I think their usage of 0xFC80 is to catch the
higher bits in older cards, looking specifically at the definitions
for the 8110 family in the linux driver. It seems that up to this
point all the cards defined in our driver can be recognized with
0x7C[8F]0, but if someone walks along with hwverid 0x9800, I
think we safely use 0xFC[8F]0. Or is that we have instead been
using Macv05 == 0x9800 & 0x7C80 == 0x1800 (as opposed to
the right mask & and the right value)?

Aidan

--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T832e366730c74bfa-M6403cd1a89c90dc67ed109ce
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] NIX this morning: move to GitHub.com/rminnich/9front/heads/nix

[ron minnich]

I'll try to throw together a TODO.md this week for people to work from.

On Sun, Mar 9, 2025 at 5:48 AM Christopher Nielsen 
wrote:

> I am still interested in helping, but life has been throwing curve balls.
> So I've been a bit preoccupied.
>
> I'll see if I can have a look at some of the simpler stuff this weekend.
> At least setup a test environment.
>
> *--*
> *"The best programs are the ones written when the programmer is supposed
> to be working on something else." -- Melinda Varian*
>
> On Sat, Mar 8, 2025, 09:18 ron minnich  wrote:
>
>> github.com/rminnich/9front/nix
>> is all nix changes to date, squashed to one commit, with some unsightly
>> language removed from pc/squidboy.c
>>
>> I'll try to get a list of TODOs out later today. Some of them are easy
>> (format code), less easy (carefully remove debug prints), harder (reduce
>> the number of things we need to change), and hard (fix trap handling).
>>
>> Something for everyone.
>>
> *9fans * / 9fans / see discussions
>  + participants
>  + delivery options
>  Permalink
> 
>

--
9fans: 9fans
Permalink: 
https://9fans.topicbox.com/groups/9fans/T6478824d1072b1d0-Md6271362af31a8d6c1603e13
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription