* [PATCH v7 05/15] irqchip: gicv3-its: platform-msi: refactor its_pmsi_prepare()
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
Adding ACPI support for platform MSI, we need to retrieve the
dev id in ACPI way instead of device tree, we already have
a well formed function its_pmsi_prepare() to get the dev id
but it's OF dependent, so collect OF related code and put them
into a single function to make its_pmsi_prepare() more friendly
to ACPI later.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Matthias Brugger <mbrugger@suse.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/irqchip/irq-gic-v3-its-platform-msi.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index 470b4aa..3c94278 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -24,15 +24,11 @@
.name = "ITS-pMSI",
};
-static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
- int nvec, msi_alloc_info_t *info)
+static int of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev,
+ u32 *dev_id)
{
- struct msi_domain_info *msi_info;
- u32 dev_id;
int ret, index = 0;
- msi_info = msi_get_domain_info(domain->parent);
-
/* Suck the DeviceID out of the msi-parent property */
do {
struct of_phandle_args args;
@@ -43,11 +39,24 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
if (args.np == irq_domain_get_of_node(domain)) {
if (WARN_ON(args.args_count != 1))
return -EINVAL;
- dev_id = args.args[0];
+ *dev_id = args.args[0];
break;
}
} while (!ret);
+ return ret;
+}
+
+static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
+ int nvec, msi_alloc_info_t *info)
+{
+ struct msi_domain_info *msi_info;
+ u32 dev_id;
+ int ret;
+
+ msi_info = msi_get_domain_info(domain->parent);
+
+ ret = of_pmsi_get_dev_id(domain, dev, &dev_id);
if (ret)
return ret;
--
1.9.1
^ permalink raw reply related
* [PATCH v7 06/15] irqchip: gicv3-its: platform-msi: refactor its_pmsi_init() to prepare for ACPI
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
Introduce its_pmsi_init_one() to refactor the code to isolate
ACPI&DT common code to prepare for ACPI later.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Tomasz Nowicki <tn@semihalf.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/irq-gic-v3-its-platform-msi.c | 45 ++++++++++++++++-----------
1 file changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index 3c94278..3d9efd1 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -82,34 +82,43 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
{},
};
-static int __init its_pmsi_init(void)
+static int __init its_pmsi_init_one(struct fwnode_handle *fwnode,
+ const char *name)
{
- struct device_node *np;
struct irq_domain *parent;
+ parent = irq_find_matching_fwnode(fwnode, DOMAIN_BUS_NEXUS);
+ if (!parent || !msi_get_domain_info(parent)) {
+ pr_err("%s: unable to locate ITS domain\n", name);
+ return -ENXIO;
+ }
+
+ if (!platform_msi_create_irq_domain(fwnode, &its_pmsi_domain_info,
+ parent)) {
+ pr_err("%s: unable to create platform domain\n", name);
+ return -ENXIO;
+ }
+
+ pr_info("Platform MSI: %s domain created\n", name);
+ return 0;
+}
+
+static void __init its_pmsi_of_init(void)
+{
+ struct device_node *np;
+
for (np = of_find_matching_node(NULL, its_device_id); np;
np = of_find_matching_node(np, its_device_id)) {
if (!of_property_read_bool(np, "msi-controller"))
continue;
- parent = irq_find_matching_host(np, DOMAIN_BUS_NEXUS);
- if (!parent || !msi_get_domain_info(parent)) {
- pr_err("%s: unable to locate ITS domain\n",
- np->full_name);
- continue;
- }
-
- if (!platform_msi_create_irq_domain(of_node_to_fwnode(np),
- &its_pmsi_domain_info,
- parent)) {
- pr_err("%s: unable to create platform domain\n",
- np->full_name);
- continue;
- }
-
- pr_info("Platform MSI: %s domain created\n", np->full_name);
+ its_pmsi_init_one(of_node_to_fwnode(np), np->full_name);
}
+}
+static int __init its_pmsi_init(void)
+{
+ its_pmsi_of_init();
return 0;
}
early_initcall(its_pmsi_init);
--
1.9.1
^ permalink raw reply related
* [PATCH v7 07/15] irqchip: gicv3-its: platform-msi: scan MADT to create platform msi domain
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
With the introduction of its_pmsi_init_one(), we can add some code
on top for ACPI support of platform MSI.
We are scanning the MADT table to get the ITS entry(ies), then use
the information to create the platform msi domain for devices connect
to it, just like the PCI MSI for ITS did.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Tomasz Nowicki <tn@semihalf.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/irq-gic-v3-its-platform-msi.c | 36 +++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index 3d9efd1..ebe933e 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -103,6 +103,41 @@ static int __init its_pmsi_init_one(struct fwnode_handle *fwnode,
return 0;
}
+#ifdef CONFIG_ACPI
+static int __init
+its_pmsi_parse_madt(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_translator *its_entry;
+ struct fwnode_handle *domain_handle;
+ const char *node_name;
+ int err = -ENXIO;
+
+ its_entry = (struct acpi_madt_generic_translator *)header;
+ node_name = kasprintf(GFP_KERNEL, "ITS at 0x%lx",
+ (long)its_entry->base_address);
+ domain_handle = iort_find_domain_token(its_entry->translation_id);
+ if (!domain_handle) {
+ pr_err("%s: Unable to locate ITS domain handle\n", node_name);
+ goto out;
+ }
+
+ err = its_pmsi_init_one(domain_handle, node_name);
+
+out:
+ kfree(node_name);
+ return err;
+}
+
+static void __init its_pmsi_acpi_init(void)
+{
+ acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+ its_pmsi_parse_madt, 0);
+}
+#else
+static inline void its_pmsi_acpi_init(void) { }
+#endif
+
static void __init its_pmsi_of_init(void)
{
struct device_node *np;
@@ -119,6 +154,7 @@ static void __init its_pmsi_of_init(void)
static int __init its_pmsi_init(void)
{
its_pmsi_of_init();
+ its_pmsi_acpi_init();
return 0;
}
early_initcall(its_pmsi_init);
--
1.9.1
^ permalink raw reply related
* [PATCH v7 08/15] ACPI: IORT: rename iort_node_map_rid() to make it generic
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
iort_node_map_rid() was designed for both PCI and platform
device, but the rid means requester id is for ITS mappings,
rename iort_node_map_rid() to iort_node_map_id() and update
its argument names to make it more generic.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Tomasz Nowicki <tn@semihalf.com>
---
drivers/acpi/arm64/iort.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 208eac9..069a690 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -355,11 +355,11 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
return NULL;
}
-static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
- u32 rid_in, u32 *rid_out,
- u8 type_mask)
+static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
+ u32 id_in, u32 *id_out,
+ u8 type_mask)
{
- u32 rid = rid_in;
+ u32 id = id_in;
/* Parse the ID mapping tree to find specified node type */
while (node) {
@@ -367,8 +367,8 @@ static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
int i;
if (IORT_TYPE_MASK(node->type) & type_mask) {
- if (rid_out)
- *rid_out = rid;
+ if (id_out)
+ *id_out = id;
return node;
}
@@ -385,9 +385,9 @@ static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
goto fail_map;
}
- /* Do the RID translation */
+ /* Do the ID translation */
for (i = 0; i < node->mapping_count; i++, map++) {
- if (!iort_id_map(map, node->type, rid, &rid))
+ if (!iort_id_map(map, node->type, id, &id))
break;
}
@@ -399,9 +399,9 @@ static struct acpi_iort_node *iort_node_map_rid(struct acpi_iort_node *node,
}
fail_map:
- /* Map input RID to output RID unchanged on mapping failure*/
- if (rid_out)
- *rid_out = rid_in;
+ /* Map input ID to output ID unchanged on mapping failure */
+ if (id_out)
+ *id_out = id_in;
return NULL;
}
@@ -439,7 +439,7 @@ u32 iort_msi_map_rid(struct device *dev, u32 req_id)
if (!node)
return req_id;
- iort_node_map_rid(node, req_id, &dev_id, IORT_MSI_TYPE);
+ iort_node_map_id(node, req_id, &dev_id, IORT_MSI_TYPE);
return dev_id;
}
@@ -462,7 +462,7 @@ static int iort_dev_find_its_id(struct device *dev, u32 req_id,
if (!node)
return -ENXIO;
- node = iort_node_map_rid(node, req_id, NULL, IORT_MSI_TYPE);
+ node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE);
if (!node)
return -ENXIO;
@@ -591,8 +591,8 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
if (!node)
return NULL;
- parent = iort_node_map_rid(node, rid, &streamid,
- IORT_IOMMU_TYPE);
+ parent = iort_node_map_id(node, rid, &streamid,
+ IORT_IOMMU_TYPE);
ops = iort_iommu_xlate(dev, parent, streamid);
--
1.9.1
^ permalink raw reply related
* [PATCH v7 09/15] ACPI: platform-msi: retrieve dev id from IORT
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
For devices connecting to ITS, it needs dev id to identify itself, and
this dev id is represented in the IORT table in named component node
[1] for platform devices, so in this patch we will scan the IORT to
retrieve device's dev id.
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), this
is needed for use cases such as NC (named component) -> SMMU -> ITS
mappings.
we have API iort_node_get_id() for step (1) above and
iort_node_map_rid() for step (2), so create a wrapper
iort_node_map_platform_id() to retrieve the dev id.
[1]: https://static.docs.arm.com/den0049/b/DEN0049B_IO_Remapping_Table.pdf
Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Suggested-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Tomasz Nowicki <tn@semihalf.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/acpi/arm64/iort.c | 56 +++++++++++++++++++++++++++
drivers/irqchip/irq-gic-v3-its-platform-msi.c | 4 +-
include/linux/acpi_iort.h | 8 ++++
3 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 069a690..95fd20b 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -30,6 +30,7 @@
#define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP)
#define IORT_IOMMU_TYPE ((1 << ACPI_IORT_NODE_SMMU) | \
(1 << ACPI_IORT_NODE_SMMU_V3))
+#define IORT_TYPE_ANY (IORT_MSI_TYPE | IORT_IOMMU_TYPE)
struct iort_its_msi_chip {
struct list_head list;
@@ -406,6 +407,34 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node,
return NULL;
}
+static
+struct acpi_iort_node *iort_node_map_platform_id(struct acpi_iort_node *node,
+ u32 *id_out, u8 type_mask,
+ int index)
+{
+ struct acpi_iort_node *parent;
+ u32 id;
+
+ /* step 1: retrieve the initial dev id */
+ parent = iort_node_get_id(node, &id, IORT_TYPE_ANY, index);
+ if (!parent)
+ return NULL;
+
+ /*
+ * optional step 2: map the initial dev id if its parent is not
+ * the target type we wanted, map it again for the use cases such
+ * as NC (named component) -> SMMU -> ITS. If the type is matched,
+ * return the parent pointer directly.
+ */
+ if (!(IORT_TYPE_MASK(parent->type) & type_mask))
+ parent = iort_node_map_id(parent, id, id_out, type_mask);
+ else
+ if (id_out)
+ *id_out = id;
+
+ return parent;
+}
+
static struct acpi_iort_node *iort_find_dev_node(struct device *dev)
{
struct pci_bus *pbus;
@@ -444,6 +473,33 @@ 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)
+{
+ int i;
+ struct acpi_iort_node *node;
+
+ if (!iort_table)
+ return -ENODEV;
+
+ node = iort_find_dev_node(dev);
+ if (!node)
+ return -ENODEV;
+
+ for (i = 0; i < node->mapping_count; i++) {
+ if(iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i))
+ return 0;
+ }
+
+ return -ENODEV;
+}
+
+/**
* iort_dev_find_its_id() - Find the ITS identifier for a device
* @dev: The device.
* @req_id: Device's requester ID
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index ebe933e..e801fc0 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -15,6 +15,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include <linux/acpi_iort.h>
#include <linux/device.h>
#include <linux/msi.h>
#include <linux/of.h>
@@ -56,7 +57,8 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
msi_info = msi_get_domain_info(domain->parent);
- ret = of_pmsi_get_dev_id(domain, dev, &dev_id);
+ ret = dev->of_node ? of_pmsi_get_dev_id(domain, dev, &dev_id) :
+ iort_pmsi_get_dev_id(dev, &dev_id);
if (ret)
return ret;
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 77e0809..ef99fd52 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -33,6 +33,7 @@
void acpi_iort_init(void);
bool iort_node_match(u8 type);
u32 iort_msi_map_rid(struct device *dev, u32 req_id);
+int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
/* IOMMU interface */
void iort_set_dma_mask(struct device *dev);
@@ -42,9 +43,16 @@ static inline void acpi_iort_init(void) { }
static inline bool iort_node_match(u8 type) { return false; }
static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
{ return req_id; }
+
static inline struct irq_domain *iort_get_device_domain(struct device *dev,
u32 req_id)
{ return NULL; }
+
+static inline int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
+{
+ return -ENODEV;
+}
+
/* IOMMU interface */
static inline void iort_set_dma_mask(struct device *dev) { }
static inline
--
1.9.1
^ permalink raw reply related
* [PATCH v7 10/15] ACPI: IORT: move over to iort_node_map_platform_id()
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
iort_node_map_platform_id() includes the function of
iort_node_get_id(), so update current iort_node_get_id()
users and move them over to iort_node_map_platform_id().
type handing in iort_node_get_id() is duplicate with
iort_node_map_platform_id(), so clean up a little
bit to make the code simpler.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Tomasz Nowicki <tn@semihalf.com>
---
drivers/acpi/arm64/iort.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 95fd20b..a6c8d2d 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -30,7 +30,6 @@
#define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP)
#define IORT_IOMMU_TYPE ((1 << ACPI_IORT_NODE_SMMU) | \
(1 << ACPI_IORT_NODE_SMMU_V3))
-#define IORT_TYPE_ANY (IORT_MSI_TYPE | IORT_IOMMU_TYPE)
struct iort_its_msi_chip {
struct list_head list;
@@ -319,8 +318,7 @@ static int iort_id_map(struct acpi_iort_id_mapping *map, u8 type, u32 rid_in,
static
struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
- u32 *id_out, u8 type_mask,
- int index)
+ u32 *id_out, int index)
{
struct acpi_iort_node *parent;
struct acpi_iort_id_mapping *map;
@@ -342,9 +340,6 @@ struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
map->output_reference);
- if (!(IORT_TYPE_MASK(parent->type) & type_mask))
- return NULL;
-
if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
@@ -416,7 +411,7 @@ struct acpi_iort_node *iort_node_map_platform_id(struct acpi_iort_node *node,
u32 id;
/* step 1: retrieve the initial dev id */
- parent = iort_node_get_id(node, &id, IORT_TYPE_ANY, index);
+ parent = iort_node_get_id(node, &id, index);
if (!parent)
return NULL;
@@ -424,7 +419,7 @@ struct acpi_iort_node *iort_node_map_platform_id(struct acpi_iort_node *node,
* optional step 2: map the initial dev id if its parent is not
* the target type we wanted, map it again for the use cases such
* as NC (named component) -> SMMU -> ITS. If the type is matched,
- * return the parent pointer directly.
+ * return the initial dev id and its parent pointer directly.
*/
if (!(IORT_TYPE_MASK(parent->type) & type_mask))
parent = iort_node_map_id(parent, id, id_out, type_mask);
@@ -660,14 +655,15 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
if (!node)
return NULL;
- parent = iort_node_get_id(node, &streamid,
- IORT_IOMMU_TYPE, i++);
+ parent = iort_node_map_platform_id(node, &streamid,
+ IORT_IOMMU_TYPE, i++);
while (parent) {
ops = iort_iommu_xlate(dev, parent, streamid);
- parent = iort_node_get_id(node, &streamid,
- IORT_IOMMU_TYPE, i++);
+ parent = iort_node_map_platform_id(node, &streamid,
+ IORT_IOMMU_TYPE,
+ i++);
}
}
--
1.9.1
^ permalink raw reply related
* [PATCH v7 11/15] ACPI: platform: setup MSI domain for ACPI based platform device
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
With the platform msi domain created, we can set up the msi domain
for a platform device when it's probed.
In order to do that, we need to get the domain that the platform
device connecting to, so the iort_get_platform_device_domain() is
introduced to retrieve the domain from iort.
After the domain is retrieved, we need a proper way to set the
domain to paltform device, as some platform devices such as an
irqchip needs the msi irqdomain to be the interrupt parent domain,
we need to get irqdomain before platform device is probed but after
the platform device is allocated (the time slot of setting the
msi domain also works for other cases). So simply call
acpi_configure_pmsi_domain() in acpi_platform_notify() for
platform devices will work.
Acked-by: Rafael J. Wysocki <rjw@rjwysocki.net> [for glue.c]
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Tomasz Nowicki <tn@semihalf.com>
---
drivers/acpi/arm64/iort.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/glue.c | 6 ++++++
include/linux/acpi_iort.h | 3 +++
3 files changed, 59 insertions(+)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index a6c8d2d..f5a32d0 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -551,6 +551,56 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
}
+/**
+ * iort_get_platform_device_domain() - Find MSI domain related to a
+ * platform device
+ * @dev: the dev pointer associated with the platform device
+ *
+ * Returns: the MSI domain for this device, NULL otherwise
+ */
+static struct irq_domain *iort_get_platform_device_domain(struct device *dev)
+{
+ struct acpi_iort_node *node, *msi_parent;
+ struct fwnode_handle *iort_fwnode;
+ struct acpi_iort_its_group *its;
+ int i;
+
+ /* find its associated iort node */
+ node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+ iort_match_node_callback, dev);
+ if (!node)
+ return NULL;
+
+ /* then find its msi parent node */
+ for (i = 0; i < node->mapping_count; i++) {
+ msi_parent = iort_node_map_platform_id(node, NULL,
+ IORT_MSI_TYPE, i);
+ if (msi_parent)
+ break;
+ }
+
+ if (!msi_parent)
+ return NULL;
+
+ /* Move to ITS specific data */
+ its = (struct acpi_iort_its_group *)msi_parent->node_data;
+
+ iort_fwnode = iort_find_domain_token(its->identifiers[0]);
+ if (!iort_fwnode)
+ return NULL;
+
+ return irq_find_matching_fwnode(iort_fwnode, DOMAIN_BUS_PLATFORM_MSI);
+}
+
+void acpi_configure_pmsi_domain(struct device *dev)
+{
+ struct irq_domain *msi_domain;
+
+ msi_domain = iort_get_platform_device_domain(dev);
+ if (msi_domain)
+ dev_set_msi_domain(dev, msi_domain);
+}
+
static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
{
u32 *rid = data;
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index fb19e1c..ec31b43 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -6,6 +6,8 @@
*
* This file is released under the GPLv2.
*/
+
+#include <linux/acpi_iort.h>
#include <linux/export.h>
#include <linux/init.h>
#include <linux/list.h>
@@ -14,6 +16,7 @@
#include <linux/rwsem.h>
#include <linux/acpi.h>
#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
#include "internal.h"
@@ -322,6 +325,9 @@ static int acpi_platform_notify(struct device *dev)
if (!adev)
goto out;
+ if (dev->bus == &platform_bus_type)
+ acpi_configure_pmsi_domain(dev);
+
if (type && type->setup)
type->setup(dev);
else if (adev->handler && adev->handler->bind)
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index ef99fd52..33f5ac3 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -38,6 +38,7 @@
/* IOMMU interface */
void iort_set_dma_mask(struct device *dev);
const struct iommu_ops *iort_iommu_configure(struct device *dev);
+void acpi_configure_pmsi_domain(struct device *dev);
#else
static inline void acpi_iort_init(void) { }
static inline bool iort_node_match(u8 type) { return false; }
@@ -58,6 +59,8 @@ static inline void iort_set_dma_mask(struct device *dev) { }
static inline
const struct iommu_ops *iort_iommu_configure(struct device *dev)
{ return NULL; }
+
+static inline void acpi_configure_pmsi_domain(struct device *dev) { }
#endif
#define IORT_ACPI_DECLARE(name, table_id, fn) \
--
1.9.1
^ permalink raw reply related
* [PATCH v7 12/15] msi: platform: make platform_msi_create_device_domain() ACPI aware
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
platform_msi_create_device_domain() is used to ctreate
irqdomain for the device such as irqchip mbigen generating
the MSIs, it's almost ready for ACPI use except
of_node_to_fwnode() is for dt only, make it ACPI aware then
things will work in both DTS and ACPI.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Greg KH <gregkh@linuxfoundation.org>
---
drivers/base/platform-msi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/base/platform-msi.c b/drivers/base/platform-msi.c
index be6a599..035ca3b 100644
--- a/drivers/base/platform-msi.c
+++ b/drivers/base/platform-msi.c
@@ -345,8 +345,7 @@ struct irq_domain *
data->host_data = host_data;
domain = irq_domain_create_hierarchy(dev->msi_domain, 0, nvec,
- of_node_to_fwnode(dev->of_node),
- ops, data);
+ dev->fwnode, ops, data);
if (!domain)
goto free_priv;
--
1.9.1
^ permalink raw reply related
* [PATCH v7 13/15] irqchip: mbigen: drop module owner
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
From: Kefeng Wang <wangkefeng.wang@huawei.com>
Module owner will be set by driver core, so drop it.
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Ma Jun <majun258@huawei.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/irq-mbigen.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 03b79b0..c01ab41 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -293,7 +293,6 @@ static int mbigen_device_probe(struct platform_device *pdev)
static struct platform_driver mbigen_platform_driver = {
.driver = {
.name = "Hisilicon MBIGEN-V2",
- .owner = THIS_MODULE,
.of_match_table = mbigen_of_match,
},
.probe = mbigen_device_probe,
--
1.9.1
^ permalink raw reply related
* [PATCH v7 14/15] irqchip: mbigen: introduce mbigen_of_create_domain()
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
From: Kefeng Wang <wangkefeng.wang@huawei.com>
Introduce mbigen_of_create_domain() to consolidate OF related
code and prepare for ACPI later, no funtional change.
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Ma Jun <majun258@huawei.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/irq-mbigen.c | 42 +++++++++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 15 deletions(-)
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index c01ab41..4e11da5 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -236,27 +236,15 @@ static int mbigen_irq_domain_alloc(struct irq_domain *domain,
.free = irq_domain_free_irqs_common,
};
-static int mbigen_device_probe(struct platform_device *pdev)
+static int mbigen_of_create_domain(struct platform_device *pdev,
+ struct mbigen_device *mgn_chip)
{
- struct mbigen_device *mgn_chip;
+ struct device *parent;
struct platform_device *child;
struct irq_domain *domain;
struct device_node *np;
- struct device *parent;
- struct resource *res;
u32 num_pins;
- mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
- if (!mgn_chip)
- return -ENOMEM;
-
- mgn_chip->pdev = pdev;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- mgn_chip->base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(mgn_chip->base))
- return PTR_ERR(mgn_chip->base);
-
for_each_child_of_node(pdev->dev.of_node, np) {
if (!of_property_read_bool(np, "interrupt-controller"))
continue;
@@ -280,6 +268,30 @@ static int mbigen_device_probe(struct platform_device *pdev)
return -ENOMEM;
}
+ return 0;
+}
+
+static int mbigen_device_probe(struct platform_device *pdev)
+{
+ struct mbigen_device *mgn_chip;
+ struct resource *res;
+ int err;
+
+ mgn_chip = devm_kzalloc(&pdev->dev, sizeof(*mgn_chip), GFP_KERNEL);
+ if (!mgn_chip)
+ return -ENOMEM;
+
+ mgn_chip->pdev = pdev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ mgn_chip->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+ if (IS_ERR(mgn_chip->base))
+ return PTR_ERR(mgn_chip->base);
+
+ err = mbigen_of_create_domain(pdev, mgn_chip);
+ if (err)
+ return err;
+
platform_set_drvdata(pdev, mgn_chip);
return 0;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v7 15/15] irqchip: mbigen: Add ACPI support
From: Hanjun Guo @ 2017-01-11 15:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484147199-4267-1-git-send-email-hanjun.guo@linaro.org>
With the preparation of platform msi support and interrupt producer
in DSDT, we can add mbigen ACPI support now.
We are using _PRS methd to indicate number of irq pins instead
of num_pins in DT to avoid _DSD usage in this case.
For mbi-gen,
Device(MBI0) {
Name(_HID, "HISI0152")
Name(_UID, Zero)
Name(_CRS, ResourceTemplate() {
Memory32Fixed(ReadWrite, 0xa0080000, 0x10000)
})
Name (_PRS, ResourceTemplate() {
Interrupt(ResourceProducer,...) {12,14,....}
})
}
For devices,
Device(COM0) {
Name(_HID, "ACPIIDxx")
Name(_UID, Zero)
Name(_CRS, ResourceTemplate() {
Memory32Fixed(ReadWrite, 0xb0030000, 0x10000)
Interrupt(ResourceConsumer,..., "\_SB.MBI0") {12}
})
}
With the helpe of platform msi and interrupt producer, then devices
will get the virq from mbi-gen's irqdomain.
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Ma Jun <majun258@huawei.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/irq-mbigen.c | 70 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 67 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 4e11da5..17d35fa 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -16,6 +16,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include <linux/acpi.h>
#include <linux/interrupt.h>
#include <linux/irqchip.h>
#include <linux/module.h>
@@ -180,7 +181,7 @@ static int mbigen_domain_translate(struct irq_domain *d,
unsigned long *hwirq,
unsigned int *type)
{
- if (is_of_node(fwspec->fwnode)) {
+ if (is_of_node(fwspec->fwnode) || is_acpi_device_node(fwspec->fwnode)) {
if (fwspec->param_count != 2)
return -EINVAL;
@@ -271,6 +272,54 @@ static int mbigen_of_create_domain(struct platform_device *pdev,
return 0;
}
+#ifdef CONFIG_ACPI
+static acpi_status mbigen_acpi_process_resource(struct acpi_resource *ares,
+ void *context)
+{
+ struct acpi_resource_extended_irq *ext_irq;
+ u32 *num_irqs = context;
+
+ switch (ares->type) {
+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+ ext_irq = &ares->data.extended_irq;
+ *num_irqs += ext_irq->interrupt_count;
+ break;
+ default:
+ break;
+ }
+
+ return AE_OK;
+}
+
+static int mbigen_acpi_create_domain(struct platform_device *pdev,
+ struct mbigen_device *mgn_chip)
+{
+ struct irq_domain *domain;
+ u32 num_msis = 0;
+ acpi_status status;
+
+ status = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__PRS,
+ mbigen_acpi_process_resource, &num_msis);
+ if (ACPI_FAILURE(status) || num_msis == 0)
+ return -EINVAL;
+
+ domain = platform_msi_create_device_domain(&pdev->dev, num_msis,
+ mbigen_write_msg,
+ &mbigen_domain_ops,
+ mgn_chip);
+ if (!domain)
+ return -ENOMEM;
+
+ return 0;
+}
+#else
+static int mbigen_acpi_create_domain(struct platform_device *pdev,
+ struct mbigen_device *mgn_chip)
+{
+ return -ENODEV;
+}
+#endif
+
static int mbigen_device_probe(struct platform_device *pdev)
{
struct mbigen_device *mgn_chip;
@@ -288,9 +337,17 @@ static int mbigen_device_probe(struct platform_device *pdev)
if (IS_ERR(mgn_chip->base))
return PTR_ERR(mgn_chip->base);
- err = mbigen_of_create_domain(pdev, mgn_chip);
- if (err)
+ if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node)
+ err = mbigen_of_create_domain(pdev, mgn_chip);
+ else if (ACPI_COMPANION(&pdev->dev))
+ err = mbigen_acpi_create_domain(pdev, mgn_chip);
+ else
+ err = -EINVAL;
+
+ if (err) {
+ dev_err(&pdev->dev, "Failed to create mbi-gen@%p irqdomain", mgn_chip->base);
return err;
+ }
platform_set_drvdata(pdev, mgn_chip);
return 0;
@@ -302,10 +359,17 @@ static int mbigen_device_probe(struct platform_device *pdev)
};
MODULE_DEVICE_TABLE(of, mbigen_of_match);
+static const struct acpi_device_id mbigen_acpi_match[] = {
+ { "HISI0152", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, mbigen_acpi_match);
+
static struct platform_driver mbigen_platform_driver = {
.driver = {
.name = "Hisilicon MBIGEN-V2",
.of_match_table = mbigen_of_match,
+ .acpi_match_table = ACPI_PTR(mbigen_acpi_match),
},
.probe = mbigen_device_probe,
};
--
1.9.1
^ permalink raw reply related
* [PATCH] simplefb: Separate clk / regulator get and enable steps
From: Bartlomiej Zolnierkiewicz @ 2017-01-11 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161104144507.18072-1-hdegoede@redhat.com>
Hi,
On Friday, November 04, 2016 03:45:07 PM Hans de Goede wrote:
> Currently when a simplefb needs both clocks and regulators and one
> of the regulators returns -EPROBE_DEFER when we try to get it, we end
> up disabling the clocks. This causes the screen to go blank; and in some
> cases my cause hardware state to be lost resulting in the framebuffer not
> working at all.
>
> This commit splits the get and enable steps and only enables
> clocks and regulators after successfully getting all of them,
> fixing the disabling of the clocks which were left enabled by
> the firmware setting up the simplefb.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Thanks, patch queued for 4.11.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> ---
> drivers/video/fbdev/simplefb.c | 56 ++++++++++++++++++++++++++++++------------
> 1 file changed, 40 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 61f799a..a3c44ec 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -180,10 +180,12 @@ static int simplefb_parse_pd(struct platform_device *pdev,
> struct simplefb_par {
> u32 palette[PSEUDO_PALETTE_SIZE];
> #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
> + bool clks_enabled;
> unsigned int clk_count;
> struct clk **clks;
> #endif
> #if defined CONFIG_OF && defined CONFIG_REGULATOR
> + bool regulators_enabled;
> u32 regulator_count;
> struct regulator **regulators;
> #endif
> @@ -208,12 +210,12 @@ struct simplefb_par {
> * the fb probe will not help us much either. So just complain and carry on,
> * and hope that the user actually gets a working fb at the end of things.
> */
> -static int simplefb_clocks_init(struct simplefb_par *par,
> - struct platform_device *pdev)
> +static int simplefb_clocks_get(struct simplefb_par *par,
> + struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct clk *clock;
> - int i, ret;
> + int i;
>
> if (dev_get_platdata(&pdev->dev) || !np)
> return 0;
> @@ -244,6 +246,14 @@ static int simplefb_clocks_init(struct simplefb_par *par,
> par->clks[i] = clock;
> }
>
> + return 0;
> +}
> +
> +static void simplefb_clocks_enable(struct simplefb_par *par,
> + struct platform_device *pdev)
> +{
> + int i, ret;
> +
> for (i = 0; i < par->clk_count; i++) {
> if (par->clks[i]) {
> ret = clk_prepare_enable(par->clks[i]);
> @@ -256,8 +266,7 @@ static int simplefb_clocks_init(struct simplefb_par *par,
> }
> }
> }
> -
> - return 0;
> + par->clks_enabled = true;
> }
>
> static void simplefb_clocks_destroy(struct simplefb_par *par)
> @@ -269,7 +278,8 @@ static void simplefb_clocks_destroy(struct simplefb_par *par)
>
> for (i = 0; i < par->clk_count; i++) {
> if (par->clks[i]) {
> - clk_disable_unprepare(par->clks[i]);
> + if (par->clks_enabled)
> + clk_disable_unprepare(par->clks[i]);
> clk_put(par->clks[i]);
> }
> }
> @@ -277,8 +287,10 @@ static void simplefb_clocks_destroy(struct simplefb_par *par)
> kfree(par->clks);
> }
> #else
> -static int simplefb_clocks_init(struct simplefb_par *par,
> +static int simplefb_clocks_get(struct simplefb_par *par,
> struct platform_device *pdev) { return 0; }
> +static void simplefb_clocks_enable(struct simplefb_par *par,
> + struct platform_device *pdev) { }
> static void simplefb_clocks_destroy(struct simplefb_par *par) { }
> #endif
>
> @@ -305,14 +317,14 @@ static void simplefb_clocks_destroy(struct simplefb_par *par) { }
> * the fb probe will not help us much either. So just complain and carry on,
> * and hope that the user actually gets a working fb at the end of things.
> */
> -static int simplefb_regulators_init(struct simplefb_par *par,
> - struct platform_device *pdev)
> +static int simplefb_regulators_get(struct simplefb_par *par,
> + struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct property *prop;
> struct regulator *regulator;
> const char *p;
> - int count = 0, i = 0, ret;
> + int count = 0, i = 0;
>
> if (dev_get_platdata(&pdev->dev) || !np)
> return 0;
> @@ -354,6 +366,14 @@ static int simplefb_regulators_init(struct simplefb_par *par,
> }
> par->regulator_count = i;
>
> + return 0;
> +}
> +
> +static void simplefb_regulators_enable(struct simplefb_par *par,
> + struct platform_device *pdev)
> +{
> + int i, ret;
> +
> /* Enable all the regulators */
> for (i = 0; i < par->regulator_count; i++) {
> ret = regulator_enable(par->regulators[i]);
> @@ -365,15 +385,14 @@ static int simplefb_regulators_init(struct simplefb_par *par,
> par->regulators[i] = NULL;
> }
> }
> -
> - return 0;
> + par->regulators_enabled = true;
> }
>
> static void simplefb_regulators_destroy(struct simplefb_par *par)
> {
> int i;
>
> - if (!par->regulators)
> + if (!par->regulators || !par->regulators_enabled)
> return;
>
> for (i = 0; i < par->regulator_count; i++)
> @@ -381,8 +400,10 @@ static void simplefb_regulators_destroy(struct simplefb_par *par)
> regulator_disable(par->regulators[i]);
> }
> #else
> -static int simplefb_regulators_init(struct simplefb_par *par,
> +static int simplefb_regulators_get(struct simplefb_par *par,
> struct platform_device *pdev) { return 0; }
> +static void simplefb_regulators_enable(struct simplefb_par *par,
> + struct platform_device *pdev) { }
> static void simplefb_regulators_destroy(struct simplefb_par *par) { }
> #endif
>
> @@ -453,14 +474,17 @@ static int simplefb_probe(struct platform_device *pdev)
> }
> info->pseudo_palette = par->palette;
>
> - ret = simplefb_clocks_init(par, pdev);
> + ret = simplefb_clocks_get(par, pdev);
> if (ret < 0)
> goto error_unmap;
>
> - ret = simplefb_regulators_init(par, pdev);
> + ret = simplefb_regulators_get(par, pdev);
> if (ret < 0)
> goto error_clocks;
>
> + simplefb_clocks_enable(par, pdev);
> + simplefb_regulators_enable(par, pdev);
> +
> dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
> info->fix.smem_start, info->fix.smem_len,
> info->screen_base);
^ permalink raw reply
* [PATCH 0/4] video: ARM CLCD: add support of an optional GPIO to enable panel
From: Linus Walleij @ 2017-01-11 15:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50499352-fa14-64c4-1a74-db4827757a2d@arm.com>
On Tue, Jan 10, 2017 at 2:47 PM, Vladimir Murzin
<vladimir.murzin@arm.com> wrote:
> In another thread Benjamin pointed at patch [1] in drm/kms part for noMMU.
>
> [1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=62a0d98a188cc4ebd8ea54b37d274ec20465e464
Problem solved?
Vladimir: I do not require in any way that you create a CLCD driver for DRM,
I just think it would be very very nice...
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] arm64: assembler: make adr_l work in modules under KASLR
From: Mark Rutland @ 2017-01-11 15:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1484146493-18460-1-git-send-email-ard.biesheuvel@linaro.org>
Hi Ard,
On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
> modules and the core kernel may exceed 4 GB, putting symbols exported
> by the core kernel out of the reach of the ordinary adrp/add instruction
> pairs used to generate relative symbol references. So make the adr_l
> macro emit a movz/movk sequence instead when executing in module context.
AFAICT, we only use adr_l in a few assembly files that shouldn't matter
to modules:
* arch/arm64/kernel/head.S
* arch/arm64/kernel/sleep.S
* arch/arm64/kvm/hyp-init.S
* arch/arm64/kvm/hyp/hyp-entry.S
... so I don't follow why we need this.
Have I missed something? Or do you intend to use this in module code in
future?
It seems somewhat surprising to me to have adr_l expand to something
that doesn't use adr/adrp, but that's not necessarily a problem.
Thanks,
Mark.
^ permalink raw reply
* [PATCH] ARM: defconfig: include QCOM_EBI2 in multi_v7
From: Linus Walleij @ 2017-01-11 15:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOesGMh-5oyzjsby=R9c=euAngsq9=cNwJFDhEEKWtzxoqzLKA@mail.gmail.com>
On Tue, Jan 10, 2017 at 7:02 PM, Olof Johansson <olof@lixom.net> wrote:
> On Tue, Jan 10, 2017 at 2:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> This adds the Qualcomm External Bus Interface 2 to the multi_v7
>> defconfig: it is hard for users to realize that this is a
>> required driver for getting things like ethernet, and a necessary
>> prerequisite to get the external bus discoverable on the
>> MSM8660/APQ8060 machines.
>
> Is it the kind of option that should really be selected through
> Kconfig instead for those platforms?
Stephen Boyd question.
AFAIK the ambition is to go with the platform to have zero
stuff in mach-qcom/* and thus it all becomes a defconfig
thing.
> I.e. is it ever really likely that we'll want it off?
Not really, if this chip should be supported in the resulting
image.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 3/5] pinctrl: sunxi: add driver for V3s SoC
From: Linus Walleij @ 2017-01-11 15:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170103151629.19447-4-icenowy@aosc.xyz>
On Tue, Jan 3, 2017 at 4:16 PM, Icenowy Zheng <icenowy@aosc.xyz> wrote:
> V3s SoC features only a pin controller (for the lack of CPUs part).
>
> Add a driver for this controller.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
Patch applied with Maxime's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] arm64: assembler: make adr_l work in modules under KASLR
From: Ard Biesheuvel @ 2017-01-11 15:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170111151822.GC26344@leverpostej>
On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
>> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
>> modules and the core kernel may exceed 4 GB, putting symbols exported
>> by the core kernel out of the reach of the ordinary adrp/add instruction
>> pairs used to generate relative symbol references. So make the adr_l
>> macro emit a movz/movk sequence instead when executing in module context.
>
> AFAICT, we only use adr_l in a few assembly files that shouldn't matter
> to modules:
>
> * arch/arm64/kernel/head.S
> * arch/arm64/kernel/sleep.S
> * arch/arm64/kvm/hyp-init.S
> * arch/arm64/kvm/hyp/hyp-entry.S
>
> ... so I don't follow why we need this.
>
> Have I missed something? Or do you intend to use this in module code in
> future?
>
Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses
the lookup tables from crypto/aes_generic.c, which may be built into
the core kernel, while the code itself may be built as a module.
But in general, if the macro is available to modules, I would like to
make sure that it does not result in code that builds fine but may
fail in some cases only at runtime, especially given the fact that
there is also a Cortex-A53 erratum regarding adrp instructions, for
which reason we build modules with -mcmodel=large (which amounts to
the same thing as the patch above)
> It seems somewhat surprising to me to have adr_l expand to something
> that doesn't use adr/adrp, but that's not necessarily a problem.
>
I did realise that, but I don't think it is a problem tbh.
^ permalink raw reply
* [PATCH 5/8] efi: Get the secure boot status [ver #6]
From: David Howells @ 2017-01-11 15:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170111143304.GA29649@codeblueprint.co.uk>
Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > + movb $0, BP_secure_boot(%rsi)
> > #ifdef CONFIG_EFI_STUB
> > /*
> > * The entry point for the PE/COFF executable is efi_pe_entry, so
>
> Is clearing ::secure_boot really necessary? Any code path that goes
> via efi_main() will set it correctly and all other code paths should
> get it cleared in sanitize_boot_params(), no?
No.
The boot_params->secure_boot parameter exists whether or not efi_main() is
traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
is of uncertain value.
Further, sanitize_boot_params() has to be modified by this patch so as not to
clobber the secure_boot flag.
> What's the distinction between the unset and unknown enums?
unset -> The flag was cleared by head.S and efi_get_secureboot() was never
called.
unknown -> efi_get_secureboot() tried and failed to access the EFI variables
that should give the state.
David
^ permalink raw reply
* [PATCH 56/62] watchdog: tangox_wdt: Convert to use device managed functions
From: Marc Gonzalez @ 2017-01-11 15:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a61eb838-4167-3032-bde8-3a8845b780f8@roeck-us.net>
On 11/01/2017 15:25, Guenter Roeck wrote:
> On 01/11/2017 04:31 AM, Marc Gonzalez wrote:
>> On 11/01/2017 11:52, Guenter Roeck wrote:
>>
>>> On 01/11/2017 01:07 AM, Marc Gonzalez wrote:
>>>
>>>>> @@ -134,12 +134,15 @@ static int tangox_wdt_probe(struct platform_device *pdev)
>>>>> err = clk_prepare_enable(dev->clk);
>>>>> if (err)
>>>>> return err;
>>>>> + err = devm_add_action_or_reset(&pdev->dev,
>>>>> + (void(*)(void *))clk_disable_unprepare,
>>>>> + dev->clk);
>>>>> + if (err)
>>>>> + return err;
>>>>
>>>> Hello Guenter,
>>>>
>>>> I would rather avoid the function pointer cast.
>>>> How about defining an auxiliary function for the cleanup action?
>>>>
>>>> clk_disable_unprepare() is static inline, so gcc will have to
>>>> define an auxiliary function either way. What do you think?
>>>
>>> Not really. It would just make it more complicated to replace the
>>> call with devm_clk_prepare_enable(), should it ever find its way
>>> into the light of day.
>>
>> More complicated, because the cleanup function will have to be deleted later?
>> The compiler will warn if someone forgets to do that.
>>
>> In my opinion, it's not a good idea to rely on the fact that casting
>> void(*)(struct clk *clk) to void(*)(void *) is likely to work as expected
>> on most platforms. (It has undefined behavior, strictly speaking.)
>
> I do hear that you object to this code.
>
> However, I must admit that you completely lost me here. It is a cast from
> one function pointer to another,
Perhaps you are used to work at the assembly level, where pointers are
just addresses, and all pointers are interchangeable.
At a slightly higher level (C abstract machine), it is not so.
> passed as argument to another function,
> with a secondary cast of its argument from a typed pointer to a void pointer.
> I don't think C permits for "undefined behavior, strictly speaking".
The C standard leaves quite a lot of behavior undefined, e.g.
char *foo = "hello";
foo[1] = 'a'; // UB
char buf[4];
*(int *)&buf = 0xdeadbeef; // UB
1 << 64; // UB
> Besides, that same mechanism is already used elsewhere, which is how I
> got the idea. Are you claiming that there are situations where it won't
> work ?
If this technique is already used elsewhere in the kernel, then I'll
crawl back under my rock (and weep).
I can see two issues with the code you propose.
First is the same for all casts: silencing potential warnings,
e.g. if the prototype of clk_disable_unprepare ever changed.
(Though casts are required for vararg function arguments.)
Second is just theory and not a real-world concern.
>> Do you really dislike the portable solution I suggested? :-(
>
> It is not more portable than the above. It is more expensive and adds more
> code.
Maybe I am mistaken. Can you tell me why adding an auxiliary function
is more expensive? (In CPU cycles?)
clk_disable_unprepare() is static inline, so an auxiliary function
exists either way (implicit or explicit).
Regards.
^ permalink raw reply
* [PATCH 8/8] efi: Add EFI_SECURE_BOOT bit [ver #6]
From: David Howells @ 2017-01-11 15:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170111145138.GB29649@codeblueprint.co.uk>
Matt Fleming <matt@codeblueprint.co.uk> wrote:
> Before we add more efi.flags bits I'd like this series to include the
> patch that makes use of EFI_SECURE_BOOT. Alternatively, you move this
> last patch to a new series.
Are you willing to take the kernel lock-down patches also?
David
^ permalink raw reply
* [PATCH] arm64: assembler: make adr_l work in modules under KASLR
From: Ard Biesheuvel @ 2017-01-11 15:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu9DxMzSTVRPdeSaJQNqe5RGjS_qzVzBU1SpGs5EZvCE1Q@mail.gmail.com>
On 11 January 2017 at 15:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi Ard,
>>
>> On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
>>> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
>>> modules and the core kernel may exceed 4 GB, putting symbols exported
>>> by the core kernel out of the reach of the ordinary adrp/add instruction
>>> pairs used to generate relative symbol references. So make the adr_l
>>> macro emit a movz/movk sequence instead when executing in module context.
>>
>> AFAICT, we only use adr_l in a few assembly files that shouldn't matter
>> to modules:
>>
>> * arch/arm64/kernel/head.S
>> * arch/arm64/kernel/sleep.S
>> * arch/arm64/kvm/hyp-init.S
>> * arch/arm64/kvm/hyp/hyp-entry.S
>>
>> ... so I don't follow why we need this.
>>
>> Have I missed something? Or do you intend to use this in module code in
>> future?
>>
>
> Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses
> the lookup tables from crypto/aes_generic.c, which may be built into
> the core kernel, while the code itself may be built as a module.
>
> But in general, if the macro is available to modules, I would like to
> make sure that it does not result in code that builds fine but may
> fail in some cases only at runtime, especially given the fact that
> there is also a Cortex-A53 erratum regarding adrp instructions, for
> which reason we build modules with -mcmodel=large (which amounts to
> the same thing as the patch above)
>
Actually, we could test for
defined(MODULE) && defined(CONFIG_ARM64_MODULE_CMODEL_LARGE)
instead, so we revert to adrp/add pairs for modules if the erratum and
KASLR are both disabled
>> It seems somewhat surprising to me to have adr_l expand to something
>> that doesn't use adr/adrp, but that's not necessarily a problem.
>>
>
> I did realise that, but I don't think it is a problem tbh.
^ permalink raw reply
* Unhandled level 2 translation fault (11) at 0x000000b8, esr 0x92000046, rpi3 (aarch64)
From: Dave Martin @ 2017-01-11 15:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170111144902.GA20288@e104818-lin.cambridge.arm.com>
On Wed, Jan 11, 2017 at 02:49:03PM +0000, Catalin Marinas wrote:
> On Tue, Jan 10, 2017 at 12:14:23PM +0000, Catalin Marinas wrote:
> > On Mon, Jan 09, 2017 at 07:06:19PM +0100, Bas van Tiel wrote:
> > > > I defined STACKSIZE to the kernel's SIGSTKSZ (16384) and it seems to run
> > > > fine, though I'll leave it longer/overnight (on a Juno board). With the
> > > > 4K signal stack it was crashing shortly after start.
> > >
> > > I tried the STACKSIZE of 16384 for both the RPI3 and the PINEA64 board
> > > and still see the same behaviour of crashing. Sometimes the process
> > > is also blocked for a long time before it crashes.
> > >
> > > Setting the interval to 200 usec [5 Khz] will help to crash it faster.
> > >
> > > To further isolate the issue I will create a kernel module (based on a
> > > hrtimer) that will sent a periodic signal to the registered process
> > > and execute the same sighandler logic to check if the problem is still
> > > there.
> >
> > I lowered the interval to 100us (it was 100ms in the original file) and
> > I can indeed trigger segfault easily on Juno. But it doesn't fail in the
> > same way every time, I sometimes get permission fault, other times bad
> > frame.
>
> With 100us interval, it segfaults on x86 fairly quickly as well, so I
> don't think it's a kernel issue.
To be able to take a signal at all, stacks need to be at least SIGSTKSZ
bytes in practice:
diff --git a/context_demo.c b/context_demo.c
index 2cc63f7..b1f3bbc 100644
--- a/context_demo.c
+++ b/context_demo.c
@@ -22,7 +22,7 @@
#define NUMCONTEXTS 10 /* how many contexts to make */
-#define STACKSIZE 4096 /* stack size */
+#define STACKSIZE SIGSTKSZ /* stack size */
#define INTERVAL 100 /* timer interval in nanoseconds */
sigset_t set; /* process wide signal mask */
The other issue looks a bit subtler, to do with signal masking.
SIGALRM will be masked on entry to timer_interrupt() and restored on
return, due to and absence of SA_NODEFER from sa_flags when calling
sigaction. (Setting SIGALRM in sa_mask also has this effect, but this
is redundant without SA_NODEFER.)
However, by explicitly clearing this signal from
signal_context.uc_sigmask, we'll enter scheduler() with SIGALRM
unmasked. If a new SIGALRM is taken before scheduler() has called
setcontext(), we'll pile up another signal on signal_stack and call
schedule() again, still on signal_stack ... and this can repeat
indefinitely.
There's no need to clear SIGALRM from the signal mask: it will be
cleared when timer_interrupt() returns after resuming an interrupted
task (as part of the signal frame restore work done by rt_sigreturn).
So:
@@ -61,7 +61,6 @@ timer_interrupt(int j, siginfo_t *si, void *old_context)
signal_context.uc_stack.ss_sp = signal_stack;
signal_context.uc_stack.ss_size = STACKSIZE;
signal_context.uc_stack.ss_flags = 0;
- sigemptyset(&signal_context.uc_sigmask);
makecontext(&signal_context, scheduler, 1);
/* save running thread, jump to scheduler */
For me, this seems to fix the problem.
It also makes sense of what we've seen: we need either short timer
intervals, slow machines, or high system load (or some combination) in
order to take enough extra signals in scheduler() to cause a stack
overflow.
I can't see the purpose of running scheduler() in its own context here,
except so that it doesn't contribute stack overhead to the thread
stacks (which hardly seems worthwhile, since its overhead is probably a
lot smaller than the signal overhead anyway -- maybe I'm missing
something).
makeconext() and swapcontext() are obsoleted by POSIX.1-2008 and
considered non-portable (see makecontext(3), swapcontext(3)). Really,
the ucontext API should not be used for anything except cooperative
switching now (certainly this covered the vast majority or real-world
usage the last time I looked into it). For anything else, pthreads
almost certainly do it better.
Cheers
---Dave
^ permalink raw reply related
* [PATCH] arm64: assembler: make adr_l work in modules under KASLR
From: Mark Rutland @ 2017-01-11 15:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu9DxMzSTVRPdeSaJQNqe5RGjS_qzVzBU1SpGs5EZvCE1Q@mail.gmail.com>
On Wed, Jan 11, 2017 at 03:25:09PM +0000, Ard Biesheuvel wrote:
> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi Ard,
> >
> > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
> >> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
> >> modules and the core kernel may exceed 4 GB, putting symbols exported
> >> by the core kernel out of the reach of the ordinary adrp/add instruction
> >> pairs used to generate relative symbol references. So make the adr_l
> >> macro emit a movz/movk sequence instead when executing in module context.
> >
> > AFAICT, we only use adr_l in a few assembly files that shouldn't matter
> > to modules:
> >
> > * arch/arm64/kernel/head.S
> > * arch/arm64/kernel/sleep.S
> > * arch/arm64/kvm/hyp-init.S
> > * arch/arm64/kvm/hyp/hyp-entry.S
> >
> > ... so I don't follow why we need this.
> >
> > Have I missed something? Or do you intend to use this in module code in
> > future?
>
> Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses
> the lookup tables from crypto/aes_generic.c, which may be built into
> the core kernel, while the code itself may be built as a module.
Ah, ok.
> But in general, if the macro is available to modules, I would like to
> make sure that it does not result in code that builds fine but may
> fail in some cases only at runtime, especially given the fact that
> there is also a Cortex-A53 erratum regarding adrp instructions, for
> which reason we build modules with -mcmodel=large (which amounts to
> the same thing as the patch above)
>
> > It seems somewhat surprising to me to have adr_l expand to something
> > that doesn't use adr/adrp, but that's not necessarily a problem.
>
> I did realise that, but I don't think it is a problem tbh.
In this case it should be fine, certainly.
There are cases like the early boot code and hyp code where it's
critical that we use adr. It's also possible that we might build
(modular) drivers which want some idmapped code, where we want adr, so
it seems unfortunate that this depends on howthe code is built.
So, maybe it's better to have a mov_sym helper for this case, to be
explicit about what we want? That can use either adr* or mov*, or the
latter consistently.
Thanks,
Mark.
^ permalink raw reply
* [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-11 15:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOAejn2eOy2sn1VkE979ne23Sj9L6+kaQDNpL1EUKb2m=6sGXw@mail.gmail.com>
On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> Hi Uwe,
>
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> > Hello Cedric,
> >
> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> >> +/*
> >> + * In standard mode:
> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk period
> >> + *
> >> + * In fast mode:
> >> + * If Duty = 0; SCL high period = 1 * CCR * I2C parent clk period
^^
> >> + * SCL low period = 2 * CCR * I2C parent clk period
^^
> >> + * If Duty = 1; SCL high period = 9 * CCR * I2C parent clk period
^^
> >> + * SCL low period = 16 * CCR * I2C parent clk period
> > s/ \*/ */ several times
>
> Sorry but I don't see where is the issue as the style for multi-line
> comments seems ok.
> Could you please clarify that point if possible ? Thanks in advance
There are several places with double spaces before * marked above.
> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always set
> >> + * Duty = 1
> >> + *
> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> > s/mode/Mode/
>
> ok thanks
>
> >
> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
> >> + * constraints defined by i2c bus specification
> >
> > I don't understand scl_period = 1 ?s for Fast Mode. For a bus freqency
> > of 400 kHz we need low + high = 2.5 ?s. Is there a factor 10 missing
> > somewhere?
>
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 ?s
> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 ?s
> So low + high = 27 ?s > 2,5 ?s
For me 9 ?s + 16 ?s is 25 ?s, resulting in 40 kHz. That's why I wondered
if there is a factor 10 missing somewhere.
> >> + */
> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
> >> [...]
> >> +
> >> +/**
> >> + * stm32f4_i2c_hw_config() - Prepare I2C block
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + int ret = 0;
> >> +
> >> + /* Disable I2C */
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> >> +
> >> + ret = stm32f4_i2c_set_periph_clk_freq(i2c_dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + stm32f4_i2c_set_rise_time(i2c_dev);
> >> +
> >> + stm32f4_i2c_set_speed_mode(i2c_dev);
> >> +
> >> + stm32f4_i2c_set_filter(i2c_dev);
> >> +
> >> + /* Enable I2C */
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE);
> >
> > This function is called after a hw reset, so there should be no need to
> > use clr_bits and set_bits because the value read from hw should be
> > known.
>
> ok thanks
>
> >
> >> + return ret;
> >
> > return 0;
>
> ok thanks
>
> >
> >> +}
> >> +
> >> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + u32 status;
> >> + int ret;
> >> +
> >> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> >> + status,
> >> + !(status & STM32F4_I2C_SR2_BUSY),
> >> + 10, 1000);
> >> + if (ret) {
> >> + dev_dbg(i2c_dev->dev, "bus not free\n");
> >> + ret = -EBUSY;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_write_ byte() - Write a byte in the data register
> >> + * @i2c_dev: Controller's private data
> >> + * @byte: Data to write in the register
> >> + */
> >> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8 byte)
> >> +{
> >> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR);
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_write_msg() - Fill the data register in write mode
> >> + * @i2c_dev: Controller's private data
> >> + *
> >> + * This function fills the data register with I2C transfer buffer
> >> + */
> >> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> +
> >> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++);
> >> + msg->count--;
> >> +}
> >> +
> >> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + u32 rbuf;
> >> +
> >> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> >> + *msg->buf++ = rbuf & 0xff;
> >
> > This is unnecessary. buf has an 8 bit wide type so
> >
> > *msg->buf++ = rbuf;
> >
> > has the same effect. (ISTR this is something I already pointed out
> > earlier?)
>
> Yes you are right.
>
> >
> >> + msg->count--;
> >> +}
> >> +
> >> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> + stm32f4_i2c_disable_irq(i2c_dev);
> >> +
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + if (msg->stop)
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + else
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> +
> >> + complete(&i2c_dev->complete);
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of write
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> + if (msg->count) {
> >> + stm32f4_i2c_write_msg(i2c_dev);
> >> + if (!msg->count) {
> >> + /* Disable buffer interrupts for RXNE/TXE events */
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> >> + }
> >> + } else {
> >> + stm32f4_i2c_terminate_xfer(i2c_dev);
> >
> > Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If
> > yes, is it then right to set STM32F4_I2C_CR1_STOP or
> > STM32F4_I2C_CR1_START?
>
> If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called.
> In that case, we return -EAGAIN and i2c-core will retry by calling
> stm32f4_i2c_xfer()
>
> >
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of read
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> +
> >> + switch (msg->count) {
> >> + case 1:
> >> + stm32f4_i2c_disable_irq(i2c_dev);
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + complete(&i2c_dev->complete);
> >> + break;
> >> + /*
> >> + * For 2 or 3-byte reception, we do not have to read the data register
> >> + * when RXNE occurs as we have to wait for byte transferred finished
> >
> > it's hard to understand because if you don't know the hardware the
> > meaning of RXNE is unknown.
>
> Ok I will replace RXNE by RX not empty in that comment
>
> >
> >> + * event before reading data. So, here we just disable buffer
> >> + * interrupt in order to avoid another system preemption due to RXNE
> >> + * event
> >> + */
> >> + case 2:
> >> + case 3:
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN);
> >> + break;
> >> + /* For N byte reception with N > 3 we directly read data register */
> >> + default:
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interrupt
> >> + * in case of read
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >
> > btf is a hw-related name. Maybe better use _done which is easier to
> > understand?
>
> OK
>
> >
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg;
> >> + u32 mask;
> >> + int i;
> >> +
> >> + switch (msg->count) {
> >> + case 2:
> >> + /*
> >> + * In order to correctly send the Stop or Repeated Start
> >> + * condition on the I2C bus, the STOP/START bit has to be set
> >> + * before reading the last two bytes.
> >> + * After that, we could read the last two bytes, disable
> >> + * remaining interrupts and notify the end of xfer to the
> >> + * client
> >
> > This is surprising. I didn't recheck the manual, but that looks very
> > uncomfortable.
>
> I agree but this exactly the hardware way of working described in the
> reference manual.
IMHO that's a hw bug. This makes it for example impossible to implement
SMBus block transfers (I think).
> > How does this work, when I only want to read a single
> > byte? Same problem for ACK below.
>
> For a single reception, we enable NACK and STOP or Repeatead START
> bits during address match.
> The NACK and STOP/START pulses are sent as soon as the data is
> received in the shift register.
> Please note that in that case, we don't have to wait BTF event to read the data.
> Data is read as soon as RXNE event occurs.
>
> >
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + if (msg->stop)
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + else
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> +
> >> + for (i = 2; i > 0; i--)
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> +
> >> + reg = i2c_dev->base + STM32F4_I2C_CR2;
> >> + mask = STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERREN;
> >> + stm32f4_i2c_clr_bits(reg, mask);
> >> +
> >> + complete(&i2c_dev->complete);
> >> + break;
> >> + case 3:
> >> + /*
> >> + * In order to correctly send the ACK on the I2C bus for the
> >> + * last two bytes, we have to set ACK bit before reading the
> >> + * third last data byte
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + break;
> >> + default:
> >> + stm32f4_i2c_read_msg(i2c_dev);
> >> + }
> >> +}
> >> +
> >> +/**
> >> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in case of
> >> + * master receiver
> >> + * @i2c_dev: Controller's private data
> >> + */
> >> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev)
> >> +{
> >> + struct stm32f4_i2c_msg *msg = &i2c_dev->msg;
> >> + void __iomem *reg;
> >> +
> >> + switch (msg->count) {
> >> + case 0:
> >> + stm32f4_i2c_terminate_xfer(i2c_dev);
> >> + /* Clear ADDR flag */
> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> + break;
> >> + case 1:
> >> + /*
> >> + * Single byte reception:
> >
> > This also happens for the last byte of a 5 byte transfer, right?
>
> For a 5 byte transfer the behavior is different:
> We have to read data from DR (data register) as soon as the RXNE (RX
> not empty) event occurs for data1, data2 and data3 (until N-2 data for
> a more generic case)
> The ACK is automatically sent as soon as the data is received in the
> shift register as the I2C controller was configured to do that during
> adress match phase.
>
> For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event
> in order to set NACK before reading DR.
> This event occurs when a new data has been received in shift register
> (in our case data4 or N-1 data) but the prevoius data in DR (in our
> case data3 or N-2 data) has not been read yet.
> In that way, the NACK pulse will be correctly generated after the last
> received data byte.
>
> For data4 and data5, we wait for BTF event (data4 or N-1 data in DR
> and data5 or N data in shift register), set STOP or repeated Start in
> order to correctly sent the right pulse after the last received data
> byte and run 2 consecutives read of DR.
So "Single byte reception" above is wrong, as this case is also used for
longer transfers and should be updated accordingly.
> >> + * Enable NACK, clear ADDR flag and generate STOP or RepSTART
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> + if (msg->stop)
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
> >> + else
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START);
> >> + break;
> >> + case 2:
> >> + /*
> >> + * 2-byte reception:
> >> + * Enable NACK and set POS
> >
> > What is POS?
> POS is used to define the position of the (N)ACK pulse
> 0: ACK is generated when the current is being received in the shift register
> 1: ACK is generated when the next byte which will be received in the
> shift register (used for 2-byte reception)
Can you please put this into the comment. "POS" isn't much helpful
there.
>
> >
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> >
> > You could get rid of this, when caching the value of CR1. Would save two
> > register reads here. This doesn't work for all registers, but it should
> > be possible to apply for most of them, maybe enough to get rid of the
> > clr_bits and set_bits function.
> >
> >> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2);
> >> + break;
> >> +
> >> + default:
> >> + /* N-byte reception: Enable ACK */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK);
> >
> > Do you need to set ACK for each byte transferred?
> I need to do that in order to be SMBus compatible and the ACK/NACK
> seems to be used by default in Documentation/i2c/i2c-protocol file.
Yeah, protocol wise you need to ack each byte. I just wondered if you
need to set the hardware bit for each byte or if it is retained in
hardware until unset by a register write.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-11 15:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOAejn2cDzPgqvWZf7ASvtST+aDeaAebf=1aMWA9Zd8CDg4pmA@mail.gmail.com>
Hello Cedric,
On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
> >
> >> + */
> >> + reg = i2c_dev->base + STM32F4_I2C_CR1;
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK);
> >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS);
> >
> > You could get rid of this, when caching the value of CR1. Would save two
> > register reads here. This doesn't work for all registers, but it should
> > be possible to apply for most of them, maybe enough to get rid of the
> > clr_bits and set_bits function.
>
> I agree at many places I could save registers read by not using
> clr_bits and set_bits function when the registers in question has been
> already read.
> But it is not enough to get rid of the clr_bits and set_bits function.
> For example when calling stm32f4_i2c_terminate_xfer(), the CR1
> register is never read before so set_bits function is useful.
I didn't double check the manual, but I would expect that CR1 isn't
modified by hardware. So you can cache the result in the driver data
structure and do the necessary modifications with that one.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox