RE: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface

From: Manne, Nava kishore
Date: Tue Feb 07 2023 - 04:24:34 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH v11 3/7] dt-bindings: arm: nuvoton: Add NPCM GFXI"
Previous message: Luca Weiss: "Re: [PATCH v3 00/12] fw_devlink improvements"
Next in thread: Xu Yilun: "Re: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Greg,

Thanks for providing the review comments.
Please find my response inline.

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, January 31, 2023 5:13 PM
> To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> Cc: michal.simek@xxxxxxxxxx; mdf@xxxxxxxxxx; hao.wu@xxxxxxxxx;
> yilun.xu@xxxxxxxxx; trix@xxxxxxxxxx; ronak.jain@xxxxxxxxxx;
> tanmay.shah@xxxxxxxxxx; Levinsky, Ben <ben.levinsky@xxxxxxx>;
> rajan.vaja@xxxxxxxxxx; harsha.harsha@xxxxxxxxxx;
> mathieu.poirier@xxxxxxxxxx; lakshmi.sai.krishna.potthuri@xxxxxxxxxx;
> arnd@xxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-fpga@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface
>
> On Tue, Jan 31, 2023 at 03:18:10PM +0530, Nava kishore Manne wrote:
> > Adds status interface for zynqmp-fpga, It's a read only interface
> > which allows the user to get the Programmable Logic(PL) configuration
> > status.
> >
> > Usage:
> > To read the Programmable Logic(PL) configuration status
> > cat /sys/class/fpga_manager/<fpga>/device/status
> >
> > Signed-off-by: Nava kishore Manne <nava.kishore.manne@xxxxxxx>
> > ---
> > Changes for v2:
> > - Updated status messages handling logic as suggested by Xu Yilun.
> >
> > Changes for v3:
> > - Updated status interface handling logic (Restrict the status
> > interface to the device-specific instead of handled by the core)
> > as suggested by Xu Yilun.
> >
> > Changes for v4:
> > - Limit the error strings to one word for each as suggested by
> > Xu Yilun
> >
> > Changes for v5:
> > - Added new sysfs-driver-zynqmp-fpga file.
> >
> > .../ABI/testing/sysfs-driver-zynqmp-fpga | 18 ++++
> > drivers/fpga/zynqmp-fpga.c | 87 +++++++++++++++++++
> > 2 files changed, 105 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-driver-zynqmp-fpga
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-zynqmp-fpga
> > b/Documentation/ABI/testing/sysfs-driver-zynqmp-fpga
> > new file mode 100644
> > index 000000000000..66c98bb8dfba
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-zynqmp-fpga
> > @@ -0,0 +1,18 @@
> > +What: /sys/class/fpga_manager/<fpga>/device/status
> > +Date: Jan 2023
> > +KernelVersion: 6.2
> > +Contact: Nava kishore Manne <nava.kishore.manne@xxxxxxx>
> > +Description: Read fpga status as a string.
> > + If FPGA programming operation fails, it could be caused by
> crc
> > + error or incompatible bitstream image. The intent of this
> > + interface is to provide more detailed information for FPGA
> > + programming errors to userspace. This is a list of strings for
> > + the supported status.
> > +
> > + * CRC-Error - CRC error detected by hardware.
> > + * Security-Error - Security error detected by hardware.
> > + * Initialization-Error - Device Initialization error.
> > + * Internal-Signal-Error - Device internal signal error.
> > + * I/Os-High-Z-state - All I/Os are placed in High-Z state.
> > + * Sequence-Error - Device start-up sequence error.
> > + * Firmware-Error - Firmware error.
> > diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> > index c60f20949c47..81d3e18527ee 100644
> > --- a/drivers/fpga/zynqmp-fpga.c
> > +++ b/drivers/fpga/zynqmp-fpga.c
> > @@ -15,6 +15,37 @@
> > /* Constant Definitions */
> > #define IXR_FPGA_DONE_MASK BIT(3)
> >
> > +/* Error Register */
> > +#define IXR_FPGA_ERR_CRC_ERR BIT(0)
> > +#define IXR_FPGA_ERR_SECURITY_ERR BIT(16)
> > +
> > +/* Signal Status Register */
> > +#define IXR_FPGA_END_OF_STARTUP BIT(4)
> > +#define IXR_FPGA_GST_CFG_B BIT(5)
> > +#define IXR_FPGA_INIT_B_INTERNAL BIT(11)
> > +#define IXR_FPGA_DONE_INTERNAL_SIGNAL BIT(13)
> > +
> > +/* FPGA error status. */
> > +enum {
> > + ZYNQMP_FPGA_STATUS_CRC_ERR,
> > + ZYNQMP_FPGA_STATUS_SECURITY_ERR,
> > + ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR,
> > + ZYNQMP_FPGA_STATUS_SIGNAL_ERR,
> > + ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR,
> > + ZYNQMP_FPGA_STATUS_EOS_ERR,
> > + ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR,
> > +};
> > +
> > +static const char * const zynqmp_fpga_error_statuses[] = {
> > + [ZYNQMP_FPGA_STATUS_CRC_ERR] = "CRC-Error",
> > + [ZYNQMP_FPGA_STATUS_SECURITY_ERR] = "Security-Error",
> > + [ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR] = "Initialization-Error",
> > + [ZYNQMP_FPGA_STATUS_SIGNAL_ERR] = "Internal-Signal-Error",
> > + [ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR] = "I/Os-High-Z-state",
> > + [ZYNQMP_FPGA_STATUS_EOS_ERR] = "Sequence-Error",
> > + [ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR] = "Firmware-
> Error", };
> > +
> > /**
> > * struct zynqmp_fpga_priv - Private data structure
> > * @dev: Device data structure
> > @@ -77,6 +108,54 @@ static enum fpga_mgr_states
> zynqmp_fpga_ops_state(struct fpga_manager *mgr)
> > return FPGA_MGR_STATE_UNKNOWN;
> > }
> >
> > +static ssize_t status_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + unsigned long status = 0;
> > + ssize_t len = 0;
> > + u32 reg_val;
> > + int ret;
> > + u8 i;
> > +
> > + ret = zynqmp_pm_fpga_get_config_status(&reg_val);
> > + if (!ret) {
> > + if (reg_val & IXR_FPGA_ERR_CRC_ERR)
> > + status |= ZYNQMP_FPGA_STATUS_CRC_ERR;
> > + if (reg_val & IXR_FPGA_ERR_SECURITY_ERR)
> > + status |= ZYNQMP_FPGA_STATUS_SECURITY_ERR;
> > + if (!(reg_val & IXR_FPGA_INIT_B_INTERNAL))
> > + status |= ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR;
> > + if (!(reg_val & IXR_FPGA_DONE_INTERNAL_SIGNAL))
> > + status |= ZYNQMP_FPGA_STATUS_SIGNAL_ERR;
> > + if (!(reg_val & IXR_FPGA_GST_CFG_B))
> > + status |=
> ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR;
> > + if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
> > + status |= ZYNQMP_FPGA_STATUS_EOS_ERR;
> > + } else {
> > + status =
> ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR;
> > + }
> > +
> > + for_each_set_bit(i, &status,
> ARRAY_SIZE(zynqmp_fpga_error_statuses))
> > + len += sysfs_emit_at(buf, len, "%s ",
> > + zynqmp_fpga_error_statuses[i]);
> > +
>
> Sorry, but no, you can not export a list of strings that userspace is somehow
> going to parse.
>
> As this is all a bit field, why not just export the raw hex value instead and let
> userspace parse what bits are set?
>

This implementation is align with the fpga core status interface(https://git.kernel.org/pub/scm/linux/kernel/git/fpga/linux-fpga.git/tree/drivers/fpga/fpga-mgr.c?h=for-6.2#n632)
I Agree to export the raw hex value instead of multiple error strings.
In case of exporting the raw hex value for status can we update this fpga-mgr.c core file (status_show() API)
instead of creating individual sysfs file per driver/device? and with this proposed implementation
will also fix the issues/Limitations with generic status interface.

Existing limitation with the generic fpga status interface:
Different vendors have different error sets defined by Hardware. If we always define the new bits when we cannot find an exact 1:1
mapping in the core the 64 bits would soon be used out.

Xu Yilun: Can you please share your thoughts here?

Regards,
Navakishore.

Next message: Krzysztof Kozlowski: "Re: [PATCH v11 3/7] dt-bindings: arm: nuvoton: Add NPCM GFXI"
Previous message: Luca Weiss: "Re: [PATCH v3 00/12] fw_devlink improvements"
Next in thread: Xu Yilun: "Re: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]