linux-kernel - Re: [PATCH] usb: phy: samsung: Add support for USB 3.0 phy for exynos5250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 19 Dec 2012 09:55:50 +0200
From:	Felipe Balbi <balbi@...com>
To:	Vivek Gautam <gautamvivek1987@...il.com>
CC:	<balbi@...com>, <linux-usb@...r.kernel.org>,
	<dianders@...omium.org>, <devicetree-discuss@...ts.ozlabs.org>,
	<linux-kernel@...r.kernel.org>,
	<linux-samsung-soc@...r.kernel.org>, <gregkh@...uxfoundation.org>,
	<kgene.kim@...sung.com>, <thomas.abraham@...aro.org>,
	<rob.herring@...xeda.com>, <grant.likely@...retlab.ca>,
	<sylvester.nawrocki@...il.com>, <av.tikhomirov@...sung.com>,
	<yulgon.kim@...sung.com>, <kishon@...com>, <p.paneri@...sung.com>,
	Vivek Gautam <gautam.vivek@...sung.com>
Subject: Re: [PATCH] usb: phy: samsung: Add support for USB 3.0 phy for
 exynos5250

Hi,

On Wed, Dec 19, 2012 at 11:52:01AM +0530, Vivek Gautam wrote:
> >>> @@ -736,7 +1035,41 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
> >>>
> >>>       sphy->clk = clk;
> >>>
> >>> -     return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
> >>> +     sphy->has_usb3 = (sphy->cpu_type == TYPE_EXYNOS5250);
> >>> +
> >>> +     if (sphy->has_usb3) {
> >>> +             struct resource *usb3phy_mem;
> >>> +             void __iomem    *usb3phy_base;
> >>> +
> >>> +             usb3phy_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >>> +             if (!usb3phy_mem) {
> >>> +                     dev_err(dev, "%s: missing mem resource\n", __func__);
> >>> +                     return -ENODEV;
> >>> +             }
> >>> +
> >>> +             usb3phy_base = devm_request_and_ioremap(dev, usb3phy_mem);
> >>> +             if (!usb3phy_base) {
> >>> +                     dev_err(dev, "%s: register mapping failed\n", __func__);
> >>> +                     return -ENXIO;
> >>> +             }
> >>> +
> >>> +             sphy->usb3phy.regs_phy          = usb3phy_base;
> >>> +             sphy->usb3phy.phy.dev           = sphy->dev;
> >>> +             sphy->usb3phy.phy.label         = "samsung-usb3phy";
> >>> +             sphy->usb3phy.phy.init          = samsung_usb3phy_init;
> >>> +             sphy->usb3phy.phy.shutdown      = samsung_usb3phy_shutdown;
> >>> +     }
> >>> +
> >>> +     ret = usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB2);
> >>> +     if (ret)
> >>> +             return ret;
> >>
> >> is this realy how your HW behaves ? USB2 and USB3 phys are a single HW
> >> entity ? I kinda doubt that :-s
> >>
> 
> They are separate HW in fact.
> So, do you expect to see a separate driver interface for USB 3.0 type phy ?

yes. Just as we did on OMAP. One driver for the USB2 part and one driver
for USB3 part (which are actually two, but you can only talk to them as
one) :-)

> That will be quite similar architecturally to current samsung-usbphy
> driver for USB 2.0 type phy,
> and may require some code duplication too.

If it duplicates code, then perhaps it's best to keep it as is but I'm
actually surprised you guys have similar programming model on both
parts. I mean, the differences at HW behavior are huge: on one side you
use ULPI/UTMI+ on the other PIPE3, on one side you have 480Mbps
half-duplex signalling, on the other you have 5Gbps dual simplex
signalling, the differences go on and on.

Also, what you say about duplicating, it seems to me that it will
duplicate only the boylerplate part (allocating memory, having a
platform_driver, and so on), because you _do_ have completely separate
functions to handle usb3 part.

One more comment below:

> +static u32 exynos5_usb3phy_set_clock(struct samsung_usbphy *sphy)
> +{
> +	u32 reg;
> +	u32 refclk;
> +
> +	refclk = sphy->ref_clk_freq;
> +
> +	reg = PHYCLKRST_REFCLKSEL_EXT_REFCLK |
> +		PHYCLKRST_FSEL(refclk);
> +
> +	switch (refclk) {
> +	case HOST_CTRL0_FSEL_CLKSEL_50M:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_50M_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x00));
> +		break;
> +	case HOST_CTRL0_FSEL_CLKSEL_20M:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x00));
> +		break;
> +	case HOST_CTRL0_FSEL_CLKSEL_19200K:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x88));
> +		break;
> +	case HOST_CTRL0_FSEL_CLKSEL_24M:
> +	default:
> +		reg |= (PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF |
> +			PHYCLKRST_SSC_REFCLKSEL(0x88));
> +		break;
> +	}
> +
> +	return reg;
> +}

looks like this should be done by commong clock framework by clock
reparenting perhaps ?

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)