From:   "Manne, Nava kishore" <nava.kishore.manne@amd.com>
To:     Greg KH <gregkh@linuxfoundation.org>,
        "yilun.xu@intel.com" <yilun.xu@intel.com>
CC:     "michal.simek@xilinx.com" <michal.simek@xilinx.com>,
        "mdf@kernel.org" <mdf@kernel.org>,
        "hao.wu@intel.com" <hao.wu@intel.com>,
        "trix@redhat.com" <trix@redhat.com>,
        "ronak.jain@xilinx.com" <ronak.jain@xilinx.com>,
        "tanmay.shah@xilinx.com" <tanmay.shah@xilinx.com>,
        "Levinsky, Ben" <ben.levinsky@amd.com>,
        "rajan.vaja@xilinx.com" <rajan.vaja@xilinx.com>,
        "harsha.harsha@xilinx.com" <harsha.harsha@xilinx.com>,
        "mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>,
        "lakshmi.sai.krishna.potthuri@xilinx.com" 
        <lakshmi.sai.krishna.potthuri@xilinx.com>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>
Subject: RE: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface
Thread-Topic: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface
Thread-Index: AQHZNVkp73EU2R5VmU2aNqwbd25YNK64Z4mAgArLZGA=
Date:   Tue, 7 Feb 2023 09:23:12 +0000
Message-ID: <DM6PR12MB39931368F0863BBBF22BC806CDDB9@DM6PR12MB3993.namprd12.prod.outlook.com>
References: <20230131094810.1945541-1-nava.kishore.manne@amd.com>
 <20230131094810.1945541-3-nava.kishore.manne@amd.com>
 <Y9j+rZQkiPcIPxOr@kroah.com>
In-Reply-To: <Y9j+rZQkiPcIPxOr@kroah.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?LfBKdTWXZ/YdxlHO7YXs2UIMFuqY2i8NvWSfdYQiFpSjmE20acVu0CeVf+D9?=
 =?us-ascii?Q?DRQPY2OXpsfDNQpuV7fToh7edcIELrYZO16lYCF6OXBcMZxK/p9ECki55wjd?=
 =?us-ascii?Q?BeS2679vP6wcuyb+gXCp6XRFy4Hto6LmJGmf1VppBpkR0iaAhX5z8g/cQufY?=
 =?us-ascii?Q?QkChfxz3b5qlrnxSaRTxgGjPmpQuebc88uye+EYlqjdR7xnYV0XPSjdnoWZF?=
 =?us-ascii?Q?ubgPixV3qBASNJnXR6+O64qr+s7zVqVoDk9VGkGGp/r4BzwUfxaZcNzAKicx?=
 =?us-ascii?Q?xVwNg+NXXEPmmW2qd1byENP5XjQiEo2h6uDihC0/oKOPUrZ/ATxiE/br4TAX?=
 =?us-ascii?Q?oWIyXOwe9Kwan9hJBYBlr23Bnr5eATO7lPcK43cPA5xsQqmdBR36zMt0fhT/?=
 =?us-ascii?Q?+xwlFQiapBIhbLRr9tdd/R6Ri21Lxgx+JagEjDRKXrT63dr+BQNeUkxBbP7P?=
 =?us-ascii?Q?I+reTpyqoNHuOqMZu3NM+sIsSmMPV4B7rIRx5iqiYrdJNq4cunxtU8zfZAXZ?=
 =?us-ascii?Q?Eq5fzVIKuCJxgwbMTam/HSs99/xJQ7bH3oC/pyNWV0aueizVzffBTjIISudz?=
 =?us-ascii?Q?fQsAtlPOQoLPHuAQmkVgbeXce1OSdcQmR8GZtBcY8KtFs1NtCMdbAvXgGrU/?=
 =?us-ascii?Q?HQfvQkmTO1mEnu4c+LWdRGQQtPgiL/6N3opNsOo4HVVWgPT6Hla+BSuTEFqK?=
 =?us-ascii?Q?eJUtvpcyFuTIuzKPyM+JhjNz6d3JlVfeezV74F/YqF7yWdtX/dWn0FOUBXnv?=
 =?us-ascii?Q?A8+awrk0K/HtVaFS5c2BX/AZAWupv5rSPC60KitXSc6ywsihfboaKS4M2/MV?=
 =?us-ascii?Q?Cl1LBLZGQPrGn4MQ4oMQTlaUO7Ancj5L4Zgij+2ZSXl1m1gr5IlmpszklUAk?=
 =?us-ascii?Q?bMw5gYPxBmHvO6Z23TNh81p0hG5d2XnMnGqof8o47MZvfN37p8/I8emcKpSn?=
 =?us-ascii?Q?hSLRoTS0DWccTJdOZ++aLtUVd9v3Mz1RplTwwZAqhkTlFnmuMEVRVn06roza?=
 =?us-ascii?Q?sZWpP/EezbpsXWXpcN5/kgv4GaCSv/qffCUTbWLGN8TAPKq4Yqr/d4xJzKAq?=
 =?us-ascii?Q?O016/Y9ZSbW1jdT9UJFWkOf1bLjHQnTRNgZmkivG7gFBEDEMqnidmHycs7ag?=
 =?us-ascii?Q?xhawZJNxPqB5fgclYDrJMci3kq2H/jkjXJzNyBFU/9nwR9hhGRqWYLG/aaL/?=
 =?us-ascii?Q?FfpFkgHmqQlo4LnCTRw0aO1xgdSPazoQ3X7wiSS42IwRhFXvqnN1IqAlv3z/?=
 =?us-ascii?Q?XHcCvhkjaS4H2HtLvOLnlrcBGJ52CZcTORiMOVUI4DznBS9xTfqxrhvL0Iva?=
 =?us-ascii?Q?Sco0ksDjJcMQIRI6w3CtYorR6VItyii6HQYL+lFguoxnNoPd+dDRdsf41bk7?=
 =?us-ascii?Q?ivmtZa8CGpR9vUy0xSJUge8wN2ra8/RNRtoR+Xk5OdLC6+UfuEhPL+ETs6j5?=
 =?us-ascii?Q?3n2Ox2JHxEXPz6tlzAKJDDRYLe3FuQ0ILj8na6DG4/PF/rYcPiSQVfOjHgEK?=
 =?us-ascii?Q?v6ILvkcCFRFo8vhf1Q67sXKvXFQuAXyW84lgnDukXIp2Ov6hIKfnmKjU1PYH?=
 =?us-ascii?Q?4bqjXv6+qwvmyrLDfag=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3993.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7d90506a-de8b-4905-b3d5-08db08ecef99
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Feb 2023 09:23:12.7563
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: pXmw4iy5OKhfmSTTiKygTycSWdmm8IZmeUIGNbYA26nhQG34Bzh4at45LXpryh+h
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB6766
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Greg,

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

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, January 31, 2023 5:13 PM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: michal.simek@xilinx.com; mdf@kernel.org; hao.wu@intel.com;
> yilun.xu@intel.com; trix@redhat.com; ronak.jain@xilinx.com;
> tanmay.shah@xilinx.com; Levinsky, Ben <ben.levinsky@amd.com>;
> rajan.vaja@xilinx.com; harsha.harsha@xilinx.com;
> mathieu.poirier@linaro.org; lakshmi.sai.krishna.potthuri@xilinx.com;
> arnd@arndb.de; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-fpga@vger.kernel.org
> Subject: Re: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface
>=20
> 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@amd.com>
> > ---
> > Changes for v2:
> >               - Updated status messages handling logic as suggested by =
Xu Yilun.
> >
> > Changes for v3:
> >               - Updated status interface handling logic (Restrict the s=
tatus
> >                 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 suggest=
ed 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@amd.com>
> > +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[] =3D {
> > +	[ZYNQMP_FPGA_STATUS_CRC_ERR] =3D "CRC-Error",
> > +	[ZYNQMP_FPGA_STATUS_SECURITY_ERR] =3D "Security-Error",
> > +	[ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR] =3D "Initialization-Error",
> > +	[ZYNQMP_FPGA_STATUS_SIGNAL_ERR] =3D "Internal-Signal-Error",
> > +	[ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR] =3D "I/Os-High-Z-state",
> > +	[ZYNQMP_FPGA_STATUS_EOS_ERR] =3D "Sequence-Error",
> > +	[ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR] =3D "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 =3D 0;
> > +	ssize_t len =3D 0;
> > +	u32 reg_val;
> > +	int ret;
> > +	u8 i;
> > +
> > +	ret =3D zynqmp_pm_fpga_get_config_status(&reg_val);
> > +	if (!ret) {
> > +		if (reg_val & IXR_FPGA_ERR_CRC_ERR)
> > +			status |=3D ZYNQMP_FPGA_STATUS_CRC_ERR;
> > +		if (reg_val & IXR_FPGA_ERR_SECURITY_ERR)
> > +			status |=3D ZYNQMP_FPGA_STATUS_SECURITY_ERR;
> > +		if (!(reg_val & IXR_FPGA_INIT_B_INTERNAL))
> > +			status |=3D ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR;
> > +		if (!(reg_val & IXR_FPGA_DONE_INTERNAL_SIGNAL))
> > +			status |=3D ZYNQMP_FPGA_STATUS_SIGNAL_ERR;
> > +		if (!(reg_val & IXR_FPGA_GST_CFG_B))
> > +			status |=3D
> ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR;
> > +		if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
> > +			status |=3D ZYNQMP_FPGA_STATUS_EOS_ERR;
> > +	} else {
> > +		status =3D
> ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR;
> > +	}
> > +
> > +	for_each_set_bit(i, &status,
> ARRAY_SIZE(zynqmp_fpga_error_statuses))
> > +		len +=3D sysfs_emit_at(buf, len, "%s ",
> > +				     zynqmp_fpga_error_statuses[i]);
> > +
>=20
> Sorry, but no, you can not export a list of strings that userspace is som=
ehow
> going to parse.
>=20
> As this is all a bit field, why not just export the raw hex value instead=
 and let
> userspace parse what bits are set?
>=20

This implementation is align with the fpga core status interface(https://gi=
t.kernel.org/pub/scm/linux/kernel/git/fpga/linux-fpga.git/tree/drivers/fpga=
/fpga-mgr.c?h=3Dfor-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-m=
gr.c core file (status_show() API)=20
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:=20
Different vendors have different error sets defined by Hardware. If we alwa=
ys define the new bits when we cannot find an exact 1:1
mapping in the core the 64 bits would soon be used out.=20

Xu Yilun: Can you please share your thoughts here? =20

Regards,
Navakishore.