From: Darren Hart <dvhart@infradead.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Qipeng Zha <qipeng.zha@intel.com>, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH V8 1/2] intel_pmc_ipc: update acpi resource structure for Punit
Date: Tue, 8 Dec 2015 16:21:05 -0800 [thread overview]
Message-ID: <20151209002105.GB11972@malice.jf.intel.com> (raw)
In-Reply-To: <1449579438.30729.48.camel@linux.intel.com>
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.
--
Darren Hart
Intel Open Source Technology Center
next prev parent reply other threads:[~2015-12-09 0:21 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
2015-12-08 12:57 ` Andy Shevchenko
2015-12-08 22:59 ` Darren Hart
2015-12-09 0:21 ` Darren Hart [this message]
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=20151209002105.GB11972@malice.jf.intel.com \
--to=dvhart@infradead.org \
--cc=andriy.shevchenko@linux.intel.com \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.