On Tue, Dec 08, 2015 at 02:57:18PM +0200, Andy Shevchenko wrote:
> On Mon, 2015-12-07 at 15:45 -0800, Darren Hart wrote:
> > On Tue, Dec 08, 2015 at 12:55:04AM +0800, Qipeng Zha wrote:
> > > BIOS restructure exported memory resources for Punit
> > > in acpi table, So update resources for Punit.
> > >
> > > Signed-off-by: Qipeng Zha <qipeng.zha@xxxxxxxxx>
> >
> > Thank you for the update Qipeng. I will review shortly.
> >
> > +Andriy who originally raised the concern over the ACPI resource
> > assumptions in
> > the previous version. Andriy, this resource allocation looks to be a
> > substantial
> > improvement to me. Do you have any further concerns?
>
> Here I have few mostly stylish concerns.
>
> >
> > > ---
> > > drivers/platform/x86/intel_pmc_ipc.c | 142
> > > +++++++++++++++++++++++------------
> > > 1 file changed, 96 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/intel_pmc_ipc.c
> > > b/drivers/platform/x86/intel_pmc_ipc.c
> > > index 28b2a12..c699950 100644
> > > --- a/drivers/platform/x86/intel_pmc_ipc.c
> > > +++ b/drivers/platform/x86/intel_pmc_ipc.c
> > > @@ -65,12 +65,16 @@
> > > #define IPC_TRIGGER_MODE_IRQ true
> > >
> > > /* exported resources from IFWI */
> > > -#define PLAT_RESOURCE_IPC_INDEX 0
> > > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX 2
> > > -#define PLAT_RESOURCE_ACPI_IO_INDEX 0
> > > +#define PLAT_RES_IPC_INDEX 0
> > > +#define PLAT_RES_IPC_SIZE 0x1000
> > > +#define PLAT_RES_GCR_SIZE 0x1000
> > > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX 1
> > > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX 2
> > > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX 4
> > > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX 5
> > > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX 6
> > > +#define PLAT_RES_PUNIT_GTD_INTER_INDEX 7
> > > +#define PLAT_RES_ACPI_IO_INDEX 0
>
> May I propose to rename a bit this one?
> For me looks like PUNIT is not needed in the naming.
>
> What about
>
> /* Resource indexes */
> #define PLAT_RESOURCE_IPC_INDEX 0
> /* P-Unit */
> #define PLAT_RESOURCE_BIOS_DATA_INDEX 1
> …
>
> #define PLAT_RESOURCE_GTD_INTER_INDEX 7
Seems reasonable, Qipeng, does PUNIT provide some kind of namespacing that
relevantly separates this from PLAT_RES_IPC_INDEX for example? Is it a different
component?
I don't want to bikeshed too much over this though, certainly don't want to hold
up getting this in next over it. But as we do have some items below to address,
please consider this.
...
> > > @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct
> > > platform_device *pdev)
> > > dev_info(&pdev->dev, "io res: %llx %x\n",
> > > (long long)res->start, (int)resource_size(res));
> > >
> > > + punit_res = punit_res_array;
> > > res = platform_get_resource(pdev, IORESOURCE_MEM,
> > > - PLAT_RESOURCE_PUNIT_DATA_INDEX
> > > );
> > > + PLAT_RES_PUNIT_BIOS_DATA_INDEX
> > > );
> > > if (!res) {
> > > - dev_err(&pdev->dev, "Failed to get punit
> > > resource\n");
> > > + dev_err(&pdev->dev, "Failed to get res of punit
> > > BIOS data\n");
> > > return -ENXIO;
> > > }
> > > - size = resource_size(res);
> > > - ipcdev.punit_base = res->start;
> > > - ipcdev.punit_size = size;
> > > - dev_info(&pdev->dev, "punit data res: %llx %x\n",
>
> > > + punit_res->start = res->start;
> > > + punit_res->end = res->start + resource_size(res) - 1;
>
> Seems like
>
> *punit_res = *res;
>
> Though punit_res is assigned to punit_res_array which seems not right
> to me.
>
> If it's a member of that array we have to explicitly show the index.
>
It seems fairly clear to me that punit_res is used to iterate through the items
of the array using pointer arithmetic, but if it could be clearer, great. What
would you prefer to see Andriy?
Unfortunatley, we can't use the defined indices for the ACPI resources as they
are neither 0 based nor sequential. This does mean that the punit driver relies
on the order the pmc driver populates this array, rather than defined indices.
This binding, however, is contained entirely within the kernel, so I'm not so
concerned as I was with the ACPI resources being assumed contiguous.
We could move the DATA, INTERFACE enums to intel_punit_ipc.h and use the
following indices without introducing new enums...
(2 * BIOS_IPC) + DATA
(2 * BIOS_IPC) + INTERFACE
(2 * GTDRIVER_IPC) + DATA
(2 * GTDRIVER_IPC) + INTERFACE
(2 * GTDRIVER_IPC) + DATA
(2 * GTDRIVER_IPC) + INTERFACE
But that's pretty horrible, so I suppose the only real solution would be to
introduce yet another set of defines:
#define PUNIT_PLAT_RES_BIOS_IPC_DATA_INDEX 0
#define PUNIT_PLAT_RES_BIOS_IPC_INTERFACE_INDEX 1
#define PUNIT_PLAT_RES_GTDRIVER_IPC_DATA_INDEX 2
#define PUNIT_PLAT_RES_GTDRIVER_IPC_INTERFACE_INDEX 3
#define PUNIT_PLAT_RES_ISPDRIVER_IPC_DATA_INDEX 4
#define PUNIT_PLAT_RES_ISPDRIVER_IPC_INTERFACE_INDEX 5
I'm not really sure this is better given the lengthy list for defines already
present.
So, if you would like to a change, please recommend what you would prefer
Andriy, because I can see the argument for either approach.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
