Re: [PATCH v8 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Wed, Dec 19, 2018 at 07:13:47PM +0800, Hanjie Lin wrote:
>
>
> On 2018/12/19 6:47, Bjorn Helgaas wrote:
> > On Tue, Dec 18, 2018 at 04:04:46PM +0800, Hanjie Lin wrote:
> >> From: Yue Wang
> >>
> >> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
> >> PCI core. This patch adds the driver support for Meson PCIe controller.
> >
> > I don't have any comments on the code itself; just the trivial things
> > below. No need to repost for these unless you're changing something
> > else.
> >
> > I thought it looked very pretty overall, thanks for paying attention
> > to that!
> >
> >> +static int meson_size_to_payload(struct meson_pcie *mp, int size)
> >> +{
> >> + struct device *dev = mp->pci.dev;
> >> +
> >> + /*
> >> + * dwc supports 2^(val+7) payload size, which val is 0~5 default to 1.
> >> + * So if input size is not 2^order alignment or less than 2^7 or bigger
> >> + * than 2^12, just set to default size 2^(1+7).
> >> + */
> >> + if (!is_power_of_2(size) || size < 128 || size > 4096) {
> >> + dev_warn(dev, "playload size %d, set to default 256\n", size);
> >
> > s/playload/payload/
> >
> >> +static void meson_set_max_payload(struct meson_pcie *mp, int size)
> >> +{
> >> + u32 val = 0;
> >
> > Unnecessary initialization.
> >
> >> + int max_payload_size = meson_size_to_payload(mp, size);
> >> +
> >> + val = meson_elb_readl(mp, PCIE_DEV_CTRL_DEV_STUS);
> >
> >> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int
> >> size,
> >> +u32 *val)
> >> +{
> >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >> + int ret;
> >> +
> >> + ret = dw_pcie_read(pci->dbi_base + where, size, val);
> >> + if (ret != PCIBIOS_SUCCESSFUL)
> >> + return ret;
> >> +
> >> + /*
> >> + * There is a bug in the MESON AXG pcie controller whereby software
> >> + * cannot programme the PCI_CLASS_DEVICE register, so we must fabricate
> >> + * the return value in the config accessors.
> >
> > s/pcie/PCIe/
> > s/programme/program/ (IIUC, "programme" is British and only used as a
> > noun, where here you need a verb)
> >
> >> +static int meson_pcie_link_up(struct dw_pcie *pci)
> >> +{
> >> + struct meson_pcie *mp = to_meson_pcie(pci);
> >> + struct device *dev = pci->dev;
> >> + u32 smlh_up = 0;
> >> + u32 ltssm_up = 0;
> >> + u32 rdlh_up = 0;
> >
> > Unnecessary initialization of smlh_up, ltssm_up, and rdlh_up.
> >
> >> + u32 speed_okay = 0;
> >> + u32 cnt = 0;
> >> + u32 state12, state17;
> >> +
> >> + do {
> >> + state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
> >> + state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
> >> + smlh_up = IS_SMLH_LINK_UP(state12);
> >> + rdlh_up = IS_RDLH_LINK_UP(state12);
> >> + ltssm_up = IS_LTSSM_UP(state12);
> >
> >> + dev_err(dev, "Error: Wait linkup timeout.\n");
> >
> > Message doesn't match others from driver (capitalization and trailing
> > period).
> >
> >> + dev_err(dev, "failed to get msi irq\n");
> >
> > s/msi irq/MSI IRQ/
> >
> >> + ret = meson_add_pcie_port(mp, pdev);
> >> + if (ret < 0) {
> >> + dev_err(dev, "Add PCIE port failed, %d\n", ret);
> >
> > s/PCIE/PCIe/
> >
> > All the messages in this function are capitalized differently than
> > other messages in the driver.
> >
> > Bjorn
> >
> > .
> >
>
> hi Bjorn:
>
> Thanks for the all suggestions and corrections.
>
> There were too many code details unnecessary initialization, typing errors
> and coding style etc. I will pay more attention to these rules and
> code-details
> in the future patches also my daily work.
I have merged your patches with Bjorn's suggestions in my pci/amlogic
branch, aiming at v4.21, please have a look.
Lorenzo
Re: [PATCH v8 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On 2018/12/19 6:47, Bjorn Helgaas wrote:
> On Tue, Dec 18, 2018 at 04:04:46PM +0800, Hanjie Lin wrote:
>> From: Yue Wang
>>
>> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
>> PCI core. This patch adds the driver support for Meson PCIe controller.
>
> I don't have any comments on the code itself; just the trivial things
> below. No need to repost for these unless you're changing something
> else.
>
> I thought it looked very pretty overall, thanks for paying attention
> to that!
>
>> +static int meson_size_to_payload(struct meson_pcie *mp, int size)
>> +{
>> +struct device *dev = mp->pci.dev;
>> +
>> +/*
>> + * dwc supports 2^(val+7) payload size, which val is 0~5 default to 1.
>> + * So if input size is not 2^order alignment or less than 2^7 or bigger
>> + * than 2^12, just set to default size 2^(1+7).
>> + */
>> +if (!is_power_of_2(size) || size < 128 || size > 4096) {
>> +dev_warn(dev, "playload size %d, set to default 256\n", size);
>
> s/playload/payload/
>
>> +static void meson_set_max_payload(struct meson_pcie *mp, int size)
>> +{
>> +u32 val = 0;
>
> Unnecessary initialization.
>
>> +int max_payload_size = meson_size_to_payload(mp, size);
>> +
>> +val = meson_elb_readl(mp, PCIE_DEV_CTRL_DEV_STUS);
>
>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>> + u32 *val)
>> +{
>> +struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +int ret;
>> +
>> +ret = dw_pcie_read(pci->dbi_base + where, size, val);
>> +if (ret != PCIBIOS_SUCCESSFUL)
>> +return ret;
>> +
>> +/*
>> + * There is a bug in the MESON AXG pcie controller whereby software
>> + * cannot programme the PCI_CLASS_DEVICE register, so we must fabricate
>> + * the return value in the config accessors.
>
> s/pcie/PCIe/
> s/programme/program/ (IIUC, "programme" is British and only used as a
> noun, where here you need a verb)
>
>> +static int meson_pcie_link_up(struct dw_pcie *pci)
>> +{
>> +struct meson_pcie *mp = to_meson_pcie(pci);
>> +struct device *dev = pci->dev;
>> +u32 smlh_up = 0;
>> +u32 ltssm_up = 0;
>> +u32 rdlh_up = 0;
>
> Unnecessary initialization of smlh_up, ltssm_up, and rdlh_up.
>
>> +u32 speed_okay = 0;
>> +u32 cnt = 0;
>> +u32 state12, state17;
>> +
>> +do {
>> +state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
>> +state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
>> +smlh_up = IS_SMLH_LINK_UP(state12);
>> +rdlh_up = IS_RDLH_LINK_UP(state12);
>> +ltssm_up = IS_LTSSM_UP(state12);
>
>> +dev_err(dev, "Error: Wait linkup timeout.\n");
>
> Message doesn't match others from driver (capitalization and trailing
> period).
>
>> +dev_err(dev, "failed to get msi irq\n");
>
> s/msi irq/MSI IRQ/
>
>> +ret = meson_add_pcie_port(mp, pdev);
>> +if (ret < 0) {
>> +dev_err(dev, "Add PCIE port failed, %d\n", ret);
>
> s/PCIE/PCIe/
>
> All the messages in this function are capitalized differently than
> other messages in the driver.
>
> Bjorn
>
> .
>
hi Bjorn:
Thanks for the all suggestions and corrections.
There were too many code details unnecessary initialization, typing errors
and coding style etc. I will pay more attention to these rules and code-details
in the future patches also my daily work.
thanks again
hanjie.
Re: [PATCH v8 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
On Tue, Dec 18, 2018 at 04:04:46PM +0800, Hanjie Lin wrote:
> From: Yue Wang
>
> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
> PCI core. This patch adds the driver support for Meson PCIe controller.
I don't have any comments on the code itself; just the trivial things
below. No need to repost for these unless you're changing something
else.
I thought it looked very pretty overall, thanks for paying attention
to that!
> +static int meson_size_to_payload(struct meson_pcie *mp, int size)
> +{
> + struct device *dev = mp->pci.dev;
> +
> + /*
> + * dwc supports 2^(val+7) payload size, which val is 0~5 default to 1.
> + * So if input size is not 2^order alignment or less than 2^7 or bigger
> + * than 2^12, just set to default size 2^(1+7).
> + */
> + if (!is_power_of_2(size) || size < 128 || size > 4096) {
> + dev_warn(dev, "playload size %d, set to default 256\n", size);
s/playload/payload/
> +static void meson_set_max_payload(struct meson_pcie *mp, int size)
> +{
> + u32 val = 0;
Unnecessary initialization.
> + int max_payload_size = meson_size_to_payload(mp, size);
> +
> + val = meson_elb_readl(mp, PCIE_DEV_CTRL_DEV_STUS);
> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> + u32 *val)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + int ret;
> +
> + ret = dw_pcie_read(pci->dbi_base + where, size, val);
> + if (ret != PCIBIOS_SUCCESSFUL)
> + return ret;
> +
> + /*
> + * There is a bug in the MESON AXG pcie controller whereby software
> + * cannot programme the PCI_CLASS_DEVICE register, so we must fabricate
> + * the return value in the config accessors.
s/pcie/PCIe/
s/programme/program/ (IIUC, "programme" is British and only used as a
noun, where here you need a verb)
> +static int meson_pcie_link_up(struct dw_pcie *pci)
> +{
> + struct meson_pcie *mp = to_meson_pcie(pci);
> + struct device *dev = pci->dev;
> + u32 smlh_up = 0;
> + u32 ltssm_up = 0;
> + u32 rdlh_up = 0;
Unnecessary initialization of smlh_up, ltssm_up, and rdlh_up.
> + u32 speed_okay = 0;
> + u32 cnt = 0;
> + u32 state12, state17;
> +
> + do {
> + state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
> + state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
> + smlh_up = IS_SMLH_LINK_UP(state12);
> + rdlh_up = IS_RDLH_LINK_UP(state12);
> + ltssm_up = IS_LTSSM_UP(state12);
> + dev_err(dev, "Error: Wait linkup timeout.\n");
Message doesn't match others from driver (capitalization and trailing
period).
> + dev_err(dev, "failed to get msi irq\n");
s/msi irq/MSI IRQ/
> + ret = meson_add_pcie_port(mp, pdev);
> + if (ret < 0) {
> + dev_err(dev, "Add PCIE port failed, %d\n", ret);
s/PCIE/PCIe/
All the messages in this function are capitalized differently than
other messages in the driver.
Bjorn
[PATCH v8 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
From: Yue Wang The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core. This patch adds the driver support for Meson PCIe controller. Signed-off-by: Yue Wang Signed-off-by: Hanjie Lin --- MAINTAINERS| 7 + drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile| 1 + drivers/pci/controller/dwc/pci-meson.c | 595 + 4 files changed, 613 insertions(+) create mode 100644 drivers/pci/controller/dwc/pci-meson.c diff --git a/MAINTAINERS b/MAINTAINERS index 7fe120f..21ed916 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11600,6 +11600,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/ S: Supported F: drivers/pci/controller/ +PCIE DRIVER FOR AMLOGIC MESON +M: Yue Wang +L: linux-...@vger.kernel.org +L: linux-amlo...@lists.infradead.org +S: Maintained +F: drivers/pci/controller/dwc/pci-meson.c + PCIE DRIVER FOR AXIS ARTPEC M: Jesper Nilsson L: linux-arm-ker...@axis.com diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 91b0194..7800322 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -193,4 +193,14 @@ config PCIE_HISI_STB help Say Y here if you want PCIe controller support on HiSilicon STB SoCs +config PCI_MESON + bool "MESON PCIe controller" + depends on PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + help + Say Y here if you want to enable PCI controller support on Amlogic + SoCs. The PCI controller on Amlogic is based on DesignWare hardware + and therefore the driver re-uses the DesignWare core functions to + implement the driver. + endmenu diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index fcf91ea..e05a015 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o +obj-$(CONFIG_PCI_MESON) += pci-meson.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access. diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c new file mode 100644 index 000..7993f9d --- /dev/null +++ b/drivers/pci/controller/dwc/pci-meson.c @@ -0,0 +1,595 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for Amlogic MESON SoCs + * + * Copyright (c) 2018 Amlogic, inc. + * Author: Yue Wang + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "pcie-designware.h" + +#define to_meson_pcie(x) dev_get_drvdata((x)->dev) + +/* External local bus interface registers */ +#define PLR_OFFSET 0x700 +#define PCIE_PORT_LINK_CTRL_OFF(PLR_OFFSET + 0x10) +#define FAST_LINK_MODE BIT(7) +#define LINK_CAPABLE_MASK GENMASK(21, 16) +#define LINK_CAPABLE_X1BIT(16) + +#define PCIE_GEN2_CTRL_OFF (PLR_OFFSET + 0x10c) +#define NUM_OF_LANES_MASK GENMASK(12, 8) +#define NUM_OF_LANES_X1BIT(8) +#define DIRECT_SPEED_CHANGEBIT(17) + +#define TYPE1_HDR_OFFSET 0x0 +#define PCIE_STATUS_COMMAND(TYPE1_HDR_OFFSET + 0x04) +#define PCI_IO_EN BIT(0) +#define PCI_MEM_SPACE_EN BIT(1) +#define PCI_BUS_MASTER_EN BIT(2) + +#define PCIE_BASE_ADDR0(TYPE1_HDR_OFFSET + 0x10) +#define PCIE_BASE_ADDR1(TYPE1_HDR_OFFSET + 0x14) + +#define PCIE_CAP_OFFSET0x70 +#define PCIE_DEV_CTRL_DEV_STUS (PCIE_CAP_OFFSET + 0x08) +#define PCIE_CAP_MAX_PAYLOAD_MASK GENMASK(7, 5) +#define PCIE_CAP_MAX_PAYLOAD_SIZE(x) ((x) << 5) +#define PCIE_CAP_MAX_READ_REQ_MASK GENMASK(14, 12) +#define PCIE_CAP_MAX_READ_REQ_SIZE(x) ((x) << 12) + +/* PCIe specific config registers */ +#define PCIE_CFG0 0x0 +#define APP_LTSSM_ENABLE BIT(7) + +#define PCIE_CFG_STATUS12 0x30 +#define IS_SMLH_LINK_UP(x) ((x) & (1 << 6)) +#define IS_RDLH_LINK_UP(x) ((x) & (1 << 16)) +#define IS_LTSSM_UP(x) x) >> 10) & 0x1f) == 0x11) + +#define PCIE_CFG_STATUS17 0x44 +#define PM_CURRENT_STATE(x)(((x) >> 7) & 0x1) + +#define WAIT_LINKUP_TIMEOUT4000 +#define PORT_CLK_RATE 1UL +#define MAX_PAYLOAD_SIZE 256 +#define MAX_READ_REQ_SIZE 256 +#define MESON_PCIE_PHY_POWERUP 0x1c +#define PCIE_RESET_DELAY 500
[PATCH v8 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
From: Yue Wang The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core. This patch adds the driver support for Meson PCIe controller. Signed-off-by: Yue Wang Signed-off-by: Hanjie Lin --- MAINTAINERS| 7 + drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile| 1 + drivers/pci/controller/dwc/pci-meson.c | 595 + 4 files changed, 613 insertions(+) create mode 100644 drivers/pci/controller/dwc/pci-meson.c diff --git a/MAINTAINERS b/MAINTAINERS index 7fe120f..21ed916 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11600,6 +11600,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/ S: Supported F: drivers/pci/controller/ +PCIE DRIVER FOR AMLOGIC MESON +M: Yue Wang +L: linux-...@vger.kernel.org +L: linux-amlo...@lists.infradead.org +S: Maintained +F: drivers/pci/controller/dwc/pci-meson.c + PCIE DRIVER FOR AXIS ARTPEC M: Jesper Nilsson L: linux-arm-ker...@axis.com diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 91b0194..7800322 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -193,4 +193,14 @@ config PCIE_HISI_STB help Say Y here if you want PCIe controller support on HiSilicon STB SoCs +config PCI_MESON + bool "MESON PCIe controller" + depends on PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + help + Say Y here if you want to enable PCI controller support on Amlogic + SoCs. The PCI controller on Amlogic is based on DesignWare hardware + and therefore the driver re-uses the DesignWare core functions to + implement the driver. + endmenu diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index fcf91ea..e05a015 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o +obj-$(CONFIG_PCI_MESON) += pci-meson.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access. diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c new file mode 100644 index 000..7993f9d --- /dev/null +++ b/drivers/pci/controller/dwc/pci-meson.c @@ -0,0 +1,595 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for Amlogic MESON SoCs + * + * Copyright (c) 2018 Amlogic, inc. + * Author: Yue Wang + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "pcie-designware.h" + +#define to_meson_pcie(x) dev_get_drvdata((x)->dev) + +/* External local bus interface registers */ +#define PLR_OFFSET 0x700 +#define PCIE_PORT_LINK_CTRL_OFF(PLR_OFFSET + 0x10) +#define FAST_LINK_MODE BIT(7) +#define LINK_CAPABLE_MASK GENMASK(21, 16) +#define LINK_CAPABLE_X1BIT(16) + +#define PCIE_GEN2_CTRL_OFF (PLR_OFFSET + 0x10c) +#define NUM_OF_LANES_MASK GENMASK(12, 8) +#define NUM_OF_LANES_X1BIT(8) +#define DIRECT_SPEED_CHANGEBIT(17) + +#define TYPE1_HDR_OFFSET 0x0 +#define PCIE_STATUS_COMMAND(TYPE1_HDR_OFFSET + 0x04) +#define PCI_IO_EN BIT(0) +#define PCI_MEM_SPACE_EN BIT(1) +#define PCI_BUS_MASTER_EN BIT(2) + +#define PCIE_BASE_ADDR0(TYPE1_HDR_OFFSET + 0x10) +#define PCIE_BASE_ADDR1(TYPE1_HDR_OFFSET + 0x14) + +#define PCIE_CAP_OFFSET0x70 +#define PCIE_DEV_CTRL_DEV_STUS (PCIE_CAP_OFFSET + 0x08) +#define PCIE_CAP_MAX_PAYLOAD_MASK GENMASK(7, 5) +#define PCIE_CAP_MAX_PAYLOAD_SIZE(x) ((x) << 5) +#define PCIE_CAP_MAX_READ_REQ_MASK GENMASK(14, 12) +#define PCIE_CAP_MAX_READ_REQ_SIZE(x) ((x) << 12) + +/* PCIe specific config registers */ +#define PCIE_CFG0 0x0 +#define APP_LTSSM_ENABLE BIT(7) + +#define PCIE_CFG_STATUS12 0x30 +#define IS_SMLH_LINK_UP(x) ((x) & (1 << 6)) +#define IS_RDLH_LINK_UP(x) ((x) & (1 << 16)) +#define IS_LTSSM_UP(x) x) >> 10) & 0x1f) == 0x11) + +#define PCIE_CFG_STATUS17 0x44 +#define PM_CURRENT_STATE(x)(((x) >> 7) & 0x1) + +#define WAIT_LINKUP_TIMEOUT4000 +#define PORT_CLK_RATE 1UL +#define MAX_PAYLOAD_SIZE 256 +#define MAX_READ_REQ_SIZE 256 +#define MESON_PCIE_PHY_POWERUP 0x1c +#define PCIE_RESET_DELAY 500
