[PATCH 1/2] ssb: fix DMA translation for some specific boards

[PATCH 1/2] ssb: fix DMA translation for some specific boards

[Rafał Miłecki]

Signed-off-by: Michael Buesch <m@bues.ch>
Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
---
 drivers/ssb/main.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
index 6ec6e09..dac2c0b 100644
--- a/drivers/ssb/main.c
+++ b/drivers/ssb/main.c
@@ -1260,16 +1260,32 @@ void ssb_device_disable(struct ssb_device *dev, u32 core_specific_flags)
 }
 EXPORT_SYMBOL(ssb_device_disable);
 
+/* Some chipsets need routing known for PCIe and 64-bit DMA */
+static bool ssb_dma_translation_special_bit(struct ssb_device *dev)
+{
+	u16 chip_id = dev->bus->chip_id;
+
+	if (dev->id.coreid == SSB_DEV_80211)
+		return (chip_id == 0x4322 || chip_id == 43221 ||
+			chip_id == 43231 || chip_id == 43222);
+	return 0;
+}
+
 u32 ssb_dma_translation(struct ssb_device *dev)
 {
 	switch (dev->bus->bustype) {
 	case SSB_BUSTYPE_SSB:
 		return 0;
 	case SSB_BUSTYPE_PCI:
-		if (ssb_read32(dev, SSB_TMSHIGH) & SSB_TMSHIGH_DMA64)
+		if (pci_is_pcie(dev->bus->host_pci) &&
+		    ssb_read32(dev, SSB_TMSHIGH) & SSB_TMSHIGH_DMA64) {
 			return SSB_PCIE_DMA_H32;
-		else
-			return SSB_PCI_DMA;
+		} else {
+			if (ssb_dma_translation_special_bit(dev))
+				return SSB_PCIE_DMA_H32;
+			else
+				return SSB_PCI_DMA;
+		}
 	default:
 		__ssb_dma_not_implemented(dev);
 	}
-- 
1.7.1

[PATCH 2/2] b43: fix DMA on some bugged hardware

[Rafał Miłecki]

Some hardware with 64-bit DMA uses lower address word for setting
routing (translation) bit. Add workaround for such boards.

Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
---
 drivers/net/wireless/b43/b43.h |    1 +
 drivers/net/wireless/b43/dma.c |   52 ++++++++++++++++++++++++++++++----------
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index c818b0b..3aee322 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -594,6 +594,7 @@ struct b43_dma {
 	struct b43_dmaring *rx_ring;
 
 	u32 translation; /* Routing bits */
+	bool translation_in_low; /* Should translation bit go into low addr? */
 	bool parity; /* Check for parity */
 };
 
diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
index 0953ce1..9a2b678 100644
--- a/drivers/net/wireless/b43/dma.c
+++ b/drivers/net/wireless/b43/dma.c
@@ -174,7 +174,10 @@ static void op64_fill_descriptor(struct b43_dmaring *ring,
 	addrhi = (((u64) dmaaddr >> 32) & ~SSB_DMA_TRANSLATION_MASK);
 	addrext = (((u64) dmaaddr >> 32) & SSB_DMA_TRANSLATION_MASK)
 	    >> SSB_DMA_TRANSLATION_SHIFT;
-	addrhi |= ring->dev->dma.translation;
+	if (ring->dev->dma.translation_in_low)
+		addrlo |= ring->dev->dma.translation;
+	else
+		addrhi |= ring->dev->dma.translation;
 	if (slot == ring->nr_slots - 1)
 		ctl0 |= B43_DMA64_DCTL0_DTABLEEND;
 	if (start)
@@ -656,10 +659,12 @@ static int alloc_initial_descbuffers(struct b43_dmaring *ring)
 static int dmacontroller_setup(struct b43_dmaring *ring)
 {
 	int err = 0;
+	int tmp;
 	u32 value;
 	u32 addrext;
 	u32 trans = ring->dev->dma.translation;
 	bool parity = ring->dev->dma.parity;
+	u32 addrs[2];
 
 	if (ring->tx) {
 		if (ring->type == B43_DMA_64BIT) {
@@ -673,12 +678,14 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
 			if (!parity)
 				value |= B43_DMA64_TXPARITYDISABLE;
 			b43_dma_write(ring, B43_DMA64_TXCTL, value);
-			b43_dma_write(ring, B43_DMA64_TXRINGLO,
-				      (ringbase & 0xFFFFFFFF));
-			b43_dma_write(ring, B43_DMA64_TXRINGHI,
-				      ((ringbase >> 32) &
-				       ~SSB_DMA_TRANSLATION_MASK)
-				      | trans);
+
+			addrs[0] = ringbase & 0xFFFFFFFF;
+			addrs[1] = ringbase >> 32;
+			tmp = ring->dev->dma.translation_in_low ? 0 : 1;
+			addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
+			addrs[tmp] |= trans;
+			b43_dma_write(ring, B43_DMA64_TXRINGLO, addrs[0]);
+			b43_dma_write(ring, B43_DMA64_TXRINGHI, addrs[1]);
 		} else {
 			u32 ringbase = (u32) (ring->dmabase);
 
@@ -710,12 +717,15 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
 			if (!parity)
 				value |= B43_DMA64_RXPARITYDISABLE;
 			b43_dma_write(ring, B43_DMA64_RXCTL, value);
-			b43_dma_write(ring, B43_DMA64_RXRINGLO,
-				      (ringbase & 0xFFFFFFFF));
-			b43_dma_write(ring, B43_DMA64_RXRINGHI,
-				      ((ringbase >> 32) &
-				       ~SSB_DMA_TRANSLATION_MASK)
-				      | trans);
+
+			addrs[0] = ringbase & 0xFFFFFFFF;
+			addrs[1] = ringbase >> 32;
+			tmp = ring->dev->dma.translation_in_low ? 0 : 1;
+			addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
+			addrs[tmp] |= trans;
+			b43_dma_write(ring, B43_DMA64_RXRINGLO, addrs[0]);
+			b43_dma_write(ring, B43_DMA64_RXRINGHI, addrs[1]);
+
 			b43_dma_write(ring, B43_DMA64_RXINDEX, ring->nr_slots *
 				      sizeof(struct b43_dmadesc64));
 		} else {
@@ -1052,6 +1062,21 @@ static int b43_dma_set_mask(struct b43_wldev *dev, u64 mask)
 	return 0;
 }
 
+/* Some hardware with 64-bit DMA seems to be bugged and looks for translation
+ * bit in low address word instead of high one.
+ */
+static bool b43_dma_translation_in_low_word(struct b43_wldev *dev)
+{
+#ifdef CONFIG_B43_SSB
+	if (dev->dev->bus_type == B43_BUS_SSB &&
+	    dev->dev->sdev->bus->bustype == SSB_BUSTYPE_PCI &&
+	    !(dev->dev->sdev->bus->host_pci->is_pcie &&
+	      ssb_read32(dev->dev->sdev, SSB_TMSHIGH) & SSB_TMSHIGH_DMA64))
+			return 1;
+#endif
+	return 0;
+}
+
 int b43_dma_init(struct b43_wldev *dev)
 {
 	struct b43_dma *dma = &dev->dma;
@@ -1077,6 +1102,7 @@ int b43_dma_init(struct b43_wldev *dev)
 		break;
 #endif
 	}
+	dma->translation_in_low = b43_dma_translation_in_low_word(dev);
 
 	dma->parity = true;
 #ifdef CONFIG_B43_BCMA
-- 
1.7.1

[PATCH 2/2] b43: fix DMA on some bugged hardware

[Michael Büsch]

On Wed, 10 Aug 2011 18:11:28 +0200
> diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
> index 0953ce1..9a2b678 100644
> --- a/drivers/net/wireless/b43/dma.c
> +++ b/drivers/net/wireless/b43/dma.c
> @@ -174,7 +174,10 @@ static void op64_fill_descriptor(struct b43_dmaring *ring,
>  	addrhi = (((u64) dmaaddr >> 32) & ~SSB_DMA_TRANSLATION_MASK);
>  	addrext = (((u64) dmaaddr >> 32) & SSB_DMA_TRANSLATION_MASK)
>  	    >> SSB_DMA_TRANSLATION_SHIFT;
> -	addrhi |= ring->dev->dma.translation;
> +	if (ring->dev->dma.translation_in_low)
> +		addrlo |= ring->dev->dma.translation;
> +	else
> +		addrhi |= ring->dev->dma.translation;
>  	if (slot == ring->nr_slots - 1)
>  		ctl0 |= B43_DMA64_DCTL0_DTABLEEND;
>  	if (start)
> @@ -656,10 +659,12 @@ static int alloc_initial_descbuffers(struct b43_dmaring *ring)
>  static int dmacontroller_setup(struct b43_dmaring *ring)
>  {
>  	int err = 0;
> +	int tmp;
>  	u32 value;
>  	u32 addrext;
>  	u32 trans = ring->dev->dma.translation;
>  	bool parity = ring->dev->dma.parity;
> +	u32 addrs[2];
>  
>  	if (ring->tx) {
>  		if (ring->type == B43_DMA_64BIT) {
> @@ -673,12 +678,14 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
>  			if (!parity)
>  				value |= B43_DMA64_TXPARITYDISABLE;
>  			b43_dma_write(ring, B43_DMA64_TXCTL, value);
> -			b43_dma_write(ring, B43_DMA64_TXRINGLO,
> -				      (ringbase & 0xFFFFFFFF));
> -			b43_dma_write(ring, B43_DMA64_TXRINGHI,
> -				      ((ringbase >> 32) &
> -				       ~SSB_DMA_TRANSLATION_MASK)
> -				      | trans);
> +
> +			addrs[0] = ringbase & 0xFFFFFFFF;
> +			addrs[1] = ringbase >> 32;
> +			tmp = ring->dev->dma.translation_in_low ? 0 : 1;
> +			addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
> +			addrs[tmp] |= trans;
> +			b43_dma_write(ring, B43_DMA64_TXRINGLO, addrs[0]);
> +			b43_dma_write(ring, B43_DMA64_TXRINGHI, addrs[1]);
>  		} else {
>  			u32 ringbase = (u32) (ring->dmabase);
>  
> @@ -710,12 +717,15 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
>  			if (!parity)
>  				value |= B43_DMA64_RXPARITYDISABLE;
>  			b43_dma_write(ring, B43_DMA64_RXCTL, value);
> -			b43_dma_write(ring, B43_DMA64_RXRINGLO,
> -				      (ringbase & 0xFFFFFFFF));
> -			b43_dma_write(ring, B43_DMA64_RXRINGHI,
> -				      ((ringbase >> 32) &
> -				       ~SSB_DMA_TRANSLATION_MASK)
> -				      | trans);
> +
> +			addrs[0] = ringbase & 0xFFFFFFFF;
> +			addrs[1] = ringbase >> 32;
> +			tmp = ring->dev->dma.translation_in_low ? 0 : 1;
> +			addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
> +			addrs[tmp] |= trans;
> +			b43_dma_write(ring, B43_DMA64_RXRINGLO, addrs[0]);
> +			b43_dma_write(ring, B43_DMA64_RXRINGHI, addrs[1]);
> +
>  			b43_dma_write(ring, B43_DMA64_RXINDEX, ring->nr_slots *
>  				      sizeof(struct b43_dmadesc64));
>  		} else {
> @@ -1052,6 +1062,21 @@ static int b43_dma_set_mask(struct b43_wldev *dev, u64 mask)
>  	return 0;
>  }


This doesn't look correct to me for several reasons:

In the fill-op the address is not masked correctly with the translation mask.
In both the fill-op and both ring setups, the actual address extension bits
are always taken from the address's high word. I guess the extension should
be the low word bits for devices where we use the low word. That's the only
thing that would make sense. But hey, it's not that we have sane hardware here.
So this has to be checked.

-- 
Greetings, Michael.

[PATCH 1/2] ssb: fix DMA translation for some specific boards

[Michael Büsch]

On Wed, 10 Aug 2011 18:11:27 +0200
> +/* Some chipsets need routing known for PCIe and 64-bit DMA */
> +static bool ssb_dma_translation_special_bit(struct ssb_device *dev)
> +{
> +	u16 chip_id = dev->bus->chip_id;
> +
> +	if (dev->id.coreid == SSB_DEV_80211)
> +		return (chip_id == 0x4322 || chip_id == 43221 ||
> +			chip_id == 43231 || chip_id == 43222);
> +	return 0;
> +}

ACK, apart from this coding style violation (missing curly braces).

-- 
Greetings, Michael.

[PATCH 1/2] ssb: fix DMA translation for some specific boards

[Rafał Miłecki]

W dniu 10 sierpnia 2011 18:40 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
> On Wed, 10 Aug 2011 18:11:27 +0200
>> +/* Some chipsets need routing known for PCIe and 64-bit DMA */
>> +static bool ssb_dma_translation_special_bit(struct ssb_device *dev)
>> +{
>> + ? ? u16 chip_id = dev->bus->chip_id;
>> +
>> + ? ? if (dev->id.coreid == SSB_DEV_80211)
>> + ? ? ? ? ? ? return (chip_id == 0x4322 || chip_id == 43221 ||
>> + ? ? ? ? ? ? ? ? ? ? chip_id == 43231 || chip_id == 43222);
>> + ? ? return 0;
>> +}
>
> ACK, apart from this coding style violation (missing curly braces).

[PATCH 1/2] ssb: fix DMA translation for some specific boards

[Michael Büsch]

On Wed, 10 Aug 2011 18:42:59 +0200
Rafa? Mi?ecki <zajec5@gmail.com> wrote:

> W dniu 10 sierpnia 2011 18:40 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
> > On Wed, 10 Aug 2011 18:11:27 +0200
> >> +/* Some chipsets need routing known for PCIe and 64-bit DMA */
> >> +static bool ssb_dma_translation_special_bit(struct ssb_device *dev)
> >> +{
> >> + ? ? u16 chip_id = dev->bus->chip_id;
> >> +
> >> + ? ? if (dev->id.coreid == SSB_DEV_80211)
> >> + ? ? ? ? ? ? return (chip_id == 0x4322 || chip_id == 43221 ||
> >> + ? ? ? ? ? ? ? ? ? ? chip_id == 43231 || chip_id == 43222);
> >> + ? ? return 0;
> >> +}
> >
> > ACK, apart from this coding style violation (missing curly braces).
> 
> From kernel CodingStyle:
> > Do not unnecessarily use braces where a single statement will do.
> >
> > if (condition)
> > 	action();
> 
> I guess simple "return" (even when using line break) is still like
> single statement...
> Am I wrong?
> 

Last time I checked this only applied to single-line-statements.
Maybe it changed, though.

-- 
Greetings, Michael.

[PATCH 2/2] b43: fix DMA on some bugged hardware

[Rafał Miłecki]

W dniu 10 sierpnia 2011 18:33 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
> On Wed, 10 Aug 2011 18:11:28 +0200
>> diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
>> index 0953ce1..9a2b678 100644
>> --- a/drivers/net/wireless/b43/dma.c
>> +++ b/drivers/net/wireless/b43/dma.c
>> @@ -174,7 +174,10 @@ static void op64_fill_descriptor(struct b43_dmaring *ring,
>> ? ? ? addrhi = (((u64) dmaaddr >> 32) & ~SSB_DMA_TRANSLATION_MASK);
>> ? ? ? addrext = (((u64) dmaaddr >> 32) & SSB_DMA_TRANSLATION_MASK)
>> ? ? ? ? ? >> SSB_DMA_TRANSLATION_SHIFT;
>> - ? ? addrhi |= ring->dev->dma.translation;
>> + ? ? if (ring->dev->dma.translation_in_low)
>> + ? ? ? ? ? ? addrlo |= ring->dev->dma.translation;
>> + ? ? else
>> + ? ? ? ? ? ? addrhi |= ring->dev->dma.translation;
>> ? ? ? if (slot == ring->nr_slots - 1)
>> ? ? ? ? ? ? ? ctl0 |= B43_DMA64_DCTL0_DTABLEEND;
>> ? ? ? if (start)
>> @@ -656,10 +659,12 @@ static int alloc_initial_descbuffers(struct b43_dmaring *ring)
>> ?static int dmacontroller_setup(struct b43_dmaring *ring)
>> ?{
>> ? ? ? int err = 0;
>> + ? ? int tmp;
>> ? ? ? u32 value;
>> ? ? ? u32 addrext;
>> ? ? ? u32 trans = ring->dev->dma.translation;
>> ? ? ? bool parity = ring->dev->dma.parity;
>> + ? ? u32 addrs[2];
>>
>> ? ? ? if (ring->tx) {
>> ? ? ? ? ? ? ? if (ring->type == B43_DMA_64BIT) {
>> @@ -673,12 +678,14 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
>> ? ? ? ? ? ? ? ? ? ? ? if (!parity)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? value |= B43_DMA64_TXPARITYDISABLE;
>> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXCTL, value);
>> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGLO,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (ringbase & 0xFFFFFFFF));
>> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGHI,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((ringbase >> 32) &
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?~SSB_DMA_TRANSLATION_MASK)
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | trans);
>> +
>> + ? ? ? ? ? ? ? ? ? ? addrs[0] = ringbase & 0xFFFFFFFF;
>> + ? ? ? ? ? ? ? ? ? ? addrs[1] = ringbase >> 32;
>> + ? ? ? ? ? ? ? ? ? ? tmp = ring->dev->dma.translation_in_low ? 0 : 1;
>> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
>> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] |= trans;
>> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGLO, addrs[0]);
>> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGHI, addrs[1]);
>> ? ? ? ? ? ? ? } else {
>> ? ? ? ? ? ? ? ? ? ? ? u32 ringbase = (u32) (ring->dmabase);
>>
>> @@ -710,12 +717,15 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
>> ? ? ? ? ? ? ? ? ? ? ? if (!parity)
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? value |= B43_DMA64_RXPARITYDISABLE;
>> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXCTL, value);
>> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGLO,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (ringbase & 0xFFFFFFFF));
>> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGHI,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((ringbase >> 32) &
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?~SSB_DMA_TRANSLATION_MASK)
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | trans);
>> +
>> + ? ? ? ? ? ? ? ? ? ? addrs[0] = ringbase & 0xFFFFFFFF;
>> + ? ? ? ? ? ? ? ? ? ? addrs[1] = ringbase >> 32;
>> + ? ? ? ? ? ? ? ? ? ? tmp = ring->dev->dma.translation_in_low ? 0 : 1;
>> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
>> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] |= trans;
>> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGLO, addrs[0]);
>> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGHI, addrs[1]);
>> +
>> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXINDEX, ring->nr_slots *
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct b43_dmadesc64));
>> ? ? ? ? ? ? ? } else {
>> @@ -1052,6 +1062,21 @@ static int b43_dma_set_mask(struct b43_wldev *dev, u64 mask)
>> ? ? ? return 0;
>> ?}
>
>
> This doesn't look correct to me for several reasons:
>
> In the fill-op the address is not masked correctly with the translation mask.
> In both the fill-op and both ring setups, the actual address extension bits
> are always taken from the address's high word. I guess the extension should
> be the low word bits for devices where we use the low word. That's the only
> thing that would make sense. But hey, it's not that we have sane hardware here.
> So this has to be checked.

Ouch, yeah, you should be right (according to common sense of design).

Unfortunately, on my machine, kernel provides low addresses for DMA purposes:
0x1f310000
0x1f318000
0x1f31c000

Can I ask/hack kernel to offer b43 addresses starting with 0x4... or
0x8... (or 0xc...)?

-- 
Rafa?

[PATCH 2/2] b43: fix DMA on some bugged hardware

[Michael Büsch]

On Wed, 10 Aug 2011 18:45:57 +0200
Rafa? Mi?ecki <zajec5@gmail.com> wrote:

> W dniu 10 sierpnia 2011 18:33 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
> > On Wed, 10 Aug 2011 18:11:28 +0200
> >> diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
> >> index 0953ce1..9a2b678 100644
> >> --- a/drivers/net/wireless/b43/dma.c
> >> +++ b/drivers/net/wireless/b43/dma.c
> >> @@ -174,7 +174,10 @@ static void op64_fill_descriptor(struct b43_dmaring *ring,
> >> ? ? ? addrhi = (((u64) dmaaddr >> 32) & ~SSB_DMA_TRANSLATION_MASK);
> >> ? ? ? addrext = (((u64) dmaaddr >> 32) & SSB_DMA_TRANSLATION_MASK)
> >> ? ? ? ? ? >> SSB_DMA_TRANSLATION_SHIFT;
> >> - ? ? addrhi |= ring->dev->dma.translation;
> >> + ? ? if (ring->dev->dma.translation_in_low)
> >> + ? ? ? ? ? ? addrlo |= ring->dev->dma.translation;
> >> + ? ? else
> >> + ? ? ? ? ? ? addrhi |= ring->dev->dma.translation;
> >> ? ? ? if (slot == ring->nr_slots - 1)
> >> ? ? ? ? ? ? ? ctl0 |= B43_DMA64_DCTL0_DTABLEEND;
> >> ? ? ? if (start)
> >> @@ -656,10 +659,12 @@ static int alloc_initial_descbuffers(struct b43_dmaring *ring)
> >> ?static int dmacontroller_setup(struct b43_dmaring *ring)
> >> ?{
> >> ? ? ? int err = 0;
> >> + ? ? int tmp;
> >> ? ? ? u32 value;
> >> ? ? ? u32 addrext;
> >> ? ? ? u32 trans = ring->dev->dma.translation;
> >> ? ? ? bool parity = ring->dev->dma.parity;
> >> + ? ? u32 addrs[2];
> >>
> >> ? ? ? if (ring->tx) {
> >> ? ? ? ? ? ? ? if (ring->type == B43_DMA_64BIT) {
> >> @@ -673,12 +678,14 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
> >> ? ? ? ? ? ? ? ? ? ? ? if (!parity)
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? value |= B43_DMA64_TXPARITYDISABLE;
> >> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXCTL, value);
> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGLO,
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (ringbase & 0xFFFFFFFF));
> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGHI,
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((ringbase >> 32) &
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?~SSB_DMA_TRANSLATION_MASK)
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | trans);
> >> +
> >> + ? ? ? ? ? ? ? ? ? ? addrs[0] = ringbase & 0xFFFFFFFF;
> >> + ? ? ? ? ? ? ? ? ? ? addrs[1] = ringbase >> 32;
> >> + ? ? ? ? ? ? ? ? ? ? tmp = ring->dev->dma.translation_in_low ? 0 : 1;
> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] |= trans;
> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGLO, addrs[0]);
> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGHI, addrs[1]);
> >> ? ? ? ? ? ? ? } else {
> >> ? ? ? ? ? ? ? ? ? ? ? u32 ringbase = (u32) (ring->dmabase);
> >>
> >> @@ -710,12 +717,15 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
> >> ? ? ? ? ? ? ? ? ? ? ? if (!parity)
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? value |= B43_DMA64_RXPARITYDISABLE;
> >> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXCTL, value);
> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGLO,
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (ringbase & 0xFFFFFFFF));
> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGHI,
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((ringbase >> 32) &
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?~SSB_DMA_TRANSLATION_MASK)
> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | trans);
> >> +
> >> + ? ? ? ? ? ? ? ? ? ? addrs[0] = ringbase & 0xFFFFFFFF;
> >> + ? ? ? ? ? ? ? ? ? ? addrs[1] = ringbase >> 32;
> >> + ? ? ? ? ? ? ? ? ? ? tmp = ring->dev->dma.translation_in_low ? 0 : 1;
> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] |= trans;
> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGLO, addrs[0]);
> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGHI, addrs[1]);
> >> +
> >> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXINDEX, ring->nr_slots *
> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct b43_dmadesc64));
> >> ? ? ? ? ? ? ? } else {
> >> @@ -1052,6 +1062,21 @@ static int b43_dma_set_mask(struct b43_wldev *dev, u64 mask)
> >> ? ? ? return 0;
> >> ?}
> >
> >
> > This doesn't look correct to me for several reasons:
> >
> > In the fill-op the address is not masked correctly with the translation mask.
> > In both the fill-op and both ring setups, the actual address extension bits
> > are always taken from the address's high word. I guess the extension should
> > be the low word bits for devices where we use the low word. That's the only
> > thing that would make sense. But hey, it's not that we have sane hardware here.
> > So this has to be checked.
> 
> Ouch, yeah, you should be right (according to common sense of design).
> 
> Unfortunately, on my machine, kernel provides low addresses for DMA purposes:
> 0x1f310000
> 0x1f318000
> 0x1f31c000
> 
> Can I ask/hack kernel to offer b43 addresses starting with 0x4... or
> 0x8... (or 0xc...)?

I'm not sure I understand..
Are you actually saying that those shiny new devices are total crap
in that their 64bit DMA engine can only do real-life-30bit, because they
completely fucked up the extension bits?
Did you try to fix the extension according to my suggestion? Does that
break stuff?

-- 
Greetings, Michael.

[PATCH 2/2] b43: fix DMA on some bugged hardware

[Rafał Miłecki]

W dniu 10 sierpnia 2011 18:55 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
> On Wed, 10 Aug 2011 18:45:57 +0200
> Rafa? Mi?ecki <zajec5@gmail.com> wrote:
>
>> W dniu 10 sierpnia 2011 18:33 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
>> > On Wed, 10 Aug 2011 18:11:28 +0200
>> >> diff --git a/drivers/net/wireless/b43/dma.c b/drivers/net/wireless/b43/dma.c
>> >> index 0953ce1..9a2b678 100644
>> >> --- a/drivers/net/wireless/b43/dma.c
>> >> +++ b/drivers/net/wireless/b43/dma.c
>> >> @@ -174,7 +174,10 @@ static void op64_fill_descriptor(struct b43_dmaring *ring,
>> >> ? ? ? addrhi = (((u64) dmaaddr >> 32) & ~SSB_DMA_TRANSLATION_MASK);
>> >> ? ? ? addrext = (((u64) dmaaddr >> 32) & SSB_DMA_TRANSLATION_MASK)
>> >> ? ? ? ? ? >> SSB_DMA_TRANSLATION_SHIFT;
>> >> - ? ? addrhi |= ring->dev->dma.translation;
>> >> + ? ? if (ring->dev->dma.translation_in_low)
>> >> + ? ? ? ? ? ? addrlo |= ring->dev->dma.translation;
>> >> + ? ? else
>> >> + ? ? ? ? ? ? addrhi |= ring->dev->dma.translation;
>> >> ? ? ? if (slot == ring->nr_slots - 1)
>> >> ? ? ? ? ? ? ? ctl0 |= B43_DMA64_DCTL0_DTABLEEND;
>> >> ? ? ? if (start)
>> >> @@ -656,10 +659,12 @@ static int alloc_initial_descbuffers(struct b43_dmaring *ring)
>> >> ?static int dmacontroller_setup(struct b43_dmaring *ring)
>> >> ?{
>> >> ? ? ? int err = 0;
>> >> + ? ? int tmp;
>> >> ? ? ? u32 value;
>> >> ? ? ? u32 addrext;
>> >> ? ? ? u32 trans = ring->dev->dma.translation;
>> >> ? ? ? bool parity = ring->dev->dma.parity;
>> >> + ? ? u32 addrs[2];
>> >>
>> >> ? ? ? if (ring->tx) {
>> >> ? ? ? ? ? ? ? if (ring->type == B43_DMA_64BIT) {
>> >> @@ -673,12 +678,14 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
>> >> ? ? ? ? ? ? ? ? ? ? ? if (!parity)
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? value |= B43_DMA64_TXPARITYDISABLE;
>> >> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXCTL, value);
>> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGLO,
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (ringbase & 0xFFFFFFFF));
>> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGHI,
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((ringbase >> 32) &
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?~SSB_DMA_TRANSLATION_MASK)
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | trans);
>> >> +
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[0] = ringbase & 0xFFFFFFFF;
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[1] = ringbase >> 32;
>> >> + ? ? ? ? ? ? ? ? ? ? tmp = ring->dev->dma.translation_in_low ? 0 : 1;
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] |= trans;
>> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGLO, addrs[0]);
>> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_TXRINGHI, addrs[1]);
>> >> ? ? ? ? ? ? ? } else {
>> >> ? ? ? ? ? ? ? ? ? ? ? u32 ringbase = (u32) (ring->dmabase);
>> >>
>> >> @@ -710,12 +717,15 @@ static int dmacontroller_setup(struct b43_dmaring *ring)
>> >> ? ? ? ? ? ? ? ? ? ? ? if (!parity)
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? value |= B43_DMA64_RXPARITYDISABLE;
>> >> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXCTL, value);
>> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGLO,
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (ringbase & 0xFFFFFFFF));
>> >> - ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGHI,
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((ringbase >> 32) &
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?~SSB_DMA_TRANSLATION_MASK)
>> >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | trans);
>> >> +
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[0] = ringbase & 0xFFFFFFFF;
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[1] = ringbase >> 32;
>> >> + ? ? ? ? ? ? ? ? ? ? tmp = ring->dev->dma.translation_in_low ? 0 : 1;
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] &= ~SSB_DMA_TRANSLATION_MASK;
>> >> + ? ? ? ? ? ? ? ? ? ? addrs[tmp] |= trans;
>> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGLO, addrs[0]);
>> >> + ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXRINGHI, addrs[1]);
>> >> +
>> >> ? ? ? ? ? ? ? ? ? ? ? b43_dma_write(ring, B43_DMA64_RXINDEX, ring->nr_slots *
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct b43_dmadesc64));
>> >> ? ? ? ? ? ? ? } else {
>> >> @@ -1052,6 +1062,21 @@ static int b43_dma_set_mask(struct b43_wldev *dev, u64 mask)
>> >> ? ? ? return 0;
>> >> ?}
>> >
>> >
>> > This doesn't look correct to me for several reasons:
>> >
>> > In the fill-op the address is not masked correctly with the translation mask.
>> > In both the fill-op and both ring setups, the actual address extension bits
>> > are always taken from the address's high word. I guess the extension should
>> > be the low word bits for devices where we use the low word. That's the only
>> > thing that would make sense. But hey, it's not that we have sane hardware here.
>> > So this has to be checked.
>>
>> Ouch, yeah, you should be right (according to common sense of design).
>>
>> Unfortunately, on my machine, kernel provides low addresses for DMA purposes:
>> 0x1f310000
>> 0x1f318000
>> 0x1f31c000
>>
>> Can I ask/hack kernel to offer b43 addresses starting with 0x4... or
>> 0x8... (or 0xc...)?
>
> I'm not sure I understand..
> Are you actually saying that those shiny new devices are total crap
> in that their 64bit DMA engine can only do real-life-30bit, because they
> completely fucked up the extension bits?
> Did you try to fix the extension according to my suggestion? Does that
> break stuff?

Sorry, I was thinking about some additional tests without really
sharing my thoughts.

kernel offers me addresses like:
0x000000001f310000
0x000000001f318000
0x000000001f31c000

However I wanted to hack kernel to receive something conflicting with:
#define SSB_DMA_TRANSLATION_MASK	0xC0000000

For example I wanted to receive from kernel address like:
0x000000005f318000
or
0x000000009f318000


However that are just some additional tests. It sounds pretty obvious
that we should take addrext from the "correct" part of ringbase (AKA
ring->dmabase).

-- 
Rafa?

[PATCH 2/2] b43: fix DMA on some bugged hardware

[Michael Büsch]

> kernel offers me addresses like:
> 0x000000001f310000

Yeah and that's why you didn't trigger the bug in the patch.
Extension is zero for both cases (low and high).

-- 
Greetings, Michael.

[PATCH 1/2] ssb: fix DMA translation for some specific boards

[Johannes Berg]

On Wed, 2011-08-10 at 18:44 +0200, Michael B?sch wrote:

> > >> +     if (dev->id.coreid == SSB_DEV_80211)
> > >> +             return (chip_id == 0x4322 || chip_id == 43221 ||
> > >> +                     chip_id == 43231 || chip_id == 43222);
> > >> +     return 0;
> > >> +}
> > >
> > > ACK, apart from this coding style violation (missing curly braces).
> > 
> > From kernel CodingStyle:
> > > Do not unnecessarily use braces where a single statement will do.
> > >
> > > if (condition)
> > > 	action();
> > 
> > I guess simple "return" (even when using line break) is still like
> > single statement...
> > Am I wrong?
> > 
> 
> Last time I checked this only applied to single-line-statements.
> Maybe it changed, though.

Just sidestep the issue and invert the test ;-)
(However, CodingStyle says nothing about single lines, only statements,
and I tend to agree)

johannes