[PATCH 0/2] PCI: mediatek: Add support for MT7629

[PATCH 0/2] PCI: mediatek: Add support for MT7629

[Jianjun Wang]

These series patches modify pcie-mediatek.c and dt-bindings compatible
string to support MT7629 PCIe host.

There was something wrong with the mail server's permission setting, sorry
for sent these mails repeatedly.

Jianjun Wang (2):
  dt-bindings: PCI: Add support for MT7629
  PCI: mediatek: Add controller support for MT7629

 .../devicetree/bindings/pci/mediatek-pcie.txt |  1 +
 drivers/pci/controller/pcie-mediatek.c        | 26 +++++++++++++++++++
 include/linux/pci_ids.h                       |  1 +
 3 files changed, 28 insertions(+)

-- 
2.19.1

[PATCH 1/2] dt-bindings: PCI: Add support for MT7629

[Jianjun Wang]

MT7629 is an arm platform Soc which has the same PCIe IP with MT7622.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 Documentation/devicetree/bindings/pci/mediatek-pcie.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek-pcie.txt
index 20227a875ac8..3790c6cc108f 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie.txt
@@ -6,6 +6,7 @@ Required properties:
 	"mediatek,mt2712-pcie"
 	"mediatek,mt7622-pcie"
 	"mediatek,mt7623-pcie"
+	"mediatek,mt7629-pcie"
 - device_type: Must be "pci"
 - reg: Base addresses and lengths of the PCIe subsys and root ports.
 - reg-names: Names of the above areas to use during resource lookup.
-- 
2.19.1

Re: [PATCH 1/2] dt-bindings: PCI: Add support for MT7629

[Rob Herring]

On Thu, 6 Dec 2018 09:09:12 +0800, Jianjun Wang wrote:
> MT7629 is an arm platform Soc which has the same PCIe IP with MT7622.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  Documentation/devicetree/bindings/pci/mediatek-pcie.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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;
+		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
-- 
2.19.1

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Honghui Zhang]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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
> 
> 

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Honghui Zhang]

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
> > 
> > 
> 
> 

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Ryder Lee]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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
> 
> 

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Bjorn Helgaas]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Bjorn Helgaas]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Lorenzo Pieralisi]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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.

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Lorenzo Pieralisi]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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.

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Bjorn Helgaas]

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.
> 

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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.
> > 

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Lorenzo Pieralisi]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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.

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Lorenzo Pieralisi]

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

Re: [PATCH 2/2] PCI: mediatek: Add controller support for MT7629

[Jianjun Wang]

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.

Re: [PATCH 0/2] PCI: mediatek: Add support for MT7629

[Ryder Lee]

On Thu, 2018-12-06 at 09:09 +0800, Jianjun Wang wrote:
> These series patches modify pcie-mediatek.c and dt-bindings compatible
> string to support MT7629 PCIe host.
> 
> There was something wrong with the mail server's permission setting, sorry
> for sent these mails repeatedly.
> 
> Jianjun Wang (2):
>   dt-bindings: PCI: Add support for MT7629
>   PCI: mediatek: Add controller support for MT7629
> 
>  .../devicetree/bindings/pci/mediatek-pcie.txt |  1 +
>  drivers/pci/controller/pcie-mediatek.c        | 26 +++++++++++++++++++
>  include/linux/pci_ids.h                       |  1 +
>  3 files changed, 28 insertions(+)
> 


Acked-by: Ryder Lee <ryder.lee@mediatek.com> for the series.