Message-ID: <20150430041839.GD15238@voom.redhat.com>
Date:	Thu, 30 Apr 2015 14:18:39 +1000
From:	David Gibson <david@...son.dropbear.id.au>
To:	Alexey Kardashevskiy <aik@...abs.ru>
Cc:	linuxppc-dev@...ts.ozlabs.org,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>,
	Alex Williamson <alex.williamson@...hat.com>,
	Gavin Shan <gwshan@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [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@...abs.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

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists