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.
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
---
.../devicetree/bindings/pci/kirin-pcie.txt | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
index 71cac2b74002..a93a8cfa1afb 100644
--- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
@@ -10,13 +10,11 @@ Additional properties are described here:
Required properties
- compatible:
"hisilicon,kirin960-pcie"
-- reg: Should contain rc_dbi, apb, phy, config registers location and length.
+- reg: Should contain rc_dbi, apb, config registers location and length.
- reg-names: Must include the following entries:
"dbi": controller configuration registers;
"apb": apb Ctrl register defined by Kirin;
- "phy": apb PHY register defined by Kirin;
"config": PCIe configuration space registers.
-- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
Optional properties:
@@ -25,8 +23,8 @@ Example based on kirin960:
pcie@f4000000 {
compatible = "hisilicon,kirin960-pcie";
reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
- <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
- reg-names = "dbi","apb","phy", "config";
+ <0x0 0xF4000000 0 0x2000>;
+ reg-names = "dbi","apb", "config";
bus-range = <0x0 0x1>;
#address-cells = <3>;
#size-cells = <2>;
@@ -39,12 +37,4 @@ Example based on kirin960:
<0x0 0 0 2 &gic 0 0 0 283 4>,
<0x0 0 0 3 &gic 0 0 0 284 4>,
<0x0 0 0 4 &gic 0 0 0 285 4>;
- clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
- <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
- <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
- <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
- <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
- clock-names = "pcie_phy_ref", "pcie_aux",
- "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk";
- reset-gpios = <&gpio11 1 0 >;
};
--
2.31.1
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?
>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
> .../devicetree/bindings/pci/kirin-pcie.txt | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/kirin-pcie.txt b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> index 71cac2b74002..a93a8cfa1afb 100644
> --- a/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/kirin-pcie.txt
> @@ -10,13 +10,11 @@ Additional properties are described here:
> Required properties
> - compatible:
> "hisilicon,kirin960-pcie"
> -- reg: Should contain rc_dbi, apb, phy, config registers location and length.
> +- reg: Should contain rc_dbi, apb, config registers location and length.
> - reg-names: Must include the following entries:
> "dbi": controller configuration registers;
> "apb": apb Ctrl register defined by Kirin;
> - "phy": apb PHY register defined by Kirin;
> "config": PCIe configuration space registers.
> -- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
>
> Optional properties:
>
> @@ -25,8 +23,8 @@ Example based on kirin960:
> pcie@f4000000 {
> compatible = "hisilicon,kirin960-pcie";
> reg = <0x0 0xf4000000 0x0 0x1000>, <0x0 0xff3fe000 0x0 0x1000>,
> - <0x0 0xf3f20000 0x0 0x40000>, <0x0 0xF4000000 0 0x2000>;
> - reg-names = "dbi","apb","phy", "config";
> + <0x0 0xF4000000 0 0x2000>;
> + reg-names = "dbi","apb", "config";
> bus-range = <0x0 0x1>;
> #address-cells = <3>;
> #size-cells = <2>;
> @@ -39,12 +37,4 @@ Example based on kirin960:
> <0x0 0 0 2 &gic 0 0 0 283 4>,
> <0x0 0 0 3 &gic 0 0 0 284 4>,
> <0x0 0 0 4 &gic 0 0 0 285 4>;
> - clocks = <&crg_ctrl HI3660_PCIEPHY_REF>,
> - <&crg_ctrl HI3660_CLK_GATE_PCIEAUX>,
> - <&crg_ctrl HI3660_PCLK_GATE_PCIE_PHY>,
> - <&crg_ctrl HI3660_PCLK_GATE_PCIE_SYS>,
> - <&crg_ctrl HI3660_ACLK_GATE_PCIE>;
> - clock-names = "pcie_phy_ref", "pcie_aux",
> - "pcie_apb_phy", "pcie_apb_sys", "pcie_aclk";
> - reset-gpios = <&gpio11 1 0 >;
> };
> --
> 2.31.1
>
>
Em Tue, 13 Jul 2021 20:28:49 -0600
Rob Herring <[email protected]> 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?
I hope so ;-)
I mean, it should be easy to add a backward-compatible code that
would make the PHY driver to use the pci-bus old schema if there's
no PHY entry at DT.
However, this is not enough, as the PHY driver won't be loaded/probed
without at least this at hi3660.dtsi:
pcie_phy: pcie-phy@f3f2000 {
compatible = "hisilicon,hi960-pcie-phy";
};
So, some (probably ugly) hack would be needed at pcie-kirin, in order
to make it to manually load and probe the PHY driver, if it
founds (for instance) "phy" reg-name as a pcie-kirin property.
Thanks,
Mauro
Em Tue, 13 Jul 2021 20:28:49 -0600
Rob Herring <[email protected]> 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@ff3b3000 {
status = "disabled";
};
+ pcie_phy: pcie-phy@f3f2000 {
+ compatible = "hisilicon,hi960-pcie-phy";
+ #phy-cells = <0>;
+ };
+
pcie@f4000000 {
compatible = "hisilicon,kirin960-pcie";
reg = <0x0 0xf4000000 0x0 0x1000>,
@@ -1012,6 +1017,7 @@ pcie@f4000000 {
#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