[PATCH v2 2/2] powerpc/mpic: add global timer support

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, August 11, 2012 4:38 AM
> To: Wang Dongsheng-B40534
> Cc: benh at kernel.crashing.org; paulus at samba.org; linuxppc-
> dev at lists.ozlabs.org; Gala Kumar-B11780; Li Yang-R58472
> Subject: Re: [PATCH v2 2/2] powerpc/mpic: add global timer support
> 
> On 08/10/2012 12:54 AM, Dongsheng.wang at freescale.com wrote:
> > +static int group_get_freq(struct group_priv *priv) {
> > +	if (priv->flags & FSL_GLOBAL_TIMER) {
> > +		ccbfreq = fsl_get_sys_freq();
> > +		priv->timerfreq = ccbfreq;
> > +	} else {
> > +		priv->timerfreq = in_be32(priv->group_tfrr);
> > +	}
> 
> FSL MPICs have TFRR too.  I'm not sure that the lack of fsl,mpic is a
> good indication that TFRR is being set (e.g. we have old device trees for
> FSL chips with U-Boot that are labelled as ordinary openpics).
> 
[Wang Dongsheng] yes, we have. But we do not used it. The TFRR register value
is zero. 

> > +
> > +	if (priv->timerfreq <= 0)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> 
> timerfreq is unsigned.  It can never be < 0.
> 
[Wang Dongsheng] Yeah. Thanks.
> > +
> > +static int group_init_regmap(struct device_node *np, struct
> > +group_priv *priv) {
> > +	priv->group_tfrr = of_iomap(np, 0);
> > +	if (!priv->group_tfrr) {
> > +		pr_err("%s: cannot ioremap tfrr address.\n",
> > +				np->full_name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	priv->regs = of_iomap(np, 1);
> > +	if (!priv->regs) {
> > +		pr_err("%s: cannot ioremap timer register address.\n",
> > +				np->full_name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!(priv->flags & FSL_GLOBAL_TIMER))
> > +		return 0;
> > +
> > +	priv->group_tcr = of_iomap(np, 2);
> > +	if (!priv->group_tcr) {
> > +		pr_err("%s: cannot ioremap tcr address.\n", np->full_name);
> > +		return -EINVAL;
> > +	}
> 
> This is not compatible with existing mpic timer nodes.
> 
[Wang Dongsheng] Yeah, next patch to support that.

> > +	p = of_get_property(np, "available-ranges", &len);
> > +	if (p && len % (2 * sizeof(u32)) != 0) {
> > +		pr_err("%s: malformed fsl,available-ranges property.\n",
> > +				np->full_name);
> > +		return -EINVAL;
> > +	}
> 
> You need to support fsl,available-ranges since that's in an accepted
> binding and people could have partitioned setups already using it.
> 
[Wang Dongsheng] FSL chip or OPEN-PIC specification(Only a group)
in each group only four timer. This is unified. So i use a generic name.
I think there is not compatible with existing mpic timer nodes.

	p = of_get_property(np, "available-ranges", &len);
	if (!p)
		p = of_get_property(np, "fsl,available-ranges", &len);

	if (p && len % (2 * sizeof(u32)) != 0) {
		pr_err("%s: malformed fsl,available-ranges property.\n",
				np->full_name);
		return -EINVAL;
	}

> You also have a mismatch between the property you check and the error
> string.
> 
[Wang Dongsheng] Yeah. :-(.

> -Scott