Message ID | 1454935264-6076-2-git-send-email-gabriele.paoloni@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Monday 08 February 2016 12:41:02 Gabriele Paoloni wrote: > + > +/* HipXX PCIe host only supports 32-bit config access */ > +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, > + u32 *val) > +{ > + u32 reg; > + u32 reg_val; > + void *walker = ®_val; > + > + walker += (where & 0x3); > + reg = where & ~0x3; > + reg_val = readl(reg_base + reg); > + > + if (size == 1) > + *val = *(u8 __force *) walker; > + else if (size == 2) > + *val = *(u16 __force *) walker; > + else if (size == 4) > + *val = reg_val; > + else > + return PCIBIOS_BAD_REGISTER_NUMBER; > + > + return PCIBIOS_SUCCESSFUL; > +} Isn't this the same hack that Qualcomm are using? Arnd
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 08 February 2016 13:50 > To: linux-arm-kernel@lists.infradead.org > Cc: Gabriele Paoloni; Guohanjun (Hanjun Guo); Wangzhou (B); liudongdong > (C); Linuxarm; qiujiang; bhelgaas@google.com; > Lorenzo.Pieralisi@arm.com; tn@semihalf.com; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org; xuwei (O); linux-acpi@vger.kernel.org; > jcm@redhat.com; zhangjukuo; Liguozhu (Kenneth) > Subject: Re: [RFC PATCH v2 1/3] PCI: hisi: re-architect Hip05/Hip06 > controllers driver to preapare for ACPI > > On Monday 08 February 2016 12:41:02 Gabriele Paoloni wrote: > > + > > +/* HipXX PCIe host only supports 32-bit config access */ > > +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int > size, > > + u32 *val) > > +{ > > + u32 reg; > > + u32 reg_val; > > + void *walker = ®_val; > > + > > + walker += (where & 0x3); > > + reg = where & ~0x3; > > + reg_val = readl(reg_base + reg); > > + > > + if (size == 1) > > + *val = *(u8 __force *) walker; > > + else if (size == 2) > > + *val = *(u16 __force *) walker; > > + else if (size == 4) > > + *val = reg_val; > > + else > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > + > > + return PCIBIOS_SUCCESSFUL; > > +} > > Isn't this the same hack that Qualcomm are using? As far as I can see Qualcomm defines its own config access mechanism only for RC config read and also it seems they're having problems with reporting the device class... https://github.com/torvalds/linux/blob/master/drivers/pci/host/pcie-qcom.c#L474 Our problem is that our HW can only perform 32b rd/wr accesses So we can't use readw/readb/writew/writeb... Thanks Gab > > Arnd
On Monday 08 February 2016 16:06:54 Gabriele Paoloni wrote: > > > > On Monday 08 February 2016 12:41:02 Gabriele Paoloni wrote: > > > + > > > +/* HipXX PCIe host only supports 32-bit config access */ > > > +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int > > size, > > > + u32 *val) > > > +{ > > > + u32 reg; > > > + u32 reg_val; > > > + void *walker = ®_val; > > > + > > > + walker += (where & 0x3); > > > + reg = where & ~0x3; > > > + reg_val = readl(reg_base + reg); > > > + > > > + if (size == 1) > > > + *val = *(u8 __force *) walker; > > > + else if (size == 2) > > > + *val = *(u16 __force *) walker; > > > + else if (size == 4) > > > + *val = reg_val; > > > + else > > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > > + > > > + return PCIBIOS_SUCCESSFUL; > > > +} > > > > Isn't this the same hack that Qualcomm are using? > > As far as I can see Qualcomm defines its own config access > mechanism only for RC config read and also it seems they're > having problems with reporting the device class... > > https://github.com/torvalds/linux/blob/master/drivers/pci/host/pcie-qcom.c#L474 > > Our problem is that our HW can only perform 32b rd/wr accesses > So we can't use readw/readb/writew/writeb... > > Sorry, my mistake, I meant Cavium not Qualcomm. See https://lkml.org/lkml/2016/2/5/689 for the patches. Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 08 February 2016 16:33 > To: Gabriele Paoloni > Cc: linux-arm-kernel@lists.infradead.org; Guohanjun (Hanjun Guo); > Wangzhou (B); liudongdong (C); Linuxarm; qiujiang; bhelgaas@google.com; > Lorenzo.Pieralisi@arm.com; tn@semihalf.com; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org; xuwei (O); linux-acpi@vger.kernel.org; > jcm@redhat.com; zhangjukuo; Liguozhu (Kenneth) > Subject: Re: [RFC PATCH v2 1/3] PCI: hisi: re-architect Hip05/Hip06 > controllers driver to preapare for ACPI > > On Monday 08 February 2016 16:06:54 Gabriele Paoloni wrote: > > > > > > On Monday 08 February 2016 12:41:02 Gabriele Paoloni wrote: > > > > + > > > > +/* HipXX PCIe host only supports 32-bit config access */ > > > > +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, > int > > > size, > > > > + u32 *val) > > > > +{ > > > > + u32 reg; > > > > + u32 reg_val; > > > > + void *walker = ®_val; > > > > + > > > > + walker += (where & 0x3); > > > > + reg = where & ~0x3; > > > > + reg_val = readl(reg_base + reg); > > > > + > > > > + if (size == 1) > > > > + *val = *(u8 __force *) walker; > > > > + else if (size == 2) > > > > + *val = *(u16 __force *) walker; > > > > + else if (size == 4) > > > > + *val = reg_val; > > > > + else > > > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > > > + > > > > + return PCIBIOS_SUCCESSFUL; > > > > +} > > > > > > Isn't this the same hack that Qualcomm are using? > > > > As far as I can see Qualcomm defines its own config access > > mechanism only for RC config read and also it seems they're > > having problems with reporting the device class... > > > > https://github.com/torvalds/linux/blob/master/drivers/pci/host/pcie- > qcom.c#L474 > > > > Our problem is that our HW can only perform 32b rd/wr accesses > > So we can't use readw/readb/writew/writeb... > > > > > > Sorry, my mistake, I meant Cavium not Qualcomm. > See https://lkml.org/lkml/2016/2/5/689 for the patches. Well, looking at it Cavium seems quite different, On read they need to trigger the retrieval of the config space info writing to the lower 32b of a 64b register, then they need to read data back on the upper 64b of the same register and adjust the content to remove the garbage... We just use 32b accesses and adjust grab the appropriate bytes depending on the read/write sizes... Thanks Gab > > Arnd
On Monday 08 February 2016 16:51:19 Gabriele Paoloni wrote: > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > On Monday 08 February 2016 16:06:54 Gabriele Paoloni wrote: > > > > On Monday 08 February 2016 12:41:02 Gabriele Paoloni wrote: > > int > > > > size, > > > > > + u32 *val) > > > > > +{ > > > > > + u32 reg; > > > > > + u32 reg_val; > > > > > + void *walker = ®_val; > > > > > + > > > > > + walker += (where & 0x3); > > > > > + reg = where & ~0x3; > > > > > + reg_val = readl(reg_base + reg); > > > > > + > > > > > + if (size == 1) > > > > > + *val = *(u8 __force *) walker; > > > > > + else if (size == 2) > > > > > + *val = *(u16 __force *) walker; > > > > > + else if (size == 4) > > > > > + *val = reg_val; > > > > > + else > > > > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > > > > + > > > > > + return PCIBIOS_SUCCESSFUL; > > > > > +} > > > > > > > > Isn't this the same hack that Qualcomm are using? > > > > > > As far as I can see Qualcomm defines its own config access > > > mechanism only for RC config read and also it seems they're > > > having problems with reporting the device class... > > > > > > https://github.com/torvalds/linux/blob/master/drivers/pci/host/pcie- > > qcom.c#L474 > > > > > > Our problem is that our HW can only perform 32b rd/wr accesses > > > So we can't use readw/readb/writew/writeb... > > > > > > > > > > Sorry, my mistake, I meant Cavium not Qualcomm. > > See https://lkml.org/lkml/2016/2/5/689 for the patches. > > Well, looking at it Cavium seems quite different, > > On read they need to trigger the retrieval of the > config space info writing to the lower 32b of a 64b register, > then they need to read data back on the upper 64b of the > same register and adjust the content to remove the garbage... > > We just use 32b accesses and adjust grab the appropriate > bytes depending on the read/write sizes... Hmm, I must have misremembered that too then, let me try once more ;-) The above appears to reimplement pci_generic_config_read32(). Can you just use that instead? Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 09 February 2016 16:27 > To: Gabriele Paoloni > Cc: linux-arm-kernel@lists.infradead.org; Guohanjun (Hanjun Guo); > Wangzhou (B); liudongdong (C); Linuxarm; qiujiang; bhelgaas@google.com; > Lorenzo.Pieralisi@arm.com; tn@semihalf.com; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org; xuwei (O); linux-acpi@vger.kernel.org; > jcm@redhat.com; zhangjukuo; Liguozhu (Kenneth) > Subject: Re: [RFC PATCH v2 1/3] PCI: hisi: re-architect Hip05/Hip06 > controllers driver to preapare for ACPI > > On Monday 08 February 2016 16:51:19 Gabriele Paoloni wrote: > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > On Monday 08 February 2016 16:06:54 Gabriele Paoloni wrote: > > > > > On Monday 08 February 2016 12:41:02 Gabriele Paoloni wrote: > > > int > > > > > size, > > > > > > + u32 *val) > > > > > > +{ > > > > > > + u32 reg; > > > > > > + u32 reg_val; > > > > > > + void *walker = ®_val; > > > > > > + > > > > > > + walker += (where & 0x3); > > > > > > + reg = where & ~0x3; > > > > > > + reg_val = readl(reg_base + reg); > > > > > > + > > > > > > + if (size == 1) > > > > > > + *val = *(u8 __force *) walker; > > > > > > + else if (size == 2) > > > > > > + *val = *(u16 __force *) walker; > > > > > > + else if (size == 4) > > > > > > + *val = reg_val; > > > > > > + else > > > > > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > > > > > + > > > > > > + return PCIBIOS_SUCCESSFUL; > > > > > > +} > > > > > > > > > > Isn't this the same hack that Qualcomm are using? > > > > > > > > As far as I can see Qualcomm defines its own config access > > > > mechanism only for RC config read and also it seems they're > > > > having problems with reporting the device class... > > > > > > > > > https://github.com/torvalds/linux/blob/master/drivers/pci/host/pcie- > > > qcom.c#L474 > > > > > > > > Our problem is that our HW can only perform 32b rd/wr accesses > > > > So we can't use readw/readb/writew/writeb... > > > > > > > > > > > > > > Sorry, my mistake, I meant Cavium not Qualcomm. > > > See https://lkml.org/lkml/2016/2/5/689 for the patches. > > > > Well, looking at it Cavium seems quite different, > > > > On read they need to trigger the retrieval of the > > config space info writing to the lower 32b of a 64b register, > > then they need to read data back on the upper 64b of the > > same register and adjust the content to remove the garbage... > > > > We just use 32b accesses and adjust grab the appropriate > > bytes depending on the read/write sizes... > > Hmm, I must have misremembered that too then, let me try once more ;-) > > The above appears to reimplement pci_generic_config_read32(). Can you > just use that instead? Nope I don't think so, When we read the root complex config space we need to use a configuration address space that is different from the one used to map the rest of the hierarchy; I think this is something to do with Designware itself. It is clear if you look at http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L657 Here you can see that in calling "dw_pcie_wr_own_conf" Designware does not pass "bus" and "devfn" that are actually required by pci_generic_config_read32() to map the addr... Many Thanks Gab > > Arnd
diff --git a/MAINTAINERS b/MAINTAINERS index 30aca4a..d69f436 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8409,7 +8409,9 @@ M: Gabriele Paoloni <gabriele.paoloni@huawei.com> L: linux-pci@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/pci/hisilicon-pcie.txt +F: drivers/pci/host/pcie-hisi.h F: drivers/pci/host/pcie-hisi.c +F: drivers/pci/host/pcie-hisi-common.c PCIE DRIVER FOR QUALCOMM MSM M: Stanimir Varbanov <svarbanov@mm-sol.com> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index 7b2f20c..8c93c0f 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -20,5 +20,5 @@ obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o -obj-$(CONFIG_PCI_HISI) += pcie-hisi.o +obj-$(CONFIG_PCI_HISI) += pcie-hisi.o pcie-hisi-common.o obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o diff --git a/drivers/pci/host/pcie-hisi-common.c b/drivers/pci/host/pcie-hisi-common.c new file mode 100644 index 0000000..6dfb4c3 --- /dev/null +++ b/drivers/pci/host/pcie-hisi-common.c @@ -0,0 +1,73 @@ +/* + * PCIe host controller common functions for HiSilicon SoCs + * + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com + * + * Author: Zhou Wang <wangzhou1@hisilicon.com> + * Dacai Zhu <zhudacai@hisilicon.com> + * Gabriele Paoloni <gabriele.paoloni@huawei.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/acpi.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/mfd/syscon.h> +#include <linux/of_pci.h> +#include <linux/regmap.h> + +#include "pcie-designware.h" +#include "pcie-hisi.h" + +/* HipXX PCIe host only supports 32-bit config access */ +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, + u32 *val) +{ + u32 reg; + u32 reg_val; + void *walker = ®_val; + + walker += (where & 0x3); + reg = where & ~0x3; + reg_val = readl(reg_base + reg); + + if (size == 1) + *val = *(u8 __force *) walker; + else if (size == 2) + *val = *(u16 __force *) walker; + else if (size == 4) + *val = reg_val; + else + return PCIBIOS_BAD_REGISTER_NUMBER; + + return PCIBIOS_SUCCESSFUL; +} + +/* HipXX PCIe host only supports 32-bit config access */ +int hisi_pcie_common_cfg_write(void __iomem *reg_base, int where, int size, + u32 val) +{ + u32 reg_val; + u32 reg; + void *walker = ®_val; + + walker += (where & 0x3); + reg = where & ~0x3; + if (size == 4) + writel(val, reg_base + reg); + else if (size == 2) { + reg_val = readl(reg_base + reg); + *(u16 __force *) walker = val; + writel(val, reg_base + reg); + } else if (size == 1) { + reg_val = readl(reg_base + reg); + *(u8 __force *) walker = val; + writel(val, reg_base + reg); + } else + return PCIBIOS_BAD_REGISTER_NUMBER; + + return PCIBIOS_SUCCESSFUL; +} diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c index 3e98d4e..458d0f8 100644 --- a/drivers/pci/host/pcie-hisi.c +++ b/drivers/pci/host/pcie-hisi.c @@ -21,6 +21,7 @@ #include <linux/regmap.h> #include "pcie-designware.h" +#include "pcie-hisi.h" #define PCIE_LTSSM_LINKUP_STATE 0x11 #define PCIE_LTSSM_STATE_MASK 0x3F @@ -30,12 +31,6 @@ #define to_hisi_pcie(x) container_of(x, struct hisi_pcie, pp) -struct hisi_pcie; - -struct pcie_soc_ops { - int (*hisi_pcie_link_up)(struct hisi_pcie *pcie); -}; - struct hisi_pcie { struct regmap *subctrl; void __iomem *reg_base; @@ -44,87 +39,24 @@ struct hisi_pcie { struct pcie_soc_ops *soc_ops; }; -static inline void hisi_pcie_apb_writel(struct hisi_pcie *pcie, - u32 val, u32 reg) -{ - writel(val, pcie->reg_base + reg); -} - -static inline u32 hisi_pcie_apb_readl(struct hisi_pcie *pcie, u32 reg) -{ - return readl(pcie->reg_base + reg); -} - -/* HipXX PCIe host only supports 32-bit config access */ -static int hisi_pcie_cfg_read(struct pcie_port *pp, int where, int size, - u32 *val) -{ - u32 reg; - u32 reg_val; - struct hisi_pcie *pcie = to_hisi_pcie(pp); - void *walker = ®_val; - - walker += (where & 0x3); - reg = where & ~0x3; - reg_val = hisi_pcie_apb_readl(pcie, reg); - - if (size == 1) - *val = *(u8 __force *) walker; - else if (size == 2) - *val = *(u16 __force *) walker; - else if (size == 4) - *val = reg_val; - else - return PCIBIOS_BAD_REGISTER_NUMBER; - - return PCIBIOS_SUCCESSFUL; -} +struct pcie_soc_ops { + int (*hisi_pcie_link_up)(struct hisi_pcie *pcie); +}; -/* HipXX PCIe host only supports 32-bit config access */ -static int hisi_pcie_cfg_write(struct pcie_port *pp, int where, int size, - u32 val) +static inline int hisi_pcie_cfg_read(struct pcie_port *pp, int where, + int size, u32 *val) { - u32 reg_val; - u32 reg; struct hisi_pcie *pcie = to_hisi_pcie(pp); - void *walker = ®_val; - - walker += (where & 0x3); - reg = where & ~0x3; - if (size == 4) - hisi_pcie_apb_writel(pcie, val, reg); - else if (size == 2) { - reg_val = hisi_pcie_apb_readl(pcie, reg); - *(u16 __force *) walker = val; - hisi_pcie_apb_writel(pcie, reg_val, reg); - } else if (size == 1) { - reg_val = hisi_pcie_apb_readl(pcie, reg); - *(u8 __force *) walker = val; - hisi_pcie_apb_writel(pcie, reg_val, reg); - } else - return PCIBIOS_BAD_REGISTER_NUMBER; - - return PCIBIOS_SUCCESSFUL; -} -static int hisi_pcie_link_up_hip05(struct hisi_pcie *hisi_pcie) -{ - u32 val; - - regmap_read(hisi_pcie->subctrl, PCIE_SUBCTRL_SYS_STATE4_REG + - 0x100 * hisi_pcie->port_id, &val); - - return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE); + return hisi_pcie_common_cfg_read(pcie->reg_base, where, size, val); } -static int hisi_pcie_link_up_hip06(struct hisi_pcie *hisi_pcie) +static inline int hisi_pcie_cfg_write(struct pcie_port *pp, int where, + int size, u32 val) { - u32 val; - - val = hisi_pcie_apb_readl(hisi_pcie, PCIE_HIP06_CTRL_OFF + - PCIE_SYS_STATE4); + struct hisi_pcie *pcie = to_hisi_pcie(pp); - return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE); + return hisi_pcie_common_cfg_write(pcie->reg_base, where, size, val); } static int hisi_pcie_link_up(struct pcie_port *pp) @@ -134,12 +66,13 @@ static int hisi_pcie_link_up(struct pcie_port *pp) return hisi_pcie->soc_ops->hisi_pcie_link_up(hisi_pcie); } -static struct pcie_host_ops hisi_pcie_host_ops = { - .rd_own_conf = hisi_pcie_cfg_read, - .wr_own_conf = hisi_pcie_cfg_write, - .link_up = hisi_pcie_link_up, +struct pcie_host_ops hisi_pcie_host_ops = { + .rd_own_conf = hisi_pcie_cfg_read, + .wr_own_conf = hisi_pcie_cfg_write, + .link_up = hisi_pcie_link_up, }; + static int hisi_add_pcie_port(struct pcie_port *pp, struct platform_device *pdev) { @@ -215,6 +148,26 @@ static int hisi_pcie_probe(struct platform_device *pdev) return 0; } +static int hisi_pcie_link_up_hip05(struct hisi_pcie *hisi_pcie) +{ + u32 val; + + regmap_read(hisi_pcie->subctrl, PCIE_SUBCTRL_SYS_STATE4_REG + + 0x100 * hisi_pcie->port_id, &val); + + return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE); +} + +static int hisi_pcie_link_up_hip06(struct hisi_pcie *hisi_pcie) +{ + u32 val; + + val = readl(hisi_pcie->reg_base + PCIE_HIP06_CTRL_OFF + + PCIE_SYS_STATE4); + + return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE); +} + static struct pcie_soc_ops hip05_ops = { &hisi_pcie_link_up_hip05 }; diff --git a/drivers/pci/host/pcie-hisi.h b/drivers/pci/host/pcie-hisi.h new file mode 100644 index 0000000..29e0790 --- /dev/null +++ b/drivers/pci/host/pcie-hisi.h @@ -0,0 +1,23 @@ +/* + * PCIe host controller driver for HiSilicon SoCs + * + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com + * + * Author: Zhou Wang <wangzhou1@hisilicon.com> + * Dacai Zhu <zhudacai@hisilicon.com> + * Gabriele Paoloni <gabriele.paoloni@huawei.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#ifndef PCIE_HISI_H_ +#define PCIE_HISI_H_ + + +int hisi_pcie_common_cfg_read(void __iomem *reg_base, int where, int size, + u32 *val); +int hisi_pcie_common_cfg_write(void __iomem *reg_base, int where, int size, + u32 val); + +#endif /* PCIE_HISI_H_ */