Current-Users archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: BCM5809S support in bnx(4) and brgphy(4)

On Wed, 1 Dec 2010 18:32:52 +0900, Izumi Tsutsui <> wrote:
We should not refer struct bnx_softc in brgphy.c
because mii devices can be configured without PCI
and bnx_softc contains PCI specific stuff.
Your patch might break GENERIC on zaurus, which
has mii via on-chip USB but no PCI bus.

To pass bnx_foo_flag values from parent bnx(4) to child brgphy(4),
we need the following two changes

 - use device properties (proplib) to pass flag values
 - split if_bnxreg.h and move PCI specific stuff
   ("Device State Data", debug macro etc.) into (new) if_bnxvar.h

as done on past brgphy(4) changes for bge(4) by msaitoh@:

Ok, I will fix that.

--- sys/dev/pci/if_bnxreg.h     19 Jan 2010 22:07:00 -0000      1.10
+++ sys/dev/pci/if_bnxreg.h     30 Nov 2010 23:58:54 -0000
+       u_int16_t tx_bd_vlan_tag;
+       u_int16_t tx_bd_flags;
        u_int16_t tx_bd_flags;
        u_int16_t tx_bd_vlan_tag;

It would be clearer to use one uint32_t member with shift ops
for bd_flags and bd_vlan_tag rather than two uint16_t members
with reorder #ifdef while the above one will also work...

I mirrored what the status_block is doing a few lines below.

Also, there is a comment above saying that:

 * The following data structures are generated from RTL code.
 * Do not modify any values below this line.

/* Do not modify any of the following data structures, they are generated */ /* from RTL code. */ /* */ /* Begin machine generated definitions. */

IMHO, moving from two uint16_t to one uint_32_t is probably not the best thing to do.

Nonetheless, what would you do? Something along this line?

--- if_bnxreg.h.orig    2010-12-01 11:10:29.000000000 +0100
+++ if_bnxreg.h 2010-12-01 11:26:26.000000000 +0100
@@ -718,8 +718,16 @@ struct tx_bd {
        u_int32_t tx_bd_haddr_hi;
        u_int32_t tx_bd_haddr_lo;
        u_int32_t tx_bd_mss_nbytes;
-       u_int16_t tx_bd_flags;
-       u_int16_t tx_bd_vlan_tag;
+       u_int32_t tx_bd_flags_vtag;
+#define VAL_HIGH(val) (((val) & 0xffff0000) >> 16)
+#define VAL_LOW(val)   ((val) & 0x0000ffff)
+#define TX_BD_VLAN_TAG(tx_bd)  VAL_HIGH((tx_bd)->tx_bd_flags_vtag)
+#define TX_BD_FLAGS(tx_bd)     VAL_LOW ((tx_bd)->tx_bd_flags_vtag)
+#define TX_BD_VLAN_TAG(tx_bd)  VAL_LOW ((tx_bd)->tx_bd_flags_vtag)
+#define TX_BD_FLAGS(tx_bd)     VAL_HIGH((tx_bd)->tx_bd_flags_vtag)
                #define TX_BD_FLAGS_CONN_FAULT          (1<<0)
                #define TX_BD_FLAGS_TCP_UDP_CKSUM       (1<<1)
                #define TX_BD_FLAGS_IP_CKSUM            (1<<2)

Looks messy :/

Thanks for your review.

Jean-Yves Migeon

Home | Main Index | Thread Index | Old Index