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@xxxxxxxxxxxxxx] >>>> Sent: Tuesday, July 18, 2017 1:28 PM >>>> To: Hante Meuleman; linux-wireless@xxxxxxxxxxxxxxx >>>> 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. >>>> >>> >>
