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

[PATCH v4 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 DFHv1
  tty: serial: 8250: add DFL bus driver for Altera 16550.

 Documentation/fpga/dfl.rst         |  96 ++++++++++++
 drivers/fpga/dfl.c                 | 234 +++++++++++++++++++++++------
 drivers/fpga/dfl.h                 |  38 +++++
 drivers/tty/serial/8250/8250_dfl.c | 149 ++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 include/linux/dfl.h                |  15 ++
 7 files changed, 496 insertions(+), 49 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

-- 
2.25.1

[PATCH v4 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>
---
v4: Remove marketing speak and separate v0 and v1 descriptions.
    Fix errors reported by "make htmldocs".

v3: no change

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

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 15b670926084..12365be435a8 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -561,6 +561,102 @@ new DFL feature via UIO direct access, its feature id should be added to the
 driver's id_table.
 
 
+Device Feature Header - Version 0
+===========================================
+The format of Version 0 of a Device Feature Header (DFH) is shown below::
+
+    +-----------------------------------------------------------------------+
+    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_L                             0| 0x08
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_H                             0| 0x10
+    +-----------------------------------------------------------------------+
+
+Offset 0x00
+Type - The type of DFH (e.g. FME, AFU, or private feature).
+DFH VER - The version of the DFH.
+Rsvd - Currently unused.
+EOL - Set if this DFH is the end of the Device Feature List (DFL).
+Next - The offset of the next DFH in the DFL from the start of the DFH.
+If EOL is set, Next refers to size of mmio for last feature in the list.
+ID - If Type field is 'private feature', then ID of the private feature.
+
+Offset 0x08
+GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
+if Type is FME or AFU.
+
+Offset 0x10
+GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
+if Type is FME or AFU.
+
+
+Device Feature Header - Version 1
+===========================================
+The format of Version 1 of a Device Feature Header (DFH) is shown below::
+
+    +-----------------------------------------------------------------------+
+    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_L                             0| 0x08
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_H                             0| 0x10
+    +-----------------------------------------------------------------------+
+    |63                 Address/Offset                            1|  Rel  0| 0x18
+    +-----------------------------------------------------------------------+
+    |63        Reg Size       32|Params 31|30 Group    16|15 Instance      0| 0x20
+    +-----------------------------------------------------------------------+
+    |63 Next      34|RSV33|EOP32|31 Param Version 16|15 Param ID           0| 0x28
+    +-----------------------------------------------------------------------+
+    |63                 Parameter Data                                     0| 0x30
+    +-----------------------------------------------------------------------+
+
+                                  ...
+
+    +-----------------------------------------------------------------------+
+    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
+    +-----------------------------------------------------------------------+
+    |63                 Parameter Data                                     0|
+    +-----------------------------------------------------------------------+
+
+Offset 0x00
+Type - The type of DFH (e.g. FME, AFU, or private feature).
+DFH VER - The version of the DFH.
+Rsvd - Currently unused.
+EOL - Set if this DFH is the end of the Device Feature List (DFL).
+Next - The offset of the next DFH in the DFL from the start of the DFH.
+If EOL is set, Next refers to size of mmio for last feature in the list.
+ID - If Type field is 'private feature', then ID of the private feature.
+
+Offset 0x08
+GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
+
+Offset 0x10
+GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
+if Type is FME or AFU.
+
+Offset 0x18
+Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
+absolute address for the location of the feature's registers.
+If Rel bit is clear, then the feature's registers start at the
+offset from the start of the DFH.
+
+Offset 0x20
+Reg Size - Size of feature's register set.
+Params - Set if DFH has one or more parameter blocks.
+Group - Id of group if feature is part of a group.
+Instance - Id of instance of feature within a group.
+
+Offset 0x28 if feature has parameters
+Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
+If EOP set, size of last parameter.
+Param Version - Version of Param ID.
+Param ID - ID of parameter.
+
+Offset 0x30
+Parameter Data - Parameter data whose size and format is defined by version
+and ID of the parameter.
+
 Open discussion
 ===============
 FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
-- 
2.25.1

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

[Bagas Sanjaya]

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

On Thu, Oct 20, 2022 at 02:26:07PM -0700, matthew.gerlach@linux.intel.com wrote:
> +Device Feature Header - Version 0
> +===========================================
> +The format of Version 0 of a Device Feature Header (DFH) is shown below::
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0| 0x08
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0| 0x10
> +    +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +
> +Device Feature Header - Version 1
> +===========================================
> +The format of Version 1 of a Device Feature Header (DFH) is shown below::
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0| 0x08
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0| 0x10
> +    +-----------------------------------------------------------------------+
> +    |63                 Address/Offset                            1|  Rel  0| 0x18
> +    +-----------------------------------------------------------------------+
> +    |63        Reg Size       32|Params 31|30 Group    16|15 Instance      0| 0x20
> +    +-----------------------------------------------------------------------+
> +    |63 Next      34|RSV33|EOP32|31 Param Version 16|15 Param ID           0| 0x28
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0| 0x30
> +    +-----------------------------------------------------------------------+
> +
> +                                  ...
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +Offset 0x18
> +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> +absolute address for the location of the feature's registers.
> +If Rel bit is clear, then the feature's registers start at the
> +offset from the start of the DFH.
> +
> +Offset 0x20
> +Reg Size - Size of feature's register set.
> +Params - Set if DFH has one or more parameter blocks.
> +Group - Id of group if feature is part of a group.
> +Instance - Id of instance of feature within a group.
> +
> +Offset 0x28 if feature has parameters
> +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> +If EOP set, size of last parameter.
> +Param Version - Version of Param ID.
> +Param ID - ID of parameter.
> +
> +Offset 0x30
> +Parameter Data - Parameter data whose size and format is defined by version
> +and ID of the parameter.
> +

The offset fields list should be formatted with nested list (with
prose improv):

---- >8 ----

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 12365be435a812..9c19ee62d4ac44 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -573,22 +573,27 @@ The format of Version 0 of a Device Feature Header (DFH) is shown below::
     |63                                 GUID_H                             0| 0x10
     +-----------------------------------------------------------------------+
 
-Offset 0x00
-Type - The type of DFH (e.g. FME, AFU, or private feature).
-DFH VER - The version of the DFH.
-Rsvd - Currently unused.
-EOL - Set if this DFH is the end of the Device Feature List (DFL).
-Next - The offset of the next DFH in the DFL from the start of the DFH.
-If EOL is set, Next refers to size of mmio for last feature in the list.
-ID - If Type field is 'private feature', then ID of the private feature.
+The fields are:
 
-Offset 0x08
-GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
-if Type is FME or AFU.
+  * Offset 0x00
 
-Offset 0x10
-GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
-if Type is FME or AFU.
+    * Type - The type of DFH (e.g. FME, AFU, or private feature).
+    * DFH VER - The version of the DFH.
+    * Rsvd - Currently unused.
+    * EOL - Set if this DFH is the end of the Device Feature List (DFL).
+
+    * Next - The offset of the next DFH in the DFL from the start of the DFH.
+      If EOL is set, Next refers to size of mmio for last feature in the list.
+
+    * ID - Private feature ID if Type is private feature.
+
+  * Offset 0x08
+
+    * GUID_L - Least significant half of a 128-bit GUID if Type is FME or AFU.
+
+  * Offset 0x10
+
+    * GUID_H - Most significant half of a 128-bit GUID if Type if FME or AFU.
 
 
 Device Feature Header - Version 1
@@ -619,43 +624,53 @@ The format of Version 1 of a Device Feature Header (DFH) is shown below::
     |63                 Parameter Data                                     0|
     +-----------------------------------------------------------------------+
 
-Offset 0x00
-Type - The type of DFH (e.g. FME, AFU, or private feature).
-DFH VER - The version of the DFH.
-Rsvd - Currently unused.
-EOL - Set if this DFH is the end of the Device Feature List (DFL).
-Next - The offset of the next DFH in the DFL from the start of the DFH.
-If EOL is set, Next refers to size of mmio for last feature in the list.
-ID - If Type field is 'private feature', then ID of the private feature.
+The fields are:
 
-Offset 0x08
-GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
+  * Offset 0x00
 
-Offset 0x10
-GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
-if Type is FME or AFU.
+    * Type - The type of DFH (e.g. FME, AFU, or private feature).
+    * DFH VER - The version of the DFH.
+    * Rsvd - Currently unused.
+    * EOL - Set if this DFH is the end of the Device Feature List (DFL).
 
-Offset 0x18
-Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
-absolute address for the location of the feature's registers.
-If Rel bit is clear, then the feature's registers start at the
-offset from the start of the DFH.
+    * Next - The offset of the next DFH in the DFL from the start of the DFH.
+      If EOL is set, Next refers to size of mmio for last feature in the list.
 
-Offset 0x20
-Reg Size - Size of feature's register set.
-Params - Set if DFH has one or more parameter blocks.
-Group - Id of group if feature is part of a group.
-Instance - Id of instance of feature within a group.
+    * ID - Private feature ID if Type is private feature.
 
-Offset 0x28 if feature has parameters
-Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
-If EOP set, size of last parameter.
-Param Version - Version of Param ID.
-Param ID - ID of parameter.
+  * Offset 0x08
 
-Offset 0x30
-Parameter Data - Parameter data whose size and format is defined by version
-and ID of the parameter.
+    * GUID_L - Least significant half of a 128-bit GUID if Type is FME or APU.
+
+  * Offset 0x10
+
+    * GUID_H - Most significant half of a 128-bit GUID if Type is FME or AFU.
+
+  * Offset 0x18
+
+    * Address/Offset - If Rel bit is set, upper 63 bits of a 16-bit aligned
+      absolute address for the location of feature registers; otherwise
+      registers of the feature start at the offset from the start of the DFH.
+
+  * Offset 0x20
+
+    * Reg Size - Size of register set of the feature.
+    * Params - Set if DFH has one or more parameter blocks.
+    * Group - ID of group if the feature is part of a group.
+    * Instance - ID of instance of the feature within a group.
+
+  * Offset 0x28 (if the feature has parameters)
+
+    * Next - Upper 30 bits of a 32-bit aligned offset to the next parameter
+      block. If EOP is set, size of last parameter.
+
+    * Param Version - Version of Param ID.
+    * Param ID - ID of parameter.
+
+  * Offset 0x30 (if the feature has parameters)
+
+    * Parameter Data - Parameter data whose size and format is defined by
+      version and ID of the parameter.
 
 Open discussion
 ===============

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[matthew.gerlach]

On Fri, 21 Oct 2022, Bagas Sanjaya wrote:

> On Thu, Oct 20, 2022 at 02:26:07PM -0700, matthew.gerlach@linux.intel.com wrote:
>> +Device Feature Header - Version 0
>> +===========================================
>> +The format of Version 0 of a Device Feature Header (DFH) is shown below::
>> +
>> +    +-----------------------------------------------------------------------+
>> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_L                             0| 0x08
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_H                             0| 0x10
>> +    +-----------------------------------------------------------------------+
>> +
>> +Offset 0x00
>> +Type - The type of DFH (e.g. FME, AFU, or private feature).
>> +DFH VER - The version of the DFH.
>> +Rsvd - Currently unused.
>> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
>> +Next - The offset of the next DFH in the DFL from the start of the DFH.
>> +If EOL is set, Next refers to size of mmio for last feature in the list.
>> +ID - If Type field is 'private feature', then ID of the private feature.
>> +
>> +Offset 0x08
>> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
>> +if Type is FME or AFU.
>> +
>> +Offset 0x10
>> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
>> +if Type is FME or AFU.
>> +
>> +
>> +Device Feature Header - Version 1
>> +===========================================
>> +The format of Version 1 of a Device Feature Header (DFH) is shown below::
>> +
>> +    +-----------------------------------------------------------------------+
>> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_L                             0| 0x08
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_H                             0| 0x10
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Address/Offset                            1|  Rel  0| 0x18
>> +    +-----------------------------------------------------------------------+
>> +    |63        Reg Size       32|Params 31|30 Group    16|15 Instance      0| 0x20
>> +    +-----------------------------------------------------------------------+
>> +    |63 Next      34|RSV33|EOP32|31 Param Version 16|15 Param ID           0| 0x28
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data                                     0| 0x30
>> +    +-----------------------------------------------------------------------+
>> +
>> +                                  ...
>> +
>> +    +-----------------------------------------------------------------------+
>> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data                                     0|
>> +    +-----------------------------------------------------------------------+
>> +
>> +Offset 0x00
>> +Type - The type of DFH (e.g. FME, AFU, or private feature).
>> +DFH VER - The version of the DFH.
>> +Rsvd - Currently unused.
>> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
>> +Next - The offset of the next DFH in the DFL from the start of the DFH.
>> +If EOL is set, Next refers to size of mmio for last feature in the list.
>> +ID - If Type field is 'private feature', then ID of the private feature.
>> +
>> +Offset 0x08
>> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
>> +
>> +Offset 0x10
>> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
>> +if Type is FME or AFU.
>> +
>> +Offset 0x18
>> +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
>> +absolute address for the location of the feature's registers.
>> +If Rel bit is clear, then the feature's registers start at the
>> +offset from the start of the DFH.
>> +
>> +Offset 0x20
>> +Reg Size - Size of feature's register set.
>> +Params - Set if DFH has one or more parameter blocks.
>> +Group - Id of group if feature is part of a group.
>> +Instance - Id of instance of feature within a group.
>> +
>> +Offset 0x28 if feature has parameters
>> +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
>> +If EOP set, size of last parameter.
>> +Param Version - Version of Param ID.
>> +Param ID - ID of parameter.
>> +
>> +Offset 0x30
>> +Parameter Data - Parameter data whose size and format is defined by version
>> +and ID of the parameter.
>> +
>
> The offset fields list should be formatted with nested list (with
> prose improv):
>
> ---- >8 ----

Thanks for the good suggestion.

>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 12365be435a812..9c19ee62d4ac44 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -573,22 +573,27 @@ The format of Version 0 of a Device Feature Header (DFH) is shown below::
>     |63                                 GUID_H                             0| 0x10
>     +-----------------------------------------------------------------------+
>
> -Offset 0x00
> -Type - The type of DFH (e.g. FME, AFU, or private feature).
> -DFH VER - The version of the DFH.
> -Rsvd - Currently unused.
> -EOL - Set if this DFH is the end of the Device Feature List (DFL).
> -Next - The offset of the next DFH in the DFL from the start of the DFH.
> -If EOL is set, Next refers to size of mmio for last feature in the list.
> -ID - If Type field is 'private feature', then ID of the private feature.
> +The fields are:
>
> -Offset 0x08
> -GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> -if Type is FME or AFU.
> +  * Offset 0x00
>
> -Offset 0x10
> -GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> -if Type is FME or AFU.
> +    * Type - The type of DFH (e.g. FME, AFU, or private feature).
> +    * DFH VER - The version of the DFH.
> +    * Rsvd - Currently unused.
> +    * EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +
> +    * Next - The offset of the next DFH in the DFL from the start of the DFH.
> +      If EOL is set, Next refers to size of mmio for last feature in the list.
> +
> +    * ID - Private feature ID if Type is private feature.
> +
> +  * Offset 0x08
> +
> +    * GUID_L - Least significant half of a 128-bit GUID if Type is FME or AFU.
> +
> +  * Offset 0x10
> +
> +    * GUID_H - Most significant half of a 128-bit GUID if Type if FME or AFU.
>
>
> Device Feature Header - Version 1
> @@ -619,43 +624,53 @@ The format of Version 1 of a Device Feature Header (DFH) is shown below::
>     |63                 Parameter Data                                     0|
>     +-----------------------------------------------------------------------+
>
> -Offset 0x00
> -Type - The type of DFH (e.g. FME, AFU, or private feature).
> -DFH VER - The version of the DFH.
> -Rsvd - Currently unused.
> -EOL - Set if this DFH is the end of the Device Feature List (DFL).
> -Next - The offset of the next DFH in the DFL from the start of the DFH.
> -If EOL is set, Next refers to size of mmio for last feature in the list.
> -ID - If Type field is 'private feature', then ID of the private feature.
> +The fields are:
>
> -Offset 0x08
> -GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> +  * Offset 0x00
>
> -Offset 0x10
> -GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> -if Type is FME or AFU.
> +    * Type - The type of DFH (e.g. FME, AFU, or private feature).
> +    * DFH VER - The version of the DFH.
> +    * Rsvd - Currently unused.
> +    * EOL - Set if this DFH is the end of the Device Feature List (DFL).
>
> -Offset 0x18
> -Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> -absolute address for the location of the feature's registers.
> -If Rel bit is clear, then the feature's registers start at the
> -offset from the start of the DFH.
> +    * Next - The offset of the next DFH in the DFL from the start of the DFH.
> +      If EOL is set, Next refers to size of mmio for last feature in the list.
>
> -Offset 0x20
> -Reg Size - Size of feature's register set.
> -Params - Set if DFH has one or more parameter blocks.
> -Group - Id of group if feature is part of a group.
> -Instance - Id of instance of feature within a group.
> +    * ID - Private feature ID if Type is private feature.
>
> -Offset 0x28 if feature has parameters
> -Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> -If EOP set, size of last parameter.
> -Param Version - Version of Param ID.
> -Param ID - ID of parameter.
> +  * Offset 0x08
>
> -Offset 0x30
> -Parameter Data - Parameter data whose size and format is defined by version
> -and ID of the parameter.
> +    * GUID_L - Least significant half of a 128-bit GUID if Type is FME or APU.
> +
> +  * Offset 0x10
> +
> +    * GUID_H - Most significant half of a 128-bit GUID if Type is FME or AFU.
> +
> +  * Offset 0x18
> +
> +    * Address/Offset - If Rel bit is set, upper 63 bits of a 16-bit aligned
> +      absolute address for the location of feature registers; otherwise
> +      registers of the feature start at the offset from the start of the DFH.
> +
> +  * Offset 0x20
> +
> +    * Reg Size - Size of register set of the feature.
> +    * Params - Set if DFH has one or more parameter blocks.
> +    * Group - ID of group if the feature is part of a group.
> +    * Instance - ID of instance of the feature within a group.
> +
> +  * Offset 0x28 (if the feature has parameters)
> +
> +    * Next - Upper 30 bits of a 32-bit aligned offset to the next parameter
> +      block. If EOP is set, size of last parameter.
> +
> +    * Param Version - Version of Param ID.
> +    * Param ID - ID of parameter.
> +
> +  * Offset 0x30 (if the feature has parameters)
> +
> +    * Parameter Data - Parameter data whose size and format is defined by
> +      version and ID of the parameter.
>
> Open discussion
> ===============
>
> Thanks.
>
> -- 
> An old man doll... just what I always wanted! - Clara
>

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

[Ilpo Järvinen]

On Thu, 20 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>
> ---
> v4: Remove marketing speak and separate v0 and v1 descriptions.
>     Fix errors reported by "make htmldocs".
> 
> v3: no change
> 
> v2: s/GUILD/GUID/
>     add picture
> ---
>  Documentation/fpga/dfl.rst | 96 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 15b670926084..12365be435a8 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -561,6 +561,102 @@ new DFL feature via UIO direct access, its feature id should be added to the
>  driver's id_table.
>  
>  
> +Device Feature Header - Version 0
> +===========================================
> +The format of Version 0 of a Device Feature Header (DFH) is shown below::
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0| 0x08
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0| 0x10
> +    +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.
> +
> +
> +Device Feature Header - Version 1
> +===========================================

While this is structurally better than the previous one. I'd still include
at least one paragraph about the purpose. Something along these lines 
(picked from v3 + edited the marketing speak/v0 compare away from it):

Version 1 of the Device Feature Header (DFHv1) provides flexibility and 
extensibility to hardware designs using Device Feature Lists. It is a
standardized mechanism for features to describe parameters/capabilities to 
software.

With DFHv1:
* GUID is mandatory for all types
* The register space of the feature is decoupled from the location of the DFH
* A list of parameter values associates to a particular feature.

After that, the header itself makes much more sense already.

> +The format of Version 1 of a Device Feature Header (DFH) is shown below::
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0| 0x08
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0| 0x10
> +    +-----------------------------------------------------------------------+
> +    |63                 Address/Offset                            1|  Rel  0| 0x18

Should this mention it's addr/offs of registers? As is it's bit hard to 
figure out from the diagram w/o the extra description. I think you have 
plenty of space for adding that extra bit of info.

> +    +-----------------------------------------------------------------------+
> +    |63        Reg Size       32|Params 31|30 Group    16|15 Instance      0| 0x20
> +    +-----------------------------------------------------------------------+
> +    |63 Next      34|RSV33|EOP32|31 Param Version 16|15 Param ID           0| 0x28
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0| 0x30
> +    +-----------------------------------------------------------------------+
> +
> +                                  ...
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
> +Offset 0x00
> +Type - The type of DFH (e.g. FME, AFU, or private feature).
> +DFH VER - The version of the DFH.
> +Rsvd - Currently unused.
> +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> +Next - The offset of the next DFH in the DFL from the start of the DFH.
> +If EOL is set, Next refers to size of mmio for last feature in the list.
> +ID - If Type field is 'private feature', then ID of the private feature.
> +
> +Offset 0x08
> +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> +
> +Offset 0x10
> +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> +if Type is FME or AFU.

A copy-paste error?

> +
> +Offset 0x18
> +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> +absolute address for the location of the feature's registers.
> +If Rel bit is clear, then the feature's registers start at the
> +offset from the start of the DFH.
> +
> +Offset 0x20
> +Reg Size - Size of feature's register set.
> +Params - Set if DFH has one or more parameter blocks.
> +Group - Id of group if feature is part of a group.
> +Instance - Id of instance of feature within a group.
> +
> +Offset 0x28 if feature has parameters
> +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> +If EOP set, size of last parameter.
> +Param Version - Version of Param ID.
> +Param ID - ID of parameter.
> +
> +Offset 0x30
> +Parameter Data - Parameter data whose size and format is defined by version
> +and ID of the parameter.

I'd reverse the order and say "ID and version" (kind of major thing 
first).

> +
>  Open discussion
>  ===============
>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> 

-- 
 i.

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

[Ilpo Järvinen]

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

On Fri, 21 Oct 2022, Ilpo Järvinen wrote:

> On Thu, 20 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>
> > ---
> > v4: Remove marketing speak and separate v0 and v1 descriptions.
> >     Fix errors reported by "make htmldocs".
> > 
> > v3: no change
> > 
> > v2: s/GUILD/GUID/
> >     add picture
> > ---
> >  Documentation/fpga/dfl.rst | 96 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> > 
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 15b670926084..12365be435a8 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -561,6 +561,102 @@ new DFL feature via UIO direct access, its feature id should be added to the
> >  driver's id_table.
> >  
> >  
> > +Device Feature Header - Version 0
> > +===========================================
> > +The format of Version 0 of a Device Feature Header (DFH) is shown below::
> > +
> > +    +-----------------------------------------------------------------------+
> > +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> > +    +-----------------------------------------------------------------------+
> > +    |63                                 GUID_L                             0| 0x08
> > +    +-----------------------------------------------------------------------+
> > +    |63                                 GUID_H                             0| 0x10
> > +    +-----------------------------------------------------------------------+
> > +
> > +Offset 0x00
> > +Type - The type of DFH (e.g. FME, AFU, or private feature).
> > +DFH VER - The version of the DFH.
> > +Rsvd - Currently unused.
> > +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> > +Next - The offset of the next DFH in the DFL from the start of the DFH.
> > +If EOL is set, Next refers to size of mmio for last feature in the list.
> > +ID - If Type field is 'private feature', then ID of the private feature.
> > +
> > +Offset 0x08
> > +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier
> > +if Type is FME or AFU.
> > +
> > +Offset 0x10
> > +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> > +if Type is FME or AFU.
> > +
> > +
> > +Device Feature Header - Version 1
> > +===========================================
> 
> While this is structurally better than the previous one. I'd still include
> at least one paragraph about the purpose. Something along these lines 
> (picked from v3 + edited the marketing speak/v0 compare away from it):
> 
> Version 1 of the Device Feature Header (DFHv1) provides flexibility and 
> extensibility to hardware designs using Device Feature Lists. It is a
> standardized mechanism for features to describe parameters/capabilities to 
> software.
> 
> With DFHv1:
> * GUID is mandatory for all types
> * The register space of the feature is decoupled from the location of the DFH
> * A list of parameter values associates to a particular feature.
> 
> After that, the header itself makes much more sense already.
> 
> > +The format of Version 1 of a Device Feature Header (DFH) is shown below::
> > +
> > +    +-----------------------------------------------------------------------+
> > +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> > +    +-----------------------------------------------------------------------+
> > +    |63                                 GUID_L                             0| 0x08
> > +    +-----------------------------------------------------------------------+
> > +    |63                                 GUID_H                             0| 0x10
> > +    +-----------------------------------------------------------------------+
> > +    |63                 Address/Offset                            1|  Rel  0| 0x18
> 
> Should this mention it's addr/offs of registers? As is it's bit hard to 
> figure out from the diagram w/o the extra description. I think you have 
> plenty of space for adding that extra bit of info.
> 
> > +    +-----------------------------------------------------------------------+
> > +    |63        Reg Size       32|Params 31|30 Group    16|15 Instance      0| 0x20
> > +    +-----------------------------------------------------------------------+
> > +    |63 Next      34|RSV33|EOP32|31 Param Version 16|15 Param ID           0| 0x28
> > +    +-----------------------------------------------------------------------+
> > +    |63                 Parameter Data                                     0| 0x30
> > +    +-----------------------------------------------------------------------+
> > +
> > +                                  ...
> > +
> > +    +-----------------------------------------------------------------------+
> > +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|

Copy-paste error on the next field.

-- 
 i.

> > +    +-----------------------------------------------------------------------+
> > +    |63                 Parameter Data                                     0|
> > +    +-----------------------------------------------------------------------+

> > +
> > +Offset 0x00
> > +Type - The type of DFH (e.g. FME, AFU, or private feature).
> > +DFH VER - The version of the DFH.
> > +Rsvd - Currently unused.
> > +EOL - Set if this DFH is the end of the Device Feature List (DFL).
> > +Next - The offset of the next DFH in the DFL from the start of the DFH.
> > +If EOL is set, Next refers to size of mmio for last feature in the list.
> > +ID - If Type field is 'private feature', then ID of the private feature.
> > +
> > +Offset 0x08
> > +GUID_L - Least significant 64 bits of a 128 bit Globally Unique Identifier.
> > +
> > +Offset 0x10
> > +GUID_H - Most significant 64 bits of a 128 bit Globally Unique Identifier
> > +if Type is FME or AFU.
> 
> A copy-paste error?
> 
> > +
> > +Offset 0x18
> > +Address/Offset - If Rel bit is set, then high 63 bits of a 16 bit aligned
> > +absolute address for the location of the feature's registers.
> > +If Rel bit is clear, then the feature's registers start at the
> > +offset from the start of the DFH.
> > +
> > +Offset 0x20
> > +Reg Size - Size of feature's register set.
> > +Params - Set if DFH has one or more parameter blocks.
> > +Group - Id of group if feature is part of a group.
> > +Instance - Id of instance of feature within a group.
> > +
> > +Offset 0x28 if feature has parameters
> > +Next - High 30 bits of a 32 bit aligned offset to the next parameter block.
> > +If EOP set, size of last parameter.
> > +Param Version - Version of Param ID.
> > +Param ID - ID of parameter.
> > +
> > +Offset 0x30
> > +Parameter Data - Parameter data whose size and format is defined by version
> > +and ID of the parameter.
> 
> I'd reverse the order and say "ID and version" (kind of major thing 
> first).
> 
> > +
> >  Open discussion
> >  ===============
> >  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> > 
> 
> 

[PATCH v4 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>
---
v4: s/MSIX/MSI_X/g
    move kerneldoc to implementation
    don't change copyright date

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

dfl.h s/MSIX/MSI_X/g move kerneldoc
---
 drivers/fpga/dfl.h  | 33 +++++++++++++++++++++++++++++++++
 include/linux/dfl.h | 11 +++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..45e6e1359a67 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -74,11 +74,44 @@
 #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_HDR_NEXT_EOL	BIT_ULL(0)
+#define DFHv1_PARAM_HDR_NEXT_MASK	GENMASK_ULL(1, 0)
+#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..fea9e16d35b6 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -11,6 +11,17 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+#define DFHv1_PARAM_ID_MSI_X		0x1
+#define DFHv1_PARAM_MSI_X_STARTV	0x0
+#define DFHv1_PARAM_MSI_X_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 v4 2/4] fpga: dfl: Add DFHv1 Register Definitions

[Ilpo Järvinen]

On Thu, 20 Oct 2022, 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.
> 
> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v4: s/MSIX/MSI_X/g
>     move kerneldoc to implementation
>     don't change copyright date
> 
> 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
> 
> dfl.h s/MSIX/MSI_X/g move kerneldoc
> ---
>  drivers/fpga/dfl.h  | 33 +++++++++++++++++++++++++++++++++
>  include/linux/dfl.h | 11 +++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 06cfcd5e84bb..45e6e1359a67 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -74,11 +74,44 @@
>  #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 */

SIZE -> SZ would remove two letters w/o loss of info (remember to 
rename the offset too if you make this change).

> +/* 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_HDR_NEXT_EOL	BIT_ULL(0)
> +#define DFHv1_PARAM_HDR_NEXT_MASK	GENMASK_ULL(1, 0)
> +#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..fea9e16d35b6 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -11,6 +11,17 @@
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
>  
> +#define DFHv1_PARAM_ID_MSI_X		0x1
> +#define DFHv1_PARAM_MSI_X_STARTV	0x0
> +#define DFHv1_PARAM_MSI_X_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)

Any particular reason why MSI_X parameters are given as offsets and 
these REG_LAYOUT ones as bitfields (both are 32-bit)?

The naming here would indicate that DFHv1_PARAM_ID_REG_WIDTH is one of the 
parameters but it's part of param data instead. I suppose you'd want 
DFHv1_PARAM_REG_LAYOUT_WIDTH instead.

-- 
 i.

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

[matthew.gerlach]

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



On Fri, 21 Oct 2022, Ilpo Järvinen wrote:

> On Thu, 20 Oct 2022, 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.
>>
>> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v4: s/MSIX/MSI_X/g
>>     move kerneldoc to implementation
>>     don't change copyright date
>>
>> 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
>>
>> dfl.h s/MSIX/MSI_X/g move kerneldoc
>> ---
>>  drivers/fpga/dfl.h  | 33 +++++++++++++++++++++++++++++++++
>>  include/linux/dfl.h | 11 +++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 06cfcd5e84bb..45e6e1359a67 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -74,11 +74,44 @@
>>  #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 */
>
> SIZE -> SZ would remove two letters w/o loss of info (remember to
> rename the offset too if you make this change).
>
>> +/* 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_HDR_NEXT_EOL	BIT_ULL(0)
>> +#define DFHv1_PARAM_HDR_NEXT_MASK	GENMASK_ULL(1, 0)
>> +#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..fea9e16d35b6 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -11,6 +11,17 @@
>>  #include <linux/device.h>
>>  #include <linux/mod_devicetable.h>
>>
>> +#define DFHv1_PARAM_ID_MSI_X		0x1
>> +#define DFHv1_PARAM_MSI_X_STARTV	0x0
>> +#define DFHv1_PARAM_MSI_X_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)
>
> Any particular reason why MSI_X parameters are given as offsets and
> these REG_LAYOUT ones as bitfields (both are 32-bit)?

I agree that it would be much better to be consistent.

>
> The naming here would indicate that DFHv1_PARAM_ID_REG_WIDTH is one of the
> parameters but it's part of param data instead. I suppose you'd want
> DFHv1_PARAM_REG_LAYOUT_WIDTH instead.

Thanks for the naming suggestions.

>
> -- 
> i.
>
>

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

[matthew.gerlach]

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

Add generic support for MSI-X 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>
---
v4: s/MSIX/MSI_X
    move kernel doc to implementation
    use structure assignment
    fix decode of absolute address
    clean up comment in parse_feature_irqs
    remove use of csr_res

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  | 234 ++++++++++++++++++++++++++++++++++----------
 drivers/fpga/dfl.h  |   5 +
 include/linux/dfl.h |   4 +
 3 files changed, 194 insertions(+), 49 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..37f995e66436 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
 	if (ddev->mmio_res.parent)
 		release_resource(&ddev->mmio_res);
 
+	kfree(ddev->params);
+
 	ida_free(&dfl_device_ida, ddev->id);
 	kfree(ddev->irqs);
 	kfree(ddev);
@@ -380,7 +382,16 @@ 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;
+	if (feature->param_size) {
+		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
+		if (!ddev->params) {
+			ret = -ENOMEM;
+			goto put_dev;
+		}
+		ddev->param_size = feature->param_size;
+	}
 
 	/* add mmio resource */
 	parent_res = &pdev->resource[feature->resource_index];
@@ -708,20 +719,27 @@ 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.
  * @irq_base: start of irq index in this sub feature.
  * @nr_irqs: number of irqs of this sub feature.
+ * @param_size: size DFH parameters.
+ * @params: DFH parameter data.
  */
 struct dfl_feature_info {
 	u16 fid;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	void __iomem *ioaddr;
 	struct list_head node;
 	unsigned int irq_base;
 	unsigned int nr_irqs;
+	unsigned int param_size;
+	u64 params[];
 };
 
 static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
@@ -797,7 +815,17 @@ 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;
 
+		if (finfo->param_size) {
+			feature->params = devm_kmemdup(binfo->dev,
+						       finfo->params, finfo->param_size,
+						       GFP_KERNEL);
+			if (!feature->params)
+				return -ENOMEM;
+
+			feature->param_size = finfo->param_size;
+		}
 		/*
 		 * the FIU header feature has some fundamental functions (sriov
 		 * set, port enable/disable) needed for the dfl bus device and
@@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
 	return 0;
 }
 
+static void *find_param(void *base, resource_size_t max, int param)
+{
+	int off = 0;
+	u64 v, next;
+
+	while (off < max) {
+		v = *(u64 *)(base + off);
+		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+			return base + off + DFHv1_PARAM_DATA;
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+			break;
+
+	}
+
+	return NULL;
+}
+
+/**
+ * dfh_find_param() - find data for the given parameter id
+ * @dfl_dev: dfl device
+ * @param: id of dfl parameter
+ *
+ * Return: pointer to parameter data on success, NULL otherwise.
+ */
+void *dfh_find_param(struct dfl_device *dfl_dev, int param)
+{
+	return find_param(dfl_dev->params, dfl_dev->param_size, param);
+}
+EXPORT_SYMBOL_GPL(dfh_find_param);
+
 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;
+	void *params = finfo->params;
 	enum dfl_id_type type;
+	u16 fid = finfo->fid;
 	int virq;
+	u32 *p;
 	u64 v;
 
-	type = feature_dev_id_type(binfo->feature_dev);
+	switch (finfo->dfh_version) {
+	case 0:
+		/*
+		 * DFHv0 only provides mmio resource information for each feature
+		 * in the DFL header.  There is no generic interrupt information.
+		 * Instead, features with interrupt functionality provide
+		 * the information in feature specific registers.
+		 */
+		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) {
+			switch (fid) {
+			case 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;
+			}
+		}
+		break;
 
-	/*
-	 * Ideally DFL framework should only read info from DFL header, but
-	 * current version DFL 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
-	 * 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);
+	case 1:
+		/*
+		 * DFHv1 provides interrupt resource information in DFHv1
+		 * parameter blocks.
+		 */
+		if (!finfo->param_size)
 			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);
+
+		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
+		if (!p)
 			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 = *p++;
+		inr = *p;
+		break;
+
+	default:
+		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
+		break;
 	}
 
 	if (!inr) {
-		*irq_base = 0;
-		*nr_irqs = 0;
+		finfo->irq_base = 0;
+		finfo->nr_irqs = 0;
 		return 0;
 	}
 
@@ -1006,12 +1086,37 @@ 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;
 }
 
+static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
+{
+	int size = 0;
+	u64 v, next;
+
+	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
+		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
+		return 0;
+
+	while (size + DFHv1_PARAM_HDR < max) {
+		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
+			return -EINVAL;
+
+		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+
+		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+			return size;
+	}
+
+	return -ENOENT;
+}
+
 /*
  * when create sub feature instances, for private features, it doesn't need
  * to provide resource size and feature id as they could be read from DFH
@@ -1023,39 +1128,70 @@ 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;
+	int dfh_psize = 0;
 	u8 revision = 0;
+	u8 dfh_ver = 0;
 	int ret;
 	u64 v;
 
 	if (fid != FEATURE_ID_AFU) {
 		v = readq(binfo->ioaddr + ofst);
 		revision = FIELD_GET(DFH_REVISION, v);
-
+		dfh_ver = 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);
+		if (dfh_ver == 1) {
+			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
+			if (dfh_psize < 0) {
+				dev_err(binfo->dev,
+					"failed to read size of DFHv1 parameters %d\n",
+					dfh_psize);
+				return dfh_psize;
+			}
+			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
+		}
 	}
 
 	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);
+	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
 	if (!finfo)
 		return -ENOMEM;
 
+	if (dfh_psize > 0) {
+		memcpy_fromio(finfo->params,
+			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
+		finfo->param_size = dfh_psize;
+	}
+
 	finfo->fid = fid;
 	finfo->revision = revision;
-	finfo->mmio_res.start = binfo->start + ofst;
-	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+	finfo->dfh_version = dfh_ver;
 	finfo->mmio_res.flags = IORESOURCE_MEM;
-	finfo->irq_base = irq_base;
-	finfo->nr_irqs = nr_irqs;
+	if (dfh_ver == 1) {
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+		if (v & DFHv1_CSR_ADDR_REL)
+			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
+		else
+			finfo->mmio_res.start = binfo->start + ofst +
+					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+		finfo->mmio_res.end = finfo->mmio_res.start +
+				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
+	} else {
+		finfo->mmio_res.start = binfo->start + ofst;
+		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+	}
+
+	ret = parse_feature_irqs(binfo, ofst, finfo);
+	if (ret) {
+		kfree(finfo);
+		return ret;
+	}
 
 	list_add_tail(&finfo->node, &binfo->sub_features);
 	binfo->feature_num++;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 45e6e1359a67..1ea7f40c1af6 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
  * @ops: ops of this sub feature.
  * @ddev: ptr to the dfl device of this sub feature.
  * @priv: priv data of this feature.
+ * @param_size: size of dfh parameters
+ * @params: point to memory copy of dfh parameters
  */
 struct dfl_feature {
 	struct platform_device *dev;
 	u16 id;
 	u8 revision;
+	u8 dfh_version;
 	int resource_index;
 	void __iomem *ioaddr;
 	struct dfl_feature_irq_ctx *irq_ctx;
@@ -285,6 +288,8 @@ struct dfl_feature {
 	const struct dfl_feature_ops *ops;
 	struct dfl_device *ddev;
 	void *priv;
+	unsigned int param_size;
+	void *params;
 };
 
 #define FEATURE_DEV_ID_UNUSED	(-1)
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index fea9e16d35b6..d00787e870b7 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -50,9 +50,12 @@ struct dfl_device {
 	u16 type;
 	u16 feature_id;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	int *irqs;
 	unsigned int num_irqs;
+	unsigned int param_size;
+	void *params;
 	struct dfl_fpga_cdev *cdev;
 	const struct dfl_device_id *id_entry;
 };
@@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
 	module_driver(__dfl_driver, dfl_driver_register, \
 		      dfl_driver_unregister)
 
+void *dfh_find_param(struct dfl_device *dfl_dev, int param);
 #endif /* __LINUX_DFL_H */
-- 
2.25.1

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

[Andy Shevchenko]

On Thu, Oct 20, 2022 at 02:26:09PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add generic support for MSI-X 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.

...

> +static void *find_param(void *base, resource_size_t max, int param)

Why base can't be u64 * to begin with?

> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {

Maybe you need a comment somewhere to tell that the caller guarantees that max
won't provoke OOB accesses.

> +		v = *(u64 *)(base + off);

Okay, if offset is not multiple of at least 4, how do you guarantee no
exception on the architectures with disallowed misaligned accesses?

Making base to be u64 * solves this, but you need to take care to provide
offset in terms of u64 words.

> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return base + off + DFHv1_PARAM_DATA;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			break;
> +
> +	}
> +
> +	return NULL;
> +}

...

> +		/*
> +		 * DFHv0 only provides mmio resource information for each feature

MMIO

> +		 * in the DFL header.  There is no generic interrupt information.
> +		 * Instead, features with interrupt functionality provide
> +		 * the information in feature specific registers.
> +		 */

...

> +		if (!finfo->param_size)
>  			break;

This is redundant as it's implied by find_param().

> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
> +		if (!p)
>  			break;

...

> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> +	int size = 0;
> +	u64 v, next;
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> +		return 0;
> +
> +	while (size + DFHv1_PARAM_HDR < max) {
> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
> +			return -EINVAL;
> +
> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			return size;

These 3 looks like they deserve different fields and hence separate FIELD_GET()
will return exactly what we need without additional masking, right?

> +	}
> +
> +	return -ENOENT;
> +}

...

> +	if (dfh_psize > 0) {

Isn't this implied by memcpy_fromio()? I mean if it's 0, nothing bad will
happen if you call the above directly.

> +		memcpy_fromio(finfo->params,
> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> +		finfo->param_size = dfh_psize;
> +	}

...

>  	finfo->mmio_res.flags = IORESOURCE_MEM;
> +	if (dfh_ver == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> +		else
> +			finfo->mmio_res.start = binfo->start + ofst +
> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->mmio_res.end = finfo->mmio_res.start +
> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> +	} else {
> +		finfo->mmio_res.start = binfo->start + ofst;
> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> +	}

You may define

	resource_size_t start, end;

locally and simplify above quite a bit.

...

> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);

+ Blank line.

>  #endif /* __LINUX_DFL_H */

-- 
With Best Regards,
Andy Shevchenko

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

[matthew.gerlach]

On Fri, 21 Oct 2022, Andy Shevchenko wrote:

> On Thu, Oct 20, 2022 at 02:26:09PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add generic support for MSI-X 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.
>
> ...
>
>> +static void *find_param(void *base, resource_size_t max, int param)
>
> Why base can't be u64 * to begin with?

It can be u64, and I will consider it for the next iteration.
>
>> +{
>> +	int off = 0;
>> +	u64 v, next;
>> +
>> +	while (off < max) {
>
> Maybe you need a comment somewhere to tell that the caller guarantees that max
> won't provoke OOB accesses.
>
>> +		v = *(u64 *)(base + off);
>
> Okay, if offset is not multiple of at least 4, how do you guarantee no
> exception on the architectures with disallowed misaligned accesses?
>
> Making base to be u64 * solves this, but you need to take care to provide
> offset in terms of u64 words.

The masking of next below ensures that the offset it at least 4 byte 
aligned, but it might make sense to define the next field in terms of 8 
byte words.

>
>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return base + off + DFHv1_PARAM_DATA;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			break;
>> +
>> +	}
>> +
>> +	return NULL;
>> +}
>
> ...
>
>> +		/*
>> +		 * DFHv0 only provides mmio resource information for each feature
>
> MMIO

I'll change mmio to MMIO here and a place in the documentation that I 
noticed.

>
>> +		 * in the DFL header.  There is no generic interrupt information.
>> +		 * Instead, features with interrupt functionality provide
>> +		 * the information in feature specific registers.
>> +		 */
>
> ...
>
>> +		if (!finfo->param_size)
>>  			break;
>
> This is redundant as it's implied by find_param().

I will remove the redundant code.

>
>> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
>> +		if (!p)
>>  			break;
>
> ...
>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>> +			return -EINVAL;
>> +
>> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			return size;
>
> These 3 looks like they deserve different fields and hence separate FIELD_GET()
> will return exactly what we need without additional masking, right?

I agree separate FIELD_GET() calls will be cleaner.

>
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>
> ...
>
>> +	if (dfh_psize > 0) {
>
> Isn't this implied by memcpy_fromio()? I mean if it's 0, nothing bad will
> happen if you call the above directly.
>
>> +		memcpy_fromio(finfo->params,
>> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
>> +		finfo->param_size = dfh_psize;
>> +	}
>
> ...
>
>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>> +	if (dfh_ver == 1) {
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> +		if (v & DFHv1_CSR_ADDR_REL)
>> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>> +		else
>> +			finfo->mmio_res.start = binfo->start + ofst +
>> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		finfo->mmio_res.end = finfo->mmio_res.start +
>> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>> +	} else {
>> +		finfo->mmio_res.start = binfo->start + ofst;
>> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	}
>
> You may define
>
> 	resource_size_t start, end;
>
> locally and simplify above quite a bit.

That is a good suggestion that should clean up the code quite a bit.

>
> ...
>
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
>
> + Blank line.
>
>>  #endif /* __LINUX_DFL_H */
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

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

[Ilpo Järvinen]

On Thu, 20 Oct 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add generic support for MSI-X 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>
> ---
> v4: s/MSIX/MSI_X
>     move kernel doc to implementation
>     use structure assignment
>     fix decode of absolute address
>     clean up comment in parse_feature_irqs
>     remove use of csr_res
> 
> 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
> ---

> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> +	int size = 0;
> +	u64 v, next;
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> +		return 0;
> +
> +	while (size + DFHv1_PARAM_HDR < max) {
> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))

In general, try to not use inverse logic for defining masks. However here, 
just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits 
(no rsvd nor eop) and you no longer need this extra masking.

> +			return -EINVAL;
> +
> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;

...Then you can drop this anding too.

> +
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)

Your docs say EOP, but here you use EOL.

Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from 
v.

-- 
 i.

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

[matthew.gerlach]

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



On Fri, 21 Oct 2022, Ilpo Järvinen wrote:

> On Thu, 20 Oct 2022, matthew.gerlach@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add generic support for MSI-X 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>
>> ---
>> v4: s/MSIX/MSI_X
>>     move kernel doc to implementation
>>     use structure assignment
>>     fix decode of absolute address
>>     clean up comment in parse_feature_irqs
>>     remove use of csr_res
>>
>> 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
>> ---
>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>
> In general, try to not use inverse logic for defining masks. However here,
> just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits
> (no rsvd nor eop) and you no longer need this extra masking.

I agree that defining the fields better and using FIELD_GET would make 
this code cleaner.

>
>> +			return -EINVAL;
>> +
>> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>
> ...Then you can drop this anding too.
>
>> +
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>
> Your docs say EOP, but here you use EOL.

Thanks for catching the inconsistency.

>
> Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from
> v.
>
> -- 
> i.
>

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

[Ilpo Järvinen]

On Thu, 20 Oct 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add generic support for MSI-X 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>
> ---

> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
>  	return 0;
>  }
>  
> +static void *find_param(void *base, resource_size_t max, int param)
> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = *(u64 *)(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return base + off + DFHv1_PARAM_DATA;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			break;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter data on success, NULL otherwise.
> + */
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
> +{
> +	return find_param(dfl_dev->params, dfl_dev->param_size, param);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);

Do you expect this split between dfh_find_param() and find_param() to
be useful in the future? If no other callers are expected, I'd just pull 
find_param() into dfh_find_param() and create local variables for base and 
max.

-- 
 i.

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

[Xu Yilun]

On 2022-10-20 at 14:26:09 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add generic support for MSI-X interrupts for DFL devices.

The first paragraph of the commit message should be the summary of the
whole change. But this seems one of the changes. Please add the proper
summary at the beginning.

> 
> 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>
> ---
> v4: s/MSIX/MSI_X
>     move kernel doc to implementation
>     use structure assignment
>     fix decode of absolute address
>     clean up comment in parse_feature_irqs
>     remove use of csr_res
> 
> 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  | 234 ++++++++++++++++++++++++++++++++++----------
>  drivers/fpga/dfl.h  |   5 +
>  include/linux/dfl.h |   4 +
>  3 files changed, 194 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..37f995e66436 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
>  	if (ddev->mmio_res.parent)
>  		release_resource(&ddev->mmio_res);
>  
> +	kfree(ddev->params);
> +
>  	ida_free(&dfl_device_ida, ddev->id);
>  	kfree(ddev->irqs);
>  	kfree(ddev);
> @@ -380,7 +382,16 @@ 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;
> +	if (feature->param_size) {
> +		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
> +		if (!ddev->params) {
> +			ret = -ENOMEM;
> +			goto put_dev;
> +		}
> +		ddev->param_size = feature->param_size;
> +	}
>  
>  	/* add mmio resource */
>  	parent_res = &pdev->resource[feature->resource_index];
> @@ -708,20 +719,27 @@ 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.
>   * @irq_base: start of irq index in this sub feature.
>   * @nr_irqs: number of irqs of this sub feature.
> + * @param_size: size DFH parameters.
> + * @params: DFH parameter data.
>   */
>  struct dfl_feature_info {
>  	u16 fid;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	void __iomem *ioaddr;
>  	struct list_head node;
>  	unsigned int irq_base;
>  	unsigned int nr_irqs;
> +	unsigned int param_size;
> +	u64 params[];
>  };
>  
>  static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> @@ -797,7 +815,17 @@ 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;
>  
> +		if (finfo->param_size) {
> +			feature->params = devm_kmemdup(binfo->dev,
> +						       finfo->params, finfo->param_size,
> +						       GFP_KERNEL);
> +			if (!feature->params)
> +				return -ENOMEM;
> +
> +			feature->param_size = finfo->param_size;
> +		}
>  		/*
>  		 * the FIU header feature has some fundamental functions (sriov
>  		 * set, port enable/disable) needed for the dfl bus device and
> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
>  	return 0;
>  }
>  
> +static void *find_param(void *base, resource_size_t max, int param)
> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = *(u64 *)(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return base + off + DFHv1_PARAM_DATA;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			break;
> +
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter data on success, NULL otherwise.
> + */
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
> +{
> +	return find_param(dfl_dev->params, dfl_dev->param_size, param);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);
> +
>  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;
> +	void *params = finfo->params;
>  	enum dfl_id_type type;
> +	u16 fid = finfo->fid;
>  	int virq;
> +	u32 *p;
>  	u64 v;
>  
> -	type = feature_dev_id_type(binfo->feature_dev);
> +	switch (finfo->dfh_version) {
> +	case 0:
> +		/*
> +		 * DFHv0 only provides mmio resource information for each feature
> +		 * in the DFL header.  There is no generic interrupt information.
> +		 * Instead, features with interrupt functionality provide
> +		 * the information in feature specific registers.
> +		 */
> +		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) {
> +			switch (fid) {
> +			case 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;
> +			}
> +		}
> +		break;
>  
> -	/*
> -	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL 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
> -	 * 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);
> +	case 1:
> +		/*
> +		 * DFHv1 provides interrupt resource information in DFHv1
> +		 * parameter blocks.
> +		 */
> +		if (!finfo->param_size)
>  			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);
> +
> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
> +		if (!p)
>  			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 = *p++;
> +		inr = *p;
> +		break;
> +
> +	default:
> +		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
> +		break;
>  	}
>  
>  	if (!inr) {
> -		*irq_base = 0;
> -		*nr_irqs = 0;
> +		finfo->irq_base = 0;
> +		finfo->nr_irqs = 0;
>  		return 0;
>  	}
>  
> @@ -1006,12 +1086,37 @@ 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;
>  }
>  
> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> +{
> +	int size = 0;
> +	u64 v, next;
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> +		return 0;
> +
> +	while (size + DFHv1_PARAM_HDR < max) {
> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
> +			return -EINVAL;
> +
> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
> +
> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
> +			return size;
> +	}
> +
> +	return -ENOENT;
> +}
> +
>  /*
>   * when create sub feature instances, for private features, it doesn't need
>   * to provide resource size and feature id as they could be read from DFH
> @@ -1023,39 +1128,70 @@ 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;
> +	int dfh_psize = 0;
>  	u8 revision = 0;
> +	u8 dfh_ver = 0;
>  	int ret;
>  	u64 v;
>  
>  	if (fid != FEATURE_ID_AFU) {
>  		v = readq(binfo->ioaddr + ofst);
>  		revision = FIELD_GET(DFH_REVISION, v);
> -
> +		dfh_ver = 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);
> +		if (dfh_ver == 1) {
> +			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
> +			if (dfh_psize < 0) {
> +				dev_err(binfo->dev,
> +					"failed to read size of DFHv1 parameters %d\n",
> +					dfh_psize);
> +				return dfh_psize;
> +			}
> +			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
> +		}
>  	}
>  
>  	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);
> +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);

The u64 flexible array in the structure, but seems dfh_get_psize could
not garantee 64bit aligned size.

What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
u32[].

Thanks,
Yilun

>  	if (!finfo)
>  		return -ENOMEM;
>  
> +	if (dfh_psize > 0) {
> +		memcpy_fromio(finfo->params,
> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> +		finfo->param_size = dfh_psize;
> +	}
> +
>  	finfo->fid = fid;
>  	finfo->revision = revision;
> -	finfo->mmio_res.start = binfo->start + ofst;
> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> +	finfo->dfh_version = dfh_ver;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
> -	finfo->irq_base = irq_base;
> -	finfo->nr_irqs = nr_irqs;
> +	if (dfh_ver == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> +		else
> +			finfo->mmio_res.start = binfo->start + ofst +
> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->mmio_res.end = finfo->mmio_res.start +
> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;

So for dflv1, no feature header resource for dfl_device, is it a problem
for dfl_uio? Does userspace driver need the raw feature header?

> +	} else {
> +		finfo->mmio_res.start = binfo->start + ofst;
> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> +	}
> +
> +	ret = parse_feature_irqs(binfo, ofst, finfo);
> +	if (ret) {
> +		kfree(finfo);
> +		return ret;
> +	}
>  
>  	list_add_tail(&finfo->node, &binfo->sub_features);
>  	binfo->feature_num++;
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 45e6e1359a67..1ea7f40c1af6 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
>   * @ops: ops of this sub feature.
>   * @ddev: ptr to the dfl device of this sub feature.
>   * @priv: priv data of this feature.
> + * @param_size: size of dfh parameters
> + * @params: point to memory copy of dfh parameters
>   */
>  struct dfl_feature {
>  	struct platform_device *dev;
>  	u16 id;
>  	u8 revision;
> +	u8 dfh_version;
>  	int resource_index;
>  	void __iomem *ioaddr;
>  	struct dfl_feature_irq_ctx *irq_ctx;
> @@ -285,6 +288,8 @@ struct dfl_feature {
>  	const struct dfl_feature_ops *ops;
>  	struct dfl_device *ddev;
>  	void *priv;
> +	unsigned int param_size;
> +	void *params;
>  };
>  
>  #define FEATURE_DEV_ID_UNUSED	(-1)
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index fea9e16d35b6..d00787e870b7 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -50,9 +50,12 @@ struct dfl_device {
>  	u16 type;
>  	u16 feature_id;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	int *irqs;
>  	unsigned int num_irqs;
> +	unsigned int param_size;
> +	void *params;
>  	struct dfl_fpga_cdev *cdev;
>  	const struct dfl_device_id *id_entry;
>  };
> @@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>  	module_driver(__dfl_driver, dfl_driver_register, \
>  		      dfl_driver_unregister)
>  
> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);

int param_id is better?

Thanks,
Yilun

>  #endif /* __LINUX_DFL_H */
> -- 
> 2.25.1
> 

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

[matthew.gerlach]

On Sat, 29 Oct 2022, Xu Yilun wrote:

> On 2022-10-20 at 14:26:09 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add generic support for MSI-X interrupts for DFL devices.
>
> The first paragraph of the commit message should be the summary of the
> whole change. But this seems one of the changes. Please add the proper
> summary at the beginning.

This is a very good suggestion.  It will be addressed in the next 
revision.

>
>>
>> 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>
>> ---
>> v4: s/MSIX/MSI_X
>>     move kernel doc to implementation
>>     use structure assignment
>>     fix decode of absolute address
>>     clean up comment in parse_feature_irqs
>>     remove use of csr_res
>>
>> 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  | 234 ++++++++++++++++++++++++++++++++++----------
>>  drivers/fpga/dfl.h  |   5 +
>>  include/linux/dfl.h |   4 +
>>  3 files changed, 194 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index b9aae85ba930..37f995e66436 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
>>  	if (ddev->mmio_res.parent)
>>  		release_resource(&ddev->mmio_res);
>>
>> +	kfree(ddev->params);
>> +
>>  	ida_free(&dfl_device_ida, ddev->id);
>>  	kfree(ddev->irqs);
>>  	kfree(ddev);
>> @@ -380,7 +382,16 @@ 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;
>> +	if (feature->param_size) {
>> +		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
>> +		if (!ddev->params) {
>> +			ret = -ENOMEM;
>> +			goto put_dev;
>> +		}
>> +		ddev->param_size = feature->param_size;
>> +	}
>>
>>  	/* add mmio resource */
>>  	parent_res = &pdev->resource[feature->resource_index];
>> @@ -708,20 +719,27 @@ 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.
>>   * @irq_base: start of irq index in this sub feature.
>>   * @nr_irqs: number of irqs of this sub feature.
>> + * @param_size: size DFH parameters.
>> + * @params: DFH parameter data.
>>   */
>>  struct dfl_feature_info {
>>  	u16 fid;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	struct resource mmio_res;
>>  	void __iomem *ioaddr;
>>  	struct list_head node;
>>  	unsigned int irq_base;
>>  	unsigned int nr_irqs;
>> +	unsigned int param_size;
>> +	u64 params[];
>>  };
>>
>>  static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
>> @@ -797,7 +815,17 @@ 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;
>>
>> +		if (finfo->param_size) {
>> +			feature->params = devm_kmemdup(binfo->dev,
>> +						       finfo->params, finfo->param_size,
>> +						       GFP_KERNEL);
>> +			if (!feature->params)
>> +				return -ENOMEM;
>> +
>> +			feature->param_size = finfo->param_size;
>> +		}
>>  		/*
>>  		 * the FIU header feature has some fundamental functions (sriov
>>  		 * set, port enable/disable) needed for the dfl bus device and
>> @@ -934,56 +962,108 @@ static u16 feature_id(u64 value)
>>  	return 0;
>>  }
>>
>> +static void *find_param(void *base, resource_size_t max, int param)
>> +{
>> +	int off = 0;
>> +	u64 v, next;
>> +
>> +	while (off < max) {
>> +		v = *(u64 *)(base + off);
>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return base + off + DFHv1_PARAM_DATA;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			break;
>> +
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * dfh_find_param() - find data for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to parameter data on success, NULL otherwise.
>> + */
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param)
>> +{
>> +	return find_param(dfl_dev->params, dfl_dev->param_size, param);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>> +
>>  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;
>> +	void *params = finfo->params;
>>  	enum dfl_id_type type;
>> +	u16 fid = finfo->fid;
>>  	int virq;
>> +	u32 *p;
>>  	u64 v;
>>
>> -	type = feature_dev_id_type(binfo->feature_dev);
>> +	switch (finfo->dfh_version) {
>> +	case 0:
>> +		/*
>> +		 * DFHv0 only provides mmio resource information for each feature
>> +		 * in the DFL header.  There is no generic interrupt information.
>> +		 * Instead, features with interrupt functionality provide
>> +		 * the information in feature specific registers.
>> +		 */
>> +		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) {
>> +			switch (fid) {
>> +			case 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;
>> +			}
>> +		}
>> +		break;
>>
>> -	/*
>> -	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL 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
>> -	 * 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);
>> +	case 1:
>> +		/*
>> +		 * DFHv1 provides interrupt resource information in DFHv1
>> +		 * parameter blocks.
>> +		 */
>> +		if (!finfo->param_size)
>>  			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);
>> +
>> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
>> +		if (!p)
>>  			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 = *p++;
>> +		inr = *p;
>> +		break;
>> +
>> +	default:
>> +		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
>> +		break;
>>  	}
>>
>>  	if (!inr) {
>> -		*irq_base = 0;
>> -		*nr_irqs = 0;
>> +		finfo->irq_base = 0;
>> +		finfo->nr_irqs = 0;
>>  		return 0;
>>  	}
>>
>> @@ -1006,12 +1086,37 @@ 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;
>>  }
>>
>> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))
>> +			return -EINVAL;
>> +
>> +		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
>> +
>> +		if (next & DFHv1_PARAM_HDR_NEXT_EOL)
>> +			return size;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>>  /*
>>   * when create sub feature instances, for private features, it doesn't need
>>   * to provide resource size and feature id as they could be read from DFH
>> @@ -1023,39 +1128,70 @@ 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;
>> +	int dfh_psize = 0;
>>  	u8 revision = 0;
>> +	u8 dfh_ver = 0;
>>  	int ret;
>>  	u64 v;
>>
>>  	if (fid != FEATURE_ID_AFU) {
>>  		v = readq(binfo->ioaddr + ofst);
>>  		revision = FIELD_GET(DFH_REVISION, v);
>> -
>> +		dfh_ver = 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);
>> +		if (dfh_ver == 1) {
>> +			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
>> +			if (dfh_psize < 0) {
>> +				dev_err(binfo->dev,
>> +					"failed to read size of DFHv1 parameters %d\n",
>> +					dfh_psize);
>> +				return dfh_psize;
>> +			}
>> +			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
>> +		}
>>  	}
>>
>>  	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);
>> +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>
> The u64 flexible array in the structure, but seems dfh_get_psize could
> not garantee 64bit aligned size.
>
> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> u32[].
>
> Thanks,
> Yilun

The mandatory alignment alignment is 64.  The documentation and field 
definitions will be updated accordingly in the next revision.

>
>>  	if (!finfo)
>>  		return -ENOMEM;
>>
>> +	if (dfh_psize > 0) {
>> +		memcpy_fromio(finfo->params,
>> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
>> +		finfo->param_size = dfh_psize;
>> +	}
>> +
>>  	finfo->fid = fid;
>>  	finfo->revision = revision;
>> -	finfo->mmio_res.start = binfo->start + ofst;
>> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	finfo->dfh_version = dfh_ver;
>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>> -	finfo->irq_base = irq_base;
>> -	finfo->nr_irqs = nr_irqs;
>> +	if (dfh_ver == 1) {
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> +		if (v & DFHv1_CSR_ADDR_REL)
>> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>> +		else
>> +			finfo->mmio_res.start = binfo->start + ofst +
>> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		finfo->mmio_res.end = finfo->mmio_res.start +
>> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>
> So for dflv1, no feature header resource for dfl_device, is it a problem
> for dfl_uio? Does userspace driver need the raw feature header?
These are two very good questions.  The dfl_uio driver question is 
particularly relevent because user space is looking at the GUIDs.

>
>> +	} else {
>> +		finfo->mmio_res.start = binfo->start + ofst;
>> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	}
>> +
>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>> +	if (ret) {
>> +		kfree(finfo);
>> +		return ret;
>> +	}
>>
>>  	list_add_tail(&finfo->node, &binfo->sub_features);
>>  	binfo->feature_num++;
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 45e6e1359a67..1ea7f40c1af6 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -273,11 +273,14 @@ struct dfl_feature_irq_ctx {
>>   * @ops: ops of this sub feature.
>>   * @ddev: ptr to the dfl device of this sub feature.
>>   * @priv: priv data of this feature.
>> + * @param_size: size of dfh parameters
>> + * @params: point to memory copy of dfh parameters
>>   */
>>  struct dfl_feature {
>>  	struct platform_device *dev;
>>  	u16 id;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	int resource_index;
>>  	void __iomem *ioaddr;
>>  	struct dfl_feature_irq_ctx *irq_ctx;
>> @@ -285,6 +288,8 @@ struct dfl_feature {
>>  	const struct dfl_feature_ops *ops;
>>  	struct dfl_device *ddev;
>>  	void *priv;
>> +	unsigned int param_size;
>> +	void *params;
>>  };
>>
>>  #define FEATURE_DEV_ID_UNUSED	(-1)
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index fea9e16d35b6..d00787e870b7 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -50,9 +50,12 @@ struct dfl_device {
>>  	u16 type;
>>  	u16 feature_id;
>>  	u8 revision;
>> +	u8 dfh_version;
>>  	struct resource mmio_res;
>>  	int *irqs;
>>  	unsigned int num_irqs;
>> +	unsigned int param_size;
>> +	void *params;
>>  	struct dfl_fpga_cdev *cdev;
>>  	const struct dfl_device_id *id_entry;
>>  };
>> @@ -95,4 +98,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>>  	module_driver(__dfl_driver, dfl_driver_register, \
>>  		      dfl_driver_unregister)
>>
>> +void *dfh_find_param(struct dfl_device *dfl_dev, int param);
>
> int param_id is better?

param_id would be better.


>
> Thanks,
> Yilun
>
>>  #endif /* __LINUX_DFL_H */
>> --
>> 2.25.1
>>
>

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

[matthew.gerlach]

On Sat, 29 Oct 2022, matthew.gerlach@linux.intel.com wrote:

>
>>
>>>  	if (!finfo)
>>>  		return -ENOMEM;
>>> 
>>> +	if (dfh_psize > 0) {
>>> +		memcpy_fromio(finfo->params,
>>> +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, 
>>> dfh_psize);
>>> +		finfo->param_size = dfh_psize;
>>> +	}
>>> +
>>>  	finfo->fid = fid;
>>>  	finfo->revision = revision;
>>> -	finfo->mmio_res.start = binfo->start + ofst;
>>> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>> +	finfo->dfh_version = dfh_ver;
>>>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>>> -	finfo->irq_base = irq_base;
>>> -	finfo->nr_irqs = nr_irqs;
>>> +	if (dfh_ver == 1) {
>>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>>> +		if (v & DFHv1_CSR_ADDR_REL)
>>> +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
>>> +		else
>>> +			finfo->mmio_res.start = binfo->start + ofst +
>>> +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, 
>>> v);
>>> +
>>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>>> +		finfo->mmio_res.end = finfo->mmio_res.start +
>>> +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 
>>> 1;
>> 
>> So for dflv1, no feature header resource for dfl_device, is it a problem
>> for dfl_uio? Does userspace driver need the raw feature header?
> These are two very good questions.  The dfl_uio driver question is 
> particularly relevent because user space is looking at the GUIDs.
>

In the case of dfl_uio driver, user space will definitely want to look at 
the feature header for the GUID and the parameters.  Since DFHv1 can have 
the DFH header and the feature registers in non-contiguous memory 
locations, a resource for the dfl_device will be required.  In earlier
revisions of this patch set, a second resource was added called csr_res 
pointing to the feature's register while mmio_res pointed at the header.
Do we just need better names or do we need an array of named resources?

>> 
>>> +	} else {
>>> +		finfo->mmio_res.start = binfo->start + ofst;
>>> +		finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>>> +	}
>>> +
>>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>>> +	if (ret) {
>>> +		kfree(finfo);
>>> +		return ret;
>>> +	}
>>>

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

[Xu Yilun]

On 2022-11-01 at 15:37:19 -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Sat, 29 Oct 2022, matthew.gerlach@linux.intel.com wrote:
> 
> > 
> > > 
> > > >  	if (!finfo)
> > > >  		return -ENOMEM;
> > > > 
> > > > +	if (dfh_psize > 0) {
> > > > +		memcpy_fromio(finfo->params,
> > > > +			      binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> > > > +		finfo->param_size = dfh_psize;
> > > > +	}
> > > > +
> > > >  	finfo->fid = fid;
> > > >  	finfo->revision = revision;
> > > > -	finfo->mmio_res.start = binfo->start + ofst;
> > > > -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > > > +	finfo->dfh_version = dfh_ver;
> > > >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> > > > -	finfo->irq_base = irq_base;
> > > > -	finfo->nr_irqs = nr_irqs;
> > > > +	if (dfh_ver == 1) {
> > > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> > > > +		if (v & DFHv1_CSR_ADDR_REL)
> > > > +			finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
> > > > +		else
> > > > +			finfo->mmio_res.start = binfo->start + ofst +
> > > > +					       FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> > > > +
> > > > +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> > > > +		finfo->mmio_res.end = finfo->mmio_res.start +
> > > > +				      FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> > > 
> > > So for dflv1, no feature header resource for dfl_device, is it a problem
> > > for dfl_uio? Does userspace driver need the raw feature header?
> > These are two very good questions.  The dfl_uio driver question is
> > particularly relevent because user space is looking at the GUIDs.
> > 
> 
> In the case of dfl_uio driver, user space will definitely want to look at
> the feature header for the GUID and the parameters.  Since DFHv1 can have
> the DFH header and the feature registers in non-contiguous memory locations,
> a resource for the dfl_device will be required.  In earlier
> revisions of this patch set, a second resource was added called csr_res
> pointing to the feature's register while mmio_res pointed at the header.
> Do we just need better names or do we need an array of named resources?

Either is OK, you could also name a resource element in an array by
struct resource:name. But my concern is still no overlapping.

Thanks,
Yilun

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

[Andy Shevchenko]

On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> On 2022-10-20 at 14:26:09 -0700, matthew.gerlach@linux.intel.com wrote:

> >  struct dfl_feature_info {
> >  	u16 fid;
> >  	u8 revision;
> > +	u8 dfh_version;
> >  	struct resource mmio_res;
> >  	void __iomem *ioaddr;
> >  	struct list_head node;
> >  	unsigned int irq_base;
> >  	unsigned int nr_irqs;
> > +	unsigned int param_size;
> > +	u64 params[];
> >  };

...

> > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);


This probably may use something from overflow.h.

> The u64 flexible array in the structure, but seems dfh_get_psize could
> not garantee 64bit aligned size.
> 
> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> u32[].

Isn't it guaranteed by the C standard / architecture ABI?

-- 
With Best Regards,
Andy Shevchenko

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

[Xu Yilun]

On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > On 2022-10-20 at 14:26:09 -0700, matthew.gerlach@linux.intel.com wrote:
> 
> > >  struct dfl_feature_info {
> > >  	u16 fid;
> > >  	u8 revision;
> > > +	u8 dfh_version;
> > >  	struct resource mmio_res;
> > >  	void __iomem *ioaddr;
> > >  	struct list_head node;
> > >  	unsigned int irq_base;
> > >  	unsigned int nr_irqs;
> > > +	unsigned int param_size;
> > > +	u64 params[];
> > >  };
> 
> ...
> 
> > > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> 
> 
> This probably may use something from overflow.h.
> 
> > The u64 flexible array in the structure, but seems dfh_get_psize could
> > not garantee 64bit aligned size.
> > 
> > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > u32[].
> 
> Isn't it guaranteed by the C standard / architecture ABI?

I'm referring to the malloc size of the structure. It reserved dfh_psize
bytes for this u64 array, but there is no garantee dfh_psize should be a
multiple of 8. So there may be memory leak when accessing the last
array element?

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

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

[Andy Shevchenko]

On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
> On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> > On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > > On 2022-10-20 at 14:26:09 -0700, matthew.gerlach@linux.intel.com wrote:
> > 
> > > >  struct dfl_feature_info {
> > > >  	u16 fid;
> > > >  	u8 revision;
> > > > +	u8 dfh_version;
> > > >  	struct resource mmio_res;
> > > >  	void __iomem *ioaddr;
> > > >  	struct list_head node;
> > > >  	unsigned int irq_base;
> > > >  	unsigned int nr_irqs;
> > > > +	unsigned int param_size;
> > > > +	u64 params[];
> > > >  };
> > 
> > ...
> > 
> > > > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > 
> > 
> > This probably may use something from overflow.h.
> > 
> > > The u64 flexible array in the structure, but seems dfh_get_psize could
> > > not garantee 64bit aligned size.
> > > 
> > > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > > u32[].
> > 
> > Isn't it guaranteed by the C standard / architecture ABI?
> 
> I'm referring to the malloc size of the structure. It reserved dfh_psize
> bytes for this u64 array, but there is no garantee dfh_psize should be a
> multiple of 8. So there may be memory leak when accessing the last
> array element?

Have you looked at macros in the overflow.h? Would the use of it solve your
concern?

-- 
With Best Regards,
Andy Shevchenko

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

[matthew.gerlach]

On Mon, 31 Oct 2022, Andy Shevchenko wrote:

> On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
>> On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
>>> On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
>>>> On 2022-10-20 at 14:26:09 -0700, matthew.gerlach@linux.intel.com wrote:
>>>
>>>>>  struct dfl_feature_info {
>>>>>  	u16 fid;
>>>>>  	u8 revision;
>>>>> +	u8 dfh_version;
>>>>>  	struct resource mmio_res;
>>>>>  	void __iomem *ioaddr;
>>>>>  	struct list_head node;
>>>>>  	unsigned int irq_base;
>>>>>  	unsigned int nr_irqs;
>>>>> +	unsigned int param_size;
>>>>> +	u64 params[];
>>>>>  };
>>>
>>> ...
>>>
>>>>> +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>>>
>>>
>>> This probably may use something from overflow.h.
>>>
>>>> The u64 flexible array in the structure, but seems dfh_get_psize could
>>>> not garantee 64bit aligned size.
>>>>
>>>> What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
>>>> of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
>>>> u32[].
>>>
>>> Isn't it guaranteed by the C standard / architecture ABI?
>>
>> I'm referring to the malloc size of the structure. It reserved dfh_psize
>> bytes for this u64 array, but there is no garantee dfh_psize should be a
>> multiple of 8. So there may be memory leak when accessing the last
>> array element?
>
> Have you looked at macros in the overflow.h? Would the use of it solve your
> concern?

By clarifying the definition of the next field in the parameter header 
as the number of 8-byte words, dfh_get_psize is guaranteed to be a multiple of 8.
This is fixed in the next revision of patches.

Matthew Gerlach

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

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

[Xu Yilun]

On 2022-10-31 at 17:34:20 +0200, Andy Shevchenko wrote:
> On Mon, Oct 31, 2022 at 09:16:19AM +0800, Xu Yilun wrote:
> > On 2022-10-31 at 00:06:28 +0200, Andy Shevchenko wrote:
> > > On Sat, Oct 29, 2022 at 09:08:44PM +0800, Xu Yilun wrote:
> > > > On 2022-10-20 at 14:26:09 -0700, matthew.gerlach@linux.intel.com wrote:
> > > 
> > > > >  struct dfl_feature_info {
> > > > >  	u16 fid;
> > > > >  	u8 revision;
> > > > > +	u8 dfh_version;
> > > > >  	struct resource mmio_res;
> > > > >  	void __iomem *ioaddr;
> > > > >  	struct list_head node;
> > > > >  	unsigned int irq_base;
> > > > >  	unsigned int nr_irqs;
> > > > > +	unsigned int param_size;
> > > > > +	u64 params[];
> > > > >  };
> > > 
> > > ...
> > > 
> > > > > +	finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > > 
> > > 
> > > This probably may use something from overflow.h.
> > > 
> > > > The u64 flexible array in the structure, but seems dfh_get_psize could
> > > > not garantee 64bit aligned size.
> > > > 
> > > > What's the mandatory alignment of param data? If 64bit aligned, bit 33-34
> > > > of PARAM_HDR should be reserved. If 32bit aligned, finfo:params should be
> > > > u32[].
> > > 
> > > Isn't it guaranteed by the C standard / architecture ABI?
> > 
> > I'm referring to the malloc size of the structure. It reserved dfh_psize
> > bytes for this u64 array, but there is no garantee dfh_psize should be a
> > multiple of 8. So there may be memory leak when accessing the last
> > array element?
> 
> Have you looked at macros in the overflow.h? Would the use of it solve your
> concern?

Yes, struct_size() or array_size() specifies the element size & count,
which solve the concern at the root.

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

[PATCH v4 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>
---
v4: use dev_err_probe() everywhere that is appropriate
    clean up noise
    change error messages to use the word, unsupported
    tried again to sort Makefile and KConfig better
    reorder probe function for easier error handling
    use new dfh_find_param API

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 | 149 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 +++
 drivers/tty/serial/8250/Makefile   |   1 +
 3 files changed, 162 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..f02f0ba2a565
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,149 @@
+// 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 dfl_device *dfl_dev, struct uart_8250_port *uart)
+{
+	struct device *dev = &dfl_dev->dev;
+	u64 v, fifo_len, reg_width;
+	u64 *p;
+
+	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
+	if (!p)
+		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
+
+	uart->port.uartclk = *p;
+	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
+
+	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
+	if (!p)
+		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
+
+	fifo_len = *p;
+	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:
+		return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
+	}
+
+	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
+	if (!p)
+		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
+
+	v = *p;
+	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", reg_width, uart->port.regshift);
+
+	switch (reg_width) {
+	case 4:
+		uart->port.iotype = UPIO_MEM32;
+		break;
+
+	case 2:
+		uart->port.iotype = UPIO_MEM16;
+		break;
+
+	default:
+		return dev_err_probe(dev, -EINVAL, "unsupported reg_width %lld\n", reg_width);
+	}
+
+	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;
+	int ret;
+
+	memset(&uart, 0, sizeof(uart));
+	uart.port.flags = UPF_IOREMAP;
+	uart.port.mapbase = dfl_dev->mmio_res.start;
+	uart.port.mapsize = resource_size(&dfl_dev->mmio_res);
+
+	ret = dfl_uart_get_params(dfl_dev, &uart);
+
+	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];
+
+	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+	if (!dfluart)
+		return -ENOMEM;
+
+	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_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);
+
+	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..4efc8ee51c18 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -370,6 +370,18 @@ config SERIAL_8250_FSL
 	  erratum for Freescale 16550 UARTs in the 8250 driver. It also
 	  enables support for ACPI enumeration.
 
+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.
+
+	  To compile this driver as a module, chose M here: the
+	  module will be called 8250_dfl.
+
 config SERIAL_8250_DW
 	tristate "Support for Synopsys DesignWare 8250 quirks"
 	depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..65bc6ad4dd01 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -28,6 +28,7 @@ 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
 obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
+obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
 obj-$(CONFIG_SERIAL_8250_IOC3)		+= 8250_ioc3.o
-- 
2.25.1

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

[Andy Shevchenko]

On Thu, Oct 20, 2022 at 02:26:10PM -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.

...

> +#include <linux/bitfield.h>

Where is device.h?

> +#include <linux/dfl.h>

> +#include <linux/io-64-nonatomic-lo-hi.h>

User?

> +#include <linux/kernel.h>

Try to use what is really needed, yet this one may be still needed for
something like ARRAY_SIZE().

> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>

Missed types.h.

...

> +	ret = dfl_uart_get_params(dfl_dev, &uart);

> +

Redundant blank line.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Greg KH]

On Thu, Oct 20, 2022 at 02:26:10PM -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.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

As per the rules of how Intel developers are supposed to send out
patches, I can't take this.  Please work with the Intel kernel developer
team to get this properly reviewed first, and the correct reviews,
before sending it out again.

thanks,

greg k-h

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

[Ilpo Järvinen]

On Thu, 20 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>
> ---

> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (!p)
> +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
> +
> +	v = *p;
> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);

Just use *p directly for the FIELD_GETs. I suppose you can drop v after 
changing it.

Other than that and what Andy noted, this patch looks good.


-- 
 i.

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

[Xu Yilun]

On 2022-10-20 at 14:26:10 -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.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v4: use dev_err_probe() everywhere that is appropriate
>     clean up noise
>     change error messages to use the word, unsupported
>     tried again to sort Makefile and KConfig better
>     reorder probe function for easier error handling
>     use new dfh_find_param API
> 
> 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 | 149 +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |  12 +++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  3 files changed, 162 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..f02f0ba2a565
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,149 @@
> +// 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 dfl_device *dfl_dev, struct uart_8250_port *uart)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	u64 v, fifo_len, reg_width;
> +	u64 *p;
> +
> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> +	if (!p)
> +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
> +
> +	uart->port.uartclk = *p;
> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> +
> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> +	if (!p)
> +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
> +
> +	fifo_len = *p;
> +	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:
> +		return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
> +	}
> +
> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (!p)
> +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
> +
> +	v = *p;
> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);

I have concern that the raw layout inside the parameter block is
still exposed to drivers and need to be parsed by each driver.

How about we define HW agnostic IDs for parameter specific fields like:

PARAM_ID		FIELD_ID
================================
MSIX			STARTV
			NUMV
--------------------------------
CLK			FREQ
--------------------------------
FIFO			LEN
--------------------------------
REG_LAYOUT		WIDTH
			SHIFT

And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)

Think further, if we have to define HW agnostic property - value pairs,
why don't we just use "Software nodes for the firmware node", see
drivers/base/swnode.c. I think this may be a better choice.

Thanks,
Yilun

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

[matthew.gerlach]

On Sat, 29 Oct 2022, Xu Yilun wrote:

> On 2022-10-20 at 14:26:10 -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.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v4: use dev_err_probe() everywhere that is appropriate
>>     clean up noise
>>     change error messages to use the word, unsupported
>>     tried again to sort Makefile and KConfig better
>>     reorder probe function for easier error handling
>>     use new dfh_find_param API
>>
>> 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 | 149 +++++++++++++++++++++++++++++
>>  drivers/tty/serial/8250/Kconfig    |  12 +++
>>  drivers/tty/serial/8250/Makefile   |   1 +
>>  3 files changed, 162 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..f02f0ba2a565
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,149 @@
>> +// 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 dfl_device *dfl_dev, struct uart_8250_port *uart)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	u64 v, fifo_len, reg_width;
>> +	u64 *p;
>> +
>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>> +	if (!p)
>> +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>> +
>> +	uart->port.uartclk = *p;
>> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>> +
>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>> +	if (!p)
>> +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
>> +
>> +	fifo_len = *p;
>> +	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:
>> +		return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
>> +	}
>> +
>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>> +	if (!p)
>> +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
>> +
>> +	v = *p;
>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>
> I have concern that the raw layout inside the parameter block is
> still exposed to drivers and need to be parsed by each driver.

Raw parameter block will always have to be passed to the driver because HW 
specific properties can be defined that will need to be parsed by the 
specific driver.

>
> How about we define HW agnostic IDs for parameter specific fields like:
>
> PARAM_ID		FIELD_ID
> ================================
> MSIX			STARTV
> 			NUMV
> --------------------------------
> CLK			FREQ
> --------------------------------
> FIFO			LEN
> --------------------------------
> REG_LAYOUT		WIDTH
> 			SHIFT
>
> And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)

I don't think dfl_find_param as defined above adds much value.

>
> Think further, if we have to define HW agnostic property - value pairs,
> why don't we just use "Software nodes for the firmware node", see
> drivers/base/swnode.c. I think this may be a better choice.

I am looking into "Software nodes for the firmware node", and it can be 
used for HW agnostic properties.  Each dfl driver will still have to 
make a function call to fetch each HW agnostice property value as well as 
a function call to find the HW specific parameters and then parse those 
parameters.

>
> Thanks,
> Yilun
>

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

[Xu Yilun]

On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Sat, 29 Oct 2022, Xu Yilun wrote:
> 
> > On 2022-10-20 at 14:26:10 -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.
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > > v4: use dev_err_probe() everywhere that is appropriate
> > >     clean up noise
> > >     change error messages to use the word, unsupported
> > >     tried again to sort Makefile and KConfig better
> > >     reorder probe function for easier error handling
> > >     use new dfh_find_param API
> > > 
> > > 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 | 149 +++++++++++++++++++++++++++++
> > >  drivers/tty/serial/8250/Kconfig    |  12 +++
> > >  drivers/tty/serial/8250/Makefile   |   1 +
> > >  3 files changed, 162 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..f02f0ba2a565
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > @@ -0,0 +1,149 @@
> > > +// 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 dfl_device *dfl_dev, struct uart_8250_port *uart)
> > > +{
> > > +	struct device *dev = &dfl_dev->dev;
> > > +	u64 v, fifo_len, reg_width;
> > > +	u64 *p;
> > > +
> > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> > > +	if (!p)
> > > +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
> > > +
> > > +	uart->port.uartclk = *p;
> > > +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> > > +
> > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> > > +	if (!p)
> > > +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
> > > +
> > > +	fifo_len = *p;
> > > +	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:
> > > +		return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
> > > +	}
> > > +
> > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> > > +	if (!p)
> > > +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
> > > +
> > > +	v = *p;
> > > +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> > > +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> > 
> > I have concern that the raw layout inside the parameter block is
> > still exposed to drivers and need to be parsed by each driver.
> 
> Raw parameter block will always have to be passed to the driver because HW
> specific properties can be defined that will need to be parsed by the
> specific driver.

So there is a question about the scope of the definitions of these parameter
blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
seems specific to uart?

If a parameter block is widely used in dfl drivers, duplicate the parsing
from HW layout in each driver may not be a good idea. While for device
specific parameter block, it's OK.

Another concern is the indexing of the parameter IDs. If some parameter
blocks should be device specific, then no need to have globally indexed
parameter IDs. Index them locally in device is OK. So put the definitions
of ID values, HW layout and their parsing operation in each driver.

Thanks,
Yilun

> 
> > 
> > How about we define HW agnostic IDs for parameter specific fields like:
> > 
> > PARAM_ID		FIELD_ID
> > ================================
> > MSIX			STARTV
> > 			NUMV
> > --------------------------------
> > CLK			FREQ
> > --------------------------------
> > FIFO			LEN
> > --------------------------------
> > REG_LAYOUT		WIDTH
> > 			SHIFT
> > 
> > And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)
> 
> I don't think dfl_find_param as defined above adds much value.
> 
> > 
> > Think further, if we have to define HW agnostic property - value pairs,
> > why don't we just use "Software nodes for the firmware node", see
> > drivers/base/swnode.c. I think this may be a better choice.
> 
> I am looking into "Software nodes for the firmware node", and it can be used
> for HW agnostic properties.  Each dfl driver will still have to make a
> function call to fetch each HW agnostice property value as well as a
> function call to find the HW specific parameters and then parse those
> parameters.
> 
> > 
> > Thanks,
> > Yilun
> > 

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

[matthew.gerlach]

On Tue, 1 Nov 2022, Xu Yilun wrote:

> On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Sat, 29 Oct 2022, Xu Yilun wrote:
>>
>>> On 2022-10-20 at 14:26:10 -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.
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>> v4: use dev_err_probe() everywhere that is appropriate
>>>>     clean up noise
>>>>     change error messages to use the word, unsupported
>>>>     tried again to sort Makefile and KConfig better
>>>>     reorder probe function for easier error handling
>>>>     use new dfh_find_param API
>>>>
>>>> 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 | 149 +++++++++++++++++++++++++++++
>>>>  drivers/tty/serial/8250/Kconfig    |  12 +++
>>>>  drivers/tty/serial/8250/Makefile   |   1 +
>>>>  3 files changed, 162 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..f02f0ba2a565
>>>> --- /dev/null
>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>>>> @@ -0,0 +1,149 @@
>>>> +// 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 dfl_device *dfl_dev, struct uart_8250_port *uart)
>>>> +{
>>>> +	struct device *dev = &dfl_dev->dev;
>>>> +	u64 v, fifo_len, reg_width;
>>>> +	u64 *p;
>>>> +
>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>>> +	if (!p)
>>>> +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>>>> +
>>>> +	uart->port.uartclk = *p;
>>>> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>>>> +
>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>>>> +	if (!p)
>>>> +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
>>>> +
>>>> +	fifo_len = *p;
>>>> +	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:
>>>> +		return dev_err_probe(dev, -EINVAL, "unsupported fifo_len %llu\n", fifo_len);
>>>> +	}
>>>> +
>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>>>> +	if (!p)
>>>> +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
>>>> +
>>>> +	v = *p;
>>>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>>>> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>>>
>>> I have concern that the raw layout inside the parameter block is
>>> still exposed to drivers and need to be parsed by each driver.
>>
>> Raw parameter block will always have to be passed to the driver because HW
>> specific properties can be defined that will need to be parsed by the
>> specific driver.
>
> So there is a question about the scope of the definitions of these parameter
> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> seems specific to uart?

There are definitely two classes of parameter blocks.  One class is HW 
agnostic parameters where the parameters are relevant to many different 
kinds of HW components.  MSI-X, and input clock-frequency are certainly HW 
agnostic, and it turns out that REG_LAYOUT is not specific to uart.  You 
can see reg_bits and reg_stride in struct regmap_config.  There are also 
device tree bindings for reg-shift and reg-io-width.  The second class of 
parameters would be specific to HW component.  In the case of this uart 
driver, all parameters would be considered HW agnostic parameters.

>
> If a parameter block is widely used in dfl drivers, duplicate the parsing
> from HW layout in each driver may not be a good idea. While for device
> specific parameter block, it's OK.

It sounds like we are in agreement.

>
> Another concern is the indexing of the parameter IDs. If some parameter
> blocks should be device specific, then no need to have globally indexed
> parameter IDs. Index them locally in device is OK. So put the definitions
> of ID values, HW layout and their parsing operation in each driver.

It may be confusing for two drivers to use the same parameter id that have 
different meanings and data layout.  Since all the parameters for this 
driver would be considered HW agnostic, we'd don't need to address this 
issue with this patchset.

>
> Thanks,
> Yilun
>
>>
>>>
>>> How about we define HW agnostic IDs for parameter specific fields like:
>>>
>>> PARAM_ID		FIELD_ID
>>> ================================
>>> MSIX			STARTV
>>> 			NUMV
>>> --------------------------------
>>> CLK			FREQ
>>> --------------------------------
>>> FIFO			LEN
>>> --------------------------------
>>> REG_LAYOUT		WIDTH
>>> 			SHIFT
>>>
>>> And define like u64 dfl_find_param(struct dfl_device *, int param_id, int field_id)
>>
>> I don't think dfl_find_param as defined above adds much value.
>>
>>>
>>> Think further, if we have to define HW agnostic property - value pairs,
>>> why don't we just use "Software nodes for the firmware node", see
>>> drivers/base/swnode.c. I think this may be a better choice.
>>
>> I am looking into "Software nodes for the firmware node", and it can be used
>> for HW agnostic properties.  Each dfl driver will still have to make a
>> function call to fetch each HW agnostice property value as well as a
>> function call to find the HW specific parameters and then parse those
>> parameters.
>>
>>>
>>> Thanks,
>>> Yilun
>>>
>

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

[Ilpo Järvinen]

On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:

> 
> 
> On Tue, 1 Nov 2022, Xu Yilun wrote:
> 
> > On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:
> > > 
> > > 
> > > On Sat, 29 Oct 2022, Xu Yilun wrote:
> > > 
> > > > On 2022-10-20 at 14:26:10 -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.
> > > > > 
> > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > ---
> > > > > v4: use dev_err_probe() everywhere that is appropriate
> > > > >     clean up noise
> > > > >     change error messages to use the word, unsupported
> > > > >     tried again to sort Makefile and KConfig better
> > > > >     reorder probe function for easier error handling
> > > > >     use new dfh_find_param API
> > > > > 
> > > > > 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 | 149
> > > > > +++++++++++++++++++++++++++++
> > > > >  drivers/tty/serial/8250/Kconfig    |  12 +++
> > > > >  drivers/tty/serial/8250/Makefile   |   1 +
> > > > >  3 files changed, 162 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..f02f0ba2a565
> > > > > --- /dev/null
> > > > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > > > @@ -0,0 +1,149 @@
> > > > > +// 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 dfl_device *dfl_dev, struct
> > > > > uart_8250_port *uart)
> > > > > +{
> > > > > +	struct device *dev = &dfl_dev->dev;
> > > > > +	u64 v, fifo_len, reg_width;
> > > > > +	u64 *p;
> > > > > +
> > > > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> > > > > +	if (!p)
> > > > > +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
> > > > > param\n");
> > > > > +
> > > > > +	uart->port.uartclk = *p;
> > > > > +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> > > > > +
> > > > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> > > > > +	if (!p)
> > > > > +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
> > > > > param\n");
> > > > > +
> > > > > +	fifo_len = *p;
> > > > > +	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:
> > > > > +		return dev_err_probe(dev, -EINVAL, "unsupported
> > > > > fifo_len %llu\n", fifo_len);
> > > > > +	}
> > > > > +
> > > > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> > > > > +	if (!p)
> > > > > +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
> > > > > param\n");
> > > > > +
> > > > > +	v = *p;
> > > > > +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> > > > > +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> > > > 
> > > > I have concern that the raw layout inside the parameter block is
> > > > still exposed to drivers and need to be parsed by each driver.
> > > 
> > > Raw parameter block will always have to be passed to the driver because HW
> > > specific properties can be defined that will need to be parsed by the
> > > specific driver.
> > 
> > So there is a question about the scope of the definitions of these parameter
> > blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> > seems specific to uart?
> 
> There are definitely two classes of parameter blocks.  One class is HW
> agnostic parameters where the parameters are relevant to many different kinds
> of HW components.  MSI-X, and input clock-frequency are certainly HW agnostic,
> and it turns out that REG_LAYOUT is not specific to uart.  You can see
> reg_bits and reg_stride in struct regmap_config.  There are also device tree
> bindings for reg-shift and reg-io-width.  The second class of parameters would
> be specific to HW component.  In the case of this uart driver, all parameters
> would be considered HW agnostic parameters.
> 
> > 
> > If a parameter block is widely used in dfl drivers, duplicate the parsing
> > from HW layout in each driver may not be a good idea. While for device
> > specific parameter block, it's OK.
> 
> It sounds like we are in agreement.
> 
> > 
> > Another concern is the indexing of the parameter IDs. If some parameter
> > blocks should be device specific, then no need to have globally indexed
> > parameter IDs. Index them locally in device is OK. So put the definitions
> > of ID values, HW layout and their parsing operation in each driver.
> 
> It may be confusing for two drivers to use the same parameter id that have
> different meanings and data layout.  Since all the parameters for this driver
> would be considered HW agnostic, we'd don't need to address this issue with
> this patchset.
>
> > > > How about we define HW agnostic IDs for parameter specific fields like:
> > > > 
> > > > PARAM_ID		FIELD_ID
> > > > ================================
> > > > MSIX			STARTV
> > > > 			NUMV
> > > > --------------------------------
> > > > CLK			FREQ
> > > > --------------------------------
> > > > FIFO			LEN
> > > > --------------------------------
> > > > REG_LAYOUT		WIDTH
> > > > 			SHIFT
> > > > 
> > > > And define like u64 dfl_find_param(struct dfl_device *, int param_id,
> > > > int field_id)
> > > 
> > > I don't think dfl_find_param as defined above adds much value.
> > > 
> > > > 
> > > > Think further, if we have to define HW agnostic property - value pairs,
> > > > why don't we just use "Software nodes for the firmware node", see
> > > > drivers/base/swnode.c. I think this may be a better choice.
> > > 
> > > I am looking into "Software nodes for the firmware node", and it can be
> > > used
> > > for HW agnostic properties.  Each dfl driver will still have to make a
> > > function call to fetch each HW agnostice property value as well as a
> > > function call to find the HW specific parameters and then parse those
> > > parameters.

Btw, another aspect this discussion has completely overlooked is the 
presence of parameter version and how it impacts data layout. Is v1 
always going be a subset of v2 or can a later version remove something
v1 had?

-- 
 i.

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

[matthew.gerlach]

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



On Tue, 1 Nov 2022, Ilpo Järvinen wrote:

> On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:
>
>>
>>
>> On Tue, 1 Nov 2022, Xu Yilun wrote:
>>
>>> On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:
>>>>
>>>>
>>>> On Sat, 29 Oct 2022, Xu Yilun wrote:
>>>>
>>>>> On 2022-10-20 at 14:26:10 -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.
>>>>>>
>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>> ---
>>>>>> v4: use dev_err_probe() everywhere that is appropriate
>>>>>>     clean up noise
>>>>>>     change error messages to use the word, unsupported
>>>>>>     tried again to sort Makefile and KConfig better
>>>>>>     reorder probe function for easier error handling
>>>>>>     use new dfh_find_param API
>>>>>>
>>>>>> 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 | 149
>>>>>> +++++++++++++++++++++++++++++
>>>>>>  drivers/tty/serial/8250/Kconfig    |  12 +++
>>>>>>  drivers/tty/serial/8250/Makefile   |   1 +
>>>>>>  3 files changed, 162 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..f02f0ba2a565
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>>>>>> @@ -0,0 +1,149 @@
>>>>>> +// 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 dfl_device *dfl_dev, struct
>>>>>> uart_8250_port *uart)
>>>>>> +{
>>>>>> +	struct device *dev = &dfl_dev->dev;
>>>>>> +	u64 v, fifo_len, reg_width;
>>>>>> +	u64 *p;
>>>>>> +
>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>>>>> +	if (!p)
>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
>>>>>> param\n");
>>>>>> +
>>>>>> +	uart->port.uartclk = *p;
>>>>>> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>>>>>> +
>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>>>>>> +	if (!p)
>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
>>>>>> param\n");
>>>>>> +
>>>>>> +	fifo_len = *p;
>>>>>> +	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:
>>>>>> +		return dev_err_probe(dev, -EINVAL, "unsupported
>>>>>> fifo_len %llu\n", fifo_len);
>>>>>> +	}
>>>>>> +
>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>>>>>> +	if (!p)
>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
>>>>>> param\n");
>>>>>> +
>>>>>> +	v = *p;
>>>>>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>>>>>> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>>>>>
>>>>> I have concern that the raw layout inside the parameter block is
>>>>> still exposed to drivers and need to be parsed by each driver.
>>>>
>>>> Raw parameter block will always have to be passed to the driver because HW
>>>> specific properties can be defined that will need to be parsed by the
>>>> specific driver.
>>>
>>> So there is a question about the scope of the definitions of these parameter
>>> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
>>> seems specific to uart?
>>
>> There are definitely two classes of parameter blocks.  One class is HW
>> agnostic parameters where the parameters are relevant to many different kinds
>> of HW components.  MSI-X, and input clock-frequency are certainly HW agnostic,
>> and it turns out that REG_LAYOUT is not specific to uart.  You can see
>> reg_bits and reg_stride in struct regmap_config.  There are also device tree
>> bindings for reg-shift and reg-io-width.  The second class of parameters would
>> be specific to HW component.  In the case of this uart driver, all parameters
>> would be considered HW agnostic parameters.
>>
>>>
>>> If a parameter block is widely used in dfl drivers, duplicate the parsing
>>> from HW layout in each driver may not be a good idea. While for device
>>> specific parameter block, it's OK.
>>
>> It sounds like we are in agreement.
>>
>>>
>>> Another concern is the indexing of the parameter IDs. If some parameter
>>> blocks should be device specific, then no need to have globally indexed
>>> parameter IDs. Index them locally in device is OK. So put the definitions
>>> of ID values, HW layout and their parsing operation in each driver.
>>
>> It may be confusing for two drivers to use the same parameter id that have
>> different meanings and data layout.  Since all the parameters for this driver
>> would be considered HW agnostic, we'd don't need to address this issue with
>> this patchset.
>>
>>>>> How about we define HW agnostic IDs for parameter specific fields like:
>>>>>
>>>>> PARAM_ID		FIELD_ID
>>>>> ================================
>>>>> MSIX			STARTV
>>>>> 			NUMV
>>>>> --------------------------------
>>>>> CLK			FREQ
>>>>> --------------------------------
>>>>> FIFO			LEN
>>>>> --------------------------------
>>>>> REG_LAYOUT		WIDTH
>>>>> 			SHIFT
>>>>>
>>>>> And define like u64 dfl_find_param(struct dfl_device *, int param_id,
>>>>> int field_id)
>>>>
>>>> I don't think dfl_find_param as defined above adds much value.
>>>>
>>>>>
>>>>> Think further, if we have to define HW agnostic property - value pairs,
>>>>> why don't we just use "Software nodes for the firmware node", see
>>>>> drivers/base/swnode.c. I think this may be a better choice.
>>>>
>>>> I am looking into "Software nodes for the firmware node", and it can be
>>>> used
>>>> for HW agnostic properties.  Each dfl driver will still have to make a
>>>> function call to fetch each HW agnostice property value as well as a
>>>> function call to find the HW specific parameters and then parse those
>>>> parameters.
>
> Btw, another aspect this discussion has completely overlooked is the
> presence of parameter version and how it impacts data layout. Is v1
> always going be a subset of v2 or can a later version remove something
> v1 had?

In general it would be preferable for v1 to be a subset of v2.  This 
allows for v1 SW to work on v2 HW.

>
> -- 
> i.
>

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

[Ilpo Järvinen]

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

On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:

> 
> 
> On Tue, 1 Nov 2022, Ilpo Järvinen wrote:
> 
> > On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:
> > 
> > > 
> > > 
> > > On Tue, 1 Nov 2022, Xu Yilun wrote:
> > > 
> > > > On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:
> > > > > 
> > > > > 
> > > > > On Sat, 29 Oct 2022, Xu Yilun wrote:
> > > > > 
> > > > > > On 2022-10-20 at 14:26:10 -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.
> > > > > > > 
> > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > ---
> > > > > > > v4: use dev_err_probe() everywhere that is appropriate
> > > > > > >     clean up noise
> > > > > > >     change error messages to use the word, unsupported
> > > > > > >     tried again to sort Makefile and KConfig better
> > > > > > >     reorder probe function for easier error handling
> > > > > > >     use new dfh_find_param API
> > > > > > > 
> > > > > > > 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 | 149
> > > > > > > +++++++++++++++++++++++++++++
> > > > > > >  drivers/tty/serial/8250/Kconfig    |  12 +++
> > > > > > >  drivers/tty/serial/8250/Makefile   |   1 +
> > > > > > >  3 files changed, 162 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..f02f0ba2a565
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/tty/serial/8250/8250_dfl.c
> > > > > > > @@ -0,0 +1,149 @@
> > > > > > > +// 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 dfl_device *dfl_dev, struct
> > > > > > > uart_8250_port *uart)
> > > > > > > +{
> > > > > > > +	struct device *dev = &dfl_dev->dev;
> > > > > > > +	u64 v, fifo_len, reg_width;
> > > > > > > +	u64 *p;
> > > > > > > +
> > > > > > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> > > > > > > +	if (!p)
> > > > > > > +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
> > > > > > > param\n");
> > > > > > > +
> > > > > > > +	uart->port.uartclk = *p;
> > > > > > > +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> > > > > > > +
> > > > > > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> > > > > > > +	if (!p)
> > > > > > > +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
> > > > > > > param\n");
> > > > > > > +
> > > > > > > +	fifo_len = *p;
> > > > > > > +	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:
> > > > > > > +		return dev_err_probe(dev, -EINVAL, "unsupported
> > > > > > > fifo_len %llu\n", fifo_len);
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> > > > > > > +	if (!p)
> > > > > > > +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
> > > > > > > param\n");
> > > > > > > +
> > > > > > > +	v = *p;
> > > > > > > +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> > > > > > > +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> > > > > > 
> > > > > > I have concern that the raw layout inside the parameter block is
> > > > > > still exposed to drivers and need to be parsed by each driver.
> > > > > 
> > > > > Raw parameter block will always have to be passed to the driver
> > > > > because HW
> > > > > specific properties can be defined that will need to be parsed by the
> > > > > specific driver.
> > > > 
> > > > So there is a question about the scope of the definitions of these
> > > > parameter
> > > > blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> > > > seems specific to uart?
> > > 
> > > There are definitely two classes of parameter blocks.  One class is HW
> > > agnostic parameters where the parameters are relevant to many different
> > > kinds
> > > of HW components.  MSI-X, and input clock-frequency are certainly HW
> > > agnostic,
> > > and it turns out that REG_LAYOUT is not specific to uart.  You can see
> > > reg_bits and reg_stride in struct regmap_config.  There are also device
> > > tree
> > > bindings for reg-shift and reg-io-width.  The second class of parameters
> > > would
> > > be specific to HW component.  In the case of this uart driver, all
> > > parameters
> > > would be considered HW agnostic parameters.
> > > 
> > > > 
> > > > If a parameter block is widely used in dfl drivers, duplicate the
> > > > parsing
> > > > from HW layout in each driver may not be a good idea. While for device
> > > > specific parameter block, it's OK.
> > > 
> > > It sounds like we are in agreement.
> > > 
> > > > 
> > > > Another concern is the indexing of the parameter IDs. If some parameter
> > > > blocks should be device specific, then no need to have globally indexed
> > > > parameter IDs. Index them locally in device is OK. So put the
> > > > definitions
> > > > of ID values, HW layout and their parsing operation in each driver.
> > > 
> > > It may be confusing for two drivers to use the same parameter id that have
> > > different meanings and data layout.  Since all the parameters for this
> > > driver
> > > would be considered HW agnostic, we'd don't need to address this issue
> > > with
> > > this patchset.
> > > 
> > > > > > How about we define HW agnostic IDs for parameter specific fields
> > > > > > like:
> > > > > > 
> > > > > > PARAM_ID		FIELD_ID
> > > > > > ================================
> > > > > > MSIX			STARTV
> > > > > > 			NUMV
> > > > > > --------------------------------
> > > > > > CLK			FREQ
> > > > > > --------------------------------
> > > > > > FIFO			LEN
> > > > > > --------------------------------
> > > > > > REG_LAYOUT		WIDTH
> > > > > > 			SHIFT
> > > > > > 
> > > > > > And define like u64 dfl_find_param(struct dfl_device *, int
> > > > > > param_id,
> > > > > > int field_id)
> > > > > 
> > > > > I don't think dfl_find_param as defined above adds much value.
> > > > > 
> > > > > > 
> > > > > > Think further, if we have to define HW agnostic property - value
> > > > > > pairs,
> > > > > > why don't we just use "Software nodes for the firmware node", see
> > > > > > drivers/base/swnode.c. I think this may be a better choice.
> > > > > 
> > > > > I am looking into "Software nodes for the firmware node", and it can
> > > > > be
> > > > > used
> > > > > for HW agnostic properties.  Each dfl driver will still have to make a
> > > > > function call to fetch each HW agnostice property value as well as a
> > > > > function call to find the HW specific parameters and then parse those
> > > > > parameters.
> > 
> > Btw, another aspect this discussion has completely overlooked is the
> > presence of parameter version and how it impacts data layout. Is v1
> > always going be a subset of v2 or can a later version remove something
> > v1 had?
> 
> In general it would be preferable for v1 to be a subset of v2.  This allows
> for v1 SW to work on v2 HW.

In that case, shouldn't the minimum acceptable version be part of 
dfh_find_param() parameters?

Currently there's no way for the caller to even look what version the 
parameter is from dfh_find_param()'s return value (except with some 
negative offset hack to access parameter header).


-- 
 i.

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

[Marco Pagani]

On 2022-11-02 10:57, Ilpo Järvinen wrote:
> On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:
> 
>>
>>
>> On Tue, 1 Nov 2022, Ilpo Järvinen wrote:
>>
>>> On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:
>>>
>>>>
>>>>
>>>> On Tue, 1 Nov 2022, Xu Yilun wrote:
>>>>
>>>>> On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:
>>>>>>
>>>>>>
>>>>>> On Sat, 29 Oct 2022, Xu Yilun wrote:
>>>>>>
>>>>>>> On 2022-10-20 at 14:26:10 -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.
>>>>>>>>
>>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>>> ---
>>>>>>>> v4: use dev_err_probe() everywhere that is appropriate
>>>>>>>>     clean up noise
>>>>>>>>     change error messages to use the word, unsupported
>>>>>>>>     tried again to sort Makefile and KConfig better
>>>>>>>>     reorder probe function for easier error handling
>>>>>>>>     use new dfh_find_param API
>>>>>>>>
>>>>>>>> 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 | 149
>>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>>  drivers/tty/serial/8250/Kconfig    |  12 +++
>>>>>>>>  drivers/tty/serial/8250/Makefile   |   1 +
>>>>>>>>  3 files changed, 162 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..f02f0ba2a565
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>>>>>>>> @@ -0,0 +1,149 @@
>>>>>>>> +// 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 dfl_device *dfl_dev, struct
>>>>>>>> uart_8250_port *uart)
>>>>>>>> +{
>>>>>>>> +	struct device *dev = &dfl_dev->dev;
>>>>>>>> +	u64 v, fifo_len, reg_width;
>>>>>>>> +	u64 *p;
>>>>>>>> +
>>>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>>>>>>> +	if (!p)
>>>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
>>>>>>>> param\n");
>>>>>>>> +
>>>>>>>> +	uart->port.uartclk = *p;
>>>>>>>> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
>>>>>>>> +
>>>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
>>>>>>>> +	if (!p)
>>>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
>>>>>>>> param\n");
>>>>>>>> +
>>>>>>>> +	fifo_len = *p;
>>>>>>>> +	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:
>>>>>>>> +		return dev_err_probe(dev, -EINVAL, "unsupported
>>>>>>>> fifo_len %llu\n", fifo_len);
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
>>>>>>>> +	if (!p)
>>>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
>>>>>>>> param\n");
>>>>>>>> +
>>>>>>>> +	v = *p;
>>>>>>>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
>>>>>>>> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
>>>>>>>
>>>>>>> I have concern that the raw layout inside the parameter block is
>>>>>>> still exposed to drivers and need to be parsed by each driver.
>>>>>>
>>>>>> Raw parameter block will always have to be passed to the driver
>>>>>> because HW
>>>>>> specific properties can be defined that will need to be parsed by the
>>>>>> specific driver.
>>>>>
>>>>> So there is a question about the scope of the definitions of these
>>>>> parameter
>>>>> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
>>>>> seems specific to uart?
>>>>
>>>> There are definitely two classes of parameter blocks.  One class is HW
>>>> agnostic parameters where the parameters are relevant to many different
>>>> kinds
>>>> of HW components.  MSI-X, and input clock-frequency are certainly HW
>>>> agnostic,
>>>> and it turns out that REG_LAYOUT is not specific to uart.  You can see
>>>> reg_bits and reg_stride in struct regmap_config.  There are also device
>>>> tree
>>>> bindings for reg-shift and reg-io-width.  The second class of parameters
>>>> would
>>>> be specific to HW component.  In the case of this uart driver, all
>>>> parameters
>>>> would be considered HW agnostic parameters.
>>>>
>>>>>
>>>>> If a parameter block is widely used in dfl drivers, duplicate the
>>>>> parsing
>>>>> from HW layout in each driver may not be a good idea. While for device
>>>>> specific parameter block, it's OK.
>>>>
>>>> It sounds like we are in agreement.
>>>>
>>>>>
>>>>> Another concern is the indexing of the parameter IDs. If some parameter
>>>>> blocks should be device specific, then no need to have globally indexed
>>>>> parameter IDs. Index them locally in device is OK. So put the
>>>>> definitions
>>>>> of ID values, HW layout and their parsing operation in each driver.
>>>>
>>>> It may be confusing for two drivers to use the same parameter id that have
>>>> different meanings and data layout.  Since all the parameters for this
>>>> driver
>>>> would be considered HW agnostic, we'd don't need to address this issue
>>>> with
>>>> this patchset.
>>>>
>>>>>>> How about we define HW agnostic IDs for parameter specific fields
>>>>>>> like:
>>>>>>>
>>>>>>> PARAM_ID		FIELD_ID
>>>>>>> ================================
>>>>>>> MSIX			STARTV
>>>>>>> 			NUMV
>>>>>>> --------------------------------
>>>>>>> CLK			FREQ
>>>>>>> --------------------------------
>>>>>>> FIFO			LEN
>>>>>>> --------------------------------
>>>>>>> REG_LAYOUT		WIDTH
>>>>>>> 			SHIFT
>>>>>>>
>>>>>>> And define like u64 dfl_find_param(struct dfl_device *, int
>>>>>>> param_id,
>>>>>>> int field_id)
>>>>>>
>>>>>> I don't think dfl_find_param as defined above adds much value.
>>>>>>
>>>>>>>
>>>>>>> Think further, if we have to define HW agnostic property - value
>>>>>>> pairs,
>>>>>>> why don't we just use "Software nodes for the firmware node", see
>>>>>>> drivers/base/swnode.c. I think this may be a better choice.
>>>>>>
>>>>>> I am looking into "Software nodes for the firmware node", and it can
>>>>>> be
>>>>>> used
>>>>>> for HW agnostic properties.  Each dfl driver will still have to make a
>>>>>> function call to fetch each HW agnostice property value as well as a
>>>>>> function call to find the HW specific parameters and then parse those
>>>>>> parameters.
>>>
>>> Btw, another aspect this discussion has completely overlooked is the
>>> presence of parameter version and how it impacts data layout. Is v1
>>> always going be a subset of v2 or can a later version remove something
>>> v1 had?
>>
>> In general it would be preferable for v1 to be a subset of v2.  This allows
>> for v1 SW to work on v2 HW.
> 
> In that case, shouldn't the minimum acceptable version be part of 
> dfh_find_param() parameters?
> 
> Currently there's no way for the caller to even look what version the 
> parameter is from dfh_find_param()'s return value (except with some 
> negative offset hack to access parameter header).
> 
> 

Why not just checking dfl_dev->dfh_version in dfl_uart_probe() before
calling dfh_find_param()? In general, any dfl_driver could potentially
do this check in its *_probe() function before reading the header to avoid
compatibility issues.

Cheers,
Marco

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

[Ilpo Järvinen]

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

On Tue, 8 Nov 2022, Marco Pagani wrote:

> 
> On 2022-11-02 10:57, Ilpo Järvinen wrote:
> > On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:
> > 
> >>
> >>
> >> On Tue, 1 Nov 2022, Ilpo Järvinen wrote:
> >>
> >>> On Tue, 1 Nov 2022, matthew.gerlach@linux.intel.com wrote:
> >>>
> >>>>
> >>>>
> >>>> On Tue, 1 Nov 2022, Xu Yilun wrote:
> >>>>
> >>>>> On 2022-10-31 at 17:34:39 -0700, matthew.gerlach@linux.intel.com wrote:
> >>>>>>
> >>>>>>
> >>>>>> On Sat, 29 Oct 2022, Xu Yilun wrote:
> >>>>>>
> >>>>>>> On 2022-10-20 at 14:26:10 -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.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >>>>>>>> ---
> >>>>>>>> v4: use dev_err_probe() everywhere that is appropriate
> >>>>>>>>     clean up noise
> >>>>>>>>     change error messages to use the word, unsupported
> >>>>>>>>     tried again to sort Makefile and KConfig better
> >>>>>>>>     reorder probe function for easier error handling
> >>>>>>>>     use new dfh_find_param API
> >>>>>>>>
> >>>>>>>> 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 | 149
> >>>>>>>> +++++++++++++++++++++++++++++
> >>>>>>>>  drivers/tty/serial/8250/Kconfig    |  12 +++
> >>>>>>>>  drivers/tty/serial/8250/Makefile   |   1 +
> >>>>>>>>  3 files changed, 162 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..f02f0ba2a565
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/drivers/tty/serial/8250/8250_dfl.c
> >>>>>>>> @@ -0,0 +1,149 @@
> >>>>>>>> +// 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 dfl_device *dfl_dev, struct
> >>>>>>>> uart_8250_port *uart)
> >>>>>>>> +{
> >>>>>>>> +	struct device *dev = &dfl_dev->dev;
> >>>>>>>> +	u64 v, fifo_len, reg_width;
> >>>>>>>> +	u64 *p;
> >>>>>>>> +
> >>>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> >>>>>>>> +	if (!p)
> >>>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ
> >>>>>>>> param\n");
> >>>>>>>> +
> >>>>>>>> +	uart->port.uartclk = *p;
> >>>>>>>> +	dev_dbg(dev, "UART_CLK_ID %u Hz\n", uart->port.uartclk);
> >>>>>>>> +
> >>>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
> >>>>>>>> +	if (!p)
> >>>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN
> >>>>>>>> param\n");
> >>>>>>>> +
> >>>>>>>> +	fifo_len = *p;
> >>>>>>>> +	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:
> >>>>>>>> +		return dev_err_probe(dev, -EINVAL, "unsupported
> >>>>>>>> fifo_len %llu\n", fifo_len);
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
> >>>>>>>> +	if (!p)
> >>>>>>>> +		return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT
> >>>>>>>> param\n");
> >>>>>>>> +
> >>>>>>>> +	v = *p;
> >>>>>>>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> >>>>>>>> +	reg_width = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
> >>>>>>>
> >>>>>>> I have concern that the raw layout inside the parameter block is
> >>>>>>> still exposed to drivers and need to be parsed by each driver.
> >>>>>>
> >>>>>> Raw parameter block will always have to be passed to the driver
> >>>>>> because HW
> >>>>>> specific properties can be defined that will need to be parsed by the
> >>>>>> specific driver.
> >>>>>
> >>>>> So there is a question about the scope of the definitions of these
> >>>>> parameter
> >>>>> blocks. MSIX seems globally used across all dfl devices. REG_LAYOUT
> >>>>> seems specific to uart?
> >>>>
> >>>> There are definitely two classes of parameter blocks.  One class is HW
> >>>> agnostic parameters where the parameters are relevant to many different
> >>>> kinds
> >>>> of HW components.  MSI-X, and input clock-frequency are certainly HW
> >>>> agnostic,
> >>>> and it turns out that REG_LAYOUT is not specific to uart.  You can see
> >>>> reg_bits and reg_stride in struct regmap_config.  There are also device
> >>>> tree
> >>>> bindings for reg-shift and reg-io-width.  The second class of parameters
> >>>> would
> >>>> be specific to HW component.  In the case of this uart driver, all
> >>>> parameters
> >>>> would be considered HW agnostic parameters.
> >>>>
> >>>>>
> >>>>> If a parameter block is widely used in dfl drivers, duplicate the
> >>>>> parsing
> >>>>> from HW layout in each driver may not be a good idea. While for device
> >>>>> specific parameter block, it's OK.
> >>>>
> >>>> It sounds like we are in agreement.
> >>>>
> >>>>>
> >>>>> Another concern is the indexing of the parameter IDs. If some parameter
> >>>>> blocks should be device specific, then no need to have globally indexed
> >>>>> parameter IDs. Index them locally in device is OK. So put the
> >>>>> definitions
> >>>>> of ID values, HW layout and their parsing operation in each driver.
> >>>>
> >>>> It may be confusing for two drivers to use the same parameter id that have
> >>>> different meanings and data layout.  Since all the parameters for this
> >>>> driver
> >>>> would be considered HW agnostic, we'd don't need to address this issue
> >>>> with
> >>>> this patchset.
> >>>>
> >>>>>>> How about we define HW agnostic IDs for parameter specific fields
> >>>>>>> like:
> >>>>>>>
> >>>>>>> PARAM_ID		FIELD_ID
> >>>>>>> ================================
> >>>>>>> MSIX			STARTV
> >>>>>>> 			NUMV
> >>>>>>> --------------------------------
> >>>>>>> CLK			FREQ
> >>>>>>> --------------------------------
> >>>>>>> FIFO			LEN
> >>>>>>> --------------------------------
> >>>>>>> REG_LAYOUT		WIDTH
> >>>>>>> 			SHIFT
> >>>>>>>
> >>>>>>> And define like u64 dfl_find_param(struct dfl_device *, int
> >>>>>>> param_id,
> >>>>>>> int field_id)
> >>>>>>
> >>>>>> I don't think dfl_find_param as defined above adds much value.
> >>>>>>
> >>>>>>>
> >>>>>>> Think further, if we have to define HW agnostic property - value
> >>>>>>> pairs,
> >>>>>>> why don't we just use "Software nodes for the firmware node", see
> >>>>>>> drivers/base/swnode.c. I think this may be a better choice.
> >>>>>>
> >>>>>> I am looking into "Software nodes for the firmware node", and it can
> >>>>>> be
> >>>>>> used
> >>>>>> for HW agnostic properties.  Each dfl driver will still have to make a
> >>>>>> function call to fetch each HW agnostice property value as well as a
> >>>>>> function call to find the HW specific parameters and then parse those
> >>>>>> parameters.
> >>>
> >>> Btw, another aspect this discussion has completely overlooked is the
> >>> presence of parameter version and how it impacts data layout. Is v1
> >>> always going be a subset of v2 or can a later version remove something
> >>> v1 had?
> >>
> >> In general it would be preferable for v1 to be a subset of v2.  This allows
> >> for v1 SW to work on v2 HW.
> > 
> > In that case, shouldn't the minimum acceptable version be part of 
> > dfh_find_param() parameters?
> > 
> > Currently there's no way for the caller to even look what version the 
> > parameter is from dfh_find_param()'s return value (except with some 
> > negative offset hack to access parameter header).
> > 
> > 
> 
> Why not just checking dfl_dev->dfh_version in dfl_uart_probe() before
> calling dfh_find_param()? In general, any dfl_driver could potentially
> do this check in its *_probe() function before reading the header to avoid
> compatibility issues.

It's about a different version. DFH has it's own version and every
parameter header has a separate version specific to that parameter.

-- 
 i.