On Wed, Apr 29, 2015 at 07:00:30PM +1000, Alexey Kardashevskiy wrote:
> On 04/29/2015 01:25 PM, David Gibson wrote:
> >On Sat, Apr 25, 2015 at 10:14:40PM +1000, Alexey Kardashevskiy wrote:
> >>At the moment the DMA setup code looks for the "ibm,opal-tce-kill" property
> >>which contains the TCE kill register address. Writes to this register
> >>invalidates TCE cache on IODA/IODA2 hub.
> >>
> >>This moves the register address from iommu_table to pnv_ioda_pe as
> >>later there will be 2 tables per PE and it will be used for both tables.
> >>
> >>This moves the property reading/remapping code to a helper to reduce
> >>code duplication.
> >>
> >>This adds a new pnv_pci_ioda2_tvt_invalidate() helper which invalidates
> >>the entire table. It should be called after every call to
> >>opal_pci_map_pe_dma_window(). It was not required before because
> >>there is just a single TCE table and 64bit DMA is handled via bypass
> >>window (which has no table so no chache is used) but this is going
> >>to change with Dynamic DMA windows (DDW).
> >>
> >>Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
> >>---
> >>Changes:
> >>v9:
> >>* new in the series
> >>---
> >> arch/powerpc/platforms/powernv/pci-ioda.c | 69
> >> +++++++++++++++++++------------
> >> arch/powerpc/platforms/powernv/pci.h | 1 +
> >> 2 files changed, 44 insertions(+), 26 deletions(-)
> >>
> >>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>index f070c44..b22b3ca 100644
> >>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>@@ -1672,7 +1672,7 @@ static void pnv_pci_ioda1_tce_invalidate(struct
> >>iommu_table *tbl,
> >> struct pnv_ioda_pe, table_group);
> >> __be64 __iomem *invalidate = rm ?
> >> (__be64 __iomem *)pe->tce_inval_reg_phys :
> >>- (__be64 __iomem *)tbl->it_index;
> >>+ pe->tce_inval_reg;
> >> unsigned long start, end, inc;
> >> const unsigned shift = tbl->it_page_shift;
> >>
> >>@@ -1743,6 +1743,18 @@ static struct iommu_table_ops pnv_ioda1_iommu_ops = {
> >> .get = pnv_tce_get,
> >> };
> >>
> >>+static inline void pnv_pci_ioda2_tvt_invalidate(struct pnv_ioda_pe *pe)
> >>+{
> >>+ /* 01xb - invalidate TCEs that match the specified PE# */
> >>+ unsigned long addr = (0x4ull << 60) | (pe->pe_number & 0xFF);
> >
> >This doesn't really look like an address, but rather the data you're
> >writing to the register.
>
>
> This thing is made of "invalidate operation" (0x4 here), "invalidate
> address" (pci address but it is zero here as we reset everything, most bits
> are here) and "invalidate PE number". So what should I call it? :)
Ah, I see. An address from the hardware point of view, but not so
much from the kernel point of view. Probably just call it 'val' or
'data'.
>
>
>
> >>+ if (!pe->tce_inval_reg)
> >>+ return;
> >>+
> >>+ mb(); /* Ensure above stores are visible */
> >>+ __raw_writeq(cpu_to_be64(addr), pe->tce_inval_reg);
> >>+}
> >>+
> >> static void pnv_pci_ioda2_tce_invalidate(struct iommu_table *tbl,
> >> unsigned long index, unsigned long npages, bool rm)
> >> {
> >>@@ -1751,7 +1763,7 @@ static void pnv_pci_ioda2_tce_invalidate(struct
> >>iommu_table *tbl,
> >> unsigned long start, end, inc;
> >> __be64 __iomem *invalidate = rm ?
> >> (__be64 __iomem *)pe->tce_inval_reg_phys :
> >>- (__be64 __iomem *)tbl->it_index;
> >>+ pe->tce_inval_reg;
> >> const unsigned shift = tbl->it_page_shift;
> >>
> >> /* We'll invalidate DMA address in PE scope */
> >>@@ -1803,13 +1815,31 @@ static struct iommu_table_ops pnv_ioda2_iommu_ops =
> >>{
> >> .get = pnv_tce_get,
> >> };
> >>
> >>+static void pnv_pci_ioda_setup_opal_tce_kill(struct pnv_phb *phb,
> >>+ struct pnv_ioda_pe *pe)
> >>+{
> >>+ const __be64 *swinvp;
> >>+
> >>+ /* OPAL variant of PHB3 invalidated TCEs */
> >>+ swinvp = of_get_property(phb->hose->dn, "ibm,opal-tce-kill", NULL);
> >>+ if (!swinvp)
> >>+ return;
> >>+
> >>+ /* We need a couple more fields -- an address and a data
> >>+ * to or. Since the bus is only printed out on table free
> >>+ * errors, and on the first pass the data will be a relative
> >>+ * bus number, print that out instead.
> >>+ */
> >
> >The comment above appears to have nothing to do with the surrounding code.
>
> I'll just remove it.
Ok, good.
>
>
> >
> >>+ pe->tce_inval_reg_phys = be64_to_cpup(swinvp);
> >>+ pe->tce_inval_reg = ioremap(pe->tce_inval_reg_phys, 8);
> >>+}
> >>+
> >> static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
> >> struct pnv_ioda_pe *pe, unsigned int base,
> >> unsigned int segs)
> >> {
> >>
> >> struct page *tce_mem = NULL;
> >>- const __be64 *swinvp;
> >> struct iommu_table *tbl;
> >> unsigned int i;
> >> int64_t rc;
> >>@@ -1823,6 +1853,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb
> >>*phb,
> >> if (WARN_ON(pe->tce32_seg >= 0))
> >> return;
> >>
> >>+ pnv_pci_ioda_setup_opal_tce_kill(phb, pe);
> >>+
> >> /* Grab a 32-bit TCE table */
> >> pe->tce32_seg = base;
> >> pe_info(pe, " Setting up 32-bit TCE table at %08x..%08x\n",
> >>@@ -1865,20 +1897,11 @@ static void pnv_pci_ioda_setup_dma_pe(struct
> >>pnv_phb *phb,
> >> base << 28, IOMMU_PAGE_SHIFT_4K);
> >>
> >> /* OPAL variant of P7IOC SW invalidated TCEs */
> >>- swinvp = of_get_property(phb->hose->dn, "ibm,opal-tce-kill", NULL);
> >>- if (swinvp) {
> >>- /* We need a couple more fields -- an address and a data
> >>- * to or. Since the bus is only printed out on table free
> >>- * errors, and on the first pass the data will be a relative
> >>- * bus number, print that out instead.
> >>- */
> >
> >.. although I guess it didn't make any more sense in its original context.
> >
> >>- pe->tce_inval_reg_phys = be64_to_cpup(swinvp);
> >>- tbl->it_index = (unsigned long)ioremap(pe->tce_inval_reg_phys,
> >>- 8);
> >>+ if (pe->tce_inval_reg)
> >> tbl->it_type |= (TCE_PCI_SWINV_CREATE |
> >> TCE_PCI_SWINV_FREE |
> >> TCE_PCI_SWINV_PAIR);
> >>- }
> >>+
> >> tbl->it_ops = &pnv_ioda1_iommu_ops;
> >> iommu_init_table(tbl, phb->hose->node);
> >>
> >>@@ -1984,7 +2007,6 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb
> >>*phb,
> >> {
> >> struct page *tce_mem = NULL;
> >> void *addr;
> >>- const __be64 *swinvp;
> >> struct iommu_table *tbl;
> >> unsigned int tce_table_size, end;
> >> int64_t rc;
> >>@@ -1993,6 +2015,8 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb
> >>*phb,
> >> if (WARN_ON(pe->tce32_seg >= 0))
> >> return;
> >>
> >>+ pnv_pci_ioda_setup_opal_tce_kill(phb, pe);
> >>+
> >> /* The PE will reserve all possible 32-bits space */
> >> pe->tce32_seg = 0;
> >> end = (1 << ilog2(phb->ioda.m32_pci_base));
> >>@@ -2023,6 +2047,8 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb
> >>*phb,
> >> goto fail;
> >> }
> >>
> >>+ pnv_pci_ioda2_tvt_invalidate(pe);
> >>+
> >
> >This looks to be a change in behavbiour - if it's replacing a previous
> >invalidation, I'm not seeing where.
>
>
> It is a new thing and the patch adds it. And it does not say anywhere that
> this patch does not change behavior.
Ah, ok, I think I see.
Seems I was even more tired than I realised yesterday and making a
bunch of mistakes while reviewing.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgpFagZYz3ZNl.pgp
Description: PGP signature
