[PATCH v2 0/6] Enhance definition of DFH and use enhancements for uart driver

[PATCH v2 0/6] 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 moves some of the DFH definitions to include/linux/dfl.h so that
they can be accessed by drivers outside of drivers/fpga.

Patch 3 adds the definitions for DFHv1.

Patch 4 defines and uses a DFHv1 parameter to provide a generic mechanism for
describing MSIX interrupts used by a particular feature instance.

Patch 5 gets the location and size of the feature's register set from DFHv1.

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

Basheer Ahmed Muddebihal (2):
  fpga: dfl: Move the DFH definitions
  fpga: dfl: Add DFHv1 Register Definitions

Matthew Gerlach (4):
  Documentation: fpga: dfl: Add documentation for DFHv1
  fpga: dfl: add generic support for MSIX interrupts
  fpga: dfl: parse the location of the feature's registers from DFHv1
  tty: serial: 8250: add DFL bus driver for Altera 16550.

 Documentation/fpga/dfl.rst         |  49 ++++++++
 drivers/fpga/dfl-afu-main.c        |   4 +-
 drivers/fpga/dfl.c                 |  88 ++++++++++++--
 drivers/fpga/dfl.h                 |  24 +---
 drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |   9 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 include/linux/dfl.h                |  79 ++++++++++++-
 8 files changed, 402 insertions(+), 29 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

-- 
2.25.1

[PATCH v2 1/6] 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>
---
v2: s/GUILD/GUID/
    add picture
---
 Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

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

Re: [PATCH v2 1/6] Documentation: fpga: dfl: Add documentation for DFHv1

[Ilpo Järvinen]

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

Is something missing here given the layout is claimed (in 2/6) to be:

"DFHv1 Register Offset definitons
In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"

?

> +    |63 Size of register set  32|Params 31|30 Group    16|15 Instance      0|
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
> +                                  ...
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
>  Open discussion
>  ===============
>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> 

-- 
 i.

Re: [PATCH v2 1/6] Documentation: fpga: dfl: Add documentation for DFHv1

[Ilpo Järvinen]

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

On Fri, 23 Sep 2022, Ilpo Järvinen wrote:

> On Fri, 23 Sep 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>
> > ---
> > v2: s/GUILD/GUID/
> >     add picture
> > ---
> >  Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 15b670926084..7c786b75b498 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
> >  driver's id_table.
> >  
> >  
> > +Extending the Device Feature Header - DFHv1
> > +===========================================
> > +The current 8 bytes of the Device Feature Header, hereafter referred to as
> > +to DFHv0, provide very little opportunity for the hardware to describe itself
> > +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
> > +to provide increased flexibility and extensibility to hardware designs using
> > +Device Feature Lists.  The list below describes some of the goals behind the
> > +changes in DFHv1:
> > +
> > +* Provide a standardized mechanism for features to describe
> > +  parameters/capabilities to software.
> > +* Standardize the use of a GUID for all DFHv1 types.
> > +* Decouple the location of the DFH from the register space of the feature itself.
> > +
> > +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
> > +a list of parameter values to a particular feature.
> > +
> > +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUID standard
> > +across all types.
> > +
> > +With DFHv0, the register map of a given feature is located immediately following
> > +the DFHv0 in the memory space.  With DFHv1, the location of the feature register
> > +map can be specified as an offset to the DFHv1 or as an absolute address.  The DFHv1
> > +structure is shown below:
> > +
> > +    +-----------------------------------------------------------------------+
> > +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
> > +    +-----------------------------------------------------------------------+
> > +    |63                                 GUID_L                             0|
> > +    +-----------------------------------------------------------------------+
> > +    |63                                 GUID_H                             0|
> > +    +-----------------------------------------------------------------------+
> > +    |63                 Address/Offset                            1|  Rel  0|
> > +    +-----------------------------------------------------------------------+
> 
> Is something missing here given the layout is claimed (in 2/6) to be:
> 
> "DFHv1 Register Offset definitons
> In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"
> 
> ?

Ah, I think I've figured it out, PARAM_HDR + PARAM_DATA combo is repeated 
n times (rather than the params being covered by the "PARAM_DATA")?

-- 
 i.

> > +    |63 Size of register set  32|Params 31|30 Group    16|15 Instance      0|
> > +    +-----------------------------------------------------------------------+
> > +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> > +    +-----------------------------------------------------------------------+
> > +    |63                 Parameter Data                                     0|
> > +    +-----------------------------------------------------------------------+
> > +
> > +                                  ...
> > +
> > +    +-----------------------------------------------------------------------+
> > +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> > +    +-----------------------------------------------------------------------+
> > +    |63                 Parameter Data                                     0|
> > +    +-----------------------------------------------------------------------+
> > +
> >  Open discussion
> >  ===============
> >  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
> > 
> 
> 

Re: [PATCH v2 1/6] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

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



On Fri, 23 Sep 2022, Ilpo Järvinen wrote:

> On Fri, 23 Sep 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>
>> ---
>> v2: s/GUILD/GUID/
>>     add picture
>> ---
>>  Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 15b670926084..7c786b75b498 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id should be added to the
>>  driver's id_table.
>>
>>
>> +Extending the Device Feature Header - DFHv1
>> +===========================================
>> +The current 8 bytes of the Device Feature Header, hereafter referred to as
>> +to DFHv0, provide very little opportunity for the hardware to describe itself
>> +to software. Version 1 of the Device Feature Header (DFHv1) is being introduced
>> +to provide increased flexibility and extensibility to hardware designs using
>> +Device Feature Lists.  The list below describes some of the goals behind the
>> +changes in DFHv1:
>> +
>> +* Provide a standardized mechanism for features to describe
>> +  parameters/capabilities to software.
>> +* Standardize the use of a GUID for all DFHv1 types.
>> +* Decouple the location of the DFH from the register space of the feature itself.
>> +
>> +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to associate
>> +a list of parameter values to a particular feature.
>> +
>> +With DFHv0, not all features types contained a GUID.  DFHv1 makes the GUID standard
>> +across all types.
>> +
>> +With DFHv0, the register map of a given feature is located immediately following
>> +the DFHv0 in the memory space.  With DFHv1, the location of the feature register
>> +map can be specified as an offset to the DFHv1 or as an absolute address.  The DFHv1
>> +structure is shown below:
>> +
>> +    +-----------------------------------------------------------------------+
>> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_L                             0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                                 GUID_H                             0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Address/Offset                            1|  Rel  0|
>> +    +-----------------------------------------------------------------------+
>
> Is something missing here given the layout is claimed (in 2/6) to be:
>
> "DFHv1 Register Offset definitons
> In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"
>
> ?


I was hesitant to have a picture because the description would then be in 
two places.  I suspect my picture is not clear, but it does line up with 
the offset definitions:

DFH offset 0x0
GUID offsets 0x8 and 0x10
CSR_START offset 0x18
CSR_SIZE offset 0x20
First PARAM_HDR, if it exists, is 0x28,
First PARAM_DATA, if it exists, is 0x30.

>
>> +    |63 Size of register set  32|Params 31|30 Group    16|15 Instance      0|
>> +    +-----------------------------------------------------------------------+
>> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data                                     0|
>> +    +-----------------------------------------------------------------------+
>> +
>> +                                  ...
>> +
>> +    +-----------------------------------------------------------------------+
>> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
>> +    +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data                                     0|
>> +    +-----------------------------------------------------------------------+
>> +
>>  Open discussion
>>  ===============
>>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
>>
>
> -- 
> i.
>
>

Re: [PATCH v2 1/6] Documentation: fpga: dfl: Add documentation for DFHv1

[Ilpo Järvinen]

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

On Tue, 27 Sep 2022, matthew.gerlach@linux.intel.com wrote:

> 
> 
> On Fri, 23 Sep 2022, Ilpo Järvinen wrote:
> 
> > On Fri, 23 Sep 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>
> > > ---
> > > v2: s/GUILD/GUID/
> > >     add picture
> > > ---
> > >  Documentation/fpga/dfl.rst | 49 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 49 insertions(+)
> > > 
> > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > > index 15b670926084..7c786b75b498 100644
> > > --- a/Documentation/fpga/dfl.rst
> > > +++ b/Documentation/fpga/dfl.rst
> > > @@ -561,6 +561,55 @@ new DFL feature via UIO direct access, its feature id
> > > should be added to the
> > >  driver's id_table.
> > > 
> > > 
> > > +Extending the Device Feature Header - DFHv1
> > > +===========================================
> > > +The current 8 bytes of the Device Feature Header, hereafter referred to
> > > as
> > > +to DFHv0, provide very little opportunity for the hardware to describe
> > > itself
> > > +to software. Version 1 of the Device Feature Header (DFHv1) is being
> > > introduced
> > > +to provide increased flexibility and extensibility to hardware designs
> > > using
> > > +Device Feature Lists.  The list below describes some of the goals behind
> > > the
> > > +changes in DFHv1:
> > > +
> > > +* Provide a standardized mechanism for features to describe
> > > +  parameters/capabilities to software.
> > > +* Standardize the use of a GUID for all DFHv1 types.
> > > +* Decouple the location of the DFH from the register space of the feature
> > > itself.
> > > +
> > > +Modeled after PCI Capabilities, DFHv1 Parameters provide a mechanism to
> > > associate
> > > +a list of parameter values to a particular feature.
> > > +
> > > +With DFHv0, not all features types contained a GUID.  DFHv1 makes the
> > > GUID standard
> > > +across all types.
> > > +
> > > +With DFHv0, the register map of a given feature is located immediately
> > > following
> > > +the DFHv0 in the memory space.  With DFHv1, the location of the feature
> > > register
> > > +map can be specified as an offset to the DFHv1 or as an absolute address.
> > > The DFHv1
> > > +structure is shown below:
> > > +
> > > +
> > > +-----------------------------------------------------------------------+
> > > +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11
> > > ID 0|
> > > +
> > > +-----------------------------------------------------------------------+
> > > +    |63                                 GUID_L
> > > 0|
> > > +
> > > +-----------------------------------------------------------------------+
> > > +    |63                                 GUID_H
> > > 0|
> > > +
> > > +-----------------------------------------------------------------------+
> > > +    |63                 Address/Offset                            1|  Rel
> > > 0|
> > > +
> > > +-----------------------------------------------------------------------+
> > 
> > Is something missing here given the layout is claimed (in 2/6) to be:
> > 
> > "DFHv1 Register Offset definitons
> > In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA"
> > 
> > ?
> 
> 
> I was hesitant to have a picture because the description would then be in two
> places.  I suspect my picture is not clear, but it does line up with the
> offset definitions:
> 
> DFH offset 0x0
> GUID offsets 0x8 and 0x10
> CSR_START offset 0x18
> CSR_SIZE offset 0x20
> First PARAM_HDR, if it exists, is 0x28,
> First PARAM_DATA, if it exists, is 0x30.

I already noted in the other email I figured it out. It was thanks to 
the offsets in the header how I found out where I had misintepreted 
things. I initially had thought PARAM_DATA would be the parameters, both 
headers and param data, but then realized that it's PARAM_HDR+PARAM_DATA 
which is repeated n times.

I don't think there's need to fix anything in here.

-- 
 i.

Re: [PATCH v2 1/6] Documentation: fpga: dfl: Add documentation for DFHv1

[Bagas Sanjaya]

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

On Fri, Sep 23, 2022 at 05:17:40AM -0700, matthew.gerlach@linux.intel.com wrote:
> +With DFHv0, the register map of a given feature is located immediately following
> +the DFHv0 in the memory space.  With DFHv1, the location of the feature register
> +map can be specified as an offset to the DFHv1 or as an absolute address.  The DFHv1
> +structure is shown below:
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0|
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Address/Offset                            1|  Rel  0|
> +    +-----------------------------------------------------------------------+
> +    |63 Size of register set  32|Params 31|30 Group    16|15 Instance      0|
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
> +                                  ...
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Next parameter offset 32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +

For consistency with DFL location diagram (which is above the DFHv1
diagram above), use literal code block instead of table:

---- >8 ----

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 7c786b75b4988f..db6bff4aee25eb 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -584,7 +584,7 @@ across all types.
 With DFHv0, the register map of a given feature is located immediately following
 the DFHv0 in the memory space.  With DFHv1, the location of the feature register
 map can be specified as an offset to the DFHv1 or as an absolute address.  The DFHv1
-structure is shown below:
+structure is shown below::
 
     +-----------------------------------------------------------------------+
     |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0|

Thanks.

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

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

[PATCH v2 2/6] fpga: dfl: Move the DFH definitions

[matthew.gerlach]

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

Moving the DFH register offset and register definitions from
drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
need to be accessed by dfl drivers that are outside of
drivers/fpga.

Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v2: remove extra space in commit
    use uniform number of digits in constants
    don't change copyright date because of removed content
---
 drivers/fpga/dfl-afu-main.c |  4 ++--
 drivers/fpga/dfl.c          |  2 +-
 drivers/fpga/dfl.h          | 20 +-------------------
 include/linux/dfl.h         | 33 ++++++++++++++++++++++++++++++++-
 4 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index 7f621e96d3b8..c26961ee33db 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -468,8 +468,8 @@ afu_id_show(struct device *dev, struct device_attribute *attr, char *buf)
 		return -EBUSY;
 	}
 
-	guidl = readq(base + GUID_L);
-	guidh = readq(base + GUID_H);
+	guidl = readq(base + DFH_GUID_L);
+	guidh = readq(base + DFH_GUID_H);
 	mutex_unlock(&pdata->lock);
 
 	return scnprintf(buf, PAGE_SIZE, "%016llx%016llx\n", guidh, guidl);
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..1132f3c10440 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -1163,7 +1163,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
 	 * find and parse FIU's child AFU via its NEXT_AFU register.
 	 * please note that only Port has valid NEXT_AFU pointer per spec.
 	 */
-	v = readq(binfo->ioaddr + NEXT_AFU);
+	v = readq(binfo->ioaddr + DFH_NEXT_AFU);
 
 	offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
 	if (offset)
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..e620fcb02b5a 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -17,6 +17,7 @@
 #include <linux/bitfield.h>
 #include <linux/cdev.h>
 #include <linux/delay.h>
+#include <linux/dfl.h>
 #include <linux/eventfd.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
@@ -53,28 +54,9 @@
 #define PORT_FEATURE_ID_UINT		0x12
 #define PORT_FEATURE_ID_STP		0x13
 
-/*
- * Device Feature Header Register Set
- *
- * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
- * For AFUs, they have DFH + GUID as common header registers.
- * For private features, they only have DFH register as common header.
- */
-#define DFH			0x0
-#define GUID_L			0x8
-#define GUID_H			0x10
-#define NEXT_AFU		0x18
-
-#define DFH_SIZE		0x8
-
 /* Device Feature Header Register Bitfield */
-#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
 #define DFH_ID_FIU_FME		0
 #define DFH_ID_FIU_PORT		1
-#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_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
 #define DFH_TYPE_AFU		1
 #define DFH_TYPE_PRIVATE	3
 #define DFH_TYPE_FIU		4
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 431636a0dc78..33d167c53b09 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -2,7 +2,7 @@
 /*
  * Header file for DFL driver and device API
  *
- * Copyright (C) 2020 Intel Corporation, Inc.
+ * Copyright (C) 2020-2022 Intel Corporation, Inc.
  */
 
 #ifndef __LINUX_DFL_H
@@ -11,6 +11,37 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+/*
+ * Device Feature Header Register Set
+ *
+ * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
+ * For AFUs, they have DFH + GUID as common header registers.
+ * For private features, they only have DFH register as common header.
+ */
+#define DFH			0x00
+#define DFH_GUID_L		0x08
+#define DFH_GUID_H		0x10
+#define DFH_NEXT_AFU		0x18
+
+/*
+ * 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 */
+#define DFHv1_PARAM_DATA	0x08  /* Offset of Param data from Param header */
+
+#define DFH_SIZE		0x08
+
+/* Device Feature Header Register Bitfield */
+#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
+#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_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
-- 
2.25.1

Re: [PATCH v2 2/6] fpga: dfl: Move the DFH definitions

[Tom Rix]

On 9/23/22 5:17 AM, matthew.gerlach@linux.intel.com wrote:
> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>
> Moving the DFH register offset and register definitions from
> drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
> need to be accessed by dfl drivers that are outside of
> drivers/fpga.

This comment does not match what is done.

A move, a change in names and the introduction new defines.

I am not sure if moving these #defines is the best approach, the later 
use of the in the uart with FIELD_GET's i think should be wrapped as 
functions and these functions exported rather than the #defines.

So split this patch and justify why #defines are added to the user's 
includes.

Tom

>
> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2: remove extra space in commit
>      use uniform number of digits in constants
>      don't change copyright date because of removed content
> ---
>   drivers/fpga/dfl-afu-main.c |  4 ++--
>   drivers/fpga/dfl.c          |  2 +-
>   drivers/fpga/dfl.h          | 20 +-------------------
>   include/linux/dfl.h         | 33 ++++++++++++++++++++++++++++++++-
>   4 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> index 7f621e96d3b8..c26961ee33db 100644
> --- a/drivers/fpga/dfl-afu-main.c
> +++ b/drivers/fpga/dfl-afu-main.c
> @@ -468,8 +468,8 @@ afu_id_show(struct device *dev, struct device_attribute *attr, char *buf)
>   		return -EBUSY;
>   	}
>   
> -	guidl = readq(base + GUID_L);
> -	guidh = readq(base + GUID_H);
> +	guidl = readq(base + DFH_GUID_L);
> +	guidh = readq(base + DFH_GUID_H);
>   	mutex_unlock(&pdata->lock);
>   
>   	return scnprintf(buf, PAGE_SIZE, "%016llx%016llx\n", guidh, guidl);
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..1132f3c10440 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -1163,7 +1163,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
>   	 * find and parse FIU's child AFU via its NEXT_AFU register.
>   	 * please note that only Port has valid NEXT_AFU pointer per spec.
>   	 */
> -	v = readq(binfo->ioaddr + NEXT_AFU);
> +	v = readq(binfo->ioaddr + DFH_NEXT_AFU);
>   
>   	offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
>   	if (offset)
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 06cfcd5e84bb..e620fcb02b5a 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -17,6 +17,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/cdev.h>
>   #include <linux/delay.h>
> +#include <linux/dfl.h>
>   #include <linux/eventfd.h>
>   #include <linux/fs.h>
>   #include <linux/interrupt.h>
> @@ -53,28 +54,9 @@
>   #define PORT_FEATURE_ID_UINT		0x12
>   #define PORT_FEATURE_ID_STP		0x13
>   
> -/*
> - * Device Feature Header Register Set
> - *
> - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> - * For AFUs, they have DFH + GUID as common header registers.
> - * For private features, they only have DFH register as common header.
> - */
> -#define DFH			0x0
> -#define GUID_L			0x8
> -#define GUID_H			0x10
> -#define NEXT_AFU		0x18
> -
> -#define DFH_SIZE		0x8
> -
>   /* Device Feature Header Register Bitfield */
> -#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
>   #define DFH_ID_FIU_FME		0
>   #define DFH_ID_FIU_PORT		1
> -#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_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>   #define DFH_TYPE_AFU		1
>   #define DFH_TYPE_PRIVATE	3
>   #define DFH_TYPE_FIU		4
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 431636a0dc78..33d167c53b09 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -2,7 +2,7 @@
>   /*
>    * Header file for DFL driver and device API
>    *
> - * Copyright (C) 2020 Intel Corporation, Inc.
> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
>    */
>   
>   #ifndef __LINUX_DFL_H
> @@ -11,6 +11,37 @@
>   #include <linux/device.h>
>   #include <linux/mod_devicetable.h>
>   
> +/*
> + * Device Feature Header Register Set
> + *
> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header registers.
> + * For AFUs, they have DFH + GUID as common header registers.
> + * For private features, they only have DFH register as common header.
> + */
> +#define DFH			0x00
> +#define DFH_GUID_L		0x08
> +#define DFH_GUID_H		0x10
> +#define DFH_NEXT_AFU		0x18
> +
> +/*
> + * 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 */
> +#define DFHv1_PARAM_DATA	0x08  /* Offset of Param data from Param header */
> +
> +#define DFH_SIZE		0x08
> +
> +/* Device Feature Header Register Bitfield */
> +#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID */
> +#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_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
> +
>   /**
>    * enum dfl_id_type - define the DFL FIU types
>    */

Re: [PATCH v2 2/6] fpga: dfl: Move the DFH definitions

[Xu Yilun]

On 2022-09-24 at 06:00:19 -0700, Tom Rix wrote:
> 
> On 9/23/22 5:17 AM, matthew.gerlach@linux.intel.com wrote:
> > From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> > 
> > Moving the DFH register offset and register definitions from
> > drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
> > need to be accessed by dfl drivers that are outside of
> > drivers/fpga.
> 
> This comment does not match what is done.
> 
> A move, a change in names and the introduction new defines.
> 
> I am not sure if moving these #defines is the best approach, the later use
> of the in the uart with FIELD_GET's i think should be wrapped as functions
> and these functions exported rather than the #defines.

I agree, to play with all domains across kernel, we'd better not expose
too much bus specific details.

Thanks,
Yilun

Re: [PATCH v2 2/6] fpga: dfl: Move the DFH definitions

[matthew.gerlach]

On Sat, 24 Sep 2022, Tom Rix wrote:

>
> On 9/23/22 5:17 AM, matthew.gerlach@linux.intel.com wrote:
>> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>> 
>> Moving the DFH register offset and register definitions from
>> drivers/fpga/dfl.h to include/linux/dfl.h. These definitions
>> need to be accessed by dfl drivers that are outside of
>> drivers/fpga.
>
> This comment does not match what is done.
>
> A move, a change in names and the introduction new defines.
>
> I am not sure if moving these #defines is the best approach, the later use of 
> the in the uart with FIELD_GET's i think should be wrapped as functions and 
> these functions exported rather than the #defines.
>
> So split this patch and justify why #defines are added to the user's 
> includes.
>
> Tom

I agree the original intent "diverged in v2".  I will minimize moving and 
make helper functions to simplify things.


>
>> 
>> Signed-off-by: Basheer Ahmed Muddebihal 
>> <basheer.ahmed.muddebihal@linux.intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v2: remove extra space in commit
>>      use uniform number of digits in constants
>>      don't change copyright date because of removed content
>> ---
>>   drivers/fpga/dfl-afu-main.c |  4 ++--
>>   drivers/fpga/dfl.c          |  2 +-
>>   drivers/fpga/dfl.h          | 20 +-------------------
>>   include/linux/dfl.h         | 33 ++++++++++++++++++++++++++++++++-
>>   4 files changed, 36 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
>> index 7f621e96d3b8..c26961ee33db 100644
>> --- a/drivers/fpga/dfl-afu-main.c
>> +++ b/drivers/fpga/dfl-afu-main.c
>> @@ -468,8 +468,8 @@ afu_id_show(struct device *dev, struct device_attribute 
>> *attr, char *buf)
>>   		return -EBUSY;
>>   	}
>>   -	guidl = readq(base + GUID_L);
>> -	guidh = readq(base + GUID_H);
>> +	guidl = readq(base + DFH_GUID_L);
>> +	guidh = readq(base + DFH_GUID_H);
>>   	mutex_unlock(&pdata->lock);
>>     	return scnprintf(buf, PAGE_SIZE, "%016llx%016llx\n", guidh, guidl);
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index b9aae85ba930..1132f3c10440 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -1163,7 +1163,7 @@ static int parse_feature_fiu(struct 
>> build_feature_devs_info *binfo,
>>   	 * find and parse FIU's child AFU via its NEXT_AFU register.
>>   	 * please note that only Port has valid NEXT_AFU pointer per spec.
>>   	 */
>> -	v = readq(binfo->ioaddr + NEXT_AFU);
>> +	v = readq(binfo->ioaddr + DFH_NEXT_AFU);
>>     	offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
>>   	if (offset)
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 06cfcd5e84bb..e620fcb02b5a 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -17,6 +17,7 @@
>>   #include <linux/bitfield.h>
>>   #include <linux/cdev.h>
>>   #include <linux/delay.h>
>> +#include <linux/dfl.h>
>>   #include <linux/eventfd.h>
>>   #include <linux/fs.h>
>>   #include <linux/interrupt.h>
>> @@ -53,28 +54,9 @@
>>   #define PORT_FEATURE_ID_UINT		0x12
>>   #define PORT_FEATURE_ID_STP		0x13
>>   -/*
>> - * Device Feature Header Register Set
>> - *
>> - * For FIUs, they all have DFH + GUID + NEXT_AFU as common header 
>> registers.
>> - * For AFUs, they have DFH + GUID as common header registers.
>> - * For private features, they only have DFH register as common header.
>> - */
>> -#define DFH			0x0
>> -#define GUID_L			0x8
>> -#define GUID_H			0x10
>> -#define NEXT_AFU		0x18
>> -
>> -#define DFH_SIZE		0x8
>> -
>>   /* Device Feature Header Register Bitfield */
>> -#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID 
>> */
>>   #define DFH_ID_FIU_FME		0
>>   #define DFH_ID_FIU_PORT		1
>> -#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_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>>   #define DFH_TYPE_AFU		1
>>   #define DFH_TYPE_PRIVATE	3
>>   #define DFH_TYPE_FIU		4
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 431636a0dc78..33d167c53b09 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -2,7 +2,7 @@
>>   /*
>>    * Header file for DFL driver and device API
>>    *
>> - * Copyright (C) 2020 Intel Corporation, Inc.
>> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
>>    */
>>     #ifndef __LINUX_DFL_H
>> @@ -11,6 +11,37 @@
>>   #include <linux/device.h>
>>   #include <linux/mod_devicetable.h>
>>   +/*
>> + * Device Feature Header Register Set
>> + *
>> + * For FIUs, they all have DFH + GUID + NEXT_AFU as common header 
>> registers.
>> + * For AFUs, they have DFH + GUID as common header registers.
>> + * For private features, they only have DFH register as common header.
>> + */
>> +#define DFH			0x00
>> +#define DFH_GUID_L		0x08
>> +#define DFH_GUID_H		0x10
>> +#define DFH_NEXT_AFU		0x18
>> +
>> +/*
>> + * 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 
>> */
>> +#define DFHv1_PARAM_DATA	0x08  /* Offset of Param data from Param 
>> header */
>> +
>> +#define DFH_SIZE		0x08
>> +
>> +/* Device Feature Header Register Bitfield */
>> +#define DFH_ID			GENMASK_ULL(11, 0)	/* Feature ID 
>> */
>> +#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_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
>> +
>>   /**
>>    * enum dfl_id_type - define the DFL FIU types
>>    */
>
>

[PATCH v2 3/6] 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>
---
v2: clean up whitespace and one line comments
---
 include/linux/dfl.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 33d167c53b09..1e53468ba8d8 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -40,8 +40,29 @@
 #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 */
 
+/*
+ * 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 */
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
-- 
2.25.1

[PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

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

Define and use a DFHv1 parameter to add generic support for MSIX
interrupts for DFL devices.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v2: fix kernel doc
    clarify use of DFH_VERSION field
---
 drivers/fpga/dfl.c  | 60 +++++++++++++++++++++++++++++++++++++++++----
 include/linux/dfl.h | 14 +++++++++++
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 1132f3c10440..dfd3f563c92d 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
 	void __iomem *base = binfo->ioaddr + ofst;
 	unsigned int i, ibase, inr = 0;
 	enum dfl_id_type type;
-	int virq;
+	int virq, off;
 	u64 v;
 
 	type = feature_dev_id_type(binfo->feature_dev);
 
 	/*
 	 * Ideally DFL framework should only read info from DFL header, but
-	 * current version DFL only provides mmio resources information for
+	 * current version, DFHv0, only provides mmio resources information for
 	 * each feature in DFL Header, no field for interrupt resources.
 	 * Interrupt resource information is provided by specific mmio
 	 * registers of each private feature which supports interrupt. So in
 	 * order to parse and assign irq resources, DFL framework has to look
 	 * into specific capability registers of these private features.
 	 *
-	 * Once future DFL version supports generic interrupt resource
-	 * information in common DFL headers, the generic interrupt parsing
-	 * code will be added. But in order to be compatible to old version
+	 * DFHv1 supports generic interrupt resource information in DFHv1
+	 * parameter blocks. But in order to be compatible to old version
 	 * DFL, the driver may still fall back to these quirks.
 	 */
 	if (type == PORT_ID) {
@@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
 		}
 	}
 
+	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
+	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
+
+		v = FIELD_GET(DFH_VERSION, readq(base));
+		switch (v) {
+		case 0:
+			break;
+
+		case 1:
+			v =  readq(base + DFHv1_CSR_SIZE_GRP);
+			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
+				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
+						     DFHv1_PARAM_ID_MSIX);
+				if (off >= 0) {
+					ibase = readl(base + DFHv1_PARAM_HDR +
+						      off + DFHv1_PARAM_MSIX_STARTV);
+					inr = readl(base + DFHv1_PARAM_HDR +
+						    off + DFHv1_PARAM_MSIX_NUMV);
+					dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
+						ibase, inr, fid);
+				}
+			}
+			break;
+
+		default:
+			dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
+			break;
+		}
+	}
+
 	if (!inr) {
 		*irq_base = 0;
 		*nr_irqs = 0;
@@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
 
+int dfl_find_param(void __iomem *base, resource_size_t max, int param)
+{
+	int off = 0;
+	u64 v, next;
+
+	while (off < max) {
+		v = readq(base + off);
+		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+			return off;
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		if (!next)
+			break;
+
+		off += next;
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(dfl_find_param);
+
 static void __exit dfl_fpga_exit(void)
 {
 	dfl_chardev_uinit();
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 1e53468ba8d8..33e21c360671 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -63,6 +63,10 @@
 #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_ID_MSIX	0x1
+#define DFHv1_PARAM_MSIX_STARTV	0x8
+#define DFHv1_PARAM_MSIX_NUMV	0xc
+
 /**
  * enum dfl_id_type - define the DFL FIU types
  */
@@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
 	module_driver(__dfl_driver, dfl_driver_register, \
 		      dfl_driver_unregister)
 
+/**
+ * dfl_find_param() - find the offset of the given parameter
+ * @base: base pointer to start of dfl parameters in DFH
+ * @max: maximum offset to search
+ * @param: id of dfl parameter
+ *
+ * Return: positive offset on success, negative error code otherwise.
+ */
+int dfl_find_param(void __iomem *base, resource_size_t max, int param);
+
 #endif /* __LINUX_DFL_H */
-- 
2.25.1

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[Ilpo Järvinen]

On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2: fix kernel doc
>     clarify use of DFH_VERSION field
> ---
>  drivers/fpga/dfl.c  | 60 +++++++++++++++++++++++++++++++++++++++++----
>  include/linux/dfl.h | 14 +++++++++++
>  2 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1132f3c10440..dfd3f563c92d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  	void __iomem *base = binfo->ioaddr + ofst;
>  	unsigned int i, ibase, inr = 0;
>  	enum dfl_id_type type;
> -	int virq;
> +	int virq, off;
>  	u64 v;
>  
>  	type = feature_dev_id_type(binfo->feature_dev);
>  
>  	/*
>  	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL only provides mmio resources information for
> +	 * current version, DFHv0, only provides mmio resources information for
>  	 * each feature in DFL Header, no field for interrupt resources.
>  	 * Interrupt resource information is provided by specific mmio
>  	 * registers of each private feature which supports interrupt. So in
>  	 * order to parse and assign irq resources, DFL framework has to look
>  	 * into specific capability registers of these private features.
>  	 *
> -	 * Once future DFL version supports generic interrupt resource
> -	 * information in common DFL headers, the generic interrupt parsing
> -	 * code will be added. But in order to be compatible to old version
> +	 * DFHv1 supports generic interrupt resource information in DFHv1
> +	 * parameter blocks. But in order to be compatible to old version
>  	 * DFL, the driver may still fall back to these quirks.
>  	 */
>  	if (type == PORT_ID) {
> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  		}
>  	}
>  
> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
> +		v = FIELD_GET(DFH_VERSION, readq(base));

I'd call this variable version (or ver) if you want to store it but it 
would also fit to switch () line so that no extra variable is needed.

> +		switch (v) {
> +		case 0:
> +			break;
> +
> +		case 1:
> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);

Extra space.

> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> +						     DFHv1_PARAM_ID_MSIX);
> +				if (off >= 0) {

I'd reverse these 2 conditions and break when there's nothing to do.

> +					ibase = readl(base + DFHv1_PARAM_HDR +
> +						      off + DFHv1_PARAM_MSIX_STARTV);
> +					inr = readl(base + DFHv1_PARAM_HDR +
> +						    off + DFHv1_PARAM_MSIX_NUMV);
> +					dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> +						ibase, inr, fid);
> +				}
> +			}
> +			break;
> +
> +		default:
> +			dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> +			break;
> +		}
> +	}
> +
>  	if (!inr) {
>  		*irq_base = 0;
>  		*nr_irqs = 0;

-- 
 i.

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

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



On Fri, 23 Sep 2022, Ilpo Järvinen wrote:

> On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v2: fix kernel doc
>>     clarify use of DFH_VERSION field
>> ---
>>  drivers/fpga/dfl.c  | 60 +++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/dfl.h | 14 +++++++++++
>>  2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1132f3c10440..dfd3f563c92d 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>  	void __iomem *base = binfo->ioaddr + ofst;
>>  	unsigned int i, ibase, inr = 0;
>>  	enum dfl_id_type type;
>> -	int virq;
>> +	int virq, off;
>>  	u64 v;
>>
>>  	type = feature_dev_id_type(binfo->feature_dev);
>>
>>  	/*
>>  	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL only provides mmio resources information for
>> +	 * current version, DFHv0, only provides mmio resources information for
>>  	 * each feature in DFL Header, no field for interrupt resources.
>>  	 * Interrupt resource information is provided by specific mmio
>>  	 * registers of each private feature which supports interrupt. So in
>>  	 * order to parse and assign irq resources, DFL framework has to look
>>  	 * into specific capability registers of these private features.
>>  	 *
>> -	 * Once future DFL version supports generic interrupt resource
>> -	 * information in common DFL headers, the generic interrupt parsing
>> -	 * code will be added. But in order to be compatible to old version
>> +	 * DFHv1 supports generic interrupt resource information in DFHv1
>> +	 * parameter blocks. But in order to be compatible to old version
>>  	 * DFL, the driver may still fall back to these quirks.
>>  	 */
>>  	if (type == PORT_ID) {
>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>  		}
>>  	}
>>
>> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>> +
>> +		v = FIELD_GET(DFH_VERSION, readq(base));
>
> I'd call this variable version (or ver) if you want to store it but it
> would also fit to switch () line so that no extra variable is needed.

I will change the v to dfh_ver to be clearer.  I want to store the 
value because it is used in the default case in the error message.  The 
error message helps to debug broken FPGA images.

>
>> +		switch (v) {
>> +		case 0:
>> +			break;
>> +
>> +		case 1:
>> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
>
> Extra space.

I will remove extra space.

>
>> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> +						     DFHv1_PARAM_ID_MSIX);
>> +				if (off >= 0) {
>
> I'd reverse these 2 conditions and break when there's nothing to do.

I'm not sure what you mean by reversing these conditions because a DFHv1 
may or may not have parameters (the first condition), and a DFHv1 may have
parameters but may not have a MSI-X parameter (the second condition).

>
>> +					ibase = readl(base + DFHv1_PARAM_HDR +
>> +						      off + DFHv1_PARAM_MSIX_STARTV);
>> +					inr = readl(base + DFHv1_PARAM_HDR +
>> +						    off + DFHv1_PARAM_MSIX_NUMV);
>> +					dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> +						ibase, inr, fid);
>> +				}
>> +			}
>> +			break;
>> +
>> +		default:
>> +			dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> +			break;
>> +		}
>> +	}
>> +
>>  	if (!inr) {
>>  		*irq_base = 0;
>>  		*nr_irqs = 0;
>
> -- 
> i.
>
>

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[Ilpo Järvinen]

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

On Mon, 26 Sep 2022, matthew.gerlach@linux.intel.com wrote:

> 
> 
> On Fri, 23 Sep 2022, Ilpo Järvinen wrote:
> 
> > On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:
> > 
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > Define and use a DFHv1 parameter to add generic support for MSIX
> > > interrupts for DFL devices.
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > > v2: fix kernel doc
> > >     clarify use of DFH_VERSION field
> > > ---
> > >  drivers/fpga/dfl.c  | 60 +++++++++++++++++++++++++++++++++++++++++----
> > >  include/linux/dfl.h | 14 +++++++++++
> > >  2 files changed, 69 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index 1132f3c10440..dfd3f563c92d 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,
> > >  	void __iomem *base = binfo->ioaddr + ofst;
> > >  	unsigned int i, ibase, inr = 0;
> > >  	enum dfl_id_type type;
> > > -	int virq;
> > > +	int virq, off;
> > >  	u64 v;
> > > 
> > >  	type = feature_dev_id_type(binfo->feature_dev);
> > > 
> > >  	/*
> > >  	 * Ideally DFL framework should only read info from DFL header, but
> > > -	 * current version DFL only provides mmio resources information for
> > > +	 * current version, DFHv0, only provides mmio resources information
> > > for
> > >  	 * each feature in DFL Header, no field for interrupt resources.
> > >  	 * Interrupt resource information is provided by specific mmio
> > >  	 * registers of each private feature which supports interrupt. So in
> > >  	 * order to parse and assign irq resources, DFL framework has to look
> > >  	 * into specific capability registers of these private features.
> > >  	 *
> > > -	 * Once future DFL version supports generic interrupt resource
> > > -	 * information in common DFL headers, the generic interrupt parsing
> > > -	 * code will be added. But in order to be compatible to old version
> > > +	 * DFHv1 supports generic interrupt resource information in DFHv1
> > > +	 * parameter blocks. But in order to be compatible to old version
> > >  	 * DFL, the driver may still fall back to these quirks.
> > >  	 */
> > >  	if (type == PORT_ID) {
> > > @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct
> > > build_feature_devs_info *binfo,
> > >  		}
> > >  	}
> > > 
> > > +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> > > +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> > > +
> > > +		v = FIELD_GET(DFH_VERSION, readq(base));
> > 
> > I'd call this variable version (or ver) if you want to store it but it
> > would also fit to switch () line so that no extra variable is needed.
> 
> I will change the v to dfh_ver to be clearer.  I want to store the value
> because it is used in the default case in the error message.  The error
> message helps to debug broken FPGA images.

Right, I missed that (or didn't think it too much and all being called 
"v" didn't help either :-)).

> > > +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> > > +				off = dfl_find_param(base + DFHv1_PARAM_HDR,
> > > ofst,
> > > +						     DFHv1_PARAM_ID_MSIX);
> > > +				if (off >= 0) {
> > 
> > I'd reverse these 2 conditions and break when there's nothing to do.
> 
> I'm not sure what you mean by reversing these conditions because a DFHv1 may
> or may not have parameters (the first condition), and a DFHv1 may have
> parameters but may not have a MSI-X parameter (the second condition).

This is what I meant:

		if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v))
			break;

		off = dfl_find_param(...);
		if (off < 0)
			break;

		ibase = ...


-- 
 i.


> > > +					ibase = readl(base + DFHv1_PARAM_HDR +
> > > +						      off +
> > > DFHv1_PARAM_MSIX_STARTV);
> > > +					inr = readl(base + DFHv1_PARAM_HDR +
> > > +						    off +
> > > DFHv1_PARAM_MSIX_NUMV);
> > > +					dev_dbg(binfo->dev, "start %d num %d
> > > fid 0x%x\n",
> > > +						ibase, inr, fid);
> > > +				}
> > > +			}
> > > +			break;

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

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



On Tue, 27 Sep 2022, Ilpo Järvinen wrote:

> On Mon, 26 Sep 2022, matthew.gerlach@linux.intel.com wrote:
>
>>
>>
>> On Fri, 23 Sep 2022, Ilpo Järvinen wrote:
>>
>>> On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:
>>>
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Define and use a DFHv1 parameter to add generic support for MSIX
>>>> interrupts for DFL devices.
>>>>
>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>> ---
>>>> v2: fix kernel doc
>>>>     clarify use of DFH_VERSION field
>>>> ---
>>>>  drivers/fpga/dfl.c  | 60 +++++++++++++++++++++++++++++++++++++++++----
>>>>  include/linux/dfl.h | 14 +++++++++++
>>>>  2 files changed, 69 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>>>> index 1132f3c10440..dfd3f563c92d 100644
>>>> --- a/drivers/fpga/dfl.c
>>>> +++ b/drivers/fpga/dfl.c
>>>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct
>>>> build_feature_devs_info *binfo,
>>>>  	void __iomem *base = binfo->ioaddr + ofst;
>>>>  	unsigned int i, ibase, inr = 0;
>>>>  	enum dfl_id_type type;
>>>> -	int virq;
>>>> +	int virq, off;
>>>>  	u64 v;
>>>>
>>>>  	type = feature_dev_id_type(binfo->feature_dev);
>>>>
>>>>  	/*
>>>>  	 * Ideally DFL framework should only read info from DFL header, but
>>>> -	 * current version DFL only provides mmio resources information for
>>>> +	 * current version, DFHv0, only provides mmio resources information
>>>> for
>>>>  	 * each feature in DFL Header, no field for interrupt resources.
>>>>  	 * Interrupt resource information is provided by specific mmio
>>>>  	 * registers of each private feature which supports interrupt. So in
>>>>  	 * order to parse and assign irq resources, DFL framework has to look
>>>>  	 * into specific capability registers of these private features.
>>>>  	 *
>>>> -	 * Once future DFL version supports generic interrupt resource
>>>> -	 * information in common DFL headers, the generic interrupt parsing
>>>> -	 * code will be added. But in order to be compatible to old version
>>>> +	 * DFHv1 supports generic interrupt resource information in DFHv1
>>>> +	 * parameter blocks. But in order to be compatible to old version
>>>>  	 * DFL, the driver may still fall back to these quirks.
>>>>  	 */
>>>>  	if (type == PORT_ID) {
>>>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct
>>>> build_feature_devs_info *binfo,
>>>>  		}
>>>>  	}
>>>>
>>>> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>>>> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>>>> +
>>>> +		v = FIELD_GET(DFH_VERSION, readq(base));
>>>
>>> I'd call this variable version (or ver) if you want to store it but it
>>> would also fit to switch () line so that no extra variable is needed.
>>
>> I will change the v to dfh_ver to be clearer.  I want to store the value
>> because it is used in the default case in the error message.  The error
>> message helps to debug broken FPGA images.
>
> Right, I missed that (or didn't think it too much and all being called
> "v" didn't help either :-)).
>
>>>> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>>>> +				off = dfl_find_param(base + DFHv1_PARAM_HDR,
>>>> ofst,
>>>> +						     DFHv1_PARAM_ID_MSIX);
>>>> +				if (off >= 0) {
>>>
>>> I'd reverse these 2 conditions and break when there's nothing to do.
>>
>> I'm not sure what you mean by reversing these conditions because a DFHv1 may
>> or may not have parameters (the first condition), and a DFHv1 may have
>> parameters but may not have a MSI-X parameter (the second condition).
>
> This is what I meant:
>
> 		if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v))
> 			break;
>
> 		off = dfl_find_param(...);
> 		if (off < 0)
> 			break;
>
> 		ibase = ...

I understand now.  This is a good suggestion because the resulting 
indentation is better.

Thanks,
Matthew


>
>
> -- 
> i.
>
>
>>>> +					ibase = readl(base + DFHv1_PARAM_HDR +
>>>> +						      off +
>>>> DFHv1_PARAM_MSIX_STARTV);
>>>> +					inr = readl(base + DFHv1_PARAM_HDR +
>>>> +						    off +
>>>> DFHv1_PARAM_MSIX_NUMV);
>>>> +					dev_dbg(binfo->dev, "start %d num %d
>>>> fid 0x%x\n",
>>>> +						ibase, inr, fid);
>>>> +				}
>>>> +			}
>>>> +			break;
>

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[Andy Shevchenko]

On Fri, Sep 23, 2022 at 05:17:43AM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.

...

> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {

> +

Unneeded blank line.

> +		v = FIELD_GET(DFH_VERSION, readq(base));
> +		switch (v) {

This v...

> +		case 0:
> +			break;
> +
> +		case 1:
> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);

Extra space.

...and this v are semantically different. It's quite hard to deduce their
semantics.

> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> +						     DFHv1_PARAM_ID_MSIX);

I guess I have suggested to use temporary variable(s) here.

			void __iomem *dfhv1 = base + DFHv1...;
			void __iomem *nth;

> +				if (off >= 0) {

					nth = dfhv1 + off;

> +					ibase = readl(base + DFHv1_PARAM_HDR +
> +						      off + DFHv1_PARAM_MSIX_STARTV);
> +					inr = readl(base + DFHv1_PARAM_HDR +
> +						    off + DFHv1_PARAM_MSIX_NUMV);

					ibase = readl(nth + DFHv1_PARAM_MSIX_STARTV);
					inr = readl(nth + DFHv1_PARAM_MSIX_NUMV);

> +					dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> +						ibase, inr, fid);
> +				}
> +			}
> +			break;
> +
> +		default:
> +			dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> +			break;
> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

On Fri, 23 Sep 2022, Andy Shevchenko wrote:

> On Fri, Sep 23, 2022 at 05:17:43AM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>
> ...
>
>> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>
>> +
>
> Unneeded blank line.

I will remove the blank line.

>
>> +		v = FIELD_GET(DFH_VERSION, readq(base));
>> +		switch (v) {
>
> This v...
>
>> +		case 0:
>> +			break;
>> +
>> +		case 1:
>> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
>
> Extra space.
>
> ...and this v are semantically different. It's quite hard to deduce their
> semantics.

I was trying to be consistent with the existing code where the read was 
stored in a temporary variable, v, and the FIELD_GET would be used for the 
specific field.  Will it be sufficiently clear if the v used above is 
changed to dfl_ver, and this use of v followed by FIELD_GET remains as is?

>
>> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> +						     DFHv1_PARAM_ID_MSIX);
>
> I guess I have suggested to use temporary variable(s) here.
>
> 			void __iomem *dfhv1 = base + DFHv1...;
> 			void __iomem *nth;
>
>> +				if (off >= 0) {
>
> 					nth = dfhv1 + off;
>
>> +					ibase = readl(base + DFHv1_PARAM_HDR +
>> +						      off + DFHv1_PARAM_MSIX_STARTV);
>> +					inr = readl(base + DFHv1_PARAM_HDR +
>> +						    off + DFHv1_PARAM_MSIX_NUMV);
>
> 					ibase = readl(nth + DFHv1_PARAM_MSIX_STARTV);
> 					inr = readl(nth + DFHv1_PARAM_MSIX_NUMV);
>
>> +					dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> +						ibase, inr, fid);
>> +				}
>> +			}
>> +			break;
>> +
>> +		default:
>> +			dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> +			break;
>> +		}
>> +	}
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[Xu Yilun]

On 2022-09-23 at 05:17:43 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Define and use a DFHv1 parameter to add generic support for MSIX
> interrupts for DFL devices.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2: fix kernel doc
>     clarify use of DFH_VERSION field
> ---
>  drivers/fpga/dfl.c  | 60 +++++++++++++++++++++++++++++++++++++++++----
>  include/linux/dfl.h | 14 +++++++++++
>  2 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 1132f3c10440..dfd3f563c92d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  	void __iomem *base = binfo->ioaddr + ofst;
>  	unsigned int i, ibase, inr = 0;
>  	enum dfl_id_type type;
> -	int virq;
> +	int virq, off;
>  	u64 v;
>  
>  	type = feature_dev_id_type(binfo->feature_dev);
>  
>  	/*
>  	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL only provides mmio resources information for
> +	 * current version, DFHv0, only provides mmio resources information for

With this patchset, it's not 'current version' anymore.

>  	 * each feature in DFL Header, no field for interrupt resources.
>  	 * Interrupt resource information is provided by specific mmio
>  	 * registers of each private feature which supports interrupt. So in
>  	 * order to parse and assign irq resources, DFL framework has to look
>  	 * into specific capability registers of these private features.
>  	 *
> -	 * Once future DFL version supports generic interrupt resource
> -	 * information in common DFL headers, the generic interrupt parsing
> -	 * code will be added. But in order to be compatible to old version
> +	 * DFHv1 supports generic interrupt resource information in DFHv1
> +	 * parameter blocks. But in order to be compatible to old version
>  	 * DFL, the driver may still fall back to these quirks.
>  	 */
>  	if (type == PORT_ID) {
> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>  		}
>  	}
>  
> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
> +
> +		v = FIELD_GET(DFH_VERSION, readq(base));
> +		switch (v) {
> +		case 0:
> +			break;

In last version, you mentioned that there will be no quirk for DFLv1, so
how about:

  v = FIELD_GET(DFH_VERSION, readq(base));

  if (v == 0) {
	/* quirks */
  } else {
	/* parse PARAM MSIX  */
  }

No need to check specific feature ids again.

Thanks,
Yilun

> +
> +		case 1:
> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
> +						     DFHv1_PARAM_ID_MSIX);
> +				if (off >= 0) {
> +					ibase = readl(base + DFHv1_PARAM_HDR +
> +						      off + DFHv1_PARAM_MSIX_STARTV);
> +					inr = readl(base + DFHv1_PARAM_HDR +
> +						    off + DFHv1_PARAM_MSIX_NUMV);
> +					dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
> +						ibase, inr, fid);
> +				}
> +			}
> +			break;
> +
> +		default:
> +			dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
> +			break;
> +		}
> +	}
> +
>  	if (!inr) {
>  		*irq_base = 0;
>  		*nr_irqs = 0;
> @@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>  
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param)
> +{
> +	int off = 0;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = readq(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return off;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!next)
> +			break;
> +
> +		off += next;
> +	}
> +
> +	return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(dfl_find_param);
> +
>  static void __exit dfl_fpga_exit(void)
>  {
>  	dfl_chardev_uinit();
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 1e53468ba8d8..33e21c360671 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -63,6 +63,10 @@
>  #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_ID_MSIX	0x1
> +#define DFHv1_PARAM_MSIX_STARTV	0x8
> +#define DFHv1_PARAM_MSIX_NUMV	0xc
> +
>  /**
>   * enum dfl_id_type - define the DFL FIU types
>   */
> @@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>  	module_driver(__dfl_driver, dfl_driver_register, \
>  		      dfl_driver_unregister)
>  
> +/**
> + * dfl_find_param() - find the offset of the given parameter
> + * @base: base pointer to start of dfl parameters in DFH
> + * @max: maximum offset to search
> + * @param: id of dfl parameter
> + *
> + * Return: positive offset on success, negative error code otherwise.
> + */
> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
> +
>  #endif /* __LINUX_DFL_H */
> -- 
> 2.25.1
> 

Re: [PATCH v2 4/6] fpga: dfl: add generic support for MSIX interrupts

[matthew.gerlach]

On Fri, 30 Sep 2022, Xu Yilun wrote:

> On 2022-09-23 at 05:17:43 -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Define and use a DFHv1 parameter to add generic support for MSIX
>> interrupts for DFL devices.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v2: fix kernel doc
>>     clarify use of DFH_VERSION field
>> ---
>>  drivers/fpga/dfl.c  | 60 +++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/dfl.h | 14 +++++++++++
>>  2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1132f3c10440..dfd3f563c92d 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -941,23 +941,22 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>  	void __iomem *base = binfo->ioaddr + ofst;
>>  	unsigned int i, ibase, inr = 0;
>>  	enum dfl_id_type type;
>> -	int virq;
>> +	int virq, off;
>>  	u64 v;
>>
>>  	type = feature_dev_id_type(binfo->feature_dev);
>>
>>  	/*
>>  	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL only provides mmio resources information for
>> +	 * current version, DFHv0, only provides mmio resources information for
>
> With this patchset, it's not 'current version' anymore.

I will update the comment. Thanks.

>
>>  	 * each feature in DFL Header, no field for interrupt resources.
>>  	 * Interrupt resource information is provided by specific mmio
>>  	 * registers of each private feature which supports interrupt. So in
>>  	 * order to parse and assign irq resources, DFL framework has to look
>>  	 * into specific capability registers of these private features.
>>  	 *
>> -	 * Once future DFL version supports generic interrupt resource
>> -	 * information in common DFL headers, the generic interrupt parsing
>> -	 * code will be added. But in order to be compatible to old version
>> +	 * DFHv1 supports generic interrupt resource information in DFHv1
>> +	 * parameter blocks. But in order to be compatible to old version
>>  	 * DFL, the driver may still fall back to these quirks.
>>  	 */
>>  	if (type == PORT_ID) {
>> @@ -981,6 +980,36 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>>  		}
>>  	}
>>
>> +	if (fid != FEATURE_ID_AFU && fid != PORT_FEATURE_ID_ERROR &&
>> +	    fid != PORT_FEATURE_ID_UINT && fid != FME_FEATURE_ID_GLOBAL_ERR) {
>> +
>> +		v = FIELD_GET(DFH_VERSION, readq(base));
>> +		switch (v) {
>> +		case 0:
>> +			break;
>
> In last version, you mentioned that there will be no quirk for DFLv1, so
> how about:
>
>  v = FIELD_GET(DFH_VERSION, readq(base));
>
>  if (v == 0) {
> 	/* quirks */
>  } else {
> 	/* parse PARAM MSIX  */
>  }
>
> No need to check specific feature ids again.

With v3 changes I will use a switch state and not need quirks for v1.

>
> Thanks,
> Yilun
>
>> +
>> +		case 1:
>> +			v =  readq(base + DFHv1_CSR_SIZE_GRP);
>> +			if (FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
>> +				off = dfl_find_param(base + DFHv1_PARAM_HDR, ofst,
>> +						     DFHv1_PARAM_ID_MSIX);
>> +				if (off >= 0) {
>> +					ibase = readl(base + DFHv1_PARAM_HDR +
>> +						      off + DFHv1_PARAM_MSIX_STARTV);
>> +					inr = readl(base + DFHv1_PARAM_HDR +
>> +						    off + DFHv1_PARAM_MSIX_NUMV);
>> +					dev_dbg(binfo->dev, "start %d num %d fid 0x%x\n",
>> +						ibase, inr, fid);
>> +				}
>> +			}
>> +			break;
>> +
>> +		default:
>> +			dev_warn(binfo->dev, "unexpected DFH version %lld\n", v);
>> +			break;
>> +		}
>> +	}
>> +
>>  	if (!inr) {
>>  		*irq_base = 0;
>>  		*nr_irqs = 0;
>> @@ -1879,6 +1908,27 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>>  }
>>  EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
>>
>> +int dfl_find_param(void __iomem *base, resource_size_t max, int param)
>> +{
>> +	int off = 0;
>> +	u64 v, next;
>> +
>> +	while (off < max) {
>> +		v = readq(base + off);
>> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return off;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!next)
>> +			break;
>> +
>> +		off += next;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_find_param);
>> +
>>  static void __exit dfl_fpga_exit(void)
>>  {
>>  	dfl_chardev_uinit();
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 1e53468ba8d8..33e21c360671 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -63,6 +63,10 @@
>>  #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_ID_MSIX	0x1
>> +#define DFHv1_PARAM_MSIX_STARTV	0x8
>> +#define DFHv1_PARAM_MSIX_NUMV	0xc
>> +
>>  /**
>>   * enum dfl_id_type - define the DFL FIU types
>>   */
>> @@ -136,4 +140,14 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>>  	module_driver(__dfl_driver, dfl_driver_register, \
>>  		      dfl_driver_unregister)
>>
>> +/**
>> + * dfl_find_param() - find the offset of the given parameter
>> + * @base: base pointer to start of dfl parameters in DFH
>> + * @max: maximum offset to search
>> + * @param: id of dfl parameter
>> + *
>> + * Return: positive offset on success, negative error code otherwise.
>> + */
>> +int dfl_find_param(void __iomem *base, resource_size_t max, int param);
>> +
>>  #endif /* __LINUX_DFL_H */
>> --
>> 2.25.1
>>
>

[PATCH v2 5/6] fpga: dfl: parse the location of the feature's registers from DFHv1

[matthew.gerlach]

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

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>
---
v2: Introduced in v2.
---
 drivers/fpga/dfl.c  | 26 +++++++++++++++++++++++++-
 drivers/fpga/dfl.h  |  4 ++++
 include/linux/dfl.h |  4 ++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index dfd3f563c92d..6fb4f30f93cf 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
 	ddev->feature_id = feature->id;
 	ddev->revision = feature->revision;
 	ddev->cdev = pdata->dfl_cdev;
+	ddev->csr_start = feature->csr_start;
+	ddev->csr_size = feature->csr_size;
 
 	/* add mmio resource */
 	parent_res = &pdev->resource[feature->resource_index];
@@ -708,18 +710,25 @@ struct build_feature_devs_info {
  * struct dfl_feature_info - sub feature info collected during feature dev build
  *
  * @fid: id of this sub feature.
+ * @revision: revision of this sub feature
+ * @dfh_version: version of Device Feature Header (DFH)
  * @mmio_res: mmio resource of this sub feature.
  * @ioaddr: mapped base address of mmio resource.
  * @node: node in sub_features linked list.
+ * @csr_start: DFHv1 start of feature registers
+ * @csr_size: DFHv1 size of feature registers
  * @irq_base: start of irq index in this sub feature.
  * @nr_irqs: number of irqs of this sub feature.
  */
 struct dfl_feature_info {
 	u16 fid;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	void __iomem *ioaddr;
 	struct list_head node;
+	resource_size_t csr_start;
+	resource_size_t csr_size;
 	unsigned int irq_base;
 	unsigned int nr_irqs;
 };
@@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 		feature->dev = fdev;
 		feature->id = finfo->fid;
 		feature->revision = finfo->revision;
+		feature->csr_start = finfo->csr_start;
+		feature->csr_size = finfo->csr_size;
 
 		/*
 		 * the FIU header feature has some fundamental functions (sriov
@@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 {
 	unsigned int irq_base, nr_irqs;
 	struct dfl_feature_info *finfo;
+	u8 dfh_version = 0;
 	u8 revision = 0;
 	int ret;
 	u64 v;
@@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 	if (fid != FEATURE_ID_AFU) {
 		v = readq(binfo->ioaddr + ofst);
 		revision = FIELD_GET(DFH_REVISION, v);
-
+		dfh_version = FIELD_GET(DFH_VERSION, v);
 		/* read feature size and id if inputs are invalid */
 		size = size ? size : feature_size(v);
 		fid = fid ? fid : feature_id(v);
@@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 
 	finfo->fid = fid;
 	finfo->revision = revision;
+	finfo->dfh_version = dfh_version;
 	finfo->mmio_res.start = binfo->start + ofst;
 	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
 	finfo->mmio_res.flags = IORESOURCE_MEM;
 	finfo->irq_base = irq_base;
 	finfo->nr_irqs = nr_irqs;
 
+	if (dfh_version == 1) {
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+		if (v & DFHv1_CSR_ADDR_REL)
+			finfo->csr_start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+		else
+			finfo->csr_start = binfo->start + ofst + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+		finfo->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
+	}
+
 	list_add_tail(&finfo->node, &binfo->sub_features);
 	binfo->feature_num++;
 
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index e620fcb02b5a..64cedd00dca4 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -217,6 +217,8 @@ struct dfl_feature_irq_ctx {
  *		    this index is used to find its mmio resource from the
  *		    feature dev (platform device)'s resources.
  * @ioaddr: mapped mmio resource address.
+ * @csr_start: DFHv1 start of feature registers
+ * @csr_size: DFHv1 size of feature registers
  * @irq_ctx: interrupt context list.
  * @nr_irqs: number of interrupt contexts.
  * @ops: ops of this sub feature.
@@ -229,6 +231,8 @@ struct dfl_feature {
 	u8 revision;
 	int resource_index;
 	void __iomem *ioaddr;
+	resource_size_t csr_start;
+	resource_size_t csr_size;
 	struct dfl_feature_irq_ctx *irq_ctx;
 	unsigned int nr_irqs;
 	const struct dfl_feature_ops *ops;
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 33e21c360671..7d74ef8d1d20 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -84,6 +84,8 @@ enum dfl_id_type {
  * @type: type of DFL FIU of the device. See enum dfl_id_type.
  * @feature_id: feature identifier local to its DFL FIU type.
  * @mmio_res: mmio resource of this dfl device.
+ * @csr_start: DFHv1 start of feature registers
+ * @csr_size: DFHv1 size of feature registers
  * @irqs: list of Linux IRQ numbers of this dfl device.
  * @num_irqs: number of IRQs supported by this dfl device.
  * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
@@ -96,6 +98,8 @@ struct dfl_device {
 	u16 feature_id;
 	u8 revision;
 	struct resource mmio_res;
+	resource_size_t csr_start;
+	resource_size_t csr_size;
 	int *irqs;
 	unsigned int num_irqs;
 	struct dfl_fpga_cdev *cdev;
-- 
2.25.1

Re: [PATCH v2 5/6] fpga: dfl: parse the location of the feature's registers from DFHv1

[Ilpo Järvinen]

On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@intel.com>
> 
> 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>
> ---
> v2: Introduced in v2.
> ---
>  drivers/fpga/dfl.c  | 26 +++++++++++++++++++++++++-
>  drivers/fpga/dfl.h  |  4 ++++
>  include/linux/dfl.h |  4 ++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index dfd3f563c92d..6fb4f30f93cf 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>  	ddev->feature_id = feature->id;
>  	ddev->revision = feature->revision;
>  	ddev->cdev = pdata->dfl_cdev;
> +	ddev->csr_start = feature->csr_start;
> +	ddev->csr_size = feature->csr_size;
>  
>  	/* add mmio resource */
>  	parent_res = &pdev->resource[feature->resource_index];
> @@ -708,18 +710,25 @@ struct build_feature_devs_info {
>   * struct dfl_feature_info - sub feature info collected during feature dev build
>   *
>   * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
>   * @mmio_res: mmio resource of this sub feature.
>   * @ioaddr: mapped base address of mmio resource.
>   * @node: node in sub_features linked list.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irq_base: start of irq index in this sub feature.
>   * @nr_irqs: number of irqs of this sub feature.
>   */
>  struct dfl_feature_info {
>  	u16 fid;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	void __iomem *ioaddr;
>  	struct list_head node;
> +	resource_size_t csr_start;
> +	resource_size_t csr_size;
>  	unsigned int irq_base;
>  	unsigned int nr_irqs;
>  };
> @@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->revision = finfo->revision;
> +		feature->csr_start = finfo->csr_start;
> +		feature->csr_size = finfo->csr_size;
>  
>  		/*
>  		 * the FIU header feature has some fundamental functions (sriov
> @@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  {
>  	unsigned int irq_base, nr_irqs;
>  	struct dfl_feature_info *finfo;
> +	u8 dfh_version = 0;
>  	u8 revision = 0;
>  	int ret;
>  	u64 v;
> @@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  	if (fid != FEATURE_ID_AFU) {
>  		v = readq(binfo->ioaddr + ofst);
>  		revision = FIELD_GET(DFH_REVISION, v);
> -
> +		dfh_version = FIELD_GET(DFH_VERSION, v);
>  		/* read feature size and id if inputs are invalid */
>  		size = size ? size : feature_size(v);
>  		fid = fid ? fid : feature_id(v);
> @@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  
>  	finfo->fid = fid;
>  	finfo->revision = revision;
> +	finfo->dfh_version = dfh_version;
>  	finfo->mmio_res.start = binfo->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>  	finfo->irq_base = irq_base;
>  	finfo->nr_irqs = nr_irqs;

Ordering here seems slightly odd. If finfo would be built before calling
parse_feature_irqs(), it could be passed into there and there would be no 
need to:
- read version for second time
- pass irq_base & nr_irqs as pointer parameters

-- 
 i.

Re: [PATCH v2 5/6] fpga: dfl: parse the location of the feature's registers from DFHv1

[Muddebihal, Basheer Ahmed]

On 9/23/2022 5:17 AM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@intel.com>
> 
> 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>
> ---
> v2: Introduced in v2.
> ---
>  drivers/fpga/dfl.c  | 26 +++++++++++++++++++++++++-
>  drivers/fpga/dfl.h  |  4 ++++
>  include/linux/dfl.h |  4 ++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index dfd3f563c92d..6fb4f30f93cf 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>  	ddev->feature_id = feature->id;
>  	ddev->revision = feature->revision;
>  	ddev->cdev = pdata->dfl_cdev;
> +	ddev->csr_start = feature->csr_start;
> +	ddev->csr_size = feature->csr_size;
>  
>  	/* add mmio resource */
>  	parent_res = &pdev->resource[feature->resource_index];
> @@ -708,18 +710,25 @@ struct build_feature_devs_info {
>   * struct dfl_feature_info - sub feature info collected during feature dev build
>   *
>   * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
>   * @mmio_res: mmio resource of this sub feature.
>   * @ioaddr: mapped base address of mmio resource.
>   * @node: node in sub_features linked list.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irq_base: start of irq index in this sub feature.
>   * @nr_irqs: number of irqs of this sub feature.
>   */
>  struct dfl_feature_info {
>  	u16 fid;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	void __iomem *ioaddr;
>  	struct list_head node;
> +	resource_size_t csr_start;
> +	resource_size_t csr_size;
>  	unsigned int irq_base;
>  	unsigned int nr_irqs;
>  };
> @@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->revision = finfo->revision;> +		feature->csr_start = finfo->csr_start;
> +		feature->csr_size = finfo->csr_size;
>  
>  		/*
>  		 * the FIU header feature has some fundamental functions (sriov
> @@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  {
>  	unsigned int irq_base, nr_irqs;
>  	struct dfl_feature_info *finfo;
> +	u8 dfh_version = 0;
>  	u8 revision = 0;
>  	int ret;
>  	u64 v;
> @@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  	if (fid != FEATURE_ID_AFU) {
>  		v = readq(binfo->ioaddr + ofst);
>  		revision = FIELD_GET(DFH_REVISION, v);
> -
> +		dfh_version = FIELD_GET(DFH_VERSION, v);
>  		/* read feature size and id if inputs are invalid */
>  		size = size ? size : feature_size(v);
>  		fid = fid ? fid : feature_id(v);
> @@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  
>  	finfo->fid = fid;
>  	finfo->revision = revision;
> +	finfo->dfh_version = dfh_version;
>  	finfo->mmio_res.start = binfo->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>  	finfo->irq_base = irq_base;
>  	finfo->nr_irqs = nr_irqs;
>  
> +	if (dfh_version == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->csr_start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +		else
> +			finfo->csr_start = binfo->start + ofst + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
> +	}
> +
>  	list_add_tail(&finfo->node, &binfo->sub_features);
>  	binfo->feature_num++;
>  
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index e620fcb02b5a..64cedd00dca4 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -217,6 +217,8 @@ struct dfl_feature_irq_ctx {
>   *		    this index is used to find its mmio resource from the
>   *		    feature dev (platform device)'s resources.
>   * @ioaddr: mapped mmio resource address.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irq_ctx: interrupt context list.
>   * @nr_irqs: number of interrupt contexts.
>   * @ops: ops of this sub feature.
> @@ -229,6 +231,8 @@ struct dfl_feature {
>  	u8 revision;
>  	int resource_index;
>  	void __iomem *ioaddr;
> +	resource_size_t csr_start;
> +	resource_size_t csr_size;
>  	struct dfl_feature_irq_ctx *irq_ctx;
>  	unsigned int nr_irqs;
>  	const struct dfl_feature_ops *ops;
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 33e21c360671..7d74ef8d1d20 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -84,6 +84,8 @@ enum dfl_id_type {
>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
>   * @feature_id: feature identifier local to its DFL FIU type.
>   * @mmio_res: mmio resource of this dfl device.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irqs: list of Linux IRQ numbers of this dfl device.
>   * @num_irqs: number of IRQs supported by this dfl device.
>   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> @@ -96,6 +98,8 @@ struct dfl_device {
>  	u16 feature_id;
>  	u8 revision;
> +	u8 dfh_version;

This is already parsed as part of info dfl_feature_info, Can we add dfh_version to store it for further use ? 

>  	struct resource mmio_res;
> +	resource_size_t csr_start;
> +	resource_size_t csr_size;
>  	int *irqs;
>  	unsigned int num_irqs;
>  	struct dfl_fpga_cdev *cdev;

Re: [PATCH v2 5/6] fpga: dfl: parse the location of the feature's registers from DFHv1

[Xu Yilun]

On 2022-09-23 at 05:17:44 -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@intel.com>
> 
> 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>
> ---
> v2: Introduced in v2.
> ---
>  drivers/fpga/dfl.c  | 26 +++++++++++++++++++++++++-
>  drivers/fpga/dfl.h  |  4 ++++
>  include/linux/dfl.h |  4 ++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index dfd3f563c92d..6fb4f30f93cf 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -381,6 +381,8 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>  	ddev->feature_id = feature->id;
>  	ddev->revision = feature->revision;
>  	ddev->cdev = pdata->dfl_cdev;
> +	ddev->csr_start = feature->csr_start;
> +	ddev->csr_size = feature->csr_size;
>  
>  	/* add mmio resource */
>  	parent_res = &pdev->resource[feature->resource_index];
> @@ -708,18 +710,25 @@ struct build_feature_devs_info {
>   * struct dfl_feature_info - sub feature info collected during feature dev build
>   *
>   * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
>   * @mmio_res: mmio resource of this sub feature.
>   * @ioaddr: mapped base address of mmio resource.
>   * @node: node in sub_features linked list.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irq_base: start of irq index in this sub feature.
>   * @nr_irqs: number of irqs of this sub feature.
>   */
>  struct dfl_feature_info {
>  	u16 fid;
>  	u8 revision;
> +	u8 dfh_version;
>  	struct resource mmio_res;
>  	void __iomem *ioaddr;
>  	struct list_head node;
> +	resource_size_t csr_start;
> +	resource_size_t csr_size;
>  	unsigned int irq_base;
>  	unsigned int nr_irqs;
>  };
> @@ -797,6 +806,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->revision = finfo->revision;
> +		feature->csr_start = finfo->csr_start;
> +		feature->csr_size = finfo->csr_size;
>  
>  		/*
>  		 * the FIU header feature has some fundamental functions (sriov
> @@ -1054,6 +1065,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  {
>  	unsigned int irq_base, nr_irqs;
>  	struct dfl_feature_info *finfo;
> +	u8 dfh_version = 0;
>  	u8 revision = 0;
>  	int ret;
>  	u64 v;
> @@ -1061,7 +1073,7 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  	if (fid != FEATURE_ID_AFU) {
>  		v = readq(binfo->ioaddr + ofst);
>  		revision = FIELD_GET(DFH_REVISION, v);
> -
> +		dfh_version = FIELD_GET(DFH_VERSION, v);
>  		/* read feature size and id if inputs are invalid */
>  		size = size ? size : feature_size(v);
>  		fid = fid ? fid : feature_id(v);
> @@ -1080,12 +1092,24 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  
>  	finfo->fid = fid;
>  	finfo->revision = revision;
> +	finfo->dfh_version = dfh_version;
>  	finfo->mmio_res.start = binfo->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>  	finfo->irq_base = irq_base;
>  	finfo->nr_irqs = nr_irqs;
>  
> +	if (dfh_version == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->csr_start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +		else
> +			finfo->csr_start = binfo->start + ofst + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->csr_size = FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v);
> +	}
> +
>  	list_add_tail(&finfo->node, &binfo->sub_features);
>  	binfo->feature_num++;
>  
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index e620fcb02b5a..64cedd00dca4 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -217,6 +217,8 @@ struct dfl_feature_irq_ctx {
>   *		    this index is used to find its mmio resource from the
>   *		    feature dev (platform device)'s resources.
>   * @ioaddr: mapped mmio resource address.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irq_ctx: interrupt context list.
>   * @nr_irqs: number of interrupt contexts.
>   * @ops: ops of this sub feature.
> @@ -229,6 +231,8 @@ struct dfl_feature {
>  	u8 revision;
>  	int resource_index;
>  	void __iomem *ioaddr;
> +	resource_size_t csr_start;
> +	resource_size_t csr_size;
>  	struct dfl_feature_irq_ctx *irq_ctx;
>  	unsigned int nr_irqs;
>  	const struct dfl_feature_ops *ops;
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 33e21c360671..7d74ef8d1d20 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -84,6 +84,8 @@ enum dfl_id_type {
>   * @type: type of DFL FIU of the device. See enum dfl_id_type.
>   * @feature_id: feature identifier local to its DFL FIU type.
>   * @mmio_res: mmio resource of this dfl device.
> + * @csr_start: DFHv1 start of feature registers
> + * @csr_size: DFHv1 size of feature registers
>   * @irqs: list of Linux IRQ numbers of this dfl device.
>   * @num_irqs: number of IRQs supported by this dfl device.
>   * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> @@ -96,6 +98,8 @@ struct dfl_device {
>  	u16 feature_id;
>  	u8 revision;
>  	struct resource mmio_res;
> +	resource_size_t csr_start;
> +	resource_size_t csr_size;

I think these register start & size info could be stored in
struct resource mmio_res. This is the generic way for the driver to
understand the register layout of the device. And it makes the dfl
driver code easier to be understood by other domains reviewers.

Thanks,
Yilun

>  	int *irqs;
>  	unsigned int num_irqs;
>  	struct dfl_fpga_cdev *cdev;
> -- 
> 2.25.1
> 

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

[matthew.gerlach]

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

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

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reported-by: kernel test robot <lkp@intel.com>
---
v2: clean up error messages
    alphabetize header files
    fix 'missing prototype' error by making function static
    tried to sort Makefile and Kconfig better
---
 drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |   9 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 include/linux/dfl.h                |   7 ++
 4 files changed, 194 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..539ca6138eda
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Ananda Ravuri <ananda.ravuri@intel.com>
+ *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/dfl.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+
+struct dfl_uart {
+	void __iomem   *csr_base;
+	struct device  *dev;
+	u64             uart_clk;
+	u64             fifo_len;
+	unsigned int    fifo_size;
+	unsigned int    reg_shift;
+	unsigned int    line;
+};
+
+static int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
+{
+	void __iomem *param_base;
+	int off;
+	u64 v;
+
+	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
+
+	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
+		dev_err(dfluart->dev, "missing required DFH parameters\n");
+		return -EINVAL;
+	}
+
+	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
+
+	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
+	if (off < 0) {
+		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
+		return -EINVAL;
+	}
+
+	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
+	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
+
+	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
+	if (off < 0) {
+		dev_err(dfluart->dev, "missing FIFO_LEN param\n");
+		return -EINVAL;
+	}
+
+	dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
+	dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
+
+	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
+	if (off < 0) {
+		dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
+		return -EINVAL;
+	}
+
+	v = readq(param_base + off + DFHv1_PARAM_DATA);
+	dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);
+	dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
+	dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
+		dfluart->fifo_size, dfluart->reg_shift);
+
+	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));
+
+	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+	if (!dfluart)
+		return -ENOMEM;
+
+	dfluart->dev = dev;
+
+	dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+	if (IS_ERR(dfluart->csr_base)) {
+		return PTR_ERR(dfluart->csr_base);
+	}
+
+	ret = feature_uart_walk(dfluart, resource_size(&dfl_dev->mmio_res));
+
+	devm_iounmap(dev, dfluart->csr_base);
+	devm_release_mem_region(dev, dfl_dev->mmio_res.start, resource_size(&dfl_dev->mmio_res));
+
+	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];
+
+	switch (dfluart->fifo_len) {
+	case 32:
+		uart.port.type = PORT_ALTR_16550_F32;
+		break;
+
+	case 64:
+		uart.port.type = PORT_ALTR_16550_F64;
+		break;
+
+	case 128:
+		uart.port.type = PORT_ALTR_16550_F128;
+		break;
+
+	default:
+		dev_err(dev, "bad fifo_len %llu\n", dfluart->fifo_len);
+		return -EINVAL;
+	}
+
+	uart.port.iotype = UPIO_MEM32;
+	uart.port.mapbase = dfl_dev->csr_start;
+	uart.port.mapsize = dfl_dev->csr_size;
+	uart.port.regshift = dfluart->reg_shift;
+	uart.port.uartclk = dfluart->uart_clk;
+	uart.port.flags |= UPF_IOREMAP;
+
+	/* register the port */
+	ret = serial8250_register_8250_port(&uart);
+	if (ret < 0) {
+		dev_err(dev, "unable to register 8250 port %d.\n", ret);
+		return -EINVAL;
+	}
+	dev_info(dev, "serial8250_register_8250_port %d\n", ret);
+	dfluart->line = ret;
+	dev_set_drvdata(dev, dfluart);
+
+	return 0;
+}
+
+static void dfl_uart_remove(struct dfl_device *dfl_dev)
+{
+	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
+
+	if (dfluart->line > 0)
+		serial8250_unregister_port(dfluart->line);
+}
+
+#define FME_FEATURE_ID_UART 0x24
+
+static const struct dfl_device_id dfl_uart_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_UART },
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
+
+static struct dfl_driver dfl_uart_driver = {
+	.drv = {
+		.name = "dfl-uart",
+	},
+	.id_table = dfl_uart_ids,
+	.probe = dfl_uart_probe,
+	.remove = dfl_uart_remove,
+};
+module_dfl_driver(dfl_uart_driver);
+
+MODULE_DESCRIPTION("DFL Intel UART driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index d0b49e15fbf5..5c6497ce5c12 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -361,6 +361,15 @@ config SERIAL_8250_BCM2835AUX
 
 	  If unsure, say N.
 
+config SERIAL_8250_DFL
+	tristate "DFL bus driver for Altera 16550 UART"
+	depends on SERIAL_8250 && FPGA_DFL
+	help
+	  This option enables support for a Device Feature List (DFL) bus
+	  driver for the Altera 16650 UART.  One or more Altera 16650 UARTs
+	  can be instantiated in a FPGA and then be discovered during
+	  enumeration of the DFL bus.
+
 config SERIAL_8250_FSL
 	bool "Freescale 16550 UART support" if COMPILE_TEST && !(PPC || ARM || ARM64)
 	depends on SERIAL_8250_CONSOLE
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index bee908f99ea0..32006e0982d1 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE)	+= 8250_early.o
 obj-$(CONFIG_SERIAL_8250_FOURPORT)	+= 8250_fourport.o
 obj-$(CONFIG_SERIAL_8250_ACCENT)	+= 8250_accent.o
 obj-$(CONFIG_SERIAL_8250_BOCA)		+= 8250_boca.o
+obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 7d74ef8d1d20..a17aeccc501e 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -67,6 +67,13 @@
 #define DFHv1_PARAM_MSIX_STARTV	0x8
 #define DFHv1_PARAM_MSIX_NUMV	0xc
 
+#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 v2 6/6] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Fri, Sep 23, 2022 at 05:17:45AM -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>

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

New feature may not be reported. How?!

-- 
With Best Regards,
Andy Shevchenko

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

[Ilpo Järvinen]

On Fri, 23 Sep 2022, matthew.gerlach@linux.intel.com wrote:

> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> v2: clean up error messages
>     alphabetize header files
>     fix 'missing prototype' error by making function static
>     tried to sort Makefile and Kconfig better
> ---
>  drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |   9 ++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  include/linux/dfl.h                |   7 ++
>  4 files changed, 194 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..539ca6138eda
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA UART
> + *
> + * Copyright (C) 2022 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Ananda Ravuri <ananda.ravuri@intel.com>
> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/dfl.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +
> +struct dfl_uart {
> +	void __iomem   *csr_base;
> +	struct device  *dev;
> +	u64             uart_clk;
> +	u64             fifo_len;
> +	unsigned int    fifo_size;
> +	unsigned int    reg_shift;

Why to make this intermediate storage for these values, wouldn't it be 
simpler to just fill them into the uart_port directly?

> +	unsigned int    line;
> +};
> +
> +static int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
> +{
> +	void __iomem *param_base;
> +	int off;
> +	u64 v;
> +
> +	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +		dev_err(dfluart->dev, "missing required DFH parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;

Are all callers of dfl_find_param() expected to run these same checks and
calculations? Perhaps some helper to find param base would be useful and
it could also run those checks.

> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing CLK_FRQ param\n");
> +		return -EINVAL;
> +	}
> +
> +	dfluart->uart_clk = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dev_dbg(dfluart->dev, "UART_CLK_ID %llu Hz\n", dfluart->uart_clk);
> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_FIFO_LEN);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing FIFO_LEN param\n");
> +		return -EINVAL;
> +	}
> +
> +	dfluart->fifo_len = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dev_dbg(dfluart->dev, "UART_FIFO_ID fifo_len %llu\n", dfluart->fifo_len);
> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_REG_LAYOUT);
> +	if (off < 0) {
> +		dev_err(dfluart->dev, "missing REG_LAYOUT param\n");
> +		return -EINVAL;
> +	}
> +
> +	v = readq(param_base + off + DFHv1_PARAM_DATA);
> +	dfluart->fifo_size = FIELD_GET(DFHv1_PARAM_ID_REG_WIDTH, v);

???

> +	dfluart->reg_shift = FIELD_GET(DFHv1_PARAM_ID_REG_SHIFT, v);
> +	dev_dbg(dfluart->dev, "UART_LAYOUT_ID width %d shift %d\n",
> +		dfluart->fifo_size, dfluart->reg_shift);
> +
> +	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));
> +
> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> +	if (!dfluart)
> +		return -ENOMEM;
> +
> +	dfluart->dev = dev;
> +
> +	dfluart->csr_base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +	if (IS_ERR(dfluart->csr_base)) {
> +		return PTR_ERR(dfluart->csr_base);
> +	}

No need for braces.

> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
> +{
> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
> +
> +	if (dfluart->line > 0)

Line 0 is valid uart port. Perhaps you'd never see it here due to how the 
8250 driver allocs ports but it would be better to not make this kind of 
assumption.

> +		serial8250_unregister_port(dfluart->line);
> +}

> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 7d74ef8d1d20..a17aeccc501e 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -67,6 +67,13 @@
>  #define DFHv1_PARAM_MSIX_STARTV	0x8
>  #define DFHv1_PARAM_MSIX_NUMV	0xc
>  
> +#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)

Should UART be included into these names or are they intended to be more 
generic parameters (for non-UART uses)?


-- 
 i.

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

[Xu Yilun]

On 2022-09-23 at 05:17:45 -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>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> v2: clean up error messages
>     alphabetize header files
>     fix 'missing prototype' error by making function static
>     tried to sort Makefile and Kconfig better
> ---
>  drivers/tty/serial/8250/8250_dfl.c | 177 +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |   9 ++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  include/linux/dfl.h                |   7 ++
>  4 files changed, 194 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..539ca6138eda
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA UART
> + *
> + * Copyright (C) 2022 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Ananda Ravuri <ananda.ravuri@intel.com>
> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/dfl.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +
> +struct dfl_uart {
> +	void __iomem   *csr_base;
> +	struct device  *dev;
> +	u64             uart_clk;
> +	u64             fifo_len;
> +	unsigned int    fifo_size;
> +	unsigned int    reg_shift;
> +	unsigned int    line;
> +};
> +
> +static int feature_uart_walk(struct dfl_uart *dfluart, resource_size_t max)
> +{
> +	void __iomem *param_base;
> +	int off;
> +	u64 v;
> +
> +	v = readq(dfluart->csr_base + DFHv1_CSR_SIZE_GRP);
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS, v)) {
> +		dev_err(dfluart->dev, "missing required DFH parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	param_base = dfluart->csr_base + DFHv1_PARAM_HDR;
> +
> +	off = dfl_find_param(param_base, max, DFHv1_PARAM_ID_CLK_FRQ);

I think the parameters of dfl_find_param are too complicated to be used
outside dfl framework (It is OK for dfl internal use).

How about:

  dfl_find_param(struct dfl_device *ddev, int param_id)

Thanks,
Yilun