From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Darren Hart <dvhart@infradead.org>, Qipeng Zha <qipeng.zha@intel.com>
Cc: platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
Date: Tue, 08 Dec 2015 13:10:21 +0200 [thread overview]
Message-ID: <1449573021.30729.34.camel@linux.intel.com> (raw)
In-Reply-To: <20151207234514.GA12897@malice.jf.intel.com>
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
> >
> >
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
next prev parent reply other threads:[~2015-12-08 11:10 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-07 16:55 [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Qipeng Zha
2015-12-07 16:55 ` [PATCH V8 2/2] platform:x86: add Intel P-Unit mailbox IPC driver Qipeng Zha
2015-12-07 23:45 ` [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit Darren Hart
2015-12-08 11:10 ` Andy Shevchenko [this message]
2015-12-08 12:57 ` Andy Shevchenko
2015-12-08 22:59 ` Darren Hart
2015-12-09 0:21 ` Darren Hart
2015-12-09 11:09 ` Andy Shevchenko
2015-12-09 16:33 ` Darren Hart
2015-12-08 13:19 ` Andy Shevchenko
2015-12-08 23:02 ` Darren Hart
2015-12-08 23:28 ` Darren Hart
2015-12-10 2:39 ` Zha, Qipeng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1449573021.30729.34.camel@linux.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=dvhart@infradead.org \
--cc=platform-driver-x86@vger.kernel.org \
--cc=qipeng.zha@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox