Hi,
This series adds initial support for the Apple CD3217/3218 chip which is also
known as Apple ACE1/2. These chips are used on Apple M1 machines.
They are based on the TI TPS6598x chips with a few differences:
- The interrupt numbers have been changed
- The secondary i2c bus and its interrupt controller are connected to the
system management controller and must not be disturbed
- The chip comes up in a low power state and must be booted using the
"SPSS" (System Power State Switch maybe) command which is not
documented in the TI manual
- The interrupt mask must be set up explicitely
The first patches prepare for adding support and introduce almost no
functional changes to the driver. Only the interrupt mask is set up for
those as well since I think that makes sense either way.
Then the last patches enable the different behavior required for the Apple
chip selected by a new compatible in the device tree.
I'm only able to test this on my M1 and would appreciate if someone could
confirm this does not break the regular TPS chips. I'm also interested to
see if the normal chips also support the "SPSS" command.
Testing this on the M1 additionaly requires a pinctrl/gpio and a i2c driver
which are not ready to be submitted upstream yet. I've collected the current
work-in-progress state in a branch at [1] though if anyone wants to give
those a try.
Best,
Sven
[1] https://github.com/AsahiLinux/linux/tree/sven/i2c-tipd-WIP
Sven Peter (9):
dt-bindings: usb: tps6598x: Add Apple CD321x compatible
usb: typec: tipd: Prepare supporting different variants
usb: typec: tipd: Allow irq controller selection
usb: typec: tipd: Add short-circuit for no irqs
usb: typec: tipd: Allow to configure irq bits
usb: typec: tipd: Setup IntMask explicitly
usb: typec: tipd: Add support for apple,cd321x
usb: typec: tipd: Switch power state to S0 for Apple variant
usb: typec: tipd: Remove FIXME about testing with I2C_FUNC_I2C
.../devicetree/bindings/usb/ti,tps6598x.yaml | 4 +
drivers/usb/typec/tipd/core.c | 140 ++++++++++++++++--
drivers/usb/typec/tipd/tps6598x.h | 12 ++
drivers/usb/typec/tipd/trace.h | 27 ++++
4 files changed, 168 insertions(+), 15 deletions(-)
--
2.25.1
A variant of the TI TPS 6598x Type-C Port Switch and Power Delivery
controller known as Apple CD321x is present on boards with Apple SoCs
such as the M1. Add its compatible to the device tree binding.
Signed-off-by: Sven Peter <[email protected]>
---
Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
index f6819bf2a3b5..a4c53b1f1af3 100644
--- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
@@ -12,10 +12,14 @@ maintainers:
description: |
Texas Instruments 6598x Type-C Port Switch and Power Delivery controller
+ A variant of this controller known as Apple CD321x or Apple ACE is also
+ present on hardware with Apple SoCs such as the M1.
+
properties:
compatible:
enum:
- ti,tps6598x
+ - apple,cd321x
reg:
maxItems: 1
--
2.25.1
Apple M1 machines come with a variant of the TI TPS6598x and will need
some changes to the current logic. Let's prepare for that by setting up
the infrastructure required to support different variants of this chip
identified by the DT compatible.
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 21b3ae25c76d..656020e7f533 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -9,6 +9,7 @@
#include <linux/i2c.h>
#include <linux/acpi.h>
#include <linux/module.h>
+#include <linux/of_device.h>
#include <linux/power_supply.h>
#include <linux/regmap.h>
#include <linux/interrupt.h>
@@ -76,6 +77,10 @@ static const char *const modes[] = {
/* Unrecognized commands will be replaced with "!CMD" */
#define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
+struct tps6598x_hw {
+};
+static const struct tps6598x_hw ti_tps6598x_data;
+
struct tps6598x {
struct device *dev;
struct regmap *regmap;
@@ -91,6 +96,8 @@ struct tps6598x {
struct power_supply *psy;
struct power_supply_desc psy_desc;
enum power_supply_usb_type usb_type;
+
+ const struct tps6598x_hw *hw;
};
static enum power_supply_property tps6598x_psy_props[] = {
@@ -590,6 +597,13 @@ static int tps6598x_probe(struct i2c_client *client)
if (!tps)
return -ENOMEM;
+ if (client->dev.of_node)
+ tps->hw = of_device_get_match_data(&client->dev);
+ else
+ tps->hw = &ti_tps6598x_data;
+ if (!tps->hw)
+ return -EINVAL;
+
mutex_init(&tps->lock);
tps->dev = &client->dev;
@@ -729,8 +743,11 @@ static int tps6598x_remove(struct i2c_client *client)
return 0;
}
+static const struct tps6598x_hw ti_tps6598x_data = {
+};
+
static const struct of_device_id tps6598x_of_match[] = {
- { .compatible = "ti,tps6598x", },
+ { .compatible = "ti,tps6598x", .data = &ti_tps6598x_data },
{}
};
MODULE_DEVICE_TABLE(of, tps6598x_of_match);
--
2.25.1
If no interrupts are set in IntEventX directly skip to the end of the
interrupt handler and return IRQ_NONE instead of IRQ_HANDLED.
This possibly allows to detect spurious interrupts if the i2c bus is fast
enough.
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index c2c399722c37..4a6d66250fef 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -434,6 +434,8 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
trace_tps6598x_irq(event1, event2);
event = event1 | event2;
+ if (!event)
+ goto err_unlock;
ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
if (ret) {
@@ -481,7 +483,9 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
err_unlock:
mutex_unlock(&tps->lock);
- return IRQ_HANDLED;
+ if (event)
+ return IRQ_HANDLED;
+ return IRQ_NONE;
}
static int tps6598x_check_mode(struct tps6598x *tps)
--
2.25.1
TI TPS6598x chips come with two separate i2c buses which each have an
independent interrupt controller. When only a single controller is
connected both can just be used.
On Apple M1 machines the secondary bus is however connected to the system
management controller and we must not modify its configuration. Prepare
for that by allowing to chose which interrupt controller(s) are used.
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 656020e7f533..c2c399722c37 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -78,6 +78,8 @@ static const char *const modes[] = {
#define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
struct tps6598x_hw {
+ bool use_int1;
+ bool use_int2;
};
static const struct tps6598x_hw ti_tps6598x_data;
@@ -411,22 +413,28 @@ static const struct typec_operations tps6598x_ops = {
static irqreturn_t tps6598x_interrupt(int irq, void *data)
{
struct tps6598x *tps = data;
- u64 event1;
- u64 event2;
+ u64 event1 = 0;
+ u64 event2 = 0;
+ u64 event = 0;
u32 status, data_status;
u16 pwr_status;
int ret;
mutex_lock(&tps->lock);
- ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1);
- ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2);
+ ret = 0;
+ if (tps->hw->use_int1)
+ ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1);
+ if (tps->hw->use_int2)
+ ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2);
if (ret) {
dev_err(tps->dev, "%s: failed to read events\n", __func__);
goto err_unlock;
}
trace_tps6598x_irq(event1, event2);
+ event = event1 | event2;
+
ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
if (ret) {
dev_err(tps->dev, "%s: failed to read status\n", __func__);
@@ -434,7 +442,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
}
trace_tps6598x_status(status);
- if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE) {
+ if (event & TPS_REG_INT_POWER_STATUS_UPDATE) {
ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
if (ret < 0) {
dev_err(tps->dev, "failed to read power status: %d\n", ret);
@@ -443,7 +451,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
trace_tps6598x_power_status(pwr_status);
}
- if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) {
+ if (event & TPS_REG_INT_DATA_STATUS_UPDATE) {
ret = tps6598x_read32(tps, TPS_REG_DATA_STATUS, &data_status);
if (ret < 0) {
dev_err(tps->dev, "failed to read data status: %d\n", ret);
@@ -453,7 +461,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
}
/* Handle plug insert or removal */
- if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT) {
+ if (event & TPS_REG_INT_PLUG_EVENT) {
if (status & TPS_STATUS_PLUG_PRESENT) {
ret = tps6598x_connect(tps, status);
if (ret)
@@ -465,8 +473,10 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
}
err_clear_ints:
- tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
- tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
+ if (tps->hw->use_int1)
+ tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
+ if (tps->hw->use_int2)
+ tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
err_unlock:
mutex_unlock(&tps->lock);
@@ -744,6 +754,8 @@ static int tps6598x_remove(struct i2c_client *client)
}
static const struct tps6598x_hw ti_tps6598x_data = {
+ .use_int1 = true,
+ .use_int2 = true,
};
static const struct of_device_id tps6598x_of_match[] = {
--
2.25.1
The Apple CD321x comes up in a low-power state after boot. Usually, the
bootloader will already power it up to S0 but let's do it here as well
in case that didn't happen.
Suggested-by: Stan Skowronek <[email protected]>
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 44 +++++++++++++++++++++++++++++++
drivers/usb/typec/tipd/tps6598x.h | 6 +++++
2 files changed, 50 insertions(+)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index e96b17fe6af6..26807c050662 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -30,6 +30,7 @@
#define TPS_REG_INT_MASK2 0x17
#define TPS_REG_INT_CLEAR1 0x18
#define TPS_REG_INT_CLEAR2 0x19
+#define TPS_REG_SYSTEM_POWER_STATE 0x20
#define TPS_REG_STATUS 0x1a
#define TPS_REG_SYSTEM_CONF 0x28
#define TPS_REG_CTRL_CONF 0x29
@@ -84,6 +85,8 @@ struct tps6598x_hw {
unsigned int irq_data_status_update;
unsigned int irq_plug_event;
void (*irq_trace)(u64 event1, u64 event2);
+
+ bool supports_spss;
};
static const struct tps6598x_hw ti_tps6598x_data;
@@ -161,6 +164,11 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
}
+static inline int tps6598x_read8(struct tps6598x *tps, u8 reg, u8 *val)
+{
+ return tps6598x_block_read(tps, reg, val, sizeof(u8));
+}
+
static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
{
return tps6598x_block_read(tps, reg, val, sizeof(u16));
@@ -572,6 +580,35 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
return ret;
}
+static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
+{
+ u8 state;
+ int ret;
+
+ if (!tps->hw->supports_spss)
+ return 0;
+
+ ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
+ if (ret)
+ return ret;
+
+ if (state == target_state)
+ return 0;
+
+ ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
+ if (ret)
+ return ret;
+
+ ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
+ if (ret)
+ return ret;
+
+ if (state != target_state)
+ return -EINVAL;
+
+ return 0;
+}
+
static int devm_tps6598_psy_register(struct tps6598x *tps)
{
struct power_supply_config psy_cfg = {};
@@ -648,6 +685,11 @@ static int tps6598x_probe(struct i2c_client *client)
if (ret)
return ret;
+ /* Switch Apple chips to the correct system power state */
+ ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
+ if (ret)
+ return ret;
+
ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
if (ret < 0)
return ret;
@@ -786,6 +828,7 @@ static const struct tps6598x_hw ti_tps6598x_data = {
.irq_data_status_update = TPS_REG_INT_DATA_STATUS_UPDATE,
.irq_plug_event = TPS_REG_INT_PLUG_EVENT,
.irq_trace = trace_tps6598x_irq,
+ .supports_spss = false,
};
static const struct tps6598x_hw apple_cd321x_data = {
@@ -795,6 +838,7 @@ static const struct tps6598x_hw apple_cd321x_data = {
.irq_data_status_update = APPLE_TPS_REG_INT_DATA_STATUS_UPDATE,
.irq_plug_event = APPLE_TPS_REG_INT_PLUG_EVENT,
.irq_trace = trace_cd321x_irq,
+ .supports_spss = true,
};
static const struct of_device_id tps6598x_of_match[] = {
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 36b482733297..5e6ff51aa657 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -135,6 +135,12 @@
#define APPLE_TPS_REG_INT_STATUS_UPDATE BIT(8)
#define APPLE_TPS_REG_INT_PLUG_EVENT BIT(1)
+/* TPS_REG_SYSTEM_POWER_STATE states */
+#define TPS_SYSTEM_POWER_STATE_S0 0x00
+#define TPS_SYSTEM_POWER_STATE_S3 0x03
+#define TPS_SYSTEM_POWER_STATE_S4 0x04
+#define TPS_SYSTEM_POWER_STATE_S5 0x05
+
/* TPS_REG_POWER_STATUS bits */
#define TPS_POWER_STATUS_CONNECTION(x) TPS_FIELD_GET(BIT(0), (x))
#define TPS_POWER_STATUS_SOURCESINK(x) TPS_FIELD_GET(BIT(1), (x))
--
2.25.1
The Apple i2c bus uses I2C_FUNC_I2C and I've tested this quite
extensivly in the past days. Remove the FIXME about that testing :-)
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 26807c050662..3b6878e22ce9 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -673,9 +673,6 @@ static int tps6598x_probe(struct i2c_client *client)
/*
* Checking can the adapter handle SMBus protocol. If it can not, the
* driver needs to take care of block reads separately.
- *
- * FIXME: Testing with I2C_FUNC_I2C. regmap-i2c uses I2C protocol
- * unconditionally if the adapter has I2C_FUNC_I2C set.
*/
if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
tps->i2c_protocol = true;
--
2.25.1
After all the preparations in the previous commits we can now finally
add support for the Apple CD321x chip which is a variant of the TI TPS
6598x chip. The major differences are the changed interrupt numbers and
the concurrent connection to the SMC which we must not disturb.
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 10 ++++++++++
drivers/usb/typec/tipd/tps6598x.h | 6 ++++++
drivers/usb/typec/tipd/trace.h | 27 +++++++++++++++++++++++++++
3 files changed, 43 insertions(+)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 2058e8cca631..e96b17fe6af6 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -788,8 +788,18 @@ static const struct tps6598x_hw ti_tps6598x_data = {
.irq_trace = trace_tps6598x_irq,
};
+static const struct tps6598x_hw apple_cd321x_data = {
+ .use_int1 = true,
+ .use_int2 = false,
+ .irq_power_status_update = APPLE_TPS_REG_INT_POWER_STATUS_UPDATE,
+ .irq_data_status_update = APPLE_TPS_REG_INT_DATA_STATUS_UPDATE,
+ .irq_plug_event = APPLE_TPS_REG_INT_PLUG_EVENT,
+ .irq_trace = trace_cd321x_irq,
+};
+
static const struct of_device_id tps6598x_of_match[] = {
{ .compatible = "ti,tps6598x", .data = &ti_tps6598x_data },
+ { .compatible = "apple,cd321x", .data = &apple_cd321x_data },
{}
};
MODULE_DEVICE_TABLE(of, tps6598x_of_match);
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 003a577be216..36b482733297 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -129,6 +129,12 @@
#define TPS_REG_INT_HARD_RESET BIT(1)
#define TPS_REG_INT_PD_SOFT_RESET BIT(0)
+/* Apple-specific TPS_REG_INT_* bits */
+#define APPLE_TPS_REG_INT_DATA_STATUS_UPDATE BIT(10)
+#define APPLE_TPS_REG_INT_POWER_STATUS_UPDATE BIT(9)
+#define APPLE_TPS_REG_INT_STATUS_UPDATE BIT(8)
+#define APPLE_TPS_REG_INT_PLUG_EVENT BIT(1)
+
/* TPS_REG_POWER_STATUS bits */
#define TPS_POWER_STATUS_CONNECTION(x) TPS_FIELD_GET(BIT(0), (x))
#define TPS_POWER_STATUS_SOURCESINK(x) TPS_FIELD_GET(BIT(1), (x))
diff --git a/drivers/usb/typec/tipd/trace.h b/drivers/usb/typec/tipd/trace.h
index 5d09d6f78930..090633a1ae1d 100644
--- a/drivers/usb/typec/tipd/trace.h
+++ b/drivers/usb/typec/tipd/trace.h
@@ -67,6 +67,13 @@
{ TPS_REG_INT_USER_VID_ALT_MODE_ATTN_VDM, "USER_VID_ALT_MODE_ATTN_VDM" }, \
{ TPS_REG_INT_USER_VID_ALT_MODE_OTHER_VDM, "USER_VID_ALT_MODE_OTHER_VDM" })
+#define show_cd321x_irq_flags(flags) \
+ __print_flags_u64(flags, "|", \
+ { APPLE_TPS_REG_INT_PLUG_EVENT, "PLUG_EVENT" }, \
+ { APPLE_TPS_REG_INT_POWER_STATUS_UPDATE, "POWER_STATUS_UPDATE" }, \
+ { APPLE_TPS_REG_INT_STATUS_UPDATE, "DATA_STATUS_UPDATE" }, \
+ { APPLE_TPS_REG_INT_PLUG_EVENT, "STATUS_UPDATE" })
+
#define TPS6598X_STATUS_FLAGS_MASK (GENMASK(31, 0) ^ (TPS_STATUS_CONN_STATE_MASK | \
TPS_STATUS_PP_5V0_SWITCH_MASK | \
TPS_STATUS_PP_HV_SWITCH_MASK | \
@@ -207,6 +214,26 @@ TRACE_EVENT(tps6598x_irq,
show_irq_flags(__entry->event2))
);
+TRACE_EVENT(cd321x_irq,
+ TP_PROTO(u64 event1,
+ u64 event2),
+ TP_ARGS(event1, event2),
+
+ TP_STRUCT__entry(
+ __field(u64, event1)
+ __field(u64, event2)
+ ),
+
+ TP_fast_assign(
+ __entry->event1 = event1;
+ __entry->event2 = event2;
+ ),
+
+ TP_printk("event1=%s, event2=%s",
+ show_cd321x_irq_flags(__entry->event1),
+ show_cd321x_irq_flags(__entry->event2))
+);
+
TRACE_EVENT(tps6598x_status,
TP_PROTO(u32 status),
TP_ARGS(status),
--
2.25.1
The Apple variant of the TI TPS6598x chip uses different interrupt
numbers. Prepare for that by allowing those to be configured depending
on the compatible.
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 4a6d66250fef..d191e7435018 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -80,6 +80,10 @@ static const char *const modes[] = {
struct tps6598x_hw {
bool use_int1;
bool use_int2;
+ unsigned int irq_power_status_update;
+ unsigned int irq_data_status_update;
+ unsigned int irq_plug_event;
+ void (*irq_trace)(u64 event1, u64 event2);
};
static const struct tps6598x_hw ti_tps6598x_data;
@@ -431,7 +435,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
dev_err(tps->dev, "%s: failed to read events\n", __func__);
goto err_unlock;
}
- trace_tps6598x_irq(event1, event2);
+ tps->hw->irq_trace(event1, event2);
event = event1 | event2;
if (!event)
@@ -444,7 +448,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
}
trace_tps6598x_status(status);
- if (event & TPS_REG_INT_POWER_STATUS_UPDATE) {
+ if (event & tps->hw->irq_power_status_update) {
ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
if (ret < 0) {
dev_err(tps->dev, "failed to read power status: %d\n", ret);
@@ -453,7 +457,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
trace_tps6598x_power_status(pwr_status);
}
- if (event & TPS_REG_INT_DATA_STATUS_UPDATE) {
+ if (event & tps->hw->irq_data_status_update) {
ret = tps6598x_read32(tps, TPS_REG_DATA_STATUS, &data_status);
if (ret < 0) {
dev_err(tps->dev, "failed to read data status: %d\n", ret);
@@ -463,7 +467,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
}
/* Handle plug insert or removal */
- if (event & TPS_REG_INT_PLUG_EVENT) {
+ if (event & tps->hw->irq_plug_event) {
if (status & TPS_STATUS_PLUG_PRESENT) {
ret = tps6598x_connect(tps, status);
if (ret)
@@ -760,6 +764,10 @@ static int tps6598x_remove(struct i2c_client *client)
static const struct tps6598x_hw ti_tps6598x_data = {
.use_int1 = true,
.use_int2 = true,
+ .irq_power_status_update = TPS_REG_INT_POWER_STATUS_UPDATE,
+ .irq_data_status_update = TPS_REG_INT_DATA_STATUS_UPDATE,
+ .irq_plug_event = TPS_REG_INT_PLUG_EVENT,
+ .irq_trace = trace_tps6598x_irq,
};
static const struct of_device_id tps6598x_of_match[] = {
--
2.25.1
Right now the code relies on the bootloader to set up the interrupt mask
correctly. This usually works but let's make sure to do it explicitly to
guarantee it will always work.
Signed-off-by: Sven Peter <[email protected]>
---
drivers/usb/typec/tipd/core.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index d191e7435018..2058e8cca631 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -728,6 +728,24 @@ static int tps6598x_probe(struct i2c_client *client)
dev_err(&client->dev, "failed to register partner\n");
}
+ if (tps->hw->use_int1) {
+ ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
+ tps->hw->irq_power_status_update |
+ tps->hw->irq_data_status_update |
+ tps->hw->irq_plug_event);
+ if (ret)
+ goto err_role_put;
+ }
+
+ if (tps->hw->use_int2) {
+ ret = tps6598x_write64(tps, TPS_REG_INT_MASK2,
+ tps->hw->irq_power_status_update |
+ tps->hw->irq_data_status_update |
+ tps->hw->irq_plug_event);
+ if (ret)
+ goto err_role_put;
+ }
+
ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
tps6598x_interrupt,
IRQF_SHARED | IRQF_ONESHOT,
--
2.25.1
(Asked off list for completeness) use_int2 is always true in this series , so I'm not sure it makes sense to configure it. But for symmetry I think it's good, so
Reviewed-by: Alyssa Rosenzweig <[email protected]>
Reviewed-by: Alyssa Rosenzweig <[email protected]>
This seems reasonable (and has my Tested-by on the M1) but I don't know
enough about non-apple TPS to feel comfortable reviewing.
Reviewed-by: Alyssa Rosenzweig <[email protected]>
Hi Sven,
Happy to see this up for review. I skipped a commit that's too deep into
tps driver guts for me to grok, but otherwise you have my r-bs, and this
series has my
Tested-by: Alyssa Rosenzweig <[email protected]>
Thanks,
Alyssa
Reviewed-by: Alyssa Rosenzweig <[email protected]>
On Sat , Sep 18, 2021 at 02:09:34PM +0200, Sven Peter wrote:
> The Apple i2c bus uses I2C_FUNC_I2C and I've tested this quite
> extensivly in the past days. Remove the FIXME about that testing :-)
>
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 26807c050662..3b6878e22ce9 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -673,9 +673,6 @@ static int tps6598x_probe(struct i2c_client *client)
> /*
> * Checking can the adapter handle SMBus protocol. If it can not, the
> * driver needs to take care of block reads separately.
> - *
> - * FIXME: Testing with I2C_FUNC_I2C. regmap-i2c uses I2C protocol
> - * unconditionally if the adapter has I2C_FUNC_I2C set.
> */
> if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> tps->i2c_protocol = true;
> --
> 2.25.1
>
Reviewed-by: Alyssa Rosenzweig <[email protected]>
On Sat , Sep 18, 2021 at 02:09:33PM +0200, Sven Peter wrote:
> The Apple CD321x comes up in a low-power state after boot. Usually, the
> bootloader will already power it up to S0 but let's do it here as well
> in case that didn't happen.
>
> Suggested-by: Stan Skowronek <[email protected]>
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 44 +++++++++++++++++++++++++++++++
> drivers/usb/typec/tipd/tps6598x.h | 6 +++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index e96b17fe6af6..26807c050662 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -30,6 +30,7 @@
> #define TPS_REG_INT_MASK2 0x17
> #define TPS_REG_INT_CLEAR1 0x18
> #define TPS_REG_INT_CLEAR2 0x19
> +#define TPS_REG_SYSTEM_POWER_STATE 0x20
> #define TPS_REG_STATUS 0x1a
> #define TPS_REG_SYSTEM_CONF 0x28
> #define TPS_REG_CTRL_CONF 0x29
> @@ -84,6 +85,8 @@ struct tps6598x_hw {
> unsigned int irq_data_status_update;
> unsigned int irq_plug_event;
> void (*irq_trace)(u64 event1, u64 event2);
> +
> + bool supports_spss;
> };
> static const struct tps6598x_hw ti_tps6598x_data;
>
> @@ -161,6 +164,11 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
> return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
> }
>
> +static inline int tps6598x_read8(struct tps6598x *tps, u8 reg, u8 *val)
> +{
> + return tps6598x_block_read(tps, reg, val, sizeof(u8));
> +}
> +
> static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
> {
> return tps6598x_block_read(tps, reg, val, sizeof(u16));
> @@ -572,6 +580,35 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
> return ret;
> }
>
> +static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
> +{
> + u8 state;
> + int ret;
> +
> + if (!tps->hw->supports_spss)
> + return 0;
> +
> + ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
> + if (ret)
> + return ret;
> +
> + if (state == target_state)
> + return 0;
> +
> + ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
> + if (ret)
> + return ret;
> +
> + ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
> + if (ret)
> + return ret;
> +
> + if (state != target_state)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int devm_tps6598_psy_register(struct tps6598x *tps)
> {
> struct power_supply_config psy_cfg = {};
> @@ -648,6 +685,11 @@ static int tps6598x_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> + /* Switch Apple chips to the correct system power state */
> + ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
> + if (ret)
> + return ret;
> +
> ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
> if (ret < 0)
> return ret;
> @@ -786,6 +828,7 @@ static const struct tps6598x_hw ti_tps6598x_data = {
> .irq_data_status_update = TPS_REG_INT_DATA_STATUS_UPDATE,
> .irq_plug_event = TPS_REG_INT_PLUG_EVENT,
> .irq_trace = trace_tps6598x_irq,
> + .supports_spss = false,
> };
>
> static const struct tps6598x_hw apple_cd321x_data = {
> @@ -795,6 +838,7 @@ static const struct tps6598x_hw apple_cd321x_data = {
> .irq_data_status_update = APPLE_TPS_REG_INT_DATA_STATUS_UPDATE,
> .irq_plug_event = APPLE_TPS_REG_INT_PLUG_EVENT,
> .irq_trace = trace_cd321x_irq,
> + .supports_spss = true,
> };
>
> static const struct of_device_id tps6598x_of_match[] = {
> diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
> index 36b482733297..5e6ff51aa657 100644
> --- a/drivers/usb/typec/tipd/tps6598x.h
> +++ b/drivers/usb/typec/tipd/tps6598x.h
> @@ -135,6 +135,12 @@
> #define APPLE_TPS_REG_INT_STATUS_UPDATE BIT(8)
> #define APPLE_TPS_REG_INT_PLUG_EVENT BIT(1)
>
> +/* TPS_REG_SYSTEM_POWER_STATE states */
> +#define TPS_SYSTEM_POWER_STATE_S0 0x00
> +#define TPS_SYSTEM_POWER_STATE_S3 0x03
> +#define TPS_SYSTEM_POWER_STATE_S4 0x04
> +#define TPS_SYSTEM_POWER_STATE_S5 0x05
> +
> /* TPS_REG_POWER_STATUS bits */
> #define TPS_POWER_STATUS_CONNECTION(x) TPS_FIELD_GET(BIT(0), (x))
> #define TPS_POWER_STATUS_SOURCESINK(x) TPS_FIELD_GET(BIT(1), (x))
> --
> 2.25.1
>
Reviewed-by: Alyssa Rosenzweig <[email protected]>
On Sat , Sep 18, 2021 at 02:09:26PM +0200, Sven Peter wrote:
> A variant of the TI TPS 6598x Type-C Port Switch and Power Delivery
> controller known as Apple CD321x is present on boards with Apple SoCs
> such as the M1. Add its compatible to the device tree binding.
>
> Signed-off-by: Sven Peter <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> index f6819bf2a3b5..a4c53b1f1af3 100644
> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> @@ -12,10 +12,14 @@ maintainers:
> description: |
> Texas Instruments 6598x Type-C Port Switch and Power Delivery controller
>
> + A variant of this controller known as Apple CD321x or Apple ACE is also
> + present on hardware with Apple SoCs such as the M1.
> +
> properties:
> compatible:
> enum:
> - ti,tps6598x
> + - apple,cd321x
> reg:
> maxItems: 1
>
> --
> 2.25.1
>
Hi,
On Sat, Sep 18, 2021 at 02:09:27PM +0200, Sven Peter wrote:
> Apple M1 machines come with a variant of the TI TPS6598x and will need
> some changes to the current logic. Let's prepare for that by setting up
> the infrastructure required to support different variants of this chip
> identified by the DT compatible.
I think in this case it would make sense to just squash this into the
next patch.
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 21b3ae25c76d..656020e7f533 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -9,6 +9,7 @@
> #include <linux/i2c.h>
> #include <linux/acpi.h>
> #include <linux/module.h>
> +#include <linux/of_device.h>
> #include <linux/power_supply.h>
> #include <linux/regmap.h>
> #include <linux/interrupt.h>
> @@ -76,6 +77,10 @@ static const char *const modes[] = {
> /* Unrecognized commands will be replaced with "!CMD" */
> #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
>
> +struct tps6598x_hw {
> +};
Black line here.
> +static const struct tps6598x_hw ti_tps6598x_data;
> +
> struct tps6598x {
> struct device *dev;
> struct regmap *regmap;
> @@ -91,6 +96,8 @@ struct tps6598x {
> struct power_supply *psy;
> struct power_supply_desc psy_desc;
> enum power_supply_usb_type usb_type;
> +
> + const struct tps6598x_hw *hw;
> };
>
> static enum power_supply_property tps6598x_psy_props[] = {
> @@ -590,6 +597,13 @@ static int tps6598x_probe(struct i2c_client *client)
> if (!tps)
> return -ENOMEM;
>
> + if (client->dev.of_node)
> + tps->hw = of_device_get_match_data(&client->dev);
> + else
> + tps->hw = &ti_tps6598x_data;
> + if (!tps->hw)
> + return -EINVAL;
tps->hw = of_device_get_match_data(&client->dev);
if (!tps->hw)
tps->hw = &ti_tps6598x_data;
> mutex_init(&tps->lock);
> tps->dev = &client->dev;
>
> @@ -729,8 +743,11 @@ static int tps6598x_remove(struct i2c_client *client)
> return 0;
> }
>
> +static const struct tps6598x_hw ti_tps6598x_data = {
> +};
You could also move that above tps6598x_probe() and get rid of the
forward declaration.
> static const struct of_device_id tps6598x_of_match[] = {
> - { .compatible = "ti,tps6598x", },
> + { .compatible = "ti,tps6598x", .data = &ti_tps6598x_data },
> {}
> };
> MODULE_DEVICE_TABLE(of, tps6598x_of_match);
thanks,
--
heikki
On Sat, Sep 18, 2021 at 02:09:28PM +0200, Sven Peter wrote:
> TI TPS6598x chips come with two separate i2c buses which each have an
> independent interrupt controller. When only a single controller is
> connected both can just be used.
> On Apple M1 machines the secondary bus is however connected to the system
> management controller and we must not modify its configuration. Prepare
> for that by allowing to chose which interrupt controller(s) are used.
This is good, but...
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 656020e7f533..c2c399722c37 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -78,6 +78,8 @@ static const char *const modes[] = {
> #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
>
> struct tps6598x_hw {
> + bool use_int1;
> + bool use_int2;
> };
Wouldn't it be better to read that information from a device
property/properties?
Driver data is OK, but device property would be better. We don't have
separate compatible for this on every board that uses it and we have
also ACPI platforms.
thanks,
--
heikki
On Sat, Sep 18, 2021 at 02:09:29PM +0200, Sven Peter wrote:
> If no interrupts are set in IntEventX directly skip to the end of the
> interrupt handler and return IRQ_NONE instead of IRQ_HANDLED.
> This possibly allows to detect spurious interrupts if the i2c bus is fast
> enough.
>
> Signed-off-by: Sven Peter <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index c2c399722c37..4a6d66250fef 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -434,6 +434,8 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
> trace_tps6598x_irq(event1, event2);
>
> event = event1 | event2;
> + if (!event)
> + goto err_unlock;
>
> ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
> if (ret) {
> @@ -481,7 +483,9 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
> err_unlock:
> mutex_unlock(&tps->lock);
>
> - return IRQ_HANDLED;
> + if (event)
> + return IRQ_HANDLED;
> + return IRQ_NONE;
> }
>
> static int tps6598x_check_mode(struct tps6598x *tps)
> --
> 2.25.1
thanks,
--
heikki
On Sat, Sep 18, 2021 at 02:09:30PM +0200, Sven Peter wrote:
> The Apple variant of the TI TPS6598x chip uses different interrupt
> numbers. Prepare for that by allowing those to be configured depending
> on the compatible.
OK, so I think this justifies having a completely separate irq
handler for your board.
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 4a6d66250fef..d191e7435018 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -80,6 +80,10 @@ static const char *const modes[] = {
> struct tps6598x_hw {
> bool use_int1;
> bool use_int2;
> + unsigned int irq_power_status_update;
> + unsigned int irq_data_status_update;
> + unsigned int irq_plug_event;
> + void (*irq_trace)(u64 event1, u64 event2);
> };
Then I believe you don't need any of that.
thanks,
--
heikki
On Sat, Sep 18, 2021 at 02:09:31PM +0200, Sven Peter wrote:
> Right now the code relies on the bootloader to set up the interrupt mask
> correctly. This usually works but let's make sure to do it explicitly to
> guarantee it will always work.
>
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index d191e7435018..2058e8cca631 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -728,6 +728,24 @@ static int tps6598x_probe(struct i2c_client *client)
> dev_err(&client->dev, "failed to register partner\n");
> }
>
> + if (tps->hw->use_int1) {
> + ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> + tps->hw->irq_power_status_update |
> + tps->hw->irq_data_status_update |
> + tps->hw->irq_plug_event);
> + if (ret)
> + goto err_role_put;
> + }
> +
> + if (tps->hw->use_int2) {
> + ret = tps6598x_write64(tps, TPS_REG_INT_MASK2,
> + tps->hw->irq_power_status_update |
> + tps->hw->irq_data_status_update |
> + tps->hw->irq_plug_event);
> + if (ret)
> + goto err_role_put;
> + }
You should first only set the mask on your board. We can then see if
it's something that should be done on other boards as well later.
thanks,
--
heikki
On Sat, Sep 18, 2021 at 02:09:34PM +0200, Sven Peter wrote:
> The Apple i2c bus uses I2C_FUNC_I2C and I've tested this quite
> extensivly in the past days. Remove the FIXME about that testing :-)
>
> Signed-off-by: Sven Peter <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 26807c050662..3b6878e22ce9 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -673,9 +673,6 @@ static int tps6598x_probe(struct i2c_client *client)
> /*
> * Checking can the adapter handle SMBus protocol. If it can not, the
> * driver needs to take care of block reads separately.
> - *
> - * FIXME: Testing with I2C_FUNC_I2C. regmap-i2c uses I2C protocol
> - * unconditionally if the adapter has I2C_FUNC_I2C set.
> */
> if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> tps->i2c_protocol = true;
> --
> 2.25.1
thanks,
--
heikki
On Sat, Sep 18, 2021 at 02:09:33PM +0200, Sven Peter wrote:
> The Apple CD321x comes up in a low-power state after boot. Usually, the
> bootloader will already power it up to S0 but let's do it here as well
> in case that didn't happen.
>
> Suggested-by: Stan Skowronek <[email protected]>
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/usb/typec/tipd/core.c | 44 +++++++++++++++++++++++++++++++
> drivers/usb/typec/tipd/tps6598x.h | 6 +++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index e96b17fe6af6..26807c050662 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -30,6 +30,7 @@
> #define TPS_REG_INT_MASK2 0x17
> #define TPS_REG_INT_CLEAR1 0x18
> #define TPS_REG_INT_CLEAR2 0x19
> +#define TPS_REG_SYSTEM_POWER_STATE 0x20
> #define TPS_REG_STATUS 0x1a
> #define TPS_REG_SYSTEM_CONF 0x28
> #define TPS_REG_CTRL_CONF 0x29
> @@ -84,6 +85,8 @@ struct tps6598x_hw {
> unsigned int irq_data_status_update;
> unsigned int irq_plug_event;
> void (*irq_trace)(u64 event1, u64 event2);
> +
> + bool supports_spss;
> };
> static const struct tps6598x_hw ti_tps6598x_data;
>
> @@ -161,6 +164,11 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
> return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
> }
>
> +static inline int tps6598x_read8(struct tps6598x *tps, u8 reg, u8 *val)
> +{
> + return tps6598x_block_read(tps, reg, val, sizeof(u8));
> +}
> +
> static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
> {
> return tps6598x_block_read(tps, reg, val, sizeof(u16));
> @@ -572,6 +580,35 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
> return ret;
> }
>
> +static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
> +{
> + u8 state;
> + int ret;
> +
> + if (!tps->hw->supports_spss)
> + return 0;
> +
> + ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
> + if (ret)
> + return ret;
> +
> + if (state == target_state)
> + return 0;
> +
> + ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
> + if (ret)
> + return ret;
> +
> + ret = tps6598x_read8(tps, TPS_REG_SYSTEM_POWER_STATE, &state);
> + if (ret)
> + return ret;
> +
> + if (state != target_state)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int devm_tps6598_psy_register(struct tps6598x *tps)
> {
> struct power_supply_config psy_cfg = {};
> @@ -648,6 +685,11 @@ static int tps6598x_probe(struct i2c_client *client)
> if (ret)
> return ret;
>
> + /* Switch Apple chips to the correct system power state */
> + ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
> + if (ret)
> + return ret;
If you call this from the same quirk where you set the mask for your
board, you don't need that supports_spss flag at all, right?
> ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
> if (ret < 0)
> return ret;
> @@ -786,6 +828,7 @@ static const struct tps6598x_hw ti_tps6598x_data = {
> .irq_data_status_update = TPS_REG_INT_DATA_STATUS_UPDATE,
> .irq_plug_event = TPS_REG_INT_PLUG_EVENT,
> .irq_trace = trace_tps6598x_irq,
> + .supports_spss = false,
> };
>
> static const struct tps6598x_hw apple_cd321x_data = {
> @@ -795,6 +838,7 @@ static const struct tps6598x_hw apple_cd321x_data = {
> .irq_data_status_update = APPLE_TPS_REG_INT_DATA_STATUS_UPDATE,
> .irq_plug_event = APPLE_TPS_REG_INT_PLUG_EVENT,
> .irq_trace = trace_cd321x_irq,
> + .supports_spss = true,
> };
thanks,
--
heikki
Hi,
Thanks a lot for quick review!
On Tue, Sep 21, 2021, at 15:10, Heikki Krogerus wrote:
> Hi,
>
> On Sat, Sep 18, 2021 at 02:09:27PM +0200, Sven Peter wrote:
>> Apple M1 machines come with a variant of the TI TPS6598x and will need
>> some changes to the current logic. Let's prepare for that by setting up
>> the infrastructure required to support different variants of this chip
>> identified by the DT compatible.
>
> I think in this case it would make sense to just squash this into the
> next patch.
Sure, and taking into account your review for the later patches I will
probably squash most of them into a single commit now.
>
>> Signed-off-by: Sven Peter <[email protected]>
>> ---
>> drivers/usb/typec/tipd/core.c | 19 ++++++++++++++++++-
>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index 21b3ae25c76d..656020e7f533 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -9,6 +9,7 @@
>> #include <linux/i2c.h>
>> #include <linux/acpi.h>
>> #include <linux/module.h>
>> +#include <linux/of_device.h>
>> #include <linux/power_supply.h>
>> #include <linux/regmap.h>
>> #include <linux/interrupt.h>
>> @@ -76,6 +77,10 @@ static const char *const modes[] = {
>> /* Unrecognized commands will be replaced with "!CMD" */
>> #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
>>
>> +struct tps6598x_hw {
>> +};
>
> Black line here.
>
>> +static const struct tps6598x_hw ti_tps6598x_data;
>> +
>> struct tps6598x {
>> struct device *dev;
>> struct regmap *regmap;
>> @@ -91,6 +96,8 @@ struct tps6598x {
>> struct power_supply *psy;
>> struct power_supply_desc psy_desc;
>> enum power_supply_usb_type usb_type;
>> +
>> + const struct tps6598x_hw *hw;
>> };
>>
>> static enum power_supply_property tps6598x_psy_props[] = {
>> @@ -590,6 +597,13 @@ static int tps6598x_probe(struct i2c_client *client)
>> if (!tps)
>> return -ENOMEM;
>>
>> + if (client->dev.of_node)
>> + tps->hw = of_device_get_match_data(&client->dev);
>> + else
>> + tps->hw = &ti_tps6598x_data;
>> + if (!tps->hw)
>> + return -EINVAL;
>
> tps->hw = of_device_get_match_data(&client->dev);
> if (!tps->hw)
> tps->hw = &ti_tps6598x_data;
>
Ah yes, that's indeed much nicer!
>> mutex_init(&tps->lock);
>> tps->dev = &client->dev;
>>
>> @@ -729,8 +743,11 @@ static int tps6598x_remove(struct i2c_client *client)
>> return 0;
>> }
>>
>> +static const struct tps6598x_hw ti_tps6598x_data = {
>> +};
>
> You could also move that above tps6598x_probe() and get rid of the
> forward declaration.
Ack.
Best,
Sven
On Tue, Sep 21, 2021, at 15:21, Heikki Krogerus wrote:
> On Sat, Sep 18, 2021 at 02:09:28PM +0200, Sven Peter wrote:
>> TI TPS6598x chips come with two separate i2c buses which each have an
>> independent interrupt controller. When only a single controller is
>> connected both can just be used.
>> On Apple M1 machines the secondary bus is however connected to the system
>> management controller and we must not modify its configuration. Prepare
>> for that by allowing to chose which interrupt controller(s) are used.
>
> This is good, but...
>
>> Signed-off-by: Sven Peter <[email protected]>
>> ---
>> drivers/usb/typec/tipd/core.c | 30 +++++++++++++++++++++---------
>> 1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index 656020e7f533..c2c399722c37 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -78,6 +78,8 @@ static const char *const modes[] = {
>> #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
>>
>> struct tps6598x_hw {
>> + bool use_int1;
>> + bool use_int2;
>> };
>
> Wouldn't it be better to read that information from a device
> property/properties?
>
> Driver data is OK, but device property would be better. We don't have
> separate compatible for this on every board that uses it and we have
> also ACPI platforms.
Good point. Taking into account your review for the later patches I
think I can drop this completely though: On the Apple hardware the
second controller is always connected to the SMC so far. If it's
required later on we can always add the additional property with
a default value then.
Best,
Sven
On Tue, Sep 21, 2021, at 15:40, Heikki Krogerus wrote:
> On Sat, Sep 18, 2021 at 02:09:31PM +0200, Sven Peter wrote:
>> Right now the code relies on the bootloader to set up the interrupt mask
>> correctly. This usually works but let's make sure to do it explicitly to
>> guarantee it will always work.
>>
>> Signed-off-by: Sven Peter <[email protected]>
>> ---
>> drivers/usb/typec/tipd/core.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index d191e7435018..2058e8cca631 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -728,6 +728,24 @@ static int tps6598x_probe(struct i2c_client *client)
>> dev_err(&client->dev, "failed to register partner\n");
>> }
>>
>> + if (tps->hw->use_int1) {
>> + ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
>> + tps->hw->irq_power_status_update |
>> + tps->hw->irq_data_status_update |
>> + tps->hw->irq_plug_event);
>> + if (ret)
>> + goto err_role_put;
>> + }
>> +
>> + if (tps->hw->use_int2) {
>> + ret = tps6598x_write64(tps, TPS_REG_INT_MASK2,
>> + tps->hw->irq_power_status_update |
>> + tps->hw->irq_data_status_update |
>> + tps->hw->irq_plug_event);
>> + if (ret)
>> + goto err_role_put;
>> + }
>
> You should first only set the mask on your board. We can then see if
> it's something that should be done on other boards as well later.
>
Make sense, I'll only call this when the cd321x variant is probed then.
Sven
On Tue, Sep 21, 2021, at 15:46, Heikki Krogerus wrote:
> On Sat, Sep 18, 2021 at 02:09:33PM +0200, Sven Peter wrote:
>> The Apple CD321x comes up in a low-power state after boot. Usually, the
>> bootloader will already power it up to S0 but let's do it here as well
>> in case that didn't happen.
>>
>> Suggested-by: Stan Skowronek <[email protected]>
>> Signed-off-by: Sven Peter <[email protected]>
>> ---
>> drivers/usb/typec/tipd/core.c | 44 +++++++++++++++++++++++++++++++
>> drivers/usb/typec/tipd/tps6598x.h | 6 +++++
>> 2 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index e96b17fe6af6..26807c050662 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -30,6 +30,7 @@
>> #define TPS_REG_INT_MASK2 0x17
>> #define TPS_REG_INT_CLEAR1 0x18
>> #define TPS_REG_INT_CLEAR2 0x19
>> +#define TPS_REG_SYSTEM_POWER_STATE 0x20
>> #define TPS_REG_STATUS 0x1a
>> #define TPS_REG_SYSTEM_CONF 0x28
>> #define TPS_REG_CTRL_CONF 0x29
>> @@ -84,6 +85,8 @@ struct tps6598x_hw {
>> unsigned int irq_data_status_update;
>> unsigned int irq_plug_event;
>> void (*irq_trace)(u64 event1, u64 event2);
>> +
>> + bool supports_spss;
>> };
[...]
>> static int devm_tps6598_psy_register(struct tps6598x *tps)
>> {
>> struct power_supply_config psy_cfg = {};
>> @@ -648,6 +685,11 @@ static int tps6598x_probe(struct i2c_client *client)
>> if (ret)
>> return ret;
>>
>> + /* Switch Apple chips to the correct system power state */
>> + ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
>> + if (ret)
>> + return ret;
>
> If you call this from the same quirk where you set the mask for your
> board, you don't need that supports_spss flag at all, right?
Yup, that one will also disappear then.
Thanks,
Sven
Hi,
On Tue, Sep 21, 2021, at 15:34, Heikki Krogerus wrote:
> On Sat, Sep 18, 2021 at 02:09:30PM +0200, Sven Peter wrote:
>> The Apple variant of the TI TPS6598x chip uses different interrupt
>> numbers. Prepare for that by allowing those to be configured depending
>> on the compatible.
>
> OK, so I think this justifies having a completely separate irq
> handler for your board.
OK, If you're fine with a bit of code duplication for the irq handler this will
all be quite a bit simpler then.
>
>> Signed-off-by: Sven Peter <[email protected]>
>> ---
>> drivers/usb/typec/tipd/core.c | 16 ++++++++++++----
>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index 4a6d66250fef..d191e7435018 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -80,6 +80,10 @@ static const char *const modes[] = {
>> struct tps6598x_hw {
>> bool use_int1;
>> bool use_int2;
>> + unsigned int irq_power_status_update;
>> + unsigned int irq_data_status_update;
>> + unsigned int irq_plug_event;
>> + void (*irq_trace)(u64 event1, u64 event2);
>> };
>
> Then I believe you don't need any of that.
Yup, I think I'll really only need something like
struct tps6598x_hw {
int type;
irq_handler_t irq_handler;
};
in that case!
Thanks,
Sven
On Sat, 18 Sep 2021 14:09:26 +0200, Sven Peter wrote:
> A variant of the TI TPS 6598x Type-C Port Switch and Power Delivery
> controller known as Apple CD321x is present on boards with Apple SoCs
> such as the M1. Add its compatible to the device tree binding.
>
> Signed-off-by: Sven Peter <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
Acked-by: Rob Herring <[email protected]>