lkml.org 
[lkml]   [2015]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH kernel v9 16/32] powerpc/powernv/ioda: Move TCE kill register address to PE
    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 <aik@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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2015-04-30 07:01    [W:4.619 / U:0.244 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site