These patches implements optional DT properties to generate
smaller sideband data from RID which can be further mapped
to MSI Device ID or Stream ID
On some of the systems, sideband data is smaller than RID
(16bits). For such system, sideband data has to be generated
by dropping some of the RID bits
the process of sideband data extracted from RID can be expressed
using optional DT property {iommu/msi}-map-drop-mask.
Example: If drop-mask is 0xFF09 then sideband data is
8 bits bus number followed by 1 bit of device number and
1 bit function number. This means drop-mask=0xFF09 will
convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).
Srinath Mannam (2):
dt-bindings: pci: Add drop mask property for MSI and IOMMU
pcie: sideband data by dropping RID bits
.../devicetree/bindings/pci/pci-iommu.txt | 31 ++++++++++++++
Documentation/devicetree/bindings/pci/pci-msi.txt | 33 +++++++++++++++
drivers/iommu/of_iommu.c | 4 +-
drivers/of/irq.c | 3 +-
drivers/of/of_pci.c | 48 ++++++++++++++++++++--
include/linux/of_pci.h | 6 ++-
6 files changed, 117 insertions(+), 8 deletions(-)
--
2.7.4
This patch adds info about optional DT properties
iommu-map-drop-mask and msi-map-drop-mask.
A drop mask represents the bits which will be
removed/dropped by system from Requester ID before
mapping it to msi ID or stream ID.
Signed-off-by: Anup Patel <[email protected]>
Signed-off-by: Oza Pawandeep <[email protected]>
Signed-off-by: Srinath Mannam <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
.../devicetree/bindings/pci/pci-iommu.txt | 31 ++++++++++++++++++++
Documentation/devicetree/bindings/pci/pci-msi.txt | 33 ++++++++++++++++++++++
2 files changed, 64 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
index 0def586..499cb27 100644
--- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
+++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
@@ -44,6 +44,9 @@ Optional properties
- iommu-map-mask: A mask to be applied to each Requester ID prior to being
mapped to an IOMMU specifier per the iommu-map property.
+- iommu-map-drop-mask: A drop mask represents the bits which will be
+ removed/dropped by system from Requester ID before mapping it to
+ stream ID.
Example (1)
===========
@@ -169,3 +172,31 @@ Example (4)
<0x8000 &iommu_b 0x0000 0x8000>;
};
};
+
+Example (5)
+===========
+
+/ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ iommu: iommu@a {
+ reg = <0xa 0x1>;
+ compatible = "vendor,some-iommu";
+ #iommu-cells = <1>;
+ };
+
+ pci: pci@f {
+ reg = <0xf 0x1>;
+ compatible = "vendor,pcie-root-complex";
+ device_type = "pci";
+
+ /*
+ * The sideband data provided to the IOMMU is a 10bit
+ * data derived from the RID by dropping 4 MSBs
+ * of device number and 2 MSBs of function number.
+ */
+ iommu-map = <0x0 &iommu 0x0 0x1024>;
+ iommu-map-drop-mask = <0xff09>;
+ };
+};
diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
index 9b3cc81..1de3f39 100644
--- a/Documentation/devicetree/bindings/pci/pci-msi.txt
+++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
@@ -49,6 +49,10 @@ Optional properties
- msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
to an msi-specifier per the msi-map property.
+- msi-map-drop-mask: A drop mask represents the bits which will be
+ removed/dropped by system from Requester ID before mapping it to
+ msi ID.
+
- msi-parent: Describes the MSI parent of the root complex itself. Where
the root complex and MSI controller do not pass sideband data with MSI
writes, this property may be used to describe the MSI controller(s)
@@ -218,3 +222,32 @@ Example (5)
<0x0000 &msi_b 0x0000 0x10000>;
};
};
+
+Example (6)
+===========
+
+/ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ msi: msi-controller@a {
+ reg = <0xa 0x1>;
+ compatible = "vendor,some-controller";
+ msi-controller;
+ #msi-cells = <1>;
+ };
+
+ pci: pci@f {
+ reg = <0xf 0x1>;
+ compatible = "vendor,pcie-root-complex";
+ device_type = "pci";
+
+ /*
+ * The sideband data provided to the MSI controller is
+ * a 10bit data derived from the RID by dropping
+ * 4 MSBs of device number and 2 MSBs of function number.
+ */
+ msi-map = <0x0 &msi_a 0x0 0x100>,
+ msi-map-drop-mask = <0xff09>
+ };
+};
--
2.7.4
The MSI Device ID or Stream ID are passed as sideband
data on the SOC bus to which PCI RC is connected.
If sideband data on SOC bus is less than 16bits then
PCI RC will have to derive sideband data from RID by
dropping selected bits.
This patch implements optional DT properties to generate
smaller sideband data from RID which can be further
mapped to MSI Device ID or Stream ID.
Sideband data generation from RID is done by dropping
bits corresponding zero bits in {iommu/msi}-map-drop-mask.
Example: If drop-mask is 0xFF09 then sideband data is
8 bits bus number followed by 1 bit of device number and
1 bit function number. This means drop-mask=0xFF09 will
convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).
Signed-off-by: Anup Patel <[email protected]>
Signed-off-by: Oza Pawandeep <[email protected]>
Signed-off-by: Srinath Mannam <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/iommu/of_iommu.c | 4 ++--
drivers/of/irq.c | 3 ++-
drivers/of/of_pci.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
include/linux/of_pci.h | 6 ++++--
4 files changed, 53 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 19779b8..f179724 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -169,8 +169,8 @@ static const struct iommu_ops
*/
iommu_spec.np = NULL;
err = of_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
- "iommu-map-mask", &iommu_spec.np,
- iommu_spec.args);
+ "iommu-map-mask", "iommu-map-drop-mask",
+ &iommu_spec.np, iommu_spec.args);
if (err)
return err == -ENODEV ? NULL : ERR_PTR(err);
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index d11437c..454f47a 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -606,7 +606,8 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
*/
for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
if (!of_pci_map_rid(parent_dev->of_node, rid_in, "msi-map",
- "msi-map-mask", np, &rid_out))
+ "msi-map-mask", "msi-map-drop-mask", np,
+ &rid_out))
break;
return rid_out;
}
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index c9d4d3a..a914bcf 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -285,6 +285,35 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
#endif /* CONFIG_OF_ADDRESS */
+static inline u32 out_masked_rid(u32 rid, u32 drop_mask)
+{
+ u32 id = 0;
+ u32 i = 0;
+
+ /* RID's BUS, DEV, FUN values not inside the mask are invalid */
+ if (rid & ~drop_mask)
+ return -EINVAL;
+
+ /*
+ * RID value is translated to sideband data using drop_mask
+ * by dropping bits corresponding zero bits in drop_mask.
+ *
+ * Example: If drop_mask is 0xFF09 then sideband data is
+ * 8 bits bus number followed by 1 bit of device number and
+ * 1 bit function number. This means drop_mask=0xFF09 will
+ * convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).
+ */
+ while (drop_mask) {
+ if (drop_mask & 0x1) {
+ id |= ((rid & 0x1) << i);
+ i++;
+ }
+ rid = rid >> 1;
+ drop_mask = drop_mask >> 1;
+ }
+
+ return id;
+}
/**
* of_pci_map_rid - Translate a requester ID through a downstream mapping.
* @np: root complex device node.
@@ -304,11 +333,11 @@ EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
*
* Return: 0 on success or a standard error code on failure.
*/
-int of_pci_map_rid(struct device_node *np, u32 rid,
- const char *map_name, const char *map_mask_name,
+int of_pci_map_rid(struct device_node *np, u32 rid, const char *map_name,
+ const char *map_mask_name, const char *drop_mask_name,
struct device_node **target, u32 *id_out)
{
- u32 map_mask, masked_rid;
+ u32 map_mask, masked_rid, drop_mask;
int map_len;
const __be32 *map = NULL;
@@ -340,7 +369,20 @@ int of_pci_map_rid(struct device_node *np, u32 rid,
if (map_mask_name)
of_property_read_u32(np, map_mask_name, &map_mask);
+ /* The default is to select all bits. */
+ drop_mask = 0xffffffff;
+
+ /*
+ * Can be overridden by "{iommu,msi}-map-drop-mask" property.
+ * If of_property_read_u32() fails, the default is used.
+ */
+ if (drop_mask_name)
+ of_property_read_u32(np, drop_mask_name, &drop_mask);
+
masked_rid = map_mask & rid;
+
+ masked_rid = out_masked_rid(masked_rid, drop_mask);
+
for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) {
struct device_node *phandle_node;
u32 rid_base = be32_to_cpup(map + 0);
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 518c8d2..98937ec 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -20,7 +20,8 @@ int of_pci_get_max_link_speed(struct device_node *node);
void of_pci_check_probe_only(void);
int of_pci_map_rid(struct device_node *np, u32 rid,
const char *map_name, const char *map_mask_name,
- struct device_node **target, u32 *id_out);
+ const char *drop_mask_name, struct device_node **target,
+ u32 *id_out);
#else
static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
{
@@ -58,7 +59,8 @@ of_get_pci_domain_nr(struct device_node *node)
static inline int of_pci_map_rid(struct device_node *np, u32 rid,
const char *map_name, const char *map_mask_name,
- struct device_node **target, u32 *id_out)
+ const char *drop_mask_name, struct device_node **target,
+ u32 *id_out)
{
return -EINVAL;
}
--
2.7.4
On Fri, Jul 07, 2017 at 12:39:57PM +0530, Srinath Mannam wrote:
> These patches implements optional DT properties to generate
> smaller sideband data from RID which can be further mapped
> to MSI Device ID or Stream ID
>
> On some of the systems, sideband data is smaller than RID
> (16bits). For such system, sideband data has to be generated
> by dropping some of the RID bits
>
> the process of sideband data extracted from RID can be expressed
> using optional DT property {iommu/msi}-map-drop-mask.
>
> Example: If drop-mask is 0xFF09 then sideband data is
> 8 bits bus number followed by 1 bit of device number and
> 1 bit function number. This means drop-mask=0xFF09 will
> convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).
So IIUC, here's you're using this not only to mask bits out, but also to
determine a *shift* to apply to the value, implicitly provided by the
(contiguous) low bits of the mask.
That's really not obvious from the name.
Mark.
>
> Srinath Mannam (2):
> dt-bindings: pci: Add drop mask property for MSI and IOMMU
> pcie: sideband data by dropping RID bits
>
> .../devicetree/bindings/pci/pci-iommu.txt | 31 ++++++++++++++
> Documentation/devicetree/bindings/pci/pci-msi.txt | 33 +++++++++++++++
> drivers/iommu/of_iommu.c | 4 +-
> drivers/of/irq.c | 3 +-
> drivers/of/of_pci.c | 48 ++++++++++++++++++++--
> include/linux/of_pci.h | 6 ++-
> 6 files changed, 117 insertions(+), 8 deletions(-)
>
> --
> 2.7.4
>
On Fri, Jul 07, 2017 at 12:39:58PM +0530, Srinath Mannam wrote:
> This patch adds info about optional DT properties
> iommu-map-drop-mask and msi-map-drop-mask.
>
> A drop mask represents the bits which will be
> removed/dropped by system from Requester ID before
> mapping it to msi ID or stream ID.
>
> Signed-off-by: Anup Patel <[email protected]>
> Signed-off-by: Oza Pawandeep <[email protected]>
> Signed-off-by: Srinath Mannam <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> .../devicetree/bindings/pci/pci-iommu.txt | 31 ++++++++++++++++++++
> Documentation/devicetree/bindings/pci/pci-msi.txt | 33 ++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> index 0def586..499cb27 100644
> --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
> @@ -44,6 +44,9 @@ Optional properties
> - iommu-map-mask: A mask to be applied to each Requester ID prior to being
> mapped to an IOMMU specifier per the iommu-map property.
>
> +- iommu-map-drop-mask: A drop mask represents the bits which will be
> + removed/dropped by system from Requester ID before mapping it to
> + stream ID.
As mentioned in my reply to the cover letter, you're expecting this to
be handled as more than a mask, so this description is inadequate.
[...]
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + iommu: iommu@a {
> + reg = <0xa 0x1>;
> + compatible = "vendor,some-iommu";
> + #iommu-cells = <1>;
> + };
> +
> + pci: pci@f {
> + reg = <0xf 0x1>;
> + compatible = "vendor,pcie-root-complex";
> + device_type = "pci";
> +
> + /*
> + * The sideband data provided to the IOMMU is a 10bit
> + * data derived from the RID by dropping 4 MSBs
> + * of device number and 2 MSBs of function number.
> + */
> + iommu-map = <0x0 &iommu 0x0 0x1024>;
> + iommu-map-drop-mask = <0xff09>;
> + };
> +};
... as this this example.
Assuming this was truly a mask of bits to drop, you'd have:
RID -> SID
0xffff -> 0x00f6
... whereas from your cover letter it seems you want:
RID -> SID
0xffff -> 0x3f
... and I've just realsied you have non-coniguous masks, so this is even
worse.
> diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
> index 9b3cc81..1de3f39 100644
> --- a/Documentation/devicetree/bindings/pci/pci-msi.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
> @@ -49,6 +49,10 @@ Optional properties
> - msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
> to an msi-specifier per the msi-map property.
>
> +- msi-map-drop-mask: A drop mask represents the bits which will be
> + removed/dropped by system from Requester ID before mapping it to
> + msi ID.
> +
> - msi-parent: Describes the MSI parent of the root complex itself. Where
> the root complex and MSI controller do not pass sideband data with MSI
> writes, this property may be used to describe the MSI controller(s)
> @@ -218,3 +222,32 @@ Example (5)
> <0x0000 &msi_b 0x0000 0x10000>;
> };
> };
> +
> +Example (6)
> +===========
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + msi: msi-controller@a {
> + reg = <0xa 0x1>;
> + compatible = "vendor,some-controller";
> + msi-controller;
> + #msi-cells = <1>;
> + };
> +
> + pci: pci@f {
> + reg = <0xf 0x1>;
> + compatible = "vendor,pcie-root-complex";
> + device_type = "pci";
> +
> + /*
> + * The sideband data provided to the MSI controller is
> + * a 10bit data derived from the RID by dropping
> + * 4 MSBs of device number and 2 MSBs of function number.
> + */
> + msi-map = <0x0 &msi_a 0x0 0x100>,
> + msi-map-drop-mask = <0xff09>
> + };
> +};
... likewise on all counts.
Your mapping can be expressed today using a number of msi-map entries,
which you can easily generate programmatically with a trivial perl
script, without requiring a new binding or any new kernel code.
Please do that instead.
Thanks,
Mark.
On Fri, Jul 07, 2017 at 12:39:59PM +0530, Srinath Mannam wrote:
> The MSI Device ID or Stream ID are passed as sideband
> data on the SOC bus to which PCI RC is connected.
>
> If sideband data on SOC bus is less than 16bits then
> PCI RC will have to derive sideband data from RID by
> dropping selected bits.
>
> This patch implements optional DT properties to generate
> smaller sideband data from RID which can be further
> mapped to MSI Device ID or Stream ID.
>
> Sideband data generation from RID is done by dropping
> bits corresponding zero bits in {iommu/msi}-map-drop-mask.
>
> Example: If drop-mask is 0xFF09 then sideband data is
> 8 bits bus number followed by 1 bit of device number and
> 1 bit function number. This means drop-mask=0xFF09 will
> convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).
As mentioned on the prior two patches, I don't think this is a good
idea, especially given this isn't a trivial masking operation.
It's already possible to express this mapping using a number of entries
in the the *-map properties, which you can generate programmatically
with a script.
Please do that instead.
Thanks,
Mark.
>
> Signed-off-by: Anup Patel <[email protected]>
> Signed-off-by: Oza Pawandeep <[email protected]>
> Signed-off-by: Srinath Mannam <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> drivers/iommu/of_iommu.c | 4 ++--
> drivers/of/irq.c | 3 ++-
> drivers/of/of_pci.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
> include/linux/of_pci.h | 6 ++++--
> 4 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 19779b8..f179724 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -169,8 +169,8 @@ static const struct iommu_ops
> */
> iommu_spec.np = NULL;
> err = of_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
> - "iommu-map-mask", &iommu_spec.np,
> - iommu_spec.args);
> + "iommu-map-mask", "iommu-map-drop-mask",
> + &iommu_spec.np, iommu_spec.args);
> if (err)
> return err == -ENODEV ? NULL : ERR_PTR(err);
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index d11437c..454f47a 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -606,7 +606,8 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> */
> for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
> if (!of_pci_map_rid(parent_dev->of_node, rid_in, "msi-map",
> - "msi-map-mask", np, &rid_out))
> + "msi-map-mask", "msi-map-drop-mask", np,
> + &rid_out))
> break;
> return rid_out;
> }
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index c9d4d3a..a914bcf 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -285,6 +285,35 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
> EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
> #endif /* CONFIG_OF_ADDRESS */
>
> +static inline u32 out_masked_rid(u32 rid, u32 drop_mask)
> +{
> + u32 id = 0;
> + u32 i = 0;
> +
> + /* RID's BUS, DEV, FUN values not inside the mask are invalid */
> + if (rid & ~drop_mask)
> + return -EINVAL;
> +
> + /*
> + * RID value is translated to sideband data using drop_mask
> + * by dropping bits corresponding zero bits in drop_mask.
> + *
> + * Example: If drop_mask is 0xFF09 then sideband data is
> + * 8 bits bus number followed by 1 bit of device number and
> + * 1 bit function number. This means drop_mask=0xFF09 will
> + * convert RID=0x1a10 (16bits) to sideband data 0x6a (10bits).
> + */
> + while (drop_mask) {
> + if (drop_mask & 0x1) {
> + id |= ((rid & 0x1) << i);
> + i++;
> + }
> + rid = rid >> 1;
> + drop_mask = drop_mask >> 1;
> + }
> +
> + return id;
> +}
> /**
> * of_pci_map_rid - Translate a requester ID through a downstream mapping.
> * @np: root complex device node.
> @@ -304,11 +333,11 @@ EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
> *
> * Return: 0 on success or a standard error code on failure.
> */
> -int of_pci_map_rid(struct device_node *np, u32 rid,
> - const char *map_name, const char *map_mask_name,
> +int of_pci_map_rid(struct device_node *np, u32 rid, const char *map_name,
> + const char *map_mask_name, const char *drop_mask_name,
> struct device_node **target, u32 *id_out)
> {
> - u32 map_mask, masked_rid;
> + u32 map_mask, masked_rid, drop_mask;
> int map_len;
> const __be32 *map = NULL;
>
> @@ -340,7 +369,20 @@ int of_pci_map_rid(struct device_node *np, u32 rid,
> if (map_mask_name)
> of_property_read_u32(np, map_mask_name, &map_mask);
>
> + /* The default is to select all bits. */
> + drop_mask = 0xffffffff;
> +
> + /*
> + * Can be overridden by "{iommu,msi}-map-drop-mask" property.
> + * If of_property_read_u32() fails, the default is used.
> + */
> + if (drop_mask_name)
> + of_property_read_u32(np, drop_mask_name, &drop_mask);
> +
> masked_rid = map_mask & rid;
> +
> + masked_rid = out_masked_rid(masked_rid, drop_mask);
> +
> for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) {
> struct device_node *phandle_node;
> u32 rid_base = be32_to_cpup(map + 0);
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index 518c8d2..98937ec 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -20,7 +20,8 @@ int of_pci_get_max_link_speed(struct device_node *node);
> void of_pci_check_probe_only(void);
> int of_pci_map_rid(struct device_node *np, u32 rid,
> const char *map_name, const char *map_mask_name,
> - struct device_node **target, u32 *id_out);
> + const char *drop_mask_name, struct device_node **target,
> + u32 *id_out);
> #else
> static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> {
> @@ -58,7 +59,8 @@ of_get_pci_domain_nr(struct device_node *node)
>
> static inline int of_pci_map_rid(struct device_node *np, u32 rid,
> const char *map_name, const char *map_mask_name,
> - struct device_node **target, u32 *id_out)
> + const char *drop_mask_name, struct device_node **target,
> + u32 *id_out)
> {
> return -EINVAL;
> }
> --
> 2.7.4
>
On 07/07/17 14:30, Mark Rutland wrote:
> On Fri, Jul 07, 2017 at 12:39:58PM +0530, Srinath Mannam wrote:
>> This patch adds info about optional DT properties
>> iommu-map-drop-mask and msi-map-drop-mask.
>>
>> A drop mask represents the bits which will be
>> removed/dropped by system from Requester ID before
>> mapping it to msi ID or stream ID.
>>
>> Signed-off-by: Anup Patel <[email protected]>
>> Signed-off-by: Oza Pawandeep <[email protected]>
>> Signed-off-by: Srinath Mannam <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Reviewed-by: Scott Branden <[email protected]>
>> ---
>> .../devicetree/bindings/pci/pci-iommu.txt | 31 ++++++++++++++++++++
>> Documentation/devicetree/bindings/pci/pci-msi.txt | 33 ++++++++++++++++++++++
>> 2 files changed, 64 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
>> index 0def586..499cb27 100644
>> --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
>> @@ -44,6 +44,9 @@ Optional properties
>> - iommu-map-mask: A mask to be applied to each Requester ID prior to being
>> mapped to an IOMMU specifier per the iommu-map property.
>>
>> +- iommu-map-drop-mask: A drop mask represents the bits which will be
>> + removed/dropped by system from Requester ID before mapping it to
>> + stream ID.
>
> As mentioned in my reply to the cover letter, you're expecting this to
> be handled as more than a mask, so this description is inadequate.
>
> [...]
>
>> +/ {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + iommu: iommu@a {
>> + reg = <0xa 0x1>;
>> + compatible = "vendor,some-iommu";
>> + #iommu-cells = <1>;
>> + };
>> +
>> + pci: pci@f {
>> + reg = <0xf 0x1>;
>> + compatible = "vendor,pcie-root-complex";
>> + device_type = "pci";
>> +
>> + /*
>> + * The sideband data provided to the IOMMU is a 10bit
>> + * data derived from the RID by dropping 4 MSBs
>> + * of device number and 2 MSBs of function number.
>> + */
>> + iommu-map = <0x0 &iommu 0x0 0x1024>;
>> + iommu-map-drop-mask = <0xff09>;
>> + };
>> +};
>
> ... as this this example.
>
> Assuming this was truly a mask of bits to drop, you'd have:
>
> RID -> SID
> 0xffff -> 0x00f6
>
> ... whereas from your cover letter it seems you want:
>
> RID -> SID
> 0xffff -> 0x3f
>
> ... and I've just realsied you have non-coniguous masks, so this is even
> worse.
>
>> diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
>> index 9b3cc81..1de3f39 100644
>> --- a/Documentation/devicetree/bindings/pci/pci-msi.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
>> @@ -49,6 +49,10 @@ Optional properties
>> - msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
>> to an msi-specifier per the msi-map property.
>>
>> +- msi-map-drop-mask: A drop mask represents the bits which will be
>> + removed/dropped by system from Requester ID before mapping it to
>> + msi ID.
>> +
>> - msi-parent: Describes the MSI parent of the root complex itself. Where
>> the root complex and MSI controller do not pass sideband data with MSI
>> writes, this property may be used to describe the MSI controller(s)
>> @@ -218,3 +222,32 @@ Example (5)
>> <0x0000 &msi_b 0x0000 0x10000>;
>> };
>> };
>> +
>> +Example (6)
>> +===========
>> +
>> +/ {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + msi: msi-controller@a {
>> + reg = <0xa 0x1>;
>> + compatible = "vendor,some-controller";
>> + msi-controller;
>> + #msi-cells = <1>;
>> + };
>> +
>> + pci: pci@f {
>> + reg = <0xf 0x1>;
>> + compatible = "vendor,pcie-root-complex";
>> + device_type = "pci";
>> +
>> + /*
>> + * The sideband data provided to the MSI controller is
>> + * a 10bit data derived from the RID by dropping
>> + * 4 MSBs of device number and 2 MSBs of function number.
>> + */
>> + msi-map = <0x0 &msi_a 0x0 0x100>,
>> + msi-map-drop-mask = <0xff09>
>> + };
>> +};
>
> ... likewise on all counts.
>
> Your mapping can be expressed today using a number of msi-map entries,
> which you can easily generate programmatically with a trivial perl
> script, without requiring a new binding or any new kernel code.
>
> Please do that instead.
Indeed. The systems I'm aware of which need to express non-trivial RID
to SID mappings tend to have the bootloader probe PCI and dynamically
generate map entries per discovered RID, but even if you wanted to
statically generate the whole lot for the worst-case bus range that's
still only 512 entries, which is not unmanageable. Notably, it's also
what would have to be done (in equivalent) for IORT, although I assume
this is an embedded platform for which nobody cares about ACPI.
Robin.
Hi Robin,
On 17-07-07 07:55 AM, Robin Murphy wrote:
> On 07/07/17 14:30, Mark Rutland wrote:
>> On Fri, Jul 07, 2017 at 12:39:58PM +0530, Srinath Mannam wrote:
>>> This patch adds info about optional DT properties
>>> iommu-map-drop-mask and msi-map-drop-mask.
>>>
>>> A drop mask represents the bits which will be
>>> removed/dropped by system from Requester ID before
>>> mapping it to msi ID or stream ID.
>>>
>>> Signed-off-by: Anup Patel <[email protected]>
>>> Signed-off-by: Oza Pawandeep <[email protected]>
>>> Signed-off-by: Srinath Mannam <[email protected]>
>>> Reviewed-by: Ray Jui <[email protected]>
>>> Reviewed-by: Scott Branden <[email protected]>
>>> ---
>>> .../devicetree/bindings/pci/pci-iommu.txt | 31 ++++++++++++++++++++
>>> Documentation/devicetree/bindings/pci/pci-msi.txt | 33 ++++++++++++++++++++++
>>> 2 files changed, 64 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>> index 0def586..499cb27 100644
>>> --- a/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>> +++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
>>> @@ -44,6 +44,9 @@ Optional properties
>>> - iommu-map-mask: A mask to be applied to each Requester ID prior to being
>>> mapped to an IOMMU specifier per the iommu-map property.
>>>
>>> +- iommu-map-drop-mask: A drop mask represents the bits which will be
>>> + removed/dropped by system from Requester ID before mapping it to
>>> + stream ID.
>> As mentioned in my reply to the cover letter, you're expecting this to
>> be handled as more than a mask, so this description is inadequate.
>>
>> [...]
>>
>>> +/ {
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> +
>>> + iommu: iommu@a {
>>> + reg = <0xa 0x1>;
>>> + compatible = "vendor,some-iommu";
>>> + #iommu-cells = <1>;
>>> + };
>>> +
>>> + pci: pci@f {
>>> + reg = <0xf 0x1>;
>>> + compatible = "vendor,pcie-root-complex";
>>> + device_type = "pci";
>>> +
>>> + /*
>>> + * The sideband data provided to the IOMMU is a 10bit
>>> + * data derived from the RID by dropping 4 MSBs
>>> + * of device number and 2 MSBs of function number.
>>> + */
>>> + iommu-map = <0x0 &iommu 0x0 0x1024>;
>>> + iommu-map-drop-mask = <0xff09>;
>>> + };
>>> +};
>> ... as this this example.
>>
>> Assuming this was truly a mask of bits to drop, you'd have:
>>
>> RID -> SID
>> 0xffff -> 0x00f6
>>
>> ... whereas from your cover letter it seems you want:
>>
>> RID -> SID
>> 0xffff -> 0x3f
>>
>> ... and I've just realsied you have non-coniguous masks, so this is even
>> worse.
>>
>>> diff --git a/Documentation/devicetree/bindings/pci/pci-msi.txt b/Documentation/devicetree/bindings/pci/pci-msi.txt
>>> index 9b3cc81..1de3f39 100644
>>> --- a/Documentation/devicetree/bindings/pci/pci-msi.txt
>>> +++ b/Documentation/devicetree/bindings/pci/pci-msi.txt
>>> @@ -49,6 +49,10 @@ Optional properties
>>> - msi-map-mask: A mask to be applied to each Requester ID prior to being mapped
>>> to an msi-specifier per the msi-map property.
>>>
>>> +- msi-map-drop-mask: A drop mask represents the bits which will be
>>> + removed/dropped by system from Requester ID before mapping it to
>>> + msi ID.
>>> +
>>> - msi-parent: Describes the MSI parent of the root complex itself. Where
>>> the root complex and MSI controller do not pass sideband data with MSI
>>> writes, this property may be used to describe the MSI controller(s)
>>> @@ -218,3 +222,32 @@ Example (5)
>>> <0x0000 &msi_b 0x0000 0x10000>;
>>> };
>>> };
>>> +
>>> +Example (6)
>>> +===========
>>> +
>>> +/ {
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> +
>>> + msi: msi-controller@a {
>>> + reg = <0xa 0x1>;
>>> + compatible = "vendor,some-controller";
>>> + msi-controller;
>>> + #msi-cells = <1>;
>>> + };
>>> +
>>> + pci: pci@f {
>>> + reg = <0xf 0x1>;
>>> + compatible = "vendor,pcie-root-complex";
>>> + device_type = "pci";
>>> +
>>> + /*
>>> + * The sideband data provided to the MSI controller is
>>> + * a 10bit data derived from the RID by dropping
>>> + * 4 MSBs of device number and 2 MSBs of function number.
>>> + */
>>> + msi-map = <0x0 &msi_a 0x0 0x100>,
>>> + msi-map-drop-mask = <0xff09>
>>> + };
>>> +};
>> ... likewise on all counts.
>>
>> Your mapping can be expressed today using a number of msi-map entries,
>> which you can easily generate programmatically with a trivial perl
>> script, without requiring a new binding or any new kernel code.
>>
>> Please do that instead.
> Indeed. The systems I'm aware of which need to express non-trivial RID
> to SID mappings tend to have the bootloader probe PCI and dynamically
> generate map entries per discovered RID, but even if you wanted to
> statically generate the whole lot for the worst-case bus range that's
> still only 512 entries, which is not unmanageable. Notably, it's also
> what would have to be done (in equivalent) for IORT, although I assume
> this is an embedded platform for which nobody cares about ACPI.
Actually we will care about ACPI and need to add it (doesn't need to be
in this patchet unless easy to do so...)
>
> Robin.
On 07/07/17 16:22, Scott Branden wrote:
> Hi Robin,
>
> On 17-07-07 07:55 AM, Robin Murphy wrote:
>> On 07/07/17 14:30, Mark Rutland wrote:
[...]
>>> Your mapping can be expressed today using a number of msi-map entries,
>>> which you can easily generate programmatically with a trivial perl
>>> script, without requiring a new binding or any new kernel code.
>>>
>>> Please do that instead.
>> Indeed. The systems I'm aware of which need to express non-trivial RID
>> to SID mappings tend to have the bootloader probe PCI and dynamically
>> generate map entries per discovered RID, but even if you wanted to
>> statically generate the whole lot for the worst-case bus range that's
>> still only 512 entries, which is not unmanageable. Notably, it's also
>> what would have to be done (in equivalent) for IORT, although I assume
>> this is an embedded platform for which nobody cares about ACPI.
> Actually we will care about ACPI and need to add it (doesn't need to be
> in this patchet unless easy to do so...)
Ah, OK, that's an even stronger argument for not adding anything new
then - DT "iommu-map" is already marginally more expressive than IORT ID
mappings can be, so there doesn't seem to be much justification for
diverging them further.
Robin.
On Fri, Jul 07, 2017 at 08:22:21AM -0700, Scott Branden wrote:
> On 17-07-07 07:55 AM, Robin Murphy wrote:
> >On 07/07/17 14:30, Mark Rutland wrote:
> >>On Fri, Jul 07, 2017 at 12:39:58PM +0530, Srinath Mannam wrote:
> >>>+Example (6)
> >>>+===========
> >>>+
> >>>+/ {
> >>>+ #address-cells = <1>;
> >>>+ #size-cells = <1>;
> >>>+
> >>>+ msi: msi-controller@a {
> >>>+ reg = <0xa 0x1>;
> >>>+ compatible = "vendor,some-controller";
> >>>+ msi-controller;
> >>>+ #msi-cells = <1>;
> >>>+ };
> >>>+
> >>>+ pci: pci@f {
> >>>+ reg = <0xf 0x1>;
> >>>+ compatible = "vendor,pcie-root-complex";
> >>>+ device_type = "pci";
> >>>+
> >>>+ /*
> >>>+ * The sideband data provided to the MSI controller is
> >>>+ * a 10bit data derived from the RID by dropping
> >>>+ * 4 MSBs of device number and 2 MSBs of function number.
> >>>+ */
> >>>+ msi-map = <0x0 &msi_a 0x0 0x100>,
> >>>+ msi-map-drop-mask = <0xff09>
> >>>+ };
> >>>+};
> >>... likewise on all counts.
> >>
> >>Your mapping can be expressed today using a number of msi-map entries,
> >>which you can easily generate programmatically with a trivial perl
> >>script, without requiring a new binding or any new kernel code.
> >>
> >>Please do that instead.
> >
> >Indeed. The systems I'm aware of which need to express non-trivial RID
> >to SID mappings tend to have the bootloader probe PCI and dynamically
> >generate map entries per discovered RID, but even if you wanted to
> >statically generate the whole lot for the worst-case bus range that's
> >still only 512 entries, which is not unmanageable. Notably, it's also
> >what would have to be done (in equivalent) for IORT, although I assume
> >this is an embedded platform for which nobody cares about ACPI.
>
> Actually we will care about ACPI and need to add it (doesn't need to
> be in this patchet unless easy to do so...)
Similarly to what I said for the DT case, with IORT you can solve this
today by using multiple ID mapping entries in a node's ID mappings
array.
I don't imagine the sort of change you are proposing will sail into the
IORT spec.
Thanks,
Mark.