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

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

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 
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?

> Thanks
> Kishon

WBR, Sergei