Re: [PATCHv3 2/4] cpm-serial: Relocate CPM buffer descriptors and SMC parameter ram.

[Laurent Pinchart <laurentp@cse-semaphore.com>]

[-- Attachment #1: Type: text/plain, Size: 3761 bytes --]

On Monday 31 March 2008 21:10, Scott Wood wrote:
> Laurent Pinchart wrote:
> > This patch relocates the buffer descriptors and the SMC parameter RAM at the
> > end of the first CPM muram chunk, as described in the device tree. This allows
> > device trees to stop excluding SMC parameter ram allocated by the boot loader
> > from the CPM muram node.
> 
> It's usually a good idea to state that something is untested if that's 
> the case. :-)

Sorry. I'll state it clearly next time.

> This patch cannot work as is.
> 
> > +static int cpm_get_virtual_address(void *devp, void **addr, int ncells)
> > +{
> > +	unsigned long xaddr;
> > +	int n;
> > +
> > +	n = getprop(devp, "virtual-reg", addr, ncells * sizeof *addr);
> > +	if (n < ncells * sizeof *addr) {
> 
> You must cast the sizeof to a signed int; otherwise, a negative return 
> from getprop will be "bigger" than the unsigned size, and you'll return 
> garbage as the address.
> 
> > +		for (n = 0; n < ncells; n++) {
> > +			if (!dt_xlate_reg(devp, n, &xaddr, NULL))
> > +				return -1;
> > +
> > +			addr[n] = (void*)xaddr;
> 
> (void *)
> 
> > +		}
> > +	}
> > +
> > +	return ncells;
> > +}
> 
> This could be a generic bootwrapper function.  It should return the 
> number of resources (ncells is a misnomer) actually found, though, 
> rather than failing if there are fewer than asked for.  Let the caller 
> decide if it's fatal.

Ok. I'll move it to devtree.c.

> > @@ -202,63 +243,62 @@ int cpm_console_init(void *devp, struct serial_console_data *scdp)
> >  	else
> >  		do_cmd = cpm1_cmd;
> >  
> > -	n = getprop(devp, "fsl,cpm-command", &cpm_cmd, 4);
> > -	if (n < 4)
> > +	if (getprop(devp, "fsl,cpm-command", &cpm_cmd, 4) < sizeof cpm_cmd)
> >  		return -1;
> 
> Standard kernel style is sizeof(foo), not sizeof foo.

Ok.

> Plus, if you're going to replace 4 with sizeof(cpm_cmd), do it both 
> places.  I don't really see the need, though; a cell is always 4 bytes.

Ok, I'll keep 4.

> > -	n = getprop(parent, "virtual-reg", reg_virt, sizeof(reg_virt));
> > -	if (n < (int)sizeof(reg_virt)) {
> > -		if (!dt_xlate_reg(parent, 0, &reg_phys, NULL))
> > -			return -1;
> > -
> > -		reg_virt[0] = (void *)reg_phys;
> > -	}
> > -
> > -	cpcr = reg_virt[0];
> > +	if (cpm_get_virtual_address(devp, &cpcr, 1) < 0)
> > +		return -1;
> 
> s/devp/parent/
> 
> >  	muram = finddevice("/soc/cpm/muram/data");
> >  	if (!muram)
> >  		return -1;
> >  
> >  	/* For bootwrapper-compatible device trees, we assume that the first
> > -	 * entry has at least 18 bytes, and that #address-cells/#data-cells
> > +	 * entry has at least 128 bytes, and that #address-cells/#data-cells
> >  	 * is one for both parent and child.
> >  	 */
> >  
> > -	n = getprop(muram, "virtual-reg", reg_virt, sizeof(reg_virt));
> > -	if (n < (int)sizeof(reg_virt)) {
> > -		if (!dt_xlate_reg(muram, 0, &reg_phys, NULL))
> > -			return -1;
> > +	if (cpm_get_virtual_address(devp, &muram_addr, 1) < 0)
> > +		return -1;
> 
> s/devp/muram/
> 
> > +	
> > +	if (getprop(muram, "reg", reg, sizeof reg) < sizeof reg)
> > +		return -1;
> 
> Should read into array of u32, not void *.

Ok.

> > +	if (is_cpm2 && is_smc) {
> > +		u16 *smc_base = (u16*)param;
> 
> (u16 *)
> 
> > +		u16 pram_offset;
> >  
> > -	muram_start = reg_virt[0];
> > +		pram_offset = cbd_offset - 64;
> > +		pram_offset = _ALIGN_DOWN(pram_offset, 64);
> > +		*smc_base = pram_offset;
> 
> Use out_be16().
> 
> The SMC should be stopped before you do this.

Right.

New patch comming.

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussée de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]