[PATCH v3 0/4] Enhance definition of DFH and use enhancements for uart driver

[PATCH v3 0/4] Enhance definition of DFH and use enhancements for uart driver

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@intel.com>

This patchset enhances the definition of the Device Feature Header (DFH) used by
the Device Feature List (DFL) bus and then uses the new enhancements in a uart
driver.

Patch 1 updates the DFL documentation to provide the motivation behind the 
enhancements to the definition of the DFH.

Patch 2 adds the definitions for DFHv1.

Patch 3 adds basic support DFHv1. It provides a generic mechanism for
describing MSIX interrupts used by a particular feature instance, and
it gets the location and size of the feature's register set from DFHv1.

Patch 4 adds a DFL uart driver that makes use of the new features of DFHv1.

Basheer Ahmed Muddebihal (1):
  fpga: dfl: Add DFHv1 Register Definitions

Matthew Gerlach (3):
  Documentation: fpga: dfl: Add documentation for DFHv1
  fpga: dfl: add basic support for DFHv1
  tty: serial: 8250: add DFL bus driver for Altera 16550.

 Documentation/fpga/dfl.rst         |  49 ++++++++
 drivers/fpga/dfl.c                 | 150 +++++++++++++++++-------
 drivers/fpga/dfl.h                 |  36 +++++-
 drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |   9 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 include/linux/dfl.h                |  33 +++++-
 7 files changed, 414 insertions(+), 41 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

-- 
2.25.1

[PATCH v3 1/4] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add documentation describing the extensions provided by Version
1 of the Device Feature Header (DFHv1).

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3: no change

v2: s/GUILD/GUID/
    add picture
---
 Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 15b670926084..7c786b75b498 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
 driver's id_table.
 
 
+Extending the Device Feature Header - DFHv1
+===========================================
+The current 8 bytes of the Device Feature Header, hereafter referred to as
+to DFHv0, provide very little opportunity for the hardware to describe itself
+to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
+to provide increased flexibility and extensibility to hardware designs using
+Device Feature Lists.  The list below describes some of the goals behind the
+changes in DFHv1:
+
+* Provide a standardized mechanism for features to describe
+  parameters/capabilities to software.
+* Standardize the use of a GUID for all DFHv1 types.
+* Decouple the location of the DFH from the register space of the feature itself.
+
+Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
+a list of parameter values to a particular feature.
+
+With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUID standard
+across all types.
+
+With DFHv0, the register map of a given feature is located immediately following
+the DFHv0 in the memory space.  With DFHv1, the location of the feature register
+map can be specified as an offset to the DFHv1 or as an absolute address.  The DFHv1
+structure is shown below:
+
+    +-----------------------------------------------------------------------+
+    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_L                             0|
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_H                             0|
+    +-----------------------------------------------------------------------+
+    |63                 Address/Offset                            1|  Rel  0|
+    +-----------------------------------------------------------------------+
+    |63 Size of register set  32|Params 31|30 Group    16|15 Instance      0|
+    +-----------------------------------------------------------------------+
+    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
+    +-----------------------------------------------------------------------+
+    |63                 Parameter Data                                     0|
+    +-----------------------------------------------------------------------+
+
+                                  ...
+
+    +-----------------------------------------------------------------------+
+    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
+    +-----------------------------------------------------------------------+
+    |63                 Parameter Data                                     0|
+    +-----------------------------------------------------------------------+
+
 Open discussion
 ===============
 FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
-- 
2.25.1

Re: [PATCH v3 1/4] Documentation: fpga: dfl: Add documentation for DFHv1

[Ilpo Järvinen]

On Tue, 4 Oct 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add documentation describing the extensions provided by Version
> 1 of the Device Feature Header (DFHv1).
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v3: no change
> 
> v2: s/GUILD/GUID/
>     add picture
> ---
>  Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 15b670926084..7c786b75b498 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
>  driver's id_table.
>  
>  
> +Extending the Device Feature Header - DFHv1
> +===========================================
> +The current 8 bytes of the Device Feature Header, hereafter referred to as
> +to DFHv0, provide very little opportunity for the hardware to describe itself
> +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
> +to provide increased flexibility and extensibility to hardware designs using
> +Device Feature Lists.  The list below describes some of the goals behind the
> +changes in DFHv1:
> +
> +* Provide a standardized mechanism for features to describe
> +  parameters/capabilities to software.
> +* Standardize the use of a GUID for all DFHv1 types.
> +* Decouple the location of the DFH from the register space of the feature itself.
> +
> +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
> +a list of parameter values to a particular feature.
> +
> +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUID standard
> +across all types.
> +
> +With DFHv0, the register map of a given feature is located immediately following
> +the DFHv0 in the memory space.  With DFHv1, the location of the feature register
> +map can be specified as an offset to the DFHv1 or as an absolute address.  The DFHv1
> +structure is shown below:

I think this is not a good place for be some kind of v1 marketing speak 
(that said, I think it's fine to include those goals you have there).

I'd restructure this so that this section only talks about DFHv1 w/o 
any comparing how v1 is better than v0. Don't base the description on 
how things changed from v0 but just describe v1, that is, like v1 is 
already there, not only being introduced to supercede/extend v0.

And then create v0 section after this section which focuses solely on v0.

-- 
 i.

> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0|
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Address/Offset                            1|  Rel  0|
> +    +-----------------------------------------------------------------------+
> +    |63 Size of register set  32|Params 31|30 Group    16|15 Instance      0|
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
> +                                  ...
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
>  Open discussion
>  ===============
>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> 

Re: [PATCH v3 1/4] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 4780 bytes --]



On Wed, 5 Oct 2022, Ilpo Järvinen wrote:

> On Tue, 4 Oct 2022, matthew.gerlach@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add documentation describing the extensions provided by Version
>> 1 of the Device Feature Header (DFHv1).
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v3: no change
>>
>> v2: s/GUILD/GUID/
>>     add picture
>> ---
>>  Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 15b670926084..7c786b75b498 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
>>  driver's id_table.
>>
>>
>> +Extending the Device Feature Header - DFHv1
>> +===========================================
>> +The current 8 bytes of the Device Feature Header, hereafter referred to as
>> +to DFHv0, provide very little opportunity for the hardware to describe itself
>> +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
>> +to provide increased flexibility and extensibility to hardware designs using
>> +Device Feature Lists.  The list below describes some of the goals behind the
>> +changes in DFHv1:
>> +
>> +* Provide a standardized mechanism for features to describe
>> +  parameters/capabilities to software.
>> +* Standardize the use of a GUID for all DFHv1 types.
>> +* Decouple the location of the DFH from the register space of the feature itself.
>> +
>> +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
>> +a list of parameter values to a particular feature.
>> +
>> +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUID standard
>> +across all types.
>> +
>> +With DFHv0, the register map of a given feature is located immediately following
>> +the DFHv0 in the memory space.  With DFHv1, the location of the feature register
>> +map can be specified as an offset to the DFHv1 or as an absolute address.  The DFHv1
>> +structure is shown below:
>
> I think this is not a good place for be some kind of v1 marketing speak
> (that said, I think it's fine to include those goals you have there).

I understand the need to avoid marketing speak here.  I will update to 
just state the facts.

>
> I'd restructure this so that this section only talks about DFHv1 w/o
> any comparing how v1 is better than v0. Don't base the description on
> how things changed from v0 but just describe v1, that is, like v1 is
> already there, not only being introduced to supercede/extend v0.
>
> And then create v0 section after this section which focuses solely on v0.

I agree that separate sections simply describing v0 and v1 would be 
better.  I may describe v0 first since it shares some fields with v1.

>
> -- 
> i.
>
>> +    +-----------------------------------------------------------------------+
>> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_L                             0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_H                             0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Address/Offset                            1|  Rel  0|
>> +    +-----------------------------------------------------------------------+
>> +    |63 Size of register set  32|Params 31|30 Group    16|15 Instance      0|
>> +    +-----------------------------------------------------------------------+
>> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data                                     0|
>> +    +-----------------------------------------------------------------------+
>> +
>> +                                  ...
>> +
>> +    +-----------------------------------------------------------------------+
>> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data                                     0|
>> +    +-----------------------------------------------------------------------+
>> +
>>  Open discussion
>>  ===============
>>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
>>
>
>

[PATCH v3 2/4] fpga: dfl: Add DFHv1 Register Definitions

[matthew.gerlach]

From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>

This patch adds the definitions for DFHv1 header and related register
bitfields.

Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3:
    keep DFHv1 definitions "hidden" in drivers/fpga/dfl.h

v2: clean up whitespace and one line comments
    remove extra space in commit
    use uniform number of digits in constants
    don't change copyright date because of removed content
---
 drivers/fpga/dfl.h  | 33 ++++++++++++++++++++++++++++++++-
 include/linux/dfl.h | 13 ++++++++++++-
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..bd8720bc5320 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -2,7 +2,7 @@
 /*
  * Driver Header File for FPGA Device Feature List (DFL) Support
  *
- * Copyright (C) 2017-2018 Intel Corporation, Inc.
+ * Copyright (C) 2017-2022 Intel Corporation, Inc.
  *
  * Authors:
  *   Kang Luwei <luwei.kang@intel.com>
@@ -74,11 +74,42 @@
 #define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
 #define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
 #define DFH_EOL			BIT_ULL(40)		/* End of list */
+#define DFH_VERSION		GENMASK_ULL(59, 52)	/* DFH version */
 #define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
 #define DFH_TYPE_AFU		1
 #define DFH_TYPE_PRIVATE	3
 #define DFH_TYPE_FIU		4
 
+/*
+ * DFHv1 Register Offset definitons
+ * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
+ * as common header registers
+ */
+#define DFHv1_CSR_ADDR		0x18  /* CSR Register start address */
+#define DFHv1_CSR_SIZE_GRP	0x20  /* Size of Reg Block and Group/tag */
+#define DFHv1_PARAM_HDR		0x28  /* Optional First Param header */
+
+/*
+ * CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),
+ * 1'b1 = absolute (ARM or other non-PCIe use)
+ */
+#define DFHv1_CSR_ADDR_REL	BIT_ULL(0)
+
+/* CSR Header Register Bit Definitions */
+#define DFHv1_CSR_ADDR_MASK       GENMASK_ULL(63, 1)  /* 63:1 of CSR address */
+
+/* CSR SIZE Goup Register Bit Definitions */
+#define DFHv1_CSR_SIZE_GRP_INSTANCE_ID	GENMASK_ULL(15, 0)	/* Enumeration instantiated IP */
+#define DFHv1_CSR_SIZE_GRP_GROUPING_ID	GENMASK_ULL(30, 16)	/* Group Features/interfaces */
+#define DFHv1_CSR_SIZE_GRP_HAS_PARAMS	BIT_ULL(31)		/* Presence of Parameters */
+#define DFHv1_CSR_SIZE_GRP_SIZE		GENMASK_ULL(63, 32)	/* Size of CSR Block in bytes */
+
+/* PARAM Header Register Bit Definitions */
+#define DFHv1_PARAM_HDR_ID		GENMASK_ULL(15, 0) /* Id of this Param  */
+#define DFHv1_PARAM_HDR_VERSION		GENMASK_ULL(31, 16) /* Version Param */
+#define DFHv1_PARAM_HDR_NEXT_OFFSET	GENMASK_ULL(63, 32) /* Offset of next Param */
+#define DFHv1_PARAM_DATA		0x08  /* Offset of Param data from Param header */
+
 /* Next AFU Register Bitfield */
 #define NEXT_AFU_NEXT_DFH_OFST	GENMASK_ULL(23, 0)	/* Offset to next AFU */
 
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 431636a0dc78..1a1a2b894687 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -2,7 +2,7 @@
 /*
  * Header file for DFL driver and device API
  *
- * Copyright (C) 2020 Intel Corporation, Inc.
+ * Copyright (C) 2020-2022 Intel Corporation, Inc.
  */
 
 #ifndef __LINUX_DFL_H
@@ -11,6 +11,17 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+#define DFHv1_PARAM_ID_MSIX	0x1
+#define DFHv1_PARAM_MSIX_STARTV	0x0
+#define DFHv1_PARAM_MSIX_NUMV	0x4
+
+#define DFHv1_PARAM_ID_CLK_FRQ    0x2
+#define DFHv1_PARAM_ID_FIFO_LEN   0x3
+
+#define DFHv1_PARAM_ID_REG_LAYOUT 0x4
+#define DFHv1_PARAM_ID_REG_WIDTH  GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_ID_REG_SHIFT  GENMASK_ULL(31, 0)
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
-- 
2.25.1

Re: [PATCH v3 2/4] fpga: dfl: Add DFHv1 Register Definitions

[Andy Shevchenko]

On Tue, Oct 04, 2022 at 07:37:16AM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> 
> This patch adds the definitions for DFHv1 header and related register
> bitfields.

...

> - * Copyright (C) 2017-2018 Intel Corporation, Inc.
> + * Copyright (C) 2017-2022 Intel Corporation, Inc.

I do not think this is correct.

What happened to the code in 2019, 2020, and 2021? It's unclear. Have you
consulted with our lawyer about this?

That said, I _think_ (not your lawyer though) that the correct one should be

 * Copyright (C) 2017-2018,2022 Intel Corporation, Inc.

If you wanted to correct that, perhaps it should be done in a separate patch
first with explanation for those years in the gap. Unfortunately I haven't
found any description for those.

Ditto for the rest similar cases.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add generic support for MSIX interrupts for DFL devices.

The location of a feature's registers is explicitly
described in DFHv1 and can be relative to the base of the DFHv1
or an absolute address.  Parse the location and pass the information
to DFL driver.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3: remove unneeded blank line
    use clearer variable name
    pass finfo into parse_feature_irqs()
    refactor code for better indentation
    use switch statement for irq parsing
    squash in code parsing register location

v2: fix kernel doc
    clarify use of DFH_VERSION field
---
 drivers/fpga/dfl.c  | 150 ++++++++++++++++++++++++++++++++------------
 drivers/fpga/dfl.h  |   3 +
 include/linux/dfl.h |  20 ++++++
 3 files changed, 134 insertions(+), 39 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..6a74317e549e 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -380,7 +380,11 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
 	ddev->type = feature_dev_id_type(pdev);
 	ddev->feature_id = feature->id;
 	ddev->revision = feature->revision;
+	ddev->dfh_version = feature->dfh_version;
 	ddev->cdev = pdata->dfl_cdev;
+	ddev->csr_res.start = feature->csr_res.start;
+	ddev->csr_res.end = feature->csr_res.end;
+	ddev->csr_res.flags = IORESOURCE_MEM;
 
 	/* add mmio resource */
 	parent_res = &pdev->resource[feature->resource_index];
@@ -708,18 +712,24 @@ struct build_feature_devs_info {
  * struct dfl_feature_info - sub feature info collected during feature dev build
  *
  * @fid: id of this sub feature.
+ * @revision: revision of this sub feature
+ * @dfh_version: version of Device Feature Header (DFH)
  * @mmio_res: mmio resource of this sub feature.
  * @ioaddr: mapped base address of mmio resource.
  * @node: node in sub_features linked list.
+ * @csr_res: resource of DFHv1 feature registers
+ * @csr_size: DFHv1 size of feature registers
  * @irq_base: start of irq index in this sub feature.
  * @nr_irqs: number of irqs of this sub feature.
  */
 struct dfl_feature_info {
 	u16 fid;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	void __iomem *ioaddr;
 	struct list_head node;
+	struct resource csr_res;
 	unsigned int irq_base;
 	unsigned int nr_irqs;
 };
@@ -797,6 +807,9 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 		feature->dev = fdev;
 		feature->id = finfo->fid;
 		feature->revision = finfo->revision;
+		feature->dfh_version = finfo->dfh_version;
+		feature->csr_res.start = finfo->csr_res.start;
+		feature->csr_res.end = finfo->csr_res.end;
 
 		/*
 		 * the FIU header feature has some fundamental functions (sriov
@@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
 }
 
 static int parse_feature_irqs(struct build_feature_devs_info *binfo,
-			      resource_size_t ofst, u16 fid,
-			      unsigned int *irq_base, unsigned int *nr_irqs)
+			      resource_size_t ofst, struct dfl_feature_info *finfo)
 {
 	void __iomem *base = binfo->ioaddr + ofst;
 	unsigned int i, ibase, inr = 0;
 	enum dfl_id_type type;
-	int virq;
-	u64 v;
-
-	type = feature_dev_id_type(binfo->feature_dev);
+	u16 fid = finfo->fid;
+	u64 v, dfh_ver;
+	int virq, off;
 
 	/*
 	 * Ideally DFL framework should only read info from DFL header, but
-	 * current version DFL only provides mmio resources information for
+	 * current version, DFHv0, only provides mmio resources information for
 	 * each feature in DFL Header, no field for interrupt resources.
 	 * Interrupt resource information is provided by specific mmio
 	 * registers of each private feature which supports interrupt. So in
 	 * order to parse and assign irq resources, DFL framework has to look
 	 * into specific capability registers of these private features.
 	 *
-	 * Once future DFL version supports generic interrupt resource
-	 * information in common DFL headers, the generic interrupt parsing
-	 * code will be added. But in order to be compatible to old version
+	 * DFHv1 supports generic interrupt resource information in DFHv1
+	 * parameter blocks. But in order to be compatible to old version
 	 * DFL, the driver may still fall back to these quirks.
 	 */
-	if (type == PORT_ID) {
-		switch (fid) {
-		case PORT_FEATURE_ID_UINT:
-			v = readq(base + PORT_UINT_CAP);
-			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
-			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+
+	switch (finfo->dfh_version) {
+	case 0:
+		type = feature_dev_id_type(binfo->feature_dev);
+		if (type == PORT_ID) {
+			switch (fid) {
+			case PORT_FEATURE_ID_UINT:
+				v = readq(base + PORT_UINT_CAP);
+				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
+				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+				break;
+			case PORT_FEATURE_ID_ERROR:
+				v = readq(base + PORT_ERROR_CAP);
+				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
+				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+				break;
+			}
+		} else if (type == FME_ID) {
+			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
+				v = readq(base + FME_ERROR_CAP);
+				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
+				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
+			}
+		}
+		break;
+
+	case 1:
+		if (!dfhv1_has_params(base))
 			break;
-		case PORT_FEATURE_ID_ERROR:
-			v = readq(base + PORT_ERROR_CAP);
-			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
-			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+
+		off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
+		if (off < 0)
 			break;
-		}
-	} else if (type == FME_ID) {
-		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
-			v = readq(base + FME_ERROR_CAP);
-			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
-			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
-		}
+
+		ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
+		inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
+		break;
+
+	default:
+		dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);
+		break;
 	}
 
 	if (!inr) {
-		*irq_base = 0;
-		*nr_irqs = 0;
+		finfo->irq_base = 0;
+		finfo->nr_irqs = 0;
 		return 0;
 	}
 
@@ -1006,8 +1038,8 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
 		}
 	}
 
-	*irq_base = ibase;
-	*nr_irqs = inr;
+	finfo->irq_base = ibase;
+	finfo->nr_irqs = inr;
 
 	return 0;
 }
@@ -1023,8 +1055,8 @@ static int
 create_feature_instance(struct build_feature_devs_info *binfo,
 			resource_size_t ofst, resource_size_t size, u16 fid)
 {
-	unsigned int irq_base, nr_irqs;
 	struct dfl_feature_info *finfo;
+	u8 dfh_version = 0;
 	u8 revision = 0;
 	int ret;
 	u64 v;
@@ -1032,7 +1064,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 	if (fid != FEATURE_ID_AFU) {
 		v = readq(binfo->ioaddr + ofst);
 		revision = FIELD_GET(DFH_REVISION, v);
-
+		dfh_version = FIELD_GET(DFH_VERSION, v);
 		/* read feature size and id if inputs are invalid */
 		size = size ? size : feature_size(v);
 		fid = fid ? fid : feature_id(v);
@@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 	if (binfo->len - ofst < size)
 		return -EINVAL;
 
-	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
-	if (ret)
-		return ret;
-
 	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
 	if (!finfo)
 		return -ENOMEM;
 
 	finfo->fid = fid;
 	finfo->revision = revision;
+	finfo->dfh_version = dfh_version;
 	finfo->mmio_res.start = binfo->start + ofst;
 	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
 	finfo->mmio_res.flags = IORESOURCE_MEM;
-	finfo->irq_base = irq_base;
-	finfo->nr_irqs = nr_irqs;
+
+	ret = parse_feature_irqs(binfo, ofst, finfo);
+	if (ret)
+		return ret;
+
+	if (dfh_version == 1) {
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+		if (v & DFHv1_CSR_ADDR_REL)
+			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+		else
+			finfo->csr_res.start = binfo->start + ofst
+					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+		finfo->csr_res.end = finfo->csr_res.start
+				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
+	}
 
 	list_add_tail(&finfo->node, &binfo->sub_features);
 	binfo->feature_num++;
@@ -1879,6 +1923,34 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
 
+int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
+{
+	int off = DFHv1_PARAM_HDR;
+	u64 v, next;
+
+	while (off < max) {
+		v = readq(base + off);
+		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+			return (DFHv1_PARAM_DATA + off);
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		if (!next)
+			break;
+
+		off += next;
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(dfhv1_find_param);
+
+int dfhv1_has_params(void __iomem *dfh_base)
+{
+	return (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
+		readq(dfh_base + DFHv1_CSR_SIZE_GRP)));
+}
+EXPORT_SYMBOL_GPL(dfhv1_has_params);
+
 static void __exit dfl_fpga_exit(void)
 {
 	dfl_chardev_uinit();
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index bd8720bc5320..0423aa8319ed 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -266,6 +266,7 @@ struct dfl_feature_irq_ctx {
  *		    this index is used to find its mmio resource from the
  *		    feature dev (platform device)'s resources.
  * @ioaddr: mapped mmio resource address.
+ * @csr_res: resource for DFHv1 feature registers
  * @irq_ctx: interrupt context list.
  * @nr_irqs: number of interrupt contexts.
  * @ops: ops of this sub feature.
@@ -276,8 +277,10 @@ struct dfl_feature {
 	struct platform_device *dev;
 	u16 id;
 	u8 revision;
+	u8 dfh_version;
 	int resource_index;
 	void __iomem *ioaddr;
+	struct resource csr_res;
 	struct dfl_feature_irq_ctx *irq_ctx;
 	unsigned int nr_irqs;
 	const struct dfl_feature_ops *ops;
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 1a1a2b894687..71760c6a25d7 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -39,6 +39,7 @@ enum dfl_id_type {
  * @type: type of DFL FIU of the device. See enum dfl_id_type.
  * @feature_id: feature identifier local to its DFL FIU type.
  * @mmio_res: mmio resource of this dfl device.
+ * @csr_res: resource for DFHv1 feature registers
  * @irqs: list of Linux IRQ numbers of this dfl device.
  * @num_irqs: number of IRQs supported by this dfl device.
  * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
@@ -50,7 +51,9 @@ struct dfl_device {
 	u16 type;
 	u16 feature_id;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
+	struct resource csr_res;
 	int *irqs;
 	unsigned int num_irqs;
 	struct dfl_fpga_cdev *cdev;
@@ -95,4 +98,21 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
 	module_driver(__dfl_driver, dfl_driver_register, \
 		      dfl_driver_unregister)
 
+/**
+ * dfhv1_find_param() - find the offset of the given parameter
+ * @base: base pointer to start of DFH
+ * @max: maximum offset to search
+ * @param: id of dfl parameter
+ *
+ * Return: positive offset of parameter data on success, negative error code otherwise.
+ */
+int dfhv1_find_param(void __iomem *base, resource_size_t max, int param);
+
+/**
+ * dfhv1_has_params() - does DFHv1 have parameters?
+ * @base: base pointer to start of DFH
+ *
+ * Return: non-zero if DFHv1 has parameters, zero otherwise.
+ */
+int dfhv1_has_params(void __iomem *base);
 #endif /* __LINUX_DFL_H */
-- 
2.25.1

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[Andy Shevchenko]

On Tue, Oct 04, 2022 at 07:37:17AM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add generic support for MSIX interrupts for DFL devices.

$ git grep -n -w MSI[xX] | wc -l
421

$ git grep -n -w MSI-[xX] | wc -l
1224

MSI-X (This is I believe the official name for that)

And everywhere.

> The location of a feature's registers is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address.  Parse the location and pass the information
> to DFL driver.

...

> +	ddev->csr_res.start = feature->csr_res.start;
> +	ddev->csr_res.end = feature->csr_res.end;
> +	ddev->csr_res.flags = IORESOURCE_MEM;

Why simple assignment of the resource can't work?

	ddev->csr_res = feature->csr_res;

(I know the downside of this, but still)

...

> +		feature->csr_res.start = finfo->csr_res.start;
> +		feature->csr_res.end = finfo->csr_res.end;

Ditto.

...

> +	case 0:
> +		type = feature_dev_id_type(binfo->feature_dev);
> +		if (type == PORT_ID) {
> +			switch (fid) {
> +			case PORT_FEATURE_ID_UINT:
> +				v = readq(base + PORT_UINT_CAP);
> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +				break;
> +			case PORT_FEATURE_ID_ERROR:
> +				v = readq(base + PORT_ERROR_CAP);
> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +				break;

No default?

> +			}
> +		} else if (type == FME_ID) {

> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {

Don't remember if that was discussed already or not, but

I would use switch-case here as well in order to be consistent with the
previous code piece pattern.

> +				v = readq(base + FME_ERROR_CAP);
> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> +			}
> +		}
> +		break;

...

> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +		else
> +			finfo->csr_res.start = binfo->start + ofst
> +					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);

Locate + on the previous line.

> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->csr_res.end = finfo->csr_res.start
> +				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;

Ditto.

...

> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
> +{
> +	int off = DFHv1_PARAM_HDR;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = readq(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))

> +			return (DFHv1_PARAM_DATA + off);

Too many parentheses.

> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!next)
> +			break;
> +
> +		off += next;
> +	}
> +
> +	return -ENOENT;
> +}

The entire function seems a bit dangerous to me. You can ask for any max which
covers (up to) 64-bit address space and then do MMIO by basically arbitrary
address. How do you protect against wrong MMIO window here? (This is FPGA, so
anything can be read from HW, i.o.w. it's _untrusted_ source of the data.)

Also, have you tested this with IOMMU enabled? How do they work together (if
there is any collision at all between two?)

...

> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param);

> +int dfhv1_has_params(void __iomem *base);

I would expect to see some struct instead of base which will provide means of
protection against wrong MMIO accesses.

...

Kernel doc usually accompanies the C-code, i.o.w. implementations and not
declarations.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[Xu Yilun]

On 2022-10-04 at 18:11:06 +0300, Andy Shevchenko wrote:
> On Tue, Oct 04, 2022 at 07:37:17AM -0700, matthew.gerlach@linux.intel.com wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > 
> > Add generic support for MSIX interrupts for DFL devices.
> 

...

> 
> > +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
> > +{
> > +	int off = DFHv1_PARAM_HDR;
> > +	u64 v, next;
> > +
> > +	while (off < max) {
> > +		v = readq(base + off);
> > +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> 
> > +			return (DFHv1_PARAM_DATA + off);
> 
> Too many parentheses.
> 
> > +
> > +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> > +		if (!next)
> > +			break;
> > +
> > +		off += next;
> > +	}
> > +
> > +	return -ENOENT;
> > +}
> 
> The entire function seems a bit dangerous to me. You can ask for any max which
> covers (up to) 64-bit address space and then do MMIO by basically arbitrary
> address. How do you protect against wrong MMIO window here? (This is FPGA, so
> anything can be read from HW, i.o.w. it's _untrusted_ source of the data.)
> 
> Also, have you tested this with IOMMU enabled? How do they work together (if
> there is any collision at all between two?)

Yeah, again I don't think this API is good to be used across modules,
even if the parameters got checked. It requires too much details for
other domain developers.

How about:

  dfl_find_param(struct dfl_device *ddev, int param_id)

Thanks,
Yilun

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

On Tue, 4 Oct 2022, Andy Shevchenko wrote:

> On Tue, Oct 04, 2022 at 07:37:17AM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add generic support for MSIX interrupts for DFL devices.
>
> $ git grep -n -w MSI[xX] | wc -l
> 421
>
> $ git grep -n -w MSI-[xX] | wc -l
> 1224
>
> MSI-X (This is I believe the official name for that)

Yes, MSI-X is the official name.  I will update accordingly.

>
> And everywhere.
>
>> The location of a feature's registers is explicitly
>> described in DFHv1 and can be relative to the base of the DFHv1
>> or an absolute address.  Parse the location and pass the information
>> to DFL driver.
>
> ...
>
>> +	ddev->csr_res.start = feature->csr_res.start;
>> +	ddev->csr_res.end = feature->csr_res.end;
>> +	ddev->csr_res.flags = IORESOURCE_MEM;
>
> Why simple assignment of the resource can't work?
>
> 	ddev->csr_res = feature->csr_res;
>
> (I know the downside of this, but still)

A simple structure assignment does look cleaner.

>
> ...
>
>> +		feature->csr_res.start = finfo->csr_res.start;
>> +		feature->csr_res.end = finfo->csr_res.end;
>
> Ditto.
>
> ...
>
>> +	case 0:
>> +		type = feature_dev_id_type(binfo->feature_dev);
>> +		if (type == PORT_ID) {
>> +			switch (fid) {
>> +			case PORT_FEATURE_ID_UINT:
>> +				v = readq(base + PORT_UINT_CAP);
>> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +				break;
>> +			case PORT_FEATURE_ID_ERROR:
>> +				v = readq(base + PORT_ERROR_CAP);
>> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +				break;
>
> No default?

The default is to do nothing.

>
>> +			}
>> +		} else if (type == FME_ID) {
>
>> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>
> Don't remember if that was discussed already or not, but
>
> I would use switch-case here as well in order to be consistent with the
> previous code piece pattern.

Using a switch statement for a single case seems a little strange to me, 
but I'll take a look to see the result seems more consistent.

>
>> +				v = readq(base + FME_ERROR_CAP);
>> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> +			}
>> +		}
>> +		break;
>
> ...
>
>> +		if (v & DFHv1_CSR_ADDR_REL)
>> +			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +		else
>> +			finfo->csr_res.start = binfo->start + ofst
>> +					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>
> Locate + on the previous line.

Got it.
>
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		finfo->csr_res.end = finfo->csr_res.start
>> +				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>
> Ditto.
>
> ...
>
>> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
>> +{
>> +	int off = DFHv1_PARAM_HDR;
>> +	u64 v, next;
>> +
>> +	while (off < max) {
>> +		v = readq(base + off);
>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>
>> +			return (DFHv1_PARAM_DATA + off);
>
> Too many parentheses.

OK, I can remove the parentheses.

>
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!next)
>> +			break;
>> +
>> +		off += next;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>
> The entire function seems a bit dangerous to me. You can ask for any max which
> covers (up to) 64-bit address space and then do MMIO by basically arbitrary
> address. How do you protect against wrong MMIO window here? (This is FPGA, so
> anything can be read from HW, i.o.w. it's _untrusted_ source of the data.)

A broken FPGA image certainly can return anything from a read.  That being 
said I think this is similar to a reg field in a device tree.  The values 
in the reg field can be broken or mistyped.

>
> Also, have you tested this with IOMMU enabled? How do they work together (if
> there is any collision at all between two?)

All my testing has been with IOMMU enabled. The registers may be in the 
same page as the DFHv1 header, but they may not.  For this reason the 
DFHv1 header is ioremapped, parsed, and then unmapped before the actual 
registers are mapped.

  > > ...
>
>> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param);
>
>> +int dfhv1_has_params(void __iomem *base);
>
> I would expect to see some struct instead of base which will provide means of
> protection against wrong MMIO accesses.
>
> ...
>
> Kernel doc usually accompanies the C-code, i.o.w. implementations and not
> declarations.

I will move kernel doc to the function implementations.

>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[Ilpo Järvinen]

Please try to remember cc all people who have commented your patches when 
sending the next version.

On Tue, 4 Oct 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add generic support for MSIX interrupts for DFL devices.
> 
> The location of a feature's registers is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address.  Parse the location and pass the information
> to DFL driver.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

> @@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
>  }
>  
>  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> -			      resource_size_t ofst, u16 fid,
> -			      unsigned int *irq_base, unsigned int *nr_irqs)
> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>  {
>  	void __iomem *base = binfo->ioaddr + ofst;
>  	unsigned int i, ibase, inr = 0;
>  	enum dfl_id_type type;
> -	int virq;
> -	u64 v;
> -
> -	type = feature_dev_id_type(binfo->feature_dev);
> +	u16 fid = finfo->fid;
> +	u64 v, dfh_ver;

Drop dfh_ver.

> +	int virq, off;
>  
>  	/*
>  	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL only provides mmio resources information for
> +	 * current version, DFHv0, only provides mmio resources information for
>  	 * each feature in DFL Header, no field for interrupt resources.
>  	 * Interrupt resource information is provided by specific mmio
>  	 * registers of each private feature which supports interrupt. So in
>  	 * order to parse and assign irq resources, DFL framework has to look
>  	 * into specific capability registers of these private features.
>  	 *
> -	 * Once future DFL version supports generic interrupt resource
> -	 * information in common DFL headers, the generic interrupt parsing
> -	 * code will be added. But in order to be compatible to old version
> +	 * DFHv1 supports generic interrupt resource information in DFHv1
> +	 * parameter blocks. But in order to be compatible to old version
>  	 * DFL, the driver may still fall back to these quirks.

I'm not convinced this comment is useful as is after the introduction of 
v1. It feels too focused on v0 limitations.

I suggest you move v0 limitations description to v0 block below and 
perhaps state in the end of it that comment that v1 is recommended for 
new things because it doesn't have those limitations. Or something along 
those lines.

>  	 */
> -	if (type == PORT_ID) {
> -		switch (fid) {
> -		case PORT_FEATURE_ID_UINT:
> -			v = readq(base + PORT_UINT_CAP);
> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +
> +	switch (finfo->dfh_version) {
> +	case 0:
> +		type = feature_dev_id_type(binfo->feature_dev);
> +		if (type == PORT_ID) {
> +			switch (fid) {
> +			case PORT_FEATURE_ID_UINT:
> +				v = readq(base + PORT_UINT_CAP);
> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +				break;
> +			case PORT_FEATURE_ID_ERROR:
> +				v = readq(base + PORT_ERROR_CAP);
> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +				break;
> +			}
> +		} else if (type == FME_ID) {
> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> +				v = readq(base + FME_ERROR_CAP);
> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> +			}
> +		}
> +		break;
> +
> +	case 1:
> +		if (!dfhv1_has_params(base))
>  			break;
> -		case PORT_FEATURE_ID_ERROR:
> -			v = readq(base + PORT_ERROR_CAP);
> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +
> +		off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
> +		if (off < 0)
>  			break;
> -		}
> -	} else if (type == FME_ID) {
> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> -			v = readq(base + FME_ERROR_CAP);
> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> -		}
> +
> +		ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
> +		inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
> +		break;
> +
> +	default:
> +		dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);

dfh_ver is uninitialized here. The compiler shouldn't have been happy with 
this.

> @@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  	if (binfo->len - ofst < size)
>  		return -EINVAL;
>  
> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> -	if (ret)
> -		return ret;
> -
>  	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>  	if (!finfo)
>  		return -ENOMEM;
>  
>  	finfo->fid = fid;
>  	finfo->revision = revision;
> +	finfo->dfh_version = dfh_version;
>  	finfo->mmio_res.start = binfo->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
> -	finfo->irq_base = irq_base;
> -	finfo->nr_irqs = nr_irqs;
> +
> +	ret = parse_feature_irqs(binfo, ofst, finfo);
> +	if (ret)
> +		return ret;

finfo has to be freed in case of an error.

Thanks for rearranging, it looks more logical now.

--
 i.

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 5752 bytes --]



On Wed, 5 Oct 2022, Ilpo Järvinen wrote:

> Please try to remember cc all people who have commented your patches when
> sending the next version.
>
> On Tue, 4 Oct 2022, matthew.gerlach@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add generic support for MSIX interrupts for DFL devices.
>>
>> The location of a feature's registers is explicitly
>> described in DFHv1 and can be relative to the base of the DFHv1
>> or an absolute address.  Parse the location and pass the information
>> to DFL driver.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
>> @@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
>>  }
>>
>>  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> -			      resource_size_t ofst, u16 fid,
>> -			      unsigned int *irq_base, unsigned int *nr_irqs)
>> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>>  {
>>  	void __iomem *base = binfo->ioaddr + ofst;
>>  	unsigned int i, ibase, inr = 0;
>>  	enum dfl_id_type type;
>> -	int virq;
>> -	u64 v;
>> -
>> -	type = feature_dev_id_type(binfo->feature_dev);
>> +	u16 fid = finfo->fid;
>> +	u64 v, dfh_ver;
>
> Drop dfh_ver.

I will drop dfh_ver.

>
>> +	int virq, off;
>>
>>  	/*
>>  	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL only provides mmio resources information for
>> +	 * current version, DFHv0, only provides mmio resources information for
>>  	 * each feature in DFL Header, no field for interrupt resources.
>>  	 * Interrupt resource information is provided by specific mmio
>>  	 * registers of each private feature which supports interrupt. So in
>>  	 * order to parse and assign irq resources, DFL framework has to look
>>  	 * into specific capability registers of these private features.
>>  	 *
>> -	 * Once future DFL version supports generic interrupt resource
>> -	 * information in common DFL headers, the generic interrupt parsing
>> -	 * code will be added. But in order to be compatible to old version
>> +	 * DFHv1 supports generic interrupt resource information in DFHv1
>> +	 * parameter blocks. But in order to be compatible to old version
>>  	 * DFL, the driver may still fall back to these quirks.
>
> I'm not convinced this comment is useful as is after the introduction of
> v1. It feels too focused on v0 limitations.
>
> I suggest you move v0 limitations description to v0 block below and
> perhaps state in the end of it that comment that v1 is recommended for
> new things because it doesn't have those limitations. Or something along
> those lines.

I think I will rework the comment by splitting the descriptions for v0 
and v1 and focusing on what each supports rather than limitations.

>
>>  	 */
>> -	if (type == PORT_ID) {
>> -		switch (fid) {
>> -		case PORT_FEATURE_ID_UINT:
>> -			v = readq(base + PORT_UINT_CAP);
>> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +
>> +	switch (finfo->dfh_version) {
>> +	case 0:
>> +		type = feature_dev_id_type(binfo->feature_dev);
>> +		if (type == PORT_ID) {
>> +			switch (fid) {
>> +			case PORT_FEATURE_ID_UINT:
>> +				v = readq(base + PORT_UINT_CAP);
>> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +				break;
>> +			case PORT_FEATURE_ID_ERROR:
>> +				v = readq(base + PORT_ERROR_CAP);
>> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		} else if (type == FME_ID) {
>> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> +				v = readq(base + FME_ERROR_CAP);
>> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> +			}
>> +		}
>> +		break;
>> +
>> +	case 1:
>> +		if (!dfhv1_has_params(base))
>>  			break;
>> -		case PORT_FEATURE_ID_ERROR:
>> -			v = readq(base + PORT_ERROR_CAP);
>> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +
>> +		off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
>> +		if (off < 0)
>>  			break;
>> -		}
>> -	} else if (type == FME_ID) {
>> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> -			v = readq(base + FME_ERROR_CAP);
>> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> -		}
>> +
>> +		ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
>> +		inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
>> +		break;
>> +
>> +	default:
>> +		dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);
>
> dfh_ver is uninitialized here. The compiler shouldn't have been happy with
> this.

I am surprised the compiler did not flag this uninitialized variable. 
Getting rid of the dfh_ver altogether is the best course of action.

>
>> @@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>>  	if (binfo->len - ofst < size)
>>  		return -EINVAL;
>>
>> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>> -	if (ret)
>> -		return ret;
>> -
>>  	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>>  	if (!finfo)
>>  		return -ENOMEM;
>>
>>  	finfo->fid = fid;
>>  	finfo->revision = revision;
>> +	finfo->dfh_version = dfh_version;
>>  	finfo->mmio_res.start = binfo->start + ofst;
>>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>> -	finfo->irq_base = irq_base;
>> -	finfo->nr_irqs = nr_irqs;
>> +
>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>> +	if (ret)
>> +		return ret;
>
> finfo has to be freed in case of an error.

Good catch.  Thanks.


>
> Thanks for rearranging, it looks more logical now.
>
> --
> i.
>

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[Xu Yilun]

On 2022-10-04 at 07:37:17 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add generic support for MSIX interrupts for DFL devices.
> 
> The location of a feature's registers is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address.  Parse the location and pass the information
> to DFL driver.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v3: remove unneeded blank line
>     use clearer variable name
>     pass finfo into parse_feature_irqs()
>     refactor code for better indentation
>     use switch statement for irq parsing
>     squash in code parsing register location
> 
> v2: fix kernel doc
>     clarify use of DFH_VERSION field
> ---
>  drivers/fpga/dfl.c  | 150 ++++++++++++++++++++++++++++++++------------
>  drivers/fpga/dfl.h  |   3 +
>  include/linux/dfl.h |  20 ++++++
>  3 files changed, 134 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..6a74317e549e 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -380,7 +380,11 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>  	ddev->type = feature_dev_id_type(pdev);
>  	ddev->feature_id = feature->id;
>  	ddev->revision = feature->revision;
> +	ddev->dfh_version = feature->dfh_version;
>  	ddev->cdev = pdata->dfl_cdev;
> +	ddev->csr_res.start = feature->csr_res.start;
> +	ddev->csr_res.end = feature->csr_res.end;
> +	ddev->csr_res.flags = IORESOURCE_MEM;
>  
>  	/* add mmio resource */
>  	parent_res = &pdev->resource[feature->resource_index];
> @@ -708,18 +712,24 @@ struct build_feature_devs_info {
>   * struct dfl_feature_info - sub feature info collected during feature dev build
>   *
>   * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
>   * @mmio_res: mmio resource of this sub feature.
>   * @ioaddr: mapped base address of mmio resource.
>   * @node: node in sub_features linked list.
> + * @csr_res: resource of DFHv1 feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irq_base: start of irq index in this sub feature.
>   * @nr_irqs: number of irqs of this sub feature.
>   */
>  struct dfl_feature_info {
>  	u16 fid;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	void __iomem *ioaddr;
>  	struct list_head node;
> +	struct resource csr_res;
>  	unsigned int irq_base;
>  	unsigned int nr_irqs;
>  };
> @@ -797,6 +807,9 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->revision = finfo->revision;
> +		feature->dfh_version = finfo->dfh_version;
> +		feature->csr_res.start = finfo->csr_res.start;
> +		feature->csr_res.end = finfo->csr_res.end;
>  
>  		/*
>  		 * the FIU header feature has some fundamental functions (sriov
> @@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
>  }
>  
>  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> -			      resource_size_t ofst, u16 fid,
> -			      unsigned int *irq_base, unsigned int *nr_irqs)
> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>  {
>  	void __iomem *base = binfo->ioaddr + ofst;
>  	unsigned int i, ibase, inr = 0;
>  	enum dfl_id_type type;
> -	int virq;
> -	u64 v;
> -
> -	type = feature_dev_id_type(binfo->feature_dev);
> +	u16 fid = finfo->fid;
> +	u64 v, dfh_ver;
> +	int virq, off;
>  
>  	/*
>  	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL only provides mmio resources information for
> +	 * current version, DFHv0, only provides mmio resources information for
>  	 * each feature in DFL Header, no field for interrupt resources.
>  	 * Interrupt resource information is provided by specific mmio
>  	 * registers of each private feature which supports interrupt. So in
>  	 * order to parse and assign irq resources, DFL framework has to look
>  	 * into specific capability registers of these private features.
>  	 *
> -	 * Once future DFL version supports generic interrupt resource
> -	 * information in common DFL headers, the generic interrupt parsing
> -	 * code will be added. But in order to be compatible to old version
> +	 * DFHv1 supports generic interrupt resource information in DFHv1
> +	 * parameter blocks. But in order to be compatible to old version
>  	 * DFL, the driver may still fall back to these quirks.
>  	 */
> -	if (type == PORT_ID) {
> -		switch (fid) {
> -		case PORT_FEATURE_ID_UINT:
> -			v = readq(base + PORT_UINT_CAP);
> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +
> +	switch (finfo->dfh_version) {
> +	case 0:
> +		type = feature_dev_id_type(binfo->feature_dev);
> +		if (type == PORT_ID) {
> +			switch (fid) {
> +			case PORT_FEATURE_ID_UINT:
> +				v = readq(base + PORT_UINT_CAP);
> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +				break;
> +			case PORT_FEATURE_ID_ERROR:
> +				v = readq(base + PORT_ERROR_CAP);
> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +				break;
> +			}
> +		} else if (type == FME_ID) {
> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> +				v = readq(base + FME_ERROR_CAP);
> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> +			}
> +		}
> +		break;
> +
> +	case 1:
> +		if (!dfhv1_has_params(base))
>  			break;
> -		case PORT_FEATURE_ID_ERROR:
> -			v = readq(base + PORT_ERROR_CAP);
> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +
> +		off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
> +		if (off < 0)
>  			break;
> -		}
> -	} else if (type == FME_ID) {
> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> -			v = readq(base + FME_ERROR_CAP);
> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> -		}
> +
> +		ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
> +		inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
> +		break;
> +
> +	default:
> +		dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);
> +		break;
>  	}
>  
>  	if (!inr) {
> -		*irq_base = 0;
> -		*nr_irqs = 0;
> +		finfo->irq_base = 0;
> +		finfo->nr_irqs = 0;
>  		return 0;
>  	}
>  
> @@ -1006,8 +1038,8 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  		}
>  	}
>  
> -	*irq_base = ibase;
> -	*nr_irqs = inr;
> +	finfo->irq_base = ibase;
> +	finfo->nr_irqs = inr;
>  
>  	return 0;
>  }
> @@ -1023,8 +1055,8 @@ static int
>  create_feature_instance(struct build_feature_devs_info *binfo,
>  			resource_size_t ofst, resource_size_t size, u16 fid)
>  {
> -	unsigned int irq_base, nr_irqs;
>  	struct dfl_feature_info *finfo;
> +	u8 dfh_version = 0;
>  	u8 revision = 0;
>  	int ret;
>  	u64 v;
> @@ -1032,7 +1064,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  	if (fid != FEATURE_ID_AFU) {
>  		v = readq(binfo->ioaddr + ofst);
>  		revision = FIELD_GET(DFH_REVISION, v);
> -
> +		dfh_version = FIELD_GET(DFH_VERSION, v);
>  		/* read feature size and id if inputs are invalid */
>  		size = size ? size : feature_size(v);
>  		fid = fid ? fid : feature_id(v);
> @@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  	if (binfo->len - ofst < size)
>  		return -EINVAL;
>  
> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> -	if (ret)
> -		return ret;
> -
>  	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>  	if (!finfo)
>  		return -ENOMEM;
>  
>  	finfo->fid = fid;
>  	finfo->revision = revision;
> +	finfo->dfh_version = dfh_version;
>  	finfo->mmio_res.start = binfo->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
> -	finfo->irq_base = irq_base;
> -	finfo->nr_irqs = nr_irqs;
> +
> +	ret = parse_feature_irqs(binfo, ofst, finfo);
> +	if (ret)
> +		return ret;
> +
> +	if (dfh_version == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +		else
> +			finfo->csr_res.start = binfo->start + ofst
> +					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->csr_res.end = finfo->csr_res.start
> +				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> +	}
>  
>  	list_add_tail(&finfo->node, &binfo->sub_features);
>  	binfo->feature_num++;
> @@ -1879,6 +1923,34 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>  
> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
> +{
> +	int off = DFHv1_PARAM_HDR;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = readq(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return (DFHv1_PARAM_DATA + off);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!next)
> +			break;
> +
> +		off += next;
> +	}
> +
> +	return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(dfhv1_find_param);
> +
> +int dfhv1_has_params(void __iomem *dfh_base)
> +{
> +	return (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		readq(dfh_base + DFHv1_CSR_SIZE_GRP)));
> +}
> +EXPORT_SYMBOL_GPL(dfhv1_has_params);
> +
>  static void __exit dfl_fpga_exit(void)
>  {
>  	dfl_chardev_uinit();
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index bd8720bc5320..0423aa8319ed 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -266,6 +266,7 @@ struct dfl_feature_irq_ctx {
>   *		    this index is used to find its mmio resource from the
>   *		    feature dev (platform device)'s resources.
>   * @ioaddr: mapped mmio resource address.
> + * @csr_res: resource for DFHv1 feature registers
>   * @irq_ctx: interrupt context list.
>   * @nr_irqs: number of interrupt contexts.
>   * @ops: ops of this sub feature.
> @@ -276,8 +277,10 @@ struct dfl_feature {
>  	struct platform_device *dev;
>  	u16 id;
>  	u8 revision;
> +	u8 dfh_version;
>  	int resource_index;
>  	void __iomem *ioaddr;
> +	struct resource csr_res;
>  	struct dfl_feature_irq_ctx *irq_ctx;
>  	unsigned int nr_irqs;
>  	const struct dfl_feature_ops *ops;
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 1a1a2b894687..71760c6a25d7 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -39,6 +39,7 @@ enum dfl_id_type {
>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
>   * @feature_id: feature identifier local to its DFL FIU type.
>   * @mmio_res: mmio resource of this dfl device.
> + * @csr_res: resource for DFHv1 feature registers

I think the combination of mmio_res & csr_res is confusing. Why a
special csr_res dedicated for DFHv1, and what the mmio_res stands for if
the csr_res exists? And they may overlap each other.

Could you present some general purpose mmio resource layout which is
compatible to dfl v0 & v1? People from other domains just need to know
the basic concept like how many register blocks in the device, what are
their ranges ...

Thanks,
Yilun

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

On Thu, 6 Oct 2022, Xu Yilun wrote:

> On 2022-10-04 at 07:37:17 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add generic support for MSIX interrupts for DFL devices.
>>
>> The location of a feature's registers is explicitly
>> described in DFHv1 and can be relative to the base of the DFHv1
>> or an absolute address.  Parse the location and pass the information
>> to DFL driver.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v3: remove unneeded blank line
>>     use clearer variable name
>>     pass finfo into parse_feature_irqs()
>>     refactor code for better indentation
>>     use switch statement for irq parsing
>>     squash in code parsing register location
>>
>> v2: fix kernel doc
>>     clarify use of DFH_VERSION field
>> ---
>>  drivers/fpga/dfl.c  | 150 ++++++++++++++++++++++++++++++++------------
>>  drivers/fpga/dfl.h  |   3 +
>>  include/linux/dfl.h |  20 ++++++
>>  3 files changed, 134 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index b9aae85ba930..6a74317e549e 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -380,7 +380,11 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>>  	ddev->type = feature_dev_id_type(pdev);
>>  	ddev->feature_id = feature->id;
>>  	ddev->revision = feature->revision;
>> +	ddev->dfh_version = feature->dfh_version;
>>  	ddev->cdev = pdata->dfl_cdev;
>> +	ddev->csr_res.start = feature->csr_res.start;
>> +	ddev->csr_res.end = feature->csr_res.end;
>> +	ddev->csr_res.flags = IORESOURCE_MEM;
>>
>>  	/* add mmio resource */
>>  	parent_res = &pdev->resource[feature->resource_index];
>> @@ -708,18 +712,24 @@ struct build_feature_devs_info {
>>   * struct dfl_feature_info - sub feature info collected during feature dev build
>>   *
>>   * @fid: id of this sub feature.
>> + * @revision: revision of this sub feature
>> + * @dfh_version: version of Device Feature Header (DFH)
>>   * @mmio_res: mmio resource of this sub feature.
>>   * @ioaddr: mapped base address of mmio resource.
>>   * @node: node in sub_features linked list.
>> + * @csr_res: resource of DFHv1 feature registers
>> + * @csr_size: DFHv1 size of feature registers
>>   * @irq_base: start of irq index in this sub feature.
>>   * @nr_irqs: number of irqs of this sub feature.
>>   */
>>  struct dfl_feature_info {
>>  	u16 fid;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	struct resource mmio_res;
>>  	void __iomem *ioaddr;
>>  	struct list_head node;
>> +	struct resource csr_res;
>>  	unsigned int irq_base;
>>  	unsigned int nr_irqs;
>>  };
>> @@ -797,6 +807,9 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>>  		feature->dev = fdev;
>>  		feature->id = finfo->fid;
>>  		feature->revision = finfo->revision;
>> +		feature->dfh_version = finfo->dfh_version;
>> +		feature->csr_res.start = finfo->csr_res.start;
>> +		feature->csr_res.end = finfo->csr_res.end;
>>
>>  		/*
>>  		 * the FIU header feature has some fundamental functions (sriov
>> @@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
>>  }
>>
>>  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> -			      resource_size_t ofst, u16 fid,
>> -			      unsigned int *irq_base, unsigned int *nr_irqs)
>> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>>  {
>>  	void __iomem *base = binfo->ioaddr + ofst;
>>  	unsigned int i, ibase, inr = 0;
>>  	enum dfl_id_type type;
>> -	int virq;
>> -	u64 v;
>> -
>> -	type = feature_dev_id_type(binfo->feature_dev);
>> +	u16 fid = finfo->fid;
>> +	u64 v, dfh_ver;
>> +	int virq, off;
>>
>>  	/*
>>  	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL only provides mmio resources information for
>> +	 * current version, DFHv0, only provides mmio resources information for
>>  	 * each feature in DFL Header, no field for interrupt resources.
>>  	 * Interrupt resource information is provided by specific mmio
>>  	 * registers of each private feature which supports interrupt. So in
>>  	 * order to parse and assign irq resources, DFL framework has to look
>>  	 * into specific capability registers of these private features.
>>  	 *
>> -	 * Once future DFL version supports generic interrupt resource
>> -	 * information in common DFL headers, the generic interrupt parsing
>> -	 * code will be added. But in order to be compatible to old version
>> +	 * DFHv1 supports generic interrupt resource information in DFHv1
>> +	 * parameter blocks. But in order to be compatible to old version
>>  	 * DFL, the driver may still fall back to these quirks.
>>  	 */
>> -	if (type == PORT_ID) {
>> -		switch (fid) {
>> -		case PORT_FEATURE_ID_UINT:
>> -			v = readq(base + PORT_UINT_CAP);
>> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +
>> +	switch (finfo->dfh_version) {
>> +	case 0:
>> +		type = feature_dev_id_type(binfo->feature_dev);
>> +		if (type == PORT_ID) {
>> +			switch (fid) {
>> +			case PORT_FEATURE_ID_UINT:
>> +				v = readq(base + PORT_UINT_CAP);
>> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +				break;
>> +			case PORT_FEATURE_ID_ERROR:
>> +				v = readq(base + PORT_ERROR_CAP);
>> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		} else if (type == FME_ID) {
>> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> +				v = readq(base + FME_ERROR_CAP);
>> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> +			}
>> +		}
>> +		break;
>> +
>> +	case 1:
>> +		if (!dfhv1_has_params(base))
>>  			break;
>> -		case PORT_FEATURE_ID_ERROR:
>> -			v = readq(base + PORT_ERROR_CAP);
>> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +
>> +		off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
>> +		if (off < 0)
>>  			break;
>> -		}
>> -	} else if (type == FME_ID) {
>> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> -			v = readq(base + FME_ERROR_CAP);
>> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> -		}
>> +
>> +		ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
>> +		inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
>> +		break;
>> +
>> +	default:
>> +		dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);
>> +		break;
>>  	}
>>
>>  	if (!inr) {
>> -		*irq_base = 0;
>> -		*nr_irqs = 0;
>> +		finfo->irq_base = 0;
>> +		finfo->nr_irqs = 0;
>>  		return 0;
>>  	}
>>
>> @@ -1006,8 +1038,8 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>  		}
>>  	}
>>
>> -	*irq_base = ibase;
>> -	*nr_irqs = inr;
>> +	finfo->irq_base = ibase;
>> +	finfo->nr_irqs = inr;
>>
>>  	return 0;
>>  }
>> @@ -1023,8 +1055,8 @@ static int
>>  create_feature_instance(struct build_feature_devs_info *binfo,
>>  			resource_size_t ofst, resource_size_t size, u16 fid)
>>  {
>> -	unsigned int irq_base, nr_irqs;
>>  	struct dfl_feature_info *finfo;
>> +	u8 dfh_version = 0;
>>  	u8 revision = 0;
>>  	int ret;
>>  	u64 v;
>> @@ -1032,7 +1064,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>>  	if (fid != FEATURE_ID_AFU) {
>>  		v = readq(binfo->ioaddr + ofst);
>>  		revision = FIELD_GET(DFH_REVISION, v);
>> -
>> +		dfh_version = FIELD_GET(DFH_VERSION, v);
>>  		/* read feature size and id if inputs are invalid */
>>  		size = size ? size : feature_size(v);
>>  		fid = fid ? fid : feature_id(v);
>> @@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>>  	if (binfo->len - ofst < size)
>>  		return -EINVAL;
>>
>> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>> -	if (ret)
>> -		return ret;
>> -
>>  	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>>  	if (!finfo)
>>  		return -ENOMEM;
>>
>>  	finfo->fid = fid;
>>  	finfo->revision = revision;
>> +	finfo->dfh_version = dfh_version;
>>  	finfo->mmio_res.start = binfo->start + ofst;
>>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>> -	finfo->irq_base = irq_base;
>> -	finfo->nr_irqs = nr_irqs;
>> +
>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (dfh_version == 1) {
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> +		if (v & DFHv1_CSR_ADDR_REL)
>> +			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +		else
>> +			finfo->csr_res.start = binfo->start + ofst
>> +					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		finfo->csr_res.end = finfo->csr_res.start
>> +				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>> +	}
>>
>>  	list_add_tail(&finfo->node, &binfo->sub_features);
>>  	binfo->feature_num++;
>> @@ -1879,6 +1923,34 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>>  }
>>  EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>>
>> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
>> +{
>> +	int off = DFHv1_PARAM_HDR;
>> +	u64 v, next;
>> +
>> +	while (off < max) {
>> +		v = readq(base + off);
>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return (DFHv1_PARAM_DATA + off);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!next)
>> +			break;
>> +
>> +		off += next;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +EXPORT_SYMBOL_GPL(dfhv1_find_param);
>> +
>> +int dfhv1_has_params(void __iomem *dfh_base)
>> +{
>> +	return (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		readq(dfh_base + DFHv1_CSR_SIZE_GRP)));
>> +}
>> +EXPORT_SYMBOL_GPL(dfhv1_has_params);
>> +
>>  static void __exit dfl_fpga_exit(void)
>>  {
>>  	dfl_chardev_uinit();
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index bd8720bc5320..0423aa8319ed 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -266,6 +266,7 @@ struct dfl_feature_irq_ctx {
>>   *		    this index is used to find its mmio resource from the
>>   *		    feature dev (platform device)'s resources.
>>   * @ioaddr: mapped mmio resource address.
>> + * @csr_res: resource for DFHv1 feature registers
>>   * @irq_ctx: interrupt context list.
>>   * @nr_irqs: number of interrupt contexts.
>>   * @ops: ops of this sub feature.
>> @@ -276,8 +277,10 @@ struct dfl_feature {
>>  	struct platform_device *dev;
>>  	u16 id;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	int resource_index;
>>  	void __iomem *ioaddr;
>> +	struct resource csr_res;
>>  	struct dfl_feature_irq_ctx *irq_ctx;
>>  	unsigned int nr_irqs;
>>  	const struct dfl_feature_ops *ops;
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 1a1a2b894687..71760c6a25d7 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -39,6 +39,7 @@ enum dfl_id_type {
>>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
>>   * @feature_id: feature identifier local to its DFL FIU type.
>>   * @mmio_res: mmio resource of this dfl device.
>> + * @csr_res: resource for DFHv1 feature registers
>
> I think the combination of mmio_res & csr_res is confusing. Why a
> special csr_res dedicated for DFHv1, and what the mmio_res stands for if
> the csr_res exists? And they may overlap each other.

With DFHv1, the registers of a feature may be in a different location than 
the location of the Device Feature Header.  So a resource is needed for 
the DFH and another resource for the actual register space.  Would 
changing the name of mmio_res to dfh_res be less confusing? 
Unfortunately, changing the name of mmio_res would impact the existing dfl 
drivers.

The two resources may overlap, but that is why in 
drivers/tty/serial/8250/8250_dfl.c the mmio_res is mapped, then DFH 
parsed, and unmapped before the csr_res is mapped in the call to 
serial8250_register_8250_port().

>
> Could you present some general purpose mmio resource layout which is
> compatible to dfl v0 & v1? People from other domains just need to know
> the basic concept like how many register blocks in the device, what are
> their ranges ...

I don't know how a single resource object can be used in the case of DFHv1 
when the registers are in a different location than the DFH.

>
> Thanks,
> Yilun
>

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[Xu Yilun]

On 2022-10-10 at 09:58:00 -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Thu, 6 Oct 2022, Xu Yilun wrote:
> 
> > On 2022-10-04 at 07:37:17 -0700, matthew.gerlach@linux.intel.com wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > Add generic support for MSIX interrupts for DFL devices.
> > > 
> > > The location of a feature's registers is explicitly
> > > described in DFHv1 and can be relative to the base of the DFHv1
> > > or an absolute address.  Parse the location and pass the information
> > > to DFL driver.
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > > v3: remove unneeded blank line
> > >     use clearer variable name
> > >     pass finfo into parse_feature_irqs()
> > >     refactor code for better indentation
> > >     use switch statement for irq parsing
> > >     squash in code parsing register location
> > > 
> > > v2: fix kernel doc
> > >     clarify use of DFH_VERSION field
> > > ---
> > >  drivers/fpga/dfl.c  | 150 ++++++++++++++++++++++++++++++++------------
> > >  drivers/fpga/dfl.h  |   3 +
> > >  include/linux/dfl.h |  20 ++++++
> > >  3 files changed, 134 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index b9aae85ba930..6a74317e549e 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -380,7 +380,11 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
> > >  	ddev->type = feature_dev_id_type(pdev);
> > >  	ddev->feature_id = feature->id;
> > >  	ddev->revision = feature->revision;
> > > +	ddev->dfh_version = feature->dfh_version;
> > >  	ddev->cdev = pdata->dfl_cdev;
> > > +	ddev->csr_res.start = feature->csr_res.start;
> > > +	ddev->csr_res.end = feature->csr_res.end;
> > > +	ddev->csr_res.flags = IORESOURCE_MEM;
> > > 
> > >  	/* add mmio resource */
> > >  	parent_res = &pdev->resource[feature->resource_index];
> > > @@ -708,18 +712,24 @@ struct build_feature_devs_info {
> > >   * struct dfl_feature_info - sub feature info collected during feature dev build
> > >   *
> > >   * @fid: id of this sub feature.
> > > + * @revision: revision of this sub feature
> > > + * @dfh_version: version of Device Feature Header (DFH)
> > >   * @mmio_res: mmio resource of this sub feature.
> > >   * @ioaddr: mapped base address of mmio resource.
> > >   * @node: node in sub_features linked list.
> > > + * @csr_res: resource of DFHv1 feature registers
> > > + * @csr_size: DFHv1 size of feature registers
> > >   * @irq_base: start of irq index in this sub feature.
> > >   * @nr_irqs: number of irqs of this sub feature.
> > >   */
> > >  struct dfl_feature_info {
> > >  	u16 fid;
> > >  	u8 revision;
> > > +	u8 dfh_version;
> > >  	struct resource mmio_res;
> > >  	void __iomem *ioaddr;
> > >  	struct list_head node;
> > > +	struct resource csr_res;
> > >  	unsigned int irq_base;
> > >  	unsigned int nr_irqs;
> > >  };
> > > @@ -797,6 +807,9 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > >  		feature->dev = fdev;
> > >  		feature->id = finfo->fid;
> > >  		feature->revision = finfo->revision;
> > > +		feature->dfh_version = finfo->dfh_version;
> > > +		feature->csr_res.start = finfo->csr_res.start;
> > > +		feature->csr_res.end = finfo->csr_res.end;
> > > 
> > >  		/*
> > >  		 * the FIU header feature has some fundamental functions (sriov
> > > @@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
> > >  }
> > > 
> > >  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> > > -			      resource_size_t ofst, u16 fid,
> > > -			      unsigned int *irq_base, unsigned int *nr_irqs)
> > > +			      resource_size_t ofst, struct dfl_feature_info *finfo)
> > >  {
> > >  	void __iomem *base = binfo->ioaddr + ofst;
> > >  	unsigned int i, ibase, inr = 0;
> > >  	enum dfl_id_type type;
> > > -	int virq;
> > > -	u64 v;
> > > -
> > > -	type = feature_dev_id_type(binfo->feature_dev);
> > > +	u16 fid = finfo->fid;
> > > +	u64 v, dfh_ver;
> > > +	int virq, off;
> > > 
> > >  	/*
> > >  	 * Ideally DFL framework should only read info from DFL header, but
> > > -	 * current version DFL only provides mmio resources information for
> > > +	 * current version, DFHv0, only provides mmio resources information for
> > >  	 * each feature in DFL Header, no field for interrupt resources.
> > >  	 * Interrupt resource information is provided by specific mmio
> > >  	 * registers of each private feature which supports interrupt. So in
> > >  	 * order to parse and assign irq resources, DFL framework has to look
> > >  	 * into specific capability registers of these private features.
> > >  	 *
> > > -	 * Once future DFL version supports generic interrupt resource
> > > -	 * information in common DFL headers, the generic interrupt parsing
> > > -	 * code will be added. But in order to be compatible to old version
> > > +	 * DFHv1 supports generic interrupt resource information in DFHv1
> > > +	 * parameter blocks. But in order to be compatible to old version
> > >  	 * DFL, the driver may still fall back to these quirks.
> > >  	 */
> > > -	if (type == PORT_ID) {
> > > -		switch (fid) {
> > > -		case PORT_FEATURE_ID_UINT:
> > > -			v = readq(base + PORT_UINT_CAP);
> > > -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > > -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > > +
> > > +	switch (finfo->dfh_version) {
> > > +	case 0:
> > > +		type = feature_dev_id_type(binfo->feature_dev);
> > > +		if (type == PORT_ID) {
> > > +			switch (fid) {
> > > +			case PORT_FEATURE_ID_UINT:
> > > +				v = readq(base + PORT_UINT_CAP);
> > > +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > > +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > > +				break;
> > > +			case PORT_FEATURE_ID_ERROR:
> > > +				v = readq(base + PORT_ERROR_CAP);
> > > +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > > +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > > +				break;
> > > +			}
> > > +		} else if (type == FME_ID) {
> > > +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> > > +				v = readq(base + FME_ERROR_CAP);
> > > +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > > +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> > > +			}
> > > +		}
> > > +		break;
> > > +
> > > +	case 1:
> > > +		if (!dfhv1_has_params(base))
> > >  			break;
> > > -		case PORT_FEATURE_ID_ERROR:
> > > -			v = readq(base + PORT_ERROR_CAP);
> > > -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > > -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > > +
> > > +		off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
> > > +		if (off < 0)
> > >  			break;
> > > -		}
> > > -	} else if (type == FME_ID) {
> > > -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> > > -			v = readq(base + FME_ERROR_CAP);
> > > -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > > -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> > > -		}
> > > +
> > > +		ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
> > > +		inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
> > > +		break;
> > > +
> > > +	default:
> > > +		dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);
> > > +		break;
> > >  	}
> > > 
> > >  	if (!inr) {
> > > -		*irq_base = 0;
> > > -		*nr_irqs = 0;
> > > +		finfo->irq_base = 0;
> > > +		finfo->nr_irqs = 0;
> > >  		return 0;
> > >  	}
> > > 
> > > @@ -1006,8 +1038,8 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> > >  		}
> > >  	}
> > > 
> > > -	*irq_base = ibase;
> > > -	*nr_irqs = inr;
> > > +	finfo->irq_base = ibase;
> > > +	finfo->nr_irqs = inr;
> > > 
> > >  	return 0;
> > >  }
> > > @@ -1023,8 +1055,8 @@ static int
> > >  create_feature_instance(struct build_feature_devs_info *binfo,
> > >  			resource_size_t ofst, resource_size_t size, u16 fid)
> > >  {
> > > -	unsigned int irq_base, nr_irqs;
> > >  	struct dfl_feature_info *finfo;
> > > +	u8 dfh_version = 0;
> > >  	u8 revision = 0;
> > >  	int ret;
> > >  	u64 v;
> > > @@ -1032,7 +1064,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> > >  	if (fid != FEATURE_ID_AFU) {
> > >  		v = readq(binfo->ioaddr + ofst);
> > >  		revision = FIELD_GET(DFH_REVISION, v);
> > > -
> > > +		dfh_version = FIELD_GET(DFH_VERSION, v);
> > >  		/* read feature size and id if inputs are invalid */
> > >  		size = size ? size : feature_size(v);
> > >  		fid = fid ? fid : feature_id(v);
> > > @@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> > >  	if (binfo->len - ofst < size)
> > >  		return -EINVAL;
> > > 
> > > -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
> > >  	if (!finfo)
> > >  		return -ENOMEM;
> > > 
> > >  	finfo->fid = fid;
> > >  	finfo->revision = revision;
> > > +	finfo->dfh_version = dfh_version;
> > >  	finfo->mmio_res.start = binfo->start + ofst;
> > >  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> > > -	finfo->irq_base = irq_base;
> > > -	finfo->nr_irqs = nr_irqs;
> > > +
> > > +	ret = parse_feature_irqs(binfo, ofst, finfo);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (dfh_version == 1) {
> > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> > > +		if (v & DFHv1_CSR_ADDR_REL)
> > > +			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> > > +		else
> > > +			finfo->csr_res.start = binfo->start + ofst
> > > +					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> > > +
> > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> > > +		finfo->csr_res.end = finfo->csr_res.start
> > > +				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> > > +	}
> > > 
> > >  	list_add_tail(&finfo->node, &binfo->sub_features);
> > >  	binfo->feature_num++;
> > > @@ -1879,6 +1923,34 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> > >  }
> > >  EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
> > > 
> > > +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
> > > +{
> > > +	int off = DFHv1_PARAM_HDR;
> > > +	u64 v, next;
> > > +
> > > +	while (off < max) {
> > > +		v = readq(base + off);
> > > +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> > > +			return (DFHv1_PARAM_DATA + off);
> > > +
> > > +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> > > +		if (!next)
> > > +			break;
> > > +
> > > +		off += next;
> > > +	}
> > > +
> > > +	return -ENOENT;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dfhv1_find_param);
> > > +
> > > +int dfhv1_has_params(void __iomem *dfh_base)
> > > +{
> > > +	return (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> > > +		readq(dfh_base + DFHv1_CSR_SIZE_GRP)));
> > > +}
> > > +EXPORT_SYMBOL_GPL(dfhv1_has_params);
> > > +
> > >  static void __exit dfl_fpga_exit(void)
> > >  {
> > >  	dfl_chardev_uinit();
> > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > > index bd8720bc5320..0423aa8319ed 100644
> > > --- a/drivers/fpga/dfl.h
> > > +++ b/drivers/fpga/dfl.h
> > > @@ -266,6 +266,7 @@ struct dfl_feature_irq_ctx {
> > >   *		    this index is used to find its mmio resource from the
> > >   *		    feature dev (platform device)'s resources.
> > >   * @ioaddr: mapped mmio resource address.
> > > + * @csr_res: resource for DFHv1 feature registers
> > >   * @irq_ctx: interrupt context list.
> > >   * @nr_irqs: number of interrupt contexts.
> > >   * @ops: ops of this sub feature.
> > > @@ -276,8 +277,10 @@ struct dfl_feature {
> > >  	struct platform_device *dev;
> > >  	u16 id;
> > >  	u8 revision;
> > > +	u8 dfh_version;
> > >  	int resource_index;
> > >  	void __iomem *ioaddr;
> > > +	struct resource csr_res;
> > >  	struct dfl_feature_irq_ctx *irq_ctx;
> > >  	unsigned int nr_irqs;
> > >  	const struct dfl_feature_ops *ops;
> > > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > > index 1a1a2b894687..71760c6a25d7 100644
> > > --- a/include/linux/dfl.h
> > > +++ b/include/linux/dfl.h
> > > @@ -39,6 +39,7 @@ enum dfl_id_type {
> > >   * @type: type of DFL FIU of the device. See enum dfl_id_type.
> > >   * @feature_id: feature identifier local to its DFL FIU type.
> > >   * @mmio_res: mmio resource of this dfl device.
> > > + * @csr_res: resource for DFHv1 feature registers
> > 
> > I think the combination of mmio_res & csr_res is confusing. Why a
> > special csr_res dedicated for DFHv1, and what the mmio_res stands for if
> > the csr_res exists? And they may overlap each other.
> 
> With DFHv1, the registers of a feature may be in a different location than
> the location of the Device Feature Header.  So a resource is needed for the
> DFH and another resource for the actual register space.  Would changing the
> name of mmio_res to dfh_res be less confusing? Unfortunately, changing the
> name of mmio_res would impact the existing dfl drivers.

There are 4 existing dfl drivers now, I think making changes is still
possible. But what is more important to me is to have a clear definition
of resource layout for dfl_device structure, cause it is used in other kernel
domains. Resource overlapping is hard to understand.

Some possible options in my mind:
1. keep the mmio_res, but it only includes the csr region.

   dfl driver couldn't access dfl header & param directly, but I think
   it's OK, dfl drivers don't have to care too much about the HW layout
   of dfl header & param. The header & param values could be parsed in dfl
   core and provided to dfl drivers by APIs.

   No extension of multiple csr regions in future.

2. struct resource mmio_res -> int num_mmio_res & struct resource *mmio_res

   Same as platform device, multiple resources for multiple memory blocks,
   maybe each tagged by name, they don't overlap each other. It impacts the
   existing dfl drivers, but may support multiple csr regions if possible.

> 
> The two resources may overlap, but that is why in
> drivers/tty/serial/8250/8250_dfl.c the mmio_res is mapped, then DFH parsed,
> and unmapped before the csr_res is mapped in the call to
> serial8250_register_8250_port().

This is what I want to avoid, every dfl driver needs the same routine
for the mmios they really want, which is a challenge to all domain
reviewers.

Thanks,
Yilun

> 
> > 
> > Could you present some general purpose mmio resource layout which is
> > compatible to dfl v0 & v1? People from other domains just need to know
> > the basic concept like how many register blocks in the device, what are
> > their ranges ...
> 
> I don't know how a single resource object can be used in the case of DFHv1
> when the registers are in a different location than the DFH.
> 
> > 
> > Thanks,
> > Yilun
> > 

Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

On Tue, 11 Oct 2022, Xu Yilun wrote:

> On 2022-10-10 at 09:58:00 -0700, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Thu, 6 Oct 2022, Xu Yilun wrote:
>>
>>> On 2022-10-04 at 07:37:17 -0700, matthew.gerlach@linux.intel.com wrote:
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Add generic support for MSIX interrupts for DFL devices.
>>>>
>>>> The location of a feature's registers is explicitly
>>>> described in DFHv1 and can be relative to the base of the DFHv1
>>>> or an absolute address.  Parse the location and pass the information
>>>> to DFL driver.
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>> v3: remove unneeded blank line
>>>>     use clearer variable name
>>>>     pass finfo into parse_feature_irqs()
>>>>     refactor code for better indentation
>>>>     use switch statement for irq parsing
>>>>     squash in code parsing register location
>>>>
>>>> v2: fix kernel doc
>>>>     clarify use of DFH_VERSION field
>>>> ---
>>>>  drivers/fpga/dfl.c  | 150 ++++++++++++++++++++++++++++++++------------
>>>>  drivers/fpga/dfl.h  |   3 +
>>>>  include/linux/dfl.h |  20 ++++++
>>>>  3 files changed, 134 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>>> index b9aae85ba930..6a74317e549e 100644
>>>> --- a/drivers/fpga/dfl.c
>>>> +++ b/drivers/fpga/dfl.c
>>>> @@ -380,7 +380,11 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>>>>  	ddev->type = feature_dev_id_type(pdev);
>>>>  	ddev->feature_id = feature->id;
>>>>  	ddev->revision = feature->revision;
>>>> +	ddev->dfh_version = feature->dfh_version;
>>>>  	ddev->cdev = pdata->dfl_cdev;
>>>> +	ddev->csr_res.start = feature->csr_res.start;
>>>> +	ddev->csr_res.end = feature->csr_res.end;
>>>> +	ddev->csr_res.flags = IORESOURCE_MEM;
>>>>
>>>>  	/* add mmio resource */
>>>>  	parent_res = &pdev->resource[feature->resource_index];
>>>> @@ -708,18 +712,24 @@ struct build_feature_devs_info {
>>>>   * struct dfl_feature_info - sub feature info collected during feature dev build
>>>>   *
>>>>   * @fid: id of this sub feature.
>>>> + * @revision: revision of this sub feature
>>>> + * @dfh_version: version of Device Feature Header (DFH)
>>>>   * @mmio_res: mmio resource of this sub feature.
>>>>   * @ioaddr: mapped base address of mmio resource.
>>>>   * @node: node in sub_features linked list.
>>>> + * @csr_res: resource of DFHv1 feature registers
>>>> + * @csr_size: DFHv1 size of feature registers
>>>>   * @irq_base: start of irq index in this sub feature.
>>>>   * @nr_irqs: number of irqs of this sub feature.
>>>>   */
>>>>  struct dfl_feature_info {
>>>>  	u16 fid;
>>>>  	u8 revision;
>>>> +	u8 dfh_version;
>>>>  	struct resource mmio_res;
>>>>  	void __iomem *ioaddr;
>>>>  	struct list_head node;
>>>> +	struct resource csr_res;
>>>>  	unsigned int irq_base;
>>>>  	unsigned int nr_irqs;
>>>>  };
>>>> @@ -797,6 +807,9 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>>>>  		feature->dev = fdev;
>>>>  		feature->id = finfo->fid;
>>>>  		feature->revision = finfo->revision;
>>>> +		feature->dfh_version = finfo->dfh_version;
>>>> +		feature->csr_res.start = finfo->csr_res.start;
>>>> +		feature->csr_res.end = finfo->csr_res.end;
>>>>
>>>>  		/*
>>>>  		 * the FIU header feature has some fundamental functions (sriov
>>>> @@ -935,55 +948,74 @@ static u16 feature_id(u64 value)
>>>>  }
>>>>
>>>>  static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>>> -			      resource_size_t ofst, u16 fid,
>>>> -			      unsigned int *irq_base, unsigned int *nr_irqs)
>>>> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>>>>  {
>>>>  	void __iomem *base = binfo->ioaddr + ofst;
>>>>  	unsigned int i, ibase, inr = 0;
>>>>  	enum dfl_id_type type;
>>>> -	int virq;
>>>> -	u64 v;
>>>> -
>>>> -	type = feature_dev_id_type(binfo->feature_dev);
>>>> +	u16 fid = finfo->fid;
>>>> +	u64 v, dfh_ver;
>>>> +	int virq, off;
>>>>
>>>>  	/*
>>>>  	 * Ideally DFL framework should only read info from DFL header, but
>>>> -	 * current version DFL only provides mmio resources information for
>>>> +	 * current version, DFHv0, only provides mmio resources information for
>>>>  	 * each feature in DFL Header, no field for interrupt resources.
>>>>  	 * Interrupt resource information is provided by specific mmio
>>>>  	 * registers of each private feature which supports interrupt. So in
>>>>  	 * order to parse and assign irq resources, DFL framework has to look
>>>>  	 * into specific capability registers of these private features.
>>>>  	 *
>>>> -	 * Once future DFL version supports generic interrupt resource
>>>> -	 * information in common DFL headers, the generic interrupt parsing
>>>> -	 * code will be added. But in order to be compatible to old version
>>>> +	 * DFHv1 supports generic interrupt resource information in DFHv1
>>>> +	 * parameter blocks. But in order to be compatible to old version
>>>>  	 * DFL, the driver may still fall back to these quirks.
>>>>  	 */
>>>> -	if (type == PORT_ID) {
>>>> -		switch (fid) {
>>>> -		case PORT_FEATURE_ID_UINT:
>>>> -			v = readq(base + PORT_UINT_CAP);
>>>> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>>>> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>>>> +
>>>> +	switch (finfo->dfh_version) {
>>>> +	case 0:
>>>> +		type = feature_dev_id_type(binfo->feature_dev);
>>>> +		if (type == PORT_ID) {
>>>> +			switch (fid) {
>>>> +			case PORT_FEATURE_ID_UINT:
>>>> +				v = readq(base + PORT_UINT_CAP);
>>>> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>>>> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>>>> +				break;
>>>> +			case PORT_FEATURE_ID_ERROR:
>>>> +				v = readq(base + PORT_ERROR_CAP);
>>>> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>>>> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>>>> +				break;
>>>> +			}
>>>> +		} else if (type == FME_ID) {
>>>> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>>>> +				v = readq(base + FME_ERROR_CAP);
>>>> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>>>> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>>>> +			}
>>>> +		}
>>>> +		break;
>>>> +
>>>> +	case 1:
>>>> +		if (!dfhv1_has_params(base))
>>>>  			break;
>>>> -		case PORT_FEATURE_ID_ERROR:
>>>> -			v = readq(base + PORT_ERROR_CAP);
>>>> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>>>> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>>>> +
>>>> +		off = dfhv1_find_param(base, ofst, DFHv1_PARAM_ID_MSIX);
>>>> +		if (off < 0)
>>>>  			break;
>>>> -		}
>>>> -	} else if (type == FME_ID) {
>>>> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>>>> -			v = readq(base + FME_ERROR_CAP);
>>>> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>>>> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>>>> -		}
>>>> +
>>>> +		ibase = readl(base + off + DFHv1_PARAM_MSIX_STARTV);
>>>> +		inr = readl(base + off + DFHv1_PARAM_MSIX_NUMV);
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		dev_warn(binfo->dev, "unexpected DFH version %lld\n", dfh_ver);
>>>> +		break;
>>>>  	}
>>>>
>>>>  	if (!inr) {
>>>> -		*irq_base = 0;
>>>> -		*nr_irqs = 0;
>>>> +		finfo->irq_base = 0;
>>>> +		finfo->nr_irqs = 0;
>>>>  		return 0;
>>>>  	}
>>>>
>>>> @@ -1006,8 +1038,8 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>>>  		}
>>>>  	}
>>>>
>>>> -	*irq_base = ibase;
>>>> -	*nr_irqs = inr;
>>>> +	finfo->irq_base = ibase;
>>>> +	finfo->nr_irqs = inr;
>>>>
>>>>  	return 0;
>>>>  }
>>>> @@ -1023,8 +1055,8 @@ static int
>>>>  create_feature_instance(struct build_feature_devs_info *binfo,
>>>>  			resource_size_t ofst, resource_size_t size, u16 fid)
>>>>  {
>>>> -	unsigned int irq_base, nr_irqs;
>>>>  	struct dfl_feature_info *finfo;
>>>> +	u8 dfh_version = 0;
>>>>  	u8 revision = 0;
>>>>  	int ret;
>>>>  	u64 v;
>>>> @@ -1032,7 +1064,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>>>>  	if (fid != FEATURE_ID_AFU) {
>>>>  		v = readq(binfo->ioaddr + ofst);
>>>>  		revision = FIELD_GET(DFH_REVISION, v);
>>>> -
>>>> +		dfh_version = FIELD_GET(DFH_VERSION, v);
>>>>  		/* read feature size and id if inputs are invalid */
>>>>  		size = size ? size : feature_size(v);
>>>>  		fid = fid ? fid : feature_id(v);
>>>> @@ -1041,21 +1073,33 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>>>>  	if (binfo->len - ofst < size)
>>>>  		return -EINVAL;
>>>>
>>>> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>>>> -	if (ret)
>>>> -		return ret;
>>>> -
>>>>  	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>>>>  	if (!finfo)
>>>>  		return -ENOMEM;
>>>>
>>>>  	finfo->fid = fid;
>>>>  	finfo->revision = revision;
>>>> +	finfo->dfh_version = dfh_version;
>>>>  	finfo->mmio_res.start = binfo->start + ofst;
>>>>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>>>> -	finfo->irq_base = irq_base;
>>>> -	finfo->nr_irqs = nr_irqs;
>>>> +
>>>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (dfh_version == 1) {
>>>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>>>> +		if (v & DFHv1_CSR_ADDR_REL)
>>>> +			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>>>> +		else
>>>> +			finfo->csr_res.start = binfo->start + ofst
>>>> +					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>>>> +
>>>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>>>> +		finfo->csr_res.end = finfo->csr_res.start
>>>> +				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>>>> +	}
>>>>
>>>>  	list_add_tail(&finfo->node, &binfo->sub_features);
>>>>  	binfo->feature_num++;
>>>> @@ -1879,6 +1923,34 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>>>>
>>>> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
>>>> +{
>>>> +	int off = DFHv1_PARAM_HDR;
>>>> +	u64 v, next;
>>>> +
>>>> +	while (off < max) {
>>>> +		v = readq(base + off);
>>>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>>>> +			return (DFHv1_PARAM_DATA + off);
>>>> +
>>>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>>>> +		if (!next)
>>>> +			break;
>>>> +
>>>> +		off += next;
>>>> +	}
>>>> +
>>>> +	return -ENOENT;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dfhv1_find_param);
>>>> +
>>>> +int dfhv1_has_params(void __iomem *dfh_base)
>>>> +{
>>>> +	return (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>>>> +		readq(dfh_base + DFHv1_CSR_SIZE_GRP)));
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dfhv1_has_params);
>>>> +
>>>>  static void __exit dfl_fpga_exit(void)
>>>>  {
>>>>  	dfl_chardev_uinit();
>>>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>>>> index bd8720bc5320..0423aa8319ed 100644
>>>> --- a/drivers/fpga/dfl.h
>>>> +++ b/drivers/fpga/dfl.h
>>>> @@ -266,6 +266,7 @@ struct dfl_feature_irq_ctx {
>>>>   *		    this index is used to find its mmio resource from the
>>>>   *		    feature dev (platform device)'s resources.
>>>>   * @ioaddr: mapped mmio resource address.
>>>> + * @csr_res: resource for DFHv1 feature registers
>>>>   * @irq_ctx: interrupt context list.
>>>>   * @nr_irqs: number of interrupt contexts.
>>>>   * @ops: ops of this sub feature.
>>>> @@ -276,8 +277,10 @@ struct dfl_feature {
>>>>  	struct platform_device *dev;
>>>>  	u16 id;
>>>>  	u8 revision;
>>>> +	u8 dfh_version;
>>>>  	int resource_index;
>>>>  	void __iomem *ioaddr;
>>>> +	struct resource csr_res;
>>>>  	struct dfl_feature_irq_ctx *irq_ctx;
>>>>  	unsigned int nr_irqs;
>>>>  	const struct dfl_feature_ops *ops;
>>>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>>>> index 1a1a2b894687..71760c6a25d7 100644
>>>> --- a/include/linux/dfl.h
>>>> +++ b/include/linux/dfl.h
>>>> @@ -39,6 +39,7 @@ enum dfl_id_type {
>>>>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
>>>>   * @feature_id: feature identifier local to its DFL FIU type.
>>>>   * @mmio_res: mmio resource of this dfl device.
>>>> + * @csr_res: resource for DFHv1 feature registers
>>>
>>> I think the combination of mmio_res & csr_res is confusing. Why a
>>> special csr_res dedicated for DFHv1, and what the mmio_res stands for if
>>> the csr_res exists? And they may overlap each other.
>>
>> With DFHv1, the registers of a feature may be in a different location than
>> the location of the Device Feature Header.  So a resource is needed for the
>> DFH and another resource for the actual register space.  Would changing the
>> name of mmio_res to dfh_res be less confusing? Unfortunately, changing the
>> name of mmio_res would impact the existing dfl drivers.
>
> There are 4 existing dfl drivers now, I think making changes is still
> possible. But what is more important to me is to have a clear definition
> of resource layout for dfl_device structure, cause it is used in other kernel
> domains. Resource overlapping is hard to understand.

I agree that we need to have a clear definition of resource layout for 
dfl_device structure, and resource overlapping is hard to understand.

>
> Some possible options in my mind:
> 1. keep the mmio_res, but it only includes the csr region.
>
>   dfl driver couldn't access dfl header & param directly, but I think
>   it's OK, dfl drivers don't have to care too much about the HW layout
>   of dfl header & param. The header & param values could be parsed in dfl
>   core and provided to dfl drivers by APIs.
>
>   No extension of multiple csr regions in future.
>
> 2. struct resource mmio_res -> int num_mmio_res & struct resource *mmio_res
>
>   Same as platform device, multiple resources for multiple memory blocks,
>   maybe each tagged by name, they don't overlap each other. It impacts the
>   existing dfl drivers, but may support multiple csr regions if possible.
>

I think we may need a combination of both of your suggested options. I 
think a limitation to a single memory resource will be insufficient for 
many IP blocks.  DFL drivers do need to know things in header like 
parameters and feature revisions, but I agree they can be parsed out and 
dfl drivers can access the information by APIs.

>>
>> The two resources may overlap, but that is why in
>> drivers/tty/serial/8250/8250_dfl.c the mmio_res is mapped, then DFH parsed,
>> and unmapped before the csr_res is mapped in the call to
>> serial8250_register_8250_port().
>
> This is what I want to avoid, every dfl driver needs the same routine
> for the mmios they really want, which is a challenge to all domain
> reviewers.

I understand your concern. Let's try to get this concern fully addressed.

>
> Thanks,
> Yilun
>
>>
>>>
>>> Could you present some general purpose mmio resource layout which is
>>> compatible to dfl v0 & v1? People from other domains just need to know
>>> the basic concept like how many register blocks in the device, what are
>>> their ranges ...
>>
>> I don't know how a single resource object can be used in the case of DFHv1
>> when the registers are in a different location than the DFH.
>>
>>>
>>> Thanks,
>>> Yilun
>>>
>

[PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add a Device Feature List (DFL) bus driver for the Altera
16550 implementation of UART.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reported-by: kernel test robot <lkp@intel.com>
---
v3: use passed in location of registers
    use cleaned up functions for parsing parameters

v2: clean up error messages
    alphabetize header files
    fix 'missing prototype' error by making function static
    tried to sort Makefile and Kconfig better
---
 drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |   9 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 3 files changed, 187 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
new file mode 100644
index 000000000000..110ad3a73459
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Ananda Ravuri <ananda.ravuri@intel.com>
+ *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/dfl.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+
+struct dfl_uart {
+	int line;
+};
+
+static int dfl_uart_get_params(struct device *dev, void __iomem *dfh_base, resource_size_t max,
+			       struct uart_8250_port *uart)
+{
+	u64 v, fifo_len, reg_width;
+	int off;
+
+	if (!dfhv1_has_params(dfh_base)) {
+		dev_err(dev, "missing required DFH parameters\n");
+		return -EINVAL;
+	}
+
+	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
+	if (off < 0) {
+		dev_err(dev, "missing CLK_FRQ param\n");
+		return -EINVAL;
+	}
+
+	uart->port.uartclk = readq(dfh_base + off);
+	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
+
+	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
+	if (off < 0) {
+		dev_err(dev, "missing FIFO_LEN param\n");
+		return -EINVAL;
+	}
+
+	fifo_len = readq(dfh_base + off);
+	dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
+
+	switch (fifo_len) {
+	case 32:
+		uart->port.type = PORT_ALTR_16550_F32;
+		break;
+
+	case 64:
+		uart->port.type = PORT_ALTR_16550_F64;
+		break;
+
+	case 128:
+		uart->port.type = PORT_ALTR_16550_F128;
+		break;
+
+	default:
+		dev_err(dev, "bad fifo_len %llu\n", fifo_len);
+		return -EINVAL;
+	}
+
+	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
+	if (off < 0) {
+		dev_err(dev, "missing REG_LAYOUT param\n");
+		return -EINVAL;
+	}
+
+	v = readq(dfh_base + off);
+	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
+	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
+
+	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
+		FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
+
+	switch (reg_width) {
+	case 4:
+		uart->port.iotype = UPIO_MEM32;
+		break;
+
+	case 2:
+		uart->port.iotype = UPIO_MEM16;
+		break;
+
+	default:
+		dev_err(dev, "invalid reg_width %lld\n", reg_width);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int dfl_uart_probe(struct dfl_device *dfl_dev)
+{
+	struct device *dev = &dfl_dev->dev;
+	struct uart_8250_port uart;
+	struct dfl_uart *dfluart;
+	resource_size_t res_size;
+	void __iomem *dfh_base;
+	int ret;
+
+	memset(&uart, 0, sizeof(uart));
+	uart.port.flags = UPF_IOREMAP;
+	uart.port.mapbase = dfl_dev->csr_res.start;
+	uart.port.mapsize = resource_size(&dfl_dev->csr_res);
+
+	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+	if (!dfluart)
+		return -ENOMEM;
+
+	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+	if (IS_ERR(dfh_base))
+		return PTR_ERR(dfh_base);
+
+	res_size = resource_size(&dfl_dev->mmio_res);
+
+	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
+
+	devm_iounmap(dev, dfh_base);
+	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
+
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed uart feature walk\n");
+
+	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
+
+	if (dfl_dev->num_irqs == 1)
+		uart.port.irq = dfl_dev->irqs[0];
+
+	/* register the port */
+	dfluart->line = serial8250_register_8250_port(&uart);
+	if (dfluart->line < 0)
+		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
+
+	dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
+	dev_set_drvdata(dev, dfluart);
+
+	return 0;
+}
+
+static void dfl_uart_remove(struct dfl_device *dfl_dev)
+{
+	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
+
+	if (dfluart->line >= 0)
+		serial8250_unregister_port(dfluart->line);
+}
+
+#define FME_FEATURE_ID_UART 0x24
+
+static const struct dfl_device_id dfl_uart_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_UART },
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
+
+static struct dfl_driver dfl_uart_driver = {
+	.drv = {
+		.name = "dfl-uart",
+	},
+	.id_table = dfl_uart_ids,
+	.probe = dfl_uart_probe,
+	.remove = dfl_uart_remove,
+};
+module_dfl_driver(dfl_uart_driver);
+
+MODULE_DESCRIPTION("DFL Intel UART driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index d0b49e15fbf5..5c6497ce5c12 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
 
 	  If unsure, say N.
 
+config SERIAL_8250_DFL
+	tristate "DFL bus driver for Altera 16550 UART"
+	depends on SERIAL_8250 && FPGA_DFL
+	help
+	  This option enables support for a Device Feature List (DFL) bus
+	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
+	  can be instantiated in a FPGA and then be discovered during
+	  enumeration of the DFL bus.
+
 config SERIAL_8250_FSL
 	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
 	depends on SERIAL_8250_CONSOLE
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..32006e0982d1 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE)	+= 8250_early.o
 obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
 obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
+obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
-- 
2.25.1

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.

...

> Reported-by: kernel test robot <lkp@intel.com>

https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

"The Reported-by tag gives credit to people who find bugs and report them and it
hopefully inspires them to help us again in the future. Please note that if the
bug was reported in private, then ask for permission first before using the
Reported-by tag. The tag is intended for bugs; please do not use it to credit
feature requests."


...

> +	if (!dfhv1_has_params(dfh_base)) {
> +		dev_err(dev, "missing required DFH parameters\n");
> +		return -EINVAL;
> +	}

Why not use dev_err_probe() everywhere since this is called only at ->probe()
stage?

...

> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> +	if (off < 0) {
> +		dev_err(dev, "missing CLK_FRQ param\n");

> +		return -EINVAL;

Why error code is being shadowed?

> +	}

...

> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> +	if (off < 0) {
> +		dev_err(dev, "missing FIFO_LEN param\n");
> +		return -EINVAL;

Ditto.

> +	}

...

> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (off < 0) {
> +		dev_err(dev, "missing REG_LAYOUT param\n");
> +		return -EINVAL;
> +	}

Ditto.

...

> +	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
> +		FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);

Casting in printf() in kernel in 99% shows the wrong specifier in use. Try to
select the best suitable one.

...

> +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +	if (IS_ERR(dfh_base))
> +		return PTR_ERR(dfh_base);
> +
> +	res_size = resource_size(&dfl_dev->mmio_res);
> +
> +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);

> +	devm_iounmap(dev, dfh_base);
> +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);

If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
to begin with? The devm_* release type of functions in 99% of the cases
indicate of the abusing devm_.

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");

...

> +	dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);

Why do we need this noise?

...

> +	if (dfluart->line >= 0)

When this can be false?

> +		serial8250_unregister_port(dfluart->line);

...

> +config SERIAL_8250_DFL
> +	tristate "DFL bus driver for Altera 16550 UART"
> +	depends on SERIAL_8250 && FPGA_DFL
> +	help
> +	  This option enables support for a Device Feature List (DFL) bus
> +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
> +	  can be instantiated in a FPGA and then be discovered during
> +	  enumeration of the DFL bus.

When m, what be the module name?

...

>  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
>  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o

This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
entries are not properly placed there and in Makefile.)

>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

On Tue, 4 Oct 2022, Andy Shevchenko wrote:

> On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>
> ...
>
>> Reported-by: kernel test robot <lkp@intel.com>
>
> https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>
> "The Reported-by tag gives credit to people who find bugs and report them and it
> hopefully inspires them to help us again in the future. Please note that if the
> bug was reported in private, then ask for permission first before using the
> Reported-by tag. The tag is intended for bugs; please do not use it to credit
> feature requests."
>

The kernel test robot did find a bug in my v1 submission.  I was missing 
the static keyword for a function declaration.  Should I remove the tag?

>
> ...
>
>> +	if (!dfhv1_has_params(dfh_base)) {
>> +		dev_err(dev, "missing required DFH parameters\n");
>> +		return -EINVAL;
>> +	}
>
> Why not use dev_err_probe() everywhere since this is called only at ->probe()
> stage?

I wasn't sure if using dev_err_probe() was correct, since the usage is 
technically in a different function.  Since the code is only called from 
->probe(), and it is much cleaner, I'll switch to dev_err_probe() 
everywhere

> > ...
>
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing CLK_FRQ param\n");
>
>> +		return -EINVAL;
>
> Why error code is being shadowed?

Definitely a mistake.

>
>> +	}
>
> ...
>
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing FIFO_LEN param\n");
>> +		return -EINVAL;
>
> Ditto.
>
>> +	}
>
> ...
>
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing REG_LAYOUT param\n");
>> +		return -EINVAL;
>> +	}
>
> Ditto.
>
> ...
>
>> +	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
>> +		FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
>
> Casting in printf() in kernel in 99% shows the wrong specifier in use. Try to
> select the best suitable one.

I will remove the casting and find the correct format specifier.

>
> ...
>
>> +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> +	if (IS_ERR(dfh_base))
>> +		return PTR_ERR(dfh_base);
>> +
>> +	res_size = resource_size(&dfl_dev->mmio_res);
>> +
>> +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>
>> +	devm_iounmap(dev, dfh_base);
>> +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>
> If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
> to begin with? The devm_* release type of functions in 99% of the cases
> indicate of the abusing devm_.

I will change the code to call ioremap() and request_mem_region() directly 
instead of the devm_ versions.

>
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
>
> ...
>
>> +	dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
>
> Why do we need this noise?

No, we do not need this noise.

>
> ...
>
>> +	if (dfluart->line >= 0)
>
> When this can be false?

This can never be false.  I will remove it.

>
>> +		serial8250_unregister_port(dfluart->line);
>
> ...
>
>> +config SERIAL_8250_DFL
>> +	tristate "DFL bus driver for Altera 16550 UART"
>> +	depends on SERIAL_8250 && FPGA_DFL
>> +	help
>> +	  This option enables support for a Device Feature List (DFL) bus
>> +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
>> +	  can be instantiated in a FPGA and then be discovered during
>> +	  enumeration of the DFL bus.
>
> When m, what be the module name?

I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed 
into /lib/modules/...  I also see "alias dfl:t0000f0024* 8250_dfl" in 
modules.alias


>
> ...
>
>>  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
>>  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
>>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
>> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>
> This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
> it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
> entries are not properly placed there and in Makefile.)

Since 8250_dfl results in its own module, and my kernel config doesn't 
have FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.

>
>>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks for the feedback.


>
>
>

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Thu, Oct 06, 2022 at 10:00:43AM -0700, matthew.gerlach@linux.intel.com wrote:
> On Tue, 4 Oct 2022, Andy Shevchenko wrote:
> > On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@linux.intel.com wrote:

...

> > > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> > 
> > "The Reported-by tag gives credit to people who find bugs and report them and it
> > hopefully inspires them to help us again in the future. Please note that if the
> > bug was reported in private, then ask for permission first before using the
> > Reported-by tag. The tag is intended for bugs; please do not use it to credit
> > feature requests."
> 
> The kernel test robot did find a bug in my v1 submission.  I was missing the
> static keyword for a function declaration.  Should I remove the tag?

What's yours take from the above documentation?

...

> > > +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > +	if (IS_ERR(dfh_base))
> > > +		return PTR_ERR(dfh_base);
> > > +
> > > +	res_size = resource_size(&dfl_dev->mmio_res);
> > > +
> > > +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
> > 
> > > +	devm_iounmap(dev, dfh_base);
> > > +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
> > 
> > If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
> > to begin with? The devm_* release type of functions in 99% of the cases
> > indicate of the abusing devm_.
> 
> I will change the code to call ioremap() and request_mem_region() directly
> instead of the devm_ versions.

But why will you need request_mem_region() in that case?

> > > +	if (ret < 0)
> > > +		return dev_err_probe(dev, ret, "failed uart feature walk\n");

...

> > > +config SERIAL_8250_DFL
> > > +	tristate "DFL bus driver for Altera 16550 UART"
> > > +	depends on SERIAL_8250 && FPGA_DFL
> > > +	help
> > > +	  This option enables support for a Device Feature List (DFL) bus
> > > +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
> > > +	  can be instantiated in a FPGA and then be discovered during
> > > +	  enumeration of the DFL bus.
> > 
> > When m, what be the module name?
> 
> I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
> /lib/modules/...  I also see "alias dfl:t0000f0024* 8250_dfl" in
> modules.alias

My point is that user who will run `make menuconfig` will read this and have
no clue after the kernel build if the module was built or not. Look into other
(recent) sections of the Kconfig for drivers in the kernel for how they inform
user about the module name (this more or less standard pattern you just need
to copy'n'paste'n'edit carefully).

...

> > >  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
> > >  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
> > >  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
> > > +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
> > 
> > This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
> > it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
> > entries are not properly placed there and in Makefile.)
> 
> Since 8250_dfl results in its own module, and my kernel config doesn't have
> FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.

The Makefile is a bit chaotic, but try to find the sorted (more or less)
group of drivers that are not 4 ports and squeeze your entry there
(I expect somewhere between the LPSS/MID lines).

It will help to sort out that mess in the future.

> > >  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
> > >  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
> > >  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

On Thu, 6 Oct 2022, Andy Shevchenko wrote:

> On Thu, Oct 06, 2022 at 10:00:43AM -0700, matthew.gerlach@linux.intel.com wrote:
>> On Tue, 4 Oct 2022, Andy Shevchenko wrote:
>>> On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@linux.intel.com wrote:
>
> ...
>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>>>
>>> "The Reported-by tag gives credit to people who find bugs and report them and it
>>> hopefully inspires them to help us again in the future. Please note that if the
>>> bug was reported in private, then ask for permission first before using the
>>> Reported-by tag. The tag is intended for bugs; please do not use it to credit
>>> feature requests."
>>
>> The kernel test robot did find a bug in my v1 submission.  I was missing the
>> static keyword for a function declaration.  Should I remove the tag?
>
> What's yours take from the above documentation?
>

Since the kernel test robot did find a bug. The tag should stay.

> ...
>
>>>> +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>>>> +	if (IS_ERR(dfh_base))
>>>> +		return PTR_ERR(dfh_base);
>>>> +
>>>> +	res_size = resource_size(&dfl_dev->mmio_res);
>>>> +
>>>> +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>>>
>>>> +	devm_iounmap(dev, dfh_base);
>>>> +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>>>
>>> If it's temporary, may be you shouldn't even consider devm_ioremap_resource()
>>> to begin with? The devm_* release type of functions in 99% of the cases
>>> indicate of the abusing devm_.
>>
>> I will change the code to call ioremap() and request_mem_region() directly
>> instead of the devm_ versions.
>
> But why will you need request_mem_region() in that case?

It doesn't seem that I need to call request_mem_regsion; so I will skip 
it.

>
>>>> +	if (ret < 0)
>>>> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
>
> ...
>
>>>> +config SERIAL_8250_DFL
>>>> +	tristate "DFL bus driver for Altera 16550 UART"
>>>> +	depends on SERIAL_8250 && FPGA_DFL
>>>> +	help
>>>> +	  This option enables support for a Device Feature List (DFL) bus
>>>> +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
>>>> +	  can be instantiated in a FPGA and then be discovered during
>>>> +	  enumeration of the DFL bus.
>>>
>>> When m, what be the module name?
>>
>> I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
>> /lib/modules/...  I also see "alias dfl:t0000f0024* 8250_dfl" in
>> modules.alias
>
> My point is that user who will run `make menuconfig` will read this and have
> no clue after the kernel build if the module was built or not. Look into other
> (recent) sections of the Kconfig for drivers in the kernel for how they inform
> user about the module name (this more or less standard pattern you just need
> to copy'n'paste'n'edit carefully).
>
> ...

I think this should be added:
           To compile this driver as a module, chose M here: the
           module will be called 8250_dfl.
>
>>>>  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
>>>>  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
>>>>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
>>>> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>>>
>>> This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
>>> it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
>>> entries are not properly placed there and in Makefile.)
>>
>> Since 8250_dfl results in its own module, and my kernel config doesn't have
>> FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
>
> The Makefile is a bit chaotic, but try to find the sorted (more or less)
> group of drivers that are not 4 ports and squeeze your entry there
> (I expect somewhere between the LPSS/MID lines).
>
> It will help to sort out that mess in the future.

I will move 8250_dfl between LPSS and MID lines in the Makefile.  Should I 
move the definition in Kconfig to be between LPSS and MID to be consistent?

>
>>>>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>>>>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>>>>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Thu, Oct 06, 2022 at 03:24:16PM -0700, matthew.gerlach@linux.intel.com wrote:
> On Thu, 6 Oct 2022, Andy Shevchenko wrote:
> > On Thu, Oct 06, 2022 at 10:00:43AM -0700, matthew.gerlach@linux.intel.com wrote:
> > > On Tue, 4 Oct 2022, Andy Shevchenko wrote:
> > > > On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@linux.intel.com wrote:

...

> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > 
> > > > https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> > > > 
> > > > "The Reported-by tag gives credit to people who find bugs and report them and it
> > > > hopefully inspires them to help us again in the future. Please note that if the
> > > > bug was reported in private, then ask for permission first before using the
> > > > Reported-by tag. The tag is intended for bugs; please do not use it to credit
> > > > feature requests."
> > > 
> > > The kernel test robot did find a bug in my v1 submission.  I was missing the
> > > static keyword for a function declaration.  Should I remove the tag?
> > 
> > What's yours take from the above documentation?
> 
> Since the kernel test robot did find a bug. The tag should stay.

I suggest otherwise because of the last sentence in the cited excerpt: "please
do not use it to credit feature requests". To distinguish "feature request" you
can ask yourself "Am I fixing _existing_ code or adding a new one?" And the
answer here is crystal clear (at least to me).

...

> > > > > +config SERIAL_8250_DFL
> > > > > +	tristate "DFL bus driver for Altera 16550 UART"
> > > > > +	depends on SERIAL_8250 && FPGA_DFL
> > > > > +	help
> > > > > +	  This option enables support for a Device Feature List (DFL) bus
> > > > > +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
> > > > > +	  can be instantiated in a FPGA and then be discovered during
> > > > > +	  enumeration of the DFL bus.
> > > > 
> > > > When m, what be the module name?
> > > 
> > > I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
> > > /lib/modules/...  I also see "alias dfl:t0000f0024* 8250_dfl" in
> > > modules.alias
> > 
> > My point is that user who will run `make menuconfig` will read this and have
> > no clue after the kernel build if the module was built or not. Look into other
> > (recent) sections of the Kconfig for drivers in the kernel for how they inform
> > user about the module name (this more or less standard pattern you just need
> > to copy'n'paste'n'edit carefully).
> 
> I think this should be added:
>           To compile this driver as a module, chose M here: the
>           module will be called 8250_dfl.

Looks good to me!


> > > > >  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
> > > > >  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
> > > > >  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
> > > > > +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
> > > > 
> > > > This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
> > > > it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
> > > > entries are not properly placed there and in Makefile.)
> > > 
> > > Since 8250_dfl results in its own module, and my kernel config doesn't have
> > > FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
> > 
> > The Makefile is a bit chaotic, but try to find the sorted (more or less)
> > group of drivers that are not 4 ports and squeeze your entry there
> > (I expect somewhere between the LPSS/MID lines).
> > 
> > It will help to sort out that mess in the future.
> 
> I will move 8250_dfl between LPSS and MID lines in the Makefile.  Should I
> move the definition in Kconfig to be between LPSS and MID to be consistent?

D is not ordered if put between L and M, I meant not to literally put it there
but think about it a bit.

Kconfig is another story because it has different approach in ordering (seems
so), try to find the best compromise there.

> > > > >  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
> > > > >  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
> > > > >  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

On Fri, 7 Oct 2022, Andy Shevchenko wrote:

> On Thu, Oct 06, 2022 at 03:24:16PM -0700, matthew.gerlach@linux.intel.com wrote:
>> On Thu, 6 Oct 2022, Andy Shevchenko wrote:
>>> On Thu, Oct 06, 2022 at 10:00:43AM -0700, matthew.gerlach@linux.intel.com wrote:
>>>> On Tue, 4 Oct 2022, Andy Shevchenko wrote:
>>>>> On Tue, Oct 04, 2022 at 07:37:18AM -0700, matthew.gerlach@linux.intel.com wrote:
>
> ...
>
>>>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>>
>>>>> https://docs.kernel.org/process/submitting-patches.html?highlight=reported#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>>>>>
>>>>> "The Reported-by tag gives credit to people who find bugs and report them and it
>>>>> hopefully inspires them to help us again in the future. Please note that if the
>>>>> bug was reported in private, then ask for permission first before using the
>>>>> Reported-by tag. The tag is intended for bugs; please do not use it to credit
>>>>> feature requests."
>>>>
>>>> The kernel test robot did find a bug in my v1 submission.  I was missing the
>>>> static keyword for a function declaration.  Should I remove the tag?
>>>
>>> What's yours take from the above documentation?
>>
>> Since the kernel test robot did find a bug. The tag should stay.
>
> I suggest otherwise because of the last sentence in the cited excerpt: "please
> do not use it to credit feature requests". To distinguish "feature request" you
> can ask yourself "Am I fixing _existing_ code or adding a new one?" And the
> answer here is crystal clear (at least to me).
>
> ...
>
>>>>>> +config SERIAL_8250_DFL
>>>>>> +	tristate "DFL bus driver for Altera 16550 UART"
>>>>>> +	depends on SERIAL_8250 && FPGA_DFL
>>>>>> +	help
>>>>>> +	  This option enables support for a Device Feature List (DFL) bus
>>>>>> +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
>>>>>> +	  can be instantiated in a FPGA and then be discovered during
>>>>>> +	  enumeration of the DFL bus.
>>>>>
>>>>> When m, what be the module name?
>>>>
>>>> I see the file, kernel/drivers/tty/serial/8250/8250_dfl.ko, installed into
>>>> /lib/modules/...  I also see "alias dfl:t0000f0024* 8250_dfl" in
>>>> modules.alias
>>>
>>> My point is that user who will run `make menuconfig` will read this and have
>>> no clue after the kernel build if the module was built or not. Look into other
>>> (recent) sections of the Kconfig for drivers in the kernel for how they inform
>>> user about the module name (this more or less standard pattern you just need
>>> to copy'n'paste'n'edit carefully).
>>
>> I think this should be added:
>>           To compile this driver as a module, chose M here: the
>>           module will be called 8250_dfl.
>
> Looks good to me!
>
>
>>>>>>  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
>>>>>>  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
>>>>>>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
>>>>>> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>>>>>
>>>>> This group of drivers for the 4 UARTs on the board or so, does FPGA belong to
>>>>> it? (Same Q, btw, for the Kconfig section. And yes, I know that some of the
>>>>> entries are not properly placed there and in Makefile.)
>>>>
>>>> Since 8250_dfl results in its own module, and my kernel config doesn't have
>>>> FOURPORT, ACCENT, nor BOCA, I guess I don't understand the problem.
>>>
>>> The Makefile is a bit chaotic, but try to find the sorted (more or less)
>>> group of drivers that are not 4 ports and squeeze your entry there
>>> (I expect somewhere between the LPSS/MID lines).
>>>
>>> It will help to sort out that mess in the future.
>>
>> I will move 8250_dfl between LPSS and MID lines in the Makefile.  Should I
>> move the definition in Kconfig to be between LPSS and MID to be consistent?
>
> D is not ordered if put between L and M, I meant not to literally put it there
> but think about it a bit.
>
> Kconfig is another story because it has different approach in ordering (seems
> so), try to find the best compromise there.

In the Kconfig, I think the driver fits into the section, Misc. 
options/drivers.  Within this section, I think SERIAL_8250_DFL should go 
before SERIAL_8250_DW to approximate alphabetical ordering.  Similarly, I 
think 8250_dfl.o should go above 8250_dw.o in the Makefile.

>
>>>>>>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>>>>>>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>>>>>>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Fri, Oct 07, 2022 at 08:10:34AM -0700, matthew.gerlach@linux.intel.com wrote:
> On Fri, 7 Oct 2022, Andy Shevchenko wrote:

...

> In the Kconfig, I think the driver fits into the section, Misc.
> options/drivers.  Within this section, I think SERIAL_8250_DFL should go
> before SERIAL_8250_DW to approximate alphabetical ordering.  Similarly, I
> think 8250_dfl.o should go above 8250_dw.o in the Makefile.

Sounds good to me!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Ilpo Järvinen]

On Tue, 4 Oct 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> v3: use passed in location of registers
>     use cleaned up functions for parsing parameters
> 
> v2: clean up error messages
>     alphabetize header files
>     fix 'missing prototype' error by making function static
>     tried to sort Makefile and Kconfig better
> ---
>  drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |   9 ++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  3 files changed, 187 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_dfl.c
> 
> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
> new file mode 100644
> index 000000000000..110ad3a73459
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@

> +static int dfl_uart_get_params(struct device *dev, void __iomem *dfh_base, resource_size_t max,
> +			       struct uart_8250_port *uart)
> +{
> +	u64 v, fifo_len, reg_width;
> +	int off;
> +
> +	if (!dfhv1_has_params(dfh_base)) {
> +		dev_err(dev, "missing required DFH parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> +	if (off < 0) {
> +		dev_err(dev, "missing CLK_FRQ param\n");
> +		return -EINVAL;
> +	}
> +
> +	uart->port.uartclk = readq(dfh_base + off);
> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> +
> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> +	if (off < 0) {
> +		dev_err(dev, "missing FIFO_LEN param\n");
> +		return -EINVAL;
> +	}
> +
> +	fifo_len = readq(dfh_base + off);
> +	dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> +
> +	switch (fifo_len) {
> +	case 32:
> +		uart->port.type = PORT_ALTR_16550_F32;
> +		break;
> +
> +	case 64:
> +		uart->port.type = PORT_ALTR_16550_F64;
> +		break;
> +
> +	case 128:
> +		uart->port.type = PORT_ALTR_16550_F128;
> +		break;
> +
> +	default:
> +		dev_err(dev, "bad fifo_len %llu\n", fifo_len);

I'd tell user "unsupported" rather than "bad".

> +		return -EINVAL;
> +	}
> +
> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (off < 0) {
> +		dev_err(dev, "missing REG_LAYOUT param\n");
> +		return -EINVAL;
> +	}
> +
> +	v = readq(dfh_base + off);
> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> +
> +	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
> +		FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);

Why not use reg_width directly?

> +	switch (reg_width) {
> +	case 4:
> +		uart->port.iotype = UPIO_MEM32;
> +		break;
> +
> +	case 2:
> +		uart->port.iotype = UPIO_MEM16;
> +		break;
> +
> +	default:
> +		dev_err(dev, "invalid reg_width %lld\n", reg_width);

unsupported ?

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	struct uart_8250_port uart;
> +	struct dfl_uart *dfluart;
> +	resource_size_t res_size;
> +	void __iomem *dfh_base;
> +	int ret;
> +
> +	memset(&uart, 0, sizeof(uart));
> +	uart.port.flags = UPF_IOREMAP;
> +	uart.port.mapbase = dfl_dev->csr_res.start;
> +	uart.port.mapsize = resource_size(&dfl_dev->csr_res);
> +
> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> +	if (!dfluart)
> +		return -ENOMEM;
> +
> +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +	if (IS_ERR(dfh_base))
> +		return PTR_ERR(dfh_base);
> +
> +	res_size = resource_size(&dfl_dev->mmio_res);
> +
> +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
> +
> +	devm_iounmap(dev, dfh_base);
> +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
> +
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
> +
> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
> +
> +	if (dfl_dev->num_irqs == 1)
> +		uart.port.irq = dfl_dev->irqs[0];
> +
> +	/* register the port */

This comment is pretty useless. Just drop it.

> +	dfluart->line = serial8250_register_8250_port(&uart);
> +	if (dfluart->line < 0)
> +		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
> +
> +	dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);

This you want to drop too. It seems a debug thing rather than info level 
stuff.


-- 
 i.

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 5147 bytes --]



On Wed, 5 Oct 2022, Ilpo Järvinen wrote:

> On Tue, 4 Oct 2022, matthew.gerlach@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>> v3: use passed in location of registers
>>     use cleaned up functions for parsing parameters
>>
>> v2: clean up error messages
>>     alphabetize header files
>>     fix 'missing prototype' error by making function static
>>     tried to sort Makefile and Kconfig better
>> ---
>>  drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
>>  drivers/tty/serial/8250/Kconfig    |   9 ++
>>  drivers/tty/serial/8250/Makefile   |   1 +
>>  3 files changed, 187 insertions(+)
>>  create mode 100644 drivers/tty/serial/8250/8250_dfl.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
>> new file mode 100644
>> index 000000000000..110ad3a73459
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,177 @@
>
>> +static int dfl_uart_get_params(struct device *dev, void __iomem *dfh_base, resource_size_t max,
>> +			       struct uart_8250_port *uart)
>> +{
>> +	u64 v, fifo_len, reg_width;
>> +	int off;
>> +
>> +	if (!dfhv1_has_params(dfh_base)) {
>> +		dev_err(dev, "missing required DFH parameters\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing CLK_FRQ param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	uart->port.uartclk = readq(dfh_base + off);
>> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>> +
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing FIFO_LEN param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	fifo_len = readq(dfh_base + off);
>> +	dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
>> +
>> +	switch (fifo_len) {
>> +	case 32:
>> +		uart->port.type = PORT_ALTR_16550_F32;
>> +		break;
>> +
>> +	case 64:
>> +		uart->port.type = PORT_ALTR_16550_F64;
>> +		break;
>> +
>> +	case 128:
>> +		uart->port.type = PORT_ALTR_16550_F128;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev, "bad fifo_len %llu\n", fifo_len);
>
> I'd tell user "unsupported" rather than "bad".

The word, unsupported, sounds better.  I will change it in both places you 
suggested.

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing REG_LAYOUT param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	v = readq(dfh_base + off);
>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>> +
>> +	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
>> +		FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
>
> Why not use reg_width directly?

Good catch.

>
>> +	switch (reg_width) {
>> +	case 4:
>> +		uart->port.iotype = UPIO_MEM32;
>> +		break;
>> +
>> +	case 2:
>> +		uart->port.iotype = UPIO_MEM16;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev, "invalid reg_width %lld\n", reg_width);
>
> unsupported ?
>
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	struct uart_8250_port uart;
>> +	struct dfl_uart *dfluart;
>> +	resource_size_t res_size;
>> +	void __iomem *dfh_base;
>> +	int ret;
>> +
>> +	memset(&uart, 0, sizeof(uart));
>> +	uart.port.flags = UPF_IOREMAP;
>> +	uart.port.mapbase = dfl_dev->csr_res.start;
>> +	uart.port.mapsize = resource_size(&dfl_dev->csr_res);
>> +
>> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
>> +	if (!dfluart)
>> +		return -ENOMEM;
>> +
>> +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> +	if (IS_ERR(dfh_base))
>> +		return PTR_ERR(dfh_base);
>> +
>> +	res_size = resource_size(&dfl_dev->mmio_res);
>> +
>> +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>> +
>> +	devm_iounmap(dev, dfh_base);
>> +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>> +
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
>> +
>> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
>> +
>> +	if (dfl_dev->num_irqs == 1)
>> +		uart.port.irq = dfl_dev->irqs[0];
>> +
>> +	/* register the port */
>
> This comment is pretty useless. Just drop it.

Will drop this useless comment.

>
>> +	dfluart->line = serial8250_register_8250_port(&uart);
>> +	if (dfluart->line < 0)
>> +		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
>> +
>> +	dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
>
> This you want to drop too. It seems a debug thing rather than info level
> stuff.

It is actually redundant output because serial8250_register_8250_port() 
produces useful output.  I will drop the line.

>
>
> -- 
> i.
>
>

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Marco Pagani]

On 2022-10-04 16:37, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> v3: use passed in location of registers
>     use cleaned up functions for parsing parameters
> 
> v2: clean up error messages
>     alphabetize header files
>     fix 'missing prototype' error by making function static
>     tried to sort Makefile and Kconfig better
> ---
>  drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |   9 ++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  3 files changed, 187 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_dfl.c
> 
> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
> new file mode 100644
> index 000000000000..110ad3a73459
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA UART
> + *
> + * Copyright (C) 2022 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Ananda Ravuri <ananda.ravuri@intel.com>
> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/dfl.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +
> +struct dfl_uart {
> +	int line;
> +};
> +
> +static int dfl_uart_get_params(struct device *dev, void __iomem *dfh_base, resource_size_t max,
> +			       struct uart_8250_port *uart)
> +{
> +	u64 v, fifo_len, reg_width;
> +	int off;
> +
> +	if (!dfhv1_has_params(dfh_base)) {
> +		dev_err(dev, "missing required DFH parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> +	if (off < 0) {
> +		dev_err(dev, "missing CLK_FRQ param\n");
> +		return -EINVAL;
> +	}
> +
> +	uart->port.uartclk = readq(dfh_base + off);
> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> +
> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> +	if (off < 0) {
> +		dev_err(dev, "missing FIFO_LEN param\n");
> +		return -EINVAL;
> +	}
> +
> +	fifo_len = readq(dfh_base + off);
> +	dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
> +
> +	switch (fifo_len) {
> +	case 32:
> +		uart->port.type = PORT_ALTR_16550_F32;
> +		break;
> +
> +	case 64:
> +		uart->port.type = PORT_ALTR_16550_F64;
> +		break;
> +
> +	case 128:
> +		uart->port.type = PORT_ALTR_16550_F128;
> +		break;
> +
> +	default:
> +		dev_err(dev, "bad fifo_len %llu\n", fifo_len);
> +		return -EINVAL;
> +	}
> +
> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (off < 0) {
> +		dev_err(dev, "missing REG_LAYOUT param\n");
> +		return -EINVAL;
> +	}
> +
> +	v = readq(dfh_base + off);
> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> +
> +	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
> +		FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
> +
> +	switch (reg_width) {
> +	case 4:
> +		uart->port.iotype = UPIO_MEM32;
> +		break;
> +
> +	case 2:
> +		uart->port.iotype = UPIO_MEM16;
> +		break;
> +
> +	default:
> +		dev_err(dev, "invalid reg_width %lld\n", reg_width);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	struct uart_8250_port uart;
> +	struct dfl_uart *dfluart;
> +	resource_size_t res_size;
> +	void __iomem *dfh_base;
> +	int ret;
> +
> +	memset(&uart, 0, sizeof(uart));
> +	uart.port.flags = UPF_IOREMAP;
> +	uart.port.mapbase = dfl_dev->csr_res.start;
> +	uart.port.mapsize = resource_size(&dfl_dev->csr_res);
> +
> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> +	if (!dfluart)
> +		return -ENOMEM;
> +
> +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +	if (IS_ERR(dfh_base))
> +		return PTR_ERR(dfh_base);
> +
> +	res_size = resource_size(&dfl_dev->mmio_res);
> +
> +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);


It seems to me that the dfl_uart driver supports only DFHv1 headers.
So why not checking dfl_dev->dfh_version in dfl_uart_probe() before
allocating, mapping, and then checking with dfl_uart_get_params()?


> +
> +	devm_iounmap(dev, dfh_base);
> +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
> +
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
> +
> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
> +
> +	if (dfl_dev->num_irqs == 1)
> +		uart.port.irq = dfl_dev->irqs[0];
> +
> +	/* register the port */
> +	dfluart->line = serial8250_register_8250_port(&uart);
> +	if (dfluart->line < 0)
> +		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
> +
> +	dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
> +	dev_set_drvdata(dev, dfluart);
> +
> +	return 0;
> +}
> +
> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
> +{
> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
> +
> +	if (dfluart->line >= 0)
> +		serial8250_unregister_port(dfluart->line);
> +}
> +
> +#define FME_FEATURE_ID_UART 0x24
> +
> +static const struct dfl_device_id dfl_uart_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_UART },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
> +
> +static struct dfl_driver dfl_uart_driver = {
> +	.drv = {
> +		.name = "dfl-uart",
> +	},
> +	.id_table = dfl_uart_ids,
> +	.probe = dfl_uart_probe,
> +	.remove = dfl_uart_remove,
> +};
> +module_dfl_driver(dfl_uart_driver);
> +
> +MODULE_DESCRIPTION("DFL Intel UART driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index d0b49e15fbf5..5c6497ce5c12 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
>  
>  	  If unsure, say N.
>  
> +config SERIAL_8250_DFL
> +	tristate "DFL bus driver for Altera 16550 UART"
> +	depends on SERIAL_8250 && FPGA_DFL
> +	help
> +	  This option enables support for a Device Feature List (DFL) bus
> +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
> +	  can be instantiated in a FPGA and then be discovered during
> +	  enumeration of the DFL bus.
> +
>  config SERIAL_8250_FSL
>  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
>  	depends on SERIAL_8250_CONSOLE
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index bee908f99ea0..32006e0982d1 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE)	+= 8250_early.o
>  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
>  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o


Thanks,
Marco

Re: [PATCH v3 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

On Mon, 10 Oct 2022, Marco Pagani wrote:

>
> On 2022-10-04 16:37, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> Reported-by: kernel test robot <lkp@intel.com>
>> ---
>> v3: use passed in location of registers
>>     use cleaned up functions for parsing parameters
>>
>> v2: clean up error messages
>>     alphabetize header files
>>     fix 'missing prototype' error by making function static
>>     tried to sort Makefile and Kconfig better
>> ---
>>  drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
>>  drivers/tty/serial/8250/Kconfig    |   9 ++
>>  drivers/tty/serial/8250/Makefile   |   1 +
>>  3 files changed, 187 insertions(+)
>>  create mode 100644 drivers/tty/serial/8250/8250_dfl.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
>> new file mode 100644
>> index 000000000000..110ad3a73459
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,177 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for FPGA UART
>> + *
>> + * Copyright (C) 2022 Intel Corporation, Inc.
>> + *
>> + * Authors:
>> + *   Ananda Ravuri <ananda.ravuri@intel.com>
>> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/dfl.h>
>> +#include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/serial.h>
>> +#include <linux/serial_8250.h>
>> +
>> +struct dfl_uart {
>> +	int line;
>> +};
>> +
>> +static int dfl_uart_get_params(struct device *dev, void __iomem *dfh_base, resource_size_t max,
>> +			       struct uart_8250_port *uart)
>> +{
>> +	u64 v, fifo_len, reg_width;
>> +	int off;
>> +
>> +	if (!dfhv1_has_params(dfh_base)) {
>> +		dev_err(dev, "missing required DFH parameters\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_CLK_FRQ);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing CLK_FRQ param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	uart->port.uartclk = readq(dfh_base + off);
>> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>> +
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_FIFO_LEN);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing FIFO_LEN param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	fifo_len = readq(dfh_base + off);
>> +	dev_dbg(dev, "UART_FIFO_ID fifo_len %llu\n", fifo_len);
>> +
>> +	switch (fifo_len) {
>> +	case 32:
>> +		uart->port.type = PORT_ALTR_16550_F32;
>> +		break;
>> +
>> +	case 64:
>> +		uart->port.type = PORT_ALTR_16550_F64;
>> +		break;
>> +
>> +	case 128:
>> +		uart->port.type = PORT_ALTR_16550_F128;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev, "bad fifo_len %llu\n", fifo_len);
>> +		return -EINVAL;
>> +	}
>> +
>> +	off = dfhv1_find_param(dfh_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
>> +	if (off < 0) {
>> +		dev_err(dev, "missing REG_LAYOUT param\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	v = readq(dfh_base + off);
>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>> +
>> +	dev_dbg(dev, "UART_LAYOUT_ID width %lld shift %d\n",
>> +		FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v), (int)uart->port.regshift);
>> +
>> +	switch (reg_width) {
>> +	case 4:
>> +		uart->port.iotype = UPIO_MEM32;
>> +		break;
>> +
>> +	case 2:
>> +		uart->port.iotype = UPIO_MEM16;
>> +		break;
>> +
>> +	default:
>> +		dev_err(dev, "invalid reg_width %lld\n", reg_width);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	struct uart_8250_port uart;
>> +	struct dfl_uart *dfluart;
>> +	resource_size_t res_size;
>> +	void __iomem *dfh_base;
>> +	int ret;
>> +
>> +	memset(&uart, 0, sizeof(uart));
>> +	uart.port.flags = UPF_IOREMAP;
>> +	uart.port.mapbase = dfl_dev->csr_res.start;
>> +	uart.port.mapsize = resource_size(&dfl_dev->csr_res);
>> +
>> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
>> +	if (!dfluart)
>> +		return -ENOMEM;
>> +
>> +	dfh_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> +	if (IS_ERR(dfh_base))
>> +		return PTR_ERR(dfh_base);
>> +
>> +	res_size = resource_size(&dfl_dev->mmio_res);
>> +
>> +	ret = dfl_uart_get_params(dev, dfh_base, res_size, &uart);
>
>
> It seems to me that the dfl_uart driver supports only DFHv1 headers.
> So why not checking dfl_dev->dfh_version in dfl_uart_probe() before
> allocating, mapping, and then checking with dfl_uart_get_params()?

Checking dfl_dev->dfh_version at the top of dfl_uart_probe() is a good 
suggestion.  I can also move the call to devm_kzalloc until after the call 
the dfl_uart_get_params.


>
>
>> +
>> +	devm_iounmap(dev, dfh_base);
>> +	devm_release_mem_region(dev, dfl_dev->mmio_res.start, res_size);
>> +
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
>> +
>> +	dev_dbg(dev, "nr_irqs %d %p\n", dfl_dev->num_irqs, dfl_dev->irqs);
>> +
>> +	if (dfl_dev->num_irqs == 1)
>> +		uart.port.irq = dfl_dev->irqs[0];
>> +
>> +	/* register the port */
>> +	dfluart->line = serial8250_register_8250_port(&uart);
>> +	if (dfluart->line < 0)
>> +		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
>> +
>> +	dev_info(dev, "serial8250_register_8250_port %d\n", dfluart->line);
>> +	dev_set_drvdata(dev, dfluart);
>> +
>> +	return 0;
>> +}
>> +
>> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
>> +{
>> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
>> +
>> +	if (dfluart->line >= 0)
>> +		serial8250_unregister_port(dfluart->line);
>> +}
>> +
>> +#define FME_FEATURE_ID_UART 0x24
>> +
>> +static const struct dfl_device_id dfl_uart_ids[] = {
>> +	{ FME_ID, FME_FEATURE_ID_UART },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
>> +
>> +static struct dfl_driver dfl_uart_driver = {
>> +	.drv = {
>> +		.name = "dfl-uart",
>> +	},
>> +	.id_table = dfl_uart_ids,
>> +	.probe = dfl_uart_probe,
>> +	.remove = dfl_uart_remove,
>> +};
>> +module_dfl_driver(dfl_uart_driver);
>> +
>> +MODULE_DESCRIPTION("DFL Intel UART driver");
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>> index d0b49e15fbf5..5c6497ce5c12 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
>>
>>  	  If unsure, say N.
>>
>> +config SERIAL_8250_DFL
>> +	tristate "DFL bus driver for Altera 16550 UART"
>> +	depends on SERIAL_8250 && FPGA_DFL
>> +	help
>> +	  This option enables support for a Device Feature List (DFL) bus
>> +	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
>> +	  can be instantiated in a FPGA and then be discovered during
>> +	  enumeration of the DFL bus.
>> +
>>  config SERIAL_8250_FSL
>>  	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
>>  	depends on SERIAL_8250_CONSOLE
>> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
>> index bee908f99ea0..32006e0982d1 100644
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE)	+= 8250_early.o
>>  obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
>>  obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
>>  obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
>> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>>  obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>
>
> Thanks,
> Marco
>
>