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

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

[Albrecht Dreß]

Hi Wolfram!

Thanks a lot for your comments!

[snip]
> > + * as the chip can be only either a 5200B or not. */
> > +static int is_mpc5200b = -1;
> > +
> > +
> 
> One empty line too much. Maybe we can also get rid of the static later in
> the
> process, but first things first.

Ooops....

[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.  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.

[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!!

> > +		if (np) {
> > +			is_mpc5200b = 1;
> > +			dev_dbg(&op->dev, "mpc5200b: using /4 prescaler\n");
> 
> Does this message respect the fallback case?

See comment above...

> 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.

Again, any insight from the device tree gurus would be appreciated!

Thanks, Albrecht.

Tolle Dekolletés oder scharfe Tatoos? Vote jetzt ... oder mach selbst mit und zeige Deine Schokoladenseite
bei Topp oder Hopp von Arcor: http://www.arcor.de/rd/footer.toh

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

[Wolfram Sang]

[-- Attachment #1.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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

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

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

[Albrecht Dreß]

Hi Wolfram:

[snip]
> > 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?)

O.k., I will add that comment...

> > 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 ;)

I agree - will make the debug output and comments clearer...

[snip]
> > > 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?

Not for the source file, but for all the dts files, if they want to benefit from the detection of the '5200B.  Basically, *all* files have to be checked and touched if necessary.  Again, I agree that this would be the clean approach, but I wanted to avoid that effort.  Grant???

[snip]
> 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;

Umm, yes, that's true of course.  Will pick it up.

Thanks, Albrecht.

Tolle Dekolletés oder scharfe Tatoos? Vote jetzt ... oder mach selbst mit und zeige Deine Schokoladenseite
bei Topp oder Hopp von Arcor: http://www.arcor.de/rd/footer.toh

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

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1060 bytes --]


> > > > 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?
> 
> Not for the source file, but for all the dts files, if they want to benefit
> from the detection of the '5200B.  Basically, *all* files have to be checked
> and touched if necessary.  Again, I agree that this would be the clean
> approach, but I wanted to avoid that effort.  Grant???

Please check the current 5200b-dts files. They already have the property,
exactly for the case that a specific driver may be added in the future. And
even if not, the behaviour of the driver would not change by missing your
driver improvement, so there is also no regression.

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

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

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

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

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

[Grant Likely]

On Tue, Mar 2, 2010 at 1:09 AM, Albrecht Dreß <albrecht.dress@arcor.de> wrote:
>> > +   /* 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!!

fsl,mpc5200b-psc-uart is already in the compatible list for all
MPC500b boards currently in the kernel tree.

>> > +           if (np) {
>> > +                   is_mpc5200b = 1;
>> > +                   dev_dbg(&op->dev, "mpc5200b: using /4 prescaler\n");
>>
>> Does this message respect the fallback case?
>
> See comment above...
>
>> 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.
>
> Again, any insight from the device tree gurus would be appreciated!

Wolfram is correct, you should set the correct divisor function in the
ops structure.  Much clearer code that way.

g.

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

[Grant Likely]

On Tue, Mar 2, 2010 at 1:56 AM, Albrecht Dreß <albrecht.dress@arcor.de> wrote:
> Hi Wolfram:
>
> [snip]
>> > 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?)
>
> O.k., I will add that comment...
>

Yes, please document your calculation approach thoroughly.

>> > > 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?
>
> Not for the source file, but for all the dts files, if they want to benefit from the detection of the '5200B.  Basically, *all* files have to be checked and touched if necessary.  Again, I agree that this would be the clean approach, but I wanted to avoid that effort.  Grant???

Already there on mainlined .dts files, and it won't break boards that
don't have it.

> [snip]
>> 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;
>
> Umm, yes, that's true of course.  Will pick it up.

When it comes to code clarity, I'd prefer you had separate mpc5200 and
mpc5200b ops structures.  I'm not worried about an extra 64 bytes
added to the kernel size for this (minus the # of bytes needed to test
and set the new op).

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.