| Message ID | 20191004132941.6660-1-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2b9f217433e31d125fb697ca7974d3de3ecc3e92 |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] PCI: rcar: Fix incorrect programming of OB windows | expand |
On Fri, Oct 04, 2019 at 02:29:41PM +0100, Andrew Murray wrote: > The outbound windows (PCIEPAUR(x), PCIEPALR(x)) describe a mapping between > a CPU address (which is determined by the window number 'x') and a > programmed PCI address - Thus allowing the controller to translate CPU > accesses into PCI accesses. > > However the existing code incorrectly writes the CPU address - lets fix > this by writing the PCI address instead. > > For memory transactions, existing DT users describe a 1:1 identity mapping > and thus this change should have no effect. However the same isn't true for > I/O. > > Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver") > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > --- > This hasn't been tested, so keen for someone to give it a try. > > Also keen for someone to confirm my understanding that the RCar windows > expect PCI addresses and that res->start refers to CPU addresses. If this > is correct then it's possible the I/O doesn't work correctly. Marek/Yoshihiro - any feedback on this? Thanks, Andrew Murray > --- > drivers/pci/controller/pcie-rcar.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index f6a669a9af41..b28d726b4aba 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -332,11 +332,12 @@ static struct pci_ops rcar_pcie_ops = { > }; > > static void rcar_pcie_setup_window(int win, struct rcar_pcie *pcie, > - struct resource *res) > + struct resource_entry *window) > { > /* Setup PCIe address space mappings for each resource */ > resource_size_t size; > resource_size_t res_start; > + struct resource *res = window->res; > u32 mask; > > rcar_pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win)); > @@ -350,9 +351,9 @@ static void rcar_pcie_setup_window(int win, struct rcar_pcie *pcie, > rcar_pci_write_reg(pcie, mask << 7, PCIEPAMR(win)); > > if (res->flags & IORESOURCE_IO) > - res_start = pci_pio_to_address(res->start); > + res_start = pci_pio_to_address(res->start) - window->offset; > else > - res_start = res->start; > + res_start = res->start - window->offset; > > rcar_pci_write_reg(pcie, upper_32_bits(res_start), PCIEPAUR(win)); > rcar_pci_write_reg(pcie, lower_32_bits(res_start) & ~0x7F, > @@ -381,7 +382,7 @@ static int rcar_pcie_setup(struct list_head *resource, struct rcar_pcie *pci) > switch (resource_type(res)) { > case IORESOURCE_IO: > case IORESOURCE_MEM: > - rcar_pcie_setup_window(i, pci, res); > + rcar_pcie_setup_window(i, pci, win); > i++; > break; > case IORESOURCE_BUS: > -- > 2.21.0 >
On 12/16/19 1:06 PM, Andrew Murray wrote: > On Fri, Oct 04, 2019 at 02:29:41PM +0100, Andrew Murray wrote: >> The outbound windows (PCIEPAUR(x), PCIEPALR(x)) describe a mapping between >> a CPU address (which is determined by the window number 'x') and a >> programmed PCI address - Thus allowing the controller to translate CPU >> accesses into PCI accesses. >> >> However the existing code incorrectly writes the CPU address - lets fix >> this by writing the PCI address instead. >> >> For memory transactions, existing DT users describe a 1:1 identity mapping >> and thus this change should have no effect. However the same isn't true for >> I/O. >> >> Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver") >> Signed-off-by: Andrew Murray <andrew.murray@arm.com> >> >> --- >> This hasn't been tested, so keen for someone to give it a try. >> >> Also keen for someone to confirm my understanding that the RCar windows >> expect PCI addresses and that res->start refers to CPU addresses. If this >> is correct then it's possible the I/O doesn't work correctly. > > Marek/Yoshihiro - any feedback on this? It does indeed look correct, Reviewed-by: Marek Vasut <marek.vasut+renesas@gmail.com> # On R8A77951 Salvator-XS with Intel 8086:f1a5 600P SSD # On R8A77965 Salvator-XS with Intel 8086:10d3 82574L NIC Tested-by: Marek Vasut <marek.vasut+renesas@gmail.com>
On Sat, Feb 08, 2020 at 10:46:25AM +0100, Marek Vasut wrote: > On 12/16/19 1:06 PM, Andrew Murray wrote: > > On Fri, Oct 04, 2019 at 02:29:41PM +0100, Andrew Murray wrote: > >> The outbound windows (PCIEPAUR(x), PCIEPALR(x)) describe a mapping between > >> a CPU address (which is determined by the window number 'x') and a > >> programmed PCI address - Thus allowing the controller to translate CPU > >> accesses into PCI accesses. > >> > >> However the existing code incorrectly writes the CPU address - lets fix > >> this by writing the PCI address instead. > >> > >> For memory transactions, existing DT users describe a 1:1 identity mapping > >> and thus this change should have no effect. However the same isn't true for > >> I/O. > >> > >> Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver") > >> Signed-off-by: Andrew Murray <andrew.murray@arm.com> > >> > >> --- > >> This hasn't been tested, so keen for someone to give it a try. > >> > >> Also keen for someone to confirm my understanding that the RCar windows > >> expect PCI addresses and that res->start refers to CPU addresses. If this > >> is correct then it's possible the I/O doesn't work correctly. > > > > Marek/Yoshihiro - any feedback on this? > > It does indeed look correct, > Reviewed-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > # On R8A77951 Salvator-XS with Intel 8086:f1a5 600P SSD > # On R8A77965 Salvator-XS with Intel 8086:10d3 82574L NIC > Tested-by: Marek Vasut <marek.vasut+renesas@gmail.com> Thanks for testing - much appreciated! Andrew Murray
On 2/8/20 7:41 PM, Andrew Murray wrote: > On Sat, Feb 08, 2020 at 10:46:25AM +0100, Marek Vasut wrote: >> On 12/16/19 1:06 PM, Andrew Murray wrote: >>> On Fri, Oct 04, 2019 at 02:29:41PM +0100, Andrew Murray wrote: >>>> The outbound windows (PCIEPAUR(x), PCIEPALR(x)) describe a mapping between >>>> a CPU address (which is determined by the window number 'x') and a >>>> programmed PCI address - Thus allowing the controller to translate CPU >>>> accesses into PCI accesses. >>>> >>>> However the existing code incorrectly writes the CPU address - lets fix >>>> this by writing the PCI address instead. >>>> >>>> For memory transactions, existing DT users describe a 1:1 identity mapping >>>> and thus this change should have no effect. However the same isn't true for >>>> I/O. >>>> >>>> Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver") >>>> Signed-off-by: Andrew Murray <andrew.murray@arm.com> >>>> >>>> --- >>>> This hasn't been tested, so keen for someone to give it a try. >>>> >>>> Also keen for someone to confirm my understanding that the RCar windows >>>> expect PCI addresses and that res->start refers to CPU addresses. If this >>>> is correct then it's possible the I/O doesn't work correctly. >>> >>> Marek/Yoshihiro - any feedback on this? >> >> It does indeed look correct, >> Reviewed-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> # On R8A77951 Salvator-XS with Intel 8086:f1a5 600P SSD >> # On R8A77965 Salvator-XS with Intel 8086:10d3 82574L NIC >> Tested-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Thanks for testing - much appreciated! > > Andrew Murray Can this be applied then ?
On Fri, Oct 04, 2019 at 02:29:41PM +0100, Andrew Murray wrote: > The outbound windows (PCIEPAUR(x), PCIEPALR(x)) describe a mapping between > a CPU address (which is determined by the window number 'x') and a > programmed PCI address - Thus allowing the controller to translate CPU > accesses into PCI accesses. > > However the existing code incorrectly writes the CPU address - lets fix > this by writing the PCI address instead. > > For memory transactions, existing DT users describe a 1:1 identity mapping > and thus this change should have no effect. However the same isn't true for > I/O. > > Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver") > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > --- > This hasn't been tested, so keen for someone to give it a try. > > Also keen for someone to confirm my understanding that the RCar windows > expect PCI addresses and that res->start refers to CPU addresses. If this > is correct then it's possible the I/O doesn't work correctly. > --- > drivers/pci/controller/pcie-rcar.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) Applied to pci/rcar, thanks ! Lorenzo > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c > index f6a669a9af41..b28d726b4aba 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -332,11 +332,12 @@ static struct pci_ops rcar_pcie_ops = { > }; > > static void rcar_pcie_setup_window(int win, struct rcar_pcie *pcie, > - struct resource *res) > + struct resource_entry *window) > { > /* Setup PCIe address space mappings for each resource */ > resource_size_t size; > resource_size_t res_start; > + struct resource *res = window->res; > u32 mask; > > rcar_pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win)); > @@ -350,9 +351,9 @@ static void rcar_pcie_setup_window(int win, struct rcar_pcie *pcie, > rcar_pci_write_reg(pcie, mask << 7, PCIEPAMR(win)); > > if (res->flags & IORESOURCE_IO) > - res_start = pci_pio_to_address(res->start); > + res_start = pci_pio_to_address(res->start) - window->offset; > else > - res_start = res->start; > + res_start = res->start - window->offset; > > rcar_pci_write_reg(pcie, upper_32_bits(res_start), PCIEPAUR(win)); > rcar_pci_write_reg(pcie, lower_32_bits(res_start) & ~0x7F, > @@ -381,7 +382,7 @@ static int rcar_pcie_setup(struct list_head *resource, struct rcar_pcie *pci) > switch (resource_type(res)) { > case IORESOURCE_IO: > case IORESOURCE_MEM: > - rcar_pcie_setup_window(i, pci, res); > + rcar_pcie_setup_window(i, pci, win); > i++; > break; > case IORESOURCE_BUS: > -- > 2.21.0 >
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index f6a669a9af41..b28d726b4aba 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -332,11 +332,12 @@ static struct pci_ops rcar_pcie_ops = { }; static void rcar_pcie_setup_window(int win, struct rcar_pcie *pcie, - struct resource *res) + struct resource_entry *window) { /* Setup PCIe address space mappings for each resource */ resource_size_t size; resource_size_t res_start; + struct resource *res = window->res; u32 mask; rcar_pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win)); @@ -350,9 +351,9 @@ static void rcar_pcie_setup_window(int win, struct rcar_pcie *pcie, rcar_pci_write_reg(pcie, mask << 7, PCIEPAMR(win)); if (res->flags & IORESOURCE_IO) - res_start = pci_pio_to_address(res->start); + res_start = pci_pio_to_address(res->start) - window->offset; else - res_start = res->start; + res_start = res->start - window->offset; rcar_pci_write_reg(pcie, upper_32_bits(res_start), PCIEPAUR(win)); rcar_pci_write_reg(pcie, lower_32_bits(res_start) & ~0x7F, @@ -381,7 +382,7 @@ static int rcar_pcie_setup(struct list_head *resource, struct rcar_pcie *pci) switch (resource_type(res)) { case IORESOURCE_IO: case IORESOURCE_MEM: - rcar_pcie_setup_window(i, pci, res); + rcar_pcie_setup_window(i, pci, win); i++; break; case IORESOURCE_BUS:
The outbound windows (PCIEPAUR(x), PCIEPALR(x)) describe a mapping between a CPU address (which is determined by the window number 'x') and a programmed PCI address - Thus allowing the controller to translate CPU accesses into PCI accesses. However the existing code incorrectly writes the CPU address - lets fix this by writing the PCI address instead. For memory transactions, existing DT users describe a 1:1 identity mapping and thus this change should have no effect. However the same isn't true for I/O. Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver") Signed-off-by: Andrew Murray <andrew.murray@arm.com> --- This hasn't been tested, so keen for someone to give it a try. Also keen for someone to confirm my understanding that the RCar windows expect PCI addresses and that res->start refers to CPU addresses. If this is correct then it's possible the I/O doesn't work correctly. --- drivers/pci/controller/pcie-rcar.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)