Re: [PATCH v2] phy: rockchip-inno-usb2: allow to force the B-Device Session Valid bit.

From: Ezequiel Garcia
Date: Fri Jun 28 2019 - 20:38:57 EST

Next message: Wu Hao: "Re: [PATCH v4 3/3] fpga: dfl: fme: add power management support"
Previous message: Stephen Rothwell: "Re: linux-next: build failure after merge of the battery tree"
In reply to: Ezequiel Garcia: "Re: [PATCH v2] phy: rockchip-inno-usb2: allow to force the B-Device Session Valid bit."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Heiko, Kishon,

I'll try to pick up this patch.
Some comments below, just for self-reference.

On Wed, 2019-06-26 at 12:32 -0300, Ezequiel Garcia wrote:
> On Wed, 2019-05-15 at 18:20 -0400, GaÃl PORTAY wrote:
> > From: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> >
> > The OTG disconnection event is generated after the presence/absence of
> > an ID connection, but some platforms don't have the ID pin connected, so
> > the event is not generated. In such case, for detecting the
> > disconnection event, we can get the cable state from an extcon driver.
> > We need, though, to force to set the B-Device Session Valid bit on the
> > PHY to have the device respond to the setup address. Otherwise, the
> > following error is shown:
> >
> > usb 2-2: Device not responding to setup address.
> > usb 2-2: device not accepting address 14, error -71
> > usb usb2-port2: unable to enumerate USB device
> >
> > The patch tells the PHY to force the B-Device Session Valid bit when the
> > OTG role is device and clear that bit if the OTG role is host, when an
> > extcon is available.
> >
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> > Signed-off-by: GaÃl PORTAY <gael.portay@xxxxxxxxxxxxx>
> > ---
> >
> > Hi all,
> >
> > The main purpose of this patch is have the Type-C port on the Samsung
> > Chromebook Plus work as a device or in OTG mode.
> >
> > That patch was originally a part of that patchset[1]; all other patches
> > was merged recently in master.
> >
> > The patch was tested on a Samsung Chromebook Plus by configuring one
> > port to work as device, configure a cdc ethernet gadget and communicate
> > via ethernet gadget my workstation with the chromebook through a usb-a
> > to type-c cable.
> >
> > Best regards,
> > GaÃl
> >
> > [1]: https://lkml.org/lkml/2018/8/15/141
> >

We still need the above devicetree changes.

> > Changes since v1:
> > - [PATCH 3/4] Remove introduction of dt property "rockchip,force-bvalid"
> > and replace cable state using extcon instead (if set).
> >
> > drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 51 +++++++++++++++++++
> > 1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index ba07121c3eff..5e9d50b5ae16 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -125,6 +125,7 @@ struct rockchip_chg_det_reg {
> > * @bvalid_det_en: vbus valid rise detection enable register.
> > * @bvalid_det_st: vbus valid rise detection status register.
> > * @bvalid_det_clr: vbus valid rise detection clear register.
> > + * @bvalid_session: force B-device session valid register.
> > * @ls_det_en: linestate detection enable register.
> > * @ls_det_st: linestate detection state register.
> > * @ls_det_clr: linestate detection clear register.
> > @@ -138,6 +139,7 @@ struct rockchip_usb2phy_port_cfg {
> > struct usb2phy_reg bvalid_det_en;
> > struct usb2phy_reg bvalid_det_st;
> > struct usb2phy_reg bvalid_det_clr;
> > + struct usb2phy_reg bvalid_session;
> > struct usb2phy_reg ls_det_en;
> > struct usb2phy_reg ls_det_st;
> > struct usb2phy_reg ls_det_clr;
> > @@ -169,6 +171,7 @@ struct rockchip_usb2phy_cfg {
> > * @port_id: flag for otg port or host port.
> > * @suspended: phy suspended flag.
> > * @vbus_attached: otg device vbus status.
> > + * @force_bvalid: force the control of the B-device session valid bit.
> > * @bvalid_irq: IRQ number assigned for vbus valid rise detection.
> > * @ls_irq: IRQ number assigned for linestate detection.
> > * @otg_mux_irq: IRQ number which multiplex otg-id/otg-bvalid/linestate
> > @@ -187,6 +190,7 @@ struct rockchip_usb2phy_port {
> > unsigned int port_id;
> > bool suspended;
> > bool vbus_attached;
> > + bool force_bvalid;
> > int bvalid_irq;
> > int ls_irq;
> > int otg_mux_irq;
> > @@ -553,6 +557,13 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> > switch (rport->state) {
> > case OTG_STATE_UNDEFINED:
> > rport->state = OTG_STATE_B_IDLE;
> > + if (rport->force_bvalid) {
> > + property_enable(rphy->grf,
> > + &rport->port_cfg->bvalid_session,
> > + true);
> > + dev_dbg(&rport->phy->dev,
> > + "set the B-Device Session Valid\n");
> > + }
> > if (!vbus_attach)
> > rockchip_usb2phy_power_off(rport->phy);
> > /* fall through */
> > @@ -560,6 +571,14 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> > if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) > 0) {
> > dev_dbg(&rport->phy->dev, "usb otg host connect\n");
> > rport->state = OTG_STATE_A_HOST;
> > + /* When leaving device mode force end the session */
> > + if (rport->force_bvalid) {
> > + property_enable(rphy->grf,
> > + &rport->port_cfg->bvalid_session,
> > + false);
> > + dev_dbg(&rport->phy->dev,
> > + "clear the B-Device Session Valid\n");
> > + }
> > rockchip_usb2phy_power_on(rport->phy);
> > return;
> > } else if (vbus_attach) {
> > @@ -634,6 +653,14 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> > if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) == 0) {
> > dev_dbg(&rport->phy->dev, "usb otg host disconnect\n");
> > rport->state = OTG_STATE_B_IDLE;
> > + /* When leaving host mode force start the session */
> > + if (rport->force_bvalid) {
> > + property_enable(rphy->grf,
> > + &rport->port_cfg->bvalid_session,
> > + true);
> > + dev_dbg(&rport->phy->dev,
> > + "set the B-Device Session Valid\n");
> > + }
> > rockchip_usb2phy_power_off(rport->phy);
> > }
> > break;
> > @@ -1016,6 +1043,28 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> > INIT_DELAYED_WORK(&rport->chg_work, rockchip_chg_detect_work);
> > INIT_DELAYED_WORK(&rport->otg_sm_work, rockchip_usb2phy_otg_sm_work);
> >
> > + /*
> > + * Some platforms doesn't have the ID pin connected to the phy, hence
> > + * the OTD ID event is not generated, but in some cases we can get the
> > + * cable state from an extcon driver. In such case we can force to set
> > + * the B-Device Session Valid bit on the PHY to have the device working
> > + * as a OTG.
> > + */
> > + if (rphy->edev) {

I might be missing something, but this check seems bogus.

edev can't be NULL as the driver creates an extcon if
there is none assigned in the devicetree.

> > + /*
> > + * Check if bvalid_session register is set in the structure
> > + * rockchip_usb2phy_cfg for this SoC.
> > + */
> > + if (rport->port_cfg->bvalid_session.offset == 0x0) {
> > + rport->force_bvalid = false;
> > + dev_err(rphy->dev,
> > + "cannot force B-device session, the register is not set for that SoC\n");
> > + } else {
> > + rport->force_bvalid = true;
> > + dev_info(rphy->dev, "force B-device session enabled\n");
> > + }

I think we should be doing something more like:

if (HAS_REG(rport->port_cfg->bvalid_session)) {
rport->force_bvalid = true;
dev_info(rphy->dev, "force B-device session enabled\n");
}

And not error verbosely on platforms that don't
care about this.

Thanks,
Ezequiel

Next message: Wu Hao: "Re: [PATCH v4 3/3] fpga: dfl: fme: add power management support"
Previous message: Stephen Rothwell: "Re: linux-next: build failure after merge of the battery tree"
In reply to: Ezequiel Garcia: "Re: [PATCH v2] phy: rockchip-inno-usb2: allow to force the B-Device Session Valid bit."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]