On 03/14/2016 10:47 PM, Ladislav Michl wrote:
On Mon, Mar 14, 2016 at 09:40:54PM +0200, Tero Kristo wrote:On 03/14/2016 05:13 PM, Ladislav Michl wrote:Hi there! seems it's been three years since: [PATCH 1/1] Fix sprz319 erratum 2.1 http://article.gmane.org/gmane.linux.ports.arm.omap/71633 errata: http://www.ti.com/lit/er/sprz319f/sprz319f.pdf Nice summary (third post by Rodrigo Lemos) can be found here: https://github.com/RobertCNelson/stable-kernel/issues/26 so I'm not going to repeat it here. Company I'm working for has few thousands of IGEPv2 boards in field. They have hub connected to usb port with udlfb display, 3G modem and usbserial: /: Bus 03.Port 1: Dev 1, Class=root_hub, Driver=ehci-omap/3p, 480M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M |__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=udlfb, 480M |__ Port 3: Dev 4, If 0, Class=Vendor Specific Class, Driver=option, 480M |__ Port 3: Dev 4, If 1, Class=Vendor Specific Class, Driver=, 480M |__ Port 3: Dev 4, If 2, Class=Vendor Specific Class, Driver=option, 480M |__ Port 3: Dev 4, If 3, Class=Vendor Specific Class, Driver=option, 480M |__ Port 3: Dev 4, If 4, Class=Mass Storage, Driver=usb-storage, 480M |__ Port 3: Dev 4, If 5, Class=Mass Storage, Driver=usb-storage, 480M |__ Port 4: Dev 5, If 0, Class=Hub, Driver=hub/4p, 480M |__ Port 3: Dev 6, If 0, Class=Communications, Driver=cdc_acm, 12M |__ Port 3: Dev 6, If 1, Class=CDC Data, Driver=cdc_acm, 12M Few tens of those disconnects USB after while (one minute to few hours) with: "usb usb3-port1: disabled by hub (EMI?), re-enabling..." IGEPv2 uses DM3730 with 26MHz crystal divided by two providing 13MHz SYS_CLK, therefore it is good candidate to fix. Patch bellow was used as a dirty hack to test if usb disconnects go away with errata applied. And indeed, device is already running for few days without any issues. Now question is, how should a proper fix look like as modifiing omap2_dpll_round_rate seems easy enough. Is that acceptable? How to test proper clock to apply errata? Comparing string is suboptimal. Should we introduce some flag?The hack applied seems rather bad to me, as it doesn't take any input frequencies into consideration. Basically you just force the divider / multiplier to the value required for 13MHz input clock, but this is going to be wrong with any other input clock.Well, hacking that took few minutes and was done only to test if issue with disconnects has anything to do with said errata. And it indeed does (doing it right way would mean I'd need to verify what's written to registers anyway, thus touching hacked functions). Otherwise I agree, but also note that previous attempt ended in various repositories, but never found its way to mainline - something I'd like to avoid, so I even didn't try to come with anything sensible.I think a proper fix would probably be to implement new clock ops for DPLL5, something similar to what was done with the original patch a few years back. This would select the divider/multiplier factors for the DPLL based on table values provided by the erratum.That seems like pretty intrusive change given the fact clock are declared as DT_CLK(NULL, "dpll5_ck", "dpll5_ck"), DT_CLK(NULL, "dpll5_m2_ck", "dpll5_m2_ck"), and registered with ti_dt_clocks_register. Or do you mean completely new clock ops including differently named DT property? Is that what you mean? Also note that for other frequencies original algo is still valid (omap2_dpll_round_rate)...
Yes, I think we should add a top-level ops for dpll5, that would determine if table dividers shall be used first, if not, then just call the generic implementation.
I would also add a new compatible string for the purpose, this means the users must update both kernel + DTB but I believe any OMAPx customers are doing this anyway.
-Tero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
