From: Sylwester Nawrocki <sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Vivek Gautam <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: l.majewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
p.paneri-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
balbi-l0cyMroinI0@public.gmane.org,
kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org,
ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org,
broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Subject: Re: [PATCH v2] usb: phy: samsung: Add support to set pmu isolation
Date: Wed, 19 Dec 2012 00:19:06 +0100 [thread overview]
Message-ID: <50D0F9EA.9090002@gmail.com> (raw)
In-Reply-To: <1355838966-11269-1-git-send-email-gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Hi Vivek,
On 12/18/2012 02:56 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>
> Changes from v1:
> - Changed the name of property for phy handler from'samsung,usb-phyctrl'
> to 'samsung,usb-phyhandle' to make it look more generic.
> - Similarly 'samsung,phyctrl-reg' is changed to 'samsung,phyhandle-reg'
> - Added a check for 'samsung,usb-phyhandle' before getting node from
> phandle.
> - Putting the node using 'of_node_put()' which had been missed.
> - Adding necessary check for the pointer in 'samsung_usbphy_set_isolation()'
> to avoid any NULL pointer dereferencing.
> - Unmapping the register ioremapped in 'samsung_usbphy_parse_dt_param()'.
>
>
> .../devicetree/bindings/usb/samsung-usbphy.txt | 12 +++
> drivers/usb/phy/samsung-usbphy.c | 94 ++++++++++++++++++--
> 2 files changed, 98 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> index 7b26e2d..a7b28b2 100644
> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
> @@ -9,3 +9,15 @@ Required properties:
> - compatible : should be "samsung,exynos4210-usbphy"
> - reg : base physical address of the phy registers and length of memory mapped
> region.
> +
> +Optional properties:
> +- samsung,usb-phyhandle : should point to usb-phyhandle sub-node which provides
> + binding data to enable/disable device PHY handled by
> + PMU register.
> +
> + Required properties:
> + - compatible : should be "samsung,usbdev-phyctrl" for
> + DEVICE type phy.
> + - samsung,phyhandle-reg: base physical address of
> + PHY_CONTROL register in PMU.
> +- samsung,enable-mask : should be '1'
This should only be 1 for Exynos4210+ SoCs, right ?
S5PV210 uses bit 0 for OTG and bit1 for USB host, doesn't it ? And for
s3c64xx
it seems to be bit 16.
How about deriving this information from 'compatible' property instead ?
Maybe you could just encode the USB PMU registers (I assume those aren't
touched by anything but the usb drivers) in a regular 'reg' property in
an usbphy subnode. Then the driver could interpret it also with help
of 'compatible' property. And you could just use of_iomap(). E.g.
usbphy@12130000 {
compatible = "samsung,exynos5250-usbphy";
reg = <0x12130000 0x100>, <0x12100000 0x100>;
usbphy-pmu {
/* USB device and host PHY_CONTROL registers */
reg = <0x10040704 8>;
};
};
Your "samsung,usb-phyhandle" approach seems over-engineered to me.
I might be missing something though.
> diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
> index 5c5e1bb5..4ceabe3 100644
> --- a/drivers/usb/phy/samsung-usbphy.c
> +++ b/drivers/usb/phy/samsung-usbphy.c
> @@ -72,6 +72,8 @@ enum samsung_cpu_type {
> * @dev: The parent device supplied to the probe function
> * @clk: usb phy clock
> * @regs: usb phy register memory base
> + * @devctrl_reg: usb device phy-control pmu register memory base
hum, this name is not really helpful in understanding what's going
on here.
Looking at arch/arm/mach-s5pv210/setup-usb-phy.c, there is only one
PHY_CONTROL (Power Management Unit) register for both OTG and USB host
PHYs. So are you really taking care of that case as well ?
> + * @en_mask: enable mask
> * @ref_clk_freq: reference clock frequency selection
> * @cpu_type: machine identifier
> */
> @@ -81,12 +83,73 @@ struct samsung_usbphy {
> struct device *dev;
> struct clk *clk;
> void __iomem *regs;
> + void __iomem *devctrl_reg;
> + u32 en_mask;
> int ref_clk_freq;
> int cpu_type;
> };
>
> #define phy_to_sphy(x) container_of((x), struct samsung_usbphy, phy)
>
> +static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)
> +{
> + struct device_node *usb_phyctrl;
> + u32 reg;
> + int lenp;
> +
> + if (!sphy->dev->of_node) {
> + sphy->devctrl_reg = NULL;
> + return -ENODEV;
> + }
> +
> + if (of_get_property(sphy->dev->of_node, "samsung,usb-phyhandle",&lenp)) {
> + usb_phyctrl = of_parse_phandle(sphy->dev->of_node,
> + "samsung,usb-phyhandle", 0);
> + if (!usb_phyctrl) {
> + dev_warn(sphy->dev, "Can't get usb-phy handle\n");
> + sphy->devctrl_reg = NULL;
> + }
> +
> + of_property_read_u32(usb_phyctrl, "samsung,phyhandle-reg",®);
> +
> + sphy->devctrl_reg = ioremap(reg, SZ_4);
What happens if invalid address value is assigned to
'samsung,phyhandle-reg' ?
> + of_property_read_u32(sphy->dev->of_node, "samsung,enable-mask",
> + &sphy->en_mask);
> + of_node_put(usb_phyctrl);
> + } else {
> + dev_warn(sphy->dev, "Can't get usb-phy handle\n");
> + sphy->devctrl_reg = NULL;
> + }
> +
> + 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)
> +{
> + void __iomem *usb_phyctrl_reg;
> + u32 en_mask = sphy->en_mask;
> + u32 reg;
> +
> + usb_phyctrl_reg = sphy->devctrl_reg;
> +
> + if (!usb_phyctrl_reg) {
> + dev_warn(sphy->dev, "Can't set pmu isolation\n");
> + return;
> + }
> +
> + reg = readl(usb_phyctrl_reg);
> +
> + if (on)
> + writel(reg& ~en_mask, usb_phyctrl_reg);
> + else
> + writel(reg | en_mask, usb_phyctrl_reg);
> +}
> +
> /*
> * Returns reference clock frequency selection value
> */
> @@ -199,6 +262,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,6 +293,8 @@ 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);
> }
> @@ -249,17 +316,12 @@ static inline int samsung_usbphy_get_driver_data(struct platform_device *pdev)
> 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 +345,20 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
> return PTR_ERR(clk);
> }
>
> - sphy->dev =&pdev->dev;
> + sphy->dev =&pdev->dev;
sphy->dev = dev;
> +
> + ret = samsung_usbphy_parse_dt_param(sphy);
> + if (ret) {
> + /* fallback to pdata */
> + if (!pdata) {
> + dev_err(&pdev->dev,
> + "%s: no device data found\n", __func__);
I find term "device data" a bit confusing here.
> + return -ENODEV;
In the original code -EINVAL was returned when platform_data was not set.
> + } else {
> + sphy->plat = pdata;
> + }
> + }
> +
How about rewriting this to:
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;
}
}
This way we won't be obfuscating any error code returned from the
OF parsing function.
> sphy->plat = pdata;
> sphy->regs = phy_base;
> sphy->clk = clk;
> @@ -305,6 +380,9 @@ static int __exit samsung_usbphy_remove(struct platform_device *pdev)
>
> usb_remove_phy(&sphy->phy);
>
> + if (sphy->devctrl_reg)
> + iounmap(sphy->devctrl_reg);
> +
> return 0;
> }
--
Regards,
Sylwester
next prev parent reply other threads:[~2012-12-18 23:19 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-18 5:56 [PATCH] usb: phy: samsung: Add support to set pmu isolation Vivek Gautam
[not found] ` <1355810167-18425-1-git-send-email-gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2012-12-18 5:56 ` Vivek Gautam
2012-12-18 13:56 ` [PATCH v2] " Vivek Gautam
[not found] ` <1355838966-11269-1-git-send-email-gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2012-12-18 23:19 ` Sylwester Nawrocki [this message]
[not found] ` <50D0F9EA.9090002-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-12-19 5:35 ` Vivek Gautam
2012-12-19 13:44 ` Vivek Gautam
2012-12-19 20:28 ` Sylwester Nawrocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50D0F9EA.9090002@gmail.com \
--to=sylvester.nawrocki-re5jqeeqqe8avxtiumwx3w@public.gmane.org \
--cc=balbi-l0cyMroinI0@public.gmane.org \
--cc=ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org \
--cc=broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org \
--cc=devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org \
--cc=gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
--cc=gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org \
--cc=kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
--cc=kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
--cc=l.majewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
--cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=p.paneri-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).