Message-ID: <20200929033655.GB9076@b29397-desktop>
Date:   Tue, 29 Sep 2020 03:37:59 +0000
From:   Peter Chen <peter.chen@....com>
To:     Pawel Laszczak <pawell@...ence.com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "colin.king@...onical.com" <colin.king@...onical.com>,
        "rogerq@...com" <rogerq@...com>,
        "jpawar@...ence.com" <jpawar@...ence.com>,
        "kurahul@...ence.com" <kurahul@...ence.com>,
        "sparmar@...ence.com" <sparmar@...ence.com>,
        "nsekhar@...com" <nsekhar@...com>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
        "chunfeng.yun@...iatek.com" <chunfeng.yun@...iatek.com>,
        "yanaijie@...wei.com" <yanaijie@...wei.com>
Subject: Re: [PATCH 2/8] usb: cdns3: Split core.c into cdns3-plat and core.c
 file

On 20-09-28 14:27:35, Pawel Laszczak wrote:
> Patch splits file core.c into core.c containing the common reusable code
> and cnd3-plat.c containing device platform specific code. These changes

cdns3-plat.c

Pawel, at 5.10-rc1, there are some cdns3 driver updates at Felipe's
next tree, you may create patches based on that to avoid rebase effects in
future.

Peter

> are required to make possible reuse DRD part of CDNS3 driver in CDNSP
> driver.
> 
> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
> ---
>  drivers/usb/cdns3/Makefile     |   2 +-
>  drivers/usb/cdns3/cdns3-plat.c | 209 +++++++++++++++++++++++++++++++++
>  drivers/usb/cdns3/core.c       | 181 +++-------------------------
>  drivers/usb/cdns3/core.h       |   8 +-
>  4 files changed, 234 insertions(+), 166 deletions(-)
>  create mode 100644 drivers/usb/cdns3/cdns3-plat.c
> 
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index d47e341a6f39..a1fe9612053a 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -2,7 +2,7 @@
>  # define_trace.h needs to know how to find our header
>  CFLAGS_trace.o				:= -I$(src)
>  
> -cdns3-y					:= core.o drd.o
> +cdns3-y					:= cdns3-plat.o core.o drd.o
>  
>  obj-$(CONFIG_USB_CDNS3)			+= cdns3.o
>  cdns3-$(CONFIG_USB_CDNS3_GADGET)	+= gadget.o ep0.o
> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> new file mode 100644
> index 000000000000..f35e9dca30fe
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdns3-plat.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence USBSS DRD Driver.
> + *
> + * Copyright (C) 2018-2020 Cadence.
> + * Copyright (C) 2017-2018 NXP
> + * Copyright (C) 2019 Texas Instrumentsq
> + *
> + *
> + * Author: Peter Chen <peter.chen@....com>
> + *         Pawel Laszczak <pawell@...ence.com>
> + *         Roger Quadros <rogerq@...com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "core.h"
> +
> +/**
> + * cdns3_plat_probe - probe for cdns3 core device
> + * @pdev: Pointer to cdns3 core platform device
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_plat_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource	*res;
> +	struct cdns3 *cdns;
> +	void __iomem *regs;
> +	int ret;
> +
> +	cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> +	if (!cdns)
> +		return -ENOMEM;
> +
> +	cdns->dev = dev;
> +
> +	platform_set_drvdata(pdev, cdns);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "host");
> +	if (!res) {
> +		dev_err(dev, "missing host IRQ\n");
> +		return -ENODEV;
> +	}
> +
> +	cdns->xhci_res[0] = *res;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "xhci");
> +	if (!res) {
> +		dev_err(dev, "couldn't get xhci resource\n");
> +		return -ENXIO;
> +	}
> +
> +	cdns->xhci_res[1] = *res;
> +
> +	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
> +	if (cdns->dev_irq == -EPROBE_DEFER)
> +		return cdns->dev_irq;
> +
> +	if (cdns->dev_irq < 0)
> +		dev_err(dev, "couldn't get peripheral irq\n");
> +
> +	regs = devm_platform_ioremap_resource_byname(pdev, "dev");
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +	cdns->dev_regs	= regs;
> +
> +	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> +	if (cdns->otg_irq == -EPROBE_DEFER)
> +		return cdns->otg_irq;
> +
> +	if (cdns->otg_irq < 0) {
> +		dev_err(dev, "couldn't get otg irq\n");
> +		return cdns->otg_irq;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
> +	if (!res) {
> +		dev_err(dev, "couldn't get otg resource\n");
> +		return -ENXIO;
> +	}
> +
> +	cdns->otg_res = *res;
> +
> +	cdns->usb2_phy = devm_phy_optional_get(dev, "cdns3,usb2-phy");
> +	if (IS_ERR(cdns->usb2_phy))
> +		return PTR_ERR(cdns->usb2_phy);
> +
> +	ret = phy_init(cdns->usb2_phy);
> +	if (ret)
> +		return ret;
> +
> +	cdns->usb3_phy = devm_phy_optional_get(dev, "cdns3,usb3-phy");
> +	if (IS_ERR(cdns->usb3_phy))
> +		return PTR_ERR(cdns->usb3_phy);
> +
> +	ret = phy_init(cdns->usb3_phy);
> +	if (ret)
> +		goto err_phy3_init;
> +
> +	ret = phy_power_on(cdns->usb2_phy);
> +	if (ret)
> +		goto err_phy2_power_on;
> +
> +	ret = phy_power_on(cdns->usb3_phy);
> +	if (ret)
> +		goto err_phy3_power_on;
> +
> +	ret = cdns3_init(cdns);
> +	if (ret)
> +		goto err_cdns_init;
> +
> +	device_set_wakeup_capable(dev, true);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	/*
> +	 * The controller needs less time between bus and controller suspend,
> +	 * and we also needs a small delay to avoid frequently entering low
> +	 * power mode.
> +	 */
> +	pm_runtime_set_autosuspend_delay(dev, 20);
> +	pm_runtime_mark_last_busy(dev);
> +
> +	return 0;
> +
> +err_cdns_init:
> +	phy_power_off(cdns->usb3_phy);
> +err_phy3_power_on:
> +	phy_power_off(cdns->usb2_phy);
> +err_phy2_power_on:
> +	phy_exit(cdns->usb3_phy);
> +err_phy3_init:
> +	phy_exit(cdns->usb2_phy);
> +
> +	return ret;
> +}
> +
> +/**
> + * cdns3_remove - unbind drd driver and clean up
> + * @pdev: Pointer to Linux platform device
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_plat_remove(struct platform_device *pdev)
> +{
> +	struct cdns3 *cdns = platform_get_drvdata(pdev);
> +	struct device *dev = cdns->dev;
> +
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	cdns3_remove(cdns);
> +	phy_power_off(cdns->usb2_phy);
> +	phy_power_off(cdns->usb3_phy);
> +	phy_exit(cdns->usb2_phy);
> +	phy_exit(cdns->usb3_phy);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +
> +static int cdns3_plat_suspend(struct device *dev)
> +{
> +	struct cdns3 *cdns = dev_get_drvdata(dev);
> +
> +	return cdns3_suspend(cdns);
> +}
> +
> +static int cdns3_plat_resume(struct device *dev)
> +{
> +	struct cdns3 *cdns = dev_get_drvdata(dev);
> +
> +	return cdns3_resume(cdns);
> +}
> +#endif
> +
> +static const struct dev_pm_ops cdns3_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(cdns3_plat_suspend, cdns3_plat_resume)
> +};
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_cdns3_match[] = {
> +	{ .compatible = "cdns,usb3" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_cdns3_match);
> +#endif
> +
> +static struct platform_driver cdns3_driver = {
> +	.probe		= cdns3_plat_probe,
> +	.remove		= cdns3_plat_remove,
> +	.driver		= {
> +		.name	= "cdns-usb3",
> +		.of_match_table	= of_match_ptr(of_cdns3_match),
> +		.pm	= &cdns3_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(cdns3_driver);
> +
> +MODULE_ALIAS("platform:cdns3");
> +MODULE_AUTHOR("Pawel Laszczak <pawell@...ence.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 371128e9a4ae..079bd2abf65d 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -2,7 +2,7 @@
>  /*
>   * Cadence USBSS DRD Driver.
>   *
> - * Copyright (C) 2018-2019 Cadence.
> + * Copyright (C) 2018-2020 Cadence.
>   * Copyright (C) 2017-2018 NXP
>   * Copyright (C) 2019 Texas Instruments
>   *
> @@ -383,17 +383,14 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
>  
>  /**
>   * cdns3_probe - probe for cdns3 core device
> - * @pdev: Pointer to cdns3 core platform device
> + * @cdns: Pointer to cdnsp structure.
>   *
>   * Returns 0 on success otherwise negative errno
>   */
> -static int cdns3_probe(struct platform_device *pdev)
> +int cdns3_init(struct cdns3 *cdns)
>  {
>  	struct usb_role_switch_desc sw_desc = { };
> -	struct device *dev = &pdev->dev;
> -	struct resource	*res;
> -	struct cdns3 *cdns;
> -	void __iomem *regs;
> +	struct device *dev = cdns->dev;
>  	int ret;
>  
>  	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> @@ -402,85 +399,8 @@ static int cdns3_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> -	if (!cdns)
> -		return -ENOMEM;
> -
> -	cdns->dev = dev;
> -
> -	platform_set_drvdata(pdev, cdns);
> -
> -	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "host");
> -	if (!res) {
> -		dev_err(dev, "missing host IRQ\n");
> -		return -ENODEV;
> -	}
> -
> -	cdns->xhci_res[0] = *res;
> -
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "xhci");
> -	if (!res) {
> -		dev_err(dev, "couldn't get xhci resource\n");
> -		return -ENXIO;
> -	}
> -
> -	cdns->xhci_res[1] = *res;
> -
> -	cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
> -	if (cdns->dev_irq == -EPROBE_DEFER)
> -		return cdns->dev_irq;
> -
> -	if (cdns->dev_irq < 0)
> -		dev_err(dev, "couldn't get peripheral irq\n");
> -
> -	regs = devm_platform_ioremap_resource_byname(pdev, "dev");
> -	if (IS_ERR(regs))
> -		return PTR_ERR(regs);
> -	cdns->dev_regs	= regs;
> -
> -	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> -	if (cdns->otg_irq == -EPROBE_DEFER)
> -		return cdns->otg_irq;
> -
> -	if (cdns->otg_irq < 0) {
> -		dev_err(dev, "couldn't get otg irq\n");
> -		return cdns->otg_irq;
> -	}
> -
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
> -	if (!res) {
> -		dev_err(dev, "couldn't get otg resource\n");
> -		return -ENXIO;
> -	}
> -
> -	cdns->otg_res = *res;
> -
>  	mutex_init(&cdns->mutex);
>  
> -	cdns->usb2_phy = devm_phy_optional_get(dev, "cdns3,usb2-phy");
> -	if (IS_ERR(cdns->usb2_phy))
> -		return PTR_ERR(cdns->usb2_phy);
> -
> -	ret = phy_init(cdns->usb2_phy);
> -	if (ret)
> -		return ret;
> -
> -	cdns->usb3_phy = devm_phy_optional_get(dev, "cdns3,usb3-phy");
> -	if (IS_ERR(cdns->usb3_phy))
> -		return PTR_ERR(cdns->usb3_phy);
> -
> -	ret = phy_init(cdns->usb3_phy);
> -	if (ret)
> -		goto err1;
> -
> -	ret = phy_power_on(cdns->usb2_phy);
> -	if (ret)
> -		goto err2;
> -
> -	ret = phy_power_on(cdns->usb3_phy);
> -	if (ret)
> -		goto err3;
> -
>  	sw_desc.set = cdns3_role_set;
>  	sw_desc.get = cdns3_role_get;
>  	sw_desc.allow_userspace_control = true;
> @@ -490,78 +410,47 @@ static int cdns3_probe(struct platform_device *pdev)
>  
>  	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
>  	if (IS_ERR(cdns->role_sw)) {
> -		ret = PTR_ERR(cdns->role_sw);
>  		dev_warn(dev, "Unable to register Role Switch\n");
> -		goto err4;
> +		return PTR_ERR(cdns->role_sw);
>  	}
>  
>  	ret = cdns3_drd_init(cdns);
>  	if (ret)
> -		goto err5;
> +		goto init_failed;
>  
>  	ret = cdns3_core_init_role(cdns);
>  	if (ret)
> -		goto err5;
> -
> -	device_set_wakeup_capable(dev, true);
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> +		goto init_failed;
>  
> -	/*
> -	 * The controller needs less time between bus and controller suspend,
> -	 * and we also needs a small delay to avoid frequently entering low
> -	 * power mode.
> -	 */
> -	pm_runtime_set_autosuspend_delay(dev, 20);
> -	pm_runtime_mark_last_busy(dev);
> -	pm_runtime_use_autosuspend(dev);
>  	dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
>  
>  	return 0;
> -err5:
> +init_failed:
>  	cdns3_drd_exit(cdns);
>  	usb_role_switch_unregister(cdns->role_sw);
> -err4:
> -	phy_power_off(cdns->usb3_phy);
> -
> -err3:
> -	phy_power_off(cdns->usb2_phy);
> -err2:
> -	phy_exit(cdns->usb3_phy);
> -err1:
> -	phy_exit(cdns->usb2_phy);
>  
>  	return ret;
>  }
>  
>  /**
>   * cdns3_remove - unbind drd driver and clean up
> - * @pdev: Pointer to Linux platform device
> + * @cdns: Pointer to cdnsp structure.
>   *
>   * Returns 0 on success otherwise negative errno
>   */
> -static int cdns3_remove(struct platform_device *pdev)
> +int cdns3_remove(struct cdns3 *cdns)
>  {
> -	struct cdns3 *cdns = platform_get_drvdata(pdev);
> -
> -	pm_runtime_get_sync(&pdev->dev);
> -	pm_runtime_disable(&pdev->dev);
> -	pm_runtime_put_noidle(&pdev->dev);
>  	cdns3_exit_roles(cdns);
>  	usb_role_switch_unregister(cdns->role_sw);
> -	phy_power_off(cdns->usb2_phy);
> -	phy_power_off(cdns->usb3_phy);
> -	phy_exit(cdns->usb2_phy);
> -	phy_exit(cdns->usb3_phy);
> +
>  	return 0;
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
>  
> -static int cdns3_suspend(struct device *dev)
> +int cdns3_suspend(struct cdns3 *cdns)
>  {
> -	struct cdns3 *cdns = dev_get_drvdata(dev);
> -	unsigned long flags;
> +	struct device *dev = cdns->dev;
>  
>  	if (cdns->role == USB_ROLE_HOST)
>  		return 0;
> @@ -569,28 +458,21 @@ static int cdns3_suspend(struct device *dev)
>  	if (pm_runtime_status_suspended(dev))
>  		pm_runtime_resume(dev);
>  
> -	if (cdns->roles[cdns->role]->suspend) {
> -		spin_lock_irqsave(&cdns->gadget_dev->lock, flags);
> +	if (cdns->roles[cdns->role]->suspend)
>  		cdns->roles[cdns->role]->suspend(cdns, false);
> -		spin_unlock_irqrestore(&cdns->gadget_dev->lock, flags);
> -	}
>  
>  	return 0;
>  }
>  
> -static int cdns3_resume(struct device *dev)
> +int cdns3_resume(struct cdns3 *cdns)
>  {
> -	struct cdns3 *cdns = dev_get_drvdata(dev);
> -	unsigned long flags;
> +	struct device *dev = cdns->dev;
>  
>  	if (cdns->role == USB_ROLE_HOST)
>  		return 0;
>  
> -	if (cdns->roles[cdns->role]->resume) {
> -		spin_lock_irqsave(&cdns->gadget_dev->lock, flags);
> +	if (cdns->roles[cdns->role]->resume)
>  		cdns->roles[cdns->role]->resume(cdns, false);
> -		spin_unlock_irqrestore(&cdns->gadget_dev->lock, flags);
> -	}
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_active(dev);
> @@ -599,32 +481,3 @@ static int cdns3_resume(struct device *dev)
>  	return 0;
>  }
>  #endif
> -
> -static const struct dev_pm_ops cdns3_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
> -};
> -
> -#ifdef CONFIG_OF
> -static const struct of_device_id of_cdns3_match[] = {
> -	{ .compatible = "cdns,usb3" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, of_cdns3_match);
> -#endif
> -
> -static struct platform_driver cdns3_driver = {
> -	.probe		= cdns3_probe,
> -	.remove		= cdns3_remove,
> -	.driver		= {
> -		.name	= "cdns-usb3",
> -		.of_match_table	= of_match_ptr(of_cdns3_match),
> -		.pm	= &cdns3_pm_ops,
> -	},
> -};
> -
> -module_platform_driver(cdns3_driver);
> -
> -MODULE_ALIAS("platform:cdns3");
> -MODULE_AUTHOR("Pawel Laszczak <pawell@...ence.com>");
> -MODULE_LICENSE("GPL v2");
> -MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> index c09fdde3ae8f..284707c19620 100644
> --- a/drivers/usb/cdns3/core.h
> +++ b/drivers/usb/cdns3/core.h
> @@ -3,7 +3,7 @@
>   * Cadence USBSS DRD Header File.
>   *
>   * Copyright (C) 2017-2018 NXP
> - * Copyright (C) 2018-2019 Cadence.
> + * Copyright (C) 2018-2020 Cadence.
>   *
>   * Authors: Peter Chen <peter.chen@....com>
>   *          Pawel Laszczak <pawell@...ence.com>
> @@ -97,5 +97,11 @@ struct cdns3 {
>  };
>  
>  int cdns3_hw_role_switch(struct cdns3 *cdns);
> +int cdns3_init(struct cdns3 *cdns);
> +int cdns3_remove(struct cdns3 *cdns);
>  
> +#ifdef CONFIG_PM_SLEEP
> +int cdns3_resume(struct cdns3 *cdns);
> +int cdns3_suspend(struct cdns3 *cdns);
> +#endif /* CONFIG_PM_SLEEP */
>  #endif /* __LINUX_CDNS3_CORE_H */
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen

Powered by blists - more mailing lists