From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>,
"Rafael J. Wysocki" <rjw@rjwysocki.net>,
linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linuxarm@huawei.com,
Thomas Gleixner <tglx@linutronix.de>,
Greg KH <gregkh@linuxfoundation.org>,
Tomasz Nowicki <tn@semihalf.com>, Ma Jun <majun258@huawei.com>,
Kefeng Wang <wangkefeng.wang@huawei.com>,
Agustin Vega-Frias <agustinv@codeaurora.org>,
Sinan Kaya <okaya@codeaurora.org>,
charles.garcia-tobin@arm.com, huxinwei@huawei.com,
yimin@huawei.com, Jon Masters <jcm@redhat.com>
Subject: Re: [PATCH v6 05/14] ACPI: platform-msi: retrieve dev id from IORT
Date: Thu, 5 Jan 2017 15:15:30 +0000 [thread overview]
Message-ID: <20170105151530.GA30852@red-moon> (raw)
In-Reply-To: <eb1a8149-7c46-39bc-f655-58ca6345f40a@linaro.org>
On Thu, Jan 05, 2017 at 08:45:37PM +0800, Hanjun Guo wrote:
> Hi Lorenzo,
>
> On 2017/1/5 3:18, Lorenzo Pieralisi wrote:
> >On Mon, Jan 02, 2017 at 09:31:36PM +0800, Hanjun Guo wrote:
> >>For devices connecting to ITS, it needs dev id to identify
> >>itself, and this dev id is represented in the IORT table in
> >>named componant node [1] for platform devices, so in this
> >>patch we will scan the IORT to retrieve device's dev id.
> >>
> >>Introduce iort_pmsi_get_dev_id() with pointer dev passed
> >>in for that purpose.
> >>
> >>[1]: https://static.docs.arm.com/den0049/b/DEN0049B_IO_Remapping_Table.pdf
> >>
> >>Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>Tested-by: Sinan Kaya <okaya@codeaurora.org>
> >>Tested-by: Majun <majun258@huawei.com>
> >>Tested-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> >>Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>Cc: Tomasz Nowicki <tn@semihalf.com>
> >>Cc: Thomas Gleixner <tglx@linutronix.de>
> >>---
> >> drivers/acpi/arm64/iort.c | 26 ++++++++++++++++++++++++++
> >> drivers/irqchip/irq-gic-v3-its-platform-msi.c | 4 +++-
> >> include/linux/acpi_iort.h | 8 ++++++++
> >> 3 files changed, 37 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >>index 174e983..ab7bae7 100644
> >>--- a/drivers/acpi/arm64/iort.c
> >>+++ b/drivers/acpi/arm64/iort.c
> >>@@ -444,6 +444,32 @@ u32 iort_msi_map_rid(struct device *dev, u32 req_id)
> >> }
> >>
> >> /**
> >>+ * iort_pmsi_get_dev_id() - Get the device id for a device
> >>+ * @dev: The device for which the mapping is to be done.
> >>+ * @dev_id: The device ID found.
> >>+ *
> >>+ * Returns: 0 for successful find a dev id, errors otherwise
> >>+ */
> >>+int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
> >>+{
> >>+ struct acpi_iort_node *node;
> >>+
> >>+ if (!iort_table)
> >>+ return -ENODEV;
> >>+
> >>+ node = iort_find_dev_node(dev);
> >>+ if (!node) {
> >>+ dev_err(dev, "can't find related IORT node\n");
> >>+ return -ENODEV;
> >>+ }
> >>+
> >>+ if(!iort_node_get_id(node, dev_id, IORT_MSI_TYPE, 0))
> >
> >I disagree with this approach. For named components we know that
> >there are always two steps involved (second optional):
> >
> >(1) Retrieve the initial id (this may well provide the final mapping)
> >(2) Map the id (optional if (1) represents the map type we need)
> >
> >That's the reason why I kept iort_node_get_id() and iort_node_map_rid()
> >separated.
> >
> >Now, what we can do is to create an iort_node_map_id() function that is
> >PCI agnostic (ie rename rid to id :)), whose rid_in is either a PCI RID
> >or the outcome of a previous call to iort_node_get_id() for named
> >components, that's in my opinion cleaner.
>
> iort_node_map_rid() was designed for that purpose, and we can use it
> for platform device, the issue that we need to pass a req id
> unconditionally which is not needed for platform device, Tomasz
> proposed a similar solution to rework iort_node_map_rid(), and
> I think it makes sense.
>
> >
> >It would be even cleaner if you passed a type_mask (or write a
> >wrapper function for that) that is:
> >
> >(IORT_MSI_TYPE | IORT_IOMMU_TYPE)
>
> Sorry, I got little lost here, could you explain it in detail?
Yes sorry I was not clear. What I wanted to say is, for named
components, that do not have an intrinsic id, we have to call
iort_node_get_id() regardless of the type mask, we have to have
a way to get the "source/initial id", so basically the type_mask
is not important at all, it becomes important when it comes to
understanding what type of id the value returned from
iort_node_get_id() is.
So basically, passing:
#define IORT_TYPE_ANY (IORT_MSI_TYPE | IORT_IOMMU_TYPE)
as type_mask to iort_node_get_id() means "retrieve any kind of
initial id", that's what I wanted to say.
In iort_iommu_configure() iort_node_get_id() is a bit different because
we want only a type of id, ie a streamid, therefore the mask that we
pass in is IORT_IOMMU_TYPE.
> >and we just use the returned parent pointer to check if the mapping
> >providing the initial id correspond to the type we are looking for (eg
> >ITS) or we need to map the retrieved initial id any further, with
> >iort_node_map_id(), to get to the final identifier.
> >
> >Thoughts ?
>
> I think rework iort_node_map_rid() and not extend iort_node_get_id()
> is the right direction, could you explain a bit more then I can demo
> the code?
What you can do is create a wrapper, say iort_node_map_platform_id()
(whose signature is equivalent to iort_node_map_rid() minus rid_in)
that carries out the two steps outlined above.
To do that I suggest the following:
(1) I send a patch to "fix" iort_node_get_id() (ie index issue you
reported)
(2) We remove type_mask handling from iort_node_get_id()
(3) We create iort_node_map_platform_id() that (pseudo-code, I can
write the patch if it is clearer):
struct acpi_iort_node *iort_node_map_platform_id(u8 type_mask, int index,
...)
{
u32 id, id_out;
struct acpi_iort_node *parent = iort_node_get_id(&id, index);
if (!parent)
return NULL;
/* we should probably rename iort_node_map_rid() too */
if (!(IORT_TYPE_MASK(parent->type) & type_mask)
parent = iort_node_map_rid(parent, id, &id_out, type_mask);
return parent;
}
(4) we update current iort_node_get_id() users and move them over
to iort_node_map_platform_id()
Let me know if that's clear so that we can agree on a way forward.
Thanks,
Lorenzo
next prev parent reply other threads:[~2017-01-05 15:13 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-02 13:31 [PATCH v6 00/14] ACPI platform MSI support and its example mbigen Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 01/14] ACPI: ARM64: IORT: minor cleanup for iort_match_node_callback() Hanjun Guo
2017-01-03 14:08 ` Lorenzo Pieralisi
2017-01-04 7:56 ` Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 02/14] irqchip: gic-v3-its: keep the head file include in alphabetic order Hanjun Guo
2017-01-11 10:20 ` Matthias Brugger
2017-01-11 10:20 ` Matthias Brugger
2017-01-11 14:16 ` Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 03/14] ACPI: ARM64: IORT: add missing comment for iort_dev_find_its_id() Hanjun Guo
2017-01-04 14:34 ` Lorenzo Pieralisi
2017-01-05 6:05 ` Hanjun Guo
2017-01-05 9:53 ` Lorenzo Pieralisi
2017-01-02 13:31 ` [PATCH v6 04/14] irqchip: gicv3-its: platform-msi: refactor its_pmsi_prepare() Hanjun Guo
2017-01-11 10:31 ` Matthias Brugger
2017-01-02 13:31 ` [PATCH v6 05/14] ACPI: platform-msi: retrieve dev id from IORT Hanjun Guo
2017-01-03 8:43 ` Tomasz Nowicki
2017-01-03 9:37 ` Tomasz Nowicki
2017-01-03 11:24 ` Tomasz Nowicki
2017-01-04 19:18 ` Lorenzo Pieralisi
2017-01-05 12:45 ` Hanjun Guo
2017-01-05 15:15 ` Lorenzo Pieralisi [this message]
2017-01-10 13:39 ` Hanjun Guo
2017-01-10 14:57 ` Lorenzo Pieralisi
2017-01-11 14:15 ` Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 06/14] irqchip: gicv3-its: platform-msi: refactor its_pmsi_init() to prepare for ACPI Hanjun Guo
2017-01-03 7:41 ` Tomasz Nowicki
2017-01-04 7:02 ` Hanjun Guo
2017-01-04 7:29 ` Tomasz Nowicki
2017-01-04 8:25 ` Hanjun Guo
2017-01-04 9:02 ` Marc Zyngier
2017-01-04 10:19 ` Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 07/14] irqchip: gicv3-its: platform-msi: scan MADT to create platform msi domain Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 08/14] ACPI: ARM64: IORT: rework iort_node_get_id() Hanjun Guo
2017-01-04 17:58 ` Lorenzo Pieralisi
2017-01-05 8:19 ` Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 09/14] ACPI: platform: setup MSI domain for ACPI based platform device Hanjun Guo
2017-01-02 21:17 ` Rafael J. Wysocki
2017-01-02 13:31 ` [PATCH v6 10/14] ACPI: ARM64: IORT: rework iort_node_get_id() for NC->SMMU->ITS case Hanjun Guo
2017-01-02 22:30 ` Sinan Kaya
2017-01-03 0:08 ` Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 11/14] msi: platform: make platform_msi_create_device_domain() ACPI aware Hanjun Guo
2017-01-04 16:49 ` Lorenzo Pieralisi
2017-01-02 13:31 ` [PATCH v6 12/14] irqchip: mbigen: drop module owner Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 13/14] irqchip: mbigen: introduce mbigen_of_create_domain() Hanjun Guo
2017-01-02 13:31 ` [PATCH v6 14/14] irqchip: mbigen: Add ACPI support Hanjun Guo
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=20170105151530.GA30852@red-moon \
--to=lorenzo.pieralisi@arm.com \
--cc=agustinv@codeaurora.org \
--cc=charles.garcia-tobin@arm.com \
--cc=gregkh@linuxfoundation.org \
--cc=hanjun.guo@linaro.org \
--cc=huxinwei@huawei.com \
--cc=jcm@redhat.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=majun258@huawei.com \
--cc=marc.zyngier@arm.com \
--cc=okaya@codeaurora.org \
--cc=rjw@rjwysocki.net \
--cc=tglx@linutronix.de \
--cc=tn@semihalf.com \
--cc=wangkefeng.wang@huawei.com \
--cc=yimin@huawei.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;
as well as URLs for NNTP newsgroup(s).