[U-Boot] [PATCH 1/2] Serial support extended up to 6 COMs

[Wolfgang Denk <wd@denx.de>]

Dear Michael Zaidman,

In message <660c0f821003230108t579e90femaac28b937e04329c@mail.gmail.com> you wrote:
> Dear Wolfgand,

s/d/g/

> All these chips are treated in the same way by this patch. Only
> frequency of crystal oscillator or external clock can differs from
> UART ports belonging to CPU. I tried to explain it in the description
> of the change placed at the head of the file. For this purpose the
> CONFIG_SYS_NS16550_HI_PORTS_BGN was added which allows us to
> distinguish between "low" ports belonging to CPU chip and "high" ports
> of additional chip while calculating clock divisor.

I don't like this distinction. I think we should rather use a flag, or
pointer to handler functions, etc. This way we can get rid of the
index checking here and there in the code.

> > Hm... I cannot help it, but this all is extremely ugly. This needs to
> > be reworked.
> >
> The intention here was to minimize memory consumption (20 bytes @
> single UART port on board) Most of the boards have 1-2 UART ports
> only. So I looked for a way to remove unused NULL pointers from the
> serial_ports array which now have 6 entries. Any better idea how to
> achieve it? The code looks ugly to me too and if you insist I will
> turn it to the previous form at the expense of memory...

We should try to come up with siomething that really scales well. Both
4 or 6 ports are pretty arbitrary limitations (OTOH - who needs to
support that many ports in U-Boot?).

I think it should be possible to use a "packed" array without unused
entries. Also, the order in this array should be flexible and not
dictated by concepts like "low" or "high" ports.

> >> + ? ? ulong clock = CONFIG_SYS_NS16550_CLK;
> >> +
> >> ?#ifdef CONFIG_OMAP1510
> >> + ? ? NS16550_t port = serial_ports[index];
> >
> > Why did you change that?
>
> Because I need the index of the port in this routine in order to
> distinguish between "low" and "high" UART ports and I did not wont to
> add second parameter to the routine?s interface.

Understood. I think we should drop this index based distinction and
use some other flag.

> The "ugly" code we discussed above places NULL pointers in > the "holes"
> up to the highest defined COM. The MAX_SER_DEV gets index of the
> highest defined COM as well. So, this loop will access all defined
> COMs. Or I misunderstood your question?

No. I did not understand the code.

> >> @@ -328,4 +374,11 @@ struct serial_device eserial3_device =
> >> ?DECLARE_ESERIAL_FUNCTIONS(4);
> >> ?struct serial_device eserial4_device =
> >> ? ? ? INIT_ESERIAL_STRUCTURE(4,"eserial3","EUART4");
> >> +DECLARE_ESERIAL_FUNCTIONS(5);
> >> +struct serial_device eserial5_device =
> >> + ? ? INIT_ESERIAL_STRUCTURE(5,"eserial4","EUART5");
> >> +DECLARE_ESERIAL_FUNCTIONS(6);
> >> +struct serial_device eserial6_device =
> >> + ? ? INIT_ESERIAL_STRUCTURE(6,"eserial5","EUART6");
> >> ?#endif /* CONFIG_SERIAL_MULTI */
> >
> > This needs rework, too.
>
> This was here before, I just added two ports. Could you please be more specific?

The code was there before, and when you added more of it it became
clear that this implementation does not scale, so we should look for
another, better approach.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A door is what a dog is perpetually on the wrong side of.
                                                        - Ogden Nash