| Message ID | 1449507305-51709-1-git-send-email-qipeng.zha@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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? > --- > 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 > > /* > * 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(&pdev->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(&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; > + dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n", > (long long)res->start, (int)resource_size(res)); > > res = platform_get_resource(pdev, IORESOURCE_MEM, > - PLAT_RESOURCE_PUNIT_INTER_INDEX); > + PLAT_RES_PUNIT_BIOS_INTER_INDEX); > if (!res) { > - dev_err(&pdev->dev, "Failed to get punit inter resource\n"); > + dev_err(&pdev->dev, "Failed to get res of punit BIOS inter\n"); > return -ENXIO; > } > - size = resource_size(res); > - ipcdev.punit_base2 = res->start; > - ipcdev.punit_size2 = size; > - dev_info(&pdev->dev, "punit interface res: %llx %x\n", > + punit_res++; > + punit_res->start = res->start; > + punit_res->end = res->start + resource_size(res) - 1; > + dev_info(&pdev->dev, "punit BIOS interface res: %llx %x\n", > + (long long)res->start, (int)resource_size(res)); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, > + PLAT_RES_PUNIT_ISP_DATA_INDEX); > + if (!res) { > + dev_err(&pdev->dev, "Failed to get res of punit ISP data\n"); > + return -ENXIO; > + } > + punit_res++; > + punit_res->start = res->start; > + punit_res->end = res->start + resource_size(res) - 1; > + dev_info(&pdev->dev, "punit ISP data res: %llx %x\n", > (long long)res->start, (int)resource_size(res)); > > res = platform_get_resource(pdev, IORESOURCE_MEM, > - PLAT_RESOURCE_IPC_INDEX); > + PLAT_RES_PUNIT_ISP_INTER_INDEX); > + if (!res) { > + dev_err(&pdev->dev, "Failed to get res of punit ISP inter\n"); > + return -ENXIO; > + } > + punit_res++; > + punit_res->start = res->start; > + punit_res->end = res->start + resource_size(res) - 1; > + dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n", > + (long long)res->start, (int)resource_size(res)); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, > + PLAT_RES_PUNIT_GTD_DATA_INDEX); > + if (!res) { > + dev_err(&pdev->dev, "Failed to get res of punit GTD data\n"); > + return -ENXIO; > + } > + punit_res++; > + punit_res->start = res->start; > + punit_res->end = res->start + resource_size(res) - 1; > + dev_info(&pdev->dev, "punit GTD data res: %llx %x\n", > + (long long)res->start, (int)resource_size(res)); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, > + PLAT_RES_PUNIT_GTD_INTER_INDEX); > + if (!res) { > + dev_err(&pdev->dev, "Failed to get res of punit GTD inter\n"); > + return -ENXIO; > + } > + punit_res++; > + punit_res->start = res->start; > + punit_res->end = res->start + resource_size(res) - 1; > + dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n", > + (long long)res->start, (int)resource_size(res)); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RES_IPC_INDEX); > if (!res) { > dev_err(&pdev->dev, "Failed to get ipc resource\n"); > return -ENXIO; > } > - size = PLAT_RESOURCE_IPC_SIZE; > + size = PLAT_RES_IPC_SIZE; > if (!request_mem_region(res->start, size, pdev->name)) { > dev_err(&pdev->dev, "Failed to request ipc resource\n"); > return -EBUSY; > @@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct platform_device *pdev) > ipcdev.ipc_base = addr; > > ipcdev.gcr_base = res->start + size; > - ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE; > + ipcdev.gcr_size = PLAT_RES_GCR_SIZE; > dev_info(&pdev->dev, "ipc res: %llx %x\n", > (long long)res->start, (int)resource_size(res)); > > @@ -714,9 +764,9 @@ err_irq: > err_device: > iounmap(ipcdev.ipc_base); > res = platform_get_resource(pdev, IORESOURCE_MEM, > - PLAT_RESOURCE_IPC_INDEX); > + PLAT_RES_IPC_INDEX); > if (res) > - release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE); > + release_mem_region(res->start, PLAT_RES_IPC_SIZE); > return ret; > } > > @@ -730,9 +780,9 @@ static int ipc_plat_remove(struct platform_device *pdev) > platform_device_unregister(ipcdev.punit_dev); > iounmap(ipcdev.ipc_base); > res = platform_get_resource(pdev, IORESOURCE_MEM, > - PLAT_RESOURCE_IPC_INDEX); > + PLAT_RES_IPC_INDEX); > if (res) > - release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE); > + release_mem_region(res->start, PLAT_RES_IPC_SIZE); > ipcdev.dev = NULL; > return 0; > } > -- > 1.8.3.2 > >
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? 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_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 > > > > /* > > * 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(&pdev->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(&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; > > + dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n", > > (long long)res->start, (int)resource_size(res)); > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_PUNIT_INTER_INDE > > X); > > + PLAT_RES_PUNIT_BIOS_INTER_INDE > > X); > > if (!res) { > > - dev_err(&pdev->dev, "Failed to get punit inter > > resource\n"); > > + dev_err(&pdev->dev, "Failed to get res of punit > > BIOS inter\n"); > > return -ENXIO; > > } > > - size = resource_size(res); > > - ipcdev.punit_base2 = res->start; > > - ipcdev.punit_size2 = size; > > - dev_info(&pdev->dev, "punit interface res: %llx %x\n", > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit BIOS interface res: %llx > > %x\n", > > + (long long)res->start, (int)resource_size(res)); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > + PLAT_RES_PUNIT_ISP_DATA_INDEX) > > ; > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > ISP data\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit ISP data res: %llx %x\n", > > (long long)res->start, (int)resource_size(res)); > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_IPC_INDEX); > > + PLAT_RES_PUNIT_ISP_INTER_INDEX > > ); > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > ISP inter\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n", > > + (long long)res->start, (int)resource_size(res)); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > + PLAT_RES_PUNIT_GTD_DATA_INDEX) > > ; > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > GTD data\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit GTD data res: %llx %x\n", > > + (long long)res->start, (int)resource_size(res)); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > + PLAT_RES_PUNIT_GTD_INTER_INDEX > > ); > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > GTD inter\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n", > > + (long long)res->start, (int)resource_size(res)); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > PLAT_RES_IPC_INDEX); > > if (!res) { > > dev_err(&pdev->dev, "Failed to get ipc > > resource\n"); > > return -ENXIO; > > } > > - size = PLAT_RESOURCE_IPC_SIZE; > > + size = PLAT_RES_IPC_SIZE; > > if (!request_mem_region(res->start, size, pdev->name)) { > > dev_err(&pdev->dev, "Failed to request ipc > > resource\n"); > > return -EBUSY; > > @@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct > > platform_device *pdev) > > ipcdev.ipc_base = addr; > > > > ipcdev.gcr_base = res->start + size; > > - ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE; > > + ipcdev.gcr_size = PLAT_RES_GCR_SIZE; > > dev_info(&pdev->dev, "ipc res: %llx %x\n", > > (long long)res->start, (int)resource_size(res)); > > > > @@ -714,9 +764,9 @@ err_irq: > > err_device: > > iounmap(ipcdev.ipc_base); > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_IPC_INDEX); > > + PLAT_RES_IPC_INDEX); > > if (res) > > - release_mem_region(res->start, > > PLAT_RESOURCE_IPC_SIZE); > > + release_mem_region(res->start, PLAT_RES_IPC_SIZE); > > return ret; > > } > > > > @@ -730,9 +780,9 @@ static int ipc_plat_remove(struct > > platform_device *pdev) > > platform_device_unregister(ipcdev.punit_dev); > > iounmap(ipcdev.ipc_base); > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_IPC_INDEX); > > + PLAT_RES_IPC_INDEX); > > if (res) > > - release_mem_region(res->start, > > PLAT_RESOURCE_IPC_SIZE); > > + release_mem_region(res->start, PLAT_RES_IPC_SIZE); > > ipcdev.dev = NULL; > > return 0; > > } > > -- > > 1.8.3.2 > > > > >
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 > > > > /* > > * 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(&pdev->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(&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. > > + dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n", > > (long long)res->start, (int)resource_size(res)); %pR (Might be another patch in the future to fix existing code to move to %pR) > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_PUNIT_INTER_INDE > > X); > > + PLAT_RES_PUNIT_BIOS_INTER_INDE > > X); > > if (!res) { > > - dev_err(&pdev->dev, "Failed to get punit inter > > resource\n"); > > + dev_err(&pdev->dev, "Failed to get res of punit > > BIOS inter\n"); Darren, can you improve this phrasing, I didn't get what this message about? > > return -ENXIO; > > } > > - size = resource_size(res); > > - ipcdev.punit_base2 = res->start; > > - ipcdev.punit_size2 = size; > > - dev_info(&pdev->dev, "punit interface res: %llx %x\n", > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit BIOS interface res: %llx > > %x\n", > > + (long long)res->start, (int)resource_size(res)); Same comments as above. > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > + PLAT_RES_PUNIT_ISP_DATA_INDEX) > > ; > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > ISP data\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit ISP data res: %llx %x\n", > > (long long)res->start, (int)resource_size(res)); Ditto. > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_IPC_INDEX); > > + PLAT_RES_PUNIT_ISP_INTER_INDEX > > ); > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > ISP inter\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n", > > + (long long)res->start, (int)resource_size(res)); Ditto. > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > + PLAT_RES_PUNIT_GTD_DATA_INDEX) > > ; > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > GTD data\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit GTD data res: %llx %x\n", > > + (long long)res->start, (int)resource_size(res)); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > + PLAT_RES_PUNIT_GTD_INTER_INDEX > > ); > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get res of punit > > GTD inter\n"); > > + return -ENXIO; > > + } > > + punit_res++; > > + punit_res->start = res->start; > > + punit_res->end = res->start + resource_size(res) - 1; > > + dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n", > > + (long long)res->start, (int)resource_size(res)); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, > > PLAT_RES_IPC_INDEX); > > if (!res) { > > dev_err(&pdev->dev, "Failed to get ipc > > resource\n"); > > return -ENXIO; > > } > > - size = PLAT_RESOURCE_IPC_SIZE; > > + size = PLAT_RES_IPC_SIZE; > > if (!request_mem_region(res->start, size, pdev->name)) { > > dev_err(&pdev->dev, "Failed to request ipc > > resource\n"); > > return -EBUSY; > > @@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct > > platform_device *pdev) > > ipcdev.ipc_base = addr; > > > > ipcdev.gcr_base = res->start + size; > > - ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE; > > + ipcdev.gcr_size = PLAT_RES_GCR_SIZE; > > dev_info(&pdev->dev, "ipc res: %llx %x\n", > > (long long)res->start, (int)resource_size(res)); > > > > @@ -714,9 +764,9 @@ err_irq: > > err_device: > > iounmap(ipcdev.ipc_base); > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_IPC_INDEX); > > + PLAT_RES_IPC_INDEX); > > if (res) > > - release_mem_region(res->start, > > PLAT_RESOURCE_IPC_SIZE); > > + release_mem_region(res->start, PLAT_RES_IPC_SIZE); > > return ret; > > } > > > > @@ -730,9 +780,9 @@ static int ipc_plat_remove(struct > > platform_device *pdev) > > platform_device_unregister(ipcdev.punit_dev); > > iounmap(ipcdev.ipc_base); > > res = platform_get_resource(pdev, IORESOURCE_MEM, > > - PLAT_RESOURCE_IPC_INDEX); > > + PLAT_RES_IPC_INDEX); > > if (res) > > - release_mem_region(res->start, > > PLAT_RESOURCE_IPC_SIZE); > > + release_mem_region(res->start, PLAT_RES_IPC_SIZE); > > ipcdev.dev = NULL; > > return 0; > > } > > -- > > 1.8.3.2 > > > > >
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? So, regarding to the second patch 1. In excerpts like following if (IS_ERR(addr)) { dev_err(&pdev->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.
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(&pdev->dev, "Failed to get punit inter > > > resource\n"); > > > + dev_err(&pdev->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(&pdev->dev, "Failed to get punit BIOS interface platform resource\n"); Similarly for all these messages.
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 <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? > > So, regarding to the second patch > > 1. In excerpts like following > > if (IS_ERR(addr)) { > dev_err(&pdev->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!
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 <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? > > So, regarding to the second patch > > 1. In excerpts like following > > if (IS_ERR(addr)) { > dev_err(&pdev->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.
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_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.
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".
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?
>> +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(&pdev->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
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 /* * 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(&pdev->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(&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; + dev_info(&pdev->dev, "punit BIOS data res: %llx %x\n", (long long)res->start, (int)resource_size(res)); res = platform_get_resource(pdev, IORESOURCE_MEM, - PLAT_RESOURCE_PUNIT_INTER_INDEX); + PLAT_RES_PUNIT_BIOS_INTER_INDEX); if (!res) { - dev_err(&pdev->dev, "Failed to get punit inter resource\n"); + dev_err(&pdev->dev, "Failed to get res of punit BIOS inter\n"); return -ENXIO; } - size = resource_size(res); - ipcdev.punit_base2 = res->start; - ipcdev.punit_size2 = size; - dev_info(&pdev->dev, "punit interface res: %llx %x\n", + punit_res++; + punit_res->start = res->start; + punit_res->end = res->start + resource_size(res) - 1; + dev_info(&pdev->dev, "punit BIOS interface res: %llx %x\n", + (long long)res->start, (int)resource_size(res)); + + res = platform_get_resource(pdev, IORESOURCE_MEM, + PLAT_RES_PUNIT_ISP_DATA_INDEX); + if (!res) { + dev_err(&pdev->dev, "Failed to get res of punit ISP data\n"); + return -ENXIO; + } + punit_res++; + punit_res->start = res->start; + punit_res->end = res->start + resource_size(res) - 1; + dev_info(&pdev->dev, "punit ISP data res: %llx %x\n", (long long)res->start, (int)resource_size(res)); res = platform_get_resource(pdev, IORESOURCE_MEM, - PLAT_RESOURCE_IPC_INDEX); + PLAT_RES_PUNIT_ISP_INTER_INDEX); + if (!res) { + dev_err(&pdev->dev, "Failed to get res of punit ISP inter\n"); + return -ENXIO; + } + punit_res++; + punit_res->start = res->start; + punit_res->end = res->start + resource_size(res) - 1; + dev_info(&pdev->dev, "punit ISP interface res: %llx %x\n", + (long long)res->start, (int)resource_size(res)); + + res = platform_get_resource(pdev, IORESOURCE_MEM, + PLAT_RES_PUNIT_GTD_DATA_INDEX); + if (!res) { + dev_err(&pdev->dev, "Failed to get res of punit GTD data\n"); + return -ENXIO; + } + punit_res++; + punit_res->start = res->start; + punit_res->end = res->start + resource_size(res) - 1; + dev_info(&pdev->dev, "punit GTD data res: %llx %x\n", + (long long)res->start, (int)resource_size(res)); + + res = platform_get_resource(pdev, IORESOURCE_MEM, + PLAT_RES_PUNIT_GTD_INTER_INDEX); + if (!res) { + dev_err(&pdev->dev, "Failed to get res of punit GTD inter\n"); + return -ENXIO; + } + punit_res++; + punit_res->start = res->start; + punit_res->end = res->start + resource_size(res) - 1; + dev_info(&pdev->dev, "punit GTD interface res: %llx %x\n", + (long long)res->start, (int)resource_size(res)); + + res = platform_get_resource(pdev, IORESOURCE_MEM, PLAT_RES_IPC_INDEX); if (!res) { dev_err(&pdev->dev, "Failed to get ipc resource\n"); return -ENXIO; } - size = PLAT_RESOURCE_IPC_SIZE; + size = PLAT_RES_IPC_SIZE; if (!request_mem_region(res->start, size, pdev->name)) { dev_err(&pdev->dev, "Failed to request ipc resource\n"); return -EBUSY; @@ -650,7 +700,7 @@ static int ipc_plat_get_res(struct platform_device *pdev) ipcdev.ipc_base = addr; ipcdev.gcr_base = res->start + size; - ipcdev.gcr_size = PLAT_RESOURCE_GCR_SIZE; + ipcdev.gcr_size = PLAT_RES_GCR_SIZE; dev_info(&pdev->dev, "ipc res: %llx %x\n", (long long)res->start, (int)resource_size(res)); @@ -714,9 +764,9 @@ err_irq: err_device: iounmap(ipcdev.ipc_base); res = platform_get_resource(pdev, IORESOURCE_MEM, - PLAT_RESOURCE_IPC_INDEX); + PLAT_RES_IPC_INDEX); if (res) - release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE); + release_mem_region(res->start, PLAT_RES_IPC_SIZE); return ret; } @@ -730,9 +780,9 @@ static int ipc_plat_remove(struct platform_device *pdev) platform_device_unregister(ipcdev.punit_dev); iounmap(ipcdev.ipc_base); res = platform_get_resource(pdev, IORESOURCE_MEM, - PLAT_RESOURCE_IPC_INDEX); + PLAT_RES_IPC_INDEX); if (res) - release_mem_region(res->start, PLAT_RESOURCE_IPC_SIZE); + release_mem_region(res->start, PLAT_RES_IPC_SIZE); ipcdev.dev = NULL; return 0; }
BIOS restructure exported memory resources for Punit in acpi table, So update resources for Punit. Signed-off-by: Qipeng Zha <qipeng.zha@intel.com> --- drivers/platform/x86/intel_pmc_ipc.c | 142 +++++++++++++++++++++++------------ 1 file changed, 96 insertions(+), 46 deletions(-)