Messages in this thread | | | Date | Wed, 09 Jan 2013 22:42:16 +0100 | From | Sylwester Nawrocki <> | Subject | Re: [PATCH v5] usb: phy: samsung: Add support to set pmu isolation |
| |
Hi,
On 12/28/2012 10:13 AM, 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> ... > --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt > +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt > @@ -9,3 +9,38 @@ 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. > +- ranges: allows valid translation between child's address space and parent's > + address space. > + > +- The child node 'usbphy-sys' to the node 'usbphy' is for the system controller > + interface for usb-phy. It should provide the following information required by > + usb-phy controller to control phy. > + - reg : base physical address of PHY control register in PMU which > + enables/disables the phy controller.
On some SoCs USB_PHY_CONTROL registers don't belong to PMU. So maybe you should drop references to PMU, or list all SoC entities where USB_PHY_CONTROL appears: PMU, "MISC REGISTER", etc.
I would just rephrase this to:
- reg : base physical address of PHY_CONTROL registers
"phy controller" might be confusing, since PHY CONTROLLER is an entity separate from PHY 0 and PHY 1 ?
> + The size of this register is the total sum of size of all phy-control
And what about using PHY_CONTROL name as per the User Manuals ? That would perhaps make it a bit easier to follow.
> + 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: > + - Exynos4210 > + > + usbphy@125B0000 { > + #address-cells =<1>; > + #size-cells =<1>; > + compatible = "samsung,exynos4210-usbphy"; > + reg =<0x125B0000 0x100>; > + ranges; > + > + usbphy-sys { > + /* USB device and host PHY_CONTROL registers */ > + reg =<0x10020704 0x8>; > + }; > + }; ... > /* > + * 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 > + * @pmureg_devphy_offset: offset to DEVICE PHY CONTROL register from > + * mapped address of system controller. > + * > + * 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 position 0 and 1 respectively. > + * Although for newer SoCs like exynos these bits belong to > + * different registers altogether placed at position 0. > + */ > +struct samsung_usbphy_drvdata { > + int cpu_type; > + int devphy_en_mask; > + u32 pmureg_devphy_offset;
Perhaps just "devphy_reg_offset" would do ?
> +}; > + > +/* > * 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
Is this more precisely:
* @regs: usb phy controller registers memory base ? > + * @pmureg: usb device phy-control pmu register memory base
Maybe something like this would be more clear:
@pmureg: USB device PHY_CONTROL registers memory region base
Note, that not on all SoCs USB_PHY_CONTROL registers belong to PMU. Haven't you considered changing "pmureg" to ctrl_regs or something else more generic ?
> * @ref_clk_freq: reference clock frequency selection > - * @cpu_type: machine identifier > + * @drv_data: driver data available for different SoCs > */ > struct samsung_usbphy { > struct usb_phy phy; > @@ -81,12 +107,63 @@ struct samsung_usbphy { > struct device *dev; > struct clk *clk; > void __iomem *regs; > + void __iomem *pmureg; > int ref_clk_freq; > - int cpu_type; > + const struct samsung_usbphy_drvdata *drv_data; > }; ... > +/* > + * Set isolation here for phy. > + * SOCs control this by controlling corresponding PMU registers
Hmm, it's not always PMU registers. I would remove this sentence and instead explain what's the meaning of 'on' argument, so it is clear the PHY is deactivated when on != 0.
> + */ > +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on) > +{ > + static DEFINE_SPINLOCK(lock);
You probably don't need a global spinlock. Couldn't the spinlock be added as struct samsung_usbhy field instead ?
> + unsigned long flags; > + void __iomem *reg; > + u32 reg_val; > + u32 en_mask; > + > + if (!sphy->pmureg) { > + dev_warn(sphy->dev, "Can't set pmu isolation\n"); > + return; > + } > + > + reg = sphy->pmureg + sphy->drv_data->pmureg_devphy_offset; > + en_mask = sphy->drv_data->devphy_en_mask; > + > + spin_lock_irqsave(&lock, flags); > + > + reg_val = readl(reg); > + reg_val = on ? (reg_val& ~en_mask) : (reg_val | en_mask);
Might be a good idea to use in this case plain if/else instead..
> + writel(reg_val, reg); > + > + spin_unlock_irqrestore(&lock, flags); > +}
Thanks, Sylwester
| |