MIPS Function Tracer question
I've been doing some testing of the MIPS Function Tracer functionality on the 3.3 kernel. I was surprised to find that the option to generate frame pointers was required for tracing. When I don't enable FRAME_POINTER along with FUNCTION_TRACER, the kernel hangs on boot. I also noticed that a checkin to the 3.4 kernel (b732d439cb43336cd6d7e804ecb2c81193ef63b0) no longer forces on FRAME_POINTER when FUNCTION_TRACER is selected. I was wondering how it works in 3.4 and beyond, so I built a Malta kernel from the latest MIPS tree with FUNCTION_TRACING enabled and tested it with QEMU. The kernel hung the same way. I can think of 2 reasons for this: 1. Function tracing is broken for MIPS in 3.4 and beyond. 2. The 4.5.3 GNU C compiler I'm using is generating different code for function tracing. I was wondering if anyone has MIPS function tracing working in 3.4 or later? I did figure out why it's hanging and I have some changes that will allow the function tracer to run without frame pointers, but before I proceed I want to rule out compiler differences. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mips: function tracer: Fix broken function tracing
I already tried using "adddiu sp, sp, 8" and it caused the kernel to randomly crash. After many hours of debugging the reason occurred to me while in bed in the middle of the night. The problem is that if we get an interrupt between the add 8 and the add -8 instructions, we trash the existing stack. The problem with the 2 nop approach is that there are a series of subroutines used to write each nop and these nested subroutines are traceable. This means on the second call to these subroutines they execute with only one nop and crash. I could write some new code that wrote the 2 nops at once, but (now that I understand "stop_machine") with the branch likely solution we should be able to stop using "stop_machine" when we write nops to the 20-30 thousand Linux functions. It looks like other platforms have stopped using stop_machine. Al On Fri, Jan 11, 2013 at 12:01 PM, David Daney wrote: > On 01/11/2013 06:33 AM, Al Cooper wrote: >> >> Function tracing is currently broken for all 32 bit MIPS platforms. >> When tracing is enabled, the kernel immediately hangs on boot. >> This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0 >> that changes the kernel/trace/Kconfig file so that is no longer >> forces FRAME_POINTER when FUNCTION_TRACING is enabled. >> >> MIPS frame pointers are generally considered to be useless because >> they cannot be used to unwind the stack. Unfortunately the MIPS >> function tracing code has bugs that are masked by the use of frame >> pointers. This commit fixes the bugs so that MIPS frame pointers do >> not need to be enabled. >> >> The bugs are a result of the odd calling sequence used to call the trace >> routine. This calling sequence is inserted into every tracable function >> when the tracing CONFIG option is enabled. This sequence is generated >> for 32bit MIPS platforms by the compiler via the "-pg" flag. >> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every >> call to the trace routine "_mcount" (some legacy thing where 2 arguments >> used to be pushed on the stack). The _mcount routine is expected to >> adjust the sp by +8 before returning. >> >> One of the bugs is that when tracing is disabled for a function, the >> "jalr _mcount" instruction is replaced with a nop, but the >> "addiu sp,sp,-8" is still executed and the stack pointer is left >> trashed. When frame pointers are enabled the problem is masked >> because any access to the stack is done through the frame >> pointer and the stack pointer is restored from the frame pointer when >> the function returns. This patch uses a branch likely instruction >> "bltzl zero, f1" instead of "nop" to disable the call because this >> instruction will not execute the "addiu sp,sp,-8" instruction in >> the delay slot. The other possible solution would be to nop out both >> the jalr and the "addiu sp,sp,-8", but this would need to be interrupt >> and SMP safe and would be much more intrusive. > > > I thought all CPUs were in stop_machine() when the modifications were done, > so that there is no issue with multi-word instruction patching. > > Am I wrong about this? > > So really I think you can do two NOP just as easily. > > The only reason I bring this up is that I am not sure all 32-bit CPUs > implement the 'Likely' branch variants. Also there may be an affect on the > branch predictor. > > A third possibility would be to replace the JALR with 'ADDIU SP,SP,8' That > way the following adjustment would be canceled out. This would work well on > single-issue CPUs, but the instructions may not be able to dual-issue on a > multi issue machine due to data dependencies. > > David Daney > > >> >> A few other bugs were fixed where the _mcount routine itself did not >> always fix the sp on return. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mips: function tracer: Fix broken function tracing
On Mon, Jan 14, 2013 at 5:12 PM, David Daney wrote: > On 01/14/2013 01:10 PM, Alan Cooper wrote: >> >> I already tried using "adddiu sp, sp, 8" and it caused the kernel to >> randomly crash. After many hours of debugging the reason occurred to >> me while in bed in the middle of the night. The problem is that if we >> get an interrupt between the add 8 and the add -8 instructions, we >> trash the existing stack. >> >> The problem with the 2 nop approach is that there are a series of >> subroutines used to write each nop and these nested subroutines are >> traceable. > > > This seems like a bug. The low-level code used to do code patching probably > should be CFLAGS_REMOVE_file.o = -pg While tracing mcount cannot be done because it's recursive, allowing tracing of the code to enable/disable the call to mcount can be done and seems useful. Also, fixing the 2 nop solution this way will still not allow us to stop using stop_machine() which is hugely disruptive to a running system. Remember that when tracing is enabled and disabled we end up modifying 20 to 30 thousand functions. Moving this functionality out of stop_machine() seems like a big benefit. > > > >> This means on the second call to these subroutines they >> execute with only one nop and crash. I could write some new code >> that wrote the 2 nops at once, but (now that I understand >> "stop_machine") with the branch likely solution we should be able to >> stop using "stop_machine" when we write nops to the 20-30 thousand >> Linux functions. It looks like other platforms have stopped using >> stop_machine. > > > I don't particularly object to the 'branch likely solution', but I think the > failures of the other approaches indicates underlying bugs in the tracing > code. Those bugs should probably be fixed. If a solution can be found that modifies a single 32bit instruction to enable/disable tracing, I don't see any bugs in the underlying code. Plus we can avoid using stop_machine(). > > David Daney > > > >> >> Al >> >> On Fri, Jan 11, 2013 at 12:01 PM, David Daney >> wrote: >>> >>> On 01/11/2013 06:33 AM, Al Cooper wrote: >>>> >>>> >>>> Function tracing is currently broken for all 32 bit MIPS platforms. >>>> When tracing is enabled, the kernel immediately hangs on boot. >>>> This is a result of commit b732d439cb43336cd6d7e804ecb2c81193ef63b0 >>>> that changes the kernel/trace/Kconfig file so that is no longer >>>> forces FRAME_POINTER when FUNCTION_TRACING is enabled. >>>> >>>> MIPS frame pointers are generally considered to be useless because >>>> they cannot be used to unwind the stack. Unfortunately the MIPS >>>> function tracing code has bugs that are masked by the use of frame >>>> pointers. This commit fixes the bugs so that MIPS frame pointers do >>>> not need to be enabled. >>>> >>>> The bugs are a result of the odd calling sequence used to call the trace >>>> routine. This calling sequence is inserted into every tracable function >>>> when the tracing CONFIG option is enabled. This sequence is generated >>>> for 32bit MIPS platforms by the compiler via the "-pg" flag. >>>> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every >>>> call to the trace routine "_mcount" (some legacy thing where 2 arguments >>>> used to be pushed on the stack). The _mcount routine is expected to >>>> adjust the sp by +8 before returning. >>>> >>>> One of the bugs is that when tracing is disabled for a function, the >>>> "jalr _mcount" instruction is replaced with a nop, but the >>>> "addiu sp,sp,-8" is still executed and the stack pointer is left >>>> trashed. When frame pointers are enabled the problem is masked >>>> because any access to the stack is done through the frame >>>> pointer and the stack pointer is restored from the frame pointer when >>>> the function returns. This patch uses a branch likely instruction >>>> "bltzl zero, f1" instead of "nop" to disable the call because this >>>> instruction will not execute the "addiu sp,sp,-8" instruction in >>>> the delay slot. The other possible solution would be to nop out both >>>> the jalr and the "addiu sp,sp,-8", but this would need to be interrupt >>>> and SMP safe and would be much more intrusive. >>> >>>
Re: [PATCH] mips: function tracer: Fix broken function tracing
On Mon, Jan 14, 2013 at 10:40 PM, Steven Rostedt wrote: > On Fri, Jan 11, 2013 at 09:01:01AM -0800, David Daney wrote: >> >> I thought all CPUs were in stop_machine() when the modifications >> were done, so that there is no issue with multi-word instruction >> patching. >> >> Am I wrong about this? >> >> So really I think you can do two NOP just as easily. > > The problem with double NOPs is that it can only work if there's no > problem executing one nop and a non NOP. Which I think is an issue here. > > > If you have something like: > > bl _mcount > addiu sp,sp,-8 > > And you convert that to: > > nop > nop > > Now if you convert that back to: > > bl ftrace_caller > addiu sp,sp,-8 > > then you can have an issue if the task was preempted after that first > nop. Because stop_machine() doesn't wait for tasks to exit kernel space. > If you have a CONFIG_PREEMPT kernel, a task can be sleeping anywhere. > Thus you have a task execute the first nop, get preempted. You update > the code to be: > > bl ftrace_caller > addiu sp,sp,-8 > > When that task gets scheduled back in, it will act like it just > executed: > > nop > addiu sp,sp,-8 > > Which is the problem you're trying to solve in the first place. > > Now that said, There's no reason we need that addiu sp,sp,-8 there. > That's just what the mips defined mcount requires. But as you can see > above, with dynamic ftrace, the defined mcount is only called at boot > up, and never again. That means at boot up you can convert to: > > nop > nop > > and then when you enable tracing just convert it to: > > bl ftrace_caller > nop > > There's nothing that states what the ftrace caller must be. We can have > it do a proper stack update. That is, only at boot up do we need to > handle the defined mcount. After that, those instructions are just place > holders for our own algorithms. If the addiu was needed for the defined > mcount, there's no reason to keep it for our own ftrace_caller. > > Would that work? > > -- Steve > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mips: function tracer: Fix broken function tracing
On Tue, Jan 15, 2013 at 4:34 PM, David Daney wrote: > On 01/15/2013 01:07 PM, Steven Rostedt wrote: >> >> On Tue, 2013-01-15 at 09:55 -0800, David Daney wrote: >> There's nothing that states what the ftrace caller must be. We can have it do a proper stack update. That is, only at boot up do we need to handle the defined mcount. After that, those instructions are just place holders for our own algorithms. If the addiu was needed for the defined mcount, there's no reason to keep it for our own ftrace_caller. Would that work? >>> >>> >>> ... either do as you suggest and dynamically change the ABI of the >>> target function. >> >> >> We already change the ABI. We have it call ftrace_caller instead of >> mcount. >> >> BTW, I've just compiled with gcc 4.6.3 against mips, and I don't see the >> issue. I have: >> >> : >> 0: 03e0082dmoveat,ra >> 4: 0c00jal 0 >> 4: R_MIPS_26_mcount >> 4: R_MIPS_NONE *ABS* >> 4: R_MIPS_NONE *ABS* >> 8: 602dmovet0,zero >> c: 2402000dli v0,13 >>10: 3c03lui v1,0x0 >> 10: R_MIPS_HI16 mem_section >> 10: R_MIPS_NONE *ABS* >> 10: R_MIPS_NONE *ABS* >>14: 000216fcdsll32 v0,v0,0x1b >>18: 6463daddiu v1,v1,0 >> >> Is it dependent on the config? > > > Yes. > > You need to select a 32-bit kernel (which in turn may require selecting a > board type that also supports it). > > The ABI is different for 32-bit and 64-bit _mcount. > > David Daney > Building for MIPS malta will show the problem. > > >> >>> >>> Or add support to GCC for a better tracing ABI (as I already said we did >>> for mips64). >> >> >> I wouldn't waste time changing gcc for this. If you're going to change >> gcc than please implement the -mfentry option. Look at x86_64 to >> understand this more. > > > A good point. But I don't really plan on doing any work related to 32-bit > mips things at this point, so any such change would have to be done by > someone else. > > David Daney > I love the idea of removing the useless stack adjust stuff at run time! The issue still remains for the initial writing of the 2 nops. It looks like the initial call to write the nops is done from ftrace_init which is called before SMP is up, so if I write the 2 nops via a single call to a function with interrupts disabled it should be safe. I also need to do this for modules at insmod time. This has been GREAT feedback! Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] mips: function tracer: Fix broken function tracing
When the kernel first boots we have to be able to handle the gcc
generated jalr, addui sequence until ftrace_init gets a chance to run
and change the sequence. At this point mcount just adjusts the stack
and returns. When ftrace_init runs, we convert the jalr/addui to nops.
Then whenever tracing is enabled we convert the first nop to a "jalr
mcount+8". The mcount+8 entry point skips the stack adjust.
On Thu, Jan 17, 2013 at 1:27 AM, Geert Uytterhoeven
wrote:
> On Thu, Jan 17, 2013 at 12:43 AM, Al Cooper wrote:
>> Part of the sequence is "addiu sp,sp,-8" in the delay slot after every
>> call to the trace routine "_mcount" (some legacy thing where 2 arguments
>> used to be pushed on the stack). The _mcount routine is expected to
>> adjust the sp by +8 before returning.
>
> So when not disabled, the original jalr and addiu will be there, so _mcount
> has
> to adjust sp.
>
>> The problem is that when tracing is disabled for a function, the
>> "jalr _mcount" instruction is replaced with a nop, but the
>> "addiu sp,sp,-8" is still executed and the stack pointer is left
>> trashed. When frame pointers are enabled the problem is masked
>> because any access to the stack is done through the frame
>> pointer and the stack pointer is restored from the frame pointer when
>> the function returns.
>>
>> This patch writes two nops starting at the address of the "jalr _mcount"
>> instruction whenever tracing is disabled. This means that the
>> "addiu sp,sp.-8" will be converted to a nop along with the "jalr".
>
> When disabled, there will be two nops.
>
>> This is SMP safe because the first time this happens is during
>> ftrace_init() which is before any other processor has been started.
>> Subsequent calls to enable/disable tracing when other CPUs ARE running
>> will still be safe because the enable will only change the first nop
>> to a "jalr" and the disable, while writing 2 nops, will only be changing
>
> When re-enabled, there will be a jalr and a nop, which differs from the
> initial
> case, so _mcount doesn't have to adjust sp?
>
>> @@ -69,7 +68,7 @@ NESTED(ftrace_caller, PT_SIZE, ra)
>> .globl _mcount
>> _mcount:
>> b ftrace_stub
>> -nop
>> + addiu sp,sp,8
>> lw t1, function_trace_stop
>> bnezt1, ftrace_stub
>> nop
>
> But _mcount will always adjust the stack pointer?
> What am I missing?
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 2/2] usb: phy: phy-brcm-usb: Add Broadcom STB USB Phy driver
On Wed, Aug 31, 2016 at 5:57 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 30 August 2016 08:00 PM, Al Cooper wrote:
>> Add a new USB Phy driver for Broadcom STB SoCs. This driver
>> supports all Broadcom STB ARM SoCs. This driver in combination
>> with the generic ohci, ehci and xhci platform drivers will enable
>> USB1.1, USB2.0 and USB3.0 support. This Phy driver also supports
>> the Broadcom UDC gadget driver.
>
> Remove usb: from $subject
Ok
>>
>> Signed-off-by: Al Cooper
>> ---
>> .../bindings/phy/brcm,brcmstb-usb-phy.txt | 39 +
>> MAINTAINERS| 7 +
>> drivers/phy/Kconfig| 10 +
>> drivers/phy/Makefile | 2 +
>> drivers/phy/phy-brcm-usb-init.c| 792
>> +
>
> other broadcom phy drivers use only "bcm" in the file name.
We have a mixed use of both names throughout the kernel. Notice
"phy-brcm-sata.c in the phy directory. "brcm" seems a little better
because it matches the device tree prefix used by all drivers and is
used by many of the latest drivers.
>> diff --git a/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt
>> b/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt
>> new file mode 100644
>> index 000..34fa9dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt
>> @@ -0,0 +1,39 @@
>> +Broadcom STB USB PHY
>> +
>> +Required properties:
>> + - compatible: brcm,brcmstb-usb-phy
>> + - reg: two offset and length pairs. The second pair specifies the
>> +USB 3.0 related registers and is only required for PHYs
>> +that support USB 3.0
>
> I think the right way to model this should be to have separate subnodes for
> usb2 and usb3.
The first register block contains both the 2.0 and 3.0 phy registers.
The second register block contains an optional "workaround" register
set used on just a few SoCs to workaround a bug in the XHCI phy. Also,
this approach allows us to use the already parsed resource and saves
some additional code to get and map the registers.
>> + - #phy-cells: Shall be 1 as it expects one argument for setting
>> +the type of the PHY. Possible values are 0 (1.1 and 2.0),
>> +1 (3.0)
>> +
>> +
> spurious blank space.
Ok
>> +Optional Properties:
>> +- clocks : phandle + clock specifier for the phy clocks
>> +- clock-names: string, clock name
>> +- ipp: Invert Port Power
>> +- ioc: Invert Over Current detection
>> +- has_xhci: Contains an optional 3.0 PHY
>
> prefix all the broadcom specific properties with "bcm," or whatever is used
> for
> broadcom specific properties.
Ok
>> +- device: PHY Device mode. Possible values are: 0 (Host), 1 (Device)
>> + or 2 (DRD)
>> +
>> +
>> +
>
> spurious blank spaces..
Ok
>> +Example:
>> +
>> +usbphy_0: usb-phy@f0470200 {
>> + reg = <0xf0470200 0xb8>,
>> + <0xf0471940 0x6c0>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>
> Why do you need address cells, size cells? Include it in the documentation.
I don't, I'll remove them.
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 19bff3a..5ff5e47 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -443,6 +443,16 @@ config PHY_CYGNUS_PCIE
>> Enable this to support the Broadcom Cygnus PCIe PHY.
>> If unsure, say N.
>>
>> +config BRCM_USB_PHY
>> + tristate "Broadcom USB PHY driver"
>> + depends on OF && USB && ARCH_BRCMSTB
>
> Please also include COMPILE_TEST
Ok
>> + select GENERIC_PHY
>> + default y
>> + help
>> + Enable this to support the Broadcom USB PHY on
>> + Broadcom STB SoCs.
>> + If unsure, say Y.
>
> Generally it is N.
Since this driver should work on any "ARCH_BRCMSTB" system and
"ARCH_BRCMSTB" is included in the "depends", it seems like this should
be defaulted to Y.
>> diff --git a/drivers/phy/phy-brcm-usb-init.c
>> b/drivers/phy/phy-brcm-usb-init.c
>> new file mode 100644
>> index 000..f5d8c32
>> --- /dev/null
>> +++ b/drivers/phy/phy-brcm-usb-init.c
>> @@ -0,0 +1,792 @@
>> +/*
>> + * Copyright (C) 2014-2016 Broadcom
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *notice, this list of conditions and the following disclaimer in the
>> + *documentation and/or other materials provided with the distribution.
>> + * 3. Neither the name of the project nor the names of its contributors
>> + *may be used to endorse or promote products derived from this software
>> + *without specific prior written permission.
>> + *
>> + * T
Re: [PATCH] PM / core: Clear the direct_complete flag on errors
> On 4 October 2018 at 11:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki
> >
> > If __device_suspend() returns early on an error or pending wakeup
> > and the power.direct_complete flag has been set for the device
> > already, the subsequent device_resume() will be confused by it
> > and it will call pm_runtime_enable() incorrectly, as runtime PM
> > has not been disabled for the device by __device_suspend().
>
> I think it would be fair to mention that is related to the async
> suspend path, in dpm_suspend().
This also fixed the issue and looks cleaner.
>
> >
> > To avoid that, clear power.direct_complete if __device_suspend()
> > is not going to disable runtime PM for the device before returning.
>
> Overall, by looking at the behavior in dpm_suspend() of async
> suspended devices, it does look a bit fragile to me.
>
> My worries is that we put asynced suspended devices in the
> dpm_suspended_list, no matter if the device was successfully suspended
> or not. This differs from the no-async path.
>
> In the long run, maybe we should change that instead?
I originally looked into this. Currently dmp_suspend moves async
devices from the prepared list to the suspended list as they are
queued and I looked at moving this to __device_suspend (after the
checks for async_error and wake_pending) but realized that this would
change normal resume ordering and was afraid that would be too
disruptive.
Al
On Thu, Oct 4, 2018 at 9:23 AM Ulf Hansson wrote:
>
> On 4 October 2018 at 11:08, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki
> >
> > If __device_suspend() returns early on an error or pending wakeup
> > and the power.direct_complete flag has been set for the device
> > already, the subsequent device_resume() will be confused by it
> > and it will call pm_runtime_enable() incorrectly, as runtime PM
> > has not been disabled for the device by __device_suspend().
>
> I think it would be fair to mention that is related to the async
> suspend path, in dpm_suspend().
>
> >
> > To avoid that, clear power.direct_complete if __device_suspend()
> > is not going to disable runtime PM for the device before returning.
>
> Overall, by looking at the behavior in dpm_suspend() of async
> suspended devices, it does look a bit fragile to me.
>
> My worries is that we put asynced suspended devices in the
> dpm_suspended_list, no matter if the device was successfully suspended
> or not. This differs from the no-async path.
>
> In the long run, maybe we should change that instead?
>
> >
> > Fixes: aae4518b3124 (PM / sleep: Mechanism to avoid resuming
> > runtime-suspended devices unnecessarily)
> > Reported-by: Al Cooper
> > Cc: 3.16+ # 3.16+
> > Signed-off-by: Rafael J. Wysocki
>
> Reviewed-by: Ulf Hansson
>
> Kind regards
> Uffe
>
> > ---
> > drivers/base/power/main.c |5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/base/power/main.c
> > ===
> > --- linux-pm.orig/drivers/base/power/main.c
> > +++ linux-pm/drivers/base/power/main.c
> > @@ -1713,8 +1713,10 @@ static int __device_suspend(struct devic
> >
> > dpm_wait_for_subordinate(dev, async);
> >
> > - if (async_error)
> > + if (async_error) {
> > + dev->power.direct_complete = false;
> > goto Complete;
> > + }
> >
> > /*
> > * If a device configured to wake up the system from sleep states
> > @@ -1726,6 +1728,7 @@ static int __device_suspend(struct devic
> > pm_wakeup_event(dev, 0);
> >
> > if (pm_wakeup_pending()) {
> > + dev->power.direct_complete = false;
> > async_error = -EBUSY;
> > goto Complete;
> > }
> >
Re: [PATCH 4/4] ARM: dts: Fix-up EMMC2 controller's frequency
Nicolas, I got a better description of the failure and it looks like the bus clock needs to be limited to 300KHz for a 500MHz core clock. What's happening is that an internal reset sequence is needed after a command timeout and the reset signal needs to be asserted for at least 2 ticks of the bus clock. This is done using a 12 bit counter clocked by the core clock. That means a 500MHz core clock produces a 122KHz reset signal which is too fast for 2 ticks of the 200KHz bus clock (100KHz) but is okay for the 300KHz (150Khz) bus clock. Al On Mon, Apr 5, 2021 at 4:45 AM Nicolas Saenz Julienne wrote: > > Hi Alan, > > On Thu, 2021-04-01 at 11:23 -0400, Alan Cooper wrote: > > Nicolas, > > > > Sorry, I just noticed this thread. > > This is a known bug in some newer Arasan cores. > > The problem happens when the difference between the core clock and the bus > > clock is too great. > > Limiting the clock to 200KHz minimum should be a good fix. > > Great, that's what I was hoping to hear :). Out of curiosity, can you share > more details on how the failure occurs? > > > In my experience, it's only eMMC that needs the clock to be retried > > > below 400KHz and not SD or SDIO. That's because the CMD signal for > > eMMC starts out as open-drain during identification and the size of > > the pull-up on the CMD signal can require the <400KHz clock. Once eMMC > > is out of identification mode the CMD signal is switched to push-pull > > and can run at much higher clock rates. > > Fair enough, I need to do some tests, some of the compute modules use an eMMC. > > > I don't think that SD and SDIO have any open-drain signals, so they > > shouldn't need to retry at slower clock speeds. > > Noted. > > > I'm trying to get more detail on the bug, like the exact ratio of core > > clock to bus clock that causes the problem. When I first found this > > bug I was told that the failure would not happen at 200KHz, but we > > were using a 405MHz core clock. > > That would be nice to have. > > > One other question. Why are you using polling for the SD card, this > > newer controller supports the interrupt driven "Card Inserted" signal > > and avoids wasting time polling? > > I believe the line isn't routed on RPi4. > > > Al >
Re: [PATCH v10 1/5] usb: xhci: Change the XHCI link order in the Makefile
Greg, Alan,
The other 4 related patches were accepted into usb-next and I just
realized that this one didn't make it. This patch will not fix the
"insmod out of order" issue, but will help our controllers work with
some poorly behaved USB devices when the drivers are builtin.
Thanks
Al
On Wed, May 13, 2020 at 3:42 PM Alan Cooper wrote:
>
> On Wed, May 13, 2020 at 1:46 PM Florian Fainelli wrote:
> >
> >
> >
> > On 5/13/2020 10:39 AM, Alan Stern wrote:
> > > On Wed, May 13, 2020 at 07:05:05PM +0200, Greg Kroah-Hartman wrote:
> > >> On Wed, May 13, 2020 at 09:31:11AM -0700, Florian Fainelli wrote:
> > >>>
> > >>>
> > >>> On 5/13/2020 9:27 AM, Greg Kroah-Hartman wrote:
> > >>>> On Wed, May 13, 2020 at 08:08:07AM -0700, Florian Fainelli wrote:
> > >>>>>
> > >>>>>
> > >>>>> On 5/13/2020 5:26 AM, Greg Kroah-Hartman wrote:
> > >>>>>> On Tue, May 12, 2020 at 11:00:15AM -0400, Al Cooper wrote:
> > >>>>>>> Some BRCMSTB USB chips have an XHCI, EHCI and OHCI controller
> > >>>>>>> on the same port where XHCI handles 3.0 devices, EHCI handles 2.0
> > >>>>>>> devices and OHCI handles <2.0 devices. Currently the Makefile
> > >>>>>>> has XHCI linking at the bottom which will result in the XHIC driver
> > >>>>>>> initalizing after the EHCI and OHCI drivers and any installed 3.0
> > >>>>>>> device will be seen as a 2.0 device. Moving the XHCI linking
> > >>>>>>> above the EHCI and OHCI linking fixes the issue.
> > >>>>>>
> > >>>>>> What happens if all of these are modules and they are loaded in a
> > >>>>>> different order? This makefile change will not help with that, you
> > >>>>>> need
> > >>>>>> to have logic in the code in order to properly coordinate this type
> > >>>>>> of
> > >>>>>> mess, sorry.
> > >>>>>
> > >>>>> I believe we should be using module soft dependencies to instruct the
> > >>>>> module loaders to load the modules in the correct order, so something
> > >>>>> like this would do (not tested) for xhci-plat-hcd.c:
> > >>>>>
> > >>>>> MODULE_SOFTDEP("post: ehci-hcd ohci-hcd");
> > >>>>>
> > >>>>> and I am not sure whether we need to add the opposite for ehci-hcd and
> > >>>>> ohci-hcd:
> > >>>>>
> > >>>>> MODULE_SOFTDEP("pre: xhci-plat-hcd");
> > >>>>
> > >>>> That's a nice start, but what happens if that isn't honored? This
> > >>>> really needs to work properly for any order as you never can guarantee
> > >>>> module/driver loading order in a system of modules.
> > >>>
> > >>> I also suggested that device links may help, though I am not sure. What
> > >>> do you suggest to be done?
> > >>
> > >> No idea. device links will help if you defer the probe properly until
> > >> you see the proper drivers binding correctly.
> > >
> > > I suspect that in general there is no way to do this properly.
> > >
> > > We can't modify ehci-hcd and ohci-hcd to make them wait. In fact, for
> > > all they know, xhci-hcd will _never_ be loaded.
> > >
> > > One thing that might be possible (although not all platforms may support
> > > it) is if xhci-hcd could somehow disconnect all devices attached to a
> > > peer port when it starts up. But that would be disruptive to any
> > > devices that aren't USB-3.
> > >
> > > We faced a very similar ordering problem between ehci-hcd and
> > > [ou]hci-hcd many years ago, and we never found a good solution.
> > > We did arrange the link order so that ehci-hcd precedes the others, and
> > > we added a warning message to ehci-hcd which gets printed if the module
> > > initialization routine runs after [ou]hci-hcd is loaded. Also, there
> > > are MODULE_SOFTDEP lines in ohci-pci.c and uhci-pci.c.
> >
> > Given that these modules are used on specific SoC platforms, where we
> > usually provide a reference implementation of user space and kernel
> > space and documentation, it seems to me that the MODULE_SOFTDEP(),
> > despite being a hint and best effort from user space module loaders is
> > probably acceptable.
> > --
> > Florian
>
> What I found in the past is that things work. For example if the ehci
> driver starts first, the USB device will come up as a 2.0 device and
> when the XHCI driver comes up the device will switch to 3.0. I've see
> the same thing happen if OHCI starts before EHCI. It's just that there
> are some poorly behaved USB devices that have trouble with this.
>
> Al
Re: [PATCH 4/4] ARM: dts: Fix-up EMMC2 controller's frequency
Nicolas,
Sorry, I just noticed this thread.
This is a known bug in some newer Arasan cores.
The problem happens when the difference between the core clock and the
bus clock is too great.
Limiting the clock to 200KHz minimum should be a good fix.
In my experience, it's only eMMC that needs the clock to be retried
below 400KHz and not SD or SDIO. That's because the CMD signal for
eMMC starts out as open-drain during identification and the size of
the pull-up on the CMD signal can require the <400KHz clock. Once eMMC
is out of identification mode the CMD signal is switched to push-pull
and can run at much higher clock rates.
I don't think that SD and SDIO have any open-drain signals, so they
shouldn't need to retry at slower clock speeds.
I'm trying to get more detail on the bug, like the exact ratio of core
clock to bus clock that causes the problem. When I first found this
bug I was told that the failure would not happen at 200KHz, but we
were using a 405MHz core clock.
One other question. Why are you using polling for the SD card, this
newer controller supports the interrupt driven "Card Inserted" signal
and avoids wasting time polling?
Al
On Fri, Mar 26, 2021 at 12:17 PM Nicolas Saenz Julienne
wrote:
>
> On Thu, 2021-03-25 at 20:11 +0100, Stefan Wahren wrote:
> > Am 24.03.21 um 16:34 schrieb Nicolas Saenz Julienne:
> > > Hi Stefan,
> > >
> > > On Wed, 2021-03-24 at 16:16 +0100, Stefan Wahren wrote:
> > > > Hi Nicolas,
> > > >
> > > > Am 22.03.21 um 19:58 schrieb Nicolas Saenz Julienne:
> > > > > From: Nicolas Saenz Julienne
> > > > >
> > > > > Force emmc2's frequency to 150MHz as the default 100MHz (set by FW)
> > > > > seems to interfere with the VPU clock when setup at frequencies bigger
> > > > > than 500MHz (a pretty common case). This ends up causing unwarranted
> > > > > SDHCI CMD hangs when no SD card is present.
> > > > >
> > > > > Signed-off-by: Nicolas Saenz Julienne
> > > > > ---
> > > > > arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 6 ++
> > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> > > > > b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> > > > > index 3b4ab947492a..9aa8408d9960 100644
> > > > > --- a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> > > > > +++ b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> > > > > @@ -257,6 +257,12 @@ &emmc2 {
> > > > > vqmmc-supply = <&sd_io_1v8_reg>;
> > > > > vmmc-supply = <&sd_vcc_reg>;
> > > > > broken-cd;
> > > > > + /*
> > > > > +* Force the frequency to 150MHz as the default 100MHz seems
> > > > > to
> > > > > +* interfere with the VPU clock when setup at frequencies
> > > > > bigger than
> > > > > +* 500MHz, causing unwarranted CMD hangs.
> > > > > +*/
> > > > > + clock-frequency = <15000>;
> > > > i don't want to bike-shed here, but is there any chance to solve this in
> > > > clk-bcm2835 in a less hacky way?
> > > What do you have in mind?
> > Sorry, nothing specific.
> > >
> > > All I can think of is adding some kind of heuristic to the clock's
> > > prepare()
> > > callback. That said, I don't feel it would be a better solution than this.
> >
> > Based on my limited knowledge and an old SD card specification, all
> > possibly connected devices could have different frequencies. So my
> > concern here is, that in case we limit the frequency to a specific value
> > we could break things just to suppress a warning.
>
> SDHCI should be able to handle up to 233MHz IIRC, and there are divisors
> available, it depends on the implementation but the worst kind provide /2^n.
> Not perfect, but good enough for things to work.
>
> Now, I've been having a deeper look into how clocks are handled, and found two
> new clues:
>
> - First of all RPi4's sdhci-iproc needs SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
>that is, the controller isn't properly identifying the clock frequency fed
>into it, and defaults to saying it's configured at 100MHz. I'm not an SDHCI
>expert, so it's possible changing frequencies also needs a special
> operation
>to recalculate this variable. But this was making all internal calculations
>wrong when paired with this series.
>
> - With this flag set SDHCI's core now properly calculates divisor values
> based
>on whatever clock frequency I set in DT. And guess what, the issue
> reappears
>even when running on 150MHz. It turns out, as I had some debugging enabled,
>the issue only happens when the controller is configured at 100KHz (that
>only happens while running the card detect thread).
>
> So, I can now do this (note that for card detection try to communicate with
> the
> card starting at 400KHz down to 100KHz in 100KHz steps):
>
> ->8-
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 536c382e2486..e5a5de63f347 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -173,6 +173,11 @@ static
Re: [PATCH] mmc: brcmstb: Fix sdhci_pltfm_suspend link error
> I just posted a new patch for this, please have a look and test it.
>
> Kind regards
> Uffe
I tested your new patch and it works.
Reviewed-and-tested-by: Al Cooper
Thanks
Al
On Tue, Jan 26, 2021 at 4:55 AM Ulf Hansson wrote:
>
> On Mon, 25 Jan 2021 at 18:40, Florian Fainelli wrote:
> >
> > +Nicolas,
> >
> > On 1/25/2021 4:50 AM, Arnd Bergmann wrote:
> > > From: Arnd Bergmann
> > >
> > > sdhci_pltfm_suspend() is only available when CONFIG_PM_SLEEP
> > > support is built into the kernel, which caused a regression
> > > in a recent bugfix:
> > >
> > > ld.lld: error: undefined symbol: sdhci_pltfm_suspend
> > referenced by sdhci-brcmstb.c
> > mmc/host/sdhci-brcmstb.o:(sdhci_brcmstb_shutdown) in
> > archive drivers/built-in.a
> > >
> > > Making the call conditional on the symbol fixes the link
> > > error.
> > >
> > > Fixes: 5b191dcba719 ("mmc: sdhci-brcmstb: Fix mmc timeout errors on S5
> > > suspend")
> > > Fixes: e7b5d63a82fe ("mmc: sdhci-brcmstb: Add shutdown callback")
> > > Cc: sta...@vger.kernel.org
> > > Signed-off-by: Arnd Bergmann
> > > ---
> > > It would be helpful if someone could test this to ensure that the
> > > driver works correctly even when CONFIG_PM_SLEEP is disabled
> >
> > Why not create stubs for sdhci_pltfm_suspend() when CONFIG_PM_SLEEP=n? I
> > don't think this is going to be a functional issue given that the
> > purpose of having the .shutdown() function is to save power if we cannot
> > that is fine, too.
> > --
> > Florian
>
> I would prefer this approach - we shouldn't leave stub functions
> unimplemented, which is what looks to me.
>
> I just posted a new patch for this, please have a look and test it.
>
> Kind regards
> Uffe
Re: [PATCH 3/6] dt-bindings: mtu3: add devicetree bindings
On Thu, May 12, 2016 at 3:24 AM, chunfeng yun wrote: >> > + - mediatek,enable-manual-drd : supports manual dual-role switch by sysfs >> > + interface; only used when receptacle is TYPE-A and also wants to >> > support >> > + dual-role mode. >> >> sysfs is a Linux detail that doesn't apply to the binding. Does the >> property mean "the IP block supports or doesn't support switching" or >> "I want to enable switching feature". Only the former belongs in DT. >> > It is the former case, and has little relation with sysfs interface. > I will modify it later, sorry for my unclear description. Could the property name be just "enable-manual-drd" instead of "mediatek,enable-manual-drd"? I expect to have to add this functionality to our USB driver in the near future.
Re: [RFC 0/6] mmc: Field Firmware Update
On Fri, Nov 13, 2015 at 9:56 AM, Holger Schurig wrote: > There have been some attempts to add FFU (field firmware update). The last > AFAIK in Nov 2014, http://www.spinics.net/lists/linux-mmc/msg29324.html > > But it seems that the committers weren't persistent enought. > > I took the liberty to take Avi's patch and make it hopefully > maintainer-review friendly. > > The first 5 patches just move functions out of mmc_test.c into core.c. Those > functions will later be used by both mmc_test.c and mmc_ffu.c. Contrary to > Avi's patch I didn't add static helper functions to mmc_test.c, e.g. > there's no mmc_test_prepare_mrq() that calls mmc_prepare_mrq(). It's > simpler to call mmc_prepare_mrq() directly. It's just one more dereference > from *mmc_card to *mmc_test_card anyway. > > The patch [6/6] is http://www.spinics.net/lists/linux-mmc/msg29326.html, but > with less checkpatch warnings. And it doesn't use mmc_send_ext_csd() > anymore, which has been deleted since November. > > I'm sending this patch as RFC now. It compiles (for me). But I get the > firmware update file from Kingston only next Tuesday. That means that so > far I haven't been testing it. It won't do anything without the proper > user-space command in mmc-utils anyway :-) > > Comments welcome (I intent to get this patch into the kernel) > > The patch is against Linux GIT (v4.3-11748-g46d862b). > > Holger > > drivers/mmc/card/Kconfig| 11 + > drivers/mmc/card/Makefile | 1 + > drivers/mmc/card/block.c| 5 + > drivers/mmc/card/mmc_ffu.c | 489 > > drivers/mmc/card/mmc_test.c | 235 + > drivers/mmc/core/core.c | 134 > drivers/mmc/core/mmc_ops.c | 4 +- > include/linux/mmc/card.h| 1 + > include/linux/mmc/core.h| 41 > include/linux/mmc/mmc.h | 6 + > 10 files changed, 739 insertions(+), 188 deletions(-) > create mode 100644 drivers/mmc/card/mmc_ffu.c The newly added ioctl MMC_IOC_MULTI_CMD allows user space to send multiple commands atomically, so mmu-utils may be a better place for this functionality. Al -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mmc: sdhci: Fix incorrect switch to HS mode
This does correct the sequence of switching to HS400 but it might be safest to just add this to the latest until it gets a little testing to make sure it doesn't expose some bug in existing controllers. Thanks Al On Tue, Sep 3, 2019 at 10:52 AM Ulf Hansson wrote: > > On Tue, 3 Sep 2019 at 13:51, Al Cooper wrote: > > > > When switching from any MMC speed mode that requires 1.8v > > (HS200, HS400 and HS400ES) to High Speed (HS) mode, the system > > ends up configured for SDR12 with a 50MHz clock which is an illegal > > mode. > > > > This happens because the SDHCI_CTRL_VDD_180 bit in the > > SDHCI_HOST_CONTROL2 register is left set and when this bit is > > set, the speed mode is controlled by the SDHCI_CTRL_UHS field > > in the SDHCI_HOST_CONTROL2 register. The SDHCI_CTRL_UHS field > > will end up being set to 0 (SDR12) by sdhci_set_uhs_signaling() > > because there is no UHS mode being set. > > > > The fix is to change sdhci_set_uhs_signaling() to set the > > SDHCI_CTRL_UHS field to SDR25 (which is the same as HS) for > > any switch to HS mode. > > > > This was found on a new eMMC controller that does strict checking > > of the speed mode and the corresponding clock rate. It caused the > > switch to HS400 mode to fail because part of the sequence to switch > > to HS400 requires a switch from HS200 to HS before going to HS400. > > > > This fix was suggested by Adrian Hunter > > > > Signed-off-by: Al Cooper > > Should this be applied for fixes and tagged for stable you think? > > Kind regards > Uffe > > > --- > > drivers/mmc/host/sdhci.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index 61d845fe0b97..068149640ecd 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -1858,7 +1858,9 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, > > unsigned timing) > > ctrl_2 |= SDHCI_CTRL_UHS_SDR104; > > else if (timing == MMC_TIMING_UHS_SDR12) > > ctrl_2 |= SDHCI_CTRL_UHS_SDR12; > > - else if (timing == MMC_TIMING_UHS_SDR25) > > + else if (timing == MMC_TIMING_SD_HS || > > +timing == MMC_TIMING_MMC_HS || > > +timing == MMC_TIMING_UHS_SDR25) > > ctrl_2 |= SDHCI_CTRL_UHS_SDR25; > > else if (timing == MMC_TIMING_UHS_SDR50) > > ctrl_2 |= SDHCI_CTRL_UHS_SDR50; > > -- > > 2.17.1 > >
Generic PHY "unload" crash
I'm seeing an issue on a system where I have a generic PHY that is used by a USB XHCI driver. The XHCI driver does the phy_init() in probe and the phy_exit() in remove. The problem happens when I use sysfs to "unload" the PHY driver before doing an unload of the XHCI driver. This is a result of the XHCI driver calling into the PHY driver's phy_exit routine after it's been removed. It seems like this issue will exist for other PHY provider/user pairs and I don't see an easy solution. I was wondering if anyone had any suggestions on how to solve this problem. Al
Re: Generic PHY "unload" crash
Here the crash dump: # echo >unbind 8b39000.xhci_v2 [168374.367560] xhci-brcm 8b39000.xhci_v2: remove, state 4 [168374.372914] usb usb2: USB disconnect, device number 1 [168374.379314] xhci-brcm 8b39000.xhci_v2: USB bus 2 deregistered [168374.385771] xhci-brcm 8b39000.xhci_v2: remove, state 1 [168374.391170] usb usb1: USB disconnect, device number 1 [168374.396425] usb 1-1: USB disconnect, device number 2 [168374.416980] xhci-brcm 8b39000.xhci_v2: USB bus 1 deregistered [168374.427437] Unable to handle kernel paging request at virtual address d1265234 [168374.434916] pgd = cc887ac0 [168374.437843] [d1265234] *pgd=8040007003, *pmd=4ca1a003, *pte= [168374.444872] Internal error: Oops: 207 [#1] SMP ARM [168374.449824] Modules linked in: [168374.453045] CPU: 0 PID: 1699 Comm: sh Not tainted 4.9.135-1.12pre #2 [168374.459544] Hardware name: Broadcom STB (Flattened Device Tree) [168374.465614] task: cc9361c0 task.stack: cb3b [168374.470306] PC is at usb_uninit_xhci+0x40/0x8c [168374.474960] LR is at brcm_usb_phy_exit+0x110/0x130 [168374.479933] pc : []lr : []psr: 200e0113 [168374.479933] sp : cb3b1d78 ip : cb3b1d90 fp : cb3b1d8c [168374.491698] r10: cc831d0c r9 : cb218a80 r8 : cb3b1f68 [168374.497096] r7 : cc960990 r6 : cc960800 r5 : cd34f38c r4 : cd34f310 [168374.503811] r3 : d1265200 r2 : c0c2dfa0 r1 : 0040 r0 : cd34f310 [168374.510523] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [168374.517901] Control: 30c5383d Table: 4c887ac0 DAC: fffd [168374.523899] Process sh (pid: 1699, stack limit = 0xcb3b0210) [168374.529718] Stack: (0xcb3b1d78 to 0xcb3b2000) [168374.534216] 1d60: cd34f310 cd34f38c [168374.542578] 1d80: cb3b1dac cb3b1d90 c05aa608 c05ab83c c05aa4f8 cc960800 cc960990 [168374.550937] 1da0: cb3b1dcc cb3b1db0 c05a9104 c05aa504 cb12b000 cb12b000 0010 c1a76a0c [168374.559346] 1dc0: cb3b1de4 cb3b1dd0 c07da9c0 c05a906c cd368410 cd368410 cb3b1dfc cb3b1de8 [168374.567717] 1de0: c068a4b0 c07da980 cd368410 c1a76a0c cb3b1e14 cb3b1e00 c0687a6c c068a488 [168374.576082] 1e00: cd368444 cd368410 cb3b1e2c cb3b1e18 c0687b34 c06879e0 c1a69a38 cd368410 [168374.584443] 1e20: cb3b1e4c cb3b1e30 c06866f4 c0687b14 c068666c cc831d00 [168374.592804] 1e40: cb3b1e64 cb3b1e50 c0685874 c0686678 c0685844 cc831d00 cb3b1e7c cb3b1e68 [168374.601223] 1e60: c03d1eec c0685850 0010 cc831d00 cb3b1eb4 cb3b1e80 c03d165c c03d1ea8 [168374.609596] 1e80: c036f488 c1a03088 c16ab240 c03d1580 cb3b1f68 [168374.618018] 1ea0: 000df2a0 0010 cb3b1f34 cb3b1eb8 c035d9f4 c03d158c 0052 [168374.626426] 1ec0: 00040987 000a cd2efe00 cc887780 000a [168374.634850] 1ee0: 0400 c037f90c cb3b1f24 cb3b1ef8 c037ef4c 00040987 [168374.643217] 1f00: cb263a80 00040987 0010 c16ab240 000df2a0 cb3b1f68 [168374.651577] 1f20: 000df2a0 0010 cb3b1f64 cb3b1f38 c035e8e4 c035d9b8 c16ab240 c037f44c [168374.659929] 1f40: c1a03088 c16ab240 c16ab240 000df2a0 cb3b1fa4 cb3b1f68 [168374.668274] 1f60: c035f804 c035e83c c037f2d4 00040987 000e0a48 0010 [168374.676618] 1f80: 000df2a0 b6eddd60 0004 c02092a4 cb3b 0004 cb3b1fa8 [168374.684961] 1fa0: c0209100 c035f7b4 0010 000df2a0 0001 000df2a0 0010 [168374.693303] 1fc0: 0010 000df2a0 b6eddd60 0004 000df2a0 0010 000b8228 000b57d8 [168374.701646] 1fe0: befc5a1c b6e40bab b6e7d2c6 00070030 0001 [168374.710015] [] (usb_uninit_xhci) from [] (brcm_usb_phy_exit+0x110/0x130) [168374.718628] [] (brcm_usb_phy_exit) from [] (phy_exit+0xa4/0xc8) [168374.726458] [] (phy_exit) from [] (xhci_brcm_remove+0x4c/0x70) [168374.734202] [] (xhci_brcm_remove) from [] (platform_drv_remove+0x34/0x4c) [168374.742904] [] (platform_drv_remove) from [] (__device_release_driver+0x98/0x134) [168374.752298] [] (__device_release_driver) from [] (device_release_driver+0x2c/0x38) [168374.761778] [] (device_release_driver) from [] (unbind_store+0x88/0x108) [168374.770385] [] (unbind_store) from [] (drv_attr_store+0x30/0x3c) [168374.778303] [] (drv_attr_store) from [] (sysfs_kf_write+0x50/0x54) [168374.786396] [] (sysfs_kf_write) from [] (kernfs_fop_write+0xdc/0x1b8) [168374.794746] [] (kernfs_fop_write) from [] (__vfs_write+0x48/0x140) [168374.802832] [] (__vfs_write) from [] (vfs_write+0xb4/0x178) [168374.810306] [] (vfs_write) from [] (SyS_write+0x5c/0xbc) [168374.817505] [] (SyS_write) from [] (ret_fast_syscall+0x0/0x1c) [168374.825242] Code: e5943000 e592101c e351 0a06 (e5932034) [168374.831584] ---[ end trace 91aec3f8a9835a40 ]--- Thanks Al On Tue, May 21, 2019 at 3:15 PM Kishon Vijay Abraham I wrote: > > Hi Alan, > > On 21/05/19 1:40 PM, Alan Cooper wrote: > > I'm seeing an issue on a system where I have a generic PHY that is > > used by a USB XHCI driver. The XHCI driver does the
Re: Issue with sequence to switch to HS400
That's an even better solution and it gets my HS400 mode working. Will you add this change or should I? Thanks Al On Thu, Jul 25, 2019 at 3:33 AM Adrian Hunter wrote: > > On 23/07/19 3:34 PM, Alan Cooper wrote: > > On Tue, Jul 23, 2019 at 1:21 AM Adrian Hunter > > wrote: > >> > >> On 23/07/19 1:31 AM, Alan Cooper wrote: > >>> I'm having a problem with a new SD/MMC controller and PHY in our > >>> latest SoC's. The issue I'm seeing is that I can't switch into HS400 > >>> mode. This looks like something the driver is doing that doesn't meet > >>> the JEDEC spec. In the "HS400 timing mode selection" section of the > >>> JEDEC spec , in step 7 it states: > >>> > >>> 7) Set the “Timing Interface” parameter in the HS_TIMING [185] field > >>> of the Extended CSD register to 0x1 to switch to High Speed mode and > >>> then set the clock frequency to a value not greater than 52 MHz. > >>> > >>> In the function mmc_select_hs400() in mmc.c, I see that a switch > >>> command is done to set the eMMC device to HS mode and then > >>> mmc_set_timing(card->host, MMC_TIMING_MMC_HS) is used to change the > >>> controller to HS mode. The problem is that the "SD Host Controller > >>> Standard Specification" states that "UHS Mode Select" field of the > >>> "Host Control 2 Register" controls the mode when the "1.8V Signaling > >>> Enable" bit in the same register is set, so mmc_set_timing() is > >>> actually leaving the controller in SDR12 mode and mmc_select_hs400() > >>> will then set the clock to 52MHz. This causes our PHY to detect an > >>> illegal combination and return an error. > >>> > >>> I think the easiest fix would be to change mmc_set_timing(card->host, > >>> MMC_TIMING_MMC_HS) to mmc_set_timing(card->host, > >>> MMC_TIMING_UHS_SDR25). The other possibility would be to change > >>> mmc_set_timing to handle the "1.8V Signaling Enable" bit properly. > >>> I'll submit a patch based on the feedback I get. > >> > >> eMMC is governed by JEDEC specs not SD specs. > > > > My understanding is that JEDEC does not have a host controller spec so > > this driver uses the "SD Host Controller Standard Specification". > > There is no spec for using eMMC with SDHCI. > > > > >> > >> Please consider making a change in your driver instead. For example, hook > >> ->set_ios() and if 1.8V is enabled and timing is set to MMC_TIMING_MMC_HS > >> then change it to MMC_TIMING_UHS_SDR25. > > > > That's an easy fix, but it still leaves all other drivers/systems > > temporarily using SDR12 at 52MHz during the switch to HS400. > > Yes, I changed my mind. Does this work: > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 59acf8e3331e..f9d241458dcd 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -1849,7 +1849,9 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, > unsigned timing) > ctrl_2 |= SDHCI_CTRL_UHS_SDR104; > else if (timing == MMC_TIMING_UHS_SDR12) > ctrl_2 |= SDHCI_CTRL_UHS_SDR12; > - else if (timing == MMC_TIMING_UHS_SDR25) > + else if (timing == MMC_TIMING_SD_HS || > +timing == MMC_TIMING_MMC_HS || > +timing == MMC_TIMING_UHS_SDR25) > ctrl_2 |= SDHCI_CTRL_UHS_SDR25; > else if (timing == MMC_TIMING_UHS_SDR50) > ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
Re: Issue with sequence to switch to HS400
No problem. Thanks Al On Tue, Jul 30, 2019 at 4:00 AM Adrian Hunter wrote: > > On 26/07/19 12:37 AM, Alan Cooper wrote: > > That's an even better solution and it gets my HS400 mode working. > > Will you add this change or should I? > > You, if you wouldn't mind. > > > > > Thanks > > Al > > > > On Thu, Jul 25, 2019 at 3:33 AM Adrian Hunter > > wrote: > >> > >> On 23/07/19 3:34 PM, Alan Cooper wrote: > >>> On Tue, Jul 23, 2019 at 1:21 AM Adrian Hunter > >>> wrote: > >>>> > >>>> On 23/07/19 1:31 AM, Alan Cooper wrote: > >>>>> I'm having a problem with a new SD/MMC controller and PHY in our > >>>>> latest SoC's. The issue I'm seeing is that I can't switch into HS400 > >>>>> mode. This looks like something the driver is doing that doesn't meet > >>>>> the JEDEC spec. In the "HS400 timing mode selection" section of the > >>>>> JEDEC spec , in step 7 it states: > >>>>> > >>>>> 7) Set the “Timing Interface” parameter in the HS_TIMING [185] field > >>>>> of the Extended CSD register to 0x1 to switch to High Speed mode and > >>>>> then set the clock frequency to a value not greater than 52 MHz. > >>>>> > >>>>> In the function mmc_select_hs400() in mmc.c, I see that a switch > >>>>> command is done to set the eMMC device to HS mode and then > >>>>> mmc_set_timing(card->host, MMC_TIMING_MMC_HS) is used to change the > >>>>> controller to HS mode. The problem is that the "SD Host Controller > >>>>> Standard Specification" states that "UHS Mode Select" field of the > >>>>> "Host Control 2 Register" controls the mode when the "1.8V Signaling > >>>>> Enable" bit in the same register is set, so mmc_set_timing() is > >>>>> actually leaving the controller in SDR12 mode and mmc_select_hs400() > >>>>> will then set the clock to 52MHz. This causes our PHY to detect an > >>>>> illegal combination and return an error. > >>>>> > >>>>> I think the easiest fix would be to change mmc_set_timing(card->host, > >>>>> MMC_TIMING_MMC_HS) to mmc_set_timing(card->host, > >>>>> MMC_TIMING_UHS_SDR25). The other possibility would be to change > >>>>> mmc_set_timing to handle the "1.8V Signaling Enable" bit properly. > >>>>> I'll submit a patch based on the feedback I get. > >>>> > >>>> eMMC is governed by JEDEC specs not SD specs. > >>> > >>> My understanding is that JEDEC does not have a host controller spec so > >>> this driver uses the "SD Host Controller Standard Specification". > >> > >> There is no spec for using eMMC with SDHCI. > >> > >>> > >>>> > >>>> Please consider making a change in your driver instead. For example, > >>>> hook > >>>> ->set_ios() and if 1.8V is enabled and timing is set to MMC_TIMING_MMC_HS > >>>> then change it to MMC_TIMING_UHS_SDR25. > >>> > >>> That's an easy fix, but it still leaves all other drivers/systems > >>> temporarily using SDR12 at 52MHz during the switch to HS400. > >> > >> Yes, I changed my mind. Does this work: > >> > >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > >> index 59acf8e3331e..f9d241458dcd 100644 > >> --- a/drivers/mmc/host/sdhci.c > >> +++ b/drivers/mmc/host/sdhci.c > >> @@ -1849,7 +1849,9 @@ void sdhci_set_uhs_signaling(struct sdhci_host > >> *host, unsigned timing) > >> ctrl_2 |= SDHCI_CTRL_UHS_SDR104; > >> else if (timing == MMC_TIMING_UHS_SDR12) > >> ctrl_2 |= SDHCI_CTRL_UHS_SDR12; > >> - else if (timing == MMC_TIMING_UHS_SDR25) > >> + else if (timing == MMC_TIMING_SD_HS || > >> +timing == MMC_TIMING_MMC_HS || > >> +timing == MMC_TIMING_UHS_SDR25) > >> ctrl_2 |= SDHCI_CTRL_UHS_SDR25; > >> else if (timing == MMC_TIMING_UHS_SDR50) > >> ctrl_2 |= SDHCI_CTRL_UHS_SDR50; > > >
Issue with sequence to switch to HS400
I'm having a problem with a new SD/MMC controller and PHY in our latest SoC's. The issue I'm seeing is that I can't switch into HS400 mode. This looks like something the driver is doing that doesn't meet the JEDEC spec. In the "HS400 timing mode selection" section of the JEDEC spec , in step 7 it states: 7) Set the “Timing Interface” parameter in the HS_TIMING [185] field of the Extended CSD register to 0x1 to switch to High Speed mode and then set the clock frequency to a value not greater than 52 MHz. In the function mmc_select_hs400() in mmc.c, I see that a switch command is done to set the eMMC device to HS mode and then mmc_set_timing(card->host, MMC_TIMING_MMC_HS) is used to change the controller to HS mode. The problem is that the "SD Host Controller Standard Specification" states that "UHS Mode Select" field of the "Host Control 2 Register" controls the mode when the "1.8V Signaling Enable" bit in the same register is set, so mmc_set_timing() is actually leaving the controller in SDR12 mode and mmc_select_hs400() will then set the clock to 52MHz. This causes our PHY to detect an illegal combination and return an error. I think the easiest fix would be to change mmc_set_timing(card->host, MMC_TIMING_MMC_HS) to mmc_set_timing(card->host, MMC_TIMING_UHS_SDR25). The other possibility would be to change mmc_set_timing to handle the "1.8V Signaling Enable" bit properly. I'll submit a patch based on the feedback I get. Thanks Al Cooper
Re: Issue with sequence to switch to HS400
On Tue, Jul 23, 2019 at 1:21 AM Adrian Hunter wrote: > > On 23/07/19 1:31 AM, Alan Cooper wrote: > > I'm having a problem with a new SD/MMC controller and PHY in our > > latest SoC's. The issue I'm seeing is that I can't switch into HS400 > > mode. This looks like something the driver is doing that doesn't meet > > the JEDEC spec. In the "HS400 timing mode selection" section of the > > JEDEC spec , in step 7 it states: > > > > 7) Set the “Timing Interface” parameter in the HS_TIMING [185] field > > of the Extended CSD register to 0x1 to switch to High Speed mode and > > then set the clock frequency to a value not greater than 52 MHz. > > > > In the function mmc_select_hs400() in mmc.c, I see that a switch > > command is done to set the eMMC device to HS mode and then > > mmc_set_timing(card->host, MMC_TIMING_MMC_HS) is used to change the > > controller to HS mode. The problem is that the "SD Host Controller > > Standard Specification" states that "UHS Mode Select" field of the > > "Host Control 2 Register" controls the mode when the "1.8V Signaling > > Enable" bit in the same register is set, so mmc_set_timing() is > > actually leaving the controller in SDR12 mode and mmc_select_hs400() > > will then set the clock to 52MHz. This causes our PHY to detect an > > illegal combination and return an error. > > > > I think the easiest fix would be to change mmc_set_timing(card->host, > > MMC_TIMING_MMC_HS) to mmc_set_timing(card->host, > > MMC_TIMING_UHS_SDR25). The other possibility would be to change > > mmc_set_timing to handle the "1.8V Signaling Enable" bit properly. > > I'll submit a patch based on the feedback I get. > > eMMC is governed by JEDEC specs not SD specs. My understanding is that JEDEC does not have a host controller spec so this driver uses the "SD Host Controller Standard Specification". > > Please consider making a change in your driver instead. For example, hook > ->set_ios() and if 1.8V is enabled and timing is set to MMC_TIMING_MMC_HS > then change it to MMC_TIMING_UHS_SDR25. That's an easy fix, but it still leaves all other drivers/systems temporarily using SDR12 at 52MHz during the switch to HS400.
[QUESTION] kexec: ARM: kexec reorders device suspend/resume order
I've found a problem on our ARM based systems where a kexec'd kernel fails coming out of S3. The problem is caused by the re-ordering of the device tree nodes done by kexec (which reconstructs the device tree from the proc file system). The re-ordered DT nodes cause the device registration to change order which in turn changes the suspend/resume order. This is breaking a few of our drivers that have dependencies on other drivers. We ordered the original DT to handle these dependencies but the kexec re-order breaks this. I can fix this by making the dependencies between drivers explicit but I was wondering if anyone had ideas on maintaining the original DT node order on kexec? Thanks Al
Re: [PATCH 1/3] dt-bindings: Add support for Broadcom USB pin map driver
On Mon, Aug 24, 2020 at 7:30 PM Rob Herring wrote:
>
> On Wed, Aug 12, 2020 at 04:20:16PM -0400, Al Cooper wrote:
> > Add DT bindings for the Broadcom USB pin map driver. This driver allows
> > some USB input and output signals to be mapped to any GPIO instead
> > of the normal dedicated pins to/from the XHCI controller.
>
> Is this a driver or h/w block because bindings are for h/w blocks?
This is a hardware block. I'll remove "driver" from the description.
>
> >
> > Signed-off-by: Al Cooper
> > ---
> > .../bindings/usb/brcm,usb-pinmap.yaml | 63 +++
> > 1 file changed, 63 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml
> > b/Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml
> > new file mode 100644
> > index ..19cf6ad36373
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml
> > @@ -0,0 +1,63 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/brcm,usb-pinmap.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Broadcom USB pin map Controller Device Tree Bindings
> > +
> > +maintainers:
> > + - Al Cooper
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - const: brcm,usb-pinmap
>
> 2 space indentation please.
Fixed.
>
> > +
> > + reg:
> > +maxItems: 1
> > +
> > + interrupts:
> > +maxItems: 1
> > +description: Must be defined if any out-gpios are specified.
>
> 'dependencies' can express this in schema.
Okay.
>
> > +
> > + in-gpios:
> > +description: Array of one or more GPIO pins used for input signals.
>
> You need to define how many GPIOs are valid.
I tried to avoid doing this because there is a possibility that future
chips will have a few more signals added and the driver was written so
new signals can be added entirely in device tree without any changes
to the driver. If this is unacceptable, I can add the current max in
and out valid gpios.
>
> > +
> > + in-names:
> > +description: Array of input signal names, one per gpio in in-gpios.
>
> No, this isn't how we name GPIOs. The part before '-gpios' is how.
This is the meant to be a description of how each gpio is being used
to help with error messages in the driver.
What if I use "brcmstb,in-functions" instead?
>
> > +
> > + in-masks:
> > +description: Array of enable and mask pairs, one per gpio in-gpios.
>
> Needs a vendor prefix.
I'll change it to "brcmstb,in-masks"
>
> > +
> > + out-gpios:
> > +description: Array of one or more GPIO pins used for output signals.
> > +
> > + out-names:
> > +description: Array of output signal names, one per gpio in out-gpios.
> > +
> > + out-masks:
> > +description: Array of enable, value, changed and clear masks, one
> > + per gpio in out-gpios.
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +usb_pinmap: usb-pinmap@22000d0 {
> > +compatible = "brcm,usb-pinmap";
> > +reg = <0x22000d0 0x4>;
> > +in-gpios = <&gpio 18 0>, <&gpio 19 0>;
> > +in-names = "VBUS", "PWRFLT";
> > +in-masks = <0x8000 0x4 0x1 0x8>;
> > +out-gpios = <&gpio 20 0>;
> > +out-names = "PWRON";
> > +out-masks = <0x2 0x80 0x40 0x20>;
> > +interrupts = <0x0 0xb2 0x4>;
> > +};
> > +
> > +...
> > --
> > 2.17.1
> >
Re: [PATCH 1/3] dt-bindings: Add support for Broadcom USB pin map driver
On Tue, Aug 25, 2020 at 11:46 AM Rob Herring wrote: > > +Linus W > > On Tue, Aug 25, 2020 at 6:26 AM Alan Cooper wrote: > > > > On Mon, Aug 24, 2020 at 7:30 PM Rob Herring wrote: > > > > > > On Wed, Aug 12, 2020 at 04:20:16PM -0400, Al Cooper wrote: > > > > Add DT bindings for the Broadcom USB pin map driver. This driver allows > > > > some USB input and output signals to be mapped to any GPIO instead > > > > of the normal dedicated pins to/from the XHCI controller. > > > > > > Is this a driver or h/w block because bindings are for h/w blocks? > > > > This is a hardware block. I'll remove "driver" from the description. > > Another question, this kind of looks like a pin mux controller. Is > that not a fit for this? If not, why? This driver is not doing any pin-muxing of a physical pin on the chip. Instead it's using standard gpio's, through gpiolib, and propagating the gpio state for in-coming signals to a special register that feeds into a XHCI host controller register and it's propagating the state of out-going signals from the special register fed by a XHCI controller register to a gpio. Both directions are interrupt driven and continually mirroring the state between the XHCI host controller registers and the gpios. I don't see any pinmux/pinctrl driver doing this kind of thing. Thanks Al > > > > > Signed-off-by: Al Cooper > > > > --- > > > > .../bindings/usb/brcm,usb-pinmap.yaml | 63 +++ > > > > 1 file changed, 63 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml > > > > b/Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml > > > > new file mode 100644 > > > > index ..19cf6ad36373 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml > > > > @@ -0,0 +1,63 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/usb/brcm,usb-pinmap.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Broadcom USB pin map Controller Device Tree Bindings > > > > + > > > > +maintainers: > > > > + - Al Cooper > > > > + > > > > +properties: > > > > + compatible: > > > > + items: > > > > + - const: brcm,usb-pinmap > > > > > > 2 space indentation please. > > > > Fixed. > > > > > > > > > + > > > > + reg: > > > > +maxItems: 1 > > > > + > > > > + interrupts: > > > > +maxItems: 1 > > > > +description: Must be defined if any out-gpios are specified. > > > > > > 'dependencies' can express this in schema. > > > > Okay. > > > > > > > > > + > > > > + in-gpios: > > > > +description: Array of one or more GPIO pins used for input signals. > > > > > > You need to define how many GPIOs are valid. > > > > I tried to avoid doing this because there is a possibility that future > > chips will have a few more signals added and the driver was written so > > new signals can be added entirely in device tree without any changes > > to the driver. If this is unacceptable, I can add the current max in > > and out valid gpios. > > A 'should be enough for a while' value is fine. The driver doesn't > have to have a max. I'd expect the binding to be updated for new SoCs > anyways. > > > > > > > > + > > > > + in-names: > > > > +description: Array of input signal names, one per gpio in in-gpios. > > > > > > No, this isn't how we name GPIOs. The part before '-gpios' is how. > > > > This is the meant to be a description of how each gpio is being used > > to help with error messages in the driver. > > What if I use "brcmstb,in-functions" instead? > > 'brcmstb' is not a vendor. But brcm,in-functions is fine. > > > > > + > > > > + in-masks: > > > > +description: Array of enable and mask pairs, one per gpio in-gpios. > > > > > > Needs a vendor prefix. > > > > I
Re: [PATCH v7 4/5] usb: ehci: Add new EHCI driver for Broadcom STB SoC's
On Fri, May 8, 2020 at 2:46 PM Alan Stern wrote: > > On Thu, 7 May 2020, Al Cooper wrote: > > > Add a new EHCI driver for Broadcom STB SoC's. A new EHCI driver > > was created instead of adding support to the existing ehci platform > > driver because of the code required to workaround bugs in the EHCI > > controller. The primary workround is for a bug where the Core > > violates the SOF interval between the first two SOFs transmitted after > > resume. This only happens if the resume occurs near the end of a > > microframe. The fix is to intercept the echi-hcd request to complete > > RESUME and align it to the start of the next microframe. > > > > Signed-off-by: Al Cooper > > Reviewed-by: Andy Shevchenko > > Adding a new EHCI platform-specific driver is okay with me. However, > this patch does not include most of the changes you discussed with > Greg. I assume you will submit a revised version with those changes in > place; when you do I will Ack it. > > Alan Stern > I mistakenly sent a partially fixed version instead of the version with all the fixes (git stashed). I'm sending v8 will all the fixes. Thanks Thanks Al
Re: [PATCH v10 1/5] usb: xhci: Change the XHCI link order in the Makefile
On Wed, May 13, 2020 at 1:46 PM Florian Fainelli wrote:
>
>
>
> On 5/13/2020 10:39 AM, Alan Stern wrote:
> > On Wed, May 13, 2020 at 07:05:05PM +0200, Greg Kroah-Hartman wrote:
> >> On Wed, May 13, 2020 at 09:31:11AM -0700, Florian Fainelli wrote:
> >>>
> >>>
> >>> On 5/13/2020 9:27 AM, Greg Kroah-Hartman wrote:
> On Wed, May 13, 2020 at 08:08:07AM -0700, Florian Fainelli wrote:
> >
> >
> > On 5/13/2020 5:26 AM, Greg Kroah-Hartman wrote:
> >> On Tue, May 12, 2020 at 11:00:15AM -0400, Al Cooper wrote:
> >>> Some BRCMSTB USB chips have an XHCI, EHCI and OHCI controller
> >>> on the same port where XHCI handles 3.0 devices, EHCI handles 2.0
> >>> devices and OHCI handles <2.0 devices. Currently the Makefile
> >>> has XHCI linking at the bottom which will result in the XHIC driver
> >>> initalizing after the EHCI and OHCI drivers and any installed 3.0
> >>> device will be seen as a 2.0 device. Moving the XHCI linking
> >>> above the EHCI and OHCI linking fixes the issue.
> >>
> >> What happens if all of these are modules and they are loaded in a
> >> different order? This makefile change will not help with that, you
> >> need
> >> to have logic in the code in order to properly coordinate this type of
> >> mess, sorry.
> >
> > I believe we should be using module soft dependencies to instruct the
> > module loaders to load the modules in the correct order, so something
> > like this would do (not tested) for xhci-plat-hcd.c:
> >
> > MODULE_SOFTDEP("post: ehci-hcd ohci-hcd");
> >
> > and I am not sure whether we need to add the opposite for ehci-hcd and
> > ohci-hcd:
> >
> > MODULE_SOFTDEP("pre: xhci-plat-hcd");
>
> That's a nice start, but what happens if that isn't honored? This
> really needs to work properly for any order as you never can guarantee
> module/driver loading order in a system of modules.
> >>>
> >>> I also suggested that device links may help, though I am not sure. What
> >>> do you suggest to be done?
> >>
> >> No idea. device links will help if you defer the probe properly until
> >> you see the proper drivers binding correctly.
> >
> > I suspect that in general there is no way to do this properly.
> >
> > We can't modify ehci-hcd and ohci-hcd to make them wait. In fact, for
> > all they know, xhci-hcd will _never_ be loaded.
> >
> > One thing that might be possible (although not all platforms may support
> > it) is if xhci-hcd could somehow disconnect all devices attached to a
> > peer port when it starts up. But that would be disruptive to any
> > devices that aren't USB-3.
> >
> > We faced a very similar ordering problem between ehci-hcd and
> > [ou]hci-hcd many years ago, and we never found a good solution.
> > We did arrange the link order so that ehci-hcd precedes the others, and
> > we added a warning message to ehci-hcd which gets printed if the module
> > initialization routine runs after [ou]hci-hcd is loaded. Also, there
> > are MODULE_SOFTDEP lines in ohci-pci.c and uhci-pci.c.
>
> Given that these modules are used on specific SoC platforms, where we
> usually provide a reference implementation of user space and kernel
> space and documentation, it seems to me that the MODULE_SOFTDEP(),
> despite being a hint and best effort from user space module loaders is
> probably acceptable.
> --
> Florian
What I found in the past is that things work. For example if the ehci
driver starts first, the USB device will come up as a 2.0 device and
when the XHCI driver comes up the device will switch to 3.0. I've see
the same thing happen if OHCI starts before EHCI. It's just that there
are some poorly behaved USB devices that have trouble with this.
Al
Re: [PATCH 3/3] usb: Add Kconfig and Makefile changes to build brcmstb-usb-pinmap
On Thu, Aug 13, 2020 at 1:40 AM Greg Kroah-Hartman wrote: > > On Wed, Aug 12, 2020 at 04:20:18PM -0400, Al Cooper wrote: > > From: Al Cooper > > > > Add Kconfig and Makefile changes to build brcmstb-usb-pinmap and > > update MAINTAINERS for the new driver. > > This can be part of the previous patch, or at least the Kconfig and > Makefile changes should be there so that we build the code when we add > it. I'll combine the 2 patches. > > > refs #SWLINUX-5537 > > What is this? This will be removed. > > > > > Signed-off-by: Al Cooper > > --- > > MAINTAINERS | 8 > > drivers/usb/host/Kconfig | 4 > > drivers/usb/host/Makefile | 1 + > > 3 files changed, 13 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index f0569cf304ca..3a44ac61899b 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3527,6 +3527,14 @@ S: Maintained > > F: Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml > > F: drivers/usb/host/ehci-brcm.* > > > > +BROADCOM BRCMSTB USB PIN MAP DRIVER > > +M: Al Cooper > > +L: linux-...@vger.kernel.org > > +L: bcm-kernel-feedback-l...@broadcom.com > > +S: Maintained > > +F: Documentation/devicetree/bindings/usb/brcm,usb-pinmap.yaml > > +F: drivers/usb/host/brcmstb-usb-pinmap.c > > + > > BROADCOM BRCMSTB USB2 and USB3 PHY DRIVER > > M: Al Cooper > > L: linux-kernel@vger.kernel.org > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > > index 1cb3004ea7b2..9c285053bb0c 100644 > > --- a/drivers/usb/host/Kconfig > > +++ b/drivers/usb/host/Kconfig > > @@ -109,12 +109,16 @@ endif # USB_XHCI_HCD > > config USB_EHCI_BRCMSTB > > tristate > > > > +config BRCM_USB_PINMAP > > + tristate > > + > > config USB_BRCMSTB > > tristate "Broadcom STB USB support" > > depends on (ARCH_BRCMSTB && PHY_BRCM_USB) || COMPILE_TEST > > select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD > > select USB_EHCI_BRCMSTB if USB_EHCI_HCD > > select USB_XHCI_PLATFORM if USB_XHCI_HCD > > + select BRCM_USB_PINMAP > > help > > Enables support for XHCI, EHCI and OHCI host controllers > > found in Broadcom STB SoC's. > > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > > index bc731332fed9..0e63ef94790d 100644 > > --- a/drivers/usb/host/Makefile > > +++ b/drivers/usb/host/Makefile > > @@ -90,3 +90,4 @@ obj-$(CONFIG_USB_HCD_BCMA) += bcma-hcd.o > > obj-$(CONFIG_USB_HCD_SSB)+= ssb-hcd.o > > obj-$(CONFIG_USB_FOTG210_HCD)+= fotg210-hcd.o > > obj-$(CONFIG_USB_MAX3421_HCD)+= max3421-hcd.o > > +obj-$(CONFIG_BRCM_USB_PINMAP)+= brcmstb-usb-pinmap.o > > Shouldn't this driver be in usb/misc/ with other drivers like this? Why > host? That does seem like a better choice, I'll move it. > > Wait, why is this a separate driver at all? Why not just build it into > the USB_BRCMSTB driver? Of the 20 or so chips supported by the BRCMSTB USB drivers, only one currently has this functionality so it seems better to have this code in it's own driver so it can be enabled in device-tree for just this chip. I also wanted to leave this in it's own driver because in the future the same hardware functionality may be added for some eMMC signals. Thanks Al > > thanks, > > greg k-h
Re: [PATCH] mmc: Some Micron eMMC devices cause reboot to hang
On Wed, Aug 5, 2020 at 4:28 AM Ulf Hansson wrote: > > On Mon, 27 Jul 2020 at 15:07, Alan Cooper wrote: > > > > On Fri, Jul 24, 2020 at 7:03 AM Ulf Hansson wrote: > > > > > > On Tue, 21 Jul 2020 at 21:18, Al Cooper wrote: > > > > > > > > When using eMMC as the boot device, some Micron eMMC devices will > > > > cause reboot to hang. This is a result of the eMMC device not going > > > > into boot mode after the hardware sends CMD0 to reset the eMMC > > > > device. This only happens if the kernel driver sends CMD5 (SLEEP_WAKE), > > > > to put the device into sleep state, before restarting the system. > > > > > > What do you mean by "boot mode"? > > > > I'm referring to the "Boot operation mode" described in Section 6.3 of > > the JEDEC spec. > > Our hardware will send a CMD0 with 0xf0f0f0f0 argument at powerup or > > when the SoC is reset, and then hold the CLK signal low for 74 clock > > cycles. This should put the eMMC device into boot mode where it > > streams consecutive blocks without additional commands. With this > > Micron device I find that if I send a CMD5 before trying to restart > > the system by resetting the SoC, that the system hangs. I worked with > > Micron on the issue and they finally said to either avoid sending the > > CMD5 on restart or use a newer version of the Micron eMMC device. > > Thanks for clarifying the test sequence! > > However, I am still not (yet) convinced that a card quirk is the right > thing to do. What does the eMMC spec say about sending a CMD0 with > 0xf0f0f0f0 to a device that "sleeps"? This is from the spec: A Device may be switched between a Sleep state and a Standby state by SLEEP/AWAKE (CMD5). In the Sleep state the power consumption of the memory device is minimized. In this state the memory device reacts only to the commands RESET (CMD0 with argument of either 0x or 0xF0F0F0F0 or H/W reset) and SLEEP/AWAKE (CMD5). All the other commands are ignored by the memory device. > > Moreover, how does your mmc host driver (and platform) treat VCC and > VCCQ at system suspend/resume, compared to when a reset is done? Is > there a difference? We don't change VCC and VCCQ during suspend. > > The point is, if the eMMC spec is being violated, we should not make a > card quirk - as it may cause problems for other platforms. The eMMC spec is being violated here by the Micron eMMC device. Our hardware has worked this way for over 10 years and has never had a problem with any of the eMMC devices from the major manufacturers including older and newer Micron eMMC devices. When I talked to Micron on the phone, one engineer said it was a firmware bug, but since it was an older chip that it wouldn't be fixed and that we should use a newer family of Micron eMMC devices. > > > > > > > > > > > When the kernel sends the CMD0 to wake up the eMMC from sleep, at > > > system resume for example, it all works fine, I guess. What is the > > > difference? > > > > On system resume the hardware will not try to put the eMMC device back > > into boot mode. > > I see. > > Does your host driver support HW busy signalling, so DAT0 is monitored > for de-assertion to confirm the CMD5 is completed by the kernel - or > do you rely on the per card sleep timeout to be used in mmc_sleep()? We use the per card sleep delay. > > Additionally, I wonder about what options you have to reset the eMMC? > Can we use something along the lines of > drivers/mmc/core/pwrseq_emmc.c? If it's not possible to do a HW reset, > we could try sending CMD0 with argument being '0' in the reset path. The problem is that the system is rebooting and it's the hardware that automatically sends the CMD0/0xf0f0f0f0 because there's no software running yet. This can't be changed. > > What do you think? > > > > > Al > > > > > > > > > The fix is to add a quirk that avoids sending the SLEEP command > > > > and to use MMC_FIXUP to set the quirk for these Micron devices. > > > > > > I am not sure this is Micron device specific, but rather some it's a > > > driver/platform bug. Maybe on the kernel side or in the bootloader > > > code. > > > > > Kind regards > Uffe > > > > > > > > > > > > Signed-off-by: Al Cooper > > > > --- > > > > drivers/mmc/core/mmc.c| 3 ++- > > > > drivers/mmc/core/quirks.h | 8 > > > > include/linux/mmc/card.h | 1 + > > > > 3 files changed, 11 insertions(+), 1 deletion(-
Re: [PATCH] mmc: Some Micron eMMC devices cause reboot to hang
On Fri, Jul 24, 2020 at 7:03 AM Ulf Hansson wrote:
>
> On Tue, 21 Jul 2020 at 21:18, Al Cooper wrote:
> >
> > When using eMMC as the boot device, some Micron eMMC devices will
> > cause reboot to hang. This is a result of the eMMC device not going
> > into boot mode after the hardware sends CMD0 to reset the eMMC
> > device. This only happens if the kernel driver sends CMD5 (SLEEP_WAKE),
> > to put the device into sleep state, before restarting the system.
>
> What do you mean by "boot mode"?
I'm referring to the "Boot operation mode" described in Section 6.3 of
the JEDEC spec.
Our hardware will send a CMD0 with 0xf0f0f0f0 argument at powerup or
when the SoC is reset, and then hold the CLK signal low for 74 clock
cycles. This should put the eMMC device into boot mode where it
streams consecutive blocks without additional commands. With this
Micron device I find that if I send a CMD5 before trying to restart
the system by resetting the SoC, that the system hangs. I worked with
Micron on the issue and they finally said to either avoid sending the
CMD5 on restart or use a newer version of the Micron eMMC device.
>
> When the kernel sends the CMD0 to wake up the eMMC from sleep, at
> system resume for example, it all works fine, I guess. What is the
> difference?
On system resume the hardware will not try to put the eMMC device back
into boot mode.
Al
>
> > The fix is to add a quirk that avoids sending the SLEEP command
> > and to use MMC_FIXUP to set the quirk for these Micron devices.
>
> I am not sure this is Micron device specific, but rather some it's a
> driver/platform bug. Maybe on the kernel side or in the bootloader
> code.
>
> But, let's see where the discussion leads us.
>
> Kind regards
> Uffe
>
> >
> > Signed-off-by: Al Cooper
> > ---
> > drivers/mmc/core/mmc.c| 3 ++-
> > drivers/mmc/core/quirks.h | 8
> > include/linux/mmc/card.h | 1 +
> > 3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index 4203303f946a..4d69e8f8fe59 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -1895,7 +1895,8 @@ static int mmc_init_card(struct mmc_host *host, u32
> > ocr,
> >
> > static int mmc_can_sleep(struct mmc_card *card)
> > {
> > - return (card && card->ext_csd.rev >= 3);
> > + return card && card->ext_csd.rev >= 3 &&
> > + ((card->quirks & MMC_QUIRK_BROKEN_SLEEP) == 0);
> > }
> >
> > static int mmc_sleep(struct mmc_host *host)
> > diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> > index 472fa2fdcf13..7263187b6323 100644
> > --- a/drivers/mmc/core/quirks.h
> > +++ b/drivers/mmc/core/quirks.h
> > @@ -99,6 +99,14 @@ static const struct mmc_fixup mmc_blk_fixups[] = {
> > MMC_FIXUP("V10016", CID_MANFID_KINGSTON, CID_OEMID_ANY,
> > add_quirk_mmc,
> > MMC_QUIRK_TRIM_BROKEN),
> >
> > + /*
> > +* Some Micron eMMC devices will not go into boot mode on
> > +* CMD0 arg: 0XF0F0F0F0 after going into SLEEP state.
> > +* This will hang a reboot.
> > +*/
> > + MMC_FIXUP(CID_NAME_ANY, CID_MANFID_NUMONYX, 0x014e, add_quirk_mmc,
> > + MMC_QUIRK_BROKEN_SLEEP),
> > +
> > END_FIXUP
> > };
> >
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index 7d46411ffaa2..0cdddcb5e17d 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -270,6 +270,7 @@ struct mmc_card {
> > #define MMC_QUIRK_BROKEN_IRQ_POLLING (1<<11) /* Polling SDIO_CCCR_INTx
> > could create a fake interrupt */
> > #define MMC_QUIRK_TRIM_BROKEN (1<<12) /* Skip trim */
> > #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI
> > support */
> > +#define MMC_QUIRK_BROKEN_SLEEP (1<<14) /* Broken sleep mode */
> >
> > boolreenable_cmdq; /* Re-enable Command Queue
> > */
> >
> > --
> > 2.17.1
> >
Re: [PATCH 0/7] usb: bdc: Updates and fixes to the USB BDC driver
On Tue, Jul 21, 2020 at 5:33 AM Felipe Balbi wrote: > > > Hi, > > Al Cooper writes: > > Updates and fixes to the Broadcom USB BDC driver. > > > > Al Cooper (4): > > dt-bindings: usb: bdc: Update compatible strings > > usb: bdc: Add compatible string for new style USB DT nodes > > usb: bdc: Adb shows offline after resuming from S2 > > usb: bdc: driver runs out of buffer descriptors on large ADB transfers > > > > Danesh Petigara (1): > > usb: bdc: Halt controller on suspend > > > > Florian Fainelli (1): > > usb: bdc: Use devm_clk_get_optional() > > > > Sasi Kumar (1): > > bdc: Fix bug causing crash after multiple disconnects > > What should we do here? There are few comments which seem > unresolved. Are we getting a new version? I'm resolving the comments and submitting v2 today. Thanks Al > > -- > balbi
Re: [PATCH 3/6] usb: bdc: driver may fail to get USB PHY
It's been very useful to have the DEFER debug message so I'd like to
leave in the check for DEFER. I should not be skipping the clock
disable, so I'll "goto clk_cleanup" for both cases.
Thanks
Al
On Fri, Jun 21, 2019 at 1:39 AM Chunfeng Yun wrote:
>
> On Thu, 2019-06-20 at 17:09 -0400, Al Cooper wrote:
> > Initialization order is important for the USB PHY and the PHY clients.
> > The init order is based on the build order of the drivers in the
> > makefiles and the PHY drivers are built early to help with
> > dependencies, but the new SCMI based clock subsystem has the side
> > effect of making some additional drivers DEFER until the clock
> > is ready. This is causing the USB PHY driver to defer which is causing
> > some PHY clients to fail when they try to get the PHY. The fix is to have
> > the client driver return DEFER when it's "get phy" routine returns DEFER.
> >
> > Signed-off-by: Al Cooper
> > ---
> > drivers/usb/gadget/udc/bdc/bdc_core.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > index 11a43de6c1c6..c794890d785b 100644
> > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > @@ -543,9 +543,13 @@ static int bdc_probe(struct platform_device *pdev)
> > dev, dev->of_node, phy_num);
> > if (IS_ERR(bdc->phys[phy_num])) {
> > ret = PTR_ERR(bdc->phys[phy_num]);
> > + if (ret == -EPROBE_DEFER) {
> > + dev_dbg(bdc->dev, "DEFER, waiting for PHY\n");
> why not disable clock here? when re-probe, will enable clock again.
> to me, no need check -EPROBE_DEFFER.
> > + return ret;
> > + }
>
> > dev_err(bdc->dev,
> > "BDC phy specified but not found:%d\n", ret);
> > - return ret;
> > + goto clk_cleanup;
> > }
> > }
> >
>
>
Re: [PATCH 2/6] usb: bdc: Cleanup clock support
> > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > index ccbd1d34eb2a..11a43de6c1c6 100644
> > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > @@ -490,8 +490,14 @@ static int bdc_probe(struct platform_device *pdev)
> >
> > dev_dbg(dev, "%s()\n", __func__);
> >
> > + bdc = devm_kzalloc(dev, sizeof(*bdc), GFP_KERNEL);
> > + if (!bdc)
> > + return -ENOMEM;
> > +
> > clk = devm_clk_get(dev, "sw_usbd");
> > if (IS_ERR(clk)) {
> > + if (PTR_ERR(clk) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> what about using devm_clk_get_optional()?
Good suggestion.
Thanks
Al
On Fri, Jun 21, 2019 at 1:26 AM Chunfeng Yun wrote:
>
> On Thu, 2019-06-20 at 17:09 -0400, Al Cooper wrote:
> > - Fix driver to defer on clk_get defer
> >
> > Signed-off-by: Al Cooper
> > ---
> > drivers/usb/gadget/udc/bdc/bdc_core.c | 15 +--
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > index ccbd1d34eb2a..11a43de6c1c6 100644
> > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > @@ -490,8 +490,14 @@ static int bdc_probe(struct platform_device *pdev)
> >
> > dev_dbg(dev, "%s()\n", __func__);
> >
> > + bdc = devm_kzalloc(dev, sizeof(*bdc), GFP_KERNEL);
> > + if (!bdc)
> > + return -ENOMEM;
> > +
> > clk = devm_clk_get(dev, "sw_usbd");
> > if (IS_ERR(clk)) {
> > + if (PTR_ERR(clk) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> what about using devm_clk_get_optional()?
>
> > dev_info(dev, "Clock not found in Device Tree\n");
> > clk = NULL;
> > }
> > @@ -501,11 +507,6 @@ static int bdc_probe(struct platform_device *pdev)
> > dev_err(dev, "could not enable clock\n");
> > return ret;
> > }
> > -
> > - bdc = devm_kzalloc(dev, sizeof(*bdc), GFP_KERNEL);
> > - if (!bdc)
> > - return -ENOMEM;
> > -
> > bdc->clk = clk;
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > @@ -551,7 +552,7 @@ static int bdc_probe(struct platform_device *pdev)
> > ret = bdc_phy_init(bdc);
> > if (ret) {
> > dev_err(bdc->dev, "BDC phy init failure:%d\n", ret);
> > - return ret;
> > + goto clk_cleanup;
> > }
> >
> > temp = bdc_readl(bdc->regs, BDC_BDCCAP1);
> > @@ -583,6 +584,8 @@ static int bdc_probe(struct platform_device *pdev)
> > bdc_hw_exit(bdc);
> > phycleanup:
> > bdc_phy_exit(bdc);
> > +clk_cleanup:
> > + clk_disable_unprepare(bdc->clk);
> > return ret;
> > }
> >
>
>
Re: [PATCH 6/6] usb: bdc: Update compatible match strings
> > Remove "brcm,bdc-v0.16" because it was never used on any system.
>
> You're not really removing it, are you?
Whoops, it was supposed to be removed.
Thanks
Al
On Fri, Jun 21, 2019 at 4:28 AM Sergei Shtylyov
wrote:
>
> Hello!
>
> On 21.06.2019 0:09, Al Cooper wrote:
>
> > Remove "brcm,bdc-v0.16" because it was never used on any system.
>
> You're not really removing it, are you?
>
> > Add "brcm,bdc-udc-v3.1" which exists for any STB system with BDC.
> >
> > Signed-off-by: Al Cooper
> > ---
> > drivers/usb/gadget/udc/bdc/bdc_core.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > index 15e28790142d..e2b2628925e6 100644
> > --- a/drivers/usb/gadget/udc/bdc/bdc_core.c
> > +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
> > @@ -644,6 +644,7 @@ static SIMPLE_DEV_PM_OPS(bdc_pm_ops, bdc_suspend,
> > bdc_resume);
> >
> > static const struct of_device_id bdc_of_match[] = {
> > + { .compatible = "brcm,bdc-udc-v3.1" },
> > { .compatible = "brcm,bdc-v0.16" },
> > { .compatible = "brcm,bdc" },
> > { /* sentinel */ }
>
> MBR, Sergei
Re: [PATCH v6 4/4] usb: host: Add ability to build new Broadcom STB USB drivers
On Tue, May 5, 2020 at 6:54 AM Greg Kroah-Hartman wrote: > > On Thu, Apr 30, 2020 at 07:12:58AM -0400, Al Cooper wrote: > > Add the build system changes needed to get the Broadcom STB XHCI, > > EHCI and OHCI functionality working. The OHCI support does not > > require anything unique to Broadcom so the standard ohci-platform > > driver is being used. The link order for XHCI was changed in the > > Makefile because of the way STB XHCI, EHCI and OHCI controllers > > share a port which requires that the XHCI driver be initialized > > first. Also update MAINTAINERS. > > > > Signed-off-by: Al Cooper > > --- > > MAINTAINERS | 8 > > drivers/usb/host/Kconfig | 16 > > drivers/usb/host/Makefile | 16 ++-- > > 3 files changed, 34 insertions(+), 6 deletions(-) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 26f281d9f32a..6147ed78d212 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3481,6 +3481,14 @@ S: Supported > > F: Documentation/devicetree/bindings/i2c/brcm,brcmstb-i2c.yaml > > F: drivers/i2c/busses/i2c-brcmstb.c > > > > +BROADCOM BRCMSTB USB EHCI DRIVER > > +M: Al Cooper > > +L: linux-...@vger.kernel.org > > +L: bcm-kernel-feedback-l...@broadcom.com > > +S: Maintained > > +F: Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml > > +F: drivers/usb/host/ehci-brcm.* > > + > > BROADCOM BRCMSTB USB2 and USB3 PHY DRIVER > > M: Al Cooper > > L: linux-kernel@vger.kernel.org > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > > index 55bdfdf11e4c..7d58fd66e412 100644 > > --- a/drivers/usb/host/Kconfig > > +++ b/drivers/usb/host/Kconfig > > @@ -97,6 +97,22 @@ config USB_XHCI_TEGRA > > > > endif # USB_XHCI_HCD > > > > +config USB_EHCI_BRCMSTB > > + tristate > > + > > +config USB_BRCMSTB > > + tristate "Broadcom STB USB support" > > + depends on (ARCH_BRCMSTB && PHY_BRCM_USB) || COMPILE_TEST > > + select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD > > + select USB_EHCI_BRCMSTB if USB_EHCI_HCD > > + select USB_XHCI_PLATFORM if USB_XHCI_HCD > > + help > > + Say Y to enable support for XHCI, EHCI and OHCI host controllers > > + found in Broadcom STB SoC's. > > + > > + Disabling this will keep the controllers and corresponding > > + PHYs powered down. > > Whhat are the module names? I'll add the module names. > > And why 2 config options here? I'd like the option to build an XHCI only system by not enabling USB_EHCI_HCD and USB_OHCI_HCD. > > > + > > config USB_EHCI_HCD > > tristate "EHCI HCD (USB 2.0) support" > > depends on HAS_DMA && HAS_IOMEM > > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > > index b191361257cc..85fa6ace552a 100644 > > --- a/drivers/usb/host/Makefile > > +++ b/drivers/usb/host/Makefile > > @@ -37,6 +37,15 @@ endif > > > > obj-$(CONFIG_USB_PCI)+= pci-quirks.o > > > > +# NOTE: BRCMSTB systems require that xhci driver be linked before the > > +# ehci/ohci drivers. > > Why? Please do this as a separate change before your makefile changes. Okay > > And what else will this break? We have relied on this link order for a > very long time, changing it could cause issues... > > I don't see how your driver needs this, please explain in great detail. For this explaination, I'm going to call our EHCI and OHCI controllers EOHCI. We have some SoC's that have an XHCI controller and an EOHCI controller on the same port, where the XHCI controller handles the 3.0 devices and the EOHCI handles the <= 2.0 devices. If the EOHCI controller comes up before the XHCI controller, any 3.0 devices installed on the port will be seen as a 2.0 device. Once the 3.0 controller comes up, most 3.0 USB devices will switch to 3.0, but this seems to cause problems on some USB devices. In the past, this wasn't a problem because we had custom XHCI, EHCI and OHCI drivers that I could order any way necessary. Now that the standard platform XHCI and OHCI drivers are being used this becomes a problem. Thanks for the review Al > > thanks, > > greg k-h
Re: [PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's
On Tue, May 5, 2020 at 7:00 AM Greg Kroah-Hartman
wrote:
>
> On Thu, Apr 30, 2020 at 07:12:57AM -0400, Al Cooper wrote:
> > Add a new EHCI driver for Broadcom STB SoC's. A new EHCI driver
> > was created instead of adding support to the existing ehci platform
> > driver because of the code required to workaround bugs in the EHCI
> > controller.
> >
> > Signed-off-by: Al Cooper
> > Reviewed-by: Andy Shevchenko
> > ---
> > drivers/usb/host/ehci-brcm.c | 290 +++
> > 1 file changed, 290 insertions(+)
> > create mode 100644 drivers/usb/host/ehci-brcm.c
>
> I need an ack from the EHCI maintainer to agree that this needs a whole
> new driver file...
>
> >
> > diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
> > new file mode 100644
> > index ..381bed5fdab0
> > --- /dev/null
> > +++ b/drivers/usb/host/ehci-brcm.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2020, Broadcom */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "ehci.h"
> > +
> > +#define hcd_to_ehci_priv(h) ((struct brcm_priv *)hcd_to_ehci(h)->priv)
> > +
> > +struct brcm_priv {
> > + struct clk *clk;
> > +};
> > +
> > +static const char brcm_hcd_name[] = "ehci-brcm";
>
> You don't use this anywhere? Are you sure this didn't cause compile
> warnings?
I'll remove it. I'm not getting a warning because it's a "static
const" which doesn't give unused warnings.
>
> > +
> > +static int (*org_hub_control)(struct usb_hcd *hcd,
> > + u16 typeReq, u16 wValue, u16 wIndex,
> > + char *buf, u16 wLength);
>
> So you only support one device per system? That feels bad...
When this driver was originally written, the ehci_hub_control()
function was a static function in ehci-hub.c and couldn't be called
directly. Instead, the function pointer was taken out of "struct
hc_driver" and since it couldn't change for multiple devices, only one
pointer was needed. The ehci_hub_control function is now global so it
can be called directly and this can be removed. It's nice to get rid
of this hack, thanks.
>
>
> > +
> > +/*
> > + * ehci_brcm_wait_for_sof
> > + * Wait for start of next microframe, then wait extra delay microseconds
> > + */
> > +static inline void ehci_brcm_wait_for_sof(struct ehci_hcd *ehci, u32 delay)
> > +{
> > + u32 frame_idx = ehci_readl(ehci, &ehci->regs->frame_index);
> > + u32 val;
> > + int res;
> > +
> > + /* Wait for next microframe (every 125 usecs) */
> > + res = readl_relaxed_poll_timeout(&ehci->regs->frame_index, val,
> > + val != frame_idx, 1, 130);
> > + if (res)
> > + dev_err(ehci_to_hcd(ehci)->self.controller,
> > + "Error waiting for SOF\n");
> > + udelay(delay);
> > +}
> > +
> > +/*
> > + * ehci_brcm_hub_control
> > + * Intercept echi-hcd request to complete RESUME and align it to the start
> > + * of the next microframe.
> > + * If RESUME is complete too late in the microframe, host controller
> > + * detects babble on suspended port and resets the port afterwards.
> > + * This s/w workaround allows to avoid this problem.
> > + * See SWLINUX-1909 for more details
> > + */
> > +static int ehci_brcm_hub_control(
> > + struct usb_hcd *hcd,
> > + u16 typeReq,
> > + u16 wValue,
> > + u16 wIndex,
> > + char*buf,
> > + u16 wLength)
> > +{
> > + struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > + int ports = HCS_N_PORTS(ehci->hcs_params);
> > + u32 __iomem *status_reg = &ehci->regs->port_status[
> > + (wIndex & 0xff) - 1];
>
> Horrid line-wrapping, put this assignment below so it can be read.
>
> And wIndex is little endian? Or native?
native
>
> > + unsigned long flags;
> > + int retval, irq_disabled = 0;
> > +
> > + /*
> > + * RESUME is cleared when GetPortStatus() is called 20ms after start
> > + * of RESUME
> > + */
> > + if ((typeReq == GetPortStatus) &&
> > + (wIndex && wIndex <= ports) &&
> > + ehci->reset_done[wIndex-1] &&
> > + time_after_eq(jiffies, ehci->reset_done[wIndex-1]) &&
> > + (ehci_readl(ehci, status_reg) & PORT_RESUME)) {
> > +
> > + /*
> > + * to make sure we are not interrupted until RESUME bit
> > + * is cleared, disable interrupts on current CPU
> > + */
> > + ehci_dbg(ehci, "SOF alignment workaround\n");
> > + irq_disabled = 1;
> > + local_irq_save(flags);
> > + ehci_brcm_wait_for_sof(ehci, 5);
> > + }
> > + retval = (*org_hub_control)(hcd, typeReq, wValue, wIndex, buf,
> > wLength);
>
> But this might not be set, did you just crash?
>
> If
Re: [PATCH v9 4/5] usb: ehci: Add new EHCI driver for Broadcom STB SoC's
On Mon, May 11, 2020 at 3:51 PM Alan Stern wrote:
>
> On Mon, 11 May 2020, Al Cooper wrote:
>
> > Add a new EHCI driver for Broadcom STB SoC's. A new EHCI driver
> > was created instead of adding support to the existing ehci platform
> > driver because of the code required to work around bugs in the EHCI
> > controller. The primary workaround is for a bug where the Core
> > violates the SOF interval between the first two SOFs transmitted after
> > resume. This only happens if the resume occurs near the end of a
> > microframe. The fix is to intercept the ehci-hcd request to complete
> > RESUME and align it to the start of the next microframe.
> >
> > Signed-off-by: Al Cooper
> > Reviewed-by: Andy Shevchenko
> > Reviewed-by: Florian Fainelli
> > ---
>
> I hate to point this out...
>
> > +static int ehci_brcm_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct resource *res_mem;
> > + struct brcm_priv *priv;
> > + struct usb_hcd *hcd;
> > + int irq;
> > + int err;
> > +
> > + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > + if (err)
> > + return err;
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq <= 0)
> > + return irq ? irq : EINVAL;
>
> That should be -EINVAL.
>
> To tell the truth, I'm not sure it's worthwhile checking for irq == 0.
> That's up to Greg to decide.
Darn, I've been looking at this code for too long :(
Since Greg originally requested <=, I'll fix this and send v10.
Thanks
Al
>
> Anyway, as far as I'm concerned you can either change EINVAL to -EINVAL
> or change the whole thing back to "if (irq < 0) return irq;". Either
> way, you may add:
>
> Acked-by: Alan Stern
>
> Alan Stern
>
Re: [PATCH v4 3/4] mmc: sdhci: enable/disable the clock in sdhci_pltfm_suspend/resume
> On 23/08/17 07:15, Masahiro Yamada wrote:
> > This commit provides similar cleanups as commit 83eacdfa2529 ("mmc:
> > sdhci: disable the clock in sdhci_pltfm_unregister()") did for
> > unregister hooks.
> >
> > sdhci-brcmstb.c and sdhci-sirf.c implement their own suspend/resume
> > hooks to handle pltfm_host->clk. Move clock handling to sdhci_pltfm.c
> > so that the drivers can reuse sdhci_pltfm_pmops.
> >
> > The following drivers did not previously touch pltfm_host->clk during
> > suspend/resume, but now do:
> > - sdhci-bcm-kona.c
> > - sdhci-dove.c
> > - sdhci-iproc.c
> > - sdhci-pxav2.c
> > - sdhci-tegra.c
> > - sdhci-xenon.c
> >
> > Signed-off-by: Masahiro Yamada
>
> Would have been nice to see some more Acks on this but from sdhci point of
> view:
>
> Acked-by: Adrian Hunter
Looks good for sdhci-brcmstb.c
Acked-by: Al Cooper
On Mon, Aug 28, 2017 at 6:34 AM, Adrian Hunter wrote:
> On 23/08/17 07:15, Masahiro Yamada wrote:
>> This commit provides similar cleanups as commit 83eacdfa2529 ("mmc:
>> sdhci: disable the clock in sdhci_pltfm_unregister()") did for
>> unregister hooks.
>>
>> sdhci-brcmstb.c and sdhci-sirf.c implement their own suspend/resume
>> hooks to handle pltfm_host->clk. Move clock handling to sdhci_pltfm.c
>> so that the drivers can reuse sdhci_pltfm_pmops.
>>
>> The following drivers did not previously touch pltfm_host->clk during
>> suspend/resume, but now do:
>> - sdhci-bcm-kona.c
>> - sdhci-dove.c
>> - sdhci-iproc.c
>> - sdhci-pxav2.c
>> - sdhci-tegra.c
>> - sdhci-xenon.c
>>
>> Signed-off-by: Masahiro Yamada
>
> Would have been nice to see some more Acks on this but from sdhci point of
> view:
>
> Acked-by: Adrian Hunter
>
>> ---
>>
>> Changes in v4:
>> - Disable clk when resume fails
>>
>> Changes in v3: None
>> Changes in v2:
>> - Fix build error reported by kbuild test robot
>>
>> drivers/mmc/host/sdhci-brcmstb.c | 37 +
>> drivers/mmc/host/sdhci-pltfm.c | 22 --
>> drivers/mmc/host/sdhci-sirf.c| 39
>> +--
>> 3 files changed, 22 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-brcmstb.c
>> b/drivers/mmc/host/sdhci-brcmstb.c
>> index e2f638338e8f..6d461fcdd663 100644
>> --- a/drivers/mmc/host/sdhci-brcmstb.c
>> +++ b/drivers/mmc/host/sdhci-brcmstb.c
>> @@ -21,41 +21,6 @@
>>
>> #include "sdhci-pltfm.h"
>>
>> -#ifdef CONFIG_PM_SLEEP
>> -
>> -static int sdhci_brcmstb_suspend(struct device *dev)
>> -{
>> - struct sdhci_host *host = dev_get_drvdata(dev);
>> - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> - int res;
>> -
>> - if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>> - mmc_retune_needed(host->mmc);
>> -
>> - res = sdhci_suspend_host(host);
>> - if (res)
>> - return res;
>> - clk_disable_unprepare(pltfm_host->clk);
>> - return res;
>> -}
>> -
>> -static int sdhci_brcmstb_resume(struct device *dev)
>> -{
>> - struct sdhci_host *host = dev_get_drvdata(dev);
>> - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> - int err;
>> -
>> - err = clk_prepare_enable(pltfm_host->clk);
>> - if (err)
>> - return err;
>> - return sdhci_resume_host(host);
>> -}
>> -
>> -#endif /* CONFIG_PM_SLEEP */
>> -
>> -static SIMPLE_DEV_PM_OPS(sdhci_brcmstb_pmops, sdhci_brcmstb_suspend,
>> - sdhci_brcmstb_resume);
>> -
>> static const struct sdhci_ops sdhci_brcmstb_ops = {
>> .set_clock = sdhci_set_clock,
>> .set_bus_width = sdhci_set_bus_width,
>> @@ -131,7 +96,7 @@ MODULE_DEVICE_TABLE(of, sdhci_brcm_of_match);
>> static struct platform_driver sdhci_brcmstb_driver = {
>> .driver = {
>> .name = "sdhci-brcmstb",
>> - .pm = &sdhci_brcmstb_pmops,
>> + .pm = &sdhci_pltfm_pmops,
>> .of_match_table = of_match_ptr(sdhci_brcm_of_match),
>> },
>> .probe = sdhci_brcmstb_probe,
>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
>> index e090d8c42ddb..4c0135e184e9 100644
>> --- a/drivers/mmc/host/sdhci-pltfm.c
>> +++ b/drivers/mmc/host/sdhci-pltfm.c
>> @@ -212,18 +212,36 @@ EXPORT_SYMBOL_GPL(sdhci_pltfm_unregister);
>> static int sdhci_pltfm_suspend(struct device *dev)
>> {
>> struct sdhci_host *host = dev_get_drvdata(dev);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + int ret;
>>
>> if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>> mmc_retune_needed(host->mmc);
>>
>> - return sdhci_suspend_host(host);
>> + ret = sdhci_suspend_host(host);
>> + if (ret)
>> + return ret;
>> +
>> + clk_disable_unprepare(pltfm_host->clk);
>> +
>> + return 0;
>> }
>>
>> static int sdhci_pltfm_resume(struct device *dev)
>> {
>> struct sdhci_host *host = dev_get_drvdata(dev);
>> + struct sdhci_pltfm_host *plt
