Re: [PATCH v5] usb: phy: samsung: Add support to set pmu isolation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux