Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit

[Darren Hart <dvhart@infradead.org>]

On Wed, Dec 09, 2015 at 01:09:51PM +0200, Andy Shevchenko wrote:
> On Tue, 2015-12-08 at 16:21 -0800, Darren Hart wrote:
> > 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@intel.com>
> > > > 
> > > > 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_I
> > > > > NDEX
> > > > > );
> > > > > +				    PLAT_RES_PUNIT_BIOS_DATA_I
> > > > > NDEX
> > > > > );
> > > > >  	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.
> 
> For simplicity sake let's leave this as current iterative approach.
> 
> Maybe we may go with the comment before each punit_res++; line to
> explain "this is index N to cover Y".

Sounds like a plan to me. Qipeng, could you include that in your final version?

-- 
Darren Hart
Intel Open Source Technology Center