From: Marcelo Roberto Jimenez <email@example.com> To: Cristian Birsan <Cristian.Birsan@microchip.com> Cc: Jonas Bonn <firstname.lastname@example.org>, email@example.com, Nicolas Ferre <Nicolas.Ferre@microchip.com>, Alexandre Belloni <firstname.lastname@example.org>, Ludovic Desroches <Ludovic.Desroches@microchip.com>, email@example.com, firstname.lastname@example.org, Greg Kroah-Hartman <email@example.com>, Felipe Balbi <firstname.lastname@example.org>, Sergio Tanzilli <email@example.com> Subject: Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc) Date: Wed, 15 Dec 2021 12:54:49 -0300 [thread overview] Message-ID: <CACjc_5qZjXbE1CwQCpc4+vzbsobfn5YKpU=UCVJpMGG1ROEfTg@mail.gmail.com> (raw) In-Reply-To: <firstname.lastname@example.org> 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 email@example.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 <firstname.lastname@example.org <mailto:email@example.com>> 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. > > > > I have not read the full USB spec, but if this is a fundamentally not resolvable problem, maybe we could have a configuration option, runtime or compile time, to enable or disable SUSPEND state, assuming that there is no problem with the original patch. > > > > In my particular application, it is more important to detect the disconnection, so that we can reconnect, than to enter SUSPEND. Whenever USB is not necessary, it will not be connected, so the energy saved will be very little in my case. > > > > My intention with this patch is also to provide a quick fix for anyone facing this problem, reverting is not necessarily the best long term solution, especially if the code is found to be correct. But assuming the chip USB controller has no design flaws, and assuming it should be possible to do without VBUS sensing, then the present code should be missing some detail. > > > > I would prefer to have a clear understanding of what is causing the issue before making any changes to the patch sent by Jonas in the upstream kernel. Indeed. We must understand properly how the code is changing the state of the USB state machine. I do not have the knowledge of this protocol/state machine inner workings and what are the code assumptions in this regard. > Marcelo, can you point where the driver hangs ? One can enable debug messages in the driver by using #define DEBUG_LEVEL DBG_ALL in atmel_usba_udc.h. As far as I know, the driver does not hang. I do not have an USB analyzer to check the protocol, but from the logs on the host side, the gadget code seems to be rejecting enumeration the second time it is plugged, and my assumption is that this is due to not having detected the disconnection. > > > > With all that said, I'm not immediately in a position to run tests on my > > SAMA5D2 board right now and the kernel on that board is 5.2. I'm not > > sure when we we would notice that USB suspend stopped working because > > there is no kernel maintenance planned for that board, as far as I know; > > someday, however, that work might happen and the lack of working USB > > suspend will be a regression in and of itself. > > > > > > > I can test it with the AT91SAM9G25 processor and I can also get a SAMA5D27 board to test with. > > I was able to run the tests on the kernel version pointed by Marcelo (5.10) on SAMA5D2 Xplained and on SAM9x60-EK (the USB IP version on this one should be closer to AT91SAM9G25). > It works on both boards with no issues. Both our boards use VBUS sensing. Unfortunately, currently I do not have access to a board with AT91SAM9G25 or to ACME Arietta to check / debug on it. From what I read in atmel_usba_udc.c, it should be possible to emulate the absence of VBUS sensing by eliminating the "atmel,vbus" or "atmel,vbus-gpio" line from the DTD file. So in principle you can test it with your boards. > > Thanks, > > Jonas > > > > > > Best regards, > > Marcelo. > > > > Regards, > Cristian Best regards, Marcelo.
next prev parent reply other threads:[~2021-12-15 16:00 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-11 18:36 Marcelo Roberto Jimenez 2021-12-12 9:48 ` Thorsten Leemhuis 2022-01-26 12:20 ` Greg Kroah-Hartman 2022-01-26 12:54 ` Thorsten Leemhuis [not found] ` <CACjc_5o9eO5YTVt4jE7v1E9nirCwFMc1=Ub-jXSFCg1_8A2M-A@mail.gmail.com> 2022-01-26 13:43 ` Marcelo Roberto Jimenez 2021-12-13 10:02 ` Jonas Bonn [not found] ` <CACjc_5pHbLStniQnOVLDW5iLbKn8ovePuQsFFqeEnQPSSYxJoQ@mail.gmail.com> 2021-12-15 13:04 ` Cristian.Birsan 2021-12-15 15:54 ` Marcelo Roberto Jimenez [this message] 2021-12-20 14:50 ` Greg Kroah-Hartman 2021-12-22 18:33 ` Cristian.Birsan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CACjc_5qZjXbE1CwQCpc4+vzbsobfn5YKpU=UCVJpMGG1ROEfTg@mail.gmail.com' \ --firstname.lastname@example.org \ --cc=Cristian.Birsan@microchip.com \ --cc=Ludovic.Desroches@microchip.com \ --cc=Nicolas.Ferre@microchip.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH] usb: gadget: atmel: Revert regression in USB Gadget (atmel_usba_udc)' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).