* [PATCH v3 0/4] platform/chrome: Add Type C connector class driver
@ 2020-02-20 0:30 Prashant Malani
2020-02-20 0:30 ` [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver Prashant Malani
2020-02-20 0:30 ` [PATCH v3 2/4] platform/chrome: Add Type C connector class driver Prashant Malani
0 siblings, 2 replies; 13+ messages in thread
From: Prashant Malani @ 2020-02-20 0:30 UTC (permalink / raw)
To: linux-kernel
Cc: heikki.krogerus, enric.balletbo, bleung, Prashant Malani,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Mark Rutland, Rob Herring
The following series introduces a Type C port driver for Chrome OS devices
that have an EC (Embedded Controller). It derives port information from
ACPI or DT entries. This patch series adds basic support, including
registering ports, and setting certain basic attributes.
v2: https://lkml.org/lkml/2020/2/7/646
v1: https://lkml.org/lkml/2020/2/5/676
Changes in v3:
- Fix DT bindings entry in Documentation to use usb-connector binding.
- Fixed minor code nits in driver file.
Prashant Malani (4):
dt-bindings: Add cros-ec Type C port driver
platform/chrome: Add Type C connector class driver
platform/chrome: typec: Get PD_CONTROL cmd version
platform/chrome: typec: Update port info from EC
.../bindings/chrome/google,cros-ec-typec.yaml | 86 +++++
drivers/platform/chrome/Kconfig | 11 +
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_typec.c | 340 ++++++++++++++++++
4 files changed, 438 insertions(+)
create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
create mode 100644 drivers/platform/chrome/cros_ec_typec.c
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-20 0:30 [PATCH v3 0/4] platform/chrome: Add Type C connector class driver Prashant Malani
@ 2020-02-20 0:30 ` Prashant Malani
2020-02-27 8:41 ` Stephen Boyd
2020-02-27 15:12 ` Heikki Krogerus
2020-02-20 0:30 ` [PATCH v3 2/4] platform/chrome: Add Type C connector class driver Prashant Malani
1 sibling, 2 replies; 13+ messages in thread
From: Prashant Malani @ 2020-02-20 0:30 UTC (permalink / raw)
To: linux-kernel
Cc: heikki.krogerus, enric.balletbo, bleung, Prashant Malani,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Mark Rutland, Rob Herring
Some Chrome OS devices with Embedded Controllers (EC) can read and
modify Type C port state.
Add an entry in the DT Bindings documentation that lists out the logical
device and describes the relevant port information, to be used by the
corresponding driver.
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
Changes in v3:
- Fixed license identifier.
- Renamed "port" to "connector".
- Made "connector" be a "usb-c-connector" compatible property.
- Updated port-number description to explain min and max values,
and removed $ref which was causing dt_binding_check errors.
- Fixed power-role, data-role and try-power-role details to make
dt_binding_check pass.
- Fixed example to include parent EC SPI DT Node.
Changes in v2:
- No changes. Patch first introduced in v2 of series.
.../bindings/chrome/google,cros-ec-typec.yaml | 86 +++++++++++++++++++
1 file changed, 86 insertions(+)
create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
new file mode 100644
index 00000000000000..97fd982612f120
--- /dev/null
+++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/chrome/google,cros-ec-typec.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Google Chrome OS EC(Embedded Controller) Type C port driver.
+
+maintainers:
+ - Benson Leung <bleung@chromium.org>
+ - Prashant Malani <pmalani@chromium.org>
+
+description:
+ Chrome OS devices have an Embedded Controller(EC) which has access to
+ Type C port state. This node is intended to allow the host to read and
+ control the Type C ports. The node for this device should be under a
+ cros-ec node like google,cros-ec-spi.
+
+properties:
+ compatible:
+ const: google,cros-ec-typec
+
+ connector:
+ description: A node that represents a physical Type C connector port
+ on the device.
+ type: object
+ properties:
+ compatible:
+ const: usb-c-connector
+ port-number:
+ description: The number used by the Chrome OS EC to identify
+ this type C port. Valid values are 0 - (EC_USB_PD_MAX_PORTS - 1).
+ power-role:
+ description: Determines the power role that the Type C port will
+ adopt.
+ maxItems: 1
+ contains:
+ enum:
+ - sink
+ - source
+ - dual
+ data-role:
+ description: Determines the data role that the Type C port will
+ adopt.
+ maxItems: 1
+ contains:
+ enum:
+ - host
+ - device
+ - dual
+ try-power-role:
+ description: Determines the preferred power role of the Type C port.
+ maxItems: 1
+ contains:
+ enum:
+ - sink
+ - source
+ - dual
+
+ required:
+ - port-number
+ - power-role
+ - data-role
+ - try-power-role
+
+required:
+ - compatible
+ - connector
+
+examples:
+ - |+
+ cros_ec: ec@0 {
+ compatible = "google,cros-ec-spi";
+
+ typec {
+ compatible = "google,cros-ec-typec";
+
+ usb_con: connector {
+ compatible = "usb-c-connector";
+ port-number = <0>;
+ power-role = "dual";
+ data-role = "dual";
+ try-power-role = "source";
+ };
+ };
+ };
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/4] platform/chrome: Add Type C connector class driver
2020-02-20 0:30 [PATCH v3 0/4] platform/chrome: Add Type C connector class driver Prashant Malani
2020-02-20 0:30 ` [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver Prashant Malani
@ 2020-02-20 0:30 ` Prashant Malani
2020-02-27 14:25 ` Heikki Krogerus
1 sibling, 1 reply; 13+ messages in thread
From: Prashant Malani @ 2020-02-20 0:30 UTC (permalink / raw)
To: linux-kernel
Cc: heikki.krogerus, enric.balletbo, bleung, Prashant Malani,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Mark Rutland, Rob Herring
Add a driver to implement the Type C connector class for Chrome OS
devices with ECs (Embedded Controllers).
The driver relies on firmware device specifications for various port
attributes. On ACPI platforms, this is specified using the logical
device with HID GOOG0014. On DT platforms, this is specified using the
DT node with compatible string "google,cros-ec-typec".
This patch reads the device FW node and uses the port attributes to
register the typec ports with the Type C connector class framework, but
doesn't do much else.
Subsequent patches will add more functionality to the driver, including
obtaining current port information (polarity, vconn role, current power
role etc.) after querying the EC.
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
Changes in v3:
- Fixed minor spacing nits, and moved a modification to probe() if check
from later patch to here instead.
Changes in v2:
- Updated Kconfig to default to MFD_CROS_EC_DEV.
- Fixed code comments.
- Moved get_num_ports() code into probe().
- Added module author.
drivers/platform/chrome/Kconfig | 11 ++
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_typec.c | 221 ++++++++++++++++++++++++
3 files changed, 233 insertions(+)
create mode 100644 drivers/platform/chrome/cros_ec_typec.c
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 5f57282a28da00..2320a4f0d93019 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -214,6 +214,17 @@ config CROS_EC_SYSFS
To compile this driver as a module, choose M here: the
module will be called cros_ec_sysfs.
+config CROS_EC_TYPEC
+ tristate "ChromeOS EC Type-C Connector Control"
+ depends on MFD_CROS_EC_DEV && TYPEC
+ default MFD_CROS_EC_DEV
+ help
+ If you say Y here, you get support for accessing Type C connector
+ information from the Chrome OS EC.
+
+ To compile this driver as a module, choose M here: the module will be
+ called cros_ec_typec.
+
config CROS_USBPD_LOGGER
tristate "Logging driver for USB PD charger"
depends on CHARGER_CROS_USBPD
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index aacd5920d8a180..caf2a9cdb5e6d1 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
+obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
new file mode 100644
index 00000000000000..6cac41f246b99f
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2020 Google LLC
+ *
+ * This driver provides the ability to view and manage Type C ports through the
+ * Chrome OS EC.
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+#include <linux/usb/typec.h>
+
+#define DRV_NAME "cros-ec-typec"
+
+/* Platform-specific data for the Chrome OS EC Type C controller. */
+struct cros_typec_data {
+ struct device *dev;
+ struct cros_ec_device *ec;
+ int num_ports;
+ /* Array of ports, indexed by port number. */
+ struct typec_port *ports[EC_USB_PD_MAX_PORTS];
+};
+
+static int cros_typec_parse_port_props(struct typec_capability *cap,
+ struct fwnode_handle *fwnode,
+ struct device *dev)
+{
+ const char *buf;
+ int ret;
+
+ memset(cap, 0, sizeof(*cap));
+ ret = fwnode_property_read_string(fwnode, "power-role", &buf);
+ if (ret) {
+ dev_err(dev, "power-role not found: %d\n", ret);
+ return ret;
+ }
+
+ ret = typec_find_port_power_role(buf);
+ if (ret < 0)
+ return ret;
+ cap->type = ret;
+
+ ret = fwnode_property_read_string(fwnode, "data-role", &buf);
+ if (ret) {
+ dev_err(dev, "data-role not found: %d\n", ret);
+ return ret;
+ }
+
+ ret = typec_find_port_data_role(buf);
+ if (ret < 0)
+ return ret;
+ cap->data = ret;
+
+ ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
+ if (ret) {
+ dev_err(dev, "try-power-role not found: %d\n", ret);
+ return ret;
+ }
+
+ ret = typec_find_power_role(buf);
+ if (ret < 0)
+ return ret;
+ cap->prefer_role = ret;
+
+ cap->fwnode = fwnode;
+
+ return 0;
+}
+
+static int cros_typec_init_ports(struct cros_typec_data *typec)
+{
+ struct device *dev = typec->dev;
+ struct typec_capability cap;
+ struct fwnode_handle *fwnode;
+ int ret;
+ int i;
+ int nports;
+ u32 port_num;
+
+ nports = device_get_child_node_count(dev);
+ if (nports == 0) {
+ dev_err(dev, "No port entries found.\n");
+ return -ENODEV;
+ }
+
+ device_for_each_child_node(dev, fwnode) {
+ if (fwnode_property_read_u32(fwnode, "port-number",
+ &port_num)) {
+ dev_err(dev, "No port-number for port, skipping.\n");
+ ret = -EINVAL;
+ goto unregister_ports;
+ }
+
+ if (port_num >= typec->num_ports) {
+ dev_err(dev, "Invalid port number.\n");
+ ret = -EINVAL;
+ goto unregister_ports;
+ }
+
+ dev_dbg(dev, "Registering port %d\n", port_num);
+
+ ret = cros_typec_parse_port_props(&cap, fwnode, dev);
+ if (ret < 0)
+ goto unregister_ports;
+
+ typec->ports[port_num] = typec_register_port(dev, &cap);
+ if (IS_ERR(typec->ports[port_num])) {
+ dev_err(dev, "Failed to register port %d\n", port_num);
+ ret = PTR_ERR(typec->ports[port_num]);
+ goto unregister_ports;
+ }
+ }
+
+ return 0;
+
+unregister_ports:
+ for (i = 0; i < typec->num_ports; i++)
+ typec_unregister_port(typec->ports[i]);
+ return ret;
+}
+
+static int cros_typec_ec_command(struct cros_typec_data *typec,
+ unsigned int version,
+ unsigned int command,
+ void *outdata,
+ unsigned int outsize,
+ void *indata,
+ unsigned int insize)
+{
+ struct cros_ec_command *msg;
+ int ret;
+
+ msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ msg->version = version;
+ msg->command = command;
+ msg->outsize = outsize;
+ msg->insize = insize;
+
+ if (outsize)
+ memcpy(msg->data, outdata, outsize);
+
+ ret = cros_ec_cmd_xfer_status(typec->ec, msg);
+ if (ret >= 0 && insize)
+ memcpy(indata, msg->data, insize);
+
+ kfree(msg);
+ return ret;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cros_typec_acpi_id[] = {
+ { "GOOG0014", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id cros_typec_of_match[] = {
+ { .compatible = "google,cros-ec-typec", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, cros_typec_of_match);
+#endif
+
+static int cros_typec_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_typec_data *typec;
+ struct ec_response_usb_pd_ports resp;
+ int ret;
+
+ typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
+ if (!typec)
+ return -ENOMEM;
+
+ typec->dev = dev;
+ typec->ec = dev_get_drvdata(pdev->dev.parent);
+ platform_set_drvdata(pdev, typec);
+
+ ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
+ &resp, sizeof(resp));
+ if (ret < 0)
+ return ret;
+
+ typec->num_ports = resp.num_ports;
+ if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
+ dev_warn(typec->dev,
+ "Too many ports reported: %d, limiting to max: %d\n",
+ typec->num_ports, EC_USB_PD_MAX_PORTS);
+ typec->num_ports = EC_USB_PD_MAX_PORTS;
+ }
+
+ ret = cros_typec_init_ports(typec);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static struct platform_driver cros_typec_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
+ .of_match_table = of_match_ptr(cros_typec_of_match),
+ },
+ .probe = cros_typec_probe,
+};
+
+module_platform_driver(cros_typec_driver);
+
+MODULE_AUTHOR("Prashant Malani <pmalani@chromium.org>");
+MODULE_DESCRIPTION("Chrome OS EC Type C control");
+MODULE_LICENSE("GPL");
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-20 0:30 ` [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver Prashant Malani
@ 2020-02-27 8:41 ` Stephen Boyd
2020-02-27 16:38 ` Heikki Krogerus
2020-02-27 15:12 ` Heikki Krogerus
1 sibling, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2020-02-27 8:41 UTC (permalink / raw)
To: Prashant Malani, linux-kernel
Cc: heikki.krogerus, enric.balletbo, bleung, Prashant Malani,
devicetree, Guenter Roeck, Mark Rutland, Rob Herring
Don't know what happened but my MUA can't seem to thread these patches.
I wonder if something got messed up during send?
Quoting Prashant Malani (2020-02-19 16:30:55)
> Some Chrome OS devices with Embedded Controllers (EC) can read and
> modify Type C port state.
>
> Add an entry in the DT Bindings documentation that lists out the logical
> device and describes the relevant port information, to be used by the
> corresponding driver.
>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>
> Changes in v3:
> - Fixed license identifier.
> - Renamed "port" to "connector".
> - Made "connector" be a "usb-c-connector" compatible property.
> - Updated port-number description to explain min and max values,
> and removed $ref which was causing dt_binding_check errors.
> - Fixed power-role, data-role and try-power-role details to make
> dt_binding_check pass.
> - Fixed example to include parent EC SPI DT Node.
>
> Changes in v2:
> - No changes. Patch first introduced in v2 of series.
>
> .../bindings/chrome/google,cros-ec-typec.yaml | 86 +++++++++++++++++++
> 1 file changed, 86 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
>
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> new file mode 100644
> index 00000000000000..97fd982612f120
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: GPL-2.0-only
Can it be GPL or BSD 2 clause?
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/chrome/google,cros-ec-typec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Chrome OS EC(Embedded Controller) Type C port driver.
> +
> +maintainers:
> + - Benson Leung <bleung@chromium.org>
> + - Prashant Malani <pmalani@chromium.org>
> +
> +description:
> + Chrome OS devices have an Embedded Controller(EC) which has access to
> + Type C port state. This node is intended to allow the host to read and
> + control the Type C ports. The node for this device should be under a
> + cros-ec node like google,cros-ec-spi.
> +
> +properties:
> + compatible:
> + const: google,cros-ec-typec
> +
> + connector:
> + description: A node that represents a physical Type C connector port
> + on the device.
> + type: object
> + properties:
> + compatible:
> + const: usb-c-connector
> + port-number:
> + description: The number used by the Chrome OS EC to identify
> + this type C port. Valid values are 0 - (EC_USB_PD_MAX_PORTS - 1).
What is EC_USB_PD_MAX_PORTS? Can this be done through a reg property
instead?
> + power-role:
> + description: Determines the power role that the Type C port will
> + adopt.
> + maxItems: 1
I don't think this is necessary with enum below. schema can figure out
that max is 1.
> + contains:
> + enum:
> + - sink
> + - source
> + - dual
Other bindings have newlines between properties. Can you please add them
to improve readability?
> + data-role:
> + description: Determines the data role that the Type C port will
> + adopt.
> + maxItems: 1
> + contains:
> + enum:
> + - host
> + - device
> + - dual
> + try-power-role:
> + description: Determines the preferred power role of the Type C port.
How does this interact with power-role above? Is it different?
> + maxItems: 1
> + contains:
> + enum:
> + - sink
> + - source
> + - dual
> +
> + required:
> + - port-number
> + - power-role
> + - data-role
> + - try-power-role
> +
> +required:
> + - compatible
> + - connector
> +
> +examples:
> + - |+
> + cros_ec: ec@0 {
> + compatible = "google,cros-ec-spi";
> +
> + typec {
> + compatible = "google,cros-ec-typec";
> +
> + usb_con: connector {
> + compatible = "usb-c-connector";
> + port-number = <0>;
> + power-role = "dual";
> + data-role = "dual";
> + try-power-role = "source";
> + };
I thought that perhaps this would be done with the OF graph APIs instead
of being a child of the ec node. I don't see how the usb connector is
anything besides a child of the top-level root node because it's
typically on the board. We put board level components at the root.
Yes, the connector is intimately involved with the EC here, but I would
think that we would have an OF graph connection from the USB controller
on the SoC to the USB connector, traversing through anything that may be
in that path, such as a USB hub. Maybe the connector node itself can
point to the EC type-c controller with some property like
connector {
...
type-c-manager = <&phandle_to_typec_manager>
};
So in the end it would look like this (note that the ec doesn't have a sub-node
to make a driver probe):
/ {
connector@0 {
compatible = "usb-c-connector";
label = "left";
reg = <0>;
power-role = "dual";
type-c-manager = <&cros_ec>;
...
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
left_ufp: endpoint {
remote-endpoint = <&usb_controller0>;
};
};
};
};
connector@1 {
compatible = "usb-c-connector";
label = "right";
reg = <1>;
power-role = "dual";
type-c-manager = <&cros_ec>;
...
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
right_ufp: endpoint {
remote-endpoint = <&usb_controller1>;
};
};
};
};
soc@0 {
#address-cells = <1>;
#size-cells = <0>;
spi@a000000 {
compatible = "soc,spi-controller";
reg = <0xa000000 0x1000>;
cros_ec: ec@0 {
compatible = "google,cros-ec-spi";
reg = <0>;
};
};
usb@ca00000 {
compatible = "soc,dwc3-controller";
reg = <0xca00000 0x1000>;
ports {
port@0 {
reg = <0>;
usb_controller0: endpoint {
remote-endpoint = <&left_ufp>;
};
};
};
};
usb@ea00000 {
compatible = "soc,dwc3-controller";
reg = <0xea00000 0x1000>;
ports {
port@0 {
reg = <0>;
usb_controller1: endpoint {
remote-endpoint = <&right_ufp>;
};
};
};
};
};
};
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/4] platform/chrome: Add Type C connector class driver
2020-02-20 0:30 ` [PATCH v3 2/4] platform/chrome: Add Type C connector class driver Prashant Malani
@ 2020-02-27 14:25 ` Heikki Krogerus
0 siblings, 0 replies; 13+ messages in thread
From: Heikki Krogerus @ 2020-02-27 14:25 UTC (permalink / raw)
To: Prashant Malani
Cc: linux-kernel, enric.balletbo, bleung,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Mark Rutland, Rob Herring
On Wed, Feb 19, 2020 at 04:30:57PM -0800, Prashant Malani wrote:
> Add a driver to implement the Type C connector class for Chrome OS
> devices with ECs (Embedded Controllers).
>
> The driver relies on firmware device specifications for various port
> attributes. On ACPI platforms, this is specified using the logical
> device with HID GOOG0014. On DT platforms, this is specified using the
> DT node with compatible string "google,cros-ec-typec".
>
> This patch reads the device FW node and uses the port attributes to
> register the typec ports with the Type C connector class framework, but
> doesn't do much else.
>
> Subsequent patches will add more functionality to the driver, including
> obtaining current port information (polarity, vconn role, current power
> role etc.) after querying the EC.
>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>
> Changes in v3:
> - Fixed minor spacing nits, and moved a modification to probe() if check
> from later patch to here instead.
>
> Changes in v2:
> - Updated Kconfig to default to MFD_CROS_EC_DEV.
> - Fixed code comments.
> - Moved get_num_ports() code into probe().
> - Added module author.
>
> drivers/platform/chrome/Kconfig | 11 ++
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/cros_ec_typec.c | 221 ++++++++++++++++++++++++
> 3 files changed, 233 insertions(+)
> create mode 100644 drivers/platform/chrome/cros_ec_typec.c
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 5f57282a28da00..2320a4f0d93019 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -214,6 +214,17 @@ config CROS_EC_SYSFS
> To compile this driver as a module, choose M here: the
> module will be called cros_ec_sysfs.
>
> +config CROS_EC_TYPEC
> + tristate "ChromeOS EC Type-C Connector Control"
> + depends on MFD_CROS_EC_DEV && TYPEC
> + default MFD_CROS_EC_DEV
> + help
> + If you say Y here, you get support for accessing Type C connector
> + information from the Chrome OS EC.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called cros_ec_typec.
> +
> config CROS_USBPD_LOGGER
> tristate "Logging driver for USB PD charger"
> depends on CHARGER_CROS_USBPD
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index aacd5920d8a180..caf2a9cdb5e6d1 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o
> obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o
> obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o
> cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o
> +obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o
> obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> new file mode 100644
> index 00000000000000..6cac41f246b99f
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Google LLC
> + *
> + * This driver provides the ability to view and manage Type C ports through the
> + * Chrome OS EC.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/typec.h>
> +
> +#define DRV_NAME "cros-ec-typec"
> +
> +/* Platform-specific data for the Chrome OS EC Type C controller. */
> +struct cros_typec_data {
> + struct device *dev;
> + struct cros_ec_device *ec;
> + int num_ports;
> + /* Array of ports, indexed by port number. */
> + struct typec_port *ports[EC_USB_PD_MAX_PORTS];
> +};
> +
> +static int cros_typec_parse_port_props(struct typec_capability *cap,
> + struct fwnode_handle *fwnode,
> + struct device *dev)
> +{
> + const char *buf;
> + int ret;
> +
> + memset(cap, 0, sizeof(*cap));
> + ret = fwnode_property_read_string(fwnode, "power-role", &buf);
> + if (ret) {
> + dev_err(dev, "power-role not found: %d\n", ret);
> + return ret;
> + }
> +
> + ret = typec_find_port_power_role(buf);
> + if (ret < 0)
> + return ret;
> + cap->type = ret;
> +
> + ret = fwnode_property_read_string(fwnode, "data-role", &buf);
> + if (ret) {
> + dev_err(dev, "data-role not found: %d\n", ret);
> + return ret;
> + }
> +
> + ret = typec_find_port_data_role(buf);
> + if (ret < 0)
> + return ret;
> + cap->data = ret;
> +
> + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf);
> + if (ret) {
> + dev_err(dev, "try-power-role not found: %d\n", ret);
> + return ret;
> + }
> +
> + ret = typec_find_power_role(buf);
> + if (ret < 0)
> + return ret;
> + cap->prefer_role = ret;
> +
> + cap->fwnode = fwnode;
> +
> + return 0;
> +}
> +
> +static int cros_typec_init_ports(struct cros_typec_data *typec)
> +{
> + struct device *dev = typec->dev;
> + struct typec_capability cap;
> + struct fwnode_handle *fwnode;
> + int ret;
> + int i;
> + int nports;
> + u32 port_num;
> +
> + nports = device_get_child_node_count(dev);
> + if (nports == 0) {
> + dev_err(dev, "No port entries found.\n");
> + return -ENODEV;
> + }
> +
> + device_for_each_child_node(dev, fwnode) {
> + if (fwnode_property_read_u32(fwnode, "port-number",
> + &port_num)) {
> + dev_err(dev, "No port-number for port, skipping.\n");
> + ret = -EINVAL;
> + goto unregister_ports;
> + }
> +
> + if (port_num >= typec->num_ports) {
> + dev_err(dev, "Invalid port number.\n");
> + ret = -EINVAL;
> + goto unregister_ports;
> + }
> +
> + dev_dbg(dev, "Registering port %d\n", port_num);
> +
> + ret = cros_typec_parse_port_props(&cap, fwnode, dev);
> + if (ret < 0)
> + goto unregister_ports;
> +
> + typec->ports[port_num] = typec_register_port(dev, &cap);
> + if (IS_ERR(typec->ports[port_num])) {
> + dev_err(dev, "Failed to register port %d\n", port_num);
> + ret = PTR_ERR(typec->ports[port_num]);
> + goto unregister_ports;
> + }
> + }
> +
> + return 0;
> +
> +unregister_ports:
> + for (i = 0; i < typec->num_ports; i++)
> + typec_unregister_port(typec->ports[i]);
> + return ret;
> +}
> +
> +static int cros_typec_ec_command(struct cros_typec_data *typec,
> + unsigned int version,
> + unsigned int command,
> + void *outdata,
> + unsigned int outsize,
> + void *indata,
> + unsigned int insize)
> +{
> + struct cros_ec_command *msg;
> + int ret;
> +
> + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + msg->version = version;
> + msg->command = command;
> + msg->outsize = outsize;
> + msg->insize = insize;
> +
> + if (outsize)
> + memcpy(msg->data, outdata, outsize);
> +
> + ret = cros_ec_cmd_xfer_status(typec->ec, msg);
> + if (ret >= 0 && insize)
> + memcpy(indata, msg->data, insize);
> +
> + kfree(msg);
> + return ret;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id cros_typec_acpi_id[] = {
> + { "GOOG0014", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id);
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id cros_typec_of_match[] = {
> + { .compatible = "google,cros-ec-typec", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, cros_typec_of_match);
> +#endif
> +
> +static int cros_typec_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_typec_data *typec;
> + struct ec_response_usb_pd_ports resp;
> + int ret;
> +
> + typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
> + if (!typec)
> + return -ENOMEM;
> +
> + typec->dev = dev;
> + typec->ec = dev_get_drvdata(pdev->dev.parent);
> + platform_set_drvdata(pdev, typec);
> +
> + ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> + &resp, sizeof(resp));
> + if (ret < 0)
> + return ret;
> +
> + typec->num_ports = resp.num_ports;
> + if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
> + dev_warn(typec->dev,
> + "Too many ports reported: %d, limiting to max: %d\n",
> + typec->num_ports, EC_USB_PD_MAX_PORTS);
> + typec->num_ports = EC_USB_PD_MAX_PORTS;
> + }
> +
> + ret = cros_typec_init_ports(typec);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static struct platform_driver cros_typec_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .acpi_match_table = ACPI_PTR(cros_typec_acpi_id),
> + .of_match_table = of_match_ptr(cros_typec_of_match),
> + },
> + .probe = cros_typec_probe,
> +};
> +
> +module_platform_driver(cros_typec_driver);
> +
> +MODULE_AUTHOR("Prashant Malani <pmalani@chromium.org>");
> +MODULE_DESCRIPTION("Chrome OS EC Type C control");
> +MODULE_LICENSE("GPL");
> --
> 2.25.0.265.gbab2e86ba0-goog
thanks,
--
heikki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-20 0:30 ` [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver Prashant Malani
2020-02-27 8:41 ` Stephen Boyd
@ 2020-02-27 15:12 ` Heikki Krogerus
2020-02-27 23:15 ` Rob Herring
1 sibling, 1 reply; 13+ messages in thread
From: Heikki Krogerus @ 2020-02-27 15:12 UTC (permalink / raw)
To: Prashant Malani
Cc: linux-kernel, enric.balletbo, bleung,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Mark Rutland, Rob Herring
Hi,
On Wed, Feb 19, 2020 at 04:30:55PM -0800, Prashant Malani wrote:
> Some Chrome OS devices with Embedded Controllers (EC) can read and
> modify Type C port state.
>
> Add an entry in the DT Bindings documentation that lists out the logical
> device and describes the relevant port information, to be used by the
> corresponding driver.
>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>
> Changes in v3:
> - Fixed license identifier.
> - Renamed "port" to "connector".
> - Made "connector" be a "usb-c-connector" compatible property.
> - Updated port-number description to explain min and max values,
> and removed $ref which was causing dt_binding_check errors.
> - Fixed power-role, data-role and try-power-role details to make
> dt_binding_check pass.
> - Fixed example to include parent EC SPI DT Node.
>
> Changes in v2:
> - No changes. Patch first introduced in v2 of series.
>
> .../bindings/chrome/google,cros-ec-typec.yaml | 86 +++++++++++++++++++
> 1 file changed, 86 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
>
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> new file mode 100644
> index 00000000000000..97fd982612f120
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/chrome/google,cros-ec-typec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Google Chrome OS EC(Embedded Controller) Type C port driver.
> +
> +maintainers:
> + - Benson Leung <bleung@chromium.org>
> + - Prashant Malani <pmalani@chromium.org>
> +
> +description:
> + Chrome OS devices have an Embedded Controller(EC) which has access to
> + Type C port state. This node is intended to allow the host to read and
> + control the Type C ports. The node for this device should be under a
> + cros-ec node like google,cros-ec-spi.
> +
> +properties:
> + compatible:
> + const: google,cros-ec-typec
> +
> + connector:
> + description: A node that represents a physical Type C connector port
> + on the device.
> + type: object
> + properties:
> + compatible:
> + const: usb-c-connector
> + port-number:
> + description: The number used by the Chrome OS EC to identify
> + this type C port. Valid values are 0 - (EC_USB_PD_MAX_PORTS - 1).
> + power-role:
> + description: Determines the power role that the Type C port will
> + adopt.
> + maxItems: 1
> + contains:
> + enum:
> + - sink
> + - source
> + - dual
> + data-role:
> + description: Determines the data role that the Type C port will
> + adopt.
> + maxItems: 1
> + contains:
> + enum:
> + - host
> + - device
> + - dual
> + try-power-role:
> + description: Determines the preferred power role of the Type C port.
> + maxItems: 1
> + contains:
> + enum:
> + - sink
> + - source
> + - dual
> +
> + required:
> + - port-number
> + - power-role
> + - data-role
> + - try-power-role
Do you really need to redefine those?
I think you just need to mention that there is a required sub-node
"connector", and the place where it's described. So something
like this:
Required sub-node:
- connector : The "usb-c-connector". The bindings of the
connector node are specified in:
Documentation/devicetree/bindings/connector/usb-connector.txt
Then you just need to define the Chrome OS EC specific properties, so
I guess just the "port-number".
thanks,
--
heikki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-27 8:41 ` Stephen Boyd
@ 2020-02-27 16:38 ` Heikki Krogerus
2020-02-27 22:07 ` Stephen Boyd
0 siblings, 1 reply; 13+ messages in thread
From: Heikki Krogerus @ 2020-02-27 16:38 UTC (permalink / raw)
To: Stephen Boyd
Cc: Prashant Malani, linux-kernel, enric.balletbo, bleung,
devicetree, Guenter Roeck, Mark Rutland, Rob Herring
Hi Stephen,
On Thu, Feb 27, 2020 at 12:41:13AM -0800, Stephen Boyd wrote:
> > +examples:
> > + - |+
> > + cros_ec: ec@0 {
> > + compatible = "google,cros-ec-spi";
> > +
> > + typec {
> > + compatible = "google,cros-ec-typec";
> > +
> > + usb_con: connector {
> > + compatible = "usb-c-connector";
> > + port-number = <0>;
> > + power-role = "dual";
> > + data-role = "dual";
> > + try-power-role = "source";
> > + };
>
> I thought that perhaps this would be done with the OF graph APIs instead
> of being a child of the ec node. I don't see how the usb connector is
> anything besides a child of the top-level root node because it's
> typically on the board. We put board level components at the root.
No.
The above follows the usb-connector bindings, so it is correct:
Documentation/devicetree/bindings/connector/usb-connector.txt
So the connector is always a child of the "CC controller" with the USB
Type-C connectors, which in this case is the EC (from operating systems
perspective). The "CC controller" controls connectors, and it doesn't
actually do anything else. So placing the connectors under the
"connector controller" is also logically correct.
> Yes, the connector is intimately involved with the EC here, but I would
> think that we would have an OF graph connection from the USB controller
> on the SoC to the USB connector, traversing through anything that may be
> in that path, such as a USB hub. Maybe the connector node itself can
> point to the EC type-c controller with some property like
I think your idea here is that there should be only a single node for
each connector that is then linked with every component that it is
physically connected to (right?), but please note that that is not
enough. Every component attached to the connector must have its own
child node that represents the "port" that is physically connected to
the USB Type-C connector.
So for example, the USB controller nodes have child nodes for every
USB2 port as well as for every USB3 port. Similarly, the GPU
controllers have child node for every DisplayPort, etc. And I believe
that is already how it has been done in DT (and also in ACPI).
Those "port" nodes then just need to be linked with the "connector"
node. I think for that the idea was to use OF graph, but I'm really
sceptical about that. The problem is that with the USB Type-C
connectors we have to be able to identify the connections, i.e. which
endpoint is the USB2 port, which is the DisplayPort and so on, and OF
graph does not give any means to do that on its own. We will have to
rely on separate device properties in order to do the identification.
Currently it is not documented anywhere which property should be used
for that.
For ACPI we are going to propose that with every type of connection,
there should be a device property that returns a reference to the
appropriate port. That way there are no problems identifying the
connections. All we need to do is to define the property names for
every type of connection. "usb2-port" for the USB2 or high speed port,
"usb3-port" for USB3, etc.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-27 16:38 ` Heikki Krogerus
@ 2020-02-27 22:07 ` Stephen Boyd
2020-02-28 16:24 ` Heikki Krogerus
0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2020-02-27 22:07 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Prashant Malani, linux-kernel, enric.balletbo, bleung,
devicetree, Guenter Roeck, Mark Rutland, Rob Herring
Quoting Heikki Krogerus (2020-02-27 08:38:25)
> Hi Stephen,
>
> On Thu, Feb 27, 2020 at 12:41:13AM -0800, Stephen Boyd wrote:
> > > +examples:
> > > + - |+
> > > + cros_ec: ec@0 {
> > > + compatible = "google,cros-ec-spi";
> > > +
> > > + typec {
> > > + compatible = "google,cros-ec-typec";
> > > +
> > > + usb_con: connector {
> > > + compatible = "usb-c-connector";
> > > + port-number = <0>;
> > > + power-role = "dual";
> > > + data-role = "dual";
> > > + try-power-role = "source";
> > > + };
> >
> > I thought that perhaps this would be done with the OF graph APIs instead
> > of being a child of the ec node. I don't see how the usb connector is
> > anything besides a child of the top-level root node because it's
> > typically on the board. We put board level components at the root.
>
> No.
>
> The above follows the usb-connector bindings, so it is correct:
> Documentation/devicetree/bindings/connector/usb-connector.txt
>
> So the connector is always a child of the "CC controller" with the USB
> Type-C connectors, which in this case is the EC (from operating systems
> perspective). The "CC controller" controls connectors, and it doesn't
> actually do anything else. So placing the connectors under the
> "connector controller" is also logically correct.
Ah ok I see. The graph binding is for describing the data path, not the
control path. Makes sense.
>
> > Yes, the connector is intimately involved with the EC here, but I would
> > think that we would have an OF graph connection from the USB controller
> > on the SoC to the USB connector, traversing through anything that may be
> > in that path, such as a USB hub. Maybe the connector node itself can
> > point to the EC type-c controller with some property like
>
> I think your idea here is that there should be only a single node for
> each connector that is then linked with every component that it is
> physically connected to (right?), but please note that that is not
> enough. Every component attached to the connector must have its own
> child node that represents the "port" that is physically connected to
> the USB Type-C connector.
>
> So for example, the USB controller nodes have child nodes for every
> USB2 port as well as for every USB3 port. Similarly, the GPU
> controllers have child node for every DisplayPort, etc. And I believe
> that is already how it has been done in DT (and also in ACPI).
It looks like perhaps you're conflating ports in USB spec with the OF
graph port? I want there to be one node per type-c connector that I can
physically see on the device. Is that not sufficient?
Are there any examples of the type-c connector in DT? I see some
NXP/Freescale boards and one Renesas board so far. Maybe there are other
discussions I can read up on?
>
> Those "port" nodes then just need to be linked with the "connector"
> node. I think for that the idea was to use OF graph, but I'm really
> sceptical about that. The problem is that with the USB Type-C
> connectors we have to be able to identify the connections, i.e. which
> endpoint is the USB2 port, which is the DisplayPort and so on, and OF
> graph does not give any means to do that on its own. We will have to
> rely on separate device properties in order to do the identification.
> Currently it is not documented anywhere which property should be used
> for that.
I hope that this patch series can document this. Why can't that work by
having multiple OF graph ports for USB2 port, DisplayPort, USB3 port,
etc? The data path goes to the connector and we can attach more
information to each port node to describe what type of endpoint is there
like a DisplayPort capable type-c connector for example.
>
> For ACPI we are going to propose that with every type of connection,
> there should be a device property that returns a reference to the
> appropriate port. That way there are no problems identifying the
> connections. All we need to do is to define the property names for
> every type of connection. "usb2-port" for the USB2 or high speed port,
> "usb3-port" for USB3, etc.
>
That sounds like something we should figure out now for DT firmwares
too. For this particular binding, I don't know if we need to do anything
besides figure out how to represent multiple connectors underneath the
EC node. The other properties seem fairly generic and so I'd expect this
series to migrate
Documentation/devicetree/bindings/connector/usb-connector.txt to YAML
and refine the binding with anything necessary, like a 'reg' property to
allow multiple ports to exist underneath the "CC controller".
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-27 15:12 ` Heikki Krogerus
@ 2020-02-27 23:15 ` Rob Herring
2020-03-04 17:53 ` Prashant Malani
0 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2020-02-27 23:15 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Prashant Malani, linux-kernel, enric.balletbo, bleung,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Mark Rutland
On Thu, Feb 27, 2020 at 05:12:16PM +0200, Heikki Krogerus wrote:
> Hi,
>
> On Wed, Feb 19, 2020 at 04:30:55PM -0800, Prashant Malani wrote:
> > Some Chrome OS devices with Embedded Controllers (EC) can read and
> > modify Type C port state.
> >
> > Add an entry in the DT Bindings documentation that lists out the logical
> > device and describes the relevant port information, to be used by the
> > corresponding driver.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Fixed license identifier.
> > - Renamed "port" to "connector".
> > - Made "connector" be a "usb-c-connector" compatible property.
> > - Updated port-number description to explain min and max values,
> > and removed $ref which was causing dt_binding_check errors.
> > - Fixed power-role, data-role and try-power-role details to make
> > dt_binding_check pass.
> > - Fixed example to include parent EC SPI DT Node.
> >
> > Changes in v2:
> > - No changes. Patch first introduced in v2 of series.
> >
> > .../bindings/chrome/google,cros-ec-typec.yaml | 86 +++++++++++++++++++
> > 1 file changed, 86 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > new file mode 100644
> > index 00000000000000..97fd982612f120
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > @@ -0,0 +1,86 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/chrome/google,cros-ec-typec.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Google Chrome OS EC(Embedded Controller) Type C port driver.
> > +
> > +maintainers:
> > + - Benson Leung <bleung@chromium.org>
> > + - Prashant Malani <pmalani@chromium.org>
> > +
> > +description:
> > + Chrome OS devices have an Embedded Controller(EC) which has access to
> > + Type C port state. This node is intended to allow the host to read and
> > + control the Type C ports. The node for this device should be under a
> > + cros-ec node like google,cros-ec-spi.
> > +
> > +properties:
> > + compatible:
> > + const: google,cros-ec-typec
> > +
> > + connector:
> > + description: A node that represents a physical Type C connector port
> > + on the device.
> > + type: object
> > + properties:
> > + compatible:
> > + const: usb-c-connector
> > + port-number:
> > + description: The number used by the Chrome OS EC to identify
> > + this type C port. Valid values are 0 - (EC_USB_PD_MAX_PORTS - 1).
> > + power-role:
> > + description: Determines the power role that the Type C port will
> > + adopt.
> > + maxItems: 1
> > + contains:
> > + enum:
> > + - sink
> > + - source
> > + - dual
> > + data-role:
> > + description: Determines the data role that the Type C port will
> > + adopt.
> > + maxItems: 1
> > + contains:
> > + enum:
> > + - host
> > + - device
> > + - dual
> > + try-power-role:
> > + description: Determines the preferred power role of the Type C port.
> > + maxItems: 1
> > + contains:
> > + enum:
> > + - sink
> > + - source
> > + - dual
> > +
> > + required:
> > + - port-number
> > + - power-role
> > + - data-role
> > + - try-power-role
>
> Do you really need to redefine those?
No.
>
> I think you just need to mention that there is a required sub-node
> "connector", and the place where it's described. So something
> like this:
>
> Required sub-node:
> - connector : The "usb-c-connector". The bindings of the
> connector node are specified in:
>
> Documentation/devicetree/bindings/connector/usb-connector.txt
Ideally, we'd convert this to schema first and then here just have:
connector:
$ref: /schemas/connector/usb-connector.yaml#
>
>
> Then you just need to define the Chrome OS EC specific properties, so
> I guess just the "port-number".
'reg' as Stephen suggested.
Rob
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-27 22:07 ` Stephen Boyd
@ 2020-02-28 16:24 ` Heikki Krogerus
2020-02-29 0:43 ` Stephen Boyd
0 siblings, 1 reply; 13+ messages in thread
From: Heikki Krogerus @ 2020-02-28 16:24 UTC (permalink / raw)
To: Stephen Boyd
Cc: Prashant Malani, linux-kernel, enric.balletbo, bleung,
devicetree, Guenter Roeck, Mark Rutland, Rob Herring
Hi Stephen,
On Thu, Feb 27, 2020 at 02:07:53PM -0800, Stephen Boyd wrote:
> Quoting Heikki Krogerus (2020-02-27 08:38:25)
> > Hi Stephen,
> >
> > On Thu, Feb 27, 2020 at 12:41:13AM -0800, Stephen Boyd wrote:
> > > > +examples:
> > > > + - |+
> > > > + cros_ec: ec@0 {
> > > > + compatible = "google,cros-ec-spi";
> > > > +
> > > > + typec {
> > > > + compatible = "google,cros-ec-typec";
> > > > +
> > > > + usb_con: connector {
> > > > + compatible = "usb-c-connector";
> > > > + port-number = <0>;
> > > > + power-role = "dual";
> > > > + data-role = "dual";
> > > > + try-power-role = "source";
> > > > + };
> > >
> > > I thought that perhaps this would be done with the OF graph APIs instead
> > > of being a child of the ec node. I don't see how the usb connector is
> > > anything besides a child of the top-level root node because it's
> > > typically on the board. We put board level components at the root.
> >
> > No.
> >
> > The above follows the usb-connector bindings, so it is correct:
> > Documentation/devicetree/bindings/connector/usb-connector.txt
> >
> > So the connector is always a child of the "CC controller" with the USB
> > Type-C connectors, which in this case is the EC (from operating systems
> > perspective). The "CC controller" controls connectors, and it doesn't
> > actually do anything else. So placing the connectors under the
> > "connector controller" is also logically correct.
>
> Ah ok I see. The graph binding is for describing the data path, not the
> control path. Makes sense.
>
> >
> > > Yes, the connector is intimately involved with the EC here, but I would
> > > think that we would have an OF graph connection from the USB controller
> > > on the SoC to the USB connector, traversing through anything that may be
> > > in that path, such as a USB hub. Maybe the connector node itself can
> > > point to the EC type-c controller with some property like
> >
> > I think your idea here is that there should be only a single node for
> > each connector that is then linked with every component that it is
> > physically connected to (right?), but please note that that is not
> > enough. Every component attached to the connector must have its own
> > child node that represents the "port" that is physically connected to
> > the USB Type-C connector.
> >
> > So for example, the USB controller nodes have child nodes for every
> > USB2 port as well as for every USB3 port. Similarly, the GPU
> > controllers have child node for every DisplayPort, etc. And I believe
> > that is already how it has been done in DT (and also in ACPI).
>
> It looks like perhaps you're conflating ports in USB spec with the OF
> graph port? I want there to be one node per type-c connector that I can
> physically see on the device. Is that not sufficient?
It is. We don't need more than one node that represents the physical
connector (and we should not have more than one node for that). And
actually, I was not mixing the OF graph ports and USB ports... I
think I should be talking about PHY instead of "port". That is
probable more clear.
My point is that every PHY that is connected to a Type-C connector
must still be represented with its own node in devicetree and ACPI. So
there still needs to be a node for the USB2 PHY, USB3 PHY, DisplayPort
PHY, etc., on top of the connector node. I got the picture that you
are proposing that we don't need those PHY nodes anymore since we have
the connector nodes, but maybe I misunderstood?
> Are there any examples of the type-c connector in DT? I see some
> NXP/Freescale boards and one Renesas board so far. Maybe there are other
> discussions I can read up on?
>
> >
> > Those "port" nodes then just need to be linked with the "connector"
> > node. I think for that the idea was to use OF graph, but I'm really
> > sceptical about that. The problem is that with the USB Type-C
> > connectors we have to be able to identify the connections, i.e. which
> > endpoint is the USB2 port, which is the DisplayPort and so on, and OF
> > graph does not give any means to do that on its own. We will have to
> > rely on separate device properties in order to do the identification.
> > Currently it is not documented anywhere which property should be used
> > for that.
>
> I hope that this patch series can document this.
Well, we do need that to be documented, but do we really need to block
this series because of that? This driver does not depend on OF graph
yet.
> Why can't that work by having multiple OF graph ports for USB2 port,
> DisplayPort, USB3 port, etc? The data path goes to the connector and
> we can attach more information to each port node to describe what
> type of endpoint is there like a DisplayPort capable type-c
> connector for example.
The PHY nodes we must still always have. So the OF graph will always
describe the connection between the PHY and the connector, and the
connection between the PHY and the controller must be described
separately.
> > For ACPI we are going to propose that with every type of connection,
> > there should be a device property that returns a reference to the
> > appropriate port. That way there are no problems identifying the
> > connections. All we need to do is to define the property names for
> > every type of connection. "usb2-port" for the USB2 or high speed port,
> > "usb3-port" for USB3, etc.
> >
>
> That sounds like something we should figure out now for DT firmwares
> too. For this particular binding, I don't know if we need to do anything
> besides figure out how to represent multiple connectors underneath the
> EC node. The other properties seem fairly generic and so I'd expect this
> series to migrate
> Documentation/devicetree/bindings/connector/usb-connector.txt to YAML
> and refine the binding with anything necessary, like a 'reg' property to
> allow multiple ports to exist underneath the "CC controller".
OK.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-28 16:24 ` Heikki Krogerus
@ 2020-02-29 0:43 ` Stephen Boyd
2020-03-05 16:57 ` Heikki Krogerus
0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2020-02-29 0:43 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Prashant Malani, linux-kernel, enric.balletbo, bleung,
devicetree, Guenter Roeck, Mark Rutland, Rob Herring
Quoting Heikki Krogerus (2020-02-28 08:24:00)
> On Thu, Feb 27, 2020 at 02:07:53PM -0800, Stephen Boyd wrote:
> > Quoting Heikki Krogerus (2020-02-27 08:38:25)
> > > No.
> > >
> > > The above follows the usb-connector bindings, so it is correct:
> > > Documentation/devicetree/bindings/connector/usb-connector.txt
> > >
> > > So the connector is always a child of the "CC controller" with the USB
> > > Type-C connectors, which in this case is the EC (from operating systems
> > > perspective). The "CC controller" controls connectors, and it doesn't
> > > actually do anything else. So placing the connectors under the
> > > "connector controller" is also logically correct.
> >
> > Ah ok I see. The graph binding is for describing the data path, not the
> > control path. Makes sense.
> >
> > >
> > > > Yes, the connector is intimately involved with the EC here, but I would
> > > > think that we would have an OF graph connection from the USB controller
> > > > on the SoC to the USB connector, traversing through anything that may be
> > > > in that path, such as a USB hub. Maybe the connector node itself can
> > > > point to the EC type-c controller with some property like
> > >
> > > I think your idea here is that there should be only a single node for
> > > each connector that is then linked with every component that it is
> > > physically connected to (right?), but please note that that is not
> > > enough. Every component attached to the connector must have its own
> > > child node that represents the "port" that is physically connected to
> > > the USB Type-C connector.
> > >
> > > So for example, the USB controller nodes have child nodes for every
> > > USB2 port as well as for every USB3 port. Similarly, the GPU
> > > controllers have child node for every DisplayPort, etc. And I believe
> > > that is already how it has been done in DT (and also in ACPI).
> >
> > It looks like perhaps you're conflating ports in USB spec with the OF
> > graph port? I want there to be one node per type-c connector that I can
> > physically see on the device. Is that not sufficient?
>
> It is. We don't need more than one node that represents the physical
> connector (and we should not have more than one node for that). And
> actually, I was not mixing the OF graph ports and USB ports... I
> think I should be talking about PHY instead of "port". That is
> probable more clear.
>
> My point is that every PHY that is connected to a Type-C connector
> must still be represented with its own node in devicetree and ACPI. So
> there still needs to be a node for the USB2 PHY, USB3 PHY, DisplayPort
> PHY, etc., on top of the connector node. I got the picture that you
> are proposing that we don't need those PHY nodes anymore since we have
> the connector nodes, but maybe I misunderstood?
Alright. Maybe a full example will help. See below. I think I understand
how it's supposed to look.
>
> > Are there any examples of the type-c connector in DT? I see some
> > NXP/Freescale boards and one Renesas board so far. Maybe there are other
> > discussions I can read up on?
> >
> > >
> > > Those "port" nodes then just need to be linked with the "connector"
> > > node. I think for that the idea was to use OF graph, but I'm really
> > > sceptical about that. The problem is that with the USB Type-C
> > > connectors we have to be able to identify the connections, i.e. which
> > > endpoint is the USB2 port, which is the DisplayPort and so on, and OF
> > > graph does not give any means to do that on its own. We will have to
> > > rely on separate device properties in order to do the identification.
> > > Currently it is not documented anywhere which property should be used
> > > for that.
> >
> > I hope that this patch series can document this.
>
> Well, we do need that to be documented, but do we really need to block
> this series because of that? This driver does not depend on OF graph
> yet.
I don't know. I think this binding patch will go for another round so
maybe it's blocked in other ways?
>
> > Why can't that work by having multiple OF graph ports for USB2 port,
> > DisplayPort, USB3 port, etc? The data path goes to the connector and
> > we can attach more information to each port node to describe what
> > type of endpoint is there like a DisplayPort capable type-c
> > connector for example.
>
> The PHY nodes we must still always have. So the OF graph will always
> describe the connection between the PHY and the connector, and the
> connection between the PHY and the controller must be described
> separately.
Got it.
Here's the same example that hopefully shows how all this stuff can
work. I've added more nonsense to try and make it as complicated as
possible.
/ {
// Expand single usb2/usb3 from SoC to 4 port hub
usb-hub {
compatible = "vendor,usb-hub-4-port";
usb-vid = <0xaaad>;
usb-pid = <0xffed>;
vdd-supply = <&pp3300_usb>;
reset-gpios = <&gpio_controller 50 GPIO_ACTIVE_LOW>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
usb2_hub0: endpoint0 {
remote-endpoint = <&left_typec2>;
};
usb3_hub0: endpoint1 {
remote-endpoint = <&left_typec3>;
};
};
port@1 {
reg = <1>;
usb2_hub1: endpoint0 {
remote-endpoint = <&right_typec2>;
};
usb3_hub1: endpoint1 {
remote-endpoint = <&right_typec3>;
};
};
port@2 {
reg = <2>;
usb2_hub2: endpoint0 {
remote-endpoint = <&left_typea2>;
};
usb3_hub2: endpoint1 {
remote-endpoint = <&left_typea3>;
};
};
port@3 {
reg = <3>;
usb2_hub3: endpoint0 {
// Not connected
};
usb3_hub3: endpoint1 {
// Not connected
};
};
port@4 {
reg = <4>;
usb2_hub_in: endpoint0 {
remote-endpoint = <&usb2_phy_out>;
};
usb3_hub_in: endpoint1 {
remote-endpoint = <&usb3_phy_out>;
};
};
};
};
// Maybe this should go under EC node too if EC controls it
// somehow?
connector {
compatible = "usb-a-connector";
label = "type-A-left"
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
left_typea2: endpoint0 {
remote-endpoint = <&usb2_hub2>;
};
left_typea3: endpoint1 {
remote-endpoint = <&usb3_hub2>;
};
};
};
// Steer DP to either left or right type-c port
mux {
compatible = "vendor,dp-mux";
// Inactive: port 0
// Active: port 1
mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
mux_dp_0: endpoint {
remote-endpoint = <&right_typec_dp>;
};
};
port@1 {
reg = <1>;
mux_dp_1: endpoint {
remote-endpoint = <&left_typec_dp>;
};
};
port@2 {
reg = <1>;
mux_dp_in: endpoint {
remote-endpoint = <&dp_phy_out>;
};
};
};
};
soc@0 {
#address-cells = <1>;
#size-cells = <0>;
spi@a000000 {
compatible = "soc,spi-controller";
reg = <0xa000000 0x1000>;
cros_ec: ec@0 {
compatible = "google,cros-ec-spi";
reg = <0>;
connector@0 {
compatible = "usb-c-connector";
label = "type-c-left";
reg = <0>;
power-role = "dual";
...
ports { // Maybe ports is overkill with only one port?
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
left_typec2: endpoint0 {
remote-endpoint = <&usb2_hub0>;
};
left_typec3: endpoint1 {
remote-endpoint = <&usb3_hub0>;
};
left_typec_dp: endpoint2 {
remote-endpoint = <&mux_dp_1>;
};
};
};
};
connector@1 {
compatible = "usb-c-connector";
label = "type-c-right";
reg = <1>;
power-role = "dual";
...
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
right_typec2: endpoint0 {
remote-endpoint = <&usb2_hub1>;
};
right_typec3: endpoint1 {
remote-endpoint = <&usb3_hub1>;
};
right_typec_dp: endpoint2 {
remote-endpoint = <&mux_dp_0>;
};
};
};
};
};
};
usb2_phy: phy@da00000 {
compatible = "soc,usb2-phy";
reg = <0xda00000 0x1000>;
ports {
port@0 {
reg = <0>;
usb2_phy_out: endpoint {
remote-endpoint = <&usb2_hub_in>;
};
};
};
};
usb3_phy: phy@db00000 {
compatible = "soc,usb3-phy";
reg = <0xdb00000 0x1000>;
ports {
port@0 {
reg = <0>;
usb3_phy_out: endpoint {
remote-endpoint = <&usb3_hub_in>;
};
};
};
};
dp_phy: phy@dc00000 {
compatible = "soc,dp-phy";
reg = <0xdc00000 0x1000>;
ports {
port@0 {
reg = <0>;
dp_phy_out: endpoint {
remote-endpoint = <&mux_dp_in>;
};
};
};
};
usb@ea00000 {
compatible = "soc,dwc3-controller";
reg = <0xea00000 0x1000>;
phys = <&usb2_phy>, <&usb3_phy>;
};
display-controller@eb00000 {
compatible = "soc,dwc3-controller";
reg = <0xeb00000 0x1000>;
phys = <&dp_phy>;
// TODO: Connect audio to DP phy somehow
};
};
};
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-27 23:15 ` Rob Herring
@ 2020-03-04 17:53 ` Prashant Malani
0 siblings, 0 replies; 13+ messages in thread
From: Prashant Malani @ 2020-03-04 17:53 UTC (permalink / raw)
To: Rob Herring
Cc: Heikki Krogerus, linux-kernel, enric.balletbo, bleung,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Guenter Roeck, Mark Rutland
Hi Rob,
Thanks for reviewing the patch. Please see inline.
On Thu, Feb 27, 2020 at 05:15:47PM -0600, Rob Herring wrote:
> On Thu, Feb 27, 2020 at 05:12:16PM +0200, Heikki Krogerus wrote:
> > Hi,
> >
> > On Wed, Feb 19, 2020 at 04:30:55PM -0800, Prashant Malani wrote:
> > > Some Chrome OS devices with Embedded Controllers (EC) can read and
> > > modify Type C port state.
> > >
> > > Add an entry in the DT Bindings documentation that lists out the logical
> > > device and describes the relevant port information, to be used by the
> > > corresponding driver.
> > >
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >
> > > Changes in v3:
> > > - Fixed license identifier.
> > > - Renamed "port" to "connector".
> > > - Made "connector" be a "usb-c-connector" compatible property.
> > > - Updated port-number description to explain min and max values,
> > > and removed $ref which was causing dt_binding_check errors.
> > > - Fixed power-role, data-role and try-power-role details to make
> > > dt_binding_check pass.
> > > - Fixed example to include parent EC SPI DT Node.
> > >
> > > Changes in v2:
> > > - No changes. Patch first introduced in v2 of series.
> > >
> > > .../bindings/chrome/google,cros-ec-typec.yaml | 86 +++++++++++++++++++
> > > 1 file changed, 86 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > new file mode 100644
> > > index 00000000000000..97fd982612f120
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > @@ -0,0 +1,86 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/chrome/google,cros-ec-typec.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Google Chrome OS EC(Embedded Controller) Type C port driver.
> > > +
> > > +maintainers:
> > > + - Benson Leung <bleung@chromium.org>
> > > + - Prashant Malani <pmalani@chromium.org>
> > > +
> > > +description:
> > > + Chrome OS devices have an Embedded Controller(EC) which has access to
> > > + Type C port state. This node is intended to allow the host to read and
> > > + control the Type C ports. The node for this device should be under a
> > > + cros-ec node like google,cros-ec-spi.
> > > +
> > > +properties:
> > > + compatible:
> > > + const: google,cros-ec-typec
> > > +
> > > + connector:
> > > + description: A node that represents a physical Type C connector port
> > > + on the device.
> > > + type: object
> > > + properties:
> > > + compatible:
> > > + const: usb-c-connector
> > > + port-number:
> > > + description: The number used by the Chrome OS EC to identify
> > > + this type C port. Valid values are 0 - (EC_USB_PD_MAX_PORTS - 1).
> > > + power-role:
> > > + description: Determines the power role that the Type C port will
> > > + adopt.
> > > + maxItems: 1
> > > + contains:
> > > + enum:
> > > + - sink
> > > + - source
> > > + - dual
> > > + data-role:
> > > + description: Determines the data role that the Type C port will
> > > + adopt.
> > > + maxItems: 1
> > > + contains:
> > > + enum:
> > > + - host
> > > + - device
> > > + - dual
> > > + try-power-role:
> > > + description: Determines the preferred power role of the Type C port.
> > > + maxItems: 1
> > > + contains:
> > > + enum:
> > > + - sink
> > > + - source
> > > + - dual
> > > +
> > > + required:
> > > + - port-number
> > > + - power-role
> > > + - data-role
> > > + - try-power-role
> >
> > Do you really need to redefine those?
>
> No.
>
> >
> > I think you just need to mention that there is a required sub-node
> > "connector", and the place where it's described. So something
> > like this:
> >
> > Required sub-node:
> > - connector : The "usb-c-connector". The bindings of the
> > connector node are specified in:
> >
> > Documentation/devicetree/bindings/connector/usb-connector.txt
>
> Ideally, we'd convert this to schema first and then here just have:
I've converted this to schema here: https://lkml.org/lkml/2020/3/4/790
I've sent that patch separately from this series, since there is ongoing
discussion regarding the structure of the bindings (and use of OF graph
API) here.
>
> connector:
> $ref: /schemas/connector/usb-connector.yaml#
>
> >
> >
> > Then you just need to define the Chrome OS EC specific properties, so
> > I guess just the "port-number".
>
> 'reg' as Stephen suggested.
>
> Rob
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver
2020-02-29 0:43 ` Stephen Boyd
@ 2020-03-05 16:57 ` Heikki Krogerus
0 siblings, 0 replies; 13+ messages in thread
From: Heikki Krogerus @ 2020-03-05 16:57 UTC (permalink / raw)
To: Stephen Boyd
Cc: Prashant Malani, linux-kernel, enric.balletbo, bleung,
devicetree, Guenter Roeck, Mark Rutland, Rob Herring
On Fri, Feb 28, 2020 at 04:43:54PM -0800, Stephen Boyd wrote:
> Quoting Heikki Krogerus (2020-02-28 08:24:00)
> > On Thu, Feb 27, 2020 at 02:07:53PM -0800, Stephen Boyd wrote:
> > > Quoting Heikki Krogerus (2020-02-27 08:38:25)
> > > > No.
> > > >
> > > > The above follows the usb-connector bindings, so it is correct:
> > > > Documentation/devicetree/bindings/connector/usb-connector.txt
> > > >
> > > > So the connector is always a child of the "CC controller" with the USB
> > > > Type-C connectors, which in this case is the EC (from operating systems
> > > > perspective). The "CC controller" controls connectors, and it doesn't
> > > > actually do anything else. So placing the connectors under the
> > > > "connector controller" is also logically correct.
> > >
> > > Ah ok I see. The graph binding is for describing the data path, not the
> > > control path. Makes sense.
> > >
> > > >
> > > > > Yes, the connector is intimately involved with the EC here, but I would
> > > > > think that we would have an OF graph connection from the USB controller
> > > > > on the SoC to the USB connector, traversing through anything that may be
> > > > > in that path, such as a USB hub. Maybe the connector node itself can
> > > > > point to the EC type-c controller with some property like
> > > >
> > > > I think your idea here is that there should be only a single node for
> > > > each connector that is then linked with every component that it is
> > > > physically connected to (right?), but please note that that is not
> > > > enough. Every component attached to the connector must have its own
> > > > child node that represents the "port" that is physically connected to
> > > > the USB Type-C connector.
> > > >
> > > > So for example, the USB controller nodes have child nodes for every
> > > > USB2 port as well as for every USB3 port. Similarly, the GPU
> > > > controllers have child node for every DisplayPort, etc. And I believe
> > > > that is already how it has been done in DT (and also in ACPI).
> > >
> > > It looks like perhaps you're conflating ports in USB spec with the OF
> > > graph port? I want there to be one node per type-c connector that I can
> > > physically see on the device. Is that not sufficient?
> >
> > It is. We don't need more than one node that represents the physical
> > connector (and we should not have more than one node for that). And
> > actually, I was not mixing the OF graph ports and USB ports... I
> > think I should be talking about PHY instead of "port". That is
> > probable more clear.
> >
> > My point is that every PHY that is connected to a Type-C connector
> > must still be represented with its own node in devicetree and ACPI. So
> > there still needs to be a node for the USB2 PHY, USB3 PHY, DisplayPort
> > PHY, etc., on top of the connector node. I got the picture that you
> > are proposing that we don't need those PHY nodes anymore since we have
> > the connector nodes, but maybe I misunderstood?
>
> Alright. Maybe a full example will help. See below. I think I understand
> how it's supposed to look.
>
> >
> > > Are there any examples of the type-c connector in DT? I see some
> > > NXP/Freescale boards and one Renesas board so far. Maybe there are other
> > > discussions I can read up on?
> > >
> > > >
> > > > Those "port" nodes then just need to be linked with the "connector"
> > > > node. I think for that the idea was to use OF graph, but I'm really
> > > > sceptical about that. The problem is that with the USB Type-C
> > > > connectors we have to be able to identify the connections, i.e. which
> > > > endpoint is the USB2 port, which is the DisplayPort and so on, and OF
> > > > graph does not give any means to do that on its own. We will have to
> > > > rely on separate device properties in order to do the identification.
> > > > Currently it is not documented anywhere which property should be used
> > > > for that.
> > >
> > > I hope that this patch series can document this.
> >
> > Well, we do need that to be documented, but do we really need to block
> > this series because of that? This driver does not depend on OF graph
> > yet.
>
> I don't know. I think this binding patch will go for another round so
> maybe it's blocked in other ways?
Let me put it this way: Since the code in this series does not utilize
the connection description, it actually should _not_ propose the
binding for it. The connection description is out side the scope of
the series.
The connection description is still far from being clear in any case.
> > > Why can't that work by having multiple OF graph ports for USB2 port,
> > > DisplayPort, USB3 port, etc? The data path goes to the connector and
> > > we can attach more information to each port node to describe what
> > > type of endpoint is there like a DisplayPort capable type-c
> > > connector for example.
> >
> > The PHY nodes we must still always have. So the OF graph will always
> > describe the connection between the PHY and the connector, and the
> > connection between the PHY and the controller must be described
> > separately.
>
> Got it.
>
> Here's the same example that hopefully shows how all this stuff can
> work. I've added more nonsense to try and make it as complicated as
> possible.
You are not suggesting anything for the identification problem below,
so how do we know where does an endpoint actually go to in the code?
But could you actually please first explain what exactly is the
benefit from using OF graph with with the USB Type-C connector? Why
not just use good old phandles, i.e. device properties that return
reference to a node? With those the device property name by itself is
the identifier.
> / {
>
> // Expand single usb2/usb3 from SoC to 4 port hub
> usb-hub {
> compatible = "vendor,usb-hub-4-port";
> usb-vid = <0xaaad>;
> usb-pid = <0xffed>;
> vdd-supply = <&pp3300_usb>;
> reset-gpios = <&gpio_controller 50 GPIO_ACTIVE_LOW>;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> reg = <0>;
> usb2_hub0: endpoint0 {
> remote-endpoint = <&left_typec2>;
> };
>
> usb3_hub0: endpoint1 {
> remote-endpoint = <&left_typec3>;
> };
> };
Note. USB2 and USB3 are separate ports.
> port@1 {
> reg = <1>;
> usb2_hub1: endpoint0 {
> remote-endpoint = <&right_typec2>;
> };
>
> usb3_hub1: endpoint1 {
> remote-endpoint = <&right_typec3>;
> };
> };
>
> port@2 {
> reg = <2>;
> usb2_hub2: endpoint0 {
> remote-endpoint = <&left_typea2>;
> };
> usb3_hub2: endpoint1 {
> remote-endpoint = <&left_typea3>;
> };
> };
>
> port@3 {
> reg = <3>;
> usb2_hub3: endpoint0 {
> // Not connected
> };
> usb3_hub3: endpoint1 {
> // Not connected
> };
> };
>
> port@4 {
> reg = <4>;
> usb2_hub_in: endpoint0 {
> remote-endpoint = <&usb2_phy_out>;
> };
> usb3_hub_in: endpoint1 {
> remote-endpoint = <&usb3_phy_out>;
> };
> };
> };
> };
I don't still see any kind of independent device nodes for the USB
ports? Is the idea to only have the OF graph "ports" to represent the
physical USB ports?
It was clearly a mistake to talk about PHY, but in any case...
All the physical ports really need to have their own device nodes. If
we are to use OF graph, then a OF graph "port" is an interface to a
physical USB port, DisplayPort, Thunderbolt 3 port, whatever port,
that then has an endpoint to the connector. OF graph ports are
generic, and they can not represent physical points on the hardware,
while the USB, DP, whatever ports are specific and represent the
physical points on the hardware.
So basically, the OF graph describes the connection (the interconnect)
between the physical ports on the components and the connector, but it
does _not_ describe the connectors nor the physical ports themselves.
That is the only way I see this ever working. Otherwise you don't have
a clear place where to put for example device nodes describing
integrated USB devices, or even a clear way to describe port specific
properties.
> // Maybe this should go under EC node too if EC controls it
> // somehow?
> connector {
> compatible = "usb-a-connector";
> label = "type-A-left"
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> port@0 {
> reg = <0>;
> left_typea2: endpoint0 {
> remote-endpoint = <&usb2_hub2>;
> };
> left_typea3: endpoint1 {
> remote-endpoint = <&usb3_hub2>;
> };
> };
>
> };
Is this actually necessary? You will never associate the connector in
this case with a real device entry (struct device), so why define the
node at all?
The node will give you the connector type, but since (AFAIK) that
information is not used anywhere in case of Type-A, why bother?
> // Steer DP to either left or right type-c port
> mux {
> compatible = "vendor,dp-mux";
> // Inactive: port 0
> // Active: port 1
> mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> port@0 {
> reg = <0>;
> mux_dp_0: endpoint {
> remote-endpoint = <&right_typec_dp>;
> };
> };
>
> port@1 {
> reg = <1>;
> mux_dp_1: endpoint {
> remote-endpoint = <&left_typec_dp>;
> };
> };
>
> port@2 {
> reg = <1>;
> mux_dp_in: endpoint {
> remote-endpoint = <&dp_phy_out>;
> };
> };
> };
> };
If you use the mux between the DP and the connector, then you actually
should do the same with the USB ports as well. They will after all go
trough the same mux, right?
But using the mux in the middle even with the DP is problematic. We
will simply not always have a mux to control. Therefore our plan was
to always describe the connections directly from the connector to
whatever location they ultimately go to without the mux in the middle.
The mux will have its connection described in the connector node, but
in parallel.
I'll skip the rest if it's OK. I think at this point we really need an
explanation to the question: why do we have to use OF graph with the
USB Type-C connectors at all?
The identification problem has to be solved if it is to be used in any
case, but in the end, what value does OF graph add? Right now it looks
like something that just adds unnecessary complexity.
I'm sure that it is useful when it is possible to predict where the
endpoints actually go. For example with the cameras, every endpoint
an image processor has is most likely a sensor. But the USB Type-C
connectors can go anywhere (I guess even to the image processor).
With USB Type-C connector, the good old reference properties would
simply be superior.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-03-05 16:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20 0:30 [PATCH v3 0/4] platform/chrome: Add Type C connector class driver Prashant Malani
2020-02-20 0:30 ` [PATCH v3 1/4] dt-bindings: Add cros-ec Type C port driver Prashant Malani
2020-02-27 8:41 ` Stephen Boyd
2020-02-27 16:38 ` Heikki Krogerus
2020-02-27 22:07 ` Stephen Boyd
2020-02-28 16:24 ` Heikki Krogerus
2020-02-29 0:43 ` Stephen Boyd
2020-03-05 16:57 ` Heikki Krogerus
2020-02-27 15:12 ` Heikki Krogerus
2020-02-27 23:15 ` Rob Herring
2020-03-04 17:53 ` Prashant Malani
2020-02-20 0:30 ` [PATCH v3 2/4] platform/chrome: Add Type C connector class driver Prashant Malani
2020-02-27 14:25 ` Heikki Krogerus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.