Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)

[<Cristian.Birsan@microchip.com>]

Hi,

On 12/20/21 4:50 PM, Greg Kroah-Hartman wrote:
> 
> On Wed, Dec 15, 2021 at 12:54:49PM -0300, Marcelo Roberto Jimenez wrote:
>> Hi Cristian,
>>
>> On Wed, Dec 15, 2021 at 10:04 AM <Cristian.Birsan@microchip.com> wrote:
>>>
>>> Hi Marcelo, Jonas,
>>>
>>> On 12/13/21 4:31 PM, Marcelo Roberto Jimenez wrote:
>>>>
>>>> Some people who received this message don't often get email from marcelo.jimenez@gmail.com. Learn why this is important <http://aka.ms/LearnAboutSenderIdentification>
>>
>> Hum, shame on me.
>>
>>>> Hi Jonas,
>>>>
>>>> Thank you for the quick response, I really appreciate it.
>>>>
>>>> On Mon, Dec 13, 2021 at 7:02 AM Jonas Bonn <jonas@norrbonn.se <mailto:jonas@norrbonn.se>> wrote:
>>>>
>>>>
>>>>     Given that the patch that you want to revert is almost 3 years old, it's
>>>>     a bit of a stretch to call this a regression.  I suspect that a cleaner
>>>>     way forward is to introduce some kind of quirk for your board.
>>>>
>>>>
>>>> Well, the board is basically the MPU, so if there is a hardware problem it would mean that it is a problem with the on chip peripheral.
>>>>
>>>>
>>>>     I have a self-powered board where the USB suspend sequence works and
>>>>     device add/remove works fine.  So fundamentally, I suspect that the
>>>>     driver is ok.
>>>>
>>>>
>>>> Maybe you have VBUS sensing enabled in your board. As I reported before, the VBUS sensing is not an option in this board. Nonetheless, the code in the kernel suggests that VBUS sensing is optional, since the presence of a VBUS sensing pin is explicitly tested in it.
>>>
>>> Is it possible to add a wire that connects VBUS to the right pin on the MPU ? Just to see if this has an impact or not ?
>>
>> Yes. Maybe I was not clear about that issue, so let me try to clarify.
>> The board _seems_ to have a provision for VBUS sensing, but it is not
>> working. I was not involved in this board's project and I found no
>> other documentation on the ACME Systems site, all I am saying here is
>> from reading the circuit diagram, so it is all my personal
>> interpretation. For hardware reasons, the aforementioned VBUS sensing
>> pin does not reach zero volts when the USB connector is removed, which
>> makes VBUS sensing ineffective. But I have tested it anyway and to
>> make it work, the first step is to prepare the board as specified here
>> https://www.acmesystems.it/arietta_power_supply in the section "Supply
>> power at 3.3 volt". The key here is to remove the R36 resistor, so
>> that the board can be fed by an external 3.3V voltage and become self
>> powered. Then, you add a line "atmel,vbus-gpio = <&pioB 16 0>;" below
>> the "usb2:" line in the Arietta DTD. After recompiling the kernel and
>> installing, it still does not work by just unplugging the USB cable.
>> You need to manually and carefully (!) short circuit the +5 USB line
>> to the ground when the cable is not connected. Only then VBUS sensing
>> will work and the device will accept enumeration again.
>>
>> In short, the VBUS sensing code in the kernel is ok. But that is not
>> my point. My point is that the kernel code implies that it is possible
>> to do without a VBUS sensing pin. The file
>> Documentation/devicetree/bindings/usb/atmel-usb.txt states that
>> "atmel,vbus-gpio" is an optional property. So, it seems to me like the
>> code should work without it, and indeed it worked. That is why I have
>> called this a regression.

In USB 2.0 specification (Chapter 9.1.1.2 Powered) there is the following
sentence: “Although self-powered devices may already be powered before they
are attached to the USB, they are not considered to be in the Powered state
until they are attached to the USB and VBUS is applied to the device.” This
makes VBUS sensing mandatory for self-powered devices and non mandatory for
bus-powered devices. This is how, in my opinion, “atmel,vbus-gpio” should
be used (optional for bus-powered devices only).

Yesterday I removed the “atmel,vbus-gpio”  from SAM9x60-EK device tree to
create a setup close to the one used by Marcelo and performed a test. After
plugging / unplugging the usb device cable the Ethernet gadget is able to
re-enumerate. This makes me think the issue is specific to Arietta board or
to SAM9G25 SoC. I retrieved a SAM9x5 from office and I need to prepare it
before I’m able to run some tests on it. This can tell us if the issue is
board or SoC related.

By reverting this patch I’m afraid we encourage board designers to not
implement VBUS detection because the driver used to just work without it
and I think is not correct.

Currently I’m not able to replicated the issue to debug it. I’m willing to
continue to investigate this and propose a patch but will need some help in
reproducing it on one of our boards. Another alternative would be to enable
debug information in the driver on Arietta board 
(by using #define DEBUG_LEVEL DBG_ALL in atmel_usba_udc.h) and send the logs
to my email.

Regards,
Cristian

> 
> Yes, hardware that used to work, and now does not, is a regression.
> 
> So, should I revert the offending patch here?  Adding new features is
> not a good reason to keep things that break systems.  Or is there a
> potential fix in this thread that I missed?
> 
> thanks,
> 
> greg k-h
>