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

[Andy Shevchenko]

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 
> > > 
> > > 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_INDEX0
> > > > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > > > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > > > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > > > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX2
> > > > -#define PLAT_RESOURCE_ACPI_IO_INDEX0
> > > > +#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_INDEX2
> > > > +#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(>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(>dev, "Failed to get punit
> > > > resource\n");
> > > > +   dev_err(>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(>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 

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

[Darren Hart]

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 
> > > > 
> > > > 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_SIZE0x1000
> > > > > +#define PLAT_RES_GCR_SIZE0x1000
> > > > > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX   1
> > > > > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX  2
> > > > > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX4
> > > > > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX   5
> > > > > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX6
> > > > > +#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(>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(>dev, "Failed to get punit
> > > > > resource\n");
> > > > > + dev_err(>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(>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 

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

[Zha, Qipeng]

>> +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?

>So, regarding to the second patch

>1. In excerpts like following

>if (IS_ERR(addr)) {
>   dev_err(>dev, "Failed to map resouce for BIOS DATA\n");
>   return PTR_ERR(addr);
>   }

>No need to have an error message. Core already has something to print at that 
>point.

Core error message will not show which resource is failed,
Anyway, I will made this change

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

[Andy Shevchenko]

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 
> 
> 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?

I will check it later.

Hmm… I am not subscribed to platform-driver-x86@ and wasn't in Cc list,
so it might delay me response.

> 
> > ---
> >  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_INDEX0
> > -#define PLAT_RESOURCE_IPC_SIZE 0x1000
> > -#define PLAT_RESOURCE_GCR_SIZE 0x1000
> > -#define PLAT_RESOURCE_PUNIT_DATA_INDEX 1
> > -#define PLAT_RESOURCE_PUNIT_INTER_INDEX2
> > -#define PLAT_RESOURCE_ACPI_IO_INDEX0
> > +#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_INDEX2
> > +#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
> >  
> >  /*
> >   * BIOS does not create an ACPI device for each PMC function,
> > @@ -105,10 +109,6 @@ static struct intel_pmc_ipc_dev {
> >     int gcr_size;
> >  
> >     /* punit */
> > -   resource_size_t punit_base;
> > -   int punit_size;
> > -   resource_size_t punit_base2;
> > -   int punit_size2;
> >     struct platform_device *punit_dev;
> >  } ipcdev;
> >  
> > @@ -444,9 +444,22 @@ static const struct attribute_group
> > intel_ipc_group = {
> >     .attrs = intel_ipc_attrs,
> >  };
> >  
> > -#define PUNIT_RESOURCE_INTER   1
> > -static struct resource punit_res[] = {
> > -   /* Punit */
> > +static struct resource punit_res_array[] = {
> > +   /* Punit BIOS */
> > +   {
> > +   .flags = IORESOURCE_MEM,
> > +   },
> > +   {
> > +   .flags = IORESOURCE_MEM,
> > +   },
> > +   /* Punit ISP */
> > +   {
> > +   .flags = IORESOURCE_MEM,
> > +   },
> > +   {
> > +   .flags = IORESOURCE_MEM,
> > +   },
> > +   /* Punit GTD */
> >     {
> >     .flags = IORESOURCE_MEM,
> >     },
> > @@ -481,7 +494,6 @@ static struct itco_wdt_platform_data tco_info =
> > {
> >  static int ipc_create_punit_device(void)
> >  {
> >     struct platform_device *pdev;
> > -   struct resource *res;
> >     int ret;
> >  
> >     pdev = platform_device_alloc(PUNIT_DEVICE_NAME, -1);
> > @@ -491,17 +503,8 @@ static int ipc_create_punit_device(void)
> >     }
> >  
> >     pdev->dev.parent = ipcdev.dev;
> > -
> > -   res = punit_res;
> > -   res->start = ipcdev.punit_base;
> > -   res->end = res->start + ipcdev.punit_size - 1;
> > -
> > -   res = punit_res + PUNIT_RESOURCE_INTER;
> > -   res->start = ipcdev.punit_base2;
> > -   res->end = res->start + ipcdev.punit_size2 - 1;
> > -
> > -   ret = platform_device_add_resources(pdev, punit_res,
> > -   ARRAY_SIZE(punit_res))
> > ;
> > +   ret = platform_device_add_resources(pdev, punit_res_array,
> > +   ARRAY_SIZE(punit_res_a
> > rray));
> >     if (ret) {
> >     dev_err(ipcdev.dev, "Failed to add platform punit
> > resources\n");
> >     goto err;
> > @@ -590,12 +593,12 @@ static int ipc_create_pmc_devices(void)
> >  
> >  static int ipc_plat_get_res(struct platform_device *pdev)
> >  {
> > -   struct resource *res;
> > +   struct resource *res, *punit_res;
> >     void __iomem *addr;
> >     int size;
> >  
> >     res = platform_get_resource(pdev, IORESOURCE_IO,
> > -   PLAT_RESOURCE_ACPI_IO_INDEX);
> > +   PLAT_RES_ACPI_IO_INDEX);
> >     if (!res) {
> >     dev_err(>dev, "Failed to get io
> > resource\n");
> >     return -ENXIO;
> > @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct
> > platform_device *pdev)
> >     dev_info(>dev, "io res: %llx %x\n",
> >      (long long)res->start, (int)resource_size(res));
> >  
> > +   punit_res = punit_res_array;
> >     res = platform_get_resource(pdev, 

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

[Andy Shevchenko]

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 
> 
> 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?

So, regarding to the second patch

1. In excerpts like following

if (IS_ERR(addr)) {
dev_err(>dev, "Failed to map resouce for BIOS
DATA\n");
return PTR_ERR(addr);
}

No need to have an error message. Core already has something to print
at that point.


2. No need to explicitly cast to / from void *.

IPC_DEV *ipcdev = (IPC_DEV *)dev_id;


Otherwise looks much better than very first version!

Thanks for an update.

-- 
Andy Shevchenko 
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" 
in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darren Hart]

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:

...

> 
> > >  
> > >   res = platform_get_resource(pdev, IORESOURCE_MEM,
> > > - PLAT_RESOURCE_PUNIT_INTER_INDE
> > > X);
> > > + PLAT_RES_PUNIT_BIOS_INTER_INDE
> > > X);
> > >   if (!res) {
> > > - dev_err(>dev, "Failed to get punit inter
> > > resource\n");
> > > + dev_err(>dev, "Failed to get res of punit
> > > BIOS inter\n");
> 
> Darren, can you improve this phrasing, I didn't get what this message
> about?

I wasn't going to mention this unless there was sure to be a v9, it seems that
is likely, so...

Qipeng, in order to ensure the message to the user is clear, it is important not
to use abbreviations (especially non-obvious ones) like "inter" and "res". Note 
that the user will not necessarily have the sourcecode handy for context.

I believe this message is just attempting to the user that the
platform_get_resource message failed for some reason when attempting to get the
ACPI provided resource for the BIOS interface resource. There are two resources
for each, interface and data.

Without knowing the details of the internals, I would think the following might
be a better message?

dev_err(>dev, "Failed to get punit BIOS interface platform 
resource\n");

Similarly for all these messages.

-- 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darren Hart]

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 
> > 
> > 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_SIZE0x1000
> > > +#define PLAT_RES_GCR_SIZE0x1000
> > > +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX   1
> > > +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX  2
> > > +#define PLAT_RES_PUNIT_ISP_DATA_INDEX4
> > > +#define PLAT_RES_PUNIT_ISP_INTER_INDEX   5
> > > +#define PLAT_RES_PUNIT_GTD_DATA_INDEX6
> > > +#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(>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(>dev, "Failed to get punit
> > > resource\n");
> > > + dev_err(>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(>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 

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

[Darren Hart]

On Tue, Dec 08, 2015 at 03:19:17PM +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 
> > 
> > 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?
> 
> So, regarding to the second patch
> 
> 1. In excerpts like following
> 
> if (IS_ERR(addr)) {
>   dev_err(>dev, "Failed to map resouce for BIOS
> DATA\n");
>   return PTR_ERR(addr);
>   }
> 
> No need to have an error message. Core already has something to print
> at that point.

Ah, so neither the DATA nor the INTER messages are necessary. I suppose this
means you can ignore my response on better wording.

-- 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darren Hart]

On Tue, Dec 08, 2015 at 03:19:17PM +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 
> > 
> > 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?
> 
> So, regarding to the second patch
> 
> 1. In excerpts like following
> 
> if (IS_ERR(addr)) {
>   dev_err(>dev, "Failed to map resouce for BIOS
> DATA\n");
>   return PTR_ERR(addr);
>   }
> 
> No need to have an error message. Core already has something to print
> at that point.
> 
> 
> 2. No need to explicitly cast to / from void *.
> 
> IPC_DEV *ipcdev = (IPC_DEV *)dev_id;
> 
> 
> Otherwise looks much better than very first version!

Good points, thanks Andriy.

Qipeng. I hate to ask for one more spin, but this is good feedback, and the
error messages from the previous note should be addressed. If you can give one
more update, we can get this queued to next and still make the 4.5 merge window.

Thanks for your efforts to get to this point. Nearly there!

-- 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darren Hart]

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 

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?

> ---
>  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_SIZE0x1000
> +#define PLAT_RES_GCR_SIZE0x1000
> +#define PLAT_RES_PUNIT_BIOS_DATA_INDEX   1
> +#define PLAT_RES_PUNIT_BIOS_INTER_INDEX  2
> +#define PLAT_RES_PUNIT_ISP_DATA_INDEX4
> +#define PLAT_RES_PUNIT_ISP_INTER_INDEX   5
> +#define PLAT_RES_PUNIT_GTD_DATA_INDEX6
> +#define PLAT_RES_PUNIT_GTD_INTER_INDEX   7
> +#define PLAT_RES_ACPI_IO_INDEX   0
>  
>  /*
>   * BIOS does not create an ACPI device for each PMC function,
> @@ -105,10 +109,6 @@ static struct intel_pmc_ipc_dev {
>   int gcr_size;
>  
>   /* punit */
> - resource_size_t punit_base;
> - int punit_size;
> - resource_size_t punit_base2;
> - int punit_size2;
>   struct platform_device *punit_dev;
>  } ipcdev;
>  
> @@ -444,9 +444,22 @@ static const struct attribute_group intel_ipc_group = {
>   .attrs = intel_ipc_attrs,
>  };
>  
> -#define PUNIT_RESOURCE_INTER 1
> -static struct resource punit_res[] = {
> - /* Punit */
> +static struct resource punit_res_array[] = {
> + /* Punit BIOS */
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + /* Punit ISP */
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + {
> + .flags = IORESOURCE_MEM,
> + },
> + /* Punit GTD */
>   {
>   .flags = IORESOURCE_MEM,
>   },
> @@ -481,7 +494,6 @@ static struct itco_wdt_platform_data tco_info = {
>  static int ipc_create_punit_device(void)
>  {
>   struct platform_device *pdev;
> - struct resource *res;
>   int ret;
>  
>   pdev = platform_device_alloc(PUNIT_DEVICE_NAME, -1);
> @@ -491,17 +503,8 @@ static int ipc_create_punit_device(void)
>   }
>  
>   pdev->dev.parent = ipcdev.dev;
> -
> - res = punit_res;
> - res->start = ipcdev.punit_base;
> - res->end = res->start + ipcdev.punit_size - 1;
> -
> - res = punit_res + PUNIT_RESOURCE_INTER;
> - res->start = ipcdev.punit_base2;
> - res->end = res->start + ipcdev.punit_size2 - 1;
> -
> - ret = platform_device_add_resources(pdev, punit_res,
> - ARRAY_SIZE(punit_res));
> + ret = platform_device_add_resources(pdev, punit_res_array,
> + ARRAY_SIZE(punit_res_array));
>   if (ret) {
>   dev_err(ipcdev.dev, "Failed to add platform punit resources\n");
>   goto err;
> @@ -590,12 +593,12 @@ static int ipc_create_pmc_devices(void)
>  
>  static int ipc_plat_get_res(struct platform_device *pdev)
>  {
> - struct resource *res;
> + struct resource *res, *punit_res;
>   void __iomem *addr;
>   int size;
>  
>   res = platform_get_resource(pdev, IORESOURCE_IO,
> - PLAT_RESOURCE_ACPI_IO_INDEX);
> + PLAT_RES_ACPI_IO_INDEX);
>   if (!res) {
>   dev_err(>dev, "Failed to get io resource\n");
>   return -ENXIO;
> @@ -606,37 +609,84 @@ static int ipc_plat_get_res(struct platform_device 
> *pdev)
>   dev_info(>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(>dev, "Failed to get punit resource\n");
> + dev_err(>dev, "Failed to get res of punit BIOS data\n");
>   return -ENXIO;
>