Add support for TISCI irqchip drivers

---
Changes since v1:
- None.

 drivers/firmware/ti_sci.c              | 83 ++++++++++++++++++++++++++
 include/linux/soc/ti/ti_sci_protocol.h | 17 ++++++
 2 files changed, 100 insertions(+)

drivers/firmware/ti_sci.c +83 -0

index 69ed1464175c..f0cafa8a2ee9 100644
@@ -1781,6 +1781,89 @@ const struct ti_sci_handle *devm_ti_sci_get_handle(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(devm_ti_sci_get_handle);
 
+/**
+ * ti_sci_get_by_phandle() - Get the TI SCI handle using DT phandle
+ * @np:		device node
+ * @propname:	property name containing phandle on TISCI node
+ *
+ * NOTE: The function does not track individual clients of the framework
+ * and is expected to be maintained by caller of TI SCI protocol library.
+ * ti_sci_put_handle must be balanced with successful ti_sci_get_by_phandle
+ * Return: pointer to handle if successful, else:
+ * -EPROBE_DEFER if the instance is not ready
+ * -ENODEV if the required node handler is missing
+ * -EINVAL if invalid conditions are encountered.
+ */
+const struct ti_sci_handle *ti_sci_get_by_phandle(struct device_node *np,
+						  const char *property)
+{
+	struct ti_sci_handle *handle = NULL;
+	struct device_node *ti_sci_np;
+	struct ti_sci_info *info;
+	struct list_head *p;
+
+	if (!np) {
+		pr_err("I need a device pointer\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	ti_sci_np = of_parse_phandle(np, property, 0);
+	if (!ti_sci_np)
+		return ERR_PTR(-ENODEV);
+
+	mutex_lock(&ti_sci_list_mutex);
+	list_for_each(p, &ti_sci_list) {
+		info = list_entry(p, struct ti_sci_info, node);
+		if (ti_sci_np == info->dev->of_node) {
+			handle = &info->handle;
+			info->users++;
+			break;
+		}
+	}
+	mutex_unlock(&ti_sci_list_mutex);
+	of_node_put(ti_sci_np);
+
+	if (!handle)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return handle;
+}
+EXPORT_SYMBOL_GPL(ti_sci_get_by_phandle);
+
+/**
+ * devm_ti_sci_get_by_phandle() - Managed get handle using phandle
+ * @dev:	Device pointer requesting TISCI handle
+ * @propname:	property name containing phandle on TISCI node
+ *
+ * NOTE: This releases the handle once the device resources are
+ * no longer needed. MUST NOT BE released with ti_sci_put_handle.
+ * The function does not track individual clients of the framework
+ * and is expected to be maintained by caller of TI SCI protocol library.
+ *
+ * Return: 0 if all went fine, else corresponding error.
+ */
+const struct ti_sci_handle *devm_ti_sci_get_by_phandle(struct device *dev,
+						       const char *property)
+{
+	const struct ti_sci_handle *handle;
+	const struct ti_sci_handle **ptr;
+
+	ptr = devres_alloc(devm_ti_sci_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+	handle = ti_sci_get_by_phandle(dev_of_node(dev), property);
+
+	if (!IS_ERR(handle)) {
+		*ptr = handle;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return handle;
+}
+EXPORT_SYMBOL_GPL(devm_ti_sci_get_by_phandle);
+
 static int tisci_reboot_handler(struct notifier_block *nb, unsigned long mode,
 				void *cmd)
 {

include/linux/soc/ti/ti_sci_protocol.h +17 -0

index 18435e5c6364..515587e9d373 100644
@@ -217,6 +217,10 @@ struct ti_sci_handle {
 const struct ti_sci_handle *ti_sci_get_handle(struct device *dev);
 int ti_sci_put_handle(const struct ti_sci_handle *handle);
 const struct ti_sci_handle *devm_ti_sci_get_handle(struct device *dev);
+const struct ti_sci_handle *ti_sci_get_by_phandle(struct device_node *np,
+						  const char *property);
+const struct ti_sci_handle *devm_ti_sci_get_by_phandle(struct device *dev,
+						       const char *property);
 
 #else	/* CONFIG_TI_SCI_PROTOCOL */

@@ -236,6 +240,19 @@ const struct ti_sci_handle *devm_ti_sci_get_handle(struct device *dev)
 	return ERR_PTR(-EINVAL);
 }
 
+static inline
+const struct ti_sci_handle *ti_sci_get_by_phandle(struct device_node *np,
+						  const char *property)
+{
+	return ERR_PTR(-EINVAL);
+}
+
+static inline
+const struct ti_sci_handle *devm_ti_sci_get_by_phandle(struct device *dev,
+						       const char *property)
+{
+	return ERR_PTR(-EINVAL);
+}
 #endif	/* CONFIG_TI_SCI_PROTOCOL */
 
 #endif	/* __TISCI_PROTOCOL_H */

Signature · 1 line

2.19.1

Lokesh Vutla

author

2018-10-18 15:40 UTC #2 lore raw

PATCH v2 2/10 firmware: ti_sci: Add support for RM core ops

TISCI provides support for getting the resources(IRQ, RING etc..) assigned to a specific device. These resources can be handled by the client and in turn sends TISCI cmd to configure the resources.

It is very important that client should keep track on usage of these resources.

Add support for TISCI commands to get resource ranges.

---
Changes since v1:
- Added support for RM mapping table. So clients only need to get subtype from DT.

 drivers/firmware/ti_sci.c              | 170 +++++++++++++++++++++++++
 drivers/firmware/ti_sci.h              |  42 ++++++
 include/linux/soc/ti/ti_sci_protocol.h |  27 ++++
 3 files changed, 239 insertions(+)

drivers/firmware/ti_sci.c +170 -0

index f0cafa8a2ee9..a2a099b8f62a 100644
@@ -64,6 +64,22 @@ struct ti_sci_xfers_info {
 	spinlock_t xfer_lock;
 };
 
+/**
+ * struct ti_sci_rm_type_map - Structure representing TISCI Resource
+ *				management representation of dev_ids.
+ * @dev_id:	TISCI device ID
+ * @type:	Corresponding id as identified by TISCI RM.
+ *
+ * Note: This is used only as a work around for using RM range apis
+ *	for AM654 SoC. For future SoCs dev_id will be used as type
+ *	for RM range APIs. In order to maintain ABI backward compatibility
+ *	type is not being changed for AM654 SoC.
+ */
+struct ti_sci_rm_type_map {
+	u32 dev_id;
+	u16 type;
+};
+
 /**
  * struct ti_sci_desc - Description of SoC integration
  * @default_host_id:	Host identifier representing the compute entity

@@ -71,12 +87,14 @@ struct ti_sci_xfers_info {
  * @max_msgs: Maximum number of messages that can be pending
  *		  simultaneously in the system
  * @max_msg_size: Maximum size of data per message that can be handled.
+ * @rm_type_map: RM resource type mapping structure.
  */
 struct ti_sci_desc {
 	u8 default_host_id;
 	int max_rx_timeout_ms;
 	int max_msgs;
 	int max_msg_size;
+	struct ti_sci_rm_type_map *rm_type_map;
 };
 
 /**

@@ -1617,6 +1635,153 @@ static int ti_sci_cmd_core_reboot(const struct ti_sci_handle *handle)
 	return ret;
 }
 
+static int ti_sci_get_resource_type(struct ti_sci_info *info, u16 dev_id,
+				    u16 *type)
+{
+	struct ti_sci_rm_type_map *rm_type_map = info->desc->rm_type_map;
+	bool found = false;
+	int i;
+
+	/* If map is not provided then assume dev_id is used as type */
+	if (!rm_type_map) {
+		*type = dev_id;
+		return 0;
+	}
+
+	for (i = 0; rm_type_map[i].dev_id; i++) {
+		if (rm_type_map[i].dev_id == dev_id) {
+			*type = rm_type_map[i].type;
+			found = true;
+			break;
+		}
+	}
+
+	if (!found)
+		return -EINVAL;
+
+	return 0;
+}
+
+/**
+ * ti_sci_get_resource_range - Helper to get a range of resources assigned
+ *			       to a host. Resource is uniquely identified by
+ *			       type and subtype.
+ * @handle:		Pointer to TISCI handle.
+ * @dev_id:		TISCI device ID.
+ * @subtype:		Resource assignment subtype that is being requested
+ *			from the given device.
+ * @s_host:		Host processor ID to which the resources are allocated
+ * @range_start:	Start index of the resource range
+ * @range_num:		Number of resources in the range
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_get_resource_range(const struct ti_sci_handle *handle,
+				     u32 dev_id, u8 subtype, u8 s_host,
+				     u16 *range_start, u16 *range_num)
+{
+	struct ti_sci_msg_resp_get_resource_range *resp;
+	struct ti_sci_msg_req_get_resource_range *req;
+	struct ti_sci_xfer *xfer;
+	struct ti_sci_info *info;
+	struct device *dev;
+	u16 type;
+	int ret = 0;
+
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
+	if (!handle)
+		return -EINVAL;
+
+	info = handle_to_ti_sci_info(handle);
+	dev = info->dev;
+
+	xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_GET_RESOURCE_RANGE,
+				   TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
+				   sizeof(*req), sizeof(*resp));
+	if (IS_ERR(xfer)) {
+		ret = PTR_ERR(xfer);
+		dev_err(dev, "Message alloc failed(%d)\n", ret);
+		return ret;
+	}
+
+	ret = ti_sci_get_resource_type(info, dev_id, &type);
+	if (ret) {
+		dev_err(dev, "rm type lookup failed for %u\n", dev_id);
+		goto fail;
+	}
+
+	req = (struct ti_sci_msg_req_get_resource_range *)xfer->xfer_buf;
+	req->secondary_host = s_host;
+	req->type = type & MSG_RM_RESOURCE_TYPE_MASK;
+	req->subtype = subtype & MSG_RM_RESOURCE_SUBTYPE_MASK;
+
+	ret = ti_sci_do_xfer(info, xfer);
+	if (ret) {
+		dev_err(dev, "Mbox send fail %d\n", ret);
+		goto fail;
+	}
+
+	resp = (struct ti_sci_msg_resp_get_resource_range *)xfer->xfer_buf;
+
+	if (!ti_sci_is_response_ack(resp)) {
+		ret = -ENODEV;
+	} else if (!resp->range_start && !resp->range_num) {
+		ret = -ENODEV;
+	} else {
+		*range_start = resp->range_start;
+		*range_num = resp->range_num;
+	};
+
+fail:
+	ti_sci_put_one_xfer(&info->minfo, xfer);
+
+	return ret;
+}
+
+/**
+ * ti_sci_cmd_get_resource_range - Get a range of resources assigned to host
+ *				   that is same as ti sci interface host.
+ * @handle:		Pointer to TISCI handle.
+ * @dev_id:		TISCI device ID.
+ * @subtype:		Resource assignment subtype that is being requested
+ *			from the given device.
+ * @range_start:	Start index of the resource range
+ * @range_num:		Number of resources in the range
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_cmd_get_resource_range(const struct ti_sci_handle *handle,
+					 u32 dev_id, u8 subtype,
+					 u16 *range_start, u16 *range_num)
+{
+	return ti_sci_get_resource_range(handle, dev_id, subtype,
+					 TI_SCI_IRQ_SECONDARY_HOST_INVALID,
+					 range_start, range_num);
+}
+
+/**
+ * ti_sci_cmd_get_resource_range_from_shost - Get a range of resources
+ *					      assigned to a specified host.
+ * @handle:		Pointer to TISCI handle.
+ * @dev_id:		TISCI device ID.
+ * @subtype:		Resource assignment subtype that is being requested
+ *			from the given device.
+ * @s_host:		Host processor ID to which the resources are allocated
+ * @range_start:	Start index of the resource range
+ * @range_num:		Number of resources in the range
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static
+int ti_sci_cmd_get_resource_range_from_shost(const struct ti_sci_handle *handle,
+					     u32 dev_id, u8 subtype, u8 s_host,
+					     u16 *range_start, u16 *range_num)
+{
+	return ti_sci_get_resource_range(handle, dev_id, subtype, s_host,
+					 range_start, range_num);
+}
+
 /*
  * ti_sci_setup_ops() - Setup the operations structures
  * @info:	pointer to TISCI pointer

@@ -1627,6 +1792,7 @@ static void ti_sci_setup_ops(struct ti_sci_info *info)
 	struct ti_sci_core_ops *core_ops = &ops->core_ops;
 	struct ti_sci_dev_ops *dops = &ops->dev_ops;
 	struct ti_sci_clk_ops *cops = &ops->clk_ops;
+	struct ti_sci_rm_core_ops *rm_core_ops = &ops->rm_core_ops;
 
 	core_ops->reboot_device = ti_sci_cmd_core_reboot;

@@ -1657,6 +1823,10 @@ static void ti_sci_setup_ops(struct ti_sci_info *info)
 	cops->get_best_match_freq = ti_sci_cmd_clk_get_match_freq;
 	cops->set_freq = ti_sci_cmd_clk_set_freq;
 	cops->get_freq = ti_sci_cmd_clk_get_freq;
+
+	rm_core_ops->get_range = ti_sci_cmd_get_resource_range;
+	rm_core_ops->get_range_from_shost =
+				ti_sci_cmd_get_resource_range_from_shost;
 }
 
 /**

drivers/firmware/ti_sci.h +42 -0

52 lines not on this page · raw message

include/linux/soc/ti/ti_sci_protocol.h +27 -0

index 515587e9d373..0c92a922db6a 100644
@@ -192,15 +192,42 @@ struct ti_sci_clk_ops {
 			u64 *current_freq);
 };
 
+/**
+ * struct ti_sci_rm_core_ops - Resource management core operations
+ * @get_range:		Get a range of resources belonging to ti sci host.
+ * @get_rage_from_shost:	Get a range of resources belonging to
+ *				specified host id.
+ *			- s_host: Host processing entity to which the
+ *				  resources are allocated
+ *
+ * NOTE: for these functions, all the parameters are consolidated and defined
+ * as below:
+ * - handle:	Pointer to TISCI handle as retrieved by *ti_sci_get_handle
+ * - dev_id:	TISCI device ID.
+ * - subtype:	Resource assignment subtype that is being requested
+ *		from the given device.
+ * - range_start:	Start index of the resource range
+ * - range_end:		Number of resources in the range
+ */
+struct ti_sci_rm_core_ops {
+	int (*get_range)(const struct ti_sci_handle *handle, u32 dev_id,
+			 u8 subtype, u16 *range_start, u16 *range_num);
+	int (*get_range_from_shost)(const struct ti_sci_handle *handle,
+				    u32 dev_id, u8 subtype, u8 s_host,
+				    u16 *range_start, u16 *range_num);
+};
+
 /**
  * struct ti_sci_ops - Function support for TI SCI
  * @dev_ops:	Device specific operations
  * @clk_ops:	Clock specific operations
+ * @rm_core_ops:	Resource management core operations.
  */
 struct ti_sci_ops {
 	struct ti_sci_core_ops core_ops;
 	struct ti_sci_dev_ops dev_ops;
 	struct ti_sci_clk_ops clk_ops;
+	struct ti_sci_rm_core_ops rm_core_ops;
 };
 
 /**

Signature · 1 line

2.19.1

Lokesh Vutla

author

2018-10-18 15:40 UTC #3 lore raw

PATCH v2 3/10 firmware: ti_sci: Add support for IRQ management

TISCI abstracts the handling of IRQ routes where interrupt sources are not directly connected to interrupt controller. Add support for the set of TISCI commands for requesting and releasing IRQs.

---
Changes since v1:
- None.

 drivers/firmware/ti_sci.c              | 446 +++++++++++++++++++++++++
 drivers/firmware/ti_sci.h              |  60 ++++
 include/linux/soc/ti/ti_sci_protocol.h |  77 +++++
 3 files changed, 583 insertions(+)

drivers/firmware/ti_sci.c +446 -0

index a2a099b8f62a..987bfb29475c 100644
@@ -1782,6 +1782,439 @@ int ti_sci_cmd_get_resource_range_from_shost(const struct ti_sci_handle *handle,
 					 range_start, range_num);
 }
 
+/**
+ * ti_sci_manage_irq() - Helper api to configure/release the irq route between
+ *			 the requested source and destination
+ * @handle:		Pointer to TISCI handle.
+ * @valid_params:	Bit fields defining the validity of certain params
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ * @type:		Request type irq set or release.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_manage_irq(const struct ti_sci_handle *handle,
+			     u32 valid_params, u16 src_id, u16 src_index,
+			     u16 dst_id, u16 dst_host_irq, u16 ia_id, u16 vint,
+			     u16 global_event, u8 vint_status_bit, u8 s_host,
+			     u16 type)
+{
+	struct ti_sci_msg_req_manage_irq *req;
+	struct ti_sci_msg_hdr *resp;
+	struct ti_sci_xfer *xfer;
+	struct ti_sci_info *info;
+	struct device *dev;
+	int ret = 0;
+
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
+	if (!handle)
+		return -EINVAL;
+
+	info = handle_to_ti_sci_info(handle);
+	dev = info->dev;
+
+	xfer = ti_sci_get_one_xfer(info, type, TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
+				   sizeof(*req), sizeof(*resp));
+	if (IS_ERR(xfer)) {
+		ret = PTR_ERR(xfer);
+		dev_err(dev, "Message alloc failed(%d)\n", ret);
+		return ret;
+	}
+	req = (struct ti_sci_msg_req_manage_irq *)xfer->xfer_buf;
+	req->valid_params = valid_params;
+	req->src_id = src_id;
+	req->src_index = src_index;
+	req->dst_id = dst_id;
+	req->dst_host_irq = dst_host_irq;
+	req->ia_id = ia_id;
+	req->vint = vint;
+	req->global_event = global_event;
+	req->vint_status_bit = vint_status_bit;
+	req->secondary_host = s_host;
+
+	ret = ti_sci_do_xfer(info, xfer);
+	if (ret) {
+		dev_err(dev, "Mbox send fail %d\n", ret);
+		goto fail;
+	}
+
+	resp = (struct ti_sci_msg_hdr *)xfer->xfer_buf;
+
+	ret = ti_sci_is_response_ack(resp) ? 0 : -ENODEV;
+
+fail:
+	ti_sci_put_one_xfer(&info->minfo, xfer);
+
+	return ret;
+}
+
+/**
+ * ti_sci_set_irq() - Helper api to configure the irq route between the
+ *		      requested source and destination
+ * @handle:		Pointer to TISCI handle.
+ * @valid_params:	Bit fields defining the validity of certain params
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_set_irq(const struct ti_sci_handle *handle, u32 valid_params,
+			  u16 src_id, u16 src_index, u16 dst_id,
+			  u16 dst_host_irq, u16 ia_id, u16 vint,
+			  u16 global_event, u8 vint_status_bit, u8 s_host)
+{
+	pr_debug("%s: IRQ set with valid_params = 0x%x from src = %d, index = %d, to dst = %d, irq = %d,via ia_id = %d, vint = %d, global event = %d,status_bit = %d\n",
+		 __func__, valid_params, src_id, src_index,
+		 dst_id, dst_host_irq, ia_id, vint, global_event,
+		 vint_status_bit);
+
+	return ti_sci_manage_irq(handle, valid_params, src_id, src_index,
+				 dst_id, dst_host_irq, ia_id, vint,
+				 global_event, vint_status_bit, s_host,
+				 TI_SCI_MSG_SET_IRQ);
+}
+
+/**
+ * ti_sci_free_irq() - Helper api to free the irq route between the
+ *			   requested source and destination
+ * @handle:		Pointer to TISCI handle.
+ * @valid_params:	Bit fields defining the validity of certain params
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_free_irq(const struct ti_sci_handle *handle, u32 valid_params,
+			   u16 src_id, u16 src_index, u16 dst_id,
+			   u16 dst_host_irq, u16 ia_id, u16 vint,
+			   u16 global_event, u8 vint_status_bit, u8 s_host)
+{
+	pr_debug("%s: IRQ release with valid_params = 0x%x from src = %d, index = %d, to dst = %d, irq = %d,via ia_id = %d, vint = %d, global event = %d,status_bit = %d\n",
+		 __func__, valid_params, src_id, src_index,
+		 dst_id, dst_host_irq, ia_id, vint, global_event,
+		 vint_status_bit);
+
+	return ti_sci_manage_irq(handle, valid_params, src_id, src_index,
+				 dst_id, dst_host_irq, ia_id, vint,
+				 global_event, vint_status_bit, s_host,
+				 TI_SCI_MSG_FREE_IRQ);
+}
+
+/**
+ * ti_sci_cmd_set_direct_irq() - Configure a non-event based direct irq route
+ *				 between the requested source and destination.
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_cmd_set_direct_irq(const struct ti_sci_handle *handle,
+				     u16 src_id, u16 src_index, u16 dst_id,
+				     u16 dst_host_irq)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID | MSG_FLAG_DST_HOST_IRQ_VALID;
+
+	return ti_sci_set_irq(handle, valid_params, src_id, src_index,
+			      dst_id, dst_host_irq, 0, 0, 0, 0, 0);
+}
+
+/**
+ * ti_sci_cmd_set_event_irq() - Configure an event based irq route between the
+ *				requested source and destination
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_cmd_set_event_irq(const struct ti_sci_handle *handle,
+				    u16 src_id, u16 src_index, u16 dst_id,
+				    u16 dst_host_irq, u16 ia_id, u16 vint,
+				    u16 global_event, u8 vint_status_bit)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID |
+			   MSG_FLAG_DST_HOST_IRQ_VALID | MSG_FLAG_IA_ID_VALID |
+			   MSG_FLAG_VINT_VALID | MSG_FLAG_GLB_EVNT_VALID |
+			   MSG_FLAG_VINT_STS_BIT_VALID;
+
+	return ti_sci_set_irq(handle, valid_params, src_id, src_index, dst_id,
+			      dst_host_irq, ia_id, vint, global_event,
+			      vint_status_bit, 0);
+}
+
+/**
+ * ti_sci_cmd_set_direct_irq_from_shost() - Configure a non-event based direct
+ *					    irq route between the source and
+ *					    destination belonging to a
+ *					    specified host.
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static
+int ti_sci_cmd_set_direct_irq_from_shost(const struct ti_sci_handle *handle,
+					 u16 src_id, u16 src_index, u16 dst_id,
+					 u16 dst_host_irq, u8 s_host)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID | MSG_FLAG_DST_HOST_IRQ_VALID |
+			   MSG_FLAG_SHOST_VALID;
+
+	return ti_sci_set_irq(handle, valid_params, src_id, src_index,
+			      dst_id, dst_host_irq, 0, 0, 0, 0, s_host);
+}
+
+/**
+ * ti_sci_cmd_set_event_irq_from_shost() - Configure an event based irq
+ *					   route between the source and
+ *					   destination belonging to a
+ *					   specified host.
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static
+int ti_sci_cmd_set_event_irq_from_shost(const struct ti_sci_handle *handle,
+					u16 src_id, u16 src_index, u16 dst_id,
+					u16 dst_host_irq, u16 ia_id, u16 vint,
+					u16 global_event, u8 vint_status_bit,
+					u8 s_host)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID |
+			   MSG_FLAG_DST_HOST_IRQ_VALID | MSG_FLAG_IA_ID_VALID |
+			   MSG_FLAG_VINT_VALID | MSG_FLAG_GLB_EVNT_VALID |
+			   MSG_FLAG_VINT_STS_BIT_VALID | MSG_FLAG_SHOST_VALID;
+
+	return ti_sci_set_irq(handle, valid_params, src_id, src_index,
+			      dst_id, dst_host_irq, ia_id, vint,
+			      global_event, vint_status_bit, s_host);
+}
+
+/**
+ * ti_sci_cmd_set_event_irq_to_poll() - Configure an event based irq
+ *					in polling mode
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_cmd_set_event_irq_to_poll(const struct ti_sci_handle *handle,
+					    u16 src_id, u16 src_index,
+					    u16 ia_id, u16 vint,
+					    u16 global_event,
+					    u8 vint_status_bit)
+{
+	u32 valid_params = MSG_FLAG_IA_ID_VALID | MSG_FLAG_VINT_VALID |
+			   MSG_FLAG_GLB_EVNT_VALID |
+			   MSG_FLAG_VINT_STS_BIT_VALID;
+
+	return ti_sci_set_irq(handle, valid_params, src_id, src_index, 0, 0,
+			      ia_id, vint, global_event, vint_status_bit, 0);
+}
+
+/**
+ * ti_sci_cmd_free_direct_irq() - Free a non-event based direct irq route
+ *				  between the requested source and destination.
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_cmd_free_direct_irq(const struct ti_sci_handle *handle,
+				      u16 src_id, u16 src_index, u16 dst_id,
+				      u16 dst_host_irq)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID | MSG_FLAG_DST_HOST_IRQ_VALID;
+
+	return ti_sci_free_irq(handle, valid_params, src_id, src_index,
+			       dst_id, dst_host_irq, 0, 0, 0, 0, 0);
+}
+
+/**
+ * ti_sci_cmd_free_event_irq() - Free an event based irq route between the
+ *				 requested source and destination
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_cmd_free_event_irq(const struct ti_sci_handle *handle,
+				     u16 src_id, u16 src_index, u16 dst_id,
+				     u16 dst_host_irq, u16 ia_id, u16 vint,
+				     u16 global_event, u8 vint_status_bit)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID |
+			   MSG_FLAG_DST_HOST_IRQ_VALID | MSG_FLAG_IA_ID_VALID |
+			   MSG_FLAG_VINT_VALID | MSG_FLAG_GLB_EVNT_VALID |
+			   MSG_FLAG_VINT_STS_BIT_VALID;
+
+	return ti_sci_free_irq(handle, valid_params, src_id, src_index,
+			       dst_id, dst_host_irq, ia_id, vint,
+			       global_event, vint_status_bit, 0);
+}
+
+/**
+ * ti_sci_cmd_free_direct_irq_from_shost() - Free a non-event based direct irq
+ *					     route between the source and
+ *					     destination belonging to a
+ *					     specified host.
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static
+int ti_sci_cmd_free_direct_irq_from_shost(const struct ti_sci_handle *handle,
+					  u16 src_id, u16 src_index, u16 dst_id,
+					  u16 dst_host_irq, u8 s_host)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID | MSG_FLAG_DST_HOST_IRQ_VALID |
+			   MSG_FLAG_SHOST_VALID;
+
+	return ti_sci_free_irq(handle, valid_params, src_id, src_index,
+			       dst_id, dst_host_irq, 0, 0, 0, 0, s_host);
+}
+
+/**
+ * ti_sci_cmd_free_event_irq_from_shost() - Free an event based irq
+ *					    route between the source and
+ *					    destination belonging to a
+ *					    specified host.
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static
+int ti_sci_cmd_free_event_irq_from_shost(const struct ti_sci_handle *handle,
+					 u16 src_id, u16 src_index, u16 dst_id,
+					 u16 dst_host_irq, u16 ia_id, u16 vint,
+					 u16 global_event, u8 vint_status_bit,
+					 u8 s_host)
+{
+	u32 valid_params = MSG_FLAG_DST_ID_VALID |
+			   MSG_FLAG_DST_HOST_IRQ_VALID | MSG_FLAG_IA_ID_VALID |
+			   MSG_FLAG_VINT_VALID | MSG_FLAG_GLB_EVNT_VALID |
+			   MSG_FLAG_VINT_STS_BIT_VALID | MSG_FLAG_SHOST_VALID;
+
+	return ti_sci_free_irq(handle, valid_params, src_id, src_index,
+			       dst_id, dst_host_irq, ia_id, vint,
+			       global_event, vint_status_bit, s_host);
+}
+
+/**
+ * ti_sci_cmd_free_event_irq_to_poll() - Free an event based irq
+ *					 in polling mode
+ * @handle:		Pointer to TISCI handle.
+ * @src_id:		Device ID of the IRQ source
+ * @src_index:		IRQ source index within the source device
+ * @dst_id:		Device ID of the IRQ destination
+ * @dt_host_irq:	IRQ number of the destination device
+ * @ia_id:		Device ID of the IA, if the IRQ flows through this IA
+ * @vint:		Virtual interrupt to be used within the IA
+ * @global_event:	Global event number to be used for the requesting event
+ * @vint_status_bit:	Virtual interrupt status bit to be used for the event
+ * @s_host:		Secondary host ID to which the irq/event is being
+ *			requested for.
+ *
+ * Return: 0 if all went fine, else return appropriate error.
+ */
+static int ti_sci_cmd_free_event_irq_to_poll(const struct ti_sci_handle *handle,
+					     u16 src_id, u16 src_index,
+					     u16 ia_id, u16 vint,
+					     u16 global_event,
+					     u8 vint_status_bit)
+{
+	u32 valid_params = MSG_FLAG_IA_ID_VALID | MSG_FLAG_VINT_VALID |
+			   MSG_FLAG_GLB_EVNT_VALID |
+			   MSG_FLAG_VINT_STS_BIT_VALID;
+
+	return ti_sci_free_irq(handle, valid_params, src_id, src_index, 0, 0,
+			       ia_id, vint, global_event, vint_status_bit, 0);
+}
+
 /*
  * ti_sci_setup_ops() - Setup the operations structures
  * @info:	pointer to TISCI pointer

@@ -1793,6 +2226,7 @@ static void ti_sci_setup_ops(struct ti_sci_info *info)
 	struct ti_sci_dev_ops *dops = &ops->dev_ops;
 	struct ti_sci_clk_ops *cops = &ops->clk_ops;
 	struct ti_sci_rm_core_ops *rm_core_ops = &ops->rm_core_ops;
+	struct ti_sci_rm_irq_ops *iops = &ops->rm_irq_ops;
 
 	core_ops->reboot_device = ti_sci_cmd_core_reboot;

@@ -1827,6 +2261,18 @@ static void ti_sci_setup_ops(struct ti_sci_info *info)
 	rm_core_ops->get_range = ti_sci_cmd_get_resource_range;
 	rm_core_ops->get_range_from_shost =
 				ti_sci_cmd_get_resource_range_from_shost;
+
+	iops->set_direct_irq = ti_sci_cmd_set_direct_irq;
+	iops->set_event_irq = ti_sci_cmd_set_event_irq;
+	iops->set_direct_irq_from_shost = ti_sci_cmd_set_direct_irq_from_shost;
+	iops->set_event_irq_from_shost = ti_sci_cmd_set_event_irq_from_shost;
+	iops->set_event_irq_to_poll = ti_sci_cmd_set_event_irq_to_poll;
+	iops->free_direct_irq = ti_sci_cmd_free_direct_irq;
+	iops->free_event_irq = ti_sci_cmd_free_event_irq;
+	iops->free_direct_irq_from_shost =
+					ti_sci_cmd_free_direct_irq_from_shost;
+	iops->free_event_irq_from_shost = ti_sci_cmd_free_event_irq_from_shost;
+	iops->free_event_irq_to_poll = ti_sci_cmd_free_event_irq_to_poll;
 }
 
 /**

drivers/firmware/ti_sci.h +60 -0

70 lines not on this page · raw message

include/linux/soc/ti/ti_sci_protocol.h +77 -0

94 lines not on this page · raw message

Signature · 1 line

2.19.1

Lokesh Vutla

author

2018-10-18 15:40 UTC #4 lore raw

PATCH v2 4/10 firmware: ti_sci: Add RM mapping table for am654

From: Peter Ujfalusi <[email protected]>

Add the resource mapping table for AM654 SoC as defined
in http://downloads.ti.com/tisci/esd/latest/5_soc_doc/am6x/resasg_types.html
Introduce a new compatible for AM654 "ti,am654-sci" for using this resource map table.

---
Changes since v1:
- new patch added.

 .../bindings/arm/keystone/ti,sci.txt          |  3 ++-
 drivers/firmware/ti_sci.c                     | 23 +++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/keystone/ti,sci.txt +2 -1

index b56a02c10ae6..6f0cd31c1520 100644
@@ -24,7 +24,8 @@ relationship between the TI-SCI parent node to the child node.
 
 Required properties:
 -------------------
-- compatible: should be "ti,k2g-sci"
+- compatible:	should be "ti,k2g-sci" for TI 66AK2G SoC
+		should be "ti,am654-sci" for for TI AM654 SoC
 - mbox-names:
 	"rx" - Mailbox corresponding to receive path
 	"tx" - Mailbox corresponding to transmit path

drivers/firmware/ti_sci.c +23 -0

index 987bfb29475c..c2f0815edab6 100644
@@ -2500,10 +2500,33 @@ static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
 	/* Limited by MBOX_TX_QUEUE_LEN. K2G can handle upto 128 messages! */
 	.max_msgs = 20,
 	.max_msg_size = 64,
+	.rm_type_map = NULL,
+};
+
+static struct ti_sci_rm_type_map ti_sci_am654_rm_type_map[] = {
+	{.dev_id = 56, .type = 0x00b}, /* GIC_IRQ */
+	{.dev_id = 179, .type = 0x000}, /* MAIN_NAV_UDMASS_IA0 */
+	{.dev_id = 187, .type = 0x009}, /* MAIN_NAV_RA */
+	{.dev_id = 188, .type = 0x006}, /* MAIN_NAV_UDMAP */
+	{.dev_id = 194, .type = 0x007}, /* MCU_NAV_UDMAP */
+	{.dev_id = 195, .type = 0x00a}, /* MCU_NAV_RA */
+	{.dev_id = 0, .type = 0x000}, /* end of table */
+};
+
+/* Description for AM654 */
+static const struct ti_sci_desc ti_sci_pmmc_am654_desc = {
+	.default_host_id = 12,
+	/* Conservative duration */
+	.max_rx_timeout_ms = 10000,
+	/* Limited by MBOX_TX_QUEUE_LEN. K2G can handle upto 128 messages! */
+	.max_msgs = 20,
+	.max_msg_size = 60,
+	.rm_type_map = ti_sci_am654_rm_type_map,
 };
 
 static const struct of_device_id ti_sci_of_match[] = {
 	{.compatible = "ti,k2g-sci", .data = &ti_sci_pmmc_k2g_desc},
+	{.compatible = "ti,am654-sci", .data = &ti_sci_pmmc_am654_desc},
 	{ /* Sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, ti_sci_of_match);

Signature · 1 line

2.19.1

Rob Herring

maintainer Reviewed-by on v2 4/10

2018-10-18 20:42 UTC #5 lore raw

Re: [PATCH v2 04/10] firmware: ti_sci: Add RM mapping table for am654

Quoted 9 lines · Lokesh Vutla

From: Peter Ujfalusi <[email protected]>
Add the resource mapping table for AM654 SoC as defined
in http://downloads.ti.com/tisci/esd/latest/5_soc_doc/am6x/resasg_types.html
Introduce a new compatible for AM654 "ti,am654-sci" for using this resource map table.
Signed-off-by Peter Ujfalusi <[email protected]>
Signed-off-by Lokesh Vutla <[email protected]>

Quoted 8 lines

---
Changes since v1:
- new patch added.

 .../bindings/arm/keystone/ti,sci.txt          |  3 ++-
 drivers/firmware/ti_sci.c                     | 23 +++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Reviewed-by Rob Herring <[email protected]>

Lokesh Vutla

author

2018-10-18 15:40 UTC #6 lore raw

PATCH v2 5/10 firmware: ti_sci: Add helper apis to manage resources

Each resource with in the device can be uniquely identified by a type and subtype as defined by TISCI. Since this is generic across the devices, resource allocation also can be made generic instead of each client driver handling the resource. So add helper apis to manage the resource.

---
Changes since v1:
- Updated to use RM mapping table
- AM6 SoC information is derived from here[1]
[1] http://downloads.ti.com/tisci/esd/latest/5_soc_doc/am6x/resasg_types.html

 drivers/firmware/ti_sci.c              | 128 +++++++++++++++++++++++++
 include/linux/soc/ti/ti_sci_protocol.h |  48 ++++++++++
 2 files changed, 176 insertions(+)

drivers/firmware/ti_sci.c +128 -0

index c2f0815edab6..581d7a151533 100644
@@ -2480,6 +2480,134 @@ const struct ti_sci_handle *devm_ti_sci_get_by_phandle(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_ti_sci_get_by_phandle);
 
+/*
+ * ti_sci_get_free_resource() - Get a free resource from TISCI resource.
+ * @res:	Pointer to the TISCI resource
+ *
+ * Return: resource num if all went ok else TI_SCI_RESOURCE_NULL.
+ */
+u16 ti_sci_get_free_resource(struct ti_sci_resource *res)
+{
+	unsigned long flags;
+	u16 set, free_bit;
+
+	raw_spin_lock_irqsave(&res->lock, flags);
+	for (set = 0; set < res->sets; set++) {
+		free_bit = find_first_zero_bit(res->desc[set].res_map,
+					       res->desc[set].num);
+		if (free_bit != res->desc[set].num) {
+			set_bit(free_bit, res->desc[set].res_map);
+			raw_spin_unlock_irqrestore(&res->lock, flags);
+			return res->desc[set].start + free_bit;
+		}
+	}
+	raw_spin_unlock_irqrestore(&res->lock, flags);
+
+	return TI_SCI_RESOURCE_NULL;
+}
+EXPORT_SYMBOL_GPL(ti_sci_get_free_resource);
+
+/**
+ * ti_sci_release_resource() - Release a resource from TISCI resource.
+ * @res:	Pointer to the TISCI resource
+ */
+void ti_sci_release_resource(struct ti_sci_resource *res, u16 id)
+{
+	unsigned long flags;
+	u16 set;
+
+	raw_spin_lock_irqsave(&res->lock, flags);
+	for (set = 0; set < res->sets; set++) {
+		if (res->desc[set].start <= id &&
+		    (res->desc[set].num + res->desc[set].start) > id)
+			clear_bit(id - res->desc[set].start,
+				  res->desc[set].res_map);
+	}
+	raw_spin_unlock_irqrestore(&res->lock, flags);
+}
+EXPORT_SYMBOL_GPL(ti_sci_release_resource);
+
+/**
+ * devm_ti_sci_get_of_resource() - Get a TISCI resource assigned to a device
+ * @handle:	TISCI handle
+ * @dev:	Device pointer to which the resource is assigned
+ * @of_prop:	property name by which the resource are represented
+ *
+ * Note: This function expects of_prop to be in the form of tuples
+ *	<type, subtype>. Allocates and initializes ti_sci_resource structure
+ *	for each of_prop. Client driver can directly call
+ *	ti_sci_(get_free, release)_resource apis for handling the resource.
+ *
+ * Return: Pointer to ti_sci_resource if all went well else appropriate
+ *	   error pointer.
+ */
+struct ti_sci_resource *
+devm_ti_sci_get_of_resource(const struct ti_sci_handle *handle,
+			    struct device *dev, u32 dev_id, char *of_prop)
+{
+	u32 resource_subtype;
+	u16 resource_type;
+	struct ti_sci_resource *res;
+	int sets, i, ret;
+
+	res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return ERR_PTR(-ENOMEM);
+
+	sets = of_property_count_elems_of_size(dev_of_node(dev), of_prop,
+					       sizeof(u32));
+	if (sets < 0) {
+		dev_err(dev, "%s resource type ids not available\n", of_prop);
+		return ERR_PTR(sets);
+	}
+
+	res->sets = sets;
+
+	res->desc = devm_kcalloc(dev, res->sets, sizeof(*res->desc),
+				 GFP_KERNEL);
+	if (!res->desc)
+		return ERR_PTR(-ENOMEM);
+
+	ret = ti_sci_get_resource_type(handle_to_ti_sci_info(handle), dev_id,
+				       &resource_type);
+	if (ret) {
+		dev_err(dev, "No valid resource type for %u\n", dev_id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	for (i = 0; i < res->sets; i++) {
+		ret = of_property_read_u32_index(dev_of_node(dev), of_prop, i,
+						 &resource_subtype);
+		if (ret)
+			return ERR_PTR(-EINVAL);
+
+		ret = handle->ops.rm_core_ops.get_range(handle, dev_id,
+							resource_subtype,
+							&res->desc[i].start,
+							&res->desc[i].num);
+		if (ret) {
+			dev_err(dev, "type %d subtype %d not allocated for host %d\n",
+				resource_type, resource_subtype,
+				handle_to_ti_sci_info(handle)->host_id);
+			return ERR_PTR(ret);
+		}
+
+		dev_dbg(dev, "res type = %d, subtype = %d, start = %d, num = %d\n",
+			resource_type, resource_subtype, res->desc[i].start,
+			res->desc[i].num);
+
+		res->desc[i].res_map =
+			devm_kzalloc(dev, BITS_TO_LONGS(res->desc[i].num) *
+				     sizeof(*res->desc[i].res_map), GFP_KERNEL);
+		if (!res->desc[i].res_map)
+			return ERR_PTR(-ENOMEM);
+	}
+	raw_spin_lock_init(&res->lock);
+
+	return res;
+}
+EXPORT_SYMBOL_GPL(devm_ti_sci_get_of_resource);
+
 static int tisci_reboot_handler(struct notifier_block *nb, unsigned long mode,
 				void *cmd)
 {

include/linux/soc/ti/ti_sci_protocol.h +48 -0

66 lines not on this page · raw message

Signature · 1 line

2.19.1

Lokesh Vutla

author

2018-10-18 15:40 UTC #7 lore raw

PATCH v2 6/10 dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

Add the DT binding documentation for Interrupt router driver.

---
Changes since v1:
- Drop dependency on GIC
- Updated supported interrupt types.

 .../interrupt-controller/ti,sci-intr.txt      | 81 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +81 -0

new file mode 100644
index 000000000000..276bb4f0ad12
@@ -0,0 +1,81 @@
+Texas Instruments K3 Interrupt Router
+=====================================
+
+The Interrupt Router (INTR) module provides a mechanism to mux M
+interrupt inputs to N interrupt outputs, where all M inputs are selectable
+to be driven per N output. There is one register per output (MUXCNTL_N) that
+controls the selection.
+
+
+                                 Interrupt Router
+                             +----------------------+
+                             |  Inputs     Outputs  |
+        +-------+            | +------+             |
+        | GPIO  |----------->| | irq0 |             |       Host IRQ
+        +-------+            | +------+             |      controller
+                             |    .        +-----+  |      +-------+
+        +-------+            |    .        |  0  |  |----->|  IRQ  |
+        | INTA  |----------->|    .        +-----+  |      +-------+
+        +-------+            |    .          .      |
+                             | +------+      .      |
+                             | | irqM |    +-----+  |
+                             | +------+    |  N  |  |
+                             |             +-----+  |
+                             +----------------------+
+
+Configuration of these MUXCNTL_N registers is done by a system controller
+(like the Device Memory and Security Controller on K3 AM654 SoC). System
+controller will keep track of the used and unused registers within the Router.
+Driver should request the system controller to get the range of GIC IRQs
+assigned to the requesting hosts. It is the drivers responsibility to keep
+track of Host IRQs.
+
+Communication between the host processor running an OS and the system
+controller happens through a protocol called TI System Control Interface
+(TISCI protocol). For more details refer:
+Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
+
+TISCI Interrupt Router Node:
+----------------------------
+- compatible:		Must be "ti,sci-intr".
+- interrupt-controller:	Identifies the node as an interrupt controller
+- #interrupt-cells:	Specifies the number of cells needed to encode an
+			interrupt source. The value should be 3.
+			First cell should contain the TISCI device ID of source
+			Second cell should contain the interrupt source offset
+			within the device
+			Third cell specifies the trigger type as defined
+			in interrupts.txt in this directory. Only level
+			sensitive trigger types are supported.
+- interrupt-parent:	phandle of irq parent for TISCI intr.
+- ti,sci:		Phandle to TI-SCI compatible System controller node.
+- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
+- ti,sci-rm-range-girq:	TISCI subtype id representing the host irqs assigned
+			to this interrupt router.
+
+Example:
+--------
+The following example demonstrates both interrupt router node and the consumer
+node(main gpio) on the AM654 SoC:
+
+main_intr: interrupt-controller at 1 {
+	compatible = "ti,sci-intr";
+	interrupt-controller;
+	interrupt-parent = <&gic>;
+	#interrupt-cells = <3>;
+	ti,sci = <&dmsc>;
+	ti,sci-dst-id = <56>;
+	ti,sci-rm-range-girq = <0x1>;
+};
+
+main_gpio0:  main_gpio0 at 600000 {
+	...
+	interrupt-parent = <&main_intr>;
+	interrupts = <57 256 IRQ_TYPE_EDGE_RISING>,
+			<57 257 IRQ_TYPE_EDGE_RISING>,
+			<57 258 IRQ_TYPE_EDGE_RISING>,
+			<57 259 IRQ_TYPE_EDGE_RISING>,
+			<57 260 IRQ_TYPE_EDGE_RISING>,
+			<57 261 IRQ_TYPE_EDGE_RISING>;
+	...
+};

MAINTAINERS +1 -0

index 06966772cad4..710cf728b9d0 100644
@@ -14686,6 +14686,7 @@ F:	Documentation/devicetree/bindings/reset/ti,sci-reset.txt
 F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F:	drivers/clk/keystone/sci-clk.c
 F:	drivers/reset/reset-ti-sci.c
+F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M:	Hans Verkuil <[email protected]>

Signature · 1 line

2.19.1

Rob Herring

maintainer

2018-10-25 18:45 UTC #8 lore raw

Re: [PATCH v2 06/10] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

Quoted 59 lines · Lokesh Vutla

Add the DT binding documentation for Interrupt router driver.

---
Changes since v1:
- Drop dependency on GIC
- Updated supported interrupt types.

 .../interrupt-controller/ti,sci-intr.txt      | 81 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +40 -0

new file mode 100644
index 000000000000..276bb4f0ad12
@@ -0,0 +1,81 @@
+Texas Instruments K3 Interrupt Router
+=====================================
+
+The Interrupt Router (INTR) module provides a mechanism to mux M
+interrupt inputs to N interrupt outputs, where all M inputs are selectable
+to be driven per N output. There is one register per output (MUXCNTL_N) that
+controls the selection.
+
+
+                                 Interrupt Router
+                             +----------------------+
+                             |  Inputs     Outputs  |
+        +-------+            | +------+             |
+        | GPIO  |----------->| | irq0 |             |       Host IRQ
+        +-------+            | +------+             |      controller
+                             |    .        +-----+  |      +-------+
+        +-------+            |    .        |  0  |  |----->|  IRQ  |
+        | INTA  |----------->|    .        +-----+  |      +-------+
+        +-------+            |    .          .      |
+                             | +------+      .      |
+                             | | irqM |    +-----+  |
+                             | +------+    |  N  |  |
+                             |             +-----+  |
+                             +----------------------+
+
+Configuration of these MUXCNTL_N registers is done by a system controller
+(like the Device Memory and Security Controller on K3 AM654 SoC). System
+controller will keep track of the used and unused registers within the Router.
+Driver should request the system controller to get the range of GIC IRQs
+assigned to the requesting hosts. It is the drivers responsibility to keep
+track of Host IRQs.
+
+Communication between the host processor running an OS and the system
+controller happens through a protocol called TI System Control Interface
+(TISCI protocol). For more details refer:
+Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
+
+TISCI Interrupt Router Node:
+----------------------------
+- compatible:		Must be "ti,sci-intr".

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt line 50

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +10 -0

+- interrupt-controller:	Identifies the node as an interrupt controller
+- #interrupt-cells:	Specifies the number of cells needed to encode an
+			interrupt source. The value should be 3.
+			First cell should contain the TISCI device ID of source
+			Second cell should contain the interrupt source offset
+			within the device
+			Third cell specifies the trigger type as defined
+			in interrupts.txt in this directory. Only level
+			sensitive trigger types are supported.
+- interrupt-parent:	phandle of irq parent for TISCI intr.

This is implied and could be in a parent node.

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt line 54

Quoted 4 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +4 -0

+- ti,sci:		Phandle to TI-SCI compatible System controller node.
+- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
+- ti,sci-rm-range-girq:	TISCI subtype id representing the host irqs assigned
+			to this interrupt router.

These need a better explanation and there's still some questions on v1 asked of me that I tried to answer.

Quoted 7 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +7 -0

+
+Example:
+--------
+The following example demonstrates both interrupt router node and the consumer
+node(main gpio) on the AM654 SoC:
+
+main_intr: interrupt-controller at 1 {

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt line 71

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +10 -0

+	compatible = "ti,sci-intr";
+	interrupt-controller;
+	interrupt-parent = <&gic>;
+	#interrupt-cells = <3>;
+	ti,sci = <&dmsc>;
+	ti,sci-dst-id = <56>;
+	ti,sci-rm-range-girq = <0x1>;
+};
+
+main_gpio0:  main_gpio0 at 600000 {

gpio at ...

Quoted 25 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +10 -0

+	...
+	interrupt-parent = <&main_intr>;
+	interrupts = <57 256 IRQ_TYPE_EDGE_RISING>,
+			<57 257 IRQ_TYPE_EDGE_RISING>,
+			<57 258 IRQ_TYPE_EDGE_RISING>,
+			<57 259 IRQ_TYPE_EDGE_RISING>,
+			<57 260 IRQ_TYPE_EDGE_RISING>,
+			<57 261 IRQ_TYPE_EDGE_RISING>;
+	...
+};

MAINTAINERS +1 -0

index 06966772cad4..710cf728b9d0 100644
@@ -14686,6 +14686,7 @@ F:	Documentation/devicetree/bindings/reset/ti,sci-reset.txt
 F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F:	drivers/clk/keystone/sci-clk.c
 F:	drivers/reset/reset-ti-sci.c
+F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M:	Hans Verkuil <[email protected]>

Signature · 1 line

2.19.1

Lokesh Vutla

author

2018-10-26 06:38 UTC #9 lore raw

Re: [PATCH v2 06/10] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings

Hi Rob,

Quoted 64 lines · Rob Herring

Quoted 63 lines · Lokesh Vutla

Add the DT binding documentation for Interrupt router driver.

[1] http://downloads.ti.com/tisci/esd/latest/5_soc_doc/am6x/hosts.html
[2] http://downloads.ti.com/tisci/esd/latest/5_soc_doc/am6x/resasg_types.html

---
Changes since v1:
- Drop dependency on GIC
- Updated supported interrupt types.

  .../interrupt-controller/ti,sci-intr.txt      | 81 +++++++++++++++++++
  MAINTAINERS                                   |  1 +
  2 files changed, 82 insertions(+)

create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +44 -0

new file mode 100644
index 000000000000..276bb4f0ad12
@@ -0,0 +1,81 @@
+Texas Instruments K3 Interrupt Router
+=====================================
+
+The Interrupt Router (INTR) module provides a mechanism to mux M
+interrupt inputs to N interrupt outputs, where all M inputs are selectable
+to be driven per N output. There is one register per output (MUXCNTL_N) that
+controls the selection.
+
+
+                                 Interrupt Router
+                             +----------------------+
+                             |  Inputs     Outputs  |
+        +-------+            | +------+             |
+        | GPIO  |----------->| | irq0 |             |       Host IRQ
+        +-------+            | +------+             |      controller
+                             |    .        +-----+  |      +-------+
+        +-------+            |    .        |  0  |  |----->|  IRQ  |
+        | INTA  |----------->|    .        +-----+  |      +-------+
+        +-------+            |    .          .      |
+                             | +------+      .      |
+                             | | irqM |    +-----+  |
+                             | +------+    |  N  |  |
+                             |             +-----+  |
+                             +----------------------+
+
+Configuration of these MUXCNTL_N registers is done by a system controller
+(like the Device Memory and Security Controller on K3 AM654 SoC). System
+controller will keep track of the used and unused registers within the Router.
+Driver should request the system controller to get the range of GIC IRQs
+assigned to the requesting hosts. It is the drivers responsibility to keep
+track of Host IRQs.
+
+Communication between the host processor running an OS and the system
+controller happens through a protocol called TI System Control Interface
+(TISCI protocol). For more details refer:
+Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
+
+TISCI Interrupt Router Node:
+----------------------------
+- compatible:		Must be "ti,sci-intr".
+- interrupt-controller:	Identifies the node as an interrupt controller
+- #interrupt-cells:	Specifies the number of cells needed to encode an
+			interrupt source. The value should be 3.
+			First cell should contain the TISCI device ID of source

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt line 50
Quoted 6 lines
Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +6 -0
+			Second cell should contain the interrupt source offset
+			within the device
+			Third cell specifies the trigger type as defined
+			in interrupts.txt in this directory. Only level
+			sensitive trigger types are supported.
+- interrupt-parent:	phandle of irq parent for TISCI intr.
This is implied and could be in a parent node.

okay, will drop it in next version.

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt line 54
Quoted 5 lines
Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +4 -0
+- ti,sci:		Phandle to TI-SCI compatible System controller node.
+- ti,sci-dst-id:	TISCI device ID of the destination IRQ controller.
+- ti,sci-rm-range-girq:	TISCI subtype id representing the host irqs assigned
+			to this interrupt router.
These need a better explanation and there's still some questions on v1 asked of me that I tried to answer.

Before I jump into the details, I would like to provide a brief on TISCI resource management:

- Host_id: Typically it is the representation of the host processing entities (example: A53 cores running in a VM) as identified by the TISCI.[1]
- Device_id: Each Device in SoC is uniquely identified by TISCI using an ID.
- Each device has Resources like interrupts, DMA channels etc. A simple example would be Interrupt Router and GIC($subject). There are n physical GIC interrupts connected to Interrupt Router. Such resources are are uniquely identified by TISCI using a type ID.[2]

For the sake of simplicity lets consider an Interrupt Router(IR) to which GIC line 32-63 are connected. Considering Isolation for each VM in picture, TISCI allows for a certain range within [32-63] to be assigned to a specific Host_ID. This is mainly to provide the ability for OSs running in virtual machines to be able to independently communicate with the firmware without the need going through a hypervisor.

- Now for Linux to know the GIC irq range that can be used by this Interrupt router, IR driver should send a message to system-controller using TISCI protocol with the resource type as parameter.
- For configuring the IRQ,(i.e. attaching an input to IR to a GIC irq), IR driver should send a message to system-controller using TISCI protocol with gic-device-id and an irq from the provided range as parameters.

For covering the above two scenarios, ti,sci-dst-id and ti,sci-rm-range-girq is introduced in DT.

Quoted 12 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +11 -0

+
+Example:
+--------
+The following example demonstrates both interrupt router node and the consumer
+node(main gpio) on the AM654 SoC:
+
+main_intr: interrupt-controller at 1 {
+	compatible = "ti,sci-intr";
+	interrupt-controller;
+	interrupt-parent = <&gic>;
+	#interrupt-cells = <3>;

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt line 71
Quoted 6 lines
Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt new file +6 -0
+	ti,sci = <&dmsc>;
+	ti,sci-dst-id = <56>;
+	ti,sci-rm-range-girq = <0x1>;
+};
+
+main_gpio0:  main_gpio0 at 600000 {
gpio at ...

Sure, will fix it in next version.

Thanks and regards,
Lokesh

Lokesh Vutla

author

2018-10-18 15:40 UTC #10 lore raw

PATCH v2 7/10 irqchip: ti-sci-intr: Add support for Interrupt Router driver

Texas Instruments' K3 generation SoCs has an IP Interrupt Router that does allows for multiplexing of input interrupts to host interrupt controller. Interrupt Router inputs are either from a peripheral or from an Interrupt Aggregator which is another interrupt controller.

Configuration of the interrupt router registers can only be done by a system co-processor and the driver needs to send a message to this co processor over TISCI protocol.

Add support for Interrupt Router driver over TISCI protocol.

---
Changes since v1:
- made Kconfig symbol as non selectable by user.
- Dropped using ti_sci_intr_irq_desc structure.
- Fixup error path in allocate_gic_irq.
- Updated handling of Interrupt Aggregator as child controller.

Note: Did not add system firmware helper apis as I did not get a response from
firmware maintainers. Also I see other firmware drivers(like arm scmi[1]) using the same approach. Will wait for some more time to get feedback from tisci maintainers.

[1] include/linux/scmi_protocol.h

 MAINTAINERS                       |   1 +
 drivers/irqchip/Kconfig           |  11 ++
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-ti-sci-intr.c | 302 ++++++++++++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 drivers/irqchip/irq-ti-sci-intr.c

MAINTAINERS +1 -0

index 710cf728b9d0..4f1bb64a15a1 100644
@@ -14687,6 +14687,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F:	drivers/clk/keystone/sci-clk.c
 F:	drivers/reset/reset-ti-sci.c
 F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
+F:	drivers/irqchip/irq-ti-sci-intr.c
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M:	Hans Verkuil <[email protected]>

drivers/irqchip/Kconfig +11 -0

index 96451b581452..f6620a6bb872 100644
@@ -374,6 +374,17 @@ config QCOM_PDC
 	  Power Domain Controller driver to manage and configure wakeup
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
+config TI_SCI_INTR_IRQCHIP
+	bool
+	depends on TI_SCI_PROTOCOL && ARCH_K3
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables the irqchip driver support for K3 Interrupt router
+	  over TI System Control Interface available on some new TI's SoCs.
+	  If you wish to use interrupt router irq resources managed by the
+	  TI System Controller, say Y here. Otherwise, say N.
+
 endmenu
 
 config SIFIVE_PLIC

drivers/irqchip/Makefile +1 -0

index b822199445ff..44bf65606d60 100644
@@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
+obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o

drivers/irqchip/irq-ti-sci-intr.c new file +302 -0

302 lines not on this page · raw message

Signature · 1 line

2.19.1

Lokesh Vutla

author

2018-10-18 15:40 UTC #11 lore raw

PATCH v2 8/10 dt-bindings: irqchip: Introduce TISCI Interrupt Aggregator bindings

Add the DT binding documentation for Interrupt Aggregator driver.

---
Changes since v1:
- New patch

 .../interrupt-controller/ti,sci-inta.txt      | 74 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt

Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt new file +74 -0

new file mode 100644
index 000000000000..17b1fbd90312
@@ -0,0 +1,74 @@
+Texas Instruments K3 Interrupt Aggregator
+=========================================
+
+The Interrupt Aggregator (INTA) provides a centralized machine
+which handles the termination of system events to that they can
+be coherently processed by the host(s) in the system. A maximum
+of 64 events can be mapped to a single interrupt.
+
+
+                              Interrupt Aggregator
+                     +-----------------------------------------+
+                     |      Intmap            VINT             |
+                     | +--------------+  +------------+        |
+            m ------>| | vint  | bit  |  | 0 |.....|63| vint0  |
+               .     | +--------------+  +------------+        |       +------+
+               .     |         .               .               |       | HOST |
+Globalevents  ------>|         .               .               |------>| IRQ  |
+               .     |         .               .               |       | CTRL |
+               .     |         .               .               |       +------+
+            n ------>| +--------------+  +------------+        |
+                     | | vint  | bit  |  | 0 |.....|63| vintx  |
+                     | +--------------+  +------------+        |
+                     |                                         |
+                     +-----------------------------------------+
+
+Configuration of these Intmap registers that maps global events to vint is done
+by a system controller (like the Device Memory and Security Controller on K3
+AM654 SoC). Driver should request the system controller to get the range
+of global events and vints assigned to the requesting host. Management
+of these requested resources should be handled by driver and requests
+system controller to map specific global event to vint, bit pair.
+
+Communication between the host processor running an OS and the system
+controller happens through a protocol called TI System Control Interface
+(TISCI protocol). For more details refer:
+Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
+
+TISCI Interrupt Aggregator Node:
+-------------------------------
+- compatible:		Must be "ti,sci-inta".
+- reg:			Should contain registers location and length.
+- interrupt-controller:	Identifies the node as an interrupt controller
+- #interrupt-cells:	Specifies the number of cells needed to encode an
+			interrupt source. The value should be 4.
+			First cell should contain the TISCI device ID of source
+			Second cell should contain the event source offset
+			within the device
+			Third cell specified the interrupt number(vint)
+			reaching Interrupt aggregator.
+			Fourth cell specifies the trigger type as defined
+			in interrupts.txt in this directory.
+- interrupt-parent:	phandle of irq parent for TISCI intr.
+- ti,sci:		Phandle to TI-SCI compatible System controller node.
+- ti,sci-dev-id:	TISCI device ID of the Interrupt Aggregator.
+- ti,sci-rm-range-vint:	TISCI subtype id representing the virtual interrupts
+			(vints) range within this IA, assigned to the
+			requesting host context.
+- ti,sci-rm-range-global-event:	TISCI subtype id representing the global
+			events range reaching this IA and are assigned
+			to the requesting host context.
+
+Example:
+--------
+main_udmass_inta: interrupt-controller at 33d00000 {
+	compatible = "ti,sci-inta";
+	reg = <0x0 0x33d00000 0x0 0x100000>;
+	interrupt-controller;
+	interrupt-parent = <&main_navss_intr>;
+	#interrupt-cells = <4>;
+	ti,sci = <&dmsc>;
+	ti,sci-dev-id = <179>;
+	ti,sci-rm-range-vint = <0x0>;
+	ti,sci-rm-range-global-event = <0x1>;
+};

MAINTAINERS +1 -0

index 4f1bb64a15a1..8cf1a6b73e6c 100644
@@ -14687,6 +14687,7 @@ F:	Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F:	drivers/clk/keystone/sci-clk.c
 F:	drivers/reset/reset-ti-sci.c
 F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
+F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt
 F:	drivers/irqchip/irq-ti-sci-intr.c
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER

Signature · 1 line

2.19.1

Lokesh Vutla

author

2018-10-18 15:40 UTC #12 lore raw

PATCH v2 9/10 irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.

Configuration of the interrupt aggregator registers can only be done by a system co-processor and the driver needs to send a message to this co processor over TISCI protocol.

Add support for Interrupt Aggregator driver over TISCI protocol.

---
Changes since v1:
- New patch

 MAINTAINERS                             |   1 +
 drivers/irqchip/Kconfig                 |  11 +
 drivers/irqchip/Makefile                |   1 +
 drivers/irqchip/irq-ti-sci-inta.c       | 613 ++++++++++++++++++++++++
 include/linux/irqchip/irq-ti-sci-inta.h |  35 ++
 5 files changed, 661 insertions(+)
 create mode 100644 drivers/irqchip/irq-ti-sci-inta.c
 create mode 100644 include/linux/irqchip/irq-ti-sci-inta.h

MAINTAINERS +1 -0

index 8cf1a6b73e6c..35c790ab0ae7 100644
@@ -14689,6 +14689,7 @@ F:	drivers/reset/reset-ti-sci.c
 F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
 F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt
 F:	drivers/irqchip/irq-ti-sci-intr.c
+F:	drivers/irqchip/irq-ti-sci-inta.c
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M:	Hans Verkuil <[email protected]>

drivers/irqchip/Kconfig +11 -0

index f6620a6bb872..895f6b47dc5b 100644
@@ -385,6 +385,17 @@ config TI_SCI_INTR_IRQCHIP
 	  If you wish to use interrupt router irq resources managed by the
 	  TI System Controller, say Y here. Otherwise, say N.
 
+config TI_SCI_INTA_IRQCHIP
+	bool
+	depends on TI_SCI_PROTOCOL && ARCH_K3
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables the irqchip driver support for K3 Interrupt aggregator
+	  over TI System Control Interface available on some new TI's SoCs.
+	  If you wish to use interrupt aggregator irq resources managed by the
+	  TI System Controller, say Y here. Otherwise, say N.
+
 endmenu
 
 config SIFIVE_PLIC

drivers/irqchip/Makefile +1 -0

index 44bf65606d60..aede4c1cc4a6 100644
@@ -90,3 +90,4 @@ obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
+obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o

drivers/irqchip/irq-ti-sci-inta.c new file +613 -0

613 lines not on this page · raw message

include/linux/irqchip/irq-ti-sci-inta.h new file +35 -0

new file mode 100644
index 000000000000..c078234fda3f
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Texas Instruments' System Control Interface (TI-SCI) irqchip
+ *
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ *	Lokesh Vutla <[email protected]>
+ */
+
+#ifndef __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H
+#define __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H
+
+#if IS_ENABLED(CONFIG_TI_SCI_INTA_IRQCHIP)
+int ti_sci_inta_register_event(struct device *dev, u16 src_id, u16 src_index,
+			       unsigned int virq, u32 flags);
+int ti_sci_inta_unregister_event(struct device *dev, u16 src_id, u16 src_index,
+				 unsigned int virq);
+
+#else /* CONFIG_TI_SCI_INTA_IRQCHIP */
+
+static inline int ti_sci_inta_register_event(struct device *dev, u16 src_id,
+					     u16 src_index, unsigned int virq,
+					     u32 flags)
+{
+	return -EINVAL;
+}
+
+static inline int ti_sci_inta_unregister_event(struct device *dev, u16 src_id,
+					       u16 src_index, unsigned int virq)
+{
+	return -EINVAL;
+}
+
+#endif /* CONFIG_TI_SCI_INTA_IRQCHIP */
+
+#endif /* __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H */

Signature · 1 line

2.19.1

Marc Zyngier

2018-10-19 15:22 UTC #13 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Lokesh,

Quoted 6 lines · Lokesh Vutla

Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.

Aren't the last two points the same thing? Also, can you please define what an "event" is? What is its semantic? If they look like interrupts, can we just name them as such?

Quoted 152 lines

Configuration of the interrupt aggregator registers can only be done by a system co-processor and the driver needs to send a message to this co processor over TISCI protocol.

Add support for Interrupt Aggregator driver over TISCI protocol.

---
Changes since v1:
- New patch

 MAINTAINERS                             |   1 +
 drivers/irqchip/Kconfig                 |  11 +
 drivers/irqchip/Makefile                |   1 +
 drivers/irqchip/irq-ti-sci-inta.c       | 613 ++++++++++++++++++++++++
 include/linux/irqchip/irq-ti-sci-inta.h |  35 ++
 5 files changed, 661 insertions(+)
 create mode 100644 drivers/irqchip/irq-ti-sci-inta.c
 create mode 100644 include/linux/irqchip/irq-ti-sci-inta.h

MAINTAINERS +1 -0

index 8cf1a6b73e6c..35c790ab0ae7 100644
@@ -14689,6 +14689,7 @@ F:	drivers/reset/reset-ti-sci.c
 F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
 F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt
 F:	drivers/irqchip/irq-ti-sci-intr.c
+F:	drivers/irqchip/irq-ti-sci-inta.c
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M:	Hans Verkuil <[email protected]>

drivers/irqchip/Kconfig +11 -0

index f6620a6bb872..895f6b47dc5b 100644
@@ -385,6 +385,17 @@ config TI_SCI_INTR_IRQCHIP
 	  If you wish to use interrupt router irq resources managed by the
 	  TI System Controller, say Y here. Otherwise, say N.
 
+config TI_SCI_INTA_IRQCHIP
+	bool
+	depends on TI_SCI_PROTOCOL && ARCH_K3
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables the irqchip driver support for K3 Interrupt aggregator
+	  over TI System Control Interface available on some new TI's SoCs.
+	  If you wish to use interrupt aggregator irq resources managed by the
+	  TI System Controller, say Y here. Otherwise, say N.
+
 endmenu
 
 config SIFIVE_PLIC

drivers/irqchip/Makefile +1 -0

index 44bf65606d60..aede4c1cc4a6 100644
@@ -90,3 +90,4 @@ obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
+obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o

drivers/irqchip/irq-ti-sci-inta.c new file +81 -0

new file mode 100644
index 000000000000..ef0a2e8b782c
@@ -0,0 +1,613 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Texas Instruments' K3 Interrupt Aggregator irqchip driver
+ *
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ *	Lokesh Vutla <[email protected]>
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/irqdomain.h>
+#include <linux/soc/ti/ti_sci_protocol.h>
+
+#define MAX_EVENTS_PER_VINT	64
+#define TI_SCI_EVENT_IRQ	BIT(31)
+
+#define VINT_ENABLE_CLR_OFFSET	0x18
+
+/**
+ * struct ti_sci_inta_irq_domain - Structure representing a TISCI based
+ *				   Interrupt Aggregator IRQ domain.
+ * @sci:	Pointer to TISCI handle
+ * @vint:	TISCI resource pointer representing IA inerrupts.
+ * @global_event:TISCI resource pointer representing global events.
+ * @base:	Base address of the memory mapped IO registers
+ * @ia_id:	TISCI device ID of this Interrupt Aggregator.
+ * @dst_id:	TISCI device ID of the destination irq controller.
+ */
+struct ti_sci_inta_irq_domain {
+	const struct ti_sci_handle *sci;
+	struct ti_sci_resource *vint;
+	struct ti_sci_resource *global_event;
+	void __iomem *base;
+	u16 ia_id;
+	u16 dst_id;
+};
+
+/**
+ * struct ti_sci_inta_event_desc - Description of an event coming to
+ *				   Interrupt Aggregator.
+ * @global_event:	Global event number corresponding to this event
+ * @src_id:		TISCI device ID of the event source
+ * @src_index:		Event source index within the device.
+ */
+struct ti_sci_inta_event_desc {
+	u16 global_event;
+	u16 src_id;
+	u16 src_index;
+};
+
+/**
+ * struct ti_sci_inta_vint_desc - Description of a virtual interrupt coming out
+ *				  of Interrupt Aggregator.
+ * @lock:		lock to guard the event map
+ * @event_map:		Bitmap to manage the allocation of events to vint.
+ * @events:		Array of event descriptors assigned to this vint.
+ * @ack_needed:		Event needs to be acked via INTA. This is used when
+ *			HW generating events cannot clear the events by itself.
+ *			Assuming that only events from the same hw block are
+ *			grouped. So all the events attached to vint needs
+ *			an ack or none needs an ack.
+ */
+struct ti_sci_inta_vint_desc {
+	raw_spinlock_t lock;
+	unsigned long *event_map;
+	struct ti_sci_inta_event_desc events[MAX_EVENTS_PER_VINT];
+	bool ack_needed;
+};
+
+static void ti_sci_inta_irq_eoi(struct irq_data *data)
+{
+	struct ti_sci_inta_irq_domain *inta = data->domain->host_data;
+	struct ti_sci_inta_vint_desc *vint_desc;
+	u64 val;
+	int bit;

drivers/irqchip/irq-ti-sci-inta.c line 91

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+
+	vint_desc = irq_data_get_irq_chip_data(data);
+	if (!vint_desc->ack_needed)
+		goto out;
+
+	for_each_set_bit(bit, vint_desc->event_map, MAX_EVENTS_PER_VINT) {
+		val = 1 << bit;
+		__raw_writeq(val, inta->base + data->hwirq * 0x1000 +
+			     VINT_ENABLE_CLR_OFFSET);
+	}

If you need an ack callback, why is this part of the eoi? We have interrupt flows that allow you to combine both, so why don't you use that?

Also, the __raw_writeq call is probably wrong, as it assumes that both the CPU and the INTA have the same endianness.

Quoted 50 lines

drivers/irqchip/irq-ti-sci-inta.c new file +50 -0

+
+out:
+	irq_chip_eoi_parent(data);
+}
+
+static struct irq_chip ti_sci_inta_irq_chip = {
+	.name			= "INTA",
+	.irq_eoi		= ti_sci_inta_irq_eoi,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+/**
+ * ti_sci_inta_irq_domain_translate() - Retrieve hwirq and type from
+ *					IRQ firmware specific handler.
+ * @domain:	Pointer to IRQ domain
+ * @fwspec:	Pointer to IRQ specific firmware structure
+ * @hwirq:	IRQ number identified by hardware
+ * @type:	IRQ type
+ *
+ * Return 0 if all went ok else appropriate error.
+ */
+static int ti_sci_inta_irq_domain_translate(struct irq_domain *domain,
+					    struct irq_fwspec *fwspec,
+					    unsigned long *hwirq,
+					    unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 4)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[2];
+		*type = fwspec->param[3] & IRQ_TYPE_SENSE_MASK;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+/**
+ * ti_sci_free_event_irq() - Free an event from vint
+ * @inta:	Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc:	Virtual interrupt descriptor containing the event.
+ * @event_index:Event Index within the vint.
+ * @dst_irq:	Destination host irq
+ * @vint:	Interrupt number within interrupt aggregator.

drivers/irqchip/irq-ti-sci-inta.c line 151

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+ */
+static void ti_sci_free_event_irq(struct ti_sci_inta_irq_domain *inta,
+				  struct ti_sci_inta_vint_desc *vint_desc,
+				  u32 event_index, u16 dst_irq, u16 vint)
+{
+	struct ti_sci_inta_event_desc *event;
+	unsigned long flags;
+
+	if (event_index >= MAX_EVENTS_PER_VINT)
+		return;

How can this happen?

Quoted 4 lines

drivers/irqchip/irq-ti-sci-inta.c new file +4 -0

+
+	event = &vint_desc->events[event_index];
+	inta->sci->ops.rm_irq_ops.free_event_irq(inta->sci,
+						 event->src_id,

drivers/irqchip/irq-ti-sci-inta.c line 165

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+						 event->src_index,
+						 inta->dst_id,
+						 dst_irq,
+						 inta->ia_id, vint,
+						 event->global_event,
+						 event_index);
+
+	raw_spin_lock_irqsave(&vint_desc->lock, flags);
+	clear_bit(event_index, vint_desc->event_map);
+	raw_spin_unlock_irqrestore(&vint_desc->lock, flags);

clear_bit is atomic. Why do you need a spinlock?

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+
+	ti_sci_release_resource(inta->global_event, event->global_event);
+}
+
+/**
+ * ti_sci_inta_irq_domain_free() - Free an IRQ from the IRQ domain
+ * @domain:	Domain to which the irqs belong
+ * @virq:	base linux virtual IRQ to be freed.
+ * @nr_irqs:	Number of continuous irqs to be freed
+ */

drivers/irqchip/irq-ti-sci-inta.c line 185

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+static void ti_sci_inta_irq_domain_free(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs)
+{
+	struct ti_sci_inta_irq_domain *inta = domain->host_data;
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_data *data, *gic_data;
+	int event_index;
+
+	data = irq_domain_get_irq_data(domain, virq);
+	gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);

That's absolutely horrid...

drivers/irqchip/irq-ti-sci-inta.c line 190

Quoted 5 lines

drivers/irqchip/irq-ti-sci-inta.c new file +5 -0

+
+	vint_desc = irq_data_get_irq_chip_data(data);
+
+	/* This is the last event in the vint */
+	event_index = find_first_bit(vint_desc->event_map, MAX_EVENTS_PER_VINT);

What guarantees that you only have a single "event" left here?

Quoted 26 lines

drivers/irqchip/irq-ti-sci-inta.c new file +26 -0

+	ti_sci_free_event_irq(inta, vint_desc, event_index,
+			      gic_data->hwirq, data->hwirq);
+	irq_domain_free_irqs_parent(domain, virq, 1);
+	irq_domain_reset_irq_data(data);
+	ti_sci_release_resource(inta->vint, data->hwirq);
+	kfree(vint_desc->event_map);
+	kfree(vint_desc);
+}
+
+/**
+ * ti_sci_allocate_event_irq() - Allocate an event to a IA vint.
+ * @inta:	Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc:	Virtual interrupt descriptor to which the event gets attached.
+ * @src_id:	TISCI device id of the event source
+ * @src_index:	Event index with in the device.
+ * @dst_irq:	Destination host irq
+ * @vint:	Interrupt number within interrupt aggregator.
+ *
+ * Return 0 if all went ok else appropriate error value.
+ */
+static int ti_sci_allocate_event_irq(struct ti_sci_inta_irq_domain *inta,
+				     struct ti_sci_inta_vint_desc *vint_desc,
+				     u16 src_id, u16 src_index, u16 dst_irq,
+				     u16 vint)
+{
+	struct ti_sci_inta_event_desc *event;

drivers/irqchip/irq-ti-sci-inta.c line 226

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+	unsigned long flags;
+	u32 free_bit;
+	int err;
+
+	raw_spin_lock_irqsave(&vint_desc->lock, flags);
+	free_bit = find_first_zero_bit(vint_desc->event_map,
+				       MAX_EVENTS_PER_VINT);
+	if (free_bit != MAX_EVENTS_PER_VINT)
+		set_bit(free_bit, vint_desc->event_map);
+	raw_spin_unlock_irqrestore(&vint_desc->lock, flags);

Why disabling the interrupts? Do you expect to take this lock concurrently with an interrupt? Why isn't it enough to just have a mutex instead?

drivers/irqchip/irq-ti-sci-inta.c line 235

Quoted 9 lines

drivers/irqchip/irq-ti-sci-inta.c new file +9 -0

+
+	if (free_bit >= MAX_EVENTS_PER_VINT)
+		return -ENODEV;
+
+	event = &vint_desc->events[free_bit];
+
+	event->src_id = src_id;
+	event->src_index = src_index;
+	event->global_event = ti_sci_get_free_resource(inta->global_event);

Reading patch #5, shouldn't you at least test for the validity of what this function returns?

Quoted 22 lines

drivers/irqchip/irq-ti-sci-inta.c new file +22 -0

+
+	err = inta->sci->ops.rm_irq_ops.set_event_irq(inta->sci,
+						      src_id, src_index,
+						      inta->dst_id,
+						      dst_irq,
+						      inta->ia_id,
+						      vint,
+						      event->global_event,
+						      free_bit);
+	if (err) {
+		pr_err("%s: Event allocation failed from src = %d, index = %d, to dst = %d,irq = %d,via ia_id = %d, vint = %d,global event = %d, status_bit = %d\n",
+		       __func__, src_id, src_index, inta->dst_id, dst_irq,
+		       inta->ia_id, vint, event->global_event, free_bit);
+		return err;
+	}
+
+	return 0;
+}
+
+/**
+ * alloc_parent_irq() - Allocate parent irq to Interrupt aggregator
+ * @domain:	IRQ domain corresponding to Interrupt Aggregator

drivers/irqchip/irq-ti-sci-inta.c line 267

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+ * @virq:	Linux virtual IRQ number
+ * @src_id:	TISCI device id of the event source
+ * @src_index:	Event index with in the device.
+ * @vint:	Virtual interrupt number within IA
+ * @flags:	Corresponding IRQ flags
+ *
+ * Return pointer to vint descriptor if all went well else corresponding
+ * error pointer.
+ */
+static struct ti_sci_inta_vint_desc *alloc_parent_irq(struct irq_domain *domain,

Please rename this function to something less ambiguous (you've prefixed all functions so far, why not this one?).

Quoted 21 lines

drivers/irqchip/irq-ti-sci-inta.c new file +21 -0

+						      unsigned int virq,
+						      u32 src_id, u32 src_index,
+						      u32 vint, u32 flags)
+{
+	struct ti_sci_inta_irq_domain *inta = domain->host_data;
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_data *gic_data;
+	struct irq_fwspec fwspec;
+	int err;
+
+	if (!irq_domain_get_of_node(domain->parent))
+		return ERR_PTR(-EINVAL);
+
+	vint_desc = kzalloc(sizeof(*vint_desc), GFP_KERNEL);
+	if (!vint_desc)
+		return ERR_PTR(-ENOMEM);
+
+	vint_desc->event_map = kcalloc(BITS_TO_LONGS(MAX_EVENTS_PER_VINT),
+				       sizeof(*vint_desc->event_map),
+				       GFP_KERNEL);
+	if (!vint_desc->event_map) {

drivers/irqchip/irq-ti-sci-inta.c line 298

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+		kfree(vint_desc);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	fwspec.fwnode = domain->parent->fwnode;
+	fwspec.param_count = 3;
+	/* Interrupt parent is Interrupt Router */
+	fwspec.param[0] = inta->ia_id;
+	fwspec.param[1] = vint;
+	fwspec.param[2] = flags | TI_SCI_EVENT_IRQ;

Why isn't that flag an additional parameter instead of mixing stuff coming from DT and things that are purely internal?

Quoted 34 lines

drivers/irqchip/irq-ti-sci-inta.c new file +34 -0

+
+	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
+	if (err)
+		goto err_irqs;
+
+	gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);
+
+	raw_spin_lock_init(&vint_desc->lock);
+
+	err = ti_sci_allocate_event_irq(inta, vint_desc, src_id, src_index,
+					gic_data->hwirq, vint);
+	if (err)
+		goto err_events;
+
+	return vint_desc;
+
+err_events:
+	irq_domain_free_irqs_parent(domain, virq, 1);
+err_irqs:
+	ti_sci_release_resource(inta->vint, vint);
+	kfree(vint_desc);
+	return ERR_PTR(err);
+}
+
+/**
+ * ti_sci_inta_irq_domain_alloc() - Allocate Interrupt aggregator IRQs
+ * @domain:	Point to the interrupt aggregator IRQ domain
+ * @virq:	Corresponding Linux virtual IRQ number
+ * @nr_irqs:	Continuous irqs to be allocated
+ * @data:	Pointer to firmware specifier
+ *
+ * Return 0 if all went well else appropriate error value.
+ */
+static int ti_sci_inta_irq_domain_alloc(struct irq_domain *domain,

drivers/irqchip/irq-ti-sci-inta.c line 342

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+					unsigned int virq, unsigned int nr_irqs,
+					void *data)
+{
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_fwspec *fwspec = data;
+	int err;
+
+	vint_desc = alloc_parent_irq(domain, virq, fwspec->param[0],
+				     fwspec->param[1], fwspec->param[2],
+				     fwspec->param[3]);

Frankly, what is the point of doing that? Why don't you simply pass the fwspec?

Quoted 93 lines

drivers/irqchip/irq-ti-sci-inta.c new file +93 -0

+	if (IS_ERR(vint_desc))
+		return PTR_ERR(vint_desc);
+
+	err = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[2],
+					    &ti_sci_inta_irq_chip, vint_desc);
+
+	return err;
+}
+
+static const struct irq_domain_ops ti_sci_inta_irq_domain_ops = {
+	.alloc		= ti_sci_inta_irq_domain_alloc,
+	.free		= ti_sci_inta_irq_domain_free,
+	.translate	= ti_sci_inta_irq_domain_translate,
+};
+
+static int ti_sci_inta_irq_domain_probe(struct platform_device *pdev)
+{
+	struct irq_domain *parent_domain, *domain;
+	struct ti_sci_inta_irq_domain *inta;
+	struct device_node *parent_node;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	parent_node = of_irq_find_parent(dev_of_node(dev));
+	if (!parent_node) {
+		dev_err(dev, "Failed to get IRQ parent node\n");
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain)
+		return -EPROBE_DEFER;
+
+	inta = devm_kzalloc(dev, sizeof(*inta), GFP_KERNEL);
+	if (!inta)
+		return -ENOMEM;
+
+	inta->sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
+	if (IS_ERR(inta->sci)) {
+		ret = PTR_ERR(inta->sci);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "ti,sci read fail %d\n", ret);
+		inta->sci = NULL;
+		return ret;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "ti,sci-dev-id",
+				   (u32 *)&inta->ia_id);
+	if (ret) {
+		dev_err(dev, "missing 'ti,sci-dev-id' property\n");
+		return -EINVAL;
+	}
+
+	inta->vint = devm_ti_sci_get_of_resource(inta->sci, dev,
+						 inta->ia_id,
+						 "ti,sci-rm-range-vint");
+	if (IS_ERR(inta->vint)) {
+		dev_err(dev, "VINT resource allocation failed\n");
+		return PTR_ERR(inta->vint);
+	}
+
+	inta->global_event =
+		devm_ti_sci_get_of_resource(inta->sci, dev,
+					    inta->ia_id,
+					    "ti,sci-rm-range-global-event");
+	if (IS_ERR(inta->global_event)) {
+		dev_err(dev, "Global event resource allocation failed\n");
+		return PTR_ERR(inta->global_event);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	inta->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(inta->base))
+		return -ENODEV;
+
+	ret = of_property_read_u32(parent_node, "ti,sci-dst-id",
+				   (u32 *)&inta->dst_id);
+
+	domain = irq_domain_add_hierarchy(parent_domain, 0, 0, dev_of_node(dev),
+					  &ti_sci_inta_irq_domain_ops, inta);
+	if (!domain) {
+		dev_err(dev, "Failed to allocate IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev:	Device pointer to source generating the event
+ * @src_id:	TISCI device ID of the event source

drivers/irqchip/irq-ti-sci-inta.c line 445

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+ * @src_index:	Event source index within the device.
+ * @virq:	Linux Virtual IRQ number
+ * @flags:	Corresponding IRQ flags
+ * @ack_needed:	If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().

NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.

I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what this event is, and how drivers get notified that such an event has fired.

So please explain what this is all about, and we'll work out something. In the meantime, I'll stop here for that particular patch.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Lokesh Vutla

author

2018-10-22 14:35 UTC #14 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

Quoted 10 lines · Marc Zyngier

Hi Lokesh,
Quoted 6 lines · Lokesh Vutla
Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.

Quoted 3 lines

Aren't the last two points the same thing? Also, can you please define what an "event" is? What is its semantic? If they look like interrupts, can we just name them as such?

Event is actually a message sent by a master via an Event transport lane. Based on the id within the message, each message is directed to corresponding Interrupt Aggregator(IA). In turn IA raises a corresponding interrupt as configured for this event.

Quoted 158 lines

Configuration of the interrupt aggregator registers can only be done by a system co-processor and the driver needs to send a message to this co processor over TISCI protocol.

Add support for Interrupt Aggregator driver over TISCI protocol.

---
Changes since v1:
- New patch

  MAINTAINERS                             |   1 +
  drivers/irqchip/Kconfig                 |  11 +
  drivers/irqchip/Makefile                |   1 +
  drivers/irqchip/irq-ti-sci-inta.c       | 613 ++++++++++++++++++++++++
  include/linux/irqchip/irq-ti-sci-inta.h |  35 ++
  5 files changed, 661 insertions(+)

create mode 100644 drivers/irqchip/irq-ti-sci-inta.c create mode 100644 include/linux/irqchip/irq-ti-sci-inta.h

MAINTAINERS +1 -0

index 8cf1a6b73e6c..35c790ab0ae7 100644
@@ -14689,6 +14689,7 @@ F:	drivers/reset/reset-ti-sci.c
  F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
  F:	Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt
  F:	drivers/irqchip/irq-ti-sci-intr.c
+F:	drivers/irqchip/irq-ti-sci-inta.c
 
  THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
  M:	Hans Verkuil <[email protected]>

drivers/irqchip/Kconfig +11 -0

index f6620a6bb872..895f6b47dc5b 100644
@@ -385,6 +385,17 @@ config TI_SCI_INTR_IRQCHIP
  	  If you wish to use interrupt router irq resources managed by the
  	  TI System Controller, say Y here. Otherwise, say N.
 
+config TI_SCI_INTA_IRQCHIP
+	bool
+	depends on TI_SCI_PROTOCOL && ARCH_K3
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables the irqchip driver support for K3 Interrupt aggregator
+	  over TI System Control Interface available on some new TI's SoCs.
+	  If you wish to use interrupt aggregator irq resources managed by the
+	  TI System Controller, say Y here. Otherwise, say N.
+
  endmenu
 
  config SIFIVE_PLIC

drivers/irqchip/Makefile +1 -0

index 44bf65606d60..aede4c1cc4a6 100644
@@ -90,3 +90,4 @@ obj-$(CONFIG_NDS32)			+= irq-ativic32.o
  obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
  obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
+obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o

drivers/irqchip/irq-ti-sci-inta.c new file +86 -0

new file mode 100644
index 000000000000..ef0a2e8b782c
@@ -0,0 +1,613 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Texas Instruments' K3 Interrupt Aggregator irqchip driver
+ *
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ *	Lokesh Vutla <[email protected]>
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/irqdomain.h>
+#include <linux/soc/ti/ti_sci_protocol.h>
+
+#define MAX_EVENTS_PER_VINT	64
+#define TI_SCI_EVENT_IRQ	BIT(31)
+
+#define VINT_ENABLE_CLR_OFFSET	0x18
+
+/**
+ * struct ti_sci_inta_irq_domain - Structure representing a TISCI based
+ *				   Interrupt Aggregator IRQ domain.
+ * @sci:	Pointer to TISCI handle
+ * @vint:	TISCI resource pointer representing IA inerrupts.
+ * @global_event:TISCI resource pointer representing global events.
+ * @base:	Base address of the memory mapped IO registers
+ * @ia_id:	TISCI device ID of this Interrupt Aggregator.
+ * @dst_id:	TISCI device ID of the destination irq controller.
+ */
+struct ti_sci_inta_irq_domain {
+	const struct ti_sci_handle *sci;
+	struct ti_sci_resource *vint;
+	struct ti_sci_resource *global_event;
+	void __iomem *base;
+	u16 ia_id;
+	u16 dst_id;
+};
+
+/**
+ * struct ti_sci_inta_event_desc - Description of an event coming to
+ *				   Interrupt Aggregator.
+ * @global_event:	Global event number corresponding to this event
+ * @src_id:		TISCI device ID of the event source
+ * @src_index:		Event source index within the device.
+ */
+struct ti_sci_inta_event_desc {
+	u16 global_event;
+	u16 src_id;
+	u16 src_index;
+};
+
+/**
+ * struct ti_sci_inta_vint_desc - Description of a virtual interrupt coming out
+ *				  of Interrupt Aggregator.
+ * @lock:		lock to guard the event map
+ * @event_map:		Bitmap to manage the allocation of events to vint.
+ * @events:		Array of event descriptors assigned to this vint.
+ * @ack_needed:		Event needs to be acked via INTA. This is used when
+ *			HW generating events cannot clear the events by itself.
+ *			Assuming that only events from the same hw block are
+ *			grouped. So all the events attached to vint needs
+ *			an ack or none needs an ack.
+ */
+struct ti_sci_inta_vint_desc {
+	raw_spinlock_t lock;
+	unsigned long *event_map;
+	struct ti_sci_inta_event_desc events[MAX_EVENTS_PER_VINT];
+	bool ack_needed;
+};
+
+static void ti_sci_inta_irq_eoi(struct irq_data *data)
+{
+	struct ti_sci_inta_irq_domain *inta = data->domain->host_data;
+	struct ti_sci_inta_vint_desc *vint_desc;
+	u64 val;
+	int bit;
+
+	vint_desc = irq_data_get_irq_chip_data(data);
+	if (!vint_desc->ack_needed)
+		goto out;
+

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 91
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+	for_each_set_bit(bit, vint_desc->event_map, MAX_EVENTS_PER_VINT) {
+		val = 1 << bit;
+		__raw_writeq(val, inta->base + data->hwirq * 0x1000 +
+			     VINT_ENABLE_CLR_OFFSET);
+	}
If you need an ack callback, why is this part of the eoi? We have interrupt flows that allow you to combine both, so why don't you use that?

Actually I started with ack_irq. But I did not see this callback being triggered when interrupt is raised. Then I was suggested to use irq_roi. Will see why ack_irq is not being triggered and update it in next version.

Quoted 3 lines

Also, the __raw_writeq call is probably wrong, as it assumes that both the CPU and the INTA have the same endianness.

hmm.. May I know what is the right call to use here?

Quoted 64 lines

Quoted 51 lines

drivers/irqchip/irq-ti-sci-inta.c new file +50 -0

+
+out:
+	irq_chip_eoi_parent(data);
+}
+
+static struct irq_chip ti_sci_inta_irq_chip = {
+	.name			= "INTA",
+	.irq_eoi		= ti_sci_inta_irq_eoi,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+/**
+ * ti_sci_inta_irq_domain_translate() - Retrieve hwirq and type from
+ *					IRQ firmware specific handler.
+ * @domain:	Pointer to IRQ domain
+ * @fwspec:	Pointer to IRQ specific firmware structure
+ * @hwirq:	IRQ number identified by hardware
+ * @type:	IRQ type
+ *
+ * Return 0 if all went ok else appropriate error.
+ */
+static int ti_sci_inta_irq_domain_translate(struct irq_domain *domain,
+					    struct irq_fwspec *fwspec,
+					    unsigned long *hwirq,
+					    unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 4)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[2];
+		*type = fwspec->param[3] & IRQ_TYPE_SENSE_MASK;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+/**
+ * ti_sci_free_event_irq() - Free an event from vint
+ * @inta:	Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc:	Virtual interrupt descriptor containing the event.
+ * @event_index:Event Index within the vint.
+ * @dst_irq:	Destination host irq
+ * @vint:	Interrupt number within interrupt aggregator.

drivers/irqchip/irq-ti-sci-inta.c line 151

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+ */
+static void ti_sci_free_event_irq(struct ti_sci_inta_irq_domain *inta,
+				  struct ti_sci_inta_vint_desc *vint_desc,
+				  u32 event_index, u16 dst_irq, u16 vint)
+{
+	struct ti_sci_inta_event_desc *event;
+	unsigned long flags;
+
+	if (event_index >= MAX_EVENTS_PER_VINT)
+		return;

How can this happen?

Quoted 16 lines

Quoted 4 lines
drivers/irqchip/irq-ti-sci-inta.c new file +4 -0
+
+	event = &vint_desc->events[event_index];
+	inta->sci->ops.rm_irq_ops.free_event_irq(inta->sci,
+						 event->src_id,
drivers/irqchip/irq-ti-sci-inta.c line 165
Quoted 10 lines
drivers/irqchip/irq-ti-sci-inta.c new file +10 -0
+						 event->src_index,
+						 inta->dst_id,
+						 dst_irq,
+						 inta->ia_id, vint,
+						 event->global_event,
+						 event_index);
+
+	raw_spin_lock_irqsave(&vint_desc->lock, flags);
+	clear_bit(event_index, vint_desc->event_map);
+	raw_spin_unlock_irqrestore(&vint_desc->lock, flags);
clear_bit is atomic. Why do you need a spinlock?

will drop the spinlock guard here.

Quoted 15 lines

drivers/irqchip/irq-ti-sci-inta.c new file +14 -0

+
+	ti_sci_release_resource(inta->global_event, event->global_event);
+}
+
+/**
+ * ti_sci_inta_irq_domain_free() - Free an IRQ from the IRQ domain
+ * @domain:	Domain to which the irqs belong
+ * @virq:	base linux virtual IRQ to be freed.
+ * @nr_irqs:	Number of continuous irqs to be freed
+ */
+static void ti_sci_inta_irq_domain_free(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs)
+{
+	struct ti_sci_inta_irq_domain *inta = domain->host_data;

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 185
Quoted 6 lines
drivers/irqchip/irq-ti-sci-inta.c new file +6 -0
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_data *data, *gic_data;
+	int event_index;
+
+	data = irq_domain_get_irq_data(domain, virq);
+	gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);
That's absolutely horrid...

I agree. But I need to get GIC irq for sending TISCI message. Can you suggest a better way of doing it?

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 190
Quoted 6 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+
+	vint_desc = irq_data_get_irq_chip_data(data);
+
+	/* This is the last event in the vint */
+	event_index = find_first_bit(vint_desc->event_map, MAX_EVENTS_PER_VINT);
What guarantees that you only have a single "event" left here?

As per the current implementation, ti_sci_inta_irq_domain_free() gets called only by irq_dispose_mapping. irq_dispose_mapping() will be called from ti_sci_inta_unregister_event() only if it is the last event attached to vint.

Quoted 33 lines

drivers/irqchip/irq-ti-sci-inta.c new file +32 -0

+	ti_sci_free_event_irq(inta, vint_desc, event_index,
+			      gic_data->hwirq, data->hwirq);
+	irq_domain_free_irqs_parent(domain, virq, 1);
+	irq_domain_reset_irq_data(data);
+	ti_sci_release_resource(inta->vint, data->hwirq);
+	kfree(vint_desc->event_map);
+	kfree(vint_desc);
+}
+
+/**
+ * ti_sci_allocate_event_irq() - Allocate an event to a IA vint.
+ * @inta:	Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc:	Virtual interrupt descriptor to which the event gets attached.
+ * @src_id:	TISCI device id of the event source
+ * @src_index:	Event index with in the device.
+ * @dst_irq:	Destination host irq
+ * @vint:	Interrupt number within interrupt aggregator.
+ *
+ * Return 0 if all went ok else appropriate error value.
+ */
+static int ti_sci_allocate_event_irq(struct ti_sci_inta_irq_domain *inta,
+				     struct ti_sci_inta_vint_desc *vint_desc,
+				     u16 src_id, u16 src_index, u16 dst_irq,
+				     u16 vint)
+{
+	struct ti_sci_inta_event_desc *event;
+	unsigned long flags;
+	u32 free_bit;
+	int err;
+
+	raw_spin_lock_irqsave(&vint_desc->lock, flags);
+	free_bit = find_first_zero_bit(vint_desc->event_map,

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 226
Quoted 4 lines
drivers/irqchip/irq-ti-sci-inta.c new file +4 -0
+				       MAX_EVENTS_PER_VINT);
+	if (free_bit != MAX_EVENTS_PER_VINT)
+		set_bit(free_bit, vint_desc->event_map);
+	raw_spin_unlock_irqrestore(&vint_desc->lock, flags);
Why disabling the interrupts? Do you expect to take this lock concurrently with an interrupt? Why isn't it enough to just have a mutex instead?

I have thought about this while coding. We are attaching multiple events to the same interrupt. Technically the events from different IPs can be attached to the same interrupt or events from the same IP can be registered at different times. So I thought it is possible that when an event is being allocated to an interrupt, an event can be raised that belongs to the same interrupt.

Quoted 13 lines

drivers/irqchip/irq-ti-sci-inta.c line 235
Quoted 10 lines
drivers/irqchip/irq-ti-sci-inta.c new file +9 -0
+
+	if (free_bit >= MAX_EVENTS_PER_VINT)
+		return -ENODEV;
+
+	event = &vint_desc->events[free_bit];
+
+	event->src_id = src_id;
+	event->src_index = src_index;
+	event->global_event = ti_sci_get_free_resource(inta->global_event);
Reading patch #5, shouldn't you at least test for the validity of what this function returns?

the below call will anyway fail and report the invlaid global_event. But you are right, will check for validity of global_event in my next version.

Quoted 28 lines

drivers/irqchip/irq-ti-sci-inta.c new file +27 -0

+
+	err = inta->sci->ops.rm_irq_ops.set_event_irq(inta->sci,
+						      src_id, src_index,
+						      inta->dst_id,
+						      dst_irq,
+						      inta->ia_id,
+						      vint,
+						      event->global_event,
+						      free_bit);
+	if (err) {
+		pr_err("%s: Event allocation failed from src = %d, index = %d, to dst = %d,irq = %d,via ia_id = %d, vint = %d,global event = %d, status_bit = %d\n",
+		       __func__, src_id, src_index, inta->dst_id, dst_irq,
+		       inta->ia_id, vint, event->global_event, free_bit);
+		return err;
+	}
+
+	return 0;
+}
+
+/**
+ * alloc_parent_irq() - Allocate parent irq to Interrupt aggregator
+ * @domain:	IRQ domain corresponding to Interrupt Aggregator
+ * @virq:	Linux virtual IRQ number
+ * @src_id:	TISCI device id of the event source
+ * @src_index:	Event index with in the device.
+ * @vint:	Virtual interrupt number within IA
+ * @flags:	Corresponding IRQ flags

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 267
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+ *
+ * Return pointer to vint descriptor if all went well else corresponding
+ * error pointer.
+ */
+static struct ti_sci_inta_vint_desc *alloc_parent_irq(struct irq_domain *domain,
Please rename this function to something less ambiguous (you've prefixed all functions so far, why not this one?).

Will fix it in my next version.

Quoted 27 lines

drivers/irqchip/irq-ti-sci-inta.c new file +26 -0

+						      unsigned int virq,
+						      u32 src_id, u32 src_index,
+						      u32 vint, u32 flags)
+{
+	struct ti_sci_inta_irq_domain *inta = domain->host_data;
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_data *gic_data;
+	struct irq_fwspec fwspec;
+	int err;
+
+	if (!irq_domain_get_of_node(domain->parent))
+		return ERR_PTR(-EINVAL);
+
+	vint_desc = kzalloc(sizeof(*vint_desc), GFP_KERNEL);
+	if (!vint_desc)
+		return ERR_PTR(-ENOMEM);
+
+	vint_desc->event_map = kcalloc(BITS_TO_LONGS(MAX_EVENTS_PER_VINT),
+				       sizeof(*vint_desc->event_map),
+				       GFP_KERNEL);
+	if (!vint_desc->event_map) {
+		kfree(vint_desc);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	fwspec.fwnode = domain->parent->fwnode;

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 298
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+	fwspec.param_count = 3;
+	/* Interrupt parent is Interrupt Router */
+	fwspec.param[0] = inta->ia_id;
+	fwspec.param[1] = vint;
+	fwspec.param[2] = flags | TI_SCI_EVENT_IRQ;
Why isn't that flag an additional parameter instead of mixing stuff coming from DT and things that are purely internal?

Since this is a single bit I tried to optimize the number of fields passing to the parent IRQ. Will update the INTR driver to take 4 fields in next version.

Quoted 40 lines

drivers/irqchip/irq-ti-sci-inta.c new file +39 -0

+
+	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
+	if (err)
+		goto err_irqs;
+
+	gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);
+
+	raw_spin_lock_init(&vint_desc->lock);
+
+	err = ti_sci_allocate_event_irq(inta, vint_desc, src_id, src_index,
+					gic_data->hwirq, vint);
+	if (err)
+		goto err_events;
+
+	return vint_desc;
+
+err_events:
+	irq_domain_free_irqs_parent(domain, virq, 1);
+err_irqs:
+	ti_sci_release_resource(inta->vint, vint);
+	kfree(vint_desc);
+	return ERR_PTR(err);
+}
+
+/**
+ * ti_sci_inta_irq_domain_alloc() - Allocate Interrupt aggregator IRQs
+ * @domain:	Point to the interrupt aggregator IRQ domain
+ * @virq:	Corresponding Linux virtual IRQ number
+ * @nr_irqs:	Continuous irqs to be allocated
+ * @data:	Pointer to firmware specifier
+ *
+ * Return 0 if all went well else appropriate error value.
+ */
+static int ti_sci_inta_irq_domain_alloc(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs,
+					void *data)
+{
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_fwspec *fwspec = data;

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 342
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+	int err;
+
+	vint_desc = alloc_parent_irq(domain, virq, fwspec->param[0],
+				     fwspec->param[1], fwspec->param[2],
+				     fwspec->param[3]);
Frankly, what is the point of doing that? Why don't you simply pass the fwspec?

okay, will fix in next version.

Quoted 109 lines

Quoted 94 lines

drivers/irqchip/irq-ti-sci-inta.c new file +93 -0

+	if (IS_ERR(vint_desc))
+		return PTR_ERR(vint_desc);
+
+	err = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[2],
+					    &ti_sci_inta_irq_chip, vint_desc);
+
+	return err;
+}
+
+static const struct irq_domain_ops ti_sci_inta_irq_domain_ops = {
+	.alloc		= ti_sci_inta_irq_domain_alloc,
+	.free		= ti_sci_inta_irq_domain_free,
+	.translate	= ti_sci_inta_irq_domain_translate,
+};
+
+static int ti_sci_inta_irq_domain_probe(struct platform_device *pdev)
+{
+	struct irq_domain *parent_domain, *domain;
+	struct ti_sci_inta_irq_domain *inta;
+	struct device_node *parent_node;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+
+	parent_node = of_irq_find_parent(dev_of_node(dev));
+	if (!parent_node) {
+		dev_err(dev, "Failed to get IRQ parent node\n");
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain)
+		return -EPROBE_DEFER;
+
+	inta = devm_kzalloc(dev, sizeof(*inta), GFP_KERNEL);
+	if (!inta)
+		return -ENOMEM;
+
+	inta->sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
+	if (IS_ERR(inta->sci)) {
+		ret = PTR_ERR(inta->sci);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "ti,sci read fail %d\n", ret);
+		inta->sci = NULL;
+		return ret;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "ti,sci-dev-id",
+				   (u32 *)&inta->ia_id);
+	if (ret) {
+		dev_err(dev, "missing 'ti,sci-dev-id' property\n");
+		return -EINVAL;
+	}
+
+	inta->vint = devm_ti_sci_get_of_resource(inta->sci, dev,
+						 inta->ia_id,
+						 "ti,sci-rm-range-vint");
+	if (IS_ERR(inta->vint)) {
+		dev_err(dev, "VINT resource allocation failed\n");
+		return PTR_ERR(inta->vint);
+	}
+
+	inta->global_event =
+		devm_ti_sci_get_of_resource(inta->sci, dev,
+					    inta->ia_id,
+					    "ti,sci-rm-range-global-event");
+	if (IS_ERR(inta->global_event)) {
+		dev_err(dev, "Global event resource allocation failed\n");
+		return PTR_ERR(inta->global_event);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	inta->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(inta->base))
+		return -ENODEV;
+
+	ret = of_property_read_u32(parent_node, "ti,sci-dst-id",
+				   (u32 *)&inta->dst_id);
+
+	domain = irq_domain_add_hierarchy(parent_domain, 0, 0, dev_of_node(dev),
+					  &ti_sci_inta_irq_domain_ops, inta);
+	if (!domain) {
+		dev_err(dev, "Failed to allocate IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev:	Device pointer to source generating the event
+ * @src_id:	TISCI device ID of the event source

drivers/irqchip/irq-ti-sci-inta.c line 445

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+ * @src_index:	Event source index within the device.
+ * @virq:	Linux Virtual IRQ number
+ * @flags:	Corresponding IRQ flags
+ * @ack_needed:	If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().

NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.

Quoted 2 lines

I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what

hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.

Quoted 1 line

this event is, and how drivers get notified that such an event has fired.

As said above, Event is a message being sent by a device using a hardware protocol. This message is sent over an Event Transport Lane(ETL) that understands this protocol. Based on the message ETL re directs the message to a specificed target(In our case it is interrupt Aggregator).

From a client drivers(that generates this event) prespective, the following
needs to be done:
- Get an index that is free and allocate it to a particular task.
- Request INTA driver to assign an irq for this index.
- do a request_irq baseed on the return value from the above step.

In case of grouping events, the client drivers has its own mechanism to identify the index that caused an interrupt(at least that is the case for the existing user).

More details can be found in TRM section 10.2.7 Interrupt Aggregator (INTR_AGGR) chapter[1]

[1] http://www.ti.com/lit/pdf/spruid7

Quoted 4 lines

So please explain what this is all about, and we'll work out something. In the meantime, I'll stop here for that particular patch.

Thanks a lot for the detailed review.

Regards,
Lokesh

Quoted 4 lines

Thanks,
	M.

Marc Zyngier

2018-10-23 13:50 UTC #15 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Lokesh,

Quoted 14 lines · Lokesh Vutla

Hi Marc,
Quoted 10 lines · Marc Zyngier
Hi Lokesh,
Quoted 6 lines · Lokesh Vutla
Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.

Quoted 8 lines

Quoted 3 lines
Aren't the last two points the same thing? Also, can you please define what an "event" is? What is its semantic? If they look like interrupts, can we just name them as such?
Event is actually a message sent by a master via an Event transport lane. Based on the id within the message, each message is directed to corresponding Interrupt Aggregator(IA). In turn IA raises a corresponding interrupt as configured for this event.

Ergo, this is an interrupt, and there is nothing more to it. HW folks may want to give it a sexy name, but as far as SW is concerned, it has the properties of an interrupt and should be modelled as such.

[...]

Quoted 9 lines

Quoted 8 lines
drivers/irqchip/irq-ti-sci-inta.c line 91
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+	for_each_set_bit(bit, vint_desc->event_map, MAX_EVENTS_PER_VINT) {
+		val = 1 << bit;
+		__raw_writeq(val, inta->base + data->hwirq * 0x1000 +
+			     VINT_ENABLE_CLR_OFFSET);
+	}
If you need an ack callback, why is this part of the eoi? We have interrupt flows that allow you to combine both, so why don't you use that?

Quoted 4 lines

Actually I started with ack_irq. But I did not see this callback being triggered when interrupt is raised. Then I was suggested to use irq_roi. Will see why ack_irq is not being triggered and update it in next version.

It is probably because you're not using the right interrupt flow.

Quoted 4 lines

Quoted 2 lines
Also, the __raw_writeq call is probably wrong, as it assumes that both the CPU and the INTA have the same endianness.
hmm.. May I know what is the right call to use here?

writeq_relaxed is most probably what you want. I assume this code will never run on a 32bit platform, right?

[...]

Quoted 19 lines

Quoted 10 lines

drivers/irqchip/irq-ti-sci-inta.c new file +10 -0

+/**
+ * ti_sci_inta_irq_domain_free() - Free an IRQ from the IRQ domain
+ * @domain:	Domain to which the irqs belong
+ * @virq:	base linux virtual IRQ to be freed.
+ * @nr_irqs:	Number of continuous irqs to be freed
+ */
+static void ti_sci_inta_irq_domain_free(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs)
+{
+	struct ti_sci_inta_irq_domain *inta = domain->host_data;

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 185
Quoted 6 lines
drivers/irqchip/irq-ti-sci-inta.c new file +6 -0
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_data *data, *gic_data;
+	int event_index;
+
+	data = irq_domain_get_irq_data(domain, virq);
+	gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);
That's absolutely horrid...

Quoted 2 lines

I agree. But I need to get GIC irq for sending TISCI message. Can you suggest a better way of doing it?

I'd say "fix the firmware to have a layered approach". But I guess that's not an option, right?

[...]

Quoted 32 lines

Quoted 23 lines

drivers/irqchip/irq-ti-sci-inta.c new file +23 -0

+/**
+ * ti_sci_allocate_event_irq() - Allocate an event to a IA vint.
+ * @inta:	Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc:	Virtual interrupt descriptor to which the event gets attached.
+ * @src_id:	TISCI device id of the event source
+ * @src_index:	Event index with in the device.
+ * @dst_irq:	Destination host irq
+ * @vint:	Interrupt number within interrupt aggregator.
+ *
+ * Return 0 if all went ok else appropriate error value.
+ */
+static int ti_sci_allocate_event_irq(struct ti_sci_inta_irq_domain *inta,
+				     struct ti_sci_inta_vint_desc *vint_desc,
+				     u16 src_id, u16 src_index, u16 dst_irq,
+				     u16 vint)
+{
+	struct ti_sci_inta_event_desc *event;
+	unsigned long flags;
+	u32 free_bit;
+	int err;
+
+	raw_spin_lock_irqsave(&vint_desc->lock, flags);
+	free_bit = find_first_zero_bit(vint_desc->event_map,

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 226
Quoted 4 lines
drivers/irqchip/irq-ti-sci-inta.c new file +4 -0
+				       MAX_EVENTS_PER_VINT);
+	if (free_bit != MAX_EVENTS_PER_VINT)
+		set_bit(free_bit, vint_desc->event_map);
+	raw_spin_unlock_irqrestore(&vint_desc->lock, flags);
Why disabling the interrupts? Do you expect to take this lock concurrently with an interrupt? Why isn't it enough to just have a mutex instead?

Quoted 6 lines

I have thought about this while coding. We are attaching multiple events to the same interrupt. Technically the events from different IPs can be attached to the same interrupt or events from the same IP can be registered at different times. So I thought it is possible that when an event is being allocated to an interrupt, an event can be raised that belongs to the same interrupt.

I strongly dispute this. Events are interrupts, and we're not requesting an interrupt from an interrupt handler. That would be just crazy.

[...]

Quoted 19 lines

Quoted 19 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.

Quoted 6 lines

Quoted 2 lines
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.

It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.

Quoted 9 lines

Quoted 2 lines
this event is, and how drivers get notified that such an event has fired.
As said above, Event is a message being sent by a device using a hardware protocol. This message is sent over an Event Transport Lane(ETL) that understands this protocol. Based on the message ETL re directs the message to a specificed target(In our case it is interrupt Aggregator).

Quoted 5 lines

From a client drivers(that generates this event) prespective, the following needs to be done:
- Get an index that is free and allocate it to a particular task.
- Request INTA driver to assign an irq for this index.
- do a request_irq baseed on the return value from the above step.

All of that can be done in the using the current MSI framework. You can either implement your own bus framework or use the platform MSI stuff. You can then rewrite the INTA driver to be what it really is, an interrupt multiplexer.

Quoted 3 lines

In case of grouping events, the client drivers has its own mechanism to identify the index that caused an interrupt(at least that is the case for the existing user).

This simply isn't acceptable. Each event must be the result of a single interrupt allocation from the point of view of the driver. If events are shared, they should be modelled as a shared interrupt.

Overall, I'm extremely concerned that you're reinventing the wheel and coming up with a new "concept" that seems incredibly similar to what we already have everywhere else, just offering an incompatible API. This means that your drivers become specialised for your new API, and this isn't going to fly.

I can only urge you to reconsider the way you provide these events, and make sure that you use the existing API to its full potential. If something is not up to the task, we can then fix it in core code.

Thanks,

M.

Signature · 1 line

Jazz is not dead, it just smell funny.

Lokesh Vutla

author

2018-10-26 06:39 UTC #16 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

Quoted 78 lines · Marc Zyngier

Hi Lokesh,
Quoted 14 lines · Lokesh Vutla
Hi Marc,
Quoted 10 lines · Marc Zyngier
Hi Lokesh,
Quoted 6 lines · Lokesh Vutla
Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.
Quoted 8 lines
Quoted 3 lines
Aren't the last two points the same thing? Also, can you please define what an "event" is? What is its semantic? If they look like interrupts, can we just name them as such?
Event is actually a message sent by a master via an Event transport lane. Based on the id within the message, each message is directed to corresponding Interrupt Aggregator(IA). In turn IA raises a corresponding interrupt as configured for this event.
Ergo, this is an interrupt, and there is nothing more to it. HW folks may want to give it a sexy name, but as far as SW is concerned, it has the properties of an interrupt and should be modelled as such.
[...]
Quoted 9 lines
Quoted 8 lines
drivers/irqchip/irq-ti-sci-inta.c line 91
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+	for_each_set_bit(bit, vint_desc->event_map, MAX_EVENTS_PER_VINT) {
+		val = 1 << bit;
+		__raw_writeq(val, inta->base + data->hwirq * 0x1000 +
+			     VINT_ENABLE_CLR_OFFSET);
+	}
If you need an ack callback, why is this part of the eoi? We have interrupt flows that allow you to combine both, so why don't you use that?
Quoted 4 lines
Actually I started with ack_irq. But I did not see this callback being triggered when interrupt is raised. Then I was suggested to use irq_roi. Will see why ack_irq is not being triggered and update it in next version.
It is probably because you're not using the right interrupt flow.
Quoted 4 lines
Quoted 2 lines
Also, the __raw_writeq call is probably wrong, as it assumes that both the CPU and the INTA have the same endianness.
hmm.. May I know what is the right call to use here?
writeq_relaxed is most probably what you want. I assume this code will never run on a 32bit platform, right?
[...]
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
drivers/irqchip/irq-ti-sci-inta.c new file +6 -0
+/**
+ * ti_sci_inta_irq_domain_free() - Free an IRQ from the IRQ domain
+ * @domain:	Domain to which the irqs belong
+ * @virq:	base linux virtual IRQ to be freed.
+ * @nr_irqs:	Number of continuous irqs to be freed
+ */
drivers/irqchip/irq-ti-sci-inta.c line 185
Quoted 10 lines
drivers/irqchip/irq-ti-sci-inta.c new file +10 -0
+static void ti_sci_inta_irq_domain_free(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs)
+{
+	struct ti_sci_inta_irq_domain *inta = domain->host_data;
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_data *data, *gic_data;
+	int event_index;
+
+	data = irq_domain_get_irq_data(domain, virq);
+	gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);
That's absolutely horrid...

Quoted 5 lines

Quoted 2 lines
I agree. But I need to get GIC irq for sending TISCI message. Can you suggest a better way of doing it?
I'd say "fix the firmware to have a layered approach". But I guess that's not an option, right?

yeah, we cannot change the APIs now.

Quoted 42 lines

[...]
Quoted 32 lines
Quoted 23 lines
Quoted 23 lines
drivers/irqchip/irq-ti-sci-inta.c new file +23 -0
+/**
+ * ti_sci_allocate_event_irq() - Allocate an event to a IA vint.
+ * @inta:	Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc:	Virtual interrupt descriptor to which the event gets attached.
+ * @src_id:	TISCI device id of the event source
+ * @src_index:	Event index with in the device.
+ * @dst_irq:	Destination host irq
+ * @vint:	Interrupt number within interrupt aggregator.
+ *
+ * Return 0 if all went ok else appropriate error value.
+ */
+static int ti_sci_allocate_event_irq(struct ti_sci_inta_irq_domain *inta,
+				     struct ti_sci_inta_vint_desc *vint_desc,
+				     u16 src_id, u16 src_index, u16 dst_irq,
+				     u16 vint)
+{
+	struct ti_sci_inta_event_desc *event;
+	unsigned long flags;
+	u32 free_bit;
+	int err;
+
+	raw_spin_lock_irqsave(&vint_desc->lock, flags);
+	free_bit = find_first_zero_bit(vint_desc->event_map,
Quoted 8 lines
drivers/irqchip/irq-ti-sci-inta.c line 226
Quoted 4 lines
drivers/irqchip/irq-ti-sci-inta.c new file +4 -0
+				       MAX_EVENTS_PER_VINT);
+	if (free_bit != MAX_EVENTS_PER_VINT)
+		set_bit(free_bit, vint_desc->event_map);
+	raw_spin_unlock_irqrestore(&vint_desc->lock, flags);
Why disabling the interrupts? Do you expect to take this lock concurrently with an interrupt? Why isn't it enough to just have a mutex instead?
Quoted 6 lines
I have thought about this while coding. We are attaching multiple events to the same interrupt. Technically the events from different IPs can be attached to the same interrupt or events from the same IP can be registered at different times. So I thought it is possible that when an event is being allocated to an interrupt, an event can be raised that belongs to the same interrupt.

Quoted 3 lines

I strongly dispute this. Events are interrupts, and we're not requesting an interrupt from an interrupt handler. That would be just crazy.

okay, will use mutex instead.

Quoted 25 lines

[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what

Quoted 7 lines

Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.

Okay. I am reworking towards using MSI for this case. Will post the series once it is done.

Once again, Thanks for the clear explanation.

Thanks and regards,
Lokesh

Quoted 43 lines

Quoted 10 lines
Quoted 3 lines
this event is, and how drivers get notified that such an event has fired.
As said above, Event is a message being sent by a device using a hardware protocol. This message is sent over an Event Transport Lane(ETL) that understands this protocol. Based on the message ETL re directs the message to a specificed target(In our case it is interrupt Aggregator).
Quoted 5 lines
From a client drivers(that generates this event) prespective, the
following needs to be done:
- Get an index that is free and allocate it to a particular task.
- Request INTA driver to assign an irq for this index.
- do a request_irq baseed on the return value from the above step.
All of that can be done in the using the current MSI framework. You can either implement your own bus framework or use the platform MSI stuff. You can then rewrite the INTA driver to be what it really is, an interrupt multiplexer.
Quoted 3 lines
In case of grouping events, the client drivers has its own mechanism to identify the index that caused an interrupt(at least that is the case for the existing user).
This simply isn't acceptable. Each event must be the result of a single interrupt allocation from the point of view of the driver. If events are shared, they should be modelled as a shared interrupt.
Overall, I'm extremely concerned that you're reinventing the wheel and coming up with a new "concept" that seems incredibly similar to what we already have everywhere else, just offering an incompatible API. This means that your drivers become specialised for your new API, and this isn't going to fly.
I can only urge you to reconsider the way you provide these events, and make sure that you use the existing API to its full potential. If something is not up to the task, we can then fix it in core code.
Thanks,
	M.

Lokesh Vutla

author

2018-10-26 20:19 UTC #17 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

[..snip..]

Quoted 24 lines

Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.

Just one more clarification.

First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC

1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.

So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed
- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.

With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).

For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.

Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:

1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC

INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.

Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.

Does the above approach make sense? Please correct me if i am wrong.

Thanks and regards,
Lokesh

Marc Zyngier

2018-10-28 13:31 UTC #18 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Lokesh,

Quoted 59 lines · Lokesh Vutla

Hi Marc,
[..snip..]
Quoted 24 lines
Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Just one more clarification.
First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC
1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.

Quoted 2 lines

So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed

THat is no different from a PCI device for example, where we need the requester ID and the number of the interrupt in the MSI-X table.

Quoted 5 lines

- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.
From what I understand of the driver, at least some of that is under

the responsibility of the firmware, right? Or is the driver under control of all three parameters? To be honest, it doesn't really matter, as the as far as the kernel is concerned, the irqchip drivers are free to deal with the routing anyway they want.

Quoted 6 lines

With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).

Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.

Quoted 9 lines

For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.
Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:
1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC

Quoted 8 lines

INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.
Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.
Does the above approach make sense? Please correct me if i am wrong.

I think this can be further simplified, as you seem to assume that dynamic allocation implies MSI. This is not the case. You can perfectly use dynamically allocated interrupts and still not use MSIs.

INTA is indeed a chained interrupt controller, as it may mux several inputs onto a single output. But the output of INTA is not an MSI. It is just a regular interrupt that can allocated when the first mapping gets established.

Also, INTA shouldn't offer any "multi-MSI". This is a PCI-specific concept that doesn't translate on any other type of bus. What you want is something that should behave like MSI-X for its allocation part, where each MSI gets allocated independently.

Hierarchy-wise, you should end-up with something like this:

TISCI-MSI Chained-intr SPI
Device ---------> INTA ------------> INTR ---> GIC

As for the bus, you have two choices:

- You create a new one altogether. See drivers/bus/fsl-mc for an example of something totally over the top. This implies that all your devices are following the exact same programming model for more than just interrupts.

- You use the platform-MSI framework to build your interrupt infrastructure, and you don't have to implement much more than that.

Hope this helps,

M.

Signature · 1 line

Jazz is not dead, it just smell funny.

Lokesh Vutla

author

2018-10-29 13:04 UTC #19 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

Quoted 69 lines · Marc Zyngier

Hi Lokesh,
Quoted 59 lines · Lokesh Vutla
Hi Marc,
[..snip..]
Quoted 24 lines
Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Just one more clarification.
First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC
1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.
Quoted 2 lines
So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed
THat is no different from a PCI device for example, where we need the requester ID and the number of the interrupt in the MSI-X table.

Quoted 7 lines

Quoted 3 lines
- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.
From what I understand of the driver, at least some of that is under
the responsibility of the firmware, right? Or is the driver under control of all three parameters? To be honest, it doesn't really

Driver should control all three parameters.

Quoted 2 lines

matter, as the as far as the kernel is concerned, the irqchip drivers are free to deal with the routing anyway they want.

Correct, that's my understanding as well.

Quoted 11 lines

Quoted 7 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.

Right, my bad. INTR is just a router and no read muxing.

Quoted 19 lines

Quoted 10 lines
For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.
Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:
1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC
Quoted 8 lines
INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.
Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.
Does the above approach make sense? Please correct me if i am wrong.

Quoted 8 lines

I think this can be further simplified, as you seem to assume that dynamic allocation implies MSI. This is not the case. You can perfectly use dynamically allocated interrupts and still not use MSIs.
INTA is indeed a chained interrupt controller, as it may mux several inputs onto a single output. But the output of INTA is not an MSI. It is just a regular interrupt that can allocated when the first mapping gets established.

okay. I guess it can just be done using irq_create_fwspec_mapping().

Quoted 10 lines

Also, INTA shouldn't offer any "multi-MSI". This is a PCI-specific concept that doesn't translate on any other type of bus. What you want is something that should behave like MSI-X for its allocation part, where each MSI gets allocated independently.
Hierarchy-wise, you should end-up with something like this:
TISCI-MSI Chained-intr SPI
Device ---------> INTA ------------> INTR ---> GIC

makes sense. Thanks for the clarification. Will re work the driver using this approach and post it.

Thanks and regards,
Lokesh

Quoted 16 lines

As for the bus, you have two choices:
- You create a new one altogether. See drivers/bus/fsl-mc for an example of something totally over the top. This implies that all your devices are following the exact same programming model for more than just interrupts.
- You use the platform-MSI framework to build your interrupt infrastructure, and you don't have to implement much more than that.
Hope this helps,
	M.

Peter Ujfalusi

2018-11-01 07:55 UTC #20 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Lokesh,

Quoted 7 lines · Lokesh Vutla

Quoted 7 lines
Quoted 6 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).

Quoted 5 lines

Quoted 3 lines
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.

INTR can mux M interrupt inputs to N interrupt outputs.
One selects which interrupt input is outputted on the given interrupt output.
It is perfectly valid (but not sane) to select the same interrupt input to be routed to _all_ interrupt output for example.

Not sure if we are going to use this for anything but 1:1 mapping, but might worth keeping in mind...

- Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Marc Zyngier

2018-11-01 09:00 UTC #21 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 17 lines · Peter Ujfalusi

Lokesh,
Quoted 7 lines · Lokesh Vutla
Quoted 7 lines
Quoted 6 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Quoted 5 lines
Quoted 3 lines
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.

Quoted 8 lines

INTR can mux M interrupt inputs to N interrupt outputs.
One selects which interrupt input is outputted on the given interrupt output.
It is perfectly valid (but not sane) to select the same interrupt input to be routed to _all_ interrupt output for example.
Not sure if we are going to use this for anything but 1:1 mapping, but might worth keeping in mind...

It's not obvious how you'd use this "feature". Interrupt replicator, should one of the output be tied to another part of the system? Or maybe that's just the result of reusing some generic block...

M.

Signature · 1 line

Jazz is not dead, it just smell funny.

Peter Ujfalusi

2018-11-01 09:14 UTC #22 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

Quoted 25 lines · Marc Zyngier

Quoted 23 lines · Peter Ujfalusi
Lokesh,
Quoted 7 lines · Lokesh Vutla
Quoted 7 lines
Quoted 6 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Quoted 5 lines
Quoted 3 lines
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.
INTR can mux M interrupt inputs to N interrupt outputs.
One selects which interrupt input is outputted on the given interrupt output.
It is perfectly valid (but not sane) to select the same interrupt input to be routed to _all_ interrupt output for example.

Quoted 6 lines

Quoted 2 lines
Not sure if we are going to use this for anything but 1:1 mapping, but might worth keeping in mind...
It's not obvious how you'd use this "feature". Interrupt replicator, should one of the output be tied to another part of the system? Or maybe that's just the result of reusing some generic block...

I think the intention is that different virtualized OS would got assigned with different range of NAVSS GIC irqs and there might be a case when more than one virtualized environment need to get a GIC irq for the same virt. Timer interrupts comes to mind first, but there could be other cases when the same virt should trigger on multiple GIC line.

- Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Marc Zyngier

2018-11-01 11:15 UTC #23 lore raw

PATCH v2 9/10 irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Peter,

Quoted 36 lines · Peter Ujfalusi

Hi Marc,
Quoted 25 lines · Marc Zyngier
Quoted 23 lines · Peter Ujfalusi
Lokesh,
Quoted 7 lines · Lokesh Vutla
Quoted 7 lines
Quoted 6 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Quoted 5 lines
Quoted 3 lines
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.
INTR can mux M interrupt inputs to N interrupt outputs.
One selects which interrupt input is outputted on the given interrupt output.
It is perfectly valid (but not sane) to select the same interrupt input to be routed to _all_ interrupt output for example.
Quoted 6 lines
Quoted 2 lines
Not sure if we are going to use this for anything but 1:1 mapping, but might worth keeping in mind...
It's not obvious how you'd use this "feature". Interrupt replicator, should one of the output be tied to another part of the system? Or maybe that's just the result of reusing some generic block...

Quoted 5 lines

I think the intention is that different virtualized OS would got assigned with different range of NAVSS GIC irqs and there might be a case when more than one virtualized environment need to get a GIC irq for the same virt. Timer interrupts comes to mind first, but there could be other cases when the same virt should trigger on multiple GIC line.

That would be strange. SPIs seen by a guest are always virtual, and the hypervisor must handle the physical interrupt to inject the virtual counterpart. By duplicating the interrupt, you double the overhead at the hypervisor level. It would be much cheaper to just inject the interrupt twice.

On top of that, this only works for edge interrupts, and breaks badly for level signalling: who talks to the device to lower the line?

Thanks,

M.

Signature · 1 line

Jazz is not dead, it just smell funny.

Lokesh Vutla

author

2018-11-05 08:08 UTC #24 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

Quoted 124 lines · Lokesh Vutla

Hi Marc,
Quoted 69 lines · Marc Zyngier
Hi Lokesh,
Quoted 59 lines · Lokesh Vutla
Hi Marc,
[..snip..]
Quoted 24 lines
Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Just one more clarification.
First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC
1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.
Quoted 2 lines
So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed
THat is no different from a PCI device for example, where we need the requester ID and the number of the interrupt in the MSI-X table.
Quoted 7 lines
Quoted 3 lines
- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.
From what I understand of the driver, at least some of that is under
the responsibility of the firmware, right? Or is the driver under control of all three parameters? To be honest, it doesn't really
Driver should control all three parameters.
Quoted 2 lines
matter, as the as far as the kernel is concerned, the irqchip drivers are free to deal with the routing anyway they want.
Correct, that's my understanding as well.
Quoted 11 lines
Quoted 7 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.
Quoted 23 lines
Quoted 10 lines
For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.
Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:
1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC
Quoted 8 lines
INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.
Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.
Does the above approach make sense? Please correct me if i am wrong.
I think this can be further simplified, as you seem to assume that dynamic allocation implies MSI. This is not the case. You can perfectly use dynamically allocated interrupts and still not use MSIs.

Quoted 7 lines

Quoted 4 lines
INTA is indeed a chained interrupt controller, as it may mux several inputs onto a single output. But the output of INTA is not an MSI. It is just a regular interrupt that can allocated when the first mapping gets established.
okay. I guess it can just be done using irq_create_fwspec_mapping().

I am facing an issue with this approach as I am trying to call irq_create_fwspec_mapping() from alloc callback of INTA driver. During allocation the function call flow looks like:

inta_msi_domain_alloc_irqs()

	msi_domain_alloc_irqs()
		__irq_domain_alloc_irqs()
			*mutex_lock(&irq_domain_mutex);*
			irq_domain_alloc_irqs_hierarchy()
				ti_sci_inta_irq_domain_alloc()
					if (first event in group)
							irq_create_fwspec_mapping()
								irq_find_matching_fwspec()
									*mutex_lock(&irq_domain_mutex);*

The mutex_lock is called again if INTR IRQ gets allocated in alloc callback of INTA driver. So I am clearly calling irq_create_fwspec_mapping() from a wrong place.

To avoid this problem, some other msi domain ops should be used to allocate INTR irq. I see msi_prepare() might be a good place to allocate parent interrupt for each group. But there is no similar callback available while freeing. Is it okay to add msi_unprepare callback?

Also I would like to get the parent(INTR) IRQ to get established first as we need the gic_irq, vint_id while configuring the IRQ route using system-controller.

Any help on avoiding this problem?

Thanks and regards,
Lokesh

Marc Zyngier

2018-11-05 15:36 UTC #25 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 151 lines · Lokesh Vutla

Hi Marc,
Quoted 124 lines · Lokesh Vutla
Hi Marc,
Quoted 69 lines · Marc Zyngier
Hi Lokesh,
Quoted 59 lines · Lokesh Vutla
Hi Marc,
[..snip..]
Quoted 24 lines
Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Just one more clarification.
First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC
1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.
Quoted 2 lines
So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed
THat is no different from a PCI device for example, where we need the requester ID and the number of the interrupt in the MSI-X table.
Quoted 7 lines
Quoted 3 lines
- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.
From what I understand of the driver, at least some of that is under
the responsibility of the firmware, right? Or is the driver under control of all three parameters? To be honest, it doesn't really
Driver should control all three parameters.
Quoted 2 lines
matter, as the as far as the kernel is concerned, the irqchip drivers are free to deal with the routing anyway they want.
Correct, that's my understanding as well.
Quoted 11 lines
Quoted 7 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.
Quoted 23 lines
Quoted 10 lines
For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.
Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:
1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC
Quoted 8 lines
INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.
Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.
Does the above approach make sense? Please correct me if i am wrong.
I think this can be further simplified, as you seem to assume that dynamic allocation implies MSI. This is not the case. You can perfectly use dynamically allocated interrupts and still not use MSIs.
Quoted 7 lines
Quoted 4 lines
INTA is indeed a chained interrupt controller, as it may mux several inputs onto a single output. But the output of INTA is not an MSI. It is just a regular interrupt that can allocated when the first mapping gets established.
okay. I guess it can just be done using irq_create_fwspec_mapping().
I am facing an issue with this approach as I am trying to call irq_create_fwspec_mapping() from alloc callback of INTA driver. During allocation the function call flow looks like:
inta_msi_domain_alloc_irqs()
	msi_domain_alloc_irqs()
		__irq_domain_alloc_irqs()
			*mutex_lock(&irq_domain_mutex);*
			irq_domain_alloc_irqs_hierarchy()
				ti_sci_inta_irq_domain_alloc()
					if (first event in group)
							irq_create_fwspec_mapping()
								irq_find_matching_fwspec()
									*mutex_lock(&irq_domain_mutex);*

Quoted 2 lines

The mutex_lock is called again if INTR IRQ gets allocated in alloc callback of INTA driver. So I am clearly calling irq_create_fwspec_mapping() from a wrong place.

The real issue is that you're are calling irq_create_fwspec_mapping at all. This is only supposed to be called by the high level code, not an irqchip driver in the middle of its own allocation.

The right API to use is irq_domain_alloc_irqs_parent(), which calls into the parent domain allocation. See the multiple uses in the tree already.

Quoted 10 lines

To avoid this problem, some other msi domain ops should be used to allocate INTR irq. I see msi_prepare() might be a good place to allocate parent interrupt for each group. But there is no similar callback available while freeing. Is it okay to add msi_unprepare callback?
Also I would like to get the parent(INTR) IRQ to get established first as we need the gic_irq, vint_id while configuring the IRQ route using system-controller.
Any help on avoiding this problem?

I think you need to fix the above first before we try and twist the API around.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Lokesh Vutla

author

2018-11-05 16:20 UTC #26 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

Quoted 155 lines · Marc Zyngier

Quoted 151 lines · Lokesh Vutla
Hi Marc,
Quoted 124 lines · Lokesh Vutla
Hi Marc,
Quoted 69 lines · Marc Zyngier
Hi Lokesh,
Quoted 59 lines · Lokesh Vutla
Hi Marc,
[..snip..]
Quoted 24 lines
Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Just one more clarification.
First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC
1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.
Quoted 2 lines
So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed
THat is no different from a PCI device for example, where we need the requester ID and the number of the interrupt in the MSI-X table.
Quoted 7 lines
Quoted 3 lines
- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.
From what I understand of the driver, at least some of that is under
the responsibility of the firmware, right? Or is the driver under control of all three parameters? To be honest, it doesn't really
Driver should control all three parameters.
Quoted 2 lines
matter, as the as far as the kernel is concerned, the irqchip drivers are free to deal with the routing anyway they want.
Correct, that's my understanding as well.
Quoted 11 lines
Quoted 7 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.
Quoted 23 lines
Quoted 10 lines
For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.
Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:
1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC
Quoted 8 lines
INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.
Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.
Does the above approach make sense? Please correct me if i am wrong.
I think this can be further simplified, as you seem to assume that dynamic allocation implies MSI. This is not the case. You can perfectly use dynamically allocated interrupts and still not use MSIs.
Quoted 7 lines
Quoted 4 lines
INTA is indeed a chained interrupt controller, as it may mux several inputs onto a single output. But the output of INTA is not an MSI. It is just a regular interrupt that can allocated when the first mapping gets established.
okay. I guess it can just be done using irq_create_fwspec_mapping().
I am facing an issue with this approach as I am trying to call irq_create_fwspec_mapping() from alloc callback of INTA driver. During allocation the function call flow looks like:
inta_msi_domain_alloc_irqs()
	msi_domain_alloc_irqs()
		__irq_domain_alloc_irqs()
			*mutex_lock(&irq_domain_mutex);*
			irq_domain_alloc_irqs_hierarchy()
				ti_sci_inta_irq_domain_alloc()
					if (first event in group)
							irq_create_fwspec_mapping()
								irq_find_matching_fwspec()
									*mutex_lock(&irq_domain_mutex);*
Quoted 2 lines
The mutex_lock is called again if INTR IRQ gets allocated in alloc callback of INTA driver. So I am clearly calling irq_create_fwspec_mapping() from a wrong place.

Quoted 6 lines

The real issue is that you're are calling irq_create_fwspec_mapping at all. This is only supposed to be called by the high level code, not an irqchip driver in the middle of its own allocation.
The right API to use is irq_domain_alloc_irqs_parent(), which calls into the parent domain allocation. See the multiple uses in the tree already.

But irq_domain_alloc_irqs_parent() doesn't create a new IRQ mapping. Or your suggestion is that when first event mapping gets established in the group, use the same Linux virq number to allocate the parent interrupts?

If yes, then wouldn't it be a problem during free irqs? client driver should make sure that first event that gets allocated in the group should be the last to be freed.

Thanks and regards,
Lokesh

Marc Zyngier

2018-11-05 16:44 UTC #27 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 162 lines · Lokesh Vutla

Hi Marc,
Quoted 159 lines · Marc Zyngier
Quoted 151 lines · Lokesh Vutla
Hi Marc,
Quoted 124 lines · Lokesh Vutla
Hi Marc,
Quoted 69 lines · Marc Zyngier
Hi Lokesh,
Quoted 59 lines · Lokesh Vutla
Hi Marc,
[..snip..]
Quoted 24 lines
Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Just one more clarification.
First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC
1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.
Quoted 2 lines
So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed
THat is no different from a PCI device for example, where we need the requester ID and the number of the interrupt in the MSI-X table.
Quoted 7 lines
Quoted 3 lines
- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.
From what I understand of the driver, at least some of that is under
the responsibility of the firmware, right? Or is the driver under control of all three parameters? To be honest, it doesn't really
Driver should control all three parameters.
Quoted 2 lines
matter, as the as far as the kernel is concerned, the irqchip drivers are free to deal with the routing anyway they want.
Correct, that's my understanding as well.
Quoted 11 lines
Quoted 7 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.
Quoted 23 lines
Quoted 10 lines
For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.
Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:
1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC
Quoted 8 lines
INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.
Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.
Does the above approach make sense? Please correct me if i am wrong.
I think this can be further simplified, as you seem to assume that dynamic allocation implies MSI. This is not the case. You can perfectly use dynamically allocated interrupts and still not use MSIs.
Quoted 7 lines
Quoted 4 lines
INTA is indeed a chained interrupt controller, as it may mux several inputs onto a single output. But the output of INTA is not an MSI. It is just a regular interrupt that can allocated when the first mapping gets established.
okay. I guess it can just be done using irq_create_fwspec_mapping().
I am facing an issue with this approach as I am trying to call irq_create_fwspec_mapping() from alloc callback of INTA driver. During allocation the function call flow looks like:
inta_msi_domain_alloc_irqs()
	msi_domain_alloc_irqs()
		__irq_domain_alloc_irqs()
			*mutex_lock(&irq_domain_mutex);*
			irq_domain_alloc_irqs_hierarchy()
				ti_sci_inta_irq_domain_alloc()
					if (first event in group)
							irq_create_fwspec_mapping()
								irq_find_matching_fwspec()
									*mutex_lock(&irq_domain_mutex);*
Quoted 2 lines
The mutex_lock is called again if INTR IRQ gets allocated in alloc callback of INTA driver. So I am clearly calling irq_create_fwspec_mapping() from a wrong place.
The real issue is that you're are calling irq_create_fwspec_mapping at all. This is only supposed to be called by the high level code, not an irqchip driver in the middle of its own allocation.

Quoted 6 lines

Quoted 2 lines
The right API to use is irq_domain_alloc_irqs_parent(), which calls into the parent domain allocation. See the multiple uses in the tree already.
But irq_domain_alloc_irqs_parent() doesn't create a new IRQ mapping. Or your suggestion is that when first event mapping gets established in the group, use the same Linux virq number to allocate the parent interrupts?

I had already forgotten that your INTA is the multiplexer in your system.

No, using the same virq is completely wrong, as you must have a unique irq for each of the outputs lines of your INTA.

One solution would be to pre-allocate all the interrupts for the output lines at probe time, so that you don't have to do much when the INTA irqs get allocated.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Lokesh Vutla

author

2018-11-05 17:56 UTC #28 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Marc,

Quoted 170 lines · Marc Zyngier

Quoted 162 lines · Lokesh Vutla
Hi Marc,
Quoted 159 lines · Marc Zyngier
Quoted 151 lines · Lokesh Vutla
Hi Marc,
Quoted 124 lines · Lokesh Vutla
Hi Marc,
Quoted 69 lines · Marc Zyngier
Hi Lokesh,
Quoted 59 lines · Lokesh Vutla
Hi Marc,
[..snip..]
Quoted 24 lines
Quoted 24 lines
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Just one more clarification.
First let me explain the IRQ routes a bit deeply. As I said earlier there are three ways in which IRQ can flow in AM65x SoC
1) Device directly connected to GIC
- Device IRQ --> GIC
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
3) Devices connected to INTA.
- Device IRQ --> INTA --> INTR --> GIC
1 and 2 are straight forward and we use DT for IRQ representation. Coming to 3 the trickier part is that Input to INTA and output from INTA and dynamically managed. To be more specific:
- By hardware design there are certain set of physical global events(interrupts) attached to an INTA. Out of which a certain range are assigned to the current linux host that can be queried from system-controller.
- Similarly out of all the INTA outputs(referenced as vints) a certain range can be used by the current linux host.
Quoted 2 lines
So for configuring an IRQ route in case 3, the following steps are needed:
- Device id and device resource index for which the interrupt is needed
THat is no different from a PCI device for example, where we need the requester ID and the number of the interrupt in the MSI-X table.
Quoted 7 lines
Quoted 3 lines
- A free event id from the range assigned to the INTA in this host context
- A free vint from the range assigned to the INTA in this host context
- A free gic IRQ from the range assigned to the INTR in this host context.
From what I understand of the driver, at least some of that is under
the responsibility of the firmware, right? Or is the driver under control of all three parameters? To be honest, it doesn't really
Driver should control all three parameters.
Quoted 2 lines
matter, as the as far as the kernel is concerned, the irqchip drivers are free to deal with the routing anyway they want.
Correct, that's my understanding as well.
Quoted 11 lines
Quoted 7 lines
With the above information, linux should send a message to system-controller using TISCI protocol. After policing the given information, system-controller does the following:
- Attaches the interrupt(INTA input) to the device resource index
- Muxes the interrupt(INTA input) to corresponding vint(INTA output)
- Muxes the vint(INTR input) to GIC irq(INTR output).
Isn't there a 1:1 mapping between *used* INTR inputs and outputs? Since INTR is a router, there is no real muxing. I assume that the third point above is just a copy-paste error.
Right, my bad. INTR is just a router and no read muxing.
Quoted 23 lines
Quoted 10 lines
For grouping of interrupts, the same vint number is to be passed to system-controller for all the requests.
Keeping all the above in mind, I see the following as software IRQ Domain Hierarchy:
1) INTA multi MSI --> 2)INTA -->3) MSI --> 4) INTR -->5) GIC
Quoted 8 lines
INTA driver has to set a chained IRQ using virq allocated from its parent MSI. This is to differentiate the grouped interrupts within INTA.
Inorder to cover the above two MSI domains, a new bus driver has to be created as I couldn't find a fit with the existing bus drivers.
Does the above approach make sense? Please correct me if i am wrong.
I think this can be further simplified, as you seem to assume that dynamic allocation implies MSI. This is not the case. You can perfectly use dynamically allocated interrupts and still not use MSIs.
Quoted 7 lines
Quoted 4 lines
INTA is indeed a chained interrupt controller, as it may mux several inputs onto a single output. But the output of INTA is not an MSI. It is just a regular interrupt that can allocated when the first mapping gets established.
okay. I guess it can just be done using irq_create_fwspec_mapping().
I am facing an issue with this approach as I am trying to call irq_create_fwspec_mapping() from alloc callback of INTA driver. During allocation the function call flow looks like:
inta_msi_domain_alloc_irqs()
	msi_domain_alloc_irqs()
		__irq_domain_alloc_irqs()
			*mutex_lock(&irq_domain_mutex);*
			irq_domain_alloc_irqs_hierarchy()
				ti_sci_inta_irq_domain_alloc()
					if (first event in group)
							irq_create_fwspec_mapping()
								irq_find_matching_fwspec()
									*mutex_lock(&irq_domain_mutex);*
Quoted 2 lines
The mutex_lock is called again if INTR IRQ gets allocated in alloc callback of INTA driver. So I am clearly calling irq_create_fwspec_mapping() from a wrong place.
The real issue is that you're are calling irq_create_fwspec_mapping at all. This is only supposed to be called by the high level code, not an irqchip driver in the middle of its own allocation.
Quoted 6 lines
Quoted 2 lines
The right API to use is irq_domain_alloc_irqs_parent(), which calls into the parent domain allocation. See the multiple uses in the tree already.
But irq_domain_alloc_irqs_parent() doesn't create a new IRQ mapping. Or your suggestion is that when first event mapping gets established in the group, use the same Linux virq number to allocate the parent interrupts?

Quoted 8 lines

I had already forgotten that your INTA is the multiplexer in your system.
No, using the same virq is completely wrong, as you must have a unique irq for each of the outputs lines of your INTA.
One solution would be to pre-allocate all the interrupts for the output lines at probe time, so that you don't have to do much when the INTA irqs get allocated.

Yes, that is one possibility but all the while I am trying to avoid that. Because the number of INTA outputs can be greater than gic IRQs assigned to this IP.

Thanks and regards,
Lokesh

Grygorii Strashko

2018-10-31 16:39 UTC #29 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

In reply to Marc Zyngier

Hi Marc,

Quoted 188 lines · Marc Zyngier

Quoted 9 lines · Lokesh Vutla
Quoted 8 lines · Marc Zyngier
Quoted 6 lines · Lokesh Vutla
Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.
Quoted 8 lines
Quoted 3 lines
Aren't the last two points the same thing? Also, can you please define what an "event" is? What is its semantic? If they look like interrupts, can we just name them as such?
Event is actually a message sent by a master via an Event transport lane. Based on the id within the message, each message is directed to corresponding Interrupt Aggregator(IA). In turn IA raises a corresponding interrupt as configured for this event.
Ergo, this is an interrupt, and there is nothing more to it. HW folks may want to give it a sexy name, but as far as SW is concerned, it has the properties of an interrupt and should be modelled as such.
[...]
Quoted 9 lines
Quoted 8 lines
drivers/irqchip/irq-ti-sci-inta.c line 91
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +5 -0
+	for_each_set_bit(bit, vint_desc->event_map, MAX_EVENTS_PER_VINT) {
+		val = 1 << bit;
+		__raw_writeq(val, inta->base + data->hwirq * 0x1000 +
+			     VINT_ENABLE_CLR_OFFSET);
+	}
If you need an ack callback, why is this part of the eoi? We have interrupt flows that allow you to combine both, so why don't you use that?
Quoted 4 lines
Actually I started with ack_irq. But I did not see this callback being triggered when interrupt is raised. Then I was suggested to use irq_roi. Will see why ack_irq is not being triggered and update it in next version.
It is probably because you're not using the right interrupt flow.
Quoted 4 lines
Quoted 2 lines
Also, the __raw_writeq call is probably wrong, as it assumes that both the CPU and the INTA have the same endianness.
hmm.. May I know what is the right call to use here?
writeq_relaxed is most probably what you want. I assume this code will never run on a 32bit platform, right?
[...]
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/irqchip/irq-ti-sci-inta.c new file +10 -0
+/**
+ * ti_sci_inta_irq_domain_free() - Free an IRQ from the IRQ domain
+ * @domain:	Domain to which the irqs belong
+ * @virq:	base linux virtual IRQ to be freed.
+ * @nr_irqs:	Number of continuous irqs to be freed
+ */
+static void ti_sci_inta_irq_domain_free(struct irq_domain *domain,
+					unsigned int virq, unsigned int nr_irqs)
+{
+	struct ti_sci_inta_irq_domain *inta = domain->host_data;
Quoted 8 lines
drivers/irqchip/irq-ti-sci-inta.c line 185
Quoted 6 lines
drivers/irqchip/irq-ti-sci-inta.c new file +6 -0
+	struct ti_sci_inta_vint_desc *vint_desc;
+	struct irq_data *data, *gic_data;
+	int event_index;
+
+	data = irq_domain_get_irq_data(domain, virq);
+	gic_data = irq_domain_get_irq_data(domain->parent->parent, virq);
That's absolutely horrid...
Quoted 2 lines
I agree. But I need to get GIC irq for sending TISCI message. Can you suggest a better way of doing it?
I'd say "fix the firmware to have a layered approach". But I guess that's not an option, right?
[...]
Quoted 32 lines
Quoted 23 lines
Quoted 23 lines
drivers/irqchip/irq-ti-sci-inta.c new file +23 -0
+/**
+ * ti_sci_allocate_event_irq() - Allocate an event to a IA vint.
+ * @inta:	Pointer to Interrupt Aggregator IRQ domain
+ * @vint_desc:	Virtual interrupt descriptor to which the event gets attached.
+ * @src_id:	TISCI device id of the event source
+ * @src_index:	Event index with in the device.
+ * @dst_irq:	Destination host irq
+ * @vint:	Interrupt number within interrupt aggregator.
+ *
+ * Return 0 if all went ok else appropriate error value.
+ */
+static int ti_sci_allocate_event_irq(struct ti_sci_inta_irq_domain *inta,
+				     struct ti_sci_inta_vint_desc *vint_desc,
+				     u16 src_id, u16 src_index, u16 dst_irq,
+				     u16 vint)
+{
+	struct ti_sci_inta_event_desc *event;
+	unsigned long flags;
+	u32 free_bit;
+	int err;
+
+	raw_spin_lock_irqsave(&vint_desc->lock, flags);
+	free_bit = find_first_zero_bit(vint_desc->event_map,
Quoted 8 lines
drivers/irqchip/irq-ti-sci-inta.c line 226
Quoted 4 lines
drivers/irqchip/irq-ti-sci-inta.c new file +4 -0
+				       MAX_EVENTS_PER_VINT);
+	if (free_bit != MAX_EVENTS_PER_VINT)
+		set_bit(free_bit, vint_desc->event_map);
+	raw_spin_unlock_irqrestore(&vint_desc->lock, flags);
Why disabling the interrupts? Do you expect to take this lock concurrently with an interrupt? Why isn't it enough to just have a mutex instead?
Quoted 6 lines
I have thought about this while coding. We are attaching multiple events to the same interrupt. Technically the events from different IPs can be attached to the same interrupt or events from the same IP can be registered at different times. So I thought it is possible that when an event is being allocated to an interrupt, an event can be raised that belongs to the same interrupt.
I strongly dispute this. Events are interrupts, and we're not requesting an interrupt from an interrupt handler. That would be just crazy.
[...]
Quoted 19 lines
Quoted 19 lines
Quoted 6 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
Quoted 8 lines
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
Quoted 6 lines
Quoted 2 lines
I have a hunch that if the IDs are generated dynamically, then the model we use for MSIs would fit this thing. I also want to understand what
hmm..I haven't thought about using MSI. Will try to explore it. But the "struct msi_msg" is not applicable in this case as device does not write to a specific location.
It doesn't need to. You can perfectly ignore the address field and only be concerned with the data. We already have MSI users that do not need programming of the doorbell address, just the data.
Quoted 9 lines
Quoted 2 lines
this event is, and how drivers get notified that such an event has fired.
As said above, Event is a message being sent by a device using a hardware protocol. This message is sent over an Event Transport Lane(ETL) that understands this protocol. Based on the message ETL re directs the message to a specificed target(In our case it is interrupt Aggregator).
Quoted 5 lines
From a client drivers(that generates this event) prespective, the
following needs to be done:
- Get an index that is free and allocate it to a particular task.
- Request INTA driver to assign an irq for this index.
- do a request_irq baseed on the return value from the above step.
All of that can be done in the using the current MSI framework. You can either implement your own bus framework or use the platform MSI stuff. You can then rewrite the INTA driver to be what it really is, an interrupt multiplexer.
Quoted 3 lines
In case of grouping events, the client drivers has its own mechanism to identify the index that caused an interrupt(at least that is the case for the existing user).
This simply isn't acceptable. Each event must be the result of a single interrupt allocation from the point of view of the driver. If events are shared, they should be modelled as a shared interrupt.
Overall, I'm extremely concerned that you're reinventing the wheel and coming up with a new "concept" that seems incredibly similar to what we already have everywhere else, just offering an incompatible API. This means that your drivers become specialised for your new API, and this isn't going to fly.

Quoted 3 lines

I can only urge you to reconsider the way you provide these events, and make sure that you use the existing API to its full potential. If something is not up to the task, we can then fix it in core code.

I'd try to provide some additional information here. (Sry, I'll still use term "events")

As Lokesh explained in other mail on K3 SoC everything is generic and most of resources allocated dynamicaly:
- generic DMA channels
- generic HW rings (used by DMA channel)
- generic events (assigned to the rings) and muxed to different cores/hosts

So, when some driver would like to perform DMA transaction It's
required to build (configure) DMA channel by allocating different type of resources and link them together to get finally working Data Movement path (situation complicated by ti-sci firmware which policies resources between cores/hosts):
- get UDMA channel from available range
- get HW rings and attach them to the UDMA channel
- get event, assign it to the ring and mux it to the core/host through IA->IR-> chain (and this step is done by ti_sci_inta_register_event() - no DT as everything is dynamic).

Next, how this is working now - ti_sci_inta_register_event():
- first call does similar things as regular DT irq mapping (end up calling irq_create_fwspec_mapping() and builds IRQ chain as below:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,

				<ringacc id>, 0, IRQF_TRIGGER_HIGH, false);

              +---------------------+
              |         IA          |
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+----->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |            +------+ |        +--------+         +------+  Linux IRQ Y
    evtA      |                     |
              |                     |
              +---------------------+

- second call updates only IA input part while keeping other parts of IRQ chain the same if valid <linux_virq> passed as input parameter:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,

				<ringacc id>, linux_virq, IRQF_TRIGGER_HIGH, false);
              +---------------------+
              |         IA          |
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+--^-->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |         |  +------+ |        +--------+         +------+  Linux IRQ Y
              |         |           |
+--------+   |         |           |
| ring 2 +----->evtB+--+           |
+--------+   |                     |
              +---------------------+

As per above, irq-ti-sci-inta and tisci fw creates shared IRQ on HW layer by attaching events to already established IA->IR->GIC IRQ chain. Any Rings events will trigger Linux IRQ Y line and keep it active until Rings are not empty.

Now why this was done this way?
Note. I'm not saying this is right, but it is the way we've done it as of now. And I hope MSI will help to move forward, but I'm not very familiar with it.

The consumer of this approach is K3 Networking driver, first of all, and this approach allows to eliminate runtime overhead in Networking hot path and provides possibility to implement driver's specific queues/rings handling policies
- like round-robin vs priority.

CPSW networking driver doesn't need to know exact ring generated IRQ - it
need to know if there is packet for processing, so current IRQ handling sequence we have (simplified):
- any ring evt -> IA -> IR -> GIC -> Linux IRQ Y
handle_fasteoi_irq() -> cpsw_irq_handler -> disable_irq() -> napi_schedule()
...
soft_irq() -> cpsw_poll():
- [1] for each ring from Hi prio to Low prio
[2] get packet
[3] if (packet) process packet & goto [2]
else goto [1]
if (no more packets) goto [4]
[4] enable_irq()

As can be seen there is no intermediate IRQ dispatchers on IA/IR levels and no IRQs-per-rings, and NAPI poll cycle allows to implement driver's specific rings handling policy.

Next: depending on the use case following optimizations are possible:
1) throughput: split all TX (or RX) rings on X groups, where X = num_cpus and allocate assign IRQ to each group for Networking XPS/RPS/RSS. For example, CPSW2G has 8 TX channels and so 8 completion rings, 4 CPUs:
rings[0,1] -(IA/IR) - Linux IRQ 1
rings[2,3] -(IA/IR) - Linux IRQ 2
rings[4,5] -(IA/IR) - Linux IRQ 3
rings[6,7] -(IA/IR) - Linux IRQ 4
each Linux IRQ assigned to separate CPU.

2) min latency:
Ring X is used by RT application for TX/RX some traffic (using AF_XDP sockets for example) Ring X can be assigned with separate IRQ while other rings still grouped to produce 1 IRQ
rings[0,6] - (IA/IR) - Linux IRQ 1
rings[7] - (IA/IR) - Linux IRQ 2
Linux IRQ 2 assigned to separate CPU where RT application is running.

Hope above will help to clarify some K3 AM6 IRQ generation questions and find the way to move forward.

Thanks a lot for you review and comments.

Signature · 2 lines

regards,
-grygorii

Marc Zyngier

2018-10-31 18:21 UTC #30 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Hi Grygorii,

On 31/10/18 16:39, Grygorii Strashko wrote:

[...]

Quoted 36 lines

I'd try to provide some additional information here. (Sry, I'll still use term "events")
As Lokesh explained in other mail on K3 SoC everything is generic and most of resources allocated dynamicaly:
- generic DMA channels
- generic HW rings (used by DMA channel)
- generic events (assigned to the rings) and muxed to different cores/hosts
So, when some driver would like to perform DMA transaction It's
required to build (configure) DMA channel by allocating different type of resources and link them together to get finally working Data Movement path (situation complicated by ti-sci firmware which policies resources between cores/hosts):
- get UDMA channel from available range
- get HW rings and attach them to the UDMA channel
- get event, assign it to the ring and mux it to the core/host through IA->IR-> chain (and this step is done by ti_sci_inta_register_event() - no DT as everything is dynamic).
Next, how this is working now - ti_sci_inta_register_event():
- first call does similar things as regular DT irq mapping (end up calling irq_create_fwspec_mapping() and builds IRQ chain as below:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,
				<ringacc id>, 0, IRQF_TRIGGER_HIGH, false);

              +---------------------+
              |         IA          |
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+----->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |            +------+ |        +--------+         +------+  Linux IRQ Y
    evtA      |                     |
              |                     |
              +---------------------+
- second call updates only IA input part while keeping other parts of IRQ chain the same if valid <linux_virq> passed as input parameter:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,

Quoted 11 lines

				<ringacc id>, linux_virq, IRQF_TRIGGER_HIGH, false);
              +---------------------+
              |         IA          |
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+--^-->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |         |  +------+ |        +--------+         +------+  Linux IRQ Y
              |         |           |
+--------+   |         |           |
| ring 2 +----->evtB+--+           |
+--------+   |                     |
              +---------------------+

This is basically equivalent requesting a bunch of MSIs for a single device, and obtaining a set of corresponding interrupts. The fact that you end-up muxing them in the IA block is an implementation detail.

Quoted 9 lines

As per above, irq-ti-sci-inta and tisci fw creates shared IRQ on HW layer by attaching events to already established IA->IR->GIC IRQ chain. Any Rings events will trigger Linux IRQ Y line and keep it active until Rings are not empty.
Now why this was done this way?
Note. I'm not saying this is right, but it is the way we've done it as of now. And I hope MSI will help to move forward, but I'm not very familiar with it.

Quoted 6 lines

The consumer of this approach is K3 Networking driver, first of all, and this approach allows to eliminate runtime overhead in Networking hot path and provides possibility to implement driver's specific queues/rings handling policies
- like round-robin vs priority.
CPSW networking driver doesn't need to know exact ring generated IRQ - it

Well, to fit the Linux model, you'll have to know. Events needs to be signalled as individual IRQs.

Quoted 3 lines

need to know if there is packet for processing, so current IRQ handling sequence we have (simplified):
- any ring evt -> IA -> IR -> GIC -> Linux IRQ Y
handle_fasteoi_irq() -> cpsw_irq_handler -> disable_irq() -> napi_schedule()

Here, disable_irq() will only affect a single "event".

Quoted 12 lines

...
soft_irq() -> cpsw_poll():
- [1] for each ring from Hi prio to Low prio
[2] get packet
[3] if (packet) process packet & goto [2] else goto [1]
if (no more packets) goto [4]
[4] enable_irq()
As can be seen there is no intermediate IRQ dispatchers on IA/IR levels and no IRQs-per-rings, and NAPI poll cycle allows to implement driver's specific rings handling policy.

Quoted 9 lines

Next: depending on the use case following optimizations are possible:
1) throughput: split all TX (or RX) rings on X groups, where X = num_cpus and allocate assign IRQ to each group for Networking XPS/RPS/RSS. For example, CPSW2G has 8 TX channels and so 8 completion rings, 4 CPUs:
rings[0,1] -(IA/IR) - Linux IRQ 1
rings[2,3] -(IA/IR) - Linux IRQ 2
rings[4,5] -(IA/IR) - Linux IRQ 3
rings[6,7] -(IA/IR) - Linux IRQ 4
each Linux IRQ assigned to separate CPU.

What you call "Linux IRQ" is what ends up being generated at the GIC level, and isn't the interrupt the driver will get. It will get an interrupt number which represent a single event. We absolutely need to maintain this 1:1 mapping between event and driver-visible interrupts. Whatever happens between the scenes is none of the driver problem.

In your "one interrupt, multiple events" paradigm, the whole IA thing would be conceptually part of your networking IP. I don't believe this is the case, and trawling the documentation seems to confirm this view.

Quoted 10 lines

2) min latency:
Ring X is used by RT application for TX/RX some traffic (using AF_XDP sockets for example) Ring X can be assigned with separate IRQ while other rings still grouped to produce 1 IRQ
rings[0,6] - (IA/IR) - Linux IRQ 1
rings[7] - (IA/IR) - Linux IRQ 2
Linux IRQ 2 assigned to separate CPU where RT application is running.
Hope above will help to clarify some K3 AM6 IRQ generation questions and find the way to move forward.

Well, I'm convinced that we do not want a networking driver to be tied to an interrupt architecture, and that the two should be completely independent. But that's my own opinion. I can only see two solutions moving forward:

1) You make the IA a real interrupt controller that exposes real interrupts (one per event), and write your networking driver independently of the underlying interrupt architecture.

2) you make the IA an integral part of your network driver, not exposing anything outside of it, and limiting the interactions with the IR *through the standard IRQ API*. You duplicate this knowledge throughout the other client drivers.

I believe that (2) would be a massive design mistake as it locks the driver to a single of the HW (and potentially a single revision of the firmware) while (1) gives you the required level of flexibility by hiding the whole event "concept" at a single location.

Yes, (1) makes you rewrite your existing, out of tree drivers. Oh well...

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Santosh Shilimkar

2018-10-31 18:38 UTC #31 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 2 lines · Marc Zyngier

Hi Grygorii,

[...]

Quoted 15 lines

Well, I'm convinced that we do not want a networking driver to be tied to an interrupt architecture, and that the two should be completely independent. But that's my own opinion. I can only see two solutions moving forward:
1) You make the IA a real interrupt controller that exposes real interrupts (one per event), and write your networking driver independently of the underlying interrupt architecture.
2) you make the IA an integral part of your network driver, not exposing anything outside of it, and limiting the interactions with the IR *through the standard IRQ API*. You duplicate this knowledge throughout the other client drivers.

Quoted 7 lines

I believe that (2) would be a massive design mistake as it locks the driver to a single of the HW (and potentially a single revision of the firmware) while (1) gives you the required level of flexibility by hiding the whole event "concept" at a single location.
Yes, (1) makes you rewrite your existing, out of tree drivers. Oh well...

My preference is also not tie the network driver with IA. BTW, this is very standard functionality with other network drivers too. And this is handled using MSI-X.

So strong NO for 1) from me as well.

regards,
Santosh

Marc Zyngier

2018-10-31 18:42 UTC #32 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 26 lines · Santosh Shilimkar

Quoted 2 lines · Marc Zyngier
Hi Grygorii,
[...]
Quoted 20 lines
Well, I'm convinced that we do not want a networking driver to be tied to an interrupt architecture, and that the two should be completely independent. But that's my own opinion. I can only see two solutions moving forward:
1) You make the IA a real interrupt controller that exposes real interrupts (one per event), and write your networking driver independently of the underlying interrupt architecture.
2) you make the IA an integral part of your network driver, not exposing anything outside of it, and limiting the interactions with the IR *through the standard IRQ API*. You duplicate this knowledge throughout the other client drivers.
I believe that (2) would be a massive design mistake as it locks the driver to a single of the HW (and potentially a single revision of the firmware) while (1) gives you the required level of flexibility by hiding the whole event "concept" at a single location.

Quoted 7 lines

Quoted 2 lines
Yes, (1) makes you rewrite your existing, out of tree drivers. Oh well...
My preference is also not tie the network driver with IA. BTW, this is very standard functionality with other network drivers too. And this is handled using MSI-X.
So strong NO for 1) from me as well.

Err. Are you opposing to (1) or (2)? From the above, I cannot really tell... ;-)

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Santosh Shilimkar

2018-10-31 18:48 UTC #33 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 29 lines · Marc Zyngier

Quoted 28 lines · Santosh Shilimkar
Quoted 2 lines · Marc Zyngier
Hi Grygorii,
[...]
Quoted 22 lines
Well, I'm convinced that we do not want a networking driver to be tied to an interrupt architecture, and that the two should be completely independent. But that's my own opinion. I can only see two solutions moving forward:
1) You make the IA a real interrupt controller that exposes real interrupts (one per event), and write your networking driver independently of the underlying interrupt architecture.
2) you make the IA an integral part of your network driver, not exposing anything outside of it, and limiting the interactions with the IR *through the standard IRQ API*. You duplicate this knowledge throughout the other client drivers.
I believe that (2) would be a massive design mistake as it locks the driver to a single of the HW (and potentially a single revision of the firmware) while (1) gives you the required level of flexibility by hiding the whole event "concept" at a single location.
Yes, (1) makes you rewrite your existing, out of tree drivers. Oh well...

Quoted 9 lines

Quoted 5 lines
My preference is also not tie the network driver with IA. BTW, this is very standard functionality with other network drivers too. And this is handled using MSI-X.
So strong NO for 1) from me as well.
Err. Are you opposing to (1) or (2)? From the above, I cannot really tell... ;-)

I mixed it up, sorry. I meant NO for (2), i.e No for making IA part of the network driver.

Regards,
Santosh

Grygorii Strashko

2018-10-31 20:33 UTC #34 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

In reply to Marc Zyngier

Quoted 72 lines · Marc Zyngier

Hi Grygorii,
On 31/10/18 16:39, Grygorii Strashko wrote:
[...]
Quoted 39 lines
I'd try to provide some additional information here. (Sry, I'll still use term "events")
As Lokesh explained in other mail on K3 SoC everything is generic and most of resources allocated dynamicaly:
- generic DMA channels
- generic HW rings (used by DMA channel)
- generic events (assigned to the rings) and muxed to different cores/hosts
So, when some driver would like to perform DMA transaction It's
required to build (configure) DMA channel by allocating different type of resources and link them together to get finally working Data Movement path (situation complicated by ti-sci firmware which policies resources between cores/hosts):
- get UDMA channel from available range
- get HW rings and attach them to the UDMA channel
- get event, assign it to the ring and mux it to the core/host through IA->IR-> chain (and this step is done by ti_sci_inta_register_event() - no DT as everything is dynamic).
Next, how this is working now - ti_sci_inta_register_event():
- first call does similar things as regular DT irq mapping (end up calling irq_create_fwspec_mapping() and builds IRQ chain as below:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,
<ringacc id>, 0, IRQF_TRIGGER_HIGH, false);
+---------------------+
| IA |
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+----->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |            +------+ |        +--------+         +------+  Linux IRQ Y
     evtA      |                     |
               |                     |
               +---------------------+
- second call updates only IA input part while keeping other parts of IRQ chain the same if valid <linux_virq> passed as input parameter:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,
<ringacc id>, linux_virq, IRQF_TRIGGER_HIGH, false);
+---------------------+
| IA |
Quoted 8 lines
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+--^-->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |         |  +------+ |        +--------+         +------+  Linux IRQ Y
               |         |           |
+--------+   |         |           |
| ring 2 +----->evtB+--+           |
+--------+   |                     |
               +---------------------+
This is basically equivalent requesting a bunch of MSIs for a single device, and obtaining a set of corresponding interrupts. The fact that you end-up muxing them in the IA block is an implementation detail.
Quoted 14 lines
As per above, irq-ti-sci-inta and tisci fw creates shared IRQ on HW layer by attaching events to already established IA->IR->GIC IRQ chain. Any Rings events will trigger Linux IRQ Y line and keep it active until Rings are not empty.
Now why this was done this way?
Note. I'm not saying this is right, but it is the way we've done it as of now. And I hope MSI will help to move forward, but I'm not very familiar with it.
The consumer of this approach is K3 Networking driver, first of all, and this approach allows to eliminate runtime overhead in Networking hot path and provides possibility to implement driver's specific queues/rings handling policies
- like round-robin vs priority.

Quoted 4 lines

Quoted 1 line
CPSW networking driver doesn't need to know exact ring generated IRQ - it
Well, to fit the Linux model, you'll have to know. Events needs to be signalled as individual IRQs.

"
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
"

Quoted 6 lines

Quoted 4 lines
need to know if there is packet for processing, so current IRQ handling sequence we have (simplified):
- any ring evt -> IA -> IR -> GIC -> Linux IRQ Y
    handle_fasteoi_irq() -> cpsw_irq_handler -> disable_irq() -> napi_schedule()
Here, disable_irq() will only affect a single "event".

No. It will disable "Linux IRQ Y". On IA level there is no mask/unmask/ack functions for ring's events. sum of rings events keeps "Linux IRQ Y" line physically active until all rings are serviced - empty. once ring empty - corresponding event auto cleared.

Quoted 23 lines

Quoted 17 lines
...
soft_irq() -> cpsw_poll():
- [1] for each ring from Hi prio to Low prio
[2] get packet
[3] if (packet) process packet & goto [2] else goto [1]
if (no more packets) goto [4]
[4] enable_irq()
As can be seen there is no intermediate IRQ dispatchers on IA/IR levels and no IRQs-per-rings, and NAPI poll cycle allows to implement driver's specific rings handling policy.
Next: depending on the use case following optimizations are possible:
1) throughput: split all TX (or RX) rings on X groups, where X = num_cpus and allocate assign IRQ to each group for Networking XPS/RPS/RSS. For example, CPSW2G has 8 TX channels and so 8 completion rings, 4 CPUs:
Quoted 5 lines
   rings[0,1] -(IA/IR) - Linux IRQ 1
   rings[2,3] -(IA/IR) - Linux IRQ 2
   rings[4,5] -(IA/IR) - Linux IRQ 3
   rings[6,7] -(IA/IR) - Linux IRQ 4
each Linux IRQ assigned to separate CPU.

Quoted 3 lines

What you call "Linux IRQ" is what ends up being generated at the GIC level, and isn't the interrupt the driver will get. It will get an interrupt number which represent a single event.

In current implementation the interrupt controller driver will not know what event was generated when this ti_sci_inta_register_event() is used as this is responsibility of consumer driver which required to get only one notification - packet received (GIC->Linux IRQ).

We need this to build fast IRQ handling path for networking - GIC->Linux IRQ considered exclusive and no other event can be assigned to it except as by using ti_sci_inta_register_event().
I think, it can be considered the same way as "reserved memory" - it exist, but linux knows nothing about it, while consumer drivers still can have access to it.

ti_sci_inta_register_event() does mostly the same - it steals set of events from IA, preforms some muxing inside HW and makes one GIC->Linux IRQ visible to Linux IRQ framework
- from Linux point of view allocated GIC->Linux IRQ is just regular irq and It doesn't know internals (while consumer driver does).

We can't split or divide it on networking/non networking part due to fact that all resources are dynamic, so ti-sci-inta + FW manages/own resources - and ti_sci_inta_register_event() is entry point for IRQ resources allocation from available ranges.

It's no too much different from OMAP CPSW device - just legacy CPSW IRQ generation schema statically implemented in HW: 8 TX/RX CPPI channels, which are representing 8 linked lists of CPPI descriptor. Any change of linked lists state triggers local CPSW CPPI IRQ which are summed to generate one (and only one) GIC->Linux IRQ. It's responsibility of CPSW networking driver to handle CPPI channels in correct order. And there is no chained IRQ controller implemented simply because of (a) runtime overhead and (b) impossibility to implement priority handling.

Difference is that CPSW CPPI IRQs need to be asked while K3 AM6 it happens automatically
- and in K3 AM6 HW need to be configured dynamically based on allocated resources.

We absolutely need to

Quoted 6 lines

maintain this 1:1 mapping between event and driver-visible interrupts. Whatever happens between the scenes is none of the driver problem.
In your "one interrupt, multiple events" paradigm, the whole IA thing would be conceptually part of your networking IP. I don't believe this is the case, and trawling the documentation seems to confirm this view.

not exactly - ti-sci-inta expected to work this way only when ti_sci_inta_register_event() is used. Other allocation will follow standard Linux approach by "maintaining "1:1 mapping".

Quoted 12 lines

Quoted 11 lines
2) min latency:
Ring X is used by RT application for TX/RX some traffic (using AF_XDP sockets for example) Ring X can be assigned with separate IRQ while other rings still grouped to produce 1 IRQ
rings[0,6] - (IA/IR) - Linux IRQ 1
rings[7] - (IA/IR) - Linux IRQ 2
Linux IRQ 2 assigned to separate CPU where RT application is running.
Hope above will help to clarify some K3 AM6 IRQ generation questions and find the way to move forward.

Quoted 8 lines

Well, I'm convinced that we do not want a networking driver to be tied to an interrupt architecture, and that the two should be completely independent. But that's my own opinion. I can only see two solutions moving forward:
1) You make the IA a real interrupt controller that exposes real interrupts (one per event), and write your networking driver independently of the underlying interrupt architecture.

And that's actually what is implemented IA is real interrupt controller which produces "1:1 mapping", but it provides possibility to steal and mux IRQ event for non-standard purposes
- networking/ipc. IA is resource owner in this case as there is no way to preallocate/assign resources statically.

Quoted 12 lines

2) you make the IA an integral part of your network driver, not exposing anything outside of it, and limiting the interactions with the IR *through the standard IRQ API*. You duplicate this knowledge throughout the other client drivers.
I believe that (2) would be a massive design mistake as it locks the driver to a single of the HW (and potentially a single revision of the firmware) while (1) gives you the required level of flexibility by hiding the whole event "concept" at a single location.
Yes, (1) makes you rewrite your existing, out of tree drivers. Oh well...

Signature · 2 lines

regards,
-grygorii

Marc Zyngier

2018-11-01 14:52 UTC #35 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 77 lines · Grygorii Strashko

Quoted 74 lines · Marc Zyngier
Hi Grygorii,
On 31/10/18 16:39, Grygorii Strashko wrote:
[...]
Quoted 39 lines
I'd try to provide some additional information here. (Sry, I'll still use term "events")
As Lokesh explained in other mail on K3 SoC everything is generic and most of resources allocated dynamicaly:
- generic DMA channels
- generic HW rings (used by DMA channel)
- generic events (assigned to the rings) and muxed to different cores/hosts
So, when some driver would like to perform DMA transaction It's
required to build (configure) DMA channel by allocating different type of resources and link them together to get finally working Data Movement path (situation complicated by ti-sci firmware which policies resources between cores/hosts):
- get UDMA channel from available range
- get HW rings and attach them to the UDMA channel
- get event, assign it to the ring and mux it to the core/host through IA->IR-> chain (and this step is done by ti_sci_inta_register_event() - no DT as everything is dynamic).
Next, how this is working now - ti_sci_inta_register_event():
- first call does similar things as regular DT irq mapping (end up calling irq_create_fwspec_mapping() and builds IRQ chain as below:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,
<ringacc id>, 0, IRQF_TRIGGER_HIGH, false);
+---------------------+
| IA |
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+----->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |            +------+ |        +--------+         +------+  Linux IRQ Y
     evtA      |                     |
               |                     |
               +---------------------+
- second call updates only IA input part while keeping other parts of IRQ chain the same if valid <linux_virq> passed as input parameter:
linux_virq = ti_sci_inta_register_event(dev, <ringacc tisci_dev_id>,
<ringacc id>, linux_virq, IRQF_TRIGGER_HIGH, false);
+---------------------+
| IA |
Quoted 8 lines
+--------+   |            +------+ |        +--------+         +------+
| ring 1 +----->evtA+--^-->VintX +---------->   IR   +--------->  GIC +-->
+--------+   |         |  +------+ |        +--------+         +------+  Linux IRQ Y
               |         |           |
+--------+   |         |           |
| ring 2 +----->evtB+--+           |
+--------+   |                     |
               +---------------------+
This is basically equivalent requesting a bunch of MSIs for a single device, and obtaining a set of corresponding interrupts. The fact that you end-up muxing them in the IA block is an implementation detail.
Quoted 15 lines
As per above, irq-ti-sci-inta and tisci fw creates shared IRQ on HW layer by attaching events to already established IA->IR->GIC IRQ chain. Any Rings events will trigger Linux IRQ Y line and keep it active until Rings are not empty.
Now why this was done this way?
Note. I'm not saying this is right, but it is the way we've done it as of now. And I hope MSI will help to move forward, but I'm not very familiar with it.
The consumer of this approach is K3 Networking driver, first of all, and this approach allows to eliminate runtime overhead in Networking hot path and provides possibility to implement driver's specific queues/rings handling policies
- like round-robin vs priority.
CPSW networking driver doesn't need to know exact ring generated IRQ - it

Quoted 8 lines

Quoted 2 lines
Well, to fit the Linux model, you'll have to know. Events needs to be signalled as individual IRQs.
"
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
"

And I stand by what I've written.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
need to know if there is packet for processing, so current IRQ handling sequence we have (simplified):
- any ring evt -> IA -> IR -> GIC -> Linux IRQ Y
    handle_fasteoi_irq() -> cpsw_irq_handler -> disable_irq() -> napi_schedule()
Here, disable_irq() will only affect a single "event".
No. It will disable "Linux IRQ Y". On IA level there is no mask/unmask/ack functions for ring's events. sum of rings events keeps "Linux IRQ Y" line physically active until all rings are serviced - empty. once ring empty - corresponding event auto cleared.

You're missing the point I'm trying to make: either you can and we fit it into the Linux model of an interrupt controller, or this thing is not an interrupt controller at all. Either event can be individually masked, and this can be modelled as an interrupt controller, and they cannot.

So if the IA cannot be represented as an interrupt controller and is so specific to a particular device, move it out of the interrupt subsystem and keep it private to your networking device.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Grygorii Strashko

2018-11-01 15:36 UTC #36 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 11 lines · Marc Zyngier

Quoted 6 lines · Grygorii Strashko
"
NAK. Either this fits in the standard model, or we adapt the standard model to catter for your particular use case. But we don't define a new, TI specific API.
"
And I stand by what I've written.

Sry, this was added by mistake during copy-pasting/editing.

Signature · 2 lines

regards,
-grygorii

Peter Ujfalusi

2018-11-01 09:09 UTC #37 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

In reply to Marc Zyngier

Hi Marc,

Quoted 14 lines · Marc Zyngier

Well, I'm convinced that we do not want a networking driver to be tied to an interrupt architecture, and that the two should be completely independent. But that's my own opinion. I can only see two solutions moving forward:
1) You make the IA a real interrupt controller that exposes real interrupts (one per event), and write your networking driver independently of the underlying interrupt architecture.
2) you make the IA an integral part of your network driver, not exposing anything outside of it, and limiting the interactions with the IR *through the standard IRQ API*. You duplicate this knowledge throughout the other client drivers.

Quoted 6 lines

I believe that (2) would be a massive design mistake as it locks the driver to a single of the HW (and potentially a single revision of the firmware) while (1) gives you the required level of flexibility by hiding the whole event "concept" at a single location.
Yes, (1) makes you rewrite your existing, out of tree drivers. Oh well...

We need to have generic support for INTA within the NAVSS for Linux. The UDMA (also part of the NAVSS) is used as system DMA and for the DMAengine driver we need to have ability to handle the events from Rings and from the UDMA itself.

Option 2 is not really an option as other components need to configure INTA to get interrupts from the Events flying within NAVSS.

In the past with Keystone II we did have similar PacketDMA engine but it was dedicated to service networking and crypto. For all other peripherals we had EDMA as generic system DMA.

With AM654 this is no longer the case as we no longer have EDMA and the NAVSS is tasked to service all peripherals, like networking and the ones we used to use EDMA.

Quoted 5 lines

Thanks,
	M.

- Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Peter Ujfalusi

2018-10-22 10:41 UTC #38 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Lokesh,

Quoted 7 lines · Lokesh Vutla

Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.

Quoted 5 lines

Configuration of the interrupt aggregator registers can only be done by a system co-processor and the driver needs to send a message to this co processor over TISCI protocol.
Add support for Interrupt Aggregator driver over TISCI protocol.

Have you compiled this?

...

Quoted 5 lines

drivers/irqchip/irq-ti-sci-inta.c new file +0 -0
new file mode 100644
index 000000000000..ef0a2e8b782c

...

Quoted 20 lines

drivers/irqchip/irq-ti-sci-inta.c new file +20 -0

+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev:	Device pointer to source generating the event
+ * @src_id:	TISCI device ID of the event source
+ * @src_index:	Event source index within the device.
+ * @virq:	Linux Virtual IRQ number
+ * @flags:	Corresponding IRQ flags
+ * @ack_needed:	If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
+ *
+ * Return virq if all went well else appropriate error value.
+ */
+int ti_sci_inta_register_event(struct device *dev, u16 src_id, u16 src_index,
+			       unsigned int virq, u32 flags, bool ack_needed)
+{

...

Quoted 10 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +4 -0

new file mode 100644
index 000000000000..c078234fda3f
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Texas Instruments' System Control Interface (TI-SCI) irqchip
+ *

include/linux/irqchip/irq-ti-sci-inta.h line 14

Quoted 10 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +10 -0

+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ *	Lokesh Vutla <[email protected]>
+ */
+
+#ifndef __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H
+#define __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H
+
+#if IS_ENABLED(CONFIG_TI_SCI_INTA_IRQCHIP)
+int ti_sci_inta_register_event(struct device *dev, u16 src_id, u16 src_index,
+			       unsigned int virq, u32 flags);

You are missing the ack_needed

include/linux/irqchip/irq-ti-sci-inta.h line 22

Quoted 8 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +8 -0

+int ti_sci_inta_unregister_event(struct device *dev, u16 src_id, u16 src_index,
+				 unsigned int virq);
+
+#else /* CONFIG_TI_SCI_INTA_IRQCHIP */
+
+static inline int ti_sci_inta_register_event(struct device *dev, u16 src_id,
+					     u16 src_index, unsigned int virq,
+					     u32 flags)

Here as well.

Quoted 4 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +3 -0
+{
+	return -EINVAL;
+}

include/linux/irqchip/irq-ti-sci-inta.h line 35

Quoted 10 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +10 -0

+
+static inline int ti_sci_inta_unregister_event(struct device *dev, u16 src_id,
+					       u16 src_index, unsigned int virq)
+{
+	return -EINVAL;
+}
+
+#endif /* CONFIG_TI_SCI_INTA_IRQCHIP */
+
+#endif /* __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H */

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Peter Ujfalusi

2018-10-22 10:43 UTC #39 lore raw

Re: [PATCH v2 09/10] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver

Quoted 39 lines · Peter Ujfalusi

Lokesh,
Quoted 7 lines · Lokesh Vutla
Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator which is an interrupt controller that does the following:
- Converts events to interrupts that can be understood by an interrupt router.
- Allows for multiplexing of events to interrupts.
- Allows for grouping of multiple events to a single interrupt.
Quoted 5 lines
Configuration of the interrupt aggregator registers can only be done by a system co-processor and the driver needs to send a message to this co processor over TISCI protocol.
Add support for Interrupt Aggregator driver over TISCI protocol.
Have you compiled this?
...
Quoted 5 lines
drivers/irqchip/irq-ti-sci-inta.c new file +0 -0
new file mode 100644
index 000000000000..ef0a2e8b782c
...
Quoted 11 lines
+/**
+ * ti_sci_inta_register_event() - Register a event to an interrupt aggregator
+ * @dev: Device pointer to source generating the event
+ * @src_id: TISCI device ID of the event source
+ * @src_index: Event source index within the device.
+ * @virq: Linux Virtual IRQ number
+ * @flags: Corresponding IRQ flags
+ * @ack_needed: If explicit clearing of event is required.
+ *
+ * Creates a new irq and attaches to IA domain if virq is not specified
+ * else attaches the event to vint corresponding to virq.

Quoted 8 lines

drivers/irqchip/irq-ti-sci-inta.c line 450
Quoted 8 lines
+ * When using TISCI within the client drivers, source indexes are always
+ * generated dynamically and cannot be represented in DT. So client
+ * drivers should call this API instead of platform_get_irq().
+ *
+ * Return virq if all went well else appropriate error value.
+ */
drivers/irqchip/irq-ti-sci-inta.c new file +2 -0
+int ti_sci_inta_register_event(struct device *dev, u16 src_id, u16 src_index,
+			       unsigned int virq, u32 flags, bool ack_needed)

And can you swap the flags and ack_needed?

Quoted 55 lines

Quoted 1 line

+{

...

Quoted 10 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +4 -0

new file mode 100644
index 000000000000..c078234fda3f
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Texas Instruments' System Control Interface (TI-SCI) irqchip
+ *

include/linux/irqchip/irq-ti-sci-inta.h line 14

Quoted 10 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +10 -0

+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ *	Lokesh Vutla <[email protected]>
+ */
+
+#ifndef __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H
+#define __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H
+
+#if IS_ENABLED(CONFIG_TI_SCI_INTA_IRQCHIP)
+int ti_sci_inta_register_event(struct device *dev, u16 src_id, u16 src_index,
+			       unsigned int virq, u32 flags);

You are missing the ack_needed

include/linux/irqchip/irq-ti-sci-inta.h line 22

Quoted 8 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +8 -0

+int ti_sci_inta_unregister_event(struct device *dev, u16 src_id, u16 src_index,
+				 unsigned int virq);
+
+#else /* CONFIG_TI_SCI_INTA_IRQCHIP */
+
+static inline int ti_sci_inta_register_event(struct device *dev, u16 src_id,
+					     u16 src_index, unsigned int virq,
+					     u32 flags)

Here as well.

Quoted 4 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +3 -0
+{
+	return -EINVAL;
+}

include/linux/irqchip/irq-ti-sci-inta.h line 35

Quoted 10 lines

include/linux/irqchip/irq-ti-sci-inta.h new file +10 -0

+
+static inline int ti_sci_inta_unregister_event(struct device *dev, u16 src_id,
+					       u16 src_index, unsigned int virq)
+{
+	return -EINVAL;
+}
+
+#endif /* CONFIG_TI_SCI_INTA_IRQCHIP */
+
+#endif /* __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H */

- Péter

Quoted 8 lines

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Lokesh Vutla

author

2018-10-18 15:40 UTC #40 lore raw

PATCH v2 10/10 soc: ti: am6: Enable interrupt controller drivers

Select all the TISCI dependent interrupt controller drivers for AM6 SoC.

---
Changes since v1:
- new patch

 drivers/soc/ti/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

drivers/soc/ti/Kconfig +3 -0

index be4570baad96..9ef2a8a92eb4 100644
@@ -5,6 +5,9 @@ if ARCH_K3
 
 config ARCH_K3_AM6_SOC
 	bool "K3 AM6 SoC"
+	select TI_SCI_PROTOCOL
+	select TI_SCI_INTR_IRQCHIP
+	select TI_SCI_INTA_IRQCHIP
 	help
 	  Enable support for TI's AM6 SoC Family support

Signature · 1 line

2.19.1

Santosh Shilimkar

2018-10-22 20:39 UTC #41 lore raw

Quoted 11 lines · Lokesh Vutla

TISCI abstracts the handling of IRQ routes where interrupt sources are not directly connected to host interrupt controller. This series adds support for:
- TISCI commands needed for IRQ configuration
- Interrupt Router(INTR) and Interrupt Aggregator(INTA) drivers
More information on TISCI IRQ management can be found here[1]. Complete TISCI resource management information can be found here[2]. AM65x SoC related TISCI information can be found here[3]. INTR and INTA related information can be found in TRM[4].

I didn't read the specs but from what you described in INTA and INTR bindings, does the flow of IRQs like below ?

Device IRQ(e.g USB) -->INTR-->INTA--->HOST IRQ controller(GIC)

The confusing part is aggregator and can multiplex as well as grouping but seems like its event grouping or multiplexing than actual device IRQ grouping or multi-plexsing.

What am I missing ?

Regards,S
antosh

Lokesh Vutla

author

2018-10-23 08:17 UTC #42 lore raw

Hi Santosh,

Quoted 12 lines · Santosh Shilimkar

Quoted 11 lines · Lokesh Vutla
TISCI abstracts the handling of IRQ routes where interrupt sources are not directly connected to host interrupt controller. This series adds support for:
- TISCI commands needed for IRQ configuration
- Interrupt Router(INTR) and Interrupt Aggregator(INTA) drivers
More information on TISCI IRQ management can be found here[1]. Complete TISCI resource management information can be found here[2]. AM65x SoC related TISCI information can be found here[3]. INTR and INTA related information can be found in TRM[4].

Quoted 4 lines

I didn't read the specs but from what you described in INTA and INTR bindings, does the flow of IRQs like below ?
Device IRQ(e.g USB) -->INTR-->INTA--->HOST IRQ controller(GIC)

Not all devices in SoC are connected to INTA. Only the devices that are capable of generating events are connected to INTA. And INTA is connected to INTR.

So there are three ways in which IRQ can flow in AM65x SoC:
1) Device directly connected to GIC
- Device IRQ --> GIC
- (Most legacy peripherals like MMC, UART falls in this case)
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
- This is cases where you want to mux IRQs. Used for GPIOs and Mailboxes
- (This is somewhat similar to crossbar on DRA7 devices)
3) Devices connected to INTA.
- Device Event --> INTA --> INTR --> GIC
- Used for DMA and networking devices.

Events are messages based on a hw protocol, sent by a master over a dedicated Event transport lane. Events are highly precise that no under/over flow of data transfer occurs at source/destination regardless of distance and latency. So this is mostly preferred in DMA and networking usecases. Now Interrupt Aggregator(IA) has the logic to converts these events to Interrupts.

Thanks and regards
Lokesh

Marc Zyngier

2018-10-23 08:27 UTC #43 lore raw

Quoted 37 lines · Lokesh Vutla

Hi Santosh,
Quoted 12 lines · Santosh Shilimkar
Quoted 11 lines · Lokesh Vutla
TISCI abstracts the handling of IRQ routes where interrupt sources are not directly connected to host interrupt controller. This series adds support for:
- TISCI commands needed for IRQ configuration
- Interrupt Router(INTR) and Interrupt Aggregator(INTA) drivers
More information on TISCI IRQ management can be found here[1]. Complete TISCI resource management information can be found here[2]. AM65x SoC related TISCI information can be found here[3]. INTR and INTA related information can be found in TRM[4].
Quoted 4 lines
I didn't read the specs but from what you described in INTA and INTR bindings, does the flow of IRQs like below ?
Device IRQ(e.g USB) -->INTR-->INTA--->HOST IRQ controller(GIC)
Not all devices in SoC are connected to INTA. Only the devices that are capable of generating events are connected to INTA. And INTA is connected to INTR.
So there are three ways in which IRQ can flow in AM65x SoC:
1) Device directly connected to GIC
- Device IRQ --> GIC
- (Most legacy peripherals like MMC, UART falls in this case)
2) Device connected to INTR.
- Device IRQ --> INTR --> GIC
- This is cases where you want to mux IRQs. Used for GPIOs and Mailboxes
- (This is somewhat similar to crossbar on DRA7 devices)
3) Devices connected to INTA.
- Device Event --> INTA --> INTR --> GIC
- Used for DMA and networking devices.

Quoted 6 lines

Events are messages based on a hw protocol, sent by a master over a dedicated Event transport lane. Events are highly precise that no under/over flow of data transfer occurs at source/destination regardless of distance and latency. So this is mostly preferred in DMA and networking usecases. Now Interrupt Aggregator(IA) has the logic to converts these events to Interrupts.

Can we stop with these events already? What you describe here *is* an interrupt. The fact that you have some other dedicated infrastructure in your SoC is an implementation detail that doesn't concern the kernel at all.

So this should be modelled as an interrupt, and not have its own special interface at all.

Thanks,

M.

Signature · 1 line

Jazz is not dead, it just smell funny.

Santosh Shilimkar

2018-10-23 17:34 UTC #44 lore raw