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: Wed, 9 Dec 2015 08:33:53 -0800 [thread overview]
Message-ID: <20151209163353.GD11972@malice.jf.intel.com> (raw)
In-Reply-To: <1449659391.30729.56.camel@linux.intel.com>
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?
--
Darren Hart
Intel Open Source Technology Center
next prev parent reply other threads:[~2015-12-09 16:33 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
2015-12-09 11:09 ` Andy Shevchenko
2015-12-09 16:33 ` Darren Hart [this message]
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=20151209163353.GD11972@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.