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

Attachment: pgpFagZYz3ZNl.pgp
Description: PGP signature

Reply via email to