Date:   Mon, 30 Oct 2017 09:02:01 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Kishon Vijay Abraham I <kishon@...com>,
        Raveendra Padasalagi <raveendra.padasalagi@...adcom.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Scott Branden <sbranden@...adcom.com>,
        Ray Jui <rjui@...adcom.com>,
        Srinath Mannam <srinath.mannam@...adcom.com>,
        Jon Mason <jonmason@...adcom.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Raviteja Garimella <raviteja.garimella@...adcom.com>,
        Rafal Milecki <rafal@...ecki.pl>,
        Arnd Bergmann <arnd@...db.de>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Jaehoon Chung <jh80.chung@...sung.com>
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com
Subject: Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB
 phy controller

Hi,

On 2017년 10월 27일 17:35, Kishon Vijay Abraham I wrote:
> +Chanwoo, for reviewing extcon
> 
> Hi.
> 
> On Tuesday 24 October 2017 10:07 AM, Raveendra Padasalagi wrote:
>> Add driver for Broadcom's USB phy controller's used in Cygnus
>> familyof SoC. Cygnus has three USB phy controller's, port 0,
>> port 1 provides USB host functionality and port 2 can be configured
>> for host/device role.
>>
>> Configuration of host/device role for port 2 is achieved based on
>> the extcon events, the driver registers to extcon framework to get
>> appropriate connect events for Host/Device cables connect/disconnect
>> states based on VBUS and ID interrupts.
>>
>> Signed-off-by: Raveendra Padasalagi <raveendra.padasalagi@...adcom.com>
>> ---
>>  drivers/phy/broadcom/Kconfig              |  14 +
>>  drivers/phy/broadcom/Makefile             |   1 +
>>  drivers/phy/broadcom/phy-bcm-cygnus-usb.c | 672 ++++++++++++++++++++++++++++++
>>  3 files changed, 687 insertions(+)
>>  create mode 100644 drivers/phy/broadcom/phy-bcm-cygnus-usb.c
>>
>> diff --git a/drivers/phy/broadcom/Kconfig b/drivers/phy/broadcom/Kconfig
>> index 64fc59c..3179daf 100644
>> --- a/drivers/phy/broadcom/Kconfig
>> +++ b/drivers/phy/broadcom/Kconfig
>> @@ -1,6 +1,20 @@
>>  #
>>  # Phy drivers for Broadcom platforms
>>  #
>> +config PHY_BCM_CYGNUS_USB
>> +	tristate "Broadcom Cygnus USB PHY support"
>> +	depends on OF
>> +	depends on ARCH_BCM_CYGNUS || COMPILE_TEST
>> +	select GENERIC_PHY
>> +	select EXTCON_USB_GPIO

IMO, it is not good to add the dependency to the specific device driver.
Instead, you better to depend on the EXTCON framework (select EXTCON)
such as 'select GENERIC_PHY'.

> 
> Didn't this throw up a warning for selecting config without caring for the
> dependency?
> 
>> +	default ARCH_BCM_CYGNUS
>> +	help
>> +	  Enable this to support three USB PHY's present in Broadcom's
>> +	  Cygnus chip.
>> +
>> +	  The phys are capable of supporting host mode on all ports and
>> +	  device mode for port 2.
>> +
>>  config PHY_CYGNUS_PCIE
>>  	tristate "Broadcom Cygnus PCIe PHY driver"
>>  	depends on OF && (ARCH_BCM_CYGNUS || COMPILE_TEST)
>> diff --git a/drivers/phy/broadcom/Makefile b/drivers/phy/broadcom/Makefile
>> index 4eb82ec..3dec23c 100644
>> --- a/drivers/phy/broadcom/Makefile
>> +++ b/drivers/phy/broadcom/Makefile
>> @@ -1,4 +1,5 @@
>>  obj-$(CONFIG_PHY_CYGNUS_PCIE)		+= phy-bcm-cygnus-pcie.o
>> +obj-$(CONFIG_PHY_BCM_CYGNUS_USB)	+= phy-bcm-cygnus-usb.o
>>  obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>>  obj-$(CONFIG_PHY_BCM_NS_USB2)		+= phy-bcm-ns-usb2.o
>>  obj-$(CONFIG_PHY_BCM_NS_USB3)		+= phy-bcm-ns-usb3.o
>> diff --git a/drivers/phy/broadcom/phy-bcm-cygnus-usb.c b/drivers/phy/broadcom/phy-bcm-cygnus-usb.c
>> new file mode 100644
>> index 0000000..ef2a94c
>> --- /dev/null
>> +++ b/drivers/phy/broadcom/phy-bcm-cygnus-usb.c
>> @@ -0,0 +1,672 @@
>> +/*
>> + * Copyright 2017 Broadcom
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation (the "GPL").
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License version 2 (GPLv2) for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * version 2 (GPLv2) along with this source code.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/delay.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/extcon.h>
>> +#include <linux/gpio.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> +
>> +#define CDRU_USBPHY_CLK_RST_SEL_OFFSET			0x0
>> +#define CDRU_USBPHY2_HOST_DEV_SEL_OFFSET		0x4
>> +#define CDRU_USB_DEV_SUSPEND_RESUME_CTRL_OFFSET		0x5C
>> +#define CDRU_USBPHY_P0_STATUS_OFFSET			0x1C
>> +#define CDRU_USBPHY_P1_STATUS_OFFSET			0x34
>> +#define CDRU_USBPHY_P2_STATUS_OFFSET			0x4C
> 
> Looks like it has 2 different blocks; CDRU and CMRU. Having a comment for each
> of the block will help.
>> +#define CRMU_USB_PHY_AON_CTRL_OFFSET			0x0
>> +
>> +#define CDRU_USBPHY_USBPHY_ILDO_ON_FLAG			BIT(1)
>> +#define CDRU_USBPHY_USBPHY_PLL_LOCK			BIT(0)
>> +#define CDRU_USB_DEV_SUSPEND_RESUME_CTRL_DISABLE	BIT(0)
>> +
>> +#define PHY2_DEV_HOST_CTRL_SEL_DEVICE			0
>> +#define PHY2_DEV_HOST_CTRL_SEL_HOST			1
>> +#define PHY2_DEV_HOST_CTRL_SEL_IDLE			2
>> +#define CRMU_USBPHY_P0_AFE_CORERDY_VDDC			BIT(1)
>> +#define CRMU_USBPHY_P0_RESETB				BIT(2)
>> +#define CRMU_USBPHY_P1_AFE_CORERDY_VDDC			BIT(9)
>> +#define CRMU_USBPHY_P1_RESETB				BIT(10)
>> +#define CRMU_USBPHY_P2_AFE_CORERDY_VDDC			BIT(17)
>> +#define CRMU_USBPHY_P2_RESETB				BIT(18)
>> +
>> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET		0x0408
>> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT_CLK_ENABLE	BIT(0)
>> +#define SUSPEND_OVERRIDE_0				13
>> +#define SUSPEND_OVERRIDE_1				14
>> +#define SUSPEND_OVERRIDE_2				15
>> +#define USB2_IDM_IDM_RESET_CONTROL_OFFSET		0x0800
>> +#define USB2_IDM_IDM_RESET_CONTROL__RESET		0
>> +#define USB2D_IDM_IDM_IO_SS_CLEAR_NAK_NEMPTY_EN_I	BIT(24)
>> +
>> +#define PLL_LOCK_RETRY_COUNT				1000
>> +#define MAX_REGULATOR_NAME_LEN				25
>> +#define DUAL_ROLE_PHY					2
>> +
>> +#define USBPHY_WQ_DELAY_MS		msecs_to_jiffies(500)
>> +#define USB2_SEL_DEVICE			0
>> +#define USB2_SEL_HOST			1
>> +#define USB2_SEL_IDLE			2
>> +#define USB_CONNECTED			1
>> +#define USB_DISCONNECTED		0
>> +#define MAX_NUM_PHYS			3
>> +
>> +static const int status_reg[] = {CDRU_USBPHY_P0_STATUS_OFFSET,
>> +				CDRU_USBPHY_P1_STATUS_OFFSET,
>> +				CDRU_USBPHY_P2_STATUS_OFFSET};
>> +
>> +struct cygnus_phy_instance;
>> +
>> +struct cygnus_phy_driver {
>> +	void __iomem *cdru_usbphy_regs;
>> +	void __iomem *crmu_usbphy_aon_ctrl_regs;
>> +	void __iomem *usb2h_idm_regs;
>> +	void __iomem *usb2d_idm_regs;
>> +	int num_phys;
>> +	bool idm_host_enabled;
>> +	struct cygnus_phy_instance *instances;
>> +	int phyto_src_clk;
>> +	struct platform_device *pdev;
>> +};
>> +
>> +struct cygnus_phy_instance {
>> +	struct cygnus_phy_driver *driver;
>> +	struct phy *generic_phy;
>> +	int port;
>> +	int new_state;		/* 1 - Host, 0 - device, 2 - idle*/
>> +	bool power;		/* 1 - powered_on 0 - powered off */
>> +	struct regulator *vbus_supply;
>> +	spinlock_t lock;
>> +	struct extcon_dev *edev;
>> +	struct notifier_block	device_nb;
>> +	struct notifier_block	host_nb;
>> +};
>> +
>> +static const unsigned int usb_extcon_cable[] = {
>> +	EXTCON_USB,
>> +	EXTCON_USB_HOST,
>> +	EXTCON_NONE,
>> +};

The usb_extcon_cable[] is not used in this driver.
Please remove it. This array is required on the extcon
provider driver. But, in this case, it is consumer driver.

>> +
>> +static inline int phy_pll_lock_stat(u32 usb_reg, int bit_mask,
>> +				    struct cygnus_phy_driver *phy_driver)
>> +{
>> +	/* Wait for the PLL lock status */
>> +	int retry = PLL_LOCK_RETRY_COUNT;
>> +	u32 reg_val;
>> +
>> +	do {
>> +		udelay(1);
>> +		reg_val = readl(phy_driver->cdru_usbphy_regs +
>> +				usb_reg);
>> +		if (reg_val & bit_mask)
>> +			return 0;
>> +	} while (--retry > 0);
>> +
>> +	return -EBUSY;
>> +}
>> +
>> +static struct phy *cygnus_phy_xlate(struct device *dev,
>> +				    struct of_phandle_args *args)
>> +{
>> +	struct cygnus_phy_driver *phy_driver = dev_get_drvdata(dev);
>> +	struct cygnus_phy_instance *instance_ptr;
>> +
>> +	if (!phy_driver)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	if (WARN_ON(args->args[0] >= phy_driver->num_phys))
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	if (WARN_ON(args->args_count < 1))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	instance_ptr = &phy_driver->instances[args->args[0]];
> 
> if the PHY consumer driver points the PHY sub-node, then of_xlate provided in
> phy core could be used instead of the custom xlate.
>> +	if (instance_ptr->port > phy_driver->phyto_src_clk)
>> +		phy_driver->phyto_src_clk = instance_ptr->port;
>> +
>> +	if (instance_ptr->port == DUAL_ROLE_PHY)
>> +		goto ret_p2;
>> +	phy_driver->instances[instance_ptr->port].new_state =
>> +						PHY2_DEV_HOST_CTRL_SEL_HOST;
>> +
>> +ret_p2:
>> +	return phy_driver->instances[instance_ptr->port].generic_phy;
>> +}
>> +
>> +static void cygnus_usbp2_dev_clock_init(struct phy *generic_phy, bool enable)
>> +{
>> +	u32 reg_val;
>> +	struct cygnus_phy_instance *instance_ptr = phy_get_drvdata(generic_phy);
>> +	struct cygnus_phy_driver *phy_driver = instance_ptr->driver;
>> +
>> +	reg_val = readl(phy_driver->usb2d_idm_regs +
>> +			USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +
>> +	if (enable)
>> +		reg_val |= USB2_IDM_IDM_IO_CONTROL_DIRECT_CLK_ENABLE;
>> +	else
>> +		reg_val &= ~USB2_IDM_IDM_IO_CONTROL_DIRECT_CLK_ENABLE;
>> +
>> +	writel(reg_val, phy_driver->usb2d_idm_regs +
>> +		USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +
>> +	reg_val = readl(phy_driver->usb2d_idm_regs +
>> +			USB2_IDM_IDM_RESET_CONTROL_OFFSET);
>> +
>> +	if (enable)
>> +		reg_val &= ~BIT(USB2_IDM_IDM_RESET_CONTROL__RESET);
>> +	else
>> +		reg_val |= BIT(USB2_IDM_IDM_RESET_CONTROL__RESET);
>> +
>> +	writel(reg_val, phy_driver->usb2d_idm_regs +
>> +		USB2_IDM_IDM_RESET_CONTROL_OFFSET);
>> +}
>> +
>> +static void cygnus_phy_clk_reset_src_switch(struct phy *generic_phy,
>> +					    u32 src_phy)
>> +{
>> +	u32 reg_val;
>> +	struct cygnus_phy_instance *instance_ptr = phy_get_drvdata(generic_phy);
>> +	struct cygnus_phy_driver *phy_driver = instance_ptr->driver;
>> +
>> +	writel(src_phy, phy_driver->cdru_usbphy_regs +
>> +		CDRU_USBPHY_CLK_RST_SEL_OFFSET);
>> +
>> +	/* Force the clock/reset source phy to not suspend */
>> +	reg_val = readl(phy_driver->usb2h_idm_regs +
>> +			USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +	reg_val &= ~(BIT(SUSPEND_OVERRIDE_0) |
>> +			BIT(SUSPEND_OVERRIDE_1) |
>> +			BIT(SUSPEND_OVERRIDE_2));
>> +	reg_val |= BIT(SUSPEND_OVERRIDE_0 + src_phy);
>> +
>> +	writel(reg_val, phy_driver->usb2h_idm_regs +
>> +		USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +}
>> +
>> +static void cygnus_phy_dual_role_init(struct cygnus_phy_instance *instance_ptr)
>> +{
>> +	u32 reg_val;
>> +	struct cygnus_phy_driver *phy_driver = instance_ptr->driver;
>> +
>> +	if (instance_ptr->new_state == PHY2_DEV_HOST_CTRL_SEL_HOST) {
>> +		writel(PHY2_DEV_HOST_CTRL_SEL_HOST,
>> +			phy_driver->cdru_usbphy_regs +
>> +			CDRU_USBPHY2_HOST_DEV_SEL_OFFSET);
> 
> If you add a return here, the below block can be placed outside the else block.
>> +	} else {
>> +		/*
>> +		 * Disable suspend/resume signals to device controller
>> +		 * when a port is in device mode
>> +		 */
>> +		writel(PHY2_DEV_HOST_CTRL_SEL_DEVICE,
>> +			phy_driver->cdru_usbphy_regs +
>> +			CDRU_USBPHY2_HOST_DEV_SEL_OFFSET);
>> +		reg_val = readl(phy_driver->cdru_usbphy_regs +
>> +				CDRU_USB_DEV_SUSPEND_RESUME_CTRL_OFFSET);
>> +		reg_val |= CDRU_USB_DEV_SUSPEND_RESUME_CTRL_DISABLE;
>> +		writel(reg_val, phy_driver->cdru_usbphy_regs +
>> +				CDRU_USB_DEV_SUSPEND_RESUME_CTRL_OFFSET);
>> +	}
>> +}
>> +
>> +static int cygnus_phy_init(struct phy *generic_phy)
>> +{
>> +	struct cygnus_phy_instance *instance_ptr = phy_get_drvdata(generic_phy);
>> +
>> +	if (instance_ptr->port == DUAL_ROLE_PHY)
>> +		cygnus_phy_dual_role_init(instance_ptr);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cygnus_phy_shutdown(struct phy *generic_phy)
>> +{
>> +	u32 reg_val;
>> +	int i, ret;
>> +	unsigned long flags;
>> +	bool power_off_flag = true;
>> +	struct cygnus_phy_instance *instance_ptr = phy_get_drvdata(generic_phy);
>> +	struct cygnus_phy_driver *phy_driver = instance_ptr->driver;
>> +
>> +	if (instance_ptr->vbus_supply) {
> 
> phy core supports phy-supply generic property which can be handled by phy-core.
>> +		ret = regulator_disable(instance_ptr->vbus_supply);
>> +		if (ret) {
>> +			dev_err(&generic_phy->dev,
>> +					"Failed to disable regulator\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	spin_lock_irqsave(&instance_ptr->lock, flags);
>> +
>> +	/* power down the phy */
>> +	reg_val = readl(phy_driver->crmu_usbphy_aon_ctrl_regs +
>> +			CRMU_USB_PHY_AON_CTRL_OFFSET);
>> +	if (instance_ptr->port == 0) {
>> +		reg_val &= ~CRMU_USBPHY_P0_AFE_CORERDY_VDDC;
>> +		reg_val &= ~CRMU_USBPHY_P0_RESETB;
>> +	} else if (instance_ptr->port == 1) {
>> +		reg_val &= ~CRMU_USBPHY_P1_AFE_CORERDY_VDDC;
>> +		reg_val &= ~CRMU_USBPHY_P1_RESETB;
>> +	} else if (instance_ptr->port == 2) {
>> +		reg_val &= ~CRMU_USBPHY_P2_AFE_CORERDY_VDDC;
>> +		reg_val &= ~CRMU_USBPHY_P2_RESETB;
>> +	}
>> +	writel(reg_val, phy_driver->crmu_usbphy_aon_ctrl_regs +
>> +		CRMU_USB_PHY_AON_CTRL_OFFSET);
>> +
>> +	instance_ptr->power = false;
>> +
>> +	/* Determine whether all the phy's are powered off */
>> +	for (i = 0; i < phy_driver->num_phys; i++) {
>> +		if (phy_driver->instances[i].power == true) {
>> +			power_off_flag = false;
>> +			break;
>> +		}
>> +	}
>> +
>> +	/* Disable clock to USB device and keep the USB device in reset */
>> +	if (instance_ptr->port == DUAL_ROLE_PHY)
>> +		cygnus_usbp2_dev_clock_init(instance_ptr->generic_phy,
>> +					    false);
>> +
>> +	/*
>> +	 * Put the host controller into reset state and
>> +	 * disable clock if all the phy's are powered off
>> +	 */
>> +	if (power_off_flag) {
>> +		reg_val = readl(phy_driver->usb2h_idm_regs +
>> +			USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +		reg_val &= ~USB2_IDM_IDM_IO_CONTROL_DIRECT_CLK_ENABLE;
>> +		writel(reg_val, phy_driver->usb2h_idm_regs +
>> +				USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +
>> +		reg_val = readl(phy_driver->usb2h_idm_regs +
>> +				 USB2_IDM_IDM_RESET_CONTROL_OFFSET);
>> +		reg_val |= BIT(USB2_IDM_IDM_RESET_CONTROL__RESET);
>> +		writel(reg_val, phy_driver->usb2h_idm_regs +
>> +				USB2_IDM_IDM_RESET_CONTROL_OFFSET);
>> +		phy_driver->idm_host_enabled = false;
>> +	}
>> +	spin_unlock_irqrestore(&instance_ptr->lock, flags);
>> +	return 0;
>> +}
>> +
>> +static int cygnus_phy_poweron(struct phy *generic_phy)
>> +{
>> +	int ret;
>> +	unsigned long flags;
>> +	u32 reg_val;
>> +	struct cygnus_phy_instance *instance_ptr = phy_get_drvdata(generic_phy);
>> +	struct cygnus_phy_driver *phy_driver = instance_ptr->driver;
>> +	u32 extcon_event = instance_ptr->new_state;
>> +
>> +	/*
>> +	 * Switch on the regulator only if in HOST mode
>> +	 */
>> +	if (instance_ptr->vbus_supply) {
>> +		ret = regulator_enable(instance_ptr->vbus_supply);
>> +		if (ret) {
>> +			dev_err(&generic_phy->dev,
>> +				"Failed to enable regulator\n");
>> +			goto err_shutdown;
>> +		}
>> +	}
>> +
>> +	spin_lock_irqsave(&instance_ptr->lock, flags);
>> +	/* Bring the AFE block out of reset to start powering up the PHY */
>> +	reg_val = readl(phy_driver->crmu_usbphy_aon_ctrl_regs +
>> +			CRMU_USB_PHY_AON_CTRL_OFFSET);
>> +	if (instance_ptr->port == 0)
>> +		reg_val |= CRMU_USBPHY_P0_AFE_CORERDY_VDDC;
>> +	else if (instance_ptr->port == 1)
>> +		reg_val |= CRMU_USBPHY_P1_AFE_CORERDY_VDDC;
>> +	else if (instance_ptr->port == 2)
>> +		reg_val |= CRMU_USBPHY_P2_AFE_CORERDY_VDDC;
>> +	writel(reg_val, phy_driver->crmu_usbphy_aon_ctrl_regs +
>> +		CRMU_USB_PHY_AON_CTRL_OFFSET);
>> +
>> +	/* Check for power on and PLL lock */
>> +	ret = phy_pll_lock_stat(status_reg[instance_ptr->port],
>> +				CDRU_USBPHY_USBPHY_ILDO_ON_FLAG, phy_driver);
>> +	if (ret < 0) {
>> +		dev_err(&generic_phy->dev,
>> +			"Timed out waiting for USBPHY_ILDO_ON_FLAG on port %d",
>> +			instance_ptr->port);
>> +		spin_unlock_irqrestore(&instance_ptr->lock, flags);
>> +		goto err_shutdown;
>> +	}
>> +	ret = phy_pll_lock_stat(status_reg[instance_ptr->port],
>> +				CDRU_USBPHY_USBPHY_PLL_LOCK, phy_driver);
>> +	if (ret < 0) {
>> +		dev_err(&generic_phy->dev,
>> +			"Timed out waiting for USBPHY_PLL_LOCK on port %d",
>> +			instance_ptr->port);
>> +		spin_unlock_irqrestore(&instance_ptr->lock, flags);
>> +		goto err_shutdown;
>> +	}
>> +
>> +	instance_ptr->power = true;
>> +
>> +	/* Enable clock to USB device and take the USB device out of reset */
>> +	if (instance_ptr->port == DUAL_ROLE_PHY)
>> +		cygnus_usbp2_dev_clock_init(instance_ptr->generic_phy, true);
>> +
>> +	/* Set clock source provider to be the last powered on phy */
>> +	if (instance_ptr->port == phy_driver->phyto_src_clk)
>> +		cygnus_phy_clk_reset_src_switch(generic_phy,
>> +				instance_ptr->port);
>> +
>> +	if (phy_driver->idm_host_enabled != true &&
>> +		extcon_event == PHY2_DEV_HOST_CTRL_SEL_HOST) {
>> +		/* Enable clock to USB host and take the host out of reset */
>> +		reg_val = readl(phy_driver->usb2h_idm_regs +
>> +				USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +		reg_val |= USB2_IDM_IDM_IO_CONTROL_DIRECT_CLK_ENABLE;
>> +		writel(reg_val, phy_driver->usb2h_idm_regs +
>> +				USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
>> +
>> +		reg_val = readl(phy_driver->usb2h_idm_regs +
>> +				USB2_IDM_IDM_RESET_CONTROL_OFFSET);
>> +		reg_val &= ~BIT(USB2_IDM_IDM_RESET_CONTROL__RESET);
>> +		writel(reg_val, phy_driver->usb2h_idm_regs +
>> +				 USB2_IDM_IDM_RESET_CONTROL_OFFSET);
>> +		phy_driver->idm_host_enabled = true;
>> +	}
>> +	spin_unlock_irqrestore(&instance_ptr->lock, flags);
>> +
>> +	return 0;
>> +err_shutdown:
>> +	cygnus_phy_shutdown(generic_phy);
>> +	return ret;
>> +}
>> +
>> +static int usbd_connect_notify(struct notifier_block *self,
>> +			       unsigned long event, void *ptr)
>> +{
>> +	struct cygnus_phy_instance *instance_ptr = container_of(self,
>> +					struct cygnus_phy_instance, device_nb);
>> +
>> +	if (event) {
>> +		instance_ptr->new_state = PHY2_DEV_HOST_CTRL_SEL_DEVICE;
>> +		cygnus_phy_dual_role_init(instance_ptr);
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static int usbh_connect_notify(struct notifier_block *self,
>> +			       unsigned long event, void *ptr)
>> +{
>> +	struct cygnus_phy_instance *instance_ptr = container_of(self,
>> +					struct cygnus_phy_instance, host_nb);
>> +	if (event) {
>> +		instance_ptr->new_state = PHY2_DEV_HOST_CTRL_SEL_HOST;
>> +		cygnus_phy_dual_role_init(instance_ptr);
>> +	}
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static int cygnus_register_extcon_notifiers(
>> +				struct cygnus_phy_instance *instance_ptr)
>> +{
>> +	int ret = 0;
>> +	struct device *dev = &instance_ptr->generic_phy->dev;
>> +
>> +	if (of_property_read_bool(dev->of_node, "extcon")) {
>> +		instance_ptr->edev = extcon_get_edev_by_phandle(dev, 0);
>> +		if (IS_ERR(instance_ptr->edev)) {
>> +			if (PTR_ERR(instance_ptr->edev) == -EPROBE_DEFER)
>> +				return -EPROBE_DEFER;
>> +			ret = PTR_ERR(instance_ptr->edev);
>> +			goto err;
>> +		}
>> +
>> +		instance_ptr->device_nb.notifier_call = usbd_connect_notify;
>> +		ret = extcon_register_notifier(instance_ptr->edev, EXTCON_USB,
>> +						&instance_ptr->device_nb);

You better to use devm_extcon_register_notifier
because you didn't handle the unregister operation in this patch.

>> +		if (ret < 0) {
>> +			dev_err(dev, "Can't register extcon device\n");

IMO, "can't register extcon notfiier' is more correct
instead of 'extcon device' expression.

>> +			goto err;
>> +		}
>> +
>> +		if (extcon_get_state(instance_ptr->edev, EXTCON_USB) == true) {
>> +			instance_ptr->new_state = PHY2_DEV_HOST_CTRL_SEL_DEVICE;
>> +			cygnus_phy_dual_role_init(instance_ptr);
>> +		}
>> +
>> +		instance_ptr->host_nb.notifier_call = usbh_connect_notify;
>> +		ret = extcon_register_notifier(instance_ptr->edev,

ditto. (devm_extcon_register_notifier)

>> +						EXTCON_USB_HOST,
>> +						&instance_ptr->host_nb);
>> +		if (ret < 0) {
>> +			dev_err(dev, "Can't register extcon device\n");

ditto.

>> +			goto err;
>> +		}
>> +
>> +		if (extcon_get_state(instance_ptr->edev, EXTCON_USB_HOST)
>> +					== true) {
>> +			instance_ptr->new_state = PHY2_DEV_HOST_CTRL_SEL_HOST;
>> +			cygnus_phy_dual_role_init(instance_ptr);
>> +		}
>> +	} else {
>> +		dev_err(dev, "Extcon device handle not found\n");
>> +		return -EINVAL;

I'm not sure about this error. Because it depend on how to
develope the device driver. Each device driver can decide whether
the extcon is necessary or optional.

>> +	}
>> +
>> +	return 0;
>> +err:
>> +	return ret;
>> +}
>> +
>> +static const struct phy_ops ops = {
>> +	.init		= cygnus_phy_init,
>> +	.power_on	= cygnus_phy_poweron,
>> +	.power_off	= cygnus_phy_shutdown,
> 
>  missing .owner
>> +};
>> +
>> +static int cygnus_phy_instance_create(struct cygnus_phy_driver *phy_driver)
>> +{
>> +	struct device_node *child;
>> +	char *vbus_name;
>> +	struct platform_device *pdev = phy_driver->pdev;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = dev->of_node;
>> +	struct cygnus_phy_instance *instance_ptr;
>> +	unsigned int id, ret;
>> +
>> +	for_each_available_child_of_node(node, child) {
>> +		if (of_property_read_u32(child, "reg", &id)) {
>> +			dev_err(dev, "missing reg property for %s\n",
>> +				child->name);
>> +			ret = -EINVAL;
>> +			goto put_child;
>> +		}
>> +
>> +		if (id >= MAX_NUM_PHYS) {
>> +			dev_err(dev, "invalid PHY id: %u\n", id);
>> +			ret = -EINVAL;
>> +			goto put_child;
>> +		}
>> +
>> +		instance_ptr = &phy_driver->instances[id];
>> +		instance_ptr->driver = phy_driver;
>> +		instance_ptr->port = id;
>> +		spin_lock_init(&instance_ptr->lock);
>> +
>> +		if (instance_ptr->generic_phy) {
>> +			dev_err(dev, "duplicated PHY id: %u\n", id);
>> +			ret = -EINVAL;
>> +			goto put_child;
>> +		}
>> +
>> +		instance_ptr->generic_phy =
>> +				devm_phy_create(dev, child, &ops);
>> +		if (IS_ERR(instance_ptr->generic_phy)) {
>> +			dev_err(dev, "Failed to create usb phy %d", id);
>> +			ret = PTR_ERR(instance_ptr->generic_phy);
>> +			goto put_child;
>> +		}
>> +
>> +		vbus_name = devm_kzalloc(&instance_ptr->generic_phy->dev,
>> +					MAX_REGULATOR_NAME_LEN,
>> +					GFP_KERNEL);
>> +		if (!vbus_name) {
>> +			ret = -ENOMEM;
>> +			goto put_child;
>> +		}
>> +
>> +		/* regulator use is optional */
>> +		sprintf(vbus_name, "vbus-p%d", id);
>> +		instance_ptr->vbus_supply =
>> +			devm_regulator_get(&instance_ptr->generic_phy->dev,
>> +					vbus_name);
>> +		if (IS_ERR(instance_ptr->vbus_supply))
>> +			instance_ptr->vbus_supply = NULL;
>> +		devm_kfree(&instance_ptr->generic_phy->dev, vbus_name);
>> +		phy_set_drvdata(instance_ptr->generic_phy, instance_ptr);
>> +	}
>> +
>> +	return 0;
>> +
>> +put_child:
>> +	of_node_put(child);
>> +	return ret;
>> +}
>> +
>> +static int cygnus_phy_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	struct cygnus_phy_driver *phy_driver;
>> +	struct phy_provider *phy_provider;
>> +	int i, ret;
>> +	u32 reg_val;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = dev->of_node;
>> +
>> +	/* allocate memory for each phy instance */
>> +	phy_driver = devm_kzalloc(dev, sizeof(struct cygnus_phy_driver),
>> +				  GFP_KERNEL);
>> +	if (!phy_driver)
>> +		return -ENOMEM;
>> +
>> +	phy_driver->num_phys = of_get_child_count(node);
>> +
>> +	if (phy_driver->num_phys == 0) {
>> +		dev_err(dev, "PHY no child node\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	phy_driver->instances = devm_kcalloc(dev, phy_driver->num_phys,
>> +					     sizeof(struct cygnus_phy_instance),
>> +					     GFP_KERNEL);
>> +	phy_driver->pdev = pdev;
>> +	platform_set_drvdata(pdev, phy_driver);
>> +
>> +	ret = cygnus_phy_instance_create(phy_driver);
>> +	if (ret)
>> +		return ret;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +					   "crmu-usbphy-aon-ctrl");
>> +	phy_driver->crmu_usbphy_aon_ctrl_regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(phy_driver->crmu_usbphy_aon_ctrl_regs))
>> +		return PTR_ERR(phy_driver->crmu_usbphy_aon_ctrl_regs);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +					   "cdru-usbphy");
>> +	phy_driver->cdru_usbphy_regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(phy_driver->cdru_usbphy_regs))
>> +		return PTR_ERR(phy_driver->cdru_usbphy_regs);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "usb2h-idm");
>> +	phy_driver->usb2h_idm_regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(phy_driver->usb2h_idm_regs))
>> +		return PTR_ERR(phy_driver->usb2h_idm_regs);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "usb2d-idm");
>> +	phy_driver->usb2d_idm_regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(phy_driver->usb2d_idm_regs))
>> +		return PTR_ERR(phy_driver->usb2d_idm_regs);
>> +
>> +	reg_val = readl(phy_driver->usb2d_idm_regs);
>> +	writel((reg_val | USB2D_IDM_IDM_IO_SS_CLEAR_NAK_NEMPTY_EN_I),
>> +		   phy_driver->usb2d_idm_regs);
>> +	phy_driver->idm_host_enabled = false;
>> +
>> +	/*
>> +	 * Shutdown all ports. They can be powered up as
>> +	 * required
>> +	 */
>> +	reg_val = readl(phy_driver->crmu_usbphy_aon_ctrl_regs +
>> +			CRMU_USB_PHY_AON_CTRL_OFFSET);
>> +	reg_val &= ~CRMU_USBPHY_P0_AFE_CORERDY_VDDC;
>> +	reg_val &= ~CRMU_USBPHY_P0_RESETB;
>> +	reg_val &= ~CRMU_USBPHY_P1_AFE_CORERDY_VDDC;
>> +	reg_val &= ~CRMU_USBPHY_P1_RESETB;
>> +	reg_val &= ~CRMU_USBPHY_P2_AFE_CORERDY_VDDC;
>> +	reg_val &= ~CRMU_USBPHY_P2_RESETB;
>> +	writel(reg_val, phy_driver->crmu_usbphy_aon_ctrl_regs +
>> +		CRMU_USB_PHY_AON_CTRL_OFFSET);
> 
> move the above to a separate function to make probe a lot cleaner.
> 
> Thanks
> Kishon
> 
> 
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Powered by blists - more mailing lists