| 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
| ||
|
Message-ID: <CAFp+6iGc1zEqwpbGKejDSDta12ASZ=_P7dFBKmtJpTnE7-VTwA@mail.gmail.com>
Date: Mon, 28 Apr 2014 11:17:53 +0530
From: Vivek Gautam <gautam.vivek@...sung.com>
To: Tomasz Figa <tomasz.figa@...il.com>
Cc: Linux USB Mailing List <linux-usb@...r.kernel.org>,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
linux-doc@...r.kernel.org, kishon <kishon@...com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Greg KH <gregkh@...uxfoundation.org>,
Felipe Balbi <balbi@...com>,
Kukjin Kim <kgene.kim@...sung.com>,
Tomasz Figa <t.figa@...sung.com>,
Kamil Debski <k.debski@...sung.com>,
Sylwester Nawrocki <sylvester.nawrocki@...il.com>
Subject: Re: [PATCH v5 1/2] phy: Add new Exynos5 USB 3.0 PHY driver
Hi Tomasz,
On Sat, Apr 26, 2014 at 4:33 AM, Tomasz Figa <tomasz.figa@...il.com> wrote:
> Hi Vivek,
>
>
> On 22.04.2014 10:03, Vivek Gautam wrote:
>>
>> Add a new driver for the USB 3.0 PHY on Exynos5 series of SoCs.
>> The new driver uses the generic PHY framework and will interact
>> with DWC3 controller present on Exynos5 series of SoCs.
>> Thereby, removing old phy-samsung-usb3 driver and related code
>> used untill now which was based on usb/phy framework.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@...sung.com>
>> ---
>> .../devicetree/bindings/phy/samsung-phy.txt | 40 ++
>> drivers/phy/Kconfig | 11 +
>> drivers/phy/Makefile | 1 +
>> drivers/phy/phy-exynos5-usbdrd.c | 629
>> ++++++++++++++++++++
>> 4 files changed, 681 insertions(+)
>> create mode 100644 drivers/phy/phy-exynos5-usbdrd.c
>>
>> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> b/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> index b422e38..51efe4c 100644
>> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> @@ -114,3 +114,43 @@ Example:
>> compatible = "samsung,exynos-sataphy-i2c";
>> reg = <0x38>;
>> };
>> +
>> +Samsung Exynos5 SoC series USB DRD PHY controller
[snip]
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 3bb05f1..8a5d2b4 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -166,4 +166,15 @@ config PHY_XGENE
>> help
>> This option enables support for APM X-Gene SoC multi-purpose
>> PHY.
>>
>> +config PHY_EXYNOS5_USBDRD
>> + tristate "Exynos5 SoC series USB DRD PHY driver"
>> + depends on ARCH_EXYNOS5 && OF
>> + depends on HAS_IOMEM
>> + select GENERIC_PHY
>> + select MFD_SYSCON
>> + help
>> + Enable USB DRD PHY support for Exynos 5 SoC series.
>> + This driver provides PHY interface for USB 3.0 DRD controller
>> + present on Exynos5 SoC series.
>> +
>
>
> I think you should probably keep the entries sorted, so this one should be
> somewhere around other EXYNOS PHYs.
Right, thanks for pointing this out.
Will move this along with other PHY_EXYNOS USB* configs.
>
>
>> endmenu
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 2faf78e..31baa0c 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -18,3 +18,4 @@ obj-$(CONFIG_PHY_EXYNOS4210_USB2) +=
>> phy-exynos4210-usb2.o
>> obj-$(CONFIG_PHY_EXYNOS4X12_USB2) += phy-exynos4x12-usb2.o
>> obj-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.o
>> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
>> +obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o
>
>
> Ditto.
Ok
>
>
>> diff --git a/drivers/phy/phy-exynos5-usbdrd.c
>> b/drivers/phy/phy-exynos5-usbdrd.c
>> new file mode 100644
>> index 0000000..89d7ae8
>> --- /dev/null
>> +++ b/drivers/phy/phy-exynos5-usbdrd.c
>> @@ -0,0 +1,629 @@
>> +/*
>> + * Samsung EXYNOS5 SoC series USB DRD PHY driver
>> + *
>> + * Phy provider for USB 3.0 DRD controller on Exynos5 SoC series
>> + *
>> + * Copyright (C) 2014 Samsung Electronics Co., Ltd.
>> + * Author: Vivek Gautam <gautam.vivek@...sung.com>
>> + *
[snip]
>> + } phys[EXYNOS5_DRDPHYS_NUM];
>> + unsigned int extrefclk;
>> + struct clk *ref_clk;
>> + unsigned long ref_rate;
>> +};
>> +
>> +#define to_usbdrd_phy(inst) \
>> + container_of((inst), struct exynos5_usbdrd_phy, \
>> + phys[(inst)->index]);
>
>
> This should be made a static inline to enforce type checking.
Ok, will make this as static inline routine, so that compiler don't
skip type checking.
>
>
>> +
>> +/*
>> + * exynos5_rate_to_clk() converts the supplied clock rate to the value
>> that
>> + * can be written to the phy register.
>> + */
>> +static unsigned int exynos5_rate_to_clk(unsigned long rate)
>> +{
>> + unsigned int clksel;
>> +
>> + /* EXYNOS5_FSEL_MASK */
>> +
>> + switch (rate) {
>> + case 9600 * KHZ:
>> + clksel = EXYNOS5_FSEL_9MHZ6;
>> + break;
>> + case 10 * MHZ:
>> + clksel = EXYNOS5_FSEL_10MHZ;
>> + break;
>> + case 12 * MHZ:
>> + clksel = EXYNOS5_FSEL_12MHZ;
>> + break;
>> + case 19200 * KHZ:
>> + clksel = EXYNOS5_FSEL_19MHZ2;
>> + break;
>> + case 20 * MHZ:
>> + clksel = EXYNOS5_FSEL_20MHZ;
>> + break;
>> + case 24 * MHZ:
>> + clksel = EXYNOS5_FSEL_24MHZ;
>> + break;
>> + case 50 * MHZ:
>> + clksel = EXYNOS5_FSEL_50MHZ;
>> + break;
>> + default:
>> + clksel = -EINVAL;
>
>
> Based on clksel (and return value of this function) being unsigned I don't
> think this is a good idea. You should probably adapt the approach from
> Exynos USB 2 PHY, where a function like this return an integer status code
> and returns the bitfield value through a pointer passed as another argument.
Right, will amend this as suggested.
>
>
>> + }
>> +
>> + return clksel;
>> +}
>> +
>> +static void exynos5_usbdrd_phy_isol(struct phy_usb_instance *inst,
>> + unsigned int on)
>> +{
>> + if (!inst->reg_pmu)
>> + return;
>> +
>> + regmap_update_bits(inst->reg_pmu, inst->pmu_offset,
>> + EXYNOS5_USBDRD_PMU_ISOL, ~on);
>
>
> I don't think ~on is correct here. Even if technically it produces the
> correct value, because bit 0 is being changed here, this should be fixed. If
> EXYNOS5_USBDRD_PMU_ISOL wasn't BIT(0), then always 1 would be written, as on
> could be 0 or 1 and ~on respectively 0xffffffff or 0xfffffffe.
>
> I'd suggest something like this:
>
> unsigned int val = on ? 0 : EXYNOS5_USBDRD_PMU_ISOL;
Sure will change this as suggested.
[snip]
>> +const struct usbdrd_phy_config exynos5_usbdrd_phy_cfg[] = {
>> + {
>> + .id = EXYNOS5_DRDPHY_UTMI,
>> + .phy_isol = exynos5_usbdrd_phy_isol,
>> + .phy_init = exynos5_usbdrd_utmi_init,
>> + .set_refclk = exynos5_usbdrd_utmi_set_refclk,
>> + },
>> + {
>> + .id = EXYNOS5_DRDPHY_PIPE3,
>> + .phy_isol = exynos5_usbdrd_phy_isol,
>> + .phy_init = exynos5_usbdrd_pipe3_init,
>> + .set_refclk = exynos5_usbdrd_pipe3_set_refclk,
>> + },
>> + {},
>
>
> You seem to use a fixed number of PHYs. Do you still need this sentinel
> entry?
Right, we don't need this sentinel entry since we have limited the
number of PHYs to EXYNOS5_DRDPHYS_NUM.
Will remove this.
>
>
>> +};
[snip]
>> +
>> + match = of_match_node(exynos5_usbdrd_phy_of_match,
>> pdev->dev.of_node);
>> + if (!match) {
>
>
> This can't happen, otherwise probe() wouldn't be called at all.
True, will remove this check.
--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
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