| Message ID | bb391a0e0f0863b66e645048315fab1a4f63f277.1634812676.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Add support for Hikey 970 PCIe | expand |
On Thu, Oct 21, 2021 at 11:45:11AM +0100, Mauro Carvalho Chehab wrote: > On HiKey970, there's a PEX 8606 PCI bridge on its PHY with > 6 lanes. Only 4 lanes are connected: > > lane 0 - connected to Kirin 970; > lane 4 - M.2 slot; > lane 5 - mini PCIe slot; > lane 6 - in-board Ethernet controller. > > Each lane has its own PERST# gpio pin, and needs a clock > request. > > Add support to parse a DT schema containing the above data. > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > Acked-by: Xiaowei Song <songxiaowei@hisilicon.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > > To mailbombing on a large number of people, only mailing lists were C/C on the cover. > See [PATCH v15 00/13] at: https://lore.kernel.org/all/cover.1634812676.git.mchehab+huawei@kernel.org/ > > drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++--- > 1 file changed, 231 insertions(+), 31 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c > index 86c13661e02d..de375795a3b8 100644 > --- a/drivers/pci/controller/dwc/pcie-kirin.c > +++ b/drivers/pci/controller/dwc/pcie-kirin.c > @@ -52,6 +52,19 @@ > #define PCIE_DEBOUNCE_PARAM 0xF0F400 > #define PCIE_OE_BYPASS (0x3 << 28) > > +/* > + * Max number of connected PCI slots at an external PCI bridge > + * > + * This is used on HiKey 970, which has a PEX 8606 bridge with has > + * 4 connected lanes (lane 0 upstream, and the other tree lanes, > + * one connected to an in-board Ethernet adapter and the other two > + * connected to M.2 and mini PCI slots. > + * > + * Each slot has a different clock source and uses a separate PERST# > + * pin. > + */ > +#define MAX_PCI_SLOTS 3 > + > enum pcie_kirin_phy_type { > PCIE_KIRIN_INTERNAL_PHY, > PCIE_KIRIN_EXTERNAL_PHY > @@ -64,6 +77,19 @@ struct kirin_pcie { > struct regmap *apb; > struct phy *phy; > void *phy_priv; /* only for PCIE_KIRIN_INTERNAL_PHY */ > + > + /* DWC PERST# */ > + int gpio_id_dwc_perst; > + > + /* Per-slot PERST# */ > + int num_slots; > + int gpio_id_reset[MAX_PCI_SLOTS]; > + const char *reset_names[MAX_PCI_SLOTS]; > + > + /* Per-slot clkreq */ > + int n_gpio_clkreq; > + int gpio_id_clkreq[MAX_PCI_SLOTS]; > + const char *clkreq_names[MAX_PCI_SLOTS]; I think there's been previous discussion about this, but I didn't follow it, so I'm just double-checking that this is what we want here. IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an external PEX 8606 bridge, which seems a little strange to be hard-coded into the kirin driver this way. I see that "hisilicon,clken-gpios" is optional, but what if some platform connects all 6 lanes? What if there's a different bridge altogether? I'll assume this is actually the way we want thing unless I hear otherwise. > }; > > /* > @@ -108,7 +134,6 @@ struct hi3660_pcie_phy { > struct clk *phy_ref_clk; > struct clk *aclk; > struct clk *aux_clk; > - int gpio_id_reset; > }; > > /* Registers in PCIePHY */ > @@ -171,16 +196,6 @@ static int hi3660_pcie_phy_get_resource(struct hi3660_pcie_phy *phy) > if (IS_ERR(phy->sysctrl)) > return PTR_ERR(phy->sysctrl); > > - /* gpios */ > - phy->gpio_id_reset = of_get_named_gpio(dev->of_node, > - "reset-gpios", 0); > - if (phy->gpio_id_reset == -EPROBE_DEFER) { > - return -EPROBE_DEFER; > - } else if (!gpio_is_valid(phy->gpio_id_reset)) { > - dev_err(phy->dev, "unable to get a valid gpio pin\n"); > - return -ENODEV; > - } > - > return 0; > } > > @@ -297,15 +312,7 @@ static int hi3660_pcie_phy_power_on(struct kirin_pcie *pcie) > if (ret) > goto disable_clks; > > - /* perst assert Endpoint */ > - if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) { > - usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX); > - ret = gpio_direction_output(phy->gpio_id_reset, 1); > - if (ret) > - goto disable_clks; > - usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX); > - return 0; > - } > + return 0; > > disable_clks: > hi3660_pcie_phy_clk_ctrl(phy, false); > @@ -347,11 +354,98 @@ static const struct regmap_config pcie_kirin_regmap_conf = { > .reg_stride = 4, > }; > > +static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie, > + struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + char name[32]; > + int ret, i; > + > + /* This is an optional property */ > + ret = of_gpio_named_count(np, "hisilicon,clken-gpios"); > + if (ret < 0) > + return 0; > + > + if (ret > MAX_PCI_SLOTS) { > + dev_err(dev, "Too many GPIO clock requests!\n"); > + return -EINVAL; > + } > + > + pcie->n_gpio_clkreq = ret; > + > + for (i = 0; i < pcie->n_gpio_clkreq; i++) { > + pcie->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node, > + "hisilicon,clken-gpios", i); > + if (pcie->gpio_id_clkreq[i] < 0) > + return pcie->gpio_id_clkreq[i]; > + > + sprintf(name, "pcie_clkreq_%d", i); > + pcie->clkreq_names[i] = devm_kstrdup_const(dev, name, > + GFP_KERNEL); > + if (!pcie->clkreq_names[i]) > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > + struct platform_device *pdev, > + struct device_node *node) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *parent, *child; > + int ret, slot, i; > + char name[32]; > + > + for_each_available_child_of_node(node, parent) { > + for_each_available_child_of_node(parent, child) { > + i = pcie->num_slots; > + > + pcie->gpio_id_reset[i] = of_get_named_gpio(child, > + "reset-gpios", 0); > + if (pcie->gpio_id_reset[i] < 0) > + continue; > + > + pcie->num_slots++; > + if (pcie->num_slots > MAX_PCI_SLOTS) { > + dev_err(dev, "Too many PCI slots!\n"); > + return -EINVAL; > + } > + > + ret = of_pci_get_devfn(child); > + if (ret < 0) { > + dev_err(dev, "failed to parse devfn: %d\n", ret); > + goto put_node; > + } > + > + slot = PCI_SLOT(ret); > + > + sprintf(name, "pcie_perst_%d", slot); > + pcie->reset_names[i] = devm_kstrdup_const(dev, name, > + GFP_KERNEL); > + if (!pcie->reset_names[i]) { > + ret = -ENOMEM; > + goto put_node; > + } > + } > + } > + > + return 0; > + > +put_node: > + of_node_put(child); > + return ret; > +} > + > static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct device_node *child, *node = dev->of_node; > void __iomem *apb_base; > + int ret; > > apb_base = devm_platform_ioremap_resource_byname(pdev, "apb"); > if (IS_ERR(apb_base)) > @@ -362,7 +456,32 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > if (IS_ERR(kirin_pcie->apb)) > return PTR_ERR(kirin_pcie->apb); > > + /* pcie internal PERST# gpio */ > + kirin_pcie->gpio_id_dwc_perst = of_get_named_gpio(dev->of_node, > + "reset-gpios", 0); > + if (kirin_pcie->gpio_id_dwc_perst == -EPROBE_DEFER) { > + return -EPROBE_DEFER; > + } else if (!gpio_is_valid(kirin_pcie->gpio_id_dwc_perst)) { > + dev_err(dev, "unable to get a valid gpio pin\n"); > + return -ENODEV; > + } > + > + ret = kirin_pcie_get_gpio_enable(kirin_pcie, pdev); > + if (ret) > + return ret; > + > + /* Parse OF children */ > + for_each_available_child_of_node(node, child) { > + ret = kirin_pcie_parse_port(kirin_pcie, pdev, child); > + if (ret) > + goto put_node; > + } > + > return 0; > + > +put_node: > + of_node_put(child); > + return ret; > } > > static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie, > @@ -419,9 +538,33 @@ static int kirin_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn, > return PCIBIOS_SUCCESSFUL; > } > > +static int kirin_pcie_add_bus(struct pci_bus *bus) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata); > + struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci); > + int i, ret; > + > + if (!kirin_pcie->num_slots) > + return 0; > + > + /* Send PERST# to each slot */ > + for (i = 0; i < kirin_pcie->num_slots; i++) { > + ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1); > + if (ret) { > + dev_err(pci->dev, "PERST# %s error: %d\n", > + kirin_pcie->reset_names[i], ret); > + } > + } > + usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX); > + > + return 0; > +} > + > + > static struct pci_ops kirin_pci_ops = { > .read = kirin_pcie_rd_own_conf, > .write = kirin_pcie_wr_own_conf, > + .add_bus = kirin_pcie_add_bus, > }; > > static u32 kirin_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, > @@ -477,6 +620,44 @@ static int kirin_pcie_host_init(struct pcie_port *pp) > return 0; > } > > +static int kirin_pcie_gpio_request(struct kirin_pcie *kirin_pcie, > + struct device *dev) > +{ > + int ret, i; > + > + for (i = 0; i < kirin_pcie->num_slots; i++) { > + if (!gpio_is_valid(kirin_pcie->gpio_id_reset[i])) { > + dev_err(dev, "unable to get a valid %s gpio\n", > + kirin_pcie->reset_names[i]); > + return -ENODEV; > + } > + > + ret = devm_gpio_request(dev, kirin_pcie->gpio_id_reset[i], > + kirin_pcie->reset_names[i]); > + if (ret) > + return ret; > + } > + > + for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) { > + if (!gpio_is_valid(kirin_pcie->gpio_id_clkreq[i])) { > + dev_err(dev, "unable to get a valid %s gpio\n", > + kirin_pcie->clkreq_names[i]); > + return -ENODEV; > + } > + > + ret = devm_gpio_request(dev, kirin_pcie->gpio_id_clkreq[i], > + kirin_pcie->clkreq_names[i]); > + if (ret) > + return ret; > + > + ret = gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 0); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > static const struct dw_pcie_ops kirin_dw_pcie_ops = { > .read_dbi = kirin_pcie_read_dbi, > .write_dbi = kirin_pcie_write_dbi, > @@ -499,24 +680,43 @@ static int kirin_pcie_power_on(struct platform_device *pdev, > if (ret) > return ret; > > - return hi3660_pcie_phy_power_on(kirin_pcie); > + ret = hi3660_pcie_phy_power_on(kirin_pcie); > + if (ret) > + return ret; > + } else { > + kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL); > + if (IS_ERR(kirin_pcie->phy)) > + return PTR_ERR(kirin_pcie->phy); > + > + ret = kirin_pcie_gpio_request(kirin_pcie, dev); > + if (ret) > + return ret; > + > + ret = phy_init(kirin_pcie->phy); > + if (ret) > + goto err; > + > + ret = phy_power_on(kirin_pcie->phy); > + if (ret) > + goto err; > } > > - kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL); > - if (IS_ERR(kirin_pcie->phy)) > - return PTR_ERR(kirin_pcie->phy); > + /* perst assert Endpoint */ > + usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX); > > - ret = phy_init(kirin_pcie->phy); > - if (ret) > - goto err; > + if (!gpio_request(kirin_pcie->gpio_id_dwc_perst, "pcie_perst_bridge")) { > + ret = gpio_direction_output(kirin_pcie->gpio_id_dwc_perst, 1); > + if (ret) > + goto err; > + } > > - ret = phy_power_on(kirin_pcie->phy); > - if (ret) > - goto err; > + usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX); > > return 0; > err: > - phy_exit(kirin_pcie->phy); > + if (kirin_pcie->type != PCIE_KIRIN_INTERNAL_PHY) > + phy_exit(kirin_pcie->phy); > + > return ret; > } > > -- > 2.31.1 >
Hi Bjorn, Em Tue, 2 Nov 2021 11:06:12 -0500 Bjorn Helgaas <helgaas@kernel.org> escreveu: > > + > > + /* Per-slot clkreq */ > > + int n_gpio_clkreq; > > + int gpio_id_clkreq[MAX_PCI_SLOTS]; > > + const char *clkreq_names[MAX_PCI_SLOTS]; > > I think there's been previous discussion about this, but I didn't > follow it, so I'm just double-checking that this is what we want here. > > IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an > external PEX 8606 bridge, which seems a little strange to be > hard-coded into the kirin driver this way. > > I see that "hisilicon,clken-gpios" is optional, but what if some > platform connects all 6 lanes? What if there's a different bridge > altogether? > > I'll assume this is actually the way we want thing unless I hear > otherwise. Yes, there was past discussions about that with Rob, with regards to how the DT would represent it, which got reflected at the code. At the end, it was decided to just add a single property inside PCIe: pcie@f4000000 { compatible = "hisilicon,kirin970-pcie"; ... hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>, <&gpio20 6 0>; I don't think this is a problem, as, if some day another bridge would need a larger number of slots, it is just a matter of changing the number at the MAX_PCI_SLOTS, as this controls only the size of the array (and the check for array overflow when parsing the properties). Regards, Mauro
On Tuesday 02 November 2021 17:44:15 Mauro Carvalho Chehab wrote: > Hi Bjorn, > > Em Tue, 2 Nov 2021 11:06:12 -0500 > Bjorn Helgaas <helgaas@kernel.org> escreveu: > > > > + > > > + /* Per-slot clkreq */ > > > + int n_gpio_clkreq; > > > + int gpio_id_clkreq[MAX_PCI_SLOTS]; > > > + const char *clkreq_names[MAX_PCI_SLOTS]; > > > > I think there's been previous discussion about this, but I didn't > > follow it, so I'm just double-checking that this is what we want here. > > > > IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an > > external PEX 8606 bridge, which seems a little strange to be > > hard-coded into the kirin driver this way. > > > > I see that "hisilicon,clken-gpios" is optional, but what if some > > platform connects all 6 lanes? What if there's a different bridge > > altogether? > > > > I'll assume this is actually the way we want thing unless I hear > > otherwise. I proposed alternative approach how to define it: https://lore.kernel.org/linux-pci/20211023144252.z7ou2l2tvm6cvtf7@pali/ Reason for a my new proposal is that currently there is lot of duplicated code in every native pcie controller driver and every driver is solving same issues by ad-doc code which is not related to host bridge / controller itself. Like configuration of devices (e.g. PCIe switch) to the host bridge itself. That is why I send also another RFC: https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/ I would be happy if we can discuss on them for future drivers. At least if my proposals make sense or completely not. > Yes, there was past discussions about that with Rob, with regards > to how the DT would represent it, which got reflected at the code. > At the end, it was decided to just add a single property inside PCIe: > > > pcie@f4000000 { > compatible = "hisilicon,kirin970-pcie"; > ... > hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>, > <&gpio20 6 0>; > > I don't think this is a problem, as, if some day another bridge would > need a larger number of slots, it is just a matter of changing the > number at the MAX_PCI_SLOTS, as this controls only the size of the array > (and the check for array overflow when parsing the properties). It is not a problem for this particular pcie controller. And really MAX macro can be increased in this driver if there is need for a larger number of slots. It should work fine. But if there are going to be added more boards with different hw topology or even with different pcie controllers then there would be new issues. E.g. how to figure out which gpio belongs to which slot? Or even hot-plugging support? Port belongs to either Root Port device or to Downstream device, which does not have to be at root level. It creates tree topology and therefore it is not possible to represent GPIOs in simple list, like it is for kirin DTS. Generally you cannot say to which slot belongs second GPIO defined in reset-gpios list. That is why I'm saying it needs some better structure and prepare pci core code for it. So native pci controller drivers do not have to invent ad-hoc solutions for specific board setups. I really think that information about PCIe switch topology should be in DTS and it should work independently of PCIe controller driver, without need for board-specific or PCIe-switch-specific ad-hoc hooks in host bridge drivers, like it is currently. Bjorn, what do you think?
On Tue, Nov 2, 2021 at 12:44 PM Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Hi Bjorn, > > Em Tue, 2 Nov 2021 11:06:12 -0500 > Bjorn Helgaas <helgaas@kernel.org> escreveu: > > > > + > > > + /* Per-slot clkreq */ > > > + int n_gpio_clkreq; > > > + int gpio_id_clkreq[MAX_PCI_SLOTS]; > > > + const char *clkreq_names[MAX_PCI_SLOTS]; > > > > I think there's been previous discussion about this, but I didn't > > follow it, so I'm just double-checking that this is what we want here. > > > > IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an > > external PEX 8606 bridge, which seems a little strange to be > > hard-coded into the kirin driver this way. > > > > I see that "hisilicon,clken-gpios" is optional, but what if some > > platform connects all 6 lanes? What if there's a different bridge > > altogether? Keep in mind this is HiKey. There's never been a 2nd board upstream for the SoCs, the boards have a short lifespan in terms of both support and availability. And yet Hikey manages to do multiple complicated things on the board design. I have a hard time caring... > > > > I'll assume this is actually the way we want thing unless I hear > > otherwise. > > Yes, there was past discussions about that with Rob, with regards > to how the DT would represent it, which got reflected at the code. At first I thought these were CLKREQ connections which absolutely should be per port/bridge like PERST, but they are not. They are simply enables for the refclk's and pretty specific to HiKey. > At the end, it was decided to just add a single property inside PCIe: > > > pcie@f4000000 { > compatible = "hisilicon,kirin970-pcie"; > ... > hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>, > <&gpio20 6 0>; > > I don't think this is a problem, as, if some day another bridge would > need a larger number of slots, it is just a matter of changing the > number at the MAX_PCI_SLOTS, as this controls only the size of the array > (and the check for array overflow when parsing the properties). It is a problem that your host bridge driver has hardcoded board specifics. That's not the first time you've heard that from me. But given the board-to-soc ratio of Hikey is 1:1, I don't care that much. Rob
Em Thu, 4 Nov 2021 10:36:52 -0500 Rob Herring <robh@kernel.org> escreveu: > On Tue, Nov 2, 2021 at 12:44 PM Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> wrote: > > > > Hi Bjorn, > > > > Em Tue, 2 Nov 2021 11:06:12 -0500 > > Bjorn Helgaas <helgaas@kernel.org> escreveu: > > > > > > + > > > > + /* Per-slot clkreq */ > > > > + int n_gpio_clkreq; > > > > + int gpio_id_clkreq[MAX_PCI_SLOTS]; > > > > + const char *clkreq_names[MAX_PCI_SLOTS]; > > > > > > I think there's been previous discussion about this, but I didn't > > > follow it, so I'm just double-checking that this is what we want here. > > > > > > IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an > > > external PEX 8606 bridge, which seems a little strange to be > > > hard-coded into the kirin driver this way. > > > > > > I see that "hisilicon,clken-gpios" is optional, but what if some > > > platform connects all 6 lanes? What if there's a different bridge > > > altogether? > > Keep in mind this is HiKey. There's never been a 2nd board upstream > for the SoCs, the boards have a short lifespan in terms of both > support and availability. And yet Hikey manages to do multiple > complicated things on the board design. I have a hard time caring... > > > > > > > I'll assume this is actually the way we want thing unless I hear > > > otherwise. > > > > Yes, there was past discussions about that with Rob, with regards > > to how the DT would represent it, which got reflected at the code. > > At first I thought these were CLKREQ connections which absolutely > should be per port/bridge like PERST, but they are not. They are > simply enables for the refclk's and pretty specific to HiKey. > > > At the end, it was decided to just add a single property inside PCIe: > > > > > > pcie@f4000000 { > > compatible = "hisilicon,kirin970-pcie"; > > ... > > hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>, > > <&gpio20 6 0>; > > > > I don't think this is a problem, as, if some day another bridge would > > need a larger number of slots, it is just a matter of changing the > > number at the MAX_PCI_SLOTS, as this controls only the size of the array > > (and the check for array overflow when parsing the properties). > > It is a problem that your host bridge driver has hardcoded board > specifics. That's not the first time you've heard that from me. But > given the board-to-soc ratio of Hikey is 1:1, I don't care that much. Ok, understood. Yeah, it sounds unlikely that we would get more boards for Kirin 960/970 with PCI support, as this SoC is used for mobile phones, where neither USB nor PCI bridges are needed. So, AFAIKT, the only the only publicly-available boards that will be using this driver are HiKey 960 and HiKey 970. Regards, Mauro
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c index 86c13661e02d..de375795a3b8 100644 --- a/drivers/pci/controller/dwc/pcie-kirin.c +++ b/drivers/pci/controller/dwc/pcie-kirin.c @@ -52,6 +52,19 @@ #define PCIE_DEBOUNCE_PARAM 0xF0F400 #define PCIE_OE_BYPASS (0x3 << 28) +/* + * Max number of connected PCI slots at an external PCI bridge + * + * This is used on HiKey 970, which has a PEX 8606 bridge with has + * 4 connected lanes (lane 0 upstream, and the other tree lanes, + * one connected to an in-board Ethernet adapter and the other two + * connected to M.2 and mini PCI slots. + * + * Each slot has a different clock source and uses a separate PERST# + * pin. + */ +#define MAX_PCI_SLOTS 3 + enum pcie_kirin_phy_type { PCIE_KIRIN_INTERNAL_PHY, PCIE_KIRIN_EXTERNAL_PHY @@ -64,6 +77,19 @@ struct kirin_pcie { struct regmap *apb; struct phy *phy; void *phy_priv; /* only for PCIE_KIRIN_INTERNAL_PHY */ + + /* DWC PERST# */ + int gpio_id_dwc_perst; + + /* Per-slot PERST# */ + int num_slots; + int gpio_id_reset[MAX_PCI_SLOTS]; + const char *reset_names[MAX_PCI_SLOTS]; + + /* Per-slot clkreq */ + int n_gpio_clkreq; + int gpio_id_clkreq[MAX_PCI_SLOTS]; + const char *clkreq_names[MAX_PCI_SLOTS]; }; /* @@ -108,7 +134,6 @@ struct hi3660_pcie_phy { struct clk *phy_ref_clk; struct clk *aclk; struct clk *aux_clk; - int gpio_id_reset; }; /* Registers in PCIePHY */ @@ -171,16 +196,6 @@ static int hi3660_pcie_phy_get_resource(struct hi3660_pcie_phy *phy) if (IS_ERR(phy->sysctrl)) return PTR_ERR(phy->sysctrl); - /* gpios */ - phy->gpio_id_reset = of_get_named_gpio(dev->of_node, - "reset-gpios", 0); - if (phy->gpio_id_reset == -EPROBE_DEFER) { - return -EPROBE_DEFER; - } else if (!gpio_is_valid(phy->gpio_id_reset)) { - dev_err(phy->dev, "unable to get a valid gpio pin\n"); - return -ENODEV; - } - return 0; } @@ -297,15 +312,7 @@ static int hi3660_pcie_phy_power_on(struct kirin_pcie *pcie) if (ret) goto disable_clks; - /* perst assert Endpoint */ - if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) { - usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX); - ret = gpio_direction_output(phy->gpio_id_reset, 1); - if (ret) - goto disable_clks; - usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX); - return 0; - } + return 0; disable_clks: hi3660_pcie_phy_clk_ctrl(phy, false); @@ -347,11 +354,98 @@ static const struct regmap_config pcie_kirin_regmap_conf = { .reg_stride = 4, }; +static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie, + struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + char name[32]; + int ret, i; + + /* This is an optional property */ + ret = of_gpio_named_count(np, "hisilicon,clken-gpios"); + if (ret < 0) + return 0; + + if (ret > MAX_PCI_SLOTS) { + dev_err(dev, "Too many GPIO clock requests!\n"); + return -EINVAL; + } + + pcie->n_gpio_clkreq = ret; + + for (i = 0; i < pcie->n_gpio_clkreq; i++) { + pcie->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node, + "hisilicon,clken-gpios", i); + if (pcie->gpio_id_clkreq[i] < 0) + return pcie->gpio_id_clkreq[i]; + + sprintf(name, "pcie_clkreq_%d", i); + pcie->clkreq_names[i] = devm_kstrdup_const(dev, name, + GFP_KERNEL); + if (!pcie->clkreq_names[i]) + return -ENOMEM; + } + + return 0; +} + +static int kirin_pcie_parse_port(struct kirin_pcie *pcie, + struct platform_device *pdev, + struct device_node *node) +{ + struct device *dev = &pdev->dev; + struct device_node *parent, *child; + int ret, slot, i; + char name[32]; + + for_each_available_child_of_node(node, parent) { + for_each_available_child_of_node(parent, child) { + i = pcie->num_slots; + + pcie->gpio_id_reset[i] = of_get_named_gpio(child, + "reset-gpios", 0); + if (pcie->gpio_id_reset[i] < 0) + continue; + + pcie->num_slots++; + if (pcie->num_slots > MAX_PCI_SLOTS) { + dev_err(dev, "Too many PCI slots!\n"); + return -EINVAL; + } + + ret = of_pci_get_devfn(child); + if (ret < 0) { + dev_err(dev, "failed to parse devfn: %d\n", ret); + goto put_node; + } + + slot = PCI_SLOT(ret); + + sprintf(name, "pcie_perst_%d", slot); + pcie->reset_names[i] = devm_kstrdup_const(dev, name, + GFP_KERNEL); + if (!pcie->reset_names[i]) { + ret = -ENOMEM; + goto put_node; + } + } + } + + return 0; + +put_node: + of_node_put(child); + return ret; +} + static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct device_node *child, *node = dev->of_node; void __iomem *apb_base; + int ret; apb_base = devm_platform_ioremap_resource_byname(pdev, "apb"); if (IS_ERR(apb_base)) @@ -362,7 +456,32 @@ static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, if (IS_ERR(kirin_pcie->apb)) return PTR_ERR(kirin_pcie->apb); + /* pcie internal PERST# gpio */ + kirin_pcie->gpio_id_dwc_perst = of_get_named_gpio(dev->of_node, + "reset-gpios", 0); + if (kirin_pcie->gpio_id_dwc_perst == -EPROBE_DEFER) { + return -EPROBE_DEFER; + } else if (!gpio_is_valid(kirin_pcie->gpio_id_dwc_perst)) { + dev_err(dev, "unable to get a valid gpio pin\n"); + return -ENODEV; + } + + ret = kirin_pcie_get_gpio_enable(kirin_pcie, pdev); + if (ret) + return ret; + + /* Parse OF children */ + for_each_available_child_of_node(node, child) { + ret = kirin_pcie_parse_port(kirin_pcie, pdev, child); + if (ret) + goto put_node; + } + return 0; + +put_node: + of_node_put(child); + return ret; } static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie, @@ -419,9 +538,33 @@ static int kirin_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn, return PCIBIOS_SUCCESSFUL; } +static int kirin_pcie_add_bus(struct pci_bus *bus) +{ + struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata); + struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci); + int i, ret; + + if (!kirin_pcie->num_slots) + return 0; + + /* Send PERST# to each slot */ + for (i = 0; i < kirin_pcie->num_slots; i++) { + ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1); + if (ret) { + dev_err(pci->dev, "PERST# %s error: %d\n", + kirin_pcie->reset_names[i], ret); + } + } + usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX); + + return 0; +} + + static struct pci_ops kirin_pci_ops = { .read = kirin_pcie_rd_own_conf, .write = kirin_pcie_wr_own_conf, + .add_bus = kirin_pcie_add_bus, }; static u32 kirin_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, @@ -477,6 +620,44 @@ static int kirin_pcie_host_init(struct pcie_port *pp) return 0; } +static int kirin_pcie_gpio_request(struct kirin_pcie *kirin_pcie, + struct device *dev) +{ + int ret, i; + + for (i = 0; i < kirin_pcie->num_slots; i++) { + if (!gpio_is_valid(kirin_pcie->gpio_id_reset[i])) { + dev_err(dev, "unable to get a valid %s gpio\n", + kirin_pcie->reset_names[i]); + return -ENODEV; + } + + ret = devm_gpio_request(dev, kirin_pcie->gpio_id_reset[i], + kirin_pcie->reset_names[i]); + if (ret) + return ret; + } + + for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) { + if (!gpio_is_valid(kirin_pcie->gpio_id_clkreq[i])) { + dev_err(dev, "unable to get a valid %s gpio\n", + kirin_pcie->clkreq_names[i]); + return -ENODEV; + } + + ret = devm_gpio_request(dev, kirin_pcie->gpio_id_clkreq[i], + kirin_pcie->clkreq_names[i]); + if (ret) + return ret; + + ret = gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 0); + if (ret) + return ret; + } + + return 0; +} + static const struct dw_pcie_ops kirin_dw_pcie_ops = { .read_dbi = kirin_pcie_read_dbi, .write_dbi = kirin_pcie_write_dbi, @@ -499,24 +680,43 @@ static int kirin_pcie_power_on(struct platform_device *pdev, if (ret) return ret; - return hi3660_pcie_phy_power_on(kirin_pcie); + ret = hi3660_pcie_phy_power_on(kirin_pcie); + if (ret) + return ret; + } else { + kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL); + if (IS_ERR(kirin_pcie->phy)) + return PTR_ERR(kirin_pcie->phy); + + ret = kirin_pcie_gpio_request(kirin_pcie, dev); + if (ret) + return ret; + + ret = phy_init(kirin_pcie->phy); + if (ret) + goto err; + + ret = phy_power_on(kirin_pcie->phy); + if (ret) + goto err; } - kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL); - if (IS_ERR(kirin_pcie->phy)) - return PTR_ERR(kirin_pcie->phy); + /* perst assert Endpoint */ + usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX); - ret = phy_init(kirin_pcie->phy); - if (ret) - goto err; + if (!gpio_request(kirin_pcie->gpio_id_dwc_perst, "pcie_perst_bridge")) { + ret = gpio_direction_output(kirin_pcie->gpio_id_dwc_perst, 1); + if (ret) + goto err; + } - ret = phy_power_on(kirin_pcie->phy); - if (ret) - goto err; + usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX); return 0; err: - phy_exit(kirin_pcie->phy); + if (kirin_pcie->type != PCIE_KIRIN_INTERNAL_PHY) + phy_exit(kirin_pcie->phy); + return ret; }