[RFC,1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

Commit Message

Sasha Finkelstein via B4 Relay Feb. 24, 2023, 10:20 a.m. UTC

From: Sasha Finkelstein <[e-mail]>

Add bindings for touchscreen controllers attached using the Z2 protocol.
Those are present in most Apple devices.

Signed-off-by: Sasha Finkelstein <[e-mail]>
---
 .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
 1 file changed, 81 insertions(+)

Comments

Mark Kettenis Feb. 24, 2023, 10:55 a.m. UTC | #1

> Date: Fri, 24 Feb 2023 11:20:06 +0100
> From: Sasha Finkelstein <[e-mail]>

Hi Sasha,

> Add bindings for touchscreen controllers attached using the Z2 protocol.
> Those are present in most Apple devices.
> 
> Signed-off-by: Sasha Finkelstein <[e-mail]>
> ---
>  .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> new file mode 100644
> index 000000000000..695594494b1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple touchscreens attached using the Z2 protocol.
> +
> +maintainers:
> +  - [e-mail]
> +  - Sasha Finkelstein <[e-mail]>
> +
> +description: A series of touschscreen controllers used in Apple products.
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: apple,z2-touchscreen
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:
> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1

What is the motivation for including the firmware name in the device
tree rather than constructing it in the driver like what is done for
the broadcom wireless?

In your example the firmware name includes the directory name.  I
think that is a bad idea since it makes assumptions about the
directory layout used for the firmware files on the OS level.  And in
particular, forcing the firmware to be in a subdirectory named "apple"
would be awkward for the way we handle firmware in OpenBSD.

> +
> +  apple,z2-device-name:
> +    description: The name to be used for the input device
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true
> +  spi-max-frequency: true
> +
> +required:
> +  - compatible
> +  - interrupts-extended
> +  - reset-gpios
> +  - cs-gpios
> +  - firmware-name
> +  - apple,z2-device-name
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            touchscreen@0 {
> +                    compatible = "apple,z2-touchscreen";
> +                    reg = <0>;
> +                    spi-max-frequency = <11500000>;
> +                    reset-gpios = <&pinctrl_ap 139 0>;
> +                    cs-gpios = <&pinctrl_ap 109 0>;
> +                    interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> +                    firmware-name = "apple/dfrmtfw-j293.bin";
> +                    touchscreen-size-x = <23045>;
> +                    touchscreen-size-y = <640>;
> +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
> +            };
> +    };
> +
> +...
> 
> -- 
> Git-137.1)
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Sven Peter Feb. 24, 2023, 11:03 a.m. UTC | #2

On Fri, Feb 24, 2023, at 11:20, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <[e-mail]>
>
> Add bindings for touchscreen controllers attached using the Z2 protocol.
> Those are present in most Apple devices.
>
> Signed-off-by: Sasha Finkelstein <[e-mail]>
> ---
>  .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>
> diff --git 
> a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml 
> b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> new file mode 100644
> index 000000000000..695594494b1e
> --- /dev/null
> +++ 
> b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: 
> http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple touchscreens attached using the Z2 protocol.
> +
> +maintainers:
> +  - [e-mail]
> +  - Sasha Finkelstein <[e-mail]>
> +
> +description: A series of touschscreen controllers used in Apple 
> products.
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: apple,z2-touchscreen
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:
> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1
> +
> +  apple,z2-device-name:
> +    description: The name to be used for the input device
> +    $ref: /schemas/types.yaml#/definitions/string

Now that I thought about this again after the brief discussion we already had:
Do we even need to specify the device name? Is there any reason we can't just
always use something like "Apple Z2 TouchBar"?



Best,


Sven

Sasha Finkelstein Feb. 24, 2023, 11:04 a.m. UTC | #3

On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <[e-mail]> wrote:

> What is the motivation for including the firmware name in the device
> tree rather than constructing it in the driver like what is done for
> the broadcom wireless?
There is no way to identify the device subtype before the firmware is
uploaded, and so i need some way of figuring out which firmware to use.

Sven Peter Feb. 24, 2023, 11:08 a.m. UTC | #4

Hi,


On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <[e-mail]> wrote:
>
>> What is the motivation for including the firmware name in the device
>> tree rather than constructing it in the driver like what is done for
>> the broadcom wireless?
> There is no way to identify the device subtype before the firmware is
> uploaded, and so i need some way of figuring out which firmware to use.

Some Broadcom bluetooth boards use the compatible of the root node (see
btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
which marcan had to extend to instead of "brcm,board-type" because different
WiFi boards can me matched to different Apple Silicon boards. I don't think
that's the case for this touchscreen though.



Sven

Sasha Finkelstein Feb. 24, 2023, 11:08 a.m. UTC | #5

On Fri, 24 Feb 2023 at 12:04, Sven Peter <[e-mail]> wrote:
> Now that I thought about this again after the brief discussion we already had:
> Do we even need to specify the device name? Is there any reason we can't just
> always use something like "Apple Z2 TouchBar"?
A similar protocol is used for primary touchscreen on idevices, which
need different
userspace handling. This is to make the driver potentially useful for
people who run
linux on checkra1n-able devices.

Rob Herring Feb. 27, 2023, 7:51 p.m. UTC | #6

On Fri, Feb 24, 2023 at 11:20:06AM +0100, Sasha Finkelstein wrote:
> Add bindings for touchscreen controllers attached using the Z2 protocol.
> Those are present in most Apple devices.
> 
> Signed-off-by: Sasha Finkelstein <[e-mail]>
> ---
>  .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> new file mode 100644
> index 000000000000..695594494b1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple touchscreens attached using the Z2 protocol.
> +
> +maintainers:
> +  - [e-mail]
> +  - Sasha Finkelstein <[e-mail]>
> +
> +description: A series of touschscreen controllers used in Apple products.
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: apple,z2-touchscreen

Is 'z2' anything other than a touchscreen? If not, '-touchscreen' is 
redundant. If so, then what else is there? You should be describing 
physical devices, not just a protocol for touchscreen.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:

Just 'interrupts' here. 'interrupts-extended' is implicitly supported.

> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:

There is a standard way to do GPIO based chip-selects. It happens to be 
'cs-gpios', but this is in the wrong place. It goes in the SPI 
controller node.

> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1
> +
> +  apple,z2-device-name:
> +    description: The name to be used for the input device
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true
> +  spi-max-frequency: true
> +
> +required:
> +  - compatible
> +  - interrupts-extended
> +  - reset-gpios
> +  - cs-gpios
> +  - firmware-name
> +  - apple,z2-device-name
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;

4 space indentation is preferred here.

> +
> +            touchscreen@0 {
> +                    compatible = "apple,z2-touchscreen";
> +                    reg = <0>;
> +                    spi-max-frequency = <11500000>;
> +                    reset-gpios = <&pinctrl_ap 139 0>;
> +                    cs-gpios = <&pinctrl_ap 109 0>;
> +                    interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> +                    firmware-name = "apple/dfrmtfw-j293.bin";
> +                    touchscreen-size-x = <23045>;
> +                    touchscreen-size-y = <640>;
> +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";

Why do we need this string? If you want a human consumed label for 
some identification, we have a property for that purpose. It's called 
'label'. But when there is only 1 instance, I don't really see the 
point.

Rob

Sasha Finkelstein Feb. 27, 2023, 8:06 p.m. UTC | #7

On Mon, 27 Feb 2023 at 20:51, Rob Herring <[e-mail]> wrote:
>
> > +properties:
> > +  compatible:
> > +    const: apple,z2-touchscreen
>
> Is 'z2' anything other than a touchscreen? If not, '-touchscreen' is
> redundant. If so, then what else is there? You should be describing
> physical devices, not just a protocol for touchscreen.
>

This is a class of touchscreen controllers that talk the z2 protocol
over spi.

> > +                    touchscreen-size-y = <640>;
> > +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
>
> Why do we need this string? If you want a human consumed label for
> some identification, we have a property for that purpose. It's called
> 'label'. But when there is only 1 instance, I don't really see the
> point.

I want a libinput-consumed label to distinguish between devices
using this protocol. It is used both for 'normal' touchscreens, and,
as is in this example a 'touchbar', which absolutely should not be
treated as a normal touchscreen, and needs special handling in
userspace.

Rob Herring Feb. 27, 2023, 10:14 p.m. UTC | #8

On Mon, Feb 27, 2023 at 09:06:28PM +0100, Sasha Finkelstein wrote:
> On Mon, 27 Feb 2023 at 20:51, Rob Herring <[e-mail]> wrote:
> >
> > > +properties:
> > > +  compatible:
> > > +    const: apple,z2-touchscreen
> >
> > Is 'z2' anything other than a touchscreen? If not, '-touchscreen' is
> > redundant. If so, then what else is there? You should be describing
> > physical devices, not just a protocol for touchscreen.
> >
> 
> This is a class of touchscreen controllers that talk the z2 protocol
> over spi.

Yes, you already said that much. So nothing else for this piece of h/w? 
Then 'apple,z2' is sufficient. Well maybe. You are assuming all h/w in 
the world speaking 'z2' is the same (to software). Usually that's not a 
safe assumption, but maybe Apple is better at not changing the h/w...

Normally, the 'protocol' to talk to a device is only part of it. There's 
other pieces like how to turn the device on and off which need h/w 
specific knowledge. If you need any of that, then you need specific 
compatibles. Adding properties for each variation doesn't end up well.


> 
> > > +                    touchscreen-size-y = <640>;
> > > +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
> >
> > Why do we need this string? If you want a human consumed label for
> > some identification, we have a property for that purpose. It's called
> > 'label'. But when there is only 1 instance, I don't really see the
> > point.
> 
> I want a libinput-consumed label to distinguish between devices
> using this protocol. 

I know little about libinput, but how would it know about 
'apple,z2-device-name'?

> It is used both for 'normal' touchscreens, and,
> as is in this example a 'touchbar', which absolutely should not be
> treated as a normal touchscreen, and needs special handling in
> userspace.

Meaning there are both touchscreens and touchbars using this? That 
sounds like s/w needs this information. From a DT perspective, 
'compatible' is how DT defines exactly what the h/w is and how to use 
it. That also doesn't sound like a unique issue. Doesn't the kernel 
provide a standard way to tell userspace what's a touchscreen vs. 
touchpad vs. ???

Rob

Rob Herring Feb. 27, 2023, 10:23 p.m. UTC | #9

On Fri, Feb 24, 2023 at 12:08:42PM +0100, Sasha Finkelstein wrote:
> On Fri, 24 Feb 2023 at 12:04, Sven Peter <[e-mail]> wrote:
> > Now that I thought about this again after the brief discussion we already had:
> > Do we even need to specify the device name? Is there any reason we can't just
> > always use something like "Apple Z2 TouchBar"?
> A similar protocol is used for primary touchscreen on idevices, which

Similar? So close, but somehow different.

> need different
> userspace handling. This is to make the driver potentially useful for
> people who run
> linux on checkra1n-able devices.

It is sounding to me like you need different compatibles. Don't try to 
parameterize the differences with properties. Unless you can define all 
the differences up front (hint: you can't).

Rob

Sasha Finkelstein Feb. 27, 2023, 10:25 p.m. UTC | #10

On Mon, 27 Feb 2023 at 23:14, Rob Herring <[e-mail]> wrote:
> I know little about libinput, but how would it know about
> 'apple,z2-device-name'?
>
The idea was to forward the contents of this property
into the input device name.

Hector Martin Feb. 28, 2023, 2:58 a.m. UTC | #11

On 24/02/2023 20.08, Sven Peter wrote:
> Hi,
> 
> 
> On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
>> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <[e-mail]> wrote:
>>
>>> What is the motivation for including the firmware name in the device
>>> tree rather than constructing it in the driver like what is done for
>>> the broadcom wireless?
>> There is no way to identify the device subtype before the firmware is
>> uploaded, and so i need some way of figuring out which firmware to use.
> 
> Some Broadcom bluetooth boards use the compatible of the root node (see
> btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
> for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
> which marcan had to extend to instead of "brcm,board-type" because different
> WiFi boards can me matched to different Apple Silicon boards. I don't think
> that's the case for this touchscreen though.

The reason why the brcmfmac stuff needs to construct the firmware name
itself is that parts of it come from the OTP contents, so there is no
way to know from the bootloader what the right firmware is.

That is not the case here, so it makes perfect sense to specify the
firmware with `firmware-name` (which is a standard DT property).

As for the layout, both bare names and paths are in common use:

qcom/sm8450-qrd.dts:    firmware-name = "qcom/sm8450/slpi.mbn";
ti/k3-am64-main.dtsi:   firmware-name = "am64-main-r5f0_0-fw";

... but the bare names in particular, judging by some Google searches,
are *actually* mapped to bare files in /lib/firmware anyway. So the
firmware-name property contains the firmware path in the linux-firmware
standard hierarchy, in every case.

I already did the same thing for the touchpad on M2s (which requires
analogous Z2 firmware passed to it, just in a different format):

dts/apple/t8112-j413.dts: firmware-name = "apple/tpmtfw-j413.bin";

Why is having a directory a problem for OpenBSD? Regardless of how
firmware is handled behind the scenes, it seems logical to organize it
by vendor somehow. It seems to me that gratuitously diverging from the
standard firmware hierarchy is only going to cause trouble for OpenBSD.
Obviously it's fine to store it somewhere other than /lib/firmware or
use a completely unrelated mechanism other than files, but why does the
*organization* of the firmware have to diverge? There can only be one DT
binding, so we need to agree on a way of specifying firmwares that works
cross-OS, and I don't see why "apple/foo.bin" couldn't be made to work
for everyone in some way or another.

- Hector

Hector Martin Feb. 28, 2023, 3:05 a.m. UTC | #12

On 28/02/2023 07.25, Sasha Finkelstein wrote:
> On Mon, 27 Feb 2023 at 23:14, Rob Herring <[e-mail]> wrote:
>> I know little about libinput, but how would it know about
>> 'apple,z2-device-name'?
>>
> The idea was to forward the contents of this property
> into the input device name.

Then you want "label", as Rob said.

But I also agree with Rob that we want per-device compatibles, even if
we don't use them upfront in the driver. That's what we do everywhere
else, and it has served us well. I suggest this hierarchy:

compatible = "apple,j293-touchbar", "apple,z2-touchbar",
"apple,z2-multitouch";
label = "Apple J293 Touch Bar";

Then let's say a hypothetical touchscreen + touchbar MacBook comes out,
we end up with:

compatible = "apple,j789-touchbar", "apple,z2-touchbar",
"apple,z2-multitouch";
label = "Apple J789 Touch Bar";

compatible = "apple,j789-touchscreen", "apple,z2-touchscreen",
"apple,z2-multitouch";
label = "Apple J789 Touchscreen";

And it all is nicely future-proof. If libinput needs a hint other than
the device name to figure out what should be treated as a touchscreen or
not, the driver can use the "apple,z2-touchbar" vs
"apple,z2-touchscreen" distinction level for that. And if per-device
quirks are never necessary, we just ignore the model number compatible,
which is already what we do all over the place in other drivers (but the
day it becomes necessary, it's ready for us). And if it turns out we
don't need any of this for a while, the driver can just bind to
"apple,z2-multitouch" and call it a day.

- Hector

Mark Kettenis Feb. 28, 2023, 8:53 p.m. UTC | #13

> Date: Tue, 28 Feb 2023 11:58:28 +0900
> From: Hector Martin <[e-mail]>
> 
> On 24/02/2023 20.08, Sven Peter wrote:
> > Hi,
> > 
> > 
> > On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
> >> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <[e-mail]> wrote:
> >>
> >>> What is the motivation for including the firmware name in the device
> >>> tree rather than constructing it in the driver like what is done for
> >>> the broadcom wireless?
> >> There is no way to identify the device subtype before the firmware is
> >> uploaded, and so i need some way of figuring out which firmware to use.
> > 
> > Some Broadcom bluetooth boards use the compatible of the root node (see
> > btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
> > for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
> > which marcan had to extend to instead of "brcm,board-type" because different
> > WiFi boards can me matched to different Apple Silicon boards. I don't think
> > that's the case for this touchscreen though.
> 
> The reason why the brcmfmac stuff needs to construct the firmware name
> itself is that parts of it come from the OTP contents, so there is no
> way to know from the bootloader what the right firmware is.

The name of the "nvram" file is constructed as well, and that uses the
compatible of the machine (the root of the device tree).  I suppose
what is special in that case is that several files are tried so a
single 'firmware-name" property wouldn't cut it.

> That is not the case here, so it makes perfect sense to specify the
> firmware with `firmware-name` (which is a standard DT property).

It certainly provides the flexibility to cater for all potential
nonsense names Apple comes up with for future hardware.

> As for the layout, both bare names and paths are in common use:
> 
> qcom/sm8450-qrd.dts:    firmware-name = "qcom/sm8450/slpi.mbn";
> ti/k3-am64-main.dtsi:   firmware-name = "am64-main-r5f0_0-fw";
> 
> ... but the bare names in particular, judging by some Google searches,
> are *actually* mapped to bare files in /lib/firmware anyway. So the
> firmware-name property contains the firmware path in the linux-firmware
> standard hierarchy, in every case.

Well, I think the device tree should not be tied to a particular OS
and therefore not be tied to things like linux-firmware.

> I already did the same thing for the touchpad on M2s (which requires
> analogous Z2 firmware passed to it, just in a different format):
> 
> dts/apple/t8112-j413.dts: firmware-name = "apple/tpmtfw-j413.bin";
> 
> Why is having a directory a problem for OpenBSD? Regardless of how
> firmware is handled behind the scenes, it seems logical to organize it
> by vendor somehow. It seems to me that gratuitously diverging from the
> standard firmware hierarchy is only going to cause trouble for OpenBSD.
> Obviously it's fine to store it somewhere other than /lib/firmware or
> use a completely unrelated mechanism other than files, but why does the
> *organization* of the firmware have to diverge? There can only be one DT
> binding, so we need to agree on a way of specifying firmwares that works
> cross-OS, and I don't see why "apple/foo.bin" couldn't be made to work
> for everyone in some way or another.

We organize the firmware by driver.  And driver names in *BSD differ
from Linux since there are different constraints.  The firmware is
organized by driver because we have separate firmware packages for
each driver that get installed as-needed by a tool that matches on the
driver name.

Rather than have the device tree dictate the layout of the firmware
files, I think it would be better to have the OS driver prepend the
directory to match the convention of the OS in question.  This is what
we typically do in OpenBSD.

Now I did indeed forget about the "dockchannel" touchpad firmware that
I already handle in OpenBSD.  That means I could handle the touchbar
firmware in the same way.  But that is mostly because these firmwares
are non-distributable, so we don't have firmware packages for them.
Instead we rely on the Asahi installer to make the firmware available
on the EFI partition and the OpenBSD installer to move the firmware in
place on the root filesystem.

So this isn't a big issue.

Cheers,

Mark

Hector Martin March 1, 2023, 3:13 a.m. UTC | #14

On 01/03/2023 05.53, Mark Kettenis wrote:
>> Date: Tue, 28 Feb 2023 11:58:28 +0900
>> From: Hector Martin <[e-mail]>
>>
>> On 24/02/2023 20.08, Sven Peter wrote:
>>> Hi,
>>>
>>>
>>> On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
>>>> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <[e-mail]> wrote:
>>>>
>>>>> What is the motivation for including the firmware name in the device
>>>>> tree rather than constructing it in the driver like what is done for
>>>>> the broadcom wireless?
>>>> There is no way to identify the device subtype before the firmware is
>>>> uploaded, and so i need some way of figuring out which firmware to use.
>>>
>>> Some Broadcom bluetooth boards use the compatible of the root node (see
>>> btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
>>> for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
>>> which marcan had to extend to instead of "brcm,board-type" because different
>>> WiFi boards can me matched to different Apple Silicon boards. I don't think
>>> that's the case for this touchscreen though.
>>
>> The reason why the brcmfmac stuff needs to construct the firmware name
>> itself is that parts of it come from the OTP contents, so there is no
>> way to know from the bootloader what the right firmware is.
> 
> The name of the "nvram" file is constructed as well, and that uses the
> compatible of the machine (the root of the device tree).  I suppose
> what is special in that case is that several files are tried so a
> single 'firmware-name" property wouldn't cut it.

No, if you look at the way the name is constructed, some of it comes
from OTP. The plain compatible stuff is for non-Apple platforms. Apple
platforms need lookup of nvram/etc per specific fields in the OTP, and
then we try multiple firmware names from most to least specific because
the distinction often isn't relevant (but in some cases it is, and this
even changes from macOS version to macOS version). Our firmware
extractor actually attempts to prune the firmware tree by deduplicating
and promoting the most popular variant up towards the root then pruning
redundant branches, because otherwise we'd end up with hundreds of
copies or links (which is what macOS does, they don't try multiple
firmware filenames).

If the OTP were easily readable from the bootloader I'd just have thrown
this in m1n1 and kept a fixed firmware-name property, but that involves
full PCIe init and power-up of the wlan module and that's way too much
junk to put in there. Hence, dynamically computing firmware names in the
kernel driver.

If it were a simple 1:1 mapping from device tree blob to firmware files,
I would certainly have advocated for "firmware-name" instead of the more
complex thing we do now.

>> That is not the case here, so it makes perfect sense to specify the
>> firmware with `firmware-name` (which is a standard DT property).
> 
> It certainly provides the flexibility to cater for all potential
> nonsense names Apple comes up with for future hardware.

We actually make up the firmware names ourselves in the extractor, so
that's not the reason. But if nothing else I'm pretty sure we already
have n:1 mappings (M2 Pro/Max laptops almost certainly share the same
touchpad firmware for at least the same size chassis models, if not all
4 - haven't looked at that yet though), so using a separate property
means we don't have to play symlink/hardlink games.

> 
>> As for the layout, both bare names and paths are in common use:
>>
>> qcom/sm8450-qrd.dts:    firmware-name = "qcom/sm8450/slpi.mbn";
>> ti/k3-am64-main.dtsi:   firmware-name = "am64-main-r5f0_0-fw";
>>
>> ... but the bare names in particular, judging by some Google searches,
>> are *actually* mapped to bare files in /lib/firmware anyway. So the
>> firmware-name property contains the firmware path in the linux-firmware
>> standard hierarchy, in every case.
> 
> Well, I think the device tree should not be tied to a particular OS
> and therefore not be tied to things like linux-firmware.

That's fine, but we need *some* source of truth, and just like the Linux
kernel tree is the system of record for device tree bindings today, I
don't see a good reason not to use linux-firmware as the defacto
standard for firmware organization. There's nothing OS-specific about,
effectively, a list of identifiers that particular firmwares should be
listed under. Think of it as a "path key => expected firmware blob"
mapping. How each OS implements that is up to the OS.

This is similar to the whole vendorfw mechanism I constructed for these
platforms. Sure, it's based on Linuxisms, but the whole thing is trivial
enough to reimplement on any OS without much trouble (just a
fixed-format CPIO archive in the ESP at a known path).

>> I already did the same thing for the touchpad on M2s (which requires
>> analogous Z2 firmware passed to it, just in a different format):
>>
>> dts/apple/t8112-j413.dts: firmware-name = "apple/tpmtfw-j413.bin";
>>
>> Why is having a directory a problem for OpenBSD? Regardless of how
>> firmware is handled behind the scenes, it seems logical to organize it
>> by vendor somehow. It seems to me that gratuitously diverging from the
>> standard firmware hierarchy is only going to cause trouble for OpenBSD.
>> Obviously it's fine to store it somewhere other than /lib/firmware or
>> use a completely unrelated mechanism other than files, but why does the
>> *organization* of the firmware have to diverge? There can only be one DT
>> binding, so we need to agree on a way of specifying firmwares that works
>> cross-OS, and I don't see why "apple/foo.bin" couldn't be made to work
>> for everyone in some way or another.
> 
> We organize the firmware by driver.  And driver names in *BSD differ
> from Linux since there are different constraints.  The firmware is
> organized by driver because we have separate firmware packages for
> each driver that get installed as-needed by a tool that matches on the
> driver name.

That's fair, but you can still have another level of hierarchy after the
driver, no? Or just throw away that level when you parse the
`firmware-name` if you prefer.

> Rather than have the device tree dictate the layout of the firmware
> files, I think it would be better to have the OS driver prepend the
> directory to match the convention of the OS in question.  This is what
> we typically do in OpenBSD.

The thing is that for better or for worse, some drivers drive devices
with firmware provided by multiple vendors, and then it can still make
sense to split off by vendor. E.g. brcmfmac wifi is already in the
process of diverging into three firmware lineages provided by
two(/three?) vendors, even if you ignore the entire Apple special
snowflake case. I expect pain to come out of that one for everyone
involved... (well, at least for Apple we can always special case
conditionals on "has OTP" which is effectively the "is Apple" flag).
ISTR that radeon/amdgpu also ended up with separate roots, but it's
mixed and with different firmware formats for each and a fallback.

> Now I did indeed forget about the "dockchannel" touchpad firmware that
> I already handle in OpenBSD.  That means I could handle the touchbar
> firmware in the same way.  But that is mostly because these firmwares
> are non-distributable, so we don't have firmware packages for them.
> Instead we rely on the Asahi installer to make the firmware available
> on the EFI partition and the OpenBSD installer to move the firmware in
> place on the root filesystem.
> 
> So this isn't a big issue.

:)

(Do let me know if you have any big issues of course, you know I don't
want to gratuitously make your life hard!)

- Hector

Patch

diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
new file mode 100644
index 000000000000..695594494b1e
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
@@ -0,0 +1,81 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple touchscreens attached using the Z2 protocol.
+
+maintainers:
+  - [e-mail]
+  - Sasha Finkelstein <[e-mail]>
+
+description: A series of touschscreen controllers used in Apple products.
+
+allOf:
+  - $ref: touchscreen.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: apple,z2-touchscreen
+
+  reg:
+    maxItems: 1
+
+  interrupts-extended:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  cs-gpios:
+    maxItems: 1
+
+  firmware-name:
+    maxItems: 1
+
+  apple,z2-device-name:
+    description: The name to be used for the input device
+    $ref: /schemas/types.yaml#/definitions/string
+
+  touchscreen-size-x: true
+  touchscreen-size-y: true
+  spi-max-frequency: true
+
+required:
+  - compatible
+  - interrupts-extended
+  - reset-gpios
+  - cs-gpios
+  - firmware-name
+  - apple,z2-device-name
+  - touchscreen-size-x
+  - touchscreen-size-y
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            touchscreen@0 {
+                    compatible = "apple,z2-touchscreen";
+                    reg = <0>;
+                    spi-max-frequency = <11500000>;
+                    reset-gpios = <&pinctrl_ap 139 0>;
+                    cs-gpios = <&pinctrl_ap 109 0>;
+                    interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
+                    firmware-name = "apple/dfrmtfw-j293.bin";
+                    touchscreen-size-x = <23045>;
+                    touchscreen-size-y = <640>;
+                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
+            };
+    };
+
+...