Message-ID: <CAOGqxeV3GzmkX0_Q-UOpOutZ0YXAUbMRLn5119qgON6Jza-ZRg@mail.gmail.com>
Date:   Wed, 31 Aug 2016 14:05:38 -0400
From:   Alan Cooper <alcooperx@...il.com>
To:     Kishon Vijay Abraham I <kishon@...com>
Cc:     USB list <linux-usb@...r.kernel.org>, linux-kernel@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com
Subject: Re: [PATCH V3 2/2] usb: phy: phy-brcm-usb: Add Broadcom STB USB Phy driver

On Wed, Aug 31, 2016 at 5:57 AM, Kishon Vijay Abraham I <kishon@...com> wrote:
> Hi,
>
> On Tuesday 30 August 2016 08:00 PM, Al Cooper wrote:
>> Add a new USB Phy driver for Broadcom STB SoCs. This driver
>> supports all Broadcom STB ARM SoCs. This driver in combination
>> with the generic ohci, ehci and xhci platform drivers will enable
>> USB1.1, USB2.0 and USB3.0 support. This Phy driver also supports
>> the Broadcom UDC gadget driver.
>
> Remove usb: from $subject

Ok


>>
>> Signed-off-by: Al Cooper <alcooperx@...il.com>
>> ---
>>  .../bindings/phy/brcm,brcmstb-usb-phy.txt          |  39 +
>>  MAINTAINERS                                        |   7 +
>>  drivers/phy/Kconfig                                |  10 +
>>  drivers/phy/Makefile                               |   2 +
>>  drivers/phy/phy-brcm-usb-init.c                    | 792 +++++++++++++++++++++
>
> other broadcom phy drivers use only "bcm" in the file name.

We have a mixed use of both names throughout the kernel. Notice
"phy-brcm-sata.c in the phy directory. "brcm" seems a little better
because it matches the device tree prefix used by all drivers and is
used by many of the latest drivers.


>> diff --git a/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt b/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt
>> new file mode 100644
>> index 0000000..34fa9dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt
>> @@ -0,0 +1,39 @@
>> +Broadcom STB USB PHY
>> +
>> +Required properties:
>> + - compatible: brcm,brcmstb-usb-phy
>> + - reg: two offset and length pairs. The second pair specifies the
>> +        USB 3.0 related registers and is only required for PHYs
>> +        that support USB 3.0
>
> I think the right way to model this should be to have separate subnodes for
> usb2 and usb3.

The first register block contains both the 2.0 and 3.0 phy registers.
The second register block contains an optional "workaround" register
set used on just a few SoCs to workaround a bug in the XHCI phy. Also,
this approach allows us to use the already parsed resource and saves
some additional code to get and map the registers.


>> + - #phy-cells: Shall be 1 as it expects one argument for setting
>> +            the type of the PHY. Possible values are 0 (1.1 and 2.0),
>> +            1 (3.0)
>> +
>> +
> spurious blank space.

Ok


>> +Optional Properties:
>> +- clocks : phandle + clock specifier for the phy clocks
>> +- clock-names: string, clock name
>> +- ipp: Invert Port Power
>> +- ioc: Invert Over Current detection
>> +- has_xhci: Contains an optional 3.0 PHY
>
> prefix all the broadcom specific properties with "bcm," or whatever is used for
> broadcom specific properties.

Ok


>> +- device: PHY Device mode. Possible values are: 0 (Host), 1 (Device)
>> +          or 2 (DRD)
>> +
>> +
>> +
>
> spurious blank spaces..

Ok


>> +Example:
>> +
>> +usbphy_0: usb-phy@...70200 {
>> +     reg = <0xf0470200 0xb8>,
>> +             <0xf0471940 0x6c0>;
>> +     #address-cells = <1>;
>> +     #size-cells = <1>;
>
> Why do you need address cells, size cells? Include it in the documentation.

I don't, I'll remove them.


>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 19bff3a..5ff5e47 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -443,6 +443,16 @@ config PHY_CYGNUS_PCIE
>>         Enable this to support the Broadcom Cygnus PCIe PHY.
>>         If unsure, say N.
>>
>> +config BRCM_USB_PHY
>> +     tristate "Broadcom USB PHY driver"
>> +     depends on OF && USB && ARCH_BRCMSTB
>
> Please also include COMPILE_TEST

Ok


>> +     select GENERIC_PHY
>> +     default y
>> +     help
>> +       Enable this to support the Broadcom USB PHY on
>> +       Broadcom STB SoCs.
>> +       If unsure, say Y.
>
> Generally it is N.

Since this driver should work on any "ARCH_BRCMSTB" system and
"ARCH_BRCMSTB" is included in the "depends", it seems like this should
be defaulted to Y.


>> diff --git a/drivers/phy/phy-brcm-usb-init.c b/drivers/phy/phy-brcm-usb-init.c
>> new file mode 100644
>> index 0000000..f5d8c32
>> --- /dev/null
>> +++ b/drivers/phy/phy-brcm-usb-init.c
>> @@ -0,0 +1,792 @@
>> +/*
>> + * Copyright (C) 2014-2016 Broadcom
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *    notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *    notice, this list of conditions and the following disclaimer in the
>> + *    documentation and/or other materials provided with the distribution.
>> + * 3. Neither the name of the project nor the names of its contributors
>> + *    may be used to endorse or promote products derived from this software
>> + *    without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE PROJECT AND CONTRIBUTORS ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE PROJECT OR CONTRIBUTORS BE LIABLE
>> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
>> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
>> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
>
> I'd prefer a standard GPL file header used by modules supporting GPL v2 (Since
> your module license has GPL v2).

Ok


>> +
>> +/*
>> + * This module is used by both the bootloader and Linux and
>
> Sorry, how can this module be used in bootloader?

A copy of the phy-brcm-usb-init.c file is also compiled as part of our
bootloader.


>
>> + * contains USB initialization for power up and S3 resume.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +
>> +#include <linux/soc/brcmstb/brcmstb.h>
>> +#include "phy-brcm-usb-init.h"
>> +
>> +/* Register definitions for the USB CTRL block */
>> +#define USB_CTRL_SETUP                       0x00
>> +#define   USB_CTRL_SETUP_IOC_MASK                    0x00000010
>> +#define   USB_CTRL_SETUP_IPP_MASK                    0x00000020
>> +#define   USB_CTRL_SETUP_BABO_MASK                   0x00000001
>> +#define   USB_CTRL_SETUP_FNHW_MASK                   0x00000002
>> +#define   USB_CTRL_SETUP_FNBO_MASK                   0x00000004
>> +#define   USB_CTRL_SETUP_WABO_MASK                   0x00000008
>> +#define   USB_CTRL_SETUP_scb1_en_MASK                        0x00004000 /* option */
>
> please avoid camelcase.

Ok


>> +
>> +#define to_brcm_usb_phy_data(phy) \
>> +     container_of((phy), struct brcm_usb_phy_data, phys[(phy)->index])
>> +
>> +static struct phy_ops brcm_usb_phy_ops = {
>> +     .owner          = THIS_MODULE,
>
> Ah.. no phy ops? Do you really use the phy framework?

We have some additional runtime suspend/resume functionality in our
PHY that will use the ops, but the hardware is not yet fully
functional. The fact that phys are probed before the OHCI, EHCI and
XHCI drivers means that I didn't have to put explicit calls in these
drivers and can just use the probe and bus suspend/resume routines.


>
> Thanks

Thanks

Powered by blists - more mailing lists