[PATCH] cxl: Enable an endpoint decoder type

[PATCH] cxl: Enable an endpoint decoder type

[Ben Widawsky]

CXL memory devices support HDM decoders. Currently, when a decoder is
instantiated there is no knowledge of the type of decoder; only the
underlying endpoint type is specified. In order to have the memory
devices reuse the existing decoder creation infrastructure it is
convenient to pass along the type of decoder on creation.

The primary difference for an endpoint decoder is that it doesn't have
dports, nor targets. The target is just the underlying media (with
offset).

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---

The consumer of this is the region, memory device, and decoder drivers which are
being worked on. The memory device driver on probe will query the HDM decoder
registers to determine how many decoders are present and instantiate a
cxl_decoder for each one. This patch enables that to work.

---
 drivers/cxl/acpi.c |  2 +-
 drivers/cxl/core.c | 43 ++++++++++++++++++++++++++++++++-----------
 drivers/cxl/cxl.h  | 29 +++++++++++++++++++++++++----
 3 files changed, 58 insertions(+), 16 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 8ae89273f58e..5215845e0f89 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -114,7 +114,7 @@ static void cxl_add_cfmws_decoders(struct device *dev,
 					    cfmws->base_hpa, cfmws->window_size,
 					    CFMWS_INTERLEAVE_WAYS(cfmws),
 					    CFMWS_INTERLEAVE_GRANULARITY(cfmws),
-					    CXL_DECODER_EXPANDER,
+					    CXL_DECODER_PLATFORM,
 					    flags);
 
 		if (IS_ERR(cxld)) {
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index a2e4d54fc7bc..196f260e2580 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -75,9 +75,9 @@ static ssize_t target_type_show(struct device *dev,
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 
 	switch (cxld->target_type) {
-	case CXL_DECODER_ACCELERATOR:
+	case CXL_DEVICE_ACCELERATOR:
 		return sysfs_emit(buf, "accelerator\n");
-	case CXL_DECODER_EXPANDER:
+	case CXL_DEVICE_EXPANDER:
 		return sysfs_emit(buf, "expander\n");
 	}
 	return -ENXIO;
@@ -167,6 +167,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
 	NULL,
 };
 
+static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
+	&cxl_decoder_base_attribute_group,
+	&cxl_base_attribute_group,
+	NULL,
+};
+
 static void cxl_decoder_release(struct device *dev)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
@@ -176,6 +182,12 @@ static void cxl_decoder_release(struct device *dev)
 	kfree(cxld);
 }
 
+static const struct device_type cxl_decoder_endpoint_type = {
+	.name = "cxl_decoder_endpoint",
+	.release = cxl_decoder_release,
+	.groups = cxl_decoder_endpoint_attribute_groups,
+};
+
 static const struct device_type cxl_decoder_switch_type = {
 	.name = "cxl_decoder_switch",
 	.release = cxl_decoder_release,
@@ -458,12 +470,14 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	if (interleave_ways < 1)
 		return ERR_PTR(-EINVAL);
 
-	device_lock(&port->dev);
-	if (list_empty(&port->dports))
-		rc = -EINVAL;
-	device_unlock(&port->dev);
-	if (rc)
-		return ERR_PTR(rc);
+	if (type != CXL_DECODER_ENDPOINT) {
+		device_lock(&port->dev);
+		if (list_empty(&port->dports))
+			rc = -EINVAL;
+		device_unlock(&port->dev);
+		if (rc)
+			return ERR_PTR(rc);
+	}
 
 	cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
 	if (!cxld)
@@ -496,10 +510,17 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	dev->bus = &cxl_bus_type;
 
 	/* root ports do not have a cxl_port_type parent */
-	if (port->dev.parent->type == &cxl_port_type)
-		dev->type = &cxl_decoder_switch_type;
-	else
+	switch (type) {
+	case CXL_DECODER_PLATFORM:
 		dev->type = &cxl_decoder_root_type;
+		break;
+	case CXL_DECODER_SWITCH:
+		dev->type = &cxl_decoder_switch_type;
+		break;
+	case CXL_DECODER_ENDPOINT:
+		dev->type = &cxl_decoder_endpoint_type;
+		break;
+	}
 
 	return cxld;
 err:
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6bda39a59e3..8fe58404ebb5 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -164,6 +164,11 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 #define CXL_RESOURCE_NONE ((resource_size_t) -1)
 #define CXL_TARGET_STRLEN 20
 
+enum cxl_device_type {
+       CXL_DEVICE_ACCELERATOR = 2,
+       CXL_DEVICE_EXPANDER = 3,
+};
+
 /*
  * cxl_decoder flags that define the type of memory / devices this
  * decoder supports as well as configuration lock status See "CXL 2.0
@@ -177,8 +182,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 #define CXL_DECODER_F_MASK  GENMASK(4, 0)
 
 enum cxl_decoder_type {
-       CXL_DECODER_ACCELERATOR = 2,
-       CXL_DECODER_EXPANDER = 3,
+       CXL_DECODER_PLATFORM,
+       CXL_DECODER_SWITCH,
+       CXL_DECODER_ENDPOINT,
 };
 
 /**
@@ -191,6 +197,21 @@ enum cxl_decoder_type {
  * @target_type: accelerator vs expander (type2 vs type3) selector
  * @flags: memory type capabilities and locking
  * @target: active ordered target list in current decoder configuration
+ *
+ * Abstractly, a CXL decoder represents one of 3 possible decoders:
+ * 1. Platform specific routing - opaque rules for the memory controller that
+ *    may be communicated via ACPI or devicetree. This decoding has implied
+ *    interleave parameters as well as physical address ranges that are directed
+ *    to the downstream ports of this decoder.
+ * 2. HDM decoder for a switch. Similar to the platform specific routing in that
+ *    it contains a set of downstream ports which receive and send traffic in an
+ *    interleave fashion, the main difference is that the interleave and address
+ *    ranges are controlled by the HDM decoder registers defined in the CXL 2.0
+ *    specification.
+ * 3. HDM decoder for an endpoint. Like the decoder in a switch, this decoder's
+ *    configuration is entirely programmable and defined in CXL spec. Unlike the
+ *    switch's decoder, there is not a set of downstream ports, only the
+ *    underlying media.
  */
 struct cxl_decoder {
 	struct device dev;
@@ -198,7 +219,7 @@ struct cxl_decoder {
 	struct range range;
 	int interleave_ways;
 	int interleave_granularity;
-	enum cxl_decoder_type target_type;
+	enum cxl_device_type target_type;
 	unsigned long flags;
 	struct cxl_dport *target[];
 };
@@ -289,7 +310,7 @@ static inline struct cxl_decoder *
 devm_cxl_add_passthrough_decoder(struct device *host, struct cxl_port *port)
 {
 	return devm_cxl_add_decoder(host, port, 1, 0, 0, 1, PAGE_SIZE,
-				    CXL_DECODER_EXPANDER, 0);
+				    CXL_DECODER_PLATFORM, 0);
 }
 
 extern struct bus_type cxl_bus_type;
-- 
2.32.0

[PATCH v2] cxl: Enable an endpoint decoder type

[Ben Widawsky]

CXL memory devices support HDM decoders. Currently, when a decoder is
instantiated there is no knowledge of the type of decoder; only the
underlying endpoint type is specified. In order to have the memory
devices reuse the existing decoder creation infrastructure it is
convenient to pass along the type of decoder on creation.

The primary difference for an endpoint decoder is that it doesn't have
dports, nor targets. The target is just the underlying media (with
offset).

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---

v2 Fixes target_type and stores the decoder type on instantiation

diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 196f260e2580..69acdd230f54 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -493,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
                        .start = base,
                        .end = base + len - 1,
                },
+               .type = type,
                .flags = flags,
                .interleave_ways = interleave_ways,
                .interleave_granularity = interleave_granularity,
-               .target_type = type,
+               .target_type = CXL_DEVICE_EXPANDER,
        };

        /* handle implied target_list */

---
 drivers/cxl/acpi.c |  2 +-
 drivers/cxl/core.c | 46 ++++++++++++++++++++++++++++++++++------------
 drivers/cxl/cxl.h  | 31 +++++++++++++++++++++++++++----
 3 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 8ae89273f58e..5215845e0f89 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -114,7 +114,7 @@ static void cxl_add_cfmws_decoders(struct device *dev,
 					    cfmws->base_hpa, cfmws->window_size,
 					    CFMWS_INTERLEAVE_WAYS(cfmws),
 					    CFMWS_INTERLEAVE_GRANULARITY(cfmws),
-					    CXL_DECODER_EXPANDER,
+					    CXL_DECODER_PLATFORM,
 					    flags);
 
 		if (IS_ERR(cxld)) {
diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index a2e4d54fc7bc..69acdd230f54 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -75,9 +75,9 @@ static ssize_t target_type_show(struct device *dev,
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 
 	switch (cxld->target_type) {
-	case CXL_DECODER_ACCELERATOR:
+	case CXL_DEVICE_ACCELERATOR:
 		return sysfs_emit(buf, "accelerator\n");
-	case CXL_DECODER_EXPANDER:
+	case CXL_DEVICE_EXPANDER:
 		return sysfs_emit(buf, "expander\n");
 	}
 	return -ENXIO;
@@ -167,6 +167,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
 	NULL,
 };
 
+static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
+	&cxl_decoder_base_attribute_group,
+	&cxl_base_attribute_group,
+	NULL,
+};
+
 static void cxl_decoder_release(struct device *dev)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
@@ -176,6 +182,12 @@ static void cxl_decoder_release(struct device *dev)
 	kfree(cxld);
 }
 
+static const struct device_type cxl_decoder_endpoint_type = {
+	.name = "cxl_decoder_endpoint",
+	.release = cxl_decoder_release,
+	.groups = cxl_decoder_endpoint_attribute_groups,
+};
+
 static const struct device_type cxl_decoder_switch_type = {
 	.name = "cxl_decoder_switch",
 	.release = cxl_decoder_release,
@@ -458,12 +470,14 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	if (interleave_ways < 1)
 		return ERR_PTR(-EINVAL);
 
-	device_lock(&port->dev);
-	if (list_empty(&port->dports))
-		rc = -EINVAL;
-	device_unlock(&port->dev);
-	if (rc)
-		return ERR_PTR(rc);
+	if (type != CXL_DECODER_ENDPOINT) {
+		device_lock(&port->dev);
+		if (list_empty(&port->dports))
+			rc = -EINVAL;
+		device_unlock(&port->dev);
+		if (rc)
+			return ERR_PTR(rc);
+	}
 
 	cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
 	if (!cxld)
@@ -479,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 			.start = base,
 			.end = base + len - 1,
 		},
+		.type = type,
 		.flags = flags,
 		.interleave_ways = interleave_ways,
 		.interleave_granularity = interleave_granularity,
-		.target_type = type,
+		.target_type = CXL_DEVICE_EXPANDER,
 	};
 
 	/* handle implied target_list */
@@ -496,10 +511,17 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
 	dev->bus = &cxl_bus_type;
 
 	/* root ports do not have a cxl_port_type parent */
-	if (port->dev.parent->type == &cxl_port_type)
-		dev->type = &cxl_decoder_switch_type;
-	else
+	switch (type) {
+	case CXL_DECODER_PLATFORM:
 		dev->type = &cxl_decoder_root_type;
+		break;
+	case CXL_DECODER_SWITCH:
+		dev->type = &cxl_decoder_switch_type;
+		break;
+	case CXL_DECODER_ENDPOINT:
+		dev->type = &cxl_decoder_endpoint_type;
+		break;
+	}
 
 	return cxld;
 err:
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6bda39a59e3..02e0af4c147c 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -164,6 +164,11 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 #define CXL_RESOURCE_NONE ((resource_size_t) -1)
 #define CXL_TARGET_STRLEN 20
 
+enum cxl_device_type {
+       CXL_DEVICE_ACCELERATOR = 2,
+       CXL_DEVICE_EXPANDER = 3,
+};
+
 /*
  * cxl_decoder flags that define the type of memory / devices this
  * decoder supports as well as configuration lock status See "CXL 2.0
@@ -177,8 +182,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 #define CXL_DECODER_F_MASK  GENMASK(4, 0)
 
 enum cxl_decoder_type {
-       CXL_DECODER_ACCELERATOR = 2,
-       CXL_DECODER_EXPANDER = 3,
+       CXL_DECODER_PLATFORM,
+       CXL_DECODER_SWITCH,
+       CXL_DECODER_ENDPOINT,
 };
 
 /**
@@ -186,19 +192,36 @@ enum cxl_decoder_type {
  * @dev: this decoder's device
  * @id: kernel device name id
  * @range: address range considered by this decoder
+ * @type: the type of this CXL decoder (platform, switch, endpoint)
  * @interleave_ways: number of cxl_dports in this decode
  * @interleave_granularity: data stride per dport
  * @target_type: accelerator vs expander (type2 vs type3) selector
  * @flags: memory type capabilities and locking
  * @target: active ordered target list in current decoder configuration
+ *
+ * Abstractly, a CXL decoder represents one of 3 possible decoders:
+ * 1. Platform specific routing - opaque rules for the memory controller that
+ *    may be communicated via ACPI or devicetree. This decoding has implied
+ *    interleave parameters as well as physical address ranges that are directed
+ *    to the downstream ports of this decoder.
+ * 2. HDM decoder for a switch. Similar to the platform specific routing in that
+ *    it contains a set of downstream ports which receive and send traffic in an
+ *    interleave fashion, the main difference is that the interleave and address
+ *    ranges are controlled by the HDM decoder registers defined in the CXL 2.0
+ *    specification.
+ * 3. HDM decoder for an endpoint. Like the decoder in a switch, this decoder's
+ *    configuration is entirely programmable and defined in CXL spec. Unlike the
+ *    switch's decoder, there is not a set of downstream ports, only the
+ *    underlying media.
  */
 struct cxl_decoder {
 	struct device dev;
 	int id;
 	struct range range;
+	enum cxl_decoder_type type;
 	int interleave_ways;
 	int interleave_granularity;
-	enum cxl_decoder_type target_type;
+	enum cxl_device_type target_type;
 	unsigned long flags;
 	struct cxl_dport *target[];
 };
@@ -289,7 +312,7 @@ static inline struct cxl_decoder *
 devm_cxl_add_passthrough_decoder(struct device *host, struct cxl_port *port)
 {
 	return devm_cxl_add_decoder(host, port, 1, 0, 0, 1, PAGE_SIZE,
-				    CXL_DECODER_EXPANDER, 0);
+				    CXL_DECODER_PLATFORM, 0);
 }
 
 extern struct bus_type cxl_bus_type;
-- 
2.32.0

Re: [PATCH v2] cxl: Enable an endpoint decoder type

[Dan Williams]

On Tue, Jul 6, 2021 at 9:01 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> CXL memory devices support HDM decoders. Currently, when a decoder is
> instantiated there is no knowledge of the type of decoder; only the
> underlying endpoint type is specified. In order to have the memory
> devices reuse the existing decoder creation infrastructure it is
> convenient to pass along the type of decoder on creation.
>
> The primary difference for an endpoint decoder is that it doesn't have
> dports, nor targets. The target is just the underlying media (with
> offset).
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>
> v2 Fixes target_type and stores the decoder type on instantiation

This depends on the memdev driver series? It's not applying for me on
top of cxl.git#next.

>
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 196f260e2580..69acdd230f54 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -493,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
>                         .start = base,
>                         .end = base + len - 1,
>                 },
> +               .type = type,

...but there's already the dev->type?

>                 .flags = flags,
>                 .interleave_ways = interleave_ways,
>                 .interleave_granularity = interleave_granularity,
> -               .target_type = type,
> +               .target_type = CXL_DEVICE_EXPANDER,

In the cxl-switch case how to indicate that only type-2 targets are supported?

I do think I misnamed the cxl_decoder_type. I also did not make it
clear that root decoders don't have a target type, they have a set of
flags indicating their restrictions, and unlike switch level decoders
they can support targeting both accelerators and expanders in the same
windows. I think the decoder can still be just the dev->type, but
there needs to be separate helpers for the 3 cases you have
identified. Something like the following where devm_cxl_add_decoder()
because private to the core:

devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
                              resource_size_t base, resource_size_t len,
                              int interleave_ways, int interleave_granularity,
                              unsigned long flags)
{
        return devm_cxl_add_decoder(host, NULL, nr_targets, base, len,
                                    interleave_ways, interleave_granularity,
                                    0, flags);
}

devm_cxl_add_switch_decoder(struct device *host, struct cxl_port *port,
                            enum cxl_decoder_type type)
{
        return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0,
                                    CXL_DECODER_UNKNOWN, 0);
}

devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
                              enum cxl_decoder_type type)
{
        return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type,
                                    CXL_DECODER_F_ENDPOINT);
}

...where 0 values are filled in by the decoder driver init or N/A.
Presumably the memdev driver calling devm_cxl_add_endpoint_decoder()
will know whether it is an expander or an accelerator. Although given
there are no CXL accelerator drivers on the horizon, maybe even that
degree of freedom can be hidden for now.

Then the dev->type determination is:

platform => no parent cxl_port
switch => parent cxl_port and flags does not have CXL_DECODER_F_ENDPOINT
endpoint =>  parent cxl_port and flags has CXL_DECODER_F_ENDPOINT

...and probably s/cxl_decoder_type/cxl_target_type/ to make it clear
that it's not related to the cxl_decoder_X_type dev->type value.

>         };
>
>         /* handle implied target_list */
>
> ---
>  drivers/cxl/acpi.c |  2 +-
>  drivers/cxl/core.c | 46 ++++++++++++++++++++++++++++++++++------------
>  drivers/cxl/cxl.h  | 31 +++++++++++++++++++++++++++----
>  3 files changed, 62 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 8ae89273f58e..5215845e0f89 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -114,7 +114,7 @@ static void cxl_add_cfmws_decoders(struct device *dev,
>                                             cfmws->base_hpa, cfmws->window_size,
>                                             CFMWS_INTERLEAVE_WAYS(cfmws),
>                                             CFMWS_INTERLEAVE_GRANULARITY(cfmws),
> -                                           CXL_DECODER_EXPANDER,
> +                                           CXL_DECODER_PLATFORM,
>                                             flags);
>
>                 if (IS_ERR(cxld)) {
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index a2e4d54fc7bc..69acdd230f54 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -75,9 +75,9 @@ static ssize_t target_type_show(struct device *dev,
>         struct cxl_decoder *cxld = to_cxl_decoder(dev);
>
>         switch (cxld->target_type) {
> -       case CXL_DECODER_ACCELERATOR:
> +       case CXL_DEVICE_ACCELERATOR:
>                 return sysfs_emit(buf, "accelerator\n");
> -       case CXL_DECODER_EXPANDER:
> +       case CXL_DEVICE_EXPANDER:
>                 return sysfs_emit(buf, "expander\n");
>         }
>         return -ENXIO;
> @@ -167,6 +167,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
>         NULL,
>  };
>
> +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
> +       &cxl_decoder_base_attribute_group,
> +       &cxl_base_attribute_group,
> +       NULL,
> +};
> +
>  static void cxl_decoder_release(struct device *dev)
>  {
>         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> @@ -176,6 +182,12 @@ static void cxl_decoder_release(struct device *dev)
>         kfree(cxld);
>  }
>
> +static const struct device_type cxl_decoder_endpoint_type = {
> +       .name = "cxl_decoder_endpoint",
> +       .release = cxl_decoder_release,
> +       .groups = cxl_decoder_endpoint_attribute_groups,
> +};
> +
>  static const struct device_type cxl_decoder_switch_type = {
>         .name = "cxl_decoder_switch",
>         .release = cxl_decoder_release,
> @@ -458,12 +470,14 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
>         if (interleave_ways < 1)
>                 return ERR_PTR(-EINVAL);
>
> -       device_lock(&port->dev);
> -       if (list_empty(&port->dports))
> -               rc = -EINVAL;
> -       device_unlock(&port->dev);
> -       if (rc)
> -               return ERR_PTR(rc);
> +       if (type != CXL_DECODER_ENDPOINT) {
> +               device_lock(&port->dev);
> +               if (list_empty(&port->dports))
> +                       rc = -EINVAL;
> +               device_unlock(&port->dev);
> +               if (rc)
> +                       return ERR_PTR(rc);
> +       }
>
>         cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
>         if (!cxld)
> @@ -479,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
>                         .start = base,
>                         .end = base + len - 1,
>                 },
> +               .type = type,
>                 .flags = flags,
>                 .interleave_ways = interleave_ways,
>                 .interleave_granularity = interleave_granularity,
> -               .target_type = type,
> +               .target_type = CXL_DEVICE_EXPANDER,
>         };
>
>         /* handle implied target_list */
> @@ -496,10 +511,17 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
>         dev->bus = &cxl_bus_type;
>
>         /* root ports do not have a cxl_port_type parent */
> -       if (port->dev.parent->type == &cxl_port_type)
> -               dev->type = &cxl_decoder_switch_type;
> -       else
> +       switch (type) {
> +       case CXL_DECODER_PLATFORM:
>                 dev->type = &cxl_decoder_root_type;
> +               break;
> +       case CXL_DECODER_SWITCH:
> +               dev->type = &cxl_decoder_switch_type;
> +               break;
> +       case CXL_DECODER_ENDPOINT:
> +               dev->type = &cxl_decoder_endpoint_type;
> +               break;
> +       }
>
>         return cxld;
>  err:
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b6bda39a59e3..02e0af4c147c 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -164,6 +164,11 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
>
> +enum cxl_device_type {
> +       CXL_DEVICE_ACCELERATOR = 2,
> +       CXL_DEVICE_EXPANDER = 3,
> +};
> +
>  /*
>   * cxl_decoder flags that define the type of memory / devices this
>   * decoder supports as well as configuration lock status See "CXL 2.0
> @@ -177,8 +182,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  #define CXL_DECODER_F_MASK  GENMASK(4, 0)
>
>  enum cxl_decoder_type {
> -       CXL_DECODER_ACCELERATOR = 2,
> -       CXL_DECODER_EXPANDER = 3,
> +       CXL_DECODER_PLATFORM,
> +       CXL_DECODER_SWITCH,
> +       CXL_DECODER_ENDPOINT,
>  };
>
>  /**
> @@ -186,19 +192,36 @@ enum cxl_decoder_type {
>   * @dev: this decoder's device
>   * @id: kernel device name id
>   * @range: address range considered by this decoder
> + * @type: the type of this CXL decoder (platform, switch, endpoint)
>   * @interleave_ways: number of cxl_dports in this decode
>   * @interleave_granularity: data stride per dport
>   * @target_type: accelerator vs expander (type2 vs type3) selector
>   * @flags: memory type capabilities and locking
>   * @target: active ordered target list in current decoder configuration
> + *
> + * Abstractly, a CXL decoder represents one of 3 possible decoders:

s/Abstractly, a/A/

Outside of that I like this documentation for describing the
cxl_decoder_X_type options.

> + * 1. Platform specific routing - opaque rules for the memory controller that
> + *    may be communicated via ACPI or devicetree. This decoding has implied
> + *    interleave parameters as well as physical address ranges that are directed
> + *    to the downstream ports of this decoder.
> + * 2. HDM decoder for a switch. Similar to the platform specific routing in that
> + *    it contains a set of downstream ports which receive and send traffic in an
> + *    interleave fashion, the main difference is that the interleave and address
> + *    ranges are controlled by the HDM decoder registers defined in the CXL 2.0
> + *    specification.
> + * 3. HDM decoder for an endpoint. Like the decoder in a switch, this decoder's
> + *    configuration is entirely programmable and defined in CXL spec. Unlike the
> + *    switch's decoder, there is not a set of downstream ports, only the
> + *    underlying media.
>   */
>  struct cxl_decoder {
>         struct device dev;
>         int id;
>         struct range range;
> +       enum cxl_decoder_type type;
>         int interleave_ways;
>         int interleave_granularity;
> -       enum cxl_decoder_type target_type;
> +       enum cxl_device_type target_type;
>         unsigned long flags;
>         struct cxl_dport *target[];
>  };
> @@ -289,7 +312,7 @@ static inline struct cxl_decoder *
>  devm_cxl_add_passthrough_decoder(struct device *host, struct cxl_port *port)
>  {
>         return devm_cxl_add_decoder(host, port, 1, 0, 0, 1, PAGE_SIZE,
> -                                   CXL_DECODER_EXPANDER, 0);
> +                                   CXL_DECODER_PLATFORM, 0);
>  }
>
>  extern struct bus_type cxl_bus_type;
> --
> 2.32.0
>

Re: [PATCH v2] cxl: Enable an endpoint decoder type

[Ben Widawsky]

On 21-07-09 16:56:48, Dan Williams wrote:
> On Tue, Jul 6, 2021 at 9:01 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > CXL memory devices support HDM decoders. Currently, when a decoder is
> > instantiated there is no knowledge of the type of decoder; only the
> > underlying endpoint type is specified. In order to have the memory
> > devices reuse the existing decoder creation infrastructure it is
> > convenient to pass along the type of decoder on creation.
> >
> > The primary difference for an endpoint decoder is that it doesn't have
> > dports, nor targets. The target is just the underlying media (with
> > offset).
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >
> > v2 Fixes target_type and stores the decoder type on instantiation
> 
> This depends on the memdev driver series? It's not applying for me on
> top of cxl.git#next.
> 

Weird, it shouldn't. All my other branches depend on core-reorg work, where
core.c isn't a thing anymore. Would you like to get this in shape before the
core rework? I think git should dtrt, but it might be easier to merge that
first.

> >
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index 196f260e2580..69acdd230f54 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -493,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >                         .start = base,
> >                         .end = base + len - 1,
> >                 },
> > +               .type = type,
> 
> ...but there's already the dev->type?

Good point. It's redundant.

> 
> >                 .flags = flags,
> >                 .interleave_ways = interleave_ways,
> >                 .interleave_granularity = interleave_granularity,
> > -               .target_type = type,
> > +               .target_type = CXL_DEVICE_EXPANDER,
> 
> In the cxl-switch case how to indicate that only type-2 targets are supported?
> 
> I do think I misnamed the cxl_decoder_type. I also did not make it
> clear that root decoders don't have a target type, they have a set of
> flags indicating their restrictions, and unlike switch level decoders
> they can support targeting both accelerators and expanders in the same
> windows. I think the decoder can still be just the dev->type, but
> there needs to be separate helpers for the 3 cases you have
> identified. Something like the following where devm_cxl_add_decoder()
> because private to the core:
> 
> devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
>                               resource_size_t base, resource_size_t len,
>                               int interleave_ways, int interleave_granularity,
>                               unsigned long flags)
> {
>         return devm_cxl_add_decoder(host, NULL, nr_targets, base, len,
>                                     interleave_ways, interleave_granularity,
>                                     0, flags);
> }
> 
> devm_cxl_add_switch_decoder(struct device *host, struct cxl_port *port,
>                             enum cxl_decoder_type type)
> {
>         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0,
>                                     CXL_DECODER_UNKNOWN, 0);
> }
> 
> devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
>                               enum cxl_decoder_type type)
> {
>         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type,
>                                     CXL_DECODER_F_ENDPOINT);
> }
> 
> ...where 0 values are filled in by the decoder driver init or N/A.
> Presumably the memdev driver calling devm_cxl_add_endpoint_decoder()
> will know whether it is an expander or an accelerator. Although given
> there are no CXL accelerator drivers on the horizon, maybe even that
> degree of freedom can be hidden for now.
> 
> Then the dev->type determination is:
> 
> platform => no parent cxl_port
> switch => parent cxl_port and flags does not have CXL_DECODER_F_ENDPOINT
> endpoint =>  parent cxl_port and flags has CXL_DECODER_F_ENDPOINT
> 

I'm definitely in favor of being more explicit. Are you opposed to having the
helper set the type and getting rid of endpoint flag? I think it's a little
non-idiomatic, but I do prefer it stylistically.

ie.
devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
                              enum cxl_decoder_type type)
{
	struct cxl_decoder *cxld;

        cxld = devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type, 0);
	if (!ret)
		cxld->dev.type = &cxl_decoder_endpoint_type;

	return cxld;
}

> ...and probably s/cxl_decoder_type/cxl_target_type/ to make it clear
> that it's not related to the cxl_decoder_X_type dev->type value.
> 

Okay.

> >         };
> >
> >         /* handle implied target_list */
> >
> > ---
> >  drivers/cxl/acpi.c |  2 +-
> >  drivers/cxl/core.c | 46 ++++++++++++++++++++++++++++++++++------------
> >  drivers/cxl/cxl.h  | 31 +++++++++++++++++++++++++++----
> >  3 files changed, 62 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 8ae89273f58e..5215845e0f89 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -114,7 +114,7 @@ static void cxl_add_cfmws_decoders(struct device *dev,
> >                                             cfmws->base_hpa, cfmws->window_size,
> >                                             CFMWS_INTERLEAVE_WAYS(cfmws),
> >                                             CFMWS_INTERLEAVE_GRANULARITY(cfmws),
> > -                                           CXL_DECODER_EXPANDER,
> > +                                           CXL_DECODER_PLATFORM,
> >                                             flags);
> >
> >                 if (IS_ERR(cxld)) {
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index a2e4d54fc7bc..69acdd230f54 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -75,9 +75,9 @@ static ssize_t target_type_show(struct device *dev,
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> >
> >         switch (cxld->target_type) {
> > -       case CXL_DECODER_ACCELERATOR:
> > +       case CXL_DEVICE_ACCELERATOR:
> >                 return sysfs_emit(buf, "accelerator\n");
> > -       case CXL_DECODER_EXPANDER:
> > +       case CXL_DEVICE_EXPANDER:
> >                 return sysfs_emit(buf, "expander\n");
> >         }
> >         return -ENXIO;
> > @@ -167,6 +167,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
> >         NULL,
> >  };
> >
> > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
> > +       &cxl_decoder_base_attribute_group,
> > +       &cxl_base_attribute_group,
> > +       NULL,
> > +};
> > +
> >  static void cxl_decoder_release(struct device *dev)
> >  {
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > @@ -176,6 +182,12 @@ static void cxl_decoder_release(struct device *dev)
> >         kfree(cxld);
> >  }
> >
> > +static const struct device_type cxl_decoder_endpoint_type = {
> > +       .name = "cxl_decoder_endpoint",
> > +       .release = cxl_decoder_release,
> > +       .groups = cxl_decoder_endpoint_attribute_groups,
> > +};
> > +
> >  static const struct device_type cxl_decoder_switch_type = {
> >         .name = "cxl_decoder_switch",
> >         .release = cxl_decoder_release,
> > @@ -458,12 +470,14 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >         if (interleave_ways < 1)
> >                 return ERR_PTR(-EINVAL);
> >
> > -       device_lock(&port->dev);
> > -       if (list_empty(&port->dports))
> > -               rc = -EINVAL;
> > -       device_unlock(&port->dev);
> > -       if (rc)
> > -               return ERR_PTR(rc);
> > +       if (type != CXL_DECODER_ENDPOINT) {
> > +               device_lock(&port->dev);
> > +               if (list_empty(&port->dports))
> > +                       rc = -EINVAL;
> > +               device_unlock(&port->dev);
> > +               if (rc)
> > +                       return ERR_PTR(rc);
> > +       }
> >
> >         cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> >         if (!cxld)
> > @@ -479,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >                         .start = base,
> >                         .end = base + len - 1,
> >                 },
> > +               .type = type,
> >                 .flags = flags,
> >                 .interleave_ways = interleave_ways,
> >                 .interleave_granularity = interleave_granularity,
> > -               .target_type = type,
> > +               .target_type = CXL_DEVICE_EXPANDER,
> >         };
> >
> >         /* handle implied target_list */
> > @@ -496,10 +511,17 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >         dev->bus = &cxl_bus_type;
> >
> >         /* root ports do not have a cxl_port_type parent */
> > -       if (port->dev.parent->type == &cxl_port_type)
> > -               dev->type = &cxl_decoder_switch_type;
> > -       else
> > +       switch (type) {
> > +       case CXL_DECODER_PLATFORM:
> >                 dev->type = &cxl_decoder_root_type;
> > +               break;
> > +       case CXL_DECODER_SWITCH:
> > +               dev->type = &cxl_decoder_switch_type;
> > +               break;
> > +       case CXL_DECODER_ENDPOINT:
> > +               dev->type = &cxl_decoder_endpoint_type;
> > +               break;
> > +       }
> >
> >         return cxld;
> >  err:
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index b6bda39a59e3..02e0af4c147c 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -164,6 +164,11 @@ int cxl_map_device_regs(struct pci_dev *pdev,
> >  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> >  #define CXL_TARGET_STRLEN 20
> >
> > +enum cxl_device_type {
> > +       CXL_DEVICE_ACCELERATOR = 2,
> > +       CXL_DEVICE_EXPANDER = 3,
> > +};
> > +
> >  /*
> >   * cxl_decoder flags that define the type of memory / devices this
> >   * decoder supports as well as configuration lock status See "CXL 2.0
> > @@ -177,8 +182,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
> >  #define CXL_DECODER_F_MASK  GENMASK(4, 0)
> >
> >  enum cxl_decoder_type {
> > -       CXL_DECODER_ACCELERATOR = 2,
> > -       CXL_DECODER_EXPANDER = 3,
> > +       CXL_DECODER_PLATFORM,
> > +       CXL_DECODER_SWITCH,
> > +       CXL_DECODER_ENDPOINT,
> >  };
> >
> >  /**
> > @@ -186,19 +192,36 @@ enum cxl_decoder_type {
> >   * @dev: this decoder's device
> >   * @id: kernel device name id
> >   * @range: address range considered by this decoder
> > + * @type: the type of this CXL decoder (platform, switch, endpoint)
> >   * @interleave_ways: number of cxl_dports in this decode
> >   * @interleave_granularity: data stride per dport
> >   * @target_type: accelerator vs expander (type2 vs type3) selector
> >   * @flags: memory type capabilities and locking
> >   * @target: active ordered target list in current decoder configuration
> > + *
> > + * Abstractly, a CXL decoder represents one of 3 possible decoders:
> 
> s/Abstractly, a/A/
> 
> Outside of that I like this documentation for describing the
> cxl_decoder_X_type options.
> 

Will incorporate.

> > + * 1. Platform specific routing - opaque rules for the memory controller that
> > + *    may be communicated via ACPI or devicetree. This decoding has implied
> > + *    interleave parameters as well as physical address ranges that are directed
> > + *    to the downstream ports of this decoder.
> > + * 2. HDM decoder for a switch. Similar to the platform specific routing in that
> > + *    it contains a set of downstream ports which receive and send traffic in an
> > + *    interleave fashion, the main difference is that the interleave and address
> > + *    ranges are controlled by the HDM decoder registers defined in the CXL 2.0
> > + *    specification.
> > + * 3. HDM decoder for an endpoint. Like the decoder in a switch, this decoder's
> > + *    configuration is entirely programmable and defined in CXL spec. Unlike the
> > + *    switch's decoder, there is not a set of downstream ports, only the
> > + *    underlying media.
> >   */
> >  struct cxl_decoder {
> >         struct device dev;
> >         int id;
> >         struct range range;
> > +       enum cxl_decoder_type type;
> >         int interleave_ways;
> >         int interleave_granularity;
> > -       enum cxl_decoder_type target_type;
> > +       enum cxl_device_type target_type;
> >         unsigned long flags;
> >         struct cxl_dport *target[];
> >  };
> > @@ -289,7 +312,7 @@ static inline struct cxl_decoder *
> >  devm_cxl_add_passthrough_decoder(struct device *host, struct cxl_port *port)
> >  {
> >         return devm_cxl_add_decoder(host, port, 1, 0, 0, 1, PAGE_SIZE,
> > -                                   CXL_DECODER_EXPANDER, 0);
> > +                                   CXL_DECODER_PLATFORM, 0);
> >  }
> >
> >  extern struct bus_type cxl_bus_type;
> > --
> > 2.32.0
> >

Re: [PATCH v2] cxl: Enable an endpoint decoder type

[Dan Williams]

On Fri, Jul 16, 2021 at 4:37 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-07-09 16:56:48, Dan Williams wrote:
> > On Tue, Jul 6, 2021 at 9:01 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > CXL memory devices support HDM decoders. Currently, when a decoder is
> > > instantiated there is no knowledge of the type of decoder; only the
> > > underlying endpoint type is specified. In order to have the memory
> > > devices reuse the existing decoder creation infrastructure it is
> > > convenient to pass along the type of decoder on creation.
> > >
> > > The primary difference for an endpoint decoder is that it doesn't have
> > > dports, nor targets. The target is just the underlying media (with
> > > offset).
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >
> > > v2 Fixes target_type and stores the decoder type on instantiation
> >
> > This depends on the memdev driver series? It's not applying for me on
> > top of cxl.git#next.
> >
>
> Weird, it shouldn't. All my other branches depend on core-reorg work, where
> core.c isn't a thing anymore. Would you like to get this in shape before the
> core rework? I think git should dtrt, but it might be easier to merge that
> first.

Yeah, I think it logically makes sense to finish off the core
representation of ports before enabling them.

>
> > >
> > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > > index 196f260e2580..69acdd230f54 100644
> > > --- a/drivers/cxl/core.c
> > > +++ b/drivers/cxl/core.c
> > > @@ -493,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> > >                         .start = base,
> > >                         .end = base + len - 1,
> > >                 },
> > > +               .type = type,
> >
> > ...but there's already the dev->type?
>
> Good point. It's redundant.
>
> >
> > >                 .flags = flags,
> > >                 .interleave_ways = interleave_ways,
> > >                 .interleave_granularity = interleave_granularity,
> > > -               .target_type = type,
> > > +               .target_type = CXL_DEVICE_EXPANDER,
> >
> > In the cxl-switch case how to indicate that only type-2 targets are supported?
> >
> > I do think I misnamed the cxl_decoder_type. I also did not make it
> > clear that root decoders don't have a target type, they have a set of
> > flags indicating their restrictions, and unlike switch level decoders
> > they can support targeting both accelerators and expanders in the same
> > windows. I think the decoder can still be just the dev->type, but
> > there needs to be separate helpers for the 3 cases you have
> > identified. Something like the following where devm_cxl_add_decoder()
> > because private to the core:
> >
> > devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
> >                               resource_size_t base, resource_size_t len,
> >                               int interleave_ways, int interleave_granularity,
> >                               unsigned long flags)
> > {
> >         return devm_cxl_add_decoder(host, NULL, nr_targets, base, len,
> >                                     interleave_ways, interleave_granularity,
> >                                     0, flags);
> > }
> >
> > devm_cxl_add_switch_decoder(struct device *host, struct cxl_port *port,
> >                             enum cxl_decoder_type type)
> > {
> >         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0,
> >                                     CXL_DECODER_UNKNOWN, 0);
> > }
> >
> > devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
> >                               enum cxl_decoder_type type)
> > {
> >         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type,
> >                                     CXL_DECODER_F_ENDPOINT);
> > }
> >
> > ...where 0 values are filled in by the decoder driver init or N/A.
> > Presumably the memdev driver calling devm_cxl_add_endpoint_decoder()
> > will know whether it is an expander or an accelerator. Although given
> > there are no CXL accelerator drivers on the horizon, maybe even that
> > degree of freedom can be hidden for now.
> >
> > Then the dev->type determination is:
> >
> > platform => no parent cxl_port
> > switch => parent cxl_port and flags does not have CXL_DECODER_F_ENDPOINT
> > endpoint =>  parent cxl_port and flags has CXL_DECODER_F_ENDPOINT
> >
>
> I'm definitely in favor of being more explicit.

The exported function names are not explicit enough?

The thought is that the baseline devm_cxl_add_port() becomes static
and private to the core.

> Are you opposed to having the
> helper set the type and getting rid of endpoint flag? I think it's a little
> non-idiomatic, but I do prefer it stylistically.

The endpoint flag is to disambiguate switches vs endpoints...

>
> ie.
> devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
>                               enum cxl_decoder_type type)

...but type is CXL_DECODER_{PLATFORM,SWITCH,ENDPOINT}, what does this
function do when it is passed anything CXL_DECODER_PLATFORM? My
original intention for 'type' was 'expander' vs 'accelerator' since
switch decoders exclusively support one or the other.

> {
>         struct cxl_decoder *cxld;
>
>         cxld = devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type, 0);
>         if (!ret)
>                 cxld->dev.type = &cxl_decoder_endpoint_type;

Unfortunately this is too late as dev->type is in use immediately upon
device_add().

Re: [PATCH v2] cxl: Enable an endpoint decoder type

[Ben Widawsky]

On 21-07-09 16:56:48, Dan Williams wrote:
> On Tue, Jul 6, 2021 at 9:01 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > CXL memory devices support HDM decoders. Currently, when a decoder is
> > instantiated there is no knowledge of the type of decoder; only the
> > underlying endpoint type is specified. In order to have the memory
> > devices reuse the existing decoder creation infrastructure it is
> > convenient to pass along the type of decoder on creation.
> >
> > The primary difference for an endpoint decoder is that it doesn't have
> > dports, nor targets. The target is just the underlying media (with
> > offset).
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >
> > v2 Fixes target_type and stores the decoder type on instantiation
> 
> This depends on the memdev driver series? It's not applying for me on
> top of cxl.git#next.
> 
> >
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index 196f260e2580..69acdd230f54 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -493,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >                         .start = base,
> >                         .end = base + len - 1,
> >                 },
> > +               .type = type,
> 
> ...but there's already the dev->type?
> 
> >                 .flags = flags,
> >                 .interleave_ways = interleave_ways,
> >                 .interleave_granularity = interleave_granularity,
> > -               .target_type = type,
> > +               .target_type = CXL_DEVICE_EXPANDER,
> 
> In the cxl-switch case how to indicate that only type-2 targets are supported?
> 
> I do think I misnamed the cxl_decoder_type. I also did not make it
> clear that root decoders don't have a target type, they have a set of
> flags indicating their restrictions, and unlike switch level decoders
> they can support targeting both accelerators and expanders in the same
> windows. I think the decoder can still be just the dev->type, but
> there needs to be separate helpers for the 3 cases you have
> identified. Something like the following where devm_cxl_add_decoder()
> because private to the core:
> 
> devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
>                               resource_size_t base, resource_size_t len,
>                               int interleave_ways, int interleave_granularity,
>                               unsigned long flags)
> {
>         return devm_cxl_add_decoder(host, NULL, nr_targets, base, len,
>                                     interleave_ways, interleave_granularity,
>                                     0, flags);
> }

How do you want me to handle the existing parent (ACPI0017) and child (ACPI0016)
relationships? I see no reason to preserve this, but perhaps you know something
I don't.

> 
> devm_cxl_add_switch_decoder(struct device *host, struct cxl_port *port,
>                             enum cxl_decoder_type type)
> {
>         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0,
>                                     CXL_DECODER_UNKNOWN, 0);
> }
> 
> devm_cxl_add_endpoint_decoder(struct device *host, struct cxl_port *port,
>                               enum cxl_decoder_type type)
> {
>         return devm_cxl_add_decoder(host, port, 0, 0, 0, 0, 0, type,
>                                     CXL_DECODER_F_ENDPOINT);
> }
> 
> ...where 0 values are filled in by the decoder driver init or N/A.
> Presumably the memdev driver calling devm_cxl_add_endpoint_decoder()
> will know whether it is an expander or an accelerator. Although given
> there are no CXL accelerator drivers on the horizon, maybe even that
> degree of freedom can be hidden for now.
> 
> Then the dev->type determination is:
> 
> platform => no parent cxl_port
> switch => parent cxl_port and flags does not have CXL_DECODER_F_ENDPOINT
> endpoint =>  parent cxl_port and flags has CXL_DECODER_F_ENDPOINT
> 
> ...and probably s/cxl_decoder_type/cxl_target_type/ to make it clear
> that it's not related to the cxl_decoder_X_type dev->type value.
> 
> >         };
> >
> >         /* handle implied target_list */
> >
> > ---
> >  drivers/cxl/acpi.c |  2 +-
> >  drivers/cxl/core.c | 46 ++++++++++++++++++++++++++++++++++------------
> >  drivers/cxl/cxl.h  | 31 +++++++++++++++++++++++++++----
> >  3 files changed, 62 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 8ae89273f58e..5215845e0f89 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -114,7 +114,7 @@ static void cxl_add_cfmws_decoders(struct device *dev,
> >                                             cfmws->base_hpa, cfmws->window_size,
> >                                             CFMWS_INTERLEAVE_WAYS(cfmws),
> >                                             CFMWS_INTERLEAVE_GRANULARITY(cfmws),
> > -                                           CXL_DECODER_EXPANDER,
> > +                                           CXL_DECODER_PLATFORM,
> >                                             flags);
> >
> >                 if (IS_ERR(cxld)) {
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index a2e4d54fc7bc..69acdd230f54 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -75,9 +75,9 @@ static ssize_t target_type_show(struct device *dev,
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> >
> >         switch (cxld->target_type) {
> > -       case CXL_DECODER_ACCELERATOR:
> > +       case CXL_DEVICE_ACCELERATOR:
> >                 return sysfs_emit(buf, "accelerator\n");
> > -       case CXL_DECODER_EXPANDER:
> > +       case CXL_DEVICE_EXPANDER:
> >                 return sysfs_emit(buf, "expander\n");
> >         }
> >         return -ENXIO;
> > @@ -167,6 +167,12 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
> >         NULL,
> >  };
> >
> > +static const struct attribute_group *cxl_decoder_endpoint_attribute_groups[] = {
> > +       &cxl_decoder_base_attribute_group,
> > +       &cxl_base_attribute_group,
> > +       NULL,
> > +};
> > +
> >  static void cxl_decoder_release(struct device *dev)
> >  {
> >         struct cxl_decoder *cxld = to_cxl_decoder(dev);
> > @@ -176,6 +182,12 @@ static void cxl_decoder_release(struct device *dev)
> >         kfree(cxld);
> >  }
> >
> > +static const struct device_type cxl_decoder_endpoint_type = {
> > +       .name = "cxl_decoder_endpoint",
> > +       .release = cxl_decoder_release,
> > +       .groups = cxl_decoder_endpoint_attribute_groups,
> > +};
> > +
> >  static const struct device_type cxl_decoder_switch_type = {
> >         .name = "cxl_decoder_switch",
> >         .release = cxl_decoder_release,
> > @@ -458,12 +470,14 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >         if (interleave_ways < 1)
> >                 return ERR_PTR(-EINVAL);
> >
> > -       device_lock(&port->dev);
> > -       if (list_empty(&port->dports))
> > -               rc = -EINVAL;
> > -       device_unlock(&port->dev);
> > -       if (rc)
> > -               return ERR_PTR(rc);
> > +       if (type != CXL_DECODER_ENDPOINT) {
> > +               device_lock(&port->dev);
> > +               if (list_empty(&port->dports))
> > +                       rc = -EINVAL;
> > +               device_unlock(&port->dev);
> > +               if (rc)
> > +                       return ERR_PTR(rc);
> > +       }
> >
> >         cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> >         if (!cxld)
> > @@ -479,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >                         .start = base,
> >                         .end = base + len - 1,
> >                 },
> > +               .type = type,
> >                 .flags = flags,
> >                 .interleave_ways = interleave_ways,
> >                 .interleave_granularity = interleave_granularity,
> > -               .target_type = type,
> > +               .target_type = CXL_DEVICE_EXPANDER,
> >         };
> >
> >         /* handle implied target_list */
> > @@ -496,10 +511,17 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> >         dev->bus = &cxl_bus_type;
> >
> >         /* root ports do not have a cxl_port_type parent */
> > -       if (port->dev.parent->type == &cxl_port_type)
> > -               dev->type = &cxl_decoder_switch_type;
> > -       else
> > +       switch (type) {
> > +       case CXL_DECODER_PLATFORM:
> >                 dev->type = &cxl_decoder_root_type;
> > +               break;
> > +       case CXL_DECODER_SWITCH:
> > +               dev->type = &cxl_decoder_switch_type;
> > +               break;
> > +       case CXL_DECODER_ENDPOINT:
> > +               dev->type = &cxl_decoder_endpoint_type;
> > +               break;
> > +       }
> >
> >         return cxld;
> >  err:
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index b6bda39a59e3..02e0af4c147c 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -164,6 +164,11 @@ int cxl_map_device_regs(struct pci_dev *pdev,
> >  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> >  #define CXL_TARGET_STRLEN 20
> >
> > +enum cxl_device_type {
> > +       CXL_DEVICE_ACCELERATOR = 2,
> > +       CXL_DEVICE_EXPANDER = 3,
> > +};
> > +
> >  /*
> >   * cxl_decoder flags that define the type of memory / devices this
> >   * decoder supports as well as configuration lock status See "CXL 2.0
> > @@ -177,8 +182,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
> >  #define CXL_DECODER_F_MASK  GENMASK(4, 0)
> >
> >  enum cxl_decoder_type {
> > -       CXL_DECODER_ACCELERATOR = 2,
> > -       CXL_DECODER_EXPANDER = 3,
> > +       CXL_DECODER_PLATFORM,
> > +       CXL_DECODER_SWITCH,
> > +       CXL_DECODER_ENDPOINT,
> >  };
> >
> >  /**
> > @@ -186,19 +192,36 @@ enum cxl_decoder_type {
> >   * @dev: this decoder's device
> >   * @id: kernel device name id
> >   * @range: address range considered by this decoder
> > + * @type: the type of this CXL decoder (platform, switch, endpoint)
> >   * @interleave_ways: number of cxl_dports in this decode
> >   * @interleave_granularity: data stride per dport
> >   * @target_type: accelerator vs expander (type2 vs type3) selector
> >   * @flags: memory type capabilities and locking
> >   * @target: active ordered target list in current decoder configuration
> > + *
> > + * Abstractly, a CXL decoder represents one of 3 possible decoders:
> 
> s/Abstractly, a/A/
> 
> Outside of that I like this documentation for describing the
> cxl_decoder_X_type options.
> 
> > + * 1. Platform specific routing - opaque rules for the memory controller that
> > + *    may be communicated via ACPI or devicetree. This decoding has implied
> > + *    interleave parameters as well as physical address ranges that are directed
> > + *    to the downstream ports of this decoder.
> > + * 2. HDM decoder for a switch. Similar to the platform specific routing in that
> > + *    it contains a set of downstream ports which receive and send traffic in an
> > + *    interleave fashion, the main difference is that the interleave and address
> > + *    ranges are controlled by the HDM decoder registers defined in the CXL 2.0
> > + *    specification.
> > + * 3. HDM decoder for an endpoint. Like the decoder in a switch, this decoder's
> > + *    configuration is entirely programmable and defined in CXL spec. Unlike the
> > + *    switch's decoder, there is not a set of downstream ports, only the
> > + *    underlying media.
> >   */
> >  struct cxl_decoder {
> >         struct device dev;
> >         int id;
> >         struct range range;
> > +       enum cxl_decoder_type type;
> >         int interleave_ways;
> >         int interleave_granularity;
> > -       enum cxl_decoder_type target_type;
> > +       enum cxl_device_type target_type;
> >         unsigned long flags;
> >         struct cxl_dport *target[];
> >  };
> > @@ -289,7 +312,7 @@ static inline struct cxl_decoder *
> >  devm_cxl_add_passthrough_decoder(struct device *host, struct cxl_port *port)
> >  {
> >         return devm_cxl_add_decoder(host, port, 1, 0, 0, 1, PAGE_SIZE,
> > -                                   CXL_DECODER_EXPANDER, 0);
> > +                                   CXL_DECODER_PLATFORM, 0);
> >  }
> >
> >  extern struct bus_type cxl_bus_type;
> > --
> > 2.32.0
> >

Re: [PATCH v2] cxl: Enable an endpoint decoder type

[Dan Williams]

On Tue, Jul 20, 2021 at 4:43 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-07-09 16:56:48, Dan Williams wrote:
> > On Tue, Jul 6, 2021 at 9:01 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > CXL memory devices support HDM decoders. Currently, when a decoder is
> > > instantiated there is no knowledge of the type of decoder; only the
> > > underlying endpoint type is specified. In order to have the memory
> > > devices reuse the existing decoder creation infrastructure it is
> > > convenient to pass along the type of decoder on creation.
> > >
> > > The primary difference for an endpoint decoder is that it doesn't have
> > > dports, nor targets. The target is just the underlying media (with
> > > offset).
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >
> > > v2 Fixes target_type and stores the decoder type on instantiation
> >
> > This depends on the memdev driver series? It's not applying for me on
> > top of cxl.git#next.
> >
> > >
> > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > > index 196f260e2580..69acdd230f54 100644
> > > --- a/drivers/cxl/core.c
> > > +++ b/drivers/cxl/core.c
> > > @@ -493,10 +493,11 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> > >                         .start = base,
> > >                         .end = base + len - 1,
> > >                 },
> > > +               .type = type,
> >
> > ...but there's already the dev->type?
> >
> > >                 .flags = flags,
> > >                 .interleave_ways = interleave_ways,
> > >                 .interleave_granularity = interleave_granularity,
> > > -               .target_type = type,
> > > +               .target_type = CXL_DEVICE_EXPANDER,
> >
> > In the cxl-switch case how to indicate that only type-2 targets are supported?
> >
> > I do think I misnamed the cxl_decoder_type. I also did not make it
> > clear that root decoders don't have a target type, they have a set of
> > flags indicating their restrictions, and unlike switch level decoders
> > they can support targeting both accelerators and expanders in the same
> > windows. I think the decoder can still be just the dev->type, but
> > there needs to be separate helpers for the 3 cases you have
> > identified. Something like the following where devm_cxl_add_decoder()
> > because private to the core:
> >
> > devm_cxl_add_platform_decoder(struct device *host, int nr_targets,
> >                               resource_size_t base, resource_size_t len,
> >                               int interleave_ways, int interleave_granularity,
> >                               unsigned long flags)
> > {
> >         return devm_cxl_add_decoder(host, NULL, nr_targets, base, len,
> >                                     interleave_ways, interleave_granularity,
> >                                     0, flags);
> > }
>
> How do you want me to handle the existing parent (ACPI0017) and child (ACPI0016)
> relationships? I see no reason to preserve this, but perhaps you know something
> I don't.

I'm not understanding the concern? ACPI0016 is already not a child of
ACPI0017. The decoder is a child of a port. ACPI0016 is a dport in a
platform decoder, and the uport for the CXL switch in the host-bridge.

Re: [PATCH] cxl: Enable an endpoint decoder type

[Dan Williams]

On Thu, Jul 1, 2021 at 9:00 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> CXL memory devices support HDM decoders. Currently, when a decoder is
> instantiated there is no knowledge of the type of decoder; only the
> underlying endpoint type is specified. In order to have the memory
> devices reuse the existing decoder creation infrastructure it is
> convenient to pass along the type of decoder on creation.
>
> The primary difference for an endpoint decoder is that it doesn't have
> dports, nor targets. The target is just the underlying media (with
> offset).
>

It's been a while, can you redo this patch just focusing on how to
represent an endpoint without redoing the typing for existing
decoders? It's not clear that cxl_decoder_type needs any updates. An
endpoint decoder is either accelerator or expander (2 or 3). What
makes an endpoint is merely that it is terminal in the sysfs decode
topology. Just like the difference between root decoders and switch
decoders is that root ones are origins of a decode hierarchy. You
identify root decoders by the parent port type and endpoints by the
lack of a target_map.

Yes, target_map is a recent addition, but it seems to naturally fit
the endpoint decoder definition since the target list is replaced with
DPA Skip Low on endpoints.

In short I don't think any new type information is needed, it can all
be inferred inside devm_cxl_add_decoder().