Messages in this thread Patch in this message | | | Date | Wed, 26 Dec 2012 23:30:27 +0100 | From | Sylwester Nawrocki <> | Subject | Re: [PATCH v4] usb: phy: samsung: Add support to set pmu isolation |
| |
On 12/26/2012 01:28 PM, Vivek Gautam wrote: > Adding support to parse device node data in order to get > required properties to set pmu isolation for usb-phy. > > Signed-off-by: Vivek Gautam<gautam.vivek@samsung.com> > --- > .../devicetree/bindings/usb/samsung-usbphy.txt | 31 ++++ > drivers/usb/phy/samsung-usbphy.c | 145 +++++++++++++++++--- > 2 files changed, 155 insertions(+), 21 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt > index 7b26e2d..6b873fd 100644 > --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt > +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt > @@ -9,3 +9,34 @@ Required properties: > - compatible : should be "samsung,exynos4210-usbphy" > - reg : base physical address of the phy registers and length of memory mapped > region. > + > +Optional properties: > +- #address-cells: should be '1' when usbphy node has a child node with 'reg' > + property. > +- #size-cells: should be '1' when usbphy node has a child node with 'reg' > + property. > + > +- The child node 'usbphy-pmu' to the node usbphy should provide the following > + information required by usb-phy controller to enable/disable the phy. > + - reg : base physical address of PHY control register in PMU which > + enables/disables the phy controller. > + The size of this register is the total sum of size of all phy-control > + registers that the SoC has. For example, the size will be > + '0x4' in case we have only one phy-control register (like in S3C64XX) or > + '0x8' in case we have two phy-control registers (like in Exynos4210) > + and so on. > + > +Example: > + - Exysno4210
s/Exysno/Exynos
> + > + usbphy@125B0000 { > + #address-cells =<1>; > + #size-cells =<1>; > + compatible = "samsung,exynos4210-usbphy"; > + reg =<0x125B0000 0x100>; > + > + usbphy-pmu { > + /* USB device and host PHY_CONTROL registers */ > + reg =<0x10020704 0x8>; > + }; > + }; > diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c > index 5c5e1bb5..b9f4f42 100644 > --- a/drivers/usb/phy/samsung-usbphy.c > +++ b/drivers/usb/phy/samsung-usbphy.c > @@ -60,20 +60,42 @@ > #define MHZ (1000*1000) > #endif > > +#define S3C64XX_USBPHY_ENABLE (0x1<< 16) > +#define EXYNOS_USBPHY_ENABLE (0x1<< 0) > + > enum samsung_cpu_type { > TYPE_S3C64XX, > TYPE_EXYNOS4210, > }; > > /* > + * struct samsung_usbphy_drvdata - driver data for various SoC variants > + * @cpu_type: machine identifier > + * @devphy_en_mask: device phy enable mask for PHY CONTROL register > + * > + * Here we have a separate mask for device type phy. > + * Having different masks for host and device type phy helps > + * in setting independent masks in case of SoCs like S5PV210, > + * in which PHY0 and PHY1 enable bits belong to same register > + * placed at [0] and [1] respectively.
"and are placed at positions 0 and 1 respectively" ?
> + * Although for newer SoCs like exynos these bits belong to > + * different registers altogether placed at [0]. > + */ > +struct samsung_usbphy_drvdata { > + int cpu_type; > + int devphy_en_mask; > +}; > + > +/* > * struct samsung_usbphy - transceiver driver state > * @phy: transceiver structure > * @plat: platform data > * @dev: The parent device supplied to the probe function > * @clk: usb phy clock > * @regs: usb phy register memory base > + * @phyctrl_pmureg: usb device phy-control pmu register memory base
nit: Perhaps we could just call it "pmureg' ?
> * @ref_clk_freq: reference clock frequency selection > - * @cpu_type: machine identifier > + * @drv_data: driver data available for different cpu types
Actually it's for different SoCs, not CPUs, right ?
> */ > struct samsung_usbphy { > struct usb_phy phy; > @@ -81,12 +103,67 @@ struct samsung_usbphy { > struct device *dev; > struct clk *clk; > void __iomem *regs; > + void __iomem *phyctrl_pmureg; > int ref_clk_freq; > - int cpu_type; > + const struct samsung_usbphy_drvdata *drv_data; > }; > > #define phy_to_sphy(x) container_of((x), struct samsung_usbphy, phy) > > +static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)
nit: s/samsung_usbphy_parse_dt_param/samsung_usbphy_parse_dt ?
> +{ > + struct device_node *usbphy_pmu; > + u32 reg[2]; > + int ret; > + > + if (!sphy->dev->of_node) { > + dev_err(sphy->dev, "Can't get usb-phy node\n"); > + return -ENODEV;
The function is called only when dev->of_node is not NULL, so this path is never executed, AFAICS. I would just drop this redundant check.
> + } > + > + usbphy_pmu = of_get_child_by_name(sphy->dev->of_node, "usbphy-pmu"); > + if (!usbphy_pmu) > + dev_warn(sphy->dev, "Can't get usb-phy pmu control node\n");
> + ret = of_property_read_u32_array(usbphy_pmu, "reg", reg, > + ARRAY_SIZE(reg)); > + if (!ret) > + sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
I don't think this is correct. reg is not really an array of u32 type, it's an array of address/size tuples. I thought you would use of_iomap() here. Any reason why you didn't do so ? It would also make it easier to handle multiple address/size pairs if required.
> + > + of_node_put(usbphy_pmu); > + > + if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {
When is sphy->phyctrl_pmureg assigned a ERR_PTR() value ? Wouldn't it be enough to simply check for NULL ?
> + dev_err(sphy->dev, "Can't get usb-phy pmu control register\n"); > + return -ENODEV; > + } > + > + return 0; > +} > + > +/* > + * Set isolation here for phy. > + * SOCs control this by controlling corresponding PMU registers > + */ > +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on) > +{ > + u32 reg; > + int en_mask; > + > + if (!sphy->phyctrl_pmureg) { > + dev_warn(sphy->dev, "Can't set pmu isolation\n"); > + return; > + } > + > + reg = readl(sphy->phyctrl_pmureg);
To make it more generic maybe it's worth to store an offset to the actual register somewhere, e.g. in the driver_data struct ? This function is supposed to handle both device and host PHYs, isn't it ?
> + en_mask = sphy->drv_data->devphy_en_mask; > + > + if (on) > + writel(reg& ~en_mask, sphy->phyctrl_pmureg); > + else > + writel(reg | en_mask, sphy->phyctrl_pmureg);
nit: This could be also written as:
reg = on ? reg & ~mask : reg | mask; writel(reg, sphy->phyctrl_pmureg); > +} > + > /* > * Returns reference clock frequency selection value > */ > @@ -112,7 +189,7 @@ static int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy) > refclk_freq = PHYCLK_CLKSEL_48M; > break; > default: > - if (sphy->cpu_type == TYPE_S3C64XX) > + if (sphy->drv_data->cpu_type == TYPE_S3C64XX) > refclk_freq = PHYCLK_CLKSEL_48M; > else > refclk_freq = PHYCLK_CLKSEL_24M; > @@ -135,7 +212,7 @@ static void samsung_usbphy_enable(struct samsung_usbphy *sphy) > phypwr = readl(regs + SAMSUNG_PHYPWR); > rstcon = readl(regs + SAMSUNG_RSTCON); > > - switch (sphy->cpu_type) { > + switch (sphy->drv_data->cpu_type) { > case TYPE_S3C64XX: > phyclk&= ~PHYCLK_COMMON_ON_N; > phypwr&= ~PHYPWR_NORMAL_MASK; > @@ -165,7 +242,7 @@ static void samsung_usbphy_disable(struct samsung_usbphy *sphy) > > phypwr = readl(regs + SAMSUNG_PHYPWR); > > - switch (sphy->cpu_type) { > + switch (sphy->drv_data->cpu_type) { > case TYPE_S3C64XX: > phypwr |= PHYPWR_NORMAL_MASK; > break; > @@ -199,6 +276,8 @@ static int samsung_usbphy_init(struct usb_phy *phy) > /* Disable phy isolation */ > if (sphy->plat&& sphy->plat->pmu_isolation) > sphy->plat->pmu_isolation(false); > + else > + samsung_usbphy_set_isolation(sphy, false); > > /* Initialize usb phy registers */ > samsung_usbphy_enable(sphy); > @@ -228,38 +307,37 @@ static void samsung_usbphy_shutdown(struct usb_phy *phy) > /* Enable phy isolation */ > if (sphy->plat&& sphy->plat->pmu_isolation) > sphy->plat->pmu_isolation(true); > + else > + samsung_usbphy_set_isolation(sphy, true); > > clk_disable_unprepare(sphy->clk); > } > > static const struct of_device_id samsung_usbphy_dt_match[]; > > -static inline int samsung_usbphy_get_driver_data(struct platform_device *pdev) > +static inline struct samsung_usbphy_drvdata > +*samsung_usbphy_get_driver_data(struct platform_device *pdev) > { > if (pdev->dev.of_node) { > const struct of_device_id *match; > match = of_match_node(samsung_usbphy_dt_match, > pdev->dev.of_node); > - return (int) match->data; > + return (struct samsung_usbphy_drvdata *) match->data; > } > > - return platform_get_device_id(pdev)->driver_data; > + return (struct samsung_usbphy_drvdata *) > + platform_get_device_id(pdev)->driver_data; > } > > static int __devinit samsung_usbphy_probe(struct platform_device *pdev) > { > struct samsung_usbphy *sphy; > - struct samsung_usbphy_data *pdata; > + struct samsung_usbphy_data *pdata = pdev->dev.platform_data; > struct device *dev =&pdev->dev; > struct resource *phy_mem; > void __iomem *phy_base; > struct clk *clk; > - > - pdata = pdev->dev.platform_data; > - if (!pdata) { > - dev_err(&pdev->dev, "%s: no platform data defined\n", __func__); > - return -EINVAL; > - } > + int ret; > > phy_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!phy_mem) { > @@ -283,7 +361,19 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev) > return PTR_ERR(clk); > } > > - sphy->dev =&pdev->dev; > + sphy->dev = dev; > + > + if (dev->of_node) { > + ret = samsung_usbphy_parse_dt_param(sphy); > + if (ret< 0) > + return ret; > + } else { > + if (!pdata) { > + dev_err(dev, "no platform data specified\n"); > + return -EINVAL; > + } > + } > + > sphy->plat = pdata; > sphy->regs = phy_base; > sphy->clk = clk;
--
Thanks, Sylwester
| |