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

[Darren Hart <dvhart@infradead.org>]

On Thu, Oct 22, 2015 at 01:01:32AM +0000, Zha, Qipeng wrote:
> >>Qipeng, can you comment on my understanding of the DSDT and the driver?
> >> >      //   Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1)  // SSRAM
> >> >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT)    // PUnit BIOS mailbox Data
> >> size: 0x1000
> >> And this would be res1 in the driver?
> >> >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF)    // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> 
> When boot up, BIOS will rewrite the size of these entries,  actually the size for this entry will change to 4B, not the default 0x1000.
> This is real strange implementation for us, as before mentioned, BIOS implement like this to make it compatible for wos driver.
> 

+Rafael - we're having some trouble mapping the ACPI declared resources to the
driver code using them. Qipeng has indicated that the BIOS team is doing
something rather bizarre to meet some requirement from the WOS drivers. Can you
have a look at this thread? I haven't been able to map the ACPI resources to the
driver resources in a way that makes sense to me, and this is holding up merging
the driver.

Qipeng, this is very strange indeed. How does BIOS change this in a way that
doesn't change the resources ACPI reports to the OS?

Is the ASL fragment you provided collected from a running Linux system using
acpidump? If so, then the resources ACPI advertises to the OS are not actually
available... and that can't be acceptable.

Qipeng, I want to get this driver merged, but we have to do the due diligence to
understand how these resources are being used. I've asked several questions to
try and clarify this, but your responses have been very short and do not fully
address the questions. I need your help to fully describe what is going on with
the BARs before I can sign this off and ask Linus to merge it.

Thanks,

Darren


> On Wed, Oct 14, 2015 at 10:16:06PM -0700, Darren Hart wrote:
> > On Sat, Oct 10, 2015 at 03:07:53AM +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.
> > > 
> > 
> > Qipeng, sorry for the delay. I've been under a rather absurd load. I'd 
> > like to close on this so we can get this into -next ASAP. Thank you 
> > for posting the DSDT.
> > 
> > Help us parse what this means so we're all seeing the same thing. I 
> > see a total of 3 memory addresses and the following IO base address. 
> > Unfortunately, the comments don't map directly to the driver code, so 
> > I need to piece this together.
> > 
> > The code in question from the driver is:
> > 
> > #define MAILBOX_REGISTER_SPACE 0x10
> > 
> > +static int intel_punit_get_bars(struct platform_device *pdev) {
> > +       struct resource *res0, *res1;
> > +       void __iomem *addr;
> > +       int size;
> > +
> > +       res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!res0) {
> > +               dev_err(&pdev->dev, "Failed to get iomem resource\n");
> > +               return -ENXIO;
> > +       }
> > +       size = resource_size(res0);
> > +       if (!devm_request_mem_region(&pdev->dev, res0->start,
> > +                                    size, pdev->name)) {
> > +               dev_err(&pdev->dev, "Failed to request iomem resouce\n");
> > +               return -EBUSY;
> > +       }
> > +
> > +       res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +       if (!res1) {
> > +               dev_err(&pdev->dev, "Failed to get iomem resource1\n");
> > +               return -ENXIO;
> > +       }
> > +       size = resource_size(res1);
> > +       if (!devm_request_mem_region(&pdev->dev, res1->start,
> > +                                    size, pdev->name)) {
> > +               dev_err(&pdev->dev, "Failed to request iomem resouce1\n");
> > +               return -EBUSY;
> > +       }
> > +
> > +       addr = ioremap_nocache(res0->start,
> > +                              resource_size(res0) + resource_size(res1));
> > +       if (!addr) {
> > +               dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> > +               return  -ENOMEM;
> > +       }
> > +       punit_ipcdev->base[BIOS_IPC] = addr;
> > +       addr += MAILBOX_REGISTER_SPACE;
> > +       punit_ipcdev->base[GTDRIVER_IPC] = addr;
> > +       addr += MAILBOX_REGISTER_SPACE;
> > +       punit_ipcdev->base[ISPDRIVER_IPC] = addr;
> > +
> > +       return 0;
> > +}
> > 
> > 
> > >   Scope (\_SB) {
> > >     Device(IPC1)
> > >     {
> > >       …
> > >       Name (RBUF, ResourceTemplate ()
> > >       {
> > >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR0)    // IPC1 Bar
> > 
> > size: 0x1000
> > IPC1 BAR, I presume this maps to res0 in the driver?
> > 
> > Then the start of this 4096 byte region is assigned by:
> > 
> > punit_ipcdev->base[BIOS_IPC] = addr;
> > 
> > And everything else assigned by intel_punit_get_bards is contained 
> > within this addr since MAILBOX_REGISTER_SPACE is only 0x10.
> > 
> > Do I have that correct?
> > 
> > >      //   Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, BAR1)  // SSRAM
> > >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT)    // PUnit BIOS mailbox Data
> > 
> > size: 0x1000
> > And this would be res1 in the driver?
> > 
> > >         Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF)    // PUnit BIOS mailbox Interface and GTD/ISPD mailbox
> > 
> > size: 0x1000
> > 
> > This seems odd, especially with the comments in the DSD. A more natural mapping would have been each of the Memory32Fixed macros mapping to the BIOS_IPC, GTDRIVER_IPC, and ISPDRIVER_IPC BARs in the driver. This would indeed be the case if MAILBOX_REGISTER_SPACE was 0x1000 instead of 0x10. Also, if that were the case, then those BARs should be set by using the mapped addr for the 3 separate resources - which is what Andy was getting at.
> > 
> > Qipeng, I assume I misinterpretted something above. Can you point out where I've got this wrong and how the driver is doing the only thing it can given the resources provided by the DSDT?
> > 
> > Thanks,
> > 
> > 
> > >         IO (Decode16, 0x400, 0x480, 0x4, 0x80)                     // ACPI IO Base address
> > >         Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, , , ) {40}  // IPC1 IRQ  
> > >       })
> > > 
> > >       …
> > >     }
> > >   }//end scope
> > 
> > --
> > Darren Hart
> > Intel Open Source Technology Center
> 
> --
> Darren Hart
> Intel Open Source Technology Center

-- 
Darren Hart
Intel Open Source Technology Center