Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi,

On Mon, Apr 10, 2017 at 2:07 PM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>>> Hi Ravi,
>>>
>>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
 Hi Kishon,

 On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
 wrote:
> Hi,
>
> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>> SoC. The phy can be configured to be in Device mode or Host
>> mode based on the type of cable connected to the port. The
>> driver registers to  extcon framework to get appropriate
>> connect events for Host/Device cables connect/disconnect
>> states based on VBUS and ID interrupts.
>
> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
> Northstar2.
>

 Will do.

> Sorry for not letting you know this earlier. But I feel the design of the
> driver should be changed. Extcon shouldn't be used here. The extcon
> notifications should be sent to the consumer driver and the consumer 
> driver
> should be responsible for invoking the phy ops.
>

 The consumer drivers here would be a UDC driver (USB device
 controller), EHCI and OHCI host controller drivers.
 I was already suggested in UDC driver review to deal with extcon in Phy 
 driver.

 This phy connects to 2 host controllers, and one device controller.
 That's the design in Broadcom Northstar2
 platform. The values of the VBUS and ID pins of this port are
 determined based on the type of the cable (device cable
 or host cable). And. phy has to be configured accordingly.

> The phy ops being invoked during extcon events doesn't look right.

 Could you please elaborate on the concern, so that we can think of
 mitigating those issues in this driver?
 Since we are dealing with phy init/shutdown in this driver itself, are
 you okay with moving the extcon handling code
 out of phy ops ?
>>>
>>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
>>> from
>>> extcon events too. Can a phy which is initialized by a phy consumer (say 
>>> your
>>> UDC invokes phy_init) can be shutdown by an extcon event?
>>>
>>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>>> will
>>> set using extcon events might help.
>>>
>>
>> Say, we have a USB pendrive which is connected to the DRD port through
>> a host cable.
>> Now the PHY will be initialized to be in host mode.
>> When the pendrive is unplugged, and we now connect the NS2 device to
>> some linux PC,
>> now the PHY has to be shutdown, and re-initialized to be in Device mode.
>>
>> On unplug event, it is set neither to Host nor Device mode (basically
>> shutdown). Next time which ever cable is connected, the PHY is
>> initialized to the respective
>> mode.
>>
>> Please let me know if it's fine to do these initializations outside
>> phy ops, because those will
>> be irrelevant for phy consumers (the controllers) as it's anyways
>> dealt in the phy driver through
>> extcon.
>
> Yes. We shouldn't add phy_ops just for the sake of it. I think this should be
> made as a purely extcon driver (though there are a couple of bits that looks
> like initializing PHY) and keep it in drivers/extcon.
>

Actually phy_ops would be required when we want phy to be shutdown/init
during power management, where USB controllers would call them.

I reworked on this driver to address the concerns raised here. Please check
PATCH v5 that I will submit shortly.
If we want to dynamically change the mode of PHY either to be in host mode
or device mode or idle, we don't have to do a full PHY init/power on (that
earlier required doing a PHY PLL lock and other resets). We just have to
program couple of register bits that are dedicated for this purpose.
I made those changes, now we don't have to call phy ops in the driver.
Phy_ops will be called only by the consumer drivers.

Thanks,
Ravi

> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi,

On Mon, Apr 10, 2017 at 2:07 PM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>>> Hi Ravi,
>>>
>>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
 Hi Kishon,

 On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
 wrote:
> Hi,
>
> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>> SoC. The phy can be configured to be in Device mode or Host
>> mode based on the type of cable connected to the port. The
>> driver registers to  extcon framework to get appropriate
>> connect events for Host/Device cables connect/disconnect
>> states based on VBUS and ID interrupts.
>
> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
> Northstar2.
>

 Will do.

> Sorry for not letting you know this earlier. But I feel the design of the
> driver should be changed. Extcon shouldn't be used here. The extcon
> notifications should be sent to the consumer driver and the consumer 
> driver
> should be responsible for invoking the phy ops.
>

 The consumer drivers here would be a UDC driver (USB device
 controller), EHCI and OHCI host controller drivers.
 I was already suggested in UDC driver review to deal with extcon in Phy 
 driver.

 This phy connects to 2 host controllers, and one device controller.
 That's the design in Broadcom Northstar2
 platform. The values of the VBUS and ID pins of this port are
 determined based on the type of the cable (device cable
 or host cable). And. phy has to be configured accordingly.

> The phy ops being invoked during extcon events doesn't look right.

 Could you please elaborate on the concern, so that we can think of
 mitigating those issues in this driver?
 Since we are dealing with phy init/shutdown in this driver itself, are
 you okay with moving the extcon handling code
 out of phy ops ?
>>>
>>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
>>> from
>>> extcon events too. Can a phy which is initialized by a phy consumer (say 
>>> your
>>> UDC invokes phy_init) can be shutdown by an extcon event?
>>>
>>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>>> will
>>> set using extcon events might help.
>>>
>>
>> Say, we have a USB pendrive which is connected to the DRD port through
>> a host cable.
>> Now the PHY will be initialized to be in host mode.
>> When the pendrive is unplugged, and we now connect the NS2 device to
>> some linux PC,
>> now the PHY has to be shutdown, and re-initialized to be in Device mode.
>>
>> On unplug event, it is set neither to Host nor Device mode (basically
>> shutdown). Next time which ever cable is connected, the PHY is
>> initialized to the respective
>> mode.
>>
>> Please let me know if it's fine to do these initializations outside
>> phy ops, because those will
>> be irrelevant for phy consumers (the controllers) as it's anyways
>> dealt in the phy driver through
>> extcon.
>
> Yes. We shouldn't add phy_ops just for the sake of it. I think this should be
> made as a purely extcon driver (though there are a couple of bits that looks
> like initializing PHY) and keep it in drivers/extcon.
>

Actually phy_ops would be required when we want phy to be shutdown/init
during power management, where USB controllers would call them.

I reworked on this driver to address the concerns raised here. Please check
PATCH v5 that I will submit shortly.
If we want to dynamically change the mode of PHY either to be in host mode
or device mode or idle, we don't have to do a full PHY init/power on (that
earlier required doing a PHY PLL lock and other resets). We just have to
program couple of register bits that are dedicated for this purpose.
I made those changes, now we don't have to call phy ops in the driver.
Phy_ops will be called only by the consumer drivers.

Thanks,
Ravi

> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
> Hi Kishon,
> 
> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>> Hi Ravi,
>>
>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>>> Hi Kishon,
>>>
>>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
>>> wrote:
 Hi,

 On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
> This is driver for USB DRD Phy used in Broadcom's Northstar2
> SoC. The phy can be configured to be in Device mode or Host
> mode based on the type of cable connected to the port. The
> driver registers to  extcon framework to get appropriate
> connect events for Host/Device cables connect/disconnect
> states based on VBUS and ID interrupts.

 $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
 Northstar2.

>>>
>>> Will do.
>>>
 Sorry for not letting you know this earlier. But I feel the design of the
 driver should be changed. Extcon shouldn't be used here. The extcon
 notifications should be sent to the consumer driver and the consumer driver
 should be responsible for invoking the phy ops.

>>>
>>> The consumer drivers here would be a UDC driver (USB device
>>> controller), EHCI and OHCI host controller drivers.
>>> I was already suggested in UDC driver review to deal with extcon in Phy 
>>> driver.
>>>
>>> This phy connects to 2 host controllers, and one device controller.
>>> That's the design in Broadcom Northstar2
>>> platform. The values of the VBUS and ID pins of this port are
>>> determined based on the type of the cable (device cable
>>> or host cable). And. phy has to be configured accordingly.
>>>
 The phy ops being invoked during extcon events doesn't look right.
>>>
>>> Could you please elaborate on the concern, so that we can think of
>>> mitigating those issues in this driver?
>>> Since we are dealing with phy init/shutdown in this driver itself, are
>>> you okay with moving the extcon handling code
>>> out of phy ops ?
>>
>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
>> extcon events too. Can a phy which is initialized by a phy consumer (say your
>> UDC invokes phy_init) can be shutdown by an extcon event?
>>
>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>> will
>> set using extcon events might help.
>>
> 
> Say, we have a USB pendrive which is connected to the DRD port through
> a host cable.
> Now the PHY will be initialized to be in host mode.
> When the pendrive is unplugged, and we now connect the NS2 device to
> some linux PC,
> now the PHY has to be shutdown, and re-initialized to be in Device mode.
> 
> On unplug event, it is set neither to Host nor Device mode (basically
> shutdown). Next time which ever cable is connected, the PHY is
> initialized to the respective
> mode.
> 
> Please let me know if it's fine to do these initializations outside
> phy ops, because those will
> be irrelevant for phy consumers (the controllers) as it's anyways
> dealt in the phy driver through
> extcon.

Yes. We shouldn't add phy_ops just for the sake of it. I think this should be
made as a purely extcon driver (though there are a couple of bits that looks
like initializing PHY) and keep it in drivers/extcon.

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
> Hi Kishon,
> 
> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>> Hi Ravi,
>>
>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>>> Hi Kishon,
>>>
>>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
>>> wrote:
 Hi,

 On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
> This is driver for USB DRD Phy used in Broadcom's Northstar2
> SoC. The phy can be configured to be in Device mode or Host
> mode based on the type of cable connected to the port. The
> driver registers to  extcon framework to get appropriate
> connect events for Host/Device cables connect/disconnect
> states based on VBUS and ID interrupts.

 $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
 Northstar2.

>>>
>>> Will do.
>>>
 Sorry for not letting you know this earlier. But I feel the design of the
 driver should be changed. Extcon shouldn't be used here. The extcon
 notifications should be sent to the consumer driver and the consumer driver
 should be responsible for invoking the phy ops.

>>>
>>> The consumer drivers here would be a UDC driver (USB device
>>> controller), EHCI and OHCI host controller drivers.
>>> I was already suggested in UDC driver review to deal with extcon in Phy 
>>> driver.
>>>
>>> This phy connects to 2 host controllers, and one device controller.
>>> That's the design in Broadcom Northstar2
>>> platform. The values of the VBUS and ID pins of this port are
>>> determined based on the type of the cable (device cable
>>> or host cable). And. phy has to be configured accordingly.
>>>
 The phy ops being invoked during extcon events doesn't look right.
>>>
>>> Could you please elaborate on the concern, so that we can think of
>>> mitigating those issues in this driver?
>>> Since we are dealing with phy init/shutdown in this driver itself, are
>>> you okay with moving the extcon handling code
>>> out of phy ops ?
>>
>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
>> extcon events too. Can a phy which is initialized by a phy consumer (say your
>> UDC invokes phy_init) can be shutdown by an extcon event?
>>
>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>> will
>> set using extcon events might help.
>>
> 
> Say, we have a USB pendrive which is connected to the DRD port through
> a host cable.
> Now the PHY will be initialized to be in host mode.
> When the pendrive is unplugged, and we now connect the NS2 device to
> some linux PC,
> now the PHY has to be shutdown, and re-initialized to be in Device mode.
> 
> On unplug event, it is set neither to Host nor Device mode (basically
> shutdown). Next time which ever cable is connected, the PHY is
> initialized to the respective
> mode.
> 
> Please let me know if it's fine to do these initializations outside
> phy ops, because those will
> be irrelevant for phy consumers (the controllers) as it's anyways
> dealt in the phy driver through
> extcon.

Yes. We shouldn't add phy_ops just for the sake of it. I think this should be
made as a purely extcon driver (though there are a couple of bits that looks
like initializing PHY) and keep it in drivers/extcon.

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi

On Mon, Apr 10, 2017 at 1:57 PM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Monday 10 April 2017 12:57 PM, Raviteja Garimella wrote:
>> Hi,
>>
>> On Mon, Apr 10, 2017 at 10:55 AM, Kishon Vijay Abraham I  
>> wrote:
>>> Hi,
>>>
>>> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
 Hi Kishon,

 On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  
 wrote:
> Hi Ravi,
>
> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
>> wrote:
>>> Hi,
>>>
>>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
 This is driver for USB DRD Phy used in Broadcom's Northstar2
 SoC. The phy can be configured to be in Device mode or Host
 mode based on the type of cable connected to the port. The
 driver registers to  extcon framework to get appropriate
 connect events for Host/Device cables connect/disconnect
 states based on VBUS and ID interrupts.
>>>
>>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for 
>>> Broadcoms
>>> Northstar2.
>>>
>>
>> Will do.
>>
>>> Sorry for not letting you know this earlier. But I feel the design of 
>>> the
>>> driver should be changed. Extcon shouldn't be used here. The extcon
>>> notifications should be sent to the consumer driver and the consumer 
>>> driver
>>> should be responsible for invoking the phy ops.
>>>
>>
>> The consumer drivers here would be a UDC driver (USB device
>> controller), EHCI and OHCI host controller drivers.
>> I was already suggested in UDC driver review to deal with extcon in Phy 
>> driver.
>>
>> This phy connects to 2 host controllers, and one device controller.
>> That's the design in Broadcom Northstar2
>> platform. The values of the VBUS and ID pins of this port are
>> determined based on the type of the cable (device cable
>> or host cable). And. phy has to be configured accordingly.
>>
>>> The phy ops being invoked during extcon events doesn't look right.
>>
>> Could you please elaborate on the concern, so that we can think of
>> mitigating those issues in this driver?
>> Since we are dealing with phy init/shutdown in this driver itself, are
>> you okay with moving the extcon handling code
>> out of phy ops ?
>
> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
> from
> extcon events too. Can a phy which is initialized by a phy consumer (say 
> your
> UDC invokes phy_init) can be shutdown by an extcon event?
>
> Maybe a clear explanation of when phy_ops here will be invoked and when 
> it will
> set using extcon events might help.
>

 Say, we have a USB pendrive which is connected to the DRD port through
 a host cable.
 Now the PHY will be initialized to be in host mode.
 When the pendrive is unplugged, and we now connect the NS2 device to
 some linux PC,
 now the PHY has to be shutdown, and re-initialized to be in Device mode.

 On unplug event, it is set neither to Host nor Device mode (basically
 shutdown). Next time which ever cable is connected, the PHY is
 initialized to the respective
 mode.

 Please let me know if it's fine to do these initializations outside
 phy ops, because those will
 be irrelevant for phy consumers (the controllers) as it's anyways
 dealt in the phy driver through
 extcon.
>>>
>>> How does the consumer get to know whether they have to operate in host mode 
>>> or
>>> device mode?
>>>
>> In NS2, we have host controllers and device controller (not OTG/other
>> that can switch
>> between host and device mode). It's only phy that can be in host/device mode.
>> Since both Host Controllers and Device Controller are connected to the same 
>> PHY,
>> it is based on the extcon logic (the type of cable connected) that PHY
>> will be in one
>> of the modes host/device and the respective controller will operate.
>
> So at a point of time either the host controller or the device controller will
> be active? and the PHY decides which of them should be active? Is that right?
>

Yes

Thanks,
Ravi

> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi

On Mon, Apr 10, 2017 at 1:57 PM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Monday 10 April 2017 12:57 PM, Raviteja Garimella wrote:
>> Hi,
>>
>> On Mon, Apr 10, 2017 at 10:55 AM, Kishon Vijay Abraham I  
>> wrote:
>>> Hi,
>>>
>>> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
 Hi Kishon,

 On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  
 wrote:
> Hi Ravi,
>
> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
>> wrote:
>>> Hi,
>>>
>>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
 This is driver for USB DRD Phy used in Broadcom's Northstar2
 SoC. The phy can be configured to be in Device mode or Host
 mode based on the type of cable connected to the port. The
 driver registers to  extcon framework to get appropriate
 connect events for Host/Device cables connect/disconnect
 states based on VBUS and ID interrupts.
>>>
>>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for 
>>> Broadcoms
>>> Northstar2.
>>>
>>
>> Will do.
>>
>>> Sorry for not letting you know this earlier. But I feel the design of 
>>> the
>>> driver should be changed. Extcon shouldn't be used here. The extcon
>>> notifications should be sent to the consumer driver and the consumer 
>>> driver
>>> should be responsible for invoking the phy ops.
>>>
>>
>> The consumer drivers here would be a UDC driver (USB device
>> controller), EHCI and OHCI host controller drivers.
>> I was already suggested in UDC driver review to deal with extcon in Phy 
>> driver.
>>
>> This phy connects to 2 host controllers, and one device controller.
>> That's the design in Broadcom Northstar2
>> platform. The values of the VBUS and ID pins of this port are
>> determined based on the type of the cable (device cable
>> or host cable). And. phy has to be configured accordingly.
>>
>>> The phy ops being invoked during extcon events doesn't look right.
>>
>> Could you please elaborate on the concern, so that we can think of
>> mitigating those issues in this driver?
>> Since we are dealing with phy init/shutdown in this driver itself, are
>> you okay with moving the extcon handling code
>> out of phy ops ?
>
> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
> from
> extcon events too. Can a phy which is initialized by a phy consumer (say 
> your
> UDC invokes phy_init) can be shutdown by an extcon event?
>
> Maybe a clear explanation of when phy_ops here will be invoked and when 
> it will
> set using extcon events might help.
>

 Say, we have a USB pendrive which is connected to the DRD port through
 a host cable.
 Now the PHY will be initialized to be in host mode.
 When the pendrive is unplugged, and we now connect the NS2 device to
 some linux PC,
 now the PHY has to be shutdown, and re-initialized to be in Device mode.

 On unplug event, it is set neither to Host nor Device mode (basically
 shutdown). Next time which ever cable is connected, the PHY is
 initialized to the respective
 mode.

 Please let me know if it's fine to do these initializations outside
 phy ops, because those will
 be irrelevant for phy consumers (the controllers) as it's anyways
 dealt in the phy driver through
 extcon.
>>>
>>> How does the consumer get to know whether they have to operate in host mode 
>>> or
>>> device mode?
>>>
>> In NS2, we have host controllers and device controller (not OTG/other
>> that can switch
>> between host and device mode). It's only phy that can be in host/device mode.
>> Since both Host Controllers and Device Controller are connected to the same 
>> PHY,
>> it is based on the extcon logic (the type of cable connected) that PHY
>> will be in one
>> of the modes host/device and the respective controller will operate.
>
> So at a point of time either the host controller or the device controller will
> be active? and the PHY decides which of them should be active? Is that right?
>

Yes

Thanks,
Ravi

> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Monday 10 April 2017 12:57 PM, Raviteja Garimella wrote:
> Hi,
> 
> On Mon, Apr 10, 2017 at 10:55 AM, Kishon Vijay Abraham I  
> wrote:
>> Hi,
>>
>> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
>>> Hi Kishon,
>>>
>>> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  
>>> wrote:
 Hi Ravi,

 On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
> Hi Kishon,
>
> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
> wrote:
>> Hi,
>>
>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>>> SoC. The phy can be configured to be in Device mode or Host
>>> mode based on the type of cable connected to the port. The
>>> driver registers to  extcon framework to get appropriate
>>> connect events for Host/Device cables connect/disconnect
>>> states based on VBUS and ID interrupts.
>>
>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for 
>> Broadcoms
>> Northstar2.
>>
>
> Will do.
>
>> Sorry for not letting you know this earlier. But I feel the design of the
>> driver should be changed. Extcon shouldn't be used here. The extcon
>> notifications should be sent to the consumer driver and the consumer 
>> driver
>> should be responsible for invoking the phy ops.
>>
>
> The consumer drivers here would be a UDC driver (USB device
> controller), EHCI and OHCI host controller drivers.
> I was already suggested in UDC driver review to deal with extcon in Phy 
> driver.
>
> This phy connects to 2 host controllers, and one device controller.
> That's the design in Broadcom Northstar2
> platform. The values of the VBUS and ID pins of this port are
> determined based on the type of the cable (device cable
> or host cable). And. phy has to be configured accordingly.
>
>> The phy ops being invoked during extcon events doesn't look right.
>
> Could you please elaborate on the concern, so that we can think of
> mitigating those issues in this driver?
> Since we are dealing with phy init/shutdown in this driver itself, are
> you okay with moving the extcon handling code
> out of phy ops ?

 yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
 from
 extcon events too. Can a phy which is initialized by a phy consumer (say 
 your
 UDC invokes phy_init) can be shutdown by an extcon event?

 Maybe a clear explanation of when phy_ops here will be invoked and when it 
 will
 set using extcon events might help.

>>>
>>> Say, we have a USB pendrive which is connected to the DRD port through
>>> a host cable.
>>> Now the PHY will be initialized to be in host mode.
>>> When the pendrive is unplugged, and we now connect the NS2 device to
>>> some linux PC,
>>> now the PHY has to be shutdown, and re-initialized to be in Device mode.
>>>
>>> On unplug event, it is set neither to Host nor Device mode (basically
>>> shutdown). Next time which ever cable is connected, the PHY is
>>> initialized to the respective
>>> mode.
>>>
>>> Please let me know if it's fine to do these initializations outside
>>> phy ops, because those will
>>> be irrelevant for phy consumers (the controllers) as it's anyways
>>> dealt in the phy driver through
>>> extcon.
>>
>> How does the consumer get to know whether they have to operate in host mode 
>> or
>> device mode?
>>
> In NS2, we have host controllers and device controller (not OTG/other
> that can switch
> between host and device mode). It's only phy that can be in host/device mode.
> Since both Host Controllers and Device Controller are connected to the same 
> PHY,
> it is based on the extcon logic (the type of cable connected) that PHY
> will be in one
> of the modes host/device and the respective controller will operate.

So at a point of time either the host controller or the device controller will
be active? and the PHY decides which of them should be active? Is that right?

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Monday 10 April 2017 12:57 PM, Raviteja Garimella wrote:
> Hi,
> 
> On Mon, Apr 10, 2017 at 10:55 AM, Kishon Vijay Abraham I  
> wrote:
>> Hi,
>>
>> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
>>> Hi Kishon,
>>>
>>> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  
>>> wrote:
 Hi Ravi,

 On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
> Hi Kishon,
>
> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
> wrote:
>> Hi,
>>
>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>>> SoC. The phy can be configured to be in Device mode or Host
>>> mode based on the type of cable connected to the port. The
>>> driver registers to  extcon framework to get appropriate
>>> connect events for Host/Device cables connect/disconnect
>>> states based on VBUS and ID interrupts.
>>
>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for 
>> Broadcoms
>> Northstar2.
>>
>
> Will do.
>
>> Sorry for not letting you know this earlier. But I feel the design of the
>> driver should be changed. Extcon shouldn't be used here. The extcon
>> notifications should be sent to the consumer driver and the consumer 
>> driver
>> should be responsible for invoking the phy ops.
>>
>
> The consumer drivers here would be a UDC driver (USB device
> controller), EHCI and OHCI host controller drivers.
> I was already suggested in UDC driver review to deal with extcon in Phy 
> driver.
>
> This phy connects to 2 host controllers, and one device controller.
> That's the design in Broadcom Northstar2
> platform. The values of the VBUS and ID pins of this port are
> determined based on the type of the cable (device cable
> or host cable). And. phy has to be configured accordingly.
>
>> The phy ops being invoked during extcon events doesn't look right.
>
> Could you please elaborate on the concern, so that we can think of
> mitigating those issues in this driver?
> Since we are dealing with phy init/shutdown in this driver itself, are
> you okay with moving the extcon handling code
> out of phy ops ?

 yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
 from
 extcon events too. Can a phy which is initialized by a phy consumer (say 
 your
 UDC invokes phy_init) can be shutdown by an extcon event?

 Maybe a clear explanation of when phy_ops here will be invoked and when it 
 will
 set using extcon events might help.

>>>
>>> Say, we have a USB pendrive which is connected to the DRD port through
>>> a host cable.
>>> Now the PHY will be initialized to be in host mode.
>>> When the pendrive is unplugged, and we now connect the NS2 device to
>>> some linux PC,
>>> now the PHY has to be shutdown, and re-initialized to be in Device mode.
>>>
>>> On unplug event, it is set neither to Host nor Device mode (basically
>>> shutdown). Next time which ever cable is connected, the PHY is
>>> initialized to the respective
>>> mode.
>>>
>>> Please let me know if it's fine to do these initializations outside
>>> phy ops, because those will
>>> be irrelevant for phy consumers (the controllers) as it's anyways
>>> dealt in the phy driver through
>>> extcon.
>>
>> How does the consumer get to know whether they have to operate in host mode 
>> or
>> device mode?
>>
> In NS2, we have host controllers and device controller (not OTG/other
> that can switch
> between host and device mode). It's only phy that can be in host/device mode.
> Since both Host Controllers and Device Controller are connected to the same 
> PHY,
> it is based on the extcon logic (the type of cable connected) that PHY
> will be in one
> of the modes host/device and the respective controller will operate.

So at a point of time either the host controller or the device controller will
be active? and the PHY decides which of them should be active? Is that right?

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi,

On Mon, Apr 10, 2017 at 10:55 AM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>>> Hi Ravi,
>>>
>>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
 Hi Kishon,

 On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
 wrote:
> Hi,
>
> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>> SoC. The phy can be configured to be in Device mode or Host
>> mode based on the type of cable connected to the port. The
>> driver registers to  extcon framework to get appropriate
>> connect events for Host/Device cables connect/disconnect
>> states based on VBUS and ID interrupts.
>
> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
> Northstar2.
>

 Will do.

> Sorry for not letting you know this earlier. But I feel the design of the
> driver should be changed. Extcon shouldn't be used here. The extcon
> notifications should be sent to the consumer driver and the consumer 
> driver
> should be responsible for invoking the phy ops.
>

 The consumer drivers here would be a UDC driver (USB device
 controller), EHCI and OHCI host controller drivers.
 I was already suggested in UDC driver review to deal with extcon in Phy 
 driver.

 This phy connects to 2 host controllers, and one device controller.
 That's the design in Broadcom Northstar2
 platform. The values of the VBUS and ID pins of this port are
 determined based on the type of the cable (device cable
 or host cable). And. phy has to be configured accordingly.

> The phy ops being invoked during extcon events doesn't look right.

 Could you please elaborate on the concern, so that we can think of
 mitigating those issues in this driver?
 Since we are dealing with phy init/shutdown in this driver itself, are
 you okay with moving the extcon handling code
 out of phy ops ?
>>>
>>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
>>> from
>>> extcon events too. Can a phy which is initialized by a phy consumer (say 
>>> your
>>> UDC invokes phy_init) can be shutdown by an extcon event?
>>>
>>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>>> will
>>> set using extcon events might help.
>>>
>>
>> Say, we have a USB pendrive which is connected to the DRD port through
>> a host cable.
>> Now the PHY will be initialized to be in host mode.
>> When the pendrive is unplugged, and we now connect the NS2 device to
>> some linux PC,
>> now the PHY has to be shutdown, and re-initialized to be in Device mode.
>>
>> On unplug event, it is set neither to Host nor Device mode (basically
>> shutdown). Next time which ever cable is connected, the PHY is
>> initialized to the respective
>> mode.
>>
>> Please let me know if it's fine to do these initializations outside
>> phy ops, because those will
>> be irrelevant for phy consumers (the controllers) as it's anyways
>> dealt in the phy driver through
>> extcon.
>
> How does the consumer get to know whether they have to operate in host mode or
> device mode?
>
In NS2, we have host controllers and device controller (not OTG/other
that can switch
between host and device mode). It's only phy that can be in host/device mode.
Since both Host Controllers and Device Controller are connected to the same PHY,
it is based on the extcon logic (the type of cable connected) that PHY
will be in one
of the modes host/device and the respective controller will operate.

> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi,

On Mon, Apr 10, 2017 at 10:55 AM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>>> Hi Ravi,
>>>
>>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
 Hi Kishon,

 On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
 wrote:
> Hi,
>
> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>> SoC. The phy can be configured to be in Device mode or Host
>> mode based on the type of cable connected to the port. The
>> driver registers to  extcon framework to get appropriate
>> connect events for Host/Device cables connect/disconnect
>> states based on VBUS and ID interrupts.
>
> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
> Northstar2.
>

 Will do.

> Sorry for not letting you know this earlier. But I feel the design of the
> driver should be changed. Extcon shouldn't be used here. The extcon
> notifications should be sent to the consumer driver and the consumer 
> driver
> should be responsible for invoking the phy ops.
>

 The consumer drivers here would be a UDC driver (USB device
 controller), EHCI and OHCI host controller drivers.
 I was already suggested in UDC driver review to deal with extcon in Phy 
 driver.

 This phy connects to 2 host controllers, and one device controller.
 That's the design in Broadcom Northstar2
 platform. The values of the VBUS and ID pins of this port are
 determined based on the type of the cable (device cable
 or host cable). And. phy has to be configured accordingly.

> The phy ops being invoked during extcon events doesn't look right.

 Could you please elaborate on the concern, so that we can think of
 mitigating those issues in this driver?
 Since we are dealing with phy init/shutdown in this driver itself, are
 you okay with moving the extcon handling code
 out of phy ops ?
>>>
>>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked 
>>> from
>>> extcon events too. Can a phy which is initialized by a phy consumer (say 
>>> your
>>> UDC invokes phy_init) can be shutdown by an extcon event?
>>>
>>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>>> will
>>> set using extcon events might help.
>>>
>>
>> Say, we have a USB pendrive which is connected to the DRD port through
>> a host cable.
>> Now the PHY will be initialized to be in host mode.
>> When the pendrive is unplugged, and we now connect the NS2 device to
>> some linux PC,
>> now the PHY has to be shutdown, and re-initialized to be in Device mode.
>>
>> On unplug event, it is set neither to Host nor Device mode (basically
>> shutdown). Next time which ever cable is connected, the PHY is
>> initialized to the respective
>> mode.
>>
>> Please let me know if it's fine to do these initializations outside
>> phy ops, because those will
>> be irrelevant for phy consumers (the controllers) as it's anyways
>> dealt in the phy driver through
>> extcon.
>
> How does the consumer get to know whether they have to operate in host mode or
> device mode?
>
In NS2, we have host controllers and device controller (not OTG/other
that can switch
between host and device mode). It's only phy that can be in host/device mode.
Since both Host Controllers and Device Controller are connected to the same PHY,
it is based on the extcon logic (the type of cable connected) that PHY
will be in one
of the modes host/device and the respective controller will operate.

> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
> Hi Kishon,
> 
> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>> Hi Ravi,
>>
>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>>> Hi Kishon,
>>>
>>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
>>> wrote:
 Hi,

 On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
> This is driver for USB DRD Phy used in Broadcom's Northstar2
> SoC. The phy can be configured to be in Device mode or Host
> mode based on the type of cable connected to the port. The
> driver registers to  extcon framework to get appropriate
> connect events for Host/Device cables connect/disconnect
> states based on VBUS and ID interrupts.

 $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
 Northstar2.

>>>
>>> Will do.
>>>
 Sorry for not letting you know this earlier. But I feel the design of the
 driver should be changed. Extcon shouldn't be used here. The extcon
 notifications should be sent to the consumer driver and the consumer driver
 should be responsible for invoking the phy ops.

>>>
>>> The consumer drivers here would be a UDC driver (USB device
>>> controller), EHCI and OHCI host controller drivers.
>>> I was already suggested in UDC driver review to deal with extcon in Phy 
>>> driver.
>>>
>>> This phy connects to 2 host controllers, and one device controller.
>>> That's the design in Broadcom Northstar2
>>> platform. The values of the VBUS and ID pins of this port are
>>> determined based on the type of the cable (device cable
>>> or host cable). And. phy has to be configured accordingly.
>>>
 The phy ops being invoked during extcon events doesn't look right.
>>>
>>> Could you please elaborate on the concern, so that we can think of
>>> mitigating those issues in this driver?
>>> Since we are dealing with phy init/shutdown in this driver itself, are
>>> you okay with moving the extcon handling code
>>> out of phy ops ?
>>
>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
>> extcon events too. Can a phy which is initialized by a phy consumer (say your
>> UDC invokes phy_init) can be shutdown by an extcon event?
>>
>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>> will
>> set using extcon events might help.
>>
> 
> Say, we have a USB pendrive which is connected to the DRD port through
> a host cable.
> Now the PHY will be initialized to be in host mode.
> When the pendrive is unplugged, and we now connect the NS2 device to
> some linux PC,
> now the PHY has to be shutdown, and re-initialized to be in Device mode.
> 
> On unplug event, it is set neither to Host nor Device mode (basically
> shutdown). Next time which ever cable is connected, the PHY is
> initialized to the respective
> mode.
> 
> Please let me know if it's fine to do these initializations outside
> phy ops, because those will
> be irrelevant for phy consumers (the controllers) as it's anyways
> dealt in the phy driver through
> extcon.

How does the consumer get to know whether they have to operate in host mode or
device mode?

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Wednesday 05 April 2017 07:40 PM, Raviteja Garimella wrote:
> Hi Kishon,
> 
> On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
>> Hi Ravi,
>>
>> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>>> Hi Kishon,
>>>
>>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  
>>> wrote:
 Hi,

 On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
> This is driver for USB DRD Phy used in Broadcom's Northstar2
> SoC. The phy can be configured to be in Device mode or Host
> mode based on the type of cable connected to the port. The
> driver registers to  extcon framework to get appropriate
> connect events for Host/Device cables connect/disconnect
> states based on VBUS and ID interrupts.

 $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
 Northstar2.

>>>
>>> Will do.
>>>
 Sorry for not letting you know this earlier. But I feel the design of the
 driver should be changed. Extcon shouldn't be used here. The extcon
 notifications should be sent to the consumer driver and the consumer driver
 should be responsible for invoking the phy ops.

>>>
>>> The consumer drivers here would be a UDC driver (USB device
>>> controller), EHCI and OHCI host controller drivers.
>>> I was already suggested in UDC driver review to deal with extcon in Phy 
>>> driver.
>>>
>>> This phy connects to 2 host controllers, and one device controller.
>>> That's the design in Broadcom Northstar2
>>> platform. The values of the VBUS and ID pins of this port are
>>> determined based on the type of the cable (device cable
>>> or host cable). And. phy has to be configured accordingly.
>>>
 The phy ops being invoked during extcon events doesn't look right.
>>>
>>> Could you please elaborate on the concern, so that we can think of
>>> mitigating those issues in this driver?
>>> Since we are dealing with phy init/shutdown in this driver itself, are
>>> you okay with moving the extcon handling code
>>> out of phy ops ?
>>
>> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
>> extcon events too. Can a phy which is initialized by a phy consumer (say your
>> UDC invokes phy_init) can be shutdown by an extcon event?
>>
>> Maybe a clear explanation of when phy_ops here will be invoked and when it 
>> will
>> set using extcon events might help.
>>
> 
> Say, we have a USB pendrive which is connected to the DRD port through
> a host cable.
> Now the PHY will be initialized to be in host mode.
> When the pendrive is unplugged, and we now connect the NS2 device to
> some linux PC,
> now the PHY has to be shutdown, and re-initialized to be in Device mode.
> 
> On unplug event, it is set neither to Host nor Device mode (basically
> shutdown). Next time which ever cable is connected, the PHY is
> initialized to the respective
> mode.
> 
> Please let me know if it's fine to do these initializations outside
> phy ops, because those will
> be irrelevant for phy consumers (the controllers) as it's anyways
> dealt in the phy driver through
> extcon.

How does the consumer get to know whether they have to operate in host mode or
device mode?

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi Kishon,

On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
> Hi Ravi,
>
> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  wrote:
>>> Hi,
>>>
>>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
 This is driver for USB DRD Phy used in Broadcom's Northstar2
 SoC. The phy can be configured to be in Device mode or Host
 mode based on the type of cable connected to the port. The
 driver registers to  extcon framework to get appropriate
 connect events for Host/Device cables connect/disconnect
 states based on VBUS and ID interrupts.
>>>
>>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
>>> Northstar2.
>>>
>>
>> Will do.
>>
>>> Sorry for not letting you know this earlier. But I feel the design of the
>>> driver should be changed. Extcon shouldn't be used here. The extcon
>>> notifications should be sent to the consumer driver and the consumer driver
>>> should be responsible for invoking the phy ops.
>>>
>>
>> The consumer drivers here would be a UDC driver (USB device
>> controller), EHCI and OHCI host controller drivers.
>> I was already suggested in UDC driver review to deal with extcon in Phy 
>> driver.
>>
>> This phy connects to 2 host controllers, and one device controller.
>> That's the design in Broadcom Northstar2
>> platform. The values of the VBUS and ID pins of this port are
>> determined based on the type of the cable (device cable
>> or host cable). And. phy has to be configured accordingly.
>>
>>> The phy ops being invoked during extcon events doesn't look right.
>>
>> Could you please elaborate on the concern, so that we can think of
>> mitigating those issues in this driver?
>> Since we are dealing with phy init/shutdown in this driver itself, are
>> you okay with moving the extcon handling code
>> out of phy ops ?
>
> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
> extcon events too. Can a phy which is initialized by a phy consumer (say your
> UDC invokes phy_init) can be shutdown by an extcon event?
>
> Maybe a clear explanation of when phy_ops here will be invoked and when it 
> will
> set using extcon events might help.
>

Say, we have a USB pendrive which is connected to the DRD port through
a host cable.
Now the PHY will be initialized to be in host mode.
When the pendrive is unplugged, and we now connect the NS2 device to
some linux PC,
now the PHY has to be shutdown, and re-initialized to be in Device mode.

On unplug event, it is set neither to Host nor Device mode (basically
shutdown). Next time which ever cable is connected, the PHY is
initialized to the respective
mode.

Please let me know if it's fine to do these initializations outside
phy ops, because those will
be irrelevant for phy consumers (the controllers) as it's anyways
dealt in the phy driver through
extcon.

Thanks,
Ravi


> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi Kishon,

On Wed, Apr 5, 2017 at 7:04 PM, Kishon Vijay Abraham I  wrote:
> Hi Ravi,
>
> On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
>> Hi Kishon,
>>
>> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  wrote:
>>> Hi,
>>>
>>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
 This is driver for USB DRD Phy used in Broadcom's Northstar2
 SoC. The phy can be configured to be in Device mode or Host
 mode based on the type of cable connected to the port. The
 driver registers to  extcon framework to get appropriate
 connect events for Host/Device cables connect/disconnect
 states based on VBUS and ID interrupts.
>>>
>>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
>>> Northstar2.
>>>
>>
>> Will do.
>>
>>> Sorry for not letting you know this earlier. But I feel the design of the
>>> driver should be changed. Extcon shouldn't be used here. The extcon
>>> notifications should be sent to the consumer driver and the consumer driver
>>> should be responsible for invoking the phy ops.
>>>
>>
>> The consumer drivers here would be a UDC driver (USB device
>> controller), EHCI and OHCI host controller drivers.
>> I was already suggested in UDC driver review to deal with extcon in Phy 
>> driver.
>>
>> This phy connects to 2 host controllers, and one device controller.
>> That's the design in Broadcom Northstar2
>> platform. The values of the VBUS and ID pins of this port are
>> determined based on the type of the cable (device cable
>> or host cable). And. phy has to be configured accordingly.
>>
>>> The phy ops being invoked during extcon events doesn't look right.
>>
>> Could you please elaborate on the concern, so that we can think of
>> mitigating those issues in this driver?
>> Since we are dealing with phy init/shutdown in this driver itself, are
>> you okay with moving the extcon handling code
>> out of phy ops ?
>
> yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
> extcon events too. Can a phy which is initialized by a phy consumer (say your
> UDC invokes phy_init) can be shutdown by an extcon event?
>
> Maybe a clear explanation of when phy_ops here will be invoked and when it 
> will
> set using extcon events might help.
>

Say, we have a USB pendrive which is connected to the DRD port through
a host cable.
Now the PHY will be initialized to be in host mode.
When the pendrive is unplugged, and we now connect the NS2 device to
some linux PC,
now the PHY has to be shutdown, and re-initialized to be in Device mode.

On unplug event, it is set neither to Host nor Device mode (basically
shutdown). Next time which ever cable is connected, the PHY is
initialized to the respective
mode.

Please let me know if it's fine to do these initializations outside
phy ops, because those will
be irrelevant for phy consumers (the controllers) as it's anyways
dealt in the phy driver through
extcon.

Thanks,
Ravi


> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi Ravi,

On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
> Hi Kishon,
> 
> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  wrote:
>> Hi,
>>
>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>>> SoC. The phy can be configured to be in Device mode or Host
>>> mode based on the type of cable connected to the port. The
>>> driver registers to  extcon framework to get appropriate
>>> connect events for Host/Device cables connect/disconnect
>>> states based on VBUS and ID interrupts.
>>
>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
>> Northstar2.
>>
> 
> Will do.
> 
>> Sorry for not letting you know this earlier. But I feel the design of the
>> driver should be changed. Extcon shouldn't be used here. The extcon
>> notifications should be sent to the consumer driver and the consumer driver
>> should be responsible for invoking the phy ops.
>>
> 
> The consumer drivers here would be a UDC driver (USB device
> controller), EHCI and OHCI host controller drivers.
> I was already suggested in UDC driver review to deal with extcon in Phy 
> driver.
> 
> This phy connects to 2 host controllers, and one device controller.
> That's the design in Broadcom Northstar2
> platform. The values of the VBUS and ID pins of this port are
> determined based on the type of the cable (device cable
> or host cable). And. phy has to be configured accordingly.
> 
>> The phy ops being invoked during extcon events doesn't look right.
> 
> Could you please elaborate on the concern, so that we can think of
> mitigating those issues in this driver?
> Since we are dealing with phy init/shutdown in this driver itself, are
> you okay with moving the extcon handling code
> out of phy ops ?

yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
extcon events too. Can a phy which is initialized by a phy consumer (say your
UDC invokes phy_init) can be shutdown by an extcon event?

Maybe a clear explanation of when phy_ops here will be invoked and when it will
set using extcon events might help.

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi Ravi,

On Wednesday 05 April 2017 06:30 PM, Raviteja Garimella wrote:
> Hi Kishon,
> 
> On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  wrote:
>> Hi,
>>
>> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>>> SoC. The phy can be configured to be in Device mode or Host
>>> mode based on the type of cable connected to the port. The
>>> driver registers to  extcon framework to get appropriate
>>> connect events for Host/Device cables connect/disconnect
>>> states based on VBUS and ID interrupts.
>>
>> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
>> Northstar2.
>>
> 
> Will do.
> 
>> Sorry for not letting you know this earlier. But I feel the design of the
>> driver should be changed. Extcon shouldn't be used here. The extcon
>> notifications should be sent to the consumer driver and the consumer driver
>> should be responsible for invoking the phy ops.
>>
> 
> The consumer drivers here would be a UDC driver (USB device
> controller), EHCI and OHCI host controller drivers.
> I was already suggested in UDC driver review to deal with extcon in Phy 
> driver.
> 
> This phy connects to 2 host controllers, and one device controller.
> That's the design in Broadcom Northstar2
> platform. The values of the VBUS and ID pins of this port are
> determined based on the type of the cable (device cable
> or host cable). And. phy has to be configured accordingly.
> 
>> The phy ops being invoked during extcon events doesn't look right.
> 
> Could you please elaborate on the concern, so that we can think of
> mitigating those issues in this driver?
> Since we are dealing with phy init/shutdown in this driver itself, are
> you okay with moving the extcon handling code
> out of phy ops ?

yeah, For e.g., ns2_drd_phy_init is part of phy_ops and is being invoked from
extcon events too. Can a phy which is initialized by a phy consumer (say your
UDC invokes phy_init) can be shutdown by an extcon event?

Maybe a clear explanation of when phy_ops here will be invoked and when it will
set using extcon events might help.

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi Kishon,

On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>> SoC. The phy can be configured to be in Device mode or Host
>> mode based on the type of cable connected to the port. The
>> driver registers to  extcon framework to get appropriate
>> connect events for Host/Device cables connect/disconnect
>> states based on VBUS and ID interrupts.
>
> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
> Northstar2.
>

Will do.

> Sorry for not letting you know this earlier. But I feel the design of the
> driver should be changed. Extcon shouldn't be used here. The extcon
> notifications should be sent to the consumer driver and the consumer driver
> should be responsible for invoking the phy ops.
>

The consumer drivers here would be a UDC driver (USB device
controller), EHCI and OHCI host controller drivers.
I was already suggested in UDC driver review to deal with extcon in Phy driver.

This phy connects to 2 host controllers, and one device controller.
That's the design in Broadcom Northstar2
platform. The values of the VBUS and ID pins of this port are
determined based on the type of the cable (device cable
or host cable). And. phy has to be configured accordingly.

> The phy ops being invoked during extcon events doesn't look right.

Could you please elaborate on the concern, so that we can think of
mitigating those issues in this driver?
Since we are dealing with phy init/shutdown in this driver itself, are
you okay with moving the extcon handling code
out of phy ops ?

Thanks,
Ravi
>
> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

Hi Kishon,

On Wed, Apr 5, 2017 at 4:30 PM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
>> This is driver for USB DRD Phy used in Broadcom's Northstar2
>> SoC. The phy can be configured to be in Device mode or Host
>> mode based on the type of cable connected to the port. The
>> driver registers to  extcon framework to get appropriate
>> connect events for Host/Device cables connect/disconnect
>> states based on VBUS and ID interrupts.
>
> $patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
> Northstar2.
>

Will do.

> Sorry for not letting you know this earlier. But I feel the design of the
> driver should be changed. Extcon shouldn't be used here. The extcon
> notifications should be sent to the consumer driver and the consumer driver
> should be responsible for invoking the phy ops.
>

The consumer drivers here would be a UDC driver (USB device
controller), EHCI and OHCI host controller drivers.
I was already suggested in UDC driver review to deal with extcon in Phy driver.

This phy connects to 2 host controllers, and one device controller.
That's the design in Broadcom Northstar2
platform. The values of the VBUS and ID pins of this port are
determined based on the type of the cable (device cable
or host cable). And. phy has to be configured accordingly.

> The phy ops being invoked during extcon events doesn't look right.

Could you please elaborate on the concern, so that we can think of
mitigating those issues in this driver?
Since we are dealing with phy init/shutdown in this driver itself, are
you okay with moving the extcon handling code
out of phy ops ?

Thanks,
Ravi
>
> Thanks
> Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
> This is driver for USB DRD Phy used in Broadcom's Northstar2
> SoC. The phy can be configured to be in Device mode or Host
> mode based on the type of cable connected to the port. The
> driver registers to  extcon framework to get appropriate
> connect events for Host/Device cables connect/disconnect
> states based on VBUS and ID interrupts.

$patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
Northstar2.

Sorry for not letting you know this earlier. But I feel the design of the
driver should be changed. Extcon shouldn't be used here. The extcon
notifications should be sent to the consumer driver and the consumer driver
should be responsible for invoking the phy ops.

The phy ops being invoked during extcon events doesn't look right.

Thanks
Kishon

Re: [PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Kishon Vijay Abraham I]

Hi,

On Tuesday 28 March 2017 05:57 PM, Raviteja Garimella wrote:
> This is driver for USB DRD Phy used in Broadcom's Northstar2
> SoC. The phy can be configured to be in Device mode or Host
> mode based on the type of cable connected to the port. The
> driver registers to  extcon framework to get appropriate
> connect events for Host/Device cables connect/disconnect
> states based on VBUS and ID interrupts.

$patch should be phy: phy-bcm-ns2-usbdrd: USB DRD Phy driver for Broadcoms
Northstar2.

Sorry for not letting you know this earlier. But I feel the design of the
driver should be changed. Extcon shouldn't be used here. The extcon
notifications should be sent to the consumer driver and the consumer driver
should be responsible for invoking the phy ops.

The phy ops being invoked during extcon events doesn't look right.

Thanks
Kishon

[PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

This is driver for USB DRD Phy used in Broadcom's Northstar2
SoC. The phy can be configured to be in Device mode or Host
mode based on the type of cable connected to the port. The
driver registers to  extcon framework to get appropriate
connect events for Host/Device cables connect/disconnect
states based on VBUS and ID interrupts.

Signed-off-by: Raviteja Garimella 
---
 drivers/phy/Kconfig  |  13 +
 drivers/phy/Makefile |   1 +
 drivers/phy/phy-bcm-ns2-usbdrd.c | 567 +++
 3 files changed, 581 insertions(+)
 create mode 100644 drivers/phy/phy-bcm-ns2-usbdrd.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index dc5277a..001bc33 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -487,6 +487,19 @@ config PHY_CYGNUS_PCIE
  Enable this to support the Broadcom Cygnus PCIe PHY.
  If unsure, say N.
 
+config PHY_NS2_USB_DRD
+   tristate "Broadcom Northstar2 USB DRD PHY support"
+   depends on OF && (ARCH_BCM_IPROC || COMPILE_TEST)
+   select GENERIC_PHY
+   select EXTCON
+   default ARCH_BCM_IPROC
+   help
+ Enable this to support the Broadcom Northstar2 USB DRD PHY.
+ This driver initializes the PHY in either HOST or DEVICE mode.
+ The host or device configuration is read from device tree.
+
+ If unsure, say N.
+
 source "drivers/phy/tegra/Kconfig"
 
 config PHY_NS2_PCIE
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index e7b0feb..122eee6 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_PHY_TUSB1210)+= phy-tusb1210.o
 obj-$(CONFIG_PHY_BRCM_SATA)+= phy-brcm-sata.o
 obj-$(CONFIG_PHY_PISTACHIO_USB)+= phy-pistachio-usb.o
 obj-$(CONFIG_PHY_CYGNUS_PCIE)  += phy-bcm-cygnus-pcie.o
+obj-$(CONFIG_PHY_NS2_USB_DRD)  += phy-bcm-ns2-usbdrd.o
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
 obj-$(CONFIG_PHY_NS2_PCIE) += phy-bcm-ns2-pcie.o
 obj-$(CONFIG_PHY_MESON8B_USB2) += phy-meson8b-usb2.o
diff --git a/drivers/phy/phy-bcm-ns2-usbdrd.c b/drivers/phy/phy-bcm-ns2-usbdrd.c
new file mode 100644
index 000..f41763f
--- /dev/null
+++ b/drivers/phy/phy-bcm-ns2-usbdrd.c
@@ -0,0 +1,567 @@
+/*
+ * Copyright (C) 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define ICFG_DRD_AFE   0x0
+#define ICFG_MISC_STAT 0x18
+#define ICFG_DRD_P0CTL 0x1C
+#define ICFG_STRAP_CTRL0x20
+#define ICFG_FSM_CTRL  0x24
+
+#define IDM_RST_BITBIT(0)
+#define AFE_CORERDY_VDDC   BIT(18)
+#define PHY_PLL_RESETB BIT(15)
+#define PHY_RESETB BIT(14)
+#define PHY_PLL_LOCK   BIT(0)
+
+#define DRD_DEV_MODE   BIT(20)
+#define OHCI_OVRCUR_POLBIT(11)
+#define ICFG_OFF_MODE  BIT(6)
+#define PLL_LOCK_RETRY 1000
+
+#define EVT_DEVICE 0
+#define EVT_HOST   1
+#define EVT_IDLE   2
+
+#define DRD_HOST_MODE  (BIT(2) | BIT(3))
+#define DRD_DEVICE_MODE(BIT(4) | BIT(5))
+#define DRD_HOST_VAL   0x803
+#define DRD_DEV_VAL0x807
+#define GPIO_DELAY 20
+#define PHY_WQ_DELAY   msecs_to_jiffies(600)
+
+struct ns2_phy_data;
+struct ns2_phy_driver {
+   void __iomem *icfgdrd_regs;
+   void __iomem *idmdrd_rst_ctrl;
+   void __iomem *crmu_usb2_ctrl;
+   void __iomem *usb2h_strap_reg;
+   spinlock_t lock; /* spin lock for phy driver */
+   struct ns2_phy_data *data;
+   struct extcon_specific_cable_nb extcon_dev;
+   struct extcon_specific_cable_nb extcon_host;
+   struct notifier_block host_nb;
+   struct notifier_block dev_nb;
+   struct delayed_work conn_work;
+   struct extcon_dev *edev;
+   struct gpio_desc *vbus_gpiod;
+   struct gpio_desc *id_gpiod;
+   int id_irq;
+   int vbus_irq;
+   unsigned long debounce_jiffies;
+   struct delayed_work wq_extcon;
+};
+
+struct ns2_phy_data {
+   struct ns2_phy_driver *driver;
+   struct phy *phy;
+   int new_state;
+};
+
+static const unsigned int usb_extcon_cable[] = {
+   EXTCON_USB,
+   EXTCON_USB_HOST,
+   EXTCON_NONE,
+};
+
+static inline int pll_lock_stat(u32 usb_reg, int reg_mask,
+  

[PATCH v4 2/3] Broadcom USB DRD Phy driver for Northstar2

[Raviteja Garimella]

This is driver for USB DRD Phy used in Broadcom's Northstar2
SoC. The phy can be configured to be in Device mode or Host
mode based on the type of cable connected to the port. The
driver registers to  extcon framework to get appropriate
connect events for Host/Device cables connect/disconnect
states based on VBUS and ID interrupts.

Signed-off-by: Raviteja Garimella 
---
 drivers/phy/Kconfig  |  13 +
 drivers/phy/Makefile |   1 +
 drivers/phy/phy-bcm-ns2-usbdrd.c | 567 +++
 3 files changed, 581 insertions(+)
 create mode 100644 drivers/phy/phy-bcm-ns2-usbdrd.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index dc5277a..001bc33 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -487,6 +487,19 @@ config PHY_CYGNUS_PCIE
  Enable this to support the Broadcom Cygnus PCIe PHY.
  If unsure, say N.
 
+config PHY_NS2_USB_DRD
+   tristate "Broadcom Northstar2 USB DRD PHY support"
+   depends on OF && (ARCH_BCM_IPROC || COMPILE_TEST)
+   select GENERIC_PHY
+   select EXTCON
+   default ARCH_BCM_IPROC
+   help
+ Enable this to support the Broadcom Northstar2 USB DRD PHY.
+ This driver initializes the PHY in either HOST or DEVICE mode.
+ The host or device configuration is read from device tree.
+
+ If unsure, say N.
+
 source "drivers/phy/tegra/Kconfig"
 
 config PHY_NS2_PCIE
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index e7b0feb..122eee6 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_PHY_TUSB1210)+= phy-tusb1210.o
 obj-$(CONFIG_PHY_BRCM_SATA)+= phy-brcm-sata.o
 obj-$(CONFIG_PHY_PISTACHIO_USB)+= phy-pistachio-usb.o
 obj-$(CONFIG_PHY_CYGNUS_PCIE)  += phy-bcm-cygnus-pcie.o
+obj-$(CONFIG_PHY_NS2_USB_DRD)  += phy-bcm-ns2-usbdrd.o
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
 obj-$(CONFIG_PHY_NS2_PCIE) += phy-bcm-ns2-pcie.o
 obj-$(CONFIG_PHY_MESON8B_USB2) += phy-meson8b-usb2.o
diff --git a/drivers/phy/phy-bcm-ns2-usbdrd.c b/drivers/phy/phy-bcm-ns2-usbdrd.c
new file mode 100644
index 000..f41763f
--- /dev/null
+++ b/drivers/phy/phy-bcm-ns2-usbdrd.c
@@ -0,0 +1,567 @@
+/*
+ * Copyright (C) 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define ICFG_DRD_AFE   0x0
+#define ICFG_MISC_STAT 0x18
+#define ICFG_DRD_P0CTL 0x1C
+#define ICFG_STRAP_CTRL0x20
+#define ICFG_FSM_CTRL  0x24
+
+#define IDM_RST_BITBIT(0)
+#define AFE_CORERDY_VDDC   BIT(18)
+#define PHY_PLL_RESETB BIT(15)
+#define PHY_RESETB BIT(14)
+#define PHY_PLL_LOCK   BIT(0)
+
+#define DRD_DEV_MODE   BIT(20)
+#define OHCI_OVRCUR_POLBIT(11)
+#define ICFG_OFF_MODE  BIT(6)
+#define PLL_LOCK_RETRY 1000
+
+#define EVT_DEVICE 0
+#define EVT_HOST   1
+#define EVT_IDLE   2
+
+#define DRD_HOST_MODE  (BIT(2) | BIT(3))
+#define DRD_DEVICE_MODE(BIT(4) | BIT(5))
+#define DRD_HOST_VAL   0x803
+#define DRD_DEV_VAL0x807
+#define GPIO_DELAY 20
+#define PHY_WQ_DELAY   msecs_to_jiffies(600)
+
+struct ns2_phy_data;
+struct ns2_phy_driver {
+   void __iomem *icfgdrd_regs;
+   void __iomem *idmdrd_rst_ctrl;
+   void __iomem *crmu_usb2_ctrl;
+   void __iomem *usb2h_strap_reg;
+   spinlock_t lock; /* spin lock for phy driver */
+   struct ns2_phy_data *data;
+   struct extcon_specific_cable_nb extcon_dev;
+   struct extcon_specific_cable_nb extcon_host;
+   struct notifier_block host_nb;
+   struct notifier_block dev_nb;
+   struct delayed_work conn_work;
+   struct extcon_dev *edev;
+   struct gpio_desc *vbus_gpiod;
+   struct gpio_desc *id_gpiod;
+   int id_irq;
+   int vbus_irq;
+   unsigned long debounce_jiffies;
+   struct delayed_work wq_extcon;
+};
+
+struct ns2_phy_data {
+   struct ns2_phy_driver *driver;
+   struct phy *phy;
+   int new_state;
+};
+
+static const unsigned int usb_extcon_cable[] = {
+   EXTCON_USB,
+   EXTCON_USB_HOST,
+   EXTCON_NONE,
+};
+
+static inline int pll_lock_stat(u32 usb_reg, int reg_mask,
+   struct ns2_phy_driver