Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On Mon, Aug 12, 2013 at 8:24 PM, Kumar Gala wrote: > So I think I'd ask you to recommend a name, should we just us 'hwspinlock'. > The general view from ePAPR and dts is the node name should be a bit more > generic (like ethernet or pci). So what would you suggest? How about 'hwlock' ? This should be generic enough to cover all instances of hardware locks (we already know hwspinlock isn't generic enough, as there are some mutex-like hardware locks which doesn't require busy looping). -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 03/14] clk: Add of_clk_match() for device drivers
On 08/12, Mike Turquette wrote:
> Quoting Stephen Boyd (2013-07-24 17:43:31)
> > In similar fashion as of_regulator_match() add an of_clk_match()
> > function that finds an initializes clock init_data structs from
> > devicetree. Drivers should use this API to find clocks that their
> > device is providing and then iterate over their match table
> > registering the clocks with the init data parsed.
> >
> > Signed-off-by: Stephen Boyd
>
> Stephen,
>
> In general I like this approach. Writing real device drivers for clock
> controllers is The Right Way and of_clk_match helps.
>
> Am I reading this correctly that the base register addresses/offsets for
> the clock nodes still come from C files? For example I still see
> pll8_desc defining reg stuff in drivers/clk/msm/gcc-8960.c.
I think we may be able to put the registers in DT but I don't
know why we need to do that if we're already matching up nodes
with C structs. It also made me want to introduce devm_of_iomap()
which just seemed wrong (if you have a dev struct why can't you
use devm_ioremap()).
>
> What do you think about fetching this data from DT? My thinking here is
> that the definition of that PLL structure would be in C, as would all of
> the control logic. But any per-clock data whatsoever should live in DTS.
> This means the clock data you supply in the DTS files in patches #9 and
> #10 would have base addresses or offsets per-clock. I think this echoes
> Mark R's concerns as well.
>
> In the future if new chips have more of that type of PLL it would not
> require changes to your clock driver, only new DTS data for the new
> chip.
>
> I could have that wrong though, there is a fair amount of indirection in
> this series...
Let's take the PLL example and see if I follow what would be in
DT and what would be in C.
Right now we have
pll8: pll8 {
#clock-cells = <0>;
compatible = "qcom,pll";
clocks = <&pxo>;
};
in DT and
static struct pll_desc pll8_desc = {
.l_reg = 0x3144,
.m_reg = 0x3148,
.n_reg = 0x314c,
.config_reg = 0x3154,
.mode_reg = 0x3140,
.status_reg = 0x3158,
.status_bit = 16,
};
in C. Do you want everything to be in DT? Something like:
pll8: pll8@3140 {
#clock-cells = <0>;
compatible = "qcom,pll";
clocks = <&pxo>;
reg = <0x3140 0x20>;
};
and then assume that all those registers are offset from the base
register and that the status bit is 16 (it usually is but not
always)?
The problem I see is this quickly breaks down with more
complicated clocks like the RCGs.
We have
gsbi5_uart_rcg: gsbi5_uart_rcg {
#clock-cells = <0>;
compatible = "qcom,p2-mn16-clock";
clocks = <&pxo>, <&vpll8>;
};
in DT and
static struct freq_tbl clk_tbl_gsbi_uart[] = {
{ 1843200, PLL8, 2, 6, 625 },
{ 3686400, PLL8, 2, 12, 625 },
{ 7372800, PLL8, 2, 24, 625 },
{ 14745600, PLL8, 2, 48, 625 },
{ 1600, PLL8, 4, 1, 6 },
{ 2400, PLL8, 4, 1, 4 },
{ 3200, PLL8, 4, 1, 3 },
{ 4000, PLL8, 1, 5, 48 },
{ 4640, PLL8, 1, 29, 240 },
{ 4800, PLL8, 4, 1, 2 },
{ 5120, PLL8, 1, 2, 15 },
{ 5600, PLL8, 1, 7, 48 },
{ 58982400, PLL8, 1, 96, 625 },
{ 6400, PLL8, 2, 1, 3 },
{ }
};
static struct rcg_desc gsbi5_uart_rcg = {
.ctl_reg = 0x2a54,
.ns_reg = 0x2a54,
.md_reg = 0x2a50,
.ctl_bit = 11,
.mnctr_en_bit = 8,
.mnctr_reset_bit = 7,
.mnctr_mode_shift = 5,
.pre_div_shift = 3,
.src_sel_shift = 0,
.n_val_shift = 16,
.m_val_shift = 16,
.parent_map = gcc_pxo_pll8_map,
.freq_tbl = clk_tbl_gsbi_uart,
};
in C. It starts to get pretty unwieldy when you put this all in
DT, plus you'll notice that the ns_reg and ctl_reg are the same
here because we've generalized the code to work with different
types of software interfaces (technically this clock has no ctl
register, just an NS and MD register). Our multimedia clock
controllers don't follow any standard base/offset pair and so the
ctl_reg can be a different offset from the md_reg depending on
which clock we're talking about. My initial try at translating
this into DT pretty much just made every struct member into a
property, including the duplicate register, expect for the
frequency table, which could probably also be DT-ified with some
work.
gsbi5_uart_rcg: gsbi5
Re: [PATCH v1 09/14] clk: msm: Add support for MSM8960's global clock controller (GCC)
On 08/08, Mark Rutland wrote:
> Hi Stephen,
>
> On Thu, Jul 25, 2013 at 01:43:37AM +0100, Stephen Boyd wrote:
> > Fill in the data and wire up the global clock controller to the
> > MSM clock driver. This should allow most non-multimedia device
> > drivers to control their clocks on 8960 based platforms.
> >
> > Cc: devicet...@vger.kernel.org
> > Signed-off-by: Stephen Boyd
> > ---
> > .../devicetree/bindings/clock/qcom,gcc.txt | 55 +++
> > drivers/clk/msm/Kconfig| 10 ++
> > drivers/clk/msm/Makefile | 2 +
> > drivers/clk/msm/core.c | 3 +
> > drivers/clk/msm/gcc-8960.c | 174
> > +
> > drivers/clk/msm/internal.h | 2 +
> > 6 files changed, 246 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > create mode 100644 drivers/clk/msm/gcc-8960.c
> >
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > new file mode 100644
> > index 000..2311e1a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > @@ -0,0 +1,55 @@
> > +MSM Global Clock Controller Binding
> > +---
> > +
> > +Required properties :
> > +- compatible : shall contain at least "qcom,gcc" and only one of the
> > + following:
> > +
> > + "qcom,gcc-8660"
> > + "qcom,gcc-8960"
> > +
> > +- reg : shall contain base register location and length
> > +- clocks : shall contain clocks supplied by the clock controller
> > +
> > +Example:
> > + clock-controller@90 {
> > + compatible = "qcom,gcc-8960", "qcom,gcc";
> > + reg = <0x90 0x4000>;
> > +
> > + clocks {
> > + pxo: pxo {
> > + #clock-cells = <0>;
> > + compatible = "fixed-clock";
> > + clock-frequency = <2700>;
> > + };
> > +
> > + pll8: pll8 {
> > + #clock-cells = <0>;
> > + compatible = "qcom,pll";
> > + clocks = <&pxo>;
> > + };
> > +
> > + vpll8: vpll8 {
> > + #clock-cells = <0>;
> > + compatible = "qcom,pll-vote";
> > + clocks = <&pll8>;
> > + };
> > +
> > + gsbi5_uart_rcg: gsbi5_uart_rcg {
> > + #clock-cells = <0>;
> > + compatible = "qcom,p2-mn16-clock";
> > + clocks = <&pxo>, <&vpll8>;
> > + };
> > +
> > + gsbi5_uart_clk: gsbi5_uart_cxc {
> > + #clock-cells = <0>;
> > + compatible = "qcom,cxc-clock";
> > + clocks = <&gsbi5_uart_rcg>;
> > + };
> > +
> > + gsbi5_uart_ahb: gsbi5_uart_ahb {
> > + #clock-cells = <0>;
> > + compatible = "qcom,cxc-hg-clock";
> > + };
> > + };
> > + };
>
> I'm slightly confused by this. How is each of the clocks described in
> the clocks node related to a portion of the register set?
The registers to control clocks and determine their state are
scattered throughout the registers in the gcc (in this example
from 0x90 to 0x903fff). If you match up gsbi5_uart_rcg with
its C struct counterpart you'll notice that there are multiple
registers used to configure the clock. It isn't as simple as one
reg property per clock even for the case where we're just
toggling a bit to turn a clock on and off either. And it isn't as
simple as saying the clock has a base register that we can offset
from because offsets are almost always different (we've tried
to correct this in future chip versions).
>
> If the set of clocks is fixed, surely the gcc node gives you enough
> information alone, and the whole block can be modelled as a single
> provider of multiple clock outputs, or it's not fixed, and some linkage
> needs to be defined?
>
> The code seems to imply the former, unless only a subset of clocks may
> be present? In that case, the set of clocks which might be present
> should be described in the binding.
The clock controller is hardware and the number of clock outputs
is fixed. Isn't all hardware fixed until you start talking about
FPGAs? The next minor revision of the clock controller may add
more clocks or remove clocks from that base design, but otherwise
the two are 90% the same and generally software compatible. It
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
On Mon, 12 Aug 2013, Felipe Balbi wrote: > > > maybe a single callback for supporting 'testmodes' ? which receives the > > > test mode as argument ? > > > > I don't have a clear picture of how you would apply such an approach to > > this case. There would have to be a way to tell the HCD to insert a > > 15-second delay between the Setup and Data stages of a particular > > control URB. How would you do that? Whatever method you choose, > > each test is started after enumerating a known Vid/Pid pair. Based on > that, you *know* which test to run. That's not what I meant. Yes, the test-device driver knows what test it wants to run, based on the VID/PID. I was asking how it would communicate this knowledge to the HCD. For example, it doesn't make sense to have a callback that means "Insert a 15-second delay into the next URB that I submit", because the HCD doesn't know where URBs come from. > > What other test modes would you want to support? > > anything that USB[23]0CV supports today. There are even link layer tests > for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one > of a large(-ish) test suite which needs to be supported. That's what I'm trying to find out. What are the special features that we would need to implement in order to support the entire test suite? > > Is it worth adding this support to the standard host controller > > drivers, or should there be a special version (a Kconfig option like > > CONFIG_RCU_TORTURE_TEST) to enable it? Putting a lot of testing code > > in distribution kernels where it will never be used seems like a big > > waste. > > right, I think it should be hidden by Kconfig, not arguing against that. Therefore we both agree the $SUBJECT patch should not be accepted in its current form. At the very least, the new code in ehci-hcd should be conditional on a CONFIG_USB_HCD_TEST_MODE option. In addition, we may want some of the work (though at this point I'm not still clear on exactly what parts) to be moved into usbcore. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/3] usb: dwc3: msm: Add device tree binding information
On Aug 12, 2013, at 1:04 PM, Felipe Balbi wrote: > On Fri, Aug 09, 2013 at 10:31:58AM -0500, Kumar Gala wrote: >> >> On Aug 9, 2013, at 4:53 AM, Ivan T. Ivanov wrote: >> >>> From: "Ivan T. Ivanov" >>> >>> MSM USB3.0 core wrapper consist of USB3.0 IP (SNPS) >> >> probably good to spell out Synopsys rather than SNPS > > Synopsys (the company) has always used snps in their bindings though. I just meant in the commit message as SNPS wasn't obvious to me and I think when someone comes back and looks the commit message doing "Synopsys (SNPS)" is a little more readable. - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 1/3] usb: dwc3: msm: Add device tree binding information
On Fri, Aug 09, 2013 at 10:31:58AM -0500, Kumar Gala wrote: > > On Aug 9, 2013, at 4:53 AM, Ivan T. Ivanov wrote: > > > From: "Ivan T. Ivanov" > > > > MSM USB3.0 core wrapper consist of USB3.0 IP (SNPS) > > probably good to spell out Synopsys rather than SNPS Synopsys (the company) has always used snps in their bindings though. > > +Required properities : > > +- compatible : sould be "qcom,dwc3-hsphy"; > > +- reg : offset and length of the register set in the memory map > > +- clocks : phandles to clock instances of the device tree nodes > > +- clock-names : > > + "xo" : External reference clock 19 MHz > > + "sleep_a_clk" : Sleep clock, used when USB3 core goes into low > > + power mode (U3). > > +-supply : phandle to the regulator device tree node > > +Required "supply-name" are: > > + "v1p8" : 1.8v supply for HSPHY > > + "v3p3" : 3.3v supply for HSPHY > > + "vbus" : vbus supply for host mode > > + "vddcx" : vdd supply for HS-PHY digital circuit operation I believe these regulators belong to the PHY, not DWC3. Please write a PHY driver. > > +Required properities : > > +- compatible : sould be "qcom,dwc3-ssphy"; > > +- reg : offset and length of the register set in the memory map > > +- clocks : phandles to clock instances of the device tree nodes > > +- clock-names : > > + "xo" : External reference clock 19 MHz > > + "ref_clk" : Reference clock - used in host mode. > > +-supply : phandle to the regulator device tree node > > +Required "supply-name" are: > > + "v1p8" : 1.8v supply for SS-PHY > > + "vddcx" : vdd supply for SS-PHY digital circuit operation likewise, these regulators should be moved to the PHY driver. > > +Required properties : > > +- compatible : should be "qcom,dwc3" > > +- reg : offset and length of the register set in the memory map > > + offset and length of the TCSR register for routing USB > > + signals to either picoPHY0 or picoPHY1. > > +- clocks : phandles to clock instances of the device tree nodes > > +- clock-names : > > + "core_clk" : Master/Core clock, have to be >= 125 MHz for SS > > + operation and >= 60MHz for HS operation > > + "iface_clk" : System bus AXI clock > > + "sleep_clk" : Sleep clock, used when USB3 core goes into low > > + power mode (U3). > > + "utmi_clk" : Generated by HS-PHY. Used to clock the low power > > + parts of thr HS Link layer. > > + > > +Optional properties : > > +- gdsc-supply : phandle to the globally distributed switch controller > > + regulator node to the USB controller. > > + > > +Sub nodes: > > +- Sub node for "DWC3 USB3 controller". > > + This sub node is required property for device node. The properties > > + of this subnode are specified in dwc3.txt. > > Is tx-fifo-resize required for qcom,dwc3? if so we should call that > out in the binding. AFAICT that's only needed for OMAP5 ES1.0. Unless Qualcomm also screwed up default TX FIFO sizes :-p -- balbi signature.asc Description: Digital signature
Re: [RFC/PATCH 2/2] usb: ehci: Add support for SINGLE_STEP_SET_FEATURE test of EHSET
Hi, On Fri, Aug 09, 2013 at 11:04:48AM -0400, Alan Stern wrote: > > > > heh, it doesn't need to be entirely in the core. Core could have the > > > > generic calls and HCDs could implement some callbacks, but I think quite > > > > a bit of the code will be similar if we implement the same thing on all > > > > HCDs. > > > > > > What generic calls and callbacks would you suggest? I assume you want > > > enough to cover not just this one test but the entire USB-CV suite. > > > > maybe a single callback for supporting 'testmodes' ? which receives the > > test mode as argument ? > > I don't have a clear picture of how you would apply such an approach to > this case. There would have to be a way to tell the HCD to insert a > 15-second delay between the Setup and Data stages of a particular > control URB. How would you do that? Whatever method you choose, each test is started after enumerating a known Vid/Pid pair. Based on that, you *know* which test to run. > implementing it in every HCD would be a huge amount of work. sure, we can support different HCDs with time, but once SINGLE_STEP is implemented in e.g. EHCI, it should be simple to port it to OHCI/xHCI/MUSB/etc. > What other test modes would you want to support? anything that USB[23]0CV supports today. There are even link layer tests for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one of a large(-ish) test suite which needs to be supported. > Is it worth adding this support to the standard host controller > drivers, or should there be a special version (a Kconfig option like > CONFIG_RCU_TORTURE_TEST) to enable it? Putting a lot of testing code > in distribution kernels where it will never be used seems like a big > waste. right, I think it should be hidden by Kconfig, not arguing against that. -- balbi signature.asc Description: Digital signature
Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On Aug 10, 2013, at 2:11 PM, Ohad Ben-Cohen wrote:
> + Grant
>
> On Thu, Aug 1, 2013 at 5:10 PM, Kumar Gala wrote:
>>> On Jul 29, 2013, at 4:40 PM, Stephen Boyd wrote:
On 07/29, Kumar Gala wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>> b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
>> new file mode 100644
>> index 000..ddd6889
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/tcsr-mutex.txt
Maybe this should go under a new hwspinlock directory?
>>>
>>> Will look for Ohad to comment on this.
>>>
>> @@ -0,0 +1,20 @@
>> +Qualcomm MSM Hardware Mutex Block:
>> +
>> +The hardware block provides mutexes utilized between different
>> processors
>> +on the SoC as part of the communication protocol used by these
>> processors.
>> +
>> +Required properties:
>> +- compatible: should be "qcom,tcsr-mutex"
>> +- reg: Should contain registers location and length of mutex registers
>> +- reg-names:
>> + "mutex-base" - string to identify mutex registers
>> +- qcom,num-locks: the number of locks/mutexes supported
>> +
>> +Example:
>> +
>> + qcom,tcsr-mutex@fd484000 {
Maybe it should be hw-mutex@fd484000?
>>>
>>> again, will look for Ohad to make some comment on this.
>>>
>> + compatible = "qcom,tcsr-mutex";
>> + reg = <0xfd484000 0x1000>;
>> + reg-names = "mutex-base";
>> + qcom,num-locks = <32>;
>> + };
>>
>> Ohad, ping.
>
> I'd prefer a DT maintainer to take a look at your two open questions
> above, especially if I'm to merge a new file into the devicetree
> Documentation folder.
>
> Grant, any chance you have a moment to take a look?
>
> Otherwise, Stephen - do we have your Ack here? I was happy to see your
> review but not sure what's the latest status.
>
> Thanks,
> Ohad.
> --
So I think I'd ask you to recommend a name, should we just us 'hwspinlock'.
The general view from ePAPR and dts is the node name should be a bit more
generic (like ethernet or pci). So what would you suggest?
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On Aug 12, 2013, at 11:30 AM, Stephen Boyd wrote: > On 08/10/13 12:11, Ohad Ben-Cohen wrote: >> Otherwise, Stephen - do we have your Ack here? I was happy to see your >> review but not sure what's the latest status. > > The smp_mb() should be removed. Otherwise I'm willing to accept that we > only build this driver on ARCH_MSM for now. We can fix that in the > future to be available to hexagon if they ever want it. After that it > just needs a DT review/ack so I'll be happy to add a reviewed-by on v3. Yeah, waiting on our DT resolution before posting v3 w/various changes like removing the smp_mb() and a few other things. - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH 4/4] ARM: msm: Add support for 8974 SMP
On Fri, Aug 02, 2013 at 03:15:25AM +0100, Rohit Vaswani wrote:
> Add the cpus bindings and the Kraitv2 release sequence
> to make SMP work for 2 cores on MSM8974.
>
> Signed-off-by: Rohit Vaswani
> ---
> Documentation/devicetree/bindings/arm/cpus.txt | 1 +
> arch/arm/boot/dts/msm8974.dts | 23
> arch/arm/mach-msm/board-dt-8974.c | 3 +
> arch/arm/mach-msm/platsmp.c| 79
> ++
> 4 files changed, 106 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt
> b/Documentation/devicetree/bindings/arm/cpus.txt
> index 1132eac..7c3c677 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -52,6 +52,7 @@ For the ARM architecture every CPU node must contain the
> following properties:
>This should be one of:
>"qcom,scss"
>"qcom,kpssv1"
> + "qcom,kpssv2"
I guess I should've looked at the whole series before responding, that
answers my prior question about what variation we expect.
>
> Example:
>
> diff --git a/arch/arm/boot/dts/msm8974.dts b/arch/arm/boot/dts/msm8974.dts
> index c31c097..ef35a9b 100644
> --- a/arch/arm/boot/dts/msm8974.dts
> +++ b/arch/arm/boot/dts/msm8974.dts
> @@ -7,6 +7,22 @@
> compatible = "qcom,msm8974";
> interrupt-parent = <&intc>;
>
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "qcom,krait";
> + device_type = "cpu";
> + enable-method = "qcom,kpssv2";
> +
> + cpu@0 {
> + reg = <0>;
> + };
> +
> + cpu@1 {
> + reg = <1>;
> + };
> + };
> +
> intc: interrupt-controller@f900 {
> compatible = "qcom,msm-qgic2";
> interrupt-controller;
> @@ -23,4 +39,11 @@
><1 1 0xf08>;
> clock-frequency = <1920>;
> };
> +
> + kpss@f9012000 {
> + compatible = "qcom,kpss";
> + reg = <0xf9012000 0x1000>,
> + <0xf9088000 0x1000>,
> + <0xf9098000 0x1000>;
> + };
In the previous examples, the number of CPUs was equal to the number of
kpss reg values. Why does it differ here. Either:
* We always have the extra regsiter here, and it should be described
even if we don't use it.
* It's a different hardware block, and needs a more specific
compatible string.
Eitehr way this strengthens my feeling that we need to define the linkage
from a CPU to the portion of the kpss which affects it.
> };
> diff --git a/arch/arm/mach-msm/board-dt-8974.c
> b/arch/arm/mach-msm/board-dt-8974.c
> index d7f84f2..06119f9 100644
> --- a/arch/arm/mach-msm/board-dt-8974.c
> +++ b/arch/arm/mach-msm/board-dt-8974.c
> @@ -13,11 +13,14 @@
> #include
> #include
>
> +#include "common.h"
> +
> static const char * const msm8974_dt_match[] __initconst = {
> "qcom,msm8974",
> NULL
> };
>
> DT_MACHINE_START(MSM8974_DT, "Qualcomm MSM (Flattened Device Tree)")
> + .smp = smp_ops(msm_smp_ops),
> .dt_compat = msm8974_dt_match,
> MACHINE_END
> diff --git a/arch/arm/mach-msm/platsmp.c b/arch/arm/mach-msm/platsmp.c
> index 82eb079..0fdae69 100644
> --- a/arch/arm/mach-msm/platsmp.c
> +++ b/arch/arm/mach-msm/platsmp.c
> @@ -124,6 +124,80 @@ static int msm8960_release_secondary(unsigned int cpu)
> return 0;
> }
>
> +static int msm8974_release_secondary(unsigned int cpu)
> +{
> + void __iomem *reg;
> + void __iomem *l2_saw_base;
> + struct device_node *dn = NULL;
> + unsigned apc_pwr_gate_ctl = 0x14;
> + unsigned reg_val;
> +
> + if (cpu == 0 || cpu >= num_possible_cpus())
> + return -EINVAL;
> +
> + dn = of_find_compatible_node(dn, NULL, "qcom,kpss");
> + if (!dn) {
> + pr_err("%s : Missing kpss node from device tree\n", __func__);
> + return -ENXIO;
> + }
> +
> + reg = of_iomap(dn, cpu+1);
This looks very fishy given the prior patch being one off from this.
why is reg[0] now different?
> + if (!reg)
> + return -ENOMEM;
> +
> + pr_debug("Starting secondary CPU %d\n", cpu);
> +
> + /* Turn on the BHS, turn off LDO Bypass and power down LDO */
> + reg_val = 0x403f0001;
Magic number?
> + writel_relaxed(reg_val, reg + apc_pwr_gate_ctl);
> +
> + /* complete the above write before the delay */
> + mb();
Use writel?
> + /* wait for the bhs to settle */
> + udelay(1);
> +
> + /* Turn on BHS segments */
> + reg_val |= 0x3f << 1;
> + writel_relaxed(reg_val, reg + apc_pwr_gate_ctl);
> +
> + /* complete the above write before the delay */
> + mb();
Use writel again?
> + /* wait for the bhs to settle */
> + udelay(1);
> +
> + /* Finally turn on the bypass so that BHS sup
Re: [PATCH v2] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On 07/29/13 15:00, Kumar Gala wrote:
> diff --git a/drivers/hwspinlock/msm_hwspinlock.c
> b/drivers/hwspinlock/msm_hwspinlock.c
> new file mode 100644
> index 000..dbd9a69
> --- /dev/null
> +++ b/drivers/hwspinlock/msm_hwspinlock.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "hwspinlock_internal.h"
> +
> +#define SPINLOCK_ID_APPS_PROC1
Is this id only for the apps processor? What about hexagon? Does it need
a different number?
> +#define BASE_ID 0
> +
> +static int msm_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> + void __iomem *lock_addr = lock->priv;
> +
> + writel_relaxed(SPINLOCK_ID_APPS_PROC, lock_addr);
> + smp_mb();
> + return readl_relaxed(lock_addr) == SPINLOCK_ID_APPS_PROC;
> +}
> +
> +static void msm_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> + int lock_owner;
This should probably be u32 to be explicit about the size of the register.
> + void __iomem *lock_addr = lock->priv;
> +
> + lock_owner = readl_relaxed(lock_addr);
> + if (lock_owner != SPINLOCK_ID_APPS_PROC) {
> + pr_err("%s: spinlock not owned by Apps (actual owner is %d)\n",
Maybe you should just say "spinlock not owned by us (actual owner is
%d)" so that this driver is agnostic to the processor it runs on?
> + __func__, lock_owner);
> + }
> +
> + writel_relaxed(0, lock_addr);
> + smp_mb();
> +}
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwspinlock/msm: Add support for Qualcomm MSM HW Mutex block
On 08/10/13 12:11, Ohad Ben-Cohen wrote: > Otherwise, Stephen - do we have your Ack here? I was happy to see your > review but not sure what's the latest status. The smp_mb() should be removed. Otherwise I'm willing to accept that we only build this driver on ARCH_MSM for now. We can fix that in the future to be available to hexagon if they ever want it. After that it just needs a DT review/ack so I'll be happy to add a reviewed-by on v3. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ARM: msm: Add SMP support for 8960
On Fri, Aug 02, 2013 at 03:15:24AM +0100, Rohit Vaswani wrote:
> Add the cpus bindings and the Krait release sequence
> to make SMP work for MSM8960
>
> Signed-off-by: Rohit Vaswani
> ---
> Documentation/devicetree/bindings/arm/cpus.txt | 2 +
> Documentation/devicetree/bindings/arm/msm/kpss.txt | 16 ++
> arch/arm/boot/dts/msm8960-cdp.dts | 22 +
> arch/arm/mach-msm/platsmp.c| 57
> ++
> arch/arm/mach-msm/scm-boot.h | 8 +--
> 5 files changed, 102 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/kpss.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt
> b/Documentation/devicetree/bindings/arm/cpus.txt
> index 327aad2..1132eac 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -45,11 +45,13 @@ For the ARM architecture every CPU node must contain the
> following properties:
> "marvell,xsc3"
> "marvell,xscale"
> "qcom,scorpion"
> + "qcom,krait"
> - enable-method: Specifies the method used to enable or take the secondary
> cores
>out of reset. This allows different reset sequence for
>different types of cpus.
>This should be one of:
>"qcom,scss"
> + "qcom,kpssv1"
Hopefully (though this series implies otherwise) we won't have an
explosion of enable-methods. We haven't listed any common ones yet (e.g.
PSCI), and both this and qcom,scss are "poke some cpu-specific
registers".
I take it by the "v1" suffix you're expecting more variation here?
>
> Example:
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/kpss.txt
> b/Documentation/devicetree/bindings/arm/msm/kpss.txt
> new file mode 100644
> index 000..7272340
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/kpss.txt
> @@ -0,0 +1,16 @@
> +* KPSS - Krait Processor Sub-system
> +
> +Properties
> +
> +- compatible : Should contain "qcom,kpss".
> +
> +- reg: Specifies the base address for the KPSS registers used for
> + booting up secondary cores.
> +
> +Example:
> +
> + kpss@2088000 {
> + compatible = "qcom,kpss";
> + reg = <0x02088000 0x1000
> + 0x02098000 0x2000>;
> + };
What's the secondary bank of registers? The binding only mentions one...
Is this a register bank per-cpu? There's no linkage to CPU ID, which
means that handling logical mapping is going to get quite painful.
For the vaguely standard "spin-table" enable-method, the address to poke
(cpu-release-addr) may be stored inside a specific cpu node. Following
that style may make more sense here, unless the kpss hardware is used
for anything more than processor hotplug.
We could have the cpu node refer to the specific kpss/register combo,
which would also allow for future expansion if the kpss unit is
per-cluster:
/ {
cpus {
device_type = "cpu";
compatible = "qcom,krait";
enable-method = "qcom,kpssv1";
cpu@0 {
reg = <0>;
qcom,kpss-reg = <&kpss 1>; /* reg[1] in kpss */
};
cpu@1 {
reg = <1>;
qcom,kpss-reg = <&kpss 0>; /* reg[0] in kpss */
};
}
kpss: kpss@2088000 {
compatible = "qcom,kpss";
reg = <0x02088000 0x1000>,
<0x02098000 0x2000>;
};
}
> diff --git a/arch/arm/boot/dts/msm8960-cdp.dts
> b/arch/arm/boot/dts/msm8960-cdp.dts
> index db2060c..8c82d5e 100644
> --- a/arch/arm/boot/dts/msm8960-cdp.dts
> +++ b/arch/arm/boot/dts/msm8960-cdp.dts
> @@ -7,6 +7,22 @@
> compatible = "qcom,msm8960-cdp", "qcom,msm8960";
> interrupt-parent = <&intc>;
>
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "qcom,krait";
> + device_type = "cpu";
> + enable-method = "qcom,kpssv1";
> +
> + cpu@0 {
> + reg = <0>;
> + };
> +
> + cpu@1 {
> + reg = <1>;
> + };
> + };
Similarly to my comments on the first patch, I like making properties
shared, but we *need* to have common infrastructure before we can do
things this way.
> +
> intc: interrupt-controller@200 {
> compatible = "qcom,msm-qgic2";
> interrupt-controller;
> @@ -37,6 +53,12 @@
> reg = <0xfd51 0x4000>;
> };
>
> + kpss@2088000 {
> + compatible = "qcom,kpss";
> + reg = <0x02088000 0x1000
> + 0x02098000 0x2000>;
> + };
> +
> serial@1644 {
> compatible = "qcom,msm-hsuart", "qcom,msm-uart"
Re: [RESEND PATCH 2/4] ARM: msm: Re-organize platsmp to make it extensible
Hi,
Apologies for the long delay for review on this.
I really like the direction this is going, but I have some qualms with
the implementation.
On Fri, Aug 02, 2013 at 03:15:23AM +0100, Rohit Vaswani wrote:
> This makes it easy to add SMP support for new targets
> by adding cpus property and the release sequence.
> We add the enable-method property for the cpus property to
> specify which release sequence to use.
> While at it, add the 8660 cpus bindings to make SMP work.
>
> Signed-off-by: Rohit Vaswani
> ---
> Documentation/devicetree/bindings/arm/cpus.txt | 6 ++
> Documentation/devicetree/bindings/arm/msm/scss.txt | 15
> arch/arm/boot/dts/msm8660-surf.dts | 23 +-
> arch/arm/mach-msm/platsmp.c| 94
> +-
> 4 files changed, 115 insertions(+), 23 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/scss.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt
> b/Documentation/devicetree/bindings/arm/cpus.txt
> index f32494d..327aad2 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -44,6 +44,12 @@ For the ARM architecture every CPU node must contain the
> following properties:
> "marvell,mohawk"
> "marvell,xsc3"
> "marvell,xscale"
> + "qcom,scorpion"
> +- enable-method: Specifies the method used to enable or take the secondary
> cores
> + out of reset. This allows different reset sequence for
> + different types of cpus.
> + This should be one of:
> + "qcom,scss"
While I'd certainly like to see a move to using enable-method for
32-bit, I think this needs a bit more thought:
What does "qcom,scss" mean, precisely? It seems to be that we poke some
registers in a "qcom,scss" device. I think we need to document
enable-methods *very* carefully (and we need to do that for 64-bit as
well with psci), as it's very likely there'll be subtle
incompatibilities between platforms, especially if firmware is involved.
We should try to leaves as little room for error as possible.
I don't want to see this devolve into meaning "whatever the Linux driver
for this happens to do at the current point in time", as that just leads
to breakage as we have no clear distinction between actual requirements
and implementation details.
Given we already have platforms without an enable-method, we can't make
it a required property either -- those platforms are already booting by
figuring out an enable method from the platform's compatible string.
With PSCI, enable-method also implies a method for disabling a
particular CPU, so it would be nice for the description to cover this.
>
> Example:
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/scss.txt
> b/Documentation/devicetree/bindings/arm/msm/scss.txt
> new file mode 100644
> index 000..21c3e26
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/scss.txt
> @@ -0,0 +1,15 @@
> +* SCSS - Scorpion Sub-system
> +
> +Properties
> +
> +- compatible : Should contain "qcom,scss".
> +
> +- reg: Specifies the base address for the SCSS registers used for
> + booting up secondary cores.
> +
> +Example:
> +
> + scss@902000 {
> + compatible = "qcom,scss";
> + reg = <0x00902000 0x2000>;
> + };
> diff --git a/arch/arm/boot/dts/msm8660-surf.dts
> b/arch/arm/boot/dts/msm8660-surf.dts
> index cdc010e..203e51a 100644
> --- a/arch/arm/boot/dts/msm8660-surf.dts
> +++ b/arch/arm/boot/dts/msm8660-surf.dts
> @@ -7,6 +7,22 @@
> compatible = "qcom,msm8660-surf", "qcom,msm8660";
> interrupt-parent = <&intc>;
>
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "qcom,scorpion";
> + device_type = "cpu";
> + enable-method = "qcom,scss";
> +
> + cpu@0 {
> + reg = <0>;
> + };
> +
> + cpu@1 {
> + reg = <1>;
> + };
> + };
> +
I *really* like moving the common properties out into the /cpus node --
ePAPR suggests it, it's less error prone, and it takes up less space.
However, I'm not sure our generic/arch code handles it properly, and I
think we need to audit that before we can start writing dts that depend
on it. I'd be happy to be wrong here if anyone can correct me. :)
We probably need to factor out the way we read properties for cpu nodes,
falling back to ones present in /cpus if not present. There's already a
lot of pain getting the node for a logical (rather than physical) cpu
id.
Sudeep recently factored out finding the cpu node for a logical cpu id
[1,2] in generic code with a per-arch callback, it shouldn't be too hard
to have shims around that to read (optionally) common properties.
[...]
> -static void prepare_cold_cpu(unsigned int cpu)
> +static
Re: [PATCH V5 4/4] scsi: ufs: Improve UFS fatal error handling
Tested-by: Dolev Raviv
> Error handling in UFS driver is broken and resets the host controller
> for fatal errors without re-initialization. Correct the fatal error
> handling sequence according to UFS Host Controller Interface (HCI)
> v1.1 specification.
>
> o Processed requests which are completed w/wo error are reported to
> SCSI layer and any pending commands that are not started are aborted
> in the controller and re-queued into scsi mid-layer queue.
>
> o Upon determining fatal error condition the host controller may hang
> forever until a reset is applied. Block SCSI layer for sending new
> requests and apply reset in a separate error handling work.
>
> o SCSI is informed about the expected Unit-Attention exception from the
> device for the immediate command after a reset so that the SCSI layer
> take necessary steps to establish communication with the device.
>
> Signed-off-by: Sujit Reddy Thumma
> ---
> drivers/scsi/ufs/ufshcd.c | 229
> -
> drivers/scsi/ufs/ufshcd.h | 10 ++-
> 2 files changed, 149 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c577e6e..4dca9b4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -79,6 +79,14 @@ enum {
> UFSHCD_EH_IN_PROGRESS = (1 << 0),
> };
>
> +/* UFSHCD UIC layer error flags */
> +enum {
> + UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
> + UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
> + UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
> + UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
> +};
> +
> /* Interrupt configuration options */
> enum {
> UFSHCD_INT_DISABLE,
> @@ -101,6 +109,8 @@ enum {
>
> static void ufshcd_tmc_handler(struct ufs_hba *hba);
> static void ufshcd_async_scan(void *data, async_cookie_t cookie);
> +static int ufshcd_reset_and_restore(struct ufs_hba *hba);
> +static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
>
> /*
> * ufshcd_wait_for_register - wait for register value to change
> @@ -1523,9 +1533,6 @@ static int ufshcd_make_hba_operational(struct
> ufs_hba *hba)
> goto out;
> }
>
> - if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> - scsi_unblock_requests(hba->host);
> -
> out:
> return err;
> }
> @@ -1651,66 +1658,6 @@ static int ufshcd_verify_dev_init(struct ufs_hba
> *hba)
> }
>
> /**
> - * ufshcd_do_reset - reset the host controller
> - * @hba: per adapter instance
> - *
> - * Returns SUCCESS/FAILED
> - */
> -static int ufshcd_do_reset(struct ufs_hba *hba)
> -{
> - struct ufshcd_lrb *lrbp;
> - unsigned long flags;
> - int tag;
> -
> - /* block commands from midlayer */
> - scsi_block_requests(hba->host);
> -
> - spin_lock_irqsave(hba->host->host_lock, flags);
> - hba->ufshcd_state = UFSHCD_STATE_RESET;
> -
> - /* send controller to reset state */
> - ufshcd_hba_stop(hba);
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> -
> - /* abort outstanding commands */
> - for (tag = 0; tag < hba->nutrs; tag++) {
> - if (test_bit(tag, &hba->outstanding_reqs)) {
> - lrbp = &hba->lrb[tag];
> - if (lrbp->cmd) {
> - scsi_dma_unmap(lrbp->cmd);
> - lrbp->cmd->result = DID_RESET << 16;
> - lrbp->cmd->scsi_done(lrbp->cmd);
> - lrbp->cmd = NULL;
> - clear_bit_unlock(tag, &hba->lrb_in_use);
> - }
> - }
> - }
> -
> - /* complete device management command */
> - if (hba->dev_cmd.complete)
> - complete(hba->dev_cmd.complete);
> -
> - /* clear outstanding request/task bit maps */
> - hba->outstanding_reqs = 0;
> - hba->outstanding_tasks = 0;
> -
> - /* Host controller enable */
> - if (ufshcd_hba_enable(hba)) {
> - dev_err(hba->dev,
> - "Reset: Controller initialization failed\n");
> - return FAILED;
> - }
> -
> - if (ufshcd_link_startup(hba)) {
> - dev_err(hba->dev,
> - "Reset: Link start-up failed\n");
> - return FAILED;
> - }
> -
> - return SUCCESS;
> -}
> -
> -/**
> * ufshcd_slave_alloc - handle initial SCSI device configurations
> * @sdev: pointer to SCSI device
> *
> @@ -1727,6 +1674,9 @@ static int ufshcd_slave_alloc(struct scsi_device
> *sdev)
> sdev->use_10_for_ms = 1;
> scsi_set_tag_type(sdev, MSG_SIMPLE_TAG);
>
> + /* allow SCSI layer to restart the device in case of errors */
> + sdev->allow_restart = 1;
> +
> /*
>* Inform SCSI Midlayer that the LUN queue depth is same as the
>* controller queue depth. If a LUN queue depth is less than the
> @@ -1930,6 +1880,9 @@ ufshcd_transfer_rsp_status(
Re: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods
Tested-by: Dolev Raviv
> As of now SCSI initiated error handling is broken because,
> the reset APIs don't try to bring back the device initialized and
> ready for further transfers.
>
> In case of timeouts, the scsi error handler takes care of handling aborts
> and resets. Improve the error handling in such scenario by resetting the
> device and host and re-initializing them in proper manner.
>
> Signed-off-by: Sujit Reddy Thumma
> ---
> drivers/scsi/ufs/ufshcd.c | 240
> +++--
> drivers/scsi/ufs/ufshcd.h |2 +
> 2 files changed, 189 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d4ee48d..c577e6e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -69,9 +69,14 @@ enum {
>
> /* UFSHCD states */
> enum {
> - UFSHCD_STATE_OPERATIONAL,
> UFSHCD_STATE_RESET,
> UFSHCD_STATE_ERROR,
> + UFSHCD_STATE_OPERATIONAL,
> +};
> +
> +/* UFSHCD error handling flags */
> +enum {
> + UFSHCD_EH_IN_PROGRESS = (1 << 0),
> };
>
> /* Interrupt configuration options */
> @@ -87,6 +92,16 @@ enum {
> INT_AGGR_CONFIG,
> };
>
> +#define ufshcd_set_eh_in_progress(h) \
> + (h->eh_flags |= UFSHCD_EH_IN_PROGRESS)
> +#define ufshcd_eh_in_progress(h) \
> + (h->eh_flags & UFSHCD_EH_IN_PROGRESS)
> +#define ufshcd_clear_eh_in_progress(h) \
> + (h->eh_flags &= ~UFSHCD_EH_IN_PROGRESS)
> +
> +static void ufshcd_tmc_handler(struct ufs_hba *hba);
> +static void ufshcd_async_scan(void *data, async_cookie_t cookie);
> +
> /*
> * ufshcd_wait_for_register - wait for register value to change
> * @hba - per-adapter interface
> @@ -840,10 +855,25 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>
> tag = cmd->request->tag;
>
> - if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + switch (hba->ufshcd_state) {
> + case UFSHCD_STATE_OPERATIONAL:
> + break;
> + case UFSHCD_STATE_RESET:
> err = SCSI_MLQUEUE_HOST_BUSY;
> - goto out;
> + goto out_unlock;
> + case UFSHCD_STATE_ERROR:
> + set_host_byte(cmd, DID_ERROR);
> + cmd->scsi_done(cmd);
> + goto out_unlock;
> + default:
> + dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
> + __func__, hba->ufshcd_state);
> + set_host_byte(cmd, DID_BAD_TARGET);
> + cmd->scsi_done(cmd);
> + goto out_unlock;
> }
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
>
> /* acquire the tag to make sure device cmds don't use it */
> if (test_and_set_bit_lock(tag, &hba->lrb_in_use)) {
> @@ -880,6 +910,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host,
> struct scsi_cmnd *cmd)
> /* issue command to the controller */
> spin_lock_irqsave(hba->host->host_lock, flags);
> ufshcd_send_command(hba, tag);
> +out_unlock:
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> out:
> return err;
> @@ -1495,8 +1526,6 @@ static int ufshcd_make_hba_operational(struct
> ufs_hba *hba)
> if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> scsi_unblock_requests(hba->host);
>
> - hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> -
> out:
> return err;
> }
> @@ -2245,8 +2274,12 @@ static void ufshcd_err_handler(struct ufs_hba *hba)
> }
> return;
> fatal_eh:
> - hba->ufshcd_state = UFSHCD_STATE_ERROR;
> - schedule_work(&hba->feh_workq);
> + /* handle fatal errors only when link is functional */
> + if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
> + /* block commands at driver layer until error is handled */
> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> + schedule_work(&hba->feh_workq);
> + }
> }
>
> /**
> @@ -2411,12 +2444,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
> *hba, int lun_id, int task_id,
> }
>
> /**
> - * ufshcd_device_reset - reset device and abort all the pending commands
> + * ufshcd_eh_device_reset_handler - device reset handler registered to
> + *scsi layer.
> * @cmd: SCSI command pointer
> *
> * Returns SUCCESS/FAILED
> */
> -static int ufshcd_device_reset(struct scsi_cmnd *cmd)
> +static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
> {
> struct Scsi_Host *host;
> struct ufs_hba *hba;
> @@ -2425,6 +2459,7 @@ static int ufshcd_device_reset(struct scsi_cmnd
> *cmd)
> int err;
> u8 resp = 0xF;
> struct ufshcd_lrb *lrbp;
> + unsigned long flags;
>
> host = cmd->device->host;
> hba = shost_priv(host);
> @@ -2433,55 +2468,33 @@ static int ufshcd_device_reset(struct scsi_cmnd
> *cmd)
> lrbp = &hba->lrb[tag];
> err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, &resp
Re: [PATCH V5 2/4] scsi: ufs: Fix hardware race conditions while aborting a command
Tested-by: Dolev Raviv
> There is a possible race condition in the hardware when the abort
> command is issued to terminate the ongoing SCSI command as described
> below:
>
> - A bit in the door-bell register is set in the controller for a
> new SCSI command.
> - In some rare situations, before controller get a chance to issue
> the command to the device, the software issued an abort command.
> - If the device recieves abort command first then it returns success
> because the command itself is not present.
> - Now if the controller commits the command to device it will be
> processed.
> - Software thinks that command is aborted and proceed while still
> the device is processing it.
> - The software, controller and device may go out of sync because of
> this race condition.
>
> To avoid this, query task presence in the device before sending abort
> task command so that after the abort operation, the command is guaranteed
> to be non-existent in both controller and the device.
>
> Signed-off-by: Sujit Reddy Thumma
> ---
> drivers/scsi/ufs/ufshcd.c | 70
> +++-
> 1 files changed, 55 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d7f3746..d4ee48d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2485,6 +2485,12 @@ static int ufshcd_host_reset(struct scsi_cmnd *cmd)
> * ufshcd_abort - abort a specific command
> * @cmd: SCSI command pointer
> *
> + * Abort the pending command in device by sending UFS_ABORT_TASK task
> management
> + * command, and in host controller by clearing the door-bell register.
> There can
> + * be race between controller sending the command to the device while
> abort is
> + * issued. To avoid that, first issue UFS_QUERY_TASK to check if the
> command is
> + * really issued and then try to abort it.
> + *
> * Returns SUCCESS/FAILED
> */
> static int ufshcd_abort(struct scsi_cmnd *cmd)
> @@ -2493,7 +2499,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> struct ufs_hba *hba;
> unsigned long flags;
> unsigned int tag;
> - int err;
> + int err = 0;
> + int poll_cnt;
> u8 resp = 0xF;
> struct ufshcd_lrb *lrbp;
>
> @@ -2501,33 +2508,59 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> hba = shost_priv(host);
> tag = cmd->request->tag;
>
> - spin_lock_irqsave(host->host_lock, flags);
> + /* If command is already aborted/completed, return SUCCESS */
> + if (!(test_bit(tag, &hba->outstanding_reqs)))
> + goto out;
>
> - /* check if command is still pending */
> - if (!(test_bit(tag, &hba->outstanding_reqs))) {
> - err = FAILED;
> - spin_unlock_irqrestore(host->host_lock, flags);
> + lrbp = &hba->lrb[tag];
> + for (poll_cnt = 100; poll_cnt; poll_cnt--) {
> + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
> + UFS_QUERY_TASK, &resp);
> + if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) {
> + /* cmd pending in the device */
> + break;
> + } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
> + u32 reg;
> +
> + /*
> + * cmd not pending in the device, check if it is
> + * in transition.
> + */
> + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> + if (reg & (1 << tag)) {
> + /* sleep for max. 2ms to stabilize */
> + usleep_range(1000, 2000);
> + continue;
> + }
> + /* command completed already */
> + goto out;
> + } else {
> + if (!err)
> + err = resp; /* service response error */
> + goto out;
> + }
> + }
> +
> + if (!poll_cnt) {
> + err = -EBUSY;
> goto out;
> }
> - spin_unlock_irqrestore(host->host_lock, flags);
>
> - lrbp = &hba->lrb[tag];
> err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
> UFS_ABORT_TASK, &resp);
> if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
> - err = FAILED;
> + if (!err)
> + err = resp; /* service response error */
> goto out;
> - } else {
> - err = SUCCESS;
> }
>
> + err = ufshcd_clear_cmd(hba, tag);
> + if (err)
> + goto out;
> +
> scsi_dma_unmap(cmd);
>
> spin_lock_irqsave(host->host_lock, flags);
> -
> - /* clear the respective UTRLCLR register bit */
> - ufshcd_utrl_clear(hba, tag);
> -
> __clear_bit(tag, &hba->outstanding_reqs);
> hba->lrb[tag].cmd =
Re: [PATCH V5 1/4] scsi: ufs: Fix broken task management command implementation
Tested-by: Dolev Raviv
> Currently, sending Task Management (TM) command to the card might
> be broken in some scenarios as listed below:
>
> Problem: If there are more than 8 TM commands the implementation
> returns error to the caller.
> Fix: Wait for one of the slots to be emptied and send the command.
>
> Problem: Sometimes it is necessary for the caller to know the TM service
> response code to determine the task status.
> Fix: Propogate the service response to the caller.
>
> Problem: If the TM command times out no proper error recovery is
> implemented.
> Fix: Clear the command in the controller door-bell register, so that
> further commands for the same slot don't fail.
>
> Problem: While preparing the TM command descriptor, the task tag used
> should be unique across SCSI/NOP/QUERY/TM commands and not the
>task tag of the command which the TM command is trying to manage.
> Fix: Use a unique task tag instead of task tag of SCSI command.
>
> Problem: Since the TM command involves H/W communication, abruptly ending
> the request on kill interrupt signal might cause h/w malfunction.
> Fix: Wait for hardware completion interrupt with TASK_UNINTERRUPTIBLE
> set.
>
> Signed-off-by: Sujit Reddy Thumma
> ---
> drivers/scsi/ufs/ufshcd.c | 173
> ++---
> drivers/scsi/ufs/ufshcd.h |8 ++-
> 2 files changed, 122 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b36ca9a..d7f3746 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -53,6 +53,9 @@
> /* Query request timeout */
> #define QUERY_REQ_TIMEOUT 30 /* msec */
>
> +/* Task management command timeout */
> +#define TM_CMD_TIMEOUT 100 /* msecs */
> +
> /* Expose the flag value from utp_upiu_query.value */
> #define MASK_QUERY_UPIU_FLAG_LOC 0xFF
>
> @@ -183,13 +186,35 @@ ufshcd_get_tmr_ocs(struct utp_task_req_desc
> *task_req_descp)
> /**
> * ufshcd_get_tm_free_slot - get a free slot for task management request
> * @hba: per adapter instance
> + * @free_slot: pointer to variable with available slot value
> *
> - * Returns maximum number of task management request slots in case of
> - * task management queue full or returns the free slot number
> + * Get a free tag and lock it until ufshcd_put_tm_slot() is called.
> + * Returns 0 if free slot is not available, else return 1 with tag value
> + * in @free_slot.
> */
> -static inline int ufshcd_get_tm_free_slot(struct ufs_hba *hba)
> +static bool ufshcd_get_tm_free_slot(struct ufs_hba *hba, int *free_slot)
> {
> - return find_first_zero_bit(&hba->outstanding_tasks, hba->nutmrs);
> + int tag;
> + bool ret = false;
> +
> + if (!free_slot)
> + goto out;
> +
> + do {
> + tag = find_first_zero_bit(&hba->tm_slots_in_use, hba->nutmrs);
> + if (tag >= hba->nutmrs)
> + goto out;
> + } while (test_and_set_bit_lock(tag, &hba->tm_slots_in_use));
> +
> + *free_slot = tag;
> + ret = true;
> +out:
> + return ret;
> +}
> +
> +static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot)
> +{
> + clear_bit_unlock(slot, &hba->tm_slots_in_use);
> }
>
> /**
> @@ -1700,10 +1725,11 @@ static void ufshcd_slave_destroy(struct
> scsi_device *sdev)
> * ufshcd_task_req_compl - handle task management request completion
> * @hba: per adapter instance
> * @index: index of the completed request
> + * @resp: task management service response
> *
> - * Returns SUCCESS/FAILED
> + * Returns non-zero value on error, zero on success
> */
> -static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index)
> +static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8
> *resp)
> {
> struct utp_task_req_desc *task_req_descp;
> struct utp_upiu_task_rsp *task_rsp_upiup;
> @@ -1724,19 +1750,15 @@ static int ufshcd_task_req_compl(struct ufs_hba
> *hba, u32 index)
> task_req_descp[index].task_rsp_upiu;
> task_result = be32_to_cpu(task_rsp_upiup->header.dword_1);
> task_result = ((task_result & MASK_TASK_RESPONSE) >> 8);
> -
> - if (task_result != UPIU_TASK_MANAGEMENT_FUNC_COMPL &&
> - task_result != UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED)
> - task_result = FAILED;
> - else
> - task_result = SUCCESS;
> + if (resp)
> + *resp = (u8)task_result;
> } else {
> - task_result = FAILED;
> - dev_err(hba->dev,
> - "trc: Invalid ocs = %x\n", ocs_value);
> + dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
> + __func__, ocs_value);
> }
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> - return task_result;
> +
> + retur
Re: [PATCH V5 0/4] scsi: ufs: Improve UFS error handling
Hi, I tested the new set of patches (V5 1-4) and it works. Thanks, Dolev > The first patch fixes many issues with current task management handling > in UFSHCD driver. Others improve error handling in various scenarios. > > These patches are rebased on: > [PATCH 9/9] drivers/scsi/ufs: don't check resource with > devm_ioremap_resource > > Changes from v4: > - Addressed comments from Seungwon Jeon on V3 > - Updated with proper locking while ufshcd state changes > - Retained LUN reset instead of device reset with DME_END_POINT_RESET > - Removed error handling decisions dependent on OCS value > - Simplified fatal error handling to abort the requests first > and then carry out reset. > Changes from v3: > - Rebased. > Changes from v2: > - [PATCH V3 1/4]: Make the task management command task tag unique > across SCSI/NOP/QUERY request tags. > - [PATCH V3 3/4]: While handling device/host reset, wait for > pending fatal handler to return if running. > Changes from v1: > - [PATCH V2 1/4]: Fix a race condition because of overloading > outstanding_tasks variable to lock the slots. A new variable > tm_slots_in_use will track which slots are in use by the driver. > - [PATCH V2 2/4]: Commit text update to clarify the hardware race > with more details. > - [PATCH V2 3/4]: Minor cleanup and rebase > - [PATCH V2 4/4]: Fix a bug - sleeping in atomic context > > Sujit Reddy Thumma (4): > scsi: ufs: Fix broken task management command implementation > scsi: ufs: Fix hardware race conditions while aborting a command > scsi: ufs: Fix device and host reset methods > scsi: ufs: Improve UFS fatal error handling > > drivers/scsi/ufs/ufshcd.c | 684 > - > drivers/scsi/ufs/ufshcd.h | 20 +- > 2 files changed, 501 insertions(+), 203 deletions(-) > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
