2011-02-15 20:42:25

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

Please don't prune CCs.

On 02/15/2011 09:12 PM, Nikolay Ledovskikh wrote:
> Dear Jiri,
>
> Should I resubmit the patch with another changelog?

Yes, definitely.

> 1. mem = res->start;
> That is what madwifi driver does
>
> dev->mem_start = KSEG1ADDR(res->start);
> dev->mem_end = KSEG1ADDR(res->end);
> sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;

That's something completely different to what you did. This assignment
makes sense to me. But still prefer ioremap. Doesn't it work for you?

I'm no mips guy, sorry.

> 2.
> I set sc->dev to NULL and setup save pointer to real device
> structure using SET_IEEE80211_DEV.
> Now it's saved in wiphy structure and then used to get
> platform_device pointer.
> I set sc->dev to NULL according to madwifi sources which look following
> sc->aps_sc.sc_bdev = NULL; (if_ath_ahb.c)
> and use this value in dma mapping functions.

Don't get inspired by madwifi sources, it's crap. The question is if you
have to do that to avoid the oops.

> Looks like addresses mapped already and we shouldn't do ioremap once
> more?!

Maybe the address you got from the platform side was already ored by
KSEG1...

regards,
--
js


2011-02-15 21:39:24

by Nickolay Ledovskikh

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

> Maybe the address you got from the platform side was already ored by
> KSEG1...

I took a look at openwrt atheros platform code and suppose you are right.
So what we should do for now? Add pointer cast (void __iomem *)?
Because ioremap_nocache doesn't work as expected. I think it's better
to rewrote the openwrt
code, but not now.

2011-02-15 21:09:03

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

On 02/15/2011 09:57 PM, Nikolay Ledovskikh wrote:
> Madwifi is a crap, but
> #define KSEG1ADDR(addr) (addr)
> and only difference is in pointer type cast
> sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;
> in my case it would be something like this:
> sc->aps_sc.sc_iobase = dev->mem_start;
> and it works.

In that case yes, but KSEG1ADDR should write as:
#define KSEG1ADDR(a) (CPHYSADDR(a) | KSEG1)

As I wrote, this needs some mips expert. But it definitely shouldn't be
a plain assignment.

regards,
--
js

2011-02-15 20:57:51

by Nickolay Ledovskikh

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

Madwifi is a crap, but
#define KSEG1ADDR(addr) (addr)
and only difference is in pointer type cast
sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;
in my case it would be something like this:
sc->aps_sc.sc_iobase = dev->mem_start;
and it works.

2011/2/15 Jiri Slaby <[email protected]>:
> Please don't prune CCs.
>
> On 02/15/2011 09:12 PM, Nikolay Ledovskikh wrote:
>> Dear Jiri,
>>
>> Should I resubmit the patch with another changelog?
>
> Yes, definitely.
>
>> 1. mem = res->start;
>>     That is what madwifi driver does
>>
>>     dev->mem_start = KSEG1ADDR(res->start);
>>     dev->mem_end = KSEG1ADDR(res->end);
>>     sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;
>
> That's something completely different to what you did. This assignment
> makes sense to me. But still prefer ioremap. Doesn't it work for you?
>
> I'm no mips guy, sorry.
>
>> 2.
>>     I set sc->dev to NULL and setup save pointer to real device
>> structure using SET_IEEE80211_DEV.
>>     Now it's saved in wiphy structure and then used to get
>> platform_device pointer.
>>     I set sc->dev to NULL according to madwifi sources which look following
>>         sc->aps_sc.sc_bdev = NULL; (if_ath_ahb.c)
>>     and use this value in dma mapping functions.
>
> Don't get inspired by madwifi sources, it's crap. The question is if you
> have to do that to avoid the oops.
>
>>     Looks like addresses mapped already and we shouldn't do ioremap once
>> more?!
>
> Maybe the address you got from the platform side was already ored by
> KSEG1...
>
> regards,
> --
> js
>



--
Best regards, Nikolay Ledovskikh.

2011-02-15 21:18:55

by Nickolay Ledovskikh

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

Thanks, I take it into account.
So we need to investigate the platform code?

2011-02-15 22:16:33

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

On 02/15/2011 10:39 PM, Nikolay Ledovskikh wrote:
>> Maybe the address you got from the platform side was already ored by
>> KSEG1...
>
> I took a look at openwrt atheros platform code and suppose you are right.
> So what we should do for now? Add pointer cast (void __iomem *)?
> Because ioremap_nocache doesn't work as expected. I think it's better
> to rewrote the openwrt
> code, but not now.

So I've found:
http://www.google.com/codesearch/p?hl=en#sayuPQDVf4c/trunk/openwrt/target/linux/atheros/patches-2.6.32/100-board.patch&q=ar231x-wmac&sa=N&cd=4&ct=rc

There, the res->start may be either of the following:
AR531X_WLAN0 .. 0x18000000
AR531X_WLAN1 .. 0x18500000
AR2315_WLAN0 .. 0xB0000000

I suppose you have the 3rd otherwise it should die without ORing KSEG1?

Or maybe MIPS guys will correct me? (The problem is that ioremap of one
of the addresses above kills the box. If Nikolaj removes the ioremap and
uses the address directly, it works for him. I'm saying it will die for
the first 2 addresses if we remove ioremap completely -- from what I
found in MIPS specs.)

I _think_ there should be (instead of ioremap):
sc->iobase = (void __iomem *)KSEG1ADDR(res->start);

Then we do readl(sc->iobase) et al. in ath5k.

thanks,
--
js

2011-02-15 22:18:56

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

On 02/15/2011 11:16 PM, Jiri Slaby wrote:
> On 02/15/2011 10:39 PM, Nikolay Ledovskikh wrote:
>>> Maybe the address you got from the platform side was already ored by
>>> KSEG1...
>>
>> I took a look at openwrt atheros platform code and suppose you are right.
>> So what we should do for now? Add pointer cast (void __iomem *)?
>> Because ioremap_nocache doesn't work as expected. I think it's better
>> to rewrote the openwrt
>> code, but not now.
>
> So I've found:
> http://www.google.com/codesearch/p?hl=en#sayuPQDVf4c/trunk/openwrt/target/linux/atheros/patches-2.6.32/100-board.patch&q=ar231x-wmac&sa=N&cd=4&ct=rc
>
> There, the res->start may be either of the following:
> AR531X_WLAN0 .. 0x18000000
> AR531X_WLAN1 .. 0x18500000


> AR2315_WLAN0 .. 0xB0000000

Or maybe this should be 0x10000000 in openwrt in the first place? Then
ioremap should do the right thing, right?

> I suppose you have the 3rd otherwise it should die without ORing KSEG1?
>
> Or maybe MIPS guys will correct me? (The problem is that ioremap of one
> of the addresses above kills the box. If Nikolaj removes the ioremap and
> uses the address directly, it works for him. I'm saying it will die for
> the first 2 addresses if we remove ioremap completely -- from what I
> found in MIPS specs.)
>
> I _think_ there should be (instead of ioremap):
> sc->iobase = (void __iomem *)KSEG1ADDR(res->start);
>
> Then we do readl(sc->iobase) et al. in ath5k.
>
> thanks,
--
js