Re: [PATCH 02/14 v2] usb: typec: tcpci: Add support when hidden tx registers are inaccessible

[Badhri Jagan Sridharan]

On Fri, Aug 28, 2020 at 6:12 AM Heikki Krogerus
 wrote:
>
> Looks like you forgot v1 somewhere :-)


Dumb me :P Yeah I was doing the patch numbers manually causing the confusion.
Greg was complaining as well.
These are all v1. I will re-upload everything as v2. Just noticed that
I can use the -v option.

>
>
> On Mon, Aug 24, 2020 at 09:21:58PM -0700, Badhri Jagan Sridharan wrote:
> > TCPCI spec forbids direct access of TX_BUF_BYTE_x register.
> > The existing version of tcpci driver assumes that those registers
> > are directly addressible. Add support for tcpci chips which do
> > not support direct access to TX_BUF_BYTE_x registers. TX_BUF_BYTE_x
> > can only be accessed by I2C_WRITE_BYTE_COUNT.
> >
> > Signed-off-by: Badhri Jagan Sridharan 
> > ---
> >  drivers/usb/typec/tcpm/tcpci.c | 49 +-
> >  drivers/usb/typec/tcpm/tcpci.h |  8 ++
> >  2 files changed, 44 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index f57d91fd0e09..90d348caa6a8 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -320,8 +320,7 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool 
> > source, bool sink)
> >   return 0;
> >  }
> >
> > -static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
> > -  enum tcpm_transmit_type type,
> > +static int tcpci_pd_transmit(struct tcpc_dev *tcpc, enum 
> > tcpm_transmit_type type,
> >const struct pd_message *msg)
>
> That does not look like a relevant change.
>
> >  {
> >   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > @@ -330,23 +329,47 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
> >   int ret;
> >
> >   cnt = msg ? pd_header_cnt(header) * 4 : 0;
> > - ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
> > - if (ret < 0)
> > - return ret;
> > + /**
> > +  * TCPCI spec forbids direct access of TCPC_TX_DATA.
> > +  * But, since some of the chipsets offer this capability,
> > +  * it's fair to support both.
> > +  */
> > + if (!tcpci->data->TX_BUF_BYTE_x_hidden) {
>
> Couldn't check if the flag is set first?

Yes will do !

>
> if (tcpci->data->TX_BUF_BYTE_x_hidden) {
> ...
>
> > + ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
> > + if (ret < 0)
> > + return ret;
> >
> > - ret = tcpci_write16(tcpci, TCPC_TX_HDR, header);
> > - if (ret < 0)
> > - return ret;
> > + 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, 
> > >payload, cnt);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + } else {
> > + u8 buf[TCPC_TRANSMIT_BUFFER_MAX_LEN] = {0,};
> > + u8 pos = 0;
> > +
> > + /* Payload + header + TCPC_TX_BYTE_CNT */
> > + buf[pos++] = cnt + 2;
> > +
> > + if (msg)
> > + memcpy([pos], >header, sizeof(msg->header));
> > +
> > + pos += sizeof(header);
> > +
> > + if (cnt > 0)
> > + memcpy([pos], msg->payload, cnt);
> >
> > - if (cnt > 0) {
> > - ret = regmap_raw_write(tcpci->regmap, TCPC_TX_DATA,
> > ->payload, cnt);
> > + pos += cnt;
> > + ret = regmap_raw_write(tcpci->regmap, TCPC_TX_BYTE_CNT, buf, 
> > pos);
> >   if (ret < 0)
> >   return ret;
> >   }
> >
> > - reg = (PD_RETRY_COUNT << TCPC_TRANSMIT_RETRY_SHIFT) |
> > - (type << TCPC_TRANSMIT_TYPE_SHIFT);
> > + 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;
> > diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
> > index fd26ca35814c..cf9d8b63adcb 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.h
> > +++ b/drivers/usb/typec/tcpm/tcpci.h
> > @@ -128,9 +128,17 @@
> >  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG   0x76
> >  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG   0x78
> >
> > +/* I2C_WRITE_BYTE_COUNT + 1 when TX_BUF_BYTE_x is only accessible 
> > I2C_WRITE_BYTE_COUNT */
> > +#define TCPC_TRANSMIT_BUFFER_MAX_LEN 31
> > +
> > +/*
> > + * @TX_BUF_BYTE_x_hidden
> > + *   optional; Set when TX_BUF_BYTE_x can only be accessed through 
> > I2C_WRITE_BYTE_COUNT.
> > + */
> >  struct tcpci;
> >  struct tcpci_data {
> >   struct regmap *regmap;
> > + unsigned char TX_BUF_BYTE_x_hidden:1;
> >   int (*init)(struct tcpci *tcpci, struct 

Re: [PATCH 02/14 v2] usb: typec: tcpci: Add support when hidden tx registers are inaccessible

[Heikki Krogerus]

Looks like you forgot v1 somewhere :-)

On Mon, Aug 24, 2020 at 09:21:58PM -0700, Badhri Jagan Sridharan wrote:
> TCPCI spec forbids direct access of TX_BUF_BYTE_x register.
> The existing version of tcpci driver assumes that those registers
> are directly addressible. Add support for tcpci chips which do
> not support direct access to TX_BUF_BYTE_x registers. TX_BUF_BYTE_x
> can only be accessed by I2C_WRITE_BYTE_COUNT.
> 
> Signed-off-by: Badhri Jagan Sridharan 
> ---
>  drivers/usb/typec/tcpm/tcpci.c | 49 +-
>  drivers/usb/typec/tcpm/tcpci.h |  8 ++
>  2 files changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index f57d91fd0e09..90d348caa6a8 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -320,8 +320,7 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool 
> source, bool sink)
>   return 0;
>  }
>  
> -static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
> -  enum tcpm_transmit_type type,
> +static int tcpci_pd_transmit(struct tcpc_dev *tcpc, enum tcpm_transmit_type 
> type,
>const struct pd_message *msg)

That does not look like a relevant change.

>  {
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> @@ -330,23 +329,47 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
>   int ret;
>  
>   cnt = msg ? pd_header_cnt(header) * 4 : 0;
> - ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
> - if (ret < 0)
> - return ret;
> + /**
> +  * TCPCI spec forbids direct access of TCPC_TX_DATA.
> +  * But, since some of the chipsets offer this capability,
> +  * it's fair to support both.
> +  */
> + if (!tcpci->data->TX_BUF_BYTE_x_hidden) {

Couldn't check if the flag is set first?

if (tcpci->data->TX_BUF_BYTE_x_hidden) {
...

> + ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
> + if (ret < 0)
> + return ret;
>  
> - ret = tcpci_write16(tcpci, TCPC_TX_HDR, header);
> - if (ret < 0)
> - return ret;
> + 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, 
> >payload, cnt);
> + if (ret < 0)
> + return ret;
> + }
> + } else {
> + u8 buf[TCPC_TRANSMIT_BUFFER_MAX_LEN] = {0,};
> + u8 pos = 0;
> +
> + /* Payload + header + TCPC_TX_BYTE_CNT */
> + buf[pos++] = cnt + 2;
> +
> + if (msg)
> + memcpy([pos], >header, sizeof(msg->header));
> +
> + pos += sizeof(header);
> +
> + if (cnt > 0)
> + memcpy([pos], msg->payload, cnt);
>  
> - if (cnt > 0) {
> - ret = regmap_raw_write(tcpci->regmap, TCPC_TX_DATA,
> ->payload, cnt);
> + pos += cnt;
> + ret = regmap_raw_write(tcpci->regmap, TCPC_TX_BYTE_CNT, buf, 
> pos);
>   if (ret < 0)
>   return ret;
>   }
>  
> - reg = (PD_RETRY_COUNT << TCPC_TRANSMIT_RETRY_SHIFT) |
> - (type << TCPC_TRANSMIT_TYPE_SHIFT);
> + 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;
> diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
> index fd26ca35814c..cf9d8b63adcb 100644
> --- a/drivers/usb/typec/tcpm/tcpci.h
> +++ b/drivers/usb/typec/tcpm/tcpci.h
> @@ -128,9 +128,17 @@
>  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG   0x76
>  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG   0x78
>  
> +/* I2C_WRITE_BYTE_COUNT + 1 when TX_BUF_BYTE_x is only accessible 
> I2C_WRITE_BYTE_COUNT */
> +#define TCPC_TRANSMIT_BUFFER_MAX_LEN 31
> +
> +/*
> + * @TX_BUF_BYTE_x_hidden
> + *   optional; Set when TX_BUF_BYTE_x can only be accessed through 
> I2C_WRITE_BYTE_COUNT.
> + */
>  struct tcpci;
>  struct tcpci_data {
>   struct regmap *regmap;
> + unsigned char TX_BUF_BYTE_x_hidden:1;
>   int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
>   int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
>bool enable);
> -- 
> 2.28.0.297.g1956fa8f8d-goog

thanks,

-- 
heikki

[PATCH 02/14 v2] usb: typec: tcpci: Add support when hidden tx registers are inaccessible

[Badhri Jagan Sridharan]

TCPCI spec forbids direct access of TX_BUF_BYTE_x register.
The existing version of tcpci driver assumes that those registers
are directly addressible. Add support for tcpci chips which do
not support direct access to TX_BUF_BYTE_x registers. TX_BUF_BYTE_x
can only be accessed by I2C_WRITE_BYTE_COUNT.

Signed-off-by: Badhri Jagan Sridharan 
---
 drivers/usb/typec/tcpm/tcpci.c | 49 +-
 drivers/usb/typec/tcpm/tcpci.h |  8 ++
 2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index f57d91fd0e09..90d348caa6a8 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -320,8 +320,7 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool 
source, bool sink)
return 0;
 }
 
-static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
-enum tcpm_transmit_type type,
+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);
@@ -330,23 +329,47 @@ static int tcpci_pd_transmit(struct tcpc_dev *tcpc,
int ret;
 
cnt = msg ? pd_header_cnt(header) * 4 : 0;
-   ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
-   if (ret < 0)
-   return ret;
+   /**
+* TCPCI spec forbids direct access of TCPC_TX_DATA.
+* But, since some of the chipsets offer this capability,
+* it's fair to support both.
+*/
+   if (!tcpci->data->TX_BUF_BYTE_x_hidden) {
+   ret = regmap_write(tcpci->regmap, TCPC_TX_BYTE_CNT, cnt + 2);
+   if (ret < 0)
+   return ret;
 
-   ret = tcpci_write16(tcpci, TCPC_TX_HDR, header);
-   if (ret < 0)
-   return ret;
+   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, 
>payload, cnt);
+   if (ret < 0)
+   return ret;
+   }
+   } else {
+   u8 buf[TCPC_TRANSMIT_BUFFER_MAX_LEN] = {0,};
+   u8 pos = 0;
+
+   /* Payload + header + TCPC_TX_BYTE_CNT */
+   buf[pos++] = cnt + 2;
+
+   if (msg)
+   memcpy([pos], >header, sizeof(msg->header));
+
+   pos += sizeof(header);
+
+   if (cnt > 0)
+   memcpy([pos], msg->payload, cnt);
 
-   if (cnt > 0) {
-   ret = regmap_raw_write(tcpci->regmap, TCPC_TX_DATA,
-  >payload, cnt);
+   pos += cnt;
+   ret = regmap_raw_write(tcpci->regmap, TCPC_TX_BYTE_CNT, buf, 
pos);
if (ret < 0)
return ret;
}
 
-   reg = (PD_RETRY_COUNT << TCPC_TRANSMIT_RETRY_SHIFT) |
-   (type << TCPC_TRANSMIT_TYPE_SHIFT);
+   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;
diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
index fd26ca35814c..cf9d8b63adcb 100644
--- a/drivers/usb/typec/tcpm/tcpci.h
+++ b/drivers/usb/typec/tcpm/tcpci.h
@@ -128,9 +128,17 @@
 #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG 0x76
 #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG 0x78
 
+/* I2C_WRITE_BYTE_COUNT + 1 when TX_BUF_BYTE_x is only accessible 
I2C_WRITE_BYTE_COUNT */
+#define TCPC_TRANSMIT_BUFFER_MAX_LEN   31
+
+/*
+ * @TX_BUF_BYTE_x_hidden
+ * optional; Set when TX_BUF_BYTE_x can only be accessed through 
I2C_WRITE_BYTE_COUNT.
+ */
 struct tcpci;
 struct tcpci_data {
struct regmap *regmap;
+   unsigned char TX_BUF_BYTE_x_hidden:1;
int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
 bool enable);
-- 
2.28.0.297.g1956fa8f8d-goog