From: Debarati Biswas <[email protected]>
Device Feature List (DFL) drivers may be defined in subdirectories other
than drivers/fpga, and each DFL driver should have access to the Device
Feature Header (DFH) register, which contains revision and type
information. This change moves the macros specific to the DFH register
from drivers/fpga/dfl.h to include/linux/dfl.h.
Signed-off-by: Debarati Biswas <[email protected]>
Signed-off-by: Russ Weight <[email protected]>
Signed-off-by: Martin Hundebøll <[email protected]>
---
drivers/fpga/dfl.h | 48 +----------------------------------------
include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 53 insertions(+), 47 deletions(-)
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 2b82c96ba56c..6ed0353e9a99 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,32 +54,6 @@
#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
-
/* Next AFU Register Bitfield */
#define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
@@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
return pdata->dev->dev.parent->parent;
}
-static inline bool dfl_feature_is_fme(void __iomem *base)
-{
- u64 v = readq(base + DFH);
-
- return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
- (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
-}
-
-static inline bool dfl_feature_is_port(void __iomem *base)
-{
- u64 v = readq(base + DFH);
-
- return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
- (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
-}
-
-static inline u8 dfl_feature_revision(void __iomem *base)
-{
- return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
-}
-
/**
* struct dfl_fpga_enum_info - DFL FPGA enumeration information
*
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 6cc10982351a..1cd86b2e7cb1 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -8,7 +8,9 @@
#ifndef __LINUX_DFL_H
#define __LINUX_DFL_H
+#include <linux/bitfield.h>
#include <linux/device.h>
+#include <linux/io.h>
#include <linux/mod_devicetable.h>
/**
@@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
module_driver(__dfl_driver, dfl_driver_register, \
dfl_driver_unregister)
+/*
+ * 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)
+#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
+
+/* Function to read from DFH and check if the Feature type is FME */
+static inline bool dfl_feature_is_fme(void __iomem *base)
+{
+ u64 v = readq(base + DFH);
+
+ return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
+ (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
+}
+
+/* Function to read from DFH and check if the Feature type is port*/
+static inline bool dfl_feature_is_port(void __iomem *base)
+{
+ u64 v = readq(base + DFH);
+
+ return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
+ (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
+}
+
+/* Function to read feature revision from DFH */
+static inline u8 dfl_feature_revision(void __iomem *base)
+{
+ return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
+}
+
#endif /* __LINUX_DFL_H */
--
2.31.0
> Subject: [PATCH 2/4] fpga: dfl: Move DFH header register macros to linux/dfl.h
>
> From: Debarati Biswas <[email protected]>
>
> Device Feature List (DFL) drivers may be defined in subdirectories other
> than drivers/fpga, and each DFL driver should have access to the Device
> Feature Header (DFH) register, which contains revision and type
> information. This change moves the macros specific to the DFH register
> from drivers/fpga/dfl.h to include/linux/dfl.h.
Looks like it requires to access the revision info in the next patch, because
current dfl_device doesn't expose related information.
@Yilun, do you have any concern to expose those info via dfl_device?
Thanks
Hao
>
> Signed-off-by: Debarati Biswas <[email protected]>
> Signed-off-by: Russ Weight <[email protected]>
> Signed-off-by: Martin Hundebøll <[email protected]>
> ---
> drivers/fpga/dfl.h | 48 +----------------------------------------
> include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 2b82c96ba56c..6ed0353e9a99 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,32 +54,6 @@
> #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
> -
> /* Next AFU Register Bitfield */
> #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to
> next AFU */
>
> @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct
> dfl_feature_platform_data *pdata)
> return pdata->dev->dev.parent->parent;
> }
>
> -static inline bool dfl_feature_is_fme(void __iomem *base)
> -{
> - u64 v = readq(base + DFH);
> -
> - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> -}
> -
> -static inline bool dfl_feature_is_port(void __iomem *base)
> -{
> - u64 v = readq(base + DFH);
> -
> - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> -}
> -
> -static inline u8 dfl_feature_revision(void __iomem *base)
> -{
> - return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> -}
> -
> /**
> * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> *
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 6cc10982351a..1cd86b2e7cb1 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -8,7 +8,9 @@
> #ifndef __LINUX_DFL_H
> #define __LINUX_DFL_H
>
> +#include <linux/bitfield.h>
> #include <linux/device.h>
> +#include <linux/io.h>
> #include <linux/mod_devicetable.h>
>
> /**
> @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> module_driver(__dfl_driver, dfl_driver_register, \
> dfl_driver_unregister)
>
> +/*
> + * 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)
> +#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
> +
> +/* Function to read from DFH and check if the Feature type is FME */
> +static inline bool dfl_feature_is_fme(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> +}
> +
> +/* Function to read from DFH and check if the Feature type is port*/
> +static inline bool dfl_feature_is_port(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> +}
> +
> +/* Function to read feature revision from DFH */
> +static inline u8 dfl_feature_revision(void __iomem *base)
> +{
> + return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> +}
> +
> #endif /* __LINUX_DFL_H */
> --
> 2.31.0
On 6/21/21 12:06 AM, Martin Hundebøll wrote:
> From: Debarati Biswas <[email protected]>
>
> Device Feature List (DFL) drivers may be defined in subdirectories other
> than drivers/fpga, and each DFL driver should have access to the Device
> Feature Header (DFH) register, which contains revision and type
> information. This change moves the macros specific to the DFH register
> from drivers/fpga/dfl.h to include/linux/dfl.h.
>
> Signed-off-by: Debarati Biswas <[email protected]>
> Signed-off-by: Russ Weight <[email protected]>
> Signed-off-by: Martin Hundebøll <[email protected]>
> ---
> drivers/fpga/dfl.h | 48 +----------------------------------------
> include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 2b82c96ba56c..6ed0353e9a99 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -17,6 +17,7 @@
> #include <linux/bitfield.h>
bitfield.h was added to linux/dfl.h
Likely both aren't needed, try removing this one.
Tom
> #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,32 +54,6 @@
> #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
> -
> /* Next AFU Register Bitfield */
> #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
>
> @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
> return pdata->dev->dev.parent->parent;
> }
>
> -static inline bool dfl_feature_is_fme(void __iomem *base)
> -{
> - u64 v = readq(base + DFH);
> -
> - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> -}
> -
> -static inline bool dfl_feature_is_port(void __iomem *base)
> -{
> - u64 v = readq(base + DFH);
> -
> - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> -}
> -
> -static inline u8 dfl_feature_revision(void __iomem *base)
> -{
> - return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> -}
> -
> /**
> * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> *
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 6cc10982351a..1cd86b2e7cb1 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -8,7 +8,9 @@
> #ifndef __LINUX_DFL_H
> #define __LINUX_DFL_H
>
> +#include <linux/bitfield.h>
> #include <linux/device.h>
> +#include <linux/io.h>
> #include <linux/mod_devicetable.h>
>
> /**
> @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> module_driver(__dfl_driver, dfl_driver_register, \
> dfl_driver_unregister)
>
> +/*
> + * 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)
> +#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
> +
> +/* Function to read from DFH and check if the Feature type is FME */
> +static inline bool dfl_feature_is_fme(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> +}
> +
> +/* Function to read from DFH and check if the Feature type is port*/
> +static inline bool dfl_feature_is_port(void __iomem *base)
> +{
> + u64 v = readq(base + DFH);
> +
> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> +}
> +
> +/* Function to read feature revision from DFH */
> +static inline u8 dfl_feature_revision(void __iomem *base)
> +{
> + return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> +}
> +
> #endif /* __LINUX_DFL_H */
On Mon, Jun 21, 2021 at 06:56:28AM -0700, Tom Rix wrote:
>
> On 6/21/21 12:06 AM, Martin Hundeb?ll wrote:
> > From: Debarati Biswas <[email protected]>
> >
> > Device Feature List (DFL) drivers may be defined in subdirectories other
> > than drivers/fpga, and each DFL driver should have access to the Device
> > Feature Header (DFH) register, which contains revision and type
> > information. This change moves the macros specific to the DFH register
> > from drivers/fpga/dfl.h to include/linux/dfl.h.
> >
> > Signed-off-by: Debarati Biswas <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > Signed-off-by: Martin Hundeb?ll <[email protected]>
> > ---
> > drivers/fpga/dfl.h | 48 +----------------------------------------
> > include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 53 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 2b82c96ba56c..6ed0353e9a99 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -17,6 +17,7 @@
> > #include <linux/bitfield.h>
>
> bitfield.h was added to linux/dfl.h
>
> Likely both aren't needed, try removing this one.
The DFL register definitions are in dfl.h, and Source files which include
dfl.h are likely to use bitfield ops for DFL register access, so could we
keep it here?
Thanks,
Yilun
>
> Tom
>
> > #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,32 +54,6 @@
> > #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
> > -
> > /* Next AFU Register Bitfield */
> > #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
> > @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
> > return pdata->dev->dev.parent->parent;
> > }
> > -static inline bool dfl_feature_is_fme(void __iomem *base)
> > -{
> > - u64 v = readq(base + DFH);
> > -
> > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > -}
> > -
> > -static inline bool dfl_feature_is_port(void __iomem *base)
> > -{
> > - u64 v = readq(base + DFH);
> > -
> > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > -}
> > -
> > -static inline u8 dfl_feature_revision(void __iomem *base)
> > -{
> > - return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > -}
> > -
> > /**
> > * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> > *
> > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > index 6cc10982351a..1cd86b2e7cb1 100644
> > --- a/include/linux/dfl.h
> > +++ b/include/linux/dfl.h
> > @@ -8,7 +8,9 @@
> > #ifndef __LINUX_DFL_H
> > #define __LINUX_DFL_H
> > +#include <linux/bitfield.h>
> > #include <linux/device.h>
> > +#include <linux/io.h>
> > #include <linux/mod_devicetable.h>
> > /**
> > @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> > module_driver(__dfl_driver, dfl_driver_register, \
> > dfl_driver_unregister)
> > +/*
> > + * 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)
> > +#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
> > +
> > +/* Function to read from DFH and check if the Feature type is FME */
> > +static inline bool dfl_feature_is_fme(void __iomem *base)
> > +{
> > + u64 v = readq(base + DFH);
> > +
> > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > +}
> > +
> > +/* Function to read from DFH and check if the Feature type is port*/
> > +static inline bool dfl_feature_is_port(void __iomem *base)
> > +{
> > + u64 v = readq(base + DFH);
> > +
> > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > +}
> > +
> > +/* Function to read feature revision from DFH */
> > +static inline u8 dfl_feature_revision(void __iomem *base)
> > +{
> > + return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > +}
> > +
> > #endif /* __LINUX_DFL_H */
On Mon, Jun 21, 2021 at 06:19:15PM +0800, Wu, Hao wrote:
> > Subject: [PATCH 2/4] fpga: dfl: Move DFH header register macros to linux/dfl.h
> >
> > From: Debarati Biswas <[email protected]>
> >
> > Device Feature List (DFL) drivers may be defined in subdirectories other
> > than drivers/fpga, and each DFL driver should have access to the Device
> > Feature Header (DFH) register, which contains revision and type
> > information. This change moves the macros specific to the DFH register
> > from drivers/fpga/dfl.h to include/linux/dfl.h.
>
> Looks like it requires to access the revision info in the next patch, because
> current dfl_device doesn't expose related information.
>
> @Yilun, do you have any concern to expose those info via dfl_device?
Exposing these header register definitions are good to me. These registers
are in DFL device's MMIO region, so it is good to share these info with
all DFL drivers.
Thanks,
Yilun
>
> Thanks
> Hao
>
> >
> > Signed-off-by: Debarati Biswas <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > Signed-off-by: Martin Hundeb?ll <[email protected]>
> > ---
> > drivers/fpga/dfl.h | 48 +----------------------------------------
> > include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 53 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 2b82c96ba56c..6ed0353e9a99 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,32 +54,6 @@
> > #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
> > -
> > /* Next AFU Register Bitfield */
> > #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to
> > next AFU */
> >
> > @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct
> > dfl_feature_platform_data *pdata)
> > return pdata->dev->dev.parent->parent;
> > }
> >
> > -static inline bool dfl_feature_is_fme(void __iomem *base)
> > -{
> > - u64 v = readq(base + DFH);
> > -
> > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > -}
> > -
> > -static inline bool dfl_feature_is_port(void __iomem *base)
> > -{
> > - u64 v = readq(base + DFH);
> > -
> > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > -}
> > -
> > -static inline u8 dfl_feature_revision(void __iomem *base)
> > -{
> > - return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > -}
> > -
> > /**
> > * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> > *
> > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > index 6cc10982351a..1cd86b2e7cb1 100644
> > --- a/include/linux/dfl.h
> > +++ b/include/linux/dfl.h
> > @@ -8,7 +8,9 @@
> > #ifndef __LINUX_DFL_H
> > #define __LINUX_DFL_H
> >
> > +#include <linux/bitfield.h>
> > #include <linux/device.h>
> > +#include <linux/io.h>
> > #include <linux/mod_devicetable.h>
The <linux/io-64-nonatomic-lo-hi.h> is needed here, or readq/writeq
definitions may be missing on some platform, as the kernel test robot
says.
Thanks,
Yilun
> >
> > /**
> > @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> > module_driver(__dfl_driver, dfl_driver_register, \
> > dfl_driver_unregister)
> >
> > +/*
> > + * 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)
> > +#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
> > +
> > +/* Function to read from DFH and check if the Feature type is FME */
> > +static inline bool dfl_feature_is_fme(void __iomem *base)
> > +{
> > + u64 v = readq(base + DFH);
> > +
> > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > +}
> > +
> > +/* Function to read from DFH and check if the Feature type is port*/
> > +static inline bool dfl_feature_is_port(void __iomem *base)
> > +{
> > + u64 v = readq(base + DFH);
> > +
> > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > +}
> > +
> > +/* Function to read feature revision from DFH */
> > +static inline u8 dfl_feature_revision(void __iomem *base)
> > +{
> > + return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > +}
> > +
> > #endif /* __LINUX_DFL_H */
> > --
> > 2.31.0
>
> On Mon, Jun 21, 2021 at 06:19:15PM +0800, Wu, Hao wrote:
> > > Subject: [PATCH 2/4] fpga: dfl: Move DFH header register macros to
> linux/dfl.h
> > >
> > > From: Debarati Biswas <[email protected]>
> > >
> > > Device Feature List (DFL) drivers may be defined in subdirectories other
> > > than drivers/fpga, and each DFL driver should have access to the Device
> > > Feature Header (DFH) register, which contains revision and type
> > > information. This change moves the macros specific to the DFH register
> > > from drivers/fpga/dfl.h to include/linux/dfl.h.
> >
> > Looks like it requires to access the revision info in the next patch, because
> > current dfl_device doesn't expose related information.
> >
> > @Yilun, do you have any concern to expose those info via dfl_device?
>
> Exposing these header register definitions are good to me. These registers
> are in DFL device's MMIO region, so it is good to share these info with
> all DFL drivers.
I mean expose revision via dfl_device, as dfl core already reads the DFL
header, it sounds duplicate read in each dfl device driver. And if we
consider this as a common need from dfl device driver, then the code
can be moved to a common place as well.
I hope from dfl device driver side, it doesn't need to know details of
how DFH register is defined, only simple way from dfl device data
structure or some simple helper function, then dfl device driver could
know all common information from DFH.
How do you think?
Thanks
Hao
>
> Thanks,
> Yilun
>
> >
> > Thanks
> > Hao
> >
> > >
> > > Signed-off-by: Debarati Biswas <[email protected]>
> > > Signed-off-by: Russ Weight <[email protected]>
> > > Signed-off-by: Martin Hundeb?ll <[email protected]>
> > > ---
> > > drivers/fpga/dfl.h | 48 +----------------------------------------
> > > include/linux/dfl.h | 52
> +++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 53 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > > index 2b82c96ba56c..6ed0353e9a99 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,32 +54,6 @@
> > > #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
> > > -
> > > /* Next AFU Register Bitfield */
> > > #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset
> to
> > > next AFU */
> > >
> > > @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct
> > > dfl_feature_platform_data *pdata)
> > > return pdata->dev->dev.parent->parent;
> > > }
> > >
> > > -static inline bool dfl_feature_is_fme(void __iomem *base)
> > > -{
> > > - u64 v = readq(base + DFH);
> > > -
> > > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > > -}
> > > -
> > > -static inline bool dfl_feature_is_port(void __iomem *base)
> > > -{
> > > - u64 v = readq(base + DFH);
> > > -
> > > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > > -}
> > > -
> > > -static inline u8 dfl_feature_revision(void __iomem *base)
> > > -{
> > > - return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > > -}
> > > -
> > > /**
> > > * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> > > *
> > > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > > index 6cc10982351a..1cd86b2e7cb1 100644
> > > --- a/include/linux/dfl.h
> > > +++ b/include/linux/dfl.h
> > > @@ -8,7 +8,9 @@
> > > #ifndef __LINUX_DFL_H
> > > #define __LINUX_DFL_H
> > >
> > > +#include <linux/bitfield.h>
> > > #include <linux/device.h>
> > > +#include <linux/io.h>
> > > #include <linux/mod_devicetable.h>
>
> The <linux/io-64-nonatomic-lo-hi.h> is needed here, or readq/writeq
> definitions may be missing on some platform, as the kernel test robot
> says.
>
> Thanks,
> Yilun
>
> > >
> > > /**
> > > @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> > > module_driver(__dfl_driver, dfl_driver_register, \
> > > dfl_driver_unregister)
> > >
> > > +/*
> > > + * 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)
> > > +#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
> > > +
> > > +/* Function to read from DFH and check if the Feature type is FME */
> > > +static inline bool dfl_feature_is_fme(void __iomem *base)
> > > +{
> > > + u64 v = readq(base + DFH);
> > > +
> > > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > > +}
> > > +
> > > +/* Function to read from DFH and check if the Feature type is port*/
> > > +static inline bool dfl_feature_is_port(void __iomem *base)
> > > +{
> > > + u64 v = readq(base + DFH);
> > > +
> > > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > > +}
> > > +
> > > +/* Function to read feature revision from DFH */
> > > +static inline u8 dfl_feature_revision(void __iomem *base)
> > > +{
> > > + return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > > +}
> > > +
> > > #endif /* __LINUX_DFL_H */
> > > --
> > > 2.31.0
> >
On 6/21/21 9:56 PM, Xu Yilun wrote:
> On Mon, Jun 21, 2021 at 06:56:28AM -0700, Tom Rix wrote:
>> On 6/21/21 12:06 AM, Martin Hundebøll wrote:
>>> From: Debarati Biswas <[email protected]>
>>>
>>> Device Feature List (DFL) drivers may be defined in subdirectories other
>>> than drivers/fpga, and each DFL driver should have access to the Device
>>> Feature Header (DFH) register, which contains revision and type
>>> information. This change moves the macros specific to the DFH register
>>> from drivers/fpga/dfl.h to include/linux/dfl.h.
>>>
>>> Signed-off-by: Debarati Biswas <[email protected]>
>>> Signed-off-by: Russ Weight <[email protected]>
>>> Signed-off-by: Martin Hundebøll <[email protected]>
>>> ---
>>> drivers/fpga/dfl.h | 48 +----------------------------------------
>>> include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 53 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>>> index 2b82c96ba56c..6ed0353e9a99 100644
>>> --- a/drivers/fpga/dfl.h
>>> +++ b/drivers/fpga/dfl.h
>>> @@ -17,6 +17,7 @@
>>> #include <linux/bitfield.h>
>> bitfield.h was added to linux/dfl.h
>>
>> Likely both aren't needed, try removing this one.
> The DFL register definitions are in dfl.h, and Source files which include
> dfl.h are likely to use bitfield ops for DFL register access, so could we
> keep it here?
Not sure which dfl.h you mean.
It looks like you are copying header includes in both files and
including the one file in the other.
So it is likely you are including headers you don't need.
Tom
>
> Thanks,
> Yilun
>
>> Tom
>>
>>> #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,32 +54,6 @@
>>> #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
>>> -
>>> /* Next AFU Register Bitfield */
>>> #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
>>> @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
>>> return pdata->dev->dev.parent->parent;
>>> }
>>> -static inline bool dfl_feature_is_fme(void __iomem *base)
>>> -{
>>> - u64 v = readq(base + DFH);
>>> -
>>> - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
>>> - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
>>> -}
>>> -
>>> -static inline bool dfl_feature_is_port(void __iomem *base)
>>> -{
>>> - u64 v = readq(base + DFH);
>>> -
>>> - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
>>> - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
>>> -}
>>> -
>>> -static inline u8 dfl_feature_revision(void __iomem *base)
>>> -{
>>> - return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
>>> -}
>>> -
>>> /**
>>> * struct dfl_fpga_enum_info - DFL FPGA enumeration information
>>> *
>>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>>> index 6cc10982351a..1cd86b2e7cb1 100644
>>> --- a/include/linux/dfl.h
>>> +++ b/include/linux/dfl.h
>>> @@ -8,7 +8,9 @@
>>> #ifndef __LINUX_DFL_H
>>> #define __LINUX_DFL_H
>>> +#include <linux/bitfield.h>
>>> #include <linux/device.h>
>>> +#include <linux/io.h>
>>> #include <linux/mod_devicetable.h>
>>> /**
>>> @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>>> module_driver(__dfl_driver, dfl_driver_register, \
>>> dfl_driver_unregister)
>>> +/*
>>> + * 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)
>>> +#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
>>> +
>>> +/* Function to read from DFH and check if the Feature type is FME */
>>> +static inline bool dfl_feature_is_fme(void __iomem *base)
>>> +{
>>> + u64 v = readq(base + DFH);
>>> +
>>> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
>>> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
>>> +}
>>> +
>>> +/* Function to read from DFH and check if the Feature type is port*/
>>> +static inline bool dfl_feature_is_port(void __iomem *base)
>>> +{
>>> + u64 v = readq(base + DFH);
>>> +
>>> + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
>>> + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
>>> +}
>>> +
>>> +/* Function to read feature revision from DFH */
>>> +static inline u8 dfl_feature_revision(void __iomem *base)
>>> +{
>>> + return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
>>> +}
>>> +
>>> #endif /* __LINUX_DFL_H */
On Tue, Jun 22, 2021 at 05:31:57AM -0700, Tom Rix wrote:
>
> On 6/21/21 9:56 PM, Xu Yilun wrote:
> > On Mon, Jun 21, 2021 at 06:56:28AM -0700, Tom Rix wrote:
> > > On 6/21/21 12:06 AM, Martin Hundeb?ll wrote:
> > > > From: Debarati Biswas <[email protected]>
> > > >
> > > > Device Feature List (DFL) drivers may be defined in subdirectories other
> > > > than drivers/fpga, and each DFL driver should have access to the Device
> > > > Feature Header (DFH) register, which contains revision and type
> > > > information. This change moves the macros specific to the DFH register
> > > > from drivers/fpga/dfl.h to include/linux/dfl.h.
> > > >
> > > > Signed-off-by: Debarati Biswas <[email protected]>
> > > > Signed-off-by: Russ Weight <[email protected]>
> > > > Signed-off-by: Martin Hundeb?ll <[email protected]>
> > > > ---
> > > > drivers/fpga/dfl.h | 48 +----------------------------------------
> > > > include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 53 insertions(+), 47 deletions(-)
> > > >
> > > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > > > index 2b82c96ba56c..6ed0353e9a99 100644
> > > > --- a/drivers/fpga/dfl.h
> > > > +++ b/drivers/fpga/dfl.h
> > > > @@ -17,6 +17,7 @@
> > > > #include <linux/bitfield.h>
> > > bitfield.h was added to linux/dfl.h
> > >
> > > Likely both aren't needed, try removing this one.
> > The DFL register definitions are in dfl.h, and Source files which include
> > dfl.h are likely to use bitfield ops for DFL register access, so could we
> > keep it here?
>
> Not sure which dfl.h you mean.
>
> It looks like you are copying header includes in both files and including
> the one file in the other.
>
> So it is likely you are including headers you don't need.
Maybe I misunderstand, I thought you suggest remove the headers in both
drivers/fpga/dfl.h and linux/dfl.h.
Move the bitfield.h from drivers/fpga/dfl.h to linux/dfl.h is good to
me.
Thanks,
Yilun
>
> Tom
>
> >
> > Thanks,
> > Yilun
> >
> > > Tom
> > >
> > > > #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,32 +54,6 @@
> > > > #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
> > > > -
> > > > /* Next AFU Register Bitfield */
> > > > #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
> > > > @@ -403,27 +378,6 @@ struct device *dfl_fpga_pdata_to_parent(struct dfl_feature_platform_data *pdata)
> > > > return pdata->dev->dev.parent->parent;
> > > > }
> > > > -static inline bool dfl_feature_is_fme(void __iomem *base)
> > > > -{
> > > > - u64 v = readq(base + DFH);
> > > > -
> > > > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > > > -}
> > > > -
> > > > -static inline bool dfl_feature_is_port(void __iomem *base)
> > > > -{
> > > > - u64 v = readq(base + DFH);
> > > > -
> > > > - return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > > - (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > > > -}
> > > > -
> > > > -static inline u8 dfl_feature_revision(void __iomem *base)
> > > > -{
> > > > - return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > > > -}
> > > > -
> > > > /**
> > > > * struct dfl_fpga_enum_info - DFL FPGA enumeration information
> > > > *
> > > > diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> > > > index 6cc10982351a..1cd86b2e7cb1 100644
> > > > --- a/include/linux/dfl.h
> > > > +++ b/include/linux/dfl.h
> > > > @@ -8,7 +8,9 @@
> > > > #ifndef __LINUX_DFL_H
> > > > #define __LINUX_DFL_H
> > > > +#include <linux/bitfield.h>
> > > > #include <linux/device.h>
> > > > +#include <linux/io.h>
> > > > #include <linux/mod_devicetable.h>
> > > > /**
> > > > @@ -83,4 +85,54 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> > > > module_driver(__dfl_driver, dfl_driver_register, \
> > > > dfl_driver_unregister)
> > > > +/*
> > > > + * 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)
> > > > +#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
> > > > +
> > > > +/* Function to read from DFH and check if the Feature type is FME */
> > > > +static inline bool dfl_feature_is_fme(void __iomem *base)
> > > > +{
> > > > + u64 v = readq(base + DFH);
> > > > +
> > > > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_FME);
> > > > +}
> > > > +
> > > > +/* Function to read from DFH and check if the Feature type is port*/
> > > > +static inline bool dfl_feature_is_port(void __iomem *base)
> > > > +{
> > > > + u64 v = readq(base + DFH);
> > > > +
> > > > + return (FIELD_GET(DFH_TYPE, v) == DFH_TYPE_FIU) &&
> > > > + (FIELD_GET(DFH_ID, v) == DFH_ID_FIU_PORT);
> > > > +}
> > > > +
> > > > +/* Function to read feature revision from DFH */
> > > > +static inline u8 dfl_feature_revision(void __iomem *base)
> > > > +{
> > > > + return (u8)FIELD_GET(DFH_REVISION, readq(base + DFH));
> > > > +}
> > > > +
> > > > #endif /* __LINUX_DFL_H */
On 21/06/2021 15.56, Tom Rix wrote:
>
> On 6/21/21 12:06 AM, Martin Hundebøll wrote:
>> From: Debarati Biswas <[email protected]>
>>
>> Device Feature List (DFL) drivers may be defined in subdirectories other
>> than drivers/fpga, and each DFL driver should have access to the Device
>> Feature Header (DFH) register, which contains revision and type
>> information. This change moves the macros specific to the DFH register
>> from drivers/fpga/dfl.h to include/linux/dfl.h.
>>
>> Signed-off-by: Debarati Biswas <[email protected]>
>> Signed-off-by: Russ Weight <[email protected]>
>> Signed-off-by: Martin Hundebøll <[email protected]>
>> ---
>> drivers/fpga/dfl.h | 48 +----------------------------------------
>> include/linux/dfl.h | 52 +++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 53 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index 2b82c96ba56c..6ed0353e9a99 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -17,6 +17,7 @@
>> #include <linux/bitfield.h>
>
> bitfield.h was added to linux/dfl.h
>
> Likely both aren't needed, try removing this one.
After this patch both headers use GENMASK_ULL() and BIT_ULL(), which are both defined in linux/bits.h, so I would expect both to include that instead. A lot of the users of dfl.h uses FIELD_GET(), which is defined in linux/bitfield.h, so I would expect that to be included there instead.
I can prepare a patch to fix that up, if you like?
// Martin
On 22/06/2021 09.39, Wu, Hao wrote:
>> On Mon, Jun 21, 2021 at 06:19:15PM +0800, Wu, Hao wrote:
>>>> Subject: [PATCH 2/4] fpga: dfl: Move DFH header register macros to
>> linux/dfl.h
>>>>
>>>> From: Debarati Biswas <[email protected]>
>>>>
>>>> Device Feature List (DFL) drivers may be defined in subdirectories other
>>>> than drivers/fpga, and each DFL driver should have access to the Device
>>>> Feature Header (DFH) register, which contains revision and type
>>>> information. This change moves the macros specific to the DFH register
>>>> from drivers/fpga/dfl.h to include/linux/dfl.h.
>>>
>>> Looks like it requires to access the revision info in the next patch, because
>>> current dfl_device doesn't expose related information.
>>>
>>> @Yilun, do you have any concern to expose those info via dfl_device?
>>
>> Exposing these header register definitions are good to me. These registers
>> are in DFL device's MMIO region, so it is good to share these info with
>> all DFL drivers.
>
> I mean expose revision via dfl_device, as dfl core already reads the DFL
> header, it sounds duplicate read in each dfl device driver. And if we
> consider this as a common need from dfl device driver, then the code
> can be moved to a common place as well.
>
> I hope from dfl device driver side, it doesn't need to know details of
> how DFH register is defined, only simple way from dfl device data
> structure or some simple helper function, then dfl device driver could
> know all common information from DFH.
>
> How do you think?
struct dfl_device {} already has "u16 type" and "u16 feature_id", so it would make sense to add "u8 feature_rev" as well?
// Martin
On 6/23/21 4:44 AM, Martin Hundebøll wrote:
> On 21/06/2021 15.56, Tom Rix wrote:
>>
>> On 6/21/21 12:06 AM, Martin Hundebøll wrote:
>>> From: Debarati Biswas <[email protected]>
>>>
>>> Device Feature List (DFL) drivers may be defined in subdirectories
>>> other
>>> than drivers/fpga, and each DFL driver should have access to the Device
>>> Feature Header (DFH) register, which contains revision and type
>>> information. This change moves the macros specific to the DFH register
>>> from drivers/fpga/dfl.h to include/linux/dfl.h.
>>>
>>> Signed-off-by: Debarati Biswas <[email protected]>
>>> Signed-off-by: Russ Weight <[email protected]>
>>> Signed-off-by: Martin Hundebøll <[email protected]>
>>> ---
>>> drivers/fpga/dfl.h | 48 +----------------------------------------
>>> include/linux/dfl.h | 52
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 53 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>>> index 2b82c96ba56c..6ed0353e9a99 100644
>>> --- a/drivers/fpga/dfl.h
>>> +++ b/drivers/fpga/dfl.h
>>> @@ -17,6 +17,7 @@
>>> #include <linux/bitfield.h>
>>
>> bitfield.h was added to linux/dfl.h
>>
>> Likely both aren't needed, try removing this one.
>
> After this patch both headers use GENMASK_ULL() and BIT_ULL(), which
> are both defined in linux/bits.h, so I would expect both to include
> that instead. A lot of the users of dfl.h uses FIELD_GET(), which is
> defined in linux/bitfield.h, so I would expect that to be included
> there instead.
>
> I can prepare a patch to fix that up, if you like?
A rule of thumb, if your commit says 'A and B' , likely it should only be A.
Remove the the linux/bitfield.h from drivers/fpga/dfl.h
Leave the linux/bitfield.h as-is in include/linux/dfl.h
If you feel include/linux/dfl.h can be optimized by the reduction of
linux/bitfield.h to linux/bits.h, do that as a follow on patchset.
Tom
>
> // Martin
>
On Wed, Jun 23, 2021 at 01:56:59PM +0200, Martin Hundeb?ll wrote:
>
>
> On 22/06/2021 09.39, Wu, Hao wrote:
> > > On Mon, Jun 21, 2021 at 06:19:15PM +0800, Wu, Hao wrote:
> > > > > Subject: [PATCH 2/4] fpga: dfl: Move DFH header register macros to
> > > linux/dfl.h
> > > > >
> > > > > From: Debarati Biswas <[email protected]>
> > > > >
> > > > > Device Feature List (DFL) drivers may be defined in subdirectories other
> > > > > than drivers/fpga, and each DFL driver should have access to the Device
> > > > > Feature Header (DFH) register, which contains revision and type
> > > > > information. This change moves the macros specific to the DFH register
> > > > > from drivers/fpga/dfl.h to include/linux/dfl.h.
> > > >
> > > > Looks like it requires to access the revision info in the next patch, because
> > > > current dfl_device doesn't expose related information.
> > > >
> > > > @Yilun, do you have any concern to expose those info via dfl_device?
> > >
> > > Exposing these header register definitions are good to me. These registers
> > > are in DFL device's MMIO region, so it is good to share these info with
> > > all DFL drivers.
> >
> > I mean expose revision via dfl_device, as dfl core already reads the DFL
> > header, it sounds duplicate read in each dfl device driver. And if we
> > consider this as a common need from dfl device driver, then the code
> > can be moved to a common place as well.
> >
> > I hope from dfl device driver side, it doesn't need to know details of
> > how DFH register is defined, only simple way from dfl device data
> > structure or some simple helper function, then dfl device driver could
> > know all common information from DFH.
> >
> > How do you think?
It's good idea.
>
> struct dfl_device {} already has "u16 type" and "u16 feature_id", so it would make sense to add "u8 feature_rev" as well?
I think we may name it "u8 revision".
Thanks,
Yilun
>
> // Martin
> On Wed, Jun 23, 2021 at 01:56:59PM +0200, Martin Hundeb?ll wrote:
> >
> >
> > On 22/06/2021 09.39, Wu, Hao wrote:
> > > > On Mon, Jun 21, 2021 at 06:19:15PM +0800, Wu, Hao wrote:
> > > > > > Subject: [PATCH 2/4] fpga: dfl: Move DFH header register macros to
> > > > linux/dfl.h
> > > > > >
> > > > > > From: Debarati Biswas <[email protected]>
> > > > > >
> > > > > > Device Feature List (DFL) drivers may be defined in subdirectories other
> > > > > > than drivers/fpga, and each DFL driver should have access to the Device
> > > > > > Feature Header (DFH) register, which contains revision and type
> > > > > > information. This change moves the macros specific to the DFH register
> > > > > > from drivers/fpga/dfl.h to include/linux/dfl.h.
> > > > >
> > > > > Looks like it requires to access the revision info in the next patch,
> because
> > > > > current dfl_device doesn't expose related information.
> > > > >
> > > > > @Yilun, do you have any concern to expose those info via dfl_device?
> > > >
> > > > Exposing these header register definitions are good to me. These registers
> > > > are in DFL device's MMIO region, so it is good to share these info with
> > > > all DFL drivers.
> > >
> > > I mean expose revision via dfl_device, as dfl core already reads the DFL
> > > header, it sounds duplicate read in each dfl device driver. And if we
> > > consider this as a common need from dfl device driver, then the code
> > > can be moved to a common place as well.
> > >
> > > I hope from dfl device driver side, it doesn't need to know details of
> > > how DFH register is defined, only simple way from dfl device data
> > > structure or some simple helper function, then dfl device driver could
> > > know all common information from DFH.
> > >
> > > How do you think?
>
> It's good idea.
>
> >
> > struct dfl_device {} already has "u16 type" and "u16 feature_id", so it would
> make sense to add "u8 feature_rev" as well?
>
> I think we may name it "u8 revision".
Sounds good.
Then we don't have to expose detailed registers information to such header file.
dfl_device data structure + helper function should be enough. : )
Thanks
Hao
>
> Thanks,
> Yilun
>
> >
> > // Martin