From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Subject: Re: [PATCH v3] phy: Renesas R-Car Gen2 PHY driver Date: Tue, 22 Apr 2014 00:13:50 +0400 Message-ID: <53557BFE.5020908@cogentembedded.com> References: <201404120415.43761.sergei.shtylyov@cogentembedded.com> <534B77E2.8060901@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <534B77E2.8060901@ti.com> Sender: linux-sh-owner@vger.kernel.org To: Kishon Vijay Abraham I , devicetree@vger.kernel.org, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, grant.likely@linaro.org Cc: linux-sh@vger.kernel.org, magnus.damm@gmail.com, linux-doc@vger.kernel.org, rdunlap@infradead.org List-Id: devicetree@vger.kernel.org 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 [...] >> 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