Re: [PATCH v2] usb: phy: samsung: Add support to set pmu isolation

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi,

On 12/19/2012 02:44 PM, Vivek Gautam wrote:
>>>> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>>>> @@ -9,3 +9,15 @@ Required properties:
>>>>    - compatible : should be "samsung,exynos4210-usbphy"
>>>>    - reg : base physical address of the phy registers and length of memory
>>>> mapped
>>>>          region.
>>>> +
>>>> +Optional properties:
>>>> +- samsung,usb-phyhandle : should point to usb-phyhandle sub-node which
>>>> provides
>>>> +                       binding data to enable/disable device PHY handled
>>>> by
>>>> +                       PMU register.
>>>> +
>>>> +                       Required properties:
>>>> +                       - compatible : should be "samsung,usbdev-phyctrl"
>>>> for
>>>> +                                       DEVICE type phy.
>>>> +                       - samsung,phyhandle-reg: base physical address of
>>>> +                                               PHY_CONTROL register in
>>>> PMU.
>>>> +- samsung,enable-mask : should be '1'
>>>
>>>
>>> This should only be 1 for Exynos4210+ SoCs, right ?
>
> Yes that's true Exynso4210+ SoCs have only single PHY for both host and device
> which gets enabled by single bit.
>
>>> S5PV210 uses bit 0 for OTG and bit1 for USB host, doesn't it ? And for
>>> s3c64xx
>>> it seems to be bit 16.
>>>
> True, S5PV210 uses two bits separately for OTG and HOST, so in that
> case we would
> require to set both these bits. Thanks for pointing out !!
>
> I couldn't see device tree support for S5PV210 and S3C64xx so thought
> of going for
> 4210+ SoCs. Better we make this more generic so that once these SoCs
> are moved to
> device tree we don't face any issue. Right ??

Fair enough. Yes, let's not make any future addition of the device tree
support for the older Samsung platforms unnecessarily difficult. It doesn't
take much effort, and there is many drivers that are shared by multiple 
SoCs
already, starting from s3c64xx to exynos4 series.

>>> How about deriving this information from 'compatible' property instead ?
>
> It will definitely be good to use the compatible property to derive
> such information,
> Need to amend the current approach.
>
>>> Maybe you could just encode the USB PMU registers (I assume those aren't
>>> touched by anything but the usb drivers) in a regular 'reg' property in
>>> an usbphy subnode. Then the driver could interpret it also with help
>>> of 'compatible' property. And you could just use of_iomap(). E.g.
>>>
>>>   usbphy@12130000 {
>>>          compatible = "samsung,exynos5250-usbphy";
>>>          reg =<0x12130000 0x100>,<0x12100000 0x100>;
>>>          usbphy-pmu {
>>>                  /* USB device and host PHY_CONTROL registers */
>>>                  reg =<0x10040704 8>;
>>>          };
>>>   };
>>>
>
> This approach seems nice.
>
>>> Your "samsung,usb-phyhandle" approach seems over-engineered to me.
>>> I might be missing something though.
>>>
>
> The idea behind using phandles for usb-phy was to get the multiple
> registers (2 in PMU
> and 1 in SYSREG) and program them separately as required.

You could still specify multiple <address, size> pairs in 'reg' property
or perhaps use separate node for SYSREG. And if on some SoCs PHY_CONTROL
registers do not immediately follow each other in memory it might make
sense to define the bindings so that each register is specified separately,
e.g.

reg = <0x10040704 4>, <0x10040708 4>, <0x10050230 4>;

However AFAICS single region for the PHY_CONTROL registers should be
sufficient for all existing SoCs.

--

Thanks,
Sylwester