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@xxxxxxxxxxx>
...
--- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
