lkml.org 
[lkml]   [2013]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2013-01-10 05:21    [W:0.078 / U:2.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site