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

[Darren Hart <dvhart@infradead.org>]

Qipeng, can you comment on my understanding of the DSDT and the driver?

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