[PATCH v3 0/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[PATCH v3 0/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Gabriele Paoloni]

From: gabriele paoloni <gabriele.paoloni@huawei.com>

This patchset:
1) fixes a bug in spear13xx when calling dw_pcie_cfg_read and
   dw_pcie_cfg_write usign the patch from Pratyush in
   https://lkml.org/lkml/2015/9/7/5
2) reworks dw_pcie_cfg_read/dw_pcie_cfg_write in pcie-designware.c in
   order to make it easier for callers to pass input parameters;
3) adds sanity checks in dw_pcie_cfg_read/dw_pcie_cfg_write to make
   sure the PCI header offset does not conflict with the size of
   the read/written field.

Change from v2:
   Replaced patch 1/3 with Pratyush one in
   https://lkml.org/lkml/2015/9/7/5

gabriele paoloni (3):
  PCIe: SPEAr13xx: fix dw_pcie_cfg_read/write() usage
  PCI: designware: change dw_pcie_cfg_write() and  dw_pcie_cfg_read()
  PCI: designware: add sanity checks on the header offset in
    dw_pcie_cfg_read and dw_pcie_cfg_write

 drivers/pci/host/pci-exynos.c      |  5 ++--
 drivers/pci/host/pci-keystone-dw.c |  4 ++--
 drivers/pci/host/pcie-designware.c | 47 +++++++++++++++++++++++---------------
 drivers/pci/host/pcie-spear13xx.c  | 18 +++++++--------
 4 files changed, 41 insertions(+), 33 deletions(-)

-- 
1.9.1

[PATCH v3 1/3] PCIe: SPEAr13xx: fix dw_pcie_cfg_read/write() usage

[Gabriele Paoloni]

From: gabriele paoloni <gabriele.paoloni@huawei.com>

w_pcie_cfg_read/write() expects 1st argument 'addr' as a 32 bit word
aligned address, 2nd argument 'where' as the offset within that word(0, 1,
2 or 3) and 3rd argument 'size' is the number of bytes need to be read at
the offset(could be 1, 2 ,4).

SPEAr13xx is using dw_pcie_cfg_read() and dw_pcie_cfg_write() incorrectly.
Without this fix, SPEAr13xx host will never work with  few buggy gen1 card
which connects with only gen1 host and also with any endpoint which would
generate a read request of more than 128 bytes.

Cc: stable@vger.kernel.org # v3.17+
Signed-off-by: Pratyush Anand <panand@redhat.com>
Reported-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pcie-spear13xx.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
index 98d2683..0754ea3 100644
--- a/drivers/pci/host/pcie-spear13xx.c
+++ b/drivers/pci/host/pcie-spear13xx.c
@@ -163,34 +163,36 @@ static int spear13xx_pcie_establish_link(struct pcie_port *pp)
 	 * default value in capability register is 512 bytes. So force
 	 * it to 128 here.
 	 */
-	dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, &val);
+	dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_DEVCTL,
+				0, 2, &val);
 	val &= ~PCI_EXP_DEVCTL_READRQ;
-	dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, val);
+	dw_pcie_cfg_write(pp->dbi_base + exp_cap_off + PCI_EXP_DEVCTL,
+				0, 2, val);
 
-	dw_pcie_cfg_write(pp->dbi_base, PCI_VENDOR_ID, 2, 0x104A);
-	dw_pcie_cfg_write(pp->dbi_base, PCI_DEVICE_ID, 2, 0xCD80);
+	dw_pcie_cfg_write(pp->dbi_base + PCI_VENDOR_ID, 0, 2, 0x104A);
+	dw_pcie_cfg_write(pp->dbi_base + PCI_VENDOR_ID, 2, 2, 0xCD80);
 
 	/*
 	 * if is_gen1 is set then handle it, so that some buggy card
 	 * also works
 	 */
 	if (spear13xx_pcie->is_gen1) {
-		dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_LNKCAP, 4,
-				 &val);
+		dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_LNKCAP,
+					0, 4, &val);
 		if ((val & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_2_5GB) {
 			val &= ~((u32)PCI_EXP_LNKCAP_SLS);
 			val |= PCI_EXP_LNKCAP_SLS_2_5GB;
-			dw_pcie_cfg_write(pp->dbi_base, exp_cap_off +
-					  PCI_EXP_LNKCAP, 4, val);
+			dw_pcie_cfg_write(pp->dbi_base + exp_cap_off +
+						PCI_EXP_LNKCAP, 0, 4, val);
 		}
 
-		dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_LNKCTL2, 4,
-				 &val);
+		dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_LNKCTL2,
+					0, 2, &val);
 		if ((val & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_2_5GB) {
 			val &= ~((u32)PCI_EXP_LNKCAP_SLS);
 			val |= PCI_EXP_LNKCAP_SLS_2_5GB;
-			dw_pcie_cfg_write(pp->dbi_base, exp_cap_off +
-					  PCI_EXP_LNKCTL2, 4, val);
+			dw_pcie_cfg_write(pp->dbi_base + exp_cap_off +
+						PCI_EXP_LNKCTL2, 0, 2, val);
 		}
 	}
 
-- 
1.9.1

[PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Gabriele Paoloni]

From: gabriele paoloni <gabriele.paoloni@huawei.com>

This patch changes the implementation of dw_pcie_cfg_read() and
dw_pcie_cfg_write() to improve the function usage from the callers
perspective.
Currently the callers are obliged to pass the 32bit aligned address
of the register that contains the field of the PCI header that they
want to read/write; also they have to pass the offset of the field
in that register. This is quite tricky to use as the callers are
obliged to sum the PCI header base address to the field offset
masked to retrieve the 32b aligned register address.

With the new API the callers have to pass the base address of the
PCI header and the offset corresponding to the field they intend to
read/write.

Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 drivers/pci/host/pci-exynos.c      |  5 ++---
 drivers/pci/host/pci-keystone-dw.c |  4 ++--
 drivers/pci/host/pcie-designware.c | 28 ++++++++++++++--------------
 drivers/pci/host/pcie-spear13xx.c  | 26 ++++++++++++--------------
 4 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
index f9f468d..8b0e04b 100644
--- a/drivers/pci/host/pci-exynos.c
+++ b/drivers/pci/host/pci-exynos.c
@@ -454,7 +454,7 @@ static int exynos_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
 	int ret;
 
 	exynos_pcie_sideband_dbi_r_mode(pp, true);
-	ret = dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, size, val);
+	ret = dw_pcie_cfg_read(pp->dbi_base, where, size, val);
 	exynos_pcie_sideband_dbi_r_mode(pp, false);
 	return ret;
 }
@@ -465,8 +465,7 @@ static int exynos_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
 	int ret;
 
 	exynos_pcie_sideband_dbi_w_mode(pp, true);
-	ret = dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3),
-			where, size, val);
+	ret = dw_pcie_cfg_write(pp->dbi_base, where, size, val);
 	exynos_pcie_sideband_dbi_w_mode(pp, false);
 	return ret;
 }
diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
index e71da99..6648f9be 100644
--- a/drivers/pci/host/pci-keystone-dw.c
+++ b/drivers/pci/host/pci-keystone-dw.c
@@ -398,7 +398,7 @@ int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 
 	addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
 
-	return dw_pcie_cfg_read(addr + (where & ~0x3), where, size, val);
+	return dw_pcie_cfg_read(addr, where, size, val);
 }
 
 int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
@@ -410,7 +410,7 @@ int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 
 	addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
 
-	return dw_pcie_cfg_write(addr + (where & ~0x3), where, size, val);
+	return dw_pcie_cfg_write(addr, where, size, val);
 }
 
 /**
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 52aa6e3..cb23e31 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -82,12 +82,14 @@ static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
 
 int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 {
+	addr += (where & ~0x3);
 	*val = readl(addr);
+	where &= 3;
 
 	if (size == 1)
-		*val = (*val >> (8 * (where & 3))) & 0xff;
+		*val = (*val >> (8 * where)) & 0xff;
 	else if (size == 2)
-		*val = (*val >> (8 * (where & 3))) & 0xffff;
+		*val = (*val >> (8 * where)) & 0xffff;
 	else if (size != 4)
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 
@@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 
 int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
 {
+	addr += where;
+
 	if (size == 4)
 		writel(val, addr);
 	else if (size == 2)
-		writew(val, addr + (where & 2));
+		writew(val, addr);
 	else if (size == 1)
-		writeb(val, addr + (where & 3));
+		writeb(val, addr);
 	else
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 
@@ -132,8 +136,7 @@ static int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
 	if (pp->ops->rd_own_conf)
 		ret = pp->ops->rd_own_conf(pp, where, size, val);
 	else
-		ret = dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where,
-				size, val);
+		ret = dw_pcie_cfg_read(pp->dbi_base, where, size, val);
 
 	return ret;
 }
@@ -146,8 +149,7 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size,
 	if (pp->ops->wr_own_conf)
 		ret = pp->ops->wr_own_conf(pp, where, size, val);
 	else
-		ret = dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where,
-				size, val);
+		ret = dw_pcie_cfg_write(pp->dbi_base, where, size, val);
 
 	return ret;
 }
@@ -539,13 +541,12 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 		u32 devfn, int where, int size, u32 *val)
 {
 	int ret, type;
-	u32 address, busdev, cfg_size;
+	u32 busdev, cfg_size;
 	u64 cpu_addr;
 	void __iomem *va_cfg_base;
 
 	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
 		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
-	address = where & ~0x3;
 
 	if (bus->parent->number == pp->root_bus_nr) {
 		type = PCIE_ATU_TYPE_CFG0;
@@ -562,7 +563,7 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 	dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
 				  type, cpu_addr,
 				  busdev, cfg_size);
-	ret = dw_pcie_cfg_read(va_cfg_base + address, where, size, val);
+	ret = dw_pcie_cfg_read(va_cfg_base, where, size, val);
 	dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
 				  PCIE_ATU_TYPE_IO, pp->io_mod_base,
 				  pp->io_bus_addr, pp->io_size);
@@ -574,13 +575,12 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 		u32 devfn, int where, int size, u32 val)
 {
 	int ret, type;
-	u32 address, busdev, cfg_size;
+	u32 busdev, cfg_size;
 	u64 cpu_addr;
 	void __iomem *va_cfg_base;
 
 	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
 		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
-	address = where & ~0x3;
 
 	if (bus->parent->number == pp->root_bus_nr) {
 		type = PCIE_ATU_TYPE_CFG0;
@@ -597,7 +597,7 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
 	dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
 				  type, cpu_addr,
 				  busdev, cfg_size);
-	ret = dw_pcie_cfg_write(va_cfg_base + address, where, size, val);
+	ret = dw_pcie_cfg_write(va_cfg_base, where, size, val);
 	dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
 				  PCIE_ATU_TYPE_IO, pp->io_mod_base,
 				  pp->io_bus_addr, pp->io_size);
diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
index 0754ea3..cd30d8a 100644
--- a/drivers/pci/host/pcie-spear13xx.c
+++ b/drivers/pci/host/pcie-spear13xx.c
@@ -163,36 +163,34 @@ static int spear13xx_pcie_establish_link(struct pcie_port *pp)
 	 * default value in capability register is 512 bytes. So force
 	 * it to 128 here.
 	 */
-	dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_DEVCTL,
-				0, 2, &val);
+	dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 2, &val);
 	val &= ~PCI_EXP_DEVCTL_READRQ;
-	dw_pcie_cfg_write(pp->dbi_base + exp_cap_off + PCI_EXP_DEVCTL,
-				0, 2, val);
+	dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 2, val);
 
-	dw_pcie_cfg_write(pp->dbi_base + PCI_VENDOR_ID, 0, 2, 0x104A);
-	dw_pcie_cfg_write(pp->dbi_base + PCI_VENDOR_ID, 2, 2, 0xCD80);
+	dw_pcie_cfg_write(pp->dbi_base, PCI_VENDOR_ID, 2, 0x104A);
+	dw_pcie_cfg_write(pp->dbi_base, PCI_DEVICE_ID, 2, 0xCD80);
 
 	/*
 	 * if is_gen1 is set then handle it, so that some buggy card
 	 * also works
 	 */
 	if (spear13xx_pcie->is_gen1) {
-		dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_LNKCAP,
-					0, 4, &val);
+		dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_LNKCAP,
+					4, &val);
 		if ((val & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_2_5GB) {
 			val &= ~((u32)PCI_EXP_LNKCAP_SLS);
 			val |= PCI_EXP_LNKCAP_SLS_2_5GB;
-			dw_pcie_cfg_write(pp->dbi_base + exp_cap_off +
-						PCI_EXP_LNKCAP, 0, 4, val);
+			dw_pcie_cfg_write(pp->dbi_base, exp_cap_off +
+				PCI_EXP_LNKCAP, 4, val);
 		}
 
-		dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_LNKCTL2,
-					0, 2, &val);
+		dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_LNKCTL2,
+					2, &val);
 		if ((val & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_2_5GB) {
 			val &= ~((u32)PCI_EXP_LNKCAP_SLS);
 			val |= PCI_EXP_LNKCAP_SLS_2_5GB;
-			dw_pcie_cfg_write(pp->dbi_base + exp_cap_off +
-						PCI_EXP_LNKCTL2, 0, 2, val);
+			dw_pcie_cfg_write(pp->dbi_base, exp_cap_off +
+					PCI_EXP_LNKCTL2, 2, val);
 		}
 	}
 
-- 
1.9.1

[PATCH v3 3/3] PCI: designware: add sanity checks on the header offset in dw_pcie_cfg_read and dw_pcie_cfg_write

[Gabriele Paoloni]

From: gabriele paoloni <gabriele.paoloni@huawei.com>

This patch adds sanity checks on "where" input parameter in
dw_pcie_cfg_read and dw_pcie_cfg_write. These checks make sure
that offset passed in by the caller is not in conflict with
the size of the PCI header field that is being read/written

Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 drivers/pci/host/pcie-designware.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index cb23e31..6812f69 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -88,9 +88,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
 
 	if (size == 1)
 		*val = (*val >> (8 * where)) & 0xff;
-	else if (size == 2)
+	else if (size == 2) {
+		if (where & 1)
+			return PCIBIOS_BAD_REGISTER_NUMBER;
 		*val = (*val >> (8 * where)) & 0xffff;
-	else if (size != 4)
+	} else if (size == 4) {
+		if (where & 3)
+			return PCIBIOS_BAD_REGISTER_NUMBER;
+	} else
 		return PCIBIOS_BAD_REGISTER_NUMBER;
 
 	return PCIBIOS_SUCCESSFUL;
@@ -100,11 +105,15 @@ int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
 {
 	addr += where;
 
-	if (size == 4)
+	if (size == 4) {
+		if ((uintptr_t)addr & 3)
+			return PCIBIOS_BAD_REGISTER_NUMBER;
 		writel(val, addr);
-	else if (size == 2)
+	} else if (size == 2) {
+		if ((uintptr_t)addr & 1)
+			return PCIBIOS_BAD_REGISTER_NUMBER;
 		writew(val, addr);
-	else if (size == 1)
+	} else if (size == 1)
 		writeb(val, addr);
 	else
 		return PCIBIOS_BAD_REGISTER_NUMBER;
-- 
1.9.1

RE: [PATCH v3 0/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Gabriele Paoloni]

SGkgUHJhdHl1c2guLi5kbyB5b3UgdGhpbmsgdGhlIHBhdGNoc2V0IGlzIGdvb2QgdG8gYmUgYWNr
ZWQtYnk/DQoNClRoYW5rcw0KDQpHYWINCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0K
PiBGcm9tOiBHYWJyaWVsZSBQYW9sb25pDQo+IFNlbnQ6IEZyaWRheSwgU2VwdGVtYmVyIDExLCAy
MDE1IDEwOjM4IEFNDQo+IFRvOiBiaGVsZ2Fhc0Bnb29nbGUuY29tOyBqaW5nb29oYW4xQGdtYWls
LmNvbTsgcHJhdHl1c2guYW5hbmRAZ21haWwuY29tDQo+IENjOiBsaW51eC1wY2lAdmdlci5rZXJu
ZWwub3JnOyBHYWJyaWVsZSBQYW9sb25pOyBXYW5nemhvdSAoQik7DQo+IFl1YW56aGljaGFuZzsg
Wmh1ZGFjYWk7IHpoYW5nanVrdW87IHFpdXpoZW5mYTsgTGlndW96aHUgKEtlbm5ldGgpDQo+IFN1
YmplY3Q6IFtQQVRDSCB2MyAwLzNdIFBDSTogZGVzaWdud2FyZTogY2hhbmdlIGR3X3BjaWVfY2Zn
X3dyaXRlKCkgYW5kDQo+IGR3X3BjaWVfY2ZnX3JlYWQoKQ0KPiANCj4gRnJvbTogZ2FicmllbGUg
cGFvbG9uaSA8Z2FicmllbGUucGFvbG9uaUBodWF3ZWkuY29tPg0KPiANCj4gVGhpcyBwYXRjaHNl
dDoNCj4gMSkgZml4ZXMgYSBidWcgaW4gc3BlYXIxM3h4IHdoZW4gY2FsbGluZyBkd19wY2llX2Nm
Z19yZWFkIGFuZA0KPiAgICBkd19wY2llX2NmZ193cml0ZSB1c2lnbiB0aGUgcGF0Y2ggZnJvbSBQ
cmF0eXVzaCBpbg0KPiAgICBodHRwczovL2xrbWwub3JnL2xrbWwvMjAxNS85LzcvNQ0KPiAyKSBy
ZXdvcmtzIGR3X3BjaWVfY2ZnX3JlYWQvZHdfcGNpZV9jZmdfd3JpdGUgaW4gcGNpZS1kZXNpZ253
YXJlLmMgaW4NCj4gICAgb3JkZXIgdG8gbWFrZSBpdCBlYXNpZXIgZm9yIGNhbGxlcnMgdG8gcGFz
cyBpbnB1dCBwYXJhbWV0ZXJzOw0KPiAzKSBhZGRzIHNhbml0eSBjaGVja3MgaW4gZHdfcGNpZV9j
ZmdfcmVhZC9kd19wY2llX2NmZ193cml0ZSB0byBtYWtlDQo+ICAgIHN1cmUgdGhlIFBDSSBoZWFk
ZXIgb2Zmc2V0IGRvZXMgbm90IGNvbmZsaWN0IHdpdGggdGhlIHNpemUgb2YNCj4gICAgdGhlIHJl
YWQvd3JpdHRlbiBmaWVsZC4NCj4gDQo+IENoYW5nZSBmcm9tIHYyOg0KPiAgICBSZXBsYWNlZCBw
YXRjaCAxLzMgd2l0aCBQcmF0eXVzaCBvbmUgaW4NCj4gICAgaHR0cHM6Ly9sa21sLm9yZy9sa21s
LzIwMTUvOS83LzUNCj4gDQo+IGdhYnJpZWxlIHBhb2xvbmkgKDMpOg0KPiAgIFBDSWU6IFNQRUFy
MTN4eDogZml4IGR3X3BjaWVfY2ZnX3JlYWQvd3JpdGUoKSB1c2FnZQ0KPiAgIFBDSTogZGVzaWdu
d2FyZTogY2hhbmdlIGR3X3BjaWVfY2ZnX3dyaXRlKCkgYW5kICBkd19wY2llX2NmZ19yZWFkKCkN
Cj4gICBQQ0k6IGRlc2lnbndhcmU6IGFkZCBzYW5pdHkgY2hlY2tzIG9uIHRoZSBoZWFkZXIgb2Zm
c2V0IGluDQo+ICAgICBkd19wY2llX2NmZ19yZWFkIGFuZCBkd19wY2llX2NmZ193cml0ZQ0KPiAN
Cj4gIGRyaXZlcnMvcGNpL2hvc3QvcGNpLWV4eW5vcy5jICAgICAgfCAgNSArKy0tDQo+ICBkcml2
ZXJzL3BjaS9ob3N0L3BjaS1rZXlzdG9uZS1kdy5jIHwgIDQgKystLQ0KPiAgZHJpdmVycy9wY2kv
aG9zdC9wY2llLWRlc2lnbndhcmUuYyB8IDQ3ICsrKysrKysrKysrKysrKysrKysrKysrLS0tLS0t
LQ0KPiAtLS0tLS0tLQ0KPiAgZHJpdmVycy9wY2kvaG9zdC9wY2llLXNwZWFyMTN4eC5jICB8IDE4
ICsrKysrKystLS0tLS0tLQ0KPiAgNCBmaWxlcyBjaGFuZ2VkLCA0MSBpbnNlcnRpb25zKCspLCAz
MyBkZWxldGlvbnMoLSkNCj4gDQo+IC0tDQo+IDEuOS4xDQoNCg==

Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Bjorn Helgaas]

On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote:
> From: gabriele paoloni <gabriele.paoloni@huawei.com>
> 
> This patch changes the implementation of dw_pcie_cfg_read() and
> dw_pcie_cfg_write() to improve the function usage from the callers
> perspective.
> Currently the callers are obliged to pass the 32bit aligned address
> of the register that contains the field of the PCI header that they
> want to read/write; also they have to pass the offset of the field
> in that register. This is quite tricky to use as the callers are
> obliged to sum the PCI header base address to the field offset
> masked to retrieve the 32b aligned register address.
> 
> With the new API the callers have to pass the base address of the
> PCI header and the offset corresponding to the field they intend to
> read/write.
> 
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>


>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>  {
> +	addr += (where & ~0x3);
>  	*val = readl(addr);
> +	where &= 3;
>  
>  	if (size == 1)
> -		*val = (*val >> (8 * (where & 3))) & 0xff;
> +		*val = (*val >> (8 * where)) & 0xff;
>  	else if (size == 2)
> -		*val = (*val >> (8 * (where & 3))) & 0xffff;
> +		*val = (*val >> (8 * where)) & 0xffff;
>  	else if (size != 4)
>  		return PCIBIOS_BAD_REGISTER_NUMBER;
>  
> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>  
>  int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
>  {
> +	addr += where;
> +
>  	if (size == 4)
>  		writel(val, addr);
>  	else if (size == 2)
> -		writew(val, addr + (where & 2));
> +		writew(val, addr);
>  	else if (size == 1)
> -		writeb(val, addr + (where & 3));
> +		writeb(val, addr);
>  	else
>  		return PCIBIOS_BAD_REGISTER_NUMBER;

I just noticed the asymmetry between dw_pcie_cfg_read() and
dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit reads and
mask out the parts we won't want, but in dw_pcie_cfg_write() we do 8-, 16-,
or 32-byte writes.

That was there even before your patch, but I wonder why.  Either both
should work the same way, or there should be a comment explaining why they
are different.

Jingoo, Pratyush?

Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Bjorn Helgaas]

On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote:
> From: gabriele paoloni <gabriele.paoloni@huawei.com>
> 
> This patch changes the implementation of dw_pcie_cfg_read() and
> dw_pcie_cfg_write() to improve the function usage from the callers
> perspective.
> Currently the callers are obliged to pass the 32bit aligned address
> of the register that contains the field of the PCI header that they
> want to read/write; also they have to pass the offset of the field
> in that register. This is quite tricky to use as the callers are
> obliged to sum the PCI header base address to the field offset
> masked to retrieve the 32b aligned register address.
> 
> With the new API the callers have to pass the base address of the
> PCI header and the offset corresponding to the field they intend to
> read/write.
> 
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  drivers/pci/host/pci-exynos.c      |  5 ++---
>  drivers/pci/host/pci-keystone-dw.c |  4 ++--
>  drivers/pci/host/pcie-designware.c | 28 ++++++++++++++--------------
>  drivers/pci/host/pcie-spear13xx.c  | 26 ++++++++++++--------------
>  4 files changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> index f9f468d..8b0e04b 100644
> --- a/drivers/pci/host/pci-exynos.c
> +++ b/drivers/pci/host/pci-exynos.c
> @@ -454,7 +454,7 @@ static int exynos_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>  	int ret;
>  
>  	exynos_pcie_sideband_dbi_r_mode(pp, true);
> -	ret = dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, size, val);
> +	ret = dw_pcie_cfg_read(pp->dbi_base, where, size, val);
>  	exynos_pcie_sideband_dbi_r_mode(pp, false);
>  	return ret;
>  }

Is there really any value in keeping "addr" and "where" separate?
dw_pcie_cfg_write() clearly doesn't care; it just adds them together.  I
don't think dw_pcie_cfg_read() needs to care either: it could round the
address down to a 32-bit boundary and use the difference to compute the
mask and shift.

So I'm proposing something like this:

  int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val)
  int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val)

>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>  {
> +	addr += (where & ~0x3);
>  	*val = readl(addr);
> +	where &= 3;
>  
>  	if (size == 1)
> -		*val = (*val >> (8 * (where & 3))) & 0xff;
> +		*val = (*val >> (8 * where)) & 0xff;
>  	else if (size == 2)
> -		*val = (*val >> (8 * (where & 3))) & 0xffff;
> +		*val = (*val >> (8 * where)) & 0xffff;
>  	else if (size != 4)
>  		return PCIBIOS_BAD_REGISTER_NUMBER;
>  
> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>  
>  int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
>  {
> +	addr += where;
> +
>  	if (size == 4)
>  		writel(val, addr);
>  	else if (size == 2)
> -		writew(val, addr + (where & 2));
> +		writew(val, addr);
>  	else if (size == 1)
> -		writeb(val, addr + (where & 3));
> +		writeb(val, addr);
>  	else
>  		return PCIBIOS_BAD_REGISTER_NUMBER;

Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Pratyush Anand]

On Sat, Sep 19, 2015 at 2:16 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote:
>> From: gabriele paoloni <gabriele.paoloni@huawei.com>
>>
>> This patch changes the implementation of dw_pcie_cfg_read() and
>> dw_pcie_cfg_write() to improve the function usage from the callers
>> perspective.
>> Currently the callers are obliged to pass the 32bit aligned address
>> of the register that contains the field of the PCI header that they
>> want to read/write; also they have to pass the offset of the field
>> in that register. This is quite tricky to use as the callers are
>> obliged to sum the PCI header base address to the field offset
>> masked to retrieve the 32b aligned register address.
>>
>> With the new API the callers have to pass the base address of the
>> PCI header and the offset corresponding to the field they intend to
>> read/write.
>>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>
>
>>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>>  {
>> +     addr += (where & ~0x3);
>>       *val = readl(addr);
>> +     where &= 3;
>>
>>       if (size == 1)
>> -             *val = (*val >> (8 * (where & 3))) & 0xff;
>> +             *val = (*val >> (8 * where)) & 0xff;
>>       else if (size == 2)
>> -             *val = (*val >> (8 * (where & 3))) & 0xffff;
>> +             *val = (*val >> (8 * where)) & 0xffff;
>>       else if (size != 4)
>>               return PCIBIOS_BAD_REGISTER_NUMBER;
>>
>> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val)
>>
>>  int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val)
>>  {
>> +     addr += where;
>> +
>>       if (size == 4)
>>               writel(val, addr);
>>       else if (size == 2)
>> -             writew(val, addr + (where & 2));
>> +             writew(val, addr);
>>       else if (size == 1)
>> -             writeb(val, addr + (where & 3));
>> +             writeb(val, addr);
>>       else
>>               return PCIBIOS_BAD_REGISTER_NUMBER;
>
> I just noticed the asymmetry between dw_pcie_cfg_read() and
> dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit reads and
> mask out the parts we won't want, but in dw_pcie_cfg_write() we do 8-, 16-,
> or 32-byte writes.
>
> That was there even before your patch, but I wonder why.  Either both
> should work the same way, or there should be a comment explaining why they
> are different.
>
> Jingoo, Pratyush?

As I said earlier, I just vaguely remember that there was some issue
with a SOC in reading non word aligned addresses.
(1) I do not have any reference for it and (2) even if some where
there could be such issue they can always have platform specific
accessor . So I support your idea and both read and write can be made
symmetric.

Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Pratyush Anand]

On Sat, Sep 19, 2015 at 2:23 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> Is there really any value in keeping "addr" and "where" separate?

Now we do not care of buggy SOCs which could have issues with non word
aligned address read. If there is any such SOC, then they can have
their own accessor.
So, I do not see any value of keeping "where".

> dw_pcie_cfg_write() clearly doesn't care; it just adds them together.  I
> don't think dw_pcie_cfg_read() needs to care either: it could round the
> address down to a 32-bit boundary and use the difference to compute the
> mask and shift.
>
> So I'm proposing something like this:
>
>   int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val)
>   int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val)

Agreed.

~Pratyush

RE: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Gabriele Paoloni]

SGkgZ3V5cyBzb3JyeSBmb3IgdGhlIGxhdGUgcmVwbHkgSSBoYXZlIGJlZW4gT09PIGZvciB0aGUg
bGFzdCA1IGRheXMNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBQcmF0
eXVzaCBBbmFuZCBbbWFpbHRvOnByYXR5dXNoLmFuYW5kQGdtYWlsLmNvbV0NCj4gU2VudDogU2F0
dXJkYXksIFNlcHRlbWJlciAxOSwgMjAxNSA0OjA0IEFNDQo+IFRvOiBCam9ybiBIZWxnYWFzDQo+
IENjOiBHYWJyaWVsZSBQYW9sb25pOyBKaW5nb28gSGFuOyBsaW51eC1wY2lAdmdlci5rZXJuZWwu
b3JnOyBXYW5nemhvdQ0KPiAoQik7IFl1YW56aGljaGFuZzsgWmh1ZGFjYWk7IHpoYW5nanVrdW87
IHFpdXpoZW5mYTsgTGlndW96aHUgKEtlbm5ldGgpDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjMg
Mi8zXSBQQ0k6IGRlc2lnbndhcmU6IGNoYW5nZSBkd19wY2llX2NmZ193cml0ZSgpDQo+IGFuZCBk
d19wY2llX2NmZ19yZWFkKCkNCj4gDQo+IE9uIFNhdCwgU2VwIDE5LCAyMDE1IGF0IDI6MTYgQU0s
IEJqb3JuIEhlbGdhYXMgPGJoZWxnYWFzQGdvb2dsZS5jb20+DQo+IHdyb3RlOg0KPiA+IE9uIEZy
aSwgU2VwIDExLCAyMDE1IGF0IDA1OjM4OjI2UE0gKzA4MDAsIEdhYnJpZWxlIFBhb2xvbmkgd3Jv
dGU6DQo+ID4+IEZyb206IGdhYnJpZWxlIHBhb2xvbmkgPGdhYnJpZWxlLnBhb2xvbmlAaHVhd2Vp
LmNvbT4NCj4gPj4NCj4gPj4gVGhpcyBwYXRjaCBjaGFuZ2VzIHRoZSBpbXBsZW1lbnRhdGlvbiBv
ZiBkd19wY2llX2NmZ19yZWFkKCkgYW5kDQo+ID4+IGR3X3BjaWVfY2ZnX3dyaXRlKCkgdG8gaW1w
cm92ZSB0aGUgZnVuY3Rpb24gdXNhZ2UgZnJvbSB0aGUgY2FsbGVycw0KPiA+PiBwZXJzcGVjdGl2
ZS4NCj4gPj4gQ3VycmVudGx5IHRoZSBjYWxsZXJzIGFyZSBvYmxpZ2VkIHRvIHBhc3MgdGhlIDMy
Yml0IGFsaWduZWQgYWRkcmVzcw0KPiA+PiBvZiB0aGUgcmVnaXN0ZXIgdGhhdCBjb250YWlucyB0
aGUgZmllbGQgb2YgdGhlIFBDSSBoZWFkZXIgdGhhdCB0aGV5DQo+ID4+IHdhbnQgdG8gcmVhZC93
cml0ZTsgYWxzbyB0aGV5IGhhdmUgdG8gcGFzcyB0aGUgb2Zmc2V0IG9mIHRoZSBmaWVsZA0KPiA+
PiBpbiB0aGF0IHJlZ2lzdGVyLiBUaGlzIGlzIHF1aXRlIHRyaWNreSB0byB1c2UgYXMgdGhlIGNh
bGxlcnMgYXJlDQo+ID4+IG9ibGlnZWQgdG8gc3VtIHRoZSBQQ0kgaGVhZGVyIGJhc2UgYWRkcmVz
cyB0byB0aGUgZmllbGQgb2Zmc2V0DQo+ID4+IG1hc2tlZCB0byByZXRyaWV2ZSB0aGUgMzJiIGFs
aWduZWQgcmVnaXN0ZXIgYWRkcmVzcy4NCj4gPj4NCj4gPj4gV2l0aCB0aGUgbmV3IEFQSSB0aGUg
Y2FsbGVycyBoYXZlIHRvIHBhc3MgdGhlIGJhc2UgYWRkcmVzcyBvZiB0aGUNCj4gPj4gUENJIGhl
YWRlciBhbmQgdGhlIG9mZnNldCBjb3JyZXNwb25kaW5nIHRvIHRoZSBmaWVsZCB0aGV5IGludGVu
ZCB0bw0KPiA+PiByZWFkL3dyaXRlLg0KPiA+Pg0KPiA+PiBTaWduZWQtb2ZmLWJ5OiBHYWJyaWVs
ZSBQYW9sb25pIDxnYWJyaWVsZS5wYW9sb25pQGh1YXdlaS5jb20+DQo+ID4NCj4gPg0KPiA+PiAg
aW50IGR3X3BjaWVfY2ZnX3JlYWQodm9pZCBfX2lvbWVtICphZGRyLCBpbnQgd2hlcmUsIGludCBz
aXplLCB1MzINCj4gKnZhbCkNCj4gPj4gIHsNCj4gPj4gKyAgICAgYWRkciArPSAod2hlcmUgJiB+
MHgzKTsNCj4gPj4gICAgICAgKnZhbCA9IHJlYWRsKGFkZHIpOw0KPiA+PiArICAgICB3aGVyZSAm
PSAzOw0KPiA+Pg0KPiA+PiAgICAgICBpZiAoc2l6ZSA9PSAxKQ0KPiA+PiAtICAgICAgICAgICAg
ICp2YWwgPSAoKnZhbCA+PiAoOCAqICh3aGVyZSAmIDMpKSkgJiAweGZmOw0KPiA+PiArICAgICAg
ICAgICAgICp2YWwgPSAoKnZhbCA+PiAoOCAqIHdoZXJlKSkgJiAweGZmOw0KPiA+PiAgICAgICBl
bHNlIGlmIChzaXplID09IDIpDQo+ID4+IC0gICAgICAgICAgICAgKnZhbCA9ICgqdmFsID4+ICg4
ICogKHdoZXJlICYgMykpKSAmIDB4ZmZmZjsNCj4gPj4gKyAgICAgICAgICAgICAqdmFsID0gKCp2
YWwgPj4gKDggKiB3aGVyZSkpICYgMHhmZmZmOw0KPiA+PiAgICAgICBlbHNlIGlmIChzaXplICE9
IDQpDQo+ID4+ICAgICAgICAgICAgICAgcmV0dXJuIFBDSUJJT1NfQkFEX1JFR0lTVEVSX05VTUJF
UjsNCj4gPj4NCj4gPj4gQEAgLTk2LDEyICs5OCwxNCBAQCBpbnQgZHdfcGNpZV9jZmdfcmVhZCh2
b2lkIF9faW9tZW0gKmFkZHIsIGludA0KPiB3aGVyZSwgaW50IHNpemUsIHUzMiAqdmFsKQ0KPiA+
Pg0KPiA+PiAgaW50IGR3X3BjaWVfY2ZnX3dyaXRlKHZvaWQgX19pb21lbSAqYWRkciwgaW50IHdo
ZXJlLCBpbnQgc2l6ZSwgdTMyDQo+IHZhbCkNCj4gPj4gIHsNCj4gPj4gKyAgICAgYWRkciArPSB3
aGVyZTsNCj4gPj4gKw0KPiA+PiAgICAgICBpZiAoc2l6ZSA9PSA0KQ0KPiA+PiAgICAgICAgICAg
ICAgIHdyaXRlbCh2YWwsIGFkZHIpOw0KPiA+PiAgICAgICBlbHNlIGlmIChzaXplID09IDIpDQo+
ID4+IC0gICAgICAgICAgICAgd3JpdGV3KHZhbCwgYWRkciArICh3aGVyZSAmIDIpKTsNCj4gPj4g
KyAgICAgICAgICAgICB3cml0ZXcodmFsLCBhZGRyKTsNCj4gPj4gICAgICAgZWxzZSBpZiAoc2l6
ZSA9PSAxKQ0KPiA+PiAtICAgICAgICAgICAgIHdyaXRlYih2YWwsIGFkZHIgKyAod2hlcmUgJiAz
KSk7DQo+ID4+ICsgICAgICAgICAgICAgd3JpdGViKHZhbCwgYWRkcik7DQo+ID4+ICAgICAgIGVs
c2UNCj4gPj4gICAgICAgICAgICAgICByZXR1cm4gUENJQklPU19CQURfUkVHSVNURVJfTlVNQkVS
Ow0KPiA+DQo+ID4gSSBqdXN0IG5vdGljZWQgdGhlIGFzeW1tZXRyeSBiZXR3ZWVuIGR3X3BjaWVf
Y2ZnX3JlYWQoKSBhbmQNCj4gPiBkd19wY2llX2NmZ193cml0ZSgpOiBpbiBkd19wY2llX2NmZ19y
ZWFkKCkgd2UgYWx3YXlzIGRvIDMyLWJpdCByZWFkcw0KPiBhbmQNCj4gPiBtYXNrIG91dCB0aGUg
cGFydHMgd2Ugd29uJ3Qgd2FudCwgYnV0IGluIGR3X3BjaWVfY2ZnX3dyaXRlKCkgd2UgZG8gOC0s
DQo+IDE2LSwNCj4gPiBvciAzMi1ieXRlIHdyaXRlcy4NCj4gPg0KPiA+IFRoYXQgd2FzIHRoZXJl
IGV2ZW4gYmVmb3JlIHlvdXIgcGF0Y2gsIGJ1dCBJIHdvbmRlciB3aHkuICBFaXRoZXIgYm90aA0K
PiA+IHNob3VsZCB3b3JrIHRoZSBzYW1lIHdheSwgb3IgdGhlcmUgc2hvdWxkIGJlIGEgY29tbWVu
dCBleHBsYWluaW5nIHdoeQ0KPiB0aGV5DQo+ID4gYXJlIGRpZmZlcmVudC4NCj4gPg0KPiA+IEpp
bmdvbywgUHJhdHl1c2g/DQo+IA0KPiBBcyBJIHNhaWQgZWFybGllciwgSSBqdXN0IHZhZ3VlbHkg
cmVtZW1iZXIgdGhhdCB0aGVyZSB3YXMgc29tZSBpc3N1ZQ0KPiB3aXRoIGEgU09DIGluIHJlYWRp
bmcgbm9uIHdvcmQgYWxpZ25lZCBhZGRyZXNzZXMuDQo+ICgxKSBJIGRvIG5vdCBoYXZlIGFueSBy
ZWZlcmVuY2UgZm9yIGl0IGFuZCAoMikgZXZlbiBpZiBzb21lIHdoZXJlDQo+IHRoZXJlIGNvdWxk
IGJlIHN1Y2ggaXNzdWUgdGhleSBjYW4gYWx3YXlzIGhhdmUgcGxhdGZvcm0gc3BlY2lmaWMNCj4g
YWNjZXNzb3IgLiBTbyBJIHN1cHBvcnQgeW91ciBpZGVhIGFuZCBib3RoIHJlYWQgYW5kIHdyaXRl
IGNhbiBiZSBtYWRlDQo+IHN5bW1ldHJpYy4NCg0KSSBhZ3JlZSB3aXRoIHRoZSBpZGVhIGJ1dCwg
d2hhdCBpZiBkb2luZyBzbyB3ZSBicmVhayBvdGhlciBkcml2ZXJzPw0KSXMgdGhlIGNvcnJlY3Qg
ZmxvdyB0byBicmVhayB0aGVtIGZpcnN0IGFuZCBsZXQgdGhlIHNpbmdsZSBtYWludGFpbmVycyBm
aXggdGhlbSB0aGVuLi4uPw0KDQo=

Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Bjorn Helgaas]

On Wed, Sep 23, 2015 at 11:20 AM, Gabriele Paoloni
<gabriele.paoloni@huawei.com> wrote:
> Hi guys sorry for the late reply I have been OOO for the last 5 days
>
>> -----Original Message-----
>> From: Pratyush Anand [mailto:pratyush.anand@gmail.com]
>> Sent: Saturday, September 19, 2015 4:04 AM
>> To: Bjorn Helgaas
>> Cc: Gabriele Paoloni; Jingoo Han; linux-pci@vger.kernel.org; Wangzhou
>> (B); Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth)
>> Subject: Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write()
>> and dw_pcie_cfg_read()
>>
>> On Sat, Sep 19, 2015 at 2:16 AM, Bjorn Helgaas <bhelgaas@google.com>
>> wrote:
>> > On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote:
>> >> From: gabriele paoloni <gabriele.paoloni@huawei.com>
>> >>
>> >> This patch changes the implementation of dw_pcie_cfg_read() and
>> >> dw_pcie_cfg_write() to improve the function usage from the callers
>> >> perspective.
>> >> Currently the callers are obliged to pass the 32bit aligned address
>> >> of the register that contains the field of the PCI header that they
>> >> want to read/write; also they have to pass the offset of the field
>> >> in that register. This is quite tricky to use as the callers are
>> >> obliged to sum the PCI header base address to the field offset
>> >> masked to retrieve the 32b aligned register address.
>> >>
>> >> With the new API the callers have to pass the base address of the
>> >> PCI header and the offset corresponding to the field they intend to
>> >> read/write.
>> >>
>> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> >
>> >
>> >>  int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32
>> *val)
>> >>  {
>> >> +     addr += (where & ~0x3);
>> >>       *val = readl(addr);
>> >> +     where &= 3;
>> >>
>> >>       if (size == 1)
>> >> -             *val = (*val >> (8 * (where & 3))) & 0xff;
>> >> +             *val = (*val >> (8 * where)) & 0xff;
>> >>       else if (size == 2)
>> >> -             *val = (*val >> (8 * (where & 3))) & 0xffff;
>> >> +             *val = (*val >> (8 * where)) & 0xffff;
>> >>       else if (size != 4)
>> >>               return PCIBIOS_BAD_REGISTER_NUMBER;
>> >>
>> >> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int
>> where, int size, u32 *val)
>> >>
>> >>  int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32
>> val)
>> >>  {
>> >> +     addr += where;
>> >> +
>> >>       if (size == 4)
>> >>               writel(val, addr);
>> >>       else if (size == 2)
>> >> -             writew(val, addr + (where & 2));
>> >> +             writew(val, addr);
>> >>       else if (size == 1)
>> >> -             writeb(val, addr + (where & 3));
>> >> +             writeb(val, addr);
>> >>       else
>> >>               return PCIBIOS_BAD_REGISTER_NUMBER;
>> >
>> > I just noticed the asymmetry between dw_pcie_cfg_read() and
>> > dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit reads
>> and
>> > mask out the parts we won't want, but in dw_pcie_cfg_write() we do 8-,
>> 16-,
>> > or 32-byte writes.
>> >
>> > That was there even before your patch, but I wonder why.  Either both
>> > should work the same way, or there should be a comment explaining why
>> they
>> > are different.
>> >
>> > Jingoo, Pratyush?
>>
>> As I said earlier, I just vaguely remember that there was some issue
>> with a SOC in reading non word aligned addresses.
>> (1) I do not have any reference for it and (2) even if some where
>> there could be such issue they can always have platform specific
>> accessor . So I support your idea and both read and write can be made
>> symmetric.
>
> I agree with the idea but, what if doing so we break other drivers?
> Is the correct flow to break them first and let the single maintainers fix them then...?

Since we don't know which (if any) systems would break, I think the
best we can do is see if anybody has objections, then put it in and
see if anything breaks.

Bjorn

RE: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Gabriele Paoloni]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmpvcm4gSGVsZ2FhcyBb
bWFpbHRvOmJoZWxnYWFzQGdvb2dsZS5jb21dDQo+IFNlbnQ6IFdlZG5lc2RheSwgU2VwdGVtYmVy
IDIzLCAyMDE1IDU6MjQgUE0NCj4gVG86IEdhYnJpZWxlIFBhb2xvbmkNCj4gQ2M6IFByYXR5dXNo
IEFuYW5kOyBKaW5nb28gSGFuOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBXYW5nemhvdSAo
Qik7DQo+IFl1YW56aGljaGFuZzsgWmh1ZGFjYWk7IHpoYW5nanVrdW87IHFpdXpoZW5mYTsgTGln
dW96aHUgKEtlbm5ldGgpDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjMgMi8zXSBQQ0k6IGRlc2ln
bndhcmU6IGNoYW5nZSBkd19wY2llX2NmZ193cml0ZSgpDQo+IGFuZCBkd19wY2llX2NmZ19yZWFk
KCkNCj4gDQo+IE9uIFdlZCwgU2VwIDIzLCAyMDE1IGF0IDExOjIwIEFNLCBHYWJyaWVsZSBQYW9s
b25pDQo+IDxnYWJyaWVsZS5wYW9sb25pQGh1YXdlaS5jb20+IHdyb3RlOg0KPiA+IEhpIGd1eXMg
c29ycnkgZm9yIHRoZSBsYXRlIHJlcGx5IEkgaGF2ZSBiZWVuIE9PTyBmb3IgdGhlIGxhc3QgNSBk
YXlzDQo+ID4NCj4gPj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPj4gRnJvbTogUHJh
dHl1c2ggQW5hbmQgW21haWx0bzpwcmF0eXVzaC5hbmFuZEBnbWFpbC5jb21dDQo+ID4+IFNlbnQ6
IFNhdHVyZGF5LCBTZXB0ZW1iZXIgMTksIDIwMTUgNDowNCBBTQ0KPiA+PiBUbzogQmpvcm4gSGVs
Z2Fhcw0KPiA+PiBDYzogR2FicmllbGUgUGFvbG9uaTsgSmluZ29vIEhhbjsgbGludXgtcGNpQHZn
ZXIua2VybmVsLm9yZzsNCj4gV2FuZ3pob3UNCj4gPj4gKEIpOyBZdWFuemhpY2hhbmc7IFpodWRh
Y2FpOyB6aGFuZ2p1a3VvOyBxaXV6aGVuZmE7IExpZ3Vvemh1DQo+IChLZW5uZXRoKQ0KPiA+PiBT
dWJqZWN0OiBSZTogW1BBVENIIHYzIDIvM10gUENJOiBkZXNpZ253YXJlOiBjaGFuZ2UNCj4gZHdf
cGNpZV9jZmdfd3JpdGUoKQ0KPiA+PiBhbmQgZHdfcGNpZV9jZmdfcmVhZCgpDQo+ID4+DQo+ID4+
IE9uIFNhdCwgU2VwIDE5LCAyMDE1IGF0IDI6MTYgQU0sIEJqb3JuIEhlbGdhYXMgPGJoZWxnYWFz
QGdvb2dsZS5jb20+DQo+ID4+IHdyb3RlOg0KPiA+PiA+IE9uIEZyaSwgU2VwIDExLCAyMDE1IGF0
IDA1OjM4OjI2UE0gKzA4MDAsIEdhYnJpZWxlIFBhb2xvbmkgd3JvdGU6DQo+ID4+ID4+IEZyb206
IGdhYnJpZWxlIHBhb2xvbmkgPGdhYnJpZWxlLnBhb2xvbmlAaHVhd2VpLmNvbT4NCj4gPj4gPj4N
Cj4gPj4gPj4gVGhpcyBwYXRjaCBjaGFuZ2VzIHRoZSBpbXBsZW1lbnRhdGlvbiBvZiBkd19wY2ll
X2NmZ19yZWFkKCkgYW5kDQo+ID4+ID4+IGR3X3BjaWVfY2ZnX3dyaXRlKCkgdG8gaW1wcm92ZSB0
aGUgZnVuY3Rpb24gdXNhZ2UgZnJvbSB0aGUNCj4gY2FsbGVycw0KPiA+PiA+PiBwZXJzcGVjdGl2
ZS4NCj4gPj4gPj4gQ3VycmVudGx5IHRoZSBjYWxsZXJzIGFyZSBvYmxpZ2VkIHRvIHBhc3MgdGhl
IDMyYml0IGFsaWduZWQNCj4gYWRkcmVzcw0KPiA+PiA+PiBvZiB0aGUgcmVnaXN0ZXIgdGhhdCBj
b250YWlucyB0aGUgZmllbGQgb2YgdGhlIFBDSSBoZWFkZXIgdGhhdA0KPiB0aGV5DQo+ID4+ID4+
IHdhbnQgdG8gcmVhZC93cml0ZTsgYWxzbyB0aGV5IGhhdmUgdG8gcGFzcyB0aGUgb2Zmc2V0IG9m
IHRoZQ0KPiBmaWVsZA0KPiA+PiA+PiBpbiB0aGF0IHJlZ2lzdGVyLiBUaGlzIGlzIHF1aXRlIHRy
aWNreSB0byB1c2UgYXMgdGhlIGNhbGxlcnMgYXJlDQo+ID4+ID4+IG9ibGlnZWQgdG8gc3VtIHRo
ZSBQQ0kgaGVhZGVyIGJhc2UgYWRkcmVzcyB0byB0aGUgZmllbGQgb2Zmc2V0DQo+ID4+ID4+IG1h
c2tlZCB0byByZXRyaWV2ZSB0aGUgMzJiIGFsaWduZWQgcmVnaXN0ZXIgYWRkcmVzcy4NCj4gPj4g
Pj4NCj4gPj4gPj4gV2l0aCB0aGUgbmV3IEFQSSB0aGUgY2FsbGVycyBoYXZlIHRvIHBhc3MgdGhl
IGJhc2UgYWRkcmVzcyBvZiB0aGUNCj4gPj4gPj4gUENJIGhlYWRlciBhbmQgdGhlIG9mZnNldCBj
b3JyZXNwb25kaW5nIHRvIHRoZSBmaWVsZCB0aGV5IGludGVuZA0KPiB0bw0KPiA+PiA+PiByZWFk
L3dyaXRlLg0KPiA+PiA+Pg0KPiA+PiA+PiBTaWduZWQtb2ZmLWJ5OiBHYWJyaWVsZSBQYW9sb25p
IDxnYWJyaWVsZS5wYW9sb25pQGh1YXdlaS5jb20+DQo+ID4+ID4NCj4gPj4gPg0KPiA+PiA+PiAg
aW50IGR3X3BjaWVfY2ZnX3JlYWQodm9pZCBfX2lvbWVtICphZGRyLCBpbnQgd2hlcmUsIGludCBz
aXplLA0KPiB1MzINCj4gPj4gKnZhbCkNCj4gPj4gPj4gIHsNCj4gPj4gPj4gKyAgICAgYWRkciAr
PSAod2hlcmUgJiB+MHgzKTsNCj4gPj4gPj4gICAgICAgKnZhbCA9IHJlYWRsKGFkZHIpOw0KPiA+
PiA+PiArICAgICB3aGVyZSAmPSAzOw0KPiA+PiA+Pg0KPiA+PiA+PiAgICAgICBpZiAoc2l6ZSA9
PSAxKQ0KPiA+PiA+PiAtICAgICAgICAgICAgICp2YWwgPSAoKnZhbCA+PiAoOCAqICh3aGVyZSAm
IDMpKSkgJiAweGZmOw0KPiA+PiA+PiArICAgICAgICAgICAgICp2YWwgPSAoKnZhbCA+PiAoOCAq
IHdoZXJlKSkgJiAweGZmOw0KPiA+PiA+PiAgICAgICBlbHNlIGlmIChzaXplID09IDIpDQo+ID4+
ID4+IC0gICAgICAgICAgICAgKnZhbCA9ICgqdmFsID4+ICg4ICogKHdoZXJlICYgMykpKSAmIDB4
ZmZmZjsNCj4gPj4gPj4gKyAgICAgICAgICAgICAqdmFsID0gKCp2YWwgPj4gKDggKiB3aGVyZSkp
ICYgMHhmZmZmOw0KPiA+PiA+PiAgICAgICBlbHNlIGlmIChzaXplICE9IDQpDQo+ID4+ID4+ICAg
ICAgICAgICAgICAgcmV0dXJuIFBDSUJJT1NfQkFEX1JFR0lTVEVSX05VTUJFUjsNCj4gPj4gPj4N
Cj4gPj4gPj4gQEAgLTk2LDEyICs5OCwxNCBAQCBpbnQgZHdfcGNpZV9jZmdfcmVhZCh2b2lkIF9f
aW9tZW0gKmFkZHIsIGludA0KPiA+PiB3aGVyZSwgaW50IHNpemUsIHUzMiAqdmFsKQ0KPiA+PiA+
Pg0KPiA+PiA+PiAgaW50IGR3X3BjaWVfY2ZnX3dyaXRlKHZvaWQgX19pb21lbSAqYWRkciwgaW50
IHdoZXJlLCBpbnQgc2l6ZSwNCj4gdTMyDQo+ID4+IHZhbCkNCj4gPj4gPj4gIHsNCj4gPj4gPj4g
KyAgICAgYWRkciArPSB3aGVyZTsNCj4gPj4gPj4gKw0KPiA+PiA+PiAgICAgICBpZiAoc2l6ZSA9
PSA0KQ0KPiA+PiA+PiAgICAgICAgICAgICAgIHdyaXRlbCh2YWwsIGFkZHIpOw0KPiA+PiA+PiAg
ICAgICBlbHNlIGlmIChzaXplID09IDIpDQo+ID4+ID4+IC0gICAgICAgICAgICAgd3JpdGV3KHZh
bCwgYWRkciArICh3aGVyZSAmIDIpKTsNCj4gPj4gPj4gKyAgICAgICAgICAgICB3cml0ZXcodmFs
LCBhZGRyKTsNCj4gPj4gPj4gICAgICAgZWxzZSBpZiAoc2l6ZSA9PSAxKQ0KPiA+PiA+PiAtICAg
ICAgICAgICAgIHdyaXRlYih2YWwsIGFkZHIgKyAod2hlcmUgJiAzKSk7DQo+ID4+ID4+ICsgICAg
ICAgICAgICAgd3JpdGViKHZhbCwgYWRkcik7DQo+ID4+ID4+ICAgICAgIGVsc2UNCj4gPj4gPj4g
ICAgICAgICAgICAgICByZXR1cm4gUENJQklPU19CQURfUkVHSVNURVJfTlVNQkVSOw0KPiA+PiA+
DQo+ID4+ID4gSSBqdXN0IG5vdGljZWQgdGhlIGFzeW1tZXRyeSBiZXR3ZWVuIGR3X3BjaWVfY2Zn
X3JlYWQoKSBhbmQNCj4gPj4gPiBkd19wY2llX2NmZ193cml0ZSgpOiBpbiBkd19wY2llX2NmZ19y
ZWFkKCkgd2UgYWx3YXlzIGRvIDMyLWJpdA0KPiByZWFkcw0KPiA+PiBhbmQNCj4gPj4gPiBtYXNr
IG91dCB0aGUgcGFydHMgd2Ugd29uJ3Qgd2FudCwgYnV0IGluIGR3X3BjaWVfY2ZnX3dyaXRlKCkg
d2UgZG8NCj4gOC0sDQo+ID4+IDE2LSwNCj4gPj4gPiBvciAzMi1ieXRlIHdyaXRlcy4NCj4gPj4g
Pg0KPiA+PiA+IFRoYXQgd2FzIHRoZXJlIGV2ZW4gYmVmb3JlIHlvdXIgcGF0Y2gsIGJ1dCBJIHdv
bmRlciB3aHkuICBFaXRoZXINCj4gYm90aA0KPiA+PiA+IHNob3VsZCB3b3JrIHRoZSBzYW1lIHdh
eSwgb3IgdGhlcmUgc2hvdWxkIGJlIGEgY29tbWVudCBleHBsYWluaW5nDQo+IHdoeQ0KPiA+PiB0
aGV5DQo+ID4+ID4gYXJlIGRpZmZlcmVudC4NCj4gPj4gPg0KPiA+PiA+IEppbmdvbywgUHJhdHl1
c2g/DQo+ID4+DQo+ID4+IEFzIEkgc2FpZCBlYXJsaWVyLCBJIGp1c3QgdmFndWVseSByZW1lbWJl
ciB0aGF0IHRoZXJlIHdhcyBzb21lIGlzc3VlDQo+ID4+IHdpdGggYSBTT0MgaW4gcmVhZGluZyBu
b24gd29yZCBhbGlnbmVkIGFkZHJlc3Nlcy4NCj4gPj4gKDEpIEkgZG8gbm90IGhhdmUgYW55IHJl
ZmVyZW5jZSBmb3IgaXQgYW5kICgyKSBldmVuIGlmIHNvbWUgd2hlcmUNCj4gPj4gdGhlcmUgY291
bGQgYmUgc3VjaCBpc3N1ZSB0aGV5IGNhbiBhbHdheXMgaGF2ZSBwbGF0Zm9ybSBzcGVjaWZpYw0K
PiA+PiBhY2Nlc3NvciAuIFNvIEkgc3VwcG9ydCB5b3VyIGlkZWEgYW5kIGJvdGggcmVhZCBhbmQg
d3JpdGUgY2FuIGJlDQo+IG1hZGUNCj4gPj4gc3ltbWV0cmljLg0KPiA+DQo+ID4gSSBhZ3JlZSB3
aXRoIHRoZSBpZGVhIGJ1dCwgd2hhdCBpZiBkb2luZyBzbyB3ZSBicmVhayBvdGhlciBkcml2ZXJz
Pw0KPiA+IElzIHRoZSBjb3JyZWN0IGZsb3cgdG8gYnJlYWsgdGhlbSBmaXJzdCBhbmQgbGV0IHRo
ZSBzaW5nbGUNCj4gbWFpbnRhaW5lcnMgZml4IHRoZW0gdGhlbi4uLj8NCj4gDQo+IFNpbmNlIHdl
IGRvbid0IGtub3cgd2hpY2ggKGlmIGFueSkgc3lzdGVtcyB3b3VsZCBicmVhaywgSSB0aGluayB0
aGUNCj4gYmVzdCB3ZSBjYW4gZG8gaXMgc2VlIGlmIGFueWJvZHkgaGFzIG9iamVjdGlvbnMsIHRo
ZW4gcHV0IGl0IGluIGFuZA0KPiBzZWUgaWYgYW55dGhpbmcgYnJlYWtzLg0KDQpSaWdodCwgSSds
bCBwdXQgaXQgaW4gdjQNCg0KTWFueSBUaGFua3MNCg0KR2FiDQoNCj4gDQo+IEJqb3JuDQo=

RE: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() and dw_pcie_cfg_read()

[Gabriele Paoloni]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogbGludXgtcGNpLW93bmVy
QHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LXBjaS0NCj4gb3duZXJAdmdlci5rZXJuZWwu
b3JnXSBPbiBCZWhhbGYgT2YgUHJhdHl1c2ggQW5hbmQNCj4gU2VudDogU2F0dXJkYXksIFNlcHRl
bWJlciAxOSwgMjAxNSA0OjExIEFNDQo+IFRvOiBCam9ybiBIZWxnYWFzDQo+IENjOiBHYWJyaWVs
ZSBQYW9sb25pOyBKaW5nb28gSGFuOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBXYW5nemhv
dQ0KPiAoQik7IFl1YW56aGljaGFuZzsgWmh1ZGFjYWk7IHpoYW5nanVrdW87IHFpdXpoZW5mYTsg
TGlndW96aHUgKEtlbm5ldGgpDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjMgMi8zXSBQQ0k6IGRl
c2lnbndhcmU6IGNoYW5nZSBkd19wY2llX2NmZ193cml0ZSgpDQo+IGFuZCBkd19wY2llX2NmZ19y
ZWFkKCkNCj4gDQo+IE9uIFNhdCwgU2VwIDE5LCAyMDE1IGF0IDI6MjMgQU0sIEJqb3JuIEhlbGdh
YXMgPGJoZWxnYWFzQGdvb2dsZS5jb20+DQo+IHdyb3RlOg0KPiA+DQo+ID4gSXMgdGhlcmUgcmVh
bGx5IGFueSB2YWx1ZSBpbiBrZWVwaW5nICJhZGRyIiBhbmQgIndoZXJlIiBzZXBhcmF0ZT8NCj4g
DQo+IE5vdyB3ZSBkbyBub3QgY2FyZSBvZiBidWdneSBTT0NzIHdoaWNoIGNvdWxkIGhhdmUgaXNz
dWVzIHdpdGggbm9uIHdvcmQNCj4gYWxpZ25lZCBhZGRyZXNzIHJlYWQuIElmIHRoZXJlIGlzIGFu
eSBzdWNoIFNPQywgdGhlbiB0aGV5IGNhbiBoYXZlDQo+IHRoZWlyIG93biBhY2Nlc3Nvci4NCj4g
U28sIEkgZG8gbm90IHNlZSBhbnkgdmFsdWUgb2Yga2VlcGluZyAid2hlcmUiLg0KPiANCj4gPiBk
d19wY2llX2NmZ193cml0ZSgpIGNsZWFybHkgZG9lc24ndCBjYXJlOyBpdCBqdXN0IGFkZHMgdGhl
bSB0b2dldGhlci4NCj4gSQ0KPiA+IGRvbid0IHRoaW5rIGR3X3BjaWVfY2ZnX3JlYWQoKSBuZWVk
cyB0byBjYXJlIGVpdGhlcjogaXQgY291bGQgcm91bmQNCj4gdGhlDQo+ID4gYWRkcmVzcyBkb3du
IHRvIGEgMzItYml0IGJvdW5kYXJ5IGFuZCB1c2UgdGhlIGRpZmZlcmVuY2UgdG8gY29tcHV0ZQ0K
PiB0aGUNCj4gPiBtYXNrIGFuZCBzaGlmdC4NCj4gPg0KPiA+IFNvIEknbSBwcm9wb3Npbmcgc29t
ZXRoaW5nIGxpa2UgdGhpczoNCj4gPg0KPiA+ICAgaW50IGR3X3BjaWVfY2ZnX3JlYWQodm9pZCBf
X2lvbWVtICphZGRyLCBpbnQgc2l6ZSwgdTMyICp2YWwpDQo+ID4gICBpbnQgZHdfcGNpZV9jZmdf
d3JpdGUodm9pZCBfX2lvbWVtICphZGRyLCBpbnQgc2l6ZSwgdTMyIHZhbCkNCg0KT2sgQWdyZWVk
DQoNCldpbGwgZG8gaW4gdjQNCg0KR2FiDQoNCj4gDQo+IEFncmVlZC4NCj4gDQo+IH5QcmF0eXVz
aA0KPiAtLQ0KPiBUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAi
dW5zdWJzY3JpYmUgbGludXgtcGNpIiBpbg0KPiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFq
b3Jkb21vQHZnZXIua2VybmVsLm9yZw0KPiBNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8v
dmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwNCg==