Re: [PATCH v3] phy: Renesas R-Car Gen2 PHY driver

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On Tuesday 22 April 2014 01:43 AM, Sergei Shtylyov wrote:
> Hello.
> 
> On 04/14/2014 09:53 AM, Kishon Vijay Abraham I wrote:
> 
>>> This PHY, though formally being a part of Renesas USBHS controller, contains
>>> the
>>> UGCTRL2 register that controls multiplexing of the USB ports (Renesas calls
>>> them
>>> channels) to the different USB controllers: channel 0 can be connected to
>>> either
>>> PCI EHCI/OHCI or USBHS controllers, channel 2 can be connected to PCI EHCI/OHCI
>>> or xHCI controllers.
> 
>>> This is a new driver for this USB PHY currently already supported under
>>> drivers/
>>> usb/phy/. The reason for writing the new driver was the requirement that the
>>> multiplexing of USB channels to the controller be dynamic, depending on what
>>> USB drivers are loaded,  rather than static as provided by the old driver.
>>> The infrastructure provided by drivers/phy/phy-core.c seems to fit that purpose
>>> ideally. The new driver only supports device tree probing for now.
> 
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> [...]
> 
>>> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
>>> =================================>>> --- /dev/null
>>> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
>>> @@ -0,0 +1,283 @@
> 
> [...]
> 
>>> +static int rcar_gen2_phy_init(struct phy *p)
>>> +{
>>> +    struct rcar_gen2_phy *phy = phy_get_drvdata(p);
>>> +    struct rcar_gen2_phy_driver *drv = phy->drv;
>>> +    unsigned long flags;
>>> +    u32 ugctrl2;
> 
>> We can just add
>>     if (!phy->select_mask)
>>         return 0;
> 
>    Yes, we can, if you'd prefer that.
> 
>>> +
>>> +    if (phy->select_mask) {
>>> +        clk_prepare_enable(drv->clk);
>>> +
>>> +        spin_lock_irqsave(&drv->lock, flags);
>>> +        ugctrl2 = readl(drv->base + USBHS_UGCTRL2);
>>> +        ugctrl2 &= ~phy->select_mask;
>>> +        ugctrl2 |= phy->select_value;
>>> +        writel(ugctrl2, drv->base + USBHS_UGCTRL2);
>>> +        spin_unlock_irqrestore(&drv->lock, flags);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
> 
> [...]
> 
>>> +static int rcar_gen2_phy_probe(struct platform_device *pdev)
>>> +{
> 
> [...]
> 
>>> +    drv->phys[0][0].select_mask  = USBHS_UGCTRL2_USB0SEL;
>>> +    drv->phys[0][0].select_value = USBHS_UGCTRL2_USB0SEL_PCI;
>>> +    drv->phys[0][1].select_mask  = USBHS_UGCTRL2_USB0SEL;
>>> +    drv->phys[0][1].select_value = USBHS_UGCTRL2_USB0SEL_HS_USB;
>>> +    drv->phys[2][0].select_mask  = USBHS_UGCTRL2_USB2SEL;
>>> +    drv->phys[2][0].select_value = USBHS_UGCTRL2_USB2SEL_PCI;
>>> +    drv->phys[2][1].select_mask  = USBHS_UGCTRL2_USB2SEL;
>>> +    drv->phys[2][1].select_value = USBHS_UGCTRL2_USB2SEL_USB30;
>>> +
>>> +    for (i = 0; i < NUM_USB_CHANNELS; i++) {
> 
>> Instead of hard coding the number of channels,
> 
>    It's hard coded in the hardware. We can even decrease that number to 2 as

right, that's why thought dt should have that information.
> for the channel #1 we have nothing to do, regardless of whether it's present or
> not...
> 
>> we can model the channels (PHYs) as sub-nodes of the main PHY
> 
>    Hm, I don't think such representation would be adequate: the channels
> themselves do not usually correspond to any particular PHY, that's why I used
> #phy-cells = <2>.
> 
>> in dt and use it to create individual PHYs.
> 
>    Well, we probably can... however, I fail to see any immediate gain from
> it here...
>    I have to ask why you've selected this particular driver for such DT
> representation experiments, despite it not being the first one supporting
> multiple PHYs?

just that it didn't strike before.. but I think all multiple PHYs should be
modelled this way.

Thanks
Kishon