Re: BCM5809S support in bnx(4) and brgphy(4)
On 01.12.2010 11:41, Jean-Yves Migeon wrote: On Wed, 1 Dec 2010 18:32:52 +0900, Izumi Tsutsui tsut...@ceres.dti.ne.jp 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. Updated [1]. Differences with previous version: - uses proplib(3) to query phyflags, and store the value in brgphy_softc, like bge code does. bnx_softc should not be exposed to brgphy(4) now. - split if_bnxreg.h in two, with macros, device state, PCI bus stuff in if_bnxvar.h, and only include if_bnxreg.h in brgphy.c - set bits in 'mii_capabilities' so mii_phy_add_media() can be used to attach BCM 5709S PHYs too (maximizes code reuse). Re-tested, and works. Unless someone has comments, I will commit in a few days (say: Thursday, 09). If bnx(4) owners could test it, I would appreciate it, especially on BE machines, like sparc64. I did my best to avoid breakage, but I can't really confirm it (I don't own any Broadcom cards for bnx(4)) [1] http://netbsd.org/~jym/bcm5709s.diff -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: BCM5809S support in bnx(4) and brgphy(4)
I have ported various modifications from OpenBSD and FreeBSD to bnx(4)
and brgphy(4) (see attach).
:
--- sys/dev/mii/brgphy.c 27 Nov 2010 17:42:04 - 1.56
+++ sys/dev/mii/brgphy.c 30 Nov 2010 23:58:49 -
:
#include dev/pci/if_bgereg.h
-#if 0
#include dev/pci/if_bnxreg.h
-#endif
:
+ if (device_is_a(parent, bnx)) {
+ struct bnx_softc *bnx_sc = device_private(parent);
:
+ if (bnx_sc-bnx_phy_flags BNX_PHY_2_5G_CAPABLE_FLAG) {
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
--- sys/dev/pci/if_bnxreg.h 19 Jan 2010 22:07:00 - 1.10
+++ sys/dev/pci/if_bnxreg.h 30 Nov 2010 23:58:54 -
:
+#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...
---
Izumi Tsutusi
Re: BCM5809S support in bnx(4) and brgphy(4)
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. */ IMO all members fetched via PCI bus master should be uint32_t if hardware supports byte swap ops since swap is always done against each uint32_t. -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) 0x) 16) +#define VAL_LOW(val) ((val) 0x) +#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 : Looks messy :/ Well, no need to use #if BYTE_ORDER if we use uint32_t with shift ops because word swap is done by hardware. (that's the reason why we have to swap uint16_t members by #ifdef) Anyway, no problem to leave them as is to sync FreeBSD/OpenBSD. --- Izum iTsutsui
Re: BCM5809S support in bnx(4) and brgphy(4)
On Wed, 1 Dec 2010 20:03:20 +0900, Izumi Tsutsui tsut...@ceres.dti.ne.jp wrote: Well, no need to use #if BYTE_ORDER if we use uint32_t with shift ops because word swap is done by hardware. (that's the reason why we have to swap uint16_t members by #ifdef) Anyway, no problem to leave them as is to sync FreeBSD/OpenBSD. Alright, I see. I would prefer to leave it as is, with this small quirk in the struct. I favor making developer's work easier by having less churn with OpenBSD/FreeBSD drivers. Cheers, -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: BCM5809S support in bnx(4) and brgphy(4)
On Wed, 1 Dec 2010, Izumi Tsutsui wrote: 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. */ IMO all members fetched via PCI bus master should be uint32_t if hardware supports byte swap ops since swap is always done against each uint32_t. Huh? [...] Well, no need to use #if BYTE_ORDER if we use uint32_t with shift ops because word swap is done by hardware. (that's the reason why we have to swap uint16_t members by #ifdef) What swap is done by what hardware? Most of the time any swapping is done by a series of shift and mask instructions with the current implementation, and swapping a 16-bit value is much less expensive than swapping a 32-bit value. Sigh. I really need to get off my butt and add a bunch of bus_dma accessors (like the bus_space ones but for DMA buffers) to solve this problem optimally across all architectures. Eduardo
Re: BCM5809S support in bnx(4) and brgphy(4)
What swap is done by what hardware? Some bus masters like bnx(4) and epic(4) treat host's memory as BE if it's configured so. No byteswap against DMA descripters is necessary in the driver in such case. Using two uint16_t members against a uint32_t descripter requires extra ifdefs to switch BE and LE environments as bitfield in structure. Sigh. I really need to get off my butt and add a bunch of bus_dma accessors (like the bus_space ones but for DMA buffers) to solve this problem optimally across all architectures. I guess you are thinking about extra support for bus controller that supports byteswap ops, not individual bus masters. --- Izumi Tsutsui
Re: BCM5809S support in bnx(4) and brgphy(4)
On Thu, 2 Dec 2010, Izumi Tsutsui wrote: What swap is done by what hardware? Some bus masters like bnx(4) and epic(4) treat host's memory as BE if it's configured so. No byteswap against DMA descripters is necessary in the driver in such case. Using two uint16_t members against a uint32_t descripter requires extra ifdefs to switch BE and LE environments as bitfield in structure. Ah, I see. Adding byte swapping to the DMA engine never works right. HW engineers are wacky. Sigh. I really need to get off my butt and add a bunch of bus_dma accessors (like the bus_space ones but for DMA buffers) to solve this problem optimally across all architectures. I guess you are thinking about extra support for bus controller that supports byteswap ops, not individual bus masters. Actually, things get a lot more complicated than that based on the hardware in question. Byte swapping can happen in lots of different places: 1 In registers (using things like the bswap instruction in x86) 2 In load/store instructions (PowerPC special LE load/store instructions) 3 In the MMU (UltraSPARCs can have a bit set in the PTE to swap the endianness of a page) 4 Bus controllers (haven't seen this one yet, but I'm sure you can do it) 5 The DMA engine (as above) 6 The device itself (This differs from #5 in that the device can issue sub-word accesses with the correct endiannes) When DMA memory is allocated or a DMA mapping is set up a negotiation must be done between the different layers to determine what the most efficient way to configure the system for a specific device. And don't forget the cache sync issues involved in accessing DMA memory. We're currently using an axe where a scalpel is probably more appropriate. Eduardo
