RE: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.

From: Pawel Laszczak <pawell@cadence.com>
To: Peter Chen <hzpeterchen@gmail.com>, "rogerq@ti.com" <rogerq@ti.com>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	Alan Douglas <adouglas@cadence.com>,
	"jbergsagel@ti.com" <jbergsagel@ti.com>,
	"nsekhar@ti.com" <nsekhar@ti.com>, "nm@ti.com" <nm@ti.com>,
	Suresh Punnoose <sureshp@cadence.com>,
	"peter.chen@nxp.com" <peter.chen@nxp.com>,
	Pawel Jez <pjez@cadence.com>, Rahul Kumar <kurahul@cadence.com>,
	Tomasz Klimek <tklimek@cadence.com>
Subject: RE: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.
Date: Wed, 5 Dec 2018 19:42:01 +0000	[thread overview]
Message-ID: <BYAPR07MB47095578C205570DD97EBFE0DDA80@BYAPR07MB4709.namprd07.prod.outlook.com> (raw)
In-Reply-To: <CAL411-rWc23fbP0Ny9Kfdpe9ESUSVt5X00G1uFjJf+rj-2jWzg@mail.gmail.com>

+ Tomek Klimek 

>> > +
>> > +static inline void cdns3_role_stop(struct cdns3 *cdns)
>> > +{
>> > +     enum cdns3_roles role = cdns->role;
>> > +
>> > +     if (role == CDNS3_ROLE_END)
>>
>> WARN_ON(role >= CNDS3_ROLE_END) ?
>>
>> > +             return;
>> > +
>> > +     mutex_lock(&cdns->mutex);
>> > +     cdns->roles[role]->stop(cdns);
>> > +     cdns->role = CDNS3_ROLE_END;
>>
>> Why change the role here? You are just stopping the role not changing it.
>> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>> if required without error.
>>
>
>The current version of this IP has some issues to detect vbus status correctly,
>we have to force vbus status accordingly, so we need a status to indicate
>vbus disconnection, and add some code to let controller know vbus
>removal, in that case, the controller's state machine can be correct.
>So, we increase one role 'CDNS3_ROLE_END' to for this purpose.

Hi, Tomek do you have any comment for this.
We have in RTL whole OTG machine and we can read all states. 

From otg specification we have in otg_state such bits:
5:3	host_otg_state	"Current state of the OTG Host FSM.
3'b000 : H_IDLE
3'b001 : H_VBUS_ON
3'b010 : H_VBUS_FAILED
3'b011 : H_OTG_HOST_MODE
3'b100 : H_HOST_MODE
3'b101 : H_SWITCH_TO_DEVICE
3'b110 : H_A_SUSPEND
3'b111 : H_WAIT_VBUS_FALL"	RO	0x0
2:0	dev_otg_state	"Current state of the OTG Device FSM.
3'b000 : DEV_IDLE
3'b001 : DEV_MODE
3'b010 : DEV_SRP
3'b011 : DEV_WAIT_VBUS_FALL
3'b100 : DEV_SWITCH_TO_HOST
3'b101 : DEV_WAIT_FOR_CONN"

>
>CDNS3_ROLE_GADGET: gadget mode and VBUS on
>CDNS3_ROLE_HOST: host mode and VBUS on
>CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
>
>So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
>and need to set role as CDNS3_ROLE_END at ->stop for further handling at
>role switch routine.
>

>> > +     mutex_unlock(&cdns->mutex);
>> > +}
>> > +
>> > +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>> > +{
>> > +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>> > +             //TODO: implements selecting device/host mode
>> > +             return CDNS3_ROLE_HOST;
>> > +     }
>> > +     return cdns->roles[CDNS3_ROLE_HOST]
>> > +             ? CDNS3_ROLE_HOST
>> > +             : CDNS3_ROLE_GADGET;

Cheers
Pawel,