Darren, Andriy:
Thanks for your kind review, try to make clear as below.
+Gavin, Our BIOS developer.
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.
>Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MDAT) // PUnit BIOS
>mailbox Data
This is map to res0
> Memory32Fixed (ReadWrite, 0x00000000, 0x00001000, MINF) // PUnit BIOS
> mailbox Interface and GTD/ISPD mailbox
This is map to res1
These two entries are firstly parsed in pmc driver firstly and reported to
punit driver.
>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.
This is real ASL code I got from BIOS team and validated on bxt platform.
Confirmed BIOS runtime code will update "acpi table" in memory before switch to
os, for those resource which need to get/read from hardware dynamically, in
this case, MINF and MDAT is set in runtime when boot BIOS and size of res0 is
set as 4B.
> 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
