lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 19 Dec 2012 11:05:51 +0530
From:	Vivek Gautam <gautamvivek1987@...il.com>
To:	linux-usb@...r.kernel.org
Cc:	dianders@...omium.org,
	Sylwester Nawrocki <sylvester.nawrocki@...il.com>,
	l.majewski@...sung.com, linux-samsung-soc@...r.kernel.org,
	p.paneri@...sung.com, gregkh@...uxfoundation.org,
	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	balbi@...com, kyungmin.park@...sung.com, kgene.kim@...sung.com,
	ben-linux@...ff.org, broonie@...nsource.wolfsonmicro.com,
	Vivek Gautam <gautam.vivek@...sung.com>
Subject: Re: [PATCH v2] usb: phy: samsung: Add support to set pmu isolation

CC: Doug Anderson


On Wed, Dec 19, 2012 at 4:49 AM, Sylwester Nawrocki
<sylvester.nawrocki@...il.com> wrote:
> 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@...sung.com>
>> ---
>>
>> 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@...30000 {
>         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",&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
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@...ts.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss



-- 
Thanks & Regards
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists