RE: [RFC PATCH v3 2/2] usb: typec: Type-C Port Controller Interface driver (tcpci)

From: Jun Li
Date: Fri Sep 30 2016 - 20:46:32 EST

Next message: Jaegeuk Kim: "Re: [f2fs-dev] [PATCH 1/2 v2] f2fs: use crc and cp version to determine roll-forward recovery"
Previous message: Theodore Ts'o: "Re: Question on ext4 directory hashes in combination with file name encryption"
In reply to: Guenter Roeck: "Re: [RFC PATCH v3 2/2] usb: typec: Type-C Port Controller Interface driver (tcpci)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,
> -----Original Message-----
> From: Guenter Roeck [mailto:groeck@xxxxxxxxxx]
> Sent: Saturday, October 01, 2016 2:45 AM
> To: Jun Li <jun.li@xxxxxxx>
> Cc: Guenter Roeck <groeck@xxxxxxxxxxxx>; Felipe Balbi
> <felipe.balbi@xxxxxxxxxxxxxxx>; Chandra Sekhar Anagani
> <chandra.sekhar.anagani@xxxxxxxxx>; Bruce Ashfield
> <bruce.ashfield@xxxxxxxxxxxxx>; Bin Gao <bin.gao@xxxxxxxxx>; Pranav Tipnis
> <pranav.tipnis@xxxxxxxxx>; Heikki Krogerus
> <heikki.krogerus@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-
> usb@xxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH v3 2/2] usb: typec: Type-C Port Controller
> Interface driver (tcpci)
>
> On Thu, Sep 29, 2016 at 11:24 PM, Jun Li <jun.li@xxxxxxx> wrote:
> > Hi Guenter,
> >
> >> -----Original Message-----
> >> From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-
> >> owner@xxxxxxxxxxxxxxx] On Behalf Of Guenter Roeck
> >> Sent: Wednesday, August 24, 2016 5:11 AM
> >> To: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
> >> Cc: Chandra Sekhar Anagani <chandra.sekhar.anagani@xxxxxxxxx>; Bruce
> >> Ashfield <bruce.ashfield@xxxxxxxxxxxxx>; Bin Gao <bin.gao@xxxxxxxxx>;
> >> Pranav Tipnis <pranav.tipnis@xxxxxxxxx>; Heikki Krogerus
> >> <heikki.krogerus@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> >> linux- usb@xxxxxxxxxxxxxxx; Guenter Roeck <groeck@xxxxxxxxxxxx>
> >> Subject: [RFC PATCH v3 2/2] usb: typec: Type-C Port Controller
> >> Interface driver (tcpci)
> >>
> >> The port controller interface driver interconnects the Type-C Port
> >> Manager with a Type-C Port Controller Interface (TCPCI) compliant port
> controller.
> >>
> >> Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxxxx>
> >> ---
> >> v3:
> >> - No change
> >>
> >> v2:
> >> - Adjust to modified callbacks into tcpm code
> >>
> >> drivers/usb/typec/Kconfig | 9 +
> >> drivers/usb/typec/Makefile | 1 +
> >> drivers/usb/typec/tcpci.c | 487
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >> drivers/usb/typec/tcpci.h | 133 +++++++++++++
> >> 4 files changed, 630 insertions(+)
> >> create mode 100644 drivers/usb/typec/tcpci.c create mode 100644
> >> drivers/usb/typec/tcpci.h
> >>
> >> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> >> index
> >> 113bb1b3589c..a92c9d1a3e00 100644
> >> --- a/drivers/usb/typec/Kconfig
> >> +++ b/drivers/usb/typec/Kconfig
> >> @@ -25,4 +25,13 @@ config TYPEC_TCPM
> >> The Type-C Port Controller Manager provides a USB PD and USB
> Type-C
> >> state machine for use with Type-C Port Controllers.
> >>
> >> +if TYPEC_TCPM
> >> +
> >> +config TYPEC_TCPCI
> >> + tristate "Type-C Port Controller Interface driver"
> >> + help
> >> + Type-C Port Controller driver for TCPCI-compliant controller.
> >> +
> >> +endif
> >> +
> >> endmenu
> >> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> >> index bbe45721cf52..7dbaf8c3911d 100644
> >> --- a/drivers/usb/typec/Makefile
> >> +++ b/drivers/usb/typec/Makefile
> >> @@ -1,3 +1,4 @@
> >> obj-$(CONFIG_TYPEC) += typec.o
> >> obj-$(CONFIG_TYPEC_WCOVE) += typec_wcove.o
> >> obj-$(CONFIG_TYPEC_TCPM) += tcpm.o
> >> +obj-$(CONFIG_TYPEC_TCPCI) += tcpci.o
> >> diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c
> >> new file mode 100644 index 000000000000..af338218a1f3
> >> --- /dev/null
> >> +++ b/drivers/usb/typec/tcpci.c
> >> @@ -0,0 +1,487 @@
> >> +/*
> >> + * Copyright 2015-2016 Google, Inc
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> +modify
> >> + * it under the terms of the GNU General Public License as published
> >> +by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * USB Type-C Port Controller Interface.
> >> + */
> >> +
> >> +#include <linux/delay.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/usb/pd.h>
> >> +#include <linux/usb/typec.h>
> >> +
> >> +#include "tcpci.h"
> >> +#include "tcpm.h"
> >> +
> >> +#define PD_RETRY_COUNT 3
> >> +
> >> +struct tcpci {
> >> + struct device *dev;
> >> + struct i2c_client *client;
> >> +
> >> + struct tcpm_port *port;
> >> +
> >> + struct regmap *regmap;
> >> +
> >> + bool controls_vbus;
> >> +
> >> + struct tcpc_dev tcpc;
> >> +};
> >> +
> >> +static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc) {
> >> + return container_of(tcpc, struct tcpci, tcpc); }
> >> +
> >> +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> >> + unsigned int *val) {
> >> + return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16)); }
> >> +
> >> +static int tcpci_write16(struct tcpci *tcpci, unsigned int reg, u16
> >> +val) {
> >> + return regmap_raw_write(tcpci->regmap, reg, &val, sizeof(u16));
> >> +}
> >> +
> >> +static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status
> >> +cc) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + unsigned int reg;
> >> + int ret;
> >> +
> >> + switch (cc) {
> >> + case TYPEC_CC_RA:
> >> + reg = (TCPC_ROLE_CTRL_CC_RA << TCPC_ROLE_CTRL_CC1_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_CC_RA <<
> TCPC_ROLE_CTRL_CC2_SHIFT);
> >> + break;
> >> + case TYPEC_CC_RD:
> >> + reg = (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_CC_RD <<
> TCPC_ROLE_CTRL_CC2_SHIFT);
> >> + break;
> >> + case TYPEC_CC_RP_DEF:
> >> + reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_RP_VAL_DEF <<
> >> + TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> >> + break;
> >> + case TYPEC_CC_RP_1_5:
> >> + reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
> >> + TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> >> + break;
> >> + case TYPEC_CC_RP_3_0:
> >> + reg = (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_RP_VAL_1_5 <<
> >> + TCPC_ROLE_CTRL_RP_VAL_SHIFT);
> >> + break;
> >> + case TYPEC_CC_OPEN:
> >> + default:
> >> + reg = (TCPC_ROLE_CTRL_CC_OPEN << TCPC_ROLE_CTRL_CC1_SHIFT)
> |
> >> + (TCPC_ROLE_CTRL_CC_OPEN <<
> TCPC_ROLE_CTRL_CC2_SHIFT);
> >> + break;
> >> + }
> >> +
> >> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool
> >> +sink) {
> >> + switch (cc) {
> >> + case 0x1:
> >> + return sink ? TYPEC_CC_RP_DEF : TYPEC_CC_RA;
> >> + case 0x2:
> >> + return sink ? TYPEC_CC_RP_1_5 : TYPEC_CC_RD;
> >> + case 0x3:
> >> + if (sink)
> >> + return TYPEC_CC_RP_3_0;
> >> + case 0x0:
> >> + default:
> >> + return TYPEC_CC_OPEN;
> >> + }
> >> +}
> >> +
> >> +static int tcpci_get_cc(struct tcpc_dev *tcpc,
> >> + enum typec_cc_status *cc1, enum typec_cc_status
> *cc2) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + unsigned int reg;
> >> + int ret;
> >> +
> >> + ret = regmap_read(tcpci->regmap, TCPC_CC_STATUS, &reg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + *cc1 = tcpci_to_typec_cc((reg >> TCPC_CC_STATUS_CC1_SHIFT) &
> >> + TCPC_CC_STATUS_CC1_MASK,
> >> + reg & TCPC_CC_STATUS_TERM);
> >> + *cc2 = tcpci_to_typec_cc((reg >> TCPC_CC_STATUS_CC2_SHIFT) &
> >> + TCPC_CC_STATUS_CC2_MASK,
> >> + reg & TCPC_CC_STATUS_TERM);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> >> + enum typec_cc_polarity polarity) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + int ret;
> >> +
> >> + ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL,
> >> + (polarity == TYPEC_POLARITY_CC2) ?
> >> + TCPC_TCPC_CTRL_ORIENTATION : 0);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + int ret;
> >> +
> >> + ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
> >> + enable ? TCPC_POWER_CTRL_VCONN_ENABLE : 0);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int tcpci_set_pd_header(struct tcpc_dev *tcpc, enum
> >> +typec_role
> >> role,
> >> + enum typec_data_role data) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + unsigned int reg;
> >> + int ret;
> >> +
> >> + reg = PD_REV20 << TCPC_MSG_HDR_INFO_REV_SHIFT;
> >> + if (role == TYPEC_SOURCE)
> >> + reg |= TCPC_MSG_HDR_INFO_PWR_ROLE;
> >> + if (data == TYPEC_HOST)
> >> + reg |= TCPC_MSG_HDR_INFO_DATA_ROLE;
> >> + ret = regmap_write(tcpci->regmap, TCPC_MSG_HDR_INFO, reg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + unsigned int reg = 0;
> >> + int ret;
> >> +
> >> + if (enable)
> >> + reg = TCPC_RX_DETECT_SOP | TCPC_RX_DETECT_HARD_RESET;
> >> + ret = regmap_write(tcpci->regmap, TCPC_RX_DETECT, reg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int tcpci_get_vbus(struct tcpc_dev *tcpc) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + unsigned int reg;
> >> + int ret;
> >> +
> >> + ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS, &reg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return !!(reg & TCPC_POWER_STATUS_VBUS_PRES); }
> >> +
> >> +static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool source, bool
> >> +sink) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + int ret;
> >> +
> >> + if (source) {
> >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >> + TCPC_CMD_SRC_VBUS_DEFAULT);
> >> + if (ret < 0)
> >> + return ret;
> >> + } else {
> >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >> + TCPC_CMD_DISABLE_SRC_VBUS);
> >> + if (ret < 0)
> >> + return ret;
> >> + }
> >> +
> >> + if (sink) {
> >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >> + TCPC_CMD_SINK_VBUS);
> >> + if (ret < 0)
> >> + return ret;
> >> + } else {
> >> + ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >> + TCPC_CMD_DISABLE_SINK_VBUS);
> >> + if (ret < 0)
> >> + return ret;
> >> + }
> >> +
> >
> > Per tcpc spec, disable should be firstly done before enable, so should
> > it be done like this?
> > if(!source) {};
> > if(!sink) {};
> > if(source) {};
> > if(sink) {};
> >
> Makes sense. Changed accordingly.
>
> >> + return 0;
> >> +}
> >> +
> >> +static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
> >> + enum tcpm_transmit_type type,
> >> + const struct pd_message *msg) {
> >> + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >> + unsigned int reg, cnt, header;
> >> + int ret;
> >> +
> >> + cnt = msg ? pd_header_cnt(msg->header) * 4 : 0;
> >> + ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt);
> >
> > Here miss the 2 bytes' header, so cnt should be cnt += 2;
> >
> > Tcpc 4.4.13
> > TRANSMIT_BYTE_COUNT:
> > The number of bytes the TCPM will write This is the number of bytes in
> > the TX_BUFFER_DATA_OBJECTS plus two (for the TX_BUF_HEADER)
> >
> Yes, you are correct. Fixed.
>
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + header = msg ? msg->header : 0;
> >> + ret = tcpci_write16(tcpci, TCPC_TX_HDR, header);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + if (cnt > 0) {
> >> + ret = regmap_raw_write(tcpci->regmap, TCPC_TX_DATA,
> >> + &msg->payload, cnt);
> >> + if (ret < 0)
> >> + return ret;
> >> + }
> >> +
> >> + reg = (PD_RETRY_COUNT << TCPC_TRANSMIT_RETRY_SHIFT) |
> >> + (type << TCPC_TRANSMIT_TYPE_SHIFT);
> >> + ret = regmap_write(tcpci->regmap, TCPC_TRANSMIT, reg);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 0;
> >> +}
> >> +
> >
> > ...
> >
> >> +
> >> +static int tcpci_probe(struct i2c_client *client,
> >> + const struct i2c_device_id *i2c_id) {
> >> + struct tcpci *tcpci;
> >> + int err;
> >> +
> >> + tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL);
> >> + if (!tcpci)
> >> + return -ENOMEM;
> >> +
> >> + tcpci->client = client;
> >> + tcpci->dev = &client->dev;
> >> + i2c_set_clientdata(client, tcpci);
> >> + tcpci->regmap = devm_regmap_init_i2c(client,
> &tcpci_regmap_config);
> >> + if (IS_ERR(tcpci->regmap))
> >> + return PTR_ERR(tcpci->regmap);
> >> +
> >> + tcpci->tcpc.init = tcpci_init;
> >> + tcpci->tcpc.get_vbus = tcpci_get_vbus;
> >> + tcpci->tcpc.set_vbus = tcpci_set_vbus;
> >> + tcpci->tcpc.set_cc = tcpci_set_cc;
> >> + tcpci->tcpc.get_cc = tcpci_get_cc;
> >> + tcpci->tcpc.set_polarity = tcpci_set_polarity;
> >> + tcpci->tcpc.set_vconn = tcpci_set_vconn;
> >> +
> >> + tcpci->tcpc.set_pd_rx = tcpci_set_pd_rx;
> >> + tcpci->tcpc.set_pd_header = tcpci_set_pd_header;
> >> + tcpci->tcpc.pd_transmit = tcpci_pd_transmit;
> >> +
> >> + err = tcpci_parse_config(tcpci);
> >> + if (err < 0)
> >> + return err;
> >> +
> >
> > As the alert mask reg is defaultly to be unmasked after power on, It's
> > better clear and mask all before request irq.
> >
> Agreed and done.
>
> >> + err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
> >> + tcpci_irq,
> >> + IRQF_TRIGGER_FALLING,
> >
> > IRQF_ONESHOT is required, and do we need the irq trigger to be
> > IRQF_TRIGGER_LOW?
> >
>
> Good question; I don't really know. IRQF_TRIGGER_FALLING definitely seems
> to be wrong (and I don't recall where I got it from). Not sure about
> IRQF_TRIGGER_LOW. Can you test on real hardware, by any chance ?
>

I tested on real hardware, IRQF_TRIGGER_LOW can work fine, so I think
the right flag should be IRQF_TRIGGER_LOW | IRQF_ONESHOT.

Li Jun

> Thanks,
> Guenter
>
>
> > Li Jun

Next message: Jaegeuk Kim: "Re: [f2fs-dev] [PATCH 1/2 v2] f2fs: use crc and cp version to determine roll-forward recovery"
Previous message: Theodore Ts'o: "Re: Question on ext4 directory hashes in combination with file name encryption"
In reply to: Guenter Roeck: "Re: [RFC PATCH v3 2/2] usb: typec: Type-C Port Controller Interface driver (tcpci)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]