| Message ID | 20191004154811.GA31397@monakov-y.office.kontur-niirs.ru |
|---|---|
| State | New |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: keystone: Fix outbound region mapping | expand |
On Fri, Oct 04, 2019 at 06:48:11PM +0300, Yurii Monakov wrote: > PCIe window memory start address should be incremented by OB_WIN_SIZE > megabytes (8 MB) instead of plain OB_WIN_SIZE (8). > > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> > --- > drivers/pci/controller/dwc/pci-keystone.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index af677254a072..f19de60ac991 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) > lower_32_bits(start) | OB_ENABLEN); > ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), > upper_32_bits(start)); > - start += OB_WIN_SIZE; > + start += OB_WIN_SIZE * SZ_1M; This looks fine, however are the earlier lines still correct? val = ilog2(OB_WIN_SIZE); ks_pcie_app_writel(ks_pcie, OB_SIZE, val); Thanks, Andrew Murray > } > > val = ks_pcie_app_readl(ks_pcie, CMD_STATUS); > -- > 2.17.1 >
> This looks fine, however are the earlier lines still correct?
Yes, according to TI Keystone PCIe datasheet pg. 3-10 OB_SIZE
register should hold log2 of actual window size in MB (bits 2-0):
0h = 1MB
1h = 2MB
2h = 4MB
3h = 8MB
But OB_OFFSET_INDEXn/OB_OFFSETn_HI register pair hold absolute
64-bit bus address, so 'start' variable sholud be incremented
by 8M to map all PCIe data space (according to the comment above
the loop).
TI confirms this bug for for kernel v4.14, but since then
some driver code relocation happend and I've decided to
report this here.
Regards,
Yurii
On Fri, Oct 04, 2019 at 07:37:23PM +0300, Yurii Monakov wrote: > > This looks fine, however are the earlier lines still correct? > Yes, according to TI Keystone PCIe datasheet pg. 3-10 OB_SIZE > register should hold log2 of actual window size in MB (bits 2-0): > > 0h = 1MB > 1h = 2MB > 2h = 4MB > 3h = 8MB > > But OB_OFFSET_INDEXn/OB_OFFSETn_HI register pair hold absolute > 64-bit bus address, so 'start' variable sholud be incremented > by 8M to map all PCIe data space (according to the comment above > the loop). > > TI confirms this bug for for kernel v4.14, but since then > some driver code relocation happend and I've decided to > report this here. Thanks for this, I'll add: Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") Acked-by: Andrew Murray <andrew.murray@arm.com> > > Regards, > Yurii >
[+cc Kishon] On Fri, Oct 04, 2019 at 06:48:11PM +0300, Yurii Monakov wrote: > PCIe window memory start address should be incremented by OB_WIN_SIZE > megabytes (8 MB) instead of plain OB_WIN_SIZE (8). > > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> I added: Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") Acked-by: Andrew Murray <andrew.murray@arm.com> Cc: stable@vger.kernel.org # v4.20+ and cc'd Kishon (author of e75043ad9792) and put this on my pci/host-keystone branch for v5.6. Lorenzo may pick this up when he returns. I'd like the commit message to say what this fixes. Currently it just restates the code change, which I can see from the diff. My *guess* is that previously, we could only access 8MB of MMIO space and this patch increases that to 8MB * num_viewport. > --- > drivers/pci/controller/dwc/pci-keystone.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index af677254a072..f19de60ac991 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) > lower_32_bits(start) | OB_ENABLEN); > ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), > upper_32_bits(start)); > - start += OB_WIN_SIZE; > + start += OB_WIN_SIZE * SZ_1M; > } > > val = ks_pcie_app_readl(ks_pcie, CMD_STATUS); > -- > 2.17.1 >
On Tue, 17 Dec 2019 08:31:13 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Kishon] > > On Fri, Oct 04, 2019 at 06:48:11PM +0300, Yurii Monakov wrote: > > PCIe window memory start address should be incremented by OB_WIN_SIZE > > megabytes (8 MB) instead of plain OB_WIN_SIZE (8). > > > > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> > > I added: > > Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") > Acked-by: Andrew Murray <andrew.murray@arm.com> > Cc: stable@vger.kernel.org # v4.20+ > > and cc'd Kishon (author of e75043ad9792) and put this on my > pci/host-keystone branch for v5.6. Lorenzo may pick this up when he > returns. > > I'd like the commit message to say what this fixes. Currently it just > restates the code change, which I can see from the diff. This was my first patch sent to LKML, I'm sorry for inconvenience. Should I take any actions to fix this? > My *guess* is that previously, we could only access 8MB of MMIO space > and this patch increases that to 8MB * num_viewport. Technically you are right. It seems that without this patch all outbound regions were mapped to first 8 MB. But this is obviously a bug, because comment above the loop states that the intent was to map num_ob_windows to linear MMIO space. And prior to e75043ad9792 'start' variable was incremented by 'tr_size = (1 << 3) * SZ_1M'. Best Regards, Yurii Monakov > > --- > > drivers/pci/controller/dwc/pci-keystone.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > > index af677254a072..f19de60ac991 100644 > > --- a/drivers/pci/controller/dwc/pci-keystone.c > > +++ b/drivers/pci/controller/dwc/pci-keystone.c > > @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) > > lower_32_bits(start) | OB_ENABLEN); > > ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), > > upper_32_bits(start)); > > - start += OB_WIN_SIZE; > > + start += OB_WIN_SIZE * SZ_1M; > > } > > > > val = ks_pcie_app_readl(ks_pcie, CMD_STATUS); > > -- > > 2.17.1 > >
On Tue, Dec 17, 2019 at 07:31:31PM +0300, Yurii Monakov wrote: > On Tue, 17 Dec 2019 08:31:13 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > [+cc Kishon] > > > > On Fri, Oct 04, 2019 at 06:48:11PM +0300, Yurii Monakov wrote: > > > PCIe window memory start address should be incremented by OB_WIN_SIZE > > > megabytes (8 MB) instead of plain OB_WIN_SIZE (8). > > > > > > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> > > > > I added: > > > > Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") > > Acked-by: Andrew Murray <andrew.murray@arm.com> > > Cc: stable@vger.kernel.org # v4.20+ > > > > and cc'd Kishon (author of e75043ad9792) and put this on my > > pci/host-keystone branch for v5.6. Lorenzo may pick this up when he > > returns. > > > > I'd like the commit message to say what this fixes. Currently it just > > restates the code change, which I can see from the diff. > This was my first patch sent to LKML, I'm sorry for inconvenience. > Should I take any actions to fix this? Great, welcome! No need for you to do anything; just let me know if I captured this correctly: commit 93c53da177c9 ("PCI: keystone: Fix outbound region mapping") Author: Yurii Monakov <monakov.y@gmail.com> Date: Fri Oct 4 18:48:11 2019 +0300 PCI: keystone: Fix outbound region mapping The Keystone outbound Address Translation Unit (ATU) maps PCI MMIO space in 8 MB windows. When programming the ATU windows, we previously incremented the starting address by 8, not 8 MB, so all the windows were mapped to the first 8 MB. Therefore, only 8 MB of MMIO space was accessible. Update the loop so it increments the starting address by 8 MB, not 8, so more MMIO space is accessible. Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") Link: https://lore.kernel.org/r/20191004154811.GA31397@monakov-y.office.kontur-niirs.ru [bhelgaas: commit log] Signed-off-by: Yurii Monakov <monakov.y@gmail.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Andrew Murray <andrew.murray@arm.com> Cc: stable@vger.kernel.org # v4.20+ diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index af677254a072..f19de60ac991 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) lower_32_bits(start) | OB_ENABLEN); ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), upper_32_bits(start)); - start += OB_WIN_SIZE; + start += OB_WIN_SIZE * SZ_1M; } val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
On Tue, 17 Dec 2019 15:54:36 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Dec 17, 2019 at 07:31:31PM +0300, Yurii Monakov wrote: > > On Tue, 17 Dec 2019 08:31:13 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > [+cc Kishon] > > > > > > On Fri, Oct 04, 2019 at 06:48:11PM +0300, Yurii Monakov wrote: > > > > PCIe window memory start address should be incremented by OB_WIN_SIZE > > > > megabytes (8 MB) instead of plain OB_WIN_SIZE (8). > > > > > > > > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> > > > > > > I added: > > > > > > Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") > > > Acked-by: Andrew Murray <andrew.murray@arm.com> > > > Cc: stable@vger.kernel.org # v4.20+ > > > > > > and cc'd Kishon (author of e75043ad9792) and put this on my > > > pci/host-keystone branch for v5.6. Lorenzo may pick this up when he > > > returns. > > > > > > I'd like the commit message to say what this fixes. Currently it just > > > restates the code change, which I can see from the diff. > > This was my first patch sent to LKML, I'm sorry for inconvenience. > > Should I take any actions to fix this? > > Great, welcome! No need for you to do anything; just let me know if I > captured this correctly: Yes, everything is correct. New commit message perfectly describes this patch. Best Regards, Yurii Monakov > > commit 93c53da177c9 ("PCI: keystone: Fix outbound region mapping") > Author: Yurii Monakov <monakov.y@gmail.com> > Date: Fri Oct 4 18:48:11 2019 +0300 > > PCI: keystone: Fix outbound region mapping > > The Keystone outbound Address Translation Unit (ATU) maps PCI MMIO space in > 8 MB windows. When programming the ATU windows, we previously incremented > the starting address by 8, not 8 MB, so all the windows were mapped to the > first 8 MB. Therefore, only 8 MB of MMIO space was accessible. > > Update the loop so it increments the starting address by 8 MB, not 8, so > more MMIO space is accessible. > > Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") > Link: https://lore.kernel.org/r/20191004154811.GA31397@monakov-y.office.kontur-niirs.ru > [bhelgaas: commit log] > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Acked-by: Andrew Murray <andrew.murray@arm.com> > Cc: stable@vger.kernel.org # v4.20+ > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index af677254a072..f19de60ac991 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) > lower_32_bits(start) | OB_ENABLEN); > ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), > upper_32_bits(start)); > - start += OB_WIN_SIZE; > + start += OB_WIN_SIZE * SZ_1M; > } > > val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
On 18/12/19 7:01 pm, Yurii Monakov wrote: > On Tue, 17 Dec 2019 15:54:36 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote: > >> On Tue, Dec 17, 2019 at 07:31:31PM +0300, Yurii Monakov wrote: >>> On Tue, 17 Dec 2019 08:31:13 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote: >>> >>>> [+cc Kishon] >>>> >>>> On Fri, Oct 04, 2019 at 06:48:11PM +0300, Yurii Monakov wrote: >>>>> PCIe window memory start address should be incremented by OB_WIN_SIZE >>>>> megabytes (8 MB) instead of plain OB_WIN_SIZE (8). >>>>> >>>>> Signed-off-by: Yurii Monakov <monakov.y@gmail.com> >>>> >>>> I added: >>>> >>>> Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") >>>> Acked-by: Andrew Murray <andrew.murray@arm.com> >>>> Cc: stable@vger.kernel.org # v4.20+ >>>> >>>> and cc'd Kishon (author of e75043ad9792) and put this on my >>>> pci/host-keystone branch for v5.6. Lorenzo may pick this up when he >>>> returns. >>>> >>>> I'd like the commit message to say what this fixes. Currently it just >>>> restates the code change, which I can see from the diff. >>> This was my first patch sent to LKML, I'm sorry for inconvenience. >>> Should I take any actions to fix this? >> >> Great, welcome! No need for you to do anything; just let me know if I >> captured this correctly: > Yes, everything is correct. New commit message perfectly describes this patch. Thanks for the patch. FWIW: Acked-by: Kishon Vijay Abraham I <kishon@ti.com> > > Best Regards, > Yurii Monakov > >> >> commit 93c53da177c9 ("PCI: keystone: Fix outbound region mapping") >> Author: Yurii Monakov <monakov.y@gmail.com> >> Date: Fri Oct 4 18:48:11 2019 +0300 >> >> PCI: keystone: Fix outbound region mapping >> >> The Keystone outbound Address Translation Unit (ATU) maps PCI MMIO space in >> 8 MB windows. When programming the ATU windows, we previously incremented >> the starting address by 8, not 8 MB, so all the windows were mapped to the >> first 8 MB. Therefore, only 8 MB of MMIO space was accessible. >> >> Update the loop so it increments the starting address by 8 MB, not 8, so >> more MMIO space is accessible. >> >> Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") >> Link: https://lore.kernel.org/r/20191004154811.GA31397@monakov-y.office.kontur-niirs.ru >> [bhelgaas: commit log] >> Signed-off-by: Yurii Monakov <monakov.y@gmail.com> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> Acked-by: Andrew Murray <andrew.murray@arm.com> >> Cc: stable@vger.kernel.org # v4.20+ >> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >> index af677254a072..f19de60ac991 100644 >> --- a/drivers/pci/controller/dwc/pci-keystone.c >> +++ b/drivers/pci/controller/dwc/pci-keystone.c >> @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) >> lower_32_bits(start) | OB_ENABLEN); >> ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), >> upper_32_bits(start)); >> - start += OB_WIN_SIZE; >> + start += OB_WIN_SIZE * SZ_1M; >> } >> >> val = ks_pcie_app_readl(ks_pcie, CMD_STATUS); >
On Tue, Dec 17, 2019 at 03:54:36PM -0600, Bjorn Helgaas wrote: > On Tue, Dec 17, 2019 at 07:31:31PM +0300, Yurii Monakov wrote: > > On Tue, 17 Dec 2019 08:31:13 -0600, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > [+cc Kishon] > > > > > > On Fri, Oct 04, 2019 at 06:48:11PM +0300, Yurii Monakov wrote: > > > > PCIe window memory start address should be incremented by OB_WIN_SIZE > > > > megabytes (8 MB) instead of plain OB_WIN_SIZE (8). > > > > > > > > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> > > > > > > I added: > > > > > > Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") > > > Acked-by: Andrew Murray <andrew.murray@arm.com> > > > Cc: stable@vger.kernel.org # v4.20+ > > > > > > and cc'd Kishon (author of e75043ad9792) and put this on my > > > pci/host-keystone branch for v5.6. Lorenzo may pick this up when he > > > returns. > > > > > > I'd like the commit message to say what this fixes. Currently it just > > > restates the code change, which I can see from the diff. > > This was my first patch sent to LKML, I'm sorry for inconvenience. > > Should I take any actions to fix this? > > Great, welcome! No need for you to do anything; just let me know if I > captured this correctly: Bjorn, I took your refactored patch below from -next and left your SOB in place because technically you changed the patch. Merged in my pci/keystone branch. Thanks, Lorenzo > commit 93c53da177c9 ("PCI: keystone: Fix outbound region mapping") > Author: Yurii Monakov <monakov.y@gmail.com> > Date: Fri Oct 4 18:48:11 2019 +0300 > > PCI: keystone: Fix outbound region mapping > > The Keystone outbound Address Translation Unit (ATU) maps PCI MMIO space in > 8 MB windows. When programming the ATU windows, we previously incremented > the starting address by 8, not 8 MB, so all the windows were mapped to the > first 8 MB. Therefore, only 8 MB of MMIO space was accessible. > > Update the loop so it increments the starting address by 8 MB, not 8, so > more MMIO space is accessible. > > Fixes: e75043ad9792 ("PCI: keystone: Cleanup outbound window configuration") > Link: https://lore.kernel.org/r/20191004154811.GA31397@monakov-y.office.kontur-niirs.ru > [bhelgaas: commit log] > Signed-off-by: Yurii Monakov <monakov.y@gmail.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Acked-by: Andrew Murray <andrew.murray@arm.com> > Cc: stable@vger.kernel.org # v4.20+ > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index af677254a072..f19de60ac991 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) > lower_32_bits(start) | OB_ENABLEN); > ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), > upper_32_bits(start)); > - start += OB_WIN_SIZE; > + start += OB_WIN_SIZE * SZ_1M; > } > > val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index af677254a072..f19de60ac991 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -422,7 +422,7 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie) lower_32_bits(start) | OB_ENABLEN); ks_pcie_app_writel(ks_pcie, OB_OFFSET_HI(i), upper_32_bits(start)); - start += OB_WIN_SIZE; + start += OB_WIN_SIZE * SZ_1M; } val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
PCIe window memory start address should be incremented by OB_WIN_SIZE megabytes (8 MB) instead of plain OB_WIN_SIZE (8). Signed-off-by: Yurii Monakov <monakov.y@gmail.com> --- drivers/pci/controller/dwc/pci-keystone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)