Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Punit Agrawal <punitagrawal@gmail.com>
To:     Ronak Jain <ronak.jain@xilinx.com>
Cc:     <michal.simek@xilinx.com>, <linux-kernel@vger.kernel.org>,
        <gregkh@linuxfoundation.org>, <rajan.vaja@xilinx.com>,
        <corbet@lwn.net>, <linux-arm-kernel@lists.infradead.org>,
        <arnd@arndb.de>, <lakshmi.sai.krishna.potthuri@xilinx.com>
Subject: Re: [PATCH v2 2/3] firmware: zynqmp: Add sysfs entry for runtime
 features
References: <20210913083955.27146-1-ronak.jain@xilinx.com>
        <20210913083955.27146-3-ronak.jain@xilinx.com>
Date:   Fri, 17 Sep 2021 08:37:58 +0900
In-Reply-To: <20210913083955.27146-3-ronak.jain@xilinx.com> (Ronak Jain's
        message of "Mon, 13 Sep 2021 01:39:54 -0700")
Message-ID: <87ee9o6qi1.fsf@stealth>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Ronak,

Ronak Jain <ronak.jain@xilinx.com> writes:

> Create sysfs entry for runtime feature configuration. The support
>  is added for an over temperature and external watchdog feature.
>
> The below listed files are used for runtime features configuration:
> /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_id
> /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
>
> In order to configure an over temperature or external watchdog
>  features, first the user need to select the valid config id and then
>  the user can configure the value for selected feature config id.
>
> Signed-off-by: Ronak Jain <ronak.jain@xilinx.com>
> ---
> Changes in v2:
> - Update commit message
> ---
>  .../ABI/stable/sysfs-driver-firmware-zynqmp   | 84 +++++++++++++++++++
>  1 file changed, 84 insertions(+)
>
> diff --git a/Documentation/ABI/stable/sysfs-driver-firmware-zynqmp b/Documentation/ABI/stable/sysfs-driver-firmware-zynqmp
> index f5724bb5b462..2fde354715a5 100644
> --- a/Documentation/ABI/stable/sysfs-driver-firmware-zynqmp
> +++ b/Documentation/ABI/stable/sysfs-driver-firmware-zynqmp
> @@ -113,3 +113,87 @@ Description:
>  		    # echo 0 > /sys/devices/platform/firmware\:zynqmp-firmware/health_status
>  
>  Users:		Xilinx
> +
> +What:		/sys/devices/platform/firmware\:zynqmp-firmware/feature_config_*
> +Date:		Aug 2021
> +KernelVersion:	5.14
> +Contact:	"Ronak Jain" <ronak.jain@xilinx.com>
> +Description:
> +		This sysfs interface allows to configure features at runtime.
> +		The user can enable or disable features running at firmware.
> +		Also, the user can configure the parameters of the features
> +		at runtime. The supported features are over temperature and
> +		external watchdog. Here, the external watchdog is completely
> +		different than the /dev/watchdog as the external watchdog is
> +		running on the firmware and it is used to monitor the health
> +		of firmware not APU(Linux). Also, the external watchdog is
> +		interfaced outside of the zynqmp soc.
> +
> +		By default the features are disabled in the firmware. The user
> +		can enable features by querying appropriate config id of the
> +		features.
> +
> +		The default limit for the over temperature is 90 Degree Celsius.
> +		The default timer interval for the external watchdog is 570ms.
> +
> +		The supported config ids are for the feature configuration is,
> +		1. PM_FEATURE_OVERTEMP_STATUS = 1, the user can enable or
> +		disable the over temperature feature.
> +		2. PM_FEATURE_OVERTEMP_VALUE = 2, the user can configure the
> +		over temperature limit in Degree Celsius.
> +		3. PM_FEATURE_EXTWDT_STATUS = 3, the user can enable or disable
> +		the external watchdog feature.
> +		4. PM_FEATURE_EXTWDT_VALUE = 4, the user can configure the
> +		external watchdog feature.
> +
> +		Usage:
> +
> +		Enable over temperature feature
> +		# echo 1 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_id
> +		# echo 1 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +
> +		Check whether the over temperature feature is enabled or not
> +		# cat /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +		The expected result is 1.
> +
> +		Disable over temperature feature
> +		# echo 1 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_id
> +		# echo 0 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +
> +		Check whether the over temperature feature is disabled or not
> +		# cat /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +		The expected result is 0.
> +
> +		Configure over temperature limit to 50 Degree Celsius
> +		# echo 2 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_id
> +		# echo 50 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +
> +		Check whether the over temperature limit is configured or not
> +		# cat /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +		The expected result is 50.

Maybe I am missing something but I was wondering why these features are
being exposed via custom sysfs nodes - can they be integrated into the
respective subsystems?

e.g., Over temperature protection would fit in nicely with the thermal
sub-system. Did you explore integrating the feature there?

> +
> +		Enable external watchdog feature
> +		# echo 3 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_id
> +		# echo 1 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +
> +		Check whether the external watchdog feature is enabled or not
> +		# cat /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +		The expected result is 1.
> +
> +		Disable external watchdog feature
> +		# echo 3 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_id
> +		# echo 0 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +
> +		Check whether the external watchdog feature is disabled or not
> +		# cat /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +		The expected result is 0.
> +
> +		Configure external watchdog timer interval to 500ms
> +		# echo 4 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_id
> +		# echo 500 > /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +
> +		Check whether the external watchdog timer interval is configured or not
> +		# cat /sys/devices/platform/firmware\:zynqmp-firmware/feature_config_value
> +		The expected result is 500.

Similarly, this would make sense to go into the watchdog subsystem.

Using sub-system interfaces would allow existing tools to just work
without having to make zynqmp specific tools or find yet another way to
interact with these features. 

Thanks,
Punit