[PATCH] ARM: s3c64xx: squash samsung_usb_phy.h into setup-usb-phy.c
This is only used by arch/arm/mach-s3c64xx/setup-usb-phy.c
$ git grep samsung_usb_phy_type
include/linux/usb/samsung_usb_phy.h:enum samsung_usb_phy_type {
$ git grep USB_PHY_TYPE_DEVICE
arch/arm/mach-s3c64xx/setup-usb-phy.c: if (type == USB_PHY_TYPE_DEVICE)
arch/arm/mach-s3c64xx/setup-usb-phy.c: if (type == USB_PHY_TYPE_DEVICE)
include/linux/usb/samsung_usb_phy.h:USB_PHY_TYPE_DEVICE,
$ git grep USB_PHY_TYPE_HOST
include/linux/usb/samsung_usb_phy.h:USB_PHY_TYPE_HOST,
Actually, 'enum samsung_usb_phy_type' is unused; the 'type' parameter
has 'int' type. Anyway, there is no need to declare this enum in the
globally visible header. Squash the header.
Signed-off-by: Masahiro Yamada
---
arch/arm/mach-s3c64xx/setup-usb-phy.c| 5 +
arch/arm/plat-samsung/include/plat/usb-phy.h | 2 --
include/linux/usb/samsung_usb_phy.h | 17 -
3 files changed, 5 insertions(+), 19 deletions(-)
delete mode 100644 include/linux/usb/samsung_usb_phy.h
diff --git a/arch/arm/mach-s3c64xx/setup-usb-phy.c
b/arch/arm/mach-s3c64xx/setup-usb-phy.c
index 46a9e955607f..61d8e8b9 100644
--- a/arch/arm/mach-s3c64xx/setup-usb-phy.c
+++ b/arch/arm/mach-s3c64xx/setup-usb-phy.c
@@ -15,6 +15,11 @@
#include "regs-sys.h"
#include "regs-usb-hsotg-phy.h"
+enum samsung_usb_phy_type {
+ USB_PHY_TYPE_DEVICE,
+ USB_PHY_TYPE_HOST,
+};
+
static int s3c_usb_otgphy_init(struct platform_device *pdev)
{
struct clk *xusbxti;
diff --git a/arch/arm/plat-samsung/include/plat/usb-phy.h
b/arch/arm/plat-samsung/include/plat/usb-phy.h
index 6d0c788beb9d..94da89ecbd3b 100644
--- a/arch/arm/plat-samsung/include/plat/usb-phy.h
+++ b/arch/arm/plat-samsung/include/plat/usb-phy.h
@@ -7,8 +7,6 @@
#ifndef __PLAT_SAMSUNG_USB_PHY_H
#define __PLAT_SAMSUNG_USB_PHY_H __FILE__
-#include
-
extern int s5p_usb_phy_init(struct platform_device *pdev, int type);
extern int s5p_usb_phy_exit(struct platform_device *pdev, int type);
diff --git a/include/linux/usb/samsung_usb_phy.h
b/include/linux/usb/samsung_usb_phy.h
deleted file mode 100644
index dc0071741695..
--- a/include/linux/usb/samsung_usb_phy.h
+++ /dev/null
@@ -1,17 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- * http://www.samsung.com/
- *
- * Defines phy types for samsung usb phy controllers - HOST or DEIVCE.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- */
-
-enum samsung_usb_phy_type {
- USB_PHY_TYPE_DEVICE,
- USB_PHY_TYPE_HOST,
-};
--
2.17.1
[PATCH 2/2] usb: host: oxu210hp-hcd: squash oxu210hp.h into oxu210hp-hcd.c
The header, oxu210hp.h is only included from oxu210hp-hcd.c
so squash it.
When I moved the code, I also fixed the following warnings from
scripts/checkpatch.pl:
drivers/usb/host/oxu210hp-hcd.c:117: warning: __packed is preferred over
__attribute__((packed))
drivers/usb/host/oxu210hp-hcd.c:196: warning: __packed is preferred over
__attribute__((packed))
drivers/usb/host/oxu210hp-hcd.c:221: warning: __packed is preferred over
__attribute__((packed))
drivers/usb/host/oxu210hp-hcd.c:266: warning: __aligned(size) is preferred over
__attribute__((aligned(size)))
drivers/usb/host/oxu210hp-hcd.c:336: warning: Prefer 'unsigned int' to bare use
of 'unsigned'
drivers/usb/host/oxu210hp-hcd.c:354: warning: __aligned(size) is preferred over
__attribute__((aligned(size)))
drivers/usb/host/oxu210hp-hcd.c:385: warning: __aligned(size) is preferred over
__attribute__((aligned(size)))
drivers/usb/host/oxu210hp-hcd.c:393: warning: __aligned(size) is preferred over
__attribute__((aligned(size)))
drivers/usb/host/oxu210hp-hcd.c:429: warning: Prefer 'unsigned int' to bare use
of 'unsigned'
drivers/usb/host/oxu210hp-hcd.c:432: warning: Prefer 'unsigned int' to bare use
of 'unsigned'
drivers/usb/host/oxu210hp-hcd.c:436: warning: Prefer 'unsigned int' to bare use
of 'unsigned'
drivers/usb/host/oxu210hp-hcd.c:451: warning: Prefer 'unsigned int' to bare use
of 'unsigned'
drivers/usb/host/oxu210hp-hcd.c:461: warning: Prefer 'unsigned int' to bare use
of 'unsigned'
drivers/usb/host/oxu210hp-hcd.c:467: warning: please, no space before tabs
Signed-off-by: Masahiro Yamada
---
drivers/usb/host/oxu210hp-hcd.c | 443 ++-
drivers/usb/host/oxu210hp.h | 446
2 files changed, 441 insertions(+), 448 deletions(-)
delete mode 100644 drivers/usb/host/oxu210hp.h
diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c
index 47c5515a9ce4..09cc19df798e 100644
--- a/drivers/usb/host/oxu210hp-hcd.c
+++ b/drivers/usb/host/oxu210hp-hcd.c
@@ -31,10 +31,449 @@
#include
#include
-#include "oxu210hp.h"
-
#define DRIVER_VERSION "0.0.50"
+#define OXU_DEVICEID 0x00
+ #define OXU_REV_MASK0x
+ #define OXU_REV_SHIFT 16
+ #define OXU_REV_21000x2100
+ #define OXU_BO_SHIFT8
+ #define OXU_BO_MASK (0x3 << OXU_BO_SHIFT)
+ #define OXU_MAJ_REV_SHIFT 4
+ #define OXU_MAJ_REV_MASK(0xf << OXU_MAJ_REV_SHIFT)
+ #define OXU_MIN_REV_SHIFT 0
+ #define OXU_MIN_REV_MASK(0xf << OXU_MIN_REV_SHIFT)
+#define OXU_HOSTIFCONFIG 0x04
+#define OXU_SOFTRESET 0x08
+ #define OXU_SRESET (1 << 0)
+
+#define OXU_PIOBURSTREADCTRL 0x0C
+
+#define OXU_CHIPIRQSTATUS 0x10
+#define OXU_CHIPIRQEN_SET 0x14
+#define OXU_CHIPIRQEN_CLR 0x18
+ #define OXU_USBSPHLPWUI 0x0080
+ #define OXU_USBOTGLPWUI 0x0040
+ #define OXU_USBSPHI 0x0002
+ #define OXU_USBOTGI 0x0001
+
+#define OXU_CLKCTRL_SET0x1C
+ #define OXU_SYSCLKEN0x0008
+ #define OXU_USBSPHCLKEN 0x0002
+ #define OXU_USBOTGCLKEN 0x0001
+
+#define OXU_ASO0x68
+ #define OXU_SPHPOEN 0x0100
+ #define OXU_OVRCCURPUPDEN 0x0800
+ #define OXU_ASO_OP (1 << 10)
+ #define OXU_COMPARATOR 0x04000
+
+#define OXU_USBMODE0x1A8
+ #define OXU_VBPS0x0020
+ #define OXU_ES_LITTLE 0x
+ #define OXU_CM_HOST_ONLY0x0003
+
+/*
+ * Proper EHCI structs & defines
+ */
+
+/* Magic numbers that can affect system performance */
+#define EHCI_TUNE_CERR 3 /* 0-3 qtd retries; 0 == don't stop */
+#define EHCI_TUNE_RL_HS4 /* nak throttle; see 4.9 */
+#define EHCI_TUNE_RL_TT0
+#define EHCI_TUNE_MULT_HS 1 /* 1-3 transactions/uframe; 4.10.3 */
+#define EHCI_TUNE_MULT_TT 1
+#define EHCI_TUNE_FLS 2 /* (small) 256 frame schedule */
+
+struct oxu_hcd;
+
+/* EHCI register interface, corresponds to EHCI Revision 0.95 specification */
+
+/* Section 2.2 Host Controller Capability Registers */
+struct ehci_caps {
+ /* these fields are specified as 8 and 16 bit registers,
+* but some hosts can't perform 8 or 16 bit PCI accesses.
+*/
+ u32 hc_capbase;
+#define HC_LENGTH(p) (((p)>>00)&0x00ff) /* bits 7:0 */
+#define HC_VERSION(p) (((p)>>16)&a
[PATCH 1/2] usb: host: oxu210hp-hcd: remove include/linux/oxu210hp.h
struct oxu210hp_platform_data is defined, but not used at all.
$ git grep oxu210hp_platform_data
include/linux/oxu210hp.h:struct oxu210hp_platform_data {
include/linux/oxu210hp.h exists just for defining an unused structure,
so it can go away.
Signed-off-by: Masahiro Yamada
---
drivers/usb/host/oxu210hp.h | 2 --
include/linux/oxu210hp.h| 8
2 files changed, 10 deletions(-)
delete mode 100644 include/linux/oxu210hp.h
diff --git a/drivers/usb/host/oxu210hp.h b/drivers/usb/host/oxu210hp.h
index 437044147862..67ebea4993b6 100644
--- a/drivers/usb/host/oxu210hp.h
+++ b/drivers/usb/host/oxu210hp.h
@@ -444,5 +444,3 @@ enum ehci_timer_action {
TIMER_ASYNC_SHRINK,
TIMER_ASYNC_OFF,
};
-
-#include
diff --git a/include/linux/oxu210hp.h b/include/linux/oxu210hp.h
deleted file mode 100644
index 94cd25165c08..
--- a/include/linux/oxu210hp.h
+++ /dev/null
@@ -1,8 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* platform data for the OXU210HP HCD */
-
-struct oxu210hp_platform_data {
- unsigned int bus16:1;
- unsigned int use_hcd_otg:1;
- unsigned int use_hcd_sph:1;
-};
--
2.17.1
[PATCH] usb: dwc3: omap: squash include/linux/platform_data/dwc3-omap.h
This enum is only used in drivers/usb/dwc3/dwc3-omap3.c
Signed-off-by: Masahiro Yamada
---
drivers/usb/dwc3/dwc3-omap.c| 7 +++-
include/linux/platform_data/dwc3-omap.h | 43 -
2 files changed, 6 insertions(+), 44 deletions(-)
delete mode 100644 include/linux/platform_data/dwc3-omap.h
diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index ed8b86517675..4f51523a07ac 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -14,7 +14,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -106,6 +105,12 @@
#define USBOTGSS_UTMI_OTG_CTRL_SESSVALID BIT(2)
#define USBOTGSS_UTMI_OTG_CTRL_VBUSVALID BIT(1)
+enum dwc3_omap_utmi_mode {
+ DWC3_OMAP_UTMI_MODE_UNKNOWN = 0,
+ DWC3_OMAP_UTMI_MODE_HW,
+ DWC3_OMAP_UTMI_MODE_SW,
+};
+
struct dwc3_omap {
struct device *dev;
diff --git a/include/linux/platform_data/dwc3-omap.h
b/include/linux/platform_data/dwc3-omap.h
deleted file mode 100644
index 1d36ca874cc8..
--- a/include/linux/platform_data/dwc3-omap.h
+++ /dev/null
@@ -1,43 +0,0 @@
-/**
- * dwc3-omap.h - OMAP Specific Glue layer, header.
- *
- * Copyright (C) 2010-2011 Texas Instruments Incorporated - http://www.ti.com
- * All rights reserved.
- *
- * Author: Felipe Balbi
- *
- * 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,
- *without modification.
- * 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. The names of the above-listed copyright holders may not be used
- *to endorse or promote products derived from this software without
- *specific prior written permission.
- *
- * ALTERNATIVELY, this software may be distributed under the terms of the
- * GNU General Public License ("GPL") version 2, as published by the Free
- * Software Foundation.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
- * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
- * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-enum dwc3_omap_utmi_mode {
- DWC3_OMAP_UTMI_MODE_UNKNOWN = 0,
- DWC3_OMAP_UTMI_MODE_HW,
- DWC3_OMAP_UTMI_MODE_SW,
-};
--
2.17.1
Re: [PATCH] usb: dwc3: Only call clk_bulk_get() on devicetree instantiated devices
2018-06-12 17:24 GMT+09:00 Hans de Goede :
> Commit fe8abf332b8f ("usb: dwc3: support clocks and resets for DWC3 core")
> adds support for handling clocks and resets in the DWC3 core, so that for
> platforms following the standard devicetree bindings this does not need
> to be duplicated in all the different glue layers.
>
> These changes intended for devicetree based platforms introduce an
> uncoditional clk_bulk_get() in the core probe path. This leads to the
> following error being logged on x86/ACPI systems:
>
> [ 26.276783] dwc3 dwc3.3.auto: Failed to get clk 'ref': -2
>
> This commits wraps the clk_bulk_get() in an if (dev->of_node) check so
> that it only is done on devicetree instantiated devices, fixing this
> error.
>
> Cc: Masahiro Yamada
The Cc: can be replaced with my
Reviewed-by: Masahiro Yamada
Thanks.
> Signed-off-by: Hans de Goede
> ---
> drivers/usb/dwc3/core.c | 23 +--
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index ea91310113b9..103807587dc6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1272,7 +1272,6 @@ static int dwc3_probe(struct platform_device *pdev)
> if (!dwc->clks)
> return -ENOMEM;
>
> - dwc->num_clks = ARRAY_SIZE(dwc3_core_clks);
> dwc->dev = dev;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1307,15 +1306,19 @@ static int dwc3_probe(struct platform_device *pdev)
> if (IS_ERR(dwc->reset))
> return PTR_ERR(dwc->reset);
>
> - ret = clk_bulk_get(dev, dwc->num_clks, dwc->clks);
> - if (ret == -EPROBE_DEFER)
> - return ret;
> - /*
> -* Clocks are optional, but new DT platforms should support all clocks
> -* as required by the DT-binding.
> -*/
> - if (ret)
> - dwc->num_clks = 0;
> + if (dev->of_node) {
> + dwc->num_clks = ARRAY_SIZE(dwc3_core_clks);
> +
> + ret = clk_bulk_get(dev, dwc->num_clks, dwc->clks);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> + /*
> +* Clocks are optional, but new DT platforms should support
> all
> +* clocks as required by the DT-binding.
> +*/
> + if (ret)
> + dwc->num_clks = 0;
> + }
>
> ret = reset_control_deassert(dwc->reset);
> if (ret)
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "usb: dwc3: support clocks and resets for DWC3 core" is causing errors on x86
Hi Hans, 2018-06-12 16:56 GMT+09:00 Hans de Goede : > Hi, > > On 12-06-18 08:27, Masahiro Yamada wrote: >> >> Hi Hans, >> >> 2018-06-08 5:57 GMT+09:00 Hans de Goede : >>> >>> Hi All, >>> >>> While testing usb-next on some Intle Bay and Cherry Trail >>> devices I noticed the following new error in my logs: >>> >>> [ 26.187958] dwc3 dwc3.3.auto: Failed to get clk 'ref': -2 >>> >>> This is caused by the new clk_bulk_get() call in >>> drivers/usb/dwc3/core.c >>> >>> If I understand the commit message correctly this is >>> intended to replace (for newer boards / SoCs) the >>> clk handling in dwc3-of-simple.c and this seems to >>> be a devicetree specific patch in general. >>> >>> Now things still work because the new clk code accepts >>> clk_bulk_get() failing and then effectively disables >>> itself. >>> >>> Maybe we should wrap the clk_bulk_get() in an >>> if (device->of_node) conditional to stop this error >>> from getting logged on non DT platforms? >>> >> >> Or, maybe drop the error logging from clk_bulk_get() ? > > > Yes, but that would require all callers to do their > own error logging. There seems to be a pattern in the > kernel where functions like clk_bulk_get (any function > which is expected to normally succeed) do the error > logging themselves so that callers don't have to do this. Right. But, clk_get() emits "ERROR: could not get clock" log in the core. To not disturb optional clocks, there are more conditions to meet for displaying the error message. See this code. https://github.com/torvalds/linux/blob/v4.17/drivers/clk/clkdev.c#L80 So, I suspect that logging is duplicated to some extent between clk_get() and clk_bulk_get(). > Note in the mean time I've prepared a patch making the > clk_bulk_get() call on dev->of_node. If we are in agreement > that that is best fix then I will post it. Fair enough. Go ahead. -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "usb: dwc3: support clocks and resets for DWC3 core" is causing errors on x86
Hi Hans, 2018-06-08 5:57 GMT+09:00 Hans de Goede : > Hi All, > > While testing usb-next on some Intle Bay and Cherry Trail > devices I noticed the following new error in my logs: > > [ 26.187958] dwc3 dwc3.3.auto: Failed to get clk 'ref': -2 > > This is caused by the new clk_bulk_get() call in > drivers/usb/dwc3/core.c > > If I understand the commit message correctly this is > intended to replace (for newer boards / SoCs) the > clk handling in dwc3-of-simple.c and this seems to > be a devicetree specific patch in general. > > Now things still work because the new clk code accepts > clk_bulk_get() failing and then effectively disables > itself. > > Maybe we should wrap the clk_bulk_get() in an > if (device->of_node) conditional to stop this error > from getting logged on non DT platforms? > Or, maybe drop the error logging from clk_bulk_get() ? -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] usb: dwc3: use local copy of resource to fix-up register offset
It is not a good idea to directly modify the resource of a platform
device. Modify its local copy, and pass it to devm_ioremap_resource()
so that we do not need to restore it in the failure path and the remove
hook.
Signed-off-by: Masahiro Yamada
Reviewed-by: Masami Hiramatsu
---
Changes in v3: None
Changes in v2: None
drivers/usb/dwc3/core.c | 32
1 file changed, 8 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a15648d..8e66edd 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1245,7 +1245,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
static int dwc3_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct resource *res;
+ struct resource *res, dwc_res;
struct dwc3 *dwc;
int ret;
@@ -1270,20 +1270,19 @@ static int dwc3_probe(struct platform_device *pdev)
dwc->xhci_resources[0].flags = res->flags;
dwc->xhci_resources[0].name = res->name;
- res->start += DWC3_GLOBALS_REGS_START;
-
/*
* Request memory region but exclude xHCI regs,
* since it will be requested by the xhci-plat driver.
*/
- regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(regs)) {
- ret = PTR_ERR(regs);
- goto err0;
- }
+ dwc_res = *res;
+ dwc_res.start += DWC3_GLOBALS_REGS_START;
+
+ regs = devm_ioremap_resource(dev, &dwc_res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
dwc->regs = regs;
- dwc->regs_size = resource_size(res);
+ dwc->regs_size = resource_size(&dwc_res);
dwc3_get_properties(dwc);
@@ -1350,29 +1349,14 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-err0:
- /*
-* restore res->start back to its original value so that, in case the
-* probe is deferred, we don't end up getting error in request the
-* memory region the next time probe is called.
-*/
- res->start -= DWC3_GLOBALS_REGS_START;
-
return ret;
}
static int dwc3_remove(struct platform_device *pdev)
{
struct dwc3 *dwc = platform_get_drvdata(pdev);
- struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pm_runtime_get_sync(&pdev->dev);
- /*
-* restore res->start back to its original value so that, in case the
-* probe is deferred, we don't end up getting error in request the
-* memory region the next time probe is called.
-*/
- res->start -= DWC3_GLOBALS_REGS_START;
dwc3_debugfs_exit(dwc);
dwc3_core_exit_mode(dwc);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/2] usb: dwc3: support clocks and resets for DWC3 core
In the current design of DWC3 driver,
the DT typically becomes a nested structure like follows:
dwc3-glue {
compatible = "foo,dwc3";
...
dwc3 {
compatible = "snps,dwc3";
...
};
}
The current DWC3 core (drivers/usb/dwc3/core.c) can not handle
clocks / resets at all.
The only solution we have now, is to put DWC3 core node under
the glue layer node, then add clocks and resets there.
Actually, dwc3-of-simple.c exists to handle clocks and resets.
As always for digital circuits, DWC3 core IP itself needs clock input.
This is specific to this IP. So, supporting clocks and resets in
dwc3/core.c makes sense.
In this version, the number of clocks (and names) is specific
to this IP, with clock names taken from Synopsys datasheet.
Masahiro Yamada (2):
usb: dwc3: use local copy of resource to fix-up register offset
usb: dwc3: support clocks and resets for DWC3 core
Documentation/devicetree/bindings/usb/dwc3.txt | 21 +
drivers/usb/dwc3/core.c| 118 +++--
drivers/usb/dwc3/core.h| 8 ++
3 files changed, 122 insertions(+), 25 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] usb: dwc3: support clocks and resets for DWC3 core
Historically, the clocks and resets are handled on the glue layer
side instead of the DWC3 core. For simple cases, dwc3-of-simple.c
takes care of arbitrary number of clocks and resets. The DT node
structure typically looks like as follows:
dwc3-glue {
compatible = "foo,dwc3";
clocks = ...;
resets = ...;
...
dwc3 {
compatible = "snps,dwc3";
...
};
}
By supporting the clocks and the reset in the dwc3/core.c, it will
be turned into a single node:
dwc3 {
compatible = "foo,dwc3", "snps,dwc3";
clocks = ...;
resets = ...;
...
}
This commit adds the binding of clocks and resets specific to this IP.
The number of clocks should generally be the same across SoCs, it is
just some SoCs either tie clocks together or do not provide software
control of some of the clocks.
I took the clock names from the Synopsys datasheet: "ref" (ref_clk),
"bus_early" (bus_clk_early), and "suspend" (suspend_clk).
I found only one reset line in the datasheet, hence the reset-names
property is omitted.
Those clocks are required for new platforms. Enforcing the new
binding breaks existing platforms since they specify clocks (and
resets) in their glue layer node, but nothing in the core node.
I listed such exceptional cases in the DT binding. The driver
code has been relaxed to accept no clock. This change is based
on the discussion [1].
I inserted reset_control_deassert() and clk_bulk_enable() before the
first register access, i.e. dwc3_cache_hwparams().
[1] https://patchwork.kernel.org/patch/10284265/
Signed-off-by: Masahiro Yamada
Reviewed-by: Rob Herring
---
Changes in v3:
- Change 'resets' DT property to optional.
Changes in v2:
- Make clocks specific to this IP based on Synopsys datasheet
- Use clk_bulk API
- Add description to struct header
Documentation/devicetree/bindings/usb/dwc3.txt | 21 ++
drivers/usb/dwc3/core.c| 88 +-
drivers/usb/dwc3/core.h| 8 +++
3 files changed, 115 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index 0dbd308..7f13ebe 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -7,6 +7,26 @@ Required properties:
- compatible: must be "snps,dwc3"
- reg : Address and length of the register set for the device
- interrupts: Interrupts used by the dwc3 controller.
+ - clock-names: should contain "ref", "bus_early", "suspend"
+ - clocks: list of phandle and clock specifier pairs corresponding to
+ entries in the clock-names property.
+
+Exception for clocks:
+ clocks are optional if the parent node (i.e. glue-layer) is compatible to
+ one of the following:
+"amlogic,meson-axg-dwc3"
+"amlogic,meson-gxl-dwc3"
+"cavium,octeon-7130-usb-uctl"
+"qcom,dwc3"
+"samsung,exynos5250-dwusb3"
+"samsung,exynos7-dwusb3"
+"sprd,sc9860-dwc3"
+"st,stih407-dwc3"
+"ti,am437x-dwc3"
+"ti,dwc3"
+"ti,keystone-dwc3"
+"rockchip,rk3399-dwc3"
+"xlnx,zynqmp-dwc3"
Optional properties:
- usb-phy : array of phandle for the PHY device. The first element
@@ -15,6 +35,7 @@ Optional properties:
- phys: from the *Generic PHY* bindings
- phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
or "usb3-phy".
+ - resets: a single pair of phandle and reset specifier
- snps,usb3_lpm_capable: determines if platform is USB3 LPM capable
- snps,disable_scramble_quirk: true when SW should disable data scrambling.
Only really useful for FPGA builds.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8e66edd..0380a85 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -8,6 +8,7 @@
* Sebastian Andrzej Siewior
*/
+#include
#include
#include
#include
@@ -24,6 +25,7 @@
#include
#include
#include
+#include
#include
#include
@@ -266,6 +268,12 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
return 0;
}
+static const struct clk_bulk_data dwc3_core_clks[] = {
+ { .id = "ref" },
+ { .id = "bus_early" },
+ { .id = "suspend" },
+};
+
/*
* dwc3_frame_length_adjustment - Adjusts frame length if required
* @dwc3: Pointer to our controller context structure
@@ -667,6 +675,9 @@ static void dwc3_core_exit(struct dwc3 *dwc)
usb_phy_set_suspend(dwc->usb3_phy, 1);
phy_power_off(dwc->usb2_generic_phy);
phy_power_off(dwc->usb3
Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core
Hi Martin,
2018-04-28 23:20 GMT+09:00 Martin Blumenstingl
:
> (adding Yixun from Amlogic to this mail)
>
> On Sat, Apr 28, 2018 at 4:41 AM, Masahiro Yamada
> wrote:
>> Hi Martin,
>>
>>
>> 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
>> :
>>> Hello,
>>>
>>> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
>>> wrote:
>>>> Historically, the clocks and resets are handled on the glue layer
>>>> side instead of the DWC3 core. For simple cases, dwc3-of-simple.c
>>>> takes care of arbitrary number of clocks and resets. The DT node
>>>> structure typically looks like as follows:
>>>>
>>>> dwc3-glue {
>>>> compatible = "foo,dwc3";
>>>> clocks = ...;
>>>> resets = ...;
>>>> ...
>>>>
>>>> dwc3 {
>>>> compatible = "snps,dwc3";
>>>> ...
>>>> };
>>>> }
>>>>
>>>> By supporting the clocks and the reset in the dwc3/core.c, it will
>>>> be turned into a single node:
>>>>
>>>> dwc3 {
>>>> compatible = "foo,dwc3", "snps,dwc3";
>>>> clocks = ...;
>>>> resets = ...;
>>>> ...
>>>> }
>>>>
>>>> This commit adds the binding of clocks and resets specific to this IP.
>>>> The number of clocks should generally be the same across SoCs, it is
>>>> just some SoCs either tie clocks together or do not provide software
>>>> control of some of the clocks.
>>>>
>>>> I took the clock names from the Synopsys datasheet: "ref" (ref_clk),
>>>> "bus_early" (bus_clk_early), and "suspend" (suspend_clk).
>>> looking at the code: this could mean that dwc3-exynos.c can be removed
>>> mid-term (assuming the PHY and regulator handling can be
>>> moved/removed/changed)
>>>
>>> does the datasheet state anything about the clock speeds? from
>>> Documentation/devicetree/bindings/usb/dwc3-xilinx.txt:
>>> "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation
>>> and >= 60MHz for HS operation
>>>
>>>> I found only one reset line in the datasheet, hence the reset-names
>>>> property is omitted.
>>> does the datasheet state whether this is a level or a pulsed reset line?
>>> on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared)
>>> reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for
>>> shared and pulsed reset lines") because the reset line is shared
>>> between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...)
>>> your current approach (having a vendor-specific "foo,dwc3" binding
>>> along with the generic "snps,dwc3") would allow having
>>> per-"of_device_id" settings which could indicate whether the reset
>>> lines are level or pulsed reset if these are "implementation specific"
>>
>> Let me ask a question about your reset controller.
>> (drivers/reset/reset-meson.c)
>>
>> All reset ID supports .reset, .assert, .deassert
>> Is this correct?
> as far as I know: yes (though I have only ever verified this with the
> Ethernet controller's reset line)
>
>>
>> I believe you and I use the same DWC3 core IP.
> this is possible - but I am not sure since I don't have access to
> Amlogic's internal resources where this should be documented (my
> knowledge mostly comes from reading Amlogic's out-of-tree kernel code
> and porting that to mainline)
>
>>
>> I suspect the difference is in the reset controller side.
>>
>> In my case, the reset line is asserted by default.
>> (that is, all FFs in the RTL are put into the initial state
>> on power-on)
>> That's why only reset_deassert() will work for me, I think.
>>
>> What about your case? Is the reset line in deassert state on power-on?
>> Then, the reset must be explicitly pulsed to put FFs into
>> the initial state. Is this correct?
> let me give you a bit of context first:
> the Amlogic Meson AXG, GXL and GXM SoCs have one reset line for "USB
> components". this is shared among:
> - the dwc3 controller
> - (depending on the SoC) 2 or 3 USB2 PHYs
> - a USB3 PHY
> - some OTG detection logic within the registers of the USB3 PHY
>
Re: [PATCH v2 1/2] usb: dwc3: use local copy of resource to fix-up register offset
Hi Felipe,
2018-04-19 20:03 GMT+09:00 Masahiro Yamada :
> It is not a good idea to directly modify the resource of a platform
> device. Modify its local copy, and pass it to devm_ioremap_resource()
> so that we do not need to restore it in the failure path and the remove
> hook.
>
> Signed-off-by: Masahiro Yamada
> Reviewed-by: Masami Hiramatsu
I want this patch applied first
unless you are opposed to this clean-up.
I'd like to avoid re-sending a trivial patch like this.
> ---
>
> Changes in v2: None
>
> drivers/usb/dwc3/core.c | 32
> 1 file changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a15648d..8e66edd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1245,7 +1245,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
> static int dwc3_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct resource *res;
> + struct resource *res, dwc_res;
> struct dwc3 *dwc;
>
> int ret;
> @@ -1270,20 +1270,19 @@ static int dwc3_probe(struct platform_device *pdev)
> dwc->xhci_resources[0].flags = res->flags;
> dwc->xhci_resources[0].name = res->name;
>
> - res->start += DWC3_GLOBALS_REGS_START;
> -
> /*
> * Request memory region but exclude xHCI regs,
> * since it will be requested by the xhci-plat driver.
> */
> - regs = devm_ioremap_resource(dev, res);
> - if (IS_ERR(regs)) {
> - ret = PTR_ERR(regs);
> - goto err0;
> - }
> + dwc_res = *res;
> + dwc_res.start += DWC3_GLOBALS_REGS_START;
> +
> + regs = devm_ioremap_resource(dev, &dwc_res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
>
> dwc->regs = regs;
> - dwc->regs_size = resource_size(res);
> + dwc->regs_size = resource_size(&dwc_res);
>
> dwc3_get_properties(dwc);
>
> @@ -1350,29 +1349,14 @@ static int dwc3_probe(struct platform_device *pdev)
> pm_runtime_put_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
> -err0:
> - /*
> -* restore res->start back to its original value so that, in case the
> -* probe is deferred, we don't end up getting error in request the
> -* memory region the next time probe is called.
> -*/
> - res->start -= DWC3_GLOBALS_REGS_START;
> -
> return ret;
> }
>
> static int dwc3_remove(struct platform_device *pdev)
> {
> struct dwc3 *dwc = platform_get_drvdata(pdev);
> - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> pm_runtime_get_sync(&pdev->dev);
> - /*
> -* restore res->start back to its original value so that, in case the
> -* probe is deferred, we don't end up getting error in request the
> -* memory region the next time probe is called.
> -*/
> - res->start -= DWC3_GLOBALS_REGS_START;
>
> dwc3_debugfs_exit(dwc);
> dwc3_core_exit_mode(dwc);
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core
Hi Martin,
2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
:
> Hello,
>
> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
> wrote:
>> Historically, the clocks and resets are handled on the glue layer
>> side instead of the DWC3 core. For simple cases, dwc3-of-simple.c
>> takes care of arbitrary number of clocks and resets. The DT node
>> structure typically looks like as follows:
>>
>> dwc3-glue {
>> compatible = "foo,dwc3";
>> clocks = ...;
>> resets = ...;
>> ...
>>
>> dwc3 {
>> compatible = "snps,dwc3";
>> ...
>> };
>> }
>>
>> By supporting the clocks and the reset in the dwc3/core.c, it will
>> be turned into a single node:
>>
>> dwc3 {
>> compatible = "foo,dwc3", "snps,dwc3";
>> clocks = ...;
>> resets = ...;
>> ...
>> }
>>
>> This commit adds the binding of clocks and resets specific to this IP.
>> The number of clocks should generally be the same across SoCs, it is
>> just some SoCs either tie clocks together or do not provide software
>> control of some of the clocks.
>>
>> I took the clock names from the Synopsys datasheet: "ref" (ref_clk),
>> "bus_early" (bus_clk_early), and "suspend" (suspend_clk).
> looking at the code: this could mean that dwc3-exynos.c can be removed
> mid-term (assuming the PHY and regulator handling can be
> moved/removed/changed)
>
> does the datasheet state anything about the clock speeds? from
> Documentation/devicetree/bindings/usb/dwc3-xilinx.txt:
> "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation
> and >= 60MHz for HS operation
>
>> I found only one reset line in the datasheet, hence the reset-names
>> property is omitted.
> does the datasheet state whether this is a level or a pulsed reset line?
> on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared)
> reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for
> shared and pulsed reset lines") because the reset line is shared
> between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...)
> your current approach (having a vendor-specific "foo,dwc3" binding
> along with the generic "snps,dwc3") would allow having
> per-"of_device_id" settings which could indicate whether the reset
> lines are level or pulsed reset if these are "implementation specific"
Let me ask a question about your reset controller.
(drivers/reset/reset-meson.c)
All reset ID supports .reset, .assert, .deassert
Is this correct?
I believe you and I use the same DWC3 core IP.
I suspect the difference is in the reset controller side.
In my case, the reset line is asserted by default.
(that is, all FFs in the RTL are put into the initial state
on power-on)
That's why only reset_deassert() will work for me, I think.
What about your case? Is the reset line in deassert state on power-on?
Then, the reset must be explicitly pulsed to put FFs into
the initial state. Is this correct?
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core
Hi Rob, 2018-04-26 0:21 GMT+09:00 Rob Herring : >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >> b/Documentation/devicetree/bindings/usb/dwc3.txt >> index 0dbd308..feb1cc33 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -7,6 +7,27 @@ Required properties: >> - compatible: must be "snps,dwc3" >> - reg : Address and length of the register set for the device >> - interrupts: Interrupts used by the dwc3 controller. >> + - clock-names: should contain "ref", "bus_early", "suspend" >> + - clocks: list of phandle and clock specifier pairs corresponding to >> + entries in the clock-names property. >> + - resets: a single pair of phandle and reset specifier > > This should be optional as some SoCs don't have separate, s/w controlled > resets of modules. OK. I will move resets to optional property. Please let ask a question. The number of clocks should be the same across SoCs. (Even if there is no s/w control for clocks, we should input something such as clk-fixed-rate.) On the other hand, the number of resets can be different across SoCs. If there is no s/w control for resets, we can make it optional. (optional = 1 or 0 reset) Is this what you mean? If we had something like reset-nop (or reset-dummy) in case no s/w control, we would be able to input something. I am not sure if this is the right thing, though. > Otherise, for the DT binding: > > Reviewed-by: Rob Herring > -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/2] usb: dwc3: support clocks and resets for DWC3 core
2018-04-24 9:11 GMT+09:00 Manu Gautam :
> HI,
>
>
> On 4/19/2018 4:03 AM, Masahiro Yamada wrote:
>> In the current design of DWC3 driver,
>> the DT typically becomes a nested structure like follows:
>>
>> dwc3-glue {
>> compatible = "foo,dwc3";
>> ...
>>
>> dwc3 {
>> compatible = "snps,dwc3";
>> ...
>> };
>> }
>>
>> The current DWC3 core (drivers/usb/dwc3/core.c) can not handle
>> clocks / resets at all.
>>
>> The only solution we have now, is to put DWC3 core node under
>> the glue layer node, then add clocks and resets there.
>> Actually, dwc3-of-simple.c exists to handle clocks and resets.
>>
>> As always for digital circuits, DWC3 core IP itself needs clock input.
>> This is specific to this IP. So, supporting clocks and resets in
>> dwc3/core.c makes sense.
>
> Why can't dwc3-of-simple be used with this IP?
> Adding core/reset handling in both core and glue drivers might
> only add to confusion and I cant think of a reason why having a parent
> node representing dwc3-of-simple glue would be any concern.
> Or are you planning to remove dwc3-of-simple.c driver?
dwc3-of-simple.c can be removed only after at least
all upstream DT files are migrated.
(and give enough time for migration of downstream DT)
At least, new platforms are not required to use this hack.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core
2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
:
> Hello,
>
> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
> wrote:
>> Historically, the clocks and resets are handled on the glue layer
>> side instead of the DWC3 core. For simple cases, dwc3-of-simple.c
>> takes care of arbitrary number of clocks and resets. The DT node
>> structure typically looks like as follows:
>>
>> dwc3-glue {
>> compatible = "foo,dwc3";
>> clocks = ...;
>> resets = ...;
>> ...
>>
>> dwc3 {
>> compatible = "snps,dwc3";
>> ...
>> };
>> }
>>
>> By supporting the clocks and the reset in the dwc3/core.c, it will
>> be turned into a single node:
>>
>> dwc3 {
>> compatible = "foo,dwc3", "snps,dwc3";
>> clocks = ...;
>> resets = ...;
>> ...
>> }
>>
>> This commit adds the binding of clocks and resets specific to this IP.
>> The number of clocks should generally be the same across SoCs, it is
>> just some SoCs either tie clocks together or do not provide software
>> control of some of the clocks.
>>
>> I took the clock names from the Synopsys datasheet: "ref" (ref_clk),
>> "bus_early" (bus_clk_early), and "suspend" (suspend_clk).
> looking at the code: this could mean that dwc3-exynos.c can be removed
> mid-term (assuming the PHY and regulator handling can be
> moved/removed/changed)
That is a good news.
> does the datasheet state anything about the clock speeds? from
> Documentation/devicetree/bindings/usb/dwc3-xilinx.txt:
> "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation
> and >= 60MHz for HS operation
Not sure.
"xlnx,zynqmp-dwc3" is a member of dwc3-of-simple.c
which just enables/disables clocks.
That information is not important.
>> I found only one reset line in the datasheet, hence the reset-names
>> property is omitted.
> does the datasheet state whether this is a level or a pulsed reset line?
> on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared)
> reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for
> shared and pulsed reset lines") because the reset line is shared
> between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...)
> your current approach (having a vendor-specific "foo,dwc3" binding
> along with the generic "snps,dwc3") would allow having
> per-"of_device_id" settings which could indicate whether the reset
> lines are level or pulsed reset if these are "implementation specific"
I guess your commit
ff0a632f08759e31f45b06fee27bc71c826c6b11
is wrong.
About the clocks, Rob Herring pointed out:
However, this should be specific as to how many clocks and their
function. This should be readily available to someone with access to
Synopsys datasheet. The number of clocks should generally be the same
across SoCs, it is just some SoCs either tie clocks together or don't
provide s/w control of some of the clocks.
The same applies to the reset control.
The reset policy should be the same across SoCs
since we are using the same IP (i.e. the same delivered RTL).
You are using a pulse reset for DWC3
just because the reset controller in your SoC
is implemented like that.
This is NOT because your DWC3 in your SoC is
specially customized, right?
You put a reset-provider matter to a reset-consumer.
>> Supporting those clocks and resets is the requirement for new platforms.
> just to confirm:
> with this series your goal is to replace the wrapper node which is
> needed due to dwc3-of-simple.c ?
In my _hope_.
But, I cannot do this by myself
since I do not have such boards.
Depends on how people make efforts to covert existing platforms.
> would other drivers which currently create a wrapper node (like
> dwc3-keystone.c) keep their wrapper node or do you have any plans for
> removing it for the other "wrapper" drivers too?
We need to take a close look per driver.
Looking at the dwc3-keystone.c,
it works like an interrupt controller with irq-domain hierarchy.
If it is moved to the irqchip subsystem,
dwc3-keystone.c could be removed.
>> Enforcing the new binding breaks existing platforms since they specify
>> clocks and resets in their glue layer node, but nothing in the core
>> node. I listed such exceptional cases in the DT binding. The driver
>> code is loosened up to accept no clock/reset. This change is based
>> on the discussion [1].
> (snip)
>
> Regards
> Martin
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core
Historically, the clocks and resets are handled on the glue layer
side instead of the DWC3 core. For simple cases, dwc3-of-simple.c
takes care of arbitrary number of clocks and resets. The DT node
structure typically looks like as follows:
dwc3-glue {
compatible = "foo,dwc3";
clocks = ...;
resets = ...;
...
dwc3 {
compatible = "snps,dwc3";
...
};
}
By supporting the clocks and the reset in the dwc3/core.c, it will
be turned into a single node:
dwc3 {
compatible = "foo,dwc3", "snps,dwc3";
clocks = ...;
resets = ...;
...
}
This commit adds the binding of clocks and resets specific to this IP.
The number of clocks should generally be the same across SoCs, it is
just some SoCs either tie clocks together or do not provide software
control of some of the clocks.
I took the clock names from the Synopsys datasheet: "ref" (ref_clk),
"bus_early" (bus_clk_early), and "suspend" (suspend_clk).
I found only one reset line in the datasheet, hence the reset-names
property is omitted.
Supporting those clocks and resets is the requirement for new platforms.
Enforcing the new binding breaks existing platforms since they specify
clocks and resets in their glue layer node, but nothing in the core
node. I listed such exceptional cases in the DT binding. The driver
code is loosened up to accept no clock/reset. This change is based
on the discussion [1].
I inserted reset_control_deassert() and clk_bulk_enable() before the
first register access, i.e. dwc3_cache_hwparams().
[1] https://patchwork.kernel.org/patch/10284265/
Signed-off-by: Masahiro Yamada
---
Changes in v2:
- Make clocks specific to this IP based on Synopsys datasheet
- Use clk_bulk API
- Add description to struct header
Documentation/devicetree/bindings/usb/dwc3.txt | 21 ++
drivers/usb/dwc3/core.c| 89 +-
drivers/usb/dwc3/core.h| 8 +++
3 files changed, 116 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index 0dbd308..feb1cc33 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -7,6 +7,27 @@ Required properties:
- compatible: must be "snps,dwc3"
- reg : Address and length of the register set for the device
- interrupts: Interrupts used by the dwc3 controller.
+ - clock-names: should contain "ref", "bus_early", "suspend"
+ - clocks: list of phandle and clock specifier pairs corresponding to
+ entries in the clock-names property.
+ - resets: a single pair of phandle and reset specifier
+
+Exception for clocks and resets:
+ clocks and resets are optional if the parent node (i.e. glue-layer)
+ is compatible to one of the following:
+"amlogic,meson-axg-dwc3"
+"amlogic,meson-gxl-dwc3"
+"cavium,octeon-7130-usb-uctl"
+"qcom,dwc3"
+"samsung,exynos5250-dwusb3"
+"samsung,exynos7-dwusb3"
+"sprd,sc9860-dwc3"
+"st,stih407-dwc3"
+"ti,am437x-dwc3"
+"ti,dwc3"
+"ti,keystone-dwc3"
+"rockchip,rk3399-dwc3"
+"xlnx,zynqmp-dwc3"
Optional properties:
- usb-phy : array of phandle for the PHY device. The first element
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8e66edd..15e1613 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -8,6 +8,7 @@
* Sebastian Andrzej Siewior
*/
+#include
#include
#include
#include
@@ -24,6 +25,7 @@
#include
#include
#include
+#include
#include
#include
@@ -266,6 +268,12 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
return 0;
}
+static const struct clk_bulk_data dwc3_core_clks[] = {
+ { .id = "ref" },
+ { .id = "bus_early" },
+ { .id = "suspend" },
+};
+
/*
* dwc3_frame_length_adjustment - Adjusts frame length if required
* @dwc3: Pointer to our controller context structure
@@ -667,6 +675,9 @@ static void dwc3_core_exit(struct dwc3 *dwc)
usb_phy_set_suspend(dwc->usb3_phy, 1);
phy_power_off(dwc->usb2_generic_phy);
phy_power_off(dwc->usb3_generic_phy);
+ clk_bulk_disable(dwc->num_clks, dwc->clks);
+ clk_bulk_unprepare(dwc->num_clks, dwc->clks);
+ reset_control_assert(dwc->reset);
}
static bool dwc3_core_is_valid(struct dwc3 *dwc)
@@ -1256,6 +1267,12 @@ static int dwc3_probe(struct platform_device *pdev)
if (!dwc)
return -ENOMEM;
+ dwc->clks = devm_kmemdup(dev, dwc3_core_clks, sizeof(dwc3_core_clks)
[PATCH v2 0/2] usb: dwc3: support clocks and resets for DWC3 core
In the current design of DWC3 driver,
the DT typically becomes a nested structure like follows:
dwc3-glue {
compatible = "foo,dwc3";
...
dwc3 {
compatible = "snps,dwc3";
...
};
}
The current DWC3 core (drivers/usb/dwc3/core.c) can not handle
clocks / resets at all.
The only solution we have now, is to put DWC3 core node under
the glue layer node, then add clocks and resets there.
Actually, dwc3-of-simple.c exists to handle clocks and resets.
As always for digital circuits, DWC3 core IP itself needs clock input.
This is specific to this IP. So, supporting clocks and resets in
dwc3/core.c makes sense.
In this version, the number of clocks (and names) is specific
to this IP, with clock names taken from Synopsys datasheet.
Masahiro Yamada (2):
usb: dwc3: use local copy of resource to fix-up register offset
usb: dwc3: support clocks and resets for DWC3 core
Documentation/devicetree/bindings/usb/dwc3.txt | 21 +
drivers/usb/dwc3/core.c| 119 +++--
drivers/usb/dwc3/core.h| 8 ++
3 files changed, 123 insertions(+), 25 deletions(-)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] usb: dwc3: use local copy of resource to fix-up register offset
It is not a good idea to directly modify the resource of a platform
device. Modify its local copy, and pass it to devm_ioremap_resource()
so that we do not need to restore it in the failure path and the remove
hook.
Signed-off-by: Masahiro Yamada
Reviewed-by: Masami Hiramatsu
---
Changes in v2: None
drivers/usb/dwc3/core.c | 32
1 file changed, 8 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a15648d..8e66edd 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1245,7 +1245,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
static int dwc3_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct resource *res;
+ struct resource *res, dwc_res;
struct dwc3 *dwc;
int ret;
@@ -1270,20 +1270,19 @@ static int dwc3_probe(struct platform_device *pdev)
dwc->xhci_resources[0].flags = res->flags;
dwc->xhci_resources[0].name = res->name;
- res->start += DWC3_GLOBALS_REGS_START;
-
/*
* Request memory region but exclude xHCI regs,
* since it will be requested by the xhci-plat driver.
*/
- regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(regs)) {
- ret = PTR_ERR(regs);
- goto err0;
- }
+ dwc_res = *res;
+ dwc_res.start += DWC3_GLOBALS_REGS_START;
+
+ regs = devm_ioremap_resource(dev, &dwc_res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
dwc->regs = regs;
- dwc->regs_size = resource_size(res);
+ dwc->regs_size = resource_size(&dwc_res);
dwc3_get_properties(dwc);
@@ -1350,29 +1349,14 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-err0:
- /*
-* restore res->start back to its original value so that, in case the
-* probe is deferred, we don't end up getting error in request the
-* memory region the next time probe is called.
-*/
- res->start -= DWC3_GLOBALS_REGS_START;
-
return ret;
}
static int dwc3_remove(struct platform_device *pdev)
{
struct dwc3 *dwc = platform_get_drvdata(pdev);
- struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pm_runtime_get_sync(&pdev->dev);
- /*
-* restore res->start back to its original value so that, in case the
-* probe is deferred, we don't end up getting error in request the
-* memory region the next time probe is called.
-*/
- res->start -= DWC3_GLOBALS_REGS_START;
dwc3_debugfs_exit(dwc);
dwc3_core_exit_mode(dwc);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb-next PATCH v11 3/8] usb: core: add a wrapper for the USB PHYs on the HCD
2018-04-06 14:15 GMT+09:00 Martin Blumenstingl : >> >> When will this be available in Linus' tree? In the current MW? > this is already available in Linus' tree. the commit for this specific > patch (#3) is known as 07dbff0ddbd86c Oh, great! Thank you. >> Sure, I can send a patch after some flaws are ironed-out. > great, thank you! > could you also let me know once you have tested this on one of your SoCs? I will. -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb-next PATCH v11 3/8] usb: core: add a wrapper for the USB PHYs on the HCD
2018-04-06 5:04 GMT+09:00 Martin Blumenstingl
:
> Hello,
>
> (great to hear that this might be useful on Socionext SoCs as well :))
>
> On Wed, Apr 4, 2018 at 2:10 PM, Masahiro Yamada
> wrote:
>> 2018-03-04 6:43 GMT+09:00 Martin Blumenstingl
>> :
>>> Many SoC platforms have separate devices for the USB PHY which are
>>> registered through the generic PHY framework. These PHYs have to be
>>> enabled to make the USB controller actually work. They also have to be
>>> disabled again on shutdown/suspend.
>>>
>>> Currently (at least) the following HCI platform drivers are using custom
>>> code to obtain all PHYs via devicetree for the roothub/controller and
>>> disable/enable them when required:
>>> - ehci-platform.c has ehci_platform_power_{on,off}
>>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>>> - ohci-platform.c has ohci_platform_power_{on,off}
>>>
>>> With this new wrapper the USB PHYs can be specified directly in the
>>> USB controller's devicetree node (just like on the drivers listed
>>> above). This allows SoCs like the Amlogic Meson GXL family to operate
>>> correctly once this is wired up correctly. These SoCs use a dwc3
>>> controller and require all USB PHYs to be initialized (if one of the USB
>>> PHYs it not initialized then none of USB port works at all).
>>>
>>> Signed-off-by: Martin Blumenstingl
>>> Tested-by: Yixun Lan
>>> Cc: Neil Armstrong
>>> Cc: Chunfeng Yun
>>> ---
>>> drivers/usb/core/Makefile | 2 +-
>>> drivers/usb/core/phy.c| 158
>>> ++
>>> drivers/usb/core/phy.h| 7 ++
>>> 3 files changed, 166 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/usb/core/phy.c
>>> create mode 100644 drivers/usb/core/phy.h
>>>
>>> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
>>> index 92c9cefb4317..18e874b0441e 100644
>>> --- a/drivers/usb/core/Makefile
>>> +++ b/drivers/usb/core/Makefile
>>> @@ -6,7 +6,7 @@
>>> usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
>>> usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
>>> usbcore-y += devio.o notify.o generic.o quirks.o devices.o
>>> -usbcore-y += port.o
>>> +usbcore-y += phy.o port.o
>>>
>>> usbcore-$(CONFIG_OF) += of.o
>>> usbcore-$(CONFIG_USB_PCI) += hcd-pci.o
>>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
>>> new file mode 100644
>>> index ..09b7c43c0ea4
>>> --- /dev/null
>>> +++ b/drivers/usb/core/phy.c
>>> @@ -0,0 +1,158 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * A wrapper for multiple PHYs which passes all phy_* function calls to
>>> + * multiple (actual) PHY devices. This is comes handy when initializing
>>> + * all PHYs on a HCD and to keep them all in the same state.
>>> + *
>>> + * Copyright (C) 2018 Martin Blumenstingl
>>>
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include "phy.h"
>>> +
>>> +struct usb_phy_roothub {
>>> + struct phy *phy;
>>> + struct list_headlist;
>>> +};
>>> +
>>> +static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>>> +{
>>> + struct usb_phy_roothub *roothub_entry;
>>> +
>>> + roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry),
>>> GFP_KERNEL);
>>> + if (!roothub_entry)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + INIT_LIST_HEAD(&roothub_entry->list);
>>> +
>>> + return roothub_entry;
>>> +}
>>> +
>>> +static int usb_phy_roothub_add_phy(struct device *dev, int index,
>>> + struct list_head *list)
>>> +{
>>> + struct usb_phy_roothub *roothub_entry;
>>> + struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node,
>>> index);
>>> +
>>> + if (IS_ERR_OR_NULL(phy)) {
>>> + if (!phy || PTR_ERR(phy) == -ENODEV)
>>> + return 0;
>>> + else
>>> + return PTR_ERR(phy);
>>> + }
>>> +
>>> + rooth
Re: [usb-next PATCH v11 3/8] usb: core: add a wrapper for the USB PHYs on the HCD
>list); > + if (err) > + goto err_out; > + } > + > + head = &phy_roothub->list; I think phy_roothub->phy is always empty, and only phy_roothub->list is used. I just wondered if we can directly put 'struct list_head' into 'struct usb_hcd'. > diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h > new file mode 100644 > index ..6fde59bfbff8 > --- /dev/null > +++ b/drivers/usb/core/phy.h > @@ -0,0 +1,7 @@ > +struct usb_phy_roothub; struct device; is also needed for usb_phy_roothub_init(). > +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev); > +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub); > + > +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub); > +void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub); Better to add an include guard? SPDX-License-Identifier for this header too? -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Multiple generic PHY instances for DWC3 USB IP
Hi Arnd, 2018-04-04 17:43 GMT+09:00 Arnd Bergmann : > On Wed, Apr 4, 2018 at 10:00 AM, Felipe Balbi > wrote: >> >> Hi, >> >> Masahiro Yamada writes: >>>>> Each DWC3 instance is connected with >>>>> multiple HS PHYs and multiple SS PHYs, >>>>> depending on the number of ports. >>>> >>>> in that case, you shouldn't need dwc3 at all. A Host-only dwc3 is xHCI >>>> compliant. If you really don't have the gadget block, there's no need >>>> for you to use dwc3. Just use xhci-plat directly. >>> >>> Sorry, I was misunderstanding. >>> >>> Some of our SoCs support gadget, >>> so we need to use the dwc3 driver. >> >> fair enough. Now we need to figure out how to pass multiply PHYs to a >> multi-port dwc3 instance. >> >> Kishon, any ideas? How do you think DT should look like? > > See this series from Martin Blumenstingl: > > https://www.spinics.net/lists/linux-usb/msg166281.html > > Arnd Very useful information! Not tested yet, but I quickly reviewed the series visually, and probably this will work for us. Thanks! -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Multiple generic PHY instances for DWC3 USB IP
2018-04-04 15:04 GMT+09:00 Felipe Balbi : > > Hi, > > Masahiro Yamada writes: >> 2018-04-04 14:36 GMT+09:00 Felipe Balbi : >>> >>> Hi, >>> >>> Masahiro Yamada writes: >>>> Currently, DWC3 core IP (drivers/usb/dwc3/core.c) >>>> can take only one PHY phandle for each of SS, HS. >>>> (phy-names DT property is "usb2-phy" and "usb3-phy" for each) >>> >>> We never had any other requirements :-) >>> >>>> The DWC3 core IP is provided by Synopsys, >>>> but some SoC-dependent parts (a.k.a glue-layer) >>>> are implemented by SoC venders. >>>> >>>> The number of connected PHY instances are SoC-dependent. >>>> >>>> If you look at generic drivers such as >>>> drivers/usb/host/ehci-platform.c >>>> the driver can handle arbitrary number of PHY instances. >>>> >>>> However, as mentioned above, DWC3 core allows only one PHY phandle >>>> for each SS/HS. >>>> This can result in a strange DT structure. >>>> >>>> For example, Socionext PXs3 SoC is integrated with 2 instances of DWC3. >>>> >>>> The instance 0 of DWC3 is connected with 2 super-speed PHYs. >>> >>> why 2 super-speed phys? Is this a two-port host-only implementation? >> >> >> Socionext SoCs only support the host-mode. >> >> >> The instance 0 has 2 ports. >> In our integration, 1 SS PHY is needed for each port. >> That's why it needs 2 SS PHYs. >> >> Each DWC3 instance is connected with >> multiple HS PHYs and multiple SS PHYs, >> depending on the number of ports. > > in that case, you shouldn't need dwc3 at all. A Host-only dwc3 is xHCI > compliant. If you really don't have the gadget block, there's no need > for you to use dwc3. Just use xhci-plat directly. Sorry, I was misunderstanding. Some of our SoCs support gadget, so we need to use the dwc3 driver. >>>> Is this OK? >>> >>> I don't know, I need a bit more details about your integration :-) >> >> >> I can send a patch. >> >> My concern is the following commit. >> I do not know which parts are using this lookups. > > Samsung SoCs, probably ;-) > > Anyway, if your IP really is host-only, then you don't need dwc3 for > anything. Just go for xHCI directly. If xHCI needs to be extended when > it comes to PHY, then you can discuss with Mathias Nyman :-) > > -- > balbi -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Multiple generic PHY instances for DWC3 USB IP
2018-04-04 14:36 GMT+09:00 Felipe Balbi :
>
> Hi,
>
> Masahiro Yamada writes:
>> Currently, DWC3 core IP (drivers/usb/dwc3/core.c)
>> can take only one PHY phandle for each of SS, HS.
>> (phy-names DT property is "usb2-phy" and "usb3-phy" for each)
>
> We never had any other requirements :-)
>
>> The DWC3 core IP is provided by Synopsys,
>> but some SoC-dependent parts (a.k.a glue-layer)
>> are implemented by SoC venders.
>>
>> The number of connected PHY instances are SoC-dependent.
>>
>> If you look at generic drivers such as
>> drivers/usb/host/ehci-platform.c
>> the driver can handle arbitrary number of PHY instances.
>>
>> However, as mentioned above, DWC3 core allows only one PHY phandle
>> for each SS/HS.
>> This can result in a strange DT structure.
>>
>> For example, Socionext PXs3 SoC is integrated with 2 instances of DWC3.
>>
>> The instance 0 of DWC3 is connected with 2 super-speed PHYs.
>
> why 2 super-speed phys? Is this a two-port host-only implementation?
Socionext SoCs only support the host-mode.
The instance 0 has 2 ports.
In our integration, 1 SS PHY is needed for each port.
That's why it needs 2 SS PHYs.
Each DWC3 instance is connected with
multiple HS PHYs and multiple SS PHYs,
depending on the number of ports.
>> The instance 1 of DWC3 is connected with 1 super-speed PHY.
>
> Are both of these instances incapable of high/full/low-speed
> communication?
Also HS PHYs are connected.
To narrow down the problem,
I picked up the DT snippet only for SS PHY device nodes.
>> @@ -894,8 +894,8 @@ struct dwc3 {
>> struct usb_phy *usb2_phy;
>> struct usb_phy *usb3_phy;
>>
>> - struct phy *usb2_generic_phy;
>> - struct phy *usb3_generic_phy;
>> + unsigned intnum_phys;
>> + struct phy **phys;
>>
>> boolphys_ready;
>>
>>
>>
>> Is this OK?
>
> I don't know, I need a bit more details about your integration :-)
I can send a patch.
My concern is the following commit.
I do not know which parts are using this lookups.
commit 08f871a3aca252b15107fc37dedcdacbac80fdb5
Author: Heikki Krogerus
Date: Wed Nov 19 17:28:23 2014 +0200
usb: dwc3: host: convey the PHYs to xhci
On some platforms a PHY may need to be handled also in the
host controller driver. Exynos5420 SoC requires some "PHY
tuning" based on the USB speed. This patch delivers dwc3's
PHYs to the xhci platform device when it's created.
Signed-off-by: Heikki Krogerus
Tested-by: Vivek Gautam
Acked-by: Felipe Balbi
Signed-off-by: Kishon Vijay Abraham I
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control
2018-04-03 19:35 GMT+09:00 Vivek Gautam :
>
>
> On 4/3/2018 3:49 PM, Masahiro Yamada wrote:
>>
>> 2018-04-03 17:46 GMT+09:00 Philipp Zabel :
>>>
>>> On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
>>>>
>>>> 2018-04-03 17:00 GMT+09:00 Philipp Zabel :
>>>>>
>>>>> On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
>>>>>>
>>>>>> This driver handles the reset control in a common manner; deassert
>>>>>> resets before use, assert them after use. There is no good reason
>>>>>> why it should be exclusive.
>>>>>
>>>>> Is this preemptive cleanup, or do you have hardware on the horizon that
>>>>> shares these reset lines with other peripherals?
>>>>
>>>> This patch is necessary for Socionext SoCs.
>>>>
>>>> The same reset lines are shared between
>>>> this dwc3-of_simple and other glue circuits.
>>>
>>> Thanks, this is helpful information.
>>>
>>>>>> Also, use devm_ for clean-up.
>>>>>>
>>>>>> Signed-off-by: Masahiro Yamada
>>>>>> ---
>>>>>>
>>>>>> CCing Philipp Zabel.
>>>>>> I see his sob in commit 06c47e6286d5.
>>>>>
>>>>> At the time I was concerned with the reset_control_array addition and
>>>>> didn't look closely at the exclusive vs shared issue.
>>>>>>
>>>>>> drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-
>>>>>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>>>>>> b/drivers/usb/dwc3/dwc3-of-simple.c
>>>>>> index e54c362..bd6ab65 100644
>>>>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>>>>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>>>>>> @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct
>>>>>> platform_device *pdev)
>>>>>>platform_set_drvdata(pdev, simple);
>>>>>>simple->dev = dev;
>>>>>>
>>>>>> - simple->resets =
>>>>>> of_reset_control_array_get_optional_exclusive(np);
>>>>>> + simple->resets =
>>>>>> devm_reset_control_array_get_optional_shared(dev);
>>>>>
>>>>> From the usage in the driver, it does indeed look like _shared reset
>>>>> usage is appropriate. I assume that the hardware has no need for the
>>>>> reset to be asserted right before probe or after remove, it just
>>>>> requires that the reset line is kept deasserted while the driver is
>>>>> probed.
>>>>>
>>>>>>if (IS_ERR(simple->resets)) {
>>>>>>ret = PTR_ERR(simple->resets);
>>>>>>dev_err(dev, "failed to get device resets, err=%d\n",
>>>>>> ret);
>>>>>> @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct
>>>>>> platform_device *pdev)
>>>>>>
>>>>>>ret = reset_control_deassert(simple->resets);
>>>>>>if (ret)
>>>>>> - goto err_resetc_put;
>>>>>> + return ret;
>>>>>>
>>>>>>ret = dwc3_of_simple_clk_init(simple,
>>>>>> of_count_phandle_with_args(np,
>>>>>>"clocks",
>>>>>> "#clock-cells"));
>>>>>> @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct
>>>>>> platform_device *pdev)
>>>>>> err_resetc_assert:
>>>>>>reset_control_assert(simple->resets);
>>>>>>
>>>>>> -err_resetc_put:
>>>>>> - reset_control_put(simple->resets);
>>>>>>return ret;
>>>>>> }
>>>>>>
>>>>>> @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct
>>>>>> platform_device *pdev)
>>>>>>simple->num_clocks = 0;
>>>>>>
>>>>>>reset_control_assert(simple->resets);
>>>>>> - reset_control_put(simple->resets);
>>>>>>
>>>>>>pm_runtime_
Multiple generic PHY instances for DWC3 USB IP
};
[ instance 1 (with 1 SS PHY) ]
usb0: dwc3@65c0 {
compatible = "snps,dwc3";
reg = <0x65c0 0xcd00>;
interrupt-names = "host", "peripheral";
interrupts = <0 137 4>, <0 138 4>;
phys = <&usb1_hsphy>, <&usb1_ssphy>;
dr_mode = "host";
};
usb1_ssphy: ss-phy@65d00300 {
compatible = "socionext,uniphier-dwc3-ssphy";
reg = <0x65d00300 0x10>;
#phy-cells = <0>;
clocks = <&sys_clk 21>;
resets = <&sys_rst 21>;
port0-supply = <&usb1_vbus0>;
};
To achieve this, I need driver changes.
My proposal is to support arbitrary number of PHY instances
like ehci-platform.c does.
@@ -894,8 +894,8 @@ struct dwc3 {
struct usb_phy *usb2_phy;
struct usb_phy *usb3_phy;
- struct phy *usb2_generic_phy;
- struct phy *usb3_generic_phy;
+ unsigned intnum_phys;
+ struct phy **phys;
boolphys_ready;
Is this OK?
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Question] MFD driver that handles clocks/resets and populates child nodes
2018-04-03 17:03 GMT+09:00 Lee Jones :
> On Mon, 02 Apr 2018, Andrew Lunn wrote:
>
>> On Mon, Apr 02, 2018 at 10:21:01PM +0900, Masahiro Yamada wrote:
>> > 2018-04-02 21:04 GMT+09:00 Andrew Lunn :
>> > >> The maintainer of DWC3, Felipe Balbi, requested to
>> > >> split the glue layer driver into small parts such as
>> > >> reset, regulator, phy, etc.
>> > >
>> > > What exactly did Felipe ask for? Did he ask that the patch be split
>> > > up, one patch per reset, regulator, phy etc?
>> >
>> >
>> > Yeah. That is what we understood from his comments.
>> >
>> >
>> > These are the feed-backs from him.
>> >
>> > https://lkml.org/lkml/2018/1/23/298
>> > https://lkml.org/lkml/2018/1/24/352
>>
>> > > Are all these resources used just by the DWC3? Or is it a true MFD,
>> > > multiple functions?
>> >
>> > I do not think this is a real MFD.
>> >
>> > This is a DWC3 glue layer, i.e.
>> > a collection of misc registers that control
>> > the DWC3 IP.
>> >
>> >
>> > Just splitting it into small pieces
>> > to use PHY, reset, regulator framework in Linux.
>> >
>> > Of course, the price of this approach
>> > is so cluttered Device Tree,
>> > honestly I do not like it much.
>>
>> This however the correct way to do this. You should have a phy driver,
>> and a regulator driver, and a reset driver. The DWC3 then uses
>> phandles to these drivers.
>>
>> How is the IO map area 65b0 split up. Can you cleanly separate it
>> into sub areas, which do not overlap, so you have a sub-area for the
>> PHY driver, a sub-area for the regulator driver and a sub-area for the
>> reset area? If you can cleanly split it up, you don't need an MFD. If
>> however the registers are in overlapping areas, you do need an
>> MFD. The MFD core provides access to the registers, while its children
>> implement PHY, reset, regulator etc.
>
> This device certainly sounds like an MFD to me.
>
> Can you share the DT you've written please?
This is still under the internal review in Socionext,
but I attached it below FWIW.
(I am not the author of this DT.
Written by Kunihiko Hayashi,
and clocks/resets parts were slightly modified by me.)
Just skimming the driver, I guess it will be possible to flatten
the node structure by separating the register space into sub-areas.
If this is success, we do not the MFD driver.
usb@65b0 {
compatible = "socionext,uniphier-ld20-usb3-glue",
"syscon";
reg = <0x65b0 0x1000>;
clock-names = "usb";
clocks = <&sys_clk 14>;
reset-names = "usb";
resets = <&sys_rst 14>;
usb_rst: reset {
compatible = "socionext,uniphier-ld20-usb3-reset";
#reset-cells = <1>;
};
regulators {
compatible = "socionext,uniphier-ld20-usb3-regulator";
usb_vbus0: vbus-0 { };
usb_vbus1: vbus-1 { };
usb_vbus2: vbus-2 { };
usb_vbus3: vbus-3 { };
};
usb_hsphy: hs-phy {
compatible = "socionext,uniphier-ld20-usb3-hsphy";
#address-cells = <1>;
#size-cells = <0>;
#phy-cells = <0>;
clock-names = "phy-clk0", "phy-clk1";
clocks = <&sys_clk 16>, <&sys_clk 17>;
reset-names = "phy-rst0", "phy-rst1";
resets = <&sys_rst 16>, <&sys_rst 17>;
port0-supply = <&usb_vbus0>;
port1-supply = <&usb_vbus1>;
port2-supply = <&usb_vbus2>;
port3-supply = <&usb_vbus3>;
port@0 {
reg = <0>;
nvmem-cell-names = "rterm", "sel_t",
"hs_i";
nvmem-cells = <&usb_rterm0>,
<&usb_sel_t0>,
<&usb_hs_i0>;
};
port@1 {
reg = <1>;
nvmem-cell-names = "rterm", "sel_t",
"hs_i";
nvmem-cells = <&usb_rterm1>,
Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control
2018-04-03 17:46 GMT+09:00 Philipp Zabel :
> On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
>> 2018-04-03 17:00 GMT+09:00 Philipp Zabel :
>> > On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
>> > > This driver handles the reset control in a common manner; deassert
>> > > resets before use, assert them after use. There is no good reason
>> > > why it should be exclusive.
>> >
>> > Is this preemptive cleanup, or do you have hardware on the horizon that
>> > shares these reset lines with other peripherals?
>>
>> This patch is necessary for Socionext SoCs.
>>
>> The same reset lines are shared between
>> this dwc3-of_simple and other glue circuits.
>
> Thanks, this is helpful information.
>
>> > > Also, use devm_ for clean-up.
>> > >
>> > > Signed-off-by: Masahiro Yamada
>> > > ---
>> > >
>> > > CCing Philipp Zabel.
>> > > I see his sob in commit 06c47e6286d5.
>> >
>> > At the time I was concerned with the reset_control_array addition and
>> > didn't look closely at the exclusive vs shared issue.
>> > > drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-
>> > > 1 file changed, 2 insertions(+), 5 deletions(-)
>> > >
>> > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>> > > b/drivers/usb/dwc3/dwc3-of-simple.c
>> > > index e54c362..bd6ab65 100644
>> > > --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> > > @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device
>> > > *pdev)
>> > > platform_set_drvdata(pdev, simple);
>> > > simple->dev = dev;
>> > >
>> > > - simple->resets = of_reset_control_array_get_optional_exclusive(np);
>> > > + simple->resets = devm_reset_control_array_get_optional_shared(dev);
>> >
>> > From the usage in the driver, it does indeed look like _shared reset
>> > usage is appropriate. I assume that the hardware has no need for the
>> > reset to be asserted right before probe or after remove, it just
>> > requires that the reset line is kept deasserted while the driver is
>> > probed.
>> >
>> > > if (IS_ERR(simple->resets)) {
>> > > ret = PTR_ERR(simple->resets);
>> > > dev_err(dev, "failed to get device resets, err=%d\n", ret);
>> > > @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct
>> > > platform_device *pdev)
>> > >
>> > > ret = reset_control_deassert(simple->resets);
>> > > if (ret)
>> > > - goto err_resetc_put;
>> > > + return ret;
>> > >
>> > > ret = dwc3_of_simple_clk_init(simple,
>> > > of_count_phandle_with_args(np,
>> > > "clocks", "#clock-cells"));
>> > > @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct
>> > > platform_device *pdev)
>> > > err_resetc_assert:
>> > > reset_control_assert(simple->resets);
>> > >
>> > > -err_resetc_put:
>> > > - reset_control_put(simple->resets);
>> > > return ret;
>> > > }
>> > >
>> > > @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct
>> > > platform_device *pdev)
>> > > simple->num_clocks = 0;
>> > >
>> > > reset_control_assert(simple->resets);
>> > > - reset_control_put(simple->resets);
>> > >
>> > > pm_runtime_put_sync(dev);
>> > > pm_runtime_disable(dev);
>> >
>> > Changing to devm_ changes the order here. Whether or not it could be a
>> > problem to assert the reset only after pm_runtime_put (or potentially
>> > never), I can't say. I assume this is a non-issue, but somebody who
>> > knows the hardware better would have to decide.
>>
>>
>>
>> I do not understand what you mean.
>
> Sorry for the confusion, I have mixed up things here.
>
>> Can you describe your concern in more details?
>>
>> I am not touching reset_control_assert() here.
>
> With the change to shared reset control, reset_control_assert
> potentially does nothing, so it could be possible that
> pm_runtime_put_sync cuts the power before the reset es
Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control
2018-04-03 17:00 GMT+09:00 Philipp Zabel :
> On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
>> This driver handles the reset control in a common manner; deassert
>> resets before use, assert them after use. There is no good reason
>> why it should be exclusive.
>
> Is this preemptive cleanup, or do you have hardware on the horizon that
> shares these reset lines with other peripherals?
This patch is necessary for Socionext SoCs.
The same reset lines are shared between
this dwc3-of_simple and other glue circuits.
>> Also, use devm_ for clean-up.
>>
>> Signed-off-by: Masahiro Yamada
>> ---
>>
>> CCing Philipp Zabel.
>> I see his sob in commit 06c47e6286d5.
>
> At the time I was concerned with the reset_control_array addition and
> didn't look closely at the exclusive vs shared issue.
>> drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>> b/drivers/usb/dwc3/dwc3-of-simple.c
>> index e54c362..bd6ab65 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device
>> *pdev)
>> platform_set_drvdata(pdev, simple);
>> simple->dev = dev;
>>
>> - simple->resets = of_reset_control_array_get_optional_exclusive(np);
>> + simple->resets = devm_reset_control_array_get_optional_shared(dev);
>
> From the usage in the driver, it does indeed look like _shared reset
> usage is appropriate. I assume that the hardware has no need for the
> reset to be asserted right before probe or after remove, it just
> requires that the reset line is kept deasserted while the driver is
> probed.
>
>> if (IS_ERR(simple->resets)) {
>> ret = PTR_ERR(simple->resets);
>> dev_err(dev, "failed to get device resets, err=%d\n", ret);
>> @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct platform_device
>> *pdev)
>>
>> ret = reset_control_deassert(simple->resets);
>> if (ret)
>> - goto err_resetc_put;
>> + return ret;
>>
>> ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
>> "clocks", "#clock-cells"));
>> @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct platform_device
>> *pdev)
>> err_resetc_assert:
>> reset_control_assert(simple->resets);
>>
>> -err_resetc_put:
>> - reset_control_put(simple->resets);
>> return ret;
>> }
>>
>> @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct platform_device
>> *pdev)
>> simple->num_clocks = 0;
>>
>> reset_control_assert(simple->resets);
>> - reset_control_put(simple->resets);
>>
>> pm_runtime_put_sync(dev);
>> pm_runtime_disable(dev);
>
> Changing to devm_ changes the order here. Whether or not it could be a
> problem to assert the reset only after pm_runtime_put (or potentially
> never), I can't say. I assume this is a non-issue, but somebody who
> knows the hardware better would have to decide.
I do not understand what you mean.
Can you describe your concern in more details?
I am not touching reset_control_assert() here.
I am delaying the call for reset_control_put().
If I understand reset_control_put() correctly,
the effects of this change are:
- The ref_count and module ownership for the reset controller
driver will be held a little longer
- The call for kfree() will be a little bit delayed.
Why do you need knowledge about this hardware?
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Question] MFD driver that handles clocks/resets and populates child nodes
2018-04-02 21:04 GMT+09:00 Andrew Lunn : >> The maintainer of DWC3, Felipe Balbi, requested to >> split the glue layer driver into small parts such as >> reset, regulator, phy, etc. > > What exactly did Felipe ask for? Did he ask that the patch be split > up, one patch per reset, regulator, phy etc? Yeah. That is what we understood from his comments. These are the feed-backs from him. https://lkml.org/lkml/2018/1/23/298 https://lkml.org/lkml/2018/1/24/352 > Are all these resources used just by the DWC3? Or is it a true MFD, > multiple functions? I do not think this is a real MFD. This is a DWC3 glue layer, i.e. a collection of misc registers that control the DWC3 IP. Just splitting it into small pieces to use PHY, reset, regulator framework in Linux. Of course, the price of this approach is so cluttered Device Tree, honestly I do not like it much. > > Andrew > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Question] MFD driver that handles clocks/resets and populates child nodes
Hi.
Some MFD drivers populate child nodes
after some basic hardware setups.
My question is, is it acceptable to upstream
a MFD driver that handles only clocks/resets,
then calls of_platform_populate.
The probe function will look like follows:
static int uniphier_usb3_glue_probe(struct platform_device *pdev)
{
[get some clocks];
[get some resets];
[enable some clocks];
[deassert some resets];
return devm_of_platform_populate(dev);
}
With this driver, the child devices do not need
to handle clocks/resets.
But, I am not sure if it is the right thing to do.
Background of this question
---
Socionext is trying to upstream
the glue layer for DWC3 USB IP.
The original patch is this.
https://patchwork.kernel.org/patch/10180167/
This driver enables clocks, deassert resets,
and sets up some more registers,
then populates the DWC3 core node.
The maintainer of DWC3, Felipe Balbi, requested to
split the glue layer driver into small parts such as
reset, regulator, phy, etc.
So, the node will look like follows:
usb-glue {
reset {
...
};
regulator {
...
};
hs-phy {
...
};
ss-phy {
...
};
}
Here, one question popped up.
Where should the hardware resources be described?
The register, clocks, resets are shared among the sub-nodes.
This is the obvious result since it is a single hardware block
in the first place, and we are splitting it into small chunks.
If the MFD driver approach is acceptable,
clocks, resets will be handles in the parent node.
(Such a driver looks stupid, though)
usb-glue {
compatible = "socionext,uniphier-ld20-usb3-glue",
"syscon";
reg = <0x65b0 0x1000>;
clocks = ;
resets = ;
reset {
...
};
regulator {
...
};
hs-phy {
...
};
ss-phy {
...
};
}
Of course, it is possible to use "simple-mfd"
and each driver can repeat the same clock/reset like follows.
(this also looks a bit clumsy...)
usb-glue {
compatible = "socionext,uniphier-ld20-usb3-glue",
"simple-mfd", "syscon";
reg = <0x65b0 0x1000>;
reset {
clocks = ;
resets = ;
...
};
regulator {
clocks = ;
resets = ;
...
};
hs-phy {
clocks = ;
resets = ;
...
};
ss-phy {
clocks = ;
resets = ;
...
};
}
Which is better?
Or any other good idea?
Thanks.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc3: of-simple: use managed and shared reset control
This driver handles the reset control in a common manner; deassert
resets before use, assert them after use. There is no good reason
why it should be exclusive.
Also, use devm_ for clean-up.
Signed-off-by: Masahiro Yamada
---
CCing Philipp Zabel.
I see his sob in commit 06c47e6286d5.
drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
b/drivers/usb/dwc3/dwc3-of-simple.c
index e54c362..bd6ab65 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, simple);
simple->dev = dev;
- simple->resets = of_reset_control_array_get_optional_exclusive(np);
+ simple->resets = devm_reset_control_array_get_optional_shared(dev);
if (IS_ERR(simple->resets)) {
ret = PTR_ERR(simple->resets);
dev_err(dev, "failed to get device resets, err=%d\n", ret);
@@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct platform_device
*pdev)
ret = reset_control_deassert(simple->resets);
if (ret)
- goto err_resetc_put;
+ return ret;
ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
"clocks", "#clock-cells"));
@@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct platform_device
*pdev)
err_resetc_assert:
reset_control_assert(simple->resets);
-err_resetc_put:
- reset_control_put(simple->resets);
return ret;
}
@@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct platform_device
*pdev)
simple->num_clocks = 0;
reset_control_assert(simple->resets);
- reset_control_put(simple->resets);
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc3: add clock and resets
2018-03-19 7:37 GMT+09:00 Masahiro Yamada :
> Hi Rob,
>
> 2018-03-18 21:52 GMT+09:00 Rob Herring :
>> On Thu, Mar 15, 2018 at 08:39:58PM +0900, Masahiro Yamada wrote:
>>> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
>>> They are both generic enough to be put into the dwc3 core. For simple
>>> cases, a nested node structure like follows:
>>>
>>> dwc3-glue {
>>> compatible = "foo,dwc3";
>>> clocks = ...;
>>> resets = ...;
>>> ...
>>>
>>> dwc3 {
>>> compatible = "snps,dwc3";
>>> ...
>>> };
>>
>> I'm not a fan of how this was done.
>>
>>
>>> }
>>>
>>> would be turned into a single node:
>>>
>>> dwc3 {
>>> compatible = "foo,dwc3", "snps,dwc3";
>>> clocks = ...;
>>> resets = ...;
>>> ...
>>> }
>>
>> And yes, this is what I'd prefer.
>
>
>
> Not only dwc3-of-simple.c, but all dwc3 nodes are
> written like this.
>
>
> omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3";
> reg = <0x4888 0x1>;
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
> ...
>
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> ...
> };
> };
>
>
> The glue layer initializes SoC-specific things,
> then populates the child "snps,dwc3".
>
>
> I think the following structure should work
> by handling EPROBE_DEFER properly.
>
> omap_dwc3_1: omap_dwc3_1@4888 {
> compatible = "ti,dwc3"; (should be "ti,dwc3-glue" or something)
> reg = <0x4888 0x1>;
> ...
> };
>
> usb1: usb@4889 {
> compatible = "snps,dwc3";
> reg = <0x4889 0x17000>;
> ...
> };
>
>
>
>>>
>>> I inserted reset_control_deassert() and clk_enable() before the first
>>> register access, i.e. dwc3_cache_hwparams().
>>>
>>> Signed-off-by: Masahiro Yamada
>>> ---
>>>
>>> Documentation/devicetree/bindings/usb/dwc3.txt | 2 +
>>> drivers/usb/dwc3/core.c| 127
>>> -
>>> drivers/usb/dwc3/core.h| 5 +
>>> 3 files changed, 132 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index 44e8bab..67e9cfb 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -9,12 +9,14 @@ Required properties:
>>> - interrupts: Interrupts used by the dwc3 controller.
>>>
>>> Optional properties:
>>> + - clocks: list of phandle and clock specifier pairs
>>
>> However, this should be specific as to how many clocks and their
>> function. This should be readily available to someone with access to
>> Synopsys datasheet. The number of clocks should generally be the same
>> across SoCs, it is just some SoCs either tie clocks together or don't
>> provide s/w control of some of the clocks.
>
>
> Make sense.
> You also implies this property is mandatory.
> The number of clocks should be available in the datasheet
> and no hardware can work with zero clock.
>
> However, making it mandatory breaks the binding
> since the existing DT files do not specify clocks at all
> in the "snps,dwc3" node.
>
>
>
> Anyway, our current situation:
>
> - We have the dwc3-core under the glue layer node
> despite they are independent in the CPU address view
> - We add all sorts of clocks and resets in the glue layer node,
> and nothing in the dwc3-core node.
>
> If these are design mistake, what should we do?
>
> Continue development based on it?
> If we fix it, how to change the course?
>
Any insight about this?
I think this is rather a general question.
If somebody upstreams a driver without clocks,
then later it turns out clocks are necessary,
adding required clocks would break existing platforms
since clk_get() fails.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc3: add clock and resets
Hi Rob,
2018-03-18 21:52 GMT+09:00 Rob Herring :
> On Thu, Mar 15, 2018 at 08:39:58PM +0900, Masahiro Yamada wrote:
>> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
>> They are both generic enough to be put into the dwc3 core. For simple
>> cases, a nested node structure like follows:
>>
>> dwc3-glue {
>> compatible = "foo,dwc3";
>> clocks = ...;
>> resets = ...;
>> ...
>>
>> dwc3 {
>> compatible = "snps,dwc3";
>> ...
>> };
>
> I'm not a fan of how this was done.
>
>
>> }
>>
>> would be turned into a single node:
>>
>> dwc3 {
>> compatible = "foo,dwc3", "snps,dwc3";
>> clocks = ...;
>> resets = ...;
>> ...
>> }
>
> And yes, this is what I'd prefer.
Not only dwc3-of-simple.c, but all dwc3 nodes are
written like this.
omap_dwc3_1: omap_dwc3_1@4888 {
compatible = "ti,dwc3";
reg = <0x4888 0x1>;
#address-cells = <1>;
#size-cells = <1>;
ranges;
...
usb1: usb@4889 {
compatible = "snps,dwc3";
reg = <0x4889 0x17000>;
...
};
};
The glue layer initializes SoC-specific things,
then populates the child "snps,dwc3".
I think the following structure should work
by handling EPROBE_DEFER properly.
omap_dwc3_1: omap_dwc3_1@4888 {
compatible = "ti,dwc3"; (should be "ti,dwc3-glue" or something)
reg = <0x48880000 0x1>;
...
};
usb1: usb@4889 {
compatible = "snps,dwc3";
reg = <0x4889 0x17000>;
...
};
>>
>> I inserted reset_control_deassert() and clk_enable() before the first
>> register access, i.e. dwc3_cache_hwparams().
>>
>> Signed-off-by: Masahiro Yamada
>> ---
>>
>> Documentation/devicetree/bindings/usb/dwc3.txt | 2 +
>> drivers/usb/dwc3/core.c| 127
>> -
>> drivers/usb/dwc3/core.h| 5 +
>> 3 files changed, 132 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 44e8bab..67e9cfb 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -9,12 +9,14 @@ Required properties:
>> - interrupts: Interrupts used by the dwc3 controller.
>>
>> Optional properties:
>> + - clocks: list of phandle and clock specifier pairs
>
> However, this should be specific as to how many clocks and their
> function. This should be readily available to someone with access to
> Synopsys datasheet. The number of clocks should generally be the same
> across SoCs, it is just some SoCs either tie clocks together or don't
> provide s/w control of some of the clocks.
Make sense.
You also implies this property is mandatory.
The number of clocks should be available in the datasheet
and no hardware can work with zero clock.
However, making it mandatory breaks the binding
since the existing DT files do not specify clocks at all
in the "snps,dwc3" node.
Anyway, our current situation:
- We have the dwc3-core under the glue layer node
despite they are independent in the CPU address view
- We add all sorts of clocks and resets in the glue layer node,
and nothing in the dwc3-core node.
If these are design mistake, what should we do?
Continue development based on it?
If we fix it, how to change the course?
>> - usb-phy : array of phandle for the PHY device. The first element
>> in the array is expected to be a handle to the USB2/HS PHY and
>> the second element is expected to be a handle to the USB3/SS PHY
>> - phys: from the *Generic PHY* bindings
>> - phy-names: from the *Generic PHY* bindings; supported names are
>> "usb2-phy"
>> or "usb3-phy".
>> + - resets: list of phandle and reset specifier pairs
>
> ditto
>
Same issue as the clocks.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: dwc3: use local copy of resource to fix-up register offset
It is not a good idea to modify the resource from the platform device.
Modify its local copy to pass it to devm_ioremap_resource() so that we
do not need to restore it in the failure path and the remove hook.
Signed-off-by: Masahiro Yamada
---
drivers/usb/dwc3/core.c | 32
1 file changed, 8 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index f1d838a..e9083a3 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1164,7 +1164,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
static int dwc3_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct resource *res;
+ struct resource *res, dwc_res;
struct dwc3 *dwc;
int ret;
@@ -1189,20 +1189,19 @@ static int dwc3_probe(struct platform_device *pdev)
dwc->xhci_resources[0].flags = res->flags;
dwc->xhci_resources[0].name = res->name;
- res->start += DWC3_GLOBALS_REGS_START;
-
/*
* Request memory region but exclude xHCI regs,
* since it will be requested by the xhci-plat driver.
*/
- regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(regs)) {
- ret = PTR_ERR(regs);
- goto err0;
- }
+ dwc_res = *res;
+ dwc_res.start += DWC3_GLOBALS_REGS_START;
+
+ regs = devm_ioremap_resource(dev, &dwc_res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
dwc->regs = regs;
- dwc->regs_size = resource_size(res);
+ dwc->regs_size = resource_size(&dwc_res);
dwc3_get_properties(dwc);
@@ -1269,29 +1268,14 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-err0:
- /*
-* restore res->start back to its original value so that, in case the
-* probe is deferred, we don't end up getting error in request the
-* memory region the next time probe is called.
-*/
- res->start -= DWC3_GLOBALS_REGS_START;
-
return ret;
}
static int dwc3_remove(struct platform_device *pdev)
{
struct dwc3 *dwc = platform_get_drvdata(pdev);
- struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pm_runtime_get_sync(&pdev->dev);
- /*
-* restore res->start back to its original value so that, in case the
-* probe is deferred, we don't end up getting error in request the
-* memory region the next time probe is called.
-*/
- res->start -= DWC3_GLOBALS_REGS_START;
dwc3_debugfs_exit(dwc);
dwc3_core_exit_mode(dwc);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: dwc3: add clock and resets
dwc3-of-simple.c only handles arbitrary number of clocks and resets.
They are both generic enough to be put into the dwc3 core. For simple
cases, a nested node structure like follows:
dwc3-glue {
compatible = "foo,dwc3";
clocks = ...;
resets = ...;
...
dwc3 {
compatible = "snps,dwc3";
...
};
}
would be turned into a single node:
dwc3 {
compatible = "foo,dwc3", "snps,dwc3";
clocks = ...;
resets = ...;
...
}
I inserted reset_control_deassert() and clk_enable() before the first
register access, i.e. dwc3_cache_hwparams().
Signed-off-by: Masahiro Yamada
---
Documentation/devicetree/bindings/usb/dwc3.txt | 2 +
drivers/usb/dwc3/core.c| 127 -
drivers/usb/dwc3/core.h| 5 +
3 files changed, 132 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index 44e8bab..67e9cfb 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -9,12 +9,14 @@ Required properties:
- interrupts: Interrupts used by the dwc3 controller.
Optional properties:
+ - clocks: list of phandle and clock specifier pairs
- usb-phy : array of phandle for the PHY device. The first element
in the array is expected to be a handle to the USB2/HS PHY and
the second element is expected to be a handle to the USB3/SS PHY
- phys: from the *Generic PHY* bindings
- phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
or "usb3-phy".
+ - resets: list of phandle and reset specifier pairs
- snps,usb3_lpm_capable: determines if platform is USB3 LPM capable
- snps,disable_scramble_quirk: true when SW should disable data scrambling.
Only really useful for FPGA builds.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index e9083a3..f17e4a9 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -8,6 +8,7 @@
* Sebastian Andrzej Siewior
*/
+#include
#include
#include
#include
@@ -24,6 +25,7 @@
#include
#include
#include
+#include
#include
#include
@@ -240,6 +242,74 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
return -ETIMEDOUT;
}
+static int dwc3_core_get_clks(struct dwc3 *dwc)
+{
+ struct device *dev = dwc->dev;
+ struct device_node *node = dev->of_node;
+ struct clk *clk;
+ int num_clks, i;
+
+ num_clks = of_count_phandle_with_args(node, "clocks", "#clock-cells");
+ if (num_clks <= 0)
+ return 0;
+
+ dwc->num_clks = num_clks;
+
+ dwc->clks = devm_kcalloc(dev, num_clks, sizeof(*dwc->clks), GFP_KERNEL);
+ if (!dwc->clks)
+ return -ENOMEM;
+
+ for (i = 0; i < num_clks; i++) {
+ clk = of_clk_get(node, i);
+ if (IS_ERR(clk))
+ goto put_clks;
+ dwc->clks[i] = clk;
+ }
+
+ return 0;
+
+put_clks:
+ while (--i >= 0)
+ clk_put(dwc->clks[i]);
+
+ return PTR_ERR(clk);
+}
+
+static void dwc3_core_put_clks(struct dwc3 *dwc)
+{
+ int i;
+
+ for (i = dwc->num_clks - 1; i >= 0; i--)
+ clk_put(dwc->clks[i]);
+}
+
+static int dwc3_core_enable_clks(struct dwc3 *dwc)
+{
+ int ret, i;
+
+ for (i = 0; i < dwc->num_clks; i++) {
+ ret = clk_prepare_enable(dwc->clks[i]);
+ if (ret)
+ goto disable_clks;
+ }
+
+ return 0;
+
+disable_clks:
+ while (--i >= 0)
+ clk_disable_unprepare(dwc->clks[i]);
+
+ return ret;
+}
+
+static void dwc3_core_disable_clks(struct dwc3 *dwc)
+{
+ int i;
+
+ for (i = dwc->num_clks - 1; i >= 0; i--)
+ clk_disable_unprepare(dwc->clks[i]);
+}
+
/*
* dwc3_frame_length_adjustment - Adjusts frame length if required
* @dwc3: Pointer to our controller context structure
@@ -641,6 +711,8 @@ static void dwc3_core_exit(struct dwc3 *dwc)
usb_phy_set_suspend(dwc->usb3_phy, 1);
phy_power_off(dwc->usb2_generic_phy);
phy_power_off(dwc->usb3_generic_phy);
+ dwc3_core_disable_clks(dwc);
+ reset_control_assert(dwc->resets);
}
static bool dwc3_core_is_valid(struct dwc3 *dwc)
@@ -1205,6 +1277,22 @@ static int dwc3_probe(struct platform_device *pdev)
dwc3_get_properties(dwc);
+ dwc->resets = devm_reset_control_array_get_optional_shared(dev);
+ if (IS_ERR(dwc->resets))
+ return PTR_ERR(dwc->resets);
+
+ ret = dwc3_core_get_clks(dwc);
+ if (ret)
+ return ret;
+
+ ret =
Re: [RFC PATCH] drivers: use 'depends on MFD_SYSCON' instead of 'select MFD_SYSCON'
2018-02-27 18:03 GMT+09:00 Arnd Bergmann : > On Tue, Feb 27, 2018 at 1:46 AM, Masahiro Yamada > wrote: >> 2018-02-26 21:43 GMT+09:00 Arnd Bergmann : >>> On Mon, Feb 26, 2018 at 12:53 PM, Masahiro Yamada >>> wrote: >>>> 2018-02-26 17:43 GMT+09:00 Arnd Bergmann : >>>>> On Sat, Feb 24, 2018 at 3:50 PM, Masahiro Yamada >>>>> wrote: >>>>>> As Documentation/kbuild/kconfig-language.txt notes, 'select' should be >>>>>> used with care - it forces a lower limit of another symbol, ignoring >>>>>> the dependency. >>>>>> >>>>>> MFD_SYSCON depends on HAS_IOMEM, but several drivers with COMPILE_TEST >>>>>> select it. >>>>>> >>>>>> This causes unmet dependencies for architecture without HAS_IOMEM. >>>>>> >>>>>> $ make ARCH=score randconfig >>>>>> scripts/kconfig/conf --randconfig Kconfig >>>>>> KCONFIG_SEED=0x27C47F43 >>>>>> warning: (HWSPINLOCK_QCOM && AHCI_MTK && STMMAC_PLATFORM && ...) >>>>>> selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM) >>>>>> >>>>>> Use 'depends on' to observe the dependency. >>>>>> >>>>>> This commit was created by the following command: >>>>>> >>>>>> $ find drivers -name 'Kconfig*' | xargs sed -i -e \ >>>>>> 's/select MFD_SYSCON$/depends on MFD_SYSCON/' >>>>>> >>>>>> Then, COMMON_CLK_NXP and S3C2410_WATCHDOG were fixed up manually. >>>>>> >>>>>> Also, make MFD_SYSCON 'default y' because some defconfig files may >>>>>> rely on someone select's MFD_SYSCON. >>>>>> >>>>>> Signed-off-by: Masahiro Yamada >>>>>> --- >>>>>> >>>>>> If you have a better idea to fix 'unmet dependencies', >>>>>> please suggest. >>>>> >>>>> Changing 'select MFD_SYSCON' to 'depends on' will definitely break lots >>>>> of defconfig configurations, I'd rather not do that. >>>> >>>> >>>> Could you explain why? >>>> >>>> I set 'default y' for MFD_SYSCON. >>>> >>>> Would it still break defconfig configurations? >>> >>> No, you are right, that would not break defconfigs, it would just mean one >>> useless driver being enabled for many configurations that don't need it. >> >> If we are unhappy about this, >> we can send per-arch patches >> to add CONFIG_MTD_SYSCON=y to defconfigs that need it. >> >> But, we need to decide what the right solution is. > > I think for consistency, we should change the existing > 'depends on MFD_SYSCON' to 'select MFD_SYSCON'. This > matches what we do with REGMAP_MMIO. > > MFD_SYSCON is really a thin wrapper around REGMAP_MMIO, > so I would keep using the same conventions here, even though > we normally prefer to not 'select' any user-visible options. > > It might be possible to make MFD_SYSCON a silent symbol > as well, but we'd have to make sure that all users select the symbol > then. > > Arnd If we agree, I can send the following three patches. [1] Add "depends on HAS_IOMEM" to all drivers selecting MFD_SYSCON (Unmet dependencies will be fixed by this) [2] For consistency, convert existing "depends on MFD_SYSCON" to "select MFD_SYSCON" + "depends on HAS_IOMEM" [3] Change MFD_SYSCON to user-unconfigurable option. But, for COMPILE_TEST, allow users to enable it independently. Like follows: config MFD_SYSCON bool "System Controller Register R/W Based on Regmap" if COMPILE_TEST select REGMAP_MMIO help Select this option to enable accessing system control registers via regmap. Is this OK? -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" 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] drivers: use 'depends on MFD_SYSCON' instead of 'select MFD_SYSCON'
2018-02-26 21:43 GMT+09:00 Arnd Bergmann : > On Mon, Feb 26, 2018 at 12:53 PM, Masahiro Yamada > wrote: >> 2018-02-26 17:43 GMT+09:00 Arnd Bergmann : >>> On Sat, Feb 24, 2018 at 3:50 PM, Masahiro Yamada >>> wrote: >>>> As Documentation/kbuild/kconfig-language.txt notes, 'select' should be >>>> used with care - it forces a lower limit of another symbol, ignoring >>>> the dependency. >>>> >>>> MFD_SYSCON depends on HAS_IOMEM, but several drivers with COMPILE_TEST >>>> select it. >>>> >>>> This causes unmet dependencies for architecture without HAS_IOMEM. >>>> >>>> $ make ARCH=score randconfig >>>> scripts/kconfig/conf --randconfig Kconfig >>>> KCONFIG_SEED=0x27C47F43 >>>> warning: (HWSPINLOCK_QCOM && AHCI_MTK && STMMAC_PLATFORM && ...) >>>> selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM) >>>> >>>> Use 'depends on' to observe the dependency. >>>> >>>> This commit was created by the following command: >>>> >>>> $ find drivers -name 'Kconfig*' | xargs sed -i -e \ >>>> 's/select MFD_SYSCON$/depends on MFD_SYSCON/' >>>> >>>> Then, COMMON_CLK_NXP and S3C2410_WATCHDOG were fixed up manually. >>>> >>>> Also, make MFD_SYSCON 'default y' because some defconfig files may >>>> rely on someone select's MFD_SYSCON. >>>> >>>> Signed-off-by: Masahiro Yamada >>>> --- >>>> >>>> If you have a better idea to fix 'unmet dependencies', >>>> please suggest. >>> >>> Changing 'select MFD_SYSCON' to 'depends on' will definitely break lots >>> of defconfig configurations, I'd rather not do that. >> >> >> Could you explain why? >> >> I set 'default y' for MFD_SYSCON. >> >> Would it still break defconfig configurations? > > No, you are right, that would not break defconfigs, it would just mean one > useless driver being enabled for many configurations that don't need it. If we are unhappy about this, we can send per-arch patches to add CONFIG_MTD_SYSCON=y to defconfigs that need it. But, we need to decide what the right solution is. >>> Only score, tile and um have some configurations that select 'NO_IOMEM'. >>> Score is getting removed now, tile might get removed later (we could make >>> PCI mandatory in the meantime to avoid that configuration), and I think for >>> um, we already have a workaround for the NO_IOMEM dependencies >>> (I forget the details). >> >> I do not think this is a stable solution. >> >> Or, do you mean to remove NO_IOMEM and HAS_IOMEM completely? > > We could either leave it for arch/um only and have that deal with the > issues (which I think we already have), or we could remove the options > entirely. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] usb: dwc3: add dwc3 glue layer for UniPhier SoCs
c3/core.c about regulators, we can do that. But we don't
> need SoC-specific hacks ;-)
>
> --
> balbi
Slightly related question.
Why don't we put clocks and resets to dwc3/core.c?
dwc3-of-simple.c only handles clocks and resets.
This is generic enough to be added to dwc3/core.c, I think.
I checked the two instances of dwc3-of-simple.
"qcom,dwc3"
https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/qcom/msm8996.dtsi#L780
"rockchip,rk3399-dwc3"
https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L395
They just contain clocks, resets, and "snps,dwc3" sub-node.
If we do that,
usb@760 {
compatible = "qcom,dwc3";
clocks = ...;
dwc3@760 {
compatible = "snps,dwc3";
reg = ...;
interrupts = ...;
phys = ...;
}
};
will be turned into
dwc3@760 {
compatible = "qcom,dwc3", "snps,dwc3";
reg = ...;
clocks = ...;
interrupts = ...;
phys = ...;
};
This looks simpler.
Also, dwc3/core.c and dwc3-of-simple.c
duplicate runtime PM.
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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] drivers: use 'depends on MFD_SYSCON' instead of 'select MFD_SYSCON'
2018-02-26 17:43 GMT+09:00 Arnd Bergmann : > On Sat, Feb 24, 2018 at 3:50 PM, Masahiro Yamada > wrote: >> As Documentation/kbuild/kconfig-language.txt notes, 'select' should be >> used with care - it forces a lower limit of another symbol, ignoring >> the dependency. >> >> MFD_SYSCON depends on HAS_IOMEM, but several drivers with COMPILE_TEST >> select it. >> >> This causes unmet dependencies for architecture without HAS_IOMEM. >> >> $ make ARCH=score randconfig >> scripts/kconfig/conf --randconfig Kconfig >> KCONFIG_SEED=0x27C47F43 >> warning: (HWSPINLOCK_QCOM && AHCI_MTK && STMMAC_PLATFORM && ...) >> selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM) >> >> Use 'depends on' to observe the dependency. >> >> This commit was created by the following command: >> >> $ find drivers -name 'Kconfig*' | xargs sed -i -e \ >> 's/select MFD_SYSCON$/depends on MFD_SYSCON/' >> >> Then, COMMON_CLK_NXP and S3C2410_WATCHDOG were fixed up manually. >> >> Also, make MFD_SYSCON 'default y' because some defconfig files may >> rely on someone select's MFD_SYSCON. >> >> Signed-off-by: Masahiro Yamada >> --- >> >> If you have a better idea to fix 'unmet dependencies', >> please suggest. > > Changing 'select MFD_SYSCON' to 'depends on' will definitely break lots > of defconfig configurations, I'd rather not do that. Could you explain why? I set 'default y' for MFD_SYSCON. Would it still break defconfig configurations? > Only score, tile and um have some configurations that select 'NO_IOMEM'. > Score is getting removed now, tile might get removed later (we could make > PCI mandatory in the meantime to avoid that configuration), and I think for > um, we already have a workaround for the NO_IOMEM dependencies > (I forget the details). I do not think this is a stable solution. Or, do you mean to remove NO_IOMEM and HAS_IOMEM completely? -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] drivers: use 'depends on MFD_SYSCON' instead of 'select MFD_SYSCON'
As Documentation/kbuild/kconfig-language.txt notes, 'select' should be used with care - it forces a lower limit of another symbol, ignoring the dependency. MFD_SYSCON depends on HAS_IOMEM, but several drivers with COMPILE_TEST select it. This causes unmet dependencies for architecture without HAS_IOMEM. $ make ARCH=score randconfig scripts/kconfig/conf --randconfig Kconfig KCONFIG_SEED=0x27C47F43 warning: (HWSPINLOCK_QCOM && AHCI_MTK && STMMAC_PLATFORM && ...) selects MFD_SYSCON which has unmet direct dependencies (HAS_IOMEM) Use 'depends on' to observe the dependency. This commit was created by the following command: $ find drivers -name 'Kconfig*' | xargs sed -i -e \ 's/select MFD_SYSCON$/depends on MFD_SYSCON/' Then, COMMON_CLK_NXP and S3C2410_WATCHDOG were fixed up manually. Also, make MFD_SYSCON 'default y' because some defconfig files may rely on someone select's MFD_SYSCON. Signed-off-by: Masahiro Yamada --- If you have a better idea to fix 'unmet dependencies', please suggest. drivers/ata/Kconfig | 2 +- drivers/clk/Kconfig | 9 - drivers/clk/imgtec/Kconfig | 2 +- drivers/clocksource/Kconfig | 4 ++-- drivers/dma/Kconfig | 2 +- drivers/gpu/drm/exynos/Kconfig | 2 +- drivers/hwspinlock/Kconfig | 2 +- drivers/input/touchscreen/Kconfig | 2 +- drivers/irqchip/Kconfig | 2 +- drivers/media/platform/Kconfig | 2 +- drivers/media/platform/exynos4-is/Kconfig | 2 +- drivers/memory/Kconfig | 2 +- drivers/mfd/Kconfig | 5 +++-- drivers/net/ethernet/hisilicon/Kconfig | 2 +- drivers/net/ethernet/stmicro/stmmac/Kconfig | 16 drivers/net/ethernet/ti/Kconfig | 2 +- drivers/pci/dwc/Kconfig | 2 +- drivers/pci/host/Kconfig| 2 +- drivers/phy/hisilicon/Kconfig | 4 ++-- drivers/phy/ralink/Kconfig | 2 +- drivers/phy/rockchip/Kconfig| 2 +- drivers/phy/samsung/Kconfig | 6 +++--- drivers/phy/ti/Kconfig | 2 +- drivers/pinctrl/Kconfig | 6 +++--- drivers/pinctrl/mvebu/Kconfig | 4 ++-- drivers/pinctrl/stm32/Kconfig | 2 +- drivers/power/reset/Kconfig | 6 +++--- drivers/remoteproc/Kconfig | 4 ++-- drivers/reset/Kconfig | 4 ++-- drivers/rtc/Kconfig | 2 +- drivers/soc/qcom/Kconfig| 2 +- drivers/staging/media/omap4iss/Kconfig | 2 +- drivers/usb/host/Kconfig| 2 +- drivers/video/fbdev/Kconfig | 2 +- drivers/watchdog/Kconfig| 4 ++-- 35 files changed, 59 insertions(+), 59 deletions(-) diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index a7120d6..e70753c 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -176,7 +176,7 @@ config AHCI_CEVA config AHCI_MTK tristate "MediaTek AHCI SATA support" depends on ARCH_MEDIATEK - select MFD_SYSCON + depends on MFD_SYSCON help This option enables support for the MediaTek SoC's onboard AHCI SATA controller. diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 98ce9fc..b4950d8 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -136,7 +136,7 @@ config COMMON_CLK_CS2000_CP config COMMON_CLK_GEMINI bool "Clock driver for Cortina Systems Gemini SoC" depends on ARCH_GEMINI || COMPILE_TEST - select MFD_SYSCON + depends on MFD_SYSCON select RESET_CONTROLLER ---help--- This driver supports the SoC clocks on the Cortina Systems Gemini @@ -146,7 +146,7 @@ config COMMON_CLK_ASPEED bool "Clock driver for Aspeed BMC SoCs" depends on ARCH_ASPEED || COMPILE_TEST default ARCH_ASPEED - select MFD_SYSCON + depends on MFD_SYSCON select RESET_CONTROLLER ---help--- This driver supports the SoC clocks on the Aspeed BMC platforms. @@ -193,9 +193,8 @@ config COMMON_CLK_XGENE Sypport for the APM X-Gene SoC reference, PLL, and device clocks. config COMMON_CLK_NXP - def_bool COMMON_CLK && (ARCH_LPC18XX || ARCH_LPC32XX) + def_bool COMMON_CLK && ((ARCH_LPC18XX && MFD_SYSCON) || ARCH_LPC32XX) select REGMAP_MMIO if ARCH_LPC32XX - select MFD_SYSCON if ARCH_LPC18XX ---help--- Support for clock providers on NXP platforms. @@ -224,7 +223,7 @@ config COMMON_CLK_PIC32 config COMMON_CLK_OXNAS bool "Clock driver for the OXNAS SoC Family&
[PATCH] usb: dwc3: of-simple: set dev_pm_ops
dwc3_of_simple_dev_pm_ops has never been used since the initial support
by commit 16adc674d0d6 ("usb: dwc3: add generic OF glue layer").
I guess it just missed to set .pm struct member.
Signed-off-by: Masahiro Yamada
---
drivers/usb/dwc3/dwc3-of-simple.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
b/drivers/usb/dwc3/dwc3-of-simple.c
index c4a4d7b..c9febdc 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -203,6 +203,7 @@ static struct platform_driver dwc3_of_simple_driver = {
.driver = {
.name = "dwc3-of-simple",
.of_match_table = of_dwc3_simple_match,
+ .pm = &dwc3_of_simple_dev_pm_ops,
},
};
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] usb: ehci-platform: use reset array API
Generic drivers like this need to control arbitrary number of reset
lines. Instead of hard-coding the maximum number of resets, use the
reset array API. It can manage a bunch of resets behind the scene.
Signed-off-by: Masahiro Yamada
---
Changes in v2:
- Goto err_put_clks when it fails to get reset_control.
drivers/usb/host/ehci-platform.c | 35 +--
1 file changed, 13 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index f1908ea..a41acd6 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -40,12 +40,11 @@
#define DRIVER_DESC "EHCI generic platform driver"
#define EHCI_MAX_CLKS 4
-#define EHCI_MAX_RSTS 4
#define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
struct ehci_platform_priv {
struct clk *clks[EHCI_MAX_CLKS];
- struct reset_control *rsts[EHCI_MAX_RSTS];
+ struct reset_control *rsts;
struct phy **phys;
int num_phys;
bool reset_on_resume;
@@ -151,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev)
struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ehci_platform_priv *priv;
struct ehci_hcd *ehci;
- int err, irq, phy_num, clk = 0, rst;
+ int err, irq, phy_num, clk = 0;
if (usb_disabled())
return -ENODEV;
@@ -239,22 +238,16 @@ static int ehci_platform_probe(struct platform_device
*dev)
}
}
- for (rst = 0; rst < EHCI_MAX_RSTS; rst++) {
- priv->rsts[rst] = devm_reset_control_get_shared_by_index(
- &dev->dev, rst);
- if (IS_ERR(priv->rsts[rst])) {
- err = PTR_ERR(priv->rsts[rst]);
- if (err == -EPROBE_DEFER)
- goto err_reset;
- priv->rsts[rst] = NULL;
- break;
- }
-
- err = reset_control_deassert(priv->rsts[rst]);
- if (err)
- goto err_reset;
+ priv->rsts = devm_reset_control_array_get_optional_shared(&dev->dev);
+ if (IS_ERR(priv->rsts)) {
+ err = PTR_ERR(priv->rsts);
+ goto err_put_clks;
}
+ err = reset_control_deassert(priv->rsts);
+ if (err)
+ goto err_put_clks;
+
if (pdata->big_endian_desc)
ehci->big_endian_desc = 1;
if (pdata->big_endian_mmio)
@@ -310,8 +303,7 @@ static int ehci_platform_probe(struct platform_device *dev)
if (pdata->power_off)
pdata->power_off(dev);
err_reset:
- while (--rst >= 0)
- reset_control_assert(priv->rsts[rst]);
+ reset_control_assert(priv->rsts);
err_put_clks:
while (--clk >= 0)
clk_put(priv->clks[clk]);
@@ -329,15 +321,14 @@ static int ehci_platform_remove(struct platform_device
*dev)
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
- int clk, rst;
+ int clk;
usb_remove_hcd(hcd);
if (pdata->power_off)
pdata->power_off(dev);
- for (rst = 0; rst < EHCI_MAX_RSTS && priv->rsts[rst]; rst++)
- reset_control_assert(priv->rsts[rst]);
+ reset_control_assert(priv->rsts);
for (clk = 0; clk < EHCI_MAX_CLKS && priv->clks[clk]; clk++)
clk_put(priv->clks[clk]);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] usb: ohci-platform: use reset array API
Generic drivers like this need to control arbitrary number of reset
lines. Instead of hard-coding the maximum number of resets, use the
reset array API. It can manage a bunch of resets behind the scene.
Signed-off-by: Masahiro Yamada
---
Changes in v2:
- Goto err_put_clks when it fails to get reset_control.
drivers/usb/host/ohci-platform.c | 37 +++--
1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 61fe2b9..05ebde6 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -34,12 +34,11 @@
#define DRIVER_DESC "OHCI generic platform driver"
#define OHCI_MAX_CLKS 3
-#define OHCI_MAX_RESETS 2
#define hcd_to_ohci_priv(h) ((struct ohci_platform_priv *)hcd_to_ohci(h)->priv)
struct ohci_platform_priv {
struct clk *clks[OHCI_MAX_CLKS];
- struct reset_control *resets[OHCI_MAX_RESETS];
+ struct reset_control *resets;
struct phy **phys;
int num_phys;
};
@@ -119,7 +118,7 @@ static int ohci_platform_probe(struct platform_device *dev)
struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ohci_platform_priv *priv;
struct ohci_hcd *ohci;
- int err, irq, phy_num, clk = 0, rst = 0;
+ int err, irq, phy_num, clk = 0;
if (usb_disabled())
return -ENODEV;
@@ -204,21 +203,17 @@ static int ohci_platform_probe(struct platform_device
*dev)
break;
}
}
- for (rst = 0; rst < OHCI_MAX_RESETS; rst++) {
- priv->resets[rst] =
- devm_reset_control_get_shared_by_index(
- &dev->dev, rst);
- if (IS_ERR(priv->resets[rst])) {
- err = PTR_ERR(priv->resets[rst]);
- if (err == -EPROBE_DEFER)
- goto err_reset;
- priv->resets[rst] = NULL;
- break;
- }
- err = reset_control_deassert(priv->resets[rst]);
- if (err)
- goto err_reset;
+
+ priv->resets = devm_reset_control_array_get_optional_shared(
+ &dev->dev);
+ if (IS_ERR(priv->resets)) {
+ err = PTR_ERR(priv->resets);
+ goto err_put_clks;
}
+
+ err = reset_control_deassert(priv->resets);
+ if (err)
+ goto err_put_clks;
}
if (pdata->big_endian_desc)
@@ -279,8 +274,7 @@ static int ohci_platform_probe(struct platform_device *dev)
pdata->power_off(dev);
err_reset:
pm_runtime_disable(&dev->dev);
- while (--rst >= 0)
- reset_control_assert(priv->resets[rst]);
+ reset_control_assert(priv->resets);
err_put_clks:
while (--clk >= 0)
clk_put(priv->clks[clk]);
@@ -298,7 +292,7 @@ static int ohci_platform_remove(struct platform_device *dev)
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
- int clk, rst;
+ int clk;
pm_runtime_get_sync(&dev->dev);
usb_remove_hcd(hcd);
@@ -306,8 +300,7 @@ static int ohci_platform_remove(struct platform_device *dev)
if (pdata->power_off)
pdata->power_off(dev);
- for (rst = 0; rst < OHCI_MAX_RESETS && priv->resets[rst]; rst++)
- reset_control_assert(priv->resets[rst]);
+ reset_control_assert(priv->resets);
for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++)
clk_put(priv->clks[clk]);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: ehci-platform: use reset array API
Hi Alan,
2017-11-02 0:32 GMT+09:00 Alan Stern :
> On Mon, 30 Oct 2017, Masahiro Yamada wrote:
>
>> Generic drivers like this need to control arbitrary numbers of reset
>> lines. Instead of hard-coding the maximum number of resets, use the
>> reset array API. It can manage a bunch of resets behind the scene.
>>
>> Signed-off-by: Masahiro Yamada
>> ---
>> drivers/usb/host/ehci-platform.c | 33 +++--
>> 1 file changed, 11 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-platform.c
>> b/drivers/usb/host/ehci-platform.c
>> index f1908ea..64fd643 100644
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -40,12 +40,11 @@
>>
>> #define DRIVER_DESC "EHCI generic platform driver"
>> #define EHCI_MAX_CLKS 4
>> -#define EHCI_MAX_RSTS 4
>> #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv
>> *)hcd_to_ehci(h)->priv)
>>
>> struct ehci_platform_priv {
>> struct clk *clks[EHCI_MAX_CLKS];
>> - struct reset_control *rsts[EHCI_MAX_RSTS];
>> + struct reset_control *rsts;
>> struct phy **phys;
>> int num_phys;
>> bool reset_on_resume;
>> @@ -151,7 +150,7 @@ static int ehci_platform_probe(struct platform_device
>> *dev)
>> struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
>> struct ehci_platform_priv *priv;
>> struct ehci_hcd *ehci;
>> - int err, irq, phy_num, clk = 0, rst;
>> + int err, irq, phy_num, clk = 0;
>>
>> if (usb_disabled())
>> return -ENODEV;
>> @@ -239,21 +238,13 @@ static int ehci_platform_probe(struct platform_device
>> *dev)
>> }
>> }
>>
>> - for (rst = 0; rst < EHCI_MAX_RSTS; rst++) {
>> - priv->rsts[rst] = devm_reset_control_get_shared_by_index(
>> - &dev->dev, rst);
>> - if (IS_ERR(priv->rsts[rst])) {
>> - err = PTR_ERR(priv->rsts[rst]);
>> - if (err == -EPROBE_DEFER)
>> - goto err_reset;
>> - priv->rsts[rst] = NULL;
>> - break;
>> - }
>> + priv->rsts = devm_reset_control_array_get_optional_shared(&dev->dev);
>> + if (IS_ERR(priv->rsts))
>> + return PTR_ERR(priv->rsts);
>
> You must not return. Instead you should set err and jump to
> err_put_clks.
>
Good catch!
Will fix.
Thank you!
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: ohci-platform: use reset array API
Generic drivers like this need to control arbitrary numbers of reset
lines. Instead of hard-coding the maximum number of resets, use the
reset array API. It can manage a bunch of resets behind the scene.
Signed-off-by: Masahiro Yamada
---
drivers/usb/host/ohci-platform.c | 37 ++---
1 file changed, 14 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 61fe2b9..cb7e190 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -34,12 +34,11 @@
#define DRIVER_DESC "OHCI generic platform driver"
#define OHCI_MAX_CLKS 3
-#define OHCI_MAX_RESETS 2
#define hcd_to_ohci_priv(h) ((struct ohci_platform_priv *)hcd_to_ohci(h)->priv)
struct ohci_platform_priv {
struct clk *clks[OHCI_MAX_CLKS];
- struct reset_control *resets[OHCI_MAX_RESETS];
+ struct reset_control *resets;
struct phy **phys;
int num_phys;
};
@@ -119,7 +118,7 @@ static int ohci_platform_probe(struct platform_device *dev)
struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ohci_platform_priv *priv;
struct ohci_hcd *ohci;
- int err, irq, phy_num, clk = 0, rst = 0;
+ int err, irq, phy_num, clk = 0;
if (usb_disabled())
return -ENODEV;
@@ -204,21 +203,15 @@ static int ohci_platform_probe(struct platform_device
*dev)
break;
}
}
- for (rst = 0; rst < OHCI_MAX_RESETS; rst++) {
- priv->resets[rst] =
- devm_reset_control_get_shared_by_index(
- &dev->dev, rst);
- if (IS_ERR(priv->resets[rst])) {
- err = PTR_ERR(priv->resets[rst]);
- if (err == -EPROBE_DEFER)
- goto err_reset;
- priv->resets[rst] = NULL;
- break;
- }
- err = reset_control_deassert(priv->resets[rst]);
- if (err)
- goto err_reset;
- }
+
+ priv->resets = devm_reset_control_array_get_optional_shared(
+ &dev->dev);
+ if (IS_ERR(priv->resets))
+ return PTR_ERR(priv->resets);
+
+ err = reset_control_deassert(priv->resets);
+ if (err)
+ goto err_put_clks;
}
if (pdata->big_endian_desc)
@@ -279,8 +272,7 @@ static int ohci_platform_probe(struct platform_device *dev)
pdata->power_off(dev);
err_reset:
pm_runtime_disable(&dev->dev);
- while (--rst >= 0)
- reset_control_assert(priv->resets[rst]);
+ reset_control_assert(priv->resets);
err_put_clks:
while (--clk >= 0)
clk_put(priv->clks[clk]);
@@ -298,7 +290,7 @@ static int ohci_platform_remove(struct platform_device *dev)
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
- int clk, rst;
+ int clk;
pm_runtime_get_sync(&dev->dev);
usb_remove_hcd(hcd);
@@ -306,8 +298,7 @@ static int ohci_platform_remove(struct platform_device *dev)
if (pdata->power_off)
pdata->power_off(dev);
- for (rst = 0; rst < OHCI_MAX_RESETS && priv->resets[rst]; rst++)
- reset_control_assert(priv->resets[rst]);
+ reset_control_assert(priv->resets);
for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++)
clk_put(priv->clks[clk]);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: ehci-platform: use reset array API
Generic drivers like this need to control arbitrary numbers of reset
lines. Instead of hard-coding the maximum number of resets, use the
reset array API. It can manage a bunch of resets behind the scene.
Signed-off-by: Masahiro Yamada
---
drivers/usb/host/ehci-platform.c | 33 +++--
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index f1908ea..64fd643 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -40,12 +40,11 @@
#define DRIVER_DESC "EHCI generic platform driver"
#define EHCI_MAX_CLKS 4
-#define EHCI_MAX_RSTS 4
#define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
struct ehci_platform_priv {
struct clk *clks[EHCI_MAX_CLKS];
- struct reset_control *rsts[EHCI_MAX_RSTS];
+ struct reset_control *rsts;
struct phy **phys;
int num_phys;
bool reset_on_resume;
@@ -151,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev)
struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ehci_platform_priv *priv;
struct ehci_hcd *ehci;
- int err, irq, phy_num, clk = 0, rst;
+ int err, irq, phy_num, clk = 0;
if (usb_disabled())
return -ENODEV;
@@ -239,21 +238,13 @@ static int ehci_platform_probe(struct platform_device
*dev)
}
}
- for (rst = 0; rst < EHCI_MAX_RSTS; rst++) {
- priv->rsts[rst] = devm_reset_control_get_shared_by_index(
- &dev->dev, rst);
- if (IS_ERR(priv->rsts[rst])) {
- err = PTR_ERR(priv->rsts[rst]);
- if (err == -EPROBE_DEFER)
- goto err_reset;
- priv->rsts[rst] = NULL;
- break;
- }
+ priv->rsts = devm_reset_control_array_get_optional_shared(&dev->dev);
+ if (IS_ERR(priv->rsts))
+ return PTR_ERR(priv->rsts);
- err = reset_control_deassert(priv->rsts[rst]);
- if (err)
- goto err_reset;
- }
+ err = reset_control_deassert(priv->rsts);
+ if (err)
+ goto err_put_clks;
if (pdata->big_endian_desc)
ehci->big_endian_desc = 1;
@@ -310,8 +301,7 @@ static int ehci_platform_probe(struct platform_device *dev)
if (pdata->power_off)
pdata->power_off(dev);
err_reset:
- while (--rst >= 0)
- reset_control_assert(priv->rsts[rst]);
+ reset_control_assert(priv->rsts);
err_put_clks:
while (--clk >= 0)
clk_put(priv->clks[clk]);
@@ -329,15 +319,14 @@ static int ehci_platform_remove(struct platform_device
*dev)
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
- int clk, rst;
+ int clk;
usb_remove_hcd(hcd);
if (pdata->power_off)
pdata->power_off(dev);
- for (rst = 0; rst < EHCI_MAX_RSTS && priv->rsts[rst]; rst++)
- reset_control_assert(priv->rsts[rst]);
+ reset_control_assert(priv->rsts);
for (clk = 0; clk < EHCI_MAX_CLKS && priv->clks[clk]; clk++)
clk_put(priv->clks[clk]);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: remove NULL pointer check for clk_disable_unprepare
2017-05-21 2:52 GMT+09:00 Masahiro Yamada : > After long term efforts of fixing non-common clock implementations, > clk_disable() is a no-op for a NULL pointer input, and this is now > tree-wide consistent. > > All clock consumers can safely call clk_disable(_unprepare) without > NULL pointer check. > > Signed-off-by: Masahiro Yamada Sorry, I retract this patch. Krzysztof pointed out cleanups only for clk_disable_unprepare() will lose the code symmetry. NULL pointer checks for clk_prepare_enable() should be removed to keep the code symmetrical. This is possible for common-clock framework because clk_prepare_enable() is also a no-op for a NULL clk input. But it is not necessarily true for non-common clock implementations. -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: remove NULL pointer check for clk_disable_unprepare
After long term efforts of fixing non-common clock implementations,
clk_disable() is a no-op for a NULL pointer input, and this is now
tree-wide consistent.
All clock consumers can safely call clk_disable(_unprepare) without
NULL pointer check.
Signed-off-by: Masahiro Yamada
---
drivers/usb/dwc2/platform.c | 3 +--
drivers/usb/gadget/udc/fsl_mxc_udc.c | 3 +--
drivers/usb/host/ehci-mxc.c | 8 ++--
drivers/usb/host/ehci-platform.c | 3 +--
drivers/usb/host/ehci-spear.c| 3 +--
drivers/usb/host/ehci-st.c | 3 +--
drivers/usb/host/fsl-mph-dr-of.c | 3 +--
drivers/usb/host/ohci-platform.c | 3 +--
drivers/usb/host/ohci-spear.c| 3 +--
drivers/usb/host/ohci-st.c | 3 +--
drivers/usb/misc/usb3503.c | 9 +++--
11 files changed, 14 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index daf0d37..8c5bf81 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -182,8 +182,7 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg
*hsotg)
if (ret)
return ret;
- if (hsotg->clk)
- clk_disable_unprepare(hsotg->clk);
+ clk_disable_unprepare(hsotg->clk);
ret = regulator_bulk_disable(ARRAY_SIZE(hsotg->supplies),
hsotg->supplies);
diff --git a/drivers/usb/gadget/udc/fsl_mxc_udc.c
b/drivers/usb/gadget/udc/fsl_mxc_udc.c
index f16e149..59dbcf7 100644
--- a/drivers/usb/gadget/udc/fsl_mxc_udc.c
+++ b/drivers/usb/gadget/udc/fsl_mxc_udc.c
@@ -118,8 +118,7 @@ int fsl_udc_clk_finalize(struct platform_device *pdev)
void fsl_udc_clk_release(void)
{
- if (mxc_per_clk)
- clk_disable_unprepare(mxc_per_clk);
+ clk_disable_unprepare(mxc_per_clk);
clk_disable_unprepare(mxc_ahb_clk);
clk_disable_unprepare(mxc_ipg_clk);
}
diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
index c7a9b31..1db5ef9 100644
--- a/drivers/usb/host/ehci-mxc.c
+++ b/drivers/usb/host/ehci-mxc.c
@@ -155,9 +155,7 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev)
if (pdata && pdata->exit)
pdata->exit(pdev);
err_init:
- if (priv->phyclk)
- clk_disable_unprepare(priv->phyclk);
-
+ clk_disable_unprepare(priv->phyclk);
clk_disable_unprepare(priv->ahbclk);
err_clk_ahb:
clk_disable_unprepare(priv->usbclk);
@@ -183,9 +181,7 @@ static int ehci_mxc_drv_remove(struct platform_device *pdev)
clk_disable_unprepare(priv->usbclk);
clk_disable_unprepare(priv->ahbclk);
-
- if (priv->phyclk)
- clk_disable_unprepare(priv->phyclk);
+ clk_disable_unprepare(priv->phyclk);
usb_put_hcd(hcd);
return 0;
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index bc7b9be..2d41447 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -127,8 +127,7 @@ static void ehci_platform_power_off(struct platform_device
*dev)
}
for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
- if (priv->clks[clk])
- clk_disable_unprepare(priv->clks[clk]);
+ clk_disable_unprepare(priv->clks[clk]);
}
static struct hc_driver __read_mostly ehci_platform_hc_driver;
diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
index 1f25c79..57257aa 100644
--- a/drivers/usb/host/ehci-spear.c
+++ b/drivers/usb/host/ehci-spear.c
@@ -138,8 +138,7 @@ static int spear_ehci_hcd_drv_remove(struct platform_device
*pdev)
usb_remove_hcd(hcd);
- if (sehci->clk)
- clk_disable_unprepare(sehci->clk);
+ clk_disable_unprepare(sehci->clk);
usb_put_hcd(hcd);
return 0;
diff --git a/drivers/usb/host/ehci-st.c b/drivers/usb/host/ehci-st.c
index be4a278..f3c433d 100644
--- a/drivers/usb/host/ehci-st.c
+++ b/drivers/usb/host/ehci-st.c
@@ -130,8 +130,7 @@ static void st_ehci_platform_power_off(struct
platform_device *dev)
phy_exit(priv->phy);
for (clk = USB_MAX_CLKS - 1; clk >= 0; clk--)
- if (priv->clks[clk])
- clk_disable_unprepare(priv->clks[clk]);
+ clk_disable_unprepare(priv->clks[clk]);
}
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index e90ddb5..ec5562d 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -325,8 +325,7 @@ static void fsl_usb2_mpc5121_exit(struct platform_device
*pdev)
pdata->regs = NULL;
- if (pdata->clk)
- clk_disable_unprepare(pdata->clk);
+ clk_disable_unprepare(pdata->clk);
}
static struct fsl_usb2_platform_data fsl_usb2_mpc5121_pd = {
diff --git a/drivers/usb/host/o
[PATCH] usb: mtu3: cleanup with list_first_entry_or_null()
The combo of list_empty() and list_first_entry() can be replaced with
list_first_entry_or_null().
Signed-off-by: Masahiro Yamada
---
drivers/usb/mtu3/mtu3.h | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
index aa6fd6a..7b6dc23 100644
--- a/drivers/usb/mtu3/mtu3.h
+++ b/drivers/usb/mtu3/mtu3.h
@@ -356,12 +356,8 @@ static inline struct mtu3_ep *to_mtu3_ep(struct usb_ep *ep)
static inline struct mtu3_request *next_request(struct mtu3_ep *mep)
{
- struct list_head *queue = &mep->req_list;
-
- if (list_empty(queue))
- return NULL;
-
- return list_first_entry(queue, struct mtu3_request, list);
+ return list_first_entry_or_null(&mep->req_list, struct mtu3_request,
+ list);
}
static inline void mtu3_writel(void __iomem *base, u32 offset, u32 data)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] mmc: allow mmc_alloc_host() and tmio_mmc_host_alloc()
2016-11-10 22:35 GMT+09:00 Greg Kroah-Hartman : > On Thu, Nov 10, 2016 at 10:24:21PM +0900, Masahiro Yamada wrote: >> >> sdhci_alloc_host() returns an error pointer when it fails. >> but mmc_alloc_host() cannot. >> >> This series allow to propagate a proper error code >> when host-allocation fails. > > Why? What can we really do about the error except give up? Why does > having a explicit error value make any difference to the caller, they > can't do anything different, right? The error code is shown in the console, like probe of 5a00.sdhc failed with error -12 The proper error code will give a clue why the driver failed to probe. > I suggest just leaving it as-is, it's simple, and you don't have to mess > with PTR_ERR() anywhere. Why? Most of driver just give up probing for any error, but we still do ERR_PTR()/PTR_ERR() here and there. I think this patch is the same pattern. If a function returns NULL on failure, we need to think about "what is the most common failure case". Currently, MMC drivers assume -ENOMEM is the best fit for mmc_alloc_host(), but the assumption is fragile. Already, mmc_alloc_host() calls a function that returns not only -ENOMEM, but also -ENOSPC. In the future, some other failure cases might be added to mmc_alloc_host(). Once we decide the API returns an error pointer, drivers just propagate the return value from the API. This is much more stable implementation. -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] mmc: allow mmc_alloc_host() and tmio_mmc_host_alloc()
sdhci_alloc_host() returns an error pointer when it fails. but mmc_alloc_host() cannot. This series allow to propagate a proper error code when host-allocation fails. Masahiro Yamada (2): mmc: allow mmc_alloc_host() to return proper error code mmc: tmio: allow tmio_mmc_host_alloc() to return proper error code drivers/mmc/core/host.c | 11 ++- drivers/mmc/host/android-goldfish.c | 4 ++-- drivers/mmc/host/atmel-mci.c| 4 ++-- drivers/mmc/host/au1xmmc.c | 4 ++-- drivers/mmc/host/bfin_sdh.c | 4 ++-- drivers/mmc/host/cb710-mmc.c| 4 ++-- drivers/mmc/host/davinci_mmc.c | 4 ++-- drivers/mmc/host/dw_mmc.c | 4 ++-- drivers/mmc/host/jz4740_mmc.c | 4 ++-- drivers/mmc/host/mmc_spi.c | 9 ++--- drivers/mmc/host/mmci.c | 4 ++-- drivers/mmc/host/moxart-mmc.c | 4 ++-- drivers/mmc/host/mtk-sd.c | 4 ++-- drivers/mmc/host/mvsdio.c | 4 ++-- drivers/mmc/host/mxcmmc.c | 4 ++-- drivers/mmc/host/mxs-mmc.c | 4 ++-- drivers/mmc/host/omap.c | 4 ++-- drivers/mmc/host/omap_hsmmc.c | 4 ++-- drivers/mmc/host/pxamci.c | 4 ++-- drivers/mmc/host/rtsx_pci_sdmmc.c | 4 ++-- drivers/mmc/host/rtsx_usb_sdmmc.c | 4 ++-- drivers/mmc/host/s3cmci.c | 4 ++-- drivers/mmc/host/sdhci.c| 4 ++-- drivers/mmc/host/sdricoh_cs.c | 4 ++-- drivers/mmc/host/sh_mmcif.c | 4 ++-- drivers/mmc/host/sh_mobile_sdhi.c | 4 ++-- drivers/mmc/host/sunxi-mmc.c| 4 ++-- drivers/mmc/host/tifm_sd.c | 4 ++-- drivers/mmc/host/tmio_mmc.c | 4 +++- drivers/mmc/host/tmio_mmc_pio.c | 4 ++-- drivers/mmc/host/toshsd.c | 4 ++-- drivers/mmc/host/usdhi6rol0.c | 4 ++-- drivers/mmc/host/ushc.c | 4 ++-- drivers/mmc/host/via-sdmmc.c| 4 ++-- drivers/mmc/host/vub300.c | 4 ++-- drivers/mmc/host/wbsd.c | 4 ++-- drivers/mmc/host/wmt-sdmmc.c| 4 ++-- drivers/staging/greybus/sdio.c | 4 ++-- 38 files changed, 85 insertions(+), 79 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] mmc: allow mmc_alloc_host() to return proper error code
Currently, mmc_alloc_host() returns NULL on error, so its callers
cannot return anything but -ENOMEM when it fails, assuming the most
failure cases are due to memory shortage, but it is not true.
Allow mmc_alloc_host() to return an error pointer, then propagate
the proper error code to its callers.
Signed-off-by: Masahiro Yamada
---
drivers/mmc/core/host.c | 11 ++-
drivers/mmc/host/android-goldfish.c | 4 ++--
drivers/mmc/host/atmel-mci.c| 4 ++--
drivers/mmc/host/au1xmmc.c | 4 ++--
drivers/mmc/host/bfin_sdh.c | 4 ++--
drivers/mmc/host/cb710-mmc.c| 4 ++--
drivers/mmc/host/davinci_mmc.c | 4 ++--
drivers/mmc/host/dw_mmc.c | 4 ++--
drivers/mmc/host/jz4740_mmc.c | 4 ++--
drivers/mmc/host/mmc_spi.c | 9 ++---
drivers/mmc/host/mmci.c | 4 ++--
drivers/mmc/host/moxart-mmc.c | 4 ++--
drivers/mmc/host/mtk-sd.c | 4 ++--
drivers/mmc/host/mvsdio.c | 4 ++--
drivers/mmc/host/mxcmmc.c | 4 ++--
drivers/mmc/host/mxs-mmc.c | 4 ++--
drivers/mmc/host/omap.c | 4 ++--
drivers/mmc/host/omap_hsmmc.c | 4 ++--
drivers/mmc/host/pxamci.c | 4 ++--
drivers/mmc/host/rtsx_pci_sdmmc.c | 4 ++--
drivers/mmc/host/rtsx_usb_sdmmc.c | 4 ++--
drivers/mmc/host/s3cmci.c | 4 ++--
drivers/mmc/host/sdhci.c| 4 ++--
drivers/mmc/host/sdricoh_cs.c | 4 ++--
drivers/mmc/host/sh_mmcif.c | 4 ++--
drivers/mmc/host/sunxi-mmc.c| 4 ++--
drivers/mmc/host/tifm_sd.c | 4 ++--
drivers/mmc/host/tmio_mmc_pio.c | 2 +-
drivers/mmc/host/toshsd.c | 4 ++--
drivers/mmc/host/usdhi6rol0.c | 4 ++--
drivers/mmc/host/ushc.c | 4 ++--
drivers/mmc/host/via-sdmmc.c| 4 ++--
drivers/mmc/host/vub300.c | 4 ++--
drivers/mmc/host/wbsd.c | 4 ++--
drivers/mmc/host/wmt-sdmmc.c| 4 ++--
drivers/staging/greybus/sdio.c | 4 ++--
36 files changed, 79 insertions(+), 75 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 98f25ff..b3e13e0 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -349,7 +349,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
host = kzalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
if (!host)
- return NULL;
+ return ERR_PTR(-ENOMEM);
/* scanning will be enabled when we're ready */
host->rescan_disable = 1;
@@ -357,7 +357,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
again:
if (!ida_pre_get(&mmc_host_ida, GFP_KERNEL)) {
kfree(host);
- return NULL;
+ return ERR_PTR(-ENOMEM);
}
spin_lock(&mmc_host_lock);
@@ -368,7 +368,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
goto again;
} else if (err) {
kfree(host);
- return NULL;
+ return ERR_PTR(err);
}
dev_set_name(&host->class_dev, "mmc%d", host->index);
@@ -379,9 +379,10 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
device_initialize(&host->class_dev);
device_enable_async_suspend(&host->class_dev);
- if (mmc_gpio_alloc(host)) {
+ err = mmc_gpio_alloc(host);
+ if (err < 0) {
put_device(&host->class_dev);
- return NULL;
+ return ERR_PTR(err);
}
spin_lock_init(&host->lock);
diff --git a/drivers/mmc/host/android-goldfish.c
b/drivers/mmc/host/android-goldfish.c
index dca5518..7363663 100644
--- a/drivers/mmc/host/android-goldfish.c
+++ b/drivers/mmc/host/android-goldfish.c
@@ -467,8 +467,8 @@ static int goldfish_mmc_probe(struct platform_device *pdev)
return -ENXIO;
mmc = mmc_alloc_host(sizeof(struct goldfish_mmc_host), &pdev->dev);
- if (mmc == NULL) {
- ret = -ENOMEM;
+ if (IS_ERR(mmc)) {
+ ret = PTR_ERR(mmc);
goto err_alloc_host_failed;
}
diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index 0ad8ef5..d0cb112 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -2296,8 +2296,8 @@ static int atmci_init_slot(struct atmel_mci *host,
struct atmel_mci_slot *slot;
mmc = mmc_alloc_host(sizeof(struct atmel_mci_slot), &host->pdev->dev);
- if (!mmc)
- return -ENOMEM;
+ if (IS_ERR(mmc))
+ return PTR_ERR(mmc);
slot = mmc_priv(mmc);
slot->mmc = mmc;
diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
index ed77fbfa..d97b409 100644
--- a/drivers/mmc/host/au1xmmc.c
+++ b/drivers/mmc/host/au1xmmc.c
@@
Re: [PATCH] usb: renesas_usbhs: simplify list handling
>> Fixes: 6acb95d4e070 ("usb: renesas_usbhs: modify packet queue control
>> method")
This is not a fix, but a clean-up patch.
>> Signed-off-by: Nicholas Mc Guire
>> ---
>> Found by simple coccinelle scanner
>>
>> Compile tested with: multi_v7_defconfig (implies
>> CONFIG_USB_RENESAS_USBHS=m)
>>
>> Patch is against 4.9.0-rc2 (localversion-next is next-20161028)
>
> Thank you for the patch!
> However, such a patch is already merged in the Felipe's usb.git repository
> unfortunately...
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=31faf878bd8c7e2c078a3b75f65efe64f23b0f18
> So, the patch will appear in linux-next repository in the future.
Likewise for
drivers/usb/dwc3/gadget.h
drivers/usb/dwc2/gadget.c
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] usb: trivial cleanups with list_first_entry_or_null()
Hi Felipe, >> >> If this series looks good, can you pick it up please? > > it's in my testing/next branch. Has been there for a while ;-) Good. Thanks for taking care of it! -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] usb: trivial cleanups with list_first_entry_or_null()
Hello Felipe, If this series looks good, can you pick it up please? Thanks, 2016-09-19 1:03 GMT+09:00 Masahiro Yamada : > Replace the chain of list_empty() and list_first_entry() > with list_first_entry_or_null(). > > Changes in v2: > - Split into per-driver patches > > > Masahiro Yamada (3): > usb: dwc2: cleanup with list_first_entry_or_null() > usb: dwc3: cleanup with list_first_entry_or_null() > usb: renesas_usbhs: cleanup with list_first_entry_or_null() > > drivers/usb/dwc2/gadget.c| 6 ++ > drivers/usb/dwc3/gadget.h| 5 + > drivers/usb/renesas_usbhs/fifo.c | 5 + > 3 files changed, 4 insertions(+), 12 deletions(-) > > -- > 1.9.1 > -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: ehci-platform: increase EHCI_MAX_RSTS to 4
Hi Greg, 2016-10-17 21:30 GMT+09:00 Greg Kroah-Hartman : > On Mon, Oct 17, 2016 at 08:11:59PM +0900, Masahiro Yamada wrote: >> Socionext LD11 SoC (arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi) >> needs to handle 4 reset lines for EHCI. > > Why? What makes it different from other EHCI implementations? > > thanks, > > greg k-h This is a generic EHCI driver, but the number of clocks/resets are SoC-specific. The following patch you picked up will remind you something? commit 73577d61799e8d8bb7d69a9acdc54923e5998138 Author: Icenowy Zheng Date: Fri Aug 12 11:06:22 2016 +0800 ehci-platform: add the max clock number to 4 Allwinner A64 EHCI requires 4 clocks to be enabled. Signed-off-by: Icenowy Zheng Acked-by: Alan Stern Signed-off-by: Greg Kroah-Hartman -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: ehci-platform: increase EHCI_MAX_RSTS to 4
Socionext LD11 SoC (arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi)
needs to handle 4 reset lines for EHCI.
Signed-off-by: Masahiro Yamada
---
drivers/usb/host/ehci-platform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 876dca4..a268d9e 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -39,7 +39,7 @@
#define DRIVER_DESC "EHCI generic platform driver"
#define EHCI_MAX_CLKS 4
-#define EHCI_MAX_RSTS 3
+#define EHCI_MAX_RSTS 4
#define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
struct ehci_platform_priv {
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] usb: trivial cleanups with list_first_entry_or_null()
Replace the chain of list_empty() and list_first_entry() with list_first_entry_or_null(). Changes in v2: - Split into per-driver patches Masahiro Yamada (3): usb: dwc2: cleanup with list_first_entry_or_null() usb: dwc3: cleanup with list_first_entry_or_null() usb: renesas_usbhs: cleanup with list_first_entry_or_null() drivers/usb/dwc2/gadget.c| 6 ++ drivers/usb/dwc3/gadget.h| 5 + drivers/usb/renesas_usbhs/fifo.c | 5 + 3 files changed, 4 insertions(+), 12 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] usb: renesas_usbhs: cleanup with list_first_entry_or_null()
The combo of list_empty() check and return list_first_entry()
can be replaced with list_first_entry_or_null().
Signed-off-by: Masahiro Yamada
---
drivers/usb/renesas_usbhs/fifo.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 857e783..d1af831 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -100,10 +100,7 @@ static void __usbhsf_pkt_del(struct usbhs_pkt *pkt)
static struct usbhs_pkt *__usbhsf_pkt_get(struct usbhs_pipe *pipe)
{
- if (list_empty(&pipe->list))
- return NULL;
-
- return list_first_entry(&pipe->list, struct usbhs_pkt, node);
+ return list_first_entry_or_null(&pipe->list, struct usbhs_pkt, node);
}
static void usbhsf_fifo_clear(struct usbhs_pipe *pipe,
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] usb: dwc2: cleanup with list_first_entry_or_null()
The combo of list_empty() check and return list_first_entry()
can be replaced with list_first_entry_or_null().
Signed-off-by: Masahiro Yamada
Acked-by: John Youn
---
drivers/usb/dwc2/gadget.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index af46adf..28a250a 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1098,10 +1098,8 @@ static int dwc2_hsotg_ep_sethalt(struct usb_ep *ep, int
value, bool now);
*/
static struct dwc2_hsotg_req *get_ep_head(struct dwc2_hsotg_ep *hs_ep)
{
- if (list_empty(&hs_ep->queue))
- return NULL;
-
- return list_first_entry(&hs_ep->queue, struct dwc2_hsotg_req, queue);
+ return list_first_entry_or_null(&hs_ep->queue, struct dwc2_hsotg_req,
+ queue);
}
/**
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] usb: dwc3: cleanup with list_first_entry_or_null()
The combo of list_empty() check and return list_first_entry()
can be replaced with list_first_entry_or_null().
Signed-off-by: Masahiro Yamada
---
drivers/usb/dwc3/gadget.h | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index e4a1d97..3129bcf 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -62,10 +62,7 @@ struct dwc3;
static inline struct dwc3_request *next_request(struct list_head *list)
{
- if (list_empty(list))
- return NULL;
-
- return list_first_entry(list, struct dwc3_request, list);
+ return list_first_entry_or_null(list, struct dwc3_request, list);
}
static inline void dwc3_gadget_move_started_request(struct dwc3_request *req)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: cleanup with list_first_entry_or_null()
The combo of list_empty() check and return list_first_entry()
can be replaced with list_first_entry_or_null().
Signed-off-by: Masahiro Yamada
---
drivers/usb/dwc2/gadget.c| 6 ++
drivers/usb/dwc3/gadget.h| 5 +
drivers/usb/renesas_usbhs/fifo.c | 5 +
3 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index af46adf..28a250a 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1098,10 +1098,8 @@ static int dwc2_hsotg_ep_sethalt(struct usb_ep *ep, int
value, bool now);
*/
static struct dwc2_hsotg_req *get_ep_head(struct dwc2_hsotg_ep *hs_ep)
{
- if (list_empty(&hs_ep->queue))
- return NULL;
-
- return list_first_entry(&hs_ep->queue, struct dwc2_hsotg_req, queue);
+ return list_first_entry_or_null(&hs_ep->queue, struct dwc2_hsotg_req,
+ queue);
}
/**
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index e4a1d97..3129bcf 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -62,10 +62,7 @@ struct dwc3;
static inline struct dwc3_request *next_request(struct list_head *list)
{
- if (list_empty(list))
- return NULL;
-
- return list_first_entry(list, struct dwc3_request, list);
+ return list_first_entry_or_null(list, struct dwc3_request, list);
}
static inline void dwc3_gadget_move_started_request(struct dwc3_request *req)
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 857e783..d1af831 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -100,10 +100,7 @@ static void __usbhsf_pkt_del(struct usbhs_pkt *pkt)
static struct usbhs_pkt *__usbhsf_pkt_get(struct usbhs_pipe *pipe)
{
- if (list_empty(&pipe->list))
- return NULL;
-
- return list_first_entry(&pipe->list, struct usbhs_pkt, node);
+ return list_first_entry_or_null(&pipe->list, struct usbhs_pkt, node);
}
static void usbhsf_fifo_clear(struct usbhs_pipe *pipe,
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: remove redundant dependency on USB_SUPPORT
2016-08-16 16:29 GMT+09:00 Felipe Balbi : > > Hi, > > Masahiro Yamada writes: >> The whole Kconfig entries of the USB subsystem are surrounded with >> "if USB_SUPPORT" ... "endif", so CONFIG_USB_SUPPORT=y is surely met >> when these two Kconfig options are visible. >> >> Signed-off-by: Masahiro Yamada >> --- >> >> drivers/usb/core/Kconfig | 1 - >> drivers/usb/dwc3/Kconfig | 2 +- >> 2 files changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig >> index dd28010..253aac7 100644 >> --- a/drivers/usb/core/Kconfig >> +++ b/drivers/usb/core/Kconfig >> @@ -85,7 +85,6 @@ config USB_OTG_FSM >> >> config USB_ULPI_BUS >> tristate "USB ULPI PHY interface support" >> - depends on USB_SUPPORT >> help >> UTMI+ Low Pin Interface (ULPI) is specification for a commonly used >> USB 2.0 PHY interface. The ULPI specification defines a standard set >> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >> index 11864e4..3c6213c 100644 >> --- a/drivers/usb/dwc3/Kconfig >> +++ b/drivers/usb/dwc3/Kconfig >> @@ -1,7 +1,7 @@ >> config USB_DWC3 >> tristate "DesignWare USB3 DRD Core Support" >> depends on (USB || USB_GADGET) && HAS_DMA >> - select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD >> + select USB_XHCI_PLATFORM if USB_XHCI_HCD >> help >> Say Y or M here if your system has a Dual Role SuperSpeed >> USB controller based on the DesignWare USB3 IP Core. > > let's split this patch in two so different maintainers can pick the > relevant pieces. Thanks Greg picked up this patch already. I see it in linux-next now. -- Best Regards Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: remove redundant dependency on USB_SUPPORT
The whole Kconfig entries of the USB subsystem are surrounded with "if USB_SUPPORT" ... "endif", so CONFIG_USB_SUPPORT=y is surely met when these two Kconfig options are visible. Signed-off-by: Masahiro Yamada --- drivers/usb/core/Kconfig | 1 - drivers/usb/dwc3/Kconfig | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig index dd28010..253aac7 100644 --- a/drivers/usb/core/Kconfig +++ b/drivers/usb/core/Kconfig @@ -85,7 +85,6 @@ config USB_OTG_FSM config USB_ULPI_BUS tristate "USB ULPI PHY interface support" - depends on USB_SUPPORT help UTMI+ Low Pin Interface (ULPI) is specification for a commonly used USB 2.0 PHY interface. The ULPI specification defines a standard set diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 11864e4..3c6213c 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -1,7 +1,7 @@ config USB_DWC3 tristate "DesignWare USB3 DRD Core Support" depends on (USB || USB_GADGET) && HAS_DMA - select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD + select USB_XHCI_PLATFORM if USB_XHCI_HCD help Say Y or M here if your system has a Dual Role SuperSpeed USB controller based on the DesignWare USB3 IP Core. -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: ohci-platform: switch over to shared reset
The recent update in the reset subsystem requires all reset consumers
to be explicit when requesting reset lines. For detail, see the log
of commit 3c35f6edc09b ("reset: Reorder inline reset_control_get*()
wrappers").
The devm_reset_control_get_optional() is deprecated, and falls into
the _exclusive variant during the migration, but the reset control
in this driver is apparently shared-tolerate. Besides, this driver
is for generic platforms, so actually should be able to work with a
shared reset line.
Signed-off-by: Masahiro Yamada
---
drivers/usb/host/ohci-platform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index ae1c988..57ee42d 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -198,7 +198,7 @@ static int ohci_platform_probe(struct platform_device *dev)
}
- priv->rst = devm_reset_control_get_optional(&dev->dev, NULL);
+ priv->rst = devm_reset_control_get_optional_shared(&dev->dev, NULL);
if (IS_ERR(priv->rst)) {
err = PTR_ERR(priv->rst);
if (err == -EPROBE_DEFER)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: ehci-platform: switch over to shared reset
The recent update in the reset subsystem requires all reset consumers
to be explicit when requesting reset lines. For detail, see the log
of commit 3c35f6edc09b ("reset: Reorder inline reset_control_get*()
wrappers").
The devm_reset_control_get_optional() is deprecated, and falls into
the _exclusive variant during the migration, but the reset control
in this driver is apparently shared-tolerate. Besides, this driver
is for generic platforms, so actually should be able to work with a
shared reset line.
Signed-off-by: Masahiro Yamada
---
drivers/usb/host/ehci-platform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 1757ebb..2f5a16c 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -234,7 +234,7 @@ static int ehci_platform_probe(struct platform_device *dev)
}
}
- priv->rst = devm_reset_control_get_optional(&dev->dev, NULL);
+ priv->rst = devm_reset_control_get_optional_shared(&dev->dev, NULL);
if (IS_ERR(priv->rst)) {
err = PTR_ERR(priv->rst);
if (err == -EPROBE_DEFER)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
