RE: [PATCH] staging: typec: handle vendor defined part and modify drp toggling flow

[Jun Li]

Hi
> -----Original Message-----
> From: shufan_lee(李?帆) [mailto:shufan_lee@xxxxxxxxxxx]
> Sent: 2018年2月28日 11:41
> To: Jun Li <jun.li@xxxxxxx>; ShuFanLee <leechu729@xxxxxxxxx>;
> heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; greg@xxxxxxxxx
> Cc: cy_huang(??原) <cy_huang@xxxxxxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> Subject: 回覆: [PATCH] staging: typec: handle vendor defined part and
> modify drp toggling flow
> 
> Hi Jun,
> 
>   For the questions of drp_toggling, our test is as following:
> 
>   According to TCPCI 4.4.5.2
> It is recommended the TCPM write ROLE_CONTROL.DRP=0 before writing to
> POWER_CONTROL.AutoDischargeDisconnect and starting the DRP toggling
> using COMMAND.Look4Connection.
> 
>   We've encounter a situation while testing with RT1711H as following:
>   We repeatedly plug in/out a device (with Rd), and not to provide VBUS(5V)
> while plugging in.
>   If we plug out the device right after TCPC detects it and stops toggling,
> TCPM will try to restart drp_toggling.
>   Here, we re-plug in the device before TCPM calls drp_toggling. Under this
> circumstance, RT1711H reports open/open after drp_toggling is called.

Why RT1711H reports open/open after drp_toggling is enabled?
This open/open is for you plug out the device, right?
Why RT1711H can't report new cc change events after you plug in the device?

Please use tcpm log to show all the events and state transitions for your
above corner case.
cat /sys/kernel/debug/tcpm/xxxxx

>   For current TCPM, it stops if open/open is reported at drp_toggling state.
> 
>   The detailed flow that causes RT1711H reporting open/open is described
> as following:
>   1. RT1711H detects the device, stops toggling and reports to TCPM.
>   2. Rd is plugged out before set_cc is called. So, ROLE_CONTROL.DRP is
> still 1.

So open/open cc change will generate.
 
>   3. Plug in the device before TCPM restarts drp_toggling
>   4. The device is detected by RT1711H's internal firmware again(TCPC
> chip's internal firmware).

What cc change event tcpc will report on step 4?

>   5. TCPM calls drp_toggling before cc_change triggered by step 4 is
> handled.
>   6. TCPM sets ROLE_CONTROL.DRP = 1, Rd/Rd and start drp_toggling.
> According to TCPCI 4.4.5.2
> The TCPM shall write B6 (DRP) = 0b and B3..0 (CC1/CC2) if it wishes to
> control the Rp/Rd directly instead of having the TCPC perform DRP toggling
> autonomousl.
> So, Rd/Rd set in step 6 will not work. (Because ROLE_CONTROL.DRP is 1
> since the first step.)
>   7. RT1711H will stop toggling immediately (Because it's internal firmware
> already detects device at step 4) and report open/open (Because CC1/CC2
> will be changed to Rd by RT1711H after LOOK4CONNECTION is set. RT1711H
> sets to Rd and device is Rd so open is reported)
>   8. TCPCI reports open/open to TCPM at drp_toggling state
> 
>   That's why we always set ROLE_CONTROL.DRP to 0 while setting Rd/Rd.
>   If this circumstance is not a general case, we can also use a vendor ops to
> replace it.

Thanks for the detailed description, the tcpm full log is required to know
what's going on here, you can apply my drp_toggling change patch[1] and
run your problem case again, then post the full tcpm log. 

Generally, I think you need check further this is a problem on the current
tcpm _or_ your tcpc chip, if the problem on the tcpm, you should resolve
this issue by improve tcpm, if the problem on your tcpc chip, you can add
a vendor ops.

I tested my tcpc HW with my drp_toggling change patch[1] (w/o your change)
by quickly pulg-in & plug-out a sink, no problem found.
Did you verify your change can pass the typec compliance test?

[1] https://www.spinics.net/lists/devicetree/msg216851.html
> ==============================================================
> ==========
> 
> I don't catch the point of how you handle private events by above change,
> maybe post your RT1711H part as a user in one series can make it clear, I
> assume this can be done in existing tcpci_irq like above vender specific
> handling as well:
> 
> For every glue driver, it registers its own irq handler and calls tcpci_irq in the
> handler.
> 
> Take RT1711H as an example:
> It registers its own irq handler in probe function and handle vendor defined
> interrupts before calling general tcpci_irq:
> 
> static irqreturn_t _tcpci_irq(int irq, void *dev_id) {
>         struct rt1711h_chip *chip = dev_id;
> 
>         /* handle vendor defined interrupts here */
> 
>         /* call general tcpci_irq */
>         return tcpci_irq(chip->tcpci);
> }
> 
> static int rt1711h_probe(struct i2c_client *client, const struct i2c_device_id
> *i2c_id) {
>         ....
>         ret = devm_request_threaded_irq(chip->dev, client->irq, NULL,
>                                           _tcpci_irq,
>                                           IRQF_ONESHOT |
> IRQF_TRIGGER_LOW,
>                                           dev_name(&client->dev),
> chip); }
> 
> 
Get it, thanks.

> Best Regards,
> *****************************************
> Shu-Fan Lee
> Richtek Technology Corporation
> TEL: +886-3-5526789 #2359
> FAX: +886-3-5526612
> *****************************************
> 
> ________________________________________
> 寄件者: Jun Li <jun.li@xxxxxxx>
> 寄件日期: 2018年2月22日 下午 06:16
> 收件者: ShuFanLee; heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx;
> greg@xxxxxxxxx
> 副本: shufan_lee(李?帆); cy_huang(??原); linux-kernel@xxxxxxxxxxxxxxx;
> linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx
> 主旨: RE: [PATCH] staging: typec: handle vendor defined part and modify drp
> toggling flow
> 
> Hi,
> 
> > -----Original Message-----
> > From: linux-usb-owner@xxxxxxxxxxxxxxx
> > [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of ShuFanLee
> > Sent: 2018年2月21日 23:02
> > To: heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx;
> > greg@xxxxxxxxx
> > Cc: shufan_lee@xxxxxxxxxxx; cy_huang@xxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> > Subject: [PATCH] staging: typec: handle vendor defined part and modify
> > drp toggling flow
> >
> > From: ShuFanLee <shufan_lee@xxxxxxxxxxx>
> >
> > Handle vendor defined behavior in tcpci_init, tcpci_set_vconn and
> > export tcpci_irq.
> > More operations can be extended in tcpci_data if needed.
> > According to TCPCI specification, 4.4.5.2 ROLE_CONTROL, TCPC shall not
> > start DRP toggling until subsequently the TCPM writes to the COMMAND
> > register to start DRP toggling.
> 
> My understanding of above statement is TCPM(this Linux driver) can enable
> DRP and CC1/CC2 in one shot, but TCPC(your typec chip internal firmware)
> needs wait until TCPM writes to COMMAND register to start drp toggling.
> 
> > DRP toggling flow is chagned as following:
> >   - Write DRP = 0 & Rd/Rd
> 
> Why fixed to be Rd/Rd? in this case, it means the starting value:
> 
> Tcpci 4.4.5.2:
> "When initiating autonomous DRP toggling, the TCPM shall write B6 (DRP)
> =1b and the starting value of Rp/Rd to B3..0 (CC1/CC2) to indicate DRP
> autonomous toggling mode to the TCPC."
> 
> >   - Write DRP = 1
> 
> What's your problem if combine above 2 in one shot?
> 
> >   - Set LOOK4CONNECTION command
> >
> > Signed-off-by: ShuFanLee <shufan_lee@xxxxxxxxxxx>
> > ---
> >  drivers/staging/typec/tcpci.c | 128
> > +++++++++++++++++++++++++++++++++---------
> >  drivers/staging/typec/tcpci.h |  13 +++++
> >  2 files changed, 115 insertions(+), 26 deletions(-)
> >
> >  patch changelogs between v1 & v2
> >  - Remove unnecessary i2c_client in the structure of tcpci
> >  - Rename structure of tcpci_vendor_data to tcpci_data
> >  - Not exporting tcpci read/write wrappers but register i2c regmap in
> > glue driver
> >  - Add set_vconn ops in tcpci_data
> >    (It is necessary for RT1711H to enable/disable idle mode before
> > disabling/enabling vconn)
> >  - Export tcpci_irq so that vendor can call it in their own IRQ
> > handler
> >
> >  patch changelogs between v2 & v3
> >  - Change the return type of tcpci_irq from int to irqreturn_t
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index 9bd4412..4959c69 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -21,7 +21,6 @@
> >
> >  struct tcpci {
> >       struct device *dev;
> > -     struct i2c_client *client;
> >
> >       struct tcpm_port *port;
> >
> > @@ -30,6 +29,12 @@ struct tcpci {
> >       bool controls_vbus;
> >
> >       struct tcpc_dev tcpc;
> > +     struct tcpci_data *data;
> > +};
> > +
> > +struct tcpci_chip {
> > +     struct tcpci *tcpci;
> > +     struct tcpci_data data;
> >  };
> >
> >  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc) @@
> > -37,8
> > +42,7 @@ 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,
> > -                     u16 *val)
> > +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16
> > +*val)
> >  {
> >       return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));  }
> > @@
> > -98,8 +102,10 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum
> > typec_cc_status cc)  static int tcpci_start_drp_toggling(struct
> > tcpc_dev *tcpc,
> >                                   enum typec_cc_status cc)  {
> > +     int ret;
> >       struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > -     unsigned int reg = TCPC_ROLE_CTRL_DRP;
> > +     unsigned int reg = (TCPC_ROLE_CTRL_CC_RD <<
> > TCPC_ROLE_CTRL_CC1_SHIFT) |
> > +                        (TCPC_ROLE_CTRL_CC_RD <<
> > TCPC_ROLE_CTRL_CC2_SHIFT);
> >
> >       switch (cc) {
> >       default:
> > @@ -117,7 +123,19 @@ static int tcpci_start_drp_toggling(struct
> > tcpc_dev *tcpc,
> >               break;
> >       }
> >
> > -     return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> > +     ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> > +     if (ret < 0)
> > +             return ret;
> > +     usleep_range(500, 1000);
> 
> Why need a wait here? It seems you actually don't want to do autonomously
> toggling from the very beginning, instead, you begin with a directly control
> on CC, keep it(Rd) for some time, then switch to use autonomously toggling,
> why you need such kind of sequence? I think it's special and not following
> tcpci spec.
> 
> > +     reg |= TCPC_ROLE_CTRL_DRP;
> > +     ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> > +                        TCPC_CMD_LOOK4CONNECTION);
> > +     if (ret < 0)
> > +             return ret;
> > +     return 0;
> >  }
> >
> >  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool
> > sink) @@ -178,6 +196,16 @@ static int tcpci_set_vconn(struct tcpc_dev
> > *tcpc, bool enable)
> >       struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> >       int ret;
> >
> > +     /* Handle vendor set vconn */
> > +     if (tcpci->data) {
> > +             if (tcpci->data->set_vconn) {
> > +                     ret = tcpci->data->set_vconn(tcpci, tcpci->data,
> > +                                                  enable);
> > +                     if (ret < 0)
> > +                             return ret;
> > +             }
> > +     }
> > +
> >       ret = regmap_write(tcpci->regmap, TCPC_POWER_CTRL,
> >                          enable ?
> TCPC_POWER_CTRL_VCONN_ENABLE : 0);
> >       if (ret < 0)
> > @@ -323,6 +351,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> >       if (time_after(jiffies, timeout))
> >               return -ETIMEDOUT;
> >
> > +     /* Handle vendor init */
> > +     if (tcpci->data) {
> > +             if (tcpci->data->init) {
> > +                     ret = tcpci->data->init(tcpci, tcpci->data);
> > +                     if (ret < 0)
> > +                             return ret;
> > +             }
> > +     }
> > +
> >       /* Clear all events */
> >       ret = tcpci_write16(tcpci, TCPC_ALERT, 0xffff);
> >       if (ret < 0)
> > @@ -344,9 +381,15 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> >       return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);  }
> >
> > -static irqreturn_t tcpci_irq(int irq, void *dev_id)
> > +static irqreturn_t _tcpci_irq(int irq, void *dev_id)
> >  {
> >       struct tcpci *tcpci = dev_id;
> > +
> > +     return tcpci_irq(tcpci);
> > +}
> > +
> > +irqreturn_t tcpci_irq(struct tcpci *tcpci) {
> >       u16 status;
> >
> >       tcpci_read16(tcpci, TCPC_ALERT, &status); @@ -412,6 +455,7 @@
> > static irqreturn_t tcpci_irq(int irq, void *dev_id)
> >
> >       return IRQ_HANDLED;
> >  }
> > +EXPORT_SYMBOL_GPL(tcpci_irq);
> 
> I don't catch the point of how you handle private events by above change,
> maybe post your RT1711H part as a user in one series can make it clear, I
> assume this can be done in existing tcpci_irq like above vender specific
> handling as well:
>       tcpci->data->priv_events(tcpci, tcpci->data);
> 
> Li Jun
> >
> >  static const struct regmap_config tcpci_regmap_config = {
> >       .reg_bits = 8,
> > @@ -435,22 +479,18 @@ static int tcpci_parse_config(struct tcpci *tcpci)
> >       return 0;
> >  }
> >
> > -static int tcpci_probe(struct i2c_client *client,
> > -                    const struct i2c_device_id *i2c_id)
> > +struct tcpci *tcpci_register_port(struct device *dev, struct
> > +tcpci_data
> > +*data)
> >  {
> >       struct tcpci *tcpci;
> >       int err;
> >
> > -     tcpci = devm_kzalloc(&client->dev, sizeof(*tcpci), GFP_KERNEL);
> > +     tcpci = devm_kzalloc(dev, sizeof(*tcpci), GFP_KERNEL);
> >       if (!tcpci)
> > -             return -ENOMEM;
> > +             return ERR_PTR(-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->dev = dev;
> > +     tcpci->data = data;
> > +     tcpci->regmap = data->regmap;
> >
> >       tcpci->tcpc.init = tcpci_init;
> >       tcpci->tcpc.get_vbus = tcpci_get_vbus; @@ -467,27 +507,63 @@
> > static int tcpci_probe(struct i2c_client *client,
> >
> >       err = tcpci_parse_config(tcpci);
> >       if (err < 0)
> > -             return err;
> > +             return ERR_PTR(err);
> > +
> > +     tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> > +     if (PTR_ERR_OR_ZERO(tcpci->port))
> > +             return ERR_CAST(tcpci->port);
> >
> > -     /* Disable chip interrupts */
> > -     tcpci_write16(tcpci, TCPC_ALERT_MASK, 0);
> > +     return tcpci;
> > +}
> > +EXPORT_SYMBOL_GPL(tcpci_register_port);
> > +
> > +void tcpci_unregister_port(struct tcpci *tcpci) {
> > +     tcpm_unregister_port(tcpci->port);
> > +}
> > +EXPORT_SYMBOL_GPL(tcpci_unregister_port);
> >
> > -     err = devm_request_threaded_irq(tcpci->dev, client->irq, NULL,
> > -                                     tcpci_irq,
> > +static int tcpci_probe(struct i2c_client *client,
> > +                    const struct i2c_device_id *i2c_id) {
> > +     struct tcpci_chip *chip;
> > +     int err;
> > +     u16 val = 0;
> > +
> > +     chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > +     if (!chip)
> > +             return -ENOMEM;
> > +
> > +     chip->data.regmap = devm_regmap_init_i2c(client,
> > &tcpci_regmap_config);
> > +     if (IS_ERR(chip->data.regmap))
> > +             return PTR_ERR(chip->data.regmap);
> > +
> > +     /* Disable chip interrupts before requesting irq */
> > +     err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK,
> &val,
> > +                            sizeof(u16));
> > +     if (err < 0)
> > +             return err;
> > +
> > +     err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> > +                                     _tcpci_irq,
> >                                       IRQF_ONESHOT |
> IRQF_TRIGGER_LOW,
> > -                                     dev_name(tcpci->dev),
> tcpci);
> > +                                     dev_name(&client->dev),
> chip);
> >       if (err < 0)
> >               return err;
> >
> > -     tcpci->port = tcpm_register_port(tcpci->dev, &tcpci->tcpc);
> > -     return PTR_ERR_OR_ZERO(tcpci->port);
> > +     chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> > +     if (PTR_ERR_OR_ZERO(chip->tcpci))
> > +             return PTR_ERR(chip->tcpci);
> > +
> > +     i2c_set_clientdata(client, chip);
> > +     return 0;
> >  }
> >
> >  static int tcpci_remove(struct i2c_client *client)  {
> > -     struct tcpci *tcpci = i2c_get_clientdata(client);
> > +     struct tcpci_chip *chip = i2c_get_clientdata(client);
> >
> > -     tcpm_unregister_port(tcpci->port);
> > +     tcpci_unregister_port(chip->tcpci);
> >
> >       return 0;
> >  }
> > diff --git a/drivers/staging/typec/tcpci.h
> > b/drivers/staging/typec/tcpci.h index fdfb06c..40025b2 100644
> > --- a/drivers/staging/typec/tcpci.h
> > +++ b/drivers/staging/typec/tcpci.h
> > @@ -59,6 +59,7 @@
> >  #define TCPC_POWER_CTRL_VCONN_ENABLE BIT(0)
> >
> >  #define TCPC_CC_STATUS                       0x1d
> > +#define TCPC_CC_STATUS_DRPRST                BIT(5)
> >  #define TCPC_CC_STATUS_TERM          BIT(4)
> >  #define TCPC_CC_STATUS_CC2_SHIFT     2
> >  #define TCPC_CC_STATUS_CC2_MASK              0x3
> > @@ -121,4 +122,16 @@
> >  #define TCPC_VBUS_VOLTAGE_ALARM_HI_CFG               0x76
> >  #define TCPC_VBUS_VOLTAGE_ALARM_LO_CFG               0x78
> >
> > +struct tcpci;
> > +struct tcpci_data {
> > +     struct regmap *regmap;
> > +     int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> > +     int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> > +                      bool enable);
> > +};
> > +
> > +struct tcpci *tcpci_register_port(struct device *dev, struct
> > +tcpci_data *data); void tcpci_unregister_port(struct tcpci *tcpci);
> > +irqreturn_t tcpci_irq(struct tcpci *tcpci);
> > +
> >  #endif /* __LINUX_USB_TCPCI_H */
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > in the body of a message to majordomo@xxxxxxxxxxxxxxx More
> majordomo
> > info at
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger
> > .kernel.org%2Fmajordomo-info.html&data=02%7C01%7Cjun.li%40nxp.co
> m
> > %7C44d11af27e0244fa4d1108d5793c3dc4%7C686ea1d3bc2b4c6fa92cd99
> c
> >
> 5c301635%7C0%7C1%7C636548222008905206&sdata=zn2ougKRvs%2BbmH
> > Qg2c8Rjj7vNe0ZiiVKdar7GdPa%2B3o%3D&reserved=0
> ************* Email Confidentiality Notice ********************
> 
> The information contained in this e-mail message (including any attachments)
> may be confidential, proprietary, privileged, or otherwise exempt from
> disclosure under applicable laws. It is intended to be conveyed only to the
> designated recipient(s). Any use, dissemination, distribution, printing,
> retaining or copying of this e-mail (including its attachments) by unintended
> recipient(s) is strictly prohibited and may be unlawful. If you are not an
> intended recipient of this e-mail, or believe that you have received this
> e-mail in error, please notify the sender immediately (by replying to this
> e-mail), delete any and all copies of this e-mail (including any attachments)
> from your system, and do not disclose the content of this e-mail to any other
> person. Thank you!