linux-kernel - Re: [PATCH v5 4/8] dt-bindings: PCI: kirin: Drop PHY properties

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]

Message-ID: <20210716132208.3cd8f404@coco.lan>
Date:   Fri, 16 Jul 2021 13:22:08 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Rob Herring <robh@...nel.org>
Cc:     Bjorn Helgaas <helgaas@...nel.org>, linuxarm@...wei.com,
        mauro.chehab@...wei.com, Manivannan Sadhasivam <mani@...nel.org>,
        Binghui Wang <wangbinghui@...ilicon.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Xiaowei Song <songxiaowei@...ilicon.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH v5 4/8] dt-bindings: PCI: kirin: Drop PHY properties

Em Tue, 13 Jul 2021 20:28:49 -0600
Rob Herring <robh@...nel.org> escreveu:

> On Tue, Jul 13, 2021 at 08:28:37AM +0200, Mauro Carvalho Chehab wrote:
> > There are several properties there that belong to the PHY
> > interface. Drop them, as a new binding file will describe
> > the PHY properties for Kirin 960.  
> 
> Folks are okay with an incompatible change on hikey960?

Accepting an incompatible change here seems the right thing to do.

Another possibility would be to create a "pcie-kirin-with-phy" driver
that would be identical to the existing one, except for the absence
of a PHY and using a different compatible string.

-

Long answer:

There aren't many alternatives here, if we want to split the PHY out of
the driver, as you requested.

I've been scratching my head in order to find a way that would keep
the Hikey960 a separate PHY driver, with a proper DT schema, but
capable of also parse the original DT schema.

See, making the phy driver parse the PCIE-based OF-node data is 
trivial (I have already a patch doing that), but it will require at
least some DT schema additions, in order to add a pcie_phy node[1]:

<snip>
diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index e0eca598af1f..6aaa2f966d74 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -1001,6 +1001,11 @@ spi3: spi@...b3000 {
                        status = "disabled";
                };
 
+               pcie_phy: pcie-phy@...2000 {
+                       compatible = "hisilicon,hi960-pcie-phy";
+                       #phy-cells = <0>;
+               };
+
                pcie@...00000 {
                        compatible = "hisilicon,kirin960-pcie";
                        reg = <0x0 0xf4000000 0x0 0x1000>,
@@ -1012,6 +1017,7 @@ pcie@...00000 {
                        #address-cells = <3>;
                        #size-cells = <2>;
                        device_type = "pci";
+                       phys = <&pcie_phy>;
                        ranges = <0x02000000 0x0 0x00000000
                                  0x0 0xf6000000
                                  0x0 0x02000000>;
</snip>

[1] or, alternatively, the pcie-kirin driver would need to dynamically
    populate DT with the above, as some ACPI drivers do when the
    firmware is broken.

Without a PHY representation at the DT schema, the PHY driver won't 
be recognized by pcie-kirin.

See, even if the pcie-kirin driver would be changed to register
the PHY without DT, with:

	phy = devm_of_phy_get(dev, NULL, "hi3660_pcie_phy");

The phy_get() implementation will internally ignore a non-DT PHY,
as internally, it uses of_property_match_string() if the caller driver
has of_node:

	struct phy *phy_get(struct device *dev, const char *string)
	{
		int index = 0;
		struct phy *phy;
		struct device_link *link;

		if (dev->of_node) {
			if (string)
				index = of_property_match_string(dev->of_node, "phy-names",
					string);
			else
				index = 0;
			phy = _of_phy_get(dev->of_node, index);
		} else {
			if (string == NULL) {
				dev_WARN(dev, "missing string\n");
				return ERR_PTR(-EINVAL);
			}
			phy = phy_find(dev, string);
		}

Thanks,
Mauro