Re: [PATCH v4 3/5] PCI: hisi: Add PCIe host support for Hisilicon Soc Hip05

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2015/7/22 6:37, Bjorn Helgaas wrote:
> Hi Zhou,
> 
> On Tue, Jul 21, 2015 at 02:48:41PM +0800, Zhou Wang wrote:
>> This patch adds PCIe host support for Hisilicon Soc Hip05.
>>
>> Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
>> ---
>>  drivers/pci/host/Kconfig     |   5 +
>>  drivers/pci/host/Makefile    |   1 +
>>  drivers/pci/host/pcie-hisi.c | 257 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 263 insertions(+)
>>  create mode 100644 drivers/pci/host/pcie-hisi.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index c132bdd..37f2075 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -145,4 +145,9 @@ config PCIE_IPROC_BCMA
>>  	  Say Y here if you want to use the Broadcom iProc PCIe controller
>>  	  through the BCMA bus interface
>>  
>> +config PCI_HISI
>> +	depends on OF && ARM64
>> +	bool "Hisilicon Soc HIP05 PCIe controller"
>> +	select PCIE_DW
> 
> Almost all the other DesignWare-based drivers select PCIEPORTBUS in
> addition to PCIE_DW.  I'm not sure that's the best Kconfiggery (maybe we
> should leave this up to the user), but it'd be nice if all the drivers at
> least did this consistently.  So do you have a reason for doing it
> differently?
>

Hi Bjorn,

I made a mistake there. PCIEPORTBUS should be selected as other DesignWare-based
drivers did.

>>  endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 140d66f..ea1dbf2 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
>> +obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
>> diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
>> new file mode 100644
>> index 0000000..9f2fac1
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-hisi.c
>> @@ -0,0 +1,257 @@
>> +/*
>> + * PCIe host controller driver for Hisilicon Hip05 SoCs
>> + *
>> + * Copyright (C) 2015 Hisilicon Co., Ltd. http://www.hisilicon.com
>> + *
>> + * Author: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
>> + *         Dacai Zhu <zhudacai@xxxxxxxxxxxxx>
>> + *
>> + * 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/interrupt.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define PCIE_SUBCTRL_MODE_REG                           0x2800
>> +#define PCIE_SUBCTRL_SYS_STATE4_REG                     0x6818
>> +#define PCIE_SLV_DBI_MODE                               0x0
>> +#define PCIE_SLV_SYSCTRL_MODE                           0x1
>> +#define PCIE_SLV_CONTENT_MODE                           0x2
>> +#define PCIE_SLV_MSI_ASID                               0x10
>> +#define PCIE_LTSSM_LINKUP_STATE                         0x11
>> +#define PCIE_LTSSM_STATE_MASK                           0x3F
>> +#define PCIE_MSI_ASID_ENABLE                            (0x1 << 12)
>> +#define PCIE_MSI_ASID_VALUE                             (0x1 << 16)
>> +#define PCIE_MSI_TRANS_ENABLE                           (0x1 << 12)
>> +#define PCIE_MSI_TRANS_REG                              0x1c8
>> +#define PCIE_MSI_LOW_ADDRESS                            0x1b4
>> +#define PCIE_MSI_HIGH_ADDRESS                           0x1c4
>> +#define PCIE_MSI_ADDRESS_VAL                            0xb7010040
>> +
>> +#define to_hisi_pcie(x)	container_of(x, struct hisi_pcie, pp)
>> +
>> +struct hisi_pcie {
>> +	void __iomem *subctrl_base;
>> +	void __iomem *reg_base;
>> +	struct msi_controller *msi;
>> +	u32 port_id;
>> +	struct pcie_port pp;
>> +};
>> +
>> +static inline void hisi_pcie_subctrl_writel(struct hisi_pcie *pcie,
>> +					    u32 val, u32 reg)
>> +{
>> +	writel(val, pcie->subctrl_base + reg);
>> +}
>> +
>> +static inline u32 hisi_pcie_subctrl_readl(struct hisi_pcie *pcie, u32 reg)
>> +{
>> +	return readl(pcie->subctrl_base + reg);
>> +}
>> +
>> +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);
>> +}
>> +
>> +/*
>> + * Change mode to indicate the same reg_base to base of PCIe host configure
>> + * registers, base of RC configure space or base of vmid/asid context table
>> + */
>> +static void hisi_pcie_change_apb_mode(struct hisi_pcie *pcie, u32 mode)
>> +{
>> +	u32 val;
>> +	u32 bit_mask;
>> +	u32 bit_shift;
>> +	u32 port_id = pcie->port_id;
>> +	u32 reg = PCIE_SUBCTRL_MODE_REG + 0x100 * port_id;
>> +
>> +	if ((port_id == 1) || (port_id == 2)) {
>> +		bit_mask = 0xc;
>> +		bit_shift = 0x2;
>> +	} else {
>> +		bit_mask = 0x6;
>> +		bit_shift = 0x1;
>> +	}
>> +
>> +	val = hisi_pcie_subctrl_readl(pcie, reg);
>> +	val = (val & (~bit_mask)) | (mode << bit_shift);
>> +	hisi_pcie_subctrl_writel(pcie, val, reg);
>> +}
>> +
>> +/* Configure vmid/asid table in PCIe host */
>> +static void hisi_pcie_config_context(struct hisi_pcie *pcie)
>> +{
>> +	int i;
>> +
>> +	hisi_pcie_change_apb_mode(pcie, PCIE_SLV_CONTENT_MODE);
>> +
>> +	/*
>> +	 * init vmid and asid tables for all PCIes device as 0
> 
> s/PCIes device as/PCIe devices to/ ?
>

Thanks, will modify this.

>> +	 * vmid table: 0 ~ 0x3ff, asid table: 0x400 ~ 0x7ff
>> +	 */
>> +	for (i = 0; i < 0x800; i++)
>> +		hisi_pcie_apb_writel(pcie, 0x0, i * 4);
>> +
>> +	hisi_pcie_change_apb_mode(pcie, PCIE_SLV_SYSCTRL_MODE);
>> +
>> +	hisi_pcie_apb_writel(pcie, PCIE_MSI_ADDRESS_VAL, PCIE_MSI_LOW_ADDRESS);
>> +	hisi_pcie_apb_writel(pcie, 0x0, PCIE_MSI_HIGH_ADDRESS);
>> +	hisi_pcie_apb_writel(pcie, PCIE_MSI_ASID_ENABLE | PCIE_MSI_ASID_VALUE,
>> +			     PCIE_SLV_MSI_ASID);
>> +	hisi_pcie_apb_writel(pcie, PCIE_MSI_TRANS_ENABLE, PCIE_MSI_TRANS_REG);
>> +
>> +	hisi_pcie_change_apb_mode(pcie, PCIE_SLV_DBI_MODE);
>> +}
>> +
>> +static int hisi_pcie_link_up(struct pcie_port *pp)
>> +{
>> +	u32 val;
>> +
> 
> Remove this extra blank line.
> 

Yes, will remove it. Thanks.

>> +	struct hisi_pcie *hisi_pcie = to_hisi_pcie(pp);
>> +
>> +	val = hisi_pcie_subctrl_readl(hisi_pcie, PCIE_SUBCTRL_SYS_STATE4_REG +
>> +				      0x100 * hisi_pcie->port_id);
>> +
>> +	return ((val & PCIE_LTSSM_STATE_MASK) == PCIE_LTSSM_LINKUP_STATE);
>> +}
>> +
>> +static
>> +int hisi_pcie_msi_host_init(struct pcie_port *pp, struct msi_controller *chip)
>> +{
>> +	struct device_node *msi_node;
>> +	struct irq_domain *irq_domain;
>> +	struct device_node *np = pp->dev->of_node;
>> +
>> +	msi_node = of_parse_phandle(np, "msi-parent", 0);
>> +	if (!msi_node) {
>> +		pr_err("failed to find msi-parent\n");
> 
> Can you use dev_err() here (and below)?  A message like this that doesn't
> have a hint about what device it's related to is not as useful as it could
> be.
>

Right, will use dev_err(). Thanks.

>> +		return -ENODEV;
>> +	}
>> +
>> +	irq_domain = irq_find_host(msi_node);
>> +	if (!irq_domain) {
>> +		pr_err("failed to find irq domain\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	pp->irq_domain = irq_domain;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct pcie_host_ops hisi_pcie_host_ops = {
>> +	.link_up = hisi_pcie_link_up,
>> +	.msi_host_init = hisi_pcie_msi_host_init,
>> +};
>> +
>> +static int __init hisi_add_pcie_port(struct pcie_port *pp,
>> +				     struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	u32 port_id;
>> +	struct resource busn;
>> +
> 
> Remove this extra blank line.

Will remove it. Thanks.

> 
>> +	struct hisi_pcie *hisi_pcie = to_hisi_pcie(pp);
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "port-id", &port_id)) {
>> +		dev_err(&pdev->dev, "failed to read port-id\n");
>> +		return -EINVAL;
>> +	}
>> +	if (port_id > 3) {
>> +		dev_err(&pdev->dev, "Invalid port-id\n");
> 
> Since you have the invalid port_id here, maybe you could include the
> invalid value in the output?
>

Right, will do like this. Thanks.

>> +		return -EINVAL;
>> +	}
>> +
>> +	hisi_pcie->port_id = port_id;
>> +
>> +	if (of_pci_parse_bus_range(pdev->dev.of_node, &busn)) {
>> +		dev_err(&pdev->dev, "failed to parse bus-ranges\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pp->root_bus_nr = busn.start;
>> +	pp->ops = &hisi_pcie_host_ops;
>> +
>> +	hisi_pcie_config_context(hisi_pcie);
>> +
>> +	ret = dw_pcie_host_init(pp);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to initialize host\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int __init hisi_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct hisi_pcie *hisi_pcie;
>> +	struct pcie_port *pp;
>> +	struct resource *reg;
>> +	struct resource *subctrl;
>> +	int ret;
>> +
>> +	hisi_pcie = devm_kzalloc(&pdev->dev, sizeof(*hisi_pcie), GFP_KERNEL);
>> +	if (!hisi_pcie)
>> +		return -ENOMEM;
>> +
>> +	pp = &hisi_pcie->pp;
>> +	pp->dev = &pdev->dev;
>> +
>> +	subctrl = platform_get_resource_byname(pdev, IORESOURCE_MEM, "subctrl");
>> +	hisi_pcie->subctrl_base = devm_ioremap_nocache(&pdev->dev,
>> +					subctrl->start, resource_size(subctrl));
>> +	if (IS_ERR(hisi_pcie->subctrl_base)) {
>> +		dev_err(pp->dev, "cannot get subctrl base\n");
>> +		return PTR_ERR(hisi_pcie->subctrl_base);
>> +	}
>> +
>> +	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbi");
>> +	hisi_pcie->reg_base = devm_ioremap_resource(&pdev->dev, reg);
>> +	if (IS_ERR(hisi_pcie->reg_base)) {
>> +		dev_err(pp->dev, "cannot get reg base\n");
> 
> Maybe this could include the actual resource name ("rc_dbi") as the message
> above does?
>

Will modify this message. Thanks.

>> +		return PTR_ERR(hisi_pcie->reg_base);
>> +	}
>> +
>> +	hisi_pcie->pp.dbi_base = hisi_pcie->reg_base;
>> +
>> +	ret = hisi_add_pcie_port(pp, pdev);
>> +	if (ret < 0)
> 
> Seems like this return value check should be the same as the one above in 
> hisi_add_pcie_port().  In both cases, the only possible returns are zero
> (success) or a negative error number.  Using "if (ret)" here has the
> advantage that below you could "return 0" and it would be crystal clear
> that you're always returning success if you get that far.
> 

Got it, will do like this. Thanks for your suggestion.

>> +		return ret;
>> +
>> +	platform_set_drvdata(pdev, hisi_pcie);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id hisi_pcie_of_match[] = {
>> +	{.compatible = "hisilicon,hip05-pcie",},
>> +	{},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, hisi_pcie_of_match);
>> +
>> +static struct platform_driver hisi_pcie_driver = {
>> +	.probe  = hisi_pcie_probe,
>> +	.driver = {
>> +		   .name = "hisi-pcie",
>> +		   .owner = THIS_MODULE,
> 
> You can drop this ".owner =" initialization.  platform_driver_register()
> supplies that automatically.
> 

Will drop it, Thanks.

Best regards,
Zhou

>> +		   .of_match_table = hisi_pcie_of_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(hisi_pcie_driver);
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux