archive mirror
 help / color / mirror / Atom feed
From: Ian Molton <>
To: Arend van Spriel <>,
	"" <>
Cc: Franky Lin <>,,
	Hante Meuleman <>
Subject: Re: brcmfmac bus level addressing issues.
Date: Tue, 18 Jul 2017 23:45:08 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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.



> Regards,
> Arend
>>> -Ian
>>>> Regards, Hante
>>>> -----Original Message-----
>>>> From: Ian Molton []
>>>> Sent: Tuesday, July 18, 2017 1:28 PM
>>>> To: Hante Meuleman;
>>>> 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.

  reply	other threads:[~2017-07-18 22:45 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-18  9:45 RFC: brcmfmac bus level addressing issues Ian Molton
2017-07-18 10:35 ` Hante Meuleman
2017-07-18 11:27   ` Ian Molton
     [not found]     ` <>
     [not found]       ` <>
2017-07-18 15:14         ` Ian Molton
2017-07-18 20:44           ` Arend van Spriel
2017-07-18 22:45             ` Ian Molton [this message]
2017-07-19  8:39               ` Hante Meuleman
2017-07-19  9:33                 ` Ian Molton
2017-07-19 11:47               ` Arend van Spriel
2017-07-19 19:25                 ` Ian Molton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).