diff mbox series

[RFC,v1,1/1] Refactor ACPI DMA to support platforms without shared info descriptor in CSRT

Message ID 20230321160241.1339538-1-niyas.sait@linaro.org
State New
Headers show
Series [RFC,v1,1/1] Refactor ACPI DMA to support platforms without shared info descriptor in CSRT | expand

Commit Message

Niyas Sait March 21, 2023, 4:02 p.m. UTC 
This patch refactors the ACPI DMA layer to support platforms without
shared info descriptor in CSRT.

Shared info descriptor is optional and vendor specific (not
standardized) and not used by Arm platforms.
---

The main changes in this patch are as follows:

- Renamed acpi_dma_controller_register to acpi_dma_controller_register_with_csrt_shared_desc to reflect its new functionality. 
- Refactored acpi_dma_controller_register to allow DMA controllers to be registered without CSRT walk. 
- Introduced acpi_dma_get_csrt_dma_descriptors_by_uid function to retrieve DMA descriptors from the CSRT table. 

An example usage is given below:

desc = acpi_dma_get_csrt_dma_descriptors_by_uid(adev, uid);

extract data from desc and populate the acpi dma descriptor

struct acpi_dma *adma = kzalloc(sizeof(*adma), GFP_KERNEL);

adma->dev = ...;
adma->acpi_dma_xlate = ...;
adma->data = ...;
adma->base_request_line = ...;
adma->end_request_line = ...;

ret = acpi_dma_controller_register(dev, adma);


 drivers/dma/acpi-dma.c   | 121 +++++++++++++++++++++++++++++++--------
 drivers/dma/dw/acpi.c    |   3 +-
 include/linux/acpi_dma.h |  44 +++++++++-----
 3 files changed, 128 insertions(+), 40 deletions(-)

Comments

Andy Shevchenko March 21, 2023, 5:56 p.m. UTC | #1 
On Tue, Mar 21, 2023 at 07:53:33PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 21, 2023 at 04:02:41PM +0000, Niyas Sait wrote:
> > This patch refactors the ACPI DMA layer to support platforms without
> > shared info descriptor in CSRT.
> > 
> > Shared info descriptor is optional and vendor specific (not
> > standardized) and not used by Arm platforms.

Btw, what is the real argument of not using this table?

Yes, I know that this is an MS extension, but why ARM needs something else and
why even that is needed at all? CSRT is only for the _shared_ DMA resources
and I think most of the IPs nowadays are using private DMA engines (or
semi-private when driver based on ID can know which channel services which
device).
Niyas Sait March 22, 2023, 7:56 a.m. UTC | #2 
On 21/03/2023 17:53, Andy Shevchenko wrote:

> can_we_avoid_long_name_of_the_functions_please() ?

Sure, will do that.

> Also is this renaming is a must?

It is not a must. I considered the existing method with shared info
as a special case as it uses non standard descriptors from CSRT table
and introduced the new function to handle it.

> Btw, what is the real argument of not using this table?
> 
> Yes, I know that this is an MS extension, but why ARM needs something else and
> why even that is needed at all? CSRT is only for the _shared_ DMA resources
> and I think most of the IPs nowadays are using private DMA engines (or
> semi-private when driver based on ID can know which channel services which
> device).

The issue is that shared info descriptor is not part of CSRT definition 
[1] and I think it is not standardized or documented anywhere.

I was specifically looking at NXP I.MX8MP platform and the DMA lines for 
devices are specified using FixedDMA resource descriptor. I think other 
Arm platforms like RPi have similar requirement.

[1] https://uefi.org/sites/default/files/resources/CSRT%20v2.pdf
Andy Shevchenko March 22, 2023, 9:34 a.m. UTC | #3 
On Wed, Mar 22, 2023 at 07:56:11AM +0000, Niyas Sait wrote:
> On 21/03/2023 17:53, Andy Shevchenko wrote:
> 
> > can_we_avoid_long_name_of_the_functions_please() ?
> 
> Sure, will do that.
> 
> > Also is this renaming is a must?
> 
> It is not a must. I considered the existing method with shared info
> as a special case as it uses non standard descriptors from CSRT table
> and introduced the new function to handle it.
> 
> > Btw, what is the real argument of not using this table?
> > 
> > Yes, I know that this is an MS extension, but why ARM needs something else and
> > why even that is needed at all? CSRT is only for the _shared_ DMA resources
> > and I think most of the IPs nowadays are using private DMA engines (or
> > semi-private when driver based on ID can know which channel services which
> > device).
> 
> The issue is that shared info descriptor is not part of CSRT definition [1]
> and I think it is not standardized or documented anywhere.
> 
> I was specifically looking at NXP I.MX8MP platform and the DMA lines for
> devices are specified using FixedDMA resource descriptor. I think other Arm
> platforms like RPi have similar requirement.

Perhaps, but my question is _why_ is it so?
I.o.w. what is the technical background for this solution.

> [1] https://uefi.org/sites/default/files/resources/CSRT%20v2.pdf
Andy Shevchenko March 22, 2023, 2:32 p.m. UTC | #4 
On Wed, Mar 22, 2023 at 12:00:36PM +0000, Niyas Sait wrote:
> On 22/03/2023 09:34, Andy Shevchenko wrote:

...

> > > > Btw, what is the real argument of not using this table?
> > > > 
> > > > Yes, I know that this is an MS extension, but why ARM needs something else and
> > > > why even that is needed at all? CSRT is only for the_shared_  DMA resources
> > > > and I think most of the IPs nowadays are using private DMA engines (or
> > > > semi-private when driver based on ID can know which channel services which
> > > > device).
> > > The issue is that shared info descriptor is not part of CSRT definition [1]
> > > and I think it is not standardized or documented anywhere.
> > > 
> > > I was specifically looking at NXP I.MX8MP platform and the DMA lines for
> > > devices are specified using FixedDMA resource descriptor. I think other Arm
> > > platforms like RPi have similar requirement.
> > Perhaps, but my question is_why_  is it so?
> > I.o.w. what is the technical background for this solution.
> > 
> 
> NXP I.MX8MP board uses shared DMA controller and the current ACPI firmware
> describes DMA request lines for devices using ACPI FixedDMA descriptors.
> 
> > JFYI: ARM platform(s) use SPCR, which is also not a part of the specification.
> 
> SPCR and CSRT tables have permissive licensing and probably okay to use
> them.
> 
> The main issue is that the shared info descriptor in the CSRT table is not a
> standard and none of the arm platforms uses them.

SPCR is not standard either. So, that's to show that this is not an argument.

Are those firmwares already in the wild? Why they can't be fixed and why
existing CSRT shared info data structure can't be used.

> >> [1]https://uefi.org/sites/default/files/resources/CSRT%20v2.pdf
diff mbox series

Patch

diff --git a/drivers/dma/acpi-dma.c b/drivers/dma/acpi-dma.c
index 5906eae26e2a..4337e724d386 100644
--- a/drivers/dma/acpi-dma.c
+++ b/drivers/dma/acpi-dma.c
@@ -112,7 +112,7 @@  static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp,
 }
 
 /**
- * acpi_dma_parse_csrt - parse CSRT to exctract additional DMA resources
+ * acpi_dma_parse_csrt_shared_info - parse CSRT shared info to extract additional DMA resources
  * @adev:	ACPI device to match with
  * @adma:	struct acpi_dma of the given DMA controller
  *
@@ -124,7 +124,7 @@  static int acpi_dma_parse_resource_group(const struct acpi_csrt_group *grp,
  * We are using this table to get the request line range of the specific DMA
  * controller to be used later.
  */
-static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
+static void acpi_dma_parse_csrt_shared_info(struct acpi_device *adev, struct acpi_dma *adma)
 {
 	struct acpi_csrt_group *grp, *end;
 	struct acpi_table_csrt *csrt;
@@ -157,12 +157,64 @@  static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
 }
 
 /**
- * acpi_dma_controller_register - Register a DMA controller to ACPI DMA helpers
- * @dev:		struct device of DMA controller
- * @acpi_dma_xlate:	translation function which converts a dma specifier
- *			into a dma_chan structure
- * @data:		pointer to controller specific data to be used by
- *			translation function
+ * acpi_dma_get_csrt_dma_descriptors_by_uid - Get DMA descriptor from CSRT table for given id
+ * @adev:		ACPI device node
+ * @uid:		Unique ID for look up
+ *
+ * Parse CSRT table and look for DMA descriptors matching the given ID
+ *
+ * Return:
+ * Pointer to DMA descriptor on success or NULL on error/no match.
+ */
+struct acpi_csrt_descriptor *
+acpi_dma_get_csrt_dma_descriptors_by_uid(struct acpi_device *adev, uint32_t uid)
+{
+	struct acpi_csrt_descriptor *desc, *desc_end, *desc_found = NULL;
+	struct acpi_csrt_group *grp, *grp_end;
+	struct acpi_table_csrt *csrt;
+	acpi_status status;
+
+	status = acpi_get_table(ACPI_SIG_CSRT, 0,
+				(struct acpi_table_header **)&csrt);
+	if (ACPI_FAILURE(status)) {
+		if (status != AE_NOT_FOUND)
+			dev_warn(&adev->dev, "failed to get the CSRT table\n");
+		return NULL;
+	}
+
+	grp = (struct acpi_csrt_group *)(csrt + 1);
+	grp_end =
+		(struct acpi_csrt_group *)((void *)csrt + csrt->header.length);
+
+	while (grp < grp_end) {
+		desc = (struct acpi_csrt_descriptor *)((void *)(grp + 1) +
+						       grp->shared_info_length);
+		desc_end = (struct acpi_csrt_descriptor *)((void *)grp +
+							   grp->length);
+		while (desc < desc_end) {
+			if (desc->uid == uid) {
+				desc_found = desc;
+				goto found;
+			}
+			desc = (struct acpi_csrt_descriptor *)(((void *)desc) +
+							       desc->length);
+		}
+		grp = (struct acpi_csrt_group *)((void *)grp + grp->length);
+	}
+
+found:
+	acpi_put_table((struct acpi_table_header *)csrt);
+
+	return desc_found;
+}
+
+/**
+ * acpi_dma_controller_register_with_csrt_shared_desc - Register a DMA controller to ACPI DMA helpers
+ * @dev:                struct device of DMA controller
+ * @acpi_dma_xlate:     translation function which converts a dma specifier
+ *                      into a dma_chan structure
+ * @data:               pointer to controller specific data to be used by
+ *                      translation function
  *
  * Allocated memory should be freed with appropriate acpi_dma_controller_free()
  * call.
@@ -170,16 +222,14 @@  static void acpi_dma_parse_csrt(struct acpi_device *adev, struct acpi_dma *adma)
  * Return:
  * 0 on success or appropriate errno value on error.
  */
-int acpi_dma_controller_register(struct device *dev,
-		struct dma_chan *(*acpi_dma_xlate)
-		(struct acpi_dma_spec *, struct acpi_dma *),
-		void *data)
+int acpi_dma_controller_register_with_csrt_shared_desc(
+	struct device *dev,
+	struct dma_chan *(*acpi_dma_xlate)(struct acpi_dma_spec *,
+					   struct acpi_dma *),
+	void *data)
 {
 	struct acpi_device *adev;
-	struct acpi_dma	*adma;
-
-	if (!dev || !acpi_dma_xlate)
-		return -EINVAL;
+	struct acpi_dma *adma;
 
 	/* Check if the device was enumerated by ACPI */
 	adev = ACPI_COMPANION(dev);
@@ -194,7 +244,34 @@  int acpi_dma_controller_register(struct device *dev,
 	adma->acpi_dma_xlate = acpi_dma_xlate;
 	adma->data = data;
 
-	acpi_dma_parse_csrt(adev, adma);
+	acpi_dma_parse_csrt_shared_info(adev, adma);
+
+	return acpi_dma_controller_register(dev, adma);
+}
+EXPORT_SYMBOL_GPL(acpi_dma_controller_register_with_csrt_shared_desc);
+
+/**
+ * acpi_dma_controller_register - Register a DMA controller to ACPI DMA helpers
+ * @dev:		struct device of DMA controller
+ * @adma:		ACPI DMA descriptor
+ *
+ * Allocated memory should be freed with appropriate acpi_dma_controller_free()
+ * call.
+ *
+ * Return:
+ * 0 on success or appropriate errno value on error.
+ */
+int acpi_dma_controller_register(struct device *dev, struct acpi_dma *adma)
+{
+	struct acpi_device *adev;
+
+	if (!dev || !adma || !adma->acpi_dma_xlate)
+		return -EINVAL;
+
+	/* Check if the device was enumerated by ACPI */
+	adev = ACPI_COMPANION(dev);
+	if (!adev)
+		return -EINVAL;
 
 	/* Now queue acpi_dma controller structure in list */
 	mutex_lock(&acpi_dma_lock);
@@ -244,8 +321,7 @@  static void devm_acpi_dma_release(struct device *dev, void *res)
 /**
  * devm_acpi_dma_controller_register - resource managed acpi_dma_controller_register()
  * @dev:		device that is registering this DMA controller
- * @acpi_dma_xlate:	translation function
- * @data:		pointer to controller specific data
+ * @adma:		ACPI specific DMA descriptor
  *
  * Managed acpi_dma_controller_register(). DMA controller registered by this
  * function are automatically freed on driver detach. See
@@ -254,10 +330,7 @@  static void devm_acpi_dma_release(struct device *dev, void *res)
  * Return:
  * 0 on success or appropriate errno value on error.
  */
-int devm_acpi_dma_controller_register(struct device *dev,
-		struct dma_chan *(*acpi_dma_xlate)
-		(struct acpi_dma_spec *, struct acpi_dma *),
-		void *data)
+int devm_acpi_dma_controller_register(struct device *dev, struct acpi_dma *adma)
 {
 	void *res;
 	int ret;
@@ -266,7 +339,7 @@  int devm_acpi_dma_controller_register(struct device *dev,
 	if (!res)
 		return -ENOMEM;
 
-	ret = acpi_dma_controller_register(dev, acpi_dma_xlate, data);
+	ret = acpi_dma_controller_register(dev, adma);
 	if (ret) {
 		devres_free(res);
 		return ret;
diff --git a/drivers/dma/dw/acpi.c b/drivers/dma/dw/acpi.c
index c510c109d2c3..45d2707f4843 100644
--- a/drivers/dma/dw/acpi.c
+++ b/drivers/dma/dw/acpi.c
@@ -37,7 +37,8 @@  void dw_dma_acpi_controller_register(struct dw_dma *dw)
 	dma_cap_set(DMA_SLAVE, info->dma_cap);
 	info->filter_fn = dw_dma_acpi_filter;
 
-	ret = acpi_dma_controller_register(dev, acpi_dma_simple_xlate, info);
+	ret = acpi_dma_controller_register_with_csrt_shared_desc(
+		dev, acpi_dma_simple_xlate, info);
 	if (ret)
 		dev_err(dev, "could not register acpi_dma_controller\n");
 }
diff --git a/include/linux/acpi_dma.h b/include/linux/acpi_dma.h
index 72cedb916a9c..c2c50dcc9c07 100644
--- a/include/linux/acpi_dma.h
+++ b/include/linux/acpi_dma.h
@@ -56,15 +56,10 @@  struct acpi_dma_filter_info {
 
 #ifdef CONFIG_DMA_ACPI
 
-int acpi_dma_controller_register(struct device *dev,
-		struct dma_chan *(*acpi_dma_xlate)
-		(struct acpi_dma_spec *, struct acpi_dma *),
-		void *data);
+int acpi_dma_controller_register(struct device *dev, struct acpi_dma *adma);
 int acpi_dma_controller_free(struct device *dev);
 int devm_acpi_dma_controller_register(struct device *dev,
-		struct dma_chan *(*acpi_dma_xlate)
-		(struct acpi_dma_spec *, struct acpi_dma *),
-		void *data);
+				      struct acpi_dma *adma);
 void devm_acpi_dma_controller_free(struct device *dev);
 
 struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev,
@@ -74,23 +69,36 @@  struct dma_chan *acpi_dma_request_slave_chan_by_name(struct device *dev,
 
 struct dma_chan *acpi_dma_simple_xlate(struct acpi_dma_spec *dma_spec,
 				       struct acpi_dma *adma);
+struct acpi_csrt_descriptor *
+acpi_dma_get_csrt_dma_descriptors_by_uid(struct acpi_device *adev,
+					 uint32_t uid);
+int acpi_dma_controller_register_with_csrt_shared_desc(
+	struct device *dev,
+	struct dma_chan *(*acpi_dma_xlate)(struct acpi_dma_spec *,
+					   struct acpi_dma *),
+	void *data);
 #else
 
-static inline int acpi_dma_controller_register(struct device *dev,
-		struct dma_chan *(*acpi_dma_xlate)
-		(struct acpi_dma_spec *, struct acpi_dma *),
-		void *data)
+static inline int acpi_dma_controller_register(struct device *dev, struct acpi_dma *adma)
 {
 	return -ENODEV;
 }
+
+static inline int acpi_dma_controller_register_with_csrt_shared_desc(
+	struct device *dev,
+	struct dma_chan *(*acpi_dma_xlate)(struct acpi_dma_spec *,
+					   struct acpi_dma *),
+	void *data)
+{
+	return -ENODEV;
+}
+
 static inline int acpi_dma_controller_free(struct device *dev)
 {
 	return -ENODEV;
 }
 static inline int devm_acpi_dma_controller_register(struct device *dev,
-		struct dma_chan *(*acpi_dma_xlate)
-		(struct acpi_dma_spec *, struct acpi_dma *),
-		void *data)
+						    struct acpi_dma *adma)
 {
 	return -ENODEV;
 }
@@ -109,7 +117,13 @@  static inline struct dma_chan *acpi_dma_request_slave_chan_by_name(
 	return ERR_PTR(-ENODEV);
 }
 
-#define acpi_dma_simple_xlate	NULL
+static inline struct acpi_csrt_descriptor *
+acpi_dma_get_csrt_dma_descriptors_by_uid(struct acpi_device *adev, uint32_t uid)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+#define acpi_dma_simple_xlate NULL
 
 #endif