Re: brcmfmac bus level addressing issues.

[Ian Molton <ian@mnementh.co.uk>]

On 18/07/17 21:44, Arend van Spriel wrote:
> 
> 
>>> Stop. Listen. Fix it.
> 
> Hi Ian,
> 
> Now stop yourself and listen. This is no collaboration in any sense.

Arend - This is not directed at you - you've been polite, and I've no
issue with you.

I had felt that the initial conflict over this was over, but then Hante
wrote his last post. I may have been a bit blunt about the code
initially, but I made *no personal attacks* up until Hante's post (I've
just read *all* my posts on this thread and checked to be sure). I hop
you can understand that his comments earlier were well out of line. I've
actually chosen to take the day off today, before I replied to him with
something I'd really regret posting. I was furious.

> but you seem to easily disregard us.

Actually, no. You yourself have been helpful and responsive.

> Also you said it yourself all your patches are
> cleanup. What needs fixing is the gscan feature detection and that
> one is on me.

Fair enough - I guess now that its in the kernel it needs fixing before
4.13 - but I would argue that no new features should go into the driver
for the short term once that is done. Lets give it a good spring clean.

I am, actually, able to put some time into this, and *want* to help - MY
current employer would benefit greatly from this driver becoming stable.

(as it stands, back to back wpasuplpicant runs make it keel over, as
does module unload and reload).

> So I am chasing that although I am actually on vacation.

Dude - enjoy your vacation. Seriously.

> As long as my wife does not notice we are ok :-p

Shhh :)

>>> You could at the very least give some feedback on the 29 patches I sent
>>> cleaning it up.
> 
> That really has to wait until I return as well as running some tests on
> older 4329 and some newer chipsets.

Fair. When do you return? No pressure - it just means I know I can hold
off and do something else on my project until then.

> Actually, Hante is not working full-time on this driver. Neither am I.

Fair enough. He did wax lyrical about his "seniority" on the driver
though. Pulling rank does not sit well with me.

> Now please stop the insults when you hit some push back or in generalf
> for that matter.

I'd recommend re-reading these threads. Whilst I'd grant my initial
comments in the first cut of the patchset were a little harsh, they were
NOT personally directed.

Hante crossed that line, but I've said my piece on the matter now. I
have nothing further to say to Hante on the matter and consider it over,
if he will do the same.

> It is awfully counterproductive for both you and others.

It certainly is - it cost me a days work, as I couldnt think straight
after this morning.

So - Lets let this be the end of it.

I propose that in the near future we sit down and plan some cleanup work
for this driver (*after* your vacation). Get it stable, make it a
shining example of how to do WiFi.

Deal?

-Ian

> 
> Regards,
> Arend
> 
>>> -Ian
>>>
>>>>
>>>> Regards, Hante
>>>>
>>>> -----Original Message-----
>>>> From: Ian Molton [mailto:ian@mnementh.co.uk]
>>>> Sent: Tuesday, July 18, 2017 1:28 PM
>>>> To: Hante Meuleman; linux-wireless@vger.kernel.org
>>>> Cc: Arend Van Spriel; Franky Lin
>>>> Subject: Re: brcmfmac bus level addressing issues.
>>>>
>>>> On 18/07/17 11:35, Hante Meuleman wrote:
>>>>> Hi Ian,
>>>>>
>>>>> I've really no idea what you mean.
>>>>
>>>> You should look at the code...
>>>>
>>>>> brcmf_pcie_select_core is redundant?
>>>>
>>>> Essentially yes - there may be a couple of corner cases where the IO
>>>> accesses are not performed via brcmf_pcie_buscore_{read,write}32() - but
>>>> other than that, brcmf_pcie_buscore_prep_addr() sets the IO window
>>>> unconditionally on every access.
>>>>
>>>>> Care to try to boot a device without this function?
>>>>
>>>> I strongly suspect it would work. Perhaps try it? Give me a device and
>>>> I'll try it.
>>>>
>>>>> Called all over the  place? Hell no, it is default pointing to PCIE2
>>>>> and functions which need to map the window to another core will do so,
>>>>> temporarily, but move it back to PCIE2, at least that is the idea, may
>>>>> be you found a bug?
>>>>
>>>> brcmf_pcie_select_core() looks up the core structure from the core id.
>>>>
>>>> it then sets BRCMF_PCIE_BAR0_WINDOW according to the core base address.
>>>>
>>>> it actually goes to the length of reading it back and trying again if its
>>>> not set, even, which is at least a little bit horrifying.
>>>>
>>>> ------------
>>>>
>>>> brcmf_pcie_buscore_{read,write}32() both call
>>>> brcmf_pcie_buscore_prep_addr()
>>>>
>>>> brcmf_pcie_buscore_prep_addr() *unconditionally* programs
>>>> BRCMF_PCIE_BAR0_WINDOW on *every single* IO access.
>>>>
>>>> If you want inefficient - its right there.
>>>>
>>>>
>>>> The SDIO version of the code is actually considerably more efficient on
>>>> this point - it at least only programs the window register only when it
>>>> changes, not on every single IO access.
>>>>
>>>>> We are
>>>>> for sure not going to hide the selecting of the window in the
>>>>> read/write routines, that would result in a giant amount of overhead.
>>>>
>>>> Actually it would result in *considerably* less overhead than the current
>>>> code, that blindly sets the window on every access.
>>>>
>>>>> Currently PCIE
>>>>> devices reach 1.5Gpbs, we need to go faster than that in the near
>>>> future.
>>>>
>>>> I dont need a lesson on writing efficient code, thanks.
>>>>
>>>>> We don't want just change that to make it bit nicer..... Why do you
>>>>> need to see the same in the SDIO and PCIE drivers? SDIO and PCIE
>>>>> differ in many aspects. Sure some things can be improved in or the
>>>>> other, but they sure don't need to look alike.
>>>>
>>>> I dont "need" to see the same in both drivers. Not where it isnt
>>>> appropriate.
>>>>
>>>> but every part of the drivers that can share code without noticeably
>>>> impacting performance *should* do so. You should be justifying to me why
>>>> the code has to be different, not the other way around. Are you sreiously
>>>> arguing that sharing common code is a bad idea?
>>>>
>>>>> It may be ugly, but thusfar it has not caused bugs
>>>>
>>>> Oh, I bet you it has... try reading the SDIO version (note the reliance on
>>>> the dangling ->sbwad pointer) and tell me again that this hasnt caused
>>>> bugs.
>>>>
>>>> Right now, the bulk of the driver code is sat on top of at least two bus
>>>> drivers with differing IO models, and is working via good luck alone.
>>>>
>>>>> The concept in pcie bus part is simple.
>>>>
>>>> And differs completely from the SDIO part.
>>>>
>>>>> The main core to select is PCIE2 (once you have booted and established
>>>>> initial communication with firmware) and every routine which needs to
>>>>> access another core will change the window temporarily and set it back
>>>>> once done. Please don't mess with this, it works, it is clear and it
>>>>> is fast.
>>>>
>>>> If is anything but fast. changing the window involves traversiong the list
>>>> of cores. Every. Single. Time. It doesnt *have* to - but thats what
>>>> brcmf_chip_get_core() does, and brcmf_pcie_select_core() calls it.
>>>>
>>>
>>