linux-kernel - Re: [PATCH v4 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips

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:   Mon, 23 Jan 2017 15:43:06 +0530
From:   Vivek Gautam <vivek.gautam@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>
Cc:     "robh+dt" <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Mark Rutland <mark.rutland@....com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Srinivas Kandagatla <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

On Wed, Jan 18, 2017 at 11:33 PM, Bjorn Andersson
<bjorn.andersson@...aro.org> wrote:
> On Wed 18 Jan 01:13 PST 2017, Vivek Gautam wrote:
>> On 01/16/2017 02:15 PM, Kishon Vijay Abraham I wrote:
>> > Hi,
>> >
>> > On Tuesday 10 January 2017 04:21 PM, Vivek Gautam wrote:
> [..]
>> > > +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.
>>
>
> The devicetree is supposed to describe which hardware components a
> certain device has, most of the time this carries a set of properties to
> describe how this piece is connected and configured.
>
> A dump of magic register values does not describe how the QMP is
> connected to anything and is, as far as this patch shows, static for
> this particular hardware block.

Yes, that's correct. The QMP and QUSB2 phy init sequences are a bunch
of static values for a particular IP version. These values hardly give a
meaningful data to put few phy bindings that could be referenced
to configure the phy further.

>
> Further more moving this blob to devicetree will not allow us to treat
> the various QMP configurations as one HW block, as there are other
> differences as well.
>
> Like many other drivers it's possible to create a generic version that
> has every bit of logic driven by configuration from devicetree, but like
> most of those cases this is not the way we split things.
>
> And this has the side effect of keeping the dts files human readable,
> human understandable and human maintainable.

That's right. These register-value pairs (100+ for qmp) don't give a human
understandable data when put in dts.

Kishon,
Please let me know if you have concerns.

If this looks good otherwise, please consider taking this for 4.11.


Regards
Vivek
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project