Date:   Wed, 30 Sep 2020 05:22:53 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Chunfeng Yun <chunfeng.yun@...iatek.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>,
        "peter.chen@....com" <peter.chen@....com>,
        Jayshri Dajiram Pawar <jpawar@...ence.com>,
        Rahul Kumar <kurahul@...ence.com>,
        Sanket Parmar <sparmar@...ence.com>,
        "nsekhar@...com" <nsekhar@...com>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.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 Mon, 2020-09-28 at 14:27 +0200, 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
>> 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");
>Use platform_get_irq_byname_optional? otherwise no need print this log,
>platform_get_irq_byname() will print it.

Hi Chnfeng, 

It's the original code from core.c file. I don't want to change any part of code 
in original code core.c, except what is necessary to make the code reusable.
Maybe the better choice is to fix it in separate patch. 
I will send such patch before next version. 

Thanks 

>
>> +
>> +	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;
>> +	}
>ditto
>
>> +
>> +	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
>No need it if only supports devicetree

Thanks. I didn't know. I will change this. 

>
>> +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 */

Powered by blists - more mailing lists