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 <[email protected]>
>> ---
>> 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
