From: Felipe Balbi <felipe.balbi@linux.intel.com>
To: Bin Gao <bin.gao@linux.intel.com>,
Heikki Krogerus <heikki.krogerus@linux.intel.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Bin Gao <bin.gao@intel.com>,
Chandra Sekhar Anagani <chandra.sekhar.anagani@intel.com>,
Pranav Tipnis <pranav.tipnis@intel.com>
Subject: Re: [PATCH v2 2/2] usb: typec: add PD sink port support for Intel Whiskey Cove PMIC Typc-C PHY driver
Date: Wed, 27 Jul 2016 11:21:49 +0300 [thread overview]
Message-ID: <87twfbfq3m.fsf@linux.intel.com> (raw)
In-Reply-To: <20160726184153.GF211765@worksta>
[-- Attachment #1: Type: text/plain, Size: 13874 bytes --]
(before anything: please have a look at the patches I wrote on top of
this. They might help you somewhat
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=bxt-typec-pd-fixes)
Bin Gao <bin.gao@linux.intel.com> writes:
> From: Chandra Sekhar Anagani <chandra.sekhar.anagani@intel.com>
>
> This adds PD sink port support for the USB Type-C PHY on Intel WhiskeyCove
> PMIC which is available on some of the Intel Broxton SoC based platforms.
>
> This patch depends on these two patches:
> https://lkml.org/lkml/2016/6/29/349
> https://lkml.org/lkml/2016/6/29/350
>
> Signed-off-by: Chandra Sekhar Anagani <chandra.sekhar.anagani@intel.com>
> Signed-off-by: Pranav Tipnis <pranav.tipnis@intel.com>
> Signed-off-by: Bin Gao <bin.gao@intel.com>
> Changes in v2:
> - Added PD support for cold boot case
like before, move it after tearline. Also, what does this mean?
> ---
> drivers/usb/typec/typec_wcove.c | 309 ++++++++++++++++++++++++++++++++++++----
> 1 file changed, 285 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
> index c7c2d28..000d6ae 100644
> --- a/drivers/usb/typec/typec_wcove.c
> +++ b/drivers/usb/typec/typec_wcove.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2016 Intel Corporation
> * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + * Author: Chandra Sekhar Anagani <chandra.sekhar.anagani@intel.com>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -10,9 +11,11 @@
> */
>
> #include <linux/acpi.h>
> +#include <linux/delay.h>
> #include <linux/module.h>
> #include <linux/interrupt.h>
> #include <linux/usb/typec.h>
> +#include <linux/usb/pd_sink.h>
> #include <linux/platform_device.h>
> #include <linux/mfd/intel_soc_pmic.h>
>
> @@ -25,6 +28,7 @@
> #define USBC_CONTROL3 0x7003
> #define USBC_CC1_CTRL 0x7004
> #define USBC_CC2_CTRL 0x7005
> +#define USBC_CC_SEL 0x7006
> #define USBC_STATUS1 0x7007
> #define USBC_STATUS2 0x7008
> #define USBC_STATUS3 0x7009
> @@ -32,7 +36,16 @@
> #define USBC_IRQ2 0x7016
> #define USBC_IRQMASK1 0x7017
> #define USBC_IRQMASK2 0x7018
> -
> +#define USBC_PD_CFG1 0x7019
> +#define USBC_PD_CFG2 0x701a
> +#define USBC_PD_CFG3 0x701b
> +#define USBC_PD_STATUS 0x701c
> +#define USBC_RX_STATUS 0x701d
> +#define USBC_RX_INFO 0x701e
> +#define USBC_TX_CMD 0x701f
> +#define USBC_TX_INFO 0x7020
> +#define USBC_RX_DATA_START 0x7028
> +#define USBC_TX_DATA_START 0x7047
> /* Register bits */
>
> #define USBC_CONTROL1_MODE_DRP(r) ((r & ~0x7) | 4)
> @@ -44,7 +57,9 @@
> #define USBC_CONTROL3_PD_DIS BIT(1)
>
> #define USBC_CC_CTRL_VCONN_EN BIT(1)
> +#define USBC_CC_CTRL_TX_EN BIT(2)
>
> +#define USBC_CC_SEL_CCSEL (BIT(0) | BIT(1))
> #define USBC_STATUS1_DET_ONGOING BIT(6)
> #define USBC_STATUS1_RSLT(r) (r & 0xf)
> #define USBC_RSLT_NOTHING 0
> @@ -79,11 +94,44 @@
> USBC_IRQ2_RX_HR | USBC_IRQ2_RX_CR | \
> USBC_IRQ2_TX_SUCCESS | USBC_IRQ2_TX_FAIL)
>
> +#define USBC_PD_CFG1_ID_FILL BIT(7)
> +
> +#define USBC_PD_CFG2_SOP_RX BIT(0)
> +
> +#define USBC_PD_CFG3_SR_SOP2 (BIT(7) | BIT(6))
> +#define USBC_PD_CFG3_SR_SOP1 (BIT(5) | BIT(4))
> +#define USBC_PD_CFG3_SR_SOP0 (BIT(3) | BIT(2))
> +#define USBC_PD_CFG3_DATAROLE BIT(1)
> +#define USBC_PD_CFG3_PWRROLE BIT(0)
> +
> +#define USBC_TX_CMD_TXBUF_RDY BIT(0)
> +#define USBC_TX_CMD_TX_START BIT(1)
> +#define USBC_TX_CMD_TXBUF_CMD(r) ((r >> 5) & 0x7)
> +
> +#define USBC_TX_INFO_TX_SOP (BIT(0) | BIT(1) | BIT(2))
> +#define USBC_TX_INFO_TX_RETRIES (BIT(3) | BIT(4) | BIT(5))
> +
> +#define USBC_RX_STATUS_RX_DATA BIT(7)
> +#define USBC_RX_STATUS_RX_OVERRUN BIT(6)
> +#define USBC_RX_STATUS_RX_CLEAR BIT(0)
> +
> +#define USBC_PD_STATUS_RX_RSLT(r) ((r >> 3) & 0x7)
> +#define USBC_PD_STATUS_TX_RSLT(r) (r & 0x7)
> +
> +#define USBC_RX_INFO_RXBYTES(r) ((r >> 3) & 0x1f)
> +#define USBC_RX_INFO_RX_SOP(r) (r & 0x7)
> +
> +#define USBC_PD_RX_BUF_LEN 30
> +#define USBC_PD_TX_BUF_LEN 30
> +#define USBC_PD_SEND_HR (3 << 5)
> +
> struct wcove_typec {
> + int pd_port_num;
> struct mutex lock; /* device lock */
> struct device *dev;
> struct regmap *regmap;
> struct typec_port *port;
> + struct pd_sink_port pd_port;
> struct typec_capability cap;
> struct typec_connection con;
> struct typec_partner partner;
> @@ -106,6 +154,50 @@ enum wcove_typec_role {
> WCOVE_ROLE_DEVICE,
> };
>
> +static struct sink_ps profiles[] = {
> +
> + {
> + .ps_type = PS_TYPE_FIXED,
> + .ps_fixed = {
> + .voltage_fixed = 100, /* 5V/50mV = 100 */
> + .current_default = 90, /* 900mA/10mA = 90 */
> + .current_max = 90, /* 900mA/10mA = 90 */
> + },
> +
> + },
> +
> + {
> + .ps_type = PS_TYPE_FIXED,
> + .ps_fixed = {
> + .voltage_fixed = 100,
> + .current_default = 300,
> + .current_max = 300,
> + },
> + },
> +
> + {
> + .ps_type = PS_TYPE_FIXED,
> + .ps_fixed = {
> + .voltage_fixed = 240,
> + .current_default = 300,
> + .current_max = 300,
> + },
> + },
> +
> +};
> +
this is board-specific knowledge. We don't want these in the kernel,
right?
> +static struct pd_sink_profile profile = {
> + .hw_goodcrc_tx = true,
> + .hw_goodcrc_rx = true,
> + .gotomin = true,
> + .usb_comm = true,
> + .spec = USB_SPEC_3X,
> + .pd_rev = PD_REVISION_2,
> + .ps = profiles,
> + .nr_ps = ARRAY_SIZE(profiles),
> + .active_ps = 2, /* voltage = 5V, current = 3A */
> +};
this is also board-specific.
> @@ -140,7 +232,102 @@ static void wcove_typec_device_mode(struct wcove_typec *wcove)
> typec_connect(wcove->port, &wcove->con);
> }
>
> -static irqreturn_t wcove_typec_irq(int irq, void *data)
> +static int wcove_typec_pd_recv_pkt_handler(struct wcove_typec *wcove)
> +{
> + unsigned int rx_status;
> + unsigned int rx_info;
> + unsigned int temp;
> + int len;
> + int ret, i;
> + char buf[USBC_PD_RX_BUF_LEN];
> +
> + ret = regmap_read(wcove->regmap, USBC_RX_STATUS, &rx_status);
> + if (ret)
> + goto err;
> +
> + while (rx_status & USBC_RX_STATUS_RX_DATA) {
> + ret = regmap_read(wcove->regmap, USBC_RX_INFO, &rx_info);
> + if (ret)
> + goto err;
> +
> + len = (USBC_RX_INFO_RXBYTES(rx_info));
> +
> + for (i = 0; i < len; i++) {
> + ret = regmap_read(wcove->regmap, USBC_RX_DATA_START + i,
> + &temp);
> + buf[i] = (char)temp;
> + if (ret)
> + goto err;
> + }
regmap_block_read()
> +
> + ret = pd_sink_handle_msg(wcove->pd_port_num, buf, len);
> + if (ret) {
> + dev_err(wcove->dev, "pd_sink_handle_msg() failed: %d\n",
> + ret);
> + /* Clear RX status */
> + regmap_update_bits(wcove->regmap, USBC_RX_STATUS,
> + USBC_RX_STATUS_RX_CLEAR,
> + USBC_RX_STATUS_RX_CLEAR);
> + goto err;
> + }
> +
> + /* Clear RX status */
> + regmap_update_bits(wcove->regmap, USBC_RX_STATUS,
> + USBC_RX_STATUS_RX_CLEAR, USBC_RX_STATUS_RX_CLEAR);
> +
> + ret = regmap_read(wcove->regmap, USBC_RX_STATUS, &rx_status);
> + if (ret)
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + return ret;
> +}
> +
> +static int wcove_typec_pd_tx_pkt_handler(int port_num, void *data,
> + void *buf, int len, enum sop_type pkt_type)
> +{
> + unsigned int tx_cmd;
> + unsigned int val;
> + int ret, i;
> + char *buf1 = buf;
> + struct wcove_typec *wcove = data;
> +
> + ret = regmap_read(wcove->regmap, USBC_TX_CMD, &tx_cmd);
> + if (ret)
> + goto err;
> +
> + if (!(tx_cmd & USBC_TX_CMD_TXBUF_RDY))
> + return -EBUSY;
> +
> + for (i = 0; i < len; i++)
> + ret = regmap_write(wcove->regmap, USBC_TX_DATA_START + i,
> + buf1[i]);
> + if (ret)
> + goto err;
regmap block write()
> + regmap_read(wcove->regmap, USBC_TX_INFO, &val);
> + ret = regmap_write(wcove->regmap, USBC_TX_INFO, 0x71);
no magic constants
> + if (ret)
> + goto err;
> +
> + ret = regmap_write(wcove->regmap, USBC_TX_CMD,
> + USBC_TX_CMD_TX_START | (1 << 5));
no magic constants
> + if (ret)
> + goto err;
> +
> + ret = regmap_read(wcove->regmap, USBC_TX_CMD, &tx_cmd);
> + if (ret)
> + goto err;
> +
> +err:
> + kfree(buf1);
> + return ret;
> +}
> +
> +static irqreturn_t wcove_typec_irq(int irq, void *data)
> {
> struct wcove_typec *wcove = data;
> unsigned int cc1_ctrl;
> @@ -149,10 +336,10 @@ static irqreturn_t wcove_typec_irq(int irq, void *data)
> unsigned int cc_irq2;
> unsigned int status1;
> unsigned int status2;
> + unsigned int rx_status;
> int ret;
>
> mutex_lock(&wcove->lock);
> -
> ret = regmap_read(wcove->regmap, USBC_IRQ1, &cc_irq1);
> if (ret)
> goto err;
> @@ -177,6 +364,10 @@ static irqreturn_t wcove_typec_irq(int irq, void *data)
> if (ret)
> goto err;
>
> + ret = regmap_read(wcove->regmap, USBC_RX_STATUS, &rx_status);
> + if (ret)
> + goto err;
you don't need to read rx_status in IRQ handler. The fact that you do
shows a bug in your driver
> +
> if (cc_irq1) {
> if (cc_irq1 & USBC_IRQ1_OVERTEMP)
> dev_err(wcove->dev, "VCONN Switch Over Temperature!\n");
> @@ -185,17 +376,9 @@ static irqreturn_t wcove_typec_irq(int irq, void *data)
> regmap_write(wcove->regmap, USBC_IRQ1, cc_irq1);
> }
>
> - if (cc_irq2) {
> - regmap_write(wcove->regmap, USBC_IRQ2, cc_irq2);
> - /*
> - * Ingoring any PD communication interrupts until the PD stack
> - * is in place
> - */
> - if (cc_irq2 & ~USBC_IRQ2_CC_CHANGE) {
> - dev_WARN(wcove->dev, "USB PD handling missing\n");
> - goto err;
> - }
> - }
> + if (cc_irq2 & USBC_IRQ2_RX_PD ||
> + rx_status & USBC_RX_STATUS_RX_DATA)
remove this rx_status check, you don't need it.
> + wcove_typec_pd_recv_pkt_handler(wcove);
>
> if (status1 & USBC_STATUS1_DET_ONGOING)
> goto out;
> @@ -211,24 +394,49 @@ static irqreturn_t wcove_typec_irq(int irq, void *data)
> WCOVE_ORIENTATION_NORMAL);
> /* Host mode by default */
> wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_HOST);
> - goto out;
> - }
>
> - if (wcove->con.partner)
> + /* reset the pd sink state */
> + if (wcove->pd_port_num >= 0)
> + pd_sink_reset_state(wcove->pd_port_num);
> +
> goto out;
> + }
>
> switch (USBC_STATUS1_ORIENT(status1)) {
> case USBC_ORIENT_NORMAL:
> wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
> WCOVE_ORIENTATION_NORMAL);
> + regmap_update_bits(wcove->regmap, USBC_CC_SEL,
> + USBC_CC_SEL_CCSEL, 0x1);
> break;
> case USBC_ORIENT_REVERSE:
> wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
> WCOVE_ORIENTATION_REVERSE);
> + regmap_update_bits(wcove->regmap, USBC_CC_SEL,
> + USBC_CC_SEL_CCSEL, 0x2);
> default:
> break;
> }
>
> + if (cc_irq2 & USBC_IRQ2_RX_HR) {
> + dev_dbg(wcove->dev, "Hard Reset received\n");
> + if (wcove->pd_port_num >= 0)
> + pd_sink_reset_state(wcove->pd_port_num);
> +
> + }
> +
> + if (cc_irq2 & USBC_IRQ2_RX_CR)
> + dev_info(wcove->dev, "RX CR not implemented\n");
why don't you clear out your RX and TX buffers when you receive cable or
hard reset?
> @@ -307,26 +520,29 @@ static int wcove_typec_probe(struct platform_device *pdev)
> return ret;
>
> ret = devm_request_threaded_irq(&pdev->dev, ret, NULL,
> - wcove_typec_irq, IRQF_ONESHOT,
> + wcove_typec_irq, IRQF_ONESHOT,
unnecessary change
> "wcove_typec", wcove);
> if (ret)
> return ret;
>
> wcove->cap.type = TYPEC_PORT_DRP;
> -
> wcove->port = typec_register_port(&pdev->dev, &wcove->cap);
> if (IS_ERR(wcove->port))
> return PTR_ERR(wcove->port);
>
> + /* PD receive packet handler */
> + wcove->pd_port_num = pd_sink_register_port(&profile,
> + wcove_typec_pd_tx_pkt_handler, wcove);
> + if (wcove->pd_port_num) {
> + dev_err(&pdev->dev, "Register pd sink port failed\n");
> + return -EIO;
> + }
> +
> if (!acpi_check_dsm(ACPI_HANDLE(&pdev->dev), uuid.b, 0, 0x1f)) {
> dev_err(&pdev->dev, "Missing _DSM functions\n");
> return -ENODEV;
> }
>
> - /* Make sure the PD PHY is disabled until PD stack is ready */
> - regmap_read(wcove->regmap, USBC_CONTROL3, &val);
> - regmap_write(wcove->regmap, USBC_CONTROL3, val | USBC_CONTROL3_PD_DIS);
> -
> /* DRP mode without accessory support */
> regmap_read(wcove->regmap, USBC_CONTROL1, &val);
> regmap_write(wcove->regmap, USBC_CONTROL1, USBC_CONTROL1_MODE_DRP(val));
> @@ -337,7 +553,50 @@ static int wcove_typec_probe(struct platform_device *pdev)
> regmap_read(wcove->regmap, USBC_IRQMASK2, &val);
> regmap_write(wcove->regmap, USBC_IRQMASK2, val & ~USBC_IRQMASK2_ALL);
>
> + /*Set HW control the ID of outgoing messages*/
comment style
> + regmap_write(wcove->regmap, USBC_PD_CFG1, BIT(7));
> +
> + /* Enable SOP messages for now */
> + regmap_write(wcove->regmap, USBC_PD_CFG2, BIT(0));
> +
> + /*Set the PD revision */
comment style
> + regmap_read(wcove->regmap, USBC_PD_CFG3, &val);
> + val = 0x14;
no magic constants
> + regmap_write(wcove->regmap, USBC_PD_CFG3, val);
> +
> platform_set_drvdata(pdev, wcove);
> +
> + ret = regmap_read(wcove->regmap, USBC_STATUS1, &status1);
> + if (ret)
> + return ret;
> +
> + /* If cable is connected while booting, we would have lost all
> + * SOURCE_CAP messages. Hence, we send Hard Reset to source so
> + * that source would send us SOURCE_CAP messages again and we
> + * can establish PD communication again.
> + */
comment style. Also, this Hard Reset is a little dangerous: if our only
power supply is this PD supply, a Hard Reset will switch us off.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
prev parent reply other threads:[~2016-07-27 8:23 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-26 18:41 [PATCH v2 2/2] usb: typec: add PD sink port support for Intel Whiskey Cove PMIC Typc-C PHY driver Bin Gao
2016-07-27 8:21 ` Felipe Balbi [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87twfbfq3m.fsf@linux.intel.com \
--to=felipe.balbi@linux.intel.com \
--cc=bin.gao@intel.com \
--cc=bin.gao@linux.intel.com \
--cc=chandra.sekhar.anagani@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=pranav.tipnis@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.