* [PATCH 0/3] platform: surface: Introduce Surface XBL Driver
@ 2021-10-28 21:17 Jarrett Schultz
2021-10-28 21:17 ` [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl Jarrett Schultz
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Jarrett Schultz @ 2021-10-28 21:17 UTC (permalink / raw)
To: Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede,
Mark Gross, Maximilian Luz
Cc: linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi,
Jarrett Schultz
Introduce the Surface Extensible Boot Loader driver for the Surface Duo.
Exposes information about the driver to user space via sysfs.
Jarrett Schultz (3):
dt-bindings: platform: microsoft: Document surface xbl
platform: surface: Add surface xbl
arm64: dts: qcom: surface-duo: Add surface xbl
.../ABI/testing/sysfs-platform-surface-xbl | 78 ++++++
.../platform/microsoft/surface-xbl.yaml | 37 +++
MAINTAINERS | 9 +
.../dts/qcom/sm8150-microsoft-surface-duo.dts | 6 +
drivers/platform/surface/Kconfig | 10 +
drivers/platform/surface/Makefile | 1 +
drivers/platform/surface/surface-xbl.c | 223 ++++++++++++++++++
7 files changed, 364 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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl 2021-10-28 21:17 [PATCH 0/3] platform: surface: Introduce Surface XBL Driver Jarrett Schultz @ 2021-10-28 21:17 ` Jarrett Schultz 2021-10-28 21:56 ` Rob Herring 2021-10-28 22:52 ` Bjorn Andersson 2021-10-28 21:17 ` [PATCH 2/3] platform: surface: Add " Jarrett Schultz 2021-10-28 21:17 ` [PATCH 3/3] arm64: dts: qcom: surface-duo: " Jarrett Schultz 2 siblings, 2 replies; 14+ messages in thread From: Jarrett Schultz @ 2021-10-28 21:17 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz Cc: linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi, Jarrett Schultz, Jarrett Schultz Introduce yaml for surface xbl driver. Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com> --- .../platform/microsoft/surface-xbl.yaml | 37 +++++++++++++++++++ 1 file changed, 37 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..3d2771322e72 --- /dev/null +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml @@ -0,0 +1,37 @@ +# SPDX-License-Identifier: GPL-2.0 +%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 <jaschultzMS@gmail.com> + +description: | + Exposes device information to user space. + +allOf: + - $ref: /schemas/platform/microsoft/surface-xbl.c# + +properties: + compatible: + const: microsoft,sm8150-surface-duo-xbl + + reg: + maxItems: 1 + +unevaluatedProperties: false + +required: + - compatible + - reg + - interrupts + +examples: + - | + xbl@146bfa94 { + compatible = "microsoft,sm8150-surface-duo-xbl"; + reg = <0x00 0x146bfa94 0x00 0x100>; + }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl 2021-10-28 21:17 ` [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl Jarrett Schultz @ 2021-10-28 21:56 ` Rob Herring 2021-10-28 22:52 ` Bjorn Andersson 1 sibling, 0 replies; 14+ messages in thread From: Rob Herring @ 2021-10-28 21:56 UTC (permalink / raw) To: Jarrett Schultz Cc: Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi, Jarrett Schultz On Thu, Oct 28, 2021 at 4:18 PM Jarrett Schultz <jaschultzms@gmail.com> wrote: > > Introduce yaml for surface xbl driver. What's surface? What's xbl? Bindings are for h/w devices, not drivers. Please send DT patches to the DT list. IOW, run get_maintainers.pl. > > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com> > --- > .../platform/microsoft/surface-xbl.yaml | 37 +++++++++++++++++++ > 1 file changed, 37 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..3d2771322e72 > --- /dev/null > +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml > @@ -0,0 +1,37 @@ > +# SPDX-License-Identifier: GPL-2.0 Dual license please. Run checkpatch.pl as that will tell you this. > +%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 <jaschultzMS@gmail.com> > + > +description: | > + Exposes device information to user space. What does that mean? > + > +allOf: > + - $ref: /schemas/platform/microsoft/surface-xbl.c# You have a C file with json-schema? > + > +properties: > + compatible: > + const: microsoft,sm8150-surface-duo-xbl > + > + reg: > + maxItems: 1 > + > +unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - interrupts > + > +examples: > + - | > + xbl@146bfa94 { > + compatible = "microsoft,sm8150-surface-duo-xbl"; > + reg = <0x00 0x146bfa94 0x00 0x100>; That's an odd address. Is this part of some other block? > + }; > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl 2021-10-28 21:17 ` [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl Jarrett Schultz 2021-10-28 21:56 ` Rob Herring @ 2021-10-28 22:52 ` Bjorn Andersson 1 sibling, 0 replies; 14+ messages in thread From: Bjorn Andersson @ 2021-10-28 22:52 UTC (permalink / raw) To: Jarrett Schultz Cc: Rob Herring, Andy Gross, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi, Jarrett Schultz On Thu 28 Oct 14:17 PDT 2021, Jarrett Schultz wrote: > Introduce yaml for surface xbl driver. > > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com> > --- > .../platform/microsoft/surface-xbl.yaml | 37 +++++++++++++++++++ > 1 file changed, 37 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..3d2771322e72 > --- /dev/null > +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml > @@ -0,0 +1,37 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%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 <jaschultzMS@gmail.com> > + > +description: | > + Exposes device information to user space. > + > +allOf: > + - $ref: /schemas/platform/microsoft/surface-xbl.c# > + > +properties: > + compatible: > + const: microsoft,sm8150-surface-duo-xbl > + > + reg: > + maxItems: 1 > + > +unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - interrupts > + > +examples: > + - | > + xbl@146bfa94 { > + compatible = "microsoft,sm8150-surface-duo-xbl"; > + reg = <0x00 0x146bfa94 0x00 0x100>; I believe this is in the middle of IMEM. If so it would be better to describe the entire IMEM section in one block and then list this as one of the pieces within. Please see e.g. imem@146aa000 in sc7180.dtsi Thanks, Bjorn > + }; > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] platform: surface: Add surface xbl 2021-10-28 21:17 [PATCH 0/3] platform: surface: Introduce Surface XBL Driver Jarrett Schultz 2021-10-28 21:17 ` [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl Jarrett Schultz @ 2021-10-28 21:17 ` Jarrett Schultz 2021-10-28 22:43 ` Randy Dunlap ` (2 more replies) 2021-10-28 21:17 ` [PATCH 3/3] arm64: dts: qcom: surface-duo: " Jarrett Schultz 2 siblings, 3 replies; 14+ messages in thread From: Jarrett Schultz @ 2021-10-28 21:17 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz Cc: linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi, Jarrett Schultz, Jarrett Schultz 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> --- .../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@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.15 +Contact: jaschultz@microsoft.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@microsoft.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@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.15 +Contact: jaschultz@microsoft.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@microsoft.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@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.15 +Contact: jaschultz@microsoft.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@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> L: platform-driver-x86@vger.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@microsoft.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@microsoft.com>"); +MODULE_DESCRIPTION("Surface Extensible Bootloader"); +MODULE_LICENSE("GPL"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] platform: surface: Add surface xbl 2021-10-28 21:17 ` [PATCH 2/3] platform: surface: Add " Jarrett Schultz @ 2021-10-28 22:43 ` Randy Dunlap 2021-10-28 23:12 ` Maximilian Luz [not found] ` <CAHp75Vfq7ZkXytuAFhGOMGuH7_AsXcYf9O=p30e4OUx+a4jMgw@mail.gmail.com> 2 siblings, 0 replies; 14+ messages in thread From: Randy Dunlap @ 2021-10-28 22:43 UTC (permalink / raw) To: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz Cc: linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi, Jarrett Schultz On 10/28/21 2:17 PM, Jarrett Schultz wrote: > 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. Hi, Can you tell the user/reader what XBL means, please? Don't assume that they will look at some documentation. thanks. -- ~Randy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] platform: surface: Add surface xbl 2021-10-28 21:17 ` [PATCH 2/3] platform: surface: Add " Jarrett Schultz 2021-10-28 22:43 ` Randy Dunlap @ 2021-10-28 23:12 ` Maximilian Luz [not found] ` <CAHp75Vfq7ZkXytuAFhGOMGuH7_AsXcYf9O=p30e4OUx+a4jMgw@mail.gmail.com> 2 siblings, 0 replies; 14+ messages in thread From: Maximilian Luz @ 2021-10-28 23:12 UTC (permalink / raw) To: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross Cc: linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi, Jarrett Schultz 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@microsoft.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@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.15 > +Contact: jaschultz@microsoft.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@microsoft.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@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.15 > +Contact: jaschultz@microsoft.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@microsoft.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@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.15 > +Contact: jaschultz@microsoft.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@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> > L: platform-driver-x86@vger.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@microsoft.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@microsoft.com>"); > +MODULE_DESCRIPTION("Surface Extensible Bootloader"); > +MODULE_LICENSE("GPL"); > ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAHp75Vfq7ZkXytuAFhGOMGuH7_AsXcYf9O=p30e4OUx+a4jMgw@mail.gmail.com>]
* Re: [PATCH 2/3] platform: surface: Add surface xbl [not found] ` <CAHp75Vfq7ZkXytuAFhGOMGuH7_AsXcYf9O=p30e4OUx+a4jMgw@mail.gmail.com> @ 2021-10-29 4:45 ` Felipe Balbi 2021-10-29 8:57 ` Andy Shevchenko 0 siblings, 1 reply; 14+ messages in thread From: Felipe Balbi @ 2021-10-29 4:45 UTC (permalink / raw) To: Andy Shevchenko Cc: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Jarrett Schultz Hi, Andy Shevchenko <andy.shevchenko@gmail.com> writes: > 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 > + * > > First of all, do not include filename in the file. > > Capital L will be better to read and understand the > abbreviation. Actually usually we do something like this: > > Extensible Boot Loader (EBL) nah, this is silly Andy. It's just capitalized as eXtensible Boot Loader, very much akin to eXtensible Host Controller Interface. > +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); > +} > > Either use corresponding io accessors in-line, or make first parameter > to be sirface_xbl pointer. Otherwise these helpers useless. I agree with passing surface_xbl point as first parameter, but calling the accessors pointless is a bit much. At a minimum, they make it easier to ftrace the entire driver by simply ftracing surface_xbl_* -- balbi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] platform: surface: Add surface xbl 2021-10-29 4:45 ` Felipe Balbi @ 2021-10-29 8:57 ` Andy Shevchenko 2021-10-29 12:32 ` Felipe Balbi 0 siblings, 1 reply; 14+ messages in thread From: Andy Shevchenko @ 2021-10-29 8:57 UTC (permalink / raw) To: Felipe Balbi Cc: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Jarrett Schultz On Fri, Oct 29, 2021 at 7:48 AM Felipe Balbi <balbi@kernel.org> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> writes: ... > > Capital L will be better to read and understand the > > abbreviation. Actually usually we do something like this: > > > > Extensible Boot Loader (EBL) > > nah, this is silly Andy. It's just capitalized as eXtensible Boot > Loader, very much akin to eXtensible Host Controller Interface. My point here is to have a full name followed by the abbreviation. and n(O)t in (F)ancy st(Y)le. ... > > +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); > > +} > > > > Either use corresponding io accessors in-line, or make first parameter > > to be sirface_xbl pointer. Otherwise these helpers useless. > > I agree with passing surface_xbl point as first parameter, but calling > the accessors pointless is a bit much. At a minimum, they make it easier > to ftrace the entire driver by simply ftracing surface_xbl_* My point is that the above seems half-baked. It's pointless to have a func(a,b) { return readl(a + b); }. It doesn't add value. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] platform: surface: Add surface xbl 2021-10-29 8:57 ` Andy Shevchenko @ 2021-10-29 12:32 ` Felipe Balbi 2021-10-29 12:56 ` Andy Shevchenko 0 siblings, 1 reply; 14+ messages in thread From: Felipe Balbi @ 2021-10-29 12:32 UTC (permalink / raw) To: Andy Shevchenko Cc: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Jarrett Schultz Hi, Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Fri, Oct 29, 2021 at 7:48 AM Felipe Balbi <balbi@kernel.org> wrote: >> Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > ... > >> > Capital L will be better to read and understand the >> > abbreviation. Actually usually we do something like this: >> > >> > Extensible Boot Loader (EBL) >> >> nah, this is silly Andy. It's just capitalized as eXtensible Boot >> Loader, very much akin to eXtensible Host Controller Interface. > > My point here is to have a full name followed by the abbreviation. and > n(O)t in (F)ancy st(Y)le. too bad my patch removing acronyms from the kernel got rejects :-p Seriously, this is pretty pointless. You're vouching for something that will just cause confusion. Every piece of internal documentation refers to xbl and you want this to be renamed to ebl because it looks nicer for you. Thanks, but no thanks. >> > +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); >> > +} >> > >> > Either use corresponding io accessors in-line, or make first parameter >> > to be sirface_xbl pointer. Otherwise these helpers useless. >> >> I agree with passing surface_xbl point as first parameter, but calling >> the accessors pointless is a bit much. At a minimum, they make it easier >> to ftrace the entire driver by simply ftracing surface_xbl_* > > My point is that the above seems half-baked. It's pointless to have a > func(a,b) { return readl(a + b); }. It doesn't add value. sure it does. echo surface_xbl_* > ftrace_filter_function (or whatever the filename was) it reason enough IMHO. Not to mention that these little accessors will likely be optimized by the compiler. -- balbi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] platform: surface: Add surface xbl 2021-10-29 12:32 ` Felipe Balbi @ 2021-10-29 12:56 ` Andy Shevchenko 2021-10-29 13:12 ` Felipe Balbi 0 siblings, 1 reply; 14+ messages in thread From: Andy Shevchenko @ 2021-10-29 12:56 UTC (permalink / raw) To: Felipe Balbi Cc: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Jarrett Schultz On Fri, Oct 29, 2021 at 3:34 PM Felipe Balbi <balbi@kernel.org> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > On Fri, Oct 29, 2021 at 7:48 AM Felipe Balbi <balbi@kernel.org> wrote: > >> Andy Shevchenko <andy.shevchenko@gmail.com> writes: ... > >> > Capital L will be better to read and understand the > >> > abbreviation. Actually usually we do something like this: > >> > > >> > Extensible Boot Loader (EBL) > >> > >> nah, this is silly Andy. It's just capitalized as eXtensible Boot > >> Loader, very much akin to eXtensible Host Controller Interface. > > > > My point here is to have a full name followed by the abbreviation. and > > n(O)t in (F)ancy st(Y)le. > > too bad my patch removing acronyms from the kernel got rejects :-p > > Seriously, this is pretty pointless. You're vouching for something that > will just cause confusion. Every piece of internal documentation refers > to xbl and you want this to be renamed to ebl because it looks nicer for > you. Thanks, but no thanks. Maybe I was too unclear. I'm not pushing for EBL, I'm pushing for the form os "Foo bAr BullSh*t (FABS)" vs. "(F)oo b(a)r (B)ull(s)h*t". If you have x there to be capitalized, do it like "eXtensible Boot Loader (XBL)". Is it too hard? ... > >> > +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); > >> > +} > >> > > >> > Either use corresponding io accessors in-line, or make first parameter > >> > to be sirface_xbl pointer. Otherwise these helpers useless. > >> > >> I agree with passing surface_xbl point as first parameter, but calling > >> the accessors pointless is a bit much. At a minimum, they make it easier > >> to ftrace the entire driver by simply ftracing surface_xbl_* > > > > My point is that the above seems half-baked. It's pointless to have a > > func(a,b) { return readl(a + b); }. It doesn't add value. > > sure it does. echo surface_xbl_* > ftrace_filter_function (or whatever > the filename was) it reason enough IMHO. Not to mention that these > little accessors will likely be optimized by the compiler. readl() will appear in the traces, no? But yeah I also was thinking about the weakness in your argument that the compiler can silently inline them anyway. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] platform: surface: Add surface xbl 2021-10-29 12:56 ` Andy Shevchenko @ 2021-10-29 13:12 ` Felipe Balbi 2021-10-29 15:02 ` Andy Shevchenko 0 siblings, 1 reply; 14+ messages in thread From: Felipe Balbi @ 2021-10-29 13:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Jarrett Schultz Hi, Andy Shevchenko <andy.shevchenko@gmail.com> writes: >> >> > Capital L will be better to read and understand the >> >> > abbreviation. Actually usually we do something like this: >> >> > >> >> > Extensible Boot Loader (EBL) >> >> >> >> nah, this is silly Andy. It's just capitalized as eXtensible Boot >> >> Loader, very much akin to eXtensible Host Controller Interface. >> > >> > My point here is to have a full name followed by the abbreviation. and >> > n(O)t in (F)ancy st(Y)le. >> >> too bad my patch removing acronyms from the kernel got rejects :-p >> >> Seriously, this is pretty pointless. You're vouching for something that >> will just cause confusion. Every piece of internal documentation refers >> to xbl and you want this to be renamed to ebl because it looks nicer for >> you. Thanks, but no thanks. > > Maybe I was too unclear. I'm not pushing for EBL, I'm pushing for the form os > > "Foo bAr BullSh*t (FABS)" vs. "(F)oo b(a)r (B)ull(s)h*t". > > If you have x there to be capitalized, do it like "eXtensible Boot > Loader (XBL)". Is it too hard? Take a breather Andy, you need it. Winter sure is coming >> >> > +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); >> >> > +} >> >> > >> >> > Either use corresponding io accessors in-line, or make first parameter >> >> > to be sirface_xbl pointer. Otherwise these helpers useless. >> >> >> >> I agree with passing surface_xbl point as first parameter, but calling >> >> the accessors pointless is a bit much. At a minimum, they make it easier >> >> to ftrace the entire driver by simply ftracing surface_xbl_* >> > >> > My point is that the above seems half-baked. It's pointless to have a >> > func(a,b) { return readl(a + b); }. It doesn't add value. >> >> sure it does. echo surface_xbl_* > ftrace_filter_function (or whatever >> the filename was) it reason enough IMHO. Not to mention that these >> little accessors will likely be optimized by the compiler. > > readl() will appear in the traces, no? But yeah I also was thinking > about the weakness in your argument that the compiler can silently > inline them anyway. In non-debug builds, when tracers are enabled a thunk will be added for runtime patching ;-) (IIRC) -- balbi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] platform: surface: Add surface xbl 2021-10-29 13:12 ` Felipe Balbi @ 2021-10-29 15:02 ` Andy Shevchenko 0 siblings, 0 replies; 14+ messages in thread From: Andy Shevchenko @ 2021-10-29 15:02 UTC (permalink / raw) To: Felipe Balbi Cc: Jarrett Schultz, Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz, linux-arm-msm, platform-driver-x86, linux-kernel, Jarrett Schultz On Fri, Oct 29, 2021 at 4:14 PM Felipe Balbi <balbi@kernel.org> wrote: > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > Take a breather Andy, you need it. Winter sure is coming Indeed. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] arm64: dts: qcom: surface-duo: Add surface xbl 2021-10-28 21:17 [PATCH 0/3] platform: surface: Introduce Surface XBL Driver Jarrett Schultz 2021-10-28 21:17 ` [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl Jarrett Schultz 2021-10-28 21:17 ` [PATCH 2/3] platform: surface: Add " Jarrett Schultz @ 2021-10-28 21:17 ` Jarrett Schultz 2 siblings, 0 replies; 14+ messages in thread From: Jarrett Schultz @ 2021-10-28 21:17 UTC (permalink / raw) To: Rob Herring, Andy Gross, Bjorn Andersson, Hans de Goede, Mark Gross, Maximilian Luz Cc: linux-arm-msm, platform-driver-x86, linux-kernel, Felipe Balbi, Jarrett Schultz, Jarrett Schultz Introduce device tree source for the surface xbl driver. Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com> --- arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts | 6 ++++++ 1 file changed, 6 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 736da9af44e0..9fd9f733a791 100644 --- a/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts +++ b/arch/arm64/boot/dts/qcom/sm8150-microsoft-surface-duo.dts @@ -57,6 +57,12 @@ vol_up { linux,code = <KEY_VOLUMEUP>; }; }; + + xbl@146bfa94 { + compatible = "microsoft,sm8150-surface-duo-xbl"; + reg = <0x00 0x146bfa94 0x00 0x100>; + status = "okay"; + }; }; &apps_rsc { -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-10-29 15:03 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-28 21:17 [PATCH 0/3] platform: surface: Introduce Surface XBL Driver Jarrett Schultz
2021-10-28 21:17 ` [PATCH 1/3] dt-bindings: platform: microsoft: Document surface xbl Jarrett Schultz
2021-10-28 21:56 ` Rob Herring
2021-10-28 22:52 ` Bjorn Andersson
2021-10-28 21:17 ` [PATCH 2/3] platform: surface: Add " Jarrett Schultz
2021-10-28 22:43 ` Randy Dunlap
2021-10-28 23:12 ` Maximilian Luz
[not found] ` <CAHp75Vfq7ZkXytuAFhGOMGuH7_AsXcYf9O=p30e4OUx+a4jMgw@mail.gmail.com>
2021-10-29 4:45 ` Felipe Balbi
2021-10-29 8:57 ` Andy Shevchenko
2021-10-29 12:32 ` Felipe Balbi
2021-10-29 12:56 ` Andy Shevchenko
2021-10-29 13:12 ` Felipe Balbi
2021-10-29 15:02 ` Andy Shevchenko
2021-10-28 21:17 ` [PATCH 3/3] arm64: dts: qcom: surface-duo: " Jarrett Schultz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.