LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH v2 4/4] KVM: PPC: Bookehv: Get vcpu's last instruction for emulation
From: David Laight @ 2014-05-02 10:12 UTC (permalink / raw)
  To: 'Alexander Graf', Mihai Caraman
  Cc: linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org,
	kvm@vger.kernel.org
In-Reply-To: <53636CFA.5050006@suse.de>

RnJvbTogQWxleGFuZGVyIEdyYWYNCi4uLg0KPiA+ICsJcGFnZSA9IHBmbl90b19wYWdlKHBmbik7
DQo+ID4gKwllYWRkciA9ICh1bnNpZ25lZCBsb25nKWttYXBfYXRvbWljKHBhZ2UpOw0KPiA+ICsJ
ZWFkZHIgfD0gYWRkciAmIH5QQUdFX01BU0s7DQo+ID4gKwkqaW5zdHIgPSAqKHUzMiAqKWVhZGRy
Ow0KPiA+ICsJa3VubWFwX2F0b21pYygodTMyICopZWFkZHIpOw0KPiANCj4gSSB0aGluayBJJ2Qg
cmF0aGVyIHdyaXRlIHRoaXMgYXMNCj4gDQo+ICAgICppbnN0ciA9ICoodTMyICopKGVhZGRyIHwg
KGFkZHIgJiB+UEFHRSkpOw0KPiAgICBrdW5tYXBfYXRvbWljKCh2b2lkKillYWRkcik7DQo+IA0K
PiB0byBtYWtlIHN1cmUgd2UgcGFzcyB0aGUgdW5tYXAgZnVuY3Rpb24gdGhlIHNhbWUgdmFsdWUg
d2UgZ290IGZyb20gdGhlDQo+IG1hcCBmdW5jdGlvbi4NCj4gDQo+IE90aGVyd2lzZSBsb29rcyBn
b29kIHRvIG1lLg0KDQpJcyB0aGVyZSBhbnkgbWlsZWFnZSBpbiBrZWVwaW5nIGEgdmlydHVhbCBh
ZGRyZXNzIHBhZ2UgYWxsb2NhdGVkIChwZXIgY3B1KQ0KZm9yIHRoaXMgKGFuZCBzaW1pbGFyKSBh
Y2Nlc3NlcyB0byBwaHlzaWNhbCBtZW1vcnk/DQpOb3QgaGF2aW5nIHRvIHNlYXJjaCBmb3IgYSBm
cmVlIFZBIHBhZ2UgbWlnaHQgc3BlZWQgdGhpbmdzIHVwIChpZiB0aGF0IG1hdHRlcnMpLg0KDQpZ
b3UgYWxzbyBwcm9iYWJseSB3YW50IHRoZSBwYWdlIG1hcHBlZCB1bmNhY2hlZCAtIG5vIHBvaW50
IHBvbGx1dGluZyB0aGUgZGF0YQ0KY2FjaGUuDQoNCglEYXZpZA0KDQo=

^ permalink raw reply

* [PATCH] powerpc: Remove non-uapi linkage.h export
From: James Hogan @ 2014-05-02 10:20 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras
  Cc: David Howells, David Woodhouse, James Hogan, linuxppc-dev,
	Al Viro

The arch/powerpc/include/asm/linkage.h is being unintentionally exported
in the kernel headers since commit e1b5bb6d1236 (consolidate
cond_syscall and SYSCALL_ALIAS declarations) when
arch/powerpc/include/uapi/asm/linkage.h was deleted but the header-y not
removed from the Kbuild file. This happens because Makefile.headersinst
still checks the old asm/ directory if the specified header doesn't
exist in the uapi directory.

The asm/linkage.h shouldn't ever have been exported anyway. No other
arch does and it doesn't contain anything useful to userland, so remove
the header-y line from the Kbuild file which triggers the export.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: David Howells <dhowells@redhat.com>
---
 arch/powerpc/include/uapi/asm/Kbuild | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/include/uapi/asm/Kbuild b/arch/powerpc/include/uapi/asm/Kbuild
index 48be855ef37b..7a3f795ac218 100644
--- a/arch/powerpc/include/uapi/asm/Kbuild
+++ b/arch/powerpc/include/uapi/asm/Kbuild
@@ -15,7 +15,6 @@ header-y += ioctls.h
 header-y += ipcbuf.h
 header-y += kvm.h
 header-y += kvm_para.h
-header-y += linkage.h
 header-y += mman.h
 header-y += msgbuf.h
 header-y += nvram.h
-- 
1.9.2

^ permalink raw reply related

* Re: [PATCH v2 4/4] KVM: PPC: Bookehv: Get vcpu's last instruction for emulation
From: Alexander Graf @ 2014-05-02 11:10 UTC (permalink / raw)
  To: David Laight
  Cc: Mihai Caraman, linuxppc-dev@lists.ozlabs.org,
	kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D0F705051@AcuExch.aculab.com>

On 05/02/2014 12:12 PM, David Laight wrote:
> From: Alexander Graf
> ...
>>> +	page = pfn_to_page(pfn);
>>> +	eaddr = (unsigned long)kmap_atomic(page);
>>> +	eaddr |= addr & ~PAGE_MASK;
>>> +	*instr = *(u32 *)eaddr;
>>> +	kunmap_atomic((u32 *)eaddr);
>> I think I'd rather write this as
>>
>>     *instr = *(u32 *)(eaddr | (addr & ~PAGE));
>>     kunmap_atomic((void*)eaddr);
>>
>> to make sure we pass the unmap function the same value we got from the
>> map function.
>>
>> Otherwise looks good to me.
> Is there any mileage in keeping a virtual address page allocated (per cpu)
> for this (and similar) accesses to physical memory?
> Not having to search for a free VA page might speed things up (if that matters).

I like the idea, though I'm not sure how that would best fit into the 
current memory mapping ecosystem.

> You also probably want the page mapped uncached - no point polluting the data
> cache.

Do e500 chips have a shared I/D cache somewhere? If they do, that 
particular instruction would already be there, so no pollution but nice 
performance.


Alex

^ permalink raw reply

* Re: [PATCH v2 4/4] KVM: PPC: Bookehv: Get vcpu's last instruction for emulation
From: Scott Wood @ 2014-05-02 15:32 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Mihai Caraman, David Laight, linuxppc-dev@lists.ozlabs.org,
	kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
In-Reply-To: <53637D3B.4010205@suse.de>

On Fri, 2014-05-02 at 13:10 +0200, Alexander Graf wrote:
> On 05/02/2014 12:12 PM, David Laight wrote:
> > You also probably want the page mapped uncached - no point polluting the data
> > cache.

We can't do that without creating an architecturally illegal alias
between cacheable and non-cacheable mappings.

> Do e500 chips have a shared I/D cache somewhere?

Yes.  Only L1 is separate.

-Scott

^ permalink raw reply

* Re: Build regressions/improvements in v3.15-rc3
From: Geert Uytterhoeven @ 2014-05-02 15:44 UTC (permalink / raw)
  To: linux-kernel@vger.kernel.org; +Cc: linuxppc-dev@lists.ozlabs.org, uml-devel
In-Reply-To: <1399045268-18344-1-git-send-email-geert@linux-m68k.org>

On Fri, May 2, 2014 at 5:41 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> JFYI, when comparing v3.15-rc3[1]  to v3.15-rc2[3], the summaries are:
>   - build errors: +5/-9

  + /scratch/kisskb/src/arch/um/os-Linux/mem.c: error: 'TMPFS_MAGIC'
undeclared (first use in this function):  => 31
  + /scratch/kisskb/src/arch/um/os-Linux/mem.c: error: (Each
undeclared identifier is reported only once:  => 31
  + /scratch/kisskb/src/arch/um/os-Linux/mem.c: error: for each
function it appears in.):  => 31
  + /scratch/kisskb/src/arch/um/os-Linux/mem.c: error: linux/magic.h:
No such file or directory:  => 16:25

um-defconfig

  + error: No rule to make target include/config/auto.conf:  => N/A

powerpc-randconfig

> [1] http://kisskb.ellerman.id.au/kisskb/head/7427/ (all 119 configs)
> [3] http://kisskb.ellerman.id.au/kisskb/head/7401/ (all 119 configs)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH v4 1/8] DMA: Freescale: remove the unnecessary FSL_DMA_LD_DEBUG
From: Vinod Koul @ 2014-05-02 16:48 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-2-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:44PM +0800, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> Some codes are calling chan_dbg with FSL_DMA_LD_DEBUG surrounded, it is really
> unnecessary to use such a macro because chan_dbg is a wrapper of dev_dbg, we do
> have corresponding DEBUG macro to switch on/off dev_dbg, and most of the other
> codes are also calling chan_dbg directly without using FSL_DMA_LD_DEBUG.
>
Applied, thanks

-- 
~Vinod
 

^ permalink raw reply

* Re: [PATCH v4 3/8] DMA: Freescale: remove attribute DMA_INTERRUPT of dmaengine
From: Vinod Koul @ 2014-05-02 16:49 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-4-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:46PM +0800, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> Delete attribute DMA_INTERRUPT because fsldma doesn't support this function,
> exception will be thrown if talitos is used to offload xor at the same time.
> 
Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH v4 4/8] DMA: Freescale: add fsl_dma_free_descriptor() to reduce code duplication
From: Vinod Koul @ 2014-05-02 16:49 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-5-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:47PM +0800, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> There are several places where descriptors are freed using identical code.
> This patch puts this code into a function to reduce code duplication.
> 
Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH v4 2/8] DMA: Freescale: unify register access methods
From: Vinod Koul @ 2014-05-02 16:48 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-3-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:45PM +0800, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> Methods of accessing DMA controller registers are inconsistent, some registers
> are accessed by DMA_IN/OUT directly, while others are accessed by functions
> get/set_* which are wrappers of DMA_IN/OUT, and even for the BCR register, it
> is read by get_bcr but written by DMA_OUT.
> This patch unifies the inconsistent methods, all registers are accessed by
> get/set_* now.
> 
Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH v4 6/8] DMA: Freescale: change descriptor release process for supporting async_tx
From: Vinod Koul @ 2014-05-02 16:50 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-7-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:49PM +0800, hongbo.zhang@freescale.com wrote:

This need review from Dan ...

-- 
~Vinod
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> Fix the potential risk when enable config NET_DMA and ASYNC_TX. Async_tx is
> lack of support in current release process of dma descriptor, all descriptors
> will be released whatever is acked or no-acked by async_tx, so there is a
> potential race condition when dma engine is uesd by others clients (e.g. when
> enable NET_DMA to offload TCP).
> 
> In our case, a race condition which is raised when use both of talitos and
> dmaengine to offload xor is because napi scheduler will sync all pending
> requests in dma channels, it affects the process of raid operations due to
> ack_tx is not checked in fsl dma. The no-acked descriptor is freed which is
> submitted just now, as a dependent tx, this freed descriptor trigger
> BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
> 
> TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf40000 CPU: 0
> GPR00: 00000001 ecf41ca0 ee44/921a94a0 0000003f 00000001 c00593e4 00000000 00000001
> GPR08: 00000000 a7a7a7a7 00000001 045/920000002 42028042 100a38d4 ed576d98 00000000
> GPR16: ed5a11b0 00000000 2b162000 00000200 046/920000000 2d555000 ed3015e8 c15a7aa0
> GPR24: 00000000 c155fc40 00000000 ecb63220 ecf41d28 e47/92f640bb0 ef640c30 ecf41ca0
> NIP [c02b048c] async_tx_submit+0x6c/0x2b4
> LR [c02b068c] async_tx_submit+0x26c/0x2b4
> Call Trace:
> [ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
> [ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c
> [ecf41d20] [c0421064] async_copy_data+0xa0/0x17c
> [ecf41d70] [c0421cf4] __raid_run_ops+0x874/0xe10
> [ecf41df0] [c0426ee4] handle_stripe+0x820/0x25e8
> [ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
> [ecf41f40] [c04329b8] md_thread+0x138/0x16c
> [ecf41f90] [c008277c] kthread+0x8c/0x90
> [ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
> 
> Another modification in this patch is the change of completed descriptors,
> there is a potential risk which caused by exception interrupt, all descriptors
> in ld_running list are seemed completed when an interrupt raised, it works fine
> under normal condition, but if there is an exception occured, it cannot work as
> our excepted. Hardware should not be depend on s/w list, the right way is to
> read current descriptor address register to find the last completed descriptor.
> If an interrupt is raised by an error, all descriptors in ld_running should not
> be seemed finished, or these unfinished descriptors in ld_running will be
> released wrongly.
> 
> A simple way to reproduce:
> Enable dmatest first, then insert some bad descriptors which can trigger
> Programming Error interrupts before the good descriptors. Last, the good
> descriptors will be freed before they are processsed because of the exception
> intrerrupt.
> 
> Note: the bad descriptors are only for simulating an exception interrupt.  This
> case can illustrate the potential risk in current fsl-dma very well.
> 
> Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>
> Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
>  drivers/dma/fsldma.c |  197 ++++++++++++++++++++++++++++++++++++--------------
>  drivers/dma/fsldma.h |   17 ++++-
>  2 files changed, 159 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index e0fec68..374ca97 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -459,6 +459,88 @@ static struct fsl_desc_sw *fsl_dma_alloc_descriptor(struct fsldma_chan *chan)
>  }
>  
>  /**
> + * fsldma_clean_completed_descriptor - free all descriptors which
> + * has been completed and acked
> + * @chan: Freescale DMA channel
> + *
> + * This function is used on all completed and acked descriptors.
> + * All descriptors should only be freed in this function.
> + */
> +static void fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
> +{
> +	struct fsl_desc_sw *desc, *_desc;
> +
> +	/* Run the callback for each descriptor, in order */
> +	list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node)
> +		if (async_tx_test_ack(&desc->async_tx))
> +			fsl_dma_free_descriptor(chan, desc);
> +}
> +
> +/**
> + * fsldma_run_tx_complete_actions - cleanup a single link descriptor
> + * @chan: Freescale DMA channel
> + * @desc: descriptor to cleanup and free
> + * @cookie: Freescale DMA transaction identifier
> + *
> + * This function is used on a descriptor which has been executed by the DMA
> + * controller. It will run any callbacks, submit any dependencies.
> + */
> +static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,
> +		struct fsl_desc_sw *desc, dma_cookie_t cookie)
> +{
> +	struct dma_async_tx_descriptor *txd = &desc->async_tx;
> +	dma_cookie_t ret = cookie;
> +
> +	BUG_ON(txd->cookie < 0);
> +
> +	if (txd->cookie > 0) {
> +		ret = txd->cookie;
> +
> +		/* Run the link descriptor callback function */
> +		if (txd->callback) {
> +			chan_dbg(chan, "LD %p callback\n", desc);
> +			txd->callback(txd->callback_param);
> +		}
> +	}
> +
> +	/* Run any dependencies */
> +	dma_run_dependencies(txd);
> +
> +	return ret;
> +}
> +
> +/**
> + * fsldma_clean_running_descriptor - move the completed descriptor from
> + * ld_running to ld_completed
> + * @chan: Freescale DMA channel
> + * @desc: the descriptor which is completed
> + *
> + * Free the descriptor directly if acked by async_tx api, or move it to
> + * queue ld_completed.
> + */
> +static void fsldma_clean_running_descriptor(struct fsldma_chan *chan,
> +		struct fsl_desc_sw *desc)
> +{
> +	/* Remove from the list of transactions */
> +	list_del(&desc->node);
> +
> +	/*
> +	 * the client is allowed to attach dependent operations
> +	 * until 'ack' is set
> +	 */
> +	if (!async_tx_test_ack(&desc->async_tx)) {
> +		/*
> +		 * Move this descriptor to the list of descriptors which is
> +		 * completed, but still awaiting the 'ack' bit to be set.
> +		 */
> +		list_add_tail(&desc->node, &chan->ld_completed);
> +		return;
> +	}
> +
> +	dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
> +}
> +
> +/**
>   * fsl_chan_xfer_ld_queue - transfer any pending transactions
>   * @chan : Freescale DMA channel
>   *
> @@ -526,31 +608,58 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
>  }
>  
>  /**
> - * fsldma_cleanup_descriptor - cleanup and free a single link descriptor
> + * fsldma_cleanup_descriptors - cleanup link descriptors which are completed
> + * and move them to ld_completed to free until flag 'ack' is set
>   * @chan: Freescale DMA channel
> - * @desc: descriptor to cleanup and free
>   *
> - * This function is used on a descriptor which has been executed by the DMA
> - * controller. It will run any callbacks, submit any dependencies, and then
> - * free the descriptor.
> + * This function is used on descriptors which have been executed by the DMA
> + * controller. It will run any callbacks, submit any dependencies, then
> + * free these descriptors if flag 'ack' is set.
>   */
> -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
> -				      struct fsl_desc_sw *desc)
> +static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
>  {
> -	struct dma_async_tx_descriptor *txd = &desc->async_tx;
> +	struct fsl_desc_sw *desc, *_desc;
> +	dma_cookie_t cookie = 0;
> +	dma_addr_t curr_phys = get_cdar(chan);
> +	int seen_current = 0;
> +
> +	fsldma_clean_completed_descriptor(chan);
> +
> +	/* Run the callback for each descriptor, in order */
> +	list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> +		/*
> +		 * do not advance past the current descriptor loaded into the
> +		 * hardware channel, subsequent descriptors are either in
> +		 * process or have not been submitted
> +		 */
> +		if (seen_current)
> +			break;
> +
> +		/*
> +		 * stop the search if we reach the current descriptor and the
> +		 * channel is busy
> +		 */
> +		if (desc->async_tx.phys == curr_phys) {
> +			seen_current = 1;
> +			if (!dma_is_idle(chan))
> +				break;
> +		}
> +
> +		cookie = fsldma_run_tx_complete_actions(chan, desc, cookie);
>  
> -	/* Run the link descriptor callback function */
> -	if (txd->callback) {
> -		chan_dbg(chan, "LD %p callback\n", desc);
> -		txd->callback(txd->callback_param);
> +		fsldma_clean_running_descriptor(chan, desc);
>  	}
>  
> -	/* Run any dependencies */
> -	dma_run_dependencies(txd);
> +	/*
> +	 * Start any pending transactions automatically
> +	 *
> +	 * In the ideal case, we keep the DMA controller busy while we go
> +	 * ahead and free the descriptors below.
> +	 */
> +	fsl_chan_xfer_ld_queue(chan);
>  
> -	dma_descriptor_unmap(txd);
> -	chan_dbg(chan, "LD %p free\n", desc);
> -	dma_pool_free(chan->desc_pool, desc, txd->phys);
> +	if (cookie > 0)
> +		chan->common.completed_cookie = cookie;
>  }
>  
>  /**
> @@ -621,8 +730,10 @@ static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
>  
>  	chan_dbg(chan, "free all channel resources\n");
>  	spin_lock_irqsave(&chan->desc_lock, flags);
> +	fsldma_cleanup_descriptors(chan);
>  	fsldma_free_desc_list(chan, &chan->ld_pending);
>  	fsldma_free_desc_list(chan, &chan->ld_running);
> +	fsldma_free_desc_list(chan, &chan->ld_completed);
>  	spin_unlock_irqrestore(&chan->desc_lock, flags);
>  
>  	dma_pool_destroy(chan->desc_pool);
> @@ -860,6 +971,7 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
>  		/* Remove and free all of the descriptors in the LD queue */
>  		fsldma_free_desc_list(chan, &chan->ld_pending);
>  		fsldma_free_desc_list(chan, &chan->ld_running);
> +		fsldma_free_desc_list(chan, &chan->ld_completed);
>  		chan->idle = true;
>  
>  		spin_unlock_irqrestore(&chan->desc_lock, flags);
> @@ -919,6 +1031,17 @@ static enum dma_status fsl_tx_status(struct dma_chan *dchan,
>  					dma_cookie_t cookie,
>  					struct dma_tx_state *txstate)
>  {
> +	struct fsldma_chan *chan = to_fsl_chan(dchan);
> +	enum dma_status ret;
> +
> +	ret = dma_cookie_status(dchan, cookie, txstate);
> +	if (ret == DMA_COMPLETE)
> +		return ret;
> +
> +	spin_lock_bh(&chan->desc_lock);
> +	fsldma_cleanup_descriptors(chan);
> +	spin_unlock_bh(&chan->desc_lock);
> +
>  	return dma_cookie_status(dchan, cookie, txstate);
>  }
>  
> @@ -996,52 +1119,19 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
>  static void dma_do_tasklet(unsigned long data)
>  {
>  	struct fsldma_chan *chan = (struct fsldma_chan *)data;
> -	struct fsl_desc_sw *desc, *_desc;
> -	LIST_HEAD(ld_cleanup);
>  	unsigned long flags;
>  
>  	chan_dbg(chan, "tasklet entry\n");
>  
>  	spin_lock_irqsave(&chan->desc_lock, flags);
>  
> -	/* update the cookie if we have some descriptors to cleanup */
> -	if (!list_empty(&chan->ld_running)) {
> -		dma_cookie_t cookie;
> -
> -		desc = to_fsl_desc(chan->ld_running.prev);
> -		cookie = desc->async_tx.cookie;
> -		dma_cookie_complete(&desc->async_tx);
> -
> -		chan_dbg(chan, "completed_cookie=%d\n", cookie);
> -	}
> -
> -	/*
> -	 * move the descriptors to a temporary list so we can drop the lock
> -	 * during the entire cleanup operation
> -	 */
> -	list_splice_tail_init(&chan->ld_running, &ld_cleanup);
> -
>  	/* the hardware is now idle and ready for more */
>  	chan->idle = true;
>  
> -	/*
> -	 * Start any pending transactions automatically
> -	 *
> -	 * In the ideal case, we keep the DMA controller busy while we go
> -	 * ahead and free the descriptors below.
> -	 */
> -	fsl_chan_xfer_ld_queue(chan);
> -	spin_unlock_irqrestore(&chan->desc_lock, flags);
> -
> -	/* Run the callback for each descriptor, in order */
> -	list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
> -
> -		/* Remove from the list of transactions */
> -		list_del(&desc->node);
> +	/* Run all cleanup for descriptors which have been completed */
> +	fsldma_cleanup_descriptors(chan);
>  
> -		/* Run all cleanup for this descriptor */
> -		fsldma_cleanup_descriptor(chan, desc);
> -	}
> +	spin_unlock_irqrestore(&chan->desc_lock, flags);
>  
>  	chan_dbg(chan, "tasklet exit\n");
>  }
> @@ -1225,6 +1315,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
>  	spin_lock_init(&chan->desc_lock);
>  	INIT_LIST_HEAD(&chan->ld_pending);
>  	INIT_LIST_HEAD(&chan->ld_running);
> +	INIT_LIST_HEAD(&chan->ld_completed);
>  	chan->idle = true;
>  
>  	chan->common.device = &fdev->common;
> diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
> index d56e835..ec19517 100644
> --- a/drivers/dma/fsldma.h
> +++ b/drivers/dma/fsldma.h
> @@ -138,8 +138,21 @@ struct fsldma_chan {
>  	char name[8];			/* Channel name */
>  	struct fsldma_chan_regs __iomem *regs;
>  	spinlock_t desc_lock;		/* Descriptor operation lock */
> -	struct list_head ld_pending;	/* Link descriptors queue */
> -	struct list_head ld_running;	/* Link descriptors queue */
> +	/*
> +	 * Descriptors which are queued to run, but have not yet been
> +	 * submitted to the hardware for execution
> +	 */
> +	struct list_head ld_pending;
> +	/*
> +	 * Descriptors which are currently being executed by the hardware
> +	 */
> +	struct list_head ld_running;
> +	/*
> +	 * Descriptors which have finished execution by the hardware. These
> +	 * descriptors have already had their cleanup actions run. They are
> +	 * waiting for the ACK bit to be set by the async_tx API.
> +	 */
> +	struct list_head ld_completed;	/* Link descriptors queue */
>  	struct dma_chan common;		/* DMA common channel */
>  	struct dma_pool *desc_pool;	/* Descriptors pool */
>  	struct device *dev;		/* Channel device */
> -- 
> 1.7.9.5
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

^ permalink raw reply

* Re: [PATCH v4 7/8] DMA: Freescale: use spin_lock_bh instead of spin_lock_irqsave
From: Vinod Koul @ 2014-05-02 16:51 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-8-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:50PM +0800, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> The usage of spin_lock_irqsave() is a stronger locking mechanism than is
> required throughout the driver. The minimum locking required should be used
> instead. Interrupts will be turned off and context will be saved, it is
> unnecessary to use irqsave.
> 
> This patch changes all instances of spin_lock_irqsave() to spin_lock_bh(). All
> manipulation of protected fields is done using tasklet context or weaker, which
> makes spin_lock_bh() the correct choice.
>

This doesnt apply, perhpas due to depends on 6/8

-- 
~Vinod
 
> Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>
> Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> ---
>  drivers/dma/fsldma.c |   25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 374ca97..6e1c9b3 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -396,10 +396,9 @@ static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  	struct fsldma_chan *chan = to_fsl_chan(tx->chan);
>  	struct fsl_desc_sw *desc = tx_to_fsl_desc(tx);
>  	struct fsl_desc_sw *child;
> -	unsigned long flags;
>  	dma_cookie_t cookie = -EINVAL;
>  
> -	spin_lock_irqsave(&chan->desc_lock, flags);
> +	spin_lock_bh(&chan->desc_lock);
>  
>  	/*
>  	 * assign cookies to all of the software descriptors
> @@ -412,7 +411,7 @@ static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  	/* put this transaction onto the tail of the pending queue */
>  	append_ld_queue(chan, desc);
>  
> -	spin_unlock_irqrestore(&chan->desc_lock, flags);
> +	spin_unlock_bh(&chan->desc_lock);
>  
>  	return cookie;
>  }
> @@ -726,15 +725,14 @@ static void fsldma_free_desc_list_reverse(struct fsldma_chan *chan,
>  static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
>  {
>  	struct fsldma_chan *chan = to_fsl_chan(dchan);
> -	unsigned long flags;
>  
>  	chan_dbg(chan, "free all channel resources\n");
> -	spin_lock_irqsave(&chan->desc_lock, flags);
> +	spin_lock_bh(&chan->desc_lock);
>  	fsldma_cleanup_descriptors(chan);
>  	fsldma_free_desc_list(chan, &chan->ld_pending);
>  	fsldma_free_desc_list(chan, &chan->ld_running);
>  	fsldma_free_desc_list(chan, &chan->ld_completed);
> -	spin_unlock_irqrestore(&chan->desc_lock, flags);
> +	spin_unlock_bh(&chan->desc_lock);
>  
>  	dma_pool_destroy(chan->desc_pool);
>  	chan->desc_pool = NULL;
> @@ -953,7 +951,6 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
>  {
>  	struct dma_slave_config *config;
>  	struct fsldma_chan *chan;
> -	unsigned long flags;
>  	int size;
>  
>  	if (!dchan)
> @@ -963,7 +960,7 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
>  
>  	switch (cmd) {
>  	case DMA_TERMINATE_ALL:
> -		spin_lock_irqsave(&chan->desc_lock, flags);
> +		spin_lock_bh(&chan->desc_lock);
>  
>  		/* Halt the DMA engine */
>  		dma_halt(chan);
> @@ -974,7 +971,7 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
>  		fsldma_free_desc_list(chan, &chan->ld_completed);
>  		chan->idle = true;
>  
> -		spin_unlock_irqrestore(&chan->desc_lock, flags);
> +		spin_unlock_bh(&chan->desc_lock);
>  		return 0;
>  
>  	case DMA_SLAVE_CONFIG:
> @@ -1016,11 +1013,10 @@ static int fsl_dma_device_control(struct dma_chan *dchan,
>  static void fsl_dma_memcpy_issue_pending(struct dma_chan *dchan)
>  {
>  	struct fsldma_chan *chan = to_fsl_chan(dchan);
> -	unsigned long flags;
>  
> -	spin_lock_irqsave(&chan->desc_lock, flags);
> +	spin_lock_bh(&chan->desc_lock);
>  	fsl_chan_xfer_ld_queue(chan);
> -	spin_unlock_irqrestore(&chan->desc_lock, flags);
> +	spin_unlock_bh(&chan->desc_lock);
>  }
>  
>  /**
> @@ -1119,11 +1115,10 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
>  static void dma_do_tasklet(unsigned long data)
>  {
>  	struct fsldma_chan *chan = (struct fsldma_chan *)data;
> -	unsigned long flags;
>  
>  	chan_dbg(chan, "tasklet entry\n");
>  
> -	spin_lock_irqsave(&chan->desc_lock, flags);
> +	spin_lock_bh(&chan->desc_lock);
>  
>  	/* the hardware is now idle and ready for more */
>  	chan->idle = true;
> @@ -1131,7 +1126,7 @@ static void dma_do_tasklet(unsigned long data)
>  	/* Run all cleanup for descriptors which have been completed */
>  	fsldma_cleanup_descriptors(chan);
>  
> -	spin_unlock_irqrestore(&chan->desc_lock, flags);
> +	spin_unlock_bh(&chan->desc_lock);
>  
>  	chan_dbg(chan, "tasklet exit\n");
>  }
> -- 
> 1.7.9.5
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

^ permalink raw reply

* Re: [PATCH v4 5/8] DMA: Freescale: move functions to avoid forward declarations
From: Vinod Koul @ 2014-05-02 16:50 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-6-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:48PM +0800, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> These functions will be modified in the next patch in the series. By moving the
> function in a patch separate from the changes, it will make review easier.
>
Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH v4 8/8] DMA: Freescale: add suspend resume functions for DMA driver
From: Vinod Koul @ 2014-05-02 16:46 UTC (permalink / raw)
  To: hongbo.zhang
  Cc: leo.li, vkoul, linux-kernel, scottwood, dmaengine, dan.j.williams,
	linuxppc-dev
In-Reply-To: <1397809071-5353-9-git-send-email-hongbo.zhang@freescale.com>

On Fri, Apr 18, 2014 at 04:17:51PM +0800, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> This patch adds suspend resume functions for Freescale DMA driver.
> .prepare callback is used to stop further descriptors from being added into the
> pending queue, and also issue pending queues into execution if there is any.
> .suspend callback makes sure all the pending jobs are cleaned up and all the
> channels are idle, and save the mode registers.
> .resume callback re-initializes the channels by restore the mode registers.
> 

> +
> +static const struct dev_pm_ops fsldma_pm_ops = {
> +	.prepare	= fsldma_prepare,
> +	.suspend	= fsldma_suspend,
> +	.resume		= fsldma_resume,
> +};
I think this is not correct. We discussed this sometime back on list. The
DMAengine drivers should use late resume and early suspend to ensure they get
suspended after clients (who should use normal ones) and resume before them

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH RFC v12 3/7] dma: mpc512x: add support for peripheral transfers
From: Vinod Koul @ 2014-05-02 17:03 UTC (permalink / raw)
  To: Alexander Popov
  Cc: Lars-Peter Clausen, Arnd Bergmann, Gerhard Sittig,
	Andy Shevchenko, dmaengine, Dan Williams, Anatolij Gustschin,
	linuxppc-dev
In-Reply-To: <1398261209-5578-4-git-send-email-a13xp0p0v88@gmail.com>

On Wed, Apr 23, 2014 at 05:53:25PM +0400, Alexander Popov wrote:
> Introduce support for slave s/g transfer preparation and the associated
> device control callback in the MPC512x DMA controller driver, which adds
> support for data transfers between memory and peripheral I/O to the
> previously supported mem-to-mem transfers.
> 
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> ---
>  drivers/dma/mpc512x_dma.c | 239 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 234 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
> index 0b17f4d..ca2fe03 100644
> --- a/drivers/dma/mpc512x_dma.c
> +++ b/drivers/dma/mpc512x_dma.c
> @@ -2,6 +2,7 @@
>   * Copyright (C) Freescale Semicondutor, Inc. 2007, 2008.
>   * Copyright (C) Semihalf 2009
>   * Copyright (C) Ilya Yanok, Emcraft Systems 2010
> + * Copyright (C) Alexander Popov, Promcontroller 2014
>   *
>   * Written by Piotr Ziecik <kosmo@semihalf.com>. Hardware description
>   * (defines, structures and comments) was taken from MPC5121 DMA driver
> @@ -29,8 +30,17 @@
>   */
>  
>  /*
> - * This is initial version of MPC5121 DMA driver. Only memory to memory
> - * transfers are supported (tested using dmatest module).
> + * MPC512x and MPC8308 DMA driver. It supports
> + * memory to memory data transfers (tested using dmatest module) and
> + * data transfers between memory and peripheral I/O memory
> + * by means of slave s/g with these limitations:
> + *  - chunked transfers (transfers with more than one part) are refused
> + *     as long as proper support for scatter/gather is missing;
> + *  - transfers on MPC8308 always start from software as this SoC appears
> + *     not to have external request lines for peripheral flow control;
> + *  - minimal memory <-> I/O memory transfer chunk is 4 bytes and consequently
> + *     source and destination addresses must be 4-byte aligned
> + *     and transfer size must be aligned on (4 * maxburst) boundary;
>   */
>  
>  #include <linux/module.h>
> @@ -189,6 +199,7 @@ struct mpc_dma_desc {
>  	dma_addr_t			tcd_paddr;
>  	int				error;
>  	struct list_head		node;
> +	int				will_access_peripheral;
>  };
>  
>  struct mpc_dma_chan {
> @@ -201,6 +212,12 @@ struct mpc_dma_chan {
>  	struct mpc_dma_tcd		*tcd;
>  	dma_addr_t			tcd_paddr;
>  
> +	/* Settings for access to peripheral FIFO */
> +	dma_addr_t			src_per_paddr;
> +	u32				src_tcd_nunits;
> +	dma_addr_t			dst_per_paddr;
> +	u32				dst_tcd_nunits;
> +
>  	/* Lock for this structure */
>  	spinlock_t			lock;
>  };
> @@ -251,8 +268,23 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
>  	struct mpc_dma_desc *mdesc;
>  	int cid = mchan->chan.chan_id;
>  
> -	/* Move all queued descriptors to active list */
> -	list_splice_tail_init(&mchan->queued, &mchan->active);
> +	while (!list_empty(&mchan->queued)) {
> +		mdesc = list_first_entry(&mchan->queued,
> +						struct mpc_dma_desc, node);
> +		/*
> +		 * Grab either several mem-to-mem transfer descriptors
> +		 * or one peripheral transfer descriptor,
> +		 * don't mix mem-to-mem and peripheral transfer descriptors
> +		 * within the same 'active' list.
> +		 */
> +		if (mdesc->will_access_peripheral) {
> +			if (list_empty(&mchan->active))
> +				list_move_tail(&mdesc->node, &mchan->active);
> +			break;
> +		} else {
> +			list_move_tail(&mdesc->node, &mchan->active);
> +		}
> +	}
>  
>  	/* Chain descriptors into one transaction */
>  	list_for_each_entry(mdesc, &mchan->active, node) {
> @@ -278,7 +310,17 @@ static void mpc_dma_execute(struct mpc_dma_chan *mchan)
>  
>  	if (first != prev)
>  		mdma->tcd[cid].e_sg = 1;
> -	out_8(&mdma->regs->dmassrt, cid);
> +
> +	if (mdma->is_mpc8308) {
> +		/* MPC8308, no request lines, software initiated start */
> +		out_8(&mdma->regs->dmassrt, cid);
> +	} else if (first->will_access_peripheral) {
> +		/* Peripherals involved, start by external request signal */
> +		out_8(&mdma->regs->dmaserq, cid);
> +	} else {
> +		/* Memory to memory transfer, software initiated start */
> +		out_8(&mdma->regs->dmassrt, cid);
> +	}
>  }
>  
>  /* Handle interrupt on one half of DMA controller (32 channels) */
> @@ -596,6 +638,7 @@ mpc_dma_prep_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
>  	}
>  
>  	mdesc->error = 0;
> +	mdesc->will_access_peripheral = 0;
>  	tcd = mdesc->tcd;
>  
>  	/* Prepare Transfer Control Descriptor for this transaction */
> @@ -643,6 +686,189 @@ mpc_dma_prep_memcpy(struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
>  	return &mdesc->desc;
>  }
>  
> +static struct dma_async_tx_descriptor *
> +mpc_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_transfer_direction direction,
> +		unsigned long flags, void *context)
> +{
> +	struct mpc_dma *mdma = dma_chan_to_mpc_dma(chan);
> +	struct mpc_dma_chan *mchan = dma_chan_to_mpc_dma_chan(chan);
> +	struct mpc_dma_desc *mdesc = NULL;
> +	dma_addr_t per_paddr;
> +	u32 tcd_nunits;
> +	struct mpc_dma_tcd *tcd;
> +	unsigned long iflags;
> +	struct scatterlist *sg;
> +	size_t len;
> +	int iter, i;
> +
> +	/* Currently there is no proper support for scatter/gather */
Why? Is this HW or SW limitation?

> +	if (sg_len != 1)
> +		return NULL;
> +
> +	if (!is_slave_direction(direction))
> +		return NULL;
> +
> +	for_each_sg(sgl, sg, sg_len, i) {
Then this doesn't make sense...

> +		spin_lock_irqsave(&mchan->lock, iflags);
> +
> +		mdesc = list_first_entry(&mchan->free,
> +						struct mpc_dma_desc, node);
> +		if (!mdesc) {
> +			spin_unlock_irqrestore(&mchan->lock, iflags);
> +			/* Try to free completed descriptors */
> +			mpc_dma_process_completed(mdma);
> +			return NULL;
> +		}
> +
> +		list_del(&mdesc->node);
> +
> +		if (direction == DMA_DEV_TO_MEM) {
> +			per_paddr = mchan->src_per_paddr;
> +			tcd_nunits = mchan->src_tcd_nunits;
> +		} else {
> +			per_paddr = mchan->dst_per_paddr;
> +			tcd_nunits = mchan->dst_tcd_nunits;
> +		}
> +
> +		spin_unlock_irqrestore(&mchan->lock, iflags);
> +
> +		if (per_paddr == 0 || tcd_nunits == 0)
> +			goto err_prep;
> +
> +		mdesc->error = 0;
> +		mdesc->will_access_peripheral = 1;
> +
> +		/* Prepare Transfer Control Descriptor for this transaction */
> +		tcd = mdesc->tcd;
> +
> +		memset(tcd, 0, sizeof(struct mpc_dma_tcd));
> +
> +		if (!IS_ALIGNED(sg_dma_address(sg), 4))
> +			goto err_prep;
> +
> +		if (direction == DMA_DEV_TO_MEM) {
> +			tcd->saddr = per_paddr;
> +			tcd->daddr = sg_dma_address(sg);
> +			tcd->soff = 0;
> +			tcd->doff = 4;
what are these?

> +		} else {
> +			tcd->saddr = sg_dma_address(sg);
> +			tcd->daddr = per_paddr;
> +			tcd->soff = 4;
> +			tcd->doff = 0;
> +		}
> +
> +		tcd->ssize = MPC_DMA_TSIZE_4;
> +		tcd->dsize = MPC_DMA_TSIZE_4;
> +
> +		len = sg_dma_len(sg);
> +		tcd->nbytes = tcd_nunits * 4;
> +		if (!IS_ALIGNED(len, tcd->nbytes))
> +			goto err_prep;
> +
> +		iter = len / tcd->nbytes;
> +		if (iter >= 1 << 15) {
> +			/* len is too big */
> +			goto err_prep;
> +		}
> +		/* citer_linkch contains the high bits of iter */
> +		tcd->biter = iter & 0x1ff;
> +		tcd->biter_linkch = iter >> 9;
> +		tcd->citer = tcd->biter;
> +		tcd->citer_linkch = tcd->biter_linkch;
> +
> +		tcd->e_sg = 0;
> +		tcd->d_req = 1;
> +
> +		/* Place descriptor in prepared list */
> +		spin_lock_irqsave(&mchan->lock, iflags);
> +		list_add_tail(&mdesc->node, &mchan->prepared);
> +		spin_unlock_irqrestore(&mchan->lock, iflags);
> +	}
> +
> +	return &mdesc->desc;
> +
> +err_prep:
> +	/* Put the descriptor back */
> +	spin_lock_irqsave(&mchan->lock, iflags);
> +	list_add_tail(&mdesc->node, &mchan->free);
> +	spin_unlock_irqrestore(&mchan->lock, iflags);
> +
> +	return NULL;
> +}
> +
> +static int mpc_dma_device_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> +							unsigned long arg)
> +{
> +	struct mpc_dma_chan *mchan;
> +	struct mpc_dma *mdma;
> +	struct dma_slave_config *cfg;
> +	unsigned long flags;
> +
> +	mchan = dma_chan_to_mpc_dma_chan(chan);
> +	switch (cmd) {
> +	case DMA_TERMINATE_ALL:
> +		/* Disable channel requests */
> +		mdma = dma_chan_to_mpc_dma(chan);
> +
> +		spin_lock_irqsave(&mchan->lock, flags);
> +
> +		out_8(&mdma->regs->dmacerq, chan->chan_id);
> +		list_splice_tail_init(&mchan->prepared, &mchan->free);
> +		list_splice_tail_init(&mchan->queued, &mchan->free);
> +		list_splice_tail_init(&mchan->active, &mchan->free);
> +
> +		spin_unlock_irqrestore(&mchan->lock, flags);
> +
> +		return 0;
empty line after this pls

> +	case DMA_SLAVE_CONFIG:
> +		/*
> +		 * Constraints:
> +		 *  - only transfers between a peripheral device and
> +		 *     memory are supported;
> +		 *  - minimal transfer chunk is 4 bytes and consequently
> +		 *     source and destination addresses must be 4-byte aligned
> +		 *     and transfer size must be aligned on (4 * maxburst)
> +		 *     boundary;
> +		 *  - during the transfer RAM address is being incremented by
> +		 *     the size of minimal transfer chunk;
> +		 *  - peripheral port's address is constant during the transfer.
> +		 */
> +
> +		cfg = (void *)arg;
> +
> +		if (cfg->src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
> +		    cfg->dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES ||
and why this limtation, doesnt seem covered above?

> +		    !IS_ALIGNED(cfg->src_addr, 4) ||
> +		    !IS_ALIGNED(cfg->dst_addr, 4)) {
> +			return -EINVAL;
> +		}
> +
> +		spin_lock_irqsave(&mchan->lock, flags);
> +
> +		mchan->src_per_paddr = cfg->src_addr;
> +		mchan->src_tcd_nunits = cfg->src_maxburst;
> +		mchan->dst_per_paddr = cfg->dst_addr;
> +		mchan->dst_tcd_nunits = cfg->dst_maxburst;
> +
> +		/* Apply defaults */
> +		if (mchan->src_tcd_nunits == 0)
> +			mchan->src_tcd_nunits = 1;
> +		if (mchan->dst_tcd_nunits == 0)
> +			mchan->dst_tcd_nunits = 1;
> +
> +		spin_unlock_irqrestore(&mchan->lock, flags);
> +
> +		return 0;
empty line here too

> +	default:
> +		/* Unknown command */
> +		break;
> +	}
> +
> +	return -ENXIO;
> +}
> +
>  static int mpc_dma_probe(struct platform_device *op)
>  {
>  	struct device_node *dn = op->dev.of_node;
> @@ -727,9 +953,12 @@ static int mpc_dma_probe(struct platform_device *op)
>  	dma->device_issue_pending = mpc_dma_issue_pending;
>  	dma->device_tx_status = mpc_dma_tx_status;
>  	dma->device_prep_dma_memcpy = mpc_dma_prep_memcpy;
> +	dma->device_prep_slave_sg = mpc_dma_prep_slave_sg;
> +	dma->device_control = mpc_dma_device_control;
>  
>  	INIT_LIST_HEAD(&dma->channels);
>  	dma_cap_set(DMA_MEMCPY, dma->cap_mask);
> +	dma_cap_set(DMA_SLAVE, dma->cap_mask);
>  
>  	for (i = 0; i < dma->chancnt; i++) {
>  		mchan = &mdma->channels[i];
> -- 
> 1.8.4.2
> 

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH RFC v12 5/7] dma: of: add common xlate function for matching by channel id
From: Vinod Koul @ 2014-05-02 17:04 UTC (permalink / raw)
  To: Alexander Popov, Arnd Bergmann
  Cc: devicetree, Lars-Peter Clausen, Gerhard Sittig, Andy Shevchenko,
	dmaengine, Dan Williams, Anatolij Gustschin, linuxppc-dev
In-Reply-To: <1398261209-5578-6-git-send-email-a13xp0p0v88@gmail.com>

On Wed, Apr 23, 2014 at 05:53:27PM +0400, Alexander Popov wrote:
> This patch adds a new common OF dma xlate callback function which will match a
> channel by it's id. The binding expects one integer argument which it will use to
> lookup the channel by the id.
> 
> Unlike of_dma_simple_xlate this function is able to handle a system with
> multiple DMA controllers. When registering the of dma provider with
> of_dma_controller_register a pointer to the dma_device struct which is
> associated with the dt node needs to passed as the data parameter.
> New function will use this pointer to match only channels which belong to the
> specified DMA controller.

This patch needs review by DT folks too

> 
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> ---
>  drivers/dma/of-dma.c   | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/of_dma.h |  4 ++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
> index e8fe9dc..d5fbeaa 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -218,3 +218,38 @@ struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
>  			&dma_spec->args[0]);
>  }
>  EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
> +
> +/**
> + * of_dma_xlate_by_chan_id - Translate dt property to DMA channel by channel id
> + * @dma_spec:	pointer to DMA specifier as found in the device tree
> + * @of_dma:	pointer to DMA controller data
> + *
> + * This function can be used as the of xlate callback for DMA driver which wants
> + * to match the channel based on the channel id. When using this xlate function
> + * the #dma-cells propety of the DMA controller dt node needs to be set to 1.
> + * The data parameter of of_dma_controller_register must be a pointer to the
> + * dma_device struct the function should match upon.
> + *
> + * Returns pointer to appropriate dma channel on success or NULL on error.
> + */
> +struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
> +					 struct of_dma *ofdma)
> +{
> +	struct dma_device *dev = ofdma->of_dma_data;
> +	struct dma_chan *chan, *candidate = NULL;
> +
> +	if (!dev || dma_spec->args_count != 1)
> +		return NULL;
> +
> +	list_for_each_entry(chan, &dev->channels, device_node)
> +		if (chan->chan_id == dma_spec->args[0]) {
> +			candidate = chan;
> +			break;
> +		}
> +
> +	if (!candidate)
> +		return NULL;
> +
> +	return dma_get_slave_channel(candidate);
> +}
> +EXPORT_SYMBOL_GPL(of_dma_xlate_by_chan_id);
> diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
> index ae36298..56bc026 100644
> --- a/include/linux/of_dma.h
> +++ b/include/linux/of_dma.h
> @@ -41,6 +41,8 @@ extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
>  						     const char *name);
>  extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
>  		struct of_dma *ofdma);
> +extern struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
> +		struct of_dma *ofdma);
>  #else
>  static inline int of_dma_controller_register(struct device_node *np,
>  		struct dma_chan *(*of_dma_xlate)
> @@ -66,6 +68,8 @@ static inline struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_s
>  	return NULL;
>  }
>  
> +#define of_dma_xlate_by_chan_id NULL
> +
>  #endif
>  
>  #endif /* __LINUX_OF_DMA_H */
> -- 
> 1.8.4.2
> 

-- 

^ permalink raw reply

* Re: [PATCH RFC v12 1/7] dma: mpc512x: reorder mpc8308 specific instructions
From: Vinod Koul @ 2014-05-02 17:06 UTC (permalink / raw)
  To: Alexander Popov
  Cc: Lars-Peter Clausen, Arnd Bergmann, Gerhard Sittig,
	Andy Shevchenko, dmaengine, Dan Williams, Anatolij Gustschin,
	linuxppc-dev
In-Reply-To: <1398261209-5578-2-git-send-email-a13xp0p0v88@gmail.com>

On Wed, Apr 23, 2014 at 05:53:23PM +0400, Alexander Popov wrote:
> Concentrate the specific code for MPC8308 in the 'if' branch
> and handle MPC512x in the 'else' branch.
> This modification only reorders instructions but doesn't change behaviour.
> 
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
> Acked-by: Anatolij Gustschin <agust@denx.de>

Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH RFC v12 2/7] dma: mpc512x: separate 'compatible' values for MPC512x and MPC8308
From: Vinod Koul @ 2014-05-02 17:07 UTC (permalink / raw)
  To: Alexander Popov
  Cc: devicetree, Lars-Peter Clausen, Arnd Bergmann, Gerhard Sittig,
	Andy Shevchenko, dmaengine, Dan Williams, Anatolij Gustschin,
	linuxppc-dev
In-Reply-To: <1398261209-5578-3-git-send-email-a13xp0p0v88@gmail.com>

On Wed, Apr 23, 2014 at 05:53:24PM +0400, Alexander Popov wrote:
> MPC512x and MPC8308 have similar DMA controllers, but are independent SoCs.
> DMA controller driver should have separate 'compatible' values for these SoCs.
> 
> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com>
Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH RFC v12 4/7] dma: mpc512x: fix freeing resources in mpc_dma_probe() and mpc_dma_remove()
From: Vinod Koul @ 2014-05-02 17:07 UTC (permalink / raw)
  To: Alexander Popov
  Cc: Lars-Peter Clausen, Arnd Bergmann, Gerhard Sittig,
	Andy Shevchenko, dmaengine, Dan Williams, Anatolij Gustschin,
	linuxppc-dev
In-Reply-To: <1398261209-5578-5-git-send-email-a13xp0p0v88@gmail.com>

On Wed, Apr 23, 2014 at 05:53:26PM +0400, Alexander Popov wrote:
> Fix mpc_dma_probe() error path and mpc_dma_remove(): manually free IRQs and
> dispose IRQ mappings before devm_* takes care of other resources.
> Moreover replace devm_request_irq() with request_irq() since there is no need
> to use it because the original code always frees IRQ manually with
> devm_free_irq(). Replace devm_free_irq() with free_irq() accordingly.
Applied, thanks

-- 
~Vinod

^ permalink raw reply

* Re: [PATCH 1/1] powerpc: crtsaveres.o needed only when -Os flag is enabled
From: Ram Pai @ 2014-05-02 17:47 UTC (permalink / raw)
  To: Aneesh Kumar K.V; +Cc: hartb, paulus, anton, tony, linuxppc-dev
In-Reply-To: <87wqe4age2.fsf@linux.vnet.ibm.com>

On Fri, May 02, 2014 at 11:09:17AM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> >     powerpc: crtsaveres.o needed only when -Os flag is enabled
> >     
> >     Currently on powerpc arch, out-of-tree module fails to build without
> >     crtsaveres.o, even when the module has no dependency on the symbols
> >     provided by the file; when built without the -Os flag.
> >     
> >     BTW: '-Os' flag is enabled when CONFIG_CC_OPTIMIZE_FOR_SIZE is
> >     configured.
> >     
> >     This patch fixes that problem.
> >     
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >
> > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> > index 4c0cedf..cf12f38 100644
> > --- a/arch/powerpc/Makefile
> > +++ b/arch/powerpc/Makefile
> > @@ -157,7 +157,10 @@ CPP		= $(CC) -E $(KBUILD_CFLAGS)
> >  
> >  CHECKFLAGS	+= -m$(CONFIG_WORD_SIZE) -D__powerpc__ -D__powerpc$(CONFIG_WORD_SIZE)__
> >  
> > +ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
> >  KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
> > +endif
> > +
> >  
> >  # No AltiVec or VSX instructions when building kernel
> >  KBUILD_CFLAGS += $(call cc-option,-mno-altivec)
> >
> 
> So if we enable CONFIG_CC_OPTIMIZE_FOR_SIZE can we build out-of-tree
> module with this patch ?

	Yes, provided crtsaveres.o is available.


If crtsaveres.o is not available; some distro dont ship it, than
out-of-tree module linking will fail.

Some distro don't ship crtsaveres.o since they do not want
kernel and modules to be built with space optimization. In such cases
requiring crtsaveres.o to be available during module building, even
when the module is not built for space optimization is wrong.

This patch fixes that issue.

RP

^ permalink raw reply

* RE: [PATCH v2 1/4] KVM: PPC: e500mc: Revert "add load inst fixup"
From: mihai.caraman @ 2014-05-02 23:14 UTC (permalink / raw)
  To: Alexander Graf
  Cc: linuxppc-dev@lists.ozlabs.org, kvm@vger.kernel.org,
	kvm-ppc@vger.kernel.org
In-Reply-To: <53636436.60002@suse.de>

> From: Alexander Graf <agraf@suse.de>=0A=
> Sent: Friday, May 2, 2014 12:24 PM=0A=
> To: Caraman Mihai Claudiu-B02008=0A=
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-dev@lists.ozla=
bs.org=0A=
> Subject: Re: [PATCH v2 1/4] KVM: PPC: e500mc: Revert "add load inst fixup=
"=0A=
> =0A=
> On 05/01/2014 02:45 AM, Mihai Caraman wrote:=0A=
> > The commit 1d628af7 "add load inst fixup" made an attempt to handle=0A=
> > failures generated by reading the guest current instruction. The fixup=
=0A=
> > code that was added works by chance hiding the real issue.=0A=
> >=0A=
> > Load external pid (lwepx) instruction, used by KVM to read guest=0A=
> > instructions, is executed in a subsituted guest translation context=0A=
> > (EPLC[EGS] =3D 1). In consequence lwepx's TLB error and data storage=0A=
> > interrupts need to be handled by KVM, even though these interrupts=0A=
> > are generated from host context (MSR[GS] =3D 0).=0A=
> >=0A=
> > Currently, KVM hooks only interrupts generated from guest context=0A=
> > (MSR[GS] =3D 1), doing minimal checks on the fast path to avoid host=0A=
> > performance degradation. As a result, the host kernel handles lwepx=0A=
> > faults searching the faulting guest data address (loaded in DEAR) in=0A=
> > its own Logical Partition ID (LPID) 0 context. In case a host translati=
on=0A=
> > is found the execution returns to the lwepx instruction instead of the=
=0A=
> > fixup, the host ending up in an infinite loop.=0A=
> >=0A=
> > Revert the commit "add load inst fixup". lwepx issue will be addressed=
=0A=
> > in a subsequent patch without needing fixup code.=0A=
> >=0A=
> > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>=0A=
> =0A=
> Just a random idea: Could we just switch IVOR2 during the critical lwepx=
=0A=
> phase? In fact, we could even do that later when we're already in C code=
=0A=
> and try to recover the last instruction. The code IVOR2 would point to=0A=
> would simply set the register we're trying to read to as LAST_INST_FAIL=
=0A=
> and rfi.=0A=
=0A=
This was the first idea that sprang to my mind inspired from how DO_KVM=0A=
is hooked on PR. I actually did a simple POC for e500mc/e5500, but this wil=
l=0A=
not work on e6500 which has shared IVORs between HW threads.=0A=
=0A=
-Mike=

^ permalink raw reply

* Re: powerpc/mpc85xx: Remove P1023 RDS support
From: Scott Wood @ 2014-05-03  0:13 UTC (permalink / raw)
  To: Lijun Pan; +Cc: linuxppc-dev, Emilian.Medve
In-Reply-To: <1397841480-2652-1-git-send-email-Lijun.Pan@freescale.com>

On Fri, Apr 18, 2014 at 12:18:00PM -0500, Lijun Pan wrote:
> P1023RDS is no longer supported by Freescale while P1023RDB is still supported.

Please wrap changelogs at no more than 75 columns.

> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
> 
> ---
> arch/powerpc/boot/dts/p1023rds.dts                 | 219 ---------------------
>  arch/powerpc/configs/mpc85xx_defconfig             |   1 -
>  arch/powerpc/configs/mpc85xx_smp_defconfig         |   1 -
>  arch/powerpc/platforms/85xx/Kconfig                |   6 +-
>  arch/powerpc/platforms/85xx/Makefile               |   2 +-
>  .../platforms/85xx/{p1023_rds.c => p1023_rdb.c}    |  36 +---
>  6 files changed, 10 insertions(+), 255 deletions(-)
>  delete mode 100644 arch/powerpc/boot/dts/p1023rds.dts
>  rename arch/powerpc/platforms/85xx/{p1023_rds.c => p1023_rdb.c} (75%)

As discussed internally, I'd like to see more of an explanation for why
it should be removed upstream -- what sort of maintenance burden it's
causing, or why this board should be considered of so little interest to
users that we should remove it regardless of current maintenance burden.

-Scott

^ permalink raw reply

* Re: powerpc/mpc85xx: Add BSC9132 QDS Support
From: Scott Wood @ 2014-05-03  0:23 UTC (permalink / raw)
  To: harninder rai; +Cc: linuxppc-dev, Ruchika Gupta
In-Reply-To: <1395128102-12983-1-git-send-email-harninder.rai@freescale.com>

On Tue, Mar 18, 2014 at 01:05:02PM +0530, harninder rai wrote:
> +&ifc {
> +	nor@0,0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "cfi-flash";
> +		reg = <0x0 0x0 0x8000000>;
> +		bank-width = <2>;
> +		device-width = <1>;
> +
> +		partition@40000 {
> +			/* 256KB for DTB Image */
> +			reg = <0x00040000 0x00040000>;
> +			label = "NOR DTB Image";
> +		};
> +
> +		partition@80000 {
> +			/* 7MB for Linux Kernel Image */
> +			reg = <0x00080000 0x00700000>;
> +			label = "NAND Linux Kernel Image";
> +		};
> +
> +		partition@800000 {
> +			/* 55MB for Root file system */
> +			reg = <0x00800000 0x03700000>;
> +			label = "NOR RFS Image";
> +		};
> +
> +		partition@3f00000 {
> +			/* This location must not be altered  */
> +			/* 512KB for u-boot Bootloader Image */
> +			/* 512KB for u-boot Environment Variables */
> +			reg = <0x03f00000 0x00100000>;
> +			label = "NOR U-boot Image";
> +			read-only;
> +		};
> +	};
> +
> +	nand@1,0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "fsl,ifc-nand";
> +		reg = <0x1 0x0 0x4000>;
> +
> +		partition@0 {
> +			/* This location must not be altered  */
> +			/* 3MB for u-boot Bootloader Image */
> +			reg = <0x0 0x00300000>;
> +			label = "NAND U-Boot Image";
> +			read-only;
> +		};
> +
> +		partition@300000 {
> +			/* 1MB for DTB Image */
> +			reg = <0x00300000 0x00100000>;
> +			label = "NAND DTB Image";
> +		};
> +
> +		partition@400000 {
> +			/* 8MB for Linux Kernel Image */
> +			reg = <0x00400000 0x00800000>;
> +			label = "NAND Linux Kernel Image";
> +		};
> +
> +		partition@c00000 {
> +			/* Rest space for Root file System Image */
> +			reg = <0x00c00000 0x07400000>;
> +			label = "NAND RFS Image";
> +		};
> +	};
> +};

Please keep partition definitions out of the dts file, as has been
recently requested of other boards.  You can use U-Boot to create the
partition nodes based on the mtdparts variable, or you can use the Linux
mtdparts command line option.

-Scott

^ permalink raw reply

* Re: powerpc/mpc85xx: Add BSC9132 QDS Support
From: Scott Wood @ 2014-05-03  0:31 UTC (permalink / raw)
  To: harninder rai; +Cc: linuxppc-dev, Ruchika Gupta
In-Reply-To: <1395128102-12983-1-git-send-email-harninder.rai@freescale.com>

On Tue, Mar 18, 2014 at 01:05:02PM +0530, harninder rai wrote:
> +&ifc {
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	compatible = "fsl,ifc", "simple-bus";
> +	/* FIXME: Test whether interrupts are split */
> +	interrupts = <16 2 0 0 20 2 0 0>;
> +};

Have you done this test yet?

-Scott

^ permalink raw reply

* Re: powerpc/mpc85xx: Remove P1023 RDS support
From: Emil Medve @ 2014-05-03  6:44 UTC (permalink / raw)
  To: Scott Wood, Lijun Pan; +Cc: linuxppc-dev
In-Reply-To: <20140503001318.GA19925@home.buserror.net>

Hello Scott,


On 05/02/2014 07:13 PM, Scott Wood wrote:
> On Fri, Apr 18, 2014 at 12:18:00PM -0500, Lijun Pan wrote:
>> P1023RDS is no longer supported by Freescale while P1023RDB is still supported.
> 
> Please wrap changelogs at no more than 75 columns.
> 
>> Signed-off-by: Lijun Pan <Lijun.Pan@freescale.com>
>>
>> ---
>> arch/powerpc/boot/dts/p1023rds.dts                 | 219 ---------------------
>>  arch/powerpc/configs/mpc85xx_defconfig             |   1 -
>>  arch/powerpc/configs/mpc85xx_smp_defconfig         |   1 -
>>  arch/powerpc/platforms/85xx/Kconfig                |   6 +-
>>  arch/powerpc/platforms/85xx/Makefile               |   2 +-
>>  .../platforms/85xx/{p1023_rds.c => p1023_rdb.c}    |  36 +---
>>  6 files changed, 10 insertions(+), 255 deletions(-)
>>  delete mode 100644 arch/powerpc/boot/dts/p1023rds.dts
>>  rename arch/powerpc/platforms/85xx/{p1023_rds.c => p1023_rdb.c} (75%)
> 
> As discussed internally, I'd like to see more of an explanation for why
> it should be removed upstream -- what sort of maintenance burden it's
> causing, or why this board should be considered of so little interest to
> users that we should remove it regardless of current maintenance burden.

When discussed internally I believe we addressed these points but I
guess not to your satisfaction. I suspect you're not asking for a replay
so I'm trying to get actual numbers of boards sold hoping that would
make the conversation a bit more objective


Cheers,

^ permalink raw reply

* Re: [PATCH 4/6] powerpc/corenet: Create the dts components for the DPAA FMan
From: Emil Medve @ 2014-05-03 10:02 UTC (permalink / raw)
  To: Scott Wood, Shruti Kanetkar, devicetree,
	linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1398118262.1694.188.camel__8135.6513932862$1398128944$gmane$org@snotra.buserror.net>

Hello Scott,


On 04/21/2014 05:11 PM, Scott Wood wrote:
> On Fri, 2014-04-18 at 07:21 -0500, Shruti Kanetkar wrote:
>> +fman@400000 {
>> +	mdio@f1000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		compatible = "fsl,fman-xmdio";
>> +		reg = <0xf1000 0x1000>;
>> +	};
>> +};
> 
> I'd like to see a complete fman binding before we start adding pieces.

The driver for the FMan 10 Gb/s MDIO has upstreamed a couple of years
ago: '9f35a73 net/fsl: introduce Freescale 10G MDIO driver', granted
without a binding writeup. This patch series should probably include a
binding blurb. However, let's not gate this patchset on a complete
binding for the FMan

As you know we don't own the FMan work and the FMan work is... not ready
for upstreaming. In an attempt to make some sort of progress we've
decided to upstream the pieces that are less controversial and MDIO is
an obvious candidate

>> +fman@400000 {
>> +	mdio0: mdio@e1120 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		compatible = "fsl,fman-mdio";
>> +		reg = <0xe1120 0xee0>;
>> +	};
>> +};
> 
> What is the difference between "fsl,fman-mdio" and "fsl,fman-xmdio"?  I
> don't see the latter on the list of compatibles in patch 3/6.

'fsl,fman-mdio' is the 1 Gb/s MDIO (Clause 22 only). 'fsl,fman-xmdio' is
the 10 Gb/s MDIO (Clause 45 only). We can respin this patch wi

I believe 'fsl,fman-mdio' (and others on that list) was added
gratuitously as the FMan MDIO is completely compatible with the
eTSEC/gianfar MDIO driver, but we can deal with that later

> Within each category, is the exact fman version discoverable from the
> mdio registers?

No, but that's irrelevant as that's not the difference between the two
compatibles

>> diff --git a/arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi
>> new file mode 100644
>> index 0000000..ced5786
>> --- /dev/null
>> +++ b/arch/powerpc/boot/dts/fsl/qoriq-fman3-1.dtsi
>> @@ -0,0 +1,52 @@
>> +/*
>> + * QorIQ FMan v3 device tree stub [ controller @ offset 0x500000 ]
>> + *
>> + * Copyright 2012 Freescale Semiconductor Inc.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions are met:
>> + *     * Redistributions of source code must retain the above copyright
>> + *       notice, this list of conditions and the following disclaimer.
>> + *     * Redistributions in binary form must reproduce the above copyright
>> + *       notice, this list of conditions and the following disclaimer in the
>> + *       documentation and/or other materials provided with the distribution.
>> + *     * Neither the name of Freescale Semiconductor nor the
>> + *       names of its contributors may be used to endorse or promote products
>> + *       derived from this software without specific prior written permission.
>> + *
>> + *
>> + * ALTERNATIVELY, this software may be distributed under the terms of the
>> + * GNU General Public License ("GPL") as published by the Free Software
>> + * Foundation, either version 2 of that License or (at your option) any
>> + * later version.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
>> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
>> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
>> + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
>> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
>> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
>> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
>> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> +*/
>> +
>> +fman@500000 {
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	compatible = "simple-bus";
> 
> Why is this simple-bus?

Because that's the translation type for the FMan sub-nodes. We need it
now to get the MDIO nodes probed and we'll needed later to probe other
nodes/devices that will have standalone drivers: MAC, MURAM. etc. I
don't think we need to add another entry into the platform probe list
(which is already quite long) as a simple synonym

>> +
>> +	/* mdio nodes for fman v3 @ 0x500000 */
>> +	mdio@fc000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		reg = <0xfc000 0x1000>;
>> +	};
>> +
>> +	mdio@fd000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		reg = <0xfd000 0x1000>;
>> +	};
>> +};
> 
> Where's the compatible?  Why is this file different from all the others?

The FMan v3 MDIO block (supports both Clause 22/45) is compatible with
the FMan v2 10 Gb/s MDIO (the xgmac-mdio driver). However, the driver
needs a small clean-up patch (still in internal review) that will get it
working for FMan v3 MDIO. With that patch will add the compatible to
these nodes. However, we need these nodes now for the board level MDIO
bus muxing support (included in this patchset)


Cheers,

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox