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@xxxxxxxxxxxxxxxxxx>
>
> [...]
>
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
