linux-kernel - Re: [PATCH v6 1/3] phy: add driver for Qualcomm IPQ40xx USB PHY

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] [day] [month] [year] [list]

Message-ID: <CA+HBbNHT7bOM68zBGAHO0Pi9WrBc244Qewwe5JV7fNhNUGPZ4Q@mail.gmail.com>
Date:   Tue, 28 Apr 2020 14:47:58 +0200
From:   Robert Marko <robert.marko@...tura.hr>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        linux-kernel@...r.kernel.org,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        robh+dt@...nel.org, Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, John Crispin <john@...ozen.org>,
        Luka Perkov <luka.perkov@...tura.hr>
Subject: Re: [PATCH v6 1/3] phy: add driver for Qualcomm IPQ40xx USB PHY

On Mon, Apr 27, 2020 at 6:45 PM Vinod Koul <vkoul@...nel.org> wrote:
>
> Hello Robert,
>
> On 01-04-20, 18:35, Robert Marko wrote:
>
> > +static int ipq4019_ss_phy_power_on(struct phy *_phy)
> > +{
> > +     struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
> > +
> > +     ipq4019_ss_phy_power_off(_phy);
> > +
> > +     reset_control_deassert(phy->por_rst);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct phy_ops ipq4019_usb_ss_phy_ops = {
> > +     .power_on       = ipq4019_ss_phy_power_on,
> > +     .power_off      = ipq4019_ss_phy_power_off,
> > +};
> > +
> > +static int ipq4019_hs_phy_power_off(struct phy *_phy)
> > +{
> > +     struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
> > +
> > +     reset_control_assert(phy->por_rst);
> > +     msleep(10);
>
> why not call ipq4019_ss_phy_power_off() here as well?
Its not necessary, SS and HS PHY-s are separated but share
the same register space.
So when HS PHY is controlled, SS PHY can remain working.
>
> > +
> > +     reset_control_assert(phy->srif_rst);
> > +     msleep(10);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ipq4019_hs_phy_power_on(struct phy *_phy)
> > +{
> > +     struct ipq4019_usb_phy *phy = phy_get_drvdata(_phy);
> > +
> > +     ipq4019_hs_phy_power_off(_phy);
> > +
> > +     reset_control_deassert(phy->srif_rst);
> > +     msleep(10);
> > +
> > +     reset_control_deassert(phy->por_rst);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct phy_ops ipq4019_usb_hs_phy_ops = {
> > +     .power_on       = ipq4019_hs_phy_power_on,
> > +     .power_off      = ipq4019_hs_phy_power_off,
> > +};
>
> So this is fiddling with resets, what about phy configuration and
> calibration, who take care of that?
As as I understand, since I don't have documentation access is that no
calibration and configuration except to properly reset them are needed.
Development hardware required some magic register values to be
written but in the previous revisions of this driver it was
discovered that they were leftovers from the development HW.
>
> > +static int ipq4019_usb_phy_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct resource *res;
> > +     struct phy_provider *phy_provider;
> > +     struct ipq4019_usb_phy *phy;
> > +     const struct of_device_id *match;
> > +
> > +     match = of_match_device(ipq4019_usb_phy_of_match, &pdev->dev);
> > +     if (!match)
> > +             return -ENODEV;
>
> you are using this to get match-data few lines below, why not use
> of_device_get_match_data() and get the match->data which you are
> interested in?
Thanks, I will look into it.
>
> --
> ~Vinod