Re: Lack of length checking in USB configuration may allow buffer overflow
On Mon, May 13, 2019 at 07:14:38PM -0700, Rick Mark wrote:
> Hey All,
>
> I was seeing a linux VM crash due to malformed USB configuration
> payloads being malformed. I'm testing this patch now which should
> provide better security checking (but this is my first patch so be
> kind if I have things wrong.)
>
> R
>
> >From d7b0dd52f3b3b38126504b17d2d9c9ceaa572edf Mon Sep 17 00:00:00 2001
> From: Rick Mark
> Date: Mon, 13 May 2019 19:06:46 -0700
> Subject: [PATCH] Security checks in USB configurations
>
> ---
> drivers/usb/core/config.c | 67
> +++
> 1 file changed, 67 insertions(+)
>
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 7b5cb28ff..8cb9a136e 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -33,6 +33,13 @@ static int find_next_descriptor(unsigned char
> *buffer, int size,
Thanks for the patch, but your email client ate all of the tabs and
line-wrapped it, making it impossible to apply, even if we wanted to :)
Also, I need a lot better changelog text and description, as well as a
signed-off-by line. Documentation/SubmittingPatches should explain all
of how to do this. If you have questions after reading this, please let
me know.
That being said, I don't think all of your patch is really needed:
>
> /* Find the next descriptor of type dt1 or dt2 */
> while (size > 0) {
> + if (size < sizeof(struct usb_descriptor_header)) {
> + printk( KERN_ERR "usb config: find_next_descriptor "
> + "with size %d not sizeof("
> + "struct usb_descriptor_header)", size );
> + break;
> + }
How can size be smaller than this, I think we check this value before
calling this function in all places. Did we miss one?
> +
> h = (struct usb_descriptor_header *) buffer;
> if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
> break;
> @@ -58,6 +65,13 @@ static void
> usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
> * The SuperSpeedPlus Isoc endpoint companion descriptor immediately
> * follows the SuperSpeed Endpoint Companion descriptor
> */
> + if (size < sizeof(struct usb_ssp_isoc_ep_comp_descriptor)) {
> +dev_warn(ddev, "Invalid size %d for SuperSpeedPlus isoc
> endpoint companion"
> + "for config %d interface %d altsetting %d ep %d.\n",
> + size, cfgno, inum, asnum, ep->desc.bEndpointAddress);
> +return;
> + }
A patch was just sent to the list to resolve a problem in this function
yesterday, can you verify that it resolves your issue as well?
> +
> desc = (struct usb_ssp_isoc_ep_comp_descriptor *) buffer;
> if (desc->bDescriptorType != USB_DT_SSP_ISOC_ENDPOINT_COMP ||
> size < USB_DT_SSP_ISOC_EP_COMP_SIZE) {
> @@ -76,6 +90,14 @@ static void usb_parse_ss_endpoint_companion(struct
> device *ddev, int cfgno,
> struct usb_ss_ep_comp_descriptor *desc;
> int max_tx;
>
> + if (size < sizeof(struct usb_ss_ep_comp_descriptor)) {
> +dev_warn(ddev, "Invalid size %d of SuperSpeed endpoint"
> + " companion for config %d "
> + " interface %d altsetting %d: "
> + "using minimum values\n",
> + size, cfgno, inum, asnum);
> +return;
> + }
> /* The SuperSpeed endpoint companion descriptor is supposed to
> * be the first thing immediately following the endpoint descriptor.
> */
We do this same check the line below this, why do it twice?
> @@ -103,6 +125,16 @@ static void
> usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
> ep->desc.wMaxPacketSize;
> return;
> }
> +
> + if ((size - desc->bLength) < 0 ||
> + size < USB_DT_SS_EP_COMP_SIZE) {
> +dev_warn(ddev, "Control endpoint with bMaxBurst = %d in "
> + "config %d interface %d altsetting %d ep %d: "
> + "has invalid bLength %d vs size %d\n",
> desc->bMaxBurst,
> + cfgno, inum, asnum, ep->desc.bEndpointAddress,
> desc->bLength, size);
> +return;
> + }
> +
Didn't we just check this above here successfully?
I didn't go through all of your others, but please be sure that we are
not already doing all of this, as I think we are.
Are you sure you are using the latest kernel version?
thanks,
greg k-h
[PATCH v2 6/8] ARM: dts: imx7ulp: add imx7ulp USBOTG1 support
Add imx7ulp USBOTG1 support.
Signed-off-by: Peter Chen
---
arch/arm/boot/dts/imx7ulp.dtsi | 31 +++
1 file changed, 31 insertions(+)
diff --git a/arch/arm/boot/dts/imx7ulp.dtsi b/arch/arm/boot/dts/imx7ulp.dtsi
index fca6e50f37c8..60c9ea116d0a 100644
--- a/arch/arm/boot/dts/imx7ulp.dtsi
+++ b/arch/arm/boot/dts/imx7ulp.dtsi
@@ -30,6 +30,7 @@
serial1 = &lpuart5;
serial2 = &lpuart6;
serial3 = &lpuart7;
+ usbphy0 = &usbphy1;
};
cpus {
@@ -133,6 +134,36 @@
clock-names = "ipg", "per";
};
+ usbotg1: usb@4033 {
+ compatible = "fsl,imx7ulp-usb", "fsl,imx6ul-usb",
+ "fsl,imx27-usb";
+ reg = <0x4033 0x200>;
+ interrupts = ;
+ clocks = <&pcc2 IMX7ULP_CLK_USB0>;
+ phys = <&usbphy1>;
+ fsl,usbmisc = <&usbmisc1 0>;
+ ahb-burst-config = <0x0>;
+ tx-burst-size-dword = <0x8>;
+ rx-burst-size-dword = <0x8>;
+ status = "disabled";
+ };
+
+ usbmisc1: usbmisc@40330200 {
+ #index-cells = <1>;
+ compatible = "fsl,imx7ulp-usbmisc", "fsl,imx7d-usbmisc",
+ "fsl,imx6q-usbmisc";
+ reg = <0x40330200 0x200>;
+ };
+
+ usbphy1: usbphy@0x4035 {
+ compatible = "fsl,imx7ulp-usbphy",
+ "fsl,imx6ul-usbphy", "fsl,imx23-usbphy";
+ reg = <0x4035 0x1000>;
+ interrupts = ;
+ clocks = <&pcc2 IMX7ULP_CLK_USB_PHY>;
+ #phy-cells = <0>;
+ };
+
usdhc0: mmc@4037 {
compatible = "fsl,imx7ulp-usdhc", "fsl,imx6sx-usdhc";
reg = <0x4037 0x1>;
--
2.14.1
[PATCH v2 4/8] doc: dt-binding: usbmisc-imx: add compatible string for imx7ulp
Add compatible string for imx7ulp Reviewed-by: Rob Herring Signed-off-by: Peter Chen --- Documentation/devicetree/bindings/usb/usbmisc-imx.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt index a85a631ec434..b353b9816487 100644 --- a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt @@ -7,6 +7,7 @@ Required properties: "fsl,vf610-usbmisc" for Vybrid vf610 "fsl,imx6sx-usbmisc" for imx6sx "fsl,imx7d-usbmisc" for imx7d + "fsl,imx7ulp-usbmisc" for imx7ulp - reg: Should contain registers location and length Examples: -- 2.14.1
[PATCH v2 5/8] usb: chipidea: imx: add imx7ulp support
Add imx7ulp support
Signed-off-by: Peter Chen
---
drivers/usb/chipidea/ci_hdrc_imx.c | 28 +++-
drivers/usb/chipidea/usbmisc_imx.c | 4
include/linux/usb/chipidea.h | 1 +
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index ceec8d5985d4..a76708501236 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include "ci.h"
#include "ci_hdrc_imx.h"
@@ -63,6 +64,11 @@ static const struct ci_hdrc_imx_platform_flag imx7d_usb_data
= {
.flags = CI_HDRC_SUPPORTS_RUNTIME_PM,
};
+static const struct ci_hdrc_imx_platform_flag imx7ulp_usb_data = {
+ .flags = CI_HDRC_SUPPORTS_RUNTIME_PM |
+ CI_HDRC_PMQOS,
+};
+
static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
{ .compatible = "fsl,imx23-usb", .data = &imx23_usb_data},
{ .compatible = "fsl,imx28-usb", .data = &imx28_usb_data},
@@ -72,6 +78,7 @@ static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
{ .compatible = "fsl,imx6sx-usb", .data = &imx6sx_usb_data},
{ .compatible = "fsl,imx6ul-usb", .data = &imx6ul_usb_data},
{ .compatible = "fsl,imx7d-usb", .data = &imx7d_usb_data},
+ { .compatible = "fsl,imx7ulp-usb", .data = &imx7ulp_usb_data},
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, ci_hdrc_imx_dt_ids);
@@ -93,6 +100,8 @@ struct ci_hdrc_imx_data {
struct clk *clk_ahb;
struct clk *clk_per;
/* - */
+ struct pm_qos_request pm_qos_req;
+ const struct ci_hdrc_imx_platform_flag *plat_data;
};
/* Common functions shared by usbmisc drivers */
@@ -309,6 +318,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
if (!data)
return -ENOMEM;
+ data->plat_data = imx_platform_flag;
+ pdata.flags |= imx_platform_flag->flags;
platform_set_drvdata(pdev, data);
data->usbmisc_data = usbmisc_get_init_data(dev);
if (IS_ERR(data->usbmisc_data))
@@ -369,6 +380,11 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
}
}
}
+
+ if (pdata.flags & CI_HDRC_PMQOS)
+ pm_qos_add_request(&data->pm_qos_req,
+ PM_QOS_CPU_DMA_LATENCY, 0);
+
ret = imx_get_clks(dev);
if (ret)
goto disable_hsic_regulator;
@@ -396,7 +412,6 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
usb_phy_init(pdata.usb_phy);
}
- pdata.flags |= imx_platform_flag->flags;
if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
data->supports_runtime_pm = true;
@@ -439,6 +454,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
disable_hsic_regulator:
if (data->hsic_pad_regulator)
ret = regulator_disable(data->hsic_pad_regulator);
+ if (pdata.flags & CI_HDRC_PMQOS)
+ pm_qos_remove_request(&data->pm_qos_req);
return ret;
}
@@ -455,6 +472,8 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
if (data->override_phy_control)
usb_phy_shutdown(data->phy);
imx_disable_unprepare_clks(&pdev->dev);
+ if (data->plat_data->flags & CI_HDRC_PMQOS)
+ pm_qos_remove_request(&data->pm_qos_req);
if (data->hsic_pad_regulator)
regulator_disable(data->hsic_pad_regulator);
@@ -480,6 +499,9 @@ static int __maybe_unused imx_controller_suspend(struct
device *dev)
}
imx_disable_unprepare_clks(dev);
+ if (data->plat_data->flags & CI_HDRC_PMQOS)
+ pm_qos_remove_request(&data->pm_qos_req);
+
data->in_lpm = true;
return 0;
@@ -497,6 +519,10 @@ static int __maybe_unused imx_controller_resume(struct
device *dev)
return 0;
}
+ if (data->plat_data->flags & CI_HDRC_PMQOS)
+ pm_qos_add_request(&data->pm_qos_req,
+ PM_QOS_CPU_DMA_LATENCY, 0);
+
ret = imx_prepare_enable_clks(dev);
if (ret)
return ret;
diff --git a/drivers/usb/chipidea/usbmisc_imx.c
b/drivers/usb/chipidea/usbmisc_imx.c
index d8b67e150b12..b7a5727d0c8a 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -763,6 +763,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = {
.compatible = "fsl,imx7d-usbmisc",
.data = &imx7d_usbmisc_ops,
},
+ {
+ .compatible = "fsl,imx7ulp-usbmisc",
+ .data = &imx7d_usbmisc_ops,
+ },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 911e05af671e..edd89b7c8f18 100644
--- a/include/linux/usb/chi
[PATCH v2 3/8] doc: dt-binding: ci-hdrc-usb2: add compatible string for imx7ulp
Add compatible string for imx7ulp. Reviewed-by: Rob Herring Signed-off-by: Peter Chen --- Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt index a254386a91ad..cfc9f40ab641 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt @@ -10,6 +10,7 @@ Required properties: "fsl,imx6sx-usb" "fsl,imx6ul-usb" "fsl,imx7d-usb" + "fsl,imx7ulp-usb" "lsi,zevio-usb" "qcom,ci-hdrc" "chipidea,usb2" -- 2.14.1
[PATCH v2 7/8] ARM: dts: imx7ulp-evk: enable USBOTG1 support
Enable USBOTG1 support for evk board, it is dual-role function
port.
Signed-off-by: Peter Chen
---
arch/arm/boot/dts/imx7ulp-evk.dts | 35 +++
1 file changed, 35 insertions(+)
diff --git a/arch/arm/boot/dts/imx7ulp-evk.dts
b/arch/arm/boot/dts/imx7ulp-evk.dts
index a09026a6d22e..c8a56a2ae9a5 100644
--- a/arch/arm/boot/dts/imx7ulp-evk.dts
+++ b/arch/arm/boot/dts/imx7ulp-evk.dts
@@ -22,6 +22,17 @@
reg = <0x6000 0x4000>;
};
+ reg_usb_otg1_vbus: regulator-usb-otg1-vbus {
+ compatible = "regulator-fixed";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usbotg1_vbus>;
+ regulator-name = "usb_otg1_vbus";
+ regulator-min-microvolt = <500>;
+ regulator-max-microvolt = <500>;
+ gpio = <&gpio_ptc 0 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+
reg_vsd_3v3: regulator-vsd-3v3 {
compatible = "regulator-fixed";
regulator-name = "VSD_3V3";
@@ -40,6 +51,17 @@
status = "okay";
};
+&usbotg1 {
+ vbus-supply = <®_usb_otg1_vbus>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usbotg1_id>;
+ srp-disable;
+ hnp-disable;
+ adp-disable;
+ over-current-active-low;
+ status = "okay";
+};
+
&usdhc0 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_usdhc0>;
@@ -57,6 +79,19 @@
bias-pull-up;
};
+ pinctrl_usbotg1_vbus: otg1vbusgrp {
+ fsl,pins = <
+ IMX7ULP_PAD_PTC0__PTC0 0x2
+ >;
+ };
+
+ pinctrl_usbotg1_id: otg1idgrp {
+ fsl,pins = <
+ IMX7ULP_PAD_PTC13__USB0_ID 0x10003
+ IMX7ULP_PAD_PTC16__USB1_OC2 0x10003
+ >;
+ };
+
pinctrl_usdhc0: usdhc0grp {
fsl,pins = <
IMX7ULP_PAD_PTD1__SDHC0_CMD 0x43
--
2.14.1
[PATCH v2 8/8] usb: chipidea: imx: "fsl,usbphy" phandle is not mandatory now
Since the chipidea common code support get the USB PHY phandle from "phys", the glue layer is not mandatory to get the "fsl,usbphy" phandle any more. Signed-off-by: Peter Chen --- drivers/usb/chipidea/ci_hdrc_imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c index a76708501236..b5abfe89190c 100644 --- a/drivers/usb/chipidea/ci_hdrc_imx.c +++ b/drivers/usb/chipidea/ci_hdrc_imx.c @@ -398,8 +398,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev) ret = PTR_ERR(data->phy); /* Return -EINVAL if no usbphy is available */ if (ret == -ENODEV) - ret = -EINVAL; - goto err_clk; + data->phy = NULL; + else + goto err_clk; } pdata.usb_phy = data->phy; -- 2.14.1
[PATCH v2 1/8] doc: dt-binding: mxs-usb-phy: add compatible for 7ulp
Add compatible for 7ulp USB PHY. Reviewed-by: Rob Herring Signed-off-by: Peter Chen --- Documentation/devicetree/bindings/phy/mxs-usb-phy.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/phy/mxs-usb-phy.txt b/Documentation/devicetree/bindings/phy/mxs-usb-phy.txt index 6ac98b3b5f57..32da8d17759a 100644 --- a/Documentation/devicetree/bindings/phy/mxs-usb-phy.txt +++ b/Documentation/devicetree/bindings/phy/mxs-usb-phy.txt @@ -7,6 +7,7 @@ Required properties: * "fsl,imx6sl-usbphy" for imx6sl * "fsl,vf610-usbphy" for Vybrid vf610 * "fsl,imx6sx-usbphy" for imx6sx + * "fsl,imx7ulp-usbphy" for imx7ulp "fsl,imx23-usbphy" is still a fallback for other strings - reg: Should contain registers location and length - interrupts: Should contain phy interrupt -- 2.14.1
[PATCH v2 2/8] usb: phy: phy-mxs-usb: add imx7ulp support
At imx7ulp, the USB related analog register is located in PHY register
region too, so we need to control PLL at PHY driver directly.
Signed-off-by: Peter Chen
---
drivers/usb/phy/phy-mxs-usb.c | 76 ++-
1 file changed, 75 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
index 1b1bb0ad40c3..90c96a8e9342 100644
--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -20,6 +20,7 @@
#define DRIVER_NAME "mxs_phy"
+/* Register Macro */
#define HW_USBPHY_PWD 0x00
#define HW_USBPHY_TX 0x10
#define HW_USBPHY_CTRL 0x30
@@ -37,6 +38,11 @@
#define GM_USBPHY_TX_TXCAL45DN(x)(((x) & 0xf) << 8)
#define GM_USBPHY_TX_D_CAL(x)(((x) & 0xf) << 0)
+/* imx7ulp */
+#define HW_USBPHY_PLL_SIC 0xa0
+#define HW_USBPHY_PLL_SIC_SET 0xa4
+#define HW_USBPHY_PLL_SIC_CLR 0xa8
+
#define BM_USBPHY_CTRL_SFTRST BIT(31)
#define BM_USBPHY_CTRL_CLKGATE BIT(30)
#define BM_USBPHY_CTRL_OTG_ID_VALUEBIT(27)
@@ -55,6 +61,12 @@
#define BM_USBPHY_IP_FIX (BIT(17) | BIT(18))
#define BM_USBPHY_DEBUG_CLKGATEBIT(30)
+/* imx7ulp */
+#define BM_USBPHY_PLL_LOCK BIT(31)
+#define BM_USBPHY_PLL_REG_ENABLE BIT(21)
+#define BM_USBPHY_PLL_BYPASS BIT(16)
+#define BM_USBPHY_PLL_POWERBIT(12)
+#define BM_USBPHY_PLL_EN_USB_CLKS BIT(6)
/* Anatop Registers */
#define ANADIG_ANA_MISC0 0x150
@@ -167,6 +179,9 @@ static const struct mxs_phy_data imx6ul_phy_data = {
.flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
};
+static const struct mxs_phy_data imx7ulp_phy_data = {
+};
+
static const struct of_device_id mxs_phy_dt_ids[] = {
{ .compatible = "fsl,imx6sx-usbphy", .data = &imx6sx_phy_data, },
{ .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_phy_data, },
@@ -174,6 +189,7 @@ static const struct of_device_id mxs_phy_dt_ids[] = {
{ .compatible = "fsl,imx23-usbphy", .data = &imx23_phy_data, },
{ .compatible = "fsl,vf610-usbphy", .data = &vf610_phy_data, },
{ .compatible = "fsl,imx6ul-usbphy", .data = &imx6ul_phy_data, },
+ { .compatible = "fsl,imx7ulp-usbphy", .data = &imx7ulp_phy_data, },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
@@ -198,6 +214,11 @@ static inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy)
return mxs_phy->data == &imx6sl_phy_data;
}
+static inline bool is_imx7ulp_phy(struct mxs_phy *mxs_phy)
+{
+ return mxs_phy->data == &imx7ulp_phy_data;
+}
+
/*
* PHY needs some 32K cycles to switch from 32K clock to
* bus (such as AHB/AXI, etc) clock.
@@ -221,14 +242,59 @@ static void mxs_phy_tx_init(struct mxs_phy *mxs_phy)
}
}
+static int wait_for_pll_lock(const void __iomem *base)
+{
+ int loop_count = 100;
+
+ /* Wait for PLL to lock */
+ do {
+ if (readl(base + HW_USBPHY_PLL_SIC) & BM_USBPHY_PLL_LOCK)
+ break;
+ usleep_range(100, 150);
+ } while (loop_count-- > 0);
+
+ return readl(base + HW_USBPHY_PLL_SIC) & BM_USBPHY_PLL_LOCK
+ ? 0 : -ETIMEDOUT;
+}
+
+static int mxs_phy_pll_enable(void __iomem *base, bool enable)
+{
+ int ret = 0;
+
+ if (enable) {
+ writel(BM_USBPHY_PLL_REG_ENABLE, base + HW_USBPHY_PLL_SIC_SET);
+ writel(BM_USBPHY_PLL_BYPASS, base + HW_USBPHY_PLL_SIC_CLR);
+ writel(BM_USBPHY_PLL_POWER, base + HW_USBPHY_PLL_SIC_SET);
+ ret = wait_for_pll_lock(base);
+ if (ret)
+ return ret;
+ writel(BM_USBPHY_PLL_EN_USB_CLKS, base +
+ HW_USBPHY_PLL_SIC_SET);
+ } else {
+ writel(BM_USBPHY_PLL_EN_USB_CLKS, base +
+ HW_USBPHY_PLL_SIC_CLR);
+ writel(BM_USBPHY_PLL_POWER, base + HW_USBPHY_PLL_SIC_CLR);
+ writel(BM_USBPHY_PLL_BYPASS, base + HW_USBPHY_PLL_SIC_SET);
+ writel(BM_USBPHY_PLL_REG_ENABLE, base + HW_USBPHY_PLL_SIC_CLR);
+ }
+
+ return ret;
+}
+
static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
{
int ret;
void __iomem *base = mxs_phy->phy.io_priv;
+ if (is_imx7ulp_phy(mxs_phy)) {
+ ret = mxs_phy_pll_enable(base, true);
+ if (ret)
+ return ret;
+ }
+
ret = stmp_reset_block(base + HW_USBPHY_CTRL);
if (ret)
- return ret;
+ goto disable_pll;
/* Power up the PHY */
writel(0, base + HW_USBPHY_PWD);
@@ -253,6 +319,11 @@ static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
[PATCH v2 0/8] Add imx7ulp USBOTG1 support
Changes for v2: - Use common 'phys' property [Patch 6/8] - Add the last patch that "fsl,usbphy" phandle is not mandatory now [Patch 8/8] - Add Reviewed-by from Rob. There is a dual-role USB controller at imx7ulp, we add support for it in this patch set, and the dual-role function is tested at imx7ulp-evk board. Thanks. Peter Chen (8): doc: dt-binding: mxs-usb-phy: add compatible for 7ulp usb: phy: phy-mxs-usb: add imx7ulp support doc: dt-binding: ci-hdrc-usb2: add compatible string for imx7ulp doc: dt-binding: usbmisc-imx: add compatible string for imx7ulp usb: chipidea: imx: add imx7ulp support ARM: dts: imx7ulp: add imx7ulp USBOTG1 support ARM: dts: imx7ulp-evk: enable USBOTG1 support usb: chipidea: imx: "fsl,usbphy" phandle is not mandatory now .../devicetree/bindings/phy/mxs-usb-phy.txt| 1 + .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 1 + .../devicetree/bindings/usb/usbmisc-imx.txt| 1 + arch/arm/boot/dts/imx7ulp-evk.dts | 35 ++ arch/arm/boot/dts/imx7ulp.dtsi | 31 + drivers/usb/chipidea/ci_hdrc_imx.c | 33 +- drivers/usb/chipidea/usbmisc_imx.c | 4 ++ drivers/usb/phy/phy-mxs-usb.c | 76 +- include/linux/usb/chipidea.h | 1 + 9 files changed, 179 insertions(+), 4 deletions(-) -- 2.14.1
Re: [PATCH v2 2/8] usb: phy: phy-mxs-usb: add imx7ulp support
On Tue, 2019-05-14 at 07:38 +, Peter Chen wrote:
> At imx7ulp, the USB related analog register is located in PHY register
> region too, so we need to control PLL at PHY driver directly.
>
> Signed-off-by: Peter Chen
> ---
> drivers/usb/phy/phy-mxs-usb.c | 76
> ++-
> 1 file changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> index 1b1bb0ad40c3..90c96a8e9342 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -20,6 +20,7 @@
>
> #define DRIVER_NAME "mxs_phy"
>
> +/* Register Macro */
> #define HW_USBPHY_PWD0x00
> #define HW_USBPHY_TX 0x10
> #define HW_USBPHY_CTRL 0x30
> @@ -37,6 +38,11 @@
> #define GM_USBPHY_TX_TXCAL45DN(x)(((x) & 0xf) << 8)
> #define GM_USBPHY_TX_D_CAL(x)(((x) & 0xf) << 0)
>
> +/* imx7ulp */
> +#define HW_USBPHY_PLL_SIC0xa0
> +#define HW_USBPHY_PLL_SIC_SET0xa4
> +#define HW_USBPHY_PLL_SIC_CLR0xa8
> +
> #define BM_USBPHY_CTRL_SFTRSTBIT(31)
> #define BM_USBPHY_CTRL_CLKGATE BIT(30)
> #define BM_USBPHY_CTRL_OTG_ID_VALUE BIT(27)
> @@ -55,6 +61,12 @@
> #define BM_USBPHY_IP_FIX (BIT(17) | BIT(18))
>
> #define BM_USBPHY_DEBUG_CLKGATE BIT(30)
> +/* imx7ulp */
> +#define BM_USBPHY_PLL_LOCK BIT(31)
> +#define BM_USBPHY_PLL_REG_ENABLE BIT(21)
> +#define BM_USBPHY_PLL_BYPASS BIT(16)
> +#define BM_USBPHY_PLL_POWER BIT(12)
> +#define BM_USBPHY_PLL_EN_USB_CLKSBIT(6)
>
> /* Anatop Registers */
> #define ANADIG_ANA_MISC0 0x150
> @@ -167,6 +179,9 @@ static const struct mxs_phy_data imx6ul_phy_data = {
> .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> };
>
> +static const struct mxs_phy_data imx7ulp_phy_data = {
> +};
> +
> static const struct of_device_id mxs_phy_dt_ids[] = {
> { .compatible = "fsl,imx6sx-usbphy", .data = &imx6sx_phy_data, },
> { .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_phy_data, },
> @@ -174,6 +189,7 @@ static const struct of_device_id mxs_phy_dt_ids[] = {
> { .compatible = "fsl,imx23-usbphy", .data = &imx23_phy_data, },
> { .compatible = "fsl,vf610-usbphy", .data = &vf610_phy_data, },
> { .compatible = "fsl,imx6ul-usbphy", .data = &imx6ul_phy_data, },
> + { .compatible = "fsl,imx7ulp-usbphy", .data = &imx7ulp_phy_data, },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
> @@ -198,6 +214,11 @@ static inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy)
> return mxs_phy->data == &imx6sl_phy_data;
> }
>
> +static inline bool is_imx7ulp_phy(struct mxs_phy *mxs_phy)
> +{
> + return mxs_phy->data == &imx7ulp_phy_data;
> +}
> +
> /*
> * PHY needs some 32K cycles to switch from 32K clock to
> * bus (such as AHB/AXI, etc) clock.
> @@ -221,14 +242,59 @@ static void mxs_phy_tx_init(struct mxs_phy *mxs_phy)
> }
> }
>
> +static int wait_for_pll_lock(const void __iomem *base)
> +{
> + int loop_count = 100;
> +
> + /* Wait for PLL to lock */
> + do {
> + if (readl(base + HW_USBPHY_PLL_SIC) & BM_USBPHY_PLL_LOCK)
> + break;
> + usleep_range(100, 150);
> + } while (loop_count-- > 0);
> +
there is a common API readl_poll_timeout(), maybe you can try it.
> + return readl(base + HW_USBPHY_PLL_SIC) & BM_USBPHY_PLL_LOCK
> + ? 0 : -ETIMEDOUT;
> +}
> +
> +static int mxs_phy_pll_enable(void __iomem *base, bool enable)
> +{
> + int ret = 0;
> +
> + if (enable) {
> + writel(BM_USBPHY_PLL_REG_ENABLE, base + HW_USBPHY_PLL_SIC_SET);
> + writel(BM_USBPHY_PLL_BYPASS, base + HW_USBPHY_PLL_SIC_CLR);
> + writel(BM_USBPHY_PLL_POWER, base + HW_USBPHY_PLL_SIC_SET);
> + ret = wait_for_pll_lock(base);
> + if (ret)
> + return ret;
> + writel(BM_USBPHY_PLL_EN_USB_CLKS, base +
> + HW_USBPHY_PLL_SIC_SET);
> + } else {
> + writel(BM_USBPHY_PLL_EN_USB_CLKS, base +
> + HW_USBPHY_PLL_SIC_CLR);
> + writel(BM_USBPHY_PLL_POWER, base + HW_USBPHY_PLL_SIC_CLR);
> + writel(BM_USBPHY_PLL_BYPASS, base + HW_USBPHY_PLL_SIC_SET);
> + writel(BM_USBPHY_PLL_REG_ENABLE, base + HW_USBPHY_PLL_SIC_CLR);
> + }
> +
> + return ret;
> +}
> +
> static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
> {
> int ret;
> void __iomem *base = mxs_phy->phy.io_priv;
>
> + if (is_imx7ulp_phy(mxs_phy)) {
> + ret = mxs_phy_pll_enable(base, true);
> + if (ret)
> + return
Re: [BUG REPORT] usb: dwc3: "failed to enable ep0out" when enabling mass storage mode
Hi,
e...@gnarbox.com writes:
> Hi Felipe,
>
> I'm picking up a bug my coworker Rob touched on in this thread:
> https://marc.info/?l=linux-usb&m=155349928622570&w=2
>
> We occasionally see the following dmesg when enabling mass storage mode:
>
> dwc3 dwc3.1.auto: failed to enable ep0out
>
> To reproduce after a clean boot:
>
> Enable mass storage mode
> Disable mass storage mode
> Enable mass storage mode
>
> I don't need to plug any devices, just switch modes.
>
> The error does not happen every boot. If I don't get the error on that
> second enable, then as far as I can tell I won't get the error at all
> during that boot.
>
> I've attached the trace and regdump. When capturing these I was running
> a 4.9.115 kernel and using the g_mass_storage driver for simplicity.
> Here is the shell session:
>
> root@gnarbox-2:~# echo device >
> /sys/class/usb_role/intel_xhci_usb_sw-role-switch/role && modprobe
> g_mass_storage file=/dev/nvme0n1p7 iSerialNumber=90405
> [ 118.627628] Mass Storage Function, version: 2009/09/11
> [ 118.633426] LUN: removable file: (no medium)
> [ 118.638283] LUN: file: /dev/nvme0n1p7
> [ 118.642397] Number of LUNs=1
> [ 118.646080] g_mass_storage gadget: Mass Storage Gadget, version:
> 2009/09/11
> [ 118.653902] g_mass_storage gadget: g_mass_storage ready
> root@gnarbox-2:~# modprobe -r g_mass_storage && echo host >
> /sys/class/usb_role/intel_xhci_usb_sw-role-switch/role
> root@gnarbox-2:~# echo device >
> /sys/class/usb_role/intel_xhci_usb_sw-role-switch/role && modprobe
> g_mass_storage file=/dev/nvme0n1p7 iSerialNumber=90405
> [ 123.416789] Mass Storage Function, version: 2009/09/11
> [ 123.422546] LUN: removable file: (no medium)
> [ 123.427386] LUN: file: /dev/nvme0n1p7
> [ 123.431531] Number of LUNs=1
> [ 123.435278] g_mass_storage gadget: Mass Storage Gadget, version:
> 2009/09/11
> [ 123.443168] g_mass_storage gadget: g_mass_storage ready
> [ 123.451998] dwc3 dwc3.1.auto: failed to enable ep0out
When this happens, I see this:
modprobe-1046 [001] d..1 123.450054: dwc3_gadget_ep_cmd: ep0out: cmd
'Start New Configuration' [9] params --> status:
Successful
modprobe-1046 [001] d..1 123.451990: dwc3_gadget_ep_cmd: ep0out: cmd
'Set Endpoint Transfer Resource' [2] params 0001 -->
status: Timed Out
Why is that waiting only 1ms? Maybe your platform takes longer,
sometimes, to complete xfer resource allocation?
Try this:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d67655384eb2..ad1069fe3b8f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -270,7 +270,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned
cmd,
{
const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
struct dwc3 *dwc = dep->dwc;
- u32 timeout = 1000;
+ u32 timeout = 5000;
u32 saved_config = 0;
u32 reg;
Let me know if it helps or not. I guess it's also time to switch this
block of code to readl_poll_timeout_atomic().
--
balbi
[v5 PATCH 0/6] add USB Type-B GPIO connector driver
Because the USB Connector is introduced and the requirement of usb-connector.txt binding, the old way using extcon to support USB Dual-Role switch is now deprecated, meanwhile there is no available common driver when use Type-B connector, typically using an input GPIO to detect USB ID pin. This patch series introduce a Type-B GPIO connector driver and try to replace the function provided by extcon-usb-gpio driver. v5 changes: 1. remove linux/of.h and put usb_role_switch when error happens, suggested by Biju 2. treat Type-B connector as USB controller's child, but not as a virtual device, suggested by Rob 3. provide and use generic property "usb-role-switch", see [1], suggested by Rob Note: this series still depends on [2] [1]: [v3] dt-binding: usb: add usb-role-switch property https://patchwork.kernel.org/patch/10934835/ [2]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h https://patchwork.kernel.org/patch/10909971/ v4 changes: 1. use switch_fwnode_match() to find fwnode suggested by Heikki 2. assign fwnode member of usb_role_switch struct suggested by Heikki 3. make [4/6] depend on [2] 3. remove linux/gpio.h suggested by Linus 4. put node when error happens [4/6] usb: roles: add API to get usb_role_switch by node [2] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h https://patchwork.kernel.org/patch/10909971/ v3 changes: 1. add GPIO direction, and use fixed-regulator for GPIO controlled VBUS regulator suggested by Rob; 2. rebuild fwnode_usb_role_switch_get() suggested by Andy and Heikki 3. treat the type-B connector as a virtual device; 4. change file name of driver again 5. select USB_ROLE_SWITCH in mtu3/Kconfig suggested by Heikki 6. rename ssusb_mode_manual_switch() to ssusb_mode_switch() v2 changes: 1. make binding clear, and add a extra compatible suggested by Hans Chunfeng Yun (6): dt-bindings: connector: add optional properties for Type-B dt-bindings: usb: add binding for Type-B GPIO connector driver dt-bindings: usb: mtu3: add properties about USB Role Switch usb: roles: add API to get usb_role_switch by node usb: roles: add USB Type-B GPIO connector driver usb: mtu3: register a USB Role Switch for dual role mode .../bindings/connector/usb-connector.txt | 14 + .../devicetree/bindings/usb/mediatek,mtu3.txt | 10 + .../bindings/usb/typeb-conn-gpio.txt | 42 +++ drivers/usb/mtu3/Kconfig | 1 + drivers/usb/mtu3/mtu3.h | 5 + drivers/usb/mtu3/mtu3_debugfs.c | 4 +- drivers/usb/mtu3/mtu3_dr.c| 48 ++- drivers/usb/mtu3/mtu3_dr.h| 6 +- drivers/usb/mtu3/mtu3_plat.c | 3 +- drivers/usb/roles/Kconfig | 11 + drivers/usb/roles/Makefile| 1 + drivers/usb/roles/class.c | 24 ++ drivers/usb/roles/typeb-conn-gpio.c | 295 ++ include/linux/usb/role.h | 8 + 14 files changed, 465 insertions(+), 7 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt create mode 100644 drivers/usb/roles/typeb-conn-gpio.c -- 2.21.0
[PATCH v5 2/6] dt-bindings: usb: add binding for Type-B GPIO connector driver
It's used to support dual role switch via GPIO when use Type-B
receptacle, typically the USB ID pin is connected to an input
GPIO pin
Signed-off-by: Chunfeng Yun
---
v5 changes:
1. treat type-B connector as child device of USB controller's, but not
as a separate virtual device, suggested by Rob
2. put connector's port node under connector node, suggested by Rob
v4 no changes
v3 changes:
1. treat type-B connector as a virtual device, but not child device of
USB controller's
v2 changes:
1. new patch to make binding clear suggested by Hans
---
.../bindings/usb/typeb-conn-gpio.txt | 42 +++
1 file changed, 42 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
diff --git a/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
new file mode 100644
index ..20dd3499a348
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
@@ -0,0 +1,42 @@
+USB Type-B GPIO Connector
+
+This is used to switch dual role mode from the USB ID pin connected to
+an input GPIO pin.
+
+Required properties:
+- compatible : should include "linux,typeb-conn-gpio" and "usb-b-connector".
+- id-gpios, vbus-gpios : either one of them must be present, and both
+ can be present as well.
+- vbus-supply : can be present if needed when supports dual role mode or
+ host mode.
+ see connector/usb-connector.txt
+
+Sub-nodes:
+- port : should be present.
+ see graph.txt
+
+Example:
+
+&mtu3 {
+ status = "okay";
+
+ connector {
+ compatible = "linux,typeb-conn-gpio", "usb-b-connector";
+ label = "micro-USB";
+ type = "micro";
+ id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
+ vbus-supply = <&usb_p0_vbus>;
+
+ port {
+ bconn_ep: endpoint@0 {
+ remote-endpoint = <&usb_role_sw>;
+ };
+ };
+ };
+
+ port {
+ usb_role_sw: endpoint@0 {
+ remote-endpoint = <&bconn_ep>;
+ };
+ };
+};
--
2.21.0
[PATCH v5 6/6] usb: mtu3: register a USB Role Switch for dual role mode
Because extcon is not allowed for new bindings, and the
dual role switch is supported by USB Role Switch,
especially for Type-C drivers, so register a USB Role
Switch to support the new way
Signed-off-by: Chunfeng Yun
---
v5 no change
v4 changes:
1. assign fwnode member of usb_role_switch struct suggested by Heikki
v3 changes:
1. select USB_ROLE_SWITCH in Kconfig suggested by Heikki
2. rename ssusb_mode_manual_switch() to ssusb_mode_switch()
v2 no change
---
drivers/usb/mtu3/Kconfig| 1 +
drivers/usb/mtu3/mtu3.h | 5
drivers/usb/mtu3/mtu3_debugfs.c | 4 +--
drivers/usb/mtu3/mtu3_dr.c | 48 -
drivers/usb/mtu3/mtu3_dr.h | 6 ++---
drivers/usb/mtu3/mtu3_plat.c| 3 ++-
6 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/mtu3/Kconfig b/drivers/usb/mtu3/Kconfig
index bcc23486c4ed..88e3db7b3016 100644
--- a/drivers/usb/mtu3/Kconfig
+++ b/drivers/usb/mtu3/Kconfig
@@ -43,6 +43,7 @@ config USB_MTU3_DUAL_ROLE
bool "Dual Role mode"
depends on ((USB=y || USB=USB_MTU3) && (USB_GADGET=y ||
USB_GADGET=USB_MTU3))
depends on (EXTCON=y || EXTCON=USB_MTU3)
+ select USB_ROLE_SWITCH
help
This is the default mode of working of MTU3 controller where
both host and gadget features are enabled.
diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
index 76ecf12fdf62..6087be236a35 100644
--- a/drivers/usb/mtu3/mtu3.h
+++ b/drivers/usb/mtu3/mtu3.h
@@ -199,6 +199,9 @@ struct mtu3_gpd_ring {
* @id_nb : notifier for iddig(idpin) detection
* @id_work : work of iddig detection notifier
* @id_event : event of iddig detecion notifier
+* @role_sw : use USB Role Switch to support dual-role switch, can't use
+* extcon at the same time, and extcon is deprecated.
+* @role_sw_used : true when the USB Role Switch is used.
* @is_u3_drd: whether port0 supports usb3.0 dual-role device or not
* @manual_drd_enabled: it's true when supports dual-role device by debugfs
* to switch host/device modes depending on user input.
@@ -212,6 +215,8 @@ struct otg_switch_mtk {
struct notifier_block id_nb;
struct work_struct id_work;
unsigned long id_event;
+ struct usb_role_switch *role_sw;
+ bool role_sw_used;
bool is_u3_drd;
bool manual_drd_enabled;
};
diff --git a/drivers/usb/mtu3/mtu3_debugfs.c b/drivers/usb/mtu3/mtu3_debugfs.c
index b7c86ccd50b4..3ed666f94dd9 100644
--- a/drivers/usb/mtu3/mtu3_debugfs.c
+++ b/drivers/usb/mtu3/mtu3_debugfs.c
@@ -453,9 +453,9 @@ static ssize_t ssusb_mode_write(struct file *file, const
char __user *ubuf,
return -EFAULT;
if (!strncmp(buf, "host", 4) && !ssusb->is_host) {
- ssusb_mode_manual_switch(ssusb, 1);
+ ssusb_mode_switch(ssusb, 1);
} else if (!strncmp(buf, "device", 6) && ssusb->is_host) {
- ssusb_mode_manual_switch(ssusb, 0);
+ ssusb_mode_switch(ssusb, 0);
} else {
dev_err(ssusb->dev, "wrong or duplicated setting\n");
return -EINVAL;
diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
index 5fcb71af875a..08e18448e8b8 100644
--- a/drivers/usb/mtu3/mtu3_dr.c
+++ b/drivers/usb/mtu3/mtu3_dr.c
@@ -7,6 +7,8 @@
* Author: Chunfeng Yun
*/
+#include
+
#include "mtu3.h"
#include "mtu3_dr.h"
#include "mtu3_debug.h"
@@ -280,7 +282,7 @@ static int ssusb_extcon_register(struct otg_switch_mtk
*otg_sx)
* This is useful in special cases, such as uses TYPE-A receptacle but also
* wants to support dual-role mode.
*/
-void ssusb_mode_manual_switch(struct ssusb_mtk *ssusb, int to_host)
+void ssusb_mode_switch(struct ssusb_mtk *ssusb, int to_host)
{
struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
@@ -318,6 +320,47 @@ void ssusb_set_force_mode(struct ssusb_mtk *ssusb,
mtu3_writel(ssusb->ippc_base, SSUSB_U2_CTRL(0), value);
}
+static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
+{
+ struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
+ bool to_host = false;
+
+ if (role == USB_ROLE_HOST)
+ to_host = true;
+
+ if (to_host ^ ssusb->is_host)
+ ssusb_mode_switch(ssusb, to_host);
+
+ return 0;
+}
+
+static enum usb_role ssusb_role_sw_get(struct device *dev)
+{
+ struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
+ enum usb_role role;
+
+ role = ssusb->is_host ? USB_ROLE_HOST : USB_ROLE_DEVICE;
+
+ return role;
+}
+
+static int ssusb_role_sw_register(struct otg_switch_mtk *otg_sx)
+{
+ struct usb_role_switch_desc role_sx_desc = { 0 };
+ struct ssusb_mtk *ssusb =
+ container_of(otg_sx, struct ssusb_mtk, otg_switch);
+
+ if (!otg_sx->role_sw_used)
+ return 0;
+
+ role_sx_desc.set = ssusb_role_sw_set;
+ role_sx_desc.get = ssusb_role_sw_get;
+ role_sx_desc.fwno
[PATCH v5 5/6] usb: roles: add USB Type-B GPIO connector driver
Due to the requirement of usb-connector.txt binding, the old way
using extcon to support USB Dual-Role switch is now deprecated
when use Type-B connector.
This patch introduces a driver of Type-B connector which typically
uses an input GPIO to detect USB ID pin, and try to replace the
function provided by extcon-usb-gpio driver
Signed-off-by: Chunfeng Yun
---
v5 changes:
1. put usb_role_switch when error happens suggested by Biju
2. don't treat bype-B connector as a virtual device suggested by Rob
v4 changes:
1. remove linux/gpio.h suggested by Linus
2. put node when error happens
v3 changes:
1. treat bype-B connector as a virtual device;
2. change file name again
v2 changes:
1. file name is changed
2. use new compatible
---
drivers/usb/roles/Kconfig | 11 ++
drivers/usb/roles/Makefile | 1 +
drivers/usb/roles/typeb-conn-gpio.c | 295
3 files changed, 307 insertions(+)
create mode 100644 drivers/usb/roles/typeb-conn-gpio.c
diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
index f8b31aa67526..d1156e18a81a 100644
--- a/drivers/usb/roles/Kconfig
+++ b/drivers/usb/roles/Kconfig
@@ -26,4 +26,15 @@ config USB_ROLES_INTEL_XHCI
To compile the driver as a module, choose M here: the module will
be called intel-xhci-usb-role-switch.
+config TYPEB_CONN_GPIO
+ tristate "USB Type-B GPIO Connector"
+ depends on GPIOLIB
+ help
+ The driver supports USB role switch between host and device via GPIO
+ based USB cable detection, used typically if an input GPIO is used
+ to detect USB ID pin.
+
+ To compile the driver as a module, choose M here: the module will
+ be called typeb-conn-gpio.ko
+
endif # USB_ROLE_SWITCH
diff --git a/drivers/usb/roles/Makefile b/drivers/usb/roles/Makefile
index 757a7d2797eb..5d5620d9d113 100644
--- a/drivers/usb/roles/Makefile
+++ b/drivers/usb/roles/Makefile
@@ -3,3 +3,4 @@
obj-$(CONFIG_USB_ROLE_SWITCH) += roles.o
roles-y:= class.o
obj-$(CONFIG_USB_ROLES_INTEL_XHCI) += intel-xhci-usb-role-switch.o
+obj-$(CONFIG_TYPEB_CONN_GPIO) += typeb-conn-gpio.o
diff --git a/drivers/usb/roles/typeb-conn-gpio.c
b/drivers/usb/roles/typeb-conn-gpio.c
new file mode 100644
index ..988ecf565f33
--- /dev/null
+++ b/drivers/usb/roles/typeb-conn-gpio.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB Type-B GPIO Connector Driver
+ *
+ * Copyright (C) 2019 MediaTek Inc.
+ *
+ * Author: Chunfeng Yun
+ *
+ * Some code borrowed from drivers/extcon/extcon-usb-gpio.c
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define USB_GPIO_DEB_MS20 /* ms */
+#define USB_GPIO_DEB_US((USB_GPIO_DEB_MS) * 1000) /* us */
+
+#define USB_CONN_IRQF \
+ (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)
+
+struct usb_conn_info {
+ struct device *dev;
+ struct usb_role_switch *role_sw;
+ enum usb_role last_role;
+ struct regulator *vbus;
+ struct delayed_work dw_det;
+ unsigned long debounce_jiffies;
+
+ struct gpio_desc *id_gpiod;
+ struct gpio_desc *vbus_gpiod;
+ int id_irq;
+ int vbus_irq;
+};
+
+/**
+ * "DEVICE" = VBUS and "HOST" = !ID, so we have:
+ * Both "DEVICE" and "HOST" can't be set as active at the same time
+ * so if "HOST" is active (i.e. ID is 0) we keep "DEVICE" inactive
+ * even if VBUS is on.
+ *
+ * Role | ID | VBUS
+ *
+ * [1] DEVICE| H | H
+ * [2] NONE | H | L
+ * [3] HOST | L | H
+ * [4] HOST | L | L
+ *
+ * In case we have only one of these signals:
+ * - VBUS only - we want to distinguish between [1] and [2], so ID is always 1
+ * - ID only - we want to distinguish between [1] and [4], so VBUS = ID
+ */
+static void usb_conn_detect_cable(struct work_struct *work)
+{
+ struct usb_conn_info *info;
+ enum usb_role role;
+ int id, vbus, ret;
+
+ info = container_of(to_delayed_work(work),
+ struct usb_conn_info, dw_det);
+
+ /* check ID and VBUS */
+ id = info->id_gpiod ?
+ gpiod_get_value_cansleep(info->id_gpiod) : 1;
+ vbus = info->vbus_gpiod ?
+ gpiod_get_value_cansleep(info->vbus_gpiod) : id;
+
+ if (!id)
+ role = USB_ROLE_HOST;
+ else if (vbus)
+ role = USB_ROLE_DEVICE;
+ else
+ role = USB_ROLE_NONE;
+
+ dev_dbg(info->dev, "role %d/%d, gpios: id %d, vbus %d\n",
+ info->last_role, role, id, vbus);
+
+ if (info->last_role == role) {
+ dev_warn(info->dev, "repeated role: %d\n", role);
+ return;
+ }
+
+ if (info->last_role == USB_ROLE_HOST)
+
[PATCH v5 3/6] dt-bindings: usb: mtu3: add properties about USB Role Switch
Now the USB Role Switch is supported, so add properties about it,
and modify some description related.
Signed-off-by: Chunfeng Yun
---
v5 changes
1. modify decription about extcon and vbus-supply properties
2. make this patch depend on [1]
[1]: [v3] dt-binding: usb: add usb-role-switch property
https://patchwork.kernel.org/patch/10934835/
v4: no changes
v3: no changes
v2 changes:
1. fix typo
2. refer new binding about connector property
---
.../devicetree/bindings/usb/mediatek,mtu3.txt | 10 ++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
b/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
index 3382b5cb471d..3a8300205cdb 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
+++ b/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
@@ -28,8 +28,13 @@ Optional properties:
parent's address space
- extcon : external connector for vbus and idpin changes detection, needed
when supports dual-role mode.
+ it's considered valid for compatibility reasons, not allowed for
+ new bindings, and use "usb-role-switch" property instead.
- vbus-supply : reference to the VBUS regulator, needed when supports
dual-role mode.
+ it's considered valid for compatibility reasons, not allowed for
+ new bindings, and put into a usb-connector node.
+ see connector/usb-connector.txt.
- pinctrl-names : a pinctrl state named "default" is optional, and need be
defined if auto drd switch is enabled, that means the property dr_mode
is set as "otg", and meanwhile the property "mediatek,enable-manual-drd"
@@ -39,6 +44,8 @@ Optional properties:
- maximum-speed : valid arguments are "super-speed", "high-speed" and
"full-speed"; refer to usb/generic.txt
+ - usb-role-switch : use USB Role Switch to support dual-role switch, but
+ not extcon; see usb/generic.txt.
- enable-manual-drd : supports manual dual-role switch via debugfs; usually
used when receptacle is TYPE-A and also wants to support dual-role
mode.
@@ -61,6 +68,9 @@ The xhci should be added as subnode to mtu3 as shown in the
following example
if host mode is enabled. The DT binding details of xhci can be found in:
Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt
+The port would be added as subnode if use "usb-role-switch" property.
+ see graph.txt
+
Example:
ssusb: usb@11271000 {
compatible = "mediatek,mt8173-mtu3";
--
2.21.0
[PATCH v5 1/6] dt-bindings: connector: add optional properties for Type-B
Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for usb-b-connector Signed-off-by: Chunfeng Yun Reviewed-by: Rob Herring --- v5 no changes v4 no changes v3 changes: 1. add GPIO direction, and use fixed-regulator for GPIO controlled VBUS regulator suggested by Rob; v2 changes: 1. describe more clear for vbus-gpios and vbus-supply suggested by Hans --- .../bindings/connector/usb-connector.txt | 14 ++ 1 file changed, 14 insertions(+) diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt index a9a2f2fc44f2..e8f9e854fd11 100644 --- a/Documentation/devicetree/bindings/connector/usb-connector.txt +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt @@ -17,6 +17,20 @@ Optional properties: - self-powered: Set this property if the usb device that has its own power source. +Optional properties for usb-b-connector: +- id-gpios: an input gpio for USB ID pin. +- vbus-gpios: an input gpio for USB VBUS pin, used to detect presence of + VBUS 5V. + see gpio/gpio.txt. +- vbus-supply: a phandle to the regulator for USB VBUS if needed when host + mode or dual role mode is supported. + Particularly, if use an output GPIO to control a VBUS regulator, should + model it as a regulator. + see regulator/fixed-regulator.yaml +- pinctrl-names : a pinctrl state named "default" is optional +- pinctrl-0 : pin control group + see pinctrl/pinctrl-bindings.txt + Optional properties for usb-c-connector: - power-role: should be one of "source", "sink" or "dual"(DRP) if typec connector has power support. -- 2.21.0
[PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node
Add fwnode_usb_role_switch_get() to make easier to get
usb_role_switch by fwnode which register it.
It's useful when there is not device_connection registered
between two drivers and only knows the fwnode which register
usb_role_switch.
Signed-off-by: Chunfeng Yun
Tested-by: Biju Das
---
v5 changes:
1. remove linux/of.h suggested by Biju
2. add tested by Biju
Note: still depends on [1]
[1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
https://patchwork.kernel.org/patch/10909971/
v4 changes:
1. use switch_fwnode_match() to find fwnode suggested by Heikki
2. this patch now depends on [1]
[1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
https://patchwork.kernel.org/patch/10909971/
v3 changes:
1. use fwnodes instead of node suggested by Andy
2. rebuild the API suggested by Heikki
v2 no changes
---
drivers/usb/roles/class.c | 24
include/linux/usb/role.h | 8
2 files changed, 32 insertions(+)
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f45d8df5cfb8..4a1f09a41ec0 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device
*dev)
}
EXPORT_SYMBOL_GPL(usb_role_switch_get);
+/**
+ * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
+ * @fwnode: The fwnode that register USB role switch
+ *
+ * Finds and returns role switch registered by @fwnode. The reference count
+ * for the found switch is incremented.
+ */
+struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
+{
+ struct usb_role_switch *sw;
+ struct device *dev;
+
+ dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
+ if (!dev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ sw = to_role_switch(dev);
+ WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+
+ return sw;
+}
+EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
+
/**
* usb_role_switch_put - Release handle to a switch
* @sw: USB Role Switch
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index da2b9641b877..35d460f9ec40 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -48,6 +48,8 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum
usb_role role);
enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw);
struct usb_role_switch *usb_role_switch_get(struct device *dev);
void usb_role_switch_put(struct usb_role_switch *sw);
+struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode);
struct usb_role_switch *
usb_role_switch_register(struct device *parent,
@@ -72,6 +74,12 @@ static inline struct usb_role_switch
*usb_role_switch_get(struct device *dev)
static inline void usb_role_switch_put(struct usb_role_switch *sw) { }
+static inline struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline struct usb_role_switch *
usb_role_switch_register(struct device *parent,
const struct usb_role_switch_desc *desc)
--
2.21.0
Re: [REGRESSION] usb: gadget: f_fs: Allow scatter-gather buffers
Hi, W dniu 13.05.2019 o 20:09, John Stultz pisze: On Mon, May 13, 2019 at 7:08 AM Andrzej Pietrasiewicz wrote: Do you get "functionfs read size 512 > requested size 24, splitting request into multiple reads" message when problems happen? Unfortunately no. Actually that's a fortunate outcome :) Is there anything in the kernel log? Nope. Just the transfers stall/hang and further attempts at adb connections fail until the USB cable is unplugged and replugged. Is there a way to try your adb without building and running the whole Android? Maybe? I have heard of folks doing it in the past, though I don't really know a quick path to get there. Is there anything else I can try for you? Have you tried compiling FunctionFS with debugging enabled? You do so bu uncommenting: /* #define DEBUG */ /* #define VERBOSE_DEBUG */ at the beginning of drivers/usb/gadget/function/f_fs.c Is there anything suspicious in the kernel log when you run it then? Remote debugging through this mailing list will incur enormous round trip time ;) The most valuable help would be helping in reproducing the problem you encounter. One question that comes to my mind is this: Does the USB transmission stall (e.g. endpoint stall) or not? In other words, is adb connection broken because USB stops transmitting anything, or because the data is transmitted but its integrity is broken during transmission and that causes adb/adbd confusion which results in stopping their operation? Does anything keep happening on FunctionFS when adb connection is broken? Andrzej
[PATCH] usb: cp210x: Add cp2108 GPIOs support
Each chip from the cp210x series got GPIOs on board. This commit
provides the support for sixteen ones placed on the cp2108 four-ports
serial console controller. All of the GPIOs are equally distributed
to four USB interfaces in accordance with GPIOs alternative functions
attachment.
cp2108 GPIOs can be either in open-drain or push-pull modes setup once
after reset was cleared. In this matter it doesn't differ from the rest
of cp210x devices supported by the driver. So with minor alterations the
standard output/intput GPIO interface is implemented for cp2108.
Aside from traditional GPIO functions like setting/getting pins value,
each GPIO is also multiplexed with alternative functions: TX/RX LEDs, RS485
TX-enable and Clocks source. These functions can't be activated on-fly.
Instead the chips firmware should be properly setup, so they would be
enabled in the ports-config structure at the controller logic startup.
Needless to say, that when the alternative functions are activated,
the GPIOs can't be used. Thus we need to check the GPIO pin config in the
request callback and deny the request if GPIO standard function is
disabled.
Signed-off-by: Serge Semin
---
drivers/usb/serial/Kconfig | 2 +-
drivers/usb/serial/cp210x.c | 158
2 files changed, 143 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/serial/Kconfig b/drivers/usb/serial/Kconfig
index 7d031911d04e..20bd4c0632c7 100644
--- a/drivers/usb/serial/Kconfig
+++ b/drivers/usb/serial/Kconfig
@@ -138,7 +138,7 @@ config USB_SERIAL_DIGI_ACCELEPORT
config USB_SERIAL_CP210X
tristate "USB CP210x family of UART Bridge Controllers"
help
- Say Y here if you want to use a CP2101/CP2102/CP2103 based USB
+ Say Y here if you want to use a CP2101/2/3/4/5/8 based USB
to RS232 converters.
To compile this driver as a module, choose M here: the
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 979bef9bfb6b..a97f04d9e99f 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -505,6 +505,56 @@ struct cp210x_gpio_write {
u8 state;
} __packed;
+/* CP2108 interfaces, gpio (per interface), port-blocks number, GPIO block. */
+#define CP2108_IFACE_NUM 4
+#define CP2108_GPIO_NUM4
+#define CP2108_PB_NUM 5
+#define CP2108_GPIO_PB 1
+
+/*
+ * CP2108 default pins state. There are five PBs. Each one is with its specific
+ * pins-set (see USB Express SDK sources or SDK-based smt application
+ * https://github.com/fancer/smt-cp210x for details).
+ */
+struct cp2108_state {
+ __le16 mode[CP2108_PB_NUM];/* 0 - Open-Drain, 1 - Push-Pull */
+ __le16 low_power[CP2108_PB_NUM];
+ __le16 latch[CP2108_PB_NUM]; /* 0 - Logic Low, 1 - Logic High */
+} __packed;
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_GET_PORTCONFIG call reads these 73 bytes.
+ * Reset/Suspend latches describe default states after reset/suspend of the
+ * pins. The rest are responsible for alternate functions settings of the
+ * chip pins (see USB Express SDK sources or SDK-based smt application
+ * https://github.com/fancer/smt-cp210x for details).
+ */
+struct cp2108_config {
+ struct cp2108_state reset_latch;
+ struct cp2108_state suspend_latch;
+ u8 ip_delay[CP2108_IFACE_NUM];
+ u8 enhanced_fxn[CP2108_IFACE_NUM];
+ u8 enhanced_fxn_dev;
+ u8 ext_clock_freq[CP2108_IFACE_NUM];
+} __packed;
+
+/* CP2108 port alternate functions fields */
+#define CP2108_GPIO_TXLED_MODE BIT(0)
+#define CP2108_GPIO_RXLED_MODE BIT(1)
+#define CP2108_GPIO_RS485_MODE BIT(2)
+#define CP2108_GPIO_RS485_LOGICBIT(3)
+#define CP2108_GPIO_CLOCK_MODE BIT(4)
+#define CP2108_DYNAMIC_SUSPEND BIT(5)
+
+/*
+ * CP210X_VENDOR_SPECIFIC, CP210X_WRITE_LATCH call writes these 0x4 bytes
+ * to CP2108 controller.
+ */
+struct cp2108_gpio_write {
+ __le16 mask;
+ __le16 state;
+} __packed;
+
/*
* Helper to get interface number when we only have struct usb_serial.
*/
@@ -1366,10 +1416,15 @@ static int cp210x_gpio_get(struct gpio_chip *gc,
unsigned int gpio)
struct usb_serial *serial = gpiochip_get_data(gc);
struct cp210x_serial_private *priv = usb_get_serial_data(serial);
u8 req_type = REQTYPE_DEVICE_TO_HOST;
- int result;
- u8 buf;
-
- if (priv->partnum == CP210X_PARTNUM_CP2105)
+ union {
+ u8 single;
+ __le16 dual;
+ } buf;
+ int result, bufsize = sizeof(buf.single);
+
+ if (priv->partnum == CP210X_PARTNUM_CP2108)
+ bufsize = sizeof(buf.dual);
+ else if (priv->partnum == CP210X_PARTNUM_CP2105)
req_type = REQTYPE_INTERFACE_TO_HOST;
result = usb_autopm_get_interface(serial->interface);
@@ -1377,39 +1432,51 @@ static int cp210x_gpio_get(struct g
Re: [PATCH 1/2] usbip: Remove repeated setting of hcd->state in vhci_bus_suspend()
On Wed, May 08, 2019 at 11:28:05AM -0600, shuah wrote: > On 5/7/19 9:49 AM, Suwan Kim wrote: > > Hi Shuah, > > > > On Mon, May 06, 2019 at 09:13:02AM -0600, shuah wrote: > > > On 5/6/19 6:55 AM, Suwan Kim wrote: > > > > When hcd suspends execution, hcd_bus_suspend() calls vhci_bus_suspend() > > > > which sets hcd->state as HC_STATE_SUSPENDED. But after calling > > > > vhci_bus_suspend(), hcd_bus_suspend() also sets hcd->state as > > > > HC_STATE_SUSPENDED. > > > > So, setting hcd->state in vhci_hcd_suspend() is unnecessary. > > > > > > > > Signed-off-by: Suwan Kim > > > > --- > > > >drivers/usb/usbip/vhci_hcd.c | 4 > > > >1 file changed, 4 deletions(-) > > > > > > > > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c > > > > index 667d9c0ec905..e6f378d00fb6 100644 > > > > --- a/drivers/usb/usbip/vhci_hcd.c > > > > +++ b/drivers/usb/usbip/vhci_hcd.c > > > > @@ -1238,10 +1238,6 @@ static int vhci_bus_suspend(struct usb_hcd *hcd) > > > > dev_dbg(&hcd->self.root_hub->dev, "%s\n", __func__); > > > > - spin_lock_irqsave(&vhci->lock, flags); > > > > - hcd->state = HC_STATE_SUSPENDED; > > > > - spin_unlock_irqrestore(&vhci->lock, flags); > > > > - > > > > return 0; > > > >} > > > > > > > > > > Tell me more about why you think this change is needed? How did you test > > > this change? > > > > I think that host controller specific functions, vhci_bus_resume() > > or vhci_bus_suspend() in the case of vhci, usually process host > > controller specific data (struct vhci_hcd) not a generic data > > (struct usb_hcd). The generic data is usually processed by generic HCD > > layer. But vhci_bus_resume() and vhci_bus_suspend() set generic data > > (hcd->state) and moreover this variable is set in generic HCD layer > > once again(hcd_bus_resume() and hcd_bus_suspend()). > > > > So, i think host controller specific functions (vhci_bus_resume() > > and vhci_bus_suspend()) don't need to set the generic data that is > > "hcd->state = HC_STATE_RUNNING or HC_STATE_SUSPEND". > > > > It depends. In this case, vhci_hcd is virtual driver and maintains > port status for devices that are remote. It checks hcd->state in > vhci_hub_status(). > > It updates the hcd->state in suspend with vhci->lock hold and checks > status from vhci_hub_status(). Removing updating hcd->state from > vhci_bus_suspend()will open a window where vhci_hub_status() might > find it wrong state. > I didn't know that and I missed. Thank you for pointing out my fault. I agree that removing updating hcd->state which is protected by vhci lock is not correct. But I still have not fully understood the relationship between vhci_bus_suspend() and vhci_hub_status() yet. I will look at it in more detail. Again, thank you for reviewing my patch. Regards Suwan Kim
Re: [PATCH v2 5/8] usb: chipidea: imx: add imx7ulp support
Hi Peter, On Tue, May 14, 2019 at 4:39 AM Peter Chen wrote: > > Add imx7ulp support Since you are adding a new flag CI_HDRC_PMQOS, it would be nice to expand the commit log a bit to explain about it.
Re: USB fuzzing with syzbot
From: Greg Kroah-Hartman Date: Thu, Apr 25, 2019 at 4:25 PM To: Andrey Konovalov Cc: Alan Stern, Gustavo A. R. Silva, USB list, Dmitry Vyukov, Kostya Serebryany, Alexander Potapenko > On Thu, Apr 25, 2019 at 02:44:11PM +0200, Andrey Konovalov wrote: > > On Wed, Apr 24, 2019 at 6:05 PM Andrey Konovalov > > wrote: > > > > > > On Fri, Apr 19, 2019 at 10:35 AM Greg Kroah-Hartman > > > wrote: > > > > > > > > > 2. Is there an easy way to figure out which config options enable > > > > > drivers reachable over USB? > > > > > > > > Looking for all options that depend on USB is a good start. > > > > > > > > > Right now our kernel config is based on one of the Debian kernel > > > > > configs, that supposedly enables enough relevant USB drivers. At the > > > > > same time it enables a lot of other unnecessary stuff, which makes the > > > > > kernel big and long to compile. Ideally, we would to have a way to > > > > > auto-generate a kernel config that enables all the relevant (enabled > > > > > by at least one of the distros) USB drivers. I've looked at whether > > > > > it's possible to figure out which particular options in some kernel > > > > > config are related to USB, but it seems that neither the option names, > > > > > nor the way they are grouped in the config file, are representative > > > > > enough. > > > > > > > > Yeah, it's hard to just carve out this type of configuration, but here's > > > > what I have done in the past to try to be sure I enabled all USB drivers > > > > in my kernel configuration. > > > > > > > > First, start with a "minimally working configuration" by running: > > > > make localmodconfig > > > > on a working system, with the needed modules for booting and operating > > > > properly already loaded. > > > > > > > > That gives you a .config file that should take only minutes to build, > > > > compared to much longer for the normal distro configuration (also be > > > > sure to disable some debugging options so you don't spend extra time > > > > building and stripping symbols). > > > > > > > > Boot and make sure that configuration works. > > > > > > > > Then, take that .config and do: > > > > - disable USB from the configuration by deleting the: > > > > CONFIG_USB_SUPPORT=y > > > > option from your .config > > > > - run 'make oldconfig' to disable all USB drivers > > > > - turn USB back on by setting CONFIG_USB_SUPPORT=y back on in > > > > your .config > > > > - run 'make oldconfig' and answer 'y' or 'm' to all of the > > > > driver options you are presented with. > > > > > > > > That usually catches almost all of them. Sometimes you need to make > > > > sure you have some other subsystem enabled (like SCSI), but odds are, if > > > > you start with a "normally stripped down" configuration that works, you > > > > should be fine. > > > > > > I suspect that make localmodconfig (+ switching CONFIG_USB_SUPPORT off > > > and on) would likely include a lot of stuff that we don't need (there > > > are many options that are =y, but not related to USB at all), but it > > > definitely sounds better than what I have right now (converting almost > > > all =m into =y). I'll give it a shot, thanks! > > > > I've tried this and unfortunately it doesn't work as desired. The > > reason is that localmodconfig will only enable options for the modules > > that are currently loaded, and if a module that some USB driver > > depends on is not loaded, then this driver won't be enabled after yes > > | make oldconfig. For example my machine didn't have the cfg80211 > > module loaded, and thus e.g. CONFIG_AT76C50X_USB didn't get enabled > > after oldconfig. However when I plug in a wireless USB adapter, > > cfg80211 gets loaded together with the USB driver for that adapter. I > > guess the same applies to other kinds of dependency modules (e.g. > > bluetooth). So this would only work if all the dependency modules are > > already loaded. > > Yes, sorry, I thought I said that with: > > > > > First, start with a "minimally working configuration" by running: > > > > make localmodconfig > > > > on a working system, with the needed modules for booting and operating > > > > properly already loaded. > > I guess "working system" implied everything that you _knew_ you wanted > to have loaded :) > > Sorry about the dependancy mess, hopefully you have sorted this out > better now. I've written a script [1], [2] on top of Kconfiglib [3] that merges in all USB configs and their dependencies from a provided (distro) config. The dependency extraction a somewhat best effort, but seems to be working. Maybe you'll find some use for it as well. Thanks! [1] https://github.com/google/syzkaller/blob/master/dashboard/config/kconfiglib-merge-usb-configs.py [2] https://github.com/google/syzkaller/blob/master/dashboard/config/generate-config-usb.sh [3] https://github.com/ulfalizer/Kconfiglib
Re: [PATCH 1/5] USB: serial: fix unthrottle races
On Mon, May 13, 2019 at 02:59:09PM +0200, Johan Hovold wrote:
On Mon, May 13, 2019 at 02:51:31PM +0200, Greg Kroah-Hartman wrote:
On Mon, May 13, 2019 at 01:46:01PM +0200, Johan Hovold wrote:
> Thanks. The issue has been there since v3.3 so I guess you could queue
> it for all stable trees.
Doesn't apply cleanly for 4.4.y or 3.18.y, so if it's really worth
adding there (and I kind of doubt it), I would need a backport :)
Ah ok, just wasn't sure why you chose 4.9+.
On 4.4 and 3.18 it just had a contextual conflict because of
3161da970d38c ("USB: serial: use variable for status"), I can queue both
3161da970d38c and 3f5edd58d040b for 4.4 and 3.18.
--
Thanks,
Sasha
Re: [PATCH 1/5] USB: serial: fix unthrottle races
On Tue, May 14, 2019 at 08:53:53AM -0400, Sasha Levin wrote:
> On Mon, May 13, 2019 at 02:59:09PM +0200, Johan Hovold wrote:
> >On Mon, May 13, 2019 at 02:51:31PM +0200, Greg Kroah-Hartman wrote:
> >> On Mon, May 13, 2019 at 01:46:01PM +0200, Johan Hovold wrote:
> >
> >> > Thanks. The issue has been there since v3.3 so I guess you could queue
> >> > it for all stable trees.
> >>
> >> Doesn't apply cleanly for 4.4.y or 3.18.y, so if it's really worth
> >> adding there (and I kind of doubt it), I would need a backport :)
> >
> >Ah ok, just wasn't sure why you chose 4.9+.
>
> On 4.4 and 3.18 it just had a contextual conflict because of
> 3161da970d38c ("USB: serial: use variable for status"), I can queue both
> 3161da970d38c and 3f5edd58d040b for 4.4 and 3.18.
Sounds good, thanks!
Johan
Re: Lack of length checking in USB configuration may allow buffer overflow
On Mon, 13 May 2019, Rick Mark wrote: > Hey All, > > I was seeing a linux VM crash due to malformed USB configuration > payloads being malformed. Can you provide more information about this crash? I would like to know exactly what errors are occurring. As far as I can tell, the existing code already tests for all the things your patch adds. > I'm testing this patch now which should > provide better security checking (but this is my first patch so be > kind if I have things wrong.) Have you read the loop in usb_parse_configuration() that starts at the comment: /* Go through the descriptors, checking their length and counting the * number of altsettings for each interface */ (approximately line 585)? This loop should carry out all the tests that your patch is trying to duplicate. Alan Stern
[RFC PATCH v2 2/3] usb: host: ohci-sm501: init genalloc for local memory
From: Laurentiu Tudor
In preparation for dropping the existing "coherent" dma mem declaration
APIs, replace the current dma_declare_coherent_memory() based mechanism
with the creation of a genalloc pool that will be used in the OHCI
subsystem as replacement for the DMA APIs.
For context, see thread here: https://lkml.org/lkml/2019/4/22/357
Signed-off-by: Laurentiu Tudor
---
drivers/usb/host/ohci-sm501.c | 60 +++
1 file changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c
index c26228c25f99..8b829eca04ab 100644
--- a/drivers/usb/host/ohci-sm501.c
+++ b/drivers/usb/host/ohci-sm501.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
@@ -110,40 +111,18 @@ static int ohci_hcd_sm501_drv_probe(struct
platform_device *pdev)
goto err0;
}
- /* The sm501 chip is equipped with local memory that may be used
-* by on-chip devices such as the video controller and the usb host.
-* This driver uses dma_declare_coherent_memory() to make sure
-* usb allocations with dma_alloc_coherent() allocate from
-* this local memory. The dma_handle returned by dma_alloc_coherent()
-* will be an offset starting from 0 for the first local memory byte.
-*
-* So as long as data is allocated using dma_alloc_coherent() all is
-* fine. This is however not always the case - buffers may be allocated
-* using kmalloc() - so the usb core needs to be told that it must copy
-* data into our local memory if the buffers happen to be placed in
-* regular memory. The HCD_LOCAL_MEM flag does just that.
-*/
-
- retval = dma_declare_coherent_memory(dev, mem->start,
-mem->start - mem->parent->start,
-resource_size(mem));
- if (retval) {
- dev_err(dev, "cannot declare coherent memory\n");
- goto err1;
- }
-
/* allocate, reserve and remap resources for registers */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res == NULL) {
dev_err(dev, "no resource definition for registers\n");
retval = -ENOENT;
- goto err2;
+ goto err1;
}
hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
if (!hcd) {
retval = -ENOMEM;
- goto err2;
+ goto err1;
}
hcd->rsrc_start = res->start;
@@ -164,6 +143,36 @@ static int ohci_hcd_sm501_drv_probe(struct platform_device
*pdev)
ohci_hcd_init(hcd_to_ohci(hcd));
+ /* The sm501 chip is equipped with local memory that may be used
+* by on-chip devices such as the video controller and the usb host.
+* This driver uses genalloc so that usb allocations with
+* gen_pool_dma_alloc() allocate from this local memory. The dma_handle
+* returned by gen_pool_dma_alloc() will be an offset starting from 0
+* for the first local memory byte.
+*
+* So as long as data is allocated using gen_pool_dma_alloc() all is
+* fine. This is however not always the case - buffers may be allocated
+* using kmalloc() - so the usb core needs to be told that it must copy
+* data into our local memory if the buffers happen to be placed in
+* regular memory. The HCD_LOCAL_MEM flag does just that.
+*/
+
+ hcd->localmem_pool = devm_gen_pool_create(dev, PAGE_SHIFT,
+ dev_to_node(dev),
+ "ohci-sm501");
+ if (IS_ERR(hcd->localmem_pool)) {
+ retval = PTR_ERR(hcd->localmem_pool);
+ goto err5;
+ }
+ retval = gen_pool_add_virt(hcd->localmem_pool,
+ (unsigned long)mem->start -
+ mem->parent->start,
+ mem->start, resource_size(mem),
+ dev_to_node(dev));
+ if (retval < 0) {
+ dev_err(dev, "failed to add to pool: %d\n", retval);
+ goto err5;
+ }
retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
if (retval)
goto err5;
@@ -181,8 +190,6 @@ static int ohci_hcd_sm501_drv_probe(struct platform_device
*pdev)
release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
err3:
usb_put_hcd(hcd);
-err2:
- dma_release_declared_memory(dev);
err1:
release_mem_region(mem->start, resource_size(mem));
err0:
@@ -197,7 +204,6 @@ static int ohci_hcd_sm501_drv_remove(struct platform_device
*pdev)
usb_remove_hcd(hcd);
release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
usb_put_hcd(hcd);
- dma_release_declared_memory(&
[RFC PATCH v2 0/3] prerequisites for device reserved local mem rework
From: Laurentiu Tudor For HCs that have local memory, replace the current DMA API usage with a genalloc generic allocator to manage the mappings for these devices. This is in preparation for dropping the existing "coherent" dma mem declaration APIs. Current implementation was relying on a short circuit in the DMA API that in the end, was acting as an allocator for these type of devices. Only compiled tested, so any volunteers willing to test are most welcome. Thank you! For context, see thread here: https://lkml.org/lkml/2019/4/22/357 Changes in v2: - use genalloc also in core usb (based on comment from Robin) - moved genpool decl to usb_hcd to be accesible from core usb Laurentiu Tudor (3): USB: use genalloc for USB HCs with local memory usb: host: ohci-sm501: init genalloc for local memory usb: host: ohci-tmio: init genalloc for local memory drivers/usb/core/buffer.c | 12 ++- drivers/usb/host/ohci-hcd.c | 23 +++--- drivers/usb/host/ohci-sm501.c | 60 +++ drivers/usb/host/ohci-tmio.c | 23 +- include/linux/usb/hcd.h | 3 ++ 5 files changed, 80 insertions(+), 41 deletions(-) -- 2.17.1
[RFC PATCH v2 1/3] USB: use genalloc for USB HCs with local memory
From: Laurentiu Tudor
For HCs that have local memory, replace the current DMA API usage
with a genalloc generic allocator to manage the mappings for these
devices.
This is in preparation for dropping the existing "coherent" dma
mem declaration APIs. Current implementation was relying on a short
circuit in the DMA API that in the end, was acting as an allocator
for these type of devices.
For context, see thread here: https://lkml.org/lkml/2019/4/22/357
Signed-off-by: Laurentiu Tudor
---
drivers/usb/core/buffer.c | 12 +++-
drivers/usb/host/ohci-hcd.c | 23 ++-
include/linux/usb/hcd.h | 3 +++
3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index f641342cdec0..5729801e82e0 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
@@ -136,6 +137,10 @@ void *hcd_buffer_alloc(
if (size <= pool_max[i])
return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
}
+
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
+
return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags);
}
@@ -165,5 +170,10 @@ void hcd_buffer_free(
return;
}
}
- dma_free_coherent(hcd->self.sysdev, size, addr, dma);
+
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ gen_pool_free(hcd->localmem_pool, (unsigned long)addr,
+ size);
+ else
+ dma_free_coherent(hcd->self.sysdev, size, addr, dma);
}
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 210181fd98d2..f9462c372943 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
@@ -505,8 +506,15 @@ static int ohci_init (struct ohci_hcd *ohci)
timer_setup(&ohci->io_watchdog, io_watchdog_func, 0);
ohci->prev_frame_no = IO_WATCHDOG_OFF;
- ohci->hcca = dma_alloc_coherent (hcd->self.controller,
- sizeof(*ohci->hcca), &ohci->hcca_dma, GFP_KERNEL);
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool,
+ sizeof(*ohci->hcca),
+ &ohci->hcca_dma);
+ else
+ ohci->hcca = dma_alloc_coherent(hcd->self.controller,
+ sizeof(*ohci->hcca),
+ &ohci->hcca_dma,
+ GFP_KERNEL);
if (!ohci->hcca)
return -ENOMEM;
@@ -990,9 +998,14 @@ static void ohci_stop (struct usb_hcd *hcd)
remove_debug_files (ohci);
ohci_mem_cleanup (ohci);
if (ohci->hcca) {
- dma_free_coherent (hcd->self.controller,
- sizeof *ohci->hcca,
- ohci->hcca, ohci->hcca_dma);
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ gen_pool_free(hcd->localmem_pool,
+ (unsigned long)ohci->hcca,
+ sizeof(*ohci->hcca));
+ else
+ dma_free_coherent(hcd->self.controller,
+ sizeof(*ohci->hcca),
+ ohci->hcca, ohci->hcca_dma);
ohci->hcca = NULL;
ohci->hcca_dma = 0;
}
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 695931b03684..0fee81ef5d52 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -215,6 +215,9 @@ struct usb_hcd {
#defineHC_IS_RUNNING(state) ((state) & __ACTIVE)
#defineHC_IS_SUSPENDED(state) ((state) & __SUSPEND)
+ /* allocator for HCs having local memory */
+ struct gen_pool *localmem_pool;
+
/* more shared queuing code would be good; it should support
* smarter scheduling, handle transaction translators, etc;
* input size of periodic table to an interrupt scheduler.
--
2.17.1
[RFC PATCH v2 3/3] usb: host: ohci-tmio: init genalloc for local memory
From: Laurentiu Tudor
In preparation for dropping the existing "coherent" dma mem declaration
APIs, replace the current dma_declare_coherent_memory() based mechanism
with the creation of a genalloc pool that will be used in the OHCI
subsystem as replacement for the DMA APIs.
For context, see thread here: https://lkml.org/lkml/2019/4/22/357
Signed-off-by: Laurentiu Tudor
---
drivers/usb/host/ohci-tmio.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/host/ohci-tmio.c b/drivers/usb/host/ohci-tmio.c
index f88a0370659f..34869382618f 100644
--- a/drivers/usb/host/ohci-tmio.c
+++ b/drivers/usb/host/ohci-tmio.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
/*-*/
@@ -224,11 +225,6 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device
*dev)
goto err_ioremap_regs;
}
- ret = dma_declare_coherent_memory(&dev->dev, sram->start, sram->start,
- resource_size(sram));
- if (ret)
- goto err_dma_declare;
-
if (cell->enable) {
ret = cell->enable(dev);
if (ret)
@@ -239,6 +235,20 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device
*dev)
ohci = hcd_to_ohci(hcd);
ohci_hcd_init(ohci);
+ hcd->localmem_pool = devm_gen_pool_create(&dev->dev, PAGE_SHIFT,
+ dev_to_node(&dev->dev),
+ "ohci-sm501");
+ if (IS_ERR(hcd->localmem_pool)) {
+ ret = PTR_ERR(hcd->localmem_pool);
+ goto err_enable;
+ }
+ ret = gen_pool_add_virt(hcd->localmem_pool, sram->start, sram->start,
+ resource_size(sram), dev_to_node(&dev->dev));
+ if (ret < 0) {
+ dev_err(&dev->dev, "failed to add to pool: %d\n", ret);
+ goto err_enable;
+ }
+
ret = usb_add_hcd(hcd, irq, 0);
if (ret)
goto err_add_hcd;
@@ -254,8 +264,6 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device
*dev)
if (cell->disable)
cell->disable(dev);
err_enable:
- dma_release_declared_memory(&dev->dev);
-err_dma_declare:
iounmap(hcd->regs);
err_ioremap_regs:
iounmap(tmio->ccr);
@@ -276,7 +284,6 @@ static int ohci_hcd_tmio_drv_remove(struct platform_device
*dev)
tmio_stop_hc(dev);
if (cell->disable)
cell->disable(dev);
- dma_release_declared_memory(&dev->dev);
iounmap(hcd->regs);
iounmap(tmio->ccr);
usb_put_hcd(hcd);
--
2.17.1
Re: [RFC PATCH v2 1/3] USB: use genalloc for USB HCs with local memory
> @@ -136,6 +137,10 @@ void *hcd_buffer_alloc( > if (size <= pool_max[i]) > return dma_pool_alloc(hcd->pool[i], mem_flags, dma); > } > + > + if (hcd->driver->flags & HCD_LOCAL_MEM) > + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma); I think this check needs to be before the above code to use the dma pools, as we should always use the HCD local memory. Probably all the way up just below the size == 0 check, that way we can also remove the other HCD_LOCAL_MEM check. > @@ -165,5 +170,10 @@ void hcd_buffer_free( > return; > } > } > - dma_free_coherent(hcd->self.sysdev, size, addr, dma); > + > + if (hcd->driver->flags & HCD_LOCAL_MEM) > + gen_pool_free(hcd->localmem_pool, (unsigned long)addr, > + size); > + else > + dma_free_coherent(hcd->self.sysdev, size, addr, dma); Same here. > @@ -505,8 +506,15 @@ static int ohci_init (struct ohci_hcd *ohci) > timer_setup(&ohci->io_watchdog, io_watchdog_func, 0); > ohci->prev_frame_no = IO_WATCHDOG_OFF; > > - ohci->hcca = dma_alloc_coherent (hcd->self.controller, > - sizeof(*ohci->hcca), &ohci->hcca_dma, GFP_KERNEL); > + if (hcd->driver->flags & HCD_LOCAL_MEM) > + ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool, > + sizeof(*ohci->hcca), > + &ohci->hcca_dma); > + else > + ohci->hcca = dma_alloc_coherent(hcd->self.controller, > + sizeof(*ohci->hcca), > + &ohci->hcca_dma, > + GFP_KERNEL); I wonder if we could just use hcd_buffer_alloc/free here, althought that would require them to be exported. I'll leave that decision to the relevant maintainers, though. Except for this the series looks exactly what I had envisioned to get rid of the device local dma_declare_coherent use case, thanks!
[BUG] usb: xhci: Possible resource leaks when xhci_run() fails
xhci_pci_setup() is assigned to hc_driver.reset;
xhci_run() is assigned to hc_driver.start();
xhci_stop() is assigned to hc_driver.stop().
xhci_pci_setup() calls xhci_gen_setup, which calls xhci_init(). And
xhci_init() calls xhci_mem_init() to allocate resources.
xhci_stop() calls xhci_mem_cleanup(), to release the resources allocated
in xhci_mem_init() (also namely xhci_pci_setup()).
xhci_run() can fail, because xhci_try_enable_msi() or
xhci_alloc_command() in this function can fail.
In drivers/usb/core/hcd.c:
retval = hcd->driver->reset(hcd);
if (retval < 0) {
..
goto err_hcd_driver_setup;
}
..
retval = hcd->driver->start(hcd);
if (retval < 0) {
..
goto err_hcd_driver_start;
}
...
hcd->driver->stop(hcd);
hcd->state = HC_STATE_HALT;
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);
err_hcd_driver_start:
if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
free_irq(irqnum, hcd);
err_request_irq:
err_hcd_driver_setup:
err_set_rh_speed:
usb_put_invalidate_rhdev(hcd);
err_allocate_root_hub:
usb_deregister_bus(&hcd->self);
err_register_bus:
hcd_buffer_destroy(hcd);
err_create_buf:
usb_phy_roothub_power_off(hcd->phy_roothub);
err_usb_phy_roothub_power_on:
usb_phy_roothub_exit(hcd->phy_roothub);
Thus, when hcd->driver->reset() succeeds and hcd->driver->start() fails,
hcd->driver->stop() is not called.
Namely, when xhci_pci_setup() successfully allocates resources, and
xhci_run() fails, xhci_stop() is not called to release the resources.
For this reason, resource leaks occur in this case.
I check the code of the ehci driver, uhci driver and ohci driver, and
find that they do not have such problem, because:
In the ehci driver, ehci_run() (namely hcd->driver->start()) never fails.
In the uhci driver, all the resources are allocated in uhci_start
(namely hcd->driver->start()), and no resources are allocated in
uhci_pci_init() (namely hcd->driver->reset()).
In the ohci driver, ohci_setup() (namely hcd->driver->reset()) also
allocates resources. But when ohci_start() (namely hcd->driver->start())
is going to fail, ohci_stop() is directly called to release the
resources allocated by ohci_setup().
Thus, there are two possible ways of fixing bugs:
1) Call xhci_stop() when xhci_run() is going to fail (like the ohci driver)
2) Move all resource-allocation operations into xhci_run() (like the
uhci driver).
I am not sure whether these ways are correct, so I only report bugs.
These bugs are found by a runtime fuzzing tool named FIZZER written by us.
Best wishes,
Jia-Ju Bai
[PATCH v3 04/15] dt-bindings: rcar-gen3-phy-usb2: Document use of usb_x1
Document the USB_X1 input and add clock-names to identify functional and USB_X1 clocks. Signed-off-by: Chris Brandt --- v3: * added clock names v2: * removed 'use_usb_x1' option * document that 'usb_x1' clock node will be detected to determine if 48MHz clock exists --- Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt index d46188f450bf..ca8a831d4273 100644 --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt @@ -28,7 +28,11 @@ Required properties: followed by the generic version. - reg: offset and length of the partial USB 2.0 Host register block. -- clocks: clock phandle and specifier pair(s). +- clocks: clock phandle and specifier pair(s). For SoCs that have a separate + dedicated USB_X1 input for the PLL, that is also listed. +- clock-names: Name of the clocks. The functional clock shall be called "fclk" + and USB_X1 shall be called "usb_x1". If only one clock is listed, + this property is not required. - #phy-cells: see phy-bindings.txt in the same directory, must be <1> (and using <0> is deprecated). -- 2.16.1
[PATCH v3 02/15] ARM: dts: rza2mevb: Add 48MHz USB clock
The RZ/A2M EVB has a 48MHz clock attached to USB_X1.
Signed-off-by: Chris Brandt
Reviewed-by: Simon Horman
---
v2:
* added reviewed-by
---
arch/arm/boot/dts/r7s9210-rza2mevb.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/r7s9210-rza2mevb.dts
b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
index 7795066d82cb..7da409170db5 100644
--- a/arch/arm/boot/dts/r7s9210-rza2mevb.dts
+++ b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
@@ -58,6 +58,11 @@
clock-frequency = <32768>;
};
+/* USB_X1 */
+&usb_x1_clk {
+ clock-frequency = <4800>;
+};
+
&pinctrl {
/* Serial Console */
scif4_pins: serial4 {
--
2.16.1
[PATCH v3 12/15] dt-bindings: usb: renesas_usbhs: Add support for r7s9210
Add support for r7s9210 (RZ/A2M) SoC Signed-off-by: Chris Brandt --- Documentation/devicetree/bindings/usb/renesas_usbhs.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt index b8acc2a994a8..11c99d079dfb 100644 --- a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt +++ b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt @@ -20,9 +20,11 @@ Required properties: - "renesas,usbhs-r8a77990" for r8a77990 (R-Car E3) compatible device - "renesas,usbhs-r8a77995" for r8a77995 (R-Car D3) compatible device - "renesas,usbhs-r7s72100" for r7s72100 (RZ/A1) compatible device + - "renesas,usbhs-r7s9210" for r7s72100 (RZ/A2) compatible device - "renesas,rcar-gen2-usbhs" for R-Car Gen2 or RZ/G1 compatible devices - "renesas,rcar-gen3-usbhs" for R-Car Gen3 or RZ/G2 compatible devices - "renesas,rza1-usbhs" for RZ/A1 compatible device + - "renesas,rza2-usbhs" for RZ/A2 compatible device When compatible with the generic version, nodes must list the SoC-specific version corresponding to the platform first followed -- 2.16.1
[PATCH v3 08/15] usb: renesas_usbhs: move flags to param
Move options from 'flags' field in private structure to param structure
where other options are already being kept.
Signed-off-by: Chris Brandt
---
drivers/usb/renesas_usbhs/common.c | 23 +++
drivers/usb/renesas_usbhs/common.h | 2 --
include/linux/usb/renesas_usbhs.h | 1 +
3 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/renesas_usbhs/common.c
b/drivers/usb/renesas_usbhs/common.c
index 0ca89de7f842..1de7a44f3415 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -43,15 +43,6 @@
* | | +---+
*/
-
-#define USBHSF_RUNTIME_PWCTRL (1 << 0)
-
-/* status */
-#define usbhsc_flags_init(p) do {(p)->flags = 0; } while (0)
-#define usbhsc_flags_set(p, b) ((p)->flags |= (b))
-#define usbhsc_flags_clr(p, b) ((p)->flags &= ~(b))
-#define usbhsc_flags_has(p, b) ((p)->flags & (b))
-
/*
* platform call back
*
@@ -479,7 +470,7 @@ static void usbhsc_hotplug(struct usbhs_priv *priv)
dev_dbg(&pdev->dev, "%s enable\n", __func__);
/* power on */
- if (usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL))
+ if (usbhs_get_dparam(priv, runtime_pwctrl))
usbhsc_power_ctrl(priv, enable);
/* bus init */
@@ -499,7 +490,7 @@ static void usbhsc_hotplug(struct usbhs_priv *priv)
usbhsc_bus_init(priv);
/* power off */
- if (usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL))
+ if (usbhs_get_dparam(priv, runtime_pwctrl))
usbhsc_power_ctrl(priv, enable);
usbhs_mod_change(priv, -1);
@@ -733,7 +724,7 @@ static int usbhs_probe(struct platform_device *pdev)
/* FIXME */
/* runtime power control ? */
if (priv->pfunc.get_vbus)
- usbhsc_flags_set(priv, USBHSF_RUNTIME_PWCTRL);
+ usbhs_get_dparam(priv, runtime_pwctrl) = 1;
/*
* priv settings
@@ -807,7 +798,7 @@ static int usbhs_probe(struct platform_device *pdev)
/* power control */
pm_runtime_enable(&pdev->dev);
- if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) {
+ if (!usbhs_get_dparam(priv, runtime_pwctrl)) {
usbhsc_power_ctrl(priv, 1);
usbhs_mod_autonomy_mode(priv);
}
@@ -848,7 +839,7 @@ static int usbhs_remove(struct platform_device *pdev)
dfunc->notify_hotplug = NULL;
/* power off */
- if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL))
+ if (!usbhs_get_dparam(priv, runtime_pwctrl))
usbhsc_power_ctrl(priv, 0);
pm_runtime_disable(&pdev->dev);
@@ -873,7 +864,7 @@ static __maybe_unused int usbhsc_suspend(struct device *dev)
usbhs_mod_change(priv, -1);
}
- if (mod || !usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL))
+ if (mod || !usbhs_get_dparam(priv, runtime_pwctrl))
usbhsc_power_ctrl(priv, 0);
return 0;
@@ -884,7 +875,7 @@ static __maybe_unused int usbhsc_resume(struct device *dev)
struct usbhs_priv *priv = dev_get_drvdata(dev);
struct platform_device *pdev = usbhs_priv_to_pdev(priv);
- if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) {
+ if (!usbhs_get_dparam(priv, runtime_pwctrl)) {
usbhsc_power_ctrl(priv, 1);
usbhs_mod_autonomy_mode(priv);
}
diff --git a/drivers/usb/renesas_usbhs/common.h
b/drivers/usb/renesas_usbhs/common.h
index de1a6638bf68..1fbffb7bbc8f 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -260,8 +260,6 @@ struct usbhs_priv {
spinlock_t lock;
- u32 flags;
-
/*
* module control
*/
diff --git a/include/linux/usb/renesas_usbhs.h
b/include/linux/usb/renesas_usbhs.h
index 53924f8e840c..17fae6e504cc 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/linux/usb/renesas_usbhs.h
@@ -189,6 +189,7 @@ struct renesas_usbhs_driver_param {
u32 has_otg:1; /* for controlling PWEN/EXTLP */
u32 has_sudmac:1; /* for SUDMAC */
u32 has_usb_dmac:1; /* for USB-DMAC */
+ u32 runtime_pwctrl:1;
#define USBHS_USB_DMAC_XFER_SIZE 32 /* hardcode the xfer size */
};
--
2.16.1
[PATCH v3 11/15] usb: renesas_usbhs: Add support for RZ/A2
The RZ/A2 is similar to the R-Car Gen3 with some small differences.
Signed-off-by: Chris Brandt
---
v3:
* Removed check for CONFIG_GENERIC_PHY
* rebase on top of Shimoda-san (v2) patch
v2:
* combined RZA1 and RZA2 for fifo setting
* added braces to make code easier to read
* fixed and clean up usbhs_rza2_power_ctrl()
---
drivers/usb/renesas_usbhs/Makefile | 2 +-
drivers/usb/renesas_usbhs/common.c | 15
drivers/usb/renesas_usbhs/rza.h| 1 +
drivers/usb/renesas_usbhs/rza2.c | 75 ++
include/linux/usb/renesas_usbhs.h | 1 +
5 files changed, 93 insertions(+), 1 deletion(-)
create mode 100644 drivers/usb/renesas_usbhs/rza2.c
diff --git a/drivers/usb/renesas_usbhs/Makefile
b/drivers/usb/renesas_usbhs/Makefile
index 5c5b51bb48ef..a1fed56b0957 100644
--- a/drivers/usb/renesas_usbhs/Makefile
+++ b/drivers/usb/renesas_usbhs/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_USB_RENESAS_USBHS)+= renesas_usbhs.o
-renesas_usbhs-y:= common.o mod.o pipe.o fifo.o rcar2.o
rcar3.o rza.o
+renesas_usbhs-y:= common.o mod.o pipe.o fifo.o rcar2.o
rcar3.o rza.o rza2.o
ifneq ($(CONFIG_USB_RENESAS_USBHS_HCD),)
renesas_usbhs-y += mod_host.o
diff --git a/drivers/usb/renesas_usbhs/common.c
b/drivers/usb/renesas_usbhs/common.c
index 734fb4e542c5..c7c9c5d75a56 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -571,6 +571,17 @@ static const struct usbhs_of_data rza1_data = {
}
};
+static const struct usbhs_of_data rza2_data = {
+ .platform_callback = &usbhs_rza2_ops,
+ .param = {
+ .type = USBHS_TYPE_RZA2,
+ .has_cnen = 1,
+ .cfifo_byte_addr = 1,
+ .pipe_configs = usbhsc_new_pipe,
+ .pipe_size = ARRAY_SIZE(usbhsc_new_pipe),
+ }
+};
+
/*
* platform functions
*/
@@ -619,6 +630,10 @@ static const struct of_device_id usbhs_of_match[] = {
.compatible = "renesas,rza1-usbhs",
.data = &rza1_data,
},
+ {
+ .compatible = "renesas,rza2-usbhs",
+ .data = &rza2_data,
+ },
{ },
};
MODULE_DEVICE_TABLE(of, usbhs_of_match);
diff --git a/drivers/usb/renesas_usbhs/rza.h b/drivers/usb/renesas_usbhs/rza.h
index ca917ca54f6d..073a53d1d442 100644
--- a/drivers/usb/renesas_usbhs/rza.h
+++ b/drivers/usb/renesas_usbhs/rza.h
@@ -2,3 +2,4 @@
#include "common.h"
extern const struct renesas_usbhs_platform_callback usbhs_rza1_ops;
+extern const struct renesas_usbhs_platform_callback usbhs_rza2_ops;
diff --git a/drivers/usb/renesas_usbhs/rza2.c b/drivers/usb/renesas_usbhs/rza2.c
new file mode 100644
index ..56409cbae33c
--- /dev/null
+++ b/drivers/usb/renesas_usbhs/rza2.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas USB driver RZ/A2 initialization and power control
+ *
+ * Copyright (C) 2019 Chris Brandt
+ * Copyright (C) 2019 Renesas Electronics Corporation
+ */
+
+#include
+#include
+#include
+#include
+#include "common.h"
+#include "rza.h"
+
+
+static int usbhs_rza2_hardware_init(struct platform_device *pdev)
+{
+ struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+ struct phy *phy = phy_get(&pdev->dev, "usb");
+
+ if (IS_ERR(phy))
+ return PTR_ERR(phy);
+
+ priv->phy = phy;
+ return 0;
+}
+
+static int usbhs_rza2_hardware_exit(struct platform_device *pdev)
+{
+ struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+
+ if (priv->phy) {
+ phy_put(priv->phy);
+ priv->phy = NULL;
+ }
+
+ return 0;
+}
+
+static int usbhs_rza2_power_ctrl(struct platform_device *pdev,
+ void __iomem *base, int enable)
+{
+ struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+ int retval = 0;
+
+ if (!priv->phy)
+ return -ENODEV;
+
+ if (enable) {
+ retval = phy_init(priv->phy);
+ usbhs_bset(priv, SUSPMODE, SUSPM, SUSPM);
+ udelay(100);/* Wait for PLL to become stable */
+ if (!retval)
+ retval = phy_power_on(priv->phy);
+ } else {
+ usbhs_bset(priv, SUSPMODE, SUSPM, 0);
+ phy_power_off(priv->phy);
+ phy_exit(priv->phy);
+ }
+
+ return retval;
+}
+
+static int usbhs_rza2_get_id(struct platform_device *pdev)
+{
+ return USBHS_GADGET;
+}
+
+const struct renesas_usbhs_platform_callback usbhs_rza2_ops = {
+ .hardware_init = usbhs_rza2_hardware_init,
+ .hardware_exit = usbhs_rza2_hardware_exit,
+ .power_ctrl = usbhs_rza2_power_ctrl,
+ .get_id = usbhs_rza2_get_id,
+};
diff --git a/include/linux/usb/renesas_usbhs.h
b/include/linux/usb/renesas_usbhs.h
index 87043fd21d54..3f53043fb56b 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/li
[PATCH v3 00/15] usb: Add host and device support for RZ/A2
NOTE:
This series requires the follow patch from Shimoda-san.
[PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_*
enums
For the most part, the RZ/A2 has the same USB 2.0 host and device
HW as the R-Car Gen3, so we can reuse a lot of the code.
However, there are a couple extra register bits, and the CFIFO
register 8-bit access works a little different.
There is a dedicated DMAC for the RZ/A2 USB Device HW, but we
have not been able to reliably get that working yet, so device
operation is pio only at the moment.
On the RZ/A2M eval board, both USB channels can be used as either
host or device. But, it's not set up for otg (ie, there are jumpers
and separate connectors). Therefore, below is an example of what it
would look like to enable USB channel 0 as a device instead of a host.
&usb2_phy0 {
pinctrl-names = "default";
pinctrl-0 = <&usb0_pins>;
dr_mode = "peripheral";
status = "okay";
};
&usbhs0 {
status = "okay";
};
Chris Brandt (15):
ARM: dts: r7s9210: Add USB clock
ARM: dts: rza2mevb: Add 48MHz USB clock
phy: renesas: rcar-gen3-usb2: detect usb_x1 clock
dt-bindings: rcar-gen3-phy-usb2: Document use of usb_x1
phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG
dt-bindings: rcar-gen3-phy-usb2: Document dr_mode
dt-bindings: rcar-gen3-phy-usb2: Add r7s9210 support
usb: renesas_usbhs: move flags to param
usb: renesas_usbhs: add support for CNEN bit
usb: renesas_usbhs: support byte addressable CFIFO
usb: renesas_usbhs: Add support for RZ/A2
dt-bindings: usb: renesas_usbhs: Add support for r7s9210
ARM: dts: r7s9210: Add USB Host support
ARM: dts: r7s9210: Add USB Device support
ARM: dts: rza2mevb: Add USB host support
.../devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 19 +++--
.../devicetree/bindings/usb/renesas_usbhs.txt | 2 +
arch/arm/boot/dts/r7s9210-rza2mevb.dts | 42 ++
arch/arm/boot/dts/r7s9210.dtsi | 97 ++
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 26 ++
drivers/usb/renesas_usbhs/Makefile | 2 +-
drivers/usb/renesas_usbhs/common.c | 44 ++
drivers/usb/renesas_usbhs/common.h | 3 +-
drivers/usb/renesas_usbhs/fifo.c | 9 +-
drivers/usb/renesas_usbhs/rza.h| 1 +
drivers/usb/renesas_usbhs/rza2.c | 75 +
include/linux/usb/renesas_usbhs.h | 4 +
12 files changed, 298 insertions(+), 26 deletions(-)
create mode 100644 drivers/usb/renesas_usbhs/rza2.c
--
2.16.1
[PATCH v3 06/15] dt-bindings: rcar-gen3-phy-usb2: Document dr_mode
Document the optional dr_mode property Signed-off-by: Chris Brandt --- Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt index ca8a831d4273..d42e180d29b8 100644 --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt @@ -50,6 +50,9 @@ channel as USB OTG: regulator will be managed during the PHY power on/off sequence. - renesas,no-otg-pins: boolean, specify when a board does not provide proper otg pins. +- dr_mode: string, indicates the working mode for the PHY. Can be "host", + "peripheral", or "otg". Should be set if otg controller is not used. + Example (R-Car H3): -- 2.16.1
[PATCH v3 15/15] ARM: dts: rza2mevb: Add USB host support
Enable USB Host support for both the Type-C connector on the CPU board
and the Type-A plug on the sub board.
Both boards are also capable of USB Device operation as well after the
appropriate Device Tree modifications.
Signed-off-by: Chris Brandt
---
v2:
* added blank line between nodes
* removed 'r7s9210-' from patch title
* removed 'renesas,uses_usb_x1' property
---
arch/arm/boot/dts/r7s9210-rza2mevb.dts | 37 ++
1 file changed, 37 insertions(+)
diff --git a/arch/arm/boot/dts/r7s9210-rza2mevb.dts
b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
index 7da409170db5..c0a4484a0bde 100644
--- a/arch/arm/boot/dts/r7s9210-rza2mevb.dts
+++ b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
@@ -107,6 +107,18 @@
pinmux = ,/* SD1_CD */
;/* SD1_WP */
};
+
+ usb0_pins: usb0 {
+ pinmux = ,/* VBUSIN0 */
+,/* VBUSEN0 */
+;/* OVRCUR0 */
+ };
+
+ usb1_pins: usb1 {
+ pinmux = ,/* VBUSIN1 */
+,/* VBUSEN1 */
+;/* OVRCUR1 */
+ };
};
/* High resolution System tick timers */
@@ -161,3 +173,28 @@
bus-width = <4>;
status = "okay";
};
+
+/* USB-0 as Host */
+/* NOTE: Requires JP3 to be fitted */
+&usb2_phy0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&usb0_pins>;
+ dr_mode = "host";
+ status = "okay";
+};
+
+&ehci0 {
+ status = "okay";
+};
+
+/* USB-1 as Host */
+&usb2_phy1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&usb1_pins>;
+ dr_mode = "host";
+ status = "okay";
+};
+
+&ehci1 {
+ status = "okay";
+};
--
2.16.1
[PATCH v3 10/15] usb: renesas_usbhs: support byte addressable CFIFO
Some SoC have a CFIFO register that is byte addressable. This means
when the CFIFO access is set to 32-bit, you can write 8-bit values to
addresses CFIFO+0, CFIFO+1, CFIFO+2, CFIFO+3.
Signed-off-by: Chris Brandt
---
v2:
* options ahve moved from flags to param
---
drivers/usb/renesas_usbhs/fifo.c | 9 +++--
include/linux/usb/renesas_usbhs.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 39fa2fc1b8b7..452b456ac24e 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -543,8 +543,13 @@ static int usbhsf_pio_try_push(struct usbhs_pkt *pkt, int
*is_done)
}
/* the rest operation */
- for (i = 0; i < len; i++)
- iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
+ if (usbhs_get_dparam(priv, cfifo_byte_addr)) {
+ for (i = 0; i < len; i++)
+ iowrite8(buf[i], addr + (i & 0x03));
+ } else {
+ for (i = 0; i < len; i++)
+ iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
+ }
/*
* variable update
diff --git a/include/linux/usb/renesas_usbhs.h
b/include/linux/usb/renesas_usbhs.h
index 9097a38fcda8..87043fd21d54 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/linux/usb/renesas_usbhs.h
@@ -191,6 +191,7 @@ struct renesas_usbhs_driver_param {
u32 has_usb_dmac:1; /* for USB-DMAC */
u32 runtime_pwctrl:1;
u32 has_cnen:1;
+ u32 cfifo_byte_addr:1; /* CFIFO is byte addressable */
#define USBHS_USB_DMAC_XFER_SIZE 32 /* hardcode the xfer size */
};
--
2.16.1
[PATCH v3 13/15] ARM: dts: r7s9210: Add USB Host support
Add EHCI and OHCI host support for RZ/A2.
Signed-off-by: Chris Brandt
---
v3:
* add usb_x1 as a clock source
* add clock-names
v2:
* changed to generic name usb@xxx
* Add space between compatible strings
---
arch/arm/boot/dts/r7s9210.dtsi | 66 ++
1 file changed, 66 insertions(+)
diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
index 73041f04fef5..c3a50206ff86 100644
--- a/arch/arm/boot/dts/r7s9210.dtsi
+++ b/arch/arm/boot/dts/r7s9210.dtsi
@@ -329,6 +329,72 @@
status = "disabled";
};
+ ohci0: usb@e8218000 {
+ compatible = "generic-ohci";
+ reg = <0xe8218000 0x100>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 61>;
+ phys = <&usb2_phy0>;
+ phy-names = "usb";
+ power-domains = <&cpg>;
+ status = "disabled";
+ };
+
+ ehci0: usb@e8218100 {
+ compatible = "generic-ehci";
+ reg = <0xe8218100 0x100>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 61>;
+ phys = <&usb2_phy0>;
+ phy-names = "usb";
+ power-domains = <&cpg>;
+ status = "disabled";
+ };
+
+ usb2_phy0: usb-phy@e8218200 {
+ compatible = "renesas,usb2-phy-r7s9210",
"renesas,rcar-gen3-usb2-phy";
+ reg = <0xe8218200 0x10>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 61>, <&usb_x1_clk>;
+ clock-names = "fclk", "usb_x1";
+ power-domains = <&cpg>;
+ #phy-cells = <0>;
+ status = "disabled";
+ };
+
+ ohci1: usb@e821a000 {
+ compatible = "generic-ohci";
+ reg = <0xe821a000 0x100>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 60>;
+ phys = <&usb2_phy1>;
+ phy-names = "usb";
+ power-domains = <&cpg>;
+ status = "disabled";
+ };
+
+ ehci1: usb@e821a100 {
+ compatible = "generic-ehci";
+ reg = <0xe821a100 0x100>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 60>;
+ phys = <&usb2_phy1>;
+ phy-names = "usb";
+ power-domains = <&cpg>;
+ status = "disabled";
+ };
+
+ usb2_phy1: usb-phy@e821a200 {
+ compatible = "renesas,usb2-phy-r7s9210",
"renesas,rcar-gen3-usb2-phy";
+ reg = <0xe821a200 0x10>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 60>, <&usb_x1_clk>;
+ clock-names = "fclk", "usb_x1";
+ power-domains = <&cpg>;
+ #phy-cells = <0>;
+ status = "disabled";
+ };
+
sdhi0: sd@e8228000 {
compatible = "renesas,sdhi-r7s9210";
reg = <0xe8228000 0x8c0>;
--
2.16.1
[PATCH v3 01/15] ARM: dts: r7s9210: Add USB clock
Add USB clock node. If present, this clock input must be 48MHz.
Signed-off-by: Chris Brandt
Reviewed-by: Simon Horman
---
v2:
* added reviewed-by
---
arch/arm/boot/dts/r7s9210.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
index 2eaa5eeba509..73041f04fef5 100644
--- a/arch/arm/boot/dts/r7s9210.dtsi
+++ b/arch/arm/boot/dts/r7s9210.dtsi
@@ -30,6 +30,13 @@
clock-frequency = <0>;
};
+ usb_x1_clk: usb_x1 {
+ #clock-cells = <0>;
+ compatible = "fixed-clock";
+ /* If clk present, value (4800) must be set by board */
+ clock-frequency = <0>;
+ };
+
cpus {
#address-cells = <1>;
#size-cells = <0>;
--
2.16.1
[PATCH v3 03/15] phy: renesas: rcar-gen3-usb2: detect usb_x1 clock
The RZ/A2 has an optional dedicated 48MHz clock input for the PLL.
If a clock node named 'usb_x1' exists and set to non-zero, then we can
assume we want it use it.
Signed-off-by: Chris Brandt
---
v3:
* avoid magic number
* use devm_clk_get and clk_get_rate
v2:
* use 'usb_x1' clock node instead of 'renesas,uses_usb_x1' property
---
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 1322185a00a2..06e0fc804226 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -34,6 +35,7 @@
#define USB2_VBCTRL0x60c
#define USB2_LINECTRL1 0x610
#define USB2_ADPCTRL 0x630
+#define USB2_PHYCLK_CTRL 0x644
/* INT_ENABLE */
#define USB2_INT_ENABLE_UCOM_INTEN BIT(3)
@@ -75,6 +77,9 @@
#define USB2_ADPCTRL_IDPULLUP BIT(5) /* 1 = ID sampling is enabled */
#define USB2_ADPCTRL_DRVVBUS BIT(4)
+/* PHYCLK_CTRL */
+#define PHYCLK_CTRL_UCLKSELBIT(0)
+
#define NUM_OF_PHYS4
enum rcar_gen3_phy_index {
PHY_INDEX_BOTH_HC,
@@ -110,6 +115,7 @@ struct rcar_gen3_chan {
bool extcon_host;
bool is_otg_channel;
bool uses_otg_pins;
+ bool uses_usb_x1;
};
/*
@@ -391,6 +397,9 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
void __iomem *usb2_base = channel->base;
u32 val;
+ if (channel->uses_usb_x1)
+ writel(PHYCLK_CTRL_UCLKSEL, usb2_base + USB2_PHYCLK_CTRL);
+
/* Initialize USB2 part */
val = readl(usb2_base + USB2_INT_ENABLE);
val |= USB2_INT_ENABLE_UCOM_INTEN | rphy->int_enable_bits;
@@ -583,6 +592,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device
*pdev)
struct device *dev = &pdev->dev;
struct rcar_gen3_chan *channel;
struct phy_provider *provider;
+ struct clk *usb_x1_clk;
struct resource *res;
const struct phy_ops *phy_usb2_ops;
int irq, ret = 0, i;
@@ -630,6 +640,10 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device
*pdev)
}
}
+ usb_x1_clk = devm_clk_get(dev, "usb_x1");
+ if (!IS_ERR(usb_x1_clk) && clk_get_rate(usb_x1_clk))
+ channel->uses_usb_x1 = true;
+
/*
* devm_phy_create() will call pm_runtime_enable(&phy->dev);
* And then, phy-core will manage runtime pm for this device.
--
2.16.1
[PATCH v3 07/15] dt-bindings: rcar-gen3-phy-usb2: Add r7s9210 support
Document RZ/A2 (R7S9210) SoC bindings. Signed-off-by: Chris Brandt --- Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt index d42e180d29b8..2eef669e78e3 100644 --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt @@ -1,10 +1,12 @@ * Renesas R-Car generation 3 USB 2.0 PHY This file provides information on what the device node for the R-Car generation -3, RZ/G1C and RZ/G2 USB 2.0 PHY contain. +3, RZ/G1C, RZ/G2 and RZ/A2 USB 2.0 PHY contain. Required properties: -- compatible: "renesas,usb2-phy-r8a77470" if the device is a part of an R8A77470 +- compatible: "renesas,usb2-phy-r7s9210" if the device is a part of an R7S9210 + SoC. + "renesas,usb2-phy-r8a77470" if the device is a part of an R8A77470 SoC. "renesas,usb2-phy-r8a774a1" if the device is a part of an R8A774A1 SoC. @@ -20,8 +22,8 @@ Required properties: R8A77990 SoC. "renesas,usb2-phy-r8a77995" if the device is a part of an R8A77995 SoC. - "renesas,rcar-gen3-usb2-phy" for a generic R-Car Gen3 or RZ/G2 - compatible device. + "renesas,rcar-gen3-usb2-phy" for a generic R-Car Gen3, RZ/G2 or + RZ/A2 compatible device. When compatible with the generic version, nodes must list the SoC-specific version corresponding to the platform first -- 2.16.1
[PATCH v3 14/15] ARM: dts: r7s9210: Add USB Device support
Add USB Device support for RZ/A2.
Signed-off-by: Chris Brandt
---
v2:
* changed to generic name usb@xxx
* Add space between compatible strings
---
arch/arm/boot/dts/r7s9210.dtsi | 24
1 file changed, 24 insertions(+)
diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
index c3a50206ff86..65a8d5b126b4 100644
--- a/arch/arm/boot/dts/r7s9210.dtsi
+++ b/arch/arm/boot/dts/r7s9210.dtsi
@@ -362,6 +362,18 @@
status = "disabled";
};
+ usbhs0: usb@e8219000 {
+ compatible = "renesas,usbhs-r7s9210",
"renesas,rza2-usbhs";
+ reg = <0xe8219000 0x724>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 61>;
+ renesas,buswait = <7>;
+ phys = <&usb2_phy0>;
+ phy-names = "usb";
+ power-domains = <&cpg>;
+ status = "disabled";
+ };
+
ohci1: usb@e821a000 {
compatible = "generic-ohci";
reg = <0xe821a000 0x100>;
@@ -395,6 +407,18 @@
status = "disabled";
};
+ usbhs1: usb@e821b000 {
+ compatible = "renesas,usbhs-r7s9210",
"renesas,rza2-usbhs";
+ reg = <0xe821b000 0x724>;
+ interrupts = ;
+ clocks = <&cpg CPG_MOD 60>;
+ renesas,buswait = <7>;
+ phys = <&usb2_phy1>;
+ phy-names = "usb";
+ power-domains = <&cpg>;
+ status = "disabled";
+ };
+
sdhi0: sd@e8228000 {
compatible = "renesas,sdhi-r7s9210";
reg = <0xe8228000 0x8c0>;
--
2.16.1
Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework
+Fredrik, Juergen On 14/05/2019 15:38, laurentiu.tu...@nxp.com wrote: From: Laurentiu Tudor For HCs that have local memory, replace the current DMA API usage with a genalloc generic allocator to manage the mappings for these devices. This is in preparation for dropping the existing "coherent" dma mem declaration APIs. Current implementation was relying on a short circuit in the DMA API that in the end, was acting as an allocator for these type of devices. Only compiled tested, so any volunteers willing to test are most welcome. I recall an out-of-tree PlayStation 2 OHCI driver being another HCD_LOCAL_MEM user - if Fredrik and Juergen are still active on that, hopefully they might be able to comment on whether this approach can work for them too. Patchwork link just in case: https://patchwork.kernel.org/project/linux-usb/list/?series=117433 Robin. Thank you! For context, see thread here: https://lkml.org/lkml/2019/4/22/357 Changes in v2: - use genalloc also in core usb (based on comment from Robin) - moved genpool decl to usb_hcd to be accesible from core usb Laurentiu Tudor (3): USB: use genalloc for USB HCs with local memory usb: host: ohci-sm501: init genalloc for local memory usb: host: ohci-tmio: init genalloc for local memory drivers/usb/core/buffer.c | 12 ++- drivers/usb/host/ohci-hcd.c | 23 +++--- drivers/usb/host/ohci-sm501.c | 60 +++ drivers/usb/host/ohci-tmio.c | 23 +- include/linux/usb/hcd.h | 3 ++ 5 files changed, 80 insertions(+), 41 deletions(-)
[PATCH v3 09/15] usb: renesas_usbhs: add support for CNEN bit
For some SoC, CNEN must be set for USB Device mode operation.
Signed-off-by: Chris Brandt
---
v2:
* options are now held in param
---
drivers/usb/renesas_usbhs/common.c | 6 ++
drivers/usb/renesas_usbhs/common.h | 1 +
include/linux/usb/renesas_usbhs.h | 1 +
3 files changed, 8 insertions(+)
diff --git a/drivers/usb/renesas_usbhs/common.c
b/drivers/usb/renesas_usbhs/common.c
index 1de7a44f3415..734fb4e542c5 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -114,6 +114,12 @@ void usbhs_sys_function_ctrl(struct usbhs_priv *priv, int
enable)
u16 mask = DCFM | DRPD | DPRPU | HSE | USBE;
u16 val = HSE | USBE;
+ /* CNEN bit is required for function operation */
+ if (usbhs_get_dparam(priv, has_cnen)) {
+ mask |= CNEN;
+ val |= CNEN;
+ }
+
/*
* if enable
*
diff --git a/drivers/usb/renesas_usbhs/common.h
b/drivers/usb/renesas_usbhs/common.h
index 1fbffb7bbc8f..de74ebd1a347 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -104,6 +104,7 @@ struct usbhs_priv;
/* SYSCFG */
#define SCKE (1 << 10) /* USB Module Clock Enable */
+#define CNEN (1 << 8)/* Single-ended receiver operation Enable */
#define HSE(1 << 7)/* High-Speed Operation Enable */
#define DCFM (1 << 6)/* Controller Function Select */
#define DRPD (1 << 5)/* D+ Line/D- Line Resistance Control */
diff --git a/include/linux/usb/renesas_usbhs.h
b/include/linux/usb/renesas_usbhs.h
index 17fae6e504cc..9097a38fcda8 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/linux/usb/renesas_usbhs.h
@@ -190,6 +190,7 @@ struct renesas_usbhs_driver_param {
u32 has_sudmac:1; /* for SUDMAC */
u32 has_usb_dmac:1; /* for USB-DMAC */
u32 runtime_pwctrl:1;
+ u32 has_cnen:1;
#define USBHS_USB_DMAC_XFER_SIZE 32 /* hardcode the xfer size */
};
--
2.16.1
[PATCH v3 05/15] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG
When not using OTG, the PHY will need to know if it should function as
host or peripheral by checking dr_mode in the PHY node (not the parent
controller node).
Signed-off-by: Chris Brandt
---
v3:
* changed 'if' to 'switch'
* use rcar_gen3_set_host_mode() instead of writel()
v2:
* added braces to else statement
* check if dr_mode is "host"
---
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 06e0fc804226..29da9c46ad9b 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -412,6 +412,18 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
if (rcar_gen3_needs_init_otg(channel))
rcar_gen3_init_otg(channel);
rphy->otg_initialized = true;
+ } else {
+ /* Not OTG, so dr_mode should be set in PHY node */
+ switch (usb_get_dr_mode(channel->dev)) {
+ case USB_DR_MODE_HOST:
+ rcar_gen3_set_host_mode(channel, 1);
+ break;
+ case USB_DR_MODE_PERIPHERAL:
+ rcar_gen3_set_host_mode(channel, 0);
+ break;
+ default:
+ break;
+ }
}
rphy->initialized = true;
--
2.16.1
Re: [BUG] usb: xhci: Possible resource leaks when xhci_run() fails
On Tue, May 14, 2019 at 10:58:05PM +0800, Jia-Ju Bai wrote:
> xhci_pci_setup() is assigned to hc_driver.reset;
> xhci_run() is assigned to hc_driver.start();
> xhci_stop() is assigned to hc_driver.stop().
>
> xhci_pci_setup() calls xhci_gen_setup, which calls xhci_init(). And
> xhci_init() calls xhci_mem_init() to allocate resources.
>
> xhci_stop() calls xhci_mem_cleanup(), to release the resources allocated in
> xhci_mem_init() (also namely xhci_pci_setup()).
>
> xhci_run() can fail, because xhci_try_enable_msi() or xhci_alloc_command()
> in this function can fail.
>
> In drivers/usb/core/hcd.c:
> retval = hcd->driver->reset(hcd);
> if (retval < 0) {
> ..
> goto err_hcd_driver_setup;
> }
> ..
> retval = hcd->driver->start(hcd);
> if (retval < 0) {
> ..
> goto err_hcd_driver_start;
> }
> ...
> hcd->driver->stop(hcd);
> hcd->state = HC_STATE_HALT;
> clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> del_timer_sync(&hcd->rh_timer);
> err_hcd_driver_start:
> if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
> free_irq(irqnum, hcd);
> err_request_irq:
> err_hcd_driver_setup:
> err_set_rh_speed:
> usb_put_invalidate_rhdev(hcd);
> err_allocate_root_hub:
> usb_deregister_bus(&hcd->self);
> err_register_bus:
> hcd_buffer_destroy(hcd);
> err_create_buf:
> usb_phy_roothub_power_off(hcd->phy_roothub);
> err_usb_phy_roothub_power_on:
> usb_phy_roothub_exit(hcd->phy_roothub);
>
> Thus, when hcd->driver->reset() succeeds and hcd->driver->start() fails,
> hcd->driver->stop() is not called.
>
> Namely, when xhci_pci_setup() successfully allocates resources, and
> xhci_run() fails, xhci_stop() is not called to release the resources.
> For this reason, resource leaks occur in this case.
>
> I check the code of the ehci driver, uhci driver and ohci driver, and find
> that they do not have such problem, because:
> In the ehci driver, ehci_run() (namely hcd->driver->start()) never fails.
> In the uhci driver, all the resources are allocated in uhci_start (namely
> hcd->driver->start()), and no resources are allocated in uhci_pci_init()
> (namely hcd->driver->reset()).
> In the ohci driver, ohci_setup() (namely hcd->driver->reset()) also
> allocates resources. But when ohci_start() (namely hcd->driver->start()) is
> going to fail, ohci_stop() is directly called to release the resources
> allocated by ohci_setup().
>
> Thus, there are two possible ways of fixing bugs:
> 1) Call xhci_stop() when xhci_run() is going to fail (like the ohci driver)
> 2) Move all resource-allocation operations into xhci_run() (like the uhci
> driver).
>
> I am not sure whether these ways are correct, so I only report bugs.
Can you create a patch to show how you would fix this potential issue?
Given that making this type of thing fail is pretty rare, it's not a
real high priority to get to, so it might be a while for anyone here to
look at it.
thanks,
greg k-h
Re: [PATCH v5 2/6] dt-bindings: usb: add binding for Type-B GPIO connector driver
On Tue, May 14, 2019 at 04:47:19PM +0800, Chunfeng Yun wrote:
> It's used to support dual role switch via GPIO when use Type-B
> receptacle, typically the USB ID pin is connected to an input
> GPIO pin
>
> Signed-off-by: Chunfeng Yun
> ---
> v5 changes:
> 1. treat type-B connector as child device of USB controller's, but not
> as a separate virtual device, suggested by Rob
> 2. put connector's port node under connector node, suggested by Rob
>
> v4 no changes
>
> v3 changes:
> 1. treat type-B connector as a virtual device, but not child device of
> USB controller's
>
> v2 changes:
> 1. new patch to make binding clear suggested by Hans
> ---
> .../bindings/usb/typeb-conn-gpio.txt | 42 +++
> 1 file changed, 42 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> new file mode 100644
> index ..20dd3499a348
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> @@ -0,0 +1,42 @@
> +USB Type-B GPIO Connector
> +
> +This is used to switch dual role mode from the USB ID pin connected to
> +an input GPIO pin.
> +
> +Required properties:
> +- compatible : should include "linux,typeb-conn-gpio" and "usb-b-connector".
I don't think we need "linux,typeb-conn-gpio". A driver can decide to
handle GPIO lines if they present or we assume the parent device handles
ID and/or Vbus if they are not present.
> +- id-gpios, vbus-gpios : either one of them must be present, and both
> + can be present as well.
Please clarify that vbus-gpios is an input to sense Vbus presence as an
output it should be modelled as a regulator only.
These should be added to usb-connector.txt.
The result of all this is you don't need this file. Just additions to
usb-connector.txt.
> +- vbus-supply : can be present if needed when supports dual role mode or
> + host mode.
> + see connector/usb-connector.txt
> +
> +Sub-nodes:
> +- port : should be present.
> + see graph.txt
> +
> +Example:
> +
> +&mtu3 {
> + status = "okay";
Don't show status in examples.
> +
> + connector {
> + compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> + label = "micro-USB";
> + type = "micro";
> + id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> + vbus-supply = <&usb_p0_vbus>;
> +
> + port {
> + bconn_ep: endpoint@0 {
> + remote-endpoint = <&usb_role_sw>;
> + };
> + };
> + };
> +
> + port {
> + usb_role_sw: endpoint@0 {
> + remote-endpoint = <&bconn_ep>;
> + };
> + };
When the host controller is the parent of the connector, you don't need
the graph unless you're describing the alternate modes in Type-C.
> +};
> --
> 2.21.0
>
Re: [PATCH v5 3/6] dt-bindings: usb: mtu3: add properties about USB Role Switch
On Tue, 14 May 2019 16:47:20 +0800, Chunfeng Yun wrote: > Now the USB Role Switch is supported, so add properties about it, > and modify some description related. > > Signed-off-by: Chunfeng Yun > --- > v5 changes > 1. modify decription about extcon and vbus-supply properties > 2. make this patch depend on [1] > > [1]: [v3] dt-binding: usb: add usb-role-switch property > https://patchwork.kernel.org/patch/10934835/ > > v4: no changes > v3: no changes > > v2 changes: > 1. fix typo > 2. refer new binding about connector property > --- > .../devicetree/bindings/usb/mediatek,mtu3.txt | 10 ++ > 1 file changed, 10 insertions(+) > Reviewed-by: Rob Herring
Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework
Thanks Robin! > > For HCs that have local memory, replace the current DMA API usage > > with a genalloc generic allocator to manage the mappings for these > > devices. > > This is in preparation for dropping the existing "coherent" dma > > mem declaration APIs. Current implementation was relying on a short > > circuit in the DMA API that in the end, was acting as an allocator > > for these type of devices. > > > > Only compiled tested, so any volunteers willing to test are most welcome. > > I recall an out-of-tree PlayStation 2 OHCI driver being another > HCD_LOCAL_MEM user - if Fredrik and Juergen are still active on that, > hopefully they might be able to comment on whether this approach can > work for them too. Patchwork link just in case: > https://patchwork.kernel.org/project/linux-usb/list/?series=117433 True. In fact I'm preparing a patch submission for this PS2 OHCI driver, along with about a hundred other patches (unrelated to the USB subsystem). Hopefully in a few weeks. My patches are currently on top of v5.0. What branch/version is recommended to try this DMA update? Here is the v5.0.11 PS2 OHCI driver, for reference: https://github.com/frno7/linux/blob/ps2-v5.0.11/drivers/usb/host/ohci-ps2.c Fredrik
Re: [PATCH v3 04/15] dt-bindings: rcar-gen3-phy-usb2: Document use of usb_x1
On Tue, 14 May 2019 09:55:54 -0500, Chris Brandt wrote: > Document the USB_X1 input and add clock-names to identify > functional and USB_X1 clocks. > > Signed-off-by: Chris Brandt > --- > v3: > * added clock names > v2: > * removed 'use_usb_x1' option > * document that 'usb_x1' clock node will be detected to determine if >48MHz clock exists > --- > Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring
Re: [PATCH v3 06/15] dt-bindings: rcar-gen3-phy-usb2: Document dr_mode
On Tue, 14 May 2019 09:55:56 -0500, Chris Brandt wrote: > Document the optional dr_mode property > > Signed-off-by: Chris Brandt > --- > Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 3 +++ > 1 file changed, 3 insertions(+) > Reviewed-by: Rob Herring
Re: [PATCH v3 07/15] dt-bindings: rcar-gen3-phy-usb2: Add r7s9210 support
On Tue, 14 May 2019 09:55:57 -0500, Chris Brandt wrote: > Document RZ/A2 (R7S9210) SoC bindings. > > Signed-off-by: Chris Brandt > --- > Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > Reviewed-by: Rob Herring
Re: [PATCH v3 12/15] dt-bindings: usb: renesas_usbhs: Add support for r7s9210
On Tue, 14 May 2019 09:56:02 -0500, Chris Brandt wrote: > Add support for r7s9210 (RZ/A2M) SoC > > Signed-off-by: Chris Brandt > --- > Documentation/devicetree/bindings/usb/renesas_usbhs.txt | 2 ++ > 1 file changed, 2 insertions(+) > Reviewed-by: Rob Herring
[PATCH 2/2] usb: core: hub: Disable hub-initiated U1/U2
If the device rejects the control transfer to enable device-initiated
U1/U2 entry, then the device will not initiate U1/U2 transition. To
improve the performance, the downstream port should not initate
transition to U1/U2 to avoid the delay from the device link command
response (no packet can be transmitted while waiting for a response from
the device). If the device has some quirks and does not implement U1/U2,
it may reject all the link state change requests, and the downstream
port may resend and flood the bus with more requests. This will affect
the device performance even further. This patch disables the
hub-initated U1/U2 if the device-initiated U1/U2 entry fails.
Reference: USB 3.2 spec 7.2.4.2.3
Signed-off-by: Thinh Nguyen
---
drivers/usb/core/hub.c | 28
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 026b652d4f38..572e8c26a129 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3999,6 +3999,9 @@ static int usb_set_lpm_timeout(struct usb_device *udev,
* control transfers to set the hub timeout or enable device-initiated U1/U2
* will be successful.
*
+ * If the control transfer to enable device-initiated U1/U2 entry fails, then
+ * hub-initiated U1/U2 will be disabled.
+ *
* If we cannot set the parent hub U1/U2 timeout, we attempt to let the xHCI
* driver know about it. If that call fails, it should be harmless, and just
* take up more slightly more bus bandwidth for unnecessary U1/U2 exit latency.
@@ -4053,23 +4056,24 @@ static void usb_enable_link_state(struct usb_hcd *hcd,
struct usb_device *udev,
* host know that this link state won't be enabled.
*/
hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
- } else {
- /* Only a configured device will accept the Set Feature
-* U1/U2_ENABLE
-*/
- if (udev->actconfig)
- usb_set_device_initiated_lpm(udev, state, true);
+ return;
+ }
- /* As soon as usb_set_lpm_timeout(timeout) returns 0, the
-* hub-initiated LPM is enabled. Thus, LPM is enabled no
-* matter the result of usb_set_device_initiated_lpm().
-* The only difference is whether device is able to initiate
-* LPM.
-*/
+ /* Only a configured device will accept the Set Feature
+* U1/U2_ENABLE
+*/
+ if (udev->actconfig &&
+ usb_set_device_initiated_lpm(udev, state, true) == 0) {
if (state == USB3_LPM_U1)
udev->usb3_lpm_u1_enabled = 1;
else if (state == USB3_LPM_U2)
udev->usb3_lpm_u2_enabled = 1;
+ } else {
+ /* Don't request U1/U2 entry if the device
+* cannot transition to U1/U2.
+*/
+ usb_set_lpm_timeout(udev, state, 0);
+ hcd->driver->disable_usb3_lpm_timeout(hcd, udev, state);
}
}
--
2.11.0
[PATCH 1/2] usb: core: hub: Enable/disable U1/U2 in configured state
SET_FEATURE(U1/U2_ENABLE) and CLEAR_FEATURE(U1/U2) only apply while the device is in configured state. Add proper check in usb_disable_lpm() and usb_enable_lpm() for enabling/disabling device-initiated U1/U2. Signed-off-by: Thinh Nguyen --- drivers/usb/core/hub.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 2f94568ba385..026b652d4f38 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4139,7 +4139,7 @@ int usb_disable_lpm(struct usb_device *udev) if (!udev || !udev->parent || udev->speed < USB_SPEED_SUPER || !udev->lpm_capable || - udev->state < USB_STATE_DEFAULT) + udev->state < USB_STATE_CONFIGURED) return 0; hcd = bus_to_hcd(udev->bus); @@ -4198,7 +4198,7 @@ void usb_enable_lpm(struct usb_device *udev) if (!udev || !udev->parent || udev->speed < USB_SPEED_SUPER || !udev->lpm_capable || - udev->state < USB_STATE_DEFAULT) + udev->state < USB_STATE_CONFIGURED) return; udev->lpm_disable_count--; -- 2.11.0
Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries
Hi Anurag,
Anurag Kumar Vulisha wrote:
> Hi Thinh,
>
>> -Original Message-
>> From: Thinh Nguyen [mailto:thinh.ngu...@synopsys.com]
>> Sent: Saturday, May 11, 2019 7:18 AM
>> To: Anurag Kumar Vulisha ; Thinh Nguyen
>> ; Greg Kroah-Hartman
>> ; Rob Herring ; Mark Rutland
>> ; Felipe Balbi ; Claus H. Stovgaard
>>
>> Cc: linux-usb@vger.kernel.org; devicet...@vger.kernel.org; linux-
>> ker...@vger.kernel.org; v.anuragku...@gmail.com
>> Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling U1
>> and U2
>> entries
>>
>> Hi,
>>
>> Anurag Kumar Vulisha wrote:
>>> Hi Thinh,
>>>
-Original Message-
From: Thinh Nguyen [mailto:thinh.ngu...@synopsys.com]
Sent: Friday, May 10, 2019 5:30 AM
To: Anurag Kumar Vulisha ; Thinh Nguyen
; Greg Kroah-Hartman
; Rob Herring ; Mark
Rutland
; Felipe Balbi ; Claus H. Stovgaard
Cc: linux-usb@vger.kernel.org; devicet...@vger.kernel.org; linux-
ker...@vger.kernel.org; v.anuragku...@gmail.com
Subject: Re: [PATCH v2 3/3] usb: dwc3: gadget: Add support for disabling
U1 and
>> U2
entries
Hi Anurag,
Anurag Kumar Vulisha wrote:
>>> + return -EINVAL;
>>>
>>> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>> if (set)
>>> @@ -626,7 +630,10 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc,
struct
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index e293400..f2d3112 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>> return 0;
>>> }
>>>
>>> +static void dwc3_gadget_config_params(struct usb_gadget *g,
>>> + struct usb_dcd_config_params
>>> *params)
>>> +{
>>> + struct dwc3 *dwc = gadget_to_dwc(g);
>>> +
>>> + /* U1 Device exit Latency */
>>> + if (dwc->dis_u1_entry_quirk)
>>> + params->bU1devExitLat = 0;
>> It doesn't make sense to have exit latency of 0. Rejecting
>> SET_FEATURE(enable U1/U2) should already let the host know that the
>> device doesn't support U1/U2.
>>
> I am okay to remove this, but I feel that it is better to report zero
> value instead
> of a non-zero value in exit latency of BOS when U1 or U2 entries are not
>> supported.
> Advantage of reporting 0 is that some hosts doesn't even send
SET_FEATURE(U1/U2)
> requests on seeing zero value in BOS descriptor. Also there can be cases
> where
>> U1 is
> disabled and U2 entry is allowed or vice versa, for these kind of cases
> the driver
>> can
> set zero exit latency value for U1 and non-zero exit latency value for U2
> . Based
>> on
this
> I think it would be better to report 0 when U1/U2 states are not enabled.
> Please
provide
> your opinion on this.
Hm... I assume you're testing against linux usb stack and xhci host. If
that's the case, it looks like host will still request the device to
enter U1/U2 despite the device rejecting SET_FEATURE(enable U1/U2). This
needs to be fixed. I think what you have is fine to workaround this issue.
>>> Thanks . Will send the next series with the other fixes that you have
>>> suggested
>>>
>>> Best Regards,
>>> Anurag Kumar Vulisha
>>>
>> I want to try something. Can you see if this helps with your performance
>> test without setting the U1/U2 exit latency to 0?
>> (No need to change what you have in your patch. This is just for testing).
>>
> With your patch , the link doesn't enter into U1/U2 and I am also getting
> better performance
>
> Thanks,
> Anurag Kumar Vulisha
Thanks for testing.
BR,
Thinh
Re: [BUG] usb: xhci: Possible resource leaks when xhci_run() fails
On 2019/5/15 0:55, Greg KH wrote:
On Tue, May 14, 2019 at 10:58:05PM +0800, Jia-Ju Bai wrote:
xhci_pci_setup() is assigned to hc_driver.reset;
xhci_run() is assigned to hc_driver.start();
xhci_stop() is assigned to hc_driver.stop().
xhci_pci_setup() calls xhci_gen_setup, which calls xhci_init(). And
xhci_init() calls xhci_mem_init() to allocate resources.
xhci_stop() calls xhci_mem_cleanup(), to release the resources allocated in
xhci_mem_init() (also namely xhci_pci_setup()).
xhci_run() can fail, because xhci_try_enable_msi() or xhci_alloc_command()
in this function can fail.
In drivers/usb/core/hcd.c:
retval = hcd->driver->reset(hcd);
if (retval < 0) {
..
goto err_hcd_driver_setup;
}
..
retval = hcd->driver->start(hcd);
if (retval < 0) {
..
goto err_hcd_driver_start;
}
...
hcd->driver->stop(hcd);
hcd->state = HC_STATE_HALT;
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);
err_hcd_driver_start:
if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
free_irq(irqnum, hcd);
err_request_irq:
err_hcd_driver_setup:
err_set_rh_speed:
usb_put_invalidate_rhdev(hcd);
err_allocate_root_hub:
usb_deregister_bus(&hcd->self);
err_register_bus:
hcd_buffer_destroy(hcd);
err_create_buf:
usb_phy_roothub_power_off(hcd->phy_roothub);
err_usb_phy_roothub_power_on:
usb_phy_roothub_exit(hcd->phy_roothub);
Thus, when hcd->driver->reset() succeeds and hcd->driver->start() fails,
hcd->driver->stop() is not called.
Namely, when xhci_pci_setup() successfully allocates resources, and
xhci_run() fails, xhci_stop() is not called to release the resources.
For this reason, resource leaks occur in this case.
I check the code of the ehci driver, uhci driver and ohci driver, and find
that they do not have such problem, because:
In the ehci driver, ehci_run() (namely hcd->driver->start()) never fails.
In the uhci driver, all the resources are allocated in uhci_start (namely
hcd->driver->start()), and no resources are allocated in uhci_pci_init()
(namely hcd->driver->reset()).
In the ohci driver, ohci_setup() (namely hcd->driver->reset()) also
allocates resources. But when ohci_start() (namely hcd->driver->start()) is
going to fail, ohci_stop() is directly called to release the resources
allocated by ohci_setup().
Thus, there are two possible ways of fixing bugs:
1) Call xhci_stop() when xhci_run() is going to fail (like the ohci driver)
2) Move all resource-allocation operations into xhci_run() (like the uhci
driver).
I am not sure whether these ways are correct, so I only report bugs.
Can you create a patch to show how you would fix this potential issue?
Given that making this type of thing fail is pretty rare, it's not a
real high priority to get to, so it might be a while for anyone here to
look at it.
Okay, I will send a patch soon.
Best wishes,
Jia-Ju Bai
[PATCH] usb: xhci: Possible resource leaks when xhci_run() fails
xhci_pci_setup() is assigned to hc_driver.reset;
xhci_run() is assigned to hc_driver.start();
xhci_stop() is assigned to hc_driver.stop().
xhci_pci_setup() calls xhci_gen_setup, which calls xhci_init(). And
xhci_init() calls xhci_mem_init() to allocate resources.
xhci_stop() calls xhci_mem_cleanup(), to release the resources allocated
in xhci_mem_init() (also namely xhci_pci_setup()).
xhci_run() can fail, because xhci_try_enable_msi() or
xhci_alloc_command() in this function can fail.
In drivers/usb/core/hcd.c:
retval = hcd->driver->reset(hcd);
if (retval < 0) {
..
goto err_hcd_driver_setup;
}
..
retval = hcd->driver->start(hcd);
if (retval < 0) {
..
goto err_hcd_driver_start;
}
...
hcd->driver->stop(hcd);
hcd->state = HC_STATE_HALT;
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);
err_hcd_driver_start:
if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
free_irq(irqnum, hcd);
err_request_irq:
err_hcd_driver_setup:
err_set_rh_speed:
usb_put_invalidate_rhdev(hcd);
err_allocate_root_hub:
usb_deregister_bus(&hcd->self);
err_register_bus:
hcd_buffer_destroy(hcd);
err_create_buf:
usb_phy_roothub_power_off(hcd->phy_roothub);
err_usb_phy_roothub_power_on:
usb_phy_roothub_exit(hcd->phy_roothub);
Thus, when hcd->driver->reset() succeeds and hcd->driver->start() fails,
hcd->driver->stop() is not called.
Namely, when xhci_pci_setup() successfully allocates resources, and
xhci_run() fails, xhci_stop() is not called to release the resources.
For this reason, resource leaks occur in this case.
The ehci driver, uhci driver and ohci driver do not have such bugs:
In the ehci driver, ehci_run() (namely hcd->driver->start()) never
fails.
In the uhci driver, all the resources are allocated in uhci_start
(namely hcd->driver->start()), and no resources are allocated in
uhci_pci_init() (namely hcd->driver->reset()).
In the ohci driver, ohci_setup() (namely hcd->driver->reset()) also
allocates resources. But when ohci_start() (namely hcd->driver->start())
is going to fail, ohci_stop() is directly called to release the
resources allocated by ohci_setup().
Referring to the ohci driver, to fix these resource leaks,
xhci_mem_cleanup() is called in error handling code of xhci_run(),
to release the allocated resources.
These bugs are found by a runtime fuzzing tool named FIZZER written by
us.
Signed-off-by: Jia-Ju Bai
---
drivers/usb/host/xhci.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 7fa58c99f126..d18893cf03cb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -651,8 +651,10 @@ int xhci_run(struct usb_hcd *hcd)
xhci_dbg_trace(xhci, trace_xhci_dbg_init, "xhci_run");
ret = xhci_try_enable_msi(hcd);
- if (ret)
+ if (ret) {
+ xhci_mem_cleanup(xhci);
return ret;
+ }
temp_64 = xhci_read_64(xhci, &xhci->ir_set->erst_dequeue);
temp_64 &= ~ERST_PTR_MASK;
@@ -683,8 +685,10 @@ int xhci_run(struct usb_hcd *hcd)
struct xhci_command *command;
command = xhci_alloc_command(xhci, false, GFP_KERNEL);
- if (!command)
+ if (!command) {
+ xhci_mem_cleanup(xhci);
return -ENOMEM;
+ }
ret = xhci_queue_vendor_command(xhci, command, 0, 0, 0,
TRB_TYPE(TRB_NEC_GET_FW));
--
2.17.0
RE: [PATCH v2 5/8] usb: chipidea: imx: add imx7ulp support
> > On Tue, May 14, 2019 at 4:39 AM Peter Chen wrote: > > > > Add imx7ulp support > > Since you are adding a new flag CI_HDRC_PMQOS, it would be nice to expand the > commit log a bit to explain about it. Ok, I will. Thanks. Peter
RE: [PATCH v2 2/8] usb: phy: phy-mxs-usb: add imx7ulp support
> > Signed-off-by: Peter Chen
> > ---
> > drivers/usb/phy/phy-mxs-usb.c | 76
> > ++-
> > 1 file changed, 75 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/phy/phy-mxs-usb.c
> > b/drivers/usb/phy/phy-mxs-usb.c index 1b1bb0ad40c3..90c96a8e9342
> > 100644
> > --- a/drivers/usb/phy/phy-mxs-usb.c
> > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > @@ -20,6 +20,7 @@
> >
> > #define DRIVER_NAME "mxs_phy"
> >
> > +/* Register Macro */
> > #define HW_USBPHY_PWD 0x00
> > #define HW_USBPHY_TX 0x10
> > #define HW_USBPHY_CTRL 0x30
> > @@ -37,6 +38,11 @@
> > #define GM_USBPHY_TX_TXCAL45DN(x)(((x) & 0xf) << 8)
> > #define GM_USBPHY_TX_D_CAL(x)(((x) & 0xf) << 0)
> >
> > +/* imx7ulp */
> > +#define HW_USBPHY_PLL_SIC 0xa0
> > +#define HW_USBPHY_PLL_SIC_SET 0xa4
> > +#define HW_USBPHY_PLL_SIC_CLR 0xa8
> > +
> > #define BM_USBPHY_CTRL_SFTRST BIT(31)
> > #define BM_USBPHY_CTRL_CLKGATE BIT(30)
> > #define BM_USBPHY_CTRL_OTG_ID_VALUEBIT(27)
> > @@ -55,6 +61,12 @@
> > #define BM_USBPHY_IP_FIX (BIT(17) | BIT(18))
> >
> > #define BM_USBPHY_DEBUG_CLKGATEBIT(30)
> > +/* imx7ulp */
> > +#define BM_USBPHY_PLL_LOCK BIT(31)
> > +#define BM_USBPHY_PLL_REG_ENABLE BIT(21)
> > +#define BM_USBPHY_PLL_BYPASS BIT(16)
> > +#define BM_USBPHY_PLL_POWERBIT(12)
> > +#define BM_USBPHY_PLL_EN_USB_CLKS BIT(6)
> >
> > /* Anatop Registers */
> > #define ANADIG_ANA_MISC0 0x150
> > @@ -167,6 +179,9 @@ static const struct mxs_phy_data imx6ul_phy_data = {
> > .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> > };
> >
> > +static const struct mxs_phy_data imx7ulp_phy_data = { };
> > +
> > static const struct of_device_id mxs_phy_dt_ids[] = {
> > { .compatible = "fsl,imx6sx-usbphy", .data = &imx6sx_phy_data, },
> > { .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_phy_data, }, @@
> > -174,6 +189,7 @@ static const struct of_device_id mxs_phy_dt_ids[] = {
> > { .compatible = "fsl,imx23-usbphy", .data = &imx23_phy_data, },
> > { .compatible = "fsl,vf610-usbphy", .data = &vf610_phy_data, },
> > { .compatible = "fsl,imx6ul-usbphy", .data = &imx6ul_phy_data, },
> > + { .compatible = "fsl,imx7ulp-usbphy", .data = &imx7ulp_phy_data, },
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids); @@ -198,6 +214,11 @@
> static
> > inline bool is_imx6sl_phy(struct mxs_phy *mxs_phy)
> > return mxs_phy->data == &imx6sl_phy_data; }
> >
> > +static inline bool is_imx7ulp_phy(struct mxs_phy *mxs_phy) {
> > + return mxs_phy->data == &imx7ulp_phy_data; }
> > +
> > /*
> > * PHY needs some 32K cycles to switch from 32K clock to
> > * bus (such as AHB/AXI, etc) clock.
> > @@ -221,14 +242,59 @@ static void mxs_phy_tx_init(struct mxs_phy *mxs_phy)
> > }
> > }
> >
> > +static int wait_for_pll_lock(const void __iomem *base)
> > +{
> > + int loop_count = 100;
> > +
> > + /* Wait for PLL to lock */
> > + do {
> > + if (readl(base + HW_USBPHY_PLL_SIC) &
> BM_USBPHY_PLL_LOCK)
> > + break;
> > + usleep_range(100, 150);
> > + } while (loop_count-- > 0);
> > +
> there is a common API readl_poll_timeout(), maybe you can try it.
>
Checked it, it can be used. Thanks.
Peter
RE: [PATCH v2] usb: renesas_usbhs: Use specific struct instead of USBHS_TYPE_* enums
Hi Simon-san,
> From: Simon Horman, Sent: Monday, May 13, 2019 9:01 PM
>
> On Mon, May 13, 2019 at 11:40:29AM +0900, Yoshihiro Shimoda wrote:
> > This patch adds a specific struct "usbhs_of_data" to add a new SoC
> > data easily instead of code basis in the future.
> >
> > Signed-off-by: Yoshihiro Shimoda
> > Reviewed-by: Geert Uytterhoeven
>
> Hi Shimoda-san,
>
> the minor suggestion below not withstanding this looks good to me.
>
> Reviewed-by: Simon Horman
Thank you for your review!
> > ---
> > Changes from v1 [1]:
> > - Use sizeof(variable) instead of sizeof(type).
> > - Add Geert-san's reviewed-by (thanks!).
> >
> > [1]
> > https://patchwork.kernel.org/patch/10938575/
> >
> > drivers/usb/renesas_usbhs/common.c | 112
> > +
> > drivers/usb/renesas_usbhs/common.h | 5 ++
> > 2 files changed, 70 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/usb/renesas_usbhs/common.c
> > b/drivers/usb/renesas_usbhs/common.c
> > index 249fbee..0ca89de 100644
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
> > @@ -598,8 +638,15 @@ static struct renesas_usbhs_platform_info
> > *usbhs_parse_dt(struct device *dev)
> > if (!info)
> > return NULL;
> >
> > + data = of_device_get_match_data(dev);
> > + if (!data)
> > + return NULL;
> > +
> > dparam = &info->driver_param;
> > - dparam->type = (uintptr_t)of_device_get_match_data(dev);
> > + memcpy(dparam, &data->param, sizeof(data->param));
> > + memcpy(&info->platform_callback, data->platform_callback,
> > + sizeof(*data->platform_callback));
>
> Can we avoid the error-proneness of calls to sizeof() as follows?
>
> *dparam = data->param;
> info->platform_callback = *data->platform_callback;
Since Chris-san has submitted a patch series [1] that is based on this patch
today,
I'd like to submit an incremental patch to avoid the error-proneness in the
renesas_usbhs
(common.c and mod_host.c) later, if possible.
Greg-san, is it acceptable?
[1]
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=117459
Best regards,
Yoshihiro Shimoda
> > +
> > if (!of_property_read_u32(dev->of_node, "renesas,buswait", &tmp))
> > dparam->buswait_bwait = tmp;
> > gpio = of_get_named_gpio_flags(dev->of_node, "renesas,enable-gpio", 0,
> > @@ -607,19 +654,6 @@ static struct renesas_usbhs_platform_info
> > *usbhs_parse_dt(struct device *dev)
> > if (gpio > 0)
> > dparam->enable_gpio = gpio;
> >
> > - if (dparam->type == USBHS_TYPE_RCAR_GEN2 ||
> > - dparam->type == USBHS_TYPE_RCAR_GEN3 ||
> > - dparam->type == USBHS_TYPE_RCAR_GEN3_WITH_PLL) {
> > - dparam->has_usb_dmac = 1;
> > - dparam->pipe_configs = usbhsc_new_pipe;
> > - dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > - }
> > -
> > - if (dparam->type == USBHS_TYPE_RZA1) {
> > - dparam->pipe_configs = usbhsc_new_pipe;
> > - dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> > - }
> > -
> > return info;
> > }
> >
> > @@ -676,29 +710,13 @@ static int usbhs_probe(struct platform_device *pdev)
> >&info->driver_param,
> >sizeof(struct renesas_usbhs_driver_param));
>
> Likewise in the (otherwise unchanged) use of memcpy above.
>
> >
> > - switch (priv->dparam.type) {
> > - case USBHS_TYPE_RCAR_GEN2:
> > - priv->pfunc = usbhs_rcar2_ops;
> > - break;
> > - case USBHS_TYPE_RCAR_GEN3:
> > - priv->pfunc = usbhs_rcar3_ops;
> > - break;
> > - case USBHS_TYPE_RCAR_GEN3_WITH_PLL:
> > - priv->pfunc = usbhs_rcar3_with_pll_ops;
> > - break;
> > - case USBHS_TYPE_RZA1:
> > - priv->pfunc = usbhs_rza1_ops;
> > - break;
> > - default:
> > - if (!info->platform_callback.get_id) {
> > - dev_err(&pdev->dev, "no platform callbacks");
> > - return -EINVAL;
> > - }
> > - memcpy(&priv->pfunc,
> > - &info->platform_callback,
> > - sizeof(struct renesas_usbhs_platform_callback));
> > - break;
> > + if (!info->platform_callback.get_id) {
> > + dev_err(&pdev->dev, "no platform callbacks");
> > + return -EINVAL;
> > }
> > + memcpy(&priv->pfunc,
> > + &info->platform_callback,
> > + sizeof(struct renesas_usbhs_platform_callback));
>
> And here too.
>
> >
> > /* set driver callback functions for platform */
> > dfunc = &info->driver_callback;
> > diff --git a/drivers/usb/renesas_usbhs/common.h
> > b/drivers/usb/renesas_usbhs/common.h
> > index 3777af8..de1a663 100644
> > --- a/drivers/usb/renesas_usbhs/common.h
> > +++ b/drivers/usb/renesas_usbhs/common.h
> > @@ -282,6 +282,11 @@ struct usbhs_priv {
> > struct clk *clks[2];
> > };
> >
> > +struct usbhs_of_data {
> > + const s
