* [PATCH V2 0/3] ADD interconnect support for USB
@ 2019-09-16 11:40 Chandana Kishori Chiluveru
2019-09-16 11:40 ` [PATCH V2 1/3] dt-bindings: Introduce interconnect bindings for usb Chandana Kishori Chiluveru
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Chandana Kishori Chiluveru @ 2019-09-16 11:40 UTC (permalink / raw)
To: balbi, agross, david.brown
Cc: linux-usb, linux-arm-msm, Chandana Kishori Chiluveru
This path series aims to add interconnect support in
dwc3-qcom driver on SDM845 SoCs.
Changes since V1:
> Comments by Georgi Djakov on "[PATCH 2/3]" addressed
> [PATCH 1/3] and [PATCH 3/3] remains unchanged
Chandana Kishori Chiluveru (3):
dt-bindings: Introduce interconnect bindings for usb
usb: dwc3: qcom: Add interconnect support in dwc3 driver
arm64: dts: sdm845: Add interconnect properties for USB
.../devicetree/bindings/usb/qcom,dwc3.txt | 13 ++
arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++
drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++-
3 files changed, 168 insertions(+), 2 deletions(-)
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH V2 1/3] dt-bindings: Introduce interconnect bindings for usb 2019-09-16 11:40 [PATCH V2 0/3] ADD interconnect support for USB Chandana Kishori Chiluveru @ 2019-09-16 11:40 ` Chandana Kishori Chiluveru 2019-09-16 11:41 ` [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver Chandana Kishori Chiluveru ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Chandana Kishori Chiluveru @ 2019-09-16 11:40 UTC (permalink / raw) To: balbi, agross, david.brown Cc: linux-usb, linux-arm-msm, Chandana Kishori Chiluveru Add documentation for the interconnects and interconnect-names bindings for USB as detailed by bindings/interconnect/interconnect.txt. Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org> --- Documentation/devicetree/bindings/usb/qcom,dwc3.txt | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt index cb695aa..7e9cb97 100644 --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt @@ -33,6 +33,16 @@ Optional clocks: Optional properties: - resets: Phandle to reset control that resets core and wrapper. +- interconnects: Pairs of phandles and interconnect provider specifier + to denote the edge source and destination ports of + the interconnect path. Please refer to + Documentation/devicetree/bindings/interconnect/ + for more details. +- interconnect-names: List of interconnect path name strings sorted in the same + order as the interconnects property. Consumers drivers will use + interconnect-names to match interconnect paths with interconnect + specifiers. Please refer to Documentation/devicetree/bindings/ + interconnect/ for more details. - interrupts: specifies interrupts from controller wrapper used to wakeup from low power/susepnd state. Must contain one or more entry for interrupt-names property @@ -74,6 +84,9 @@ Example device nodes: #size-cells = <1>; ranges; + interconnects = <&qnoc MASTER_USB3_0 &qnoc SLAVE_EBI1>, + <&qnoc MASTER_APPSS_PROC &qnoc SLAVE_USB3_0>; + interconnect-names = "usb-ddr", "apps-usb"; interrupts = <0 131 0>, <0 486 0>, <0 488 0>, <0 489 0>; interrupt-names = "hs_phy_irq", "ss_phy_irq", "dm_hs_phy_irq", "dp_hs_phy_irq"; -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver 2019-09-16 11:40 [PATCH V2 0/3] ADD interconnect support for USB Chandana Kishori Chiluveru 2019-09-16 11:40 ` [PATCH V2 1/3] dt-bindings: Introduce interconnect bindings for usb Chandana Kishori Chiluveru @ 2019-09-16 11:41 ` Chandana Kishori Chiluveru 2019-09-16 22:24 ` Matthias Kaehlcke 2019-09-16 11:41 ` [PATCH V2 3/3] arm64: dts: sdm845: Add interconnect properties for USB Chandana Kishori Chiluveru 2019-09-17 3:54 ` [PATCH V2 0/3] ADD interconnect support " Manu Gautam 3 siblings, 1 reply; 9+ messages in thread From: Chandana Kishori Chiluveru @ 2019-09-16 11:41 UTC (permalink / raw) To: balbi, agross, david.brown Cc: linux-usb, linux-arm-msm, Chandana Kishori Chiluveru Add interconnect support in dwc3-qcom driver to vote for bus bandwidth. This requires for two different paths - from USB master to DDR slave. The other is from APPS master to USB slave. Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org> --- drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 143 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 184df4d..4f6c9736 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -14,6 +14,7 @@ #include <linux/extcon.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/interconnect.h> #include <linux/phy/phy.h> #include <linux/usb/of.h> #include <linux/reset.h> @@ -59,8 +60,13 @@ struct dwc3_qcom { enum usb_dr_mode mode; bool is_suspended; bool pm_suspended; + struct icc_path *usb_ddr_icc_path; + struct icc_path *apps_usb_icc_path; }; +static int usb_interconnect_enable(struct dwc3_qcom *qcom); +static int usb_interconnect_disable(struct dwc3_qcom *qcom); + static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) { u32 reg; @@ -222,7 +228,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) { u32 val; - int i; + int i, ret; if (qcom->is_suspended) return 0; @@ -234,6 +240,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) for (i = qcom->num_clocks - 1; i >= 0; i--) clk_disable_unprepare(qcom->clks[i]); + /* Remove bus voting */ + ret = usb_interconnect_disable(qcom); + if (ret) + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); + qcom->is_suspended = true; dwc3_qcom_enable_interrupts(qcom); @@ -259,6 +270,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) } } + /* Add bus voting */ + ret = usb_interconnect_enable(qcom); + if (ret) + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); + /* Clear existing events from PHY related to L2 in/out */ dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); @@ -268,6 +284,117 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) return 0; } +/* Interconnect path bandwidths in MBps */ +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240) +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700) +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000) +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500) +#define APPS_USB_AVG_BW 0 +#define APPS_USB_PEAK_BW MBps_to_icc(40) + +/** + * usb_interconnect_init() - Request to get interconnect path handle + * @qcom: Pointer to the concerned usb core. + * + */ +static int usb_interconnect_init(struct dwc3_qcom *qcom) +{ + struct device *dev = qcom->dev; + + qcom->usb_ddr_icc_path = of_icc_get(dev, "usb-ddr"); + if (IS_ERR(qcom->usb_ddr_icc_path)) { + dev_err(dev, "Error: (%ld) failed getting %s path\n", + PTR_ERR(qcom->usb_ddr_icc_path), "usb-ddr"); + return PTR_ERR(qcom->usb_ddr_icc_path); + } + + qcom->apps_usb_icc_path = of_icc_get(dev, "apps-usb"); + if (IS_ERR(qcom->apps_usb_icc_path)) { + dev_err(dev, "Error: (%ld) failed getting %s path\n", + PTR_ERR(qcom->apps_usb_icc_path), "apps-usb"); + return PTR_ERR(qcom->usb_ddr_icc_path); + } + + return 0; +} + +/** + * geni_interconnect_exit() - Request to release interconnect path handle + * @qcom: Pointer to the concerned usb core. + * + * This function is used to release interconnect path handle. + */ +static void usb_interconnect_exit(struct dwc3_qcom *qcom) +{ + icc_put(qcom->usb_ddr_icc_path); + icc_put(qcom->apps_usb_icc_path); +} + +/* Currently we only use bandwidth level, so just "enable" interconnects */ +static int usb_interconnect_enable(struct dwc3_qcom *qcom) +{ + struct dwc3 *dwc; + int ret; + + dwc = platform_get_drvdata(qcom->dwc3); + if (!dwc) { + dev_err(qcom->dev, "Failed to get dwc3 device\n"); + return -EPROBE_DEFER; + } + + if (dwc->maximum_speed == USB_SPEED_SUPER) { + ret = icc_set_bw(qcom->usb_ddr_icc_path, + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); + if (ret) + return ret; + } else { + ret = icc_set_bw(qcom->usb_ddr_icc_path, + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW); + if (ret) + return ret; + } + + ret = icc_set_bw(qcom->apps_usb_icc_path, + APPS_USB_AVG_BW, APPS_USB_PEAK_BW); + if (ret) + goto err_disable_mem_path; + + return 0; + +err_disable_mem_path: + icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); + + return ret; +} + +/* To disable an interconnect, we just set its bandwidth to 0 */ +static int usb_interconnect_disable(struct dwc3_qcom *qcom) +{ + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); + int ret; + + ret = icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); + if (ret) + return ret; + + ret = icc_set_bw(qcom->apps_usb_icc_path, 0, 0); + if (ret) + goto err_reenable_memory_path; + + return 0; + + /* Re-enable things in the event of an error */ +err_reenable_memory_path: + if (dwc->maximum_speed == USB_SPEED_SUPER) + icc_set_bw(qcom->usb_ddr_icc_path, + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); + else + icc_set_bw(qcom->usb_ddr_icc_path, + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW); + + return ret; +} + static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) { struct dwc3_qcom *qcom = data; @@ -494,6 +621,17 @@ static int dwc3_qcom_probe(struct platform_device *pdev) goto depopulate; } + ret = usb_interconnect_init(qcom); + if (ret) { + dev_err(dev, "failed to get interconnect handle ret:%d\n", ret); + goto depopulate; + } + ret = usb_interconnect_enable(qcom); + if (ret) { + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); + goto interconnect_exit; + } + qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); /* enable vbus override for device mode */ @@ -503,7 +641,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) /* register extcon to override sw_vbus on Vbus change later */ ret = dwc3_qcom_register_extcon(qcom); if (ret) - goto depopulate; + goto interconnect_exit; device_init_wakeup(&pdev->dev, 1); qcom->is_suspended = false; @@ -513,6 +651,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev) return 0; +interconnect_exit: + usb_interconnect_exit(qcom); depopulate: of_platform_depopulate(&pdev->dev); clk_disable: @@ -540,6 +680,7 @@ static int dwc3_qcom_remove(struct platform_device *pdev) } qcom->num_clocks = 0; + usb_interconnect_exit(qcom); reset_control_assert(qcom->resets); pm_runtime_allow(dev); -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver 2019-09-16 11:41 ` [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver Chandana Kishori Chiluveru @ 2019-09-16 22:24 ` Matthias Kaehlcke 2019-09-17 11:09 ` cchiluve 0 siblings, 1 reply; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-16 22:24 UTC (permalink / raw) To: Chandana Kishori Chiluveru Cc: balbi, agross, david.brown, linux-usb, linux-arm-msm Hi Chandana, On Mon, Sep 16, 2019 at 05:11:00PM +0530, Chandana Kishori Chiluveru wrote: > Add interconnect support in dwc3-qcom driver to vote for bus > bandwidth. > > This requires for two different paths - from USB master to > DDR slave. The other is from APPS master to USB slave. > > Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 143 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 184df4d..4f6c9736 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -14,6 +14,7 @@ > #include <linux/extcon.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/interconnect.h> please use alphabetical order, i.e. this should be after 'linux/extcon.h'. > #include <linux/phy/phy.h> > #include <linux/usb/of.h> > #include <linux/reset.h> > @@ -59,8 +60,13 @@ struct dwc3_qcom { > enum usb_dr_mode mode; > bool is_suspended; > bool pm_suspended; > + struct icc_path *usb_ddr_icc_path; > + struct icc_path *apps_usb_icc_path; > }; > > +static int usb_interconnect_enable(struct dwc3_qcom *qcom); > +static int usb_interconnect_disable(struct dwc3_qcom *qcom); > + > static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) > { > u32 reg; > @@ -222,7 +228,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > { > u32 val; > - int i; > + int i, ret; > > if (qcom->is_suspended) > return 0; > @@ -234,6 +240,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > for (i = qcom->num_clocks - 1; i >= 0; i--) > clk_disable_unprepare(qcom->clks[i]); > > + /* Remove bus voting */ IMO the comment doesn't add much, especially since there is a log in case of failure. > + ret = usb_interconnect_disable(qcom); > + if (ret) > + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); This should probably be a warning, since the function continues with normal execution. nit: the function is called usb_interconnect_disable(), but the message says "bus bw voting failed". The function name encapsulates what the function does, but the message talks about implementation details. To be consistent either the function should have something about 'voting' in it's name, or the message should be "failed to disable interconnect" or similar. > + > qcom->is_suspended = true; > dwc3_qcom_enable_interrupts(qcom); > > @@ -259,6 +270,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) > } > } > > + /* Add bus voting */ > + ret = usb_interconnect_enable(qcom); > + if (ret) > + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); > + same comments as for suspend() > /* Clear existing events from PHY related to L2 in/out */ > dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, > PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); > @@ -268,6 +284,117 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) > return 0; > } > > +/* Interconnect path bandwidths in MBps */ > +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240) > +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700) > +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000) > +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500) > +#define APPS_USB_AVG_BW 0 > +#define APPS_USB_PEAK_BW MBps_to_icc(40) > + > +/** > + * usb_interconnect_init() - A function named 'usb_*' is typically associated to be a USB core function. I would suggest to use the 'dwc3_qcom_' prefix like all other functions in this file. Applicable to all new functions. > Request to get interconnect path handle nit: handles Get rid of "Request to"? > + * @qcom: Pointer to the concerned usb core. > + * > + */ > +static int usb_interconnect_init(struct dwc3_qcom *qcom) > +{ > + struct device *dev = qcom->dev; > + > + qcom->usb_ddr_icc_path = of_icc_get(dev, "usb-ddr"); > + if (IS_ERR(qcom->usb_ddr_icc_path)) { > + dev_err(dev, "Error: (%ld) failed getting %s path\n", > + PTR_ERR(qcom->usb_ddr_icc_path), "usb-ddr"); Why not put 'usb-ddr' in the format string, instead of using '%s'? > + return PTR_ERR(qcom->usb_ddr_icc_path); > + } > + > + qcom->apps_usb_icc_path = of_icc_get(dev, "apps-usb"); > + if (IS_ERR(qcom->apps_usb_icc_path)) { > + dev_err(dev, "Error: (%ld) failed getting %s path\n", > + PTR_ERR(qcom->apps_usb_icc_path), "apps-usb"); same comment as above. icc_put(qcom->usb_ddr_icc_path); > + return PTR_ERR(qcom->usb_ddr_icc_path); should be 'qcom->apps_usb_icc_path' > + } > + > + return 0; > +} > + > +/** > + * geni_interconnect_exit() - wrong prefix > Request to release interconnect path handle nit: handles Get rid of "Request to"? > + * @qcom: Pointer to the concerned usb core. > + * > + * This function is used to release interconnect path handle. > + */ > +static void usb_interconnect_exit(struct dwc3_qcom *qcom) > +{ > + icc_put(qcom->usb_ddr_icc_path); > + icc_put(qcom->apps_usb_icc_path); > +} > + > +/* Currently we only use bandwidth level, so just "enable" interconnects */ > +static int usb_interconnect_enable(struct dwc3_qcom *qcom) > +{ > + struct dwc3 *dwc; > + int ret; > + > + dwc = platform_get_drvdata(qcom->dwc3); > + if (!dwc) { > + dev_err(qcom->dev, "Failed to get dwc3 device\n"); > + return -EPROBE_DEFER; > + } This should never happen, drvdata is set in _probe(). If it happens -EPROBE_DEFER doesn't seem to be an appropriate error code. I suggest to remove the condition entirely. > + > + if (dwc->maximum_speed == USB_SPEED_SUPER) { > + ret = icc_set_bw(qcom->usb_ddr_icc_path, > + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); > + if (ret) > + return ret; > + } else { > + ret = icc_set_bw(qcom->usb_ddr_icc_path, > + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW); > + if (ret) > + return ret; > + } > + > + ret = icc_set_bw(qcom->apps_usb_icc_path, > + APPS_USB_AVG_BW, APPS_USB_PEAK_BW); > + if (ret) > + goto err_disable_mem_path; > + > + return 0; > + > +err_disable_mem_path: > + icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); > + > + return ret; > +} > + > +/* To disable an interconnect, we just set its bandwidth to 0 */ > +static int usb_interconnect_disable(struct dwc3_qcom *qcom) > +{ > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > + int ret; > + > + ret = icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); > + if (ret) > + return ret; > + > + ret = icc_set_bw(qcom->apps_usb_icc_path, 0, 0); > + if (ret) > + goto err_reenable_memory_path; > + > + return 0; > + > + /* Re-enable things in the event of an error */ > +err_reenable_memory_path: > + if (dwc->maximum_speed == USB_SPEED_SUPER) > + icc_set_bw(qcom->usb_ddr_icc_path, > + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); > + else > + icc_set_bw(qcom->usb_ddr_icc_path, > + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW); > + > + return ret; > +} > + > static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) > { > struct dwc3_qcom *qcom = data; > @@ -494,6 +621,17 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > goto depopulate; > } > > + ret = usb_interconnect_init(qcom); > + if (ret) { > + dev_err(dev, "failed to get interconnect handle ret:%d\n", ret); similar to my comment above, the function name adds an abstraction, but the comment talks about implementation details. I'd suggest something like 'failed to init interconnect'. > + goto depopulate; > + } > + ret = usb_interconnect_enable(qcom); > + if (ret) { > + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); same comment as for 'dwc3_qcom_suspend' above. I think the _interconnect_enable() call should be part of _interconnect_init(). I was earlier considering to suggest to change the name of _interconnect_init() to something else, since it doesn't really do any interconnect initialization, but didn't have a good suggestion for an alternative name. Moving the _enable() call into _init() would make _init() and actual init routine, besides removing clutter from _probe(). > + goto interconnect_exit; > + } > + > qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); > > /* enable vbus override for device mode */ > @@ -503,7 +641,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > /* register extcon to override sw_vbus on Vbus change later */ > ret = dwc3_qcom_register_extcon(qcom); > if (ret) > - goto depopulate; > + goto interconnect_exit; > > device_init_wakeup(&pdev->dev, 1); > qcom->is_suspended = false; > @@ -513,6 +651,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > return 0; > > +interconnect_exit: > + usb_interconnect_exit(qcom); > depopulate: > of_platform_depopulate(&pdev->dev); > clk_disable: > @@ -540,6 +680,7 @@ static int dwc3_qcom_remove(struct platform_device *pdev) > } > qcom->num_clocks = 0; > > + usb_interconnect_exit(qcom); > reset_control_assert(qcom->resets); > > pm_runtime_allow(dev); Cheers Matthias ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver 2019-09-16 22:24 ` Matthias Kaehlcke @ 2019-09-17 11:09 ` cchiluve 2019-09-17 17:58 ` Matthias Kaehlcke 0 siblings, 1 reply; 9+ messages in thread From: cchiluve @ 2019-09-17 11:09 UTC (permalink / raw) To: Matthias Kaehlcke; +Cc: balbi, agross, david.brown, linux-usb, linux-arm-msm Hi Matthias, Thanks for the review. I Will address below comments and post changes in new version. On 2019-09-17 03:54, Matthias Kaehlcke wrote: > Hi Chandana, > > On Mon, Sep 16, 2019 at 05:11:00PM +0530, Chandana Kishori Chiluveru > wrote: >> Add interconnect support in dwc3-qcom driver to vote for bus >> bandwidth. >> >> This requires for two different paths - from USB master to >> DDR slave. The other is from APPS master to USB slave. >> >> Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 145 >> ++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 143 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c >> b/drivers/usb/dwc3/dwc3-qcom.c >> index 184df4d..4f6c9736 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -14,6 +14,7 @@ >> #include <linux/extcon.h> >> #include <linux/of_platform.h> >> #include <linux/platform_device.h> >> +#include <linux/interconnect.h> > > please use alphabetical order, i.e. this should be after > 'linux/extcon.h'. > >> #include <linux/phy/phy.h> >> #include <linux/usb/of.h> >> #include <linux/reset.h> >> @@ -59,8 +60,13 @@ struct dwc3_qcom { >> enum usb_dr_mode mode; >> bool is_suspended; >> bool pm_suspended; >> + struct icc_path *usb_ddr_icc_path; >> + struct icc_path *apps_usb_icc_path; >> }; >> >> +static int usb_interconnect_enable(struct dwc3_qcom *qcom); >> +static int usb_interconnect_disable(struct dwc3_qcom *qcom); >> + >> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, >> u32 val) >> { >> u32 reg; >> @@ -222,7 +228,7 @@ static void dwc3_qcom_enable_interrupts(struct >> dwc3_qcom *qcom) >> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) >> { >> u32 val; >> - int i; >> + int i, ret; >> >> if (qcom->is_suspended) >> return 0; >> @@ -234,6 +240,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom >> *qcom) >> for (i = qcom->num_clocks - 1; i >= 0; i--) >> clk_disable_unprepare(qcom->clks[i]); >> >> + /* Remove bus voting */ > > IMO the comment doesn't add much, especially since there is a log in > case of failure. > >> + ret = usb_interconnect_disable(qcom); >> + if (ret) >> + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); > > This should probably be a warning, since the function continues with > normal execution. > > nit: the function is called usb_interconnect_disable(), but the > message says "bus bw voting failed". The function name encapsulates > what the function does, but the message talks about implementation > details. To be consistent either the function should have something > about 'voting' in it's name, or the message should be "failed to > disable interconnect" or similar. > >> + >> qcom->is_suspended = true; >> dwc3_qcom_enable_interrupts(qcom); >> >> @@ -259,6 +270,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom >> *qcom) >> } >> } >> >> + /* Add bus voting */ >> + ret = usb_interconnect_enable(qcom); >> + if (ret) >> + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); >> + > > same comments as for suspend() > >> /* Clear existing events from PHY related to L2 in/out */ >> dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, >> PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); >> @@ -268,6 +284,117 @@ static int dwc3_qcom_resume(struct dwc3_qcom >> *qcom) >> return 0; >> } >> >> +/* Interconnect path bandwidths in MBps */ >> +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240) >> +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700) >> +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000) >> +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500) >> +#define APPS_USB_AVG_BW 0 >> +#define APPS_USB_PEAK_BW MBps_to_icc(40) >> + >> +/** >> + * usb_interconnect_init() - > > A function named 'usb_*' is typically associated to be a USB core > function. I would suggest to use the 'dwc3_qcom_' prefix like all > other functions in this file. Applicable to all new functions. > >> Request to get interconnect path handle > > nit: handles > > Get rid of "Request to"? > >> + * @qcom: Pointer to the concerned usb core. >> + * >> + */ >> +static int usb_interconnect_init(struct dwc3_qcom *qcom) >> +{ >> + struct device *dev = qcom->dev; >> + >> + qcom->usb_ddr_icc_path = of_icc_get(dev, "usb-ddr"); >> + if (IS_ERR(qcom->usb_ddr_icc_path)) { >> + dev_err(dev, "Error: (%ld) failed getting %s path\n", >> + PTR_ERR(qcom->usb_ddr_icc_path), "usb-ddr"); > > Why not put 'usb-ddr' in the format string, instead of using '%s'? > >> + return PTR_ERR(qcom->usb_ddr_icc_path); >> + } >> + >> + qcom->apps_usb_icc_path = of_icc_get(dev, "apps-usb"); >> + if (IS_ERR(qcom->apps_usb_icc_path)) { >> + dev_err(dev, "Error: (%ld) failed getting %s path\n", >> + PTR_ERR(qcom->apps_usb_icc_path), "apps-usb"); > > same comment as above. > > icc_put(qcom->usb_ddr_icc_path); > >> + return PTR_ERR(qcom->usb_ddr_icc_path); > > should be 'qcom->apps_usb_icc_path' > >> + } >> + >> + return 0; >> +} >> + >> +/** >> + * geni_interconnect_exit() - > > wrong prefix > >> Request to release interconnect path handle > > nit: handles > > Get rid of "Request to"? > >> + * @qcom: Pointer to the concerned usb core. >> + * >> + * This function is used to release interconnect path handle. >> + */ >> +static void usb_interconnect_exit(struct dwc3_qcom *qcom) >> +{ >> + icc_put(qcom->usb_ddr_icc_path); >> + icc_put(qcom->apps_usb_icc_path); >> +} >> + >> +/* Currently we only use bandwidth level, so just "enable" >> interconnects */ >> +static int usb_interconnect_enable(struct dwc3_qcom *qcom) >> +{ >> + struct dwc3 *dwc; >> + int ret; >> + >> + dwc = platform_get_drvdata(qcom->dwc3); >> + if (!dwc) { >> + dev_err(qcom->dev, "Failed to get dwc3 device\n"); >> + return -EPROBE_DEFER; >> + } > > This should never happen, drvdata is set in _probe(). If it happens > -EPROBE_DEFER doesn't seem to be an appropriate error code. I suggest > to remove the condition entirely. > In my testing i have seen a null pointer crash with "dwc->maximum_speed". To prevent the crash i have added this logic. Agree that drvdata is set in dwc3_probe(). Sometimes i can see that dwc3_probe never getting completed before it can go and access dwc->maximum_speed pointer here. This is leading to null pointer access crash. if i defer the probe and try again i can see that drvdata getting updated successfully in dwc3_probe. >> + >> + if (dwc->maximum_speed == USB_SPEED_SUPER) { >> + ret = icc_set_bw(qcom->usb_ddr_icc_path, >> + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); >> + if (ret) >> + return ret; >> + } else { >> + ret = icc_set_bw(qcom->usb_ddr_icc_path, >> + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW); >> + if (ret) >> + return ret; >> + } >> + >> + ret = icc_set_bw(qcom->apps_usb_icc_path, >> + APPS_USB_AVG_BW, APPS_USB_PEAK_BW); >> + if (ret) >> + goto err_disable_mem_path; >> + >> + return 0; >> + >> +err_disable_mem_path: >> + icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); >> + >> + return ret; >> +} >> + >> +/* To disable an interconnect, we just set its bandwidth to 0 */ >> +static int usb_interconnect_disable(struct dwc3_qcom *qcom) >> +{ >> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >> + int ret; >> + >> + ret = icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); >> + if (ret) >> + return ret; >> + >> + ret = icc_set_bw(qcom->apps_usb_icc_path, 0, 0); >> + if (ret) >> + goto err_reenable_memory_path; >> + >> + return 0; >> + >> + /* Re-enable things in the event of an error */ >> +err_reenable_memory_path: >> + if (dwc->maximum_speed == USB_SPEED_SUPER) >> + icc_set_bw(qcom->usb_ddr_icc_path, >> + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); >> + else >> + icc_set_bw(qcom->usb_ddr_icc_path, >> + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW); >> + >> + return ret; >> +} >> + >> static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) >> { >> struct dwc3_qcom *qcom = data; >> @@ -494,6 +621,17 @@ static int dwc3_qcom_probe(struct platform_device >> *pdev) >> goto depopulate; >> } >> >> + ret = usb_interconnect_init(qcom); >> + if (ret) { >> + dev_err(dev, "failed to get interconnect handle ret:%d\n", ret); > > similar to my comment above, the function name adds an abstraction, > but the comment talks about implementation details. I'd suggest > something like 'failed to init interconnect'. > >> + goto depopulate; >> + } >> + ret = usb_interconnect_enable(qcom); >> + if (ret) { >> + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); > > same comment as for 'dwc3_qcom_suspend' above. > > I think the _interconnect_enable() call should be part of > _interconnect_init(). I was earlier considering to suggest to > change the name of _interconnect_init() to something else, since > it doesn't really do any interconnect initialization, but didn't have > a good suggestion for an alternative name. Moving the _enable() call > into _init() would make _init() and actual init routine, besides > removing clutter from _probe(). > >> + goto interconnect_exit; >> + } >> + >> qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); >> >> /* enable vbus override for device mode */ >> @@ -503,7 +641,7 @@ static int dwc3_qcom_probe(struct platform_device >> *pdev) >> /* register extcon to override sw_vbus on Vbus change later */ >> ret = dwc3_qcom_register_extcon(qcom); >> if (ret) >> - goto depopulate; >> + goto interconnect_exit; >> >> device_init_wakeup(&pdev->dev, 1); >> qcom->is_suspended = false; >> @@ -513,6 +651,8 @@ static int dwc3_qcom_probe(struct platform_device >> *pdev) >> >> return 0; >> >> +interconnect_exit: >> + usb_interconnect_exit(qcom); >> depopulate: >> of_platform_depopulate(&pdev->dev); >> clk_disable: >> @@ -540,6 +680,7 @@ static int dwc3_qcom_remove(struct platform_device >> *pdev) >> } >> qcom->num_clocks = 0; >> >> + usb_interconnect_exit(qcom); >> reset_control_assert(qcom->resets); >> >> pm_runtime_allow(dev); > > Cheers > > Matthias ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver 2019-09-17 11:09 ` cchiluve @ 2019-09-17 17:58 ` Matthias Kaehlcke 0 siblings, 0 replies; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-17 17:58 UTC (permalink / raw) To: cchiluve; +Cc: balbi, agross, david.brown, linux-usb, linux-arm-msm Hi Chandana, On Tue, Sep 17, 2019 at 04:39:20PM +0530, cchiluve@codeaurora.org wrote: > Hi Matthias, > > Thanks for the review. I Will address below comments and post changes in new > version. > > On 2019-09-17 03:54, Matthias Kaehlcke wrote: > > Hi Chandana, > > > > On Mon, Sep 16, 2019 at 05:11:00PM +0530, Chandana Kishori Chiluveru > > wrote: > > > Add interconnect support in dwc3-qcom driver to vote for bus > > > bandwidth. > > > > > > This requires for two different paths - from USB master to > > > DDR slave. The other is from APPS master to USB slave. > > > > > > Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org> > > > --- > > > drivers/usb/dwc3/dwc3-qcom.c | 145 > > > ++++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 143 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c > > > b/drivers/usb/dwc3/dwc3-qcom.c > > > index 184df4d..4f6c9736 100644 > > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > > @@ -14,6 +14,7 @@ > > > #include <linux/extcon.h> > > > #include <linux/of_platform.h> > > > #include <linux/platform_device.h> > > > +#include <linux/interconnect.h> > > > > please use alphabetical order, i.e. this should be after > > 'linux/extcon.h'. > > > > > #include <linux/phy/phy.h> > > > #include <linux/usb/of.h> > > > #include <linux/reset.h> > > > @@ -59,8 +60,13 @@ struct dwc3_qcom { > > > enum usb_dr_mode mode; > > > bool is_suspended; > > > bool pm_suspended; > > > + struct icc_path *usb_ddr_icc_path; > > > + struct icc_path *apps_usb_icc_path; > > > }; > > > > > > +static int usb_interconnect_enable(struct dwc3_qcom *qcom); > > > +static int usb_interconnect_disable(struct dwc3_qcom *qcom); > > > + > > > static inline void dwc3_qcom_setbits(void __iomem *base, u32 > > > offset, u32 val) > > > { > > > u32 reg; > > > @@ -222,7 +228,7 @@ static void dwc3_qcom_enable_interrupts(struct > > > dwc3_qcom *qcom) > > > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > > > { > > > u32 val; > > > - int i; > > > + int i, ret; > > > > > > if (qcom->is_suspended) > > > return 0; > > > @@ -234,6 +240,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom > > > *qcom) > > > for (i = qcom->num_clocks - 1; i >= 0; i--) > > > clk_disable_unprepare(qcom->clks[i]); > > > > > > + /* Remove bus voting */ > > > > IMO the comment doesn't add much, especially since there is a log in > > case of failure. > > > > > + ret = usb_interconnect_disable(qcom); > > > + if (ret) > > > + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); > > > > This should probably be a warning, since the function continues with > > normal execution. > > > > nit: the function is called usb_interconnect_disable(), but the > > message says "bus bw voting failed". The function name encapsulates > > what the function does, but the message talks about implementation > > details. To be consistent either the function should have something > > about 'voting' in it's name, or the message should be "failed to > > disable interconnect" or similar. > > > > > + > > > qcom->is_suspended = true; > > > dwc3_qcom_enable_interrupts(qcom); > > > > > > @@ -259,6 +270,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom > > > *qcom) > > > } > > > } > > > > > > + /* Add bus voting */ > > > + ret = usb_interconnect_enable(qcom); > > > + if (ret) > > > + dev_err(qcom->dev, "bus bw voting failed %d\n", ret); > > > + > > > > same comments as for suspend() > > > > > /* Clear existing events from PHY related to L2 in/out */ > > > dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, > > > PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); > > > @@ -268,6 +284,117 @@ static int dwc3_qcom_resume(struct dwc3_qcom > > > *qcom) > > > return 0; > > > } > > > > > > +/* Interconnect path bandwidths in MBps */ > > > +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240) > > > +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700) > > > +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000) > > > +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500) > > > +#define APPS_USB_AVG_BW 0 > > > +#define APPS_USB_PEAK_BW MBps_to_icc(40) > > > + > > > +/** > > > + * usb_interconnect_init() - > > > > A function named 'usb_*' is typically associated to be a USB core > > function. I would suggest to use the 'dwc3_qcom_' prefix like all > > other functions in this file. Applicable to all new functions. > > > > > Request to get interconnect path handle > > > > nit: handles > > > > Get rid of "Request to"? > > > > > + * @qcom: Pointer to the concerned usb core. > > > + * > > > + */ > > > +static int usb_interconnect_init(struct dwc3_qcom *qcom) > > > +{ > > > + struct device *dev = qcom->dev; > > > + > > > + qcom->usb_ddr_icc_path = of_icc_get(dev, "usb-ddr"); > > > + if (IS_ERR(qcom->usb_ddr_icc_path)) { > > > + dev_err(dev, "Error: (%ld) failed getting %s path\n", > > > + PTR_ERR(qcom->usb_ddr_icc_path), "usb-ddr"); > > > > Why not put 'usb-ddr' in the format string, instead of using '%s'? > > > > > + return PTR_ERR(qcom->usb_ddr_icc_path); > > > + } > > > + > > > + qcom->apps_usb_icc_path = of_icc_get(dev, "apps-usb"); > > > + if (IS_ERR(qcom->apps_usb_icc_path)) { > > > + dev_err(dev, "Error: (%ld) failed getting %s path\n", > > > + PTR_ERR(qcom->apps_usb_icc_path), "apps-usb"); > > > > same comment as above. > > > > icc_put(qcom->usb_ddr_icc_path); > > > > > + return PTR_ERR(qcom->usb_ddr_icc_path); > > > > should be 'qcom->apps_usb_icc_path' > > > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +/** > > > + * geni_interconnect_exit() - > > > > wrong prefix > > > > > Request to release interconnect path handle > > > > nit: handles > > > > Get rid of "Request to"? > > > > > + * @qcom: Pointer to the concerned usb core. > > > + * > > > + * This function is used to release interconnect path handle. > > > + */ > > > +static void usb_interconnect_exit(struct dwc3_qcom *qcom) > > > +{ > > > + icc_put(qcom->usb_ddr_icc_path); > > > + icc_put(qcom->apps_usb_icc_path); > > > +} > > > + > > > +/* Currently we only use bandwidth level, so just "enable" > > > interconnects */ > > > +static int usb_interconnect_enable(struct dwc3_qcom *qcom) > > > +{ > > > + struct dwc3 *dwc; > > > + int ret; > > > + > > > + dwc = platform_get_drvdata(qcom->dwc3); > > > + if (!dwc) { > > > + dev_err(qcom->dev, "Failed to get dwc3 device\n"); > > > + return -EPROBE_DEFER; > > > + } > > > > This should never happen, drvdata is set in _probe(). If it happens > > -EPROBE_DEFER doesn't seem to be an appropriate error code. I suggest > > to remove the condition entirely. > > > In my testing i have seen a null pointer crash with "dwc->maximum_speed". To > prevent the crash i have added this logic. > Agree that drvdata is set in dwc3_probe(). Sometimes i can see that > dwc3_probe never getting completed before it can go and access > dwc->maximum_speed pointer here. > This is leading to null pointer access crash. if i defer the probe and try > again i can see that drvdata getting updated successfully in dwc3_probe. I see, _probe() sets the drvdata of the dwc3_qcom pdev, however _enable() gets the drvdata from the dwc3 pdev. Apparently dwc3_probe() and dwc3_qcom_probe() can run in paralel: dwc3_probe // stuff dwc3_get_properties dwc->maximum_speed = usb_get_maximum_speed(dev); platform_set_drvdata(pdev, dwc) // more stuff dwc3_qcom_probe dwc3_qcom_of_register_core qcom->dwc3 = of_find_device_by_node() usb_interconnect_enable dwc = platform_get_drvdata(qcom->dwc3); When dwc3_qcom_of_register_core() is called the dwc3 device may not be fully initialized yet. Also after the drvdata is set initialization is still ongoing. You can't really rely on the dwc3 struct to be fully initialized, but at least the maximum_speed attribute should be set when the drvdata is not NULL. I wonder if it would be clearer to do the check in _probe() instead of _interconnect_enable(), the -EPROBE_DEFER return value is confusing, especially since the function can be called from a non-probe context. You could possibly use device_is_bound() to confirm that dwc3_probe() is completed, the function isn't widely used though. Checking that the device is fully initialized before using it seems like a good idea, however there might be reasons for not using device_is_bound() which I ignore. Maybe others can offer advice on this. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 3/3] arm64: dts: sdm845: Add interconnect properties for USB 2019-09-16 11:40 [PATCH V2 0/3] ADD interconnect support for USB Chandana Kishori Chiluveru 2019-09-16 11:40 ` [PATCH V2 1/3] dt-bindings: Introduce interconnect bindings for usb Chandana Kishori Chiluveru 2019-09-16 11:41 ` [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver Chandana Kishori Chiluveru @ 2019-09-16 11:41 ` Chandana Kishori Chiluveru 2019-09-16 22:56 ` Matthias Kaehlcke 2019-09-17 3:54 ` [PATCH V2 0/3] ADD interconnect support " Manu Gautam 3 siblings, 1 reply; 9+ messages in thread From: Chandana Kishori Chiluveru @ 2019-09-16 11:41 UTC (permalink / raw) To: balbi, agross, david.brown Cc: linux-usb, linux-arm-msm, Chandana Kishori Chiluveru Populate USB DT node with interconnect properties. Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org> --- arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index fcb9330..1c41922 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -1837,6 +1837,12 @@ resets = <&gcc GCC_USB30_PRIM_BCR>; + interconnects = <&rsc_hlos MASTER_USB3_0 + &rsc_hlos SLAVE_EBI1>, + <&rsc_hlos MASTER_APPSS_PROC + &rsc_hlos SLAVE_USB3_0>; + interconnect-names = "usb-ddr", "apps-usb"; + usb_1_dwc3: dwc3@a600000 { compatible = "snps,dwc3"; reg = <0 0x0a600000 0 0xcd00>; @@ -1881,6 +1887,12 @@ resets = <&gcc GCC_USB30_SEC_BCR>; + interconnects = <&rsc_hlos MASTER_USB3_1 + &rsc_hlos SLAVE_EBI1>, + <&rsc_hlos MASTER_APPSS_PROC + &rsc_hlos SLAVE_USB3_1>; + interconnect-names = "usb-ddr", "apps-usb"; + usb_2_dwc3: dwc3@a800000 { compatible = "snps,dwc3"; reg = <0 0x0a800000 0 0xcd00>; -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 3/3] arm64: dts: sdm845: Add interconnect properties for USB 2019-09-16 11:41 ` [PATCH V2 3/3] arm64: dts: sdm845: Add interconnect properties for USB Chandana Kishori Chiluveru @ 2019-09-16 22:56 ` Matthias Kaehlcke 0 siblings, 0 replies; 9+ messages in thread From: Matthias Kaehlcke @ 2019-09-16 22:56 UTC (permalink / raw) To: Chandana Kishori Chiluveru Cc: balbi, agross, david.brown, linux-usb, linux-arm-msm Hi Chandana. On Mon, Sep 16, 2019 at 05:11:01PM +0530, Chandana Kishori Chiluveru wrote: > Populate USB DT node with interconnect properties. nit: nodes > Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index fcb9330..1c41922 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -1837,6 +1837,12 @@ > > resets = <&gcc GCC_USB30_PRIM_BCR>; > > + interconnects = <&rsc_hlos MASTER_USB3_0 > + &rsc_hlos SLAVE_EBI1>, nit: align second line after '<' for better readability (like 'interrupts' or 'clocks' properties): interconnects = <&rsc_hlos MASTER_USB3_0 &rsc_hlos SLAVE_EBI1>, same for the other entries. > + <&rsc_hlos MASTER_APPSS_PROC > + &rsc_hlos SLAVE_USB3_0>; > + interconnect-names = "usb-ddr", "apps-usb"; > + > usb_1_dwc3: dwc3@a600000 { > compatible = "snps,dwc3"; > reg = <0 0x0a600000 0 0xcd00>; > @@ -1881,6 +1887,12 @@ > > resets = <&gcc GCC_USB30_SEC_BCR>; > > + interconnects = <&rsc_hlos MASTER_USB3_1 > + &rsc_hlos SLAVE_EBI1>, > + <&rsc_hlos MASTER_APPSS_PROC > + &rsc_hlos SLAVE_USB3_1>; > + interconnect-names = "usb-ddr", "apps-usb"; > + > usb_2_dwc3: dwc3@a800000 { > compatible = "snps,dwc3"; > reg = <0 0x0a800000 0 0xcd00>; Besides the nits: Reviewed-by: Matthias Kaehlcke <mka@chromium.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 0/3] ADD interconnect support for USB 2019-09-16 11:40 [PATCH V2 0/3] ADD interconnect support for USB Chandana Kishori Chiluveru ` (2 preceding siblings ...) 2019-09-16 11:41 ` [PATCH V2 3/3] arm64: dts: sdm845: Add interconnect properties for USB Chandana Kishori Chiluveru @ 2019-09-17 3:54 ` Manu Gautam 3 siblings, 0 replies; 9+ messages in thread From: Manu Gautam @ 2019-09-17 3:54 UTC (permalink / raw) To: Chandana Kishori Chiluveru, balbi, agross, david.brown Cc: linux-usb, linux-arm-msm On 9/16/2019 5:10 PM, Chandana Kishori Chiluveru wrote: > This path series aims to add interconnect support in > dwc3-qcom driver on SDM845 SoCs. As Matthias commented on other patch, can you update the subject to mention that series is for dwc3-qcom only? And update same dt-bindings patch commit subject as well > > Changes since V1: > > Comments by Georgi Djakov on "[PATCH 2/3]" addressed > > [PATCH 1/3] and [PATCH 3/3] remains unchanged > > Chandana Kishori Chiluveru (3): > dt-bindings: Introduce interconnect bindings for usb > usb: dwc3: qcom: Add interconnect support in dwc3 driver > arm64: dts: sdm845: Add interconnect properties for USB > > .../devicetree/bindings/usb/qcom,dwc3.txt | 13 ++ > arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++ > drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++- > 3 files changed, 168 insertions(+), 2 deletions(-) > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-09-17 17:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-16 11:40 [PATCH V2 0/3] ADD interconnect support for USB Chandana Kishori Chiluveru 2019-09-16 11:40 ` [PATCH V2 1/3] dt-bindings: Introduce interconnect bindings for usb Chandana Kishori Chiluveru 2019-09-16 11:41 ` [PATCH V2 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver Chandana Kishori Chiluveru 2019-09-16 22:24 ` Matthias Kaehlcke 2019-09-17 11:09 ` cchiluve 2019-09-17 17:58 ` Matthias Kaehlcke 2019-09-16 11:41 ` [PATCH V2 3/3] arm64: dts: sdm845: Add interconnect properties for USB Chandana Kishori Chiluveru 2019-09-16 22:56 ` Matthias Kaehlcke 2019-09-17 3:54 ` [PATCH V2 0/3] ADD interconnect support " Manu Gautam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).