[PATCH 0/3] Add driver for Broadcom Cygnus USB phy controller

[PATCH 0/3] Add driver for Broadcom Cygnus USB phy controller

[Raveendra Padasalagi]

Add driver for Broadcom's USB phy controller's used in Cygnus family
of SoC and it's based on 4.14-rc3 tag.

The patch set can be fetched from iproc-cyg-usb-v1 branch of
https://github.com/Broadcom/arm64-linux.git

Raveendra Padasalagi (3):
  Documentation: DT: Add Cygnus usb phy binding
  drivers: phy: broadcom: Add driver for Cygnus USB phy controller
  ARM: dts: Add dt node for Broadcom Cygnus USB phy

 .../bindings/phy/brcm,cygnus-usb-phy.txt           | 101 ++++
 arch/arm/boot/dts/bcm-cygnus.dtsi                  |  35 ++
 drivers/phy/broadcom/Kconfig                       |  14 +
 drivers/phy/broadcom/Makefile                      |   1 +
 drivers/phy/broadcom/phy-bcm-cygnus-usb.c          | 672 +++++++++++++++++++++
 5 files changed, 823 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
 create mode 100644 drivers/phy/broadcom/phy-bcm-cygnus-usb.c

-- 
1.9.1

[PATCH 1/3] Documentation: DT: Add Cygnus usb phy binding

[Raveendra Padasalagi]

Add devicetree binding document for broadcom's
Cygnus SoC specific usb phy controller driver.

Signed-off-by: Raveendra Padasalagi <raveendra.padasalagi@broadcom.com>
---
 .../bindings/phy/brcm,cygnus-usb-phy.txt           | 101 +++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
new file mode 100644
index 0000000..2d99fea
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
@@ -0,0 +1,101 @@
+BROADCOM CYGNUS USB PHY
+
+Required Properties:
+- compatible:  brcm,cygnus-usb-phy
+- reg : the register start address and length for crmu_usbphy_aon_ctrl,
+  cdru usb phy control and reset registers, usb host idm registers,
+  usb device idm registers.
+- reg-names: a list of the names corresponding to the previous register
+  ranges. Should contain "crmu-usbphy-aon-ctrl", "cdru-usbphy",
+  "usb2h-idm", "usb2d-idm".
+- address-cells: should be 1
+- size-cells: should be 0
+
+Sub-nodes:
+  Each port's PHY should be represented as a sub-node.
+
+Sub-nodes required properties:
+- reg: the PHY number
+- #phy-cells must be 1
+  The node that uses the phy must provide 1 integer argument specifying
+  port number.
+
+Optional Properties:
+- vbus-p#-supply : The regulator for vbus out control for the host
+  functionality enabled ports.
+- vbus-gpios: vbus gpio binding
+  This is mandatory for port 2, as port 2 is used as dual role phy.
+  Based on the vbus and id values device or host role is determined
+  for phy 2.
+
+- extcon: extcon phandle
+  This is mandatory for port 2,  as port 2 is used as dual role phy.
+  extcon should be phandle to external usb gpio module which provide
+  device or host role notifications based on the ID and VBUS gpio's state.
+
+
+Refer to phy/phy-bindings.txt for the generic PHY binding properties
+
+NOTE: port 0 and port 1 are host only and port 2 is dual role port.
+
+Example of phy :
+	usbphy: phy@0301c028 {
+		compatible = "brcm,cygnus-usb-phy";
+		reg = <0x0301c028 0x4>,
+		      <0x0301d1b4 0x5c>,
+		      <0x18115000 0xa00>,
+		      <0x18111000 0xa00>;
+		reg-names = "crmu-usbphy-aon-ctrl", "cdru-usbphy",
+			    "usb2h-idm", "usb2d-idm";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		usbphy0: usb-phy@0 {
+			reg = <0>;
+			#phy-cells = <1>;
+		};
+
+		usbphy1: usb-phy@1 {
+			reg = <1>;
+			#phy-cells = <1>;
+		};
+
+		usbphy2: usb-phy@2 {
+			reg = <2>;
+			#phy-cells = <1>;
+			extcon = <&extcon_usb>;
+		};
+	};
+
+	extcon_usb: extcon_usb {
+		compatible = "linux,extcon-usb-gpio";
+		vbus-gpio = <&gpio_asiu 121 0>;
+		id-gpio = <&gpio_asiu 122 0>;
+		status = "okay";
+	};
+
+
+Example of node using the phy:
+
+	/* This nodes declares all three ports, port 0
+	and port 1 are host and port 2 is device */
+
+	ehci0: usb@18048000 {
+		compatible = "generic-ehci";
+		reg = <0x18048000 0x100>;
+		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+		phys = <&usbphy0 0 &usbphy1 1 &usbphy2 2>;
+		phy-names = "usbp0","usbp1","usbp2";
+		status = "okay";
+	};
+
+	/* This node declares port 2 phy
+	and configures it for device */
+
+	usbd_udc_dwc1: usbd_udc_dwc@1804c000 {
+		compatible = "iproc-udc";
+		reg = <0x1804c000 0x2000>;
+		interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
+		phys = <&usbphy2 2>;
+		phy-names = "usbdrd";
+	};
-- 
1.9.1

Re: [PATCH 1/3] Documentation: DT: Add Cygnus usb phy binding

[Rob Herring]

On Tue, Oct 24, 2017 at 10:07:00AM +0530, Raveendra Padasalagi wrote:
> Add devicetree binding document for broadcom's
> Cygnus SoC specific usb phy controller driver.

"dt-bindings: phy: ..." for the subject please.

> 
> Signed-off-by: Raveendra Padasalagi <raveendra.padasalagi@broadcom.com>
> ---
>  .../bindings/phy/brcm,cygnus-usb-phy.txt           | 101 +++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
> new file mode 100644
> index 0000000..2d99fea
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
> @@ -0,0 +1,101 @@
> +BROADCOM CYGNUS USB PHY
> +
> +Required Properties:
> +- compatible:  brcm,cygnus-usb-phy
> +- reg : the register start address and length for crmu_usbphy_aon_ctrl,
> +  cdru usb phy control and reset registers, usb host idm registers,
> +  usb device idm registers.

Make this list 1 per line.

> +- reg-names: a list of the names corresponding to the previous register
> +  ranges. Should contain "crmu-usbphy-aon-ctrl", "cdru-usbphy",
> +  "usb2h-idm", "usb2d-idm".
> +- address-cells: should be 1
> +- size-cells: should be 0
> +
> +Sub-nodes:
> +  Each port's PHY should be represented as a sub-node.
> +
> +Sub-nodes required properties:
> +- reg: the PHY number
> +- #phy-cells must be 1
> +  The node that uses the phy must provide 1 integer argument specifying
> +  port number.

Either you need to move #phy-cells up a level or #phy-cells should be 0.

> +
> +Optional Properties:
> +- vbus-p#-supply : The regulator for vbus out control for the host
> +  functionality enabled ports.
> +- vbus-gpios: vbus gpio binding

Are you using these or extcon?

Don't use extcon. It needs to be redesigned and I don't want to see new 
users.

> +  This is mandatory for port 2, as port 2 is used as dual role phy.
> +  Based on the vbus and id values device or host role is determined
> +  for phy 2.
> +
> +- extcon: extcon phandle
> +  This is mandatory for port 2,  as port 2 is used as dual role phy.
> +  extcon should be phandle to external usb gpio module which provide
> +  device or host role notifications based on the ID and VBUS gpio's state.
> +
> +
> +Refer to phy/phy-bindings.txt for the generic PHY binding properties
> +
> +NOTE: port 0 and port 1 are host only and port 2 is dual role port.
> +
> +Example of phy :
> +	usbphy: phy@0301c028 {

usb-phy@301c028

> +		compatible = "brcm,cygnus-usb-phy";
> +		reg = <0x0301c028 0x4>,
> +		      <0x0301d1b4 0x5c>,
> +		      <0x18115000 0xa00>,
> +		      <0x18111000 0xa00>;
> +		reg-names = "crmu-usbphy-aon-ctrl", "cdru-usbphy",
> +			    "usb2h-idm", "usb2d-idm";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		usbphy0: usb-phy@0 {
> +			reg = <0>;
> +			#phy-cells = <1>;
> +		};
> +
> +		usbphy1: usb-phy@1 {
> +			reg = <1>;
> +			#phy-cells = <1>;
> +		};
> +
> +		usbphy2: usb-phy@2 {
> +			reg = <2>;
> +			#phy-cells = <1>;
> +			extcon = <&extcon_usb>;
> +		};
> +	};
> +
> +	extcon_usb: extcon_usb {
> +		compatible = "linux,extcon-usb-gpio";
> +		vbus-gpio = <&gpio_asiu 121 0>;
> +		id-gpio = <&gpio_asiu 122 0>;
> +		status = "okay";
> +	};
> +
> +
> +Example of node using the phy:
> +
> +	/* This nodes declares all three ports, port 0
> +	and port 1 are host and port 2 is device */
> +
> +	ehci0: usb@18048000 {
> +		compatible = "generic-ehci";
> +		reg = <0x18048000 0x100>;
> +		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> +		phys = <&usbphy0 0 &usbphy1 1 &usbphy2 2>;
> +		phy-names = "usbp0","usbp1","usbp2";
> +		status = "okay";
> +	};
> +
> +	/* This node declares port 2 phy
> +	and configures it for device */
> +
> +	usbd_udc_dwc1: usbd_udc_dwc@1804c000 {

usb@...

> +		compatible = "iproc-udc";
> +		reg = <0x1804c000 0x2000>;
> +		interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
> +		phys = <&usbphy2 2>;
> +		phy-names = "usbdrd";
> +	};
> -- 
> 1.9.1
> 

Re: [PATCH 1/3] Documentation: DT: Add Cygnus usb phy binding

[Raveendra Padasalagi]

Hi Rob,

On Fri, Oct 27, 2017 at 9:09 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Oct 24, 2017 at 10:07:00AM +0530, Raveendra Padasalagi wrote:
>> Add devicetree binding document for broadcom's
>> Cygnus SoC specific usb phy controller driver.
>
> "dt-bindings: phy: ..." for the subject please.

Ok. I will update it in the next patch set version.

>>
>> Signed-off-by: Raveendra Padasalagi <raveendra.padasalagi@broadcom.com>
>> ---
>>  .../bindings/phy/brcm,cygnus-usb-phy.txt           | 101 +++++++++++++++++++++
>>  1 file changed, 101 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>> new file mode 100644
>> index 0000000..2d99fea
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>> @@ -0,0 +1,101 @@
>> +BROADCOM CYGNUS USB PHY
>> +
>> +Required Properties:
>> +- compatible:  brcm,cygnus-usb-phy
>> +- reg : the register start address and length for crmu_usbphy_aon_ctrl,
>> +  cdru usb phy control and reset registers, usb host idm registers,
>> +  usb device idm registers.
>
> Make this list 1 per line.
Ok
>> +- reg-names: a list of the names corresponding to the previous register
>> +  ranges. Should contain "crmu-usbphy-aon-ctrl", "cdru-usbphy",
>> +  "usb2h-idm", "usb2d-idm".
>> +- address-cells: should be 1
>> +- size-cells: should be 0
>> +
>> +Sub-nodes:
>> +  Each port's PHY should be represented as a sub-node.
>> +
>> +Sub-nodes required properties:
>> +- reg: the PHY number
>> +- #phy-cells must be 1
>> +  The node that uses the phy must provide 1 integer argument specifying
>> +  port number.
>
> Either you need to move #phy-cells up a level or #phy-cells should be 0.
Ok
>> +
>> +Optional Properties:
>> +- vbus-p#-supply : The regulator for vbus out control for the host
>> +  functionality enabled ports.
>> +- vbus-gpios: vbus gpio binding
>
> Are you using these or extcon?
Yes, using extcon in phy driver to receive USB Device/Host connect/disconnect
notifications.

> Don't use extcon. It needs to be redesigned and I don't want to see new
> users.

Without extcon I need to duplicate the code in phy driver to implement
extcon functionality,
which is again bad. Once the extcon redesign is done may be we can
adopt the changes in
this driver at that time.

>> +  This is mandatory for port 2, as port 2 is used as dual role phy.
>> +  Based on the vbus and id values device or host role is determined
>> +  for phy 2.
>> +
>> +- extcon: extcon phandle
>> +  This is mandatory for port 2,  as port 2 is used as dual role phy.
>> +  extcon should be phandle to external usb gpio module which provide
>> +  device or host role notifications based on the ID and VBUS gpio's state.
>> +
>> +
>> +Refer to phy/phy-bindings.txt for the generic PHY binding properties
>> +
>> +NOTE: port 0 and port 1 are host only and port 2 is dual role port.
>> +
>> +Example of phy :
>> +     usbphy: phy@0301c028 {
>
> usb-phy@301c028
>
>> +             compatible = "brcm,cygnus-usb-phy";
>> +             reg = <0x0301c028 0x4>,
>> +                   <0x0301d1b4 0x5c>,
>> +                   <0x18115000 0xa00>,
>> +                   <0x18111000 0xa00>;
>> +             reg-names = "crmu-usbphy-aon-ctrl", "cdru-usbphy",
>> +                         "usb2h-idm", "usb2d-idm";
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +
>> +             usbphy0: usb-phy@0 {
>> +                     reg = <0>;
>> +                     #phy-cells = <1>;
>> +             };
>> +
>> +             usbphy1: usb-phy@1 {
>> +                     reg = <1>;
>> +                     #phy-cells = <1>;
>> +             };
>> +
>> +             usbphy2: usb-phy@2 {
>> +                     reg = <2>;
>> +                     #phy-cells = <1>;
>> +                     extcon = <&extcon_usb>;
>> +             };
>> +     };
>> +
>> +     extcon_usb: extcon_usb {
>> +             compatible = "linux,extcon-usb-gpio";
>> +             vbus-gpio = <&gpio_asiu 121 0>;
>> +             id-gpio = <&gpio_asiu 122 0>;
>> +             status = "okay";
>> +     };
>> +
>> +
>> +Example of node using the phy:
>> +
>> +     /* This nodes declares all three ports, port 0
>> +     and port 1 are host and port 2 is device */
>> +
>> +     ehci0: usb@18048000 {
>> +             compatible = "generic-ehci";
>> +             reg = <0x18048000 0x100>;
>> +             interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
>> +             phys = <&usbphy0 0 &usbphy1 1 &usbphy2 2>;
>> +             phy-names = "usbp0","usbp1","usbp2";
>> +             status = "okay";
>> +     };
>> +
>> +     /* This node declares port 2 phy
>> +     and configures it for device */
>> +
>> +     usbd_udc_dwc1: usbd_udc_dwc@1804c000 {
>
> usb@...
Ok.
>> +             compatible = "iproc-udc";
>> +             reg = <0x1804c000 0x2000>;
>> +             interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
>> +             phys = <&usbphy2 2>;
>> +             phy-names = "usbdrd";
>> +     };
>> --
>> 1.9.1
>>

[PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Raveendra Padasalagi]

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@broadcom.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
+	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
+#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,
+};
+
+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 (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);
+	} 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) {
+		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);
+		if (ret < 0) {
+			dev_err(dev, "Can't register extcon device\n");
+			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,
+						EXTCON_USB_HOST,
+						&instance_ptr->host_nb);
+		if (ret < 0) {
+			dev_err(dev, "Can't register extcon device\n");
+			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;
+	}
+
+	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,
+};
+
+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);
+
+	phy_provider = devm_of_phy_provider_register(dev, cygnus_phy_xlate);
+	if (IS_ERR(phy_provider)) {
+		dev_err(dev, "Failed to register as phy provider\n");
+		ret = PTR_ERR(phy_provider);
+		return ret;
+	}
+
+	for (i = 0; i < phy_driver->num_phys; i++) {
+		if (phy_driver->instances[i].port == DUAL_ROLE_PHY) {
+			ret = cygnus_register_extcon_notifiers(
+				&phy_driver->instances[DUAL_ROLE_PHY]);
+			if (ret) {
+				dev_err(dev, "Failed to register notifier\n");
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id cygnus_phy_dt_ids[] = {
+	{ .compatible = "brcm,cygnus-usb-phy", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bcm_phy_dt_ids);
+
+static struct platform_driver cygnus_phy_driver = {
+	.probe = cygnus_phy_probe,
+	.driver = {
+		.name = "bcm-cygnus-usbphy",
+		.of_match_table = of_match_ptr(cygnus_phy_dt_ids),
+	},
+};
+module_platform_driver(cygnus_phy_driver);
+
+MODULE_ALIAS("platform:bcm-cygnus-usbphy");
+MODULE_AUTHOR("Raveendra Padasalagi <Raveendra.padasalagi@broadcom.com");
+MODULE_DESCRIPTION("Broadcom Cygnus USB PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Rafał Miłecki]

On 2017-10-24 06:37, 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.

Minor issues commented inline.


> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET		0x0408
> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT_CLK_ENABLE	BIT(0)

Here you define reg bits using BIT(n).


> +#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

And here without BIT(n). Either is fine but it may be better to be
consistent about it.


> +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);

I don't think kcalloc is safe here. E.g. In cygnus_phy_shutdown you
iterate over all instances reading the .power value. If
cygnus_phy_shutdown gets called before having each instance powered up,
you'll read random memory as .power value.

Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Raveendra Padasalagi]

On Tue, Oct 24, 2017 at 11:16 AM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 2017-10-24 06:37, 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.
>
>
> Minor issues commented inline.
>
>
>> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET          0x0408
>> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT_CLK_ENABLE      BIT(0)
>
>
> Here you define reg bits using BIT(n).
>
>
>> +#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
>
>
> And here without BIT(n). Either is fine but it may be better to be
> consistent about it.

Thanks, will fix it in the next version of the patch.

>
>
>> +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);
>
>
> I don't think kcalloc is safe here. E.g. In cygnus_phy_shutdown you
> iterate over all instances reading the .power value. If
> cygnus_phy_shutdown gets called before having each instance powered up,
> you'll read random memory as .power value.

Yes, Need to use devm_kzalloc() to make sure contents of
phy_driver->instances is zero initialized.
Will fix it in the next patch.

Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Kishon Vijay Abraham I]

+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@broadcom.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

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

Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Chanwoo Choi]

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@broadcom.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

Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Raveendra Padasalagi]

Hi Chanwoo,

On Mon, Oct 30, 2017 at 5:32 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> 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@broadcom.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'.

GENERIC_PHY dependency is anyway needed and it's added as dependency.
Let me add "depends on EXTCON" instead of "select EXTCON"

>>
>> 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.

Ok, Thanks. I will fix it in the next version of the patch.
>>> +
>>> +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.

Ok, I will fix it in the next version of the 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.

Ok, I will fix it in the next version of the patch.

>>> +                    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)

Ok, I will fix it in the next version of the patch.

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

Ok, I will fix it in the next version of the patch.

>>> +                    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.

In our case extcon is necessary because all of our SoC's will have
dual role phy.
So decided to return -EINVAL.

>>> +    }
>>> +
>>> +    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

Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Chanwoo Choi]

Hi,

On 2017년 10월 30일 14:02, Raveendra Padasalagi wrote:
> Hi Chanwoo,
> 
> On Mon, Oct 30, 2017 at 5:32 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> 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@broadcom.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'.
> 
> GENERIC_PHY dependency is anyway needed and it's added as dependency.
> Let me add "depends on EXTCON" instead of "select EXTCON"

I think that 'select EXTCON' is enough. Actually, EXTCON is not
a upper framework of this device driver. This device driver uses
the EXTCON API as the additional feature.

If user enables 'PHY_BCM_CYGNUS_USB', CONFG_EXTCON is enabled.
I think it is enough.


> 
>>>
>>> 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.
> 
> Ok, Thanks. I will fix it in the next version of the patch.
>>>> +
>>>> +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);
>>>> +    }

I have a question. This function handles when event is 1. It means
that this function handles the attached state. Is it right
that you don't consider the detached state?

>>>> +
>>>> +    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);
>>>> +    }

ditto.

>>>> +
>>>> +    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.
> 
> Ok, I will fix it in the next version of the 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.
> 
> Ok, I will fix it in the next version of the patch.
> 
>>>> +                    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)
> 
> Ok, I will fix it in the next version of the patch.
> 
>>>> +                                            EXTCON_USB_HOST,
>>>> +                                            &instance_ptr->host_nb);
>>>> +            if (ret < 0) {
>>>> +                    dev_err(dev, "Can't register extcon device\n");
>>
>> ditto.
> 
> Ok, I will fix it in the next version of the patch.
> 
>>>> +                    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.
> 
> In our case extcon is necessary because all of our SoC's will have
> dual role phy.
> So decided to return -EINVAL.
> 
>>>> +    }
>>>> +
>>>> +    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
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/3] drivers: phy: broadcom: Add driver for Cygnus USB phy controller

[Raveendra Padasalagi]

Hi Kishon,

On Fri, Oct 27, 2017 at 2:05 PM, Kishon Vijay Abraham I <kishon@ti.com> 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@broadcom.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
>
> Didn't this throw up a warning for selecting config without caring for the
> dependency?

No, it didn't throw any warning. Let me remove select and place it as
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.

Ok, I will fix it in the next version of the patch.

>> +#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,
>> +};
>> +
>> +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.

Custome xlate needed here for the reason that phy driver needs to know
the port number
which is largest in number in order to use that phy as the clock
source provider to host controller.

>> +     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.

Ok, I will fix it in the next version of the patch.

>> +     } 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.

Vbus supply mentioned here is not for the phy core, it's used to
enable vbus to device.
So I can't use phy-supply property.

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

Ok, I will fix it in the next version of the patch.

>> +};
>> +
>> +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.

Ok, I will fix it in the next version of the patch.

> Thanks
> Kishon

[PATCH 3/3] ARM: dts: Add dt node for Broadcom Cygnus USB phy

[Raveendra Padasalagi]

Add DT node for Broadcom's USB phy controller's used
in Cygnus family of SoC.

Signed-off-by: Raveendra Padasalagi <raveendra.padasalagi@broadcom.com>
---
 arch/arm/boot/dts/bcm-cygnus.dtsi | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 7c957ea..810df68 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -96,6 +96,41 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 
+		extcon_usb: extcon_usb {
+			compatible = "linux,extcon-usb-gpio";
+			vbus-gpio = <&gpio_asiu 121 0>;
+			id-gpio = <&gpio_asiu 122 0>;
+			status = "okay";
+		};
+
+		usbphy: phy@0301c028 {
+			compatible = "brcm,cygnus-usb-phy";
+			reg = <0x0301c028 0x4>,
+			      <0x0301d1b4 0x5c>,
+			      <0x18115000 0xa00>,
+			      <0x18111000 0xa00>;
+			reg-names = "crmu-usbphy-aon-ctrl", "cdru-usbphy",
+				    "usb2h-idm", "usb2d-idm";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			usbphy0: usb-phy@0 {
+				#phy-cells = <1>;
+				reg = <0>;
+			};
+
+			usbphy1: usb-phy@1 {
+				#phy-cells = <1>;
+				reg = <1>;
+			};
+
+			usbphy2: usb-phy@2 {
+				#phy-cells = <1>;
+				reg = <2>;
+				extcon = <&extcon_usb>;
+			};
+		};
+
 		otp: otp@0301c800 {
 			compatible = "brcm,ocotp";
 			reg = <0x0301c800 0x2c>;
-- 
1.9.1