Message-ID: <fd367719-425b-a22d-d0a2-213e62310a89@gmail.com>
Date:   Fri, 29 Oct 2021 01:12:49 +0200
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Jarrett Schultz <jaschultzms@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <mgross@...ux.intel.com>
Cc:     linux-arm-msm@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
        Jarrett Schultz <jaschultz@...rosoft.com>
Subject: Re: [PATCH 2/3] platform: surface: Add surface xbl

On 10/28/21 23:17, Jarrett Schultz wrote:
> Introduce support for the Extensible Boot Loader driver found on the
> Surface Duo. Makes device information available to users via sysfs.
> 
> Signed-off-by: Jarrett Schultz <jaschultz@...rosoft.com>

In addition to the comments of Andy and Randy:

  - Your S-o-b address doesn't match the address you've sent this from.

  - Are you aware that the platform/surface directory as a whole depends
    on CONFIG_ACPI? As I understand it this patch is intended for an ARM
    device with DT/OF, so you might want to add a patch that moves the
    CONFIG_ACPI dependency to the individual drivers. A dependency of
    CONFIG_ACPI for the XBL driver seems rather unnecessary.

> ---
>   .../ABI/testing/sysfs-platform-surface-xbl    |  78 ++++++
>   MAINTAINERS                                   |   9 +
>   drivers/platform/surface/Kconfig              |  10 +
>   drivers/platform/surface/Makefile             |   1 +
>   drivers/platform/surface/surface-xbl.c        | 223 ++++++++++++++++++
>   5 files changed, 321 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
>   create mode 100644 drivers/platform/surface/surface-xbl.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-surface-xbl b/Documentation/ABI/testing/sysfs-platform-surface-xbl
> new file mode 100644
> index 000000000000..d3104dbbc6c1
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-surface-xbl
> @@ -0,0 +1,78 @@
> +What:		/sys/devices/platform/146bfa94.xbl/battery_present
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns whether the battery is present. Valid
> +		values are:
> +			0 - battery absent
> +			1 - battery present
> +
> +What:		/sys/devices/platform/146bfa94.xbl/board_id
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns the board id.
> +
> +What:		/sys/devices/platform/146bfa94.xbl/hw_init_retries
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns retries attempted to initialize the
> +		discrete hardware circuit.
> +
> +What:		/sys/devices/platform/146bfa94.xbl/is_act_mode
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns whether ACT mode is enabled. Valid values
> +		are:
> +			0 - ACT disabled
> +			1 - ACT enabled
> +
> +		ACT mode is used to run checks and put the device to shipmode
> +		at factory.
> +
> +What:		/sys/devices/platform/146bfa94.xbl/is_customer_mode
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns whether the device is in manufacturing
> +		mode. Valid values are:
> +			0 - Not in manufacturing mode
> +			1 - In manufacturing mode
> +
> +What:		/sys/devices/platform/146bfa94.xbl/ocp_error_location
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns 0 or which power rail has the OCP error.
> +		Valid values are:
> +			Bit(s)		Meaning
> +			15		More than one OCP error occurred
> +			14-12		PMIC
> +			11-7		SMPS
> +			6-2		LDO
> +			1-0		BOB
> +
> +What:		/sys/devices/platform/146bfa94.xbl/pmic_reset_reason
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns the reason for the reset. Valid values
> +		are:
> +			0 - no reason lol
> +			9 - Battery driver triggered
> +
> +What:		/sys/devices/platform/146bfa94.xbl/touch_fw_version
> +Date:		October 2021
> +KernelVersion:	5.15
> +Contact:	jaschultz@...rosoft.com
> +Description:
> +		Read only. It returns the version of the firmware.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index eeb4c70b3d5b..d08b68d626f6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12423,6 +12423,15 @@ F:	Documentation/driver-api/surface_aggregator/clients/dtx.rst
>   F:	drivers/platform/surface/surface_dtx.c
>   F:	include/uapi/linux/surface_aggregator/dtx.h
>   
> +MICROSOFT SURFACE DUO XBL DRIVER
> +M:	Jarrett Schultz <jaschultz@...rosoft.com>
> +L:	linux-arm-msm@...r.kernel.org
> +L:	platform-driver-x86@...r.kernel.org
> +S:	Supported
> +F:	Documentation/ABI/testing/sysfs-platform-surface-xbl
> +F:	Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> +F:	drivers/platform/surface/surface-xbl.c
> +
>   MICROSOFT SURFACE GPE LID SUPPORT DRIVER
>   M:	Maximilian Luz <luzmaximilian@...il.com>
>   L:	platform-driver-x86@...r.kernel.org
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index 3105f651614f..ca0546397414 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -180,6 +180,16 @@ config SURFACE_PRO3_BUTTON
>   	help
>   	  This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3/4 tablet.
>   
> +config SURFACE_XBL
> +	tristate "Surface XBL Driver"
> +	depends on ARM64 || COMPILE_TEST
> +	help
> +	  If you say 'Y' to this option, support will be included for the
> +	  Surface XBL Driver.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called surface-xbl.
> +
>   source "drivers/platform/surface/aggregator/Kconfig"
>   
>   endif # SURFACE_PLATFORMS
> diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
> index 32889482de55..1ed5808301e9 100644
> --- a/drivers/platform/surface/Makefile
> +++ b/drivers/platform/surface/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_SURFACE_GPE)		+= surface_gpe.o
>   obj-$(CONFIG_SURFACE_HOTPLUG)		+= surface_hotplug.o
>   obj-$(CONFIG_SURFACE_PLATFORM_PROFILE)	+= surface_platform_profile.o
>   obj-$(CONFIG_SURFACE_PRO3_BUTTON)	+= surfacepro3_button.o
> +obj-$(CONFIG_SURFACE_XBL)		+= surface-xbl.o
> diff --git a/drivers/platform/surface/surface-xbl.c b/drivers/platform/surface/surface-xbl.c
> new file mode 100644
> index 000000000000..910287f0c987
> --- /dev/null
> +++ b/drivers/platform/surface/surface-xbl.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * surface-xbl.c - Surface E(x)tensible (B)oot(l)oader
> + *
> + * Copyright (C) 2021 Microsoft Corporation
> + * Author: Jarrett Schultz <jaschultz@...rosoft.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/kstrtox.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define SURFACE_XBL_MAX_VERSION_LEN	16
> +#define SURFACE_XBL_BOARD_ID		0
> +#define SURFACE_XBL_BATTERY_PRESENT	1
> +#define SURFACE_XBL_HW_INIT_RETRIES	2
> +#define SURFACE_XBL_IS_CUSTOMER_MODE	3
> +#define SURFACE_XBL_IS_ACT_MODE		4
> +#define SURFACE_XBL_PMIC_RESET_REASON	5
> +#define SURFACE_XBL_TOUCH_FW_VERSION	6
> +#define SURFACE_XBL_OCP_ERROR_LOCATION	((SURFACE_XBL_TOUCH_FW_VERSION) +\
> +					(SURFACE_XBL_MAX_VERSION_LEN))
> +
> +struct surface_xbl {
> +	struct device	*dev;
> +	void __iomem	*regs;
> +
> +	u8		board_id;
> +	u8		battery_present;
> +	u8		hw_init_retries;
> +	u8		is_customer_mode;
> +	u8		is_act_mode;
> +	u8		pmic_reset_reason;
> +	char		touch_fw_version[SURFACE_XBL_MAX_VERSION_LEN];
> +	u16		ocp_error_location;
> +} __packed;
> +
> +static ssize_t
> +board_id_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sxbl->board_id);
> +}
> +static DEVICE_ATTR_RO(board_id);
> +
> +static ssize_t
> +battery_present_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sxbl->battery_present);
> +}
> +static DEVICE_ATTR_RO(battery_present);
> +
> +static ssize_t
> +hw_init_retries_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sxbl->hw_init_retries);
> +}
> +static DEVICE_ATTR_RO(hw_init_retries);
> +
> +static ssize_t
> +is_customer_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sxbl->is_customer_mode);
> +}
> +static DEVICE_ATTR_RO(is_customer_mode);
> +
> +static ssize_t
> +is_act_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sxbl->is_act_mode);
> +}
> +static DEVICE_ATTR_RO(is_act_mode);
> +
> +static ssize_t
> +pmic_reset_reason_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sxbl->pmic_reset_reason);
> +}
> +static DEVICE_ATTR_RO(pmic_reset_reason);
> +
> +static ssize_t
> +touch_fw_version_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "0x%s\n", sxbl->touch_fw_version);
> +}
> +static DEVICE_ATTR_RO(touch_fw_version);
> +
> +static ssize_t
> +ocp_error_location_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct surface_xbl	*sxbl = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sxbl->ocp_error_location);
> +}
> +static DEVICE_ATTR_RO(ocp_error_location);
> +
> +static struct attribute *inputs_attrs[] = {
> +	&dev_attr_board_id.attr,
> +	&dev_attr_battery_present.attr,
> +	&dev_attr_hw_init_retries.attr,
> +	&dev_attr_is_customer_mode.attr,
> +	&dev_attr_is_act_mode.attr,
> +	&dev_attr_pmic_reset_reason.attr,
> +	&dev_attr_touch_fw_version.attr,
> +	&dev_attr_ocp_error_location.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group inputs_attr_group = {
> +	.attrs = inputs_attrs,
> +};
> +
> +static u8 surface_xbl_readb(void __iomem *base, u32 offset)
> +{
> +	return readb(base + offset);
> +}
> +
> +static u16 surface_xbl_readw(void __iomem *base, u32 offset)
> +{
> +	return readw(base + offset);
> +}
> +
> +static int surface_xbl_probe(struct platform_device *pdev)
> +{
> +	struct surface_xbl	*sxbl;
> +	struct device		*dev;
> +	void __iomem		*regs;
> +	int					index;
> +	int					retval;
> +
> +	dev = &pdev->dev;
> +	sxbl = devm_kzalloc(dev, sizeof(*sxbl), GFP_KERNEL);
> +	if (!sxbl)
> +		return -ENOMEM;
> +
> +	sxbl->dev = dev;
> +
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	sxbl->regs = regs;
> +
> +	platform_set_drvdata(pdev, sxbl);
> +
> +	sxbl->board_id = surface_xbl_readb(sxbl->regs,
> +					   SURFACE_XBL_BOARD_ID);
> +	sxbl->battery_present = surface_xbl_readb(sxbl->regs,
> +						  SURFACE_XBL_BATTERY_PRESENT);
> +	sxbl->hw_init_retries = surface_xbl_readb(sxbl->regs,
> +						  SURFACE_XBL_HW_INIT_RETRIES);
> +	sxbl->is_customer_mode = surface_xbl_readb(sxbl->regs,
> +						   SURFACE_XBL_IS_CUSTOMER_MODE);
> +	sxbl->is_act_mode = surface_xbl_readb(sxbl->regs,
> +					      SURFACE_XBL_IS_ACT_MODE);
> +	sxbl->pmic_reset_reason = surface_xbl_readb(sxbl->regs,
> +						    SURFACE_XBL_PMIC_RESET_REASON);
> +
> +	for (index = 0; index < SURFACE_XBL_MAX_VERSION_LEN; index++)
> +		sxbl->touch_fw_version[index] = surface_xbl_readb(sxbl->regs,
> +							SURFACE_XBL_TOUCH_FW_VERSION + index);
> +
> +	sxbl->ocp_error_location = surface_xbl_readw(sxbl->regs,
> +						     SURFACE_XBL_OCP_ERROR_LOCATION);
> +
> +	retval = sysfs_create_group(&sxbl->dev->kobj, &inputs_attr_group);
> +	if (retval < 0) {
> +		dev_dbg(sxbl->dev,
> +			"Can't register sysfs attr group: %d\n", retval);
> +		return retval;
> +	}
> +
> +	return 0;
> +}
> +
> +static int surface_xbl_remove(struct platform_device *pdev)
> +{
> +	sysfs_remove_group(&pdev->dev.kobj, &inputs_attr_group);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id surface_xbl_of_match[] = {
> +	{
> +		.compatible = "microsoft,sm8150-surface-duo-xbl"
> +	},
> +	{  }, /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(of, surface_xbl_of_match);
> +#endif
> +
> +static struct platform_driver surface_xbl_driver = {
> +	.probe		= surface_xbl_probe,
> +	.remove		= surface_xbl_remove,
> +	.driver		= {
> +		.name	= "surface-xbl",
> +		.of_match_table = of_match_ptr(surface_xbl_of_match),
> +	},
> +};
> +
> +module_platform_driver(surface_xbl_driver);
> +
> +MODULE_AUTHOR("Jarrett Schultz <jaschultz@...rosoft.com>");
> +MODULE_DESCRIPTION("Surface Extensible Bootloader");
> +MODULE_LICENSE("GPL");
> 

Powered by blists - more mailing lists