Re: [PATCH v3 1/3] usb: chipidea: ci_hdrc_imx: add wakeup clock and keep it always on

[Stefan Wahren <wahrenst@xxxxxxx>]

Hi Xu,

Am 28.12.23 um 12:07 schrieb Xu Yang:
> Some platform using ChipIdea IP  may keep 32KHz wakeup clock always
> on without usb driver intervention. And some may need driver to handle
> this clock. For now only i.MX93 needs this wakeup clock. This patch will
> get wakeup clock and keep it always on to make controller work properly.
>
> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> Acked-by: Peter Chen <peter.chen@xxxxxxxxxx>
i've tested successfully the whole series on a custom i.MX28 board
(EVAcharge SE).

Tested-by: Stefan Wahren <wahrenst@xxxxxxx>

Looking deeper into this patch i'm a little bit confused. The commit
message suggests that this patch introduce USB support for i.MX93, but
ci-hdrc-usb2.yaml and imx93.dtsi doesn't contain the necessary parts.
After a quick search i noticed a separate pending series which contains
these parts.

https://lore.kernel.org/linux-arm-kernel/DB7PR04MB5146920CF37DBF212199C1958C95A@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/T/#u

Please keep in such a case: dt-bindings, driver and DTS changes in one
series. This makes reviewing much easier. Especially for a series
without any cover letter.

Best regards
> ---
> Changes in v2:
>   - modify commit msg to make it clear
> Changes in v3:
>   - use dev_err_probe as suggested by Stefan Wahren
> ---
>   drivers/usb/chipidea/ci_hdrc_imx.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index e28bb2f2612d..ae9a6a17ec6e 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -96,6 +96,7 @@ struct ci_hdrc_imx_data {
>   	struct usb_phy *phy;
>   	struct platform_device *ci_pdev;
>   	struct clk *clk;
> +	struct clk *clk_wakeup;
>   	struct imx_usbmisc_data *usbmisc_data;
>   	bool supports_runtime_pm;
>   	bool override_phy_control;
> @@ -199,7 +200,7 @@ static int imx_get_clks(struct device *dev)
>
>   	data->clk_ipg = devm_clk_get(dev, "ipg");
>   	if (IS_ERR(data->clk_ipg)) {
> -		/* If the platform only needs one clocks */
> +		/* If the platform only needs one primary clock */
>   		data->clk = devm_clk_get(dev, NULL);
>   		if (IS_ERR(data->clk)) {
>   			ret = PTR_ERR(data->clk);
> @@ -208,6 +209,13 @@ static int imx_get_clks(struct device *dev)
>   				PTR_ERR(data->clk), PTR_ERR(data->clk_ipg));
>   			return ret;
>   		}
> +		/* Get wakeup clock. Not all of the platforms need to
> +		 * handle this clock. So make it optional.
> +		 */
> +		data->clk_wakeup = devm_clk_get_optional(dev, "usb_wakeup_clk");
> +		if (IS_ERR(data->clk_wakeup))
> +			ret = dev_err_probe(dev, PTR_ERR(data->clk_wakeup),
> +					"Failed to get wakeup clk\n");
>   		return ret;
>   	}
>
> @@ -423,6 +431,10 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto disable_hsic_regulator;
>
> +	ret = clk_prepare_enable(data->clk_wakeup);
> +	if (ret)
> +		goto err_wakeup_clk;
> +
>   	data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
>   	if (IS_ERR(data->phy)) {
>   		ret = PTR_ERR(data->phy);
> @@ -504,6 +516,8 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>   disable_device:
>   	ci_hdrc_remove_device(data->ci_pdev);
>   err_clk:
> +	clk_disable_unprepare(data->clk_wakeup);
> +err_wakeup_clk:
>   	imx_disable_unprepare_clks(dev);
>   disable_hsic_regulator:
>   	if (data->hsic_pad_regulator)
> @@ -530,6 +544,7 @@ static void ci_hdrc_imx_remove(struct platform_device *pdev)
>   		usb_phy_shutdown(data->phy);
>   	if (data->ci_pdev) {
>   		imx_disable_unprepare_clks(&pdev->dev);
> +		clk_disable_unprepare(data->clk_wakeup);
>   		if (data->plat_data->flags & CI_HDRC_PMQOS)
>   			cpu_latency_qos_remove_request(&data->pm_qos_req);
>   		if (data->hsic_pad_regulator)