> [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/ |
signature.asc
Description: Digital signature
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
