spi_mpc8xxx: issue with using definition of pram in Device Tree

This patch applies to 2.6.34.7 and 2.6.35.4
It fixes an issue during the probe for CPM1 with definition of parameter ram from DTS

Signed-off-by: christophe leroy <christophe.leroy@c-s.fr>

Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

On Thu, Sep 16, 2010 at 09:05:03AM +0200, christophe leroy wrote:
> This patch applies to 2.6.34.7 and 2.6.35.4
> It fixes an issue during the probe for CPM1 with definition of parameter ram from DTS
> 
> Signed-off-by: christophe leroy <christophe.leroy@c-s.fr>

I'm sorry, I don't understand the fix from the given description.
What is the problem, and why is cpm_muram_alloc_fixed() the wrong
thing to call on CPM1?  Does CPM2 still need it?

g.

> 
> diff -urN b/drivers/spi/spi_mpc8xxx.c c/drivers/spi/spi_mpc8xxx.c
> --- b/drivers/spi/spi_mpc8xxx.c	2010-09-08 16:43:50.000000000 +0200
> +++ c/drivers/spi/spi_mpc8xxx.c	2010-09-08 16:44:03.000000000 +0200
> @@ -822,7 +822,7 @@
>  	if (!iprop || size != sizeof(*iprop) * 4)
>  		return -ENOMEM;
>  
> -	spi_base_ofs = cpm_muram_alloc_fixed(iprop[2], 2);
> +	spi_base_ofs = iprop[2];
>  	if (IS_ERR_VALUE(spi_base_ofs))
>  		return -ENOMEM;
>  
> @@ -844,7 +844,6 @@
>  			return spi_base_ofs;
>  	}
>  
> -	cpm_muram_free(spi_base_ofs);
>  	return pram_ofs;
>  }

Hello,

The issue is that cpm_muram_alloc_fixed() allocates memory from the 
general purpose muram area (from 0x0 to 0x1bff).
Here we need to return a pointer to the parameter RAM, which is located 
somewhere starting at 0x1c00. It is not a dynamic allocation that is 
required here but only to point on the correct location in the parameter 
RAM.

For the CPM2, I don't know. I'm working with a MPC866.

Attached is a previous discussion on the subject where I explain a bit 
more in details the issue.

Regards
C. Leroy

Le 24/09/2010 09:10, Grant Likely a écrit :
> On Thu, Sep 16, 2010 at 09:05:03AM +0200, christophe leroy wrote:
>> This patch applies to 2.6.34.7 and 2.6.35.4
>> It fixes an issue during the probe for CPM1 with definition of parameter ram from DTS
>>
>> Signed-off-by: christophe leroy<christophe.leroy@c-s.fr>
> I'm sorry, I don't understand the fix from the given description.
> What is the problem, and why is cpm_muram_alloc_fixed() the wrong
> thing to call on CPM1?  Does CPM2 still need it?
>
> g.
>
>> diff -urN b/drivers/spi/spi_mpc8xxx.c c/drivers/spi/spi_mpc8xxx.c
>> --- b/drivers/spi/spi_mpc8xxx.c	2010-09-08 16:43:50.000000000 +0200
>> +++ c/drivers/spi/spi_mpc8xxx.c	2010-09-08 16:44:03.000000000 +0200
>> @@ -822,7 +822,7 @@
>>   	if (!iprop || size != sizeof(*iprop) * 4)
>>   		return -ENOMEM;
>>
>> -	spi_base_ofs = cpm_muram_alloc_fixed(iprop[2], 2);
>> +	spi_base_ofs = iprop[2];
>>   	if (IS_ERR_VALUE(spi_base_ofs))
>>   		return -ENOMEM;
>>
>> @@ -844,7 +844,6 @@
>>   			return spi_base_ofs;
>>   	}
>>
>> -	cpm_muram_free(spi_base_ofs);
>>   	return pram_ofs;
>>   }
On Tue, 7 Sep 2010 11:17:17 +0200
LEROY Christophe <christophe.leroy@c-s.fr> wrote:

> 
>   Dear Kumar,
> 
> I have a small issue in the init of spi_mpc8xxx.c with MPC866 (CPM1)
> 
> Unlike cpm_uart that maps the parameter ram directly using 
> of_iomap(np,1), spi_mpc8xxx.c uses cpm_muram_alloc_fixed().
> 
> This has two impacts in the .dts file:
> * The driver must be declared with pram at 1d80 instead of 3d80 whereas 
> it is not a child of muram@2000 but a child of cpm@9c0
> * muram@2000/data@0 must be declared with reg = <0x0 0x2000>   whereas 
> is should be reg=<0x0 0x1c00> to avoid cpm_muram_alloc() to allocate 
> space from parameters ram.
> 
> Maybe I misunderstood something ?

Don't make the device tree lie, fix the driver instead.

The allocator should not be given any chunks of muram that are
dedicated to a fixed purpose -- it might hand it out to something else
before you reserve it.  I don't think that cpm_muram_alloc_fixed() has
any legitimate use at all.

-Scott

Hello,

On Fri, Sep 24, 2010 at 09:20:27AM +0200, LEROY Christophe wrote:
> The issue is that cpm_muram_alloc_fixed() allocates memory from the
> general purpose muram area (from 0x0 to 0x1bff).
> Here we need to return a pointer to the parameter RAM, which is
> located somewhere starting at 0x1c00. It is not a dynamic allocation
> that is required here but only to point on the correct location in
> the parameter RAM.
> 
> For the CPM2, I don't know. I'm working with a MPC866.
> 
> Attached is a previous discussion on the subject where I explain a
> bit more in details the issue.

The patch looks OK, I think.

Doesn't explain why that worked on MPC8272 (CPM2) and MPC8560
(also CPM2) machines though. But here's my guess (I no longer
have these boards to test it):

On 8272 I used this node:

+                       spi@4c0 {
+                               #address-cells = <1>;
+                               #size-cells = <0>;
+                               compatible = "fsl,cpm2-spi", "fsl,spi";
+                               reg = <0x11a80 0x40 0x89fc 0x2>;

On that SOC there are two muram data regions 0x0..0x2000 and
0x9000..0x9100. Note that we actually don't want "data" regions,
and the only reason why that worked is that sysdev/cpm_common.c
maps muram(0)..muram(max).

Thanks,

On Fri, 24 Sep 2010 01:10:06 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Thu, Sep 16, 2010 at 09:05:03AM +0200, christophe leroy wrote:
> > This patch applies to 2.6.34.7 and 2.6.35.4
> > It fixes an issue during the probe for CPM1 with definition of parameter ram from DTS
> > 
> > Signed-off-by: christophe leroy <christophe.leroy@c-s.fr>
> 
> I'm sorry, I don't understand the fix from the given description.
> What is the problem, and why is cpm_muram_alloc_fixed() the wrong
> thing to call on CPM1?  Does CPM2 still need it?

I don't see how cpm_muram_alloc_fixed() can be used safely at all.  If
you need a fixed address, it shouldn't be part of the general
allocation pool, or something else might get it first.

-Scott

On Fri, 24 Sep 2010 11:57:40 +0400
Anton Vorontsov <cbouatmailru@gmail.com> wrote:

> Doesn't explain why that worked on MPC8272 (CPM2) and MPC8560
> (also CPM2) machines though. But here's my guess (I no longer
> have these boards to test it):
> 
> On 8272 I used this node:
> 
> +                       spi@4c0 {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +                               compatible = "fsl,cpm2-spi", "fsl,spi";
> +                               reg = <0x11a80 0x40 0x89fc 0x2>;
> 
> On that SOC there are two muram data regions 0x0..0x2000 and
> 0x9000..0x9100. Note that we actually don't want "data" regions,
> and the only reason why that worked is that sysdev/cpm_common.c
> maps muram(0)..muram(max).

Wouldn't it still fail the rh_alloc_fixed call?

-Scott