| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABXOdTdmFdaOEt7LyLOAK8Uuh0-hQmLULgJWSnvoiTLuFh8GFw@mail.gmail.com>
Date: Fri, 30 Sep 2016 11:45:28 -0700
From: Guenter Roeck <groeck@...gle.com>
To: Jun Li <jun.li@....com>
Cc: Guenter Roeck <groeck@...omium.org>,
Felipe Balbi <felipe.balbi@...ux.intel.com>,
Chandra Sekhar Anagani <chandra.sekhar.anagani@...el.com>,
Bruce Ashfield <bruce.ashfield@...driver.com>,
Bin Gao <bin.gao@...el.com>,
Pranav Tipnis <pranav.tipnis@...el.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
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@....com> wrote:
> Hi Guenter,
>
>> -----Original Message-----
>> From: linux-usb-owner@...r.kernel.org [mailto:linux-usb-
>> owner@...r.kernel.org] On Behalf Of Guenter Roeck
>> Sent: Wednesday, August 24, 2016 5:11 AM
>> To: Felipe Balbi <felipe.balbi@...ux.intel.com>
>> Cc: Chandra Sekhar Anagani <chandra.sekhar.anagani@...el.com>; Bruce
>> Ashfield <bruce.ashfield@...driver.com>; Bin Gao <bin.gao@...el.com>;
>> Pranav Tipnis <pranav.tipnis@...el.com>; Heikki Krogerus
>> <heikki.krogerus@...ux.intel.com>; linux-kernel@...r.kernel.org; linux-
>> usb@...r.kernel.org; Guenter Roeck <groeck@...omium.org>
>> 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@...omium.org>
>> ---
>> 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, ®);
>> + 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, ®);
>> + 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
?
Thanks,
Guenter
> Li Jun
Powered by blists - more mailing lists