Re: [Patch] mpc5200b: improve baud rate calculation (reach high baud rates, better accuracy)

[Wolfram Sang <w.sang@pengutronix.de>]

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

> [snip]
> > > +	if (is_mpc5200b == 1)
> > > +		return mpc5xxx_get_bus_frequency(p) * 4;
> > > +	else
> > > +		return mpc5xxx_get_bus_frequency(p) / 2;
> > 
> > Isn't this wrong? You can also have /32 on the 5200B (the fallback).
> 
> Yes, but I do all /calculations/ with the /4 prescaler for higher accuracy.
> If the divisor exceeds the available 16 bits of the counter reg, I round
> (divisor / 8) to use the /32 prescaler.  Think of a 19-bit counter value,
> where I can choose to use either the lower or the higher 16 bits for the
> counter reg.

Okay, now I got it. (Maybe this is an indication for another comment above the
set divisor function?)

> Remember also that using the higher 16 bits (/32 prescaler) is
> probably the exceptional case - with an IPB frequency of 132 MHz this will
> happen only for standard baud rates B300 and slower.

Even the rare cases have to be correct ;)

> [snip]
> > > +	/* Check only once if we are running on a mpc5200b or not */
> > > +	if (is_mpc5200b == -1) {
> > > +		struct device_node *np;
> > > +
> > > +		np = of_find_compatible_node(NULL, NULL, "fsl,mpc5200b-immr");
> > 
> > This should be handled using a new compatible-entry
> > "fsl,mpc5200b-psc-uart".
> 

> I agree that this would be a lot cleaner, but it's also a lot more intrusive.
> CC'ing the device tree discussion list here... comments, please!!

Why intrusive? Maybe I miss something?

> > You could also have a set_divisor-function for 5200 and 5200B and set it
> > here in the function struct (one reason less for the static ;))
> 
> Hmmm, but then I would need a 'static struct psc_ops mpc5200b_psc_ops', where
> only two functions differ from the generic 52xx struct as it is implemented
> now.  Using the static int needs less space.  However, in combination with
> the new compatible entry, it would of course make sense.

Leave those two function pointers empty and fill them during probe (probe has
access to the compatible-property it was matched against, see its arguments).
So it should be a matter of:

if (matched_property == 5200b)
	ops->func = this_one;
else
	ops->func = that_one;

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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