| Message ID | 1544058553-10936-3-git-send-email-jianjun.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: mediatek: Add support for MT7629 | expand |
On Thu, 2018-12-06 at 09:09 +0800, Jianjun Wang wrote: > MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > in arm64 but bogus alignment values at arm32, the pcie device and devices :s /the pcie device /the bridge device > behind this bridge will not be enabled. Fix it's BAR0 resource size to > guarantee the pcie devices will be enabled correctly. > > The HW default value of its device id is invalid, fix it's device id to > match the hardware implementation. > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ > include/linux/pci_ids.h | 1 + > 2 files changed, 27 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index d20cf461ba00..f8937cc3c87c 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -73,6 +73,7 @@ > #define PCIE_MSI_VECTOR 0x0c0 > > #define PCIE_CONF_VEND_ID 0x100 > +#define PCIE_CONF_DEVICE_ID 0x102 > #define PCIE_CONF_CLASS_ID 0x106 > > #define PCIE_INT_MASK 0x420 > @@ -135,12 +136,14 @@ struct mtk_pcie_port; > /** > * struct mtk_pcie_soc - differentiate between host generations > * @need_fix_class_id: whether this host's class ID needed to be fixed or not > + * @need_fix_device_id: whether this host's device ID needed to be fixed or not > * @ops: pointer to configuration access functions > * @startup: pointer to controller setting functions > * @setup_irq: pointer to initialize IRQ functions > */ > struct mtk_pcie_soc { > bool need_fix_class_id; > + bool need_fix_device_id; > struct pci_ops *ops; > int (*startup)(struct mtk_pcie_port *port); > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); > @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > writew(val, port->base + PCIE_CONF_CLASS_ID); > } > > + if (soc->need_fix_device_id) { > + val = PCI_DEVICE_ID_MEDIATEK_7629; > + writew(val, port->base + PCIE_CONF_DEVICE_ID); > + } > + > /* 100ms timeout value should be enough for Gen1/2 training */ > err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, > !!(val & PCIE_PORT_LINKUP_V2), 20, > @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > .setup_irq = mtk_pcie_setup_irq, > }; > > +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { > + .need_fix_class_id = true, > + .need_fix_device_id = true, > + .ops = &mtk_pcie_ops_v2, > + .startup = mtk_pcie_startup_port_v2, > + .setup_irq = mtk_pcie_setup_irq, > +}; > + > +static void mtk_fixup_bar_size(struct pci_dev *dev) > +{ > + struct resource *dev_res = &dev->resource[0]; > + /* 32bit resource length will calculate size to 0, set it smaller */ > + dev_res->end = 0xfffffffe; > +} I'm not sure assign the BAR0 size in driver to fit in the bogus alignment is a good idea. Seems the size value of 0xffff_fffe also is an arbitrary value. Do we have a chance to change the resource framework code to make it adopt this scenario? Thanks. > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, > + mtk_fixup_bar_size); > + > static const struct of_device_id mtk_pcie_ids[] = { > { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, > { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, > { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, > { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, > + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, > {}, > }; > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index 69f0abe1ba1a..77b278bac3a8 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2126,6 +2126,7 @@ > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > #define PCI_VENDOR_ID_MEDIATEK 0x14c3 > +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 > > #define PCI_VENDOR_ID_TITAN 0x14D2 > #define PCI_DEVICE_ID_TITAN_010L 0x8001
On Thu, 2018-12-06 at 13:53 +0800, Honghui Zhang wrote: > On Thu, 2018-12-06 at 09:09 +0800, Jianjun Wang wrote: > > MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. > > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > :s /the pcie device /the bridge device > > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > guarantee the pcie devices will be enabled correctly. > > > > The HW default value of its device id is invalid, fix it's device id to > > match the hardware implementation. > > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ > > include/linux/pci_ids.h | 1 + > > 2 files changed, 27 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > index d20cf461ba00..f8937cc3c87c 100644 > > --- a/drivers/pci/controller/pcie-mediatek.c > > +++ b/drivers/pci/controller/pcie-mediatek.c > > @@ -73,6 +73,7 @@ > > #define PCIE_MSI_VECTOR 0x0c0 > > > > #define PCIE_CONF_VEND_ID 0x100 > > +#define PCIE_CONF_DEVICE_ID 0x102 > > #define PCIE_CONF_CLASS_ID 0x106 > > > > #define PCIE_INT_MASK 0x420 > > @@ -135,12 +136,14 @@ struct mtk_pcie_port; > > /** > > * struct mtk_pcie_soc - differentiate between host generations > > * @need_fix_class_id: whether this host's class ID needed to be fixed or not > > + * @need_fix_device_id: whether this host's device ID needed to be fixed or not > > * @ops: pointer to configuration access functions > > * @startup: pointer to controller setting functions > > * @setup_irq: pointer to initialize IRQ functions > > */ > > struct mtk_pcie_soc { > > bool need_fix_class_id; > > + bool need_fix_device_id; > > struct pci_ops *ops; > > int (*startup)(struct mtk_pcie_port *port); > > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); > > @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > > writew(val, port->base + PCIE_CONF_CLASS_ID); > > } > > > > + if (soc->need_fix_device_id) { > > + val = PCI_DEVICE_ID_MEDIATEK_7629; > > + writew(val, port->base + PCIE_CONF_DEVICE_ID); > > + } > > + > > /* 100ms timeout value should be enough for Gen1/2 training */ > > err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, > > !!(val & PCIE_PORT_LINKUP_V2), 20, > > @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > > .setup_irq = mtk_pcie_setup_irq, > > }; > > > > +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { > > + .need_fix_class_id = true, > > + .need_fix_device_id = true, > > + .ops = &mtk_pcie_ops_v2, > > + .startup = mtk_pcie_startup_port_v2, > > + .setup_irq = mtk_pcie_setup_irq, > > +}; > > + > > +static void mtk_fixup_bar_size(struct pci_dev *dev) > > +{ > > + struct resource *dev_res = &dev->resource[0]; > > + /* 32bit resource length will calculate size to 0, set it smaller */ > > + dev_res->end = 0xfffffffe; > > +} > > I'm not sure assign the BAR0 size in driver to fit in the bogus > alignment is a good idea. Seems the size value of 0xffff_fffe also is an > arbitrary value. > Do we have a chance to change the resource framework code to make it > adopt this scenario? > > Thanks. I'm afraid not, the resource size length defined by phys_addr_t, which related to the hardware's physical address length. It will be much more better if the BAR0 size is not related with the pcie to axi window, when we set the window to 4GB, the BAR0 size still have initial value, and we can set the BAR0 size value or just disable it independently. The BAR0 size value need bigger than the MMIO space size, so the software will think it's a invalid resource but not a bogus alignment one, since we can't disable the BAR0 by hardware, and the flow of enable devices will keep going. > > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, > > + mtk_fixup_bar_size); > > + > > static const struct of_device_id mtk_pcie_ids[] = { > > { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, > > { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, > > { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, > > { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, > > + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, > > {}, > > }; > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > index 69f0abe1ba1a..77b278bac3a8 100644 > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -2126,6 +2126,7 @@ > > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > > > #define PCI_VENDOR_ID_MEDIATEK 0x14c3 > > +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 > > > > #define PCI_VENDOR_ID_TITAN 0x14D2 > > #define PCI_DEVICE_ID_TITAN_010L 0x8001 > >
On Fri, 2018-12-07 at 20:56 +0800, Jianjun Wang wrote: > On Thu, 2018-12-06 at 13:53 +0800, Honghui Zhang wrote: > > On Thu, 2018-12-06 at 09:09 +0800, Jianjun Wang wrote: > > > MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > > > :s /the pcie device /the bridge device > > > > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > > guarantee the pcie devices will be enabled correctly. > > > > > > The HW default value of its device id is invalid, fix it's device id to > > > match the hardware implementation. > > > > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > > > --- > > > drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ > > > include/linux/pci_ids.h | 1 + > > > 2 files changed, 27 insertions(+) > > > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > > index d20cf461ba00..f8937cc3c87c 100644 > > > --- a/drivers/pci/controller/pcie-mediatek.c > > > +++ b/drivers/pci/controller/pcie-mediatek.c > > > @@ -73,6 +73,7 @@ > > > #define PCIE_MSI_VECTOR 0x0c0 > > > > > > #define PCIE_CONF_VEND_ID 0x100 > > > +#define PCIE_CONF_DEVICE_ID 0x102 > > > #define PCIE_CONF_CLASS_ID 0x106 > > > > > > #define PCIE_INT_MASK 0x420 > > > @@ -135,12 +136,14 @@ struct mtk_pcie_port; > > > /** > > > * struct mtk_pcie_soc - differentiate between host generations > > > * @need_fix_class_id: whether this host's class ID needed to be fixed or not > > > + * @need_fix_device_id: whether this host's device ID needed to be fixed or not > > > * @ops: pointer to configuration access functions > > > * @startup: pointer to controller setting functions > > > * @setup_irq: pointer to initialize IRQ functions > > > */ > > > struct mtk_pcie_soc { > > > bool need_fix_class_id; > > > + bool need_fix_device_id; > > > struct pci_ops *ops; > > > int (*startup)(struct mtk_pcie_port *port); > > > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); > > > @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > > > writew(val, port->base + PCIE_CONF_CLASS_ID); > > > } > > > > > > + if (soc->need_fix_device_id) { > > > + val = PCI_DEVICE_ID_MEDIATEK_7629; > > > + writew(val, port->base + PCIE_CONF_DEVICE_ID); > > > + } > > > + > > > /* 100ms timeout value should be enough for Gen1/2 training */ > > > err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, > > > !!(val & PCIE_PORT_LINKUP_V2), 20, > > > @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > > > .setup_irq = mtk_pcie_setup_irq, > > > }; > > > > > > +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { > > > + .need_fix_class_id = true, > > > + .need_fix_device_id = true, > > > + .ops = &mtk_pcie_ops_v2, > > > + .startup = mtk_pcie_startup_port_v2, > > > + .setup_irq = mtk_pcie_setup_irq, > > > +}; > > > + > > > +static void mtk_fixup_bar_size(struct pci_dev *dev) > > > +{ > > > + struct resource *dev_res = &dev->resource[0]; > > > + /* 32bit resource length will calculate size to 0, set it smaller */ > > > + dev_res->end = 0xfffffffe; > > > +} > > > > I'm not sure assign the BAR0 size in driver to fit in the bogus > > alignment is a good idea. Seems the size value of 0xffff_fffe also is an > > arbitrary value. > > Do we have a chance to change the resource framework code to make it > > adopt this scenario? I have take a look at the resource assign routine, it's better keep it in current way. Thanks. > > > > Thanks. > > I'm afraid not, the resource size length defined by phys_addr_t, which > related to the hardware's physical address length. > It will be much more better if the BAR0 size is not related with the > pcie to axi window, when we set the window to 4GB, the BAR0 size still > have initial value, and we can set the BAR0 size value or just disable > it independently. > The BAR0 size value need bigger than the MMIO space size, so the > software will think it's a invalid resource but not a bogus alignment > one, since we can't disable the BAR0 by hardware, and the flow of enable > devices will keep going. > > > > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, > > > + mtk_fixup_bar_size); > > > + > > > static const struct of_device_id mtk_pcie_ids[] = { > > > { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, > > > { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, > > > { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, > > > { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, > > > + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, > > > {}, > > > }; > > > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > > index 69f0abe1ba1a..77b278bac3a8 100644 > > > --- a/include/linux/pci_ids.h > > > +++ b/include/linux/pci_ids.h > > > @@ -2126,6 +2126,7 @@ > > > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > > > > > #define PCI_VENDOR_ID_MEDIATEK 0x14c3 > > > +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 > > > > > > #define PCI_VENDOR_ID_TITAN 0x14D2 > > > #define PCI_DEVICE_ID_TITAN_010L 0x8001 > > > > > >
Hi, On Thu, 2018-12-06 at 09:09 +0800, Jianjun Wang wrote: > MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > in arm64 but bogus alignment values at arm32, the pcie device and devices > behind this bridge will not be enabled. Fix it's BAR0 resource size to > guarantee the pcie devices will be enabled correctly. > > The HW default value of its device id is invalid, fix it's device id to > match the hardware implementation. > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ > include/linux/pci_ids.h | 1 + > 2 files changed, 27 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index d20cf461ba00..f8937cc3c87c 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -73,6 +73,7 @@ > #define PCIE_MSI_VECTOR 0x0c0 > > #define PCIE_CONF_VEND_ID 0x100 > +#define PCIE_CONF_DEVICE_ID 0x102 > #define PCIE_CONF_CLASS_ID 0x106 > > #define PCIE_INT_MASK 0x420 > @@ -135,12 +136,14 @@ struct mtk_pcie_port; > /** > * struct mtk_pcie_soc - differentiate between host generations > * @need_fix_class_id: whether this host's class ID needed to be fixed or not > + * @need_fix_device_id: whether this host's device ID needed to be fixed or not > * @ops: pointer to configuration access functions > * @startup: pointer to controller setting functions > * @setup_irq: pointer to initialize IRQ functions > */ > struct mtk_pcie_soc { > bool need_fix_class_id; > + bool need_fix_device_id; > struct pci_ops *ops; > int (*startup)(struct mtk_pcie_port *port); > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); > @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > writew(val, port->base + PCIE_CONF_CLASS_ID); > } > > + if (soc->need_fix_device_id) { > + val = PCI_DEVICE_ID_MEDIATEK_7629; <just_checking> Could we have a generic way to fix it? It's better not to add SoC specific settings in shared function. </just_checking> > + writew(val, port->base + PCIE_CONF_DEVICE_ID); > + } > + > /* 100ms timeout value should be enough for Gen1/2 training */ > err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, > !!(val & PCIE_PORT_LINKUP_V2), 20, > @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > .setup_irq = mtk_pcie_setup_irq, > }; > > +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { > + .need_fix_class_id = true, > + .need_fix_device_id = true, > + .ops = &mtk_pcie_ops_v2, > + .startup = mtk_pcie_startup_port_v2, > + .setup_irq = mtk_pcie_setup_irq, > +}; > + > +static void mtk_fixup_bar_size(struct pci_dev *dev) > +{ > + struct resource *dev_res = &dev->resource[0]; > + /* 32bit resource length will calculate size to 0, set it smaller */ > + dev_res->end = 0xfffffffe; > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, > + mtk_fixup_bar_size); > + > static const struct of_device_id mtk_pcie_ids[] = { > { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, > { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, > { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, > { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, > + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, > {}, > }; > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index 69f0abe1ba1a..77b278bac3a8 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2126,6 +2126,7 @@ > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > #define PCI_VENDOR_ID_MEDIATEK 0x14c3 > +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 > > #define PCI_VENDOR_ID_TITAN 0x14D2 > #define PCI_DEVICE_ID_TITAN_010L 0x8001
On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. s/arm/ARM/ > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > in arm64 but bogus alignment values at arm32, the pcie device and devices > behind this bridge will not be enabled. Fix it's BAR0 resource size to > guarantee the pcie devices will be enabled correctly. So this is a hardware erratum? Per spec, a memory BAR has bit 0 hardwired to 0, and an IO BAR has bit 1 hardwired to 0. > The HW default value of its device id is invalid, fix it's device id to > match the hardware implementation. s/pcie/PCIe/ (all places above) s/it's/its/ (all places above) s/device id/Device ID/ > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ > include/linux/pci_ids.h | 1 + > 2 files changed, 27 insertions(+) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index d20cf461ba00..f8937cc3c87c 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -73,6 +73,7 @@ > #define PCIE_MSI_VECTOR 0x0c0 > > #define PCIE_CONF_VEND_ID 0x100 > +#define PCIE_CONF_DEVICE_ID 0x102 > #define PCIE_CONF_CLASS_ID 0x106 > > #define PCIE_INT_MASK 0x420 > @@ -135,12 +136,14 @@ struct mtk_pcie_port; > /** > * struct mtk_pcie_soc - differentiate between host generations > * @need_fix_class_id: whether this host's class ID needed to be fixed or not > + * @need_fix_device_id: whether this host's device ID needed to be fixed or not > * @ops: pointer to configuration access functions > * @startup: pointer to controller setting functions > * @setup_irq: pointer to initialize IRQ functions > */ > struct mtk_pcie_soc { > bool need_fix_class_id; > + bool need_fix_device_id; > struct pci_ops *ops; > int (*startup)(struct mtk_pcie_port *port); > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); > @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > writew(val, port->base + PCIE_CONF_CLASS_ID); > } > > + if (soc->need_fix_device_id) { > + val = PCI_DEVICE_ID_MEDIATEK_7629; > + writew(val, port->base + PCIE_CONF_DEVICE_ID); > + } > + > /* 100ms timeout value should be enough for Gen1/2 training */ > err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, > !!(val & PCIE_PORT_LINKUP_V2), 20, > @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > .setup_irq = mtk_pcie_setup_irq, > }; > > +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { > + .need_fix_class_id = true, > + .need_fix_device_id = true, > + .ops = &mtk_pcie_ops_v2, > + .startup = mtk_pcie_startup_port_v2, > + .setup_irq = mtk_pcie_setup_irq, > +}; > + > +static void mtk_fixup_bar_size(struct pci_dev *dev) > +{ > + struct resource *dev_res = &dev->resource[0]; Add a blank line here. > + /* 32bit resource length will calculate size to 0, set it smaller */ > + dev_res->end = 0xfffffffe; Presumably you know the size of the BAR because that's fixed by the hardware and documented in the spec. You should do something like this so you don't depend on what the BAR contains, e.g., res->end = res->start + size - 1; > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, > + mtk_fixup_bar_size); > + > static const struct of_device_id mtk_pcie_ids[] = { > { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, > { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, > { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, > { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, > + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, > {}, > }; > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index 69f0abe1ba1a..77b278bac3a8 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2126,6 +2126,7 @@ > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > #define PCI_VENDOR_ID_MEDIATEK 0x14c3 > +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 > > #define PCI_VENDOR_ID_TITAN 0x14D2 > #define PCI_DEVICE_ID_TITAN_010L 0x8001 > -- > 2.19.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, 2018-12-13 at 11:39 +0800, Ryder Lee wrote: > Hi, > > On Thu, 2018-12-06 at 09:09 +0800, Jianjun Wang wrote: > > MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. > > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > guarantee the pcie devices will be enabled correctly. > > > > The HW default value of its device id is invalid, fix it's device id to > > match the hardware implementation. > > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ > > include/linux/pci_ids.h | 1 + > > 2 files changed, 27 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > index d20cf461ba00..f8937cc3c87c 100644 > > --- a/drivers/pci/controller/pcie-mediatek.c > > +++ b/drivers/pci/controller/pcie-mediatek.c > > @@ -73,6 +73,7 @@ > > #define PCIE_MSI_VECTOR 0x0c0 > > > > #define PCIE_CONF_VEND_ID 0x100 > > +#define PCIE_CONF_DEVICE_ID 0x102 > > #define PCIE_CONF_CLASS_ID 0x106 > > > > #define PCIE_INT_MASK 0x420 > > @@ -135,12 +136,14 @@ struct mtk_pcie_port; > > /** > > * struct mtk_pcie_soc - differentiate between host generations > > * @need_fix_class_id: whether this host's class ID needed to be fixed or not > > + * @need_fix_device_id: whether this host's device ID needed to be fixed or not > > * @ops: pointer to configuration access functions > > * @startup: pointer to controller setting functions > > * @setup_irq: pointer to initialize IRQ functions > > */ > > struct mtk_pcie_soc { > > bool need_fix_class_id; > > + bool need_fix_device_id; > > struct pci_ops *ops; > > int (*startup)(struct mtk_pcie_port *port); > > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); > > @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > > writew(val, port->base + PCIE_CONF_CLASS_ID); > > } > > > > + if (soc->need_fix_device_id) { > > + val = PCI_DEVICE_ID_MEDIATEK_7629; > > <just_checking> > Could we have a generic way to fix it? It's better not to add SoC > specific settings in shared function. > </just_checking> > Yes, I will modify the code in next patch, thanks. > > + writew(val, port->base + PCIE_CONF_DEVICE_ID); > > + } > > + > > /* 100ms timeout value should be enough for Gen1/2 training */ > > err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, > > !!(val & PCIE_PORT_LINKUP_V2), 20, > > @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > > .setup_irq = mtk_pcie_setup_irq, > > }; > > > > +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { > > + .need_fix_class_id = true, > > + .need_fix_device_id = true, > > + .ops = &mtk_pcie_ops_v2, > > + .startup = mtk_pcie_startup_port_v2, > > + .setup_irq = mtk_pcie_setup_irq, > > +}; > > + > > +static void mtk_fixup_bar_size(struct pci_dev *dev) > > +{ > > + struct resource *dev_res = &dev->resource[0]; > > + /* 32bit resource length will calculate size to 0, set it smaller */ > > + dev_res->end = 0xfffffffe; > > +} > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, > > + mtk_fixup_bar_size); > > + > > static const struct of_device_id mtk_pcie_ids[] = { > > { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, > > { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, > > { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, > > { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, > > + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, > > {}, > > }; > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > index 69f0abe1ba1a..77b278bac3a8 100644 > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -2126,6 +2126,7 @@ > > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > > > #define PCI_VENDOR_ID_MEDIATEK 0x14c3 > > +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 > > > > #define PCI_VENDOR_ID_TITAN 0x14D2 > > #define PCI_DEVICE_ID_TITAN_010L 0x8001 > >
On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. > > s/arm/ARM/ > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > guarantee the pcie devices will be enabled correctly. > > So this is a hardware erratum? Per spec, a memory BAR has bit 0 hardwired > to 0, and an IO BAR has bit 1 hardwired to 0. Yes, it only works properly on 64bit platform. > > > The HW default value of its device id is invalid, fix it's device id to > > match the hardware implementation. > > s/pcie/PCIe/ (all places above) > s/it's/its/ (all places above) > s/device id/Device ID/ OK, thanks. > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ > > include/linux/pci_ids.h | 1 + > > 2 files changed, 27 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > index d20cf461ba00..f8937cc3c87c 100644 > > --- a/drivers/pci/controller/pcie-mediatek.c > > +++ b/drivers/pci/controller/pcie-mediatek.c > > @@ -73,6 +73,7 @@ > > #define PCIE_MSI_VECTOR 0x0c0 > > > > #define PCIE_CONF_VEND_ID 0x100 > > +#define PCIE_CONF_DEVICE_ID 0x102 > > #define PCIE_CONF_CLASS_ID 0x106 > > > > #define PCIE_INT_MASK 0x420 > > @@ -135,12 +136,14 @@ struct mtk_pcie_port; > > /** > > * struct mtk_pcie_soc - differentiate between host generations > > * @need_fix_class_id: whether this host's class ID needed to be fixed or not > > + * @need_fix_device_id: whether this host's device ID needed to be fixed or not > > * @ops: pointer to configuration access functions > > * @startup: pointer to controller setting functions > > * @setup_irq: pointer to initialize IRQ functions > > */ > > struct mtk_pcie_soc { > > bool need_fix_class_id; > > + bool need_fix_device_id; > > struct pci_ops *ops; > > int (*startup)(struct mtk_pcie_port *port); > > int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); > > @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) > > writew(val, port->base + PCIE_CONF_CLASS_ID); > > } > > > > + if (soc->need_fix_device_id) { > > + val = PCI_DEVICE_ID_MEDIATEK_7629; > > + writew(val, port->base + PCIE_CONF_DEVICE_ID); > > + } > > + > > /* 100ms timeout value should be enough for Gen1/2 training */ > > err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, > > !!(val & PCIE_PORT_LINKUP_V2), 20, > > @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { > > .setup_irq = mtk_pcie_setup_irq, > > }; > > > > +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { > > + .need_fix_class_id = true, > > + .need_fix_device_id = true, > > + .ops = &mtk_pcie_ops_v2, > > + .startup = mtk_pcie_startup_port_v2, > > + .setup_irq = mtk_pcie_setup_irq, > > +}; > > + > > +static void mtk_fixup_bar_size(struct pci_dev *dev) > > +{ > > + struct resource *dev_res = &dev->resource[0]; > > Add a blank line here. OK. > > > + /* 32bit resource length will calculate size to 0, set it smaller */ > > + dev_res->end = 0xfffffffe; > > Presumably you know the size of the BAR because that's fixed by the > hardware and documented in the spec. You should do something like > this so you don't depend on what the BAR contains, e.g., > > res->end = res->start + size - 1; Yes, it will be much more better, I will modify the code in next version, thanks. > > > +} > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, > > + mtk_fixup_bar_size); > > + > > static const struct of_device_id mtk_pcie_ids[] = { > > { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, > > { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, > > { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, > > { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, > > + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, > > {}, > > }; > > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > > index 69f0abe1ba1a..77b278bac3a8 100644 > > --- a/include/linux/pci_ids.h > > +++ b/include/linux/pci_ids.h > > @@ -2126,6 +2126,7 @@ > > #define PCI_VENDOR_ID_MYRICOM 0x14c1 > > > > #define PCI_VENDOR_ID_MEDIATEK 0x14c3 > > +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 > > > > #define PCI_VENDOR_ID_TITAN 0x14D2 > > #define PCI_DEVICE_ID_TITAN_010L 0x8001 > > -- > > 2.19.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > > guarantee the pcie devices will be enabled correctly. > > > > So this is a hardware erratum? Per spec, a memory BAR has bit 0 hardwired > > to 0, and an IO BAR has bit 1 hardwired to 0. > > Yes, it only works properly on 64bit platform. I don't understand. BARs are supposed to work the same regardless of whether it's a 32- or 64-bit platform. If this is a workaround for a hardware defect, please just say that explicitly. Bjorn
On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > > > guarantee the pcie devices will be enabled correctly. > > > > > > So this is a hardware erratum? Per spec, a memory BAR has bit 0 hardwired > > > to 0, and an IO BAR has bit 1 hardwired to 0. > > > > Yes, it only works properly on 64bit platform. > > I don't understand. BARs are supposed to work the same regardless of > whether it's a 32- or 64-bit platform. If this is a workaround for a > hardware defect, please just say that explicitly. I do not understand this either. First thing to do is to describe the problem properly so that we can actually find a solution to it. Lorenzo
On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > > > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > > > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > > > > guarantee the pcie devices will be enabled correctly. > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has bit 0 hardwired > > > > to 0, and an IO BAR has bit 1 hardwired to 0. > > > > > > Yes, it only works properly on 64bit platform. > > > > I don't understand. BARs are supposed to work the same regardless of > > whether it's a 32- or 64-bit platform. If this is a workaround for a > > hardware defect, please just say that explicitly. > > I do not understand this either. First thing to do is to describe the > problem properly so that we can actually find a solution to it. > > Lorenzo This BAR0 is a 64-bit memory BAR, the HW default values for this BAR is 0xffff_ffff_0000_0000 and it could not be changed except by config write operation. The calculated BAR size will be 0 in 32-bit platform since the phys_addr_t is a 32bit value in 32-bit platform. Actually MediaTek's HW does not using this BAR0, just omit it when assign resource is totally fine. When assign the resource for each device, software will check the resource alignment first, and the resource of length zero will be regarded as a bogus alignment resource, it will be ignored and won't claim a resource parent for it. When drivers try to enable the PCIe devices, the software will enable it's resources, but it will return an error number when found a unclaimed resource, in that case, the flow of enable devices will be interrupted and PCIe devices won't work properly. Thanks.
On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > > > > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > > > > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > > > > > guarantee the pcie devices will be enabled correctly. > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has bit 0 hardwired > > > > > to 0, and an IO BAR has bit 1 hardwired to 0. > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > I don't understand. BARs are supposed to work the same regardless of > > > whether it's a 32- or 64-bit platform. If this is a workaround for a > > > hardware defect, please just say that explicitly. > > > > I do not understand this either. First thing to do is to describe the > > problem properly so that we can actually find a solution to it. > > > > Lorenzo > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR is > 0xffff_ffff_0000_0000 and it could not be changed except by config write > operation. > > The calculated BAR size will be 0 in 32-bit platform since the > phys_addr_t is a 32bit value in 32-bit platform. > > Actually MediaTek's HW does not using this BAR0, just omit it when > assign resource is totally fine. > > When assign the resource for each device, software will check the > resource alignment first, and the resource of length zero will be > regarded as a bogus alignment resource, it will be ignored and won't > claim a resource parent for it. > > When drivers try to enable the PCIe devices, the software will enable > it's resources, but it will return an error number when found a > unclaimed resource, in that case, the flow of enable devices will be > interrupted and PCIe devices won't work properly. As a starting point, please provide kernel logs for both 64-bit and 32-bit platforms (without this patch applied) and also a: cat /proc/iomem and lspci output for both configurations. Thanks, Lorenzo
On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be > > > > > > calculated as 4GB in arm64 but bogus alignment values at > > > > > > arm32, the pcie device and devices behind this bridge will > > > > > > not be enabled. Fix it's BAR0 resource size to guarantee > > > > > > the pcie devices will be enabled correctly. > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has > > > > > bit 0 hardwired to 0, and an IO BAR has bit 1 hardwired to > > > > > 0. > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > I don't understand. BARs are supposed to work the same > > > regardless of whether it's a 32- or 64-bit platform. If this is > > > a workaround for a hardware defect, please just say that > > > explicitly. > > > > I do not understand this either. First thing to do is to describe > > the problem properly so that we can actually find a solution to > > it. > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR > is 0xffff_ffff_0000_0000 and it could not be changed except by > config write operation. If you literally get 0xffff_ffff_0000_0000 when reading the BAR, that is out of spec because the low-order 4 bits of a 64-bit memory BAR cannot all be zero. A 64-bit BAR consumes two DWORDS in config space. For a 64-bit BAR0, the DWORD at 0x10 contains the low-order bits, and the DWORD at 0x14 contains the upper 32 bits. Bits 0-3 of the low-order DWORD (the one at 0x10) are read-only, and in this case should contain the value 0b1100 (0xc). That means the range is prefetchable (bit 3 == 1) and the BAR is 64 bits (bits 2:1 == 10). > The calculated BAR size will be 0 in 32-bit platform since the > phys_addr_t is a 32bit value in 32-bit platform. Either (1) this is a hardware defect that feeds incorrect data to the BAR size calculation, or (2) there's a problem in the BAR size calculation code. We need to figure out which one and work around or fix it correctly. > Actually MediaTek's HW does not using this BAR0, just omit it when > assign resource is totally fine. It's totally fine to work around hardware defects, but we have to clearly understand the problem so we do it correctly. For example, we probably can't just clear out the BAR0 resource in the pci_dev, because the BAR in the hardware device still contains a value, and if we enable memory decoding for the device, it will still respond to the region described by the BAR. > When assign the resource for each device, software will check the > resource alignment first, and the resource of length zero will be > regarded as a bogus alignment resource, it will be ignored and won't > claim a resource parent for it. > > When drivers try to enable the PCIe devices, the software will enable > it's resources, but it will return an error number when found a > unclaimed resource, in that case, the flow of enable devices will be > interrupted and PCIe devices won't work properly. > > Thanks. >
On Tue, 2018-12-18 at 15:32 +0000, Lorenzo Pieralisi wrote: > On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB > > > > > > > in arm64 but bogus alignment values at arm32, the pcie device and devices > > > > > > > behind this bridge will not be enabled. Fix it's BAR0 resource size to > > > > > > > guarantee the pcie devices will be enabled correctly. > > > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has bit 0 hardwired > > > > > > to 0, and an IO BAR has bit 1 hardwired to 0. > > > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > > > I don't understand. BARs are supposed to work the same regardless of > > > > whether it's a 32- or 64-bit platform. If this is a workaround for a > > > > hardware defect, please just say that explicitly. > > > > > > I do not understand this either. First thing to do is to describe the > > > problem properly so that we can actually find a solution to it. > > > > > > Lorenzo > > > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR is > > 0xffff_ffff_0000_0000 and it could not be changed except by config write > > operation. > > > > The calculated BAR size will be 0 in 32-bit platform since the > > phys_addr_t is a 32bit value in 32-bit platform. > > > > Actually MediaTek's HW does not using this BAR0, just omit it when > > assign resource is totally fine. > > > > When assign the resource for each device, software will check the > > resource alignment first, and the resource of length zero will be > > regarded as a bogus alignment resource, it will be ignored and won't > > claim a resource parent for it. > > > > When drivers try to enable the PCIe devices, the software will enable > > it's resources, but it will return an error number when found a > > unclaimed resource, in that case, the flow of enable devices will be > > interrupted and PCIe devices won't work properly. > > As a starting point, please provide kernel logs for both 64-bit and > 32-bit platforms (without this patch applied) and also a: > > cat /proc/iomem > > and > > lspci > > output for both configurations. > > Thanks, > Lorenzo I compared with the mt2712 platform, which is an arm64 platform, and the EP deivce is an Intel e1000e NIC. Compiled these drivers to modules (without this patch applied both), and insert them separately after system startup. After insert these modules, there will be an Ethernet device on both platform, and the 64-bit platform works properly, but the 32-bit platform doesn't. 1.The following logs is from the 64-bit platform (mt2712): 1.1. insmod pcie-mediatek.ko: mtk-pcie 11700000.pcie: host bridge /pcie@11700000 ranges: mtk-pcie 11700000.pcie: MEM 0x20000000..0x2fffffff -> 0x20000000 mtk-pcie 11700000.pcie: PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [bus 00-ff] pci_bus 0000:00: root bus resource [mem 0x20000000-0x2fffffff] pci 0000:00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring pci 0000:01:00.0: 2.000 Gb/s available PCIe bandwidth, limited by 2.5 GT/s x1 link at 0000:00:00.0 (capable of 7.876 Gb/s with 8 GT/s x1 link) pci 0000:00:00.0: BAR 0: no space for [mem size 0x100000000 64bit pref] pci 0000:00:00.0: BAR 0: failed to assign [mem size 0x100000000 64bit pref] pci 0000:00:00.0: BAR 8: assigned [mem 0x20000000-0x200fffff] pci 0000:01:00.0: BAR 1: assigned [mem 0x20000000-0x2007ffff] pci 0000:01:00.0: BAR 6: assigned [mem 0x20080000-0x200bffff pref] pci 0000:01:00.0: BAR 0: assigned [mem 0x200c0000-0x200dffff] pci 0000:01:00.0: BAR 3: assigned [mem 0x200e0000-0x200e3fff] pci 0000:01:00.0: BAR 2: no space for [io size 0x0020] pci 0000:01:00.0: BAR 2: failed to assign [io size 0x0020] pci 0000:00:00.0: PCI bridge to [bus 01] pci 0000:00:00.0: bridge window [mem 0x20000000-0x200fffff] 1.2. insmod e1000e.ko: e1000e: Intel(R) PRO/1000 Network Driver - 3.2.6-k e1000e: Copyright(c) 1999 - 2015 Intel Corporation. pci 0000:00:00.0: enabling device (0000 -> 0002) e1000e 0000:01:00.0: enabling device (0000 -> 0002) e1000e 0000:01:00.0: Interrupt Throttling Rate (ints/sec) set to dynamic conservative mode e1000e 0000:01:00.0 0000:01:00.0 (uninitialized): registered PHC clock e1000e 0000:01:00.0 eth0: (PCI Express:2.5GT/s:Width x1) 2c:53:4a:03:89:e9 e1000e 0000:01:00.0 eth0: Intel(R) PRO/1000 Network Connection e1000e 0000:01:00.0 eth0: MAC: 3, PHY: 8, PBA No: FFFFFF-0FF 1.3. lspci -xxx 00:00.0 Class 0604: Device 14c3:2712 00: c3 14 12 27 06 00 10 00 00 01 04 06 00 00 01 00 10: 0c 00 00 00 00 00 00 00 00 01 01 40 00 00 20 04 20: 00 20 00 20 00 00 00 00 00 00 00 00 00 00 00 00 30: 00 00 00 00 50 00 00 00 00 00 00 00 00 01 00 00 40: 00 00 00 00 60 61 12 02 00 00 00 00 00 00 00 00 50: 05 78 80 00 00 00 00 00 00 00 00 00 00 00 00 00 60: 00 00 00 00 00 00 00 00 11 78 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 01 80 c3 01 08 00 00 00 80: 10 00 42 01 41 83 00 00 10 28 20 00 12 8c 60 01 90: 08 00 11 10 00 00 04 00 00 00 00 00 00 00 00 00 a0: 00 00 00 00 00 08 00 00 00 00 00 00 00 00 00 00 b0: 02 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01:00.0 Class 0200: Device 8086:10d3 00: 86 80 d3 10 06 04 18 00 00 00 00 02 10 00 00 00 10: 00 00 0c 20 00 00 00 20 01 00 00 00 00 00 0e 20 20: 00 00 00 00 00 00 00 00 00 00 00 00 1a 1d 00 00 30: 00 00 00 00 c8 00 00 00 00 00 00 00 f3 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a0: 11 00 04 80 03 00 00 00 03 20 00 00 00 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 01 d0 22 c8 00 20 00 14 d0: 05 e0 80 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 10 a0 01 00 c1 8c 00 00 10 28 10 00 11 1c 03 01 f0: 00 00 11 10 00 00 00 00 00 00 00 00 00 00 00 00 1.4 lspci -v 00:00.0 Class 0604: Device 14c3:2712 (prog-if 01) Flags: bus master, fast devsel, latency 0 Memory at <unassigned> (64-bit, prefetchable) Bus: primary=00, secondary=01, subordinate=01, sec-latency=64 I/O behind bridge: 00000000-00000fff [size=4K] Memory behind bridge: 20000000-200fffff [size=1M] Prefetchable memory behind bridge: 00000000-000fffff [size=1M] Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ Capabilities: [78] Power Management version 3 Capabilities: [80] Express Root Port (Slot+), MSI 00 Capabilities: [100] Virtual Channel Capabilities: [400] L1 PM Substates Capabilities: [600] Latency Tolerance Reporting 01:00.0 Class 0200: Device 8086:10d3 Subsystem: Device 1d1a:0000 Flags: bus master, fast devsel, latency 0, IRQ 243 Memory at 200c0000 (32-bit, non-prefetchable) [size=128K] Memory at 20000000 (32-bit, non-prefetchable) [size=512K] I/O ports at <unassigned> [disabled] Memory at 200e0000 (32-bit, non-prefetchable) [size=16K] [virtual] Expansion ROM at 20080000 [disabled] [size=256K] Capabilities: [c8] Power Management version 2 Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+ Capabilities: [e0] Express Endpoint, MSI 00 Capabilities: [a0] MSI-X: Enable+ Count=5 Masked- Capabilities: [100] Advanced Error Reporting Capabilities: [140] Device Serial Number 2c-53-4a-ff-ff-03-89-e9 Kernel driver in use: e1000e 1.5. cat /proc/iomem 20000000-2fffffff : pcie@11700000 20000000-200fffff : PCI Bus 0000:01 20000000-2007ffff : 0000:01:00.0 20000000-2007ffff : e1000e 20080000-200bffff : 0000:01:00.0 200c0000-200dffff : 0000:01:00.0 200c0000-200dffff : e1000e 200e0000-200e3fff : 0000:01:00.0 200e0000-200e3fff : e1000e 2. The following logs is from the 32-bit platform (mt7629): 2.1. insmod pcie-mediatek.ko mtk-pcie 1a140000.pcie: host bridge /pcie@1a140000 ranges: mtk-pcie 1a140000.pcie: MEM 0x20000000..0x2fffffff -> 0x20000000 mtk-pcie 1a140000.pcie: PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [bus 00-ff] pci_bus 0000:00: root bus resource [mem 0x20000000-0x2fffffff] PCI: bus0: Fast back to back transfers disabled pci 0000:00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring pci 0000:01:00.0: 2.000 Gb/s available PCIe bandwidth, limited by 2.5 GT/s x1 link at 0000:00:01.0 (capable of 7.876 Gb/s with 8 GT/s x1 link) PCI: bus1: Fast back to back transfers disabled pci 0000:00:01.0: BAR 0: [mem 0x00000000-0xffffffff 64bit pref] has bogus alignment pci 0000:00:01.0: BAR 8: assigned [mem 0x20000000-0x200fffff] pci 0000:01:00.0: BAR 1: assigned [mem 0x20000000-0x2007ffff] pci 0000:01:00.0: BAR 6: assigned [mem 0x20080000-0x200bffff pref] pci 0000:01:00.0: BAR 0: assigned [mem 0x200c0000-0x200dffff] pci 0000:01:00.0: BAR 3: assigned [mem 0x200e0000-0x200e3fff] pci 0000:01:00.0: BAR 2: no space for [io size 0x0020] pci 0000:01:00.0: BAR 2: failed to assign [io size 0x0020] pci 0000:00:01.0: PCI bridge to [bus 01] pci 0000:00:01.0: bridge window [mem 0x20000000-0x200fffff] 2.2. insmod e1000e.ko e1000e: Intel(R) PRO/1000 Network Driver - 3.2.6-k e1000e: Copyright(c) 1999 - 2015 Intel Corporation. pci 0000:00:01.0: can't enable device: BAR 0 [mem 0x00000000-0xffffffff 64bit pref] not claimed pci 0000:00:01.0: Error enabling bridge (-22), continuing e1000e 0000:01:00.0: enabling device (0140 -> 0142) e1000e 0000:01:00.0: Interrupt Throttling Rate (ints/sec) set to dynamic conservative mode e1000e 0000:01:00.0 eth0: (PCI Express:2.5GT/s:Width x1) 2c:53:4a:03:89:e9 e1000e 0000:01:00.0 eth0: Intel(R) PRO/1000 Network Connection e1000e 0000:01:00.0 eth0: MAC: 3, PHY: 8, PBA No: FFFFFF-0FF 2.3. lspci -xxx 00:01.0 Class 0604: Device 14c3:7629 00: c3 14 29 76 40 01 10 00 00 01 04 06 10 00 01 00 10: 0c 00 00 00 00 00 00 00 00 01 01 40 00 00 20 04 20: 00 20 00 20 00 00 00 00 00 00 00 00 00 00 00 00 30: 00 00 00 00 50 00 00 00 00 00 00 00 00 01 01 00 40: 00 00 00 00 60 61 12 02 00 00 00 00 00 00 00 00 50: 05 78 80 00 00 00 00 00 00 00 00 00 00 00 00 00 60: 00 00 00 00 00 00 00 00 11 78 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 01 80 c3 01 08 00 00 00 80: 10 00 42 01 41 83 00 00 10 28 20 00 22 8c 60 01 90: 08 00 11 10 00 00 04 00 00 00 00 00 00 00 00 00 a0: 00 00 00 00 00 08 00 00 00 00 00 00 00 00 00 00 b0: 02 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01:00.0 Class 0200: Device 8086:10d3 00: 86 80 d3 10 40 01 10 00 00 00 00 02 10 00 00 00 10: 00 00 0c 20 00 00 00 20 01 00 00 00 00 00 0e 20 20: 00 00 00 00 00 00 00 00 00 00 00 00 1a 1d 00 00 30: 00 00 00 00 c8 00 00 00 00 00 00 00 00 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a0: 11 00 04 00 03 00 00 00 03 20 00 00 00 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 01 d0 22 c8 00 20 00 14 d0: 05 e0 80 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 10 a0 01 00 c1 8c 00 00 10 28 10 00 11 1c 03 01 f0: 00 00 11 10 00 00 00 00 00 00 00 00 00 00 00 00 2.4. lspci -v 00:01.0 Class 0604: Device 14c3:7629 (prog-if 01) Flags: bus master, fast devsel, latency 0 Memory at <unassigned> (64-bit, prefetchable) [disabled] [size=4G] Bus: primary=00, secondary=01, subordinate=01, sec-latency=64 I/O behind bridge: 00000000-00000fff [size=4K] Memory behind bridge: 20000000-200fffff [size=1M] Prefetchable memory behind bridge: 00000000-000fffff [size=1M] Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ Capabilities: [78] Power Management version 3 Capabilities: [80] Express Root Port (Slot+), MSI 00 Capabilities: [100] Virtual Channel Capabilities: [400] L1 PM Substates Capabilities: [600] Latency Tolerance Reporting 01:00.0 Class 0200: Device 8086:10d3 Subsystem: Device 1d1a:0000 Flags: bus master, fast devsel, latency 0, IRQ 110 Memory at 200c0000 (32-bit, non-prefetchable) [size=128K] Memory at 20000000 (32-bit, non-prefetchable) [size=512K] I/O ports at <unassigned> [disabled] Memory at 200e0000 (32-bit, non-prefetchable) [size=16K] [virtual] Expansion ROM at 20080000 [disabled] [size=256K] Capabilities: [c8] Power Management version 2 Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+ Capabilities: [e0] Express Endpoint, MSI 00 Capabilities: [a0] MSI-X: Enable+ Count=5 Masked- Capabilities: [100] Advanced Error Reporting Capabilities: [140] Device Serial Number 2c-53-4a-ff-ff-03-89-e9 Kernel driver in use: e1000e 2.5. cat /proc/iomem 20000000-2fffffff : pcie@1a140000 20000000-200fffff : PCI Bus 0000:01 20000000-2007ffff : 0000:01:00.0 20000000-2007ffff : e1000e 20080000-200bffff : 0000:01:00.0 200c0000-200dffff : 0000:01:00.0 200c0000-200dffff : e1000e 200e0000-200e3fff : 0000:01:00.0 200e0000-200e3fff : e1000e Thanks.
On Thu, 2018-12-20 at 12:20 -0600, Bjorn Helgaas wrote: > On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be > > > > > > > calculated as 4GB in arm64 but bogus alignment values at > > > > > > > arm32, the pcie device and devices behind this bridge will > > > > > > > not be enabled. Fix it's BAR0 resource size to guarantee > > > > > > > the pcie devices will be enabled correctly. > > > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has > > > > > > bit 0 hardwired to 0, and an IO BAR has bit 1 hardwired to > > > > > > 0. > > > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > > > I don't understand. BARs are supposed to work the same > > > > regardless of whether it's a 32- or 64-bit platform. If this is > > > > a workaround for a hardware defect, please just say that > > > > explicitly. > > > > > > I do not understand this either. First thing to do is to describe > > > the problem properly so that we can actually find a solution to > > > it. > > > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR > > is 0xffff_ffff_0000_0000 and it could not be changed except by > > config write operation. > > If you literally get 0xffff_ffff_0000_0000 when reading the BAR, that > is out of spec because the low-order 4 bits of a 64-bit memory BAR > cannot all be zero. > > A 64-bit BAR consumes two DWORDS in config space. For a 64-bit BAR0, > the DWORD at 0x10 contains the low-order bits, and the DWORD at 0x14 > contains the upper 32 bits. Bits 0-3 of the low-order DWORD (the > one at 0x10) are read-only, and in this case should contain the value > 0b1100 (0xc). That means the range is prefetchable (bit 3 == 1) and > the BAR is 64 bits (bits 2:1 == 10). Sorry, I have confused the HW default value and the read value of BAR size. The hardware default value is 0xffff_ffff_0000_000c, it's a 64-bit BAR with prefetchable range. When we start to decoding the BAR, the read value of BAR0 at 0x10 is 0x0c, and the value at 0x14 is 0xffff_ffff, so the read value of BAR size is 0xffff_ffff_0000_0000, which will be decoded to 0xffff_ffff, and it will be set to the end value of BAR0 resource in the pci_dev. > > > The calculated BAR size will be 0 in 32-bit platform since the > > phys_addr_t is a 32bit value in 32-bit platform. > > Either (1) this is a hardware defect that feeds incorrect data to the > BAR size calculation, or (2) there's a problem in the BAR size > calculation code. We need to figure out which one and work around or > fix it correctly. The BAR size is calculated by the code (res->end - res->start + 1) is fine, I think it's a hardware defect because that we can not change the hardware default value or just disable it since we don't using it. > > > Actually MediaTek's HW does not using this BAR0, just omit it when > > assign resource is totally fine. > > It's totally fine to work around hardware defects, but we have to > clearly understand the problem so we do it correctly. For example, we > probably can't just clear out the BAR0 resource in the pci_dev, > because the BAR in the hardware device still contains a value, and if > we enable memory decoding for the device, it will still respond to the > region described by the BAR. The BAR0 resource value in the pci_dev is depend on the hardware value, it can be cleared out after all devices have been scanned, but we should set it's size bigger than MMIO space, so the software will think it's a invalid resource and won't assign a resource for it. Thanks. > > > When assign the resource for each device, software will check the > > resource alignment first, and the resource of length zero will be > > regarded as a bogus alignment resource, it will be ignored and won't > > claim a resource parent for it. > > > > When drivers try to enable the PCIe devices, the software will enable > > it's resources, but it will return an error number when found a > > unclaimed resource, in that case, the flow of enable devices will be > > interrupted and PCIe devices won't work properly. > > > > Thanks. > >
On Mon, Dec 24, 2018 at 07:40:28PM +0800, Jianjun Wang wrote: > On Thu, 2018-12-20 at 12:20 -0600, Bjorn Helgaas wrote: > > On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > > > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be > > > > > > > > calculated as 4GB in arm64 but bogus alignment values at > > > > > > > > arm32, the pcie device and devices behind this bridge will > > > > > > > > not be enabled. Fix it's BAR0 resource size to guarantee > > > > > > > > the pcie devices will be enabled correctly. > > > > > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has > > > > > > > bit 0 hardwired to 0, and an IO BAR has bit 1 hardwired to > > > > > > > 0. > > > > > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > > > > > I don't understand. BARs are supposed to work the same > > > > > regardless of whether it's a 32- or 64-bit platform. If this is > > > > > a workaround for a hardware defect, please just say that > > > > > explicitly. > > > > > > > > I do not understand this either. First thing to do is to describe > > > > the problem properly so that we can actually find a solution to > > > > it. > > > > > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR > > > is 0xffff_ffff_0000_0000 and it could not be changed except by > > > config write operation. > > > > If you literally get 0xffff_ffff_0000_0000 when reading the BAR, that > > is out of spec because the low-order 4 bits of a 64-bit memory BAR > > cannot all be zero. > > > > A 64-bit BAR consumes two DWORDS in config space. For a 64-bit BAR0, > > the DWORD at 0x10 contains the low-order bits, and the DWORD at 0x14 > > contains the upper 32 bits. Bits 0-3 of the low-order DWORD (the > > one at 0x10) are read-only, and in this case should contain the value > > 0b1100 (0xc). That means the range is prefetchable (bit 3 == 1) and > > the BAR is 64 bits (bits 2:1 == 10). > > Sorry, I have confused the HW default value and the read value of BAR > size. The hardware default value is 0xffff_ffff_0000_000c, it's a 64-bit > BAR with prefetchable range. > > When we start to decoding the BAR, the read value of BAR0 at 0x10 is > 0x0c, and the value at 0x14 is 0xffff_ffff, so the read value of BAR > size is 0xffff_ffff_0000_0000, which will be decoded to 0xffff_ffff, and > it will be set to the end value of BAR0 resource in the pci_dev. > > > > > The calculated BAR size will be 0 in 32-bit platform since the > > > phys_addr_t is a 32bit value in 32-bit platform. > > > > Either (1) this is a hardware defect that feeds incorrect data to the > > BAR size calculation, or (2) there's a problem in the BAR size > > calculation code. We need to figure out which one and work around or > > fix it correctly. > > The BAR size is calculated by the code (res->end - res->start + 1) is > fine, I think it's a hardware defect because that we can not change the > hardware default value or just disable it since we don't using it. Apologies for the delay in getting back to this. This looks like a kernel defect, not a HW defect. I need some time to make up my mind on what the right fix for this but it is most certainly not this patch. Lorenzo
On Wed, 2019-01-23 at 15:40 +0000, Lorenzo Pieralisi wrote: > On Mon, Dec 24, 2018 at 07:40:28PM +0800, Jianjun Wang wrote: > > On Thu, 2018-12-20 at 12:20 -0600, Bjorn Helgaas wrote: > > > On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > > > > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > > > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be > > > > > > > > > calculated as 4GB in arm64 but bogus alignment values at > > > > > > > > > arm32, the pcie device and devices behind this bridge will > > > > > > > > > not be enabled. Fix it's BAR0 resource size to guarantee > > > > > > > > > the pcie devices will be enabled correctly. > > > > > > > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has > > > > > > > > bit 0 hardwired to 0, and an IO BAR has bit 1 hardwired to > > > > > > > > 0. > > > > > > > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > > > > > > > I don't understand. BARs are supposed to work the same > > > > > > regardless of whether it's a 32- or 64-bit platform. If this is > > > > > > a workaround for a hardware defect, please just say that > > > > > > explicitly. > > > > > > > > > > I do not understand this either. First thing to do is to describe > > > > > the problem properly so that we can actually find a solution to > > > > > it. > > > > > > > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR > > > > is 0xffff_ffff_0000_0000 and it could not be changed except by > > > > config write operation. > > > > > > If you literally get 0xffff_ffff_0000_0000 when reading the BAR, that > > > is out of spec because the low-order 4 bits of a 64-bit memory BAR > > > cannot all be zero. > > > > > > A 64-bit BAR consumes two DWORDS in config space. For a 64-bit BAR0, > > > the DWORD at 0x10 contains the low-order bits, and the DWORD at 0x14 > > > contains the upper 32 bits. Bits 0-3 of the low-order DWORD (the > > > one at 0x10) are read-only, and in this case should contain the value > > > 0b1100 (0xc). That means the range is prefetchable (bit 3 == 1) and > > > the BAR is 64 bits (bits 2:1 == 10). > > > > Sorry, I have confused the HW default value and the read value of BAR > > size. The hardware default value is 0xffff_ffff_0000_000c, it's a 64-bit > > BAR with prefetchable range. > > > > When we start to decoding the BAR, the read value of BAR0 at 0x10 is > > 0x0c, and the value at 0x14 is 0xffff_ffff, so the read value of BAR > > size is 0xffff_ffff_0000_0000, which will be decoded to 0xffff_ffff, and > > it will be set to the end value of BAR0 resource in the pci_dev. > > > > > > > The calculated BAR size will be 0 in 32-bit platform since the > > > > phys_addr_t is a 32bit value in 32-bit platform. > > > > > > Either (1) this is a hardware defect that feeds incorrect data to the > > > BAR size calculation, or (2) there's a problem in the BAR size > > > calculation code. We need to figure out which one and work around or > > > fix it correctly. > > > > The BAR size is calculated by the code (res->end - res->start + 1) is > > fine, I think it's a hardware defect because that we can not change the > > hardware default value or just disable it since we don't using it. > > Apologies for the delay in getting back to this. > > This looks like a kernel defect, not a HW defect. > > I need some time to make up my mind on what the right fix for this > but it is most certainly not this patch. > > Lorenzo Hi Lorenzo, Is there any better idea about this patch? Thanks.
On Tue, Feb 19, 2019 at 03:01:39PM +0800, Jianjun Wang wrote: > On Wed, 2019-01-23 at 15:40 +0000, Lorenzo Pieralisi wrote: > > On Mon, Dec 24, 2018 at 07:40:28PM +0800, Jianjun Wang wrote: > > > On Thu, 2018-12-20 at 12:20 -0600, Bjorn Helgaas wrote: > > > > On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > > > > > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > > > > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > > > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be > > > > > > > > > > calculated as 4GB in arm64 but bogus alignment values at > > > > > > > > > > arm32, the pcie device and devices behind this bridge will > > > > > > > > > > not be enabled. Fix it's BAR0 resource size to guarantee > > > > > > > > > > the pcie devices will be enabled correctly. > > > > > > > > > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has > > > > > > > > > bit 0 hardwired to 0, and an IO BAR has bit 1 hardwired to > > > > > > > > > 0. > > > > > > > > > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > > > > > > > > > I don't understand. BARs are supposed to work the same > > > > > > > regardless of whether it's a 32- or 64-bit platform. If this is > > > > > > > a workaround for a hardware defect, please just say that > > > > > > > explicitly. > > > > > > > > > > > > I do not understand this either. First thing to do is to describe > > > > > > the problem properly so that we can actually find a solution to > > > > > > it. > > > > > > > > > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR > > > > > is 0xffff_ffff_0000_0000 and it could not be changed except by > > > > > config write operation. > > > > > > > > If you literally get 0xffff_ffff_0000_0000 when reading the BAR, that > > > > is out of spec because the low-order 4 bits of a 64-bit memory BAR > > > > cannot all be zero. > > > > > > > > A 64-bit BAR consumes two DWORDS in config space. For a 64-bit BAR0, > > > > the DWORD at 0x10 contains the low-order bits, and the DWORD at 0x14 > > > > contains the upper 32 bits. Bits 0-3 of the low-order DWORD (the > > > > one at 0x10) are read-only, and in this case should contain the value > > > > 0b1100 (0xc). That means the range is prefetchable (bit 3 == 1) and > > > > the BAR is 64 bits (bits 2:1 == 10). > > > > > > Sorry, I have confused the HW default value and the read value of BAR > > > size. The hardware default value is 0xffff_ffff_0000_000c, it's a 64-bit > > > BAR with prefetchable range. > > > > > > When we start to decoding the BAR, the read value of BAR0 at 0x10 is > > > 0x0c, and the value at 0x14 is 0xffff_ffff, so the read value of BAR > > > size is 0xffff_ffff_0000_0000, which will be decoded to 0xffff_ffff, and > > > it will be set to the end value of BAR0 resource in the pci_dev. > > > > > > > > > The calculated BAR size will be 0 in 32-bit platform since the > > > > > phys_addr_t is a 32bit value in 32-bit platform. > > > > > > > > Either (1) this is a hardware defect that feeds incorrect data to the > > > > BAR size calculation, or (2) there's a problem in the BAR size > > > > calculation code. We need to figure out which one and work around or > > > > fix it correctly. > > > > > > The BAR size is calculated by the code (res->end - res->start + 1) is > > > fine, I think it's a hardware defect because that we can not change the > > > hardware default value or just disable it since we don't using it. > > > > Apologies for the delay in getting back to this. > > > > This looks like a kernel defect, not a HW defect. > > > > I need some time to make up my mind on what the right fix for this > > but it is most certainly not this patch. > > > > Lorenzo > > Hi Lorenzo, > > Is there any better idea about this patch? Hi, I did not have time to investigate the issue in core code that triggers this defect but this patch is not the solution to the problem it is a plaster that papers over it, I won't merge it. I would appreciate some help. If you could have a look at core code that triggers the failure we can analyze what should be done to make it work, I do not think it is a defect in your IP. Lorenzo
On Tue, 2019-02-19 at 23:03 +0800, Lorenzo Pieralisi wrote: > On Tue, Feb 19, 2019 at 03:01:39PM +0800, Jianjun Wang wrote: > > On Wed, 2019-01-23 at 15:40 +0000, Lorenzo Pieralisi wrote: > > > On Mon, Dec 24, 2018 at 07:40:28PM +0800, Jianjun Wang wrote: > > > > On Thu, 2018-12-20 at 12:20 -0600, Bjorn Helgaas wrote: > > > > > On Tue, Dec 18, 2018 at 05:19:24PM +0800, Jianjun Wang wrote: > > > > > > On Mon, 2018-12-17 at 15:46 +0000, Lorenzo Pieralisi wrote: > > > > > > > On Mon, Dec 17, 2018 at 08:32:47AM -0600, Bjorn Helgaas wrote: > > > > > > > > On Mon, Dec 17, 2018 at 04:19:39PM +0800, Jianjun Wang wrote: > > > > > > > > > On Thu, 2018-12-13 at 08:55 -0600, Bjorn Helgaas wrote: > > > > > > > > > > On Thu, Dec 06, 2018 at 09:09:13AM +0800, Jianjun Wang wrote: > > > > > > > > > > > The read value of BAR0 is 0xffff_ffff, it's size will be > > > > > > > > > > > calculated as 4GB in arm64 but bogus alignment values at > > > > > > > > > > > arm32, the pcie device and devices behind this bridge will > > > > > > > > > > > not be enabled. Fix it's BAR0 resource size to guarantee > > > > > > > > > > > the pcie devices will be enabled correctly. > > > > > > > > > > > > > > > > > > > > So this is a hardware erratum? Per spec, a memory BAR has > > > > > > > > > > bit 0 hardwired to 0, and an IO BAR has bit 1 hardwired to > > > > > > > > > > 0. > > > > > > > > > > > > > > > > > > Yes, it only works properly on 64bit platform. > > > > > > > > > > > > > > > > I don't understand. BARs are supposed to work the same > > > > > > > > regardless of whether it's a 32- or 64-bit platform. If this is > > > > > > > > a workaround for a hardware defect, please just say that > > > > > > > > explicitly. > > > > > > > > > > > > > > I do not understand this either. First thing to do is to describe > > > > > > > the problem properly so that we can actually find a solution to > > > > > > > it. > > > > > > > > > > > > This BAR0 is a 64-bit memory BAR, the HW default values for this BAR > > > > > > is 0xffff_ffff_0000_0000 and it could not be changed except by > > > > > > config write operation. > > > > > > > > > > If you literally get 0xffff_ffff_0000_0000 when reading the BAR, that > > > > > is out of spec because the low-order 4 bits of a 64-bit memory BAR > > > > > cannot all be zero. > > > > > > > > > > A 64-bit BAR consumes two DWORDS in config space. For a 64-bit BAR0, > > > > > the DWORD at 0x10 contains the low-order bits, and the DWORD at 0x14 > > > > > contains the upper 32 bits. Bits 0-3 of the low-order DWORD (the > > > > > one at 0x10) are read-only, and in this case should contain the value > > > > > 0b1100 (0xc). That means the range is prefetchable (bit 3 == 1) and > > > > > the BAR is 64 bits (bits 2:1 == 10). > > > > > > > > Sorry, I have confused the HW default value and the read value of BAR > > > > size. The hardware default value is 0xffff_ffff_0000_000c, it's a 64-bit > > > > BAR with prefetchable range. > > > > > > > > When we start to decoding the BAR, the read value of BAR0 at 0x10 is > > > > 0x0c, and the value at 0x14 is 0xffff_ffff, so the read value of BAR > > > > size is 0xffff_ffff_0000_0000, which will be decoded to 0xffff_ffff, and > > > > it will be set to the end value of BAR0 resource in the pci_dev. > > > > > > > > > > > The calculated BAR size will be 0 in 32-bit platform since the > > > > > > phys_addr_t is a 32bit value in 32-bit platform. > > > > > > > > > > Either (1) this is a hardware defect that feeds incorrect data to the > > > > > BAR size calculation, or (2) there's a problem in the BAR size > > > > > calculation code. We need to figure out which one and work around or > > > > > fix it correctly. > > > > > > > > The BAR size is calculated by the code (res->end - res->start + 1) is > > > > fine, I think it's a hardware defect because that we can not change the > > > > hardware default value or just disable it since we don't using it. > > > > > > Apologies for the delay in getting back to this. > > > > > > This looks like a kernel defect, not a HW defect. > > > > > > I need some time to make up my mind on what the right fix for this > > > but it is most certainly not this patch. > > > > > > Lorenzo > > > > Hi Lorenzo, > > > > Is there any better idea about this patch? > > Hi, > > I did not have time to investigate the issue in core code that triggers > this defect but this patch is not the solution to the problem it is a > plaster that papers over it, I won't merge it. > > I would appreciate some help. If you could have a look at core code that > triggers the failure we can analyze what should be done to make it work, > I do not think it is a defect in your IP. > > Lorenzo Hi Lorenzo, This BAR size issue has been fixed by commit "01b37f851ca150554496fd6e79c6d9a67992a2c0 PCI: Make pci_size() return real BAR size" So there is no need to add the fixup method, I will remove it in next version. Thanks.
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c index d20cf461ba00..f8937cc3c87c 100644 --- a/drivers/pci/controller/pcie-mediatek.c +++ b/drivers/pci/controller/pcie-mediatek.c @@ -73,6 +73,7 @@ #define PCIE_MSI_VECTOR 0x0c0 #define PCIE_CONF_VEND_ID 0x100 +#define PCIE_CONF_DEVICE_ID 0x102 #define PCIE_CONF_CLASS_ID 0x106 #define PCIE_INT_MASK 0x420 @@ -135,12 +136,14 @@ struct mtk_pcie_port; /** * struct mtk_pcie_soc - differentiate between host generations * @need_fix_class_id: whether this host's class ID needed to be fixed or not + * @need_fix_device_id: whether this host's device ID needed to be fixed or not * @ops: pointer to configuration access functions * @startup: pointer to controller setting functions * @setup_irq: pointer to initialize IRQ functions */ struct mtk_pcie_soc { bool need_fix_class_id; + bool need_fix_device_id; struct pci_ops *ops; int (*startup)(struct mtk_pcie_port *port); int (*setup_irq)(struct mtk_pcie_port *port, struct device_node *node); @@ -692,6 +695,11 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port) writew(val, port->base + PCIE_CONF_CLASS_ID); } + if (soc->need_fix_device_id) { + val = PCI_DEVICE_ID_MEDIATEK_7629; + writew(val, port->base + PCIE_CONF_DEVICE_ID); + } + /* 100ms timeout value should be enough for Gen1/2 training */ err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val, !!(val & PCIE_PORT_LINKUP_V2), 20, @@ -1238,11 +1246,29 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7622 = { .setup_irq = mtk_pcie_setup_irq, }; +static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = { + .need_fix_class_id = true, + .need_fix_device_id = true, + .ops = &mtk_pcie_ops_v2, + .startup = mtk_pcie_startup_port_v2, + .setup_irq = mtk_pcie_setup_irq, +}; + +static void mtk_fixup_bar_size(struct pci_dev *dev) +{ + struct resource *dev_res = &dev->resource[0]; + /* 32bit resource length will calculate size to 0, set it smaller */ + dev_res->end = 0xfffffffe; +} +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MEDIATEK, PCI_DEVICE_ID_MEDIATEK_7629, + mtk_fixup_bar_size); + static const struct of_device_id mtk_pcie_ids[] = { { .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 }, { .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 }, { .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 }, { .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_mt7622 }, + { .compatible = "mediatek,mt7629-pcie", .data = &mtk_pcie_soc_mt7629 }, {}, }; diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 69f0abe1ba1a..77b278bac3a8 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -2126,6 +2126,7 @@ #define PCI_VENDOR_ID_MYRICOM 0x14c1 #define PCI_VENDOR_ID_MEDIATEK 0x14c3 +#define PCI_DEVICE_ID_MEDIATEK_7629 0x7629 #define PCI_VENDOR_ID_TITAN 0x14D2 #define PCI_DEVICE_ID_TITAN_010L 0x8001
MT7629 is an arm platform SoC which has the same PCIe IP with MT7622. The read value of BAR0 is 0xffff_ffff, it's size will be calculated as 4GB in arm64 but bogus alignment values at arm32, the pcie device and devices behind this bridge will not be enabled. Fix it's BAR0 resource size to guarantee the pcie devices will be enabled correctly. The HW default value of its device id is invalid, fix it's device id to match the hardware implementation. Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com> --- drivers/pci/controller/pcie-mediatek.c | 26 ++++++++++++++++++++++++++ include/linux/pci_ids.h | 1 + 2 files changed, 27 insertions(+)