Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560319 > unrolled thread

[RFC v2 0/5] Platform driver support for 'amd5536udc' driver

Back to article view | Back to linux.kernel

Contents

  [RFC v2 0/5] Platform driver support for 'amd5536udc' driver  Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-17 09:10 +0100
    [RFC v2 3/5] UDC: Provide correct arguments for 'dma_pool_create' Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-17 09:10 +0100
    [RFC v2 2/5] UDC: make debug prints compatible with both pci and platform devices Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-17 09:10 +0100
    [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-17 09:20 +0100
      Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Rob Herring <robh@kernel.org> - 2017-01-19 18:40 +0100
        Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Scott Branden <scott.branden@broadcom.com> - 2017-01-19 20:40 +0100
          Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Florian Fainelli <f.fainelli@gmail.com> - 2017-01-19 20:50 +0100
            Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Scott Branden <scott.branden@broadcom.com> - 2017-01-19 21:10 +0100
              Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Florian Fainelli <f.fainelli@gmail.com> - 2017-01-19 21:20 +0100
                Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Ray Jui <ray.jui@broadcom.com> - 2017-01-19 23:10 +0100
                  Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Scott Branden <scott.branden@broadcom.com> - 2017-01-19 23:40 +0100
                    Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform  driver Florian Fainelli <florian.fainelli@broadcom.com> - 2017-01-20 00:00 +0100
                      Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver Rob Herring <robh@kernel.org> - 2017-01-20 15:00 +0100
        Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-20 13:10 +0100
    [RFC v2 5/5] UDC: Add Synopsys UDC Platform driver Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-17 09:20 +0100
    Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys  core driver Florian Fainelli <f.fainelli@gmail.com> - 2017-01-18 20:00 +0100
      Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys  core driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-01-18 21:20 +0100
      Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys  core driver Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-19 11:50 +0100
        Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys  core driver Florian Fainelli <f.fainelli@gmail.com> - 2017-01-19 20:30 +0100
          Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys  core driver Raviteja Garimella <raviteja.garimella@broadcom.com> - 2017-01-23 14:10 +0100

#1560319 — [RFC v2 0/5] Platform driver support for 'amd5536udc' driver

This is RFC for the changes made as per the review comments made for
the previous version. I would like to know  if this approach (the way
the driver is split and the naming and all)looks good to be submitted.

Changes from v1:
===============
1. Split the driver into platform/pci specific drivers with a core driver
   file that handles the common driver routines that are exported.

2. Split the driver into number of patches as suggested in previous
   review comments.

3. Added the devicetree bindings documentation for Synopsys platform
   driver.

Introduction from previous version:
===================================
This patch adds platform device support to the existing 'amd5536udc'
driver.

The UDC is based on Synopsys Designware core USB (2.0) Device controller
IP.

The driver so far supports UDCs that are a part of AMD southbridge
and is connected through PCI bus.

The same driver can be used with UDCs that are integrated into SoCs
like Broadcom's Northstar2/Cygnus platforms by adding platform device
suooprt.

This patch contains all the changes that were required to get the driver
functional on Broadcom's Northstar2 platform. 

This is a request for comments from maintainers/others regarding approach
on whether to have 2 different drivers (one each for AMD and Broadcom)
with a common library (3 files in total), or have a single driver like
it's done in this patch and have the driver filename changed to some
common name based on ther underlying IP, like snps_udc.c.

Below are the main changes done:

1. Added OF based platform device registration -- so that the driver gets
   probed based on the device tree entry. Like wise, remove routine and
   platform PM ops are supported.

2. Modified debug prints to be compatible with both pci and platform
   devices.

3. Added members to 'struct udc' in header file for extcon and phy support.
   This is required if the UDC is connected to a Dual Role Device Phy
   where the Phy can be configured to be in Device mode or Host mode based
   on the type of external cable that is connected to the port.
 
4. Added checks in udc connect/disconnect routines so as to return if the
   routine is already called.

5. Modified the arguments passed to dma_pool_create routine -- which
   expects struct device, whereas NULL is passed in the existing version.
 
6. Kconfig changes are done so that the driver now depends on either of
   CONFIG_OF or CONFIG_PCI. More description about the Synopsys IP is
   provided.

Raviteja Garimella (5):
  UDC: Split the driver into amd (pci) and Synopsys core driver
  UDC: make debug prints compatible with both pci and platform devices
  UDC: Provide correct arguments for 'dma_pool_create'
  DT bindings documentation for Synopsys UDC platform driver
  UDC: Add Synopsys UDC Platform driver

 .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    |   27 +
 drivers/usb/gadget/udc/Kconfig                     |   32 +
 drivers/usb/gadget/udc/Makefile                    |    2 +
 drivers/usb/gadget/udc/amd5536udc.c                | 3249 +-------------------
 drivers/usb/gadget/udc/amd5536udc.h                |   54 +-
 drivers/usb/gadget/udc/snps_udc_core.c             | 3232 +++++++++++++++++++
 drivers/usb/gadget/udc/snps_udc_plat.c             |  342 +++
 7 files changed, 3714 insertions(+), 3224 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
 create mode 100644 drivers/usb/gadget/udc/snps_udc_core.c
 create mode 100644 drivers/usb/gadget/udc/snps_udc_plat.c

-- 
2.1.0

[toc] | [next] | [standalone]

#1560321 — [RFC v2 3/5] UDC: Provide correct arguments for 'dma_pool_create'

Change the argument from NULL to a struct device for the
dma_pool_create call during dma init.

Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
---
 drivers/usb/gadget/udc/snps_udc_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/snps_udc_core.c b/drivers/usb/gadget/udc/snps_udc_core.c
index b7a6218..556fa0e 100644
--- a/drivers/usb/gadget/udc/snps_udc_core.c
+++ b/drivers/usb/gadget/udc/snps_udc_core.c
@@ -3094,7 +3094,7 @@ int init_dma_pools(struct udc *dev)
 	}
 
 	/* DMA setup */
-	dev->data_requests = dma_pool_create("data_requests", NULL,
+	dev->data_requests = dma_pool_create("data_requests", dev->dev,
 		sizeof(struct udc_data_dma), 0, 0);
 	if (!dev->data_requests) {
 		DBG(dev, "can't get request data pool\n");
@@ -3105,7 +3105,7 @@ int init_dma_pools(struct udc *dev)
 	dev->ep[UDC_EP0IN_IX].dma = &dev->regs->ctl;
 
 	/* dma desc for setup data */
-	dev->stp_requests = dma_pool_create("setup requests", NULL,
+	dev->stp_requests = dma_pool_create("setup requests", dev->dev,
 		sizeof(struct udc_stp_dma), 0, 0);
 	if (!dev->stp_requests) {
 		DBG(dev, "can't get stp request pool\n");
-- 
2.1.0

[toc] | [prev] | [next] | [standalone]

#1560327 — [RFC v2 2/5] UDC: make debug prints compatible with both pci and platform devices

This patch adds a struct device member to UDC data structure and
makes changes to the arguments of dev_err and dev_dbg calls so that
the debug prints work for both pci and platform devices.

Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
---
 drivers/usb/gadget/udc/amd5536udc.c    |  1 +
 drivers/usb/gadget/udc/amd5536udc.h    |  4 +++-
 drivers/usb/gadget/udc/snps_udc_core.c | 28 ++++++++++++++--------------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/gadget/udc/amd5536udc.c b/drivers/usb/gadget/udc/amd5536udc.c
index c0acb80..3e66b08 100644
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@ -166,6 +166,7 @@ static int udc_pci_probe(
 	dev->phys_addr = resource;
 	dev->irq = pdev->irq;
 	dev->pdev = pdev;
+	dev->dev = &pdev->dev;
 
 	/* general probing */
 	if (udc_probe(dev)) {
diff --git a/drivers/usb/gadget/udc/amd5536udc.h b/drivers/usb/gadget/udc/amd5536udc.h
index bd2a18e..c252457 100644
--- a/drivers/usb/gadget/udc/amd5536udc.h
+++ b/drivers/usb/gadget/udc/amd5536udc.h
@@ -563,6 +563,8 @@ struct udc {
 	u16				cur_config;
 	u16				cur_intf;
 	u16				cur_alt;
+
+	struct device			*dev;
 };
 
 #define to_amd5536_udc(g)	(container_of((g), struct udc, gadget))
@@ -639,7 +641,7 @@ MODULE_PARM_DESC(use_fullspeed, "true for fullspeed only");
 
 /* debug macros ------------------------------------------------------------*/
 
-#define DBG(udc , args...)	dev_dbg(&(udc)->pdev->dev, args)
+#define DBG(udc , args...)	dev_dbg(udc->dev, args)
 
 #ifdef UDC_VERBOSE
 #define VDBG			DBG
diff --git a/drivers/usb/gadget/udc/snps_udc_core.c b/drivers/usb/gadget/udc/snps_udc_core.c
index 69a94bc..b7a6218 100644
--- a/drivers/usb/gadget/udc/snps_udc_core.c
+++ b/drivers/usb/gadget/udc/snps_udc_core.c
@@ -202,18 +202,18 @@ static void print_regs(struct udc *dev)
 	if (use_dma && use_dma_ppb && !use_dma_ppb_du) {
 		DBG(dev, "DMA mode       = PPBNDU (packet per buffer "
 			"WITHOUT desc. update)\n");
-		dev_info(&dev->pdev->dev, "DMA mode (%s)\n", "PPBNDU");
+		dev_info(dev->dev, "DMA mode (%s)\n", "PPBNDU");
 	} else if (use_dma && use_dma_ppb && use_dma_ppb_du) {
 		DBG(dev, "DMA mode       = PPBDU (packet per buffer "
 			"WITH desc. update)\n");
-		dev_info(&dev->pdev->dev, "DMA mode (%s)\n", "PPBDU");
+		dev_info(dev->dev, "DMA mode (%s)\n", "PPBDU");
 	}
 	if (use_dma && use_dma_bufferfill_mode) {
 		DBG(dev, "DMA mode       = BF (buffer fill mode)\n");
-		dev_info(&dev->pdev->dev, "DMA mode (%s)\n", "BF");
+		dev_info(dev->dev, "DMA mode (%s)\n", "BF");
 	}
 	if (!use_dma)
-		dev_info(&dev->pdev->dev, "FIFO mode\n");
+		dev_info(dev->dev, "FIFO mode\n");
 	DBG(dev, "-------------------------------------------------------\n");
 }
 
@@ -1621,7 +1621,7 @@ static void udc_setup_endpoints(struct udc *dev)
 static void usb_connect(struct udc *dev)
 {
 
-	dev_info(&dev->pdev->dev, "USB Connect\n");
+	dev_info(dev->dev, "USB Connect\n");
 
 	dev->connected = 1;
 
@@ -1639,7 +1639,7 @@ static void usb_connect(struct udc *dev)
 static void usb_disconnect(struct udc *dev)
 {
 
-	dev_info(&dev->pdev->dev, "USB Disconnect\n");
+	dev_info(dev->dev, "USB Disconnect\n");
 
 	dev->connected = 0;
 
@@ -2103,7 +2103,7 @@ static irqreturn_t udc_data_out_isr(struct udc *dev, int ep_ix)
 	}
 	/* HE event ? */
 	if (tmp & AMD_BIT(UDC_EPSTS_HE)) {
-		dev_err(&dev->pdev->dev, "HE ep%dout occurred\n", ep->num);
+		dev_err(dev->dev, "HE ep%dout occurred\n", ep->num);
 
 		/* clear HE */
 		writel(tmp | AMD_BIT(UDC_EPSTS_HE), &ep->regs->sts);
@@ -2302,7 +2302,7 @@ static irqreturn_t udc_data_in_isr(struct udc *dev, int ep_ix)
 	if (use_dma) {
 		/* BNA ? */
 		if (epsts & AMD_BIT(UDC_EPSTS_BNA)) {
-			dev_err(&dev->pdev->dev,
+			dev_err(dev->dev,
 				"BNA ep%din occurred - DESPTR = %08lx\n",
 				ep->num,
 				(unsigned long) readl(&ep->regs->desptr));
@@ -2315,7 +2315,7 @@ static irqreturn_t udc_data_in_isr(struct udc *dev, int ep_ix)
 	}
 	/* HE event ? */
 	if (epsts & AMD_BIT(UDC_EPSTS_HE)) {
-		dev_err(&dev->pdev->dev,
+		dev_err(dev->dev,
 			"HE ep%dn occurred - DESPTR = %08lx\n",
 			ep->num, (unsigned long) readl(&ep->regs->desptr));
 
@@ -2953,7 +2953,7 @@ __acquires(dev->lock)
 
 		/* link up all endpoints */
 		udc_setup_endpoints(dev);
-		dev_info(&dev->pdev->dev, "Connect: %s\n",
+		dev_info(dev->dev, "Connect: %s\n",
 			 usb_speed_string(dev->gadget.speed));
 
 		/* init ep 0 */
@@ -3165,20 +3165,20 @@ int udc_probe(struct udc *dev)
 	/* init registers, interrupts, ... */
 	startup_registers(dev);
 
-	dev_info(&dev->pdev->dev, "%s\n", mod_desc);
+	dev_info(dev->dev, "%s\n", mod_desc);
 
 	snprintf(tmp, sizeof(tmp), "%d", dev->irq);
-	dev_info(&dev->pdev->dev,
+	dev_info(dev->dev,
 		 "irq %s, pci mem %08lx, chip rev %02x(Geode5536 %s)\n",
 		 tmp, dev->phys_addr, dev->chiprev,
 		 (dev->chiprev == UDC_HSA0_REV) ? "A0" : "B1");
 	strcpy(tmp, UDC_DRIVER_VERSION_STRING);
 	if (dev->chiprev == UDC_HSA0_REV) {
-		dev_err(&dev->pdev->dev, "chip revision is A0; too old\n");
+		dev_err(dev->dev, "chip revision is A0; too old\n");
 		retval = -ENODEV;
 		goto finished;
 	}
-	dev_info(&dev->pdev->dev,
+	dev_info(dev->dev,
 		 "driver version: %s(for Geode5536 B1)\n", tmp);
 	udc = dev;
 
-- 
2.1.0

[toc] | [prev] | [next] | [standalone]

#1560328 — [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

This patch adds device tree bindings documentation for Synopsys
USB device controller platform driver.

Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
---
 .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt

diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
new file mode 100644
index 0000000..0c18327
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
@@ -0,0 +1,27 @@
+Synopsys USB Device controller.
+
+The device node is used for Synopsys Designware Cores AHB
+Subsystem Device Controller (UDC).
+
+This device node is used by UDCs integrated it Broadcom's
+Northstar2 and Cygnus SoC's.
+
+Required properties:
+ - compatible: should be "snps,dw-ahb-udc"
+ - reg: Offset and length of UDC register set
+ - interrupts: description of interrupt line
+ - phys: phandle to phy node.
+ - extcon: phandle to the extcon device. This is optional and
+   not required for those that don't require extcon support.
+   Extcon support will be required if the UDC is connected to
+   a Dual Role Device Phy that supports both Host and Device
+   mode based on the external cable.
+
+Example:
+	udc_dwc: usb@664e0000 {
+		compatible = "snps,dw-ahb-udc";
+		reg = <0x664e0000 0x2000>;
+		interrupts = <GIC_SPI 424 IRQ_TYPE_LEVEL_HIGH>;
+		phys = <&usbdrd_phy>;
+		extcon = <&usbdrd_phy>";
+	};
-- 
2.1.0

[toc] | [prev] | [next] | [standalone]

#1562966 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
> This patch adds device tree bindings documentation for Synopsys
> USB device controller platform driver.

Bindings describe h/w, not drivers.
> 
> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
> ---
>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> new file mode 100644
> index 0000000..0c18327
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> @@ -0,0 +1,27 @@
> +Synopsys USB Device controller.
> +
> +The device node is used for Synopsys Designware Cores AHB
> +Subsystem Device Controller (UDC).
> +
> +This device node is used by UDCs integrated it Broadcom's
> +Northstar2 and Cygnus SoC's.

You need compatible strings for these in addition.

> +
> +Required properties:
> + - compatible: should be "snps,dw-ahb-udc"

This is a different IP than DWC2?

> + - reg: Offset and length of UDC register set
> + - interrupts: description of interrupt line
> + - phys: phandle to phy node.
> + - extcon: phandle to the extcon device. This is optional and
> +   not required for those that don't require extcon support.
> +   Extcon support will be required if the UDC is connected to
> +   a Dual Role Device Phy that supports both Host and Device
> +   mode based on the external cable.

Drop this. It should be a part of the phy. Also, I don't care to see new 
users of extcon binding because it needs redoing.

[toc] | [prev] | [next] | [standalone]

#1563032 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

Hi Rob,

On 17-01-19 09:36 AM, Rob Herring wrote:
> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
>> This patch adds device tree bindings documentation for Synopsys
>> USB device controller platform driver.
>
> Bindings describe h/w, not drivers.
>>
>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>> ---
>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27 ++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> new file mode 100644
>> index 0000000..0c18327
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> @@ -0,0 +1,27 @@
>> +Synopsys USB Device controller.
>> +
>> +The device node is used for Synopsys Designware Cores AHB
>> +Subsystem Device Controller (UDC).
>> +
>> +This device node is used by UDCs integrated it Broadcom's
>> +Northstar2 and Cygnus SoC's.
>
> You need compatible strings for these in addition.
>
We don't need compatibility strings when an IP block is integrated into 
an SoC.  Otherwise each time we add the IP block to a new SoC we would 
need to update ever linux driver that supports that SoC.  That doesn't 
make sense?

Cygnus and Northstar2 use existing drivers for such block as UARTs, SPI 
controllers, NAND controllers, etc, etc.  We haven't added compatibility 
strings for those drivers and won't be.

Perhaps comment above can be:
This device node is used by UDCs integrated it such as Broadcom's
Northstar2 and Cygnus SoC's.
>> +
>> +Required properties:
>> + - compatible: should be "snps,dw-ahb-udc"
>
> This is a different IP than DWC2?
>
>> + - reg: Offset and length of UDC register set
>> + - interrupts: description of interrupt line
>> + - phys: phandle to phy node.
>> + - extcon: phandle to the extcon device. This is optional and
>> +   not required for those that don't require extcon support.
>> +   Extcon support will be required if the UDC is connected to
>> +   a Dual Role Device Phy that supports both Host and Device
>> +   mode based on the external cable.
>
> Drop this. It should be a part of the phy. Also, I don't care to see new
> users of extcon binding because it needs redoing.
>
We may need extcon support to support DRD Phy though.  We have to work 
within the framework that exists in linux today.  If modified in the 
future adapt to it as needed?

Regards,
  Scott

[toc] | [prev] | [next] | [standalone]

#1563045 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

On 01/19/2017 11:30 AM, Scott Branden wrote:
> Hi Rob,
> 
> On 17-01-19 09:36 AM, Rob Herring wrote:
>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
>>> This patch adds device tree bindings documentation for Synopsys
>>> USB device controller platform driver.
>>
>> Bindings describe h/w, not drivers.
>>>
>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>>> ---
>>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27
>>> ++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>> new file mode 100644
>>> index 0000000..0c18327
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>> @@ -0,0 +1,27 @@
>>> +Synopsys USB Device controller.
>>> +
>>> +The device node is used for Synopsys Designware Cores AHB
>>> +Subsystem Device Controller (UDC).
>>> +
>>> +This device node is used by UDCs integrated it Broadcom's
>>> +Northstar2 and Cygnus SoC's.
>>
>> You need compatible strings for these in addition.
>>
> We don't need compatibility strings when an IP block is integrated into
> an SoC.  Otherwise each time we add the IP block to a new SoC we would
> need to update ever linux driver that supports that SoC.  That doesn't
> make sense?

You probably do need such a thing, here is how the compatible strings
for IP blocks integrated into SoCs could be used:

- provide a compatible strings which describes exactly the integration
of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for
that is that you want to be able to capture the specific IP block
integration into a specific SoC and all its quirks

- if the block has its own revision scheme (and it can be relied on),
provide it: brcm,udc-v1.2 and that is probably the most meaningful
compatible string for a client program here

- have a some kind of fallback/catchall compatible string that describes
the block: brcm,udc which may also work just fine, although is not preferred

Defining compatible strings is meant to avoid making (possibly
incompatible) Device Tree binding changes in the future, and you always
have the liberty as a client program (OS, bootloader) to match only the
compatible strings you care about, from the most specific (which
includes the exact SoC) to the least specific.

The key thing is that, if the full set of compatible strings are present
and available, you can retroactively fix your driver to be more
specific, very much less so your Device Tree blob (although there is
disagreement).
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1563053 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

Hi Florian,

On 17-01-19 11:40 AM, Florian Fainelli wrote:
> On 01/19/2017 11:30 AM, Scott Branden wrote:
>> Hi Rob,
>>
>> On 17-01-19 09:36 AM, Rob Herring wrote:
>>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
>>>> This patch adds device tree bindings documentation for Synopsys
>>>> USB device controller platform driver.
>>>
>>> Bindings describe h/w, not drivers.
>>>>
>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>>>> ---
>>>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27
>>>> ++++++++++++++++++++++
>>>>  1 file changed, 27 insertions(+)
>>>>  create mode 100644
>>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>> new file mode 100644
>>>> index 0000000..0c18327
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>> @@ -0,0 +1,27 @@
>>>> +Synopsys USB Device controller.
>>>> +
>>>> +The device node is used for Synopsys Designware Cores AHB
>>>> +Subsystem Device Controller (UDC).
>>>> +
>>>> +This device node is used by UDCs integrated it Broadcom's
>>>> +Northstar2 and Cygnus SoC's.
>>>
>>> You need compatible strings for these in addition.
>>>
>> We don't need compatibility strings when an IP block is integrated into
>> an SoC.  Otherwise each time we add the IP block to a new SoC we would
>> need to update ever linux driver that supports that SoC.  That doesn't
>> make sense?
>
> You probably do need such a thing, here is how the compatible strings
> for IP blocks integrated into SoCs could be used:
>
> - provide a compatible strings which describes exactly the integration
> of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for
> that is that you want to be able to capture the specific IP block
> integration into a specific SoC and all its quirks
>
> - if the block has its own revision scheme (and it can be relied on),
> provide it: brcm,udc-v1.2 and that is probably the most meaningful
> compatible string for a client program here
>
> - have a some kind of fallback/catchall compatible string that describes
> the block: brcm,udc which may also work just fine, although is not preferred
>
> Defining compatible strings is meant to avoid making (possibly
> incompatible) Device Tree binding changes in the future, and you always
> have the liberty as a client program (OS, bootloader) to match only the
> compatible strings you care about, from the most specific (which
> includes the exact SoC) to the least specific.
>
> The key thing is that, if the full set of compatible strings are present
> and available, you can retroactively fix your driver to be more
> specific, very much less so your Device Tree blob (although there is
> disagreement).
>
The driver stands alone from the SoC and does not need compatibility 
strings per SoC.  New SoCs will use the exact same block.

We don't add compatibility strings to any other drivers when we add the 
same block to a new SoC.

Yes, if the version of the IP changes then a version or feature 
compatibility string is added to the driver.

[toc] | [prev] | [next] | [standalone]

#1563058 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

On 01/19/2017 12:07 PM, Scott Branden wrote:
> Hi Florian,
> 
> On 17-01-19 11:40 AM, Florian Fainelli wrote:
>> On 01/19/2017 11:30 AM, Scott Branden wrote:
>>> Hi Rob,
>>>
>>> On 17-01-19 09:36 AM, Rob Herring wrote:
>>>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
>>>>> This patch adds device tree bindings documentation for Synopsys
>>>>> USB device controller platform driver.
>>>>
>>>> Bindings describe h/w, not drivers.
>>>>>
>>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>>>>> ---
>>>>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27
>>>>> ++++++++++++++++++++++
>>>>>  1 file changed, 27 insertions(+)
>>>>>  create mode 100644
>>>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>> new file mode 100644
>>>>> index 0000000..0c18327
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>> @@ -0,0 +1,27 @@
>>>>> +Synopsys USB Device controller.
>>>>> +
>>>>> +The device node is used for Synopsys Designware Cores AHB
>>>>> +Subsystem Device Controller (UDC).
>>>>> +
>>>>> +This device node is used by UDCs integrated it Broadcom's
>>>>> +Northstar2 and Cygnus SoC's.
>>>>
>>>> You need compatible strings for these in addition.
>>>>
>>> We don't need compatibility strings when an IP block is integrated into
>>> an SoC.  Otherwise each time we add the IP block to a new SoC we would
>>> need to update ever linux driver that supports that SoC.  That doesn't
>>> make sense?
>>
>> You probably do need such a thing, here is how the compatible strings
>> for IP blocks integrated into SoCs could be used:
>>
>> - provide a compatible strings which describes exactly the integration
>> of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for
>> that is that you want to be able to capture the specific IP block
>> integration into a specific SoC and all its quirks
>>
>> - if the block has its own revision scheme (and it can be relied on),
>> provide it: brcm,udc-v1.2 and that is probably the most meaningful
>> compatible string for a client program here
>>
>> - have a some kind of fallback/catchall compatible string that describes
>> the block: brcm,udc which may also work just fine, although is not
>> preferred
>>
>> Defining compatible strings is meant to avoid making (possibly
>> incompatible) Device Tree binding changes in the future, and you always
>> have the liberty as a client program (OS, bootloader) to match only the
>> compatible strings you care about, from the most specific (which
>> includes the exact SoC) to the least specific.
>>
>> The key thing is that, if the full set of compatible strings are present
>> and available, you can retroactively fix your driver to be more
>> specific, very much less so your Device Tree blob (although there is
>> disagreement).
>>
> The driver stands alone from the SoC and does not need compatibility
> strings per SoC.  New SoCs will use the exact same block.

Even if you take the exact same block and put it in a different SoC,
that's still an integration work that 99% of the time goes just fine
because the validation worked great, and the 1% of the time where you
need to capture an integration bug, you are glad this SoC-specific
compatible string exists such that you can work around it in the driver.

One way to solve that is to use SoC specific compatible strings because
that presents itself as a self-contained and standardized way, or you
can have your driver call into a piece of code that reads the SoC
type/revision, but AFAICT this seems to be frowned upon because it
presents some kind of layering violation.

> 
> We don't add compatibility strings to any other drivers when we add the
> same block to a new SoC.

Ideally we would define new compatible strings for each new SoC we tape
out, yet don't necessarily match them in client programs, but just
define them as a safeguard in case something went wrong at the
integration stage that is discovered after the fact.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1563131 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

On 1/19/2017 12:17 PM, Florian Fainelli wrote:
> On 01/19/2017 12:07 PM, Scott Branden wrote:
>> Hi Florian,
>>
>> On 17-01-19 11:40 AM, Florian Fainelli wrote:
>>> On 01/19/2017 11:30 AM, Scott Branden wrote:
>>>> Hi Rob,
>>>>
>>>> On 17-01-19 09:36 AM, Rob Herring wrote:
>>>>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
>>>>>> This patch adds device tree bindings documentation for Synopsys
>>>>>> USB device controller platform driver.
>>>>>
>>>>> Bindings describe h/w, not drivers.
>>>>>>
>>>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27
>>>>>> ++++++++++++++++++++++
>>>>>>  1 file changed, 27 insertions(+)
>>>>>>  create mode 100644
>>>>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..0c18327
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>> @@ -0,0 +1,27 @@
>>>>>> +Synopsys USB Device controller.
>>>>>> +
>>>>>> +The device node is used for Synopsys Designware Cores AHB
>>>>>> +Subsystem Device Controller (UDC).
>>>>>> +
>>>>>> +This device node is used by UDCs integrated it Broadcom's
>>>>>> +Northstar2 and Cygnus SoC's.
>>>>>
>>>>> You need compatible strings for these in addition.
>>>>>
>>>> We don't need compatibility strings when an IP block is integrated into
>>>> an SoC.  Otherwise each time we add the IP block to a new SoC we would
>>>> need to update ever linux driver that supports that SoC.  That doesn't
>>>> make sense?
>>>
>>> You probably do need such a thing, here is how the compatible strings
>>> for IP blocks integrated into SoCs could be used:
>>>
>>> - provide a compatible strings which describes exactly the integration
>>> of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for
>>> that is that you want to be able to capture the specific IP block
>>> integration into a specific SoC and all its quirks
>>>
>>> - if the block has its own revision scheme (and it can be relied on),
>>> provide it: brcm,udc-v1.2 and that is probably the most meaningful
>>> compatible string for a client program here
>>>
>>> - have a some kind of fallback/catchall compatible string that describes
>>> the block: brcm,udc which may also work just fine, although is not
>>> preferred
>>>
>>> Defining compatible strings is meant to avoid making (possibly
>>> incompatible) Device Tree binding changes in the future, and you always
>>> have the liberty as a client program (OS, bootloader) to match only the
>>> compatible strings you care about, from the most specific (which
>>> includes the exact SoC) to the least specific.
>>>
>>> The key thing is that, if the full set of compatible strings are present
>>> and available, you can retroactively fix your driver to be more
>>> specific, very much less so your Device Tree blob (although there is
>>> disagreement).
>>>
>> The driver stands alone from the SoC and does not need compatibility
>> strings per SoC.  New SoCs will use the exact same block.
> 
> Even if you take the exact same block and put it in a different SoC,
> that's still an integration work that 99% of the time goes just fine
> because the validation worked great, and the 1% of the time where you
> need to capture an integration bug, you are glad this SoC-specific
> compatible string exists such that you can work around it in the driver.

That's a very conservative estimate. Based on my experience, it's more
like 50/50, i.e., roughly half of the time we found SoC integration
specific quirks or workaround are needed.

> 
> One way to solve that is to use SoC specific compatible strings because
> that presents itself as a self-contained and standardized way, or you
> can have your driver call into a piece of code that reads the SoC
> type/revision, but AFAICT this seems to be frowned upon because it
> presents some kind of layering violation.
> 
>>
>> We don't add compatibility strings to any other drivers when we add the
>> same block to a new SoC.
> 
> Ideally we would define new compatible strings for each new SoC we tape
> out, yet don't necessarily match them in client programs, but just
> define them as a safeguard in case something went wrong at the
> integration stage that is discovered after the fact.
> 

[toc] | [prev] | [next] | [standalone]

#1563166 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

On 17-01-19 01:55 PM, Ray Jui wrote:
>
>
> On 1/19/2017 12:17 PM, Florian Fainelli wrote:
>> On 01/19/2017 12:07 PM, Scott Branden wrote:
>>> Hi Florian,
>>>
>>> On 17-01-19 11:40 AM, Florian Fainelli wrote:
>>>> On 01/19/2017 11:30 AM, Scott Branden wrote:
>>>>> Hi Rob,
>>>>>
>>>>> On 17-01-19 09:36 AM, Rob Herring wrote:
>>>>>> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
>>>>>>> This patch adds device tree bindings documentation for Synopsys
>>>>>>> USB device controller platform driver.
>>>>>>
>>>>>> Bindings describe h/w, not drivers.
>>>>>>>
>>>>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27
>>>>>>> ++++++++++++++++++++++
>>>>>>>  1 file changed, 27 insertions(+)
>>>>>>>  create mode 100644
>>>>>>> Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..0c18327
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>>>>>> @@ -0,0 +1,27 @@
>>>>>>> +Synopsys USB Device controller.
>>>>>>> +
>>>>>>> +The device node is used for Synopsys Designware Cores AHB
>>>>>>> +Subsystem Device Controller (UDC).
>>>>>>> +
>>>>>>> +This device node is used by UDCs integrated it Broadcom's
>>>>>>> +Northstar2 and Cygnus SoC's.
>>>>>>
>>>>>> You need compatible strings for these in addition.
>>>>>>
>>>>> We don't need compatibility strings when an IP block is integrated into
>>>>> an SoC.  Otherwise each time we add the IP block to a new SoC we would
>>>>> need to update ever linux driver that supports that SoC.  That doesn't
>>>>> make sense?
>>>>
>>>> You probably do need such a thing, here is how the compatible strings
>>>> for IP blocks integrated into SoCs could be used:
>>>>
>>>> - provide a compatible strings which describes exactly the integration
>>>> of this peripheral into a given SoC, e.g: brcm,udc-ns2, the reason for
>>>> that is that you want to be able to capture the specific IP block
>>>> integration into a specific SoC and all its quirks
>>>>
>>>> - if the block has its own revision scheme (and it can be relied on),
>>>> provide it: brcm,udc-v1.2 and that is probably the most meaningful
>>>> compatible string for a client program here
>>>>
>>>> - have a some kind of fallback/catchall compatible string that describes
>>>> the block: brcm,udc which may also work just fine, although is not
>>>> preferred
>>>>
>>>> Defining compatible strings is meant to avoid making (possibly
>>>> incompatible) Device Tree binding changes in the future, and you always
>>>> have the liberty as a client program (OS, bootloader) to match only the
>>>> compatible strings you care about, from the most specific (which
>>>> includes the exact SoC) to the least specific.
>>>>
>>>> The key thing is that, if the full set of compatible strings are present
>>>> and available, you can retroactively fix your driver to be more
>>>> specific, very much less so your Device Tree blob (although there is
>>>> disagreement).
>>>>
>>> The driver stands alone from the SoC and does not need compatibility
>>> strings per SoC.  New SoCs will use the exact same block.
>>
>> Even if you take the exact same block and put it in a different SoC,
>> that's still an integration work that 99% of the time goes just fine
>> because the validation worked great, and the 1% of the time where you
>> need to capture an integration bug, you are glad this SoC-specific
>> compatible string exists such that you can work around it in the driver.
>
> That's a very conservative estimate. Based on my experience, it's more
> like 50/50, i.e., roughly half of the time we found SoC integration
> specific quirks or workaround are needed.
>
50% is an exaggeration for sure.  Maybe a driver you are has that issue 
but that is not the case with most drivers.  We have many IP blocks in 
the SoC - both internal and externally sourced IP.  We integrate SP805 
timer driver into many SoCs and never specify a SoC specific 
compatibility string with it (nor should we).

That being said - if your driver needs to know SoC specifics is should 
not need to have an SoC specific compatibility string added per driver. 
Why can your driver just not query that information from the upper level 
SoC specific info already present in device tree?

Each SoC is already specified in device tree at the upper level already.
Example:
arch/arm/boot/dts/bcm7445.dtsi has this compatibility info already 
present in its device tree:

	compatible = "brcm,bcm7445", "brcm,brcmstb";

If needed, a driver should query this info rather than adding SoC 
specific compatibility strings to every single device tree entry.

We should only add driver revision numbers as needed, not SoC specific 
names.  That way drivers don't change when the (same revision) of the IP 
block is added to a new SoCs.  And then if a SoC specific workaround is 
needed the upper level compatibility string can be queried should be 
utilized.  It already exists today and is available for use to all drivers.

>>
>> One way to solve that is to use SoC specific compatible strings because
>> that presents itself as a self-contained and standardized way, or you
>> can have your driver call into a piece of code that reads the SoC
>> type/revision, but AFAICT this seems to be frowned upon because it
>> presents some kind of layering violation.
>>
>>>
>>> We don't add compatibility strings to any other drivers when we add the
>>> same block to a new SoC.
>>
>> Ideally we would define new compatible strings for each new SoC we tape
>> out, yet don't necessarily match them in client programs, but just
>> define them as a safeguard in case something went wrong at the
>> integration stage that is discovered after the fact.
>>

[toc] | [prev] | [next] | [standalone]

#1563180 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

On 01/19/2017 02:36 PM, Scott Branden wrote:
>>>> The driver stands alone from the SoC and does not need compatibility
>>>> strings per SoC.  New SoCs will use the exact same block.
>>>
>>> Even if you take the exact same block and put it in a different SoC,
>>> that's still an integration work that 99% of the time goes just fine
>>> because the validation worked great, and the 1% of the time where you
>>> need to capture an integration bug, you are glad this SoC-specific
>>> compatible string exists such that you can work around it in the driver.
>>
>> That's a very conservative estimate. Based on my experience, it's more
>> like 50/50, i.e., roughly half of the time we found SoC integration
>> specific quirks or workaround are needed.
>>
> 50% is an exaggeration for sure.  Maybe a driver you are has that issue
> but that is not the case with most drivers.  We have many IP blocks in
> the SoC - both internal and externally sourced IP.  We integrate SP805
> timer driver into many SoCs and never specify a SoC specific
> compatibility string with it (nor should we).

Well, that's a good example where in premise, each SoC vendor
integrating such a peripheral from a third party should actually have
defined its own SoC/vendor compatible string to document the
integration. And you can sometimes see some vendors having to workaround
such essential peripherals and ending-up documenting compatible strings
(or close enough in the example at [1]).

[1]: http://www.spinics.net/lists/devicetree/msg159585.html

It's a bad example though in that it's an IP that came from ARM, so the
confidence level in getting the integration right is just higher
(typically above level 9000), because ripping apart a third party is
governed by strict architecture licensing agreements that usually
prevents people suffering from the Not Invented Here syndrome from
making damage.

> 
> That being said - if your driver needs to know SoC specifics is should
> not need to have an SoC specific compatibility string added per driver.
> Why can your driver just not query that information from the upper level
> SoC specific info already present in device tree?

You could do that, but that just does not happen to be a common or
recommended practice AFAICT, although I could be just wrong here of course.

> 
> Each SoC is already specified in device tree at the upper level already.
> Example:
> arch/arm/boot/dts/bcm7445.dtsi has this compatibility info already
> present in its device tree:
> 
>     compatible = "brcm,bcm7445", "brcm,brcmstb";
> 
> If needed, a driver should query this info rather than adding SoC
> specific compatibility strings to every single device tree entry.

Or you could just put it in the compatible string list for a given
peripheral, and yes, this is a repetition of information that is already
there at a higher level from that particular node, but, it has the
advantage of making all this information self contained within that
node's context, and that's a good design goal.

> 
> We should only add driver revision numbers as needed, not SoC specific
> names.  That way drivers don't change when the (same revision) of the IP
> block is added to a new SoCs.  And then if a SoC specific workaround is
> needed the upper level compatibility string can be queried should be
> utilized.  It already exists today and is available for use to all drivers.

The point is to plan ahead for information that you *may*, but *wish*
you did not need.

Quite frankly, I don't think you are going to win any argument where you
don't add a SoC compatible string to the binding, because there are tons
of precedents and good practices that suggest doing it. You might as
well just do it, it's documented, it's there, if you end up using it or
not, that's totally up to the driver author.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1563611 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

On Thu, Jan 19, 2017 at 4:56 PM, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
> On 01/19/2017 02:36 PM, Scott Branden wrote:
>>>>> The driver stands alone from the SoC and does not need compatibility
>>>>> strings per SoC.  New SoCs will use the exact same block.
>>>>
>>>> Even if you take the exact same block and put it in a different SoC,
>>>> that's still an integration work that 99% of the time goes just fine
>>>> because the validation worked great, and the 1% of the time where you
>>>> need to capture an integration bug, you are glad this SoC-specific
>>>> compatible string exists such that you can work around it in the driver.
>>>
>>> That's a very conservative estimate. Based on my experience, it's more
>>> like 50/50, i.e., roughly half of the time we found SoC integration
>>> specific quirks or workaround are needed.
>>>
>> 50% is an exaggeration for sure.  Maybe a driver you are has that issue
>> but that is not the case with most drivers.  We have many IP blocks in
>> the SoC - both internal and externally sourced IP.  We integrate SP805
>> timer driver into many SoCs and never specify a SoC specific
>> compatibility string with it (nor should we).

Even if it was only 10%, that's still reason to do it.

> Well, that's a good example where in premise, each SoC vendor
> integrating such a peripheral from a third party should actually have
> defined its own SoC/vendor compatible string to document the
> integration. And you can sometimes see some vendors having to workaround
> such essential peripherals and ending-up documenting compatible strings
> (or close enough in the example at [1]).

ARM peripherals are a bit unique because they have ID registers and
vendors tend to change them if they change the IP. And we can also set
the ID in the DT.

It's also a huge difference between a timer and a USB controller.
There's very little in a timer that vendors can f*ck up as well as few
revisions and config options. Experience has shown that USB always
gets integrated in different ways. We can't even get the number of
clocks right on licensed IP blocks. Like many things, there is a
judgement call here.

> [1]: http://www.spinics.net/lists/devicetree/msg159585.html
>
> It's a bad example though in that it's an IP that came from ARM, so the
> confidence level in getting the integration right is just higher
> (typically above level 9000), because ripping apart a third party is
> governed by strict architecture licensing agreements that usually
> prevents people suffering from the Not Invented Here syndrome from
> making damage.
>
>>
>> That being said - if your driver needs to know SoC specifics is should
>> not need to have an SoC specific compatibility string added per driver.
>> Why can your driver just not query that information from the upper level
>> SoC specific info already present in device tree?
>
> You could do that, but that just does not happen to be a common or
> recommended practice AFAICT, although I could be just wrong here of course.

We did a lot of work to get rid machine_is_X(). Let's say you have 10
SoCs and 5 have a quirk in a device and 5 don't. You need 2 device
compatible strings to match in that case. If you check at the top
level you may have to check 5 strings because you can't claim all 5
SoCs to be compatible with each other (that only works for
sub/supersets). You would also have to update the driver for new SoCs
depending if they had the quirk or not.

>> Each SoC is already specified in device tree at the upper level already.
>> Example:
>> arch/arm/boot/dts/bcm7445.dtsi has this compatibility info already
>> present in its device tree:
>>
>>     compatible = "brcm,bcm7445", "brcm,brcmstb";
>>
>> If needed, a driver should query this info rather than adding SoC
>> specific compatibility strings to every single device tree entry.
>
> Or you could just put it in the compatible string list for a given
> peripheral, and yes, this is a repetition of information that is already
> there at a higher level from that particular node, but, it has the
> advantage of making all this information self contained within that
> node's context, and that's a good design goal.
>
>>
>> We should only add driver revision numbers as needed, not SoC specific
>> names.  That way drivers don't change when the (same revision) of the IP
>> block is added to a new SoCs.  And then if a SoC specific workaround is
>> needed the upper level compatibility string can be queried should be
>> utilized.  It already exists today and is available for use to all drivers.
>
> The point is to plan ahead for information that you *may*, but *wish*
> you did not need.
>
> Quite frankly, I don't think you are going to win any argument where you
> don't add a SoC compatible string to the binding, because there are tons
> of precedents and good practices that suggest doing it. You might as
> well just do it, it's documented, it's there, if you end up using it or
> not, that's totally up to the driver author.

Right.

Rob

[toc] | [prev] | [next] | [standalone]

#1563516 — Re: [RFC v2 4/5] DT bindings documentation for Synopsys UDC platform driver

Hi Rob,

On Thu, Jan 19, 2017 at 11:06 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 17, 2017 at 01:35:07PM +0530, Raviteja Garimella wrote:
>> This patch adds device tree bindings documentation for Synopsys
>> USB device controller platform driver.
>
> Bindings describe h/w, not drivers.

 Will correct the commit message.

>>
>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>> ---
>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt    | 27 ++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> new file mode 100644
>> index 0000000..0c18327
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> @@ -0,0 +1,27 @@
>> +Synopsys USB Device controller.
>> +
>> +The device node is used for Synopsys Designware Cores AHB
>> +Subsystem Device Controller (UDC).
>> +
>> +This device node is used by UDCs integrated it Broadcom's
>> +Northstar2 and Cygnus SoC's.
>
> You need compatible strings for these in addition.

Is it fine to have "brcm,iproc-udc"?
iProc refers to a Broadcom family of processors that includes
above mentioned SoCs.

I see there are some compatible strings that are based on the IP,
and some based on the SoCs. I chose to have the IP based string.
Please let me know which one would be agreeable in this case.

I will also correct the typo in the above notes -- it meant to be
UDCs integrated into Broadcom's Northstar2 and Cygnus SoC's.

>
>> +
>> +Required properties:
>> + - compatible: should be "snps,dw-ahb-udc"
>
> This is a different IP than DWC2?

Yes, this is different IP. DWC2 is HS OTG.

>
>> + - reg: Offset and length of UDC register set
>> + - interrupts: description of interrupt line
>> + - phys: phandle to phy node.
>> + - extcon: phandle to the extcon device. This is optional and
>> +   not required for those that don't require extcon support.
>> +   Extcon support will be required if the UDC is connected to
>> +   a Dual Role Device Phy that supports both Host and Device
>> +   mode based on the external cable.
>
> Drop this. It should be a part of the phy. Also, I don't care to see new
> users of extcon binding because it needs redoing.

Currently we can't get the extcon node from Phy.
"extcon_get_edev_by_phandle" requires "extcon" property, else would fail.
As Scott said in one of the comments, we can drop this when we get that
support in kernel. Is it fine?

Thanks,
Ravi

[toc] | [prev] | [next] | [standalone]

#1560338 — [RFC v2 5/5] UDC: Add Synopsys UDC Platform driver

This patch adds platform driver support for Synopsys UDC.

A new driver file (snps_udc_plat.c) is created for this purpose,
where the platform driver registration is done based on the OF
node.

New members are added to the UDC data structure for having platform
device support along with extcon and phy support.

Kconfig and Makefile have the required changes to compile the same.

Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
---
 drivers/usb/gadget/udc/Kconfig         |  15 ++
 drivers/usb/gadget/udc/Makefile        |   1 +
 drivers/usb/gadget/udc/amd5536udc.h    |  14 ++
 drivers/usb/gadget/udc/snps_udc_core.c |  54 ++++--
 drivers/usb/gadget/udc/snps_udc_plat.c | 342 +++++++++++++++++++++++++++++++++
 5 files changed, 407 insertions(+), 19 deletions(-)
 create mode 100644 drivers/usb/gadget/udc/snps_udc_plat.c

diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 9d889bb..44a2058 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -252,6 +252,21 @@ config USB_SNP_CORE
 	  This IP is different to the High Speed OTG IP that can be enabled
 	  by selecting USB_DWC2 or USB_DWC3 options.
 
+config USB_SNP_UDC_PLAT
+	tristate "Synopsys USB 2.0 Device controller"
+	select USB_SNP_CORE
+	select USB_GADGET_DUALSPEED
+	depends on (USB_GADGET && OF)
+	default ARCH_BCM_IPROC
+	help
+	  This adds Platform Device support for Synopsys Designware core
+	  AHB subsystem USB2.0 Device Controller (UDC).
+
+	  This driver works with UDCs integrated into Broadcom's Northstar2
+	  and Cygnus SoCs.
+
+	  If unsure, say N.
+
 #
 # Controllers available in both integrated and discrete versions
 #
diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile
index 3929f6f1..7451516 100644
--- a/drivers/usb/gadget/udc/Makefile
+++ b/drivers/usb/gadget/udc/Makefile
@@ -37,4 +37,5 @@ obj-$(CONFIG_USB_FOTG210_UDC)	+= fotg210-udc.o
 obj-$(CONFIG_USB_MV_U3D)	+= mv_u3d_core.o
 obj-$(CONFIG_USB_GR_UDC)	+= gr_udc.o
 obj-$(CONFIG_USB_GADGET_XILINX)	+= udc-xilinx.o
+obj-$(CONFIG_USB_SNP_UDC_PLAT)	+= snps_udc_plat.o
 obj-$(CONFIG_USB_BDC_UDC)	+= bdc/
diff --git a/drivers/usb/gadget/udc/amd5536udc.h b/drivers/usb/gadget/udc/amd5536udc.h
index c252457..7884281 100644
--- a/drivers/usb/gadget/udc/amd5536udc.h
+++ b/drivers/usb/gadget/udc/amd5536udc.h
@@ -16,6 +16,7 @@
 /* debug control */
 /* #define UDC_VERBOSE */
 
+#include <linux/extcon.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 
@@ -28,6 +29,9 @@
 #define UDC_HSA0_REV 1
 #define UDC_HSB1_REV 2
 
+/* Broadcom chip rev. */
+#define UDC_BCM_REV 10
+
 /*
  * SETUP usb commands
  * needed, because some SETUP's are handled in hw, but must be passed to
@@ -112,6 +116,7 @@
 #define UDC_DEVCTL_BRLEN_MASK			0x00ff0000
 #define UDC_DEVCTL_BRLEN_OFS			16
 
+#define UDC_DEVCTL_SRX_FLUSH			14
 #define UDC_DEVCTL_CSR_DONE			13
 #define UDC_DEVCTL_DEVNAK			12
 #define UDC_DEVCTL_SD				10
@@ -564,7 +569,15 @@ struct udc {
 	u16				cur_intf;
 	u16				cur_alt;
 
+	/* for platform device and extcon support */
 	struct device			*dev;
+	struct phy			*udc_phy;
+	struct extcon_dev		*edev;
+	struct extcon_specific_cable_nb	extcon_nb;
+	struct notifier_block		nb;
+	struct delayed_work		drd_work;
+	struct workqueue_struct		*drd_wq;
+	u32				conn_type;
 };
 
 #define to_amd5536_udc(g)	(container_of((g), struct udc, gadget))
@@ -580,6 +593,7 @@ int udc_enable_dev_setup_interrupts(struct udc *dev);
 int udc_mask_unused_interrupts(struct udc *dev);
 irqreturn_t udc_irq(int irq, void *pdev);
 void gadget_release(struct device *pdev);
+void empty_req_queue(struct udc_ep *ep);
 void udc_basic_init(struct udc *dev);
 void free_dma_pools(struct udc *dev);
 int init_dma_pools(struct udc *dev);
diff --git a/drivers/usb/gadget/udc/snps_udc_core.c b/drivers/usb/gadget/udc/snps_udc_core.c
index 556fa0e..59e2d6a 100644
--- a/drivers/usb/gadget/udc/snps_udc_core.c
+++ b/drivers/usb/gadget/udc/snps_udc_core.c
@@ -34,7 +34,6 @@
 #include "amd5536udc.h"
 
 static void udc_tasklet_disconnect(unsigned long);
-static void empty_req_queue(struct udc_ep *);
 static void udc_setup_endpoints(struct udc *dev);
 static void udc_soft_reset(struct udc *dev);
 static struct udc_request *udc_alloc_bna_dummy(struct udc_ep *ep);
@@ -1241,7 +1240,7 @@ udc_queue(struct usb_ep *usbep, struct usb_request *usbreq, gfp_t gfp)
 }
 
 /* Empty request queue of an endpoint; caller holds spinlock */
-static void empty_req_queue(struct udc_ep *ep)
+void empty_req_queue(struct udc_ep *ep)
 {
 	struct udc_request	*req;
 
@@ -1253,6 +1252,7 @@ static void empty_req_queue(struct udc_ep *ep)
 		complete_req(ep, req, -ESHUTDOWN);
 	}
 }
+EXPORT_SYMBOL_GPL(empty_req_queue);
 
 /* Dequeues a request packet, called by gadget driver */
 static int udc_dequeue(struct usb_ep *usbep, struct usb_request *usbreq)
@@ -1620,6 +1620,9 @@ static void udc_setup_endpoints(struct udc *dev)
 /* Bringup after Connect event, initial bringup to be ready for ep0 events */
 static void usb_connect(struct udc *dev)
 {
+	/* Return if already connected */
+	if (dev->connected)
+		return;
 
 	dev_info(dev->dev, "USB Connect\n");
 
@@ -1638,6 +1641,9 @@ static void usb_connect(struct udc *dev)
  */
 static void usb_disconnect(struct udc *dev)
 {
+	/* Return if already disconnected */
+	if (!dev->connected)
+		return;
 
 	dev_info(dev->dev, "USB Disconnect\n");
 
@@ -1712,11 +1718,15 @@ static void udc_soft_reset(struct udc *dev)
 	/* device int. status reset */
 	writel(UDC_DEV_MSK_DISABLE, &dev->regs->irqsts);
 
-	spin_lock_irqsave(&udc_irq_spinlock, flags);
-	writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg);
-	readl(&dev->regs->cfg);
-	spin_unlock_irqrestore(&udc_irq_spinlock, flags);
-
+	/* Don't do this for Broadcom UDC since this is a reserved
+	 * bit.
+	 */
+	if (dev->chiprev != UDC_BCM_REV) {
+		spin_lock_irqsave(&udc_irq_spinlock, flags);
+		writel(AMD_BIT(UDC_DEVCFG_SOFTRESET), &dev->regs->cfg);
+		readl(&dev->regs->cfg);
+		spin_unlock_irqrestore(&udc_irq_spinlock, flags);
+	}
 }
 
 /* RDE timer callback to set RDE bit */
@@ -3168,21 +3178,27 @@ int udc_probe(struct udc *dev)
 	dev_info(dev->dev, "%s\n", mod_desc);
 
 	snprintf(tmp, sizeof(tmp), "%d", dev->irq);
-	dev_info(dev->dev,
-		 "irq %s, pci mem %08lx, chip rev %02x(Geode5536 %s)\n",
-		 tmp, dev->phys_addr, dev->chiprev,
-		 (dev->chiprev == UDC_HSA0_REV) ? "A0" : "B1");
-	strcpy(tmp, UDC_DRIVER_VERSION_STRING);
-	if (dev->chiprev == UDC_HSA0_REV) {
-		dev_err(dev->dev, "chip revision is A0; too old\n");
-		retval = -ENODEV;
-		goto finished;
+
+	/* Print this device info for AMD chips only*/
+	if (dev->chiprev == UDC_HSA0_REV ||
+	    dev->chiprev == UDC_HSB1_REV) {
+		dev_info(dev->dev, "irq %s, pci mem %08lx, chip rev %02x(Geode5536 %s)\n",
+			 tmp, dev->phys_addr, dev->chiprev,
+			 (dev->chiprev == UDC_HSA0_REV) ?
+			 "A0" : "B1");
+		strcpy(tmp, UDC_DRIVER_VERSION_STRING);
+		if (dev->chiprev == UDC_HSA0_REV) {
+			dev_err(dev->dev, "chip revision is A0; too old\n");
+			retval = -ENODEV;
+			goto finished;
+		}
+		dev_info(dev->dev,
+			 "driver version: %s(for Geode5536 B1)\n", tmp);
 	}
-	dev_info(dev->dev,
-		 "driver version: %s(for Geode5536 B1)\n", tmp);
+
 	udc = dev;
 
-	retval = usb_add_gadget_udc_release(&udc->pdev->dev, &dev->gadget,
+	retval = usb_add_gadget_udc_release(udc->dev, &dev->gadget,
 					    gadget_release);
 	if (retval)
 		goto finished;
diff --git a/drivers/usb/gadget/udc/snps_udc_plat.c b/drivers/usb/gadget/udc/snps_udc_plat.c
new file mode 100644
index 0000000..f5ee2f8
--- /dev/null
+++ b/drivers/usb/gadget/udc/snps_udc_plat.c
@@ -0,0 +1,342 @@
+/*
+ * snps_udc_plat.c - Synopsys UDC Platform Driver
+ *
+ * Copyright (C) 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/extcon.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/module.h>
+#include <linux/dmapool.h>
+#include <linux/interrupt.h>
+#include <linux/moduleparam.h>
+#include "amd5536udc.h"
+
+/* description */
+#define UDC_MOD_DESCRIPTION     "Synopsys UDC platform driver"
+
+void start_udc(struct udc *udc)
+{
+	if (udc->driver) {
+		dev_info(udc->dev, "Connecting...\n");
+		udc_enable_dev_setup_interrupts(udc);
+		udc_basic_init(udc);
+		udc->connected = 1;
+	}
+}
+
+void stop_udc(struct udc *udc)
+{
+	int tmp;
+	u32 reg;
+
+	spin_lock(&udc->lock);
+
+	/* Flush the receieve fifo */
+	reg = readl(&udc->regs->ctl);
+	reg |= AMD_BIT(UDC_DEVCTL_SRX_FLUSH);
+	writel(reg, &udc->regs->ctl);
+
+	reg = readl(&udc->regs->ctl);
+	reg &= ~(AMD_BIT(UDC_DEVCTL_SRX_FLUSH));
+	writel(reg, &udc->regs->ctl);
+	dev_dbg(udc->dev, "ep rx queue flushed\n");
+
+	/* Mask interrupts. Required more so when the
+	 * UDC is connected to a DRD phy.
+	 */
+	udc_mask_unused_interrupts(udc);
+
+	/* Disconnect gadget driver */
+	if (udc->driver) {
+		spin_unlock(&udc->lock);
+		udc->driver->disconnect(&udc->gadget);
+		spin_lock(&udc->lock);
+
+		/* empty queues */
+		for (tmp = 0; tmp < UDC_EP_NUM; tmp++)
+			empty_req_queue(&udc->ep[tmp]);
+	}
+	udc->connected = 0;
+
+	spin_unlock(&udc->lock);
+	dev_info(udc->dev, "Device disconnected\n");
+}
+
+void udc_drd_work(struct work_struct *work)
+{
+	struct udc *udc;
+
+	udc = container_of(to_delayed_work(work),
+			   struct udc, drd_work);
+
+	if (udc->conn_type) {
+		dev_dbg(udc->dev, "idle -> device\n");
+		start_udc(udc);
+	} else {
+		dev_dbg(udc->dev, "device -> idle\n");
+		stop_udc(udc);
+	}
+}
+
+static int usbd_connect_notify(struct notifier_block *self,
+			       unsigned long event, void *ptr)
+{
+	struct udc *udc = container_of(self, struct udc, nb);
+
+	dev_dbg(udc->dev, "%s: event: %lu\n", __func__, event);
+
+	udc->conn_type = event;
+
+	schedule_delayed_work(&udc->drd_work, 0);
+
+	return NOTIFY_OK;
+}
+
+static int udc_plat_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct udc *udc;
+	int ret;
+
+	udc = devm_kzalloc(dev, sizeof(*udc), GFP_KERNEL);
+	if (!udc)
+		return -ENOMEM;
+
+	spin_lock_init(&udc->lock);
+	udc->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	udc->virt_addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(udc->regs))
+		return PTR_ERR(udc->regs);
+
+	/* udc csr registers base */
+	udc->csr = udc->virt_addr + UDC_CSR_ADDR;
+
+	/* dev registers base */
+	udc->regs = udc->virt_addr + UDC_DEVCFG_ADDR;
+
+	/* ep registers base */
+	udc->ep_regs = udc->virt_addr + UDC_EPREGS_ADDR;
+
+	/* fifo's base */
+	udc->rxfifo = (u32 __iomem *)(udc->virt_addr + UDC_RXFIFO_ADDR);
+	udc->txfifo = (u32 __iomem *)(udc->virt_addr + UDC_TXFIFO_ADDR);
+
+	udc->phys_addr = (unsigned long)res->start;
+
+	udc->irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (udc->irq <= 0) {
+		dev_err(dev, "Can't parse and map interrupt\n");
+		return -EINVAL;
+	}
+
+	udc->udc_phy = devm_of_phy_get_by_index(dev, dev->of_node, 0);
+	if (IS_ERR(udc->udc_phy)) {
+		dev_err(dev, "Failed to obtain phy from device tree\n");
+		return PTR_ERR(udc->udc_phy);
+	}
+
+	ret = phy_init(udc->udc_phy);
+	if (ret) {
+		dev_err(dev, "UDC phy init failed");
+		return ret;
+	}
+
+	ret = phy_power_on(udc->udc_phy);
+	if (ret) {
+		dev_err(dev, "UDC phy power on failed");
+		phy_exit(udc->udc_phy);
+		return ret;
+	}
+
+	/* Register for extcon if supported */
+	if (of_get_property(dev->of_node, "extcon", NULL)) {
+		udc->edev = extcon_get_edev_by_phandle(dev, 0);
+		if (IS_ERR(udc->edev)) {
+			if (PTR_ERR(udc->edev) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			dev_err(dev, "Invalid or missing extcon\n");
+			ret = PTR_ERR(udc->edev);
+			goto exit_phy;
+		}
+
+		udc->nb.notifier_call = usbd_connect_notify;
+		ret = extcon_register_notifier(udc->edev, EXTCON_USB,
+					       &udc->nb);
+		if (ret < 0) {
+			dev_err(dev, "Can't register extcon device\n");
+			goto exit_phy;
+		}
+
+		ret = extcon_get_cable_state_(udc->edev, EXTCON_USB);
+		if (ret < 0) {
+			dev_err(dev, "Can't get cable state\n");
+			goto exit_extcon;
+		} else if (ret) {
+			udc->conn_type = ret;
+		}
+		INIT_DELAYED_WORK(&udc->drd_work, udc_drd_work);
+	}
+
+	/* init dma pools */
+	if (use_dma) {
+		ret = init_dma_pools(udc);
+		if (ret != 0)
+			goto exit_extcon;
+	}
+
+	ret = devm_request_irq(dev, udc->irq, udc_irq, IRQF_SHARED,
+			       "snps-udc", udc);
+	if (ret < 0) {
+		dev_err(dev, "Request irq %d failed for UDC\n", udc->irq);
+		goto exit_dma;
+	}
+
+	platform_set_drvdata(pdev, udc);
+	udc->chiprev = UDC_BCM_REV;
+
+	if (udc_probe(udc)) {
+		ret = -ENODEV;
+		goto exit_dma;
+	}
+	dev_info(dev, "Synopsys UDC platform driver probe successful\n");
+
+	return 0;
+
+exit_dma:
+	if (use_dma)
+		free_dma_pools(udc);
+exit_extcon:
+	if (udc->edev)
+		extcon_unregister_notifier(udc->edev, EXTCON_USB, &udc->nb);
+exit_phy:
+	if (udc->udc_phy) {
+		phy_power_off(udc->udc_phy);
+		phy_exit(udc->udc_phy);
+	}
+	return ret;
+}
+
+static int udc_plat_remove(struct platform_device *pdev)
+{
+	struct udc *dev;
+
+	dev = platform_get_drvdata(pdev);
+
+	usb_del_gadget_udc(&dev->gadget);
+	/* gadget driver must not be registered */
+	if (WARN_ON(dev->driver))
+		return 0;
+
+	/* dma pool cleanup */
+	free_dma_pools(dev);
+
+	udc_remove(dev);
+
+	platform_set_drvdata(pdev, NULL);
+
+	if (dev->drd_wq) {
+		flush_workqueue(dev->drd_wq);
+		destroy_workqueue(dev->drd_wq);
+	}
+
+	phy_power_off(dev->udc_phy);
+	phy_exit(dev->udc_phy);
+	extcon_unregister_notifier(dev->edev, EXTCON_USB, &dev->nb);
+
+	dev_info(&pdev->dev, "Synopsys UDC platform driver removed\n");
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int udc_plat_suspend(struct device *dev)
+{
+	struct udc *udc;
+
+	udc = dev_get_drvdata(dev);
+	usb_disconnect(udc);
+
+	if (extcon_get_cable_state_(udc->edev, EXTCON_USB) > 0) {
+		dev_dbg(udc->dev, "device -> idle\n");
+		stop_udc(udc);
+	}
+	phy_power_off(udc->udc_phy);
+	phy_exit(udc->udc_phy);
+
+	return 0;
+}
+
+static int udc_plat_resume(struct device *dev)
+{
+	struct udc *udc;
+	int ret;
+
+	udc = dev_get_drvdata(dev);
+
+	ret = phy_init(udc->udc_phy);
+	if (ret) {
+		dev_err(udc->dev, "UDC phy init failure");
+		return ret;
+	}
+
+	ret = phy_power_on(udc->udc_phy);
+	if (ret) {
+		dev_err(udc->dev, "UDC phy power on failure");
+		phy_exit(udc->udc_phy);
+		return ret;
+	}
+
+	if (extcon_get_cable_state_(udc->edev, EXTCON_USB) > 0) {
+		dev_dbg(udc->dev, "idle -> device\n");
+		start_udc(udc);
+	}
+
+	return 0;
+}
+static const struct dev_pm_ops udc_plat_pm_ops = {
+	.suspend	= udc_plat_suspend,
+	.resume		= udc_plat_resume,
+};
+#endif
+
+#if defined(CONFIG_OF)
+static const struct of_device_id of_udc_match[] = {
+	{ .compatible = "snps,dw-ahb-udc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, of_udc_match);
+#endif
+
+static struct platform_driver udc_plat_driver = {
+	.probe		= udc_plat_probe,
+	.remove		= udc_plat_remove,
+	.driver		= {
+		.name	= "snps-udc-plat",
+		.of_match_table = of_match_ptr(of_udc_match),
+#ifdef CONFIG_PM_SLEEP
+		.pm	= &udc_plat_pm_ops,
+#endif
+	},
+};
+module_platform_driver(udc_plat_driver);
+
+MODULE_DESCRIPTION(UDC_MOD_DESCRIPTION);
+MODULE_AUTHOR("Broadcom");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

[toc] | [prev] | [next] | [standalone]

#1562061 — Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

On 01/17/2017 12:05 AM, Raviteja Garimella wrote:
> This patch splits the amd5536udc driver into two -- one that does
> pci device registration and the other file that does the rest of
> the driver tasks like the gadget/ep ops etc for Synopsys UDC.
> 
> This way of splitting helps in exporting core driver symbols which
> can be used by any other platform/pci driver that is written for
> the same Synopsys USB device controller.
> 
> The current patch also includes a change in the Kconfig and Makefile.
> A new config option USB_SNP_CORE will be selected automatically when
> any one of the platform or pci driver for the same UDC is selected.
> 
> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>

Although the changes you have done make sense and it is most certainly a
good idea to split udc core from bus specific glue logic, it is really
hard to review the changes per-se because of the file rename, could that
happen at a later time?

Also, keep in mind that anytime a driver file is renamed, this poses a
backport/maintenance issue where backporting fixes from latest upstream
to a kernel version that has a different file/directory structure is a
major pain.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1562133 — Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

On Wed, Jan 18, 2017 at 10:45:39AM -0800, Florian Fainelli wrote:
> On 01/17/2017 12:05 AM, Raviteja Garimella wrote:
> > This patch splits the amd5536udc driver into two -- one that does
> > pci device registration and the other file that does the rest of
> > the driver tasks like the gadget/ep ops etc for Synopsys UDC.
> > 
> > This way of splitting helps in exporting core driver symbols which
> > can be used by any other platform/pci driver that is written for
> > the same Synopsys USB device controller.
> > 
> > The current patch also includes a change in the Kconfig and Makefile.
> > A new config option USB_SNP_CORE will be selected automatically when
> > any one of the platform or pci driver for the same UDC is selected.
> > 
> > Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
> 
> Although the changes you have done make sense and it is most certainly a
> good idea to split udc core from bus specific glue logic, it is really
> hard to review the changes per-se because of the file rename, could that
> happen at a later time?
> 
> Also, keep in mind that anytime a driver file is renamed, this poses a
> backport/maintenance issue where backporting fixes from latest upstream
> to a kernel version that has a different file/directory structure is a
> major pain.

Don't ever let stable tree work prevent you from doing the right thing.
If this needs to be split, wonderful, stable kernel work will just have
to deal with it.  Hint, we handle it all the time just fine :)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1562600 — Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

Hi,

On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 01/17/2017 12:05 AM, Raviteja Garimella wrote:
>> This patch splits the amd5536udc driver into two -- one that does
>> pci device registration and the other file that does the rest of
>> the driver tasks like the gadget/ep ops etc for Synopsys UDC.
>>
>> This way of splitting helps in exporting core driver symbols which
>> can be used by any other platform/pci driver that is written for
>> the same Synopsys USB device controller.
>>
>> The current patch also includes a change in the Kconfig and Makefile.
>> A new config option USB_SNP_CORE will be selected automatically when
>> any one of the platform or pci driver for the same UDC is selected.
>>
>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>
> Although the changes you have done make sense and it is most certainly a
> good idea to split udc core from bus specific glue logic, it is really
> hard to review the changes per-se because of the file rename, could that
> happen at a later time?

If you start looking at this specific patch from the header file (amd5536udc.h),
the additions in there comprise of:
- 9 function declarations
- some module parameter variable declarations that's moved out from the older
  common file amd5536udc.c
- 2 #includes that are needed by all files.

So, basically what's done for this split is that:
1. the static keyword is removed from those 9 functions in the new file
    snps_udc_core.c and are exported.
2. The module parameters declarations (since they are used in both core
    and pci file) are moved to the header file now.

Rest all is same as in old amd5536udc.c common file. It's just a copy from the
old file.

And, the file amd5536udc.c will now only do the pci device probe and
remove functions.

I hope this helps. Please let me know of any clarifications needed.
Since both the files are required to be reviewed, I think renaming is
inevitable.

Thanks,
Ravi

>
> Also, keep in mind that anytime a driver file is renamed, this poses a
> backport/maintenance issue where backporting fixes from latest upstream
> to a kernel version that has a different file/directory structure is a
> major pain.
> --
> Florian

[toc] | [prev] | [next] | [standalone]

#1563025 — Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

On 01/19/2017 02:44 AM, Raviteja Garimella wrote:
> Hi,
> 
> On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 01/17/2017 12:05 AM, Raviteja Garimella wrote:
>>> This patch splits the amd5536udc driver into two -- one that does
>>> pci device registration and the other file that does the rest of
>>> the driver tasks like the gadget/ep ops etc for Synopsys UDC.
>>>
>>> This way of splitting helps in exporting core driver symbols which
>>> can be used by any other platform/pci driver that is written for
>>> the same Synopsys USB device controller.
>>>
>>> The current patch also includes a change in the Kconfig and Makefile.
>>> A new config option USB_SNP_CORE will be selected automatically when
>>> any one of the platform or pci driver for the same UDC is selected.
>>>
>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>>
>> Although the changes you have done make sense and it is most certainly a
>> good idea to split udc core from bus specific glue logic, it is really
>> hard to review the changes per-se because of the file rename, could that
>> happen at a later time?
> 
> If you start looking at this specific patch from the header file (amd5536udc.h),
> the additions in there comprise of:
> - 9 function declarations
> - some module parameter variable declarations that's moved out from the older
>   common file amd5536udc.c
> - 2 #includes that are needed by all files.

Well, I don't really question the changes themselves, rather how this is
presented as a patch series to reviewers.

What I would do, to help introduce both the rename, and the splitting of
core vs. bus-glue specific changes is:

- have an initial patch which extracts the core functionality of the
driver and the PCI bus glue logic into adm5536udc_pci.c and left
adm5536udc.c intact (that would be a small delta to review)

- have a second patch that performs the file rename from adm5536udc.c
into snps_udc_core.c and updates adm5536udc_pci.c eventually as a result
of that, then again, a reviewer can ignore the rename part (don't format
to generate your patches with git format-patch -M in that case) and just
focus on the conversion part for adm5536udc_pci.c

> 
> So, basically what's done for this split is that:
> 1. the static keyword is removed from those 9 functions in the new file
>     snps_udc_core.c and are exported.
> 2. The module parameters declarations (since they are used in both core
>     and pci file) are moved to the header file now.

These should really be part of the commit messages for each commit doing
the changes, this is meant to help a reviewer understand what you are
doing, and to some degree, will help him/her make an educated decision
as to what part of the code the focus should be put on.

Thanks
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1564968 — Re: [RFC v2 1/5] UDC: Split the driver into amd (pci) and Synopsys core driver

Hi Florian,

On Fri, Jan 20, 2017 at 12:58 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 01/19/2017 02:44 AM, Raviteja Garimella wrote:
>> Hi,
>>
>> On Thu, Jan 19, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 01/17/2017 12:05 AM, Raviteja Garimella wrote:
>>>> This patch splits the amd5536udc driver into two -- one that does
>>>> pci device registration and the other file that does the rest of
>>>> the driver tasks like the gadget/ep ops etc for Synopsys UDC.
>>>>
>>>> This way of splitting helps in exporting core driver symbols which
>>>> can be used by any other platform/pci driver that is written for
>>>> the same Synopsys USB device controller.
>>>>
>>>> The current patch also includes a change in the Kconfig and Makefile.
>>>> A new config option USB_SNP_CORE will be selected automatically when
>>>> any one of the platform or pci driver for the same UDC is selected.
>>>>
>>>> Signed-off-by: Raviteja Garimella <raviteja.garimella@broadcom.com>
>>>
>>> Although the changes you have done make sense and it is most certainly a
>>> good idea to split udc core from bus specific glue logic, it is really
>>> hard to review the changes per-se because of the file rename, could that
>>> happen at a later time?
>>
>> If you start looking at this specific patch from the header file (amd5536udc.h),
>> the additions in there comprise of:
>> - 9 function declarations
>> - some module parameter variable declarations that's moved out from the older
>>   common file amd5536udc.c
>> - 2 #includes that are needed by all files.
>
> Well, I don't really question the changes themselves, rather how this is
> presented as a patch series to reviewers.
>
> What I would do, to help introduce both the rename, and the splitting of
> core vs. bus-glue specific changes is:
>
> - have an initial patch which extracts the core functionality of the
> driver and the PCI bus glue logic into adm5536udc_pci.c and left
> adm5536udc.c intact (that would be a small delta to review)
>
> - have a second patch that performs the file rename from adm5536udc.c
> into snps_udc_core.c and updates adm5536udc_pci.c eventually as a result
> of that, then again, a reviewer can ignore the rename part (don't format
> to generate your patches with git format-patch -M in that case) and just
> focus on the conversion part for adm5536udc_pci.c
>

Just waited for any more comments coming in. I will submit the next version
as PATCH like the way you suggested.

>>
>> So, basically what's done for this split is that:
>> 1. the static keyword is removed from those 9 functions in the new file
>>     snps_udc_core.c and are exported.
>> 2. The module parameters declarations (since they are used in both core
>>     and pci file) are moved to the header file now.
>
> These should really be part of the commit messages for each commit doing
> the changes, this is meant to help a reviewer understand what you are
> doing, and to some degree, will help him/her make an educated decision
> as to what part of the code the focus should be put on.
>

Will do.

Thanks,
Ravi

> Thanks
> --
> Florian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web