* [PATCH v7 1/4] Documentation: fpga: dfl: Add documentation for DFHv1
2022-12-20 16:36 [PATCH v7 0/4] Enhance definition of DFH and use enhancements for UART driver matthew.gerlach
@ 2022-12-20 16:36 ` matthew.gerlach
2022-12-20 16:53 ` Andy Shevchenko
2022-12-20 16:36 ` [PATCH v7 2/4] fpga: dfl: Add DFHv1 Register Definitions matthew.gerlach
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: matthew.gerlach @ 2022-12-20 16:36 UTC (permalink / raw)
To: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
andriy.shevchenko, niklas.soderlund+renesas, macro, johan, lukas,
ilpo.jarvinen, marpagan, bagasdotme
Cc: 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v7: shorten long lines and wording suggestions by bagasdotme@gmail.com
v6: no change
v5: use nested list for field descriptions
clean up prose
add reviewed-by and comments from Ilpo Järvinen
v4: Remove marketing speak and separate v0 and v1 descriptions.
Fix errors reported by "make htmldocs".
v3: no change
v2: s/GUILD/GUID/
add picture
---
Documentation/fpga/dfl.rst | 112 +++++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 15b670926084..42fe7ff2b86a 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -561,6 +561,118 @@ new DFL feature via UIO direct access, its feature id should be added to the
driver's id_table.
+Device Feature Header - Version 0
+===========================================
+Version 0 (DFHv0) is the original version of the Device Feature Header.
+The format of DFHv0 is shown below::
+
+ +-----------------------------------------------------------------------+
+ |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+ +-----------------------------------------------------------------------+
+ |63 GUID_L 0| 0x08
+ +-----------------------------------------------------------------------+
+ |63 GUID_H 0| 0x10
+ +-----------------------------------------------------------------------+
+
+- Offset 0x00
+
+ * Type - The type of DFH (e.g. FME, AFU, or private feature).
+ * DFH VER - The version of the DFH.
+ * Rsvd - Currently unused.
+ * EOL - Set if the DFH is the end of the Device Feature List (DFL).
+ * Next - The offset of the next DFH in the DFL from the DFH start.
+ If EOL is set, Next is the size of MMIO of the last feature in the list.
+ * ID - The feature ID if Type is private feature.
+
+- Offset 0x08
+
+ * GUID_L - Least significant 64 bits of a 128-bit Globally Unique Identifier
+ (present only if Type is FME or AFU).
+
+- Offset 0x10
+
+ * GUID_H - Most significant 64 bits of a 128-bit Globally Unique Identifier
+ (present only if Type is FME or AFU).
+
+
+Device Feature Header - Version 1
+===========================================
+Version 1 (DFHv1) of the Device Feature Header adds the following functionality:
+
+* Provides a standardized mechanism for features to describe
+ parameters/capabilities to software.
+* Standardize the use of a GUID for all DFHv1 types.
+* Decouples the DFH location from the register space of the feature itself.
+
+The format of Version 1 of the Device Feature Header (DFH) is shown below::
+
+ +-----------------------------------------------------------------------+
+ |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+ +-----------------------------------------------------------------------+
+ |63 GUID_L 0| 0x08
+ +-----------------------------------------------------------------------+
+ |63 GUID_H 0| 0x10
+ +-----------------------------------------------------------------------+
+ |63 Reg Address/Offset 1| Rel 0| 0x18
+ +-----------------------------------------------------------------------+
+ |63 Reg Size 32|Params 31|30 Group 16|15 Instance 0| 0x20
+ +-----------------------------------------------------------------------+
+ |63 Next 35|34RSV33|EOP32|31 Param Version 16|15 Param ID 0| 0x28
+ +-----------------------------------------------------------------------+
+ |63 Parameter Data 0| 0x30
+ +-----------------------------------------------------------------------+
+
+ ...
+
+ +-----------------------------------------------------------------------+
+ |63 Next 35|34RSV33|EOP32|31 Param Version 16|15 Param ID 0|
+ +-----------------------------------------------------------------------+
+ |63 Parameter Data 0|
+ +-----------------------------------------------------------------------+
+
+- Offset 0x00
+
+ * Type - The type of DFH (e.g. FME, AFU, or private feature).
+ * DFH VER - The version of the DFH.
+ * Rsvd - Currently unused.
+ * EOL - Set if the DFH is the end of the Device Feature List (DFL).
+ * Next - The offset of the next DFH in the DFL from the DFH start.
+ If EOL is set, Next is the size of MMIO of the last feature in the list.
+ * ID - The feature ID if Type is private feature.
+
+- Offset 0x08
+
+ * GUID_L - Least significant 64 bits of a 128-bit Globally Unique Identifier.
+
+- Offset 0x10
+
+ * GUID_H - Most significant 64 bits of a 128-bit Globally Unique Identifier.
+
+- Offset 0x18
+
+ * Reg Address/Offset - If Rel bit is set, then the value is the high 63 bits
+ of a 16-bit aligned absolute address of the feature's registers. Otherwise
+ the value is the offset from the start of the DFH of the feature's registers.
+
+- Offset 0x20
+
+ * Reg Size - Size of feature's register set in bytes.
+ * Params - Set if DFH has a list of parameter blocks.
+ * Group - Id of group if feature is part of a group.
+ * Instance - Id of feature instance within a group.
+
+- Offset 0x28 if feature has parameters
+
+ * Next - Offset to the next parameter block in 8 byte words. If EOP set,
+ size in 8 byte words of last parameter.
+ * Param Version - Version of Param ID.
+ * Param ID - ID of parameter.
+
+- Offset 0x30
+
+ * Parameter Data - Parameter data whose size and format is defined by
+ version and ID of the parameter.
+
Open discussion
===============
FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/4] Documentation: fpga: dfl: Add documentation for DFHv1
2022-12-20 16:36 ` [PATCH v7 1/4] Documentation: fpga: dfl: Add documentation for DFHv1 matthew.gerlach
@ 2022-12-20 16:53 ` Andy Shevchenko
2022-12-21 16:52 ` matthew.gerlach
0 siblings, 1 reply; 22+ messages in thread
From: Andy Shevchenko @ 2022-12-20 16:53 UTC (permalink / raw)
To: matthew.gerlach
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
marpagan, bagasdotme
On Tue, Dec 20, 2022 at 08:36:49AM -0800, 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).
...
> +Device Feature Header - Version 0
> +===========================================
Shouldn't this be the same length as the title itself?
Have you run make htmldocs and kernel doc validator on
this file after your change? Also you can use rst2pdf
to see PDF rendering.
Same to the other title.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 1/4] Documentation: fpga: dfl: Add documentation for DFHv1
2022-12-20 16:53 ` Andy Shevchenko
@ 2022-12-21 16:52 ` matthew.gerlach
0 siblings, 0 replies; 22+ messages in thread
From: matthew.gerlach @ 2022-12-21 16:52 UTC (permalink / raw)
To: Andy Shevchenko
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
marpagan, bagasdotme
On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> On Tue, Dec 20, 2022 at 08:36:49AM -0800, 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).
>
> ...
>
>> +Device Feature Header - Version 0
>> +===========================================
>
> Shouldn't this be the same length as the title itself?
> Have you run make htmldocs and kernel doc validator on
> this file after your change? Also you can use rst2pdf
> to see PDF rendering.
Yes, it should be the same as the title itself. I will change it
accordingly. I ran "make htmldocs" and rst2pdf, and resulting html and
pdf looked fine.
Thanks for the review,
Matthew Gerlach
>
> Same to the other title.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 2/4] fpga: dfl: Add DFHv1 Register Definitions
2022-12-20 16:36 [PATCH v7 0/4] Enhance definition of DFH and use enhancements for UART driver matthew.gerlach
2022-12-20 16:36 ` [PATCH v7 1/4] Documentation: fpga: dfl: Add documentation for DFHv1 matthew.gerlach
@ 2022-12-20 16:36 ` matthew.gerlach
2022-12-20 16:54 ` Andy Shevchenko
2022-12-20 16:36 ` [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1 matthew.gerlach
2022-12-20 16:36 ` [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550 matthew.gerlach
3 siblings, 1 reply; 22+ messages in thread
From: matthew.gerlach @ 2022-12-20 16:36 UTC (permalink / raw)
To: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
andriy.shevchenko, niklas.soderlund+renesas, macro, johan, lukas,
ilpo.jarvinen, marpagan, bagasdotme
Cc: Basheer Ahmed Muddebihal, 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>
Co-developed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v7: no change
v6: remove parameter definitions from include/linux/dfl.h
v5: consistently use fields for parameter data
s/EOL/EOP/ to match doc
remove unneeded mask
added Co-developed-by
v4: s/MSIX/MSI_X/g
move kerneldoc to implementation
don't change copyright date
v3:
keep DFHv1 definitions "hidden" in drivers/fpga/dfl.h
v2: clean up white space and one line comments
remove extra space in commit
use uniform number of digits in constants
don't change copyright date because of removed content
---
drivers/fpga/dfl.h | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..fc59f33367ee 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -74,11 +74,43 @@
#define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
#define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
#define DFH_EOL BIT_ULL(40) /* End of list */
+#define DFH_VERSION GENMASK_ULL(59, 52) /* DFH version */
#define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
#define DFH_TYPE_AFU 1
#define DFH_TYPE_PRIVATE 3
#define DFH_TYPE_FIU 4
+/*
+ * DFHv1 Register Offset definitons
+ * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
+ * as common header registers
+ */
+#define DFHv1_CSR_ADDR 0x18 /* CSR Register start address */
+#define DFHv1_CSR_SIZE_GRP 0x20 /* Size of Reg Block and Group/tag */
+#define DFHv1_PARAM_HDR 0x28 /* Optional First Param header */
+
+/*
+ * CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),
+ * 1'b1 = absolute (ARM or other non-PCIe use)
+ */
+#define DFHv1_CSR_ADDR_REL BIT_ULL(0)
+
+/* CSR Header Register Bit Definitions */
+#define DFHv1_CSR_ADDR_MASK GENMASK_ULL(63, 1) /* 63:1 of CSR address */
+
+/* CSR SIZE Goup Register Bit Definitions */
+#define DFHv1_CSR_SIZE_GRP_INSTANCE_ID GENMASK_ULL(15, 0) /* Enumeration instantiated IP */
+#define DFHv1_CSR_SIZE_GRP_GROUPING_ID GENMASK_ULL(30, 16) /* Group Features/interfaces */
+#define DFHv1_CSR_SIZE_GRP_HAS_PARAMS BIT_ULL(31) /* Presence of Parameters */
+#define DFHv1_CSR_SIZE_GRP_SIZE GENMASK_ULL(63, 32) /* Size of CSR Block in bytes */
+
+/* PARAM Header Register Bit Definitions */
+#define DFHv1_PARAM_HDR_ID GENMASK_ULL(15, 0) /* Id of this Param */
+#define DFHv1_PARAM_HDR_VER GENMASK_ULL(31, 16) /* Version Param */
+#define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 35) /* Offset of next Param */
+#define DFHv1_PARAM_HDR_NEXT_EOP BIT_ULL(32)
+#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
+
/* Next AFU Register Bitfield */
#define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v7 2/4] fpga: dfl: Add DFHv1 Register Definitions
2022-12-20 16:36 ` [PATCH v7 2/4] fpga: dfl: Add DFHv1 Register Definitions matthew.gerlach
@ 2022-12-20 16:54 ` Andy Shevchenko
0 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2022-12-20 16:54 UTC (permalink / raw)
To: matthew.gerlach
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
marpagan, bagasdotme, Basheer Ahmed Muddebihal
On Tue, Dec 20, 2022 at 08:36:50AM -0800, matthew.gerlach@linux.intel.com wrote:
> From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
>
> This patch adds the definitions for DFHv1 header and related register
> bitfields.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
> Co-developed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v7: no change
>
> v6: remove parameter definitions from include/linux/dfl.h
>
> v5: consistently use fields for parameter data
> s/EOL/EOP/ to match doc
> remove unneeded mask
> added Co-developed-by
>
> v4: s/MSIX/MSI_X/g
> move kerneldoc to implementation
> don't change copyright date
>
> v3:
> keep DFHv1 definitions "hidden" in drivers/fpga/dfl.h
>
> v2: clean up white space and one line comments
> remove extra space in commit
> use uniform number of digits in constants
> don't change copyright date because of removed content
> ---
> drivers/fpga/dfl.h | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 06cfcd5e84bb..fc59f33367ee 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -74,11 +74,43 @@
> #define DFH_REVISION GENMASK_ULL(15, 12) /* Feature revision */
> #define DFH_NEXT_HDR_OFST GENMASK_ULL(39, 16) /* Offset to next DFH */
> #define DFH_EOL BIT_ULL(40) /* End of list */
> +#define DFH_VERSION GENMASK_ULL(59, 52) /* DFH version */
> #define DFH_TYPE GENMASK_ULL(63, 60) /* Feature type */
> #define DFH_TYPE_AFU 1
> #define DFH_TYPE_PRIVATE 3
> #define DFH_TYPE_FIU 4
>
> +/*
> + * DFHv1 Register Offset definitons
> + * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
> + * as common header registers
> + */
> +#define DFHv1_CSR_ADDR 0x18 /* CSR Register start address */
> +#define DFHv1_CSR_SIZE_GRP 0x20 /* Size of Reg Block and Group/tag */
> +#define DFHv1_PARAM_HDR 0x28 /* Optional First Param header */
> +
> +/*
> + * CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),
> + * 1'b1 = absolute (ARM or other non-PCIe use)
> + */
> +#define DFHv1_CSR_ADDR_REL BIT_ULL(0)
> +
> +/* CSR Header Register Bit Definitions */
> +#define DFHv1_CSR_ADDR_MASK GENMASK_ULL(63, 1) /* 63:1 of CSR address */
> +
> +/* CSR SIZE Goup Register Bit Definitions */
> +#define DFHv1_CSR_SIZE_GRP_INSTANCE_ID GENMASK_ULL(15, 0) /* Enumeration instantiated IP */
> +#define DFHv1_CSR_SIZE_GRP_GROUPING_ID GENMASK_ULL(30, 16) /* Group Features/interfaces */
> +#define DFHv1_CSR_SIZE_GRP_HAS_PARAMS BIT_ULL(31) /* Presence of Parameters */
> +#define DFHv1_CSR_SIZE_GRP_SIZE GENMASK_ULL(63, 32) /* Size of CSR Block in bytes */
> +
> +/* PARAM Header Register Bit Definitions */
> +#define DFHv1_PARAM_HDR_ID GENMASK_ULL(15, 0) /* Id of this Param */
> +#define DFHv1_PARAM_HDR_VER GENMASK_ULL(31, 16) /* Version Param */
> +#define DFHv1_PARAM_HDR_NEXT_OFFSET GENMASK_ULL(63, 35) /* Offset of next Param */
> +#define DFHv1_PARAM_HDR_NEXT_EOP BIT_ULL(32)
> +#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
> +
> /* Next AFU Register Bitfield */
> #define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
>
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-20 16:36 [PATCH v7 0/4] Enhance definition of DFH and use enhancements for UART driver matthew.gerlach
2022-12-20 16:36 ` [PATCH v7 1/4] Documentation: fpga: dfl: Add documentation for DFHv1 matthew.gerlach
2022-12-20 16:36 ` [PATCH v7 2/4] fpga: dfl: Add DFHv1 Register Definitions matthew.gerlach
@ 2022-12-20 16:36 ` matthew.gerlach
2022-12-20 17:03 ` Andy Shevchenko
2022-12-21 11:58 ` Ilpo Järvinen
2022-12-20 16:36 ` [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550 matthew.gerlach
3 siblings, 2 replies; 22+ messages in thread
From: matthew.gerlach @ 2022-12-20 16:36 UTC (permalink / raw)
To: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
andriy.shevchenko, niklas.soderlund+renesas, macro, johan, lukas,
ilpo.jarvinen, marpagan, bagasdotme
Cc: Matthew Gerlach
From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Version 1 of the Device Feature Header (DFH) definition adds
functionality to the DFL bus.
A DFHv1 header may have one or more parameter blocks that
further describes the HW to SW. Add support to the DFL bus
to parse the MSI-X parameter.
The location of a feature's register set 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v7: no change
v6: move MSI_X parameter definitions to drivers/fpga/dfl.h
v5: update field names
fix find_param/dfh_get_psize
clean up mmio_res assignments
use u64* instead of void*
use FIELD_GET instead of masking
v4: s/MSIX/MSI_X
move kernel doc to implementation
use structure assignment
fix decode of absolute address
clean up comment in parse_feature_irqs
remove use of csr_res
v3: remove unneeded blank line
use clearer variable name
pass finfo into parse_feature_irqs()
refactor code for better indentation
use switch statement for irq parsing
squash in code parsing register location
v2: fix kernel doc
clarify use of DFH_VERSION field
---
drivers/fpga/dfl.c | 234 ++++++++++++++++++++++++++++++++++----------
drivers/fpga/dfl.h | 9 ++
include/linux/dfl.h | 4 +
3 files changed, 196 insertions(+), 51 deletions(-)
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..df63c96bcbfc 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -342,6 +342,8 @@ static void release_dfl_dev(struct device *dev)
if (ddev->mmio_res.parent)
release_resource(&ddev->mmio_res);
+ kfree(ddev->params);
+
ida_free(&dfl_device_ida, ddev->id);
kfree(ddev->irqs);
kfree(ddev);
@@ -380,7 +382,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
ddev->type = feature_dev_id_type(pdev);
ddev->feature_id = feature->id;
ddev->revision = feature->revision;
+ ddev->dfh_version = feature->dfh_version;
ddev->cdev = pdata->dfl_cdev;
+ if (feature->param_size) {
+ ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
+ if (!ddev->params) {
+ ret = -ENOMEM;
+ goto put_dev;
+ }
+ ddev->param_size = feature->param_size;
+ }
/* add mmio resource */
parent_res = &pdev->resource[feature->resource_index];
@@ -708,20 +719,27 @@ struct build_feature_devs_info {
* struct dfl_feature_info - sub feature info collected during feature dev build
*
* @fid: id of this sub feature.
+ * @revision: revision of this sub feature
+ * @dfh_version: version of Device Feature Header (DFH)
* @mmio_res: mmio resource of this sub feature.
* @ioaddr: mapped base address of mmio resource.
* @node: node in sub_features linked list.
* @irq_base: start of irq index in this sub feature.
* @nr_irqs: number of irqs of this sub feature.
+ * @param_size: size DFH parameters.
+ * @params: DFH parameter data.
*/
struct dfl_feature_info {
u16 fid;
u8 revision;
+ u8 dfh_version;
struct resource mmio_res;
void __iomem *ioaddr;
struct list_head node;
unsigned int irq_base;
unsigned int nr_irqs;
+ unsigned int param_size;
+ u64 params[];
};
static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
@@ -797,7 +815,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
feature->dev = fdev;
feature->id = finfo->fid;
feature->revision = finfo->revision;
+ feature->dfh_version = finfo->dfh_version;
+ if (finfo->param_size) {
+ feature->params = devm_kmemdup(binfo->dev,
+ finfo->params, finfo->param_size,
+ GFP_KERNEL);
+ if (!feature->params)
+ return -ENOMEM;
+
+ feature->param_size = finfo->param_size;
+ }
/*
* the FIU header feature has some fundamental functions (sriov
* set, port enable/disable) needed for the dfl bus device and
@@ -934,56 +962,105 @@ static u16 feature_id(u64 value)
return 0;
}
+static u64 *find_param(u64 *params, resource_size_t max, int param_id)
+{
+ u64 *end = params + max / sizeof(u64);
+ u64 v, next;
+
+ while (params < end) {
+ v = *params;
+ if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+ return params;
+
+ next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+ params += next;
+ if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
+ break;
+ }
+
+ return NULL;
+}
+
+/**
+ * dfh_find_param() - find data for the given parameter id
+ * @dfl_dev: dfl device
+ * @param: id of dfl parameter
+ *
+ * Return: pointer to parameter header on success, NULL otherwise.
+ */
+u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
+{
+ return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
+}
+EXPORT_SYMBOL_GPL(dfh_find_param);
+
static int parse_feature_irqs(struct build_feature_devs_info *binfo,
- resource_size_t ofst, u16 fid,
- unsigned int *irq_base, unsigned int *nr_irqs)
+ resource_size_t ofst, struct dfl_feature_info *finfo)
{
void __iomem *base = binfo->ioaddr + ofst;
unsigned int i, ibase, inr = 0;
+ void *params = finfo->params;
enum dfl_id_type type;
+ u16 fid = finfo->fid;
int virq;
+ u64 *p;
u64 v;
- type = feature_dev_id_type(binfo->feature_dev);
+ switch (finfo->dfh_version) {
+ case 0:
+ /*
+ * DFHv0 only provides MMIO resource information for each feature
+ * in the DFL header. There is no generic interrupt information.
+ * Instead, features with interrupt functionality provide
+ * the information in feature specific registers.
+ */
+ type = feature_dev_id_type(binfo->feature_dev);
+ if (type == PORT_ID) {
+ switch (fid) {
+ case PORT_FEATURE_ID_UINT:
+ v = readq(base + PORT_UINT_CAP);
+ ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
+ inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+ break;
+ case PORT_FEATURE_ID_ERROR:
+ v = readq(base + PORT_ERROR_CAP);
+ ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
+ inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+ break;
+ }
+ } else if (type == FME_ID) {
+ switch (fid) {
+ case FME_FEATURE_ID_GLOBAL_ERR:
+ v = readq(base + FME_ERROR_CAP);
+ ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
+ inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
+ break;
+ }
+ }
+ break;
- /*
- * Ideally DFL framework should only read info from DFL header, but
- * current version DFL only provides mmio resources information for
- * each feature in DFL Header, no field for interrupt resources.
- * Interrupt resource information is provided by specific mmio
- * registers of each private feature which supports interrupt. So in
- * order to parse and assign irq resources, DFL framework has to look
- * into specific capability registers of these private features.
- *
- * Once future DFL version supports generic interrupt resource
- * information in common DFL headers, the generic interrupt parsing
- * code will be added. But in order to be compatible to old version
- * DFL, the driver may still fall back to these quirks.
- */
- if (type == PORT_ID) {
- switch (fid) {
- case PORT_FEATURE_ID_UINT:
- v = readq(base + PORT_UINT_CAP);
- ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
- inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
- break;
- case PORT_FEATURE_ID_ERROR:
- v = readq(base + PORT_ERROR_CAP);
- ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
- inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+ case 1:
+ /*
+ * DFHv1 provides interrupt resource information in DFHv1
+ * parameter blocks.
+ */
+ p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
+ if (!p)
break;
- }
- } else if (type == FME_ID) {
- if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
- v = readq(base + FME_ERROR_CAP);
- ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
- inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
- }
+
+ p++;
+ ibase = FIELD_GET(DFHv1_PARAM_MSI_X_STARTV, *p);
+ inr = FIELD_GET(DFHv1_PARAM_MSI_X_NUMV, *p);
+ break;
+
+ default:
+ dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
+ break;
}
if (!inr) {
- *irq_base = 0;
- *nr_irqs = 0;
+ finfo->irq_base = 0;
+ finfo->nr_irqs = 0;
return 0;
}
@@ -1006,12 +1083,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
}
}
- *irq_base = ibase;
- *nr_irqs = inr;
+ finfo->irq_base = ibase;
+ finfo->nr_irqs = inr;
return 0;
}
+static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
+{
+ int size = 0;
+ u64 v, next;
+
+ if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
+ readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
+ return 0;
+
+ while (size + DFHv1_PARAM_HDR < max) {
+ v = readq(dfh_base + DFHv1_PARAM_HDR + size);
+
+ next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+ if (!next)
+ return -EINVAL;
+
+ size += next * sizeof(u64);
+
+ if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
+ return size;
+ }
+
+ return -ENOENT;
+}
+
/*
* when create sub feature instances, for private features, it doesn't need
* to provide resource size and feature id as they could be read from DFH
@@ -1023,39 +1125,69 @@ static int
create_feature_instance(struct build_feature_devs_info *binfo,
resource_size_t ofst, resource_size_t size, u16 fid)
{
- unsigned int irq_base, nr_irqs;
struct dfl_feature_info *finfo;
+ resource_size_t start, end;
+ int dfh_psize = 0;
u8 revision = 0;
+ u64 v, addr_off;
+ u8 dfh_ver = 0;
int ret;
- u64 v;
if (fid != FEATURE_ID_AFU) {
v = readq(binfo->ioaddr + ofst);
revision = FIELD_GET(DFH_REVISION, v);
-
+ dfh_ver = FIELD_GET(DFH_VERSION, v);
/* read feature size and id if inputs are invalid */
size = size ? size : feature_size(v);
fid = fid ? fid : feature_id(v);
+ if (dfh_ver == 1) {
+ dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
+ if (dfh_psize < 0) {
+ dev_err(binfo->dev,
+ "failed to read size of DFHv1 parameters %d\n",
+ dfh_psize);
+ return dfh_psize;
+ }
+ dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
+ }
}
if (binfo->len - ofst < size)
return -EINVAL;
- ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
- if (ret)
- return ret;
-
- finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
+ finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
if (!finfo)
return -ENOMEM;
+ memcpy_fromio(finfo->params, binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
+ finfo->param_size = dfh_psize;
+
finfo->fid = fid;
finfo->revision = revision;
- finfo->mmio_res.start = binfo->start + ofst;
- finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+ finfo->dfh_version = dfh_ver;
+ if (dfh_ver == 1) {
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+ addr_off = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+ if (FIELD_GET(DFHv1_CSR_ADDR_REL, v))
+ start = addr_off << 1;
+ else
+ start = binfo->start + ofst + addr_off;
+
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+ end = start + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
+ } else {
+ start = binfo->start + ofst;
+ end = start + size - 1;
+ }
finfo->mmio_res.flags = IORESOURCE_MEM;
- finfo->irq_base = irq_base;
- finfo->nr_irqs = nr_irqs;
+ finfo->mmio_res.start = start;
+ finfo->mmio_res.end = end;
+
+ ret = parse_feature_irqs(binfo, ofst, finfo);
+ if (ret) {
+ kfree(finfo);
+ return ret;
+ }
list_add_tail(&finfo->node, &binfo->sub_features);
binfo->feature_num++;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index fc59f33367ee..a21065cec04e 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -111,6 +111,10 @@
#define DFHv1_PARAM_HDR_NEXT_EOP BIT_ULL(32)
#define DFHv1_PARAM_DATA 0x08 /* Offset of Param data from Param header */
+#define DFHv1_PARAM_ID_MSI_X 0x1
+#define DFHv1_PARAM_MSI_X_NUMV GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_MSI_X_STARTV GENMASK_ULL(31, 0)
+
/* Next AFU Register Bitfield */
#define NEXT_AFU_NEXT_DFH_OFST GENMASK_ULL(23, 0) /* Offset to next AFU */
@@ -272,11 +276,14 @@ struct dfl_feature_irq_ctx {
* @ops: ops of this sub feature.
* @ddev: ptr to the dfl device of this sub feature.
* @priv: priv data of this feature.
+ * @param_size: size of dfh parameters
+ * @params: point to memory copy of dfh parameters
*/
struct dfl_feature {
struct platform_device *dev;
u16 id;
u8 revision;
+ u8 dfh_version;
int resource_index;
void __iomem *ioaddr;
struct dfl_feature_irq_ctx *irq_ctx;
@@ -284,6 +291,8 @@ struct dfl_feature {
const struct dfl_feature_ops *ops;
struct dfl_device *ddev;
void *priv;
+ unsigned int param_size;
+ void *params;
};
#define FEATURE_DEV_ID_UNUSED (-1)
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 431636a0dc78..a9b7ae84e5fd 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -39,9 +39,12 @@ struct dfl_device {
u16 type;
u16 feature_id;
u8 revision;
+ u8 dfh_version;
struct resource mmio_res;
int *irqs;
unsigned int num_irqs;
+ unsigned int param_size;
+ void *params;
struct dfl_fpga_cdev *cdev;
const struct dfl_device_id *id_entry;
};
@@ -84,4 +87,5 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
module_driver(__dfl_driver, dfl_driver_register, \
dfl_driver_unregister)
+u64 *dfh_find_param(struct dfl_device *dfl_dev, int param);
#endif /* __LINUX_DFL_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-20 16:36 ` [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1 matthew.gerlach
@ 2022-12-20 17:03 ` Andy Shevchenko
2022-12-21 19:14 ` matthew.gerlach
2022-12-21 11:58 ` Ilpo Järvinen
1 sibling, 1 reply; 22+ messages in thread
From: Andy Shevchenko @ 2022-12-20 17:03 UTC (permalink / raw)
To: matthew.gerlach
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
marpagan, bagasdotme
On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Version 1 of the Device Feature Header (DFH) definition adds
> functionality to the DFL bus.
>
> A DFHv1 header may have one or more parameter blocks that
> further describes the HW to SW. Add support to the DFL bus
> to parse the MSI-X parameter.
>
> The location of a feature's register set 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.
...
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter header on success, NULL otherwise.
header is a bit confusing here, does it mean we give and ID and we got
something more than just a data as summary above suggests?
In such case summary and this text should clarify what exactly we get
and layout of the data. Since this is a pointer, who is responsible of
checking out-of-boundary accesses? For instance, if the parameters are
variadic by length the length should be returned as well. Otherwise it
should be specified as a constant somewhere, right?
> + */
> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> +{
> + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);
...
> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
It sounds like a candidate for struct_size() from overflow.h.
I.o.w. check that header and come up with the best what can
suit your case.
> if (!finfo)
> return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-20 17:03 ` Andy Shevchenko
@ 2022-12-21 19:14 ` matthew.gerlach
2022-12-26 3:15 ` Xu Yilun
0 siblings, 1 reply; 22+ messages in thread
From: matthew.gerlach @ 2022-12-21 19:14 UTC (permalink / raw)
To: Andy Shevchenko
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
marpagan, bagasdotme
On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Version 1 of the Device Feature Header (DFH) definition adds
>> functionality to the DFL bus.
>>
>> A DFHv1 header may have one or more parameter blocks that
>> further describes the HW to SW. Add support to the DFL bus
>> to parse the MSI-X parameter.
>>
>> The location of a feature's register set 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.
>
> ...
>
>> +/**
>> + * dfh_find_param() - find data for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to parameter header on success, NULL otherwise.
>
> header is a bit confusing here, does it mean we give and ID and we got
> something more than just a data as summary above suggests?
Yes, the summary is not correct. It should say "find the parameter block
for the given parameter id".
>
> In such case summary and this text should clarify what exactly we get
> and layout of the data. Since this is a pointer, who is responsible of
> checking out-of-boundary accesses? For instance, if the parameters are
> variadic by length the length should be returned as well. Otherwise it
> should be specified as a constant somewhere, right?
The parameter header has the next/size field; so the caller of
dfh_find_param should perform boundary checking as part of interpreting
the parameter data. I think a function to perform this checking
and data interpretation would help here.
>
>> + */
>> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
>> +{
>> + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>
> ...
>
>> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>
> It sounds like a candidate for struct_size() from overflow.h.
> I.o.w. check that header and come up with the best what can
> suit your case.
finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)),
GFP_KERNEL);
Does seem better.
Thanks for the suggestion,
Matthew Gerlach
>
>> if (!finfo)
>> return -ENOMEM;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-21 19:14 ` matthew.gerlach
@ 2022-12-26 3:15 ` Xu Yilun
2022-12-31 20:46 ` matthew.gerlach
0 siblings, 1 reply; 22+ messages in thread
From: Xu Yilun @ 2022-12-26 3:15 UTC (permalink / raw)
To: matthew.gerlach
Cc: Andy Shevchenko, hao.wu, russell.h.weight,
basheer.ahmed.muddebihal, trix, mdf, linux-fpga, linux-doc,
linux-kernel, tianfei.zhang, corbet, gregkh, linux-serial,
jirislaby, geert+renesas, niklas.soderlund+renesas, macro, johan,
lukas, ilpo.jarvinen, marpagan, bagasdotme
On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@linux.intel.com wrote:
>
>
> On Tue, 20 Dec 2022, Andy Shevchenko wrote:
>
> > On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > >
> > > Version 1 of the Device Feature Header (DFH) definition adds
> > > functionality to the DFL bus.
> > >
> > > A DFHv1 header may have one or more parameter blocks that
> > > further describes the HW to SW. Add support to the DFL bus
> > > to parse the MSI-X parameter.
> > >
> > > The location of a feature's register set 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.
> >
> > ...
> >
> > > +/**
> > > + * dfh_find_param() - find data for the given parameter id
> > > + * @dfl_dev: dfl device
> > > + * @param: id of dfl parameter
> > > + *
> > > + * Return: pointer to parameter header on success, NULL otherwise.
> >
> > header is a bit confusing here, does it mean we give and ID and we got
> > something more than just a data as summary above suggests?
>
> Yes, the summary is not correct. It should say "find the parameter block
> for the given parameter id".
>
> >
> > In such case summary and this text should clarify what exactly we get
> > and layout of the data. Since this is a pointer, who is responsible of
> > checking out-of-boundary accesses? For instance, if the parameters are
> > variadic by length the length should be returned as well. Otherwise it
> > should be specified as a constant somewhere, right?
>
> The parameter header has the next/size field; so the caller of
> dfh_find_param should perform boundary checking as part of interpreting the
> parameter data. I think a function to perform this checking and data
> interpretation would help here.
It is better the DFL core provides the size of the parameter block, just
in this API. It provides the pointer and should be ensured the memory
for the pointer be correctly understood.
>
> >
> > > + */
> > > +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> > > +{
> > > + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> > > +}
> > > +EXPORT_SYMBOL_GPL(dfh_find_param);
> >
> > ...
> >
> > > + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> >
> > It sounds like a candidate for struct_size() from overflow.h.
> > I.o.w. check that header and come up with the best what can
> > suit your case.
>
> finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)),
> GFP_KERNEL);
>
> Does seem better.
How about we change the dfh_get_psize() to like dfh_get_pcount(), so we
don't have to multiply & divide back and forth.
Or we just use size_add()?
Thanks,
Yilun
>
> Thanks for the suggestion,
> Matthew Gerlach
>
>
> >
> > > if (!finfo)
> > > return -ENOMEM;
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-26 3:15 ` Xu Yilun
@ 2022-12-31 20:46 ` matthew.gerlach
2023-01-03 4:22 ` Xu Yilun
0 siblings, 1 reply; 22+ messages in thread
From: matthew.gerlach @ 2022-12-31 20:46 UTC (permalink / raw)
To: Xu Yilun
Cc: Andy Shevchenko, hao.wu, russell.h.weight,
basheer.ahmed.muddebihal, trix, mdf, linux-fpga, linux-doc,
linux-kernel, tianfei.zhang, corbet, gregkh, linux-serial,
jirislaby, geert+renesas, niklas.soderlund+renesas, macro, johan,
lukas, ilpo.jarvinen, marpagan, bagasdotme
On Mon, 26 Dec 2022, Xu Yilun wrote:
> On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Tue, 20 Dec 2022, Andy Shevchenko wrote:
>>
>>> On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote:
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Version 1 of the Device Feature Header (DFH) definition adds
>>>> functionality to the DFL bus.
>>>>
>>>> A DFHv1 header may have one or more parameter blocks that
>>>> further describes the HW to SW. Add support to the DFL bus
>>>> to parse the MSI-X parameter.
>>>>
>>>> The location of a feature's register set 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.
>>>
>>> ...
>>>
>>>> +/**
>>>> + * dfh_find_param() - find data for the given parameter id
>>>> + * @dfl_dev: dfl device
>>>> + * @param: id of dfl parameter
>>>> + *
>>>> + * Return: pointer to parameter header on success, NULL otherwise.
>>>
>>> header is a bit confusing here, does it mean we give and ID and we got
>>> something more than just a data as summary above suggests?
>>
>> Yes, the summary is not correct. It should say "find the parameter block
>> for the given parameter id".
>>
>>>
>>> In such case summary and this text should clarify what exactly we get
>>> and layout of the data. Since this is a pointer, who is responsible of
>>> checking out-of-boundary accesses? For instance, if the parameters are
>>> variadic by length the length should be returned as well. Otherwise it
>>> should be specified as a constant somewhere, right?
>>
>> The parameter header has the next/size field; so the caller of
>> dfh_find_param should perform boundary checking as part of interpreting the
>> parameter data. I think a function to perform this checking and data
>> interpretation would help here.
>
> It is better the DFL core provides the size of the parameter block, just
> in this API. It provides the pointer and should be ensured the memory
> for the pointer be correctly understood.
Ok, how about the following API for dfh_find_param?
/**
* dfh_find_param() - find parameter block for the given parameter id
* @dfl_dev: dfl device
* @param_id: id of dfl parameter
* @pver: destination to store parameter version
* @pcount: destination to store size of parameter data in u64 bit words
*
* Return: pointer to start of parameter data, PTR_ERR otherwise.
*/
void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, unsigned
*pver, unsigned *pcount)
>
>>
>>>
>>>> + */
>>>> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
>>>> +{
>>>> + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>>>
>>> ...
>>>
>>>> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>>>
>>> It sounds like a candidate for struct_size() from overflow.h.
>>> I.o.w. check that header and come up with the best what can
>>> suit your case.
>>
>> finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)),
>> GFP_KERNEL);
>>
>> Does seem better.
>
> How about we change the dfh_get_psize() to like dfh_get_pcount(), so we
> don't have to multiply & divide back and forth.
We need the size in bytes for calls to kmemdup, devm_kmemdup, and
memcpy_fromio, but we only need to divide once here.
>
> Or we just use size_add()?
I think using struct_size is better because the params member
of struct dfl_feature_info is a trailing flexible array.
Thanks for the feedback,
Matthew
>
> Thanks,
> Yilun
>
>>
>> Thanks for the suggestion,
>> Matthew Gerlach
>>
>>
>>>
>>>> if (!finfo)
>>>> return -ENOMEM;
>>>
>>> --
>>> With Best Regards,
>>> Andy Shevchenko
>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-31 20:46 ` matthew.gerlach
@ 2023-01-03 4:22 ` Xu Yilun
2023-01-03 19:50 ` matthew.gerlach
0 siblings, 1 reply; 22+ messages in thread
From: Xu Yilun @ 2023-01-03 4:22 UTC (permalink / raw)
To: matthew.gerlach
Cc: Andy Shevchenko, hao.wu, russell.h.weight,
basheer.ahmed.muddebihal, trix, mdf, linux-fpga, linux-doc,
linux-kernel, tianfei.zhang, corbet, gregkh, linux-serial,
jirislaby, geert+renesas, niklas.soderlund+renesas, macro, johan,
lukas, ilpo.jarvinen, marpagan, bagasdotme
On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@linux.intel.com wrote:
>
>
> On Mon, 26 Dec 2022, Xu Yilun wrote:
>
> > On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@linux.intel.com wrote:
> > >
> > >
> > > On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> > >
> > > > On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote:
> > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > >
> > > > > Version 1 of the Device Feature Header (DFH) definition adds
> > > > > functionality to the DFL bus.
> > > > >
> > > > > A DFHv1 header may have one or more parameter blocks that
> > > > > further describes the HW to SW. Add support to the DFL bus
> > > > > to parse the MSI-X parameter.
> > > > >
> > > > > The location of a feature's register set 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.
> > > >
> > > > ...
> > > >
> > > > > +/**
> > > > > + * dfh_find_param() - find data for the given parameter id
> > > > > + * @dfl_dev: dfl device
> > > > > + * @param: id of dfl parameter
> > > > > + *
> > > > > + * Return: pointer to parameter header on success, NULL otherwise.
> > > >
> > > > header is a bit confusing here, does it mean we give and ID and we got
> > > > something more than just a data as summary above suggests?
> > >
> > > Yes, the summary is not correct. It should say "find the parameter block
> > > for the given parameter id".
> > >
> > > >
> > > > In such case summary and this text should clarify what exactly we get
> > > > and layout of the data. Since this is a pointer, who is responsible of
> > > > checking out-of-boundary accesses? For instance, if the parameters are
> > > > variadic by length the length should be returned as well. Otherwise it
> > > > should be specified as a constant somewhere, right?
> > >
> > > The parameter header has the next/size field; so the caller of
> > > dfh_find_param should perform boundary checking as part of interpreting the
> > > parameter data. I think a function to perform this checking and data
> > > interpretation would help here.
> >
> > It is better the DFL core provides the size of the parameter block, just
> > in this API. It provides the pointer and should be ensured the memory
> > for the pointer be correctly understood.
>
> Ok, how about the following API for dfh_find_param?
>
> /**
> * dfh_find_param() - find parameter block for the given parameter id
> * @dfl_dev: dfl device
> * @param_id: id of dfl parameter
> * @pver: destination to store parameter version
> * @pcount: destination to store size of parameter data in u64 bit words
The size of the parameter data could just be number of bytes (size_t is
ok?), this is the most common way for a data block.
> *
> * Return: pointer to start of parameter data, PTR_ERR otherwise.
> */
> void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, unsigned
> *pver, unsigned *pcount)
For now no driver is caring about parameter version, so we could just have
a simplified API without version, like:
void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, size_t *psize)
I assume this simplified API should be most commonly used by drivers,
changing the layout of the parameter block is not such a good idea to
me, try best not to do so.
If more property is to be added without changing the existing fields,
drivers could be aware of this just by the parameter size?
Anyway, if version is really needed in future, create another API like:
void *dfh_find_param_version(struct dfl_device *dfl_dev, int param_id,
size_t *psize, unsigned int *pver)
Thanks,
Yilun
>
>
> >
> > >
> > > >
> > > > > + */
> > > > > +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> > > > > +{
> > > > > + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(dfh_find_param);
> > > >
> > > > ...
> > > >
> > > > > + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > > >
> > > > It sounds like a candidate for struct_size() from overflow.h.
> > > > I.o.w. check that header and come up with the best what can
> > > > suit your case.
> > >
> > > finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)),
> > > GFP_KERNEL);
> > >
> > > Does seem better.
> >
> > How about we change the dfh_get_psize() to like dfh_get_pcount(), so we
> > don't have to multiply & divide back and forth.
>
> We need the size in bytes for calls to kmemdup, devm_kmemdup, and
When the count of u64 is caculated, you could still convert it to size of
bytes when needed.
> memcpy_fromio, but we only need to divide once here.
>
>
> >
> > Or we just use size_add()?
>
> I think using struct_size is better because the params member of struct
> dfl_feature_info is a trailing flexible array.
That's OK.
>
> Thanks for the feedback,
> Matthew
>
>
> >
> > Thanks,
> > Yilun
> >
> > >
> > > Thanks for the suggestion,
> > > Matthew Gerlach
> > >
> > >
> > > >
> > > > > if (!finfo)
> > > > > return -ENOMEM;
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > >
> > > >
> > > >
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2023-01-03 4:22 ` Xu Yilun
@ 2023-01-03 19:50 ` matthew.gerlach
2023-01-04 2:34 ` Xu Yilun
0 siblings, 1 reply; 22+ messages in thread
From: matthew.gerlach @ 2023-01-03 19:50 UTC (permalink / raw)
To: Xu Yilun
Cc: Andy Shevchenko, hao.wu, russell.h.weight,
basheer.ahmed.muddebihal, trix, mdf, linux-fpga, linux-doc,
linux-kernel, tianfei.zhang, corbet, gregkh, linux-serial,
jirislaby, geert+renesas, niklas.soderlund+renesas, macro, johan,
lukas, ilpo.jarvinen, marpagan, bagasdotme
On Tue, 3 Jan 2023, Xu Yilun wrote:
> On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Mon, 26 Dec 2022, Xu Yilun wrote:
>>
>>> On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@linux.intel.com wrote:
>>>>
>>>>
>>>> On Tue, 20 Dec 2022, Andy Shevchenko wrote:
>>>>
>>>>> On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote:
>>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>>
>>>>>> Version 1 of the Device Feature Header (DFH) definition adds
>>>>>> functionality to the DFL bus.
>>>>>>
>>>>>> A DFHv1 header may have one or more parameter blocks that
>>>>>> further describes the HW to SW. Add support to the DFL bus
>>>>>> to parse the MSI-X parameter.
>>>>>>
>>>>>> The location of a feature's register set 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.
>>>>>
>>>>> ...
>>>>>
>>>>>> +/**
>>>>>> + * dfh_find_param() - find data for the given parameter id
>>>>>> + * @dfl_dev: dfl device
>>>>>> + * @param: id of dfl parameter
>>>>>> + *
>>>>>> + * Return: pointer to parameter header on success, NULL otherwise.
>>>>>
>>>>> header is a bit confusing here, does it mean we give and ID and we got
>>>>> something more than just a data as summary above suggests?
>>>>
>>>> Yes, the summary is not correct. It should say "find the parameter block
>>>> for the given parameter id".
>>>>
>>>>>
>>>>> In such case summary and this text should clarify what exactly we get
>>>>> and layout of the data. Since this is a pointer, who is responsible of
>>>>> checking out-of-boundary accesses? For instance, if the parameters are
>>>>> variadic by length the length should be returned as well. Otherwise it
>>>>> should be specified as a constant somewhere, right?
>>>>
>>>> The parameter header has the next/size field; so the caller of
>>>> dfh_find_param should perform boundary checking as part of interpreting the
>>>> parameter data. I think a function to perform this checking and data
>>>> interpretation would help here.
>>>
>>> It is better the DFL core provides the size of the parameter block, just
>>> in this API. It provides the pointer and should be ensured the memory
>>> for the pointer be correctly understood.
>>
>> Ok, how about the following API for dfh_find_param?
>>
>> /**
>> * dfh_find_param() - find parameter block for the given parameter id
>> * @dfl_dev: dfl device
>> * @param_id: id of dfl parameter
>> * @pver: destination to store parameter version
>> * @pcount: destination to store size of parameter data in u64 bit words
>
> The size of the parameter data could just be number of bytes (size_t is
> ok?), this is the most common way for a data block.
>
>> *
>> * Return: pointer to start of parameter data, PTR_ERR otherwise.
>> */
>> void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, unsigned
>> *pver, unsigned *pcount)
>
> For now no driver is caring about parameter version, so we could just have
> a simplified API without version, like:
>
> void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, size_t *psize)
Using size_t and the simplified API you suggest is fine with me.
>
> I assume this simplified API should be most commonly used by drivers,
> changing the layout of the parameter block is not such a good idea to
> me, try best not to do so.
>
> If more property is to be added without changing the existing fields,
> drivers could be aware of this just by the parameter size?
>
>
> Anyway, if version is really needed in future, create another API like:
>
> void *dfh_find_param_version(struct dfl_device *dfl_dev, int param_id,
> size_t *psize, unsigned int *pver)
Sure, we can add API when it is actually used, as you point out, the
structure of a particular paramater should not change very often.
>
> Thanks,
> Yilun
>
>>
>>
>>>
>>>>
>>>>>
>>>>>> + */
>>>>>> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
>>>>>> +{
>>>>>> + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>>>>>
>>>>> ...
>>>>>
>>>>>> + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
>>>>>
>>>>> It sounds like a candidate for struct_size() from overflow.h.
>>>>> I.o.w. check that header and come up with the best what can
>>>>> suit your case.
>>>>
>>>> finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)),
>>>> GFP_KERNEL);
>>>>
>>>> Does seem better.
>>>
>>> How about we change the dfh_get_psize() to like dfh_get_pcount(), so we
>>> don't have to multiply & divide back and forth.
>>
>> We need the size in bytes for calls to kmemdup, devm_kmemdup, and
>
> When the count of u64 is caculated, you could still convert it to size of
> bytes when needed.
We need to use number of bytes more often than than count of u64. How
would calculating bytes from counts of u64 three times be better than
calculating counts of u64 once, like it is now?
Thanks,
Matthew Gerlach
>
>> memcpy_fromio, but we only need to divide once here.
>>
>>
>>>
>>> Or we just use size_add()?
>>
>> I think using struct_size is better because the params member of struct
>> dfl_feature_info is a trailing flexible array.
>
> That's OK.
>
>>
>> Thanks for the feedback,
>> Matthew
>>
>>
>>>
>>> Thanks,
>>> Yilun
>>>
>>>>
>>>> Thanks for the suggestion,
>>>> Matthew Gerlach
>>>>
>>>>
>>>>>
>>>>>> if (!finfo)
>>>>>> return -ENOMEM;
>>>>>
>>>>> --
>>>>> With Best Regards,
>>>>> Andy Shevchenko
>>>>>
>>>>>
>>>>>
>>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2023-01-03 19:50 ` matthew.gerlach
@ 2023-01-04 2:34 ` Xu Yilun
0 siblings, 0 replies; 22+ messages in thread
From: Xu Yilun @ 2023-01-04 2:34 UTC (permalink / raw)
To: matthew.gerlach
Cc: Andy Shevchenko, hao.wu, russell.h.weight,
basheer.ahmed.muddebihal, trix, mdf, linux-fpga, linux-doc,
linux-kernel, tianfei.zhang, corbet, gregkh, linux-serial,
jirislaby, geert+renesas, niklas.soderlund+renesas, macro, johan,
lukas, ilpo.jarvinen, marpagan, bagasdotme
On 2023-01-03 at 11:50:04 -0800, matthew.gerlach@linux.intel.com wrote:
>
>
> On Tue, 3 Jan 2023, Xu Yilun wrote:
>
> > On 2022-12-31 at 12:46:28 -0800, matthew.gerlach@linux.intel.com wrote:
> > >
> > >
> > > On Mon, 26 Dec 2022, Xu Yilun wrote:
> > >
> > > > On 2022-12-21 at 11:14:59 -0800, matthew.gerlach@linux.intel.com wrote:
> > > > >
> > > > >
> > > > > On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> > > > >
> > > > > > On Tue, Dec 20, 2022 at 08:36:51AM -0800, matthew.gerlach@linux.intel.com wrote:
> > > > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > >
> > > > > > > Version 1 of the Device Feature Header (DFH) definition adds
> > > > > > > functionality to the DFL bus.
> > > > > > >
> > > > > > > A DFHv1 header may have one or more parameter blocks that
> > > > > > > further describes the HW to SW. Add support to the DFL bus
> > > > > > > to parse the MSI-X parameter.
> > > > > > >
> > > > > > > The location of a feature's register set 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.
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > +/**
> > > > > > > + * dfh_find_param() - find data for the given parameter id
> > > > > > > + * @dfl_dev: dfl device
> > > > > > > + * @param: id of dfl parameter
> > > > > > > + *
> > > > > > > + * Return: pointer to parameter header on success, NULL otherwise.
> > > > > >
> > > > > > header is a bit confusing here, does it mean we give and ID and we got
> > > > > > something more than just a data as summary above suggests?
> > > > >
> > > > > Yes, the summary is not correct. It should say "find the parameter block
> > > > > for the given parameter id".
> > > > >
> > > > > >
> > > > > > In such case summary and this text should clarify what exactly we get
> > > > > > and layout of the data. Since this is a pointer, who is responsible of
> > > > > > checking out-of-boundary accesses? For instance, if the parameters are
> > > > > > variadic by length the length should be returned as well. Otherwise it
> > > > > > should be specified as a constant somewhere, right?
> > > > >
> > > > > The parameter header has the next/size field; so the caller of
> > > > > dfh_find_param should perform boundary checking as part of interpreting the
> > > > > parameter data. I think a function to perform this checking and data
> > > > > interpretation would help here.
> > > >
> > > > It is better the DFL core provides the size of the parameter block, just
> > > > in this API. It provides the pointer and should be ensured the memory
> > > > for the pointer be correctly understood.
> > >
> > > Ok, how about the following API for dfh_find_param?
> > >
> > > /**
> > > * dfh_find_param() - find parameter block for the given parameter id
> > > * @dfl_dev: dfl device
> > > * @param_id: id of dfl parameter
> > > * @pver: destination to store parameter version
> > > * @pcount: destination to store size of parameter data in u64 bit words
> >
> > The size of the parameter data could just be number of bytes (size_t is
> > ok?), this is the most common way for a data block.
> >
> > > *
> > > * Return: pointer to start of parameter data, PTR_ERR otherwise.
> > > */
> > > void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, unsigned
> > > *pver, unsigned *pcount)
> >
> > For now no driver is caring about parameter version, so we could just have
> > a simplified API without version, like:
> >
> > void *dfh_find_param(struct dfl_device *dfl_dev, int param_id, size_t *psize)
>
> Using size_t and the simplified API you suggest is fine with me.
>
> >
> > I assume this simplified API should be most commonly used by drivers,
> > changing the layout of the parameter block is not such a good idea to
> > me, try best not to do so.
> >
> > If more property is to be added without changing the existing fields,
> > drivers could be aware of this just by the parameter size?
> >
> >
> > Anyway, if version is really needed in future, create another API like:
> >
> > void *dfh_find_param_version(struct dfl_device *dfl_dev, int param_id,
> > size_t *psize, unsigned int *pver)
>
> Sure, we can add API when it is actually used, as you point out, the
> structure of a particular paramater should not change very often.
>
> >
> > Thanks,
> > Yilun
> >
> > >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > + */
> > > > > > > +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> > > > > > > +{
> > > > > > > + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(dfh_find_param);
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > + finfo = kzalloc(sizeof(*finfo) + dfh_psize, GFP_KERNEL);
> > > > > >
> > > > > > It sounds like a candidate for struct_size() from overflow.h.
> > > > > > I.o.w. check that header and come up with the best what can
> > > > > > suit your case.
> > > > >
> > > > > finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)),
> > > > > GFP_KERNEL);
> > > > >
> > > > > Does seem better.
> > > >
> > > > How about we change the dfh_get_psize() to like dfh_get_pcount(), so we
> > > > don't have to multiply & divide back and forth.
> > >
> > > We need the size in bytes for calls to kmemdup, devm_kmemdup, and
> >
> > When the count of u64 is caculated, you could still convert it to size of
> > bytes when needed.
>
> We need to use number of bytes more often than than count of u64. How would
> calculating bytes from counts of u64 three times be better than calculating
> counts of u64 once, like it is now?
And adding a local variable
dfh_psize = dfh_pcount * sizeof(u64) solves your concern.
Using pcount for struct_size is more straightforward to me. dfh_psize
could be truncated if it is not aligned to u64. People need to look into
the dfh_get_psize() to check the correctness.
Anyway this is trivial, I'm also OK with the change in v8.
Thanks,
Yilun
>
> Thanks,
> Matthew Gerlach
>
> >
> > > memcpy_fromio, but we only need to divide once here.
> > >
> > >
> > > >
> > > > Or we just use size_add()?
> > >
> > > I think using struct_size is better because the params member of struct
> > > dfl_feature_info is a trailing flexible array.
> >
> > That's OK.
> >
> > >
> > > Thanks for the feedback,
> > > Matthew
> > >
> > >
> > > >
> > > > Thanks,
> > > > Yilun
> > > >
> > > > >
> > > > > Thanks for the suggestion,
> > > > > Matthew Gerlach
> > > > >
> > > > >
> > > > > >
> > > > > > > if (!finfo)
> > > > > > > return -ENOMEM;
> > > > > >
> > > > > > --
> > > > > > With Best Regards,
> > > > > > Andy Shevchenko
> > > > > >
> > > > > >
> > > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-20 16:36 ` [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1 matthew.gerlach
2022-12-20 17:03 ` Andy Shevchenko
@ 2022-12-21 11:58 ` Ilpo Järvinen
2022-12-21 22:29 ` matthew.gerlach
1 sibling, 1 reply; 22+ messages in thread
From: Ilpo Järvinen @ 2022-12-21 11:58 UTC (permalink / raw)
To: Matthew Gerlach
Cc: hao.wu, yilun.xu, Russ Weight, basheer.ahmed.muddebihal, trix,
mdf, linux-fpga, linux-doc, LKML, tianfei.zhang, corbet,
Greg Kroah-Hartman, linux-serial, Jiri Slaby, geert+renesas,
Andy Shevchenko, niklas.soderlund+renesas, macro, johan,
Lukas Wunner, marpagan, bagasdotme
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]
On Tue, 20 Dec 2022, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Version 1 of the Device Feature Header (DFH) definition adds
> functionality to the DFL bus.
>
> A DFHv1 header may have one or more parameter blocks that
> further describes the HW to SW. Add support to the DFL bus
> to parse the MSI-X parameter.
>
> The location of a feature's register set 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>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v7: no change
>
> v6: move MSI_X parameter definitions to drivers/fpga/dfl.h
>
> v5: update field names
> fix find_param/dfh_get_psize
> clean up mmio_res assignments
> use u64* instead of void*
> use FIELD_GET instead of masking
>
> v4: s/MSIX/MSI_X
> move kernel doc to implementation
> use structure assignment
> fix decode of absolute address
> clean up comment in parse_feature_irqs
> remove use of csr_res
>
> v3: remove unneeded blank line
> use clearer variable name
> pass finfo into parse_feature_irqs()
> refactor code for better indentation
> use switch statement for irq parsing
> squash in code parsing register location
>
> v2: fix kernel doc
> clarify use of DFH_VERSION field
> ---
> +static u64 *find_param(u64 *params, resource_size_t max, int param_id)
> +{
> + u64 *end = params + max / sizeof(u64);
> + u64 v, next;
> +
> + while (params < end) {
> + v = *params;
> + if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> + return params;
> +
> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> + params += next;
> + if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
> + break;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find data for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to parameter header on success, NULL otherwise.
> + */
> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> +{
> + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);
BTW, should there be a way for the caller to ensure the parameter is long
enough?
All callers probably want to ensure the length of the parameter is valid
so it would perhaps make sense to add a parameter for the required
(minimum) length?
--
i.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1
2022-12-21 11:58 ` Ilpo Järvinen
@ 2022-12-21 22:29 ` matthew.gerlach
0 siblings, 0 replies; 22+ messages in thread
From: matthew.gerlach @ 2022-12-21 22:29 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: hao.wu, yilun.xu, Russ Weight, basheer.ahmed.muddebihal, trix,
mdf, linux-fpga, linux-doc, LKML, tianfei.zhang, corbet,
Greg Kroah-Hartman, linux-serial, Jiri Slaby, geert+renesas,
Andy Shevchenko, niklas.soderlund+renesas, macro, johan,
Lukas Wunner, marpagan, bagasdotme
[-- Attachment #1: Type: text/plain, Size: 3044 bytes --]
On Wed, 21 Dec 2022, Ilpo Järvinen wrote:
> On Tue, 20 Dec 2022, matthew.gerlach@linux.intel.com wrote:
>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Version 1 of the Device Feature Header (DFH) definition adds
>> functionality to the DFL bus.
>>
>> A DFHv1 header may have one or more parameter blocks that
>> further describes the HW to SW. Add support to the DFL bus
>> to parse the MSI-X parameter.
>>
>> The location of a feature's register set 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>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> v7: no change
>>
>> v6: move MSI_X parameter definitions to drivers/fpga/dfl.h
>>
>> v5: update field names
>> fix find_param/dfh_get_psize
>> clean up mmio_res assignments
>> use u64* instead of void*
>> use FIELD_GET instead of masking
>>
>> v4: s/MSIX/MSI_X
>> move kernel doc to implementation
>> use structure assignment
>> fix decode of absolute address
>> clean up comment in parse_feature_irqs
>> remove use of csr_res
>>
>> v3: remove unneeded blank line
>> use clearer variable name
>> pass finfo into parse_feature_irqs()
>> refactor code for better indentation
>> use switch statement for irq parsing
>> squash in code parsing register location
>>
>> v2: fix kernel doc
>> clarify use of DFH_VERSION field
>> ---
>
>> +static u64 *find_param(u64 *params, resource_size_t max, int param_id)
>> +{
>> + u64 *end = params + max / sizeof(u64);
>> + u64 v, next;
>> +
>> + while (params < end) {
>> + v = *params;
>> + if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> + return params;
>> +
>> + next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> + params += next;
>> + if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
>> + break;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +/**
>> + * dfh_find_param() - find data for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to parameter header on success, NULL otherwise.
>> + */
>> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
>> +{
>> + return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>
> BTW, should there be a way for the caller to ensure the parameter is long
> enough?
The caller can look at the DFHv1_PARAM_HDR_NEXT_OFFSET field of the
parameter header to see the size of the parameter block (header plus
data).
>
> All callers probably want to ensure the length of the parameter is valid
> so it would perhaps make sense to add a parameter for the required
> (minimum) length?
Yes, all callers should ensure that the length of the parameter is valid.
I will add another API call that performs length checking.
Thanks for the feedback,
Matthew Gerlach
>
>
> --
> i.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.
2022-12-20 16:36 [PATCH v7 0/4] Enhance definition of DFH and use enhancements for UART driver matthew.gerlach
` (2 preceding siblings ...)
2022-12-20 16:36 ` [PATCH v7 3/4] fpga: dfl: add basic support for DFHv1 matthew.gerlach
@ 2022-12-20 16:36 ` matthew.gerlach
2022-12-20 17:09 ` Andy Shevchenko
3 siblings, 1 reply; 22+ messages in thread
From: matthew.gerlach @ 2022-12-20 16:36 UTC (permalink / raw)
To: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
andriy.shevchenko, niklas.soderlund+renesas, macro, johan, lukas,
ilpo.jarvinen, marpagan, bagasdotme
Cc: 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>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v7: no change
v6: move driver specific parameter definitions to limit scope
v5: removed unneeded blank line
removed unneeded includes
included device.h and types.h
removed unneeded local variable
remove calls to dev_dbg
memset -> { }
remove space after period
explicitly include used headers
remove redundant Inc from Copyright
fix format specifier
v4: use dev_err_probe() everywhere that is appropriate
clean up noise
change error messages to use the word, unsupported
tried again to sort Makefile and KConfig better
reorder probe function for easier error handling
use new dfh_find_param API
v3: use passed in location of registers
use cleaned up functions for parsing parameters
v2: clean up error messages
alphabetize header files
fix 'missing prototype' error by making function static
tried to sort Makefile and Kconfig better
---
drivers/tty/serial/8250/8250_dfl.c | 154 +++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 12 +++
drivers/tty/serial/8250/Makefile | 1 +
3 files changed, 167 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..606279d420e0
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation.
+ *
+ * Authors:
+ * Ananda Ravuri <ananda.ravuri@intel.com>
+ * Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/dfl.h>
+#include <linux/errno.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+
+#define DFHv1_PARAM_ID_CLK_FRQ 0x2
+#define DFHv1_PARAM_ID_FIFO_LEN 0x3
+
+#define DFHv1_PARAM_ID_REG_LAYOUT 0x4
+#define DFHv1_PARAM_REG_LAYOUT_WIDTH GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_REG_LAYOUT_SHIFT GENMASK_ULL(31, 0)
+
+struct dfl_uart {
+ int line;
+};
+
+static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
+{
+ struct device *dev = &dfl_dev->dev;
+ u32 reg_width;
+ u64 fifo_len;
+ u64 *p;
+
+ p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
+ if (!p)
+ return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
+
+ p++;
+ uart->port.uartclk = *p;
+
+ p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN);
+ if (!p)
+ return dev_err_probe(dev, -EINVAL, "missing FIFO_LEN param\n");
+
+ p++;
+ fifo_len = *p;
+ switch (fifo_len) {
+ case 32:
+ uart->port.type = PORT_ALTR_16550_F32;
+ break;
+
+ case 64:
+ uart->port.type = PORT_ALTR_16550_F64;
+ break;
+
+ case 128:
+ uart->port.type = PORT_ALTR_16550_F128;
+ break;
+
+ default:
+ return dev_err_probe(dev, -EINVAL, "unsupported FIFO_LEN %llu\n", fifo_len);
+ }
+
+ p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT);
+ if (!p)
+ return dev_err_probe(dev, -EINVAL, "missing REG_LAYOUT param\n");
+
+ p++;
+ uart->port.regshift = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_SHIFT, *p);
+ reg_width = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_WIDTH, *p);
+ switch (reg_width) {
+ case 4:
+ uart->port.iotype = UPIO_MEM32;
+ break;
+
+ case 2:
+ uart->port.iotype = UPIO_MEM16;
+ break;
+
+ default:
+ return dev_err_probe(dev, -EINVAL, "unsupported reg-width %u\n", reg_width);
+
+ }
+
+ return 0;
+}
+
+static int dfl_uart_probe(struct dfl_device *dfl_dev)
+{
+ struct device *dev = &dfl_dev->dev;
+ struct uart_8250_port uart = { };
+ struct dfl_uart *dfluart;
+ int ret;
+
+ uart.port.flags = UPF_IOREMAP;
+ uart.port.mapbase = dfl_dev->mmio_res.start;
+ uart.port.mapsize = resource_size(&dfl_dev->mmio_res);
+
+ ret = dfl_uart_get_params(dfl_dev, &uart);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed uart feature walk\n");
+
+ if (dfl_dev->num_irqs == 1)
+ uart.port.irq = dfl_dev->irqs[0];
+
+ dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+ if (!dfluart)
+ return -ENOMEM;
+
+ dfluart->line = serial8250_register_8250_port(&uart);
+ if (dfluart->line < 0)
+ return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
+
+ dev_set_drvdata(dev, dfluart);
+
+ return 0;
+}
+
+static void dfl_uart_remove(struct dfl_device *dfl_dev)
+{
+ struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
+
+ serial8250_unregister_port(dfluart->line);
+}
+
+#define FME_FEATURE_ID_UART 0x24
+
+static const struct dfl_device_id dfl_uart_ids[] = {
+ { FME_ID, FME_FEATURE_ID_UART },
+ { }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
+
+static struct dfl_driver dfl_uart_driver = {
+ .drv = {
+ .name = "dfl-uart",
+ },
+ .id_table = dfl_uart_ids,
+ .probe = dfl_uart_probe,
+ .remove = dfl_uart_remove,
+};
+module_dfl_driver(dfl_uart_driver);
+
+MODULE_DESCRIPTION("DFL Intel UART driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index b0f62345bc84..08af2acd4645 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -370,6 +370,18 @@ config SERIAL_8250_FSL
erratum for Freescale 16550 UARTs in the 8250 driver. It also
enables support for ACPI enumeration.
+config SERIAL_8250_DFL
+ tristate "DFL bus driver for Altera 16550 UART"
+ depends on SERIAL_8250 && FPGA_DFL
+ help
+ This option enables support for a Device Feature List (DFL) bus
+ driver for the Altera 16650 UART. One or more Altera 16650 UARTs
+ can be instantiated in a FPGA and then be discovered during
+ enumeration of the DFL bus.
+
+ To compile this driver as a module, chose M here: the
+ module will be called 8250_dfl.
+
config SERIAL_8250_DW
tristate "Support for Synopsys DesignWare 8250 quirks"
depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 1615bfdde2a0..4e1a32812683 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554) += 8250_exar_st16c554.o
obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_hub6.o
obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
obj-$(CONFIG_SERIAL_8250_MEN_MCB) += 8250_men_mcb.o
+obj-$(CONFIG_SERIAL_8250_DFL) += 8250_dfl.o
obj-$(CONFIG_SERIAL_8250_DW) += 8250_dw.o
obj-$(CONFIG_SERIAL_8250_EM) += 8250_em.o
obj-$(CONFIG_SERIAL_8250_IOC3) += 8250_ioc3.o
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.
2022-12-20 16:36 ` [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550 matthew.gerlach
@ 2022-12-20 17:09 ` Andy Shevchenko
2022-12-21 17:26 ` Marco Pagani
2022-12-21 22:16 ` matthew.gerlach
0 siblings, 2 replies; 22+ messages in thread
From: Andy Shevchenko @ 2022-12-20 17:09 UTC (permalink / raw)
To: matthew.gerlach
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
marpagan, bagasdotme
On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
In general the code here looks good to me, but one thing to discuss due to
comment to the previous patch(es).
...
> + u64 *p;
> +
> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
> + if (!p)
> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
> +
> + p++;
> + uart->port.uartclk = *p;
So, here and the below is using always the second u64 from the returned data.
Does it mean:
- we always skip the first u64 from the returned buffer and hence... (see below)
- we may actually return the second u64 as a plain number (not a pointer) from
(an additional?) API? In such case we would not need to take care about this
p++; lines here and there.
- we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.
2022-12-20 17:09 ` Andy Shevchenko
@ 2022-12-21 17:26 ` Marco Pagani
2022-12-21 19:52 ` Marco Pagani
2022-12-21 22:16 ` matthew.gerlach
1 sibling, 1 reply; 22+ messages in thread
From: Marco Pagani @ 2022-12-21 17:26 UTC (permalink / raw)
To: Andy Shevchenko, matthew.gerlach
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
bagasdotme
On 2022-12-20 18:09, Andy Shevchenko wrote:
> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
>
> In general the code here looks good to me, but one thing to discuss due to
> comment to the previous patch(es).
>
> ...
>
>> + u64 *p;
>> +
>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>> + if (!p)
>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>> +
>> + p++;
>> + uart->port.uartclk = *p;
>
> So, here and the below is using always the second u64 from the returned data.
> Does it mean:
> - we always skip the first u64 from the returned buffer and hence... (see below)
> - we may actually return the second u64 as a plain number (not a pointer) from
> (an additional?) API? In such case we would not need to take care about this
> p++; lines here and there.
> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
>
I also had the impression that this method of getting and incrementing a pointer
to the beginning of the parameter block is a bit more error-prone than necessary.
Since parameter blocks are now standardized, wouldn't be easier and safer to wrap
the access logic into a helper function like:
u16 dfh_get_param_data(struct dfl_device *dfl_dev, u16 param_id, u64 *data)
that directly provides a copy of the parameter's data into a pointer provided by
the caller and returns the parameter version or an error if not found?
Thanks,
Marco
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.
2022-12-21 17:26 ` Marco Pagani
@ 2022-12-21 19:52 ` Marco Pagani
2022-12-21 22:37 ` matthew.gerlach
0 siblings, 1 reply; 22+ messages in thread
From: Marco Pagani @ 2022-12-21 19:52 UTC (permalink / raw)
To: Andy Shevchenko, matthew.gerlach
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
bagasdotme
On 2022-12-21 18:26, Marco Pagani wrote:
>
>
> On 2022-12-20 18:09, Andy Shevchenko wrote:
>> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
>>
>> In general the code here looks good to me, but one thing to discuss due to
>> comment to the previous patch(es).
>>
>> ...
>>
>>> + u64 *p;
>>> +
>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>> + if (!p)
>>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>>> +
>>> + p++;
>>> + uart->port.uartclk = *p;
>>
>> So, here and the below is using always the second u64 from the returned data.
>> Does it mean:
>> - we always skip the first u64 from the returned buffer and hence... (see below)
>> - we may actually return the second u64 as a plain number (not a pointer) from
>> (an additional?) API? In such case we would not need to take care about this
>> p++; lines here and there.
>> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
>>
>
> I also had the impression that this method of getting and incrementing a pointer
> to the beginning of the parameter block is a bit more error-prone than necessary.
> Since parameter blocks are now standardized, wouldn't be easier and safer to wrap
> the access logic into a helper function like:
>
> u16 dfh_get_param_data(struct dfl_device *dfl_dev, u16 param_id, u64 *data)
>
> that directly provides a copy of the parameter's data into a pointer provided by
> the caller and returns the parameter version or an error if not found?
Please ignore the last part of my reply. The diagram in the documentation made
me think that parameter data are always 64-bit wide. Since the parameter data
"payload" size depends on the version and ID, an eventual helper function could
then return a pointer to the data payload and the version number to the caller.
Thanks,
Marco
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.
2022-12-21 19:52 ` Marco Pagani
@ 2022-12-21 22:37 ` matthew.gerlach
0 siblings, 0 replies; 22+ messages in thread
From: matthew.gerlach @ 2022-12-21 22:37 UTC (permalink / raw)
To: Marco Pagani
Cc: Andy Shevchenko, hao.wu, yilun.xu, russell.h.weight,
basheer.ahmed.muddebihal, trix, mdf, linux-fpga, linux-doc,
linux-kernel, tianfei.zhang, corbet, gregkh, linux-serial,
jirislaby, geert+renesas, niklas.soderlund+renesas, macro, johan,
lukas, ilpo.jarvinen, bagasdotme
On Wed, 21 Dec 2022, Marco Pagani wrote:
>
> On 2022-12-21 18:26, Marco Pagani wrote:
>>
>>
>> On 2022-12-20 18:09, Andy Shevchenko wrote:
>>> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
>>>
>>> In general the code here looks good to me, but one thing to discuss due to
>>> comment to the previous patch(es).
>>>
>>> ...
>>>
>>>> + u64 *p;
>>>> +
>>>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>>>> + if (!p)
>>>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>>>> +
>>>> + p++;
>>>> + uart->port.uartclk = *p;
>>>
>>> So, here and the below is using always the second u64 from the returned data.
>>> Does it mean:
>>> - we always skip the first u64 from the returned buffer and hence... (see below)
>>> - we may actually return the second u64 as a plain number (not a pointer) from
>>> (an additional?) API? In such case we would not need to take care about this
>>> p++; lines here and there.
>>> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
>>>
>>
>> I also had the impression that this method of getting and incrementing a pointer
>> to the beginning of the parameter block is a bit more error-prone than necessary.
>> Since parameter blocks are now standardized, wouldn't be easier and safer to wrap
>> the access logic into a helper function like:
>>
>> u16 dfh_get_param_data(struct dfl_device *dfl_dev, u16 param_id, u64 *data)
>>
>> that directly provides a copy of the parameter's data into a pointer provided by
>> the caller and returns the parameter version or an error if not found?
>
> Please ignore the last part of my reply. The diagram in the documentation made
> me think that parameter data are always 64-bit wide. Since the parameter data
> "payload" size depends on the version and ID, an eventual helper function could
> then return a pointer to the data payload and the version number to the caller.
Certainly helper functions should be created to make accessing the actual
parameter easier and safer.
Thanks for the feedback,
Matthew Gerlach
>
> Thanks,
> Marco
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.
2022-12-20 17:09 ` Andy Shevchenko
2022-12-21 17:26 ` Marco Pagani
@ 2022-12-21 22:16 ` matthew.gerlach
1 sibling, 0 replies; 22+ messages in thread
From: matthew.gerlach @ 2022-12-21 22:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: hao.wu, yilun.xu, russell.h.weight, basheer.ahmed.muddebihal,
trix, mdf, linux-fpga, linux-doc, linux-kernel, tianfei.zhang,
corbet, gregkh, linux-serial, jirislaby, geert+renesas,
niklas.soderlund+renesas, macro, johan, lukas, ilpo.jarvinen,
marpagan, bagasdotme
On Tue, 20 Dec 2022, Andy Shevchenko wrote:
> On Tue, Dec 20, 2022 at 08:36:52AM -0800, 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.
>
> In general the code here looks good to me, but one thing to discuss due to
> comment to the previous patch(es).
>
> ...
>
>> + u64 *p;
>> +
>> + p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>> + if (!p)
>> + return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>> +
>> + p++;
>> + uart->port.uartclk = *p;
>
> So, here and the below is using always the second u64 from the returned data.
> Does it mean:
> - we always skip the first u64 from the returned buffer and hence... (see below)
The first u64 of the parameter block, the parameter header, contains a
version field and a next/size field that a parameter consumer might use.
The version field determines the exact layout of the data, and the
next/size field could/should be used to prevent out of bounds accesses.
> - we may actually return the second u64 as a plain number (not a pointer) from
> (an additional?) API? In such case we would not need to take care about this
> p++; lines here and there.
I think an additional API that can be used to fetch an array of u64's
while also checking boundary conditions would be helpful.
> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
The length and layout of the parameter data is determined by the parameter
id and version. So the data portion of a parameter is not fixed length.
Thanks for the feedback,
Matthew Gerlach
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread