* [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties
From: Javier Carrasco @ 2023-10-17 11:00 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
In-Reply-To: <20230510-feature-ts_virtobj_patch-v5-0-ff6b5c4db693@wolfvision.net>
The overlay-touchscreen object defines an area within the touchscreen
where touch events are reported and their coordinates get converted to
the overlay origin. This object avoids getting events from areas that
are physically hidden by overlay frames.
For touchscreens where overlay buttons on the touchscreen surface are
provided, the overlay-buttons object contains a node for every button
and the key event that should be reported when pressed.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
.../bindings/input/touchscreen/touchscreen.yaml | 143 +++++++++++++++++++++
1 file changed, 143 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
index 431c13335c40..5c58eb79ee9a 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
@@ -87,6 +87,129 @@ properties:
touchscreen-y-plate-ohms:
description: Resistance of the Y-plate in Ohms
+ overlay-touchscreen:
+ description: Clipped touchscreen area
+
+ This object can be used to describe a frame that restricts the area
+ within touch events are reported, ignoring the events that occur outside
+ this area. This is of special interest if the touchscreen is shipped
+ with a physical overlay on top of it with a frame that hides some part
+ of the original touchscreen area.
+
+ The x-origin and y-origin properties of this object define the offset of
+ a new origin from where the touchscreen events are referenced.
+ This offset is applied to the events accordingly. The x-size and y-size
+ properties define the size of the overlay-touchscreen (effective area).
+
+ The following example shows the new touchscreen area and the new origin
+ (0',0') for the touch events generated by the device.
+
+ Touchscreen (full area)
+ ┌────────────────────────────────────────┐
+ │ ┌───────────────────────────────┐ │
+ │ │ │ │
+ │ ├ y-size │ │
+ │ │ │ │
+ │ │ overlay-touchscreen │ │
+ │ │ │ │
+ │ │ │ │
+ │ │ x-size │ │
+ │ ┌└──────────────┴────────────────┘ │
+ │(0',0') │
+ ┌└────────────────────────────────────────┘
+ (0,0)
+
+ where (0',0') = (0+x-origin,0+y-origin)
+
+ type: object
+ $ref: '#/$defs/overlay-node'
+ unevaluatedProperties: false
+
+ required:
+ - x-origin
+ - y-origin
+ - x-size
+ - y-size
+
+ overlay-buttons:
+ description: list of nodes defining the buttons on the touchscreen
+
+ This object can be used to describe buttons on the touchscreen area,
+ reporting the touch events on their surface as key events instead of
+ the original touch events.
+
+ This is of special interest if the touchscreen is shipped with a
+ physical overlay on top of it where a number of buttons with some
+ predefined functionality are printed. In that case a specific behavior
+ is expected from those buttons instead of raw touch events.
+
+ The overlay-buttons properties define a per-button area as well as an
+ origin relative to the real touchscreen origin. Touch events within the
+ button area are reported as the key event defined in the linux,code
+ property. Given that the key events do not provide coordinates, the
+ button origin is only used to place the button area on the touchscreen
+ surface. Any event outside the overlay-buttons object is reported as a
+ touch event with no coordinate transformation.
+
+ The following example shows a touchscreen with a single button on it
+
+ Touchscreen (full area)
+ ┌───────────────────────────────────┐
+ │ │
+ │ │
+ │ ┌─────────┐ │
+ │ │button 0 │ │
+ │ │KEY_POWER│ │
+ │ └─────────┘ │
+ │ │
+ │ │
+ ┌└───────────────────────────────────┘
+ (0,0)
+
+ The overlay-buttons object can be combined with the overlay-touchscreen
+ object as shown in the following example. In that case only the events
+ within the overlay-touchscreen object are reported as touch events.
+
+ Touchscreen (full area)
+ ┌─────────┬──────────────────────────────┐
+ │ │ │
+ │ │ ┌───────────────────────┐ │
+ │ button 0│ │ │ │
+ │KEY_POWER│ │ │ │
+ │ │ │ │ │
+ ├─────────┤ │ overlay-touchscreen │ │
+ │ │ │ │ │
+ │ │ │ │ │
+ │ button 1│ │ │ │
+ │ KEY_INFO│ ┌└───────────────────────┘ │
+ │ │(0',0') │
+ ┌└─────────┴──────────────────────────────┘
+ (0,0)
+
+ type: object
+
+ patternProperties:
+ '^button-':
+ type: object
+ description:
+ Each button (key) is represented as a sub-node.
+ $ref: '#/$defs/overlay-node'
+ unevaluatedProperties: false
+
+ properties:
+ label:
+ $ref: /schemas/types.yaml#/definitions/string
+ description: descriptive name of the button
+
+ linux,code: true
+
+ required:
+ - linux,code
+ - x-origin
+ - y-origin
+ - x-size
+ - y-size
+
dependencies:
touchscreen-size-x: [ touchscreen-size-y ]
touchscreen-size-y: [ touchscreen-size-x ]
@@ -94,3 +217,23 @@ dependencies:
touchscreen-y-mm: [ touchscreen-x-mm ]
additionalProperties: true
+
+$defs:
+ overlay-node:
+ type: object
+ properties:
+ x-origin:
+ description: horizontal origin of the node area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ y-origin:
+ description: vertical origin of the node area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ x-size:
+ description: horizontal resolution of the node area
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ y-size:
+ description: vertical resolution of the node area
+ $ref: /schemas/types.yaml#/definitions/uint32
--
2.39.2
^ permalink raw reply related
* [PATCH v5 3/4] Input: st1232 - add touch overlays handling
From: Javier Carrasco @ 2023-10-17 11:00 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
In-Reply-To: <20230510-feature-ts_virtobj_patch-v5-0-ff6b5c4db693@wolfvision.net>
Use touch-overlay to support overlay objects such as buttons and resized
frames defined in the device tree.
If an overlay-touchscreen is provided, ignore touch events outside of
the area defined by its properties. Otherwise, transform the event
coordinates to the overlay-touchscreen x and y-axis if required.
If buttons are provided, register an additional device to report key
events separately. A key event will be generated if the coordinates
of a touch event are within the area defined by the button properties.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
drivers/input/touchscreen/st1232.c | 70 ++++++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 6475084aee1b..d22f7d57f468 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -22,6 +22,7 @@
#include <linux/pm_qos.h>
#include <linux/slab.h>
#include <linux/types.h>
+#include <linux/input/touch-overlay.h>
#define ST1232_TS_NAME "st1232-ts"
#define ST1633_TS_NAME "st1633-ts"
@@ -56,6 +57,8 @@ struct st1232_ts_data {
struct touchscreen_properties prop;
struct dev_pm_qos_request low_latency_req;
struct gpio_desc *reset_gpio;
+ struct input_dev *overlay_keypad;
+ struct touch_overlay_map *overlay_map;
const struct st_chip_info *chip_info;
int read_buf_len;
u8 *read_buf;
@@ -130,6 +133,7 @@ static int st1232_ts_read_resolution(struct st1232_ts_data *ts, u16 *max_x,
static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
{
struct input_dev *input = ts->input_dev;
+ struct input_dev *keypad = ts->overlay_keypad;
struct input_mt_pos pos[ST_TS_MAX_FINGERS];
u8 z[ST_TS_MAX_FINGERS];
int slots[ST_TS_MAX_FINGERS];
@@ -138,14 +142,20 @@ static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
for (i = 0; i < ts->chip_info->max_fingers; i++) {
u8 *buf = &ts->read_buf[i * 4];
+ bool contact = buf[0] & BIT(7);
+ unsigned int x, y;
- if (buf[0] & BIT(7)) {
- unsigned int x = ((buf[0] & 0x70) << 4) | buf[1];
- unsigned int y = ((buf[0] & 0x07) << 8) | buf[2];
-
- touchscreen_set_mt_pos(&pos[n_contacts],
- &ts->prop, x, y);
+ if (contact) {
+ x = ((buf[0] & 0x70) << 4) | buf[1];
+ y = ((buf[0] & 0x07) << 8) | buf[2];
+ }
+ if (touch_overlay_process_event(ts->overlay_map, keypad,
+ contact ? &x : NULL,
+ contact ? &y : NULL, i))
+ continue;
+ if (contact) {
+ touchscreen_set_mt_pos(&pos[n_contacts], &ts->prop, x, y);
/* st1232 includes a z-axis / touch strength */
if (ts->chip_info->have_z)
z[n_contacts] = ts->read_buf[i + 6];
@@ -226,6 +236,7 @@ static int st1232_ts_probe(struct i2c_client *client)
const struct st_chip_info *match;
struct st1232_ts_data *ts;
struct input_dev *input_dev;
+ struct input_dev *overlay_keypad;
u16 max_x, max_y;
int error;
@@ -263,8 +274,13 @@ static int st1232_ts_probe(struct i2c_client *client)
if (!input_dev)
return -ENOMEM;
+ overlay_keypad = devm_input_allocate_device(&client->dev);
+ if (!overlay_keypad)
+ return -ENOMEM;
+
ts->client = client;
ts->input_dev = input_dev;
+ ts->overlay_keypad = overlay_keypad;
ts->reset_gpio = devm_gpiod_get_optional(&client->dev, NULL,
GPIOD_OUT_HIGH);
@@ -286,24 +302,37 @@ static int st1232_ts_probe(struct i2c_client *client)
input_dev->name = "st1232-touchscreen";
input_dev->id.bustype = BUS_I2C;
+ overlay_keypad->name = "st1232-keypad";
+ overlay_keypad->id.bustype = BUS_I2C;
/* Wait until device is ready */
error = st1232_ts_wait_ready(ts);
if (error)
return error;
- /* Read resolution from the chip */
- error = st1232_ts_read_resolution(ts, &max_x, &max_y);
- if (error) {
- dev_err(&client->dev,
- "Failed to read resolution: %d\n", error);
- return error;
- }
-
if (ts->chip_info->have_z)
input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
ts->chip_info->max_area, 0, 0);
+ /* map overlay objects if defined in the device tree */
+ ts->overlay_map = touch_overlay_map_overlay(ts->overlay_keypad);
+ if (IS_ERR(ts->overlay_map))
+ return PTR_ERR(ts->overlay_map);
+
+ if (touch_overlay_mapped_touchscreen(ts->overlay_map)) {
+ /* Read resolution from the overlay touchscreen if defined */
+ touch_overlay_get_touchscreen_abs(ts->overlay_map,
+ &max_x, &max_y);
+ } else {
+ /* Read resolution from the chip */
+ error = st1232_ts_read_resolution(ts, &max_x, &max_y);
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to read resolution: %d\n", error);
+ return error;
+ }
+ }
+
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, max_x, 0, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
@@ -335,6 +364,19 @@ static int st1232_ts_probe(struct i2c_client *client)
return error;
}
+ /* Register keypad input device if overlay buttons were defined */
+ if (touch_overlay_mapped_buttons(ts->overlay_map)) {
+ error = input_register_device(ts->overlay_keypad);
+ if (error) {
+ dev_err(&client->dev,
+ "Unable to register %s input device\n",
+ overlay_keypad->name);
+ return error;
+ }
+ } else {
+ input_free_device(ts->overlay_keypad);
+ }
+
i2c_set_clientdata(client, ts);
return 0;
--
2.39.2
^ permalink raw reply related
* [PATCH v5 4/4] dt-bindings: input: touchscreen: st1232: add touch-overlay example
From: Javier Carrasco @ 2023-10-17 11:00 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch
Cc: linux-kernel, linux-input, devicetree, Javier Carrasco
In-Reply-To: <20230510-feature-ts_virtobj_patch-v5-0-ff6b5c4db693@wolfvision.net>
The st1232 driver supports the overlay-touchscreen and overlay-buttons
objects defined in the generic touchscreen bindings and implemented in
the touch-overlay module.
Add nodes for an overlay touchscreen and overlay buttons to the existing
example.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
.../input/touchscreen/sitronix,st1232.yaml | 28 ++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
index 1d8ca19fd37a..f33fc0113a67 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/sitronix,st1232.yaml
@@ -37,6 +37,7 @@ unevaluatedProperties: false
examples:
- |
+ #include <dt-bindings/input/linux-event-codes.h>
i2c {
#address-cells = <1>;
#size-cells = <0>;
@@ -46,5 +47,32 @@ examples:
reg = <0x55>;
interrupts = <2 0>;
gpios = <&gpio1 166 0>;
+
+ overlay-touchscreen {
+ x-origin = <0>;
+ x-size = <240>;
+ y-origin = <40>;
+ y-size = <280>;
+ };
+
+ overlay-buttons {
+ button-light {
+ label = "Camera light";
+ linux,code = <KEY_LIGHTS_TOGGLE>;
+ x-origin = <40>;
+ x-size = <40>;
+ y-origin = <0>;
+ y-size = <40>;
+ };
+
+ button-power {
+ label = "Power";
+ linux,code = <KEY_POWER>;
+ x-origin = <160>;
+ x-size = <40>;
+ y-origin = <0>;
+ y-size = <40>;
+ };
+ };
};
};
--
2.39.2
^ permalink raw reply related
* [PATCH v0001 1/1] input/ts: ili210x: send abs-mt-pressure for untouched
From: Markus Burri @ 2023-10-17 13:10 UTC (permalink / raw)
To: linux-kernel; +Cc: Markus Burri, Dmitry Torokhov, Henrik Rydberg, linux-input
Send the ABS_MT_PRESSURE event in case of untouched with zero pressure.
Multitouch pressure is only sent on first touch.
Therefore send an ABS_MT_PRESSURE event in case of untouched with zero pressure.
This avoids that the next ABS_MT_PRESSURE event will
be filtered out by input_defuzz_abs_event() in input.c since the pressure
value has not changed.
Signed-off-by: Markus Burri <markus.burri@mt.com>
---
drivers/input/touchscreen/ili210x.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index e9bd36a..6b79513 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -318,7 +318,8 @@ static bool ili210x_report_events(struct ili210x *priv, u8 *touchdata)
if (priv->chip->has_pressure_reg)
input_report_abs(input, ABS_MT_PRESSURE, z);
contact = true;
- }
+ } else if (priv->chip->has_pressure_reg)
+ input_report_abs(input, ABS_MT_PRESSURE, 0);
}
input_mt_report_pointer_emulation(input, false);
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 0/3] Input: atkbd - add skip_commands module parameter
From: Shang Ye @ 2023-10-17 13:53 UTC (permalink / raw)
To: Hans de Goede; +Cc: Shang Ye, Dmitry Torokhov, linux-input
In-Reply-To: <20231005201544.26983-1-hdegoede@redhat.com>
Hi Hans,
I very much support the inclusion of this patch, because there has been
a similar keyboard issue on at least 3 (presumably 9) types of Lenovo
laptops, which may also be avoided by simply skipping the GETID command.
My patch and a list of the affected laptop types may be found at:
https://github.com/yescallop/atkbd-nogetid
In my last patch submission, I have included the issue details:
https://lore.kernel.org/linux-input/20230530131340.39961-1-yesh25@mail2.sysu.edu.cn/
There were also two other patch submissions aimed at enabling
`i8042.dumbkbd` on some HP laptops in order to avoid sending the GETID
command, which isn't very desirable because it breaks the Caps Lock LED:
https://lore.kernel.org/linux-input/2iAJTwqZV6lQs26cTb38RNYqxvsink6SRmrZ5h0cBUSuf9NT0tZTsf9fEAbbto2maavHJEOP8GA1evlKa6xjKOsaskDhtJWxjcnrgPigzVo=@gurevit.ch/
https://lore.kernel.org/linux-input/20210609073333.8425-1-egori@altlinux.org/
And another patch submisson aimed at fixing the issue generically,
which, sadly, did not work on my laptop because the GETID command would
trigger more errornous behaviours on it:
https://lore.kernel.org/linux-input/20210201160336.16008-1-anton@cpp.in/
I hope that these materials will help people better understand the
nature of the issue and the urgency to address it.
Below are some comments on the patch:
> +MODULE_PARM_DESC(skip_commands, "Bitfield where each bits skips a specific keyboard cmd (0 - 0x3f)");
"bits" -> "bit"?
I think we may also need to document the new module parameter at
Documentation/admin-guide/kernel-parameters.txt and clarify which bit
skips which keyboard command.
Lastly, would you think it is appropriate to include in this patch
series the quirks for Lenovo laptops on which my patch was tested to
work? If so, the quirk table entries would be:
System vendor: "LENOVO"
Product names: "82G2", "82NC", "82TK"
Driver data : ATKBD_SKIP_GETID
Above all, thank you for working out this nice patch.
Regards,
Shang
On 2023/10/06 04:15, Hans de Goede wrote:
> Hi all,
>
> While debugging a keyboard issue on some HP laptops adding i8042.dumbkbd
> helped to avoid the issue. So one of the commands send by atkbd.c seemed
> to be the culprit.
>
> This series a skip_commands option to help debug cases like this by adding
> a bit-field which allows disabling a subset of the ps2_command()
> calls the atkbd driver makes.
>
> It also replaces the existing atkbd_skip_deactivate flag
> with the new parameter and adds a DMI quirk for the HP laptops
> to avoid the keyboard issue there.
>
> Regards,
>
> Hans
>
>
> Hans de Goede (3):
> Input: atkbd - add skip_commands module parameter
> Input: atkbd - drop atkbd_skip_deactivate flag
> Input: atkbd - set skip_commands = ATKBD_SKIP_GETID for HP laptop
> 15s-fq* laptops
>
> drivers/input/keyboard/atkbd.c | 88 ++++++++++++++++++++++++++--------
> 1 file changed, 69 insertions(+), 19 deletions(-)
>
> --
> 2.41.0
>
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: Introduce Himax HID-over-SPI device
From: Conor Dooley @ 2023-10-17 13:59 UTC (permalink / raw)
To: Tylor Yang
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, jikos,
benjamin.tissoires, linux-input, devicetree, linux-kernel,
poyuan_chang, jingyliang, hbarnor, wuxy23, luolm1, poyu_hung
In-Reply-To: <20231017091900.801989-2-tylor_yang@himax.corp-partner.google.com>
[-- Attachment #1: Type: text/plain, Size: 5772 bytes --]
Yo,
On Tue, Oct 17, 2023 at 05:18:57PM +0800, Tylor Yang wrote:
> The Himax HID-over-SPI framework support for Himax touchscreen ICs
> that report HID packet through SPI bus. The driver core need reset
> pin to meet reset timing spec. of IC. An interrupt to disable
> and enable interrupt when suspend/resume. Two optional power control
> if target board needed.
>
> Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> ---
> .../devicetree/bindings/input/himax,hid.yaml | 123 ++++++++++++++++++
> MAINTAINERS | 6 +
> 2 files changed, 129 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/himax,hid.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/himax,hid.yaml b/Documentation/devicetree/bindings/input/himax,hid.yaml
> new file mode 100644
> index 000000000000..9ba86fe1b7da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/himax,hid.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/himax,hid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Himax TDDI devices using SPI to send HID packets
> +
> +maintainers:
> + - Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> +
> +description: |
> + Support the Himax TDDI devices which using SPI interface to acquire
> + HID packets from the device. The device needs to be initialized using
> + Himax protocol before it start sending HID packets.
> +
> +properties:
> + compatible:
> + const: himax,hid
This compatible seems far too generic. Why are there not device specific
compatibles for each TDDI device?
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + reset:
> + maxItems: 1
> + description: Reset device, active low signal.
> +
> + vccd-supply:
> + description:
> + Optional regulator for the 1.8V voltage.
> +
> + vcca-supply:
> + description:
> + Optional regulator for the analog 3.3V voltage.
> +
> + himax,id-gpios:
> + maxItems: 8
> + description: GPIOs to read physical Panel ID. Optional.
> +
> + spi-cpha: true
> + spi-cpol: true
> + himax,ic-det-delay-ms:
> + description:
> + Due to TDDI properties, the TPIC detection timing must after the
> + display panel initialized. This property is used to specify the
> + delay time when TPIC detection and display panel initialization
> + timing are overlapped. How much milliseconds to delay before TPIC
> + detection start.
> +
> + himax,ic-resume-delay-ms:
> + description:
> + Due to TDDI properties, the TPIC resume timing must after the
> + display panel resumed. This property is used to specify the
> + delay time when TPIC resume and display panel resume
> + timing are overlapped. How much milliseconds to delay before TPIC
> + resume start.
> + panel:
> + description:
> + The node of the display panel device. The driver will use this
> + node to get the project ID of the display panel. Optional.
> + type: object
> + additionalProperties: false
> +
> + properties:
> + himax,pid:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 1
> + maxItems: 8
> + items:
> + minimum: 0
> + maximum: 65535
> + description:
> + When only one value exist, represent Project ID of the device.
> + When multiple values exist, order in event number value represnet
> + id value from id-gpios and odd number value represent Project ID
> + relatives to prior id value. This is used to specify the firmware
> + for the device.
I am sorry, but I still fail to understand why using device specific
compatibles & firmware-name does not work here. It still seems like this
property exists purely because you do not know what device you are
because of a lack of specific compatibles.
Thanks,
Conor.
> +
> + required:
> + - himax,pid
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - reset
> +
> +unevaluatedProperties: false
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/gpio/gpio.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + touchscreen@0 {
> + compatible = "himax,hid";
> + reg = <0x0>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> + pinctrl-0 = <&touch_pins>;
> + pinctrl-names = "default";
> +
> + spi-max-frequency = <12500000>;
> + spi-cpha;
> + spi-cpol;
> +
> + reset = <&gpio1 8 GPIO_ACTIVE_LOW>;
> + himax,ic-det-delay-ms = <500>;
> + himax,ic-resume-delay-ms = <100>;
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a7bd8bd80e9..883870ab316f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9340,6 +9340,12 @@ L: linux-kernel@vger.kernel.org
> S: Maintained
> F: drivers/misc/hisi_hikey_usb.c
>
> +HIMAX HID OVER SPI TOUCHSCREEN SUPPORT
> +M: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> +L: linux-input@vger.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/input/himax,hid.yaml
> +
> HIMAX HX83112B TOUCHSCREEN SUPPORT
> M: Job Noorman <job@noorman.info>
> L: linux-input@vger.kernel.org
> --
> 2.25.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RESEND 1/1] Add support for touch screens using the General Touch ST6001S controller.
From: Gareth Randall @ 2023-10-17 14:11 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <83a934eb-e1ee-fd79-33ee-76413be2e6ea@virgin.net>
Dear Dmitry,
I've noticed that this update from a couple of weeks ago hasn't come up
in the Patchwork system, probably because the original post was too long
ago.
I'll happily resubmit this if you (or any other admins) would like me to.
Thanks.
Gareth
On 03/10/2023 19:32, Gareth Randall wrote:
> Hi Dmitry,
>
> Thanks very much for the feedback. Apologies for the long delay in
> getting back to you on this. Updated patch is below, and apologies for
> the email layout. I've also added comments to your points in the quoted
> message.
[snip]
^ permalink raw reply
* [PATCH] Input: i8042 - add Fujitsu Lifebook U728 to i8042 quirk table
From: Szilard Fabian @ 2023-10-17 15:15 UTC (permalink / raw)
To: linux-kernel, linux-input, dmitry.torokhov; +Cc: Szilard Fabian
Another Fujitsu-related patch.
In the initial boot stage the integrated keyboard of Fujitsu Lifebook U728
refuses to work and it's not possible to type for example a dm-crypt
passphrase without the help of an external keyboard.
i8042.nomux kernel parameter resolves this issue but using that a PS/2
mouse is detected. This input device is unused even when the i2c-hid-acpi
kernel module is blacklisted making the integrated ELAN touchpad
(04F3:3092) not working at all.
So this notebook uses a hid-over-i2c touchpad which is managed by the
i2c_designware input driver. Since you can't find a PS/2 mouse port on this
computer and you can't connect a PS/2 mouse to it even with an official
port replicator I think it's safe to not use the PS/2 mouse port at all.
Signed-off-by: Szilard Fabian <szfabian@bluemarch.art>
---
I think the same configuration could be applied to Lifebook U748 and U758
too but I can't test this theory on these machines.
---
drivers/input/serio/i8042-acpipnpio.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index 028e45bd050b..983f31014330 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -618,6 +618,14 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
},
.driver_data = (void *)(SERIO_QUIRK_NOMUX)
},
+ {
+ /* Fujitsu Lifebook U728 */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK U728"),
+ },
+ .driver_data = (void *)(SERIO_QUIRK_NOAUX)
+ },
{
/* Gigabyte M912 */
.matches = {
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v3 1/4] dt-bindings: input: Introduce Himax HID-over-SPI device
From: Krzysztof Kozlowski @ 2023-10-17 16:58 UTC (permalink / raw)
To: Conor Dooley, Tylor Yang
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, jikos,
benjamin.tissoires, linux-input, devicetree, linux-kernel,
poyuan_chang, jingyliang, hbarnor, wuxy23, luolm1, poyu_hung
In-Reply-To: <20231017-womb-lantern-186f16ce67af@spud>
On 17/10/2023 15:59, Conor Dooley wrote:
> Yo,
>
> On Tue, Oct 17, 2023 at 05:18:57PM +0800, Tylor Yang wrote:
>> The Himax HID-over-SPI framework support for Himax touchscreen ICs
>> that report HID packet through SPI bus. The driver core need reset
>> pin to meet reset timing spec. of IC. An interrupt to disable
>> and enable interrupt when suspend/resume. Two optional power control
>> if target board needed.
>>
>> Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
>> ---
>> .../devicetree/bindings/input/himax,hid.yaml | 123 ++++++++++++++++++
>> MAINTAINERS | 6 +
>> 2 files changed, 129 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/himax,hid.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/input/himax,hid.yaml b/Documentation/devicetree/bindings/input/himax,hid.yaml
>> new file mode 100644
>> index 000000000000..9ba86fe1b7da
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/himax,hid.yaml
>> @@ -0,0 +1,123 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/input/himax,hid.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Himax TDDI devices using SPI to send HID packets
>> +
>> +maintainers:
>> + - Tylor Yang <tylor_yang@himax.corp-partner.google.com>
>> +
>> +description: |
>> + Support the Himax TDDI devices which using SPI interface to acquire
>> + HID packets from the device. The device needs to be initialized using
>> + Himax protocol before it start sending HID packets.
>> +
>> +properties:
>> + compatible:
>> + const: himax,hid
>
> This compatible seems far too generic. Why are there not device specific
> compatibles for each TDDI device?
Which was pointed out by Rob in v2, so his feedback was ignored.
>
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + reset:
>> + maxItems: 1
>> + description: Reset device, active low signal.
No, come one, read feedback from Rob.
>> +
>> + vccd-supply:
>> + description:
>> + Optional regulator for the 1.8V voltage.
>> +
>> + vcca-supply:
>> + description:
>> + Optional regulator for the analog 3.3V voltage.
>> +
>> + himax,id-gpios:
>> + maxItems: 8
>> + description: GPIOs to read physical Panel ID. Optional.
>> +
>> + spi-cpha: true
>> + spi-cpol: true
>
>> + himax,ic-det-delay-ms:
>> + description:
>> + Due to TDDI properties, the TPIC detection timing must after the
>> + display panel initialized. This property is used to specify the
>> + delay time when TPIC detection and display panel initialization
>> + timing are overlapped. How much milliseconds to delay before TPIC
>> + detection start.
>> +
>> + himax,ic-resume-delay-ms:
>> + description:
>> + Due to TDDI properties, the TPIC resume timing must after the
>> + display panel resumed. This property is used to specify the
>> + delay time when TPIC resume and display panel resume
>> + timing are overlapped. How much milliseconds to delay before TPIC
>> + resume start.
>
No improvements here...
You must implement all feedback from v2. Not pieces of it.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 2/4] HID: touchscreen: Add initial support for Himax HID-over-SPI
From: Krzysztof Kozlowski @ 2023-10-17 17:01 UTC (permalink / raw)
To: Tylor Yang, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel
Cc: poyuan_chang, jingyliang, hbarnor, wuxy23, luolm1, poyu_hung
In-Reply-To: <20231017091900.801989-3-tylor_yang@himax.corp-partner.google.com>
On 17/10/2023 11:18, Tylor Yang wrote:
> The hx83102j is a TDDI IC (Touch with Display Driver). The
> IC using SPI to transferring HID packet to host CPU. The IC also
> report HID report descriptor for driver to register HID device.
> The driver is designed as a framework for future expansion and
> hx83102j is the first case. Each hx_spi_hid_hx8xxxxx modules are
> mutual exclusive, it should be initiate one at a time.
>
> This driver takes a position similar to i2c-hid, it initialize
> and control the touch IC below and register HID to upper hid-core.
> When touch ic report an interrupt, it receive the data from IC
> and report as HID input to hid-core. Let hid-core dispatch input
> to registered hid-protocol and report to related input sub-system.
>
> This driver also provide advanced functions by hidraw interface:
> - runtime firmware update
> - debug functions, such as reg r/w
> - self test for touch panel
>
> Due to patch size is too big, separate into 3 part. This is part 1.
>
> Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> ---
> MAINTAINERS | 1 +
> drivers/hid/hx-hid/hx_acpi.c | 81 ++
> drivers/hid/hx-hid/hx_core.c | 1605 +++++++++++++++++++++++++++++
> drivers/hid/hx-hid/hx_core.h | 489 +++++++++
> drivers/hid/hx-hid/hx_hid.c | 753 ++++++++++++++
> drivers/hid/hx-hid/hx_hid.h | 96 ++
> drivers/hid/hx-hid/hx_ic_83102j.c | 340 ++++++
> drivers/hid/hx-hid/hx_ic_83102j.h | 42 +
> 8 files changed, 3407 insertions(+)
> create mode 100644 drivers/hid/hx-hid/hx_acpi.c
> create mode 100644 drivers/hid/hx-hid/hx_core.c
> create mode 100644 drivers/hid/hx-hid/hx_core.h
> create mode 100644 drivers/hid/hx-hid/hx_hid.c
> create mode 100644 drivers/hid/hx-hid/hx_hid.h
> create mode 100644 drivers/hid/hx-hid/hx_ic_83102j.c
> create mode 100644 drivers/hid/hx-hid/hx_ic_83102j.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 883870ab316f..95ea8159eced 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9345,6 +9345,7 @@ M: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> L: linux-input@vger.kernel.org
> S: Supported
> F: Documentation/devicetree/bindings/input/himax,hid.yaml
> +F: drivers/hid/hx-hid/
>
> HIMAX HX83112B TOUCHSCREEN SUPPORT
> M: Job Noorman <job@noorman.info>
> diff --git a/drivers/hid/hx-hid/hx_acpi.c b/drivers/hid/hx-hid/hx_acpi.c
> new file mode 100644
> index 000000000000..2dc7c611a61a
> --- /dev/null
> +++ b/drivers/hid/hx-hid/hx_acpi.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Himax Driver Code for Common IC to simulate HID
> + *
> + * Copyright (C) 2023 Himax Corporation.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Drop boiler plate. It's gone since some years. Having it here suggests
you just push downstream crappy code. Please don't. Start from scratch
taking existing driver.
> + */
> +
> +#include "hx_core.h"
> +
> +int himax_parse_acpi(struct device *dev,
> + struct himax_platform_data *pdata)
> +{
> + int ret = 0;
> + struct gpio_desc *desc;
> + const u32 interrupt_pin_idx = 0;
> + // const u32 reset_pin_idx = 1;
> + const char *interrupt_pin_dsd_name = "irq"; // to name "irq-gpios"
> + const char *reset_pin_dsd_name = "reset"; // to name "reset-gpios"
This style, dead code, comments is not a Linux coding style.
> +
> + D("Entered");
OK, I'll finish review. A lot further looks even worse. This is not code
suitable for inclusion in mainline. Please start from scratch from
existing code and customize it per your needs. This way you will keep
Linux coding style instead introducing some totally different coding
style from downstream, terrible quality driver.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 4/4] HID: touchscreen: Add initial support for Himax HID-over-SPI
From: Krzysztof Kozlowski @ 2023-10-17 17:03 UTC (permalink / raw)
To: Tylor Yang, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, jikos, benjamin.tissoires, linux-input, devicetree,
linux-kernel
Cc: poyuan_chang, jingyliang, hbarnor, wuxy23, luolm1, poyu_hung
In-Reply-To: <20231017091900.801989-5-tylor_yang@himax.corp-partner.google.com>
On 17/10/2023 11:19, Tylor Yang wrote:
...
> +
> +#if defined(CONFIG_FB)
> +int fb_notifier_callback(struct notifier_block *self,
> + unsigned long event, void *data)
> +{
> + const struct fb_event *evdata = data;
> + int *blank;
> + struct himax_ts_data *ts =
> + container_of(self, struct himax_ts_data, fb_notif);
> +
> + I("entered");
> +
> + if (!ts) {
> + E("ts is NULL");
> + return -ECANCELED;
> + }
There are so many wrong things with this.... First, tell me, how
container of valid pointer can be NULL?
Second, this is not Linux coding style.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 0/4] HID: touchscreen: add himax hid-over-spi driver
From: Krzysztof Kozlowski @ 2023-10-17 17:08 UTC (permalink / raw)
To: Tylor Yang, Doug Anderson, Tomasz Figa, jingyliang, poyuan_chang,
hbarnor
Cc: jikos, wuxy23, conor+dt, luolm1, robh+dt, dmitry.torokhov,
devicetree, krzysztof.kozlowski+dt, poyu_hung, linux-kernel,
linux-input, benjamin.tissoires
In-Reply-To: <20231017091900.801989-1-tylor_yang@himax.corp-partner.google.com>
On 17/10/2023 11:18, Tylor Yang wrote:
> Hello,
>
> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> This driver takes a position in [1], it intends to take advantage of SPI
> transfer speed and HID interface.
>
Dear Google/Chromium folks,
As a multi-billion company I am sure you can spare some small amount of
time/effort/money for internal review before using community for this
purpose. I mean reviewing trivial issues, like coding style, or just
running checkpatch. You know, the obvious things.
There is no need to use expensive time of community reviewers to review
very simple mistakes, the ones which we fixed in Linux kernel years ago
(also with automated tools). You can and you should do it, before
submitting drivers for community review.
Thanks in advance.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 0/4] HID: touchscreen: add himax hid-over-spi driver
From: Doug Anderson @ 2023-10-17 21:41 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Tylor Yang, Tomasz Figa, jingyliang, poyuan_chang, hbarnor, jikos,
wuxy23, conor+dt, luolm1, robh+dt, dmitry.torokhov, devicetree,
krzysztof.kozlowski+dt, poyu_hung, linux-kernel, linux-input,
benjamin.tissoires
In-Reply-To: <6c7d9c92-7616-4fad-806e-44302c33b63c@linaro.org>
Hi,
On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/10/2023 11:18, Tylor Yang wrote:
> > Hello,
> >
> > This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> > This driver takes a position in [1], it intends to take advantage of SPI
> > transfer speed and HID interface.
> >
>
> Dear Google/Chromium folks,
>
> As a multi-billion company I am sure you can spare some small amount of
> time/effort/money for internal review before using community for this
> purpose. I mean reviewing trivial issues, like coding style, or just
> running checkpatch. You know, the obvious things.
>
> There is no need to use expensive time of community reviewers to review
> very simple mistakes, the ones which we fixed in Linux kernel years ago
> (also with automated tools). You can and you should do it, before
> submitting drivers for community review.
We can certainly talk more about this, but a quick reply is:
1. If a patch really looks super bad to you then the right thing for
you to do is to respond to the patch with some canned response saying
"you didn't even do these basic things--please read the documentation
and work with someone at Google to get a basic review". This seems
like a perfectly legit response and I don't think you should do more
than that.
2. IMO as a general rule "internal review" should be considered
harmful. When you're a new submitter then absolutely you should get
some internal review from someone who has done this before, but making
"internal review" a requirement for all patches leads to frustration
all around. It leads to people redesigning their code in response to
"internal review" and then getting frustrated when external
maintainers tell them to do something totally different. ...then
upstream reviewers respond to the frustration with "Why were you
designing your code behind closed doors? If you had done the review in
the public and on the mailing lists then someone could have stopped
you before you changed everything".
3. The ChromeOS team is organized much more like the upstream
community than a big hierarchical corporation. Just as it's not easy
for you to control the behavior of other maintainers, it is not
trivial for one person on the team to control what others on the team
will do. We could make an attempt to institute rules like "all patches
must go through internal review before being posted", but as per #2 I
don't think this is a good idea. The ChromeOS team has even less
control over what our partners may or may not do. In general it is
always a struggle to get partners to even start working upstream and
IMO it's a win when I see a partner post a patch. We should certainly
help partners be successful here, but the right way to do that is by
offering them support.
About the best we can do is to provide good documentation for people
learning how to send patches. Right now the ChromeOS kernel docs [1]
suggest using "patman" to send patches and I have seen many partners
do this. Patman will, at the very least, run checkpatch for you. Our
instructions also say that you should make sure you run "checkpatch"
yourself if you don't run patman. If people aren't following these
docs that we already have then there's not much we can do.
So I guess the tl;dr from my side:
a) People should absolutely be posting on mailing lists and not (as a
rule) doing "internal review".
b) If a patch looks really broken to you, don't get upset and don't
waste your time. Just respond and say that you'll look at it once it
looks better and suggest that they get a review (preferably on the
mailing lists!) from someone they're working with at Google.
https://chromium.googlesource.com/chromiumos/docs/+/HEAD/kernel_development.md#send-out-the-patch-using-patman
-Doug
^ permalink raw reply
* Re: [PATCH v3 0/4] HID: touchscreen: add himax hid-over-spi driver
From: Krzysztof Kozlowski @ 2023-10-18 6:07 UTC (permalink / raw)
To: Doug Anderson
Cc: Tylor Yang, Tomasz Figa, jingyliang, poyuan_chang, hbarnor, jikos,
wuxy23, conor+dt, luolm1, robh+dt, dmitry.torokhov, devicetree,
krzysztof.kozlowski+dt, poyu_hung, linux-kernel, linux-input,
benjamin.tissoires
In-Reply-To: <CAD=FV=X2kZcyeyK1SBcXaViBft4F6XYtA6+JwBqJswU41V9kUQ@mail.gmail.com>
On 17/10/2023 23:41, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 17/10/2023 11:18, Tylor Yang wrote:
>>> Hello,
>>>
>>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
>>> This driver takes a position in [1], it intends to take advantage of SPI
>>> transfer speed and HID interface.
>>>
>>
>> Dear Google/Chromium folks,
>>
>> As a multi-billion company I am sure you can spare some small amount of
>> time/effort/money for internal review before using community for this
>> purpose. I mean reviewing trivial issues, like coding style, or just
>> running checkpatch. You know, the obvious things.
>>
>> There is no need to use expensive time of community reviewers to review
>> very simple mistakes, the ones which we fixed in Linux kernel years ago
>> (also with automated tools). You can and you should do it, before
>> submitting drivers for community review.
>
> We can certainly talk more about this, but a quick reply is:
>
> 1. If a patch really looks super bad to you then the right thing for
> you to do is to respond to the patch with some canned response saying
> "you didn't even do these basic things--please read the documentation
> and work with someone at Google to get a basic review". This seems
> like a perfectly legit response and I don't think you should do more
> than that.
>
> 2. IMO as a general rule "internal review" should be considered
> harmful. When you're a new submitter then absolutely you should get
> some internal review from someone who has done this before, but making
> "internal review" a requirement for all patches leads to frustration
> all around. It leads to people redesigning their code in response to
> "internal review" and then getting frustrated when external
> maintainers tell them to do something totally different. ...then
> upstream reviewers respond to the frustration with "Why were you
> designing your code behind closed doors? If you had done the review in
> the public and on the mailing lists then someone could have stopped
> you before you changed everything".
No one expects forced internal review on mature contributions. We talk
here about a first time contribution where already basic mistakes were
made: like not using get_maintainers.pl, not using checkpatch, not using
other tools and finally sending code which does not look like Linux
kernel code at all.
>
> 3. The ChromeOS team is organized much more like the upstream
> community than a big hierarchical corporation. Just as it's not easy
> for you to control the behavior of other maintainers, it is not
> trivial for one person on the team to control what others on the team
> will do. We could make an attempt to institute rules like "all patches
> must go through internal review before being posted", but as per #2 I
> don't think this is a good idea. The ChromeOS team has even less
> control over what our partners may or may not do. In general it is
> always a struggle to get partners to even start working upstream and
> IMO it's a win when I see a partner post a patch. We should certainly
> help partners be successful here, but the right way to do that is by
> offering them support.
I don't know who is exactly core team, who is partner. I see
"google.com" domain, so Google folks are responsible for not wasting
time of the community. If Google disagrees, please change the domain so
I will understand that and not feel like Google wants to use us all. I
am fine and I understand if small companies or individuals make such
mistakes. It feels like a waste of our time if Google makes such
mistakes. Google's (Alphabet's) revenue for 2022 was 282 billions USD
and net revenue was 59 billions USD.
>
> About the best we can do is to provide good documentation for people
> learning how to send patches. Right now the ChromeOS kernel docs [1]
> suggest using "patman" to send patches and I have seen many partners
> do this. Patman will, at the very least, run checkpatch for you. Our
> instructions also say that you should make sure you run "checkpatch"
> yourself if you don't run patman. If people aren't following these
> docs that we already have then there's not much we can do.
>
>
> So I guess the tl;dr from my side:
>
> a) People should absolutely be posting on mailing lists and not (as a
> rule) doing "internal review".
>
> b) If a patch looks really broken to you, don't get upset and don't
> waste your time. Just respond and say that you'll look at it once it
> looks better and suggest that they get a review (preferably on the
> mailing lists!) from someone they're working with at Google.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 0/4] HID: touchscreen: add himax hid-over-spi driver
From: Benjamin Tissoires @ 2023-10-18 6:33 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Doug Anderson, Tylor Yang, Tomasz Figa, jingyliang, poyuan_chang,
hbarnor, jikos, wuxy23, conor+dt, luolm1, robh+dt,
dmitry.torokhov, devicetree, krzysztof.kozlowski+dt, poyu_hung,
linux-kernel, linux-input
In-Reply-To: <9e1233ce-1a6d-443d-873e-6efb3ed0207c@linaro.org>
Hi Doug, Krzysztof,
On Wed, Oct 18, 2023 at 8:07 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 17/10/2023 23:41, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Oct 17, 2023 at 10:08 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 17/10/2023 11:18, Tylor Yang wrote:
> >>> Hello,
> >>>
> >>> This patch series adds the driver for Himax HID-over-SPI touchscreen ICs.
> >>> This driver takes a position in [1], it intends to take advantage of SPI
> >>> transfer speed and HID interface.
> >>>
> >>
> >> Dear Google/Chromium folks,
> >>
> >> As a multi-billion company I am sure you can spare some small amount of
> >> time/effort/money for internal review before using community for this
> >> purpose. I mean reviewing trivial issues, like coding style, or just
> >> running checkpatch. You know, the obvious things.
> >>
> >> There is no need to use expensive time of community reviewers to review
> >> very simple mistakes, the ones which we fixed in Linux kernel years ago
> >> (also with automated tools). You can and you should do it, before
> >> submitting drivers for community review.
> >
> > We can certainly talk more about this, but a quick reply is:
> >
> > 1. If a patch really looks super bad to you then the right thing for
> > you to do is to respond to the patch with some canned response saying
> > "you didn't even do these basic things--please read the documentation
> > and work with someone at Google to get a basic review". This seems
> > like a perfectly legit response and I don't think you should do more
> > than that.
As explained below, the point here is not to do an internal review to
come up with a final patch that has been signed-off by everybody. The
point is to have a minimum of hand holding when a submission looks
terrible.
We have seen a bunch of (small) patches that have multiple
signed-off-by when they are submitted for the first time, and it's
perfectly fine. It often leads to a fast forward inclusion process.
But in that particular scenario, the patches could have looked less
rude toward the community by having a first internal swipe to say:
- please run checkpatch before submitting
- please use kernel coding style
- please do not send a 9000 loc single patch (it got slightly better
in v3, but still is way too big for anybody to make a correct review
IMO).
And if you prefer to do these things openly, I would have expected
someone from Google to have at least said that publicly.
> >
> > 2. IMO as a general rule "internal review" should be considered
> > harmful. When you're a new submitter then absolutely you should get
> > some internal review from someone who has done this before, but making
> > "internal review" a requirement for all patches leads to frustration
> > all around. It leads to people redesigning their code in response to
> > "internal review" and then getting frustrated when external
> > maintainers tell them to do something totally different. ...then
> > upstream reviewers respond to the frustration with "Why were you
> > designing your code behind closed doors? If you had done the review in
> > the public and on the mailing lists then someone could have stopped
> > you before you changed everything".
>
> No one expects forced internal review on mature contributions. We talk
> here about a first time contribution where already basic mistakes were
> made: like not using get_maintainers.pl, not using checkpatch, not using
> other tools and finally sending code which does not look like Linux
> kernel code at all.
Basically, what we want is a little bit of mentoring: "can I send this
to the community? - well, there are obvious things that will be a
problem".
>
> >
> > 3. The ChromeOS team is organized much more like the upstream
> > community than a big hierarchical corporation. Just as it's not easy
> > for you to control the behavior of other maintainers, it is not
> > trivial for one person on the team to control what others on the team
> > will do. We could make an attempt to institute rules like "all patches
> > must go through internal review before being posted", but as per #2 I
> > don't think this is a good idea. The ChromeOS team has even less
> > control over what our partners may or may not do. In general it is
> > always a struggle to get partners to even start working upstream and
> > IMO it's a win when I see a partner post a patch. We should certainly
> > help partners be successful here, but the right way to do that is by
> > offering them support.
>
> I don't know who is exactly core team, who is partner. I see
> "google.com" domain, so Google folks are responsible for not wasting
> time of the community. If Google disagrees, please change the domain so
> I will understand that and not feel like Google wants to use us all. I
> am fine and I understand if small companies or individuals make such
> mistakes. It feels like a waste of our time if Google makes such
> mistakes. Google's (Alphabet's) revenue for 2022 was 282 billions USD
> and net revenue was 59 billions USD.
>
> >
> > About the best we can do is to provide good documentation for people
> > learning how to send patches. Right now the ChromeOS kernel docs [1]
> > suggest using "patman" to send patches and I have seen many partners
> > do this. Patman will, at the very least, run checkpatch for you. Our
> > instructions also say that you should make sure you run "checkpatch"
> > yourself if you don't run patman. If people aren't following these
> > docs that we already have then there's not much we can do.
> >
> >
> > So I guess the tl;dr from my side:
> >
> > a) People should absolutely be posting on mailing lists and not (as a
> > rule) doing "internal review".
> >
> > b) If a patch looks really broken to you, don't get upset and don't
> > waste your time. Just respond and say that you'll look at it once it
> > looks better and suggest that they get a review (preferably on the
> > mailing lists!) from someone they're working with at Google.
The problem is that the form of the very first submission looked
terrible, with a single patch of 9000 changes. It was even rejected by
the mailing list.
I think I understand why this happened, but if that patch is not split
in functional simple logic blocks, there is no way we are going to
review and merge this.
Anyway, many thanks to Krzysztof for trying to review the code, which
I dropped already from v1.
Cheers,
Benjamin
^ permalink raw reply
* [PATCH v4 00/17] Introduce PMF Smart PC Solution Builder Feature
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
Smart PC Solutions Builder allows for OEM to define a large number of
custom system states to dynamically switch to. The system states are
referred to as policies, and multiple policies can be loaded onto the
system at any given time, however only one policy can be active at a
given time.
Policy is a combination of PMF input and output capabilities. The inputs
are the incoming information from the other kernel subsystems like LID
state, Sensor info, GPU info etc and the actions are the updating the
power limits of SMU etc.
The policy binary is signed and encrypted by a special key from AMD. This
policy binary shall have the inputs and outputs which the OEMs can build
for the platform customization that can enhance the user experience and
system behavior.
This series adds the initial support for Smart PC solution to PMF driver.
Note that, on platforms where CnQF and Smart PC is advertised, Smart PC
shall have higher precedence and same applies for Auto Mode.
v3->v4:
---------
- Split v3 9/16 into 2 patches, that addresses using generic fn names
- Add softdep [Ilpo] instead of request_module()
- return proper ACPI status [Mario]
- Update comments in code [Mario]
- Remove missed double _ remarks
- handle battery status branches [Ilpo]
- Address KASAN problems
v2->v3:
---------
- Remove pci_get_device() for getting gpu handle
- add .suspend handler for pmf driver
- remove unwanted type caste
- Align comments, spaces etc.
- add wrapper for print_hex_dump_debug()
- Remove lkp tags in commit-msg
- Add macros for magic numbers
- use right format specifiers for printing
- propagate error codes back to the caller
- remove unwanted comments
v1->v2:
---------
- Remove __func__ macros
- Remove manual function names inside prints
- Handle tee_shm_get_va() failure
- Remove double _
- Add meaningful prints
- pass amd_pmf_set_dram_addr() failure errors
- Add more information to commit messages
- use right format specifiers
- use devm_ioremap() instead of ioremap()
- address unsigned long vs u32 problems
- Fix lkp reported issues
- Add amd_pmf_remove_pb() to remove the debugfs files created(if any).
- Make amd_pmf_open_pb() as static.
- Add cooling device APIs for controlling amdgpu backlight
- handle amd_pmf_apply_policies() failures
- Split v1 14/15 into 2 patches further
- use linux/units.h for better handling
- add "depends on" AMD_SFH_HID for interaction with SFH
- other cosmetic remarks
Basavaraj Natikar (3):
HID: amd_sfh: rename float_to_int() to amd_sfh_float_to_int()
platform/x86/amd/pmf: Add PMF-AMDSFH interface for HPD
platform/x86/amd/pmf: Add PMF-AMDSFH interface for ALS
Shyam Sundar S K (14):
platform/x86/amd/pmf: Add PMF TEE interface
platform/x86/amd/pmf: Add support for PMF-TA interaction
platform/x86/amd/pmf: Change return type of amd_pmf_set_dram_addr()
platform/x86/amd/pmf: Add support for PMF Policy Binary
platform/x86/amd/pmf: change amd_pmf_init_features() call sequence
platform/x86/amd/pmf: Add support to get inputs from other subsystems
platform/x86/amd/pmf: Add support update p3t limit
platform/x86/amd/pmf: Add support to update system state
platform/x86/amd/pmf: Make source_as_str() as non-static
platform/x86/amd/pmf: Add facility to dump TA inputs
platform/x86/amd/pmf: Add capability to sideload of policy binary
platform/x86/amd/pmf: dump policy binary data
platform/x86/amd/pmf: Add PMF-AMDGPU get interface
platform/x86/amd/pmf: Add PMF-AMDGPU set interface
Documentation/admin-guide/index.rst | 1 +
Documentation/admin-guide/pmf.rst | 24 +
drivers/gpu/drm/amd/amdgpu/Makefile | 2 +
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c | 156 ++++++
drivers/hid/amd-sfh-hid/amd_sfh_common.h | 6 +
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 22 +-
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 17 +
.../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 48 ++
.../amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 1 +
drivers/platform/x86/amd/pmf/Kconfig | 3 +
drivers/platform/x86/amd/pmf/Makefile | 3 +-
drivers/platform/x86/amd/pmf/acpi.c | 37 ++
drivers/platform/x86/amd/pmf/core.c | 64 ++-
drivers/platform/x86/amd/pmf/pmf.h | 208 +++++++
drivers/platform/x86/amd/pmf/spc.c | 198 +++++++
drivers/platform/x86/amd/pmf/sps.c | 5 +-
drivers/platform/x86/amd/pmf/tee-if.c | 508 ++++++++++++++++++
include/linux/amd-pmf-io.h | 55 ++
19 files changed, 1331 insertions(+), 28 deletions(-)
create mode 100644 Documentation/admin-guide/pmf.rst
create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_pmf.c
create mode 100644 drivers/platform/x86/amd/pmf/spc.c
create mode 100644 drivers/platform/x86/amd/pmf/tee-if.c
create mode 100644 include/linux/amd-pmf-io.h
--
2.25.1
^ permalink raw reply
* [PATCH v4 01/17] platform/x86/amd/pmf: Add PMF TEE interface
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
AMD PMF driver loads the PMF TA (Trusted Application) into the AMD
ASP's (AMD Security Processor) TEE (Trusted Execution Environment).
PMF Trusted Application is a secured firmware placed under
/lib/firmware/amdtee gets loaded only when the TEE environment is
initialized. Add the initial code path to build these pipes.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/Kconfig | 1 +
drivers/platform/x86/amd/pmf/Makefile | 3 +-
drivers/platform/x86/amd/pmf/core.c | 10 ++-
drivers/platform/x86/amd/pmf/pmf.h | 16 ++++
drivers/platform/x86/amd/pmf/tee-if.c | 105 ++++++++++++++++++++++++++
5 files changed, 130 insertions(+), 5 deletions(-)
create mode 100644 drivers/platform/x86/amd/pmf/tee-if.c
diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index 3064bc8ea167..32a029e8db80 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -9,6 +9,7 @@ config AMD_PMF
depends on POWER_SUPPLY
depends on AMD_NB
select ACPI_PLATFORM_PROFILE
+ depends on TEE
help
This driver provides support for the AMD Platform Management Framework.
The goal is to enhance end user experience by making AMD PCs smarter,
diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
index fdededf54392..d2746ee7369f 100644
--- a/drivers/platform/x86/amd/pmf/Makefile
+++ b/drivers/platform/x86/amd/pmf/Makefile
@@ -6,4 +6,5 @@
obj-$(CONFIG_AMD_PMF) += amd-pmf.o
amd-pmf-objs := core.o acpi.o sps.o \
- auto-mode.o cnqf.o
+ auto-mode.o cnqf.o \
+ tee-if.o
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 78ed3ee22555..ec92d1cc0dac 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -309,13 +309,13 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n");
}
- /* Enable Auto Mode */
- if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
+ if (!amd_pmf_init_smart_pc(dev)) {
+ dev_dbg(dev->dev, "Smart PC Solution Enabled\n");
+ } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
amd_pmf_init_auto_mode(dev);
dev_dbg(dev->dev, "Auto Mode Init done\n");
} else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
- /* Enable Cool n Quiet Framework (CnQF) */
ret = amd_pmf_init_cnqf(dev);
if (ret)
dev_warn(dev->dev, "CnQF Init failed\n");
@@ -330,7 +330,9 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
amd_pmf_deinit_sps(dev);
}
- if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
+ if (!dev->smart_pc_enabled) {
+ amd_pmf_deinit_smart_pc(dev);
+ } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
amd_pmf_deinit_auto_mode(dev);
} else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index deba88e6e4c8..bd40458937ba 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -179,6 +179,12 @@ struct amd_pmf_dev {
bool cnqf_enabled;
bool cnqf_supported;
struct notifier_block pwr_src_notifier;
+ /* Smart PC solution builder */
+ struct tee_context *tee_ctx;
+ struct tee_shm *fw_shm_pool;
+ u32 session_id;
+ void *shbuf;
+ bool smart_pc_enabled;
};
struct apmf_sps_prop_granular {
@@ -389,6 +395,13 @@ struct apmf_dyn_slider_output {
struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
} __packed;
+struct ta_pmf_shared_memory {
+ int command_id;
+ int resp_id;
+ u32 pmf_result;
+ u32 if_version;
+};
+
/* Core Layer */
int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
@@ -433,4 +446,7 @@ void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
extern const struct attribute_group cnqf_feature_attribute_group;
+/* Smart PC builder Layer */
+int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
+void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
#endif /* PMF_H */
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
new file mode 100644
index 000000000000..6ec8c3726624
--- /dev/null
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Platform Management Framework Driver - TEE Interface
+ *
+ * Copyright (c) 2023, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ */
+
+#include <linux/tee_drv.h>
+#include <linux/uuid.h>
+#include "pmf.h"
+
+#define MAX_TEE_PARAM 4
+static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
+ 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
+
+static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+ return ver->impl_id == TEE_IMPL_ID_AMDTEE;
+}
+
+static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
+{
+ struct tee_ioctl_open_session_arg sess_arg = {};
+ int rc;
+
+ export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid);
+ sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
+ sess_arg.num_params = 0;
+
+ rc = tee_client_open_session(ctx, &sess_arg, NULL);
+ if (rc < 0 || sess_arg.ret != 0) {
+ pr_err("Failed to open TEE session err:%#x, rc:%d\n", sess_arg.ret, rc);
+ return rc;
+ }
+
+ *id = sess_arg.session;
+
+ return rc;
+}
+
+static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
+{
+ u32 size;
+ int ret;
+
+ dev->tee_ctx = tee_client_open_context(NULL, amd_pmf_amdtee_ta_match, NULL, NULL);
+ if (IS_ERR(dev->tee_ctx)) {
+ dev_err(dev->dev, "Failed to open TEE context\n");
+ return PTR_ERR(dev->tee_ctx);
+ }
+
+ ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id);
+ if (ret) {
+ dev_err(dev->dev, "Failed to open TA session (%d)\n", ret);
+ ret = -EINVAL;
+ goto out_ctx;
+ }
+
+ size = sizeof(struct ta_pmf_shared_memory);
+ dev->fw_shm_pool = tee_shm_alloc_kernel_buf(dev->tee_ctx, size);
+ if (IS_ERR(dev->fw_shm_pool)) {
+ dev_err(dev->dev, "Failed to alloc TEE shared memory\n");
+ ret = PTR_ERR(dev->fw_shm_pool);
+ goto out_sess;
+ }
+
+ dev->shbuf = tee_shm_get_va(dev->fw_shm_pool, 0);
+ if (IS_ERR(dev->shbuf)) {
+ dev_err(dev->dev, "Failed to get TEE virtual address\n");
+ ret = PTR_ERR(dev->shbuf);
+ goto out_shm;
+ }
+ dev_dbg(dev->dev, "TEE init done\n");
+
+ return 0;
+
+out_shm:
+ tee_shm_free(dev->fw_shm_pool);
+out_sess:
+ tee_client_close_session(dev->tee_ctx, dev->session_id);
+out_ctx:
+ tee_client_close_context(dev->tee_ctx);
+
+ return ret;
+}
+
+static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
+{
+ tee_shm_free(dev->fw_shm_pool);
+ tee_client_close_session(dev->tee_ctx, dev->session_id);
+ tee_client_close_context(dev->tee_ctx);
+}
+
+int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
+{
+ return amd_pmf_tee_init(dev);
+}
+
+void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
+{
+ amd_pmf_tee_deinit(dev);
+}
--
2.25.1
^ permalink raw reply related
* [PATCH v4 02/17] platform/x86/amd/pmf: Add support for PMF-TA interaction
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
PMF TA (Trusted Application) loads via the TEE environment into the
AMD ASP.
PMF-TA supports two commands:
1) Init: Initialize the TA with the PMF Smart PC policy binary and
start the policy engine. A policy is a combination of inputs and
outputs, where;
- the inputs are the changing dynamics of the system like the user
behaviour, system heuristics etc.
- the outputs, which are the actions to be set on the system which
lead to better power management and enhanced user experience.
PMF driver acts as a central manager in this case to supply the
inputs required to the TA (either by getting the information from
the other kernel subsystems or from userland)
2) Enact: Enact the output actions from the TA. The action could be
applying a new thermal limit to boost/throttle the power limits or
change system behavior.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/pmf.h | 10 +++
drivers/platform/x86/amd/pmf/tee-if.c | 97 ++++++++++++++++++++++++++-
2 files changed, 106 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index bd40458937ba..a24e34e42032 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -59,6 +59,9 @@
#define ARG_NONE 0
#define AVG_SAMPLE_SIZE 3
+/* TA macros */
+#define PMF_TA_IF_VERSION_MAJOR 1
+
/* AMD PMF BIOS interfaces */
struct apmf_verify_interface {
u16 size;
@@ -184,6 +187,7 @@ struct amd_pmf_dev {
struct tee_shm *fw_shm_pool;
u32 session_id;
void *shbuf;
+ struct delayed_work pb_work;
bool smart_pc_enabled;
};
@@ -395,6 +399,12 @@ struct apmf_dyn_slider_output {
struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
} __packed;
+/* Command ids for TA communication */
+enum ta_pmf_command {
+ TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE,
+ TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES,
+};
+
struct ta_pmf_shared_memory {
int command_id;
int resp_id;
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 6ec8c3726624..4036f435f1e2 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -13,9 +13,96 @@
#include "pmf.h"
#define MAX_TEE_PARAM 4
+
+/* Policy binary actions sampling frequency (in ms) */
+static int pb_actions_ms = MSEC_PER_SEC;
+#ifdef CONFIG_AMD_PMF_DEBUG
+module_param(pb_actions_ms, int, 0644);
+MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (default = 1000ms)");
+#endif
+
static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
+static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
+ struct tee_ioctl_invoke_arg *arg,
+ struct tee_param *param)
+{
+ memset(arg, 0, sizeof(*arg));
+ memset(param, 0, MAX_TEE_PARAM * sizeof(*param));
+
+ arg->func = cmd;
+ arg->session = dev->session_id;
+ arg->num_params = MAX_TEE_PARAM;
+
+ /* Fill invoke cmd params */
+ param[0].u.memref.size = sizeof(struct ta_pmf_shared_memory);
+ param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
+ param[0].u.memref.shm = dev->fw_shm_pool;
+ param[0].u.memref.shm_offs = 0;
+}
+
+static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
+{
+ struct ta_pmf_shared_memory *ta_sm = NULL;
+ struct tee_param param[MAX_TEE_PARAM];
+ struct tee_ioctl_invoke_arg arg;
+ int ret = 0;
+
+ if (!dev->tee_ctx)
+ return -ENODEV;
+
+ ta_sm = dev->shbuf;
+ memset(ta_sm, 0, sizeof(*ta_sm));
+ ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES;
+ ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
+
+ amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES, &arg, param);
+
+ ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
+ if (ret < 0 || arg.ret != 0) {
+ dev_err(dev->dev, "TEE enact cmd failed. err: %x, ret:%d\n", arg.ret, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
+{
+ struct ta_pmf_shared_memory *ta_sm = NULL;
+ struct tee_param param[MAX_TEE_PARAM];
+ struct tee_ioctl_invoke_arg arg;
+ int ret = 0;
+
+ if (!dev->tee_ctx) {
+ dev_err(dev->dev, "Failed to get TEE context\n");
+ return -ENODEV;
+ }
+
+ ta_sm = dev->shbuf;
+ ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE;
+ ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
+
+ amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE, &arg, param);
+
+ ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
+ if (ret < 0 || arg.ret != 0) {
+ dev_err(dev->dev, "Failed to invoke TEE init cmd. err: %x, ret:%d\n", arg.ret, ret);
+ return ret;
+ }
+
+ return ta_sm->pmf_result;
+}
+
+static void amd_pmf_invoke_cmd(struct work_struct *work)
+{
+ struct amd_pmf_dev *dev = container_of(work, struct amd_pmf_dev, pb_work.work);
+
+ amd_pmf_invoke_cmd_enact(dev);
+ schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms));
+}
+
static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
{
return ver->impl_id == TEE_IMPL_ID_AMDTEE;
@@ -96,10 +183,18 @@ static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
{
- return amd_pmf_tee_init(dev);
+ int ret;
+
+ ret = amd_pmf_tee_init(dev);
+ if (ret)
+ return ret;
+
+ INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
+ return 0;
}
void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
{
+ cancel_delayed_work_sync(&dev->pb_work);
amd_pmf_tee_deinit(dev);
}
--
2.25.1
^ permalink raw reply related
* [PATCH v4 03/17] platform/x86/amd/pmf: Change return type of amd_pmf_set_dram_addr()
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
In the current code, the metrics table information was required only
for auto-mode or CnQF at a given time. Hence keeping the return type
of amd_pmf_set_dram_addr() as static made sense.
But with the addition of Smart PC builder feature, the metrics table
information has to be shared by the Smart PC also and this feature
resides outside of core.c.
To make amd_pmf_set_dram_addr() visible outside of core.c make it
as a non-static function and move the allocation of memory for
metrics table from amd_pmf_init_metrics_table() to amd_pmf_set_dram_addr()
as amd_pmf_set_dram_addr() is the common function to set the DRAM
address.
Add a suspend handler that can free up the allocated memory for getting
the metrics table information.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/core.c | 42 ++++++++++++++++++++++-------
drivers/platform/x86/amd/pmf/pmf.h | 1 +
2 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index ec92d1cc0dac..f50b7ec9a625 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -251,29 +251,35 @@ static const struct pci_device_id pmf_pci_ids[] = {
{ }
};
-static void amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
+int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
{
u64 phys_addr;
u32 hi, low;
+ /* Get Metrics Table Address */
+ dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
+ if (!dev->buf)
+ return -ENOMEM;
+
phys_addr = virt_to_phys(dev->buf);
hi = phys_addr >> 32;
low = phys_addr & GENMASK(31, 0);
amd_pmf_send_cmd(dev, SET_DRAM_ADDR_HIGH, 0, hi, NULL);
amd_pmf_send_cmd(dev, SET_DRAM_ADDR_LOW, 0, low, NULL);
+
+ return 0;
}
int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
{
- /* Get Metrics Table Address */
- dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
- if (!dev->buf)
- return -ENOMEM;
+ int ret;
INIT_DELAYED_WORK(&dev->work_buffer, amd_pmf_get_metrics);
- amd_pmf_set_dram_addr(dev);
+ ret = amd_pmf_set_dram_addr(dev);
+ if (ret)
+ return ret;
/*
* Start collecting the metrics data after a small delay
@@ -284,17 +290,35 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
return 0;
}
+static int amd_pmf_suspend_handler(struct device *dev)
+{
+ struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
+
+ /*
+ * Free the buffer allocated for storing the metrics table
+ * information, as will have to allocate it freshly after
+ * resume.
+ */
+ kfree(pdev->buf);
+
+ return 0;
+}
+
static int amd_pmf_resume_handler(struct device *dev)
{
struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
+ int ret;
- if (pdev->buf)
- amd_pmf_set_dram_addr(pdev);
+ if (pdev->buf) {
+ ret = amd_pmf_set_dram_addr(pdev);
+ if (ret)
+ return ret;
+ }
return 0;
}
-static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmf_pm, NULL, amd_pmf_resume_handler);
+static DEFINE_SIMPLE_DEV_PM_OPS(amd_pmf_pm, amd_pmf_suspend_handler, amd_pmf_resume_handler);
static void amd_pmf_init_features(struct amd_pmf_dev *dev)
{
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index a24e34e42032..6a0e4c446dd3 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -421,6 +421,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev);
int amd_pmf_get_power_source(void);
int apmf_install_handler(struct amd_pmf_dev *pmf_dev);
int apmf_os_power_slider_update(struct amd_pmf_dev *dev, u8 flag);
+int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev);
/* SPS Layer */
int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
--
2.25.1
^ permalink raw reply related
* [PATCH v4 04/17] platform/x86/amd/pmf: Add support for PMF Policy Binary
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
PMF Policy binary is a encrypted and signed binary that will be part
of the BIOS. PMF driver via the ACPI interface checks the existence
of Smart PC bit. If the advertised bit is found, PMF driver walks
the acpi namespace to find out the policy binary size and the address
which has to be passed to the TA during the TA init sequence.
The policy binary is comprised of inputs (or the events) and outputs
(or the actions). With the PMF ecosystem, OEMs generate the policy
binary (or could be multiple binaries) that contains a supported set
of inputs and outputs which could be specifically carved out for each
usage segment (or for each user also) that could influence the system
behavior either by enriching the user experience or/and boost/throttle
power limits.
Once the TA init command succeeds, the PMF driver sends the changing
events in the current environment to the TA for a constant sampling
frequency time (the event here could be a lid close or open) and
if the policy binary has corresponding action built within it, the
TA sends the action for it in the subsequent enact command.
If the inputs sent to the TA has no output defined in the policy
binary generated by OEMs, there will be no action to be performed
by the PMF driver.
Example policies:
1) if slider is performance ; set the SPL to 40W
Here PMF driver registers with the platform profile interface and
when the slider position is changed, PMF driver lets the TA know
about this. TA sends back an action to update the Sustained
Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
2) if user_away ; then lock the system
Here PMF driver hooks to the AMD SFH driver to know the user presence
and send the inputs to TA and if the condition is met, the TA sends
the action of locking the system. PMF driver generates a uevent and
based on the udev rule in the userland the system gets locked with
systemctl.
The intent here is to provide the OEM's to make a policy to lock the
system when the user is away ; but the userland can make a choice to
ignore it.
and so on.
The OEMs will have an utility to create numerous such policies and
the policies shall be reviewed by AMD before signing and encrypting
them. Policies are shared between operating systems to have seemless user
experience.
Since all this action has to happen via the "amdtee" driver, currently
there is no caller for it in the kernel which can load the amdtee driver.
Without amdtee driver loading onto the system the "tee" calls shall fail
from the PMF driver. Hence an explicit MODULE_SOFTDEP has been added
to address this.
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/Kconfig | 2 +-
drivers/platform/x86/amd/pmf/acpi.c | 37 +++++++
drivers/platform/x86/amd/pmf/core.c | 9 ++
drivers/platform/x86/amd/pmf/pmf.h | 141 ++++++++++++++++++++++++
drivers/platform/x86/amd/pmf/tee-if.c | 147 +++++++++++++++++++++++++-
5 files changed, 333 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index 32a029e8db80..f246252bddd8 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -9,7 +9,7 @@ config AMD_PMF
depends on POWER_SUPPLY
depends on AMD_NB
select ACPI_PLATFORM_PROFILE
- depends on TEE
+ depends on TEE && AMDTEE
help
This driver provides support for the AMD Platform Management Framework.
The goal is to enhance end user experience by making AMD PCs smarter,
diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index 3fc5e4547d9f..4ec7957eb707 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -286,6 +286,43 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
return 0;
}
+static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
+{
+ struct amd_pmf_dev *dev = data;
+
+ switch (res->type) {
+ case ACPI_RESOURCE_TYPE_ADDRESS64:
+ dev->policy_addr = res->data.address64.address.minimum;
+ dev->policy_sz = res->data.address64.address.address_length;
+ break;
+ case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
+ dev->policy_addr = res->data.fixed_memory32.address;
+ dev->policy_sz = res->data.fixed_memory32.address_length;
+ break;
+ }
+
+ if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
+ pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
+ return AE_ERROR;
+ }
+
+ return AE_OK;
+}
+
+int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
+{
+ acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
+ acpi_status status;
+
+ status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
+ if (ACPI_FAILURE(status)) {
+ dev_err(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
{
acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index f50b7ec9a625..c8f6ec4c2e2c 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -395,6 +395,14 @@ static int amd_pmf_probe(struct platform_device *pdev)
return -ENOMEM;
dev->dev = &pdev->dev;
+ err = apmf_check_smart_pc(dev);
+ if (err)
+ /*
+ * Lets not return from here if Smart PC bit is not advertised in
+ * the BIOS. This way, there will be some amount of power savings
+ * to the user with static slider (if enabled).
+ */
+ pr_err("PMF Smart PC not advertised in BIOS!:%d\n", err);
rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
if (!rdev || !pci_match_id(pmf_pci_ids, rdev)) {
@@ -474,3 +482,4 @@ module_platform_driver(amd_pmf_driver);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("AMD Platform Management Framework Driver");
+MODULE_SOFTDEP("pre: amdtee");
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 6a0e4c446dd3..092be501d4d3 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -14,6 +14,11 @@
#include <linux/acpi.h>
#include <linux/platform_profile.h>
+#define POLICY_BUF_MAX_SZ 0x4b000
+#define POLICY_SIGN_COOKIE 0x31535024
+#define POLICY_COOKIE_OFFSET 0x10
+#define POLICY_COOKIE_LEN 0x14
+
/* APMF Functions */
#define APMF_FUNC_VERIFY_INTERFACE 0
#define APMF_FUNC_GET_SYS_PARAMS 1
@@ -59,8 +64,21 @@
#define ARG_NONE 0
#define AVG_SAMPLE_SIZE 3
+/* Policy Actions */
+#define PMF_POLICY_SPL 2
+#define PMF_POLICY_SPPT 3
+#define PMF_POLICY_FPPT 4
+#define PMF_POLICY_SPPT_APU_ONLY 5
+#define PMF_POLICY_STT_MIN 6
+#define PMF_POLICY_STT_SKINTEMP_APU 7
+#define PMF_POLICY_STT_SKINTEMP_HS2 8
+
/* TA macros */
#define PMF_TA_IF_VERSION_MAJOR 1
+#define TA_PMF_ACTION_MAX 32
+#define TA_PMF_UNDO_MAX 8
+#define TA_OUTPUT_RESERVED_MEM 906
+#define MAX_OPERATION_PARAMS 4
/* AMD PMF BIOS interfaces */
struct apmf_verify_interface {
@@ -183,11 +201,16 @@ struct amd_pmf_dev {
bool cnqf_supported;
struct notifier_block pwr_src_notifier;
/* Smart PC solution builder */
+ unsigned char *policy_buf;
+ u32 policy_sz;
struct tee_context *tee_ctx;
struct tee_shm *fw_shm_pool;
u32 session_id;
void *shbuf;
struct delayed_work pb_work;
+ struct pmf_action_table *prev_data;
+ u64 policy_addr;
+ void *policy_base;
bool smart_pc_enabled;
};
@@ -399,17 +422,134 @@ struct apmf_dyn_slider_output {
struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
} __packed;
+enum smart_pc_status {
+ PMF_SMART_PC_ENABLED,
+ PMF_SMART_PC_DISABLED,
+};
+
+/* Smart PC - TA internals */
+enum ta_slider {
+ TA_BEST_BATTERY,
+ TA_BETTER_BATTERY,
+ TA_BETTER_PERFORMANCE,
+ TA_BEST_PERFORMANCE,
+ TA_MAX,
+};
+
/* Command ids for TA communication */
enum ta_pmf_command {
TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE,
TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES,
};
+enum ta_pmf_error_type {
+ TA_PMF_TYPE_SUCCESS,
+ TA_PMF_ERROR_TYPE_GENERIC,
+ TA_PMF_ERROR_TYPE_CRYPTO,
+ TA_PMF_ERROR_TYPE_CRYPTO_VALIDATE,
+ TA_PMF_ERROR_TYPE_CRYPTO_VERIFY_OEM,
+ TA_PMF_ERROR_TYPE_POLICY_BUILDER,
+ TA_PMF_ERROR_TYPE_PB_CONVERT,
+ TA_PMF_ERROR_TYPE_PB_SETUP,
+ TA_PMF_ERROR_TYPE_PB_ENACT,
+ TA_PMF_ERROR_TYPE_ASD_GET_DEVICE_INFO,
+ TA_PMF_ERROR_TYPE_ASD_GET_DEVICE_PCIE_INFO,
+ TA_PMF_ERROR_TYPE_SYS_DRV_FW_VALIDATION,
+ TA_PMF_ERROR_TYPE_MAX,
+};
+
+struct pmf_action_table {
+ u32 spl; /* in mW */
+ u32 sppt; /* in mW */
+ u32 sppt_apuonly; /* in mW */
+ u32 fppt; /* in mW */
+ u32 stt_minlimit; /* in mW */
+ u32 stt_skintemp_apu; /* in C */
+ u32 stt_skintemp_hs2; /* in C */
+};
+
+/* Input conditions */
+struct ta_pmf_condition_info {
+ u32 power_source;
+ u32 bat_percentage;
+ u32 power_slider;
+ u32 lid_state;
+ bool user_present;
+ u32 rsvd1[2];
+ u32 monitor_count;
+ u32 rsvd2[2];
+ u32 bat_design;
+ u32 full_charge_capacity;
+ int drain_rate;
+ bool user_engaged;
+ u32 device_state;
+ u32 socket_power;
+ u32 skin_temperature;
+ u32 rsvd3[5];
+ u32 ambient_light;
+ u32 length;
+ u32 avg_c0residency;
+ u32 max_c0residency;
+ u32 s0i3_entry;
+ u32 gfx_busy;
+ u32 rsvd4[7];
+ bool camera_state;
+ u32 workload_type;
+ u32 display_type;
+ u32 display_state;
+ u32 rsvd5[150];
+};
+
+struct ta_pmf_load_policy_table {
+ u32 table_size;
+ u8 table[POLICY_BUF_MAX_SZ];
+};
+
+/* TA initialization params */
+struct ta_pmf_init_table {
+ u32 frequency; /* SMU sampling frequency */
+ bool validate;
+ bool sku_check;
+ bool metadata_macrocheck;
+ struct ta_pmf_load_policy_table policies_table;
+};
+
+/* Everything the TA needs to Enact Policies */
+struct ta_pmf_enact_table {
+ struct ta_pmf_condition_info ev_info;
+ u32 name;
+};
+
+struct ta_pmf_action {
+ u32 action_index;
+ u32 value;
+};
+
+/* Output actions from TA */
+struct ta_pmf_enact_result {
+ u32 actions_count;
+ struct ta_pmf_action actions_list[TA_PMF_ACTION_MAX];
+ u32 undo_count;
+ struct ta_pmf_action undo_list[TA_PMF_UNDO_MAX];
+};
+
+union ta_pmf_input {
+ struct ta_pmf_enact_table enact_table;
+ struct ta_pmf_init_table init_table;
+};
+
+union ta_pmf_output {
+ struct ta_pmf_enact_result policy_apply_table;
+ u32 rsvd[TA_OUTPUT_RESERVED_MEM];
+};
+
struct ta_pmf_shared_memory {
int command_id;
int resp_id;
u32 pmf_result;
u32 if_version;
+ union ta_pmf_output pmf_output;
+ union ta_pmf_input pmf_input;
};
/* Core Layer */
@@ -460,4 +600,5 @@ extern const struct attribute_group cnqf_feature_attribute_group;
/* Smart PC builder Layer */
int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
+int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
#endif /* PMF_H */
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 4036f435f1e2..92290d51a4af 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -42,9 +42,77 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
param[0].u.memref.shm_offs = 0;
}
+static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
+{
+ u32 val;
+ int idx;
+
+ for (idx = 0; idx < out->actions_count; idx++) {
+ val = out->actions_list[idx].value;
+ switch (out->actions_list[idx].action_index) {
+ case PMF_POLICY_SPL:
+ if (dev->prev_data->spl != val) {
+ amd_pmf_send_cmd(dev, SET_SPL, false, val, NULL);
+ dev_dbg(dev->dev, "update SPL : %u\n", val);
+ dev->prev_data->spl = val;
+ }
+ break;
+
+ case PMF_POLICY_SPPT:
+ if (dev->prev_data->sppt != val) {
+ amd_pmf_send_cmd(dev, SET_SPPT, false, val, NULL);
+ dev_dbg(dev->dev, "update SPPT : %u\n", val);
+ dev->prev_data->sppt = val;
+ }
+ break;
+
+ case PMF_POLICY_FPPT:
+ if (dev->prev_data->fppt != val) {
+ amd_pmf_send_cmd(dev, SET_FPPT, false, val, NULL);
+ dev_dbg(dev->dev, "update FPPT : %u\n", val);
+ dev->prev_data->fppt = val;
+ }
+ break;
+
+ case PMF_POLICY_SPPT_APU_ONLY:
+ if (dev->prev_data->sppt_apuonly != val) {
+ amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, val, NULL);
+ dev_dbg(dev->dev, "update SPPT_APU_ONLY : %u\n", val);
+ dev->prev_data->sppt_apuonly = val;
+ }
+ break;
+
+ case PMF_POLICY_STT_MIN:
+ if (dev->prev_data->stt_minlimit != val) {
+ amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, val, NULL);
+ dev_dbg(dev->dev, "update STT_MIN : %u\n", val);
+ dev->prev_data->stt_minlimit = val;
+ }
+ break;
+
+ case PMF_POLICY_STT_SKINTEMP_APU:
+ if (dev->prev_data->stt_skintemp_apu != val) {
+ amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
+ dev_dbg(dev->dev, "update STT_SKINTEMP_APU : %u\n", val);
+ dev->prev_data->stt_skintemp_apu = val;
+ }
+ break;
+
+ case PMF_POLICY_STT_SKINTEMP_HS2:
+ if (dev->prev_data->stt_skintemp_hs2 != val) {
+ amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
+ dev_dbg(dev->dev, "update STT_SKINTEMP_HS2 : %u\n", val);
+ dev->prev_data->stt_skintemp_hs2 = val;
+ }
+ break;
+ }
+ }
+}
+
static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
{
struct ta_pmf_shared_memory *ta_sm = NULL;
+ struct ta_pmf_enact_result *out = NULL;
struct tee_param param[MAX_TEE_PARAM];
struct tee_ioctl_invoke_arg arg;
int ret = 0;
@@ -52,7 +120,10 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
if (!dev->tee_ctx)
return -ENODEV;
+ memset(dev->shbuf, 0, dev->policy_sz);
ta_sm = dev->shbuf;
+ out = &ta_sm->pmf_output.policy_apply_table;
+
memset(ta_sm, 0, sizeof(*ta_sm));
ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES;
ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
@@ -65,6 +136,12 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
return ret;
}
+ if (ta_sm->pmf_result == TA_PMF_TYPE_SUCCESS && out->actions_count) {
+ dev_dbg(dev->dev, "action count:%u result:%x\n", out->actions_count,
+ ta_sm->pmf_result);
+ amd_pmf_apply_policies(dev, out);
+ }
+
return 0;
}
@@ -72,6 +149,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
{
struct ta_pmf_shared_memory *ta_sm = NULL;
struct tee_param param[MAX_TEE_PARAM];
+ struct ta_pmf_init_table *in = NULL;
struct tee_ioctl_invoke_arg arg;
int ret = 0;
@@ -80,10 +158,21 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
return -ENODEV;
}
+ dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
+ memset(dev->shbuf, 0, dev->policy_sz);
ta_sm = dev->shbuf;
+ in = &ta_sm->pmf_input.init_table;
+
ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE;
ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
+ in->metadata_macrocheck = false;
+ in->sku_check = false;
+ in->validate = true;
+ in->frequency = pb_actions_ms;
+ in->policies_table.table_size = dev->policy_sz;
+
+ memcpy(in->policies_table.table, dev->policy_buf, dev->policy_sz);
amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE, &arg, param);
ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
@@ -103,6 +192,52 @@ static void amd_pmf_invoke_cmd(struct work_struct *work)
schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms));
}
+static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
+{
+ u32 cookie, length;
+ int res;
+
+ cookie = readl(dev->policy_buf + POLICY_COOKIE_OFFSET);
+ length = readl(dev->policy_buf + POLICY_COOKIE_LEN);
+
+ if (cookie != POLICY_SIGN_COOKIE || !length)
+ return -EINVAL;
+
+ /* Update the actual length */
+ dev->policy_sz = length + 512;
+ res = amd_pmf_invoke_cmd_init(dev);
+ if (res == TA_PMF_TYPE_SUCCESS) {
+ /* Now its safe to announce that smart pc is enabled */
+ dev->smart_pc_enabled = PMF_SMART_PC_ENABLED;
+ /*
+ * Start collecting the data from TA FW after a small delay
+ * or else, we might end up getting stale values.
+ */
+ schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3));
+ } else {
+ dev_err(dev->dev, "ta invoke cmd init failed err: %x\n", res);
+ dev->smart_pc_enabled = PMF_SMART_PC_DISABLED;
+ return res;
+ }
+
+ return 0;
+}
+
+static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
+{
+ dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
+ if (!dev->policy_buf)
+ return -ENOMEM;
+
+ dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
+ if (!dev->policy_base)
+ return -ENOMEM;
+
+ memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
+
+ return amd_pmf_start_policy_engine(dev);
+}
+
static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
{
return ver->impl_id == TEE_IMPL_ID_AMDTEE;
@@ -146,7 +281,7 @@ static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
goto out_ctx;
}
- size = sizeof(struct ta_pmf_shared_memory);
+ size = sizeof(struct ta_pmf_shared_memory) + dev->policy_sz;
dev->fw_shm_pool = tee_shm_alloc_kernel_buf(dev->tee_ctx, size);
if (IS_ERR(dev->fw_shm_pool)) {
dev_err(dev->dev, "Failed to alloc TEE shared memory\n");
@@ -190,11 +325,19 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
return ret;
INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
- return 0;
+ amd_pmf_set_dram_addr(dev);
+ amd_pmf_get_bios_buffer(dev);
+ dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
+ if (!dev->prev_data)
+ return -ENOMEM;
+
+ return dev->smart_pc_enabled;
}
void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
{
+ kfree(dev->prev_data);
+ kfree(dev->policy_buf);
cancel_delayed_work_sync(&dev->pb_work);
amd_pmf_tee_deinit(dev);
}
--
2.25.1
^ permalink raw reply related
* [PATCH v4 05/17] platform/x86/amd/pmf: change amd_pmf_init_features() call sequence
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
To sideload pmf policy binaries, the Smart PC Solution Builder provides a
debugfs file called "update_policy"; that gets created under a new debugfs
directory called "pb" and this new directory has to be associated with
existing parent directory for PMF driver called "amd_pmf".
In the current code structure, amd_pmf_dbgfs_register() is called after
amd_pmf_init_features(). This will not help when the Smart PC builder
feature has to be assoicated to the parent directory.
Hence change the order of amd_pmf_dbgfs_register() and call it before
amd_pmf_init_features() so that when the Smart PC init happens, it has the
parent debugfs directory to get itself hooked.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index c8f6ec4c2e2c..4b8156033fa6 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -442,9 +442,9 @@ static int amd_pmf_probe(struct platform_device *pdev)
apmf_acpi_init(dev);
platform_set_drvdata(pdev, dev);
+ amd_pmf_dbgfs_register(dev);
amd_pmf_init_features(dev);
apmf_install_handler(dev);
- amd_pmf_dbgfs_register(dev);
dev_info(dev->dev, "registered PMF device successfully\n");
--
2.25.1
^ permalink raw reply related
* [PATCH v4 06/17] platform/x86/amd/pmf: Add support to get inputs from other subsystems
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
PMF driver sends changing inputs from each subystem to TA for evaluating
the conditions in the policy binary.
Add initial support of plumbing in the PMF driver for Smart PC to get
information from other subsystems in the kernel.
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/Makefile | 2 +-
drivers/platform/x86/amd/pmf/pmf.h | 18 ++++
drivers/platform/x86/amd/pmf/spc.c | 120 ++++++++++++++++++++++++++
drivers/platform/x86/amd/pmf/tee-if.c | 3 +
4 files changed, 142 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/x86/amd/pmf/spc.c
diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
index d2746ee7369f..6b26e48ce8ad 100644
--- a/drivers/platform/x86/amd/pmf/Makefile
+++ b/drivers/platform/x86/amd/pmf/Makefile
@@ -7,4 +7,4 @@
obj-$(CONFIG_AMD_PMF) += amd-pmf.o
amd-pmf-objs := core.o acpi.o sps.o \
auto-mode.o cnqf.o \
- tee-if.o
+ tee-if.o spc.o
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 092be501d4d3..a4a73b845c09 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -150,6 +150,21 @@ struct smu_pmf_metrics {
u16 infra_gfx_maxfreq; /* in MHz */
u16 skin_temp; /* in centi-Celsius */
u16 device_state;
+ u16 curtemp; /* in centi-Celsius */
+ u16 filter_alpha_value;
+ u16 avg_gfx_clkfrequency;
+ u16 avg_fclk_frequency;
+ u16 avg_gfx_activity;
+ u16 avg_socclk_frequency;
+ u16 avg_vclk_frequency;
+ u16 avg_vcn_activity;
+ u16 avg_dram_reads;
+ u16 avg_dram_writes;
+ u16 avg_socket_power;
+ u16 avg_core_power[2];
+ u16 avg_core_c0residency[16];
+ u16 spare1;
+ u32 metrics_counter;
} __packed;
enum amd_stt_skin_temp {
@@ -601,4 +616,7 @@ extern const struct attribute_group cnqf_feature_attribute_group;
int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
+
+/* Smart PC - TA interfaces */
+void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
#endif /* PMF_H */
diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
new file mode 100644
index 000000000000..bd5061fdfdbe
--- /dev/null
+++ b/drivers/platform/x86/amd/pmf/spc.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AMD Platform Management Framework Driver - Smart PC Capabilities
+ *
+ * Copyright (c) 2023, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ * Patil Rajesh Reddy <Patil.Reddy@amd.com>
+ */
+
+#include <acpi/button.h>
+#include <linux/power_supply.h>
+#include <linux/units.h>
+#include "pmf.h"
+
+static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
+{
+ u16 max, avg = 0;
+ int i;
+
+ memset(dev->buf, 0, sizeof(dev->m_table));
+ amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
+ memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
+
+ in->ev_info.socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
+ in->ev_info.skin_temperature = dev->m_table.skin_temp;
+
+ /* Get the avg and max C0 residency of all the cores */
+ max = dev->m_table.avg_core_c0residency[0];
+ for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
+ avg += dev->m_table.avg_core_c0residency[i];
+ if (dev->m_table.avg_core_c0residency[i] > max)
+ max = dev->m_table.avg_core_c0residency[i];
+ }
+
+ avg = DIV_ROUND_CLOSEST(avg, ARRAY_SIZE(dev->m_table.avg_core_c0residency));
+ in->ev_info.avg_c0residency = avg;
+ in->ev_info.max_c0residency = max;
+ in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
+}
+
+static const char * const pmf_battery_supply_name[] = {
+ "BATT",
+ "BAT0",
+};
+
+static int amd_pmf_get_battery_prop(enum power_supply_property prop)
+{
+ union power_supply_propval value;
+ struct power_supply *psy;
+ int i, ret;
+
+ for (i = 0; i < ARRAY_SIZE(pmf_battery_supply_name); i++) {
+ psy = power_supply_get_by_name(pmf_battery_supply_name[i]);
+ if (!psy)
+ continue;
+
+ ret = power_supply_get_property(psy, prop, &value);
+ if (ret) {
+ power_supply_put(psy);
+ return ret;
+ }
+ }
+
+ return value.intval;
+}
+
+static int amd_pmf_get_battery_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
+{
+ int val;
+
+ val = amd_pmf_get_battery_prop(POWER_SUPPLY_PROP_PRESENT);
+ if (val >= 0 && val != 1)
+ return -ENODEV;
+
+ in->ev_info.bat_percentage = amd_pmf_get_battery_prop(POWER_SUPPLY_PROP_CAPACITY);
+ /* all values in mWh metrics */
+ in->ev_info.bat_design = amd_pmf_get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN) /
+ MILLIWATT_PER_WATT;
+ in->ev_info.full_charge_capacity = amd_pmf_get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL) /
+ MILLIWATT_PER_WATT;
+ in->ev_info.drain_rate = amd_pmf_get_battery_prop(POWER_SUPPLY_PROP_POWER_NOW) /
+ MILLIWATT_PER_WATT;
+
+ return 0;
+}
+
+static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
+{
+ int val;
+
+ switch (dev->current_profile) {
+ case PLATFORM_PROFILE_PERFORMANCE:
+ val = TA_BEST_PERFORMANCE;
+ break;
+ case PLATFORM_PROFILE_BALANCED:
+ val = TA_BETTER_PERFORMANCE;
+ break;
+ case PLATFORM_PROFILE_LOW_POWER:
+ val = TA_BEST_BATTERY;
+ break;
+ default:
+ dev_err(dev->dev, "Unknown Platform Profile.\n");
+ return -EOPNOTSUPP;
+ }
+ in->ev_info.power_slider = val;
+
+ return 0;
+}
+
+void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
+{
+ /* TA side lid open is 1 and close is 0, hence the ! here */
+ in->ev_info.lid_state = !acpi_lid_open();
+ in->ev_info.power_source = amd_pmf_get_power_source();
+ amd_pmf_get_smu_info(dev, in);
+ amd_pmf_get_battery_info(dev, in);
+ amd_pmf_get_slider_info(dev, in);
+}
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 92290d51a4af..da6520f202e7 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -113,6 +113,7 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
{
struct ta_pmf_shared_memory *ta_sm = NULL;
struct ta_pmf_enact_result *out = NULL;
+ struct ta_pmf_enact_table *in = NULL;
struct tee_param param[MAX_TEE_PARAM];
struct tee_ioctl_invoke_arg arg;
int ret = 0;
@@ -123,11 +124,13 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
memset(dev->shbuf, 0, dev->policy_sz);
ta_sm = dev->shbuf;
out = &ta_sm->pmf_output.policy_apply_table;
+ in = &ta_sm->pmf_input.enact_table;
memset(ta_sm, 0, sizeof(*ta_sm));
ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES;
ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
+ amd_pmf_populate_ta_inputs(dev, in);
amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES, &arg, param);
ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
--
2.25.1
^ permalink raw reply related
* [PATCH v4 07/17] platform/x86/amd/pmf: Add support update p3t limit
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
P3T (Peak Package Power Limit) is a metric within the SMU controller
that can influence the power limits. Add support from the driver
to update P3T limits accordingly.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/pmf.h | 3 +++
drivers/platform/x86/amd/pmf/tee-if.c | 8 ++++++++
2 files changed, 11 insertions(+)
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index a4a73b845c09..781989c7dddd 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -49,6 +49,7 @@
#define GET_STT_MIN_LIMIT 0x1F
#define GET_STT_LIMIT_APU 0x20
#define GET_STT_LIMIT_HS2 0x21
+#define SET_P3T 0x23 /* P3T: Peak Package Power Limit */
/* OS slider update notification */
#define DC_BEST_PERF 0
@@ -72,6 +73,7 @@
#define PMF_POLICY_STT_MIN 6
#define PMF_POLICY_STT_SKINTEMP_APU 7
#define PMF_POLICY_STT_SKINTEMP_HS2 8
+#define PMF_POLICY_P3T 38
/* TA macros */
#define PMF_TA_IF_VERSION_MAJOR 1
@@ -481,6 +483,7 @@ struct pmf_action_table {
u32 stt_minlimit; /* in mW */
u32 stt_skintemp_apu; /* in C */
u32 stt_skintemp_hs2; /* in C */
+ u32 p3t_limit; /* in mW */
};
/* Input conditions */
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index da6520f202e7..6f4a59950b27 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -105,6 +105,14 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
dev->prev_data->stt_skintemp_hs2 = val;
}
break;
+
+ case PMF_POLICY_P3T:
+ if (dev->prev_data->p3t_limit != val) {
+ amd_pmf_send_cmd(dev, SET_P3T, false, val, NULL);
+ dev_dbg(dev->dev, "update P3T : %u\n", val);
+ dev->prev_data->p3t_limit = val;
+ }
+ break;
}
}
}
--
2.25.1
^ permalink raw reply related
* [PATCH v4 08/17] platform/x86/amd/pmf: Add support to update system state
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
PMF driver based on the output actions from the TA can request to update
the system states like entering s0i3, lock screen etc. by generating
an uevent. Based on the udev rules set in the userspace the event id
matching the uevent shall get updated accordingly using the systemctl.
Sample udev rules under Documentation/admin-guide/pmf.rst.
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
Documentation/admin-guide/index.rst | 1 +
Documentation/admin-guide/pmf.rst | 24 +++++++++++++++++++
drivers/platform/x86/amd/pmf/pmf.h | 9 +++++++
drivers/platform/x86/amd/pmf/tee-if.c | 34 +++++++++++++++++++++++++++
4 files changed, 68 insertions(+)
create mode 100644 Documentation/admin-guide/pmf.rst
diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
index 43ea35613dfc..fb40a1f6f79e 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -119,6 +119,7 @@ configure specific aspects of kernel behavior to your liking.
parport
perf-security
pm/index
+ pmf
pnp
rapidio
ras
diff --git a/Documentation/admin-guide/pmf.rst b/Documentation/admin-guide/pmf.rst
new file mode 100644
index 000000000000..9ee729ffc19b
--- /dev/null
+++ b/Documentation/admin-guide/pmf.rst
@@ -0,0 +1,24 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Set udev rules for PMF Smart PC Builder
+---------------------------------------
+
+AMD PMF(Platform Management Framework) Smart PC Solution builder has to set the system states
+like S0i3, Screen lock, hibernate etc, based on the output actions provided by the PMF
+TA (Trusted Application).
+
+In order for this to work the PMF driver generates a uevent for userspace to react to. Below are
+sample udev rules that can facilitate this experience when a machine has PMF Smart PC solution builder
+enabled.
+
+Please add the following line(s) to
+``/etc/udev/rules.d/99-local.rules``::
+
+ DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="0", RUN+="/usr/bin/systemctl suspend"
+ DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="1", RUN+="/usr/bin/systemctl hibernate"
+ DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2", RUN+="/bin/loginctl lock-sessions"
+
+EVENT_ID values:
+0= Put the system to S0i3/S2Idle
+1= Put the system to hibernate
+2= Lock the screen
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 781989c7dddd..fb9ce2593236 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -73,6 +73,7 @@
#define PMF_POLICY_STT_MIN 6
#define PMF_POLICY_STT_SKINTEMP_APU 7
#define PMF_POLICY_STT_SKINTEMP_HS2 8
+#define PMF_POLICY_SYSTEM_STATE 9
#define PMF_POLICY_P3T 38
/* TA macros */
@@ -445,6 +446,13 @@ enum smart_pc_status {
};
/* Smart PC - TA internals */
+enum system_state {
+ SYSTEM_STATE_S0i3,
+ SYSTEM_STATE_S4,
+ SYSTEM_STATE_SCREEN_LOCK,
+ SYSTEM_STATE_MAX,
+};
+
enum ta_slider {
TA_BEST_BATTERY,
TA_BETTER_BATTERY,
@@ -476,6 +484,7 @@ enum ta_pmf_error_type {
};
struct pmf_action_table {
+ enum system_state system_state;
u32 spl; /* in mW */
u32 sppt; /* in mW */
u32 sppt_apuonly; /* in mW */
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 6f4a59950b27..d48f980fb1db 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -24,6 +24,20 @@ MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (defau
static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
+static const char *amd_pmf_uevent_as_str(unsigned int state)
+{
+ switch (state) {
+ case SYSTEM_STATE_S0i3:
+ return "S0i3";
+ case SYSTEM_STATE_S4:
+ return "S4";
+ case SYSTEM_STATE_SCREEN_LOCK:
+ return "SCREEN_LOCK";
+ default:
+ return "Unknown Smart PC event";
+ }
+}
+
static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
struct tee_ioctl_invoke_arg *arg,
struct tee_param *param)
@@ -42,6 +56,20 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
param[0].u.memref.shm_offs = 0;
}
+static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
+{
+ char *envp[2] = {};
+
+ envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
+ if (!envp[0])
+ return -EINVAL;
+
+ kobject_uevent_env(&dev->dev->kobj, KOBJ_CHANGE, envp);
+
+ kfree(envp[0]);
+ return 0;
+}
+
static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
{
u32 val;
@@ -113,6 +141,12 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
dev->prev_data->p3t_limit = val;
}
break;
+
+ case PMF_POLICY_SYSTEM_STATE:
+ amd_pmf_update_uevents(dev, val);
+ dev_dbg(dev->dev, "update SYSTEM_STATE : %s\n",
+ amd_pmf_uevent_as_str(val));
+ break;
}
}
}
--
2.25.1
^ permalink raw reply related
* [PATCH v4 09/17] platform/x86/amd/pmf: Make source_as_str() as non-static
From: Shyam Sundar S K @ 2023-10-18 7:02 UTC (permalink / raw)
To: hdegoede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel, Shyam Sundar S K
In-Reply-To: <20231018070241.2041529-1-Shyam-sundar.S-k@amd.com>
Add amd_pmf prefix to source_as_str() function, so that the function name
does not look generic. As this is a helper function make it as non-static
so that it can be reused across multiple PMF features.
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
drivers/platform/x86/amd/pmf/pmf.h | 1 +
drivers/platform/x86/amd/pmf/sps.c | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index fb9ce2593236..216a9f795436 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -600,6 +600,7 @@ int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev,
struct apmf_static_slider_granular_output *output);
bool is_pprof_balanced(struct amd_pmf_dev *pmf);
int amd_pmf_power_slider_update_event(struct amd_pmf_dev *dev);
+const char *amd_pmf_source_as_str(unsigned int state);
int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx);
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index a70e67749be3..33e23e25c8b1 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -27,7 +27,7 @@ static const char *slider_as_str(unsigned int state)
}
}
-static const char *source_as_str(unsigned int state)
+const char *amd_pmf_source_as_str(unsigned int state)
{
switch (state) {
case POWER_SOURCE_AC:
@@ -47,7 +47,8 @@ static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *dat
for (i = 0; i < POWER_SOURCE_MAX; i++) {
for (j = 0; j < POWER_MODE_MAX; j++) {
- pr_debug("--- Source:%s Mode:%s ---\n", source_as_str(i), slider_as_str(j));
+ pr_debug("--- Source:%s Mode:%s ---\n", amd_pmf_source_as_str(i),
+ slider_as_str(j));
pr_debug("SPL: %u mW\n", data->prop[i][j].spl);
pr_debug("SPPT: %u mW\n", data->prop[i][j].sppt);
pr_debug("SPPT_ApuOnly: %u mW\n", data->prop[i][j].sppt_apu_only);
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox