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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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".

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy