diff mbox

[v2,2/3] e1000: allow command-line selection of card model

Message ID 20140529160058.GE1676@ERROL.INI.CMU.EDU
State New
Headers show

Commit Message

Gabriel L. Somlo May 29, 2014, 4 p.m. UTC 
Hi Peter,

Thanks for the QOM crash course !

So I did s/TYPE_E1000/TYPE_E1000_BASE/ as suggested by Stefan, and
also moved PCI_VENDOR_ID_INTEL back into e1000_class_init() and away
from e1000_devices[] as you suggested. That's going to be part of
v3, as soon as we're done talking about QOM, see below :)

On Wed, May 28, 2014 at 11:02:10PM +1000, Peter Crosthwaite wrote:
> So I would guess with a bit more QOMification of the subtypes, it's
> possible to promote the phy_id2 to class data and remove the (late)
> lookup giving some decoupling between PCI dev id and this phy id.
> 
> I'll make some notes through the patch on the basic idea.
> 
> You could also do it for your is_8257x flag, just set it true in the
> .info's that matter. Then grab it from class to obj at init time
> (slightly optional, but avoids QOM-in-data-path).

I've attached a new patch (could be fourth in the series, or merged in
with the main dev_id patch. My only concern regarding a replacement of
e1000_phy_id2_init() and e1000_is_8257x() is that now I'll *have* to
include a .phy_id2 (and possibly a .is_8257x) field with every entry
in the e1000_devices[] array (unless there's a way to set a default
"phy_id2 = 0xc20" and a default "is_8257x = false" somewhere, and
inherit it unless explicitly overridden for the relevant device IDs
during initialization ?

Note that we're not even supporting any 8257x cards, so the switch
statement is just there for documentation purposes at this time...

Thanks much,

--Gabriel

Comments

Peter Crosthwaite May 29, 2014, 10:06 p.m. UTC | #1 
On Fri, May 30, 2014 at 2:00 AM, Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> Hi Peter,
>
> Thanks for the QOM crash course !
>
> So I did s/TYPE_E1000/TYPE_E1000_BASE/ as suggested by Stefan, and
> also moved PCI_VENDOR_ID_INTEL back into e1000_class_init() and away
> from e1000_devices[] as you suggested. That's going to be part of
> v3, as soon as we're done talking about QOM, see below :)
>
> On Wed, May 28, 2014 at 11:02:10PM +1000, Peter Crosthwaite wrote:
>> So I would guess with a bit more QOMification of the subtypes, it's
>> possible to promote the phy_id2 to class data and remove the (late)
>> lookup giving some decoupling between PCI dev id and this phy id.
>>
>> I'll make some notes through the patch on the basic idea.
>>
>> You could also do it for your is_8257x flag, just set it true in the
>> .info's that matter. Then grab it from class to obj at init time
>> (slightly optional, but avoids QOM-in-data-path).
>
> I've attached a new patch (could be fourth in the series, or merged in
> with the main dev_id patch. My only concern regarding a replacement of
> e1000_phy_id2_init() and e1000_is_8257x() is that now I'll *have* to
> include a .phy_id2 (and possibly a .is_8257x) field with every entry
> in the e1000_devices[] array (unless there's a way to set a default
> "phy_id2 = 0xc20"

Well class_init is open coded and gives you a chance to implement
semantics like that when translating the Info struct to the actual
usable classes. If you do it that early stages, then all your classes
will have correct default phy_id2 with only one LOC needed. I am
however assuming 0 is not a valid phy_id2?

> and a default "is_8257x = false"

And a "false" or 0 default cost you nothing, as you just rely on
static const structs init to 0 anyway.

Regards,
Peter
diff mbox

Patch

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 7a1a681..33a93ba 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -160,11 +160,21 @@  typedef struct E1000State_st {
     bool is_8257x;
 } E1000State;
 
+typedef struct E1000DeviceClass {
+    PCIDeviceClass parent_class;
+    uint16_t phy_id2;
+} E1000DeviceClass;
+
 #define TYPE_E1000_BASE "e1000-base"
 
 #define E1000(obj) \
     OBJECT_CHECK(E1000State, (obj), TYPE_E1000_BASE)
 
+#define E1000_DEVICE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(E1000DeviceClass, (klass), TYPE_E1000_BASE)
+#define E1000_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(E1000DeviceClass, (obj), TYPE_E1000_BASE)
+
 #define	defreg(x)	x = (E1000_##x>>2)
 enum {
     defreg(CTRL),	defreg(EECD),	defreg(EERD),	defreg(GPRC),
@@ -384,7 +394,7 @@  rxbufsize(uint32_t v)
 static void e1000_reset(void *opaque)
 {
     E1000State *d = opaque;
-    PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(d);
+    E1000DeviceClass *edc = E1000_DEVICE_GET_CLASS(d);
     uint8_t *macaddr = d->conf.macaddr.a;
     int i;
 
@@ -395,7 +405,7 @@  static void e1000_reset(void *opaque)
     d->mit_ide = 0;
     memset(d->phy_reg, 0, sizeof d->phy_reg);
     memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
-    d->phy_reg[PHY_ID2] = e1000_phy_id2_init(pdc->device_id);
+    d->phy_reg[PHY_ID2] = edc->phy_id2;
     memset(d->mac_reg, 0, sizeof d->mac_reg);
     memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init);
     d->rxbuf_min_shift = 1;
@@ -1613,12 +1623,14 @@  typedef struct E1000Info_st {
     const char *name;
     uint16_t   device_id;
     uint8_t    revision;
+    uint16_t   phy_id2;
 } E1000Info;
 
 static void e1000_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    E1000DeviceClass *e = E1000_DEVICE_CLASS(klass);
     const E1000Info *info = data;
 
     k->init = pci_e1000_init;
@@ -1627,6 +1639,7 @@  static void e1000_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = info->device_id;
     k->revision = info->revision;
+    e->phy_id2 = info->phy_id2;
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
     dc->desc = "Intel Gigabit Ethernet";
@@ -1639,29 +1652,33 @@  static const TypeInfo e1000_base_info = {
     .name          = TYPE_E1000_BASE,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(E1000State),
+    .class_size    = sizeof(E1000DeviceClass),
     .abstract      = true,
 };
 
+static const TypeInfo e1000_default_info = {
+    .name          = "e1000",
+    .parent        = "e1000-82540em",
+};
+
 static const E1000Info e1000_devices[] = {
     {
-        .name      = "e1000", /* default, alias for "e1000-82540em" */
-        .device_id = E1000_DEV_ID_82540EM,
-        .revision  = 0x03,
-    },
-    {
         .name      = "e1000-82540em",
         .device_id = E1000_DEV_ID_82540EM,
         .revision  = 0x03,
+        .phy_id2   = 0xc20,
     },
     {
         .name      = "e1000-82544gc",
         .device_id = E1000_DEV_ID_82544GC_COPPER,
         .revision  = 0x03,
+        .phy_id2   = 0xc30,
     },
     {
         .name      = "e1000-82545em",
         .device_id = E1000_DEV_ID_82545EM_COPPER,
         .revision  = 0x03,
+        .phy_id2   = 0xc20,
     },
 };
 
@@ -1670,6 +1687,7 @@  static void e1000_register_types(void)
     int i;
 
     type_register_static(&e1000_base_info);
+    type_register_static(&e1000_default_info);
     for (i = 0; i < ARRAY_SIZE(e1000_devices); i++) {
         const E1000Info *info = &e1000_devices[i];
         TypeInfo type_info = {};