Problem with understanding DMA on some machines (known solution!), specs needed?

[Michael Büsch]

On Tue, 26 Jul 2011 20:55:31 +0200
Michael Büsch <m at bues.ch> wrote:

> On Tue, 26 Jul 2011 18:32:15 +0200
> Rafał Miłecki <zajec5 at gmail.com> wrote:
> 
> > W dniu 26 lipca 2011 17:33 użytkownik Larry Finger
> > <Larry.Finger at lwfinger.net> napisał:
> > > On 07/26/2011 03:24 AM, Rafał Miłecki wrote:
> > >>
> > >> W dniu 25 lipca 2011 23:54 użytkownik Rafał Miłecki<zajec5 at gmail.com>
> > >>  napisał:
> > >>>
> > >>> Now, the question: when for real we should use such a solution?
> > >>>
> > >>> Larry, could you check your driver? Can you see anything about this?
> > >>> Is this maybe PCI (not PCIe!) specific?
> > >>
> > >> I've checked thread "Interesting 14e4:4321". It seems both: 14e4:4321
> > >> and 14e4:4322 are using PCI slot and both are not working in DMA mode.
> > >> I start believing it's PCI specific.
> > >>
> > >> If you take a look at current ssb code and defines:
> > >>>
> > >>> if (ssb_read32(dev, SSB_TMSHIGH)&  SSB_TMSHIGH_DMA64)
> > >>>        return SSB_PCIE_DMA_H32;
> > >>> else
> > >>>        return SSB_PCI_DMA;
> > >>
> > >> You can see 0x80000000 (SSB_PCIE_DMA_H32) has actually "PCIE" in it's
> > >> name. This can be true that 0x80000000 is *only* for *64-bit DMA* on
> > >> *PCIe*.
> > >
> > > That is almost correct. This time I found it. The pseudo code is:
> > >
> > > dma_addr_lo = 0
> > > dma_addr_hi = 0
> > > if PCI || PCIe
> > >        if PCIe && 64-bit DMA
> > >                dma_addr_hi = 0x80000000
> > >        else
> > >                if chipID is 0x4322, 43221, 43231, or 43222
> > >                        dma_addr_lo = 0x80000000
> > >                else
> > >                        dma_addr_lo = 0x40000000    <== your case
> > >
> > > Thus it is just a little more complicated than a PCI/PCIe split, as it also
> > > depends on the chip ID.
> > >
> > > I'll add this to the specs.
> > 
> > Can you (anyone, not just Larry ;) ) give me some tip, how to
> > implement this correctly? From programming POV.
> > 
> > We should return two infos from ssb code now:
> > 1) Routing bit
> > 2) Address which should be used
> > 
> > Should I add new function for this? Or create struct
> > dma_translation_info with 2 fields? Or return array? Or...?
> 
> I would do it something like this (completely untested. Not even compiled):
> 
> 
> Index: wireless-testing/drivers/ssb/main.c
> ===================================================================
> --- wireless-testing.orig/drivers/ssb/main.c	2011-07-21 23:20:56.000000000 +0200
> +++ wireless-testing/drivers/ssb/main.c	2011-07-26 20:50:31.920182657 +0200
> @@ -1260,16 +1260,32 @@ void ssb_device_disable(struct ssb_devic
>  }
>  EXPORT_SYMBOL(ssb_device_disable);
>  
> +static bool let_the_software_guys_fix_it(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 (dev->bus->host_pci->is_pcie &&
> +		    (ssb_read32(dev, SSB_TMSHIGH) & SSB_TMSHIGH_DMA64)) {
>  			return SSB_PCIE_DMA_H32;
> -		else
> -			return SSB_PCI_DMA;
> +		} else {
> +			if (let_the_software_guys_fix_it(dev))
> +				return SSB_PCIE_DMA_H32;
> +			else
> +				return SSB_PCI_DMA;
> +		}
>  	default:
>  		__ssb_dma_not_implemented(dev);
>  	}
> Index: wireless-testing/drivers/net/wireless/b43/dma.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/b43/dma.c	2011-07-21 23:20:55.000000000 +0200
> +++ wireless-testing/drivers/net/wireless/b43/dma.c	2011-07-26 20:53:40.688974405 +0200
> @@ -1052,6 +1052,21 @@ static int b43_dma_set_mask(struct b43_w
>  	return 0;
>  }
>  
> +static bool dma_translation_in_low_word(struct b43_wldev *dev)
> +{
> +#ifdef CONFIG_B43_SSB
> +	if (dev->dev->bus_type != B43_BUS_SSB)
> +		return 0;
> +	if (dev->dev->sdev->bus->bustype == SSB_BUSTYPE_PCI) {
> +		if (dev->dev->sdev->bus->host_pci->is_pcie &&
> +		    (ssb_read32(dev->dev->sdev, SSB_TMSHIGH) & SSB_TMSHIGH_DMA64))
> +			return 0;
> +		return 1;
> +	}
> +#endif
> +	return 0;
> +}
> +
>  int b43_dma_init(struct b43_wldev *dev)
>  {
>  	struct b43_dma *dma = &dev->dma;
> @@ -1074,6 +1089,7 @@ int b43_dma_init(struct b43_wldev *dev)
>  #ifdef CONFIG_B43_SSB
>  	case B43_BUS_SSB:
>  		dma->translation = ssb_dma_translation(dev->dev->sdev);
> +		//Hurr durr use dma_translation_in_low_word()
>  		break;
>  #endif
>  	}
> 

Does that sound sane? Any comments on this?

-- 
Greetings, Michael.