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

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

On Wed, 1 Dec 2010 18:32:52 +0900, Izumi Tsutsui 
<tsutsui%ceres.dti.ne.jp@localhost> 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@:
> http://mail-index.NetBSD.org/current-users/2009/04/20/msg009101.html
> http://mail-index.NetBSD.org/source-changes/2009/04/23/msg220278.html

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
>  :
> > +#if BYTE_ORDER == BIG_ENDIAN
> > +       u_int16_t tx_bd_vlan_tag;
> > +       u_int16_t tx_bd_flags;
> > +#else
> >         u_int16_t tx_bd_flags;
> >         u_int16_t tx_bd_vlan_tag;
> > +#endif
>
> 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)
+#if BYTE_ORDER == BIG_ENDIAN
+#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)
+#else
+#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)
+#endif
                #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
jeanyves.migeon%free.fr@localhost