Re: [PATCH v7] platform:x86: add Intel P-Unit mailbox IPC driver

[Darren Hart <dvhart@infradead.org>]

On Thu, Oct 15, 2015 at 10:39:35AM +0000, Shevchenko, Andriy wrote:
> On Thu, 2015-10-15 at 13:35 +0300, Andy Shevchenko wrote:
> > On Sat, 2015-10-10 at 03:07 +0000, Zha, Qipeng wrote:
> > > > 
> > > > Everything is quite okay, except this BAR thingy.
> > > 
> > > > Can you provide a DSDT excerpt for the device to see what is 
> > > > there?
> > > 
> > > > I can't find such device (by ACPI id) in the tables of the 
> > > > accessible hardware in our lab.
> > > 
> > > Please check below acpi device definition from BIOS.
> > > Punit device is created in pmc driver, since BIOS finally reject to 
> > > 
> > > create a separate device for Punit.
> > 
> > Thank you for mention this one. It's unfortunately a show stopper for
> > using this module as a driver (you can't assign two drivers to the 
> > same
> > device). You have to convert is to a library.
> 
> Oh, I'm sorry, I really missed that the IDs are different.
> So, discard this part.
> 
> > 
> > Moreover, I briefly looked at the intel_pmc_ipc and it should be
> > refactored a in a few ways: a) split to core part, PCI driver, and 
> > ACPI
> > driver, b) improved regarding to comments you got in this review 
> > (many
> > comments are applied to what we have there).
> > 
> > Darren, your opinion?
> 
> This by the way still valid.

I'm a bit confused. Are you just bringing up that the intel_pmc_ipc driver could
also be improved? Or are you sugesting that this driver needs to be refactored
together with the intel_pmc_ipc driver?

With respect to the intel_punit_ipc driver, I've tried to parse/map the DSDT to
the BAR mapping in the driver which you raised concerns about. Can you review
that bit and help us come to a conclusion on that since you asked for the DSDT?
(See separate response to Qipeng on that).

Thanks,

> 
> > 
> > > 
> > >   Scope (\_SB) {
> > >     Device(IPC1)
> > >     {
> > >       …
> > >       Name (RBUF, ResourceTemplate ()
> > >       {
> > >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0)   
> > >  // 
> > > IPC1 Bar
> > >      //   Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1) 
> > >  // 
> > > SSRAM
> > >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT)   
> > >  // 
> > > PUnit BIOS mailbox Data
> > >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF)   
> > >  // 
> > > PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> > >         IO (Decode16, 0x400, 0x480, 0x4, 0x80)                    
> > >  // 
> > > ACPI IO Base address
> > >         Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , 
> > > , 
> > > ) {40}  // IPC1 IRQ  
> > >       })
> > > 
> > >       …
> > >     }
> > >   }//end scope
> > 
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@intel.com>
> Intel Finland Oy
> ---------------------------------------------------------------------
> Intel Finland Oy
> Registered Address: PL 281, 00181 Helsinki 
> Business Identity Code: 0357606 - 4 
> Domiciled in Helsinki 
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

-- 
Darren Hart
Intel Open Source Technology Center