Re: [PATCH v6 00/12] platform/x86: introduce p2sb_bar() helper

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Wed, 13 Jul 2022, Henning Schild wrote:

> Am Wed, 13 Jul 2022 19:40:23 +0300
> schrieb Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:
> 
> > On Wed, Jun 29, 2022 at 07:14:06PM +0200, Henning Schild wrote:
> > > Am Wed, 29 Jun 2022 19:39:58 +0300
> > > schrieb Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>:
> > >   
> > > > +Cc: Rafael
> > > > 
> > > > On Tue, Jun 21, 2022 at 02:58:16PM +0300, Andy Shevchenko wrote:  
> > > > > On Wed, Jun 08, 2022 at 12:50:44PM +0200, Andy Shevchenko
> > > > > wrote:    
> > > > > > On Wed, Jun 8, 2022 at 9:42 AM Lee Jones
> > > > > > <lee.jones@xxxxxxxxxx> wrote:    
> > > > > > > On Mon, 06 Jun 2022, Andy Shevchenko wrote:
> > > > > > >    
> > > > > > > > There are a few users that would like to utilize P2SB
> > > > > > > > mechanism of hiding and unhiding a device from the PCI
> > > > > > > > configuration space.
> > > > > > > >
> > > > > > > > Here is the series to consolidate p2sb handling code for
> > > > > > > > existing users and to provide a generic way for new
> > > > > > > > comer(s).
> > > > > > > >
> > > > > > > > It also includes a patch to enable GPIO controllers on
> > > > > > > > Apollo Lake when it's used with ABL bootloader w/o ACPI
> > > > > > > > support.
> > > > > > > >
> > > > > > > > The patch that brings the helper ("platform/x86/intel: Add
> > > > > > > > Primary to Sideband (P2SB) bridge support") has a commit
> > > > > > > > message that sheds a light on what the P2SB is and why
> > > > > > > > this is needed.
> > > > > > > >
> > > > > > > > I have tested this on Apollo Lake platform (I'm able to
> > > > > > > > see SPI NOR and since we have an ACPI device for GPIO I
> > > > > > > > do not see any attempts to recreate one).
> > > > > > > >
> > > > > > > > The series is ready to be merged via MFD tree, but see
> > > > > > > > below.
> > > > > > > >
> > > > > > > > The series also includes updates for Simatic IPC drivers
> > > > > > > > that partially tagged by respective maintainers (the main
> > > > > > > > question is if Pavel is okay with the last three patches,
> > > > > > > > since I believe Hans is okay with removing some code
> > > > > > > > under PDx86). Hence the first 8 patches can be merged
> > > > > > > > right away and the rest when Pavel does his review.    
> > > > > > >
> > > > > > > Can we just wait for Pavel's review, then merge them all at
> > > > > > > once?    
> > > > > > 
> > > > > > Sure, it would be the best course of action.    
> > > > > 
> > > > > Pavel, do you have a chance to review the patches (last three)
> > > > > that touch LED drivers? What would be your verdict?    
> > > > 
> > > > Lee, Rafael,
> > > > 
> > > > It seems quite hard to get Pavel's attention to this series [1].
> > > > It's already passed more than 3 weeks for any sign of review of
> > > > three top patches of the series that touched LED subsystem. The
> > > > entire series has all necessary tags, but for LED changes.
> > > > 
> > > > Note, that the top of this series is not done by me and was sent
> > > > for preliminary review much earlier [2], altogether it makes
> > > > months of no response from the maintainer.
> > > > 
> > > > The nature of patches is pretty simple and doesn't touch any of
> > > > other than Simatic LED drivers nor LED core. Moreover, it was
> > > > written by Siemens, who produces the H/W in question and very
> > > > well tested as a separate change and as part of the series.  
> > > 
> > > The code has been reviewed and is in fact pretty simple. The only
> > > questionable but pragmatic change that might catch the attention of
> > > a pedantic reviewer is that i did put the gpio implementation of the
> > > driver under the same/existing kernel config switch.
> > >   
> > > > I think to move forward we may ask Rafael to review it on behalf
> > > > of good maintainer and with his approval apply entire series.
> > > > 
> > > > Thoughts?  
> > > 
> > > Thanks for pushing this Andy. I was wondering how and when that
> > > story would continue. Technically these changes should really go in
> > > one badge or we need to find a way to separate them somehow. I
> > > would try to go that extra mile to get out of your way. But i am
> > > kind of afraid such an effort might also end up touching the same
> > > files and block us at the same maintainer.
> > > 
> > > Did anyone check whether Pavel was active at all in those last
> > > months and maybe other patches waiting for review? Hope he is fine
> > > and active and just somehow forgot/overlooked/ignored this one.  
> > 
> > I have send a private mail to Pavel and have got no response.
> > Can we move this forward, let's say, by applying first 8 patches?
> 
> I am sorry that situation is now coming. Both simatic-ipc and that
> appollo lake pinctrl driver compete for the same device memory. That
> conflict was known and we agreed on sorting it out together somehow.
> Not applying my patches could leave my LED drivers simply not working
> any longer, or worse ... them making the apollolake platform stuff act
> up somehow weird with unexpected EBUSY.
> 
> The series can not be split, or we have to write additional code to
> properly deal with the conflict. I could envision my LED drivers still
> accessing raw memory and ignoring EBUSY (very hacky! ... and touching
> "we need Pavel code")
> 
> Another way could maybe be. Do the whole P2SB but do not make
> apollolake pinctrl come up without ACPI. Somewhere in patches 1-8 there
> is code which makes the pinctrl stuff come up for certain CPUs without
> ACPI. It is really only some out of many CPUs which have pinctrl, and i
> am not sure i remember what that has to do with the P2SB helpers as
> such. The helpers are a refactoring, while the "bring up apollolake
> pinctrl at all times" is a functional change ... now causing conflict.
> 
> And maybe there is a way/process to escalate to another maintainer.
> Does anyone even know what is going on with Pavel? 

I'll take the hit.  He had his chance.

I'm happy to move forward with Andy's review.

(Side note: Seeing as Pavel hasn't been seen for 2 months, I'll also
 follow-up on  the LED ML to offer to become temporary maintainer for a
 bit)

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog