On Sun, 2011-10-09 at 12:30 +0200, Eli Cohen wrote: > > Ideally you want to avoid that swapping altogether and use the right > > accessor that indicates that your register is BE to start with. IE. > > remove the swab32 completely and then use something like > > iowrite32be() instead of writel(). > I agree, this looks better but does it work on memory mapped io or > only on io pci space? All our registers are memory mapped...
The iomap functions work on both. > > Basically, the problem you have is that writel() has an implicit "write > > to LE register" semantic. Your register is BE. the "iomap" variants > > provide you with more fine grained "be" variants to use in that case. > > There's also writel_be() but that one doesn't exist on every > > architecture afaik. > So writel_be is the function I should use for memory mapped io? If it > does not exist for all platforms it's a pitty :-( Just use the iomap variant. Usually you also use pci_iomap() instead of ioremap() but afaik, for straight MMIO, it works with normal ioremap as well. > > Now, once the mmio problem is out of the way, let's look back at how you > > then use that qpn. > > > > With the current code, you've generated something in memory which is > > byte reversed, so essentially "LE" on ppc and "BE" on x86. > > > > Then, this statement: > > > > *(u32 *) (&tx_desc->ctrl.vlan_tag) |= ring->doorbell_qpn; > > > > Will essentially write it out as-is in memory for use by the chip. The chip, > > from what you say, expects BE, so this will be broken on PPC. > I see. So this field is layed in le for ppc and the rest of the > descriptor is be. so I assum that __iowrite64_copy() does not swap > anything but we still have tx_desc->ctrl.vlan_tag in the wrong > endianess. Yes because you had swapped it initially. IE your original swab32 is what busts it for you on ppc. > > Here too, the right solution is to instead not do that swab32 to begin > > with (ring->doorbell_qpn remains a native endian value) and instead do, > > in addition to the above mentioned change to the writel: > > > > *(u32 *) (&tx_desc->ctrl.vlan_tag) |= cpu_to_be32(ring->doorbell_qpn); > > > > (Also get rid of that cast and define vlan_tag as a __be32 to start > > with). > > > > Cheers, > > Ben. > > Thanks for your review. I will send another patch which should fix the > deficiencies. Cheers, Ben. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
