* Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver
2019-09-17 11:10 ` [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver Chandana Kishori Chiluveru
@ 2019-09-17 19:44 ` Matthias Kaehlcke
2020-02-04 19:05 ` Evan Green
2020-02-04 21:25 ` Evan Green
2 siblings, 0 replies; 12+ messages in thread
From: Matthias Kaehlcke @ 2019-09-17 19:44 UTC (permalink / raw)
To: Chandana Kishori Chiluveru
Cc: balbi, agross, david.brown, linux-usb, linux-arm-msm
On Tue, Sep 17, 2019 at 04:40:48PM +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..2a2f5af 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>
> ...
>
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
nit: "Initialize the interconnect" or similar?
> + * @qcom: Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
> + int ret;
> +
> + 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 usb-ddr path\n",
> + PTR_ERR(qcom->usb_ddr_icc_path));
> + 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 apps-usb path\n",
> + PTR_ERR(qcom->apps_usb_icc_path));
> + return PTR_ERR(qcom->apps_usb_icc_path);
> + }
> +
> + ret = dwc3_qcom_interconnect_enable(qcom);
> + if (ret) {
> + dev_err(dev, "failed to enable interconnect %d\n", ret);
nit: 'interconnect: %d', otherwise the message could be read as
'failed to enable interconnect number N'.
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * dwc3_qcom_interconnect_exit() - Release interconnect path handles
> + * @qcom: Pointer to the concerned usb core.
> + *
> + * This function is used to release interconnect path handle.
> + */
> +static void dwc3_qcom_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 dwc3_qcom_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;
> + }
I understand the need for the/some check (see
https://patchwork.kernel.org/patch/11146903/#22885491 and my reply),
but I'm not convinced it should be done here. The function can be
called from other contexts than _probe(), so returning -EPROBE_DEFER
seems wrong, although I understand that you want _probe() to return
this value.
I would suggest to do this (or another) check early in
_probe(). Returning -EPROBE_DEFER from that context makes sense, and
it should be the only time the check is actually needed.
As commented on v2 I don't particularly like the idea of using a half
initialized struct (dwc3), even more when what is initialized or not
varies at runtime (since dwc3_probe() and qwc3_qcom_probe() appear to
run in parallel). IIUC device_is_bound() returns true when a device is
fully initialized (i.e. it's _probe() completed), I'd suggest to
evaluate to use it instead of checking the drvdata. In this case the
drvdata check should be ok, but in general these kind of races don't
seem a good idea, maybe tomorrow someone wants to access another
struct member, which isn't always initialized.
Others: please correct me if using half-initialized structs is
routine in kernel drivers or device_is_bound() is the wrong tool.
> +
> + 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 dwc3_qcom_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);
This is essentially the same as in _interconnect_enable(). You could
consider a helper function (inline?) for 'enabling' the memory path,
which would make things more compact and also allow to get rid of the
goto:
if (ret)
qcom_dwc3_interconnect_enable_mempath(qcom);
return ret;
> +
> + return ret;
> +}
> +
> static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
> {
> struct dwc3_qcom *qcom = data;
> @@ -494,6 +626,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> goto depopulate;
> }
>
> + ret = dwc3_qcom_interconnect_init(qcom);
> + if (ret) {
> + dev_err(dev, "failed to init interconnect handles %d\n", ret);
nit: remove 'handles' (and add ':' before the error code), it's an
implementation detail, and now _icc_init() also calls _icc_enable(),
hence it's not only the handles.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver
2019-09-17 11:10 ` [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver Chandana Kishori Chiluveru
2019-09-17 19:44 ` Matthias Kaehlcke
@ 2020-02-04 19:05 ` Evan Green
2020-02-04 19:16 ` Matthias Kaehlcke
2020-02-04 21:25 ` Evan Green
2 siblings, 1 reply; 12+ messages in thread
From: Evan Green @ 2020-02-04 19:05 UTC (permalink / raw)
To: Chandana Kishori Chiluveru
Cc: balbi, Andy Gross, David Brown, linux-usb, linux-arm-msm,
Matthias Kaehlcke
Hi Chandana,
Are you going to spin this series? My comments are below, I see
Matthias also has comments. Please CC us both on the next spin.
-Evan
On Tue, Sep 17, 2019 at 4:11 AM Chandana Kishori Chiluveru
<cchiluve@codeaurora.org> 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..2a2f5af 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -12,6 +12,7 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/extcon.h>
> +#include <linux/interconnect.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/phy/phy.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 dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom);
> +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom);
Is there any reason you didn't just define these functions earlier?
> +
> 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,10 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> for (i = qcom->num_clocks - 1; i >= 0; i--)
> clk_disable_unprepare(qcom->clks[i]);
>
> + ret = dwc3_qcom_interconnect_disable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to disable interconnect %d\n", ret);
Shouldn't you propagate the failure if this doesn't work?
> +
> qcom->is_suspended = true;
> dwc3_qcom_enable_interrupts(qcom);
>
> @@ -259,6 +269,10 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> }
> }
>
> + ret = dwc3_qcom_interconnect_enable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to enable interconnect %d\n", ret);
Same here, isn't this important? In theory, if you cannot enable
bandwidth to the device, you aren't really allowed to touch it (with
odd exceptions for boot proxy votes, which might now be gone).
> +
> /* 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 +282,124 @@ 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)
Can you share at all how these numbers were arrived at? I thought HS
for instance was 480MB/s.
> +
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> + * @qcom: Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
> + int ret;
> +
> + 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 usb-ddr path\n",
> + PTR_ERR(qcom->usb_ddr_icc_path));
> + 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 apps-usb path\n",
> + PTR_ERR(qcom->apps_usb_icc_path));
You're leaking usb_ddr_icc_path here.
> + return PTR_ERR(qcom->apps_usb_icc_path);
> + }
> +
> + ret = dwc3_qcom_interconnect_enable(qcom);
> + if (ret) {
> + dev_err(dev, "failed to enable interconnect %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * dwc3_qcom_interconnect_exit() - Release interconnect path handles
> + * @qcom: Pointer to the concerned usb core.
> + *
> + * This function is used to release interconnect path handle.
> + */
> +static void dwc3_qcom_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 dwc3_qcom_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 dwc3_qcom_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);
Yeah, this is weird. If you're failing to clear your votes, things are
in a pretty broken state, and these calls to re-enable bandwidth have
almost no chance of succeeding. Maybe just return failure here.
> +
> + return ret;
> +}
> +
> static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
> {
> struct dwc3_qcom *qcom = data;
> @@ -494,6 +626,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> goto depopulate;
> }
>
> + ret = dwc3_qcom_interconnect_init(qcom);
> + if (ret) {
> + dev_err(dev, "failed to init interconnect handles %d\n", ret);
> + goto depopulate;
> + }
> +
> 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:
> + dwc3_qcom_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;
>
> + dwc3_qcom_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 [flat|nested] 12+ messages in thread* Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver
2020-02-04 19:05 ` Evan Green
@ 2020-02-04 19:16 ` Matthias Kaehlcke
2020-02-04 19:28 ` Evan Green
0 siblings, 1 reply; 12+ messages in thread
From: Matthias Kaehlcke @ 2020-02-04 19:16 UTC (permalink / raw)
To: Evan Green
Cc: Chandana Kishori Chiluveru, balbi, Andy Gross, David Brown,
linux-usb, linux-arm-msm
On Tue, Feb 04, 2020 at 11:05:41AM -0800, Evan Green wrote:
> Hi Chandana,
> Are you going to spin this series? My comments are below, I see
> Matthias also has comments. Please CC us both on the next spin.
v4 was actually posted a few days later:
https://patchwork.kernel.org/project/linux-usb/list/?series=176783
I also missed it since neither linux-arm-msm@ nor myself are in cc.
> On Tue, Sep 17, 2019 at 4:11 AM Chandana Kishori Chiluveru
> <cchiluve@codeaurora.org> 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..2a2f5af 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -12,6 +12,7 @@
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/extcon.h>
> > +#include <linux/interconnect.h>
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > #include <linux/phy/phy.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 dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom);
> > +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom);
>
> Is there any reason you didn't just define these functions earlier?
>
> > +
> > 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,10 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> > for (i = qcom->num_clocks - 1; i >= 0; i--)
> > clk_disable_unprepare(qcom->clks[i]);
> >
> > + ret = dwc3_qcom_interconnect_disable(qcom);
> > + if (ret)
> > + dev_warn(qcom->dev, "failed to disable interconnect %d\n", ret);
>
> Shouldn't you propagate the failure if this doesn't work?
>
> > +
> > qcom->is_suspended = true;
> > dwc3_qcom_enable_interrupts(qcom);
> >
> > @@ -259,6 +269,10 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> > }
> > }
> >
> > + ret = dwc3_qcom_interconnect_enable(qcom);
> > + if (ret)
> > + dev_warn(qcom->dev, "failed to enable interconnect %d\n", ret);
>
> Same here, isn't this important? In theory, if you cannot enable
> bandwidth to the device, you aren't really allowed to touch it (with
> odd exceptions for boot proxy votes, which might now be gone).
>
> > +
> > /* 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 +282,124 @@ 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)
>
> Can you share at all how these numbers were arrived at? I thought HS
> for instance was 480MB/s.
>
> > +
> > +/**
> > + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> > + * @qcom: Pointer to the concerned usb core.
> > + *
> > + */
> > +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> > +{
> > + struct device *dev = qcom->dev;
> > + int ret;
> > +
> > + 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 usb-ddr path\n",
> > + PTR_ERR(qcom->usb_ddr_icc_path));
> > + 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 apps-usb path\n",
> > + PTR_ERR(qcom->apps_usb_icc_path));
>
> You're leaking usb_ddr_icc_path here.
>
> > + return PTR_ERR(qcom->apps_usb_icc_path);
> > + }
> > +
> > + ret = dwc3_qcom_interconnect_enable(qcom);
> > + if (ret) {
> > + dev_err(dev, "failed to enable interconnect %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * dwc3_qcom_interconnect_exit() - Release interconnect path handles
> > + * @qcom: Pointer to the concerned usb core.
> > + *
> > + * This function is used to release interconnect path handle.
> > + */
> > +static void dwc3_qcom_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 dwc3_qcom_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 dwc3_qcom_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);
>
> Yeah, this is weird. If you're failing to clear your votes, things are
> in a pretty broken state, and these calls to re-enable bandwidth have
> almost no chance of succeeding. Maybe just return failure here.
>
> > +
> > + return ret;
> > +}
> > +
> > static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
> > {
> > struct dwc3_qcom *qcom = data;
> > @@ -494,6 +626,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> > goto depopulate;
> > }
> >
> > + ret = dwc3_qcom_interconnect_init(qcom);
> > + if (ret) {
> > + dev_err(dev, "failed to init interconnect handles %d\n", ret);
> > + goto depopulate;
> > + }
> > +
> > 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:
> > + dwc3_qcom_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;
> >
> > + dwc3_qcom_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 [flat|nested] 12+ messages in thread* Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver
2020-02-04 19:16 ` Matthias Kaehlcke
@ 2020-02-04 19:28 ` Evan Green
0 siblings, 0 replies; 12+ messages in thread
From: Evan Green @ 2020-02-04 19:28 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Chandana Kishori Chiluveru, balbi, Andy Gross, David Brown,
linux-usb, linux-arm-msm
On Tue, Feb 4, 2020 at 11:17 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Feb 04, 2020 at 11:05:41AM -0800, Evan Green wrote:
> > Hi Chandana,
> > Are you going to spin this series? My comments are below, I see
> > Matthias also has comments. Please CC us both on the next spin.
>
> v4 was actually posted a few days later:
>
> https://patchwork.kernel.org/project/linux-usb/list/?series=176783
>
> I also missed it since neither linux-arm-msm@ nor myself are in cc.
Ah, thanks for the pointer, Matthias.
Yes, please CC linux-arm-msm@ and linux-kernel@ as well as us.
-Evan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver
2019-09-17 11:10 ` [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver Chandana Kishori Chiluveru
2019-09-17 19:44 ` Matthias Kaehlcke
2020-02-04 19:05 ` Evan Green
@ 2020-02-04 21:25 ` Evan Green
2 siblings, 0 replies; 12+ messages in thread
From: Evan Green @ 2020-02-04 21:25 UTC (permalink / raw)
To: Chandana Kishori Chiluveru
Cc: balbi, Andy Gross, David Brown, linux-usb, linux-arm-msm
On Tue, Sep 17, 2019 at 4:11 AM Chandana Kishori Chiluveru
<cchiluve@codeaurora.org> 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..2a2f5af 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -12,6 +12,7 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/extcon.h>
> +#include <linux/interconnect.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/phy/phy.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 dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom);
> +static int dwc3_qcom_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,10 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> for (i = qcom->num_clocks - 1; i >= 0; i--)
> clk_disable_unprepare(qcom->clks[i]);
>
> + ret = dwc3_qcom_interconnect_disable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to disable interconnect %d\n", ret);
> +
> qcom->is_suspended = true;
> dwc3_qcom_enable_interrupts(qcom);
>
> @@ -259,6 +269,10 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> }
> }
>
> + ret = dwc3_qcom_interconnect_enable(qcom);
> + if (ret)
> + dev_warn(qcom->dev, "failed to enable interconnect %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 +282,124 @@ 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)
> +
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles
> + * @qcom: Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
I'm not subscribed to linux-usb, so I don't have the V4 edition. So
I'm commenting here. In the v4 version there's a line:
struct dwc3 *dwc;
that results in an unused local variable compiler warning.
-Evan
^ permalink raw reply [flat|nested] 12+ messages in thread