[PATCH v5 0/4] platform: surface: Introduce Surface XBL Driver

[PATCH v5 0/4] platform: surface: Introduce Surface XBL Driver

[Jarrett Schultz]

From: Jarrett Schultz <jaschultz@microsoft.com>

After v4, there were some suggestions to change the driver again to use
nvmem that would include some other changes to the sm8150 dtsi. While
the suggestions make sense, this driver was supposed to remain simple
for the introduction in order to get it into the tree and I think that
it would be best to implement those and any other suggestions in a future
patch. Hopefully this patch is now in a state where it can be accepted.
Thanks to all who have helped and been patient along the way, this was
my first patch :)

~ Jarrett

---

Introduce the Surface Extensible Boot Loader driver for the Surface Duo.
Exposes information about the driver to user space via sysfs for
consumption in manufacturing mode.

---

Changes in v5:

 - Minor changes to yaml

---

Changes in v4:

 - Small binding definition changes
 - Removed ACPI propagation from patch series since it has been
   cherry-picked
 - Fixed the Signed-off-by: and From: mismatch

---

Changes in v3:
 - For the yaml documentation:
    * Updated description
    * Fixed examples
    * Updated 'required' field
 - Further propogated ACPI dependency in Kconfigs
 - Updated sysfs several binding descriptions
 - Renamed files to conform to naming conventions

---

Changes in v2:
 - Per Maximilian, added patch 2: propagated ACPI dependency from the
   directory as a whole to each individual driver
 - For the yaml documentation:
    * Removed json-schema dependence
    * Elaborated on description of driver
    * Updated example
 - Changed target KernelVersion in sysfs documentation
 - Updated MAINTAINER changes to be properly applied across patches
 - For the driver itself,
    * Added types.h inclusion and removed unused inclusions
    * Minor updates to code and acronym style
    * Remove __packed attribute on driver struct
    * Use .dev_groups for sysfs
 - Added more in-depth description of driver in Kconfig
 - Modified dts to reference a newly added section in sm8150.dtsi

---

Jarrett Schultz (4):
  dt-bindings: platform: microsoft: Document surface xbl
  platform: surface: Add surface xbl
  arm64: dts: qcom: sm8150: Add imem section
  arm64: dts: qcom: surface-duo: Add surface xbl

 .../ABI/testing/sysfs-platform-surface-xbl    |  79 ++++++++
 .../platform/microsoft/surface-xbl.yaml       |  70 +++++++
 MAINTAINERS                                   |   9 +
 .../dts/qcom/sm8150-microsoft-surface-duo.dts |  10 +
 arch/arm64/boot/dts/qcom/sm8150.dtsi          |   8 +
 drivers/platform/surface/Kconfig              |  12 ++
 drivers/platform/surface/Makefile             |   1 +
 drivers/platform/surface/surface_xbl.c        | 186 ++++++++++++++++++
 8 files changed, 375 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
 create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
 create mode 100644 drivers/platform/surface/surface_xbl.c

-- 
2.25.1

[PATCH v5 1/4] dt-bindings: platform: microsoft: Document surface xbl

[Jarrett Schultz]

From: Jarrett Schultz <jaschultz@microsoft.com>

Introduce yaml for surface xbl driver.

Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>

---

Changed in v5:

 - Updated description
 - Added child node for imem
 - Simplified example

---

Changes in v4:
 - Addressed small formatting changes
 - Removed unnecessary lines

---

Changes in v3:
 - Updated description to only pertain to the hardware
 - Updated the required field to properly reflect the binding
 - Removed the first example
 - Fixed the size of the reg field in the second example

---

Changes in v2:
 - Removed json-schema dependence
 - Elaborated on description of driver
 - Updated example
---
 .../platform/microsoft/surface-xbl.yaml       | 70 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 2 files changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml

diff --git a/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
new file mode 100644
index 000000000000..648476670bbd
--- /dev/null
+++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/platform/microsoft/surface-xbl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Surface Extensible Bootloader for Microsoft Surface Duo
+
+maintainers:
+  - Jarrett Schultz <jaschultz@microsoft.com>
+
+description: |
+  Defined to expose information that is used during production when
+  the device is in manufacturing mode. Some of the information included
+  in the imem section is -
+    * board_id
+    * battery_present
+    * hw_init_retries
+    * is_customer_mode
+    * is_act_mode
+    * pmic_reset_reason
+    * touch_fw_version
+    * ocp_error_location
+
+properties:
+  compatible:
+    const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  child-node:
+    description: A description of the xbl space within imem
+
+    type: object
+
+    properties:
+      compatible: 
+        const: microsoft,sm8150-surface-duo-xbl
+
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - ranges
+  - address-cells
+  - size-cells
+
+examples:
+  - |
+    imem@146bf000 {
+      compatible = "simple-mfd";
+      reg = <0x146bf000 0x1000>;
+      ranges = <0x0 0x146bf000 0x1000>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      xbl@a94 {
+        compatible = "microsoft,sm8150-surface-duo-xbl";
+        reg = <0xa94 0x100>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 13f9a84a617e..5d0ca2a98b57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12649,6 +12649,13 @@ 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@microsoft.com>
+L:	linux-arm-msm@vger.kernel.org
+L:	platform-driver-x86@vger.kernel.org
+S:	Supported
+F:	Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
+
 MICROSOFT SURFACE GPE LID SUPPORT DRIVER
 M:	Maximilian Luz <luzmaximilian@gmail.com>
 L:	platform-driver-x86@vger.kernel.org
-- 
2.25.1

[PATCH v5 2/4] platform: surface: Add surface xbl

[Jarrett Schultz]

From: Jarrett Schultz <jaschultz@microsoft.com>

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@microsoft.com>

---

Changes in v4:
 - N/A

---

Changes in v3:
 - Updated several descriptions in the yaml to be more descriptive
 - Renamed files to use an underscore instead of a dash

---

Changes in v2:
 - Added types.h inclusion and removed unused inclusions
 - Minor updates to code and acronym style
 - Remove __packed attribute on driver struct
 - Use .dev_groups for sysfs
 - Added more in-depth description of driver in Kconfig
 - Changed target KernelVersion in sysfs documentation
---
 .../ABI/testing/sysfs-platform-surface-xbl    |  79 ++++++++
 MAINTAINERS                                   |   2 +
 drivers/platform/surface/Kconfig              |  12 ++
 drivers/platform/surface/Makefile             |   1 +
 drivers/platform/surface/surface_xbl.c        | 186 ++++++++++++++++++
 5 files changed, 280 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..505a662d8511
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-surface-xbl
@@ -0,0 +1,79 @@
+What:		/sys/devices/platform/146bfa94.xbl/battery_present
+Date:		October 2021
+KernelVersion:	5.16
+Contact:	jaschultz@microsoft.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.16
+Contact:	jaschultz@microsoft.com
+Description:
+		Read only. It returns a Microsoft specific board id.
+
+What:		/sys/devices/platform/146bfa94.xbl/hw_init_retries
+Date:		October 2021
+KernelVersion:	5.16
+Contact:	jaschultz@microsoft.com
+Description:
+		Read only. It returns a Microsoft specific value related to the
+		retries attempted to properly initialize the battery charging
+		subsystem.
+
+What:		/sys/devices/platform/146bfa94.xbl/is_act_mode
+Date:		October 2021
+KernelVersion:	5.16
+Contact:	jaschultz@microsoft.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.16
+Contact:	jaschultz@microsoft.com
+Description:
+
+		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.16
+Contact:	jaschultz@microsoft.com
+Description:
+		Read only. It returns 0 or the id of the power rail which 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.16
+Contact:	jaschultz@microsoft.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.16
+Contact:	jaschultz@microsoft.com
+Description:
+		Read only. It returns the version of the firmware.
diff --git a/MAINTAINERS b/MAINTAINERS
index 5d0ca2a98b57..e65eddf5d25d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12654,7 +12654,9 @@ M:	Jarrett Schultz <jaschultz@microsoft.com>
 L:	linux-arm-msm@vger.kernel.org
 L:	platform-driver-x86@vger.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@gmail.com>
diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
index 5f0578e25f71..96d6030d8d49 100644
--- a/drivers/platform/surface/Kconfig
+++ b/drivers/platform/surface/Kconfig
@@ -185,6 +185,18 @@ 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
+        depends on OF
+        help
+          If you say 'Y' to this option, support will be included for the
+          Surface Extensible Boot Loader (XBL) Driver. This driver exposes
+          information about the device through sysfs.
+
+          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..eef05d247f75 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..e6272b704526
--- /dev/null
+++ b/drivers/platform/surface/surface_xbl.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Surface eXtensible Boot Loader (XBL)
+ *
+ * Copyright (C) 2021 Microsoft Corporation
+ * Author: Jarrett Schultz <jaschultz@microsoft.com>
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.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;
+
+	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;
+};
+
+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 *xbl_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 xbl_attr_group = {
+	.attrs = xbl_attrs,
+};
+
+const struct attribute_group *xbl_sysfs_groups[] = {
+	&xbl_attr_group,
+	NULL
+};
+
+static int surface_xbl_probe(struct platform_device *pdev)
+{
+	struct surface_xbl	*sxbl;
+	void __iomem		*regs;
+	int			i;
+
+	sxbl = devm_kzalloc(&pdev->dev, sizeof(*sxbl), GFP_KERNEL);
+	if (!sxbl)
+		return -ENOMEM;
+
+	sxbl->dev = &pdev->dev;
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	platform_set_drvdata(pdev, sxbl);
+
+	sxbl->board_id = readb(regs + SURFACE_XBL_BOARD_ID);
+	sxbl->battery_present = readb(regs + SURFACE_XBL_BATTERY_PRESENT);
+	sxbl->hw_init_retries = readb(regs + SURFACE_XBL_HW_INIT_RETRIES);
+	sxbl->is_customer_mode = readb(regs + SURFACE_XBL_IS_CUSTOMER_MODE);
+	sxbl->is_act_mode = readb(regs + SURFACE_XBL_IS_ACT_MODE);
+	sxbl->pmic_reset_reason = readb(regs + SURFACE_XBL_PMIC_RESET_REASON);
+
+	for (i = 0; i < SURFACE_XBL_MAX_VERSION_LEN; i++)
+		sxbl->touch_fw_version[i] = readb(regs + SURFACE_XBL_TOUCH_FW_VERSION + i);
+
+	sxbl->ocp_error_location = readb(regs + SURFACE_XBL_OCP_ERROR_LOCATION);
+
+	return 0;
+}
+
+static const struct of_device_id surface_xbl_of_match[] = {
+	{
+		.compatible = "microsoft,sm8150-surface-duo-xbl"
+	},
+	{  }
+};
+MODULE_DEVICE_TABLE(of, surface_xbl_of_match);
+
+static struct platform_driver surface_xbl_driver = {
+	.driver	= {
+		.name		= "surface-xbl",
+		.of_match_table = surface_xbl_of_match,
+		.dev_groups	= xbl_sysfs_groups
+	},
+	.probe		= surface_xbl_probe,
+};
+module_platform_driver(surface_xbl_driver);
+
+MODULE_AUTHOR("Jarrett Schultz <jaschultz@microsoft.com>");
+MODULE_DESCRIPTION("Surface Extensible Bootloader");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH v5 3/4] arm64: dts: qcom: sm8150: Add imem section

[Jarrett Schultz]

From: Jarrett Schultz <jaschultz@microsoft.com>

Introduce the imem section in preparation for the surface xbl driver.

Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---

Changes in v4:
 - N/A

---

Changes in v3:
 - N/A

---

Changes in v2:
 - Created to properly reference the xbl section inside of imem
---
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index 81b4ff2cc4cd..bcfcb7306cdc 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -1175,6 +1175,14 @@ gpi_dma1: dma-controller@a00000 {
 			status = "disabled";
 		};
 
+		imem: imem@146bf000 {
+			compatible = "simple-mfd";
+			reg = <0x0 0x146bf000 0x0 0x1000>;
+			ranges = <0x0 0x0 0x146bf000 0x1000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
+
 		qupv3_id_1: geniqup@ac0000 {
 			compatible = "qcom,geni-se-qup";
 			reg = <0x0 0x00ac0000 0x0 0x6000>;
-- 
2.25.1

[PATCH v5 4/4] arm64: dts: qcom: surface-duo: Add surface xbl

[Jarrett Schultz]

From: Jarrett Schultz <jaschultz@microsoft.com>

Introduce device tree source for the surface xbl driver.

Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>

---

Changes in v4:
 - N/A

---

Changes in v3:
 - N/A

---

Changes in v2:
 - Updated to reference an offset inside of imem
---
 .../boot/dts/qcom/sm8150-microsoft-surface-duo.dts     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts b/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
index 5901c28e6696..abb7964daac4 100644
--- a/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
+++ b/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts
@@ -430,6 +430,16 @@ &i2c19 {
 	/* MAX34417 @ 0x1e */
 };
 
+&imem {
+	status = "okay";
+
+	xbl@a94 {
+		compatible = "microsoft,sm8150-surface-duo-xbl";
+		reg = <0xa94 0x100>;
+		status = "okay";
+	};
+};
+
 &pon {
 	pwrkey {
 		status = "okay";
-- 
2.25.1

Re: [PATCH v5 0/4] platform: surface: Introduce Surface XBL Driver

[Hans de Goede]

Hi Jarrett,

On 1/1/70 01:00, Jarrett Schultz wrote:
> After v4, there were some suggestions to change the driver again to use
> nvmem that would include some other changes to the sm8150 dtsi. While
> the suggestions make sense, this driver was supposed to remain simple
> for the introduction in order to get it into the tree and I think that
> it would be best to implement those and any other suggestions in a future
> patch. Hopefully this patch is now in a state where it can be accepted.
> Thanks to all who have helped and been patient along the way, this was
> my first patch :)
I appreciate your efforts to get this upstream, but this is not
how upstream development typically works. We usually iterate a patch
until all stakeholders are happy and then merge it.

So unless Rob changes its mind and gives his Reviewed-by for the
devicetree bits from this v5, then this cannot be merged as is.

Regards,

Hans



> ~ Jarrett
> 
> ---
> 
> Introduce the Surface Extensible Boot Loader driver for the Surface Duo.
> Exposes information about the driver to user space via sysfs for
> consumption in manufacturing mode.
> 
> ---
> 
> Changes in v5:
> 
>  - Minor changes to yaml
> 
> ---
> 
> Changes in v4:
> 
>  - Small binding definition changes
>  - Removed ACPI propagation from patch series since it has been
>    cherry-picked
>  - Fixed the Signed-off-by: and From: mismatch
> 
> ---
> 
> Changes in v3:
>  - For the yaml documentation:
>     * Updated description
>     * Fixed examples
>     * Updated 'required' field
>  - Further propogated ACPI dependency in Kconfigs
>  - Updated sysfs several binding descriptions
>  - Renamed files to conform to naming conventions
> 
> ---
> 
> Changes in v2:
>  - Per Maximilian, added patch 2: propagated ACPI dependency from the
>    directory as a whole to each individual driver
>  - For the yaml documentation:
>     * Removed json-schema dependence
>     * Elaborated on description of driver
>     * Updated example
>  - Changed target KernelVersion in sysfs documentation
>  - Updated MAINTAINER changes to be properly applied across patches
>  - For the driver itself,
>     * Added types.h inclusion and removed unused inclusions
>     * Minor updates to code and acronym style
>     * Remove __packed attribute on driver struct
>     * Use .dev_groups for sysfs
>  - Added more in-depth description of driver in Kconfig
>  - Modified dts to reference a newly added section in sm8150.dtsi
> 
> ---
> 
> Jarrett Schultz (4):
>   dt-bindings: platform: microsoft: Document surface xbl
>   platform: surface: Add surface xbl
>   arm64: dts: qcom: sm8150: Add imem section
>   arm64: dts: qcom: surface-duo: Add surface xbl
> 
>  .../ABI/testing/sysfs-platform-surface-xbl    |  79 ++++++++
>  .../platform/microsoft/surface-xbl.yaml       |  70 +++++++
>  MAINTAINERS                                   |   9 +
>  .../dts/qcom/sm8150-microsoft-surface-duo.dts |  10 +
>  arch/arm64/boot/dts/qcom/sm8150.dtsi          |   8 +
>  drivers/platform/surface/Kconfig              |  12 ++
>  drivers/platform/surface/Makefile             |   1 +
>  drivers/platform/surface/surface_xbl.c        | 186 ++++++++++++++++++
>  8 files changed, 375 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
>  create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
>  create mode 100644 drivers/platform/surface/surface_xbl.c
> 

Re: [PATCH v5 1/4] dt-bindings: platform: microsoft: Document surface xbl

[Rob Herring]

On Tue, Apr 05, 2022 at 02:07:47PM -0700, Jarrett Schultz wrote:
> From: Jarrett Schultz <jaschultz@microsoft.com>
> 
> Introduce yaml for surface xbl driver.

From Bjorn's reply on v4, it sounds like the QCom folks need to work out 
how to describe 'imem' first. I'd think that would use 'mmio-sram' 
binding and then this could be a child of that. If it's DDR, then it 
belongs under /reserved-memory node. Either way, that's all kind of 
outside the scope of the binding unless there's something special about 
'imem'.

> 
> Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> 
> ---
> 
> Changed in v5:
> 
>  - Updated description
>  - Added child node for imem
>  - Simplified example
> 
> ---
> 
> Changes in v4:
>  - Addressed small formatting changes
>  - Removed unnecessary lines
> 
> ---
> 
> Changes in v3:
>  - Updated description to only pertain to the hardware
>  - Updated the required field to properly reflect the binding
>  - Removed the first example
>  - Fixed the size of the reg field in the second example
> 
> ---
> 
> Changes in v2:
>  - Removed json-schema dependence
>  - Elaborated on description of driver
>  - Updated example
> ---
>  .../platform/microsoft/surface-xbl.yaml       | 70 +++++++++++++++++++

That's not a binding path. Probably bindings/nvmem/.

>  MAINTAINERS                                   |  7 ++
>  2 files changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> new file mode 100644
> index 000000000000..648476670bbd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/platform/microsoft/surface-xbl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Surface Extensible Bootloader for Microsoft Surface Duo
> +
> +maintainers:
> +  - Jarrett Schultz <jaschultz@microsoft.com>
> +
> +description: |
> +  Defined to expose information that is used during production when
> +  the device is in manufacturing mode. Some of the information included
> +  in the imem section is -
> +    * board_id
> +    * battery_present
> +    * hw_init_retries
> +    * is_customer_mode
> +    * is_act_mode
> +    * pmic_reset_reason
> +    * touch_fw_version
> +    * ocp_error_location
> +
> +properties:
> +  compatible:
> +    const: simple-mfd

This schema will never be applied (your example would fail if it did) 
because we filter out matching on 'simple-mfd' as it is not valid on its 
own.

> +
> +  reg:
> +    maxItems: 1
> +
> +  child-node:

This literally means you have a node called 'child-node'.

> +    description: A description of the xbl space within imem
> +
> +    type: object
> +
> +    properties:
> +      compatible: 
> +        const: microsoft,sm8150-surface-duo-xbl

This should be moved up a level removing 'simple-mfd'.

> +
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - ranges

> +  - address-cells
> +  - size-cells

Not documented and also the wrong names (missing '#').

> +
> +examples:
> +  - |
> +    imem@146bf000 {
> +      compatible = "simple-mfd";
> +      reg = <0x146bf000 0x1000>;
> +      ranges = <0x0 0x146bf000 0x1000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      xbl@a94 {
> +        compatible = "microsoft,sm8150-surface-duo-xbl";
> +        reg = <0xa94 0x100>;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 13f9a84a617e..5d0ca2a98b57 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12649,6 +12649,13 @@ 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@microsoft.com>
> +L:	linux-arm-msm@vger.kernel.org
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> +
>  MICROSOFT SURFACE GPE LID SUPPORT DRIVER
>  M:	Maximilian Luz <luzmaximilian@gmail.com>
>  L:	platform-driver-x86@vger.kernel.org
> -- 
> 2.25.1
> 
> 

Re: [PATCH v5 0/4] platform: surface: Introduce Surface XBL Driver

[Rob Herring]

On Mon, Apr 11, 2022 at 02:13:42PM +0200, Hans de Goede wrote:
> Hi Jarrett,
> 
> On 1/1/70 01:00, Jarrett Schultz wrote:
> > After v4, there were some suggestions to change the driver again to use
> > nvmem that would include some other changes to the sm8150 dtsi. While
> > the suggestions make sense, this driver was supposed to remain simple
> > for the introduction in order to get it into the tree and I think that
> > it would be best to implement those and any other suggestions in a future
> > patch. Hopefully this patch is now in a state where it can be accepted.
> > Thanks to all who have helped and been patient along the way, this was
> > my first patch :)
> I appreciate your efforts to get this upstream, but this is not
> how upstream development typically works. We usually iterate a patch
> until all stakeholders are happy and then merge it.
> 
> So unless Rob changes its mind and gives his Reviewed-by for the
> devicetree bits from this v5, then this cannot be merged as is.

This is dependent on the QCom folks. We can't really define the child 
without first defining the parent binding.

Rob

RE: [EXTERNAL] Re: [PATCH v5 1/4] dt-bindings: platform: microsoft: Document surface xbl

[Jarrett Schultz]

> From: Rob Herring <robh@kernel.org>
> On Wednesday, April 13, 2022 at 11:27 AM, Rob wrote:
> 
> On Tue, Apr 05, 2022 at 02:07:47PM -0700, Jarrett Schultz wrote:
> > From: Jarrett Schultz <jaschultz@microsoft.com>
> >
> > Introduce yaml for surface xbl driver.
> 
> From Bjorn's reply on v4, it sounds like the QCom folks need to work out how
> to describe 'imem' first. I'd think that would use 'mmio-sram'
> binding and then this could be a child of that. If it's DDR, then it belongs
> under /reserved-memory node. Either way, that's all kind of outside the
> scope of the binding unless there's something special about 'imem'.
> 
> >
> > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> >

Hi Rajendra and Vinod,

I'm currently working with Felipe Balbi on submitting an XBL driver that reads from imem.
Could you help me properly describe the 'imem' binding for sm8150?

Thank you,
Jarrett Schultz

RE: [EXTERNAL] Re: [PATCH v5 1/4] dt-bindings: platform: microsoft: Document surface xbl

[Jarrett Schultz]

> On Monday, April 18, 2022 3:53 PM, Jarrett Schultz wrote:
> From: Jarrett Schultz <jaschultz@microsoft.com>
>
> > From: Rob Herring <robh@kernel.org>
> > On Wednesday, April 13, 2022 at 11:27 AM, Rob wrote:
> >
> > On Tue, Apr 05, 2022 at 02:07:47PM -0700, Jarrett Schultz wrote:
> > > From: Jarrett Schultz <jaschultz@microsoft.com>
> > >
> > > Introduce yaml for surface xbl driver.
> >
> > From Bjorn's reply on v4, it sounds like the QCom folks need to work
> > out how to describe 'imem' first. I'd think that would use 'mmio-sram'
> > binding and then this could be a child of that. If it's DDR, then it
> > belongs under /reserved-memory node. Either way, that's all kind of
> > outside the scope of the binding unless there's something special about
> 'imem'.
> >
> > >
> > > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> > >
> 
> Hi Rajendra and Vinod,
> 
> I'm currently working with Felipe Balbi on submitting an XBL driver that reads
> from imem.
> Could you help me properly describe the 'imem' binding for sm8150?
> 
> Thank you,
> Jarrett Schultz

Oops, forgot to CC in Vinod and Rejendra.

-Jarrett