| Message ID | 20220416135458.104048-5-linux@fw-web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | RK3568 PCIe V3 support | expand |
On Sat, Apr 16, 2022 at 03:54:56PM +0200, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > PCIe Lanes can be split to 2 slots with bifurcation. > Add support for this in existing pcie driver. Please s/pcie/PCIe/ in subject and above to be consistent. You also have kind of a random usage in other patches. Mention the DT property used for this in the commit log. Is the "rockchip,bifurcation" DT property something that should be generalized so it's not rockchip-specific? Other controllers are likely to support similar functionality. > Co-developed-by: Peter Geis <pgwipeout@gmail.com> > Signed-off-by: Frank Wunderlich <frank-w@public-files.de> > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index 863374604fb1..1b0c2115b32e 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -20,6 +20,7 @@ > #include <linux/platform_device.h> > #include <linux/regmap.h> > #include <linux/reset.h> > +#include <linux/phy/pcie.h> > > #include "pcie-designware.h" > > @@ -59,6 +60,7 @@ struct rockchip_pcie { > struct regulator *vpcie3v3; > struct irq_domain *irq_domain; > raw_spinlock_t irq_lock; > + bool bifurcation; > }; > > static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > @@ -273,6 +275,12 @@ static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) > return dev_err_probe(dev, PTR_ERR(rockchip->phy), > "missing PHY\n"); > > + if (rockchip->bifurcation) { > + ret = phy_set_mode_ext(rockchip->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_BIFURCATION); > + if (ret) > + return ret; > + } > + > ret = phy_init(rockchip->phy); > if (ret < 0) > return ret; > @@ -345,6 +353,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > } > } > > + if (device_property_read_bool(dev, "rockchip,bifurcation")) > + rockchip->bifurcation = true; > + > ret = rockchip_pcie_phy_init(rockchip); > if (ret) > goto disable_regulator; > -- > 2.25.1 > > > -- > linux-phy mailing list > linux-phy@lists.infradead.org > https://lists.infradead.org/mailman/listinfo/linux-phy
Hi, > Gesendet: Sonntag, 17. April 2022 um 01:30 Uhr > Von: "Bjorn Helgaas" <helgaas@kernel.org> thanks for first review > On Sat, Apr 16, 2022 at 03:54:56PM +0200, Frank Wunderlich wrote: > > From: Frank Wunderlich <frank-w@public-files.de> > > > > PCIe Lanes can be split to 2 slots with bifurcation. > > Add support for this in existing pcie driver. > > Please s/pcie/PCIe/ in subject and above to be consistent. You also > have kind of a random usage in other patches. will do > Mention the DT property used for this in the commit log. good point noticed that i forgot to add it to pcie-bindings (rockchip-dw-pcie.yaml). > Is the "rockchip,bifurcation" DT property something that should be > generalized so it's not rockchip-specific? Other controllers are > likely to support similar functionality. I do not know if other controllers support similar functionality, but i ack a property without vendor prefix is better. Should i use "bifurcation" as name or do you think about a different name which is more generic? regards Frank
On Sun, Apr 17, 2022 at 11:08:02AM +0200, Frank Wunderlich wrote: > > On Sat, Apr 16, 2022 at 03:54:56PM +0200, Frank Wunderlich wrote: > > > From: Frank Wunderlich <frank-w@public-files.de> > > > > > > PCIe Lanes can be split to 2 slots with bifurcation. > > > Add support for this in existing pcie driver. > > Is the "rockchip,bifurcation" DT property something that should be > > generalized so it's not rockchip-specific? Other controllers are > > likely to support similar functionality. > > I do not know if other controllers support similar functionality, > but i ack a property without vendor prefix is better. Should i use > "bifurcation" as name or do you think about a different name which > is more generic? Really a question for Rob about what name would be good and where it should go.
On Mon, Apr 18, 2022 at 11:53 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Sun, Apr 17, 2022 at 11:08:02AM +0200, Frank Wunderlich wrote: > > > On Sat, Apr 16, 2022 at 03:54:56PM +0200, Frank Wunderlich wrote: > > > > From: Frank Wunderlich <frank-w@public-files.de> > > > > > > > > PCIe Lanes can be split to 2 slots with bifurcation. > > > > Add support for this in existing pcie driver. > > > > Is the "rockchip,bifurcation" DT property something that should be > > > generalized so it's not rockchip-specific? Other controllers are > > > likely to support similar functionality. > > > > I do not know if other controllers support similar functionality, > > but i ack a property without vendor prefix is better. Should i use > > "bifurcation" as name or do you think about a different name which > > is more generic? > > Really a question for Rob about what name would be good and where it > should go. It might be good to define this as a lane map. In the Rockchip implementation it's only 2+0 or 1+1, but that isn't guaranteed if this is made into a standard definition. So perhaps: pcie-bifurcation-map = <0>, <1>; pcie-bifurcation-map = <1>; pcie-bifurcation-map = <4>, <5>, <6>, <7>;
> Gesendet: Montag, 18. April 2022 um 18:17 Uhr > Von: "Peter Geis" <pgwipeout@gmail.com> > > On Sun, Apr 17, 2022 at 11:08:02AM +0200, Frank Wunderlich wrote: > > > > On Sat, Apr 16, 2022 at 03:54:56PM +0200, Frank Wunderlich wrote: > > > > > From: Frank Wunderlich <frank-w@public-files.de> > > > > > > > > > > PCIe Lanes can be split to 2 slots with bifurcation. > > > > > Add support for this in existing pcie driver. > > > > > > Is the "rockchip,bifurcation" DT property something that should be > > > > generalized so it's not rockchip-specific? Other controllers are > > > > likely to support similar functionality. > > > > > > I do not know if other controllers support similar functionality, > > > but i ack a property without vendor prefix is better. Should i use > > > "bifurcation" as name or do you think about a different name which > > > is more generic? > > > > Really a question for Rob about what name would be good and where it > > should go. > > It might be good to define this as a lane map. > In the Rockchip implementation it's only 2+0 or 1+1, but that isn't > guaranteed if this is made into a standard definition. > So perhaps: > pcie-bifurcation-map = <0>, <1>; > pcie-bifurcation-map = <1>; > pcie-bifurcation-map = <4>, <5>, <6>, <7>; how about a lane-map like this (from controllers point of view): rockchip with only 2 lanes (like rk3568): controller 1: lane-map = <1 0>; controller 2: lane-map = <0 1>; here bifurcation is set if a controller does not aquire all lanes.Afaik rk3568 cannot select specific lanes so i end up with bifurcation = true/false (an aggregation-mode on phy) again. but it makes dts-property more usable for other devices/SoC. this contains the maximum of lanes and as mask the lanes to take by the current controller. It is scalable to support more pcie-lanes (x2 x4 x8) example for 2 controllers with PCIe x4 (with 8 lanes available): lane-map=<0 0 0 0 1 1 1 1>; lane-map=<1 1 1 1 0 0 0 0>; of course they can be mixed, if driver supports this. lane-map=<0 1 0 1 0 1 0 1>; lane-map=<1 0 1 0 1 0 1 0>; such lane-map is more flexible regards Frank
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c index 863374604fb1..1b0c2115b32e 100644 --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -20,6 +20,7 @@ #include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/reset.h> +#include <linux/phy/pcie.h> #include "pcie-designware.h" @@ -59,6 +60,7 @@ struct rockchip_pcie { struct regulator *vpcie3v3; struct irq_domain *irq_domain; raw_spinlock_t irq_lock; + bool bifurcation; }; static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, @@ -273,6 +275,12 @@ static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) return dev_err_probe(dev, PTR_ERR(rockchip->phy), "missing PHY\n"); + if (rockchip->bifurcation) { + ret = phy_set_mode_ext(rockchip->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_BIFURCATION); + if (ret) + return ret; + } + ret = phy_init(rockchip->phy); if (ret < 0) return ret; @@ -345,6 +353,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev) } } + if (device_property_read_bool(dev, "rockchip,bifurcation")) + rockchip->bifurcation = true; + ret = rockchip_pcie_phy_init(rockchip); if (ret) goto disable_regulator;