Hi Chandana,
On Fri, Sep 20, 2019 at 06:23:16PM +0530, cchiluve wrote:
> From: Chandana Kishori Chiluveru <cchiluve@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> ---
> 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..3a540f7 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)
typically constants are defined before the function and struct
definitions, I'd suggest to move this up, after the other
constant definitions.
> +/**
> + * dwc3_qcom_interconnect_init() - Initialize the interconnect
> + * @qcom: Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> + struct device *dev = qcom->dev;
> + struct dwc3 *dwc;
The variable is not used, remove it.
> + int ret;
> +
> + if (!device_is_bound(&qcom->dwc3->dev)) {
> + dev_err(qcom->dev, "device is not bound\n");
An error message seems very harsh for a deferred probe. I'd say remove
the logging altogether ("git grep -B3 'return -EPROBE_DEFER'" shows that
there's rarely a specific message logged when the probe is deferred), if
you really really want it downgrade it to dev_dbg() or dev_info().
> + return -EPROBE_DEFER;
> + }
> +
> + 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);
> + 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 = platform_get_drvdata(qcom->dwc3);
> + int ret;
> +
> + 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;
Since the error check is the same for high speed and super speed
it can be moved outside of the branch.
> + }
> +
> + 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;
I'm not convinced the use of goto simplifies the code in this case. It
could just be:
if (ret)
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);
> +
> + 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: %d\n", ret);
Not sure if this message is really needed, dwc3_qcom_interconnect_init()
logs a message in all error paths (except maybe for -EPROBE_DEFER in the
future). At the very least this message should be omitted for -EPROBE_DEFER.
