| 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
| ||
|
Date: Wed, 18 Jan 2017 14:43:08 +0530
From: Vivek Gautam <vivek.gautam@...eaurora.org>
To: Kishon Vijay Abraham I <kishon@...com>, robh+dt@...nel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Cc: mark.rutland@....com, sboyd@...eaurora.org,
bjorn.andersson@...aro.org, srinivas.kandagatla@...aro.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v4 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom
chips
Hi Kishon,
On 01/16/2017 02:15 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 10 January 2017 04:21 PM, Vivek Gautam wrote:
>> PHY transceiver driver for QUSB2 phy controller that provides
>> HighSpeed functionality for DWC3 controller present on
>> Qualcomm chipsets.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@...eaurora.org>
>> Reviewed-by: Stephen Boyd <sboyd@...eaurora.org>
>> ---
>>
>> Changes since v3:
>> - Added 'Reviewed-by' from Stephen.
>> - Fixed debug message for qusb2_phy_set_tune2_param().
>> - Replaced devm_reset_control_get() with devm_reset_control_get_by_index()
>> since we are requesting only one reset.
>> - Updated devm_nvmem_cell_get() with a NULL cell id.
>> - Made error labels more idiomatic.
>> - Refactored qusb2_setbits() and qusb2_clrbits() a little bit to accept
>> base address and register offset as two separate arguments.
>>
>> Changes since v2:
>> - Removed selecting 'RESET_CONTROLLER' config.
>> - Added error handling for clk_prepare_enable paths.
>> - Removed explicitly setting ref_clk rate to 19.2 MHz. Don't need to
>> do that since 'xo' is modeled as parent to this clock.
>> - Removed 'ref_clk_src' handling. Driver doesn't need to request and
>> handle this clock.
>> - Moved nvmem_cell_get() to probe function.
>> - Simplified phy pll status handling.
>> - Using of_device_get_match_data() to get match data.
>> - Uniformly using lowercase for hex numbers.
>> - Fixed sparse warnings.
>> - Using shorter variable names in structure and in functions.
>> - Handling various comment style shortcomings.
>>
>> Changes since v1:
>> - removed reference to clk_enabled/pwr_enabled.
>> - moved clock and regulator enable code to phy_power_on/off() callbacks.
>> - fixed return on EPROBE_DEFER in qusb2_phy_probe().
>> - fixed phy create and phy register ordering.
>> - removed references to non-lkml links from commit message.
>> - took care of other minor nits.
>> - Fixed coccinelle warnings -
>> 'PTR_ERR applied after initialization to constant'
>> - Addressed review comment, regarding qfprom access for tune2 param value.
>> This driver is now based on qfprom patch[1] that allows byte access now.
>>
>> drivers/phy/Kconfig | 10 +
>> drivers/phy/Makefile | 1 +
>> drivers/phy/phy-qcom-qusb2.c | 539 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 550 insertions(+)
>> create mode 100644 drivers/phy/phy-qcom-qusb2.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index e8eb7f225a88..0ed53d018b23 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -430,6 +430,16 @@ config PHY_STIH407_USB
>> Enable this support to enable the picoPHY device used by USB2
>> and USB3 controllers on STMicroelectronics STiH407 SoC families.
>>
>> +config PHY_QCOM_QUSB2
>> + tristate "Qualcomm QUSB2 PHY Driver"
>> + depends on OF && (ARCH_QCOM || COMPILE_TEST)
>> + select GENERIC_PHY
>> + help
>> + Enable this to support the HighSpeed QUSB2 PHY transceiver for USB
>> + controllers on Qualcomm chips. This driver supports the high-speed
>> + PHY which is usually paired with either the ChipIdea or Synopsys DWC3
>> + USB IPs on MSM SOCs.
>> +
>> config PHY_QCOM_UFS
>> tristate "Qualcomm UFS PHY driver"
>> depends on OF && ARCH_QCOM
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 65eb2f436a41..dad1682b80e3 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -49,6 +49,7 @@ obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY) += phy-spear1310-miphy.o
>> obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) += phy-spear1340-miphy.o
>> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
>> obj-$(CONFIG_PHY_STIH407_USB) += phy-stih407-usb.o
>> +obj-$(CONFIG_PHY_QCOM_QUSB2) += phy-qcom-qusb2.o
>> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs.o
>> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-20nm.o
>> obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-14nm.o
>> diff --git a/drivers/phy/phy-qcom-qusb2.c b/drivers/phy/phy-qcom-qusb2.c
>> new file mode 100644
>> index 000000000000..c69118610164
>> --- /dev/null
>> +++ b/drivers/phy/phy-qcom-qusb2.c
>> @@ -0,0 +1,539 @@
>> +/*
>> + * Copyright (c) 2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/nvmem-consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#define QUSB2PHY_PLL_TEST 0x04
>> +#define CLK_REF_SEL BIT(7)
>> +
>> +#define QUSB2PHY_PLL_TUNE 0x08
>> +#define QUSB2PHY_PLL_USER_CTL1 0x0c
>> +#define QUSB2PHY_PLL_USER_CTL2 0x10
>> +#define QUSB2PHY_PLL_AUTOPGM_CTL1 0x1c
>> +#define QUSB2PHY_PLL_PWR_CTRL 0x18
>> +
>> +#define QUSB2PHY_PLL_STATUS 0x38
>> +#define PLL_LOCKED BIT(5)
>> +
>> +#define QUSB2PHY_PORT_TUNE1 0x80
>> +#define QUSB2PHY_PORT_TUNE2 0x84
>> +#define QUSB2PHY_PORT_TUNE3 0x88
>> +#define QUSB2PHY_PORT_TUNE4 0x8c
>> +#define QUSB2PHY_PORT_TUNE5 0x90
>> +#define QUSB2PHY_PORT_TEST2 0x9c
>> +
>> +#define QUSB2PHY_PORT_POWERDOWN 0xb4
>> +#define CLAMP_N_EN BIT(5)
>> +#define FREEZIO_N BIT(1)
>> +#define POWER_DOWN BIT(0)
>> +
>> +#define QUSB2PHY_REFCLK_ENABLE BIT(0)
>> +
>> +#define PHY_CLK_SCHEME_SEL BIT(0)
>> +
>> +struct qusb2_phy_init_tbl {
>> + unsigned int offset;
>> + unsigned int val;
>> +};
>> +#define QUSB2_PHY_INIT_CFG(o, v) \
>> + { \
>> + .offset = o, \
>> + .val = v, \
>> + }
>> +
>> +static const struct qusb2_phy_init_tbl msm8996_init_tbl[] = {
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE1, 0xf8),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE2, 0xb3),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE3, 0x83),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TUNE4, 0xc0),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_TUNE, 0x30),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL1, 0x79),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_USER_CTL2, 0x21),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PORT_TEST2, 0x14),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_AUTOPGM_CTL1, 0x9f),
>> + QUSB2_PHY_INIT_CFG(QUSB2PHY_PLL_PWR_CTRL, 0x00),
>> +};
> I wish all this data comes from device tree and one API in phy-core can do all
> these settings. Your other driver qcom-qmp also seems to have a bunch of
> similar settings.
>
> The problem is every vendor driver adds a bunch of code to perform the same
> thing again and again when all of these settings can be done by a single phy API.
Yes, i understand this. You have commented similar thing in the patch
from Jaehoon -
[PATCH V2 2/5] phy: phy-exynos-pcie: Add support for Exynos PCIe phy
I would like to understand the requirements here.
Would you like me to get all this information from the device tree -
an array of register offset and value pair, which we can then program
by calling a phy_ops (may be calibrate) ? Something of this sort:
phy-calibrate-data = <val1, register_offset1>,
<val2, register_offset2>,
<val3, register_offset3>,
....
I am sure having such information in the driver (like i have in my patch)
makes the driver look more clumsy.
But, all this data can be pretty huge - a set of some 100+
register-value pairs
for QMP phy, for example. So, will it be okay to get this from device tree ?
We also note here that such information changes from one IP version to
another.
I remember Rob having some concerns about it.
Regards
Vivek
> Thanks
> Kishon
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Powered by blists - more mailing lists