Re: [OpenBIOS] [PATCH] ppc: fix ESCC reg properties

[Mark Cave-Ayland]

On 03/02/16 06:29, Hervé Poussineau wrote:
> I/O offsets were wrong in legacy mode and some were missing in non-legacy mode.
> This fixes serial port detection in NetBSD, which was ignoring it.
> This also partly fixes MacOS 9.2, which relies on them to initialize OpenTransport.
> Signed-off-by: Hervé Poussineau
>
> drivers/escc.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/escc.c b/drivers/escc.c
> index afb97fa..8f122d8 100644
> --- a/drivers/escc.c
> +++ b/drivers/escc.c
> @@ -384,6 +384,8 @@ escc_add_channel(const char *path, const char *node, phys_addr_t addr,
>  {
>      char buf[64], tty[32];
>      phandle_t dnode, aliases;
>
> int legacy_offsets[][3] = { { 0x4, 0x6, 0xa }, { 0x0, 0x2, 0x8 } };
>
> int offsets[][3] = { { 0x20, 0x30, 0x50 }, { 0x00, 0x10, 0x40 } };
>
> cell props[10];
> int offset;
>
> @@ -425,18 +427,18 @@ escc_add_channel(const char *path, const char *node, phys_addr_t addr,
>      set_property(dnode, "compatible", buf, 9);
>  if (legacy) {
>    props[0] = IO_ESCC_LEGACY_OFFSET + offset * 0x4;
>    props[1] = 0x00000001;
>    props[2] = IO_ESCC_LEGACY_OFFSET + offset * 0x4 + 2;
>    props[3] = 0x00000001;
>    props[4] = IO_ESCC_LEGACY_OFFSET + offset * 0x4 + 6;
>    props[5] = 0x00000001;
>    set_property(dnode, "reg", (char *)&props, 6 * sizeof(cell));
>    props[0] = IO_ESCC_LEGACY_OFFSET + legacy_offsets[offset][0];
>    props[2] = IO_ESCC_LEGACY_OFFSET + legacy_offsets[offset][1];
>    props[4] = IO_ESCC_LEGACY_OFFSET + legacy_offsets[offset][2];
> } else {
>    props[0] = IO_ESCC_OFFSET + offset * 0x20;
>    props[1] = 0x00000020;
>    set_property(dnode, "reg", (char *)&props, 2 * sizeof(cell));
>    props[0] = IO_ESCC_LEGACY_OFFSET + offsets[offset][0];
>    props[2] = IO_ESCC_LEGACY_OFFSET + offsets[offset][1];
>    props[4] = IO_ESCC_LEGACY_OFFSET + offsets[offset][2];
>
> }
>
> props[1] = 0x00000001;
>
> props[3] = 0x00000001;
>
> props[5] = 0x00000001;
>
> set_property(dnode, "reg", (char *)&props, 6 * sizeof(cell));
>
> if (legacy) {
>     props[0] = addr + IO_ESCC_LEGACY_OFFSET + offset * 0x4;

Good catch! I've just compared this with an G4 AGP device tree and it
does look right - not quite sure how I missed this first time around.

My two comments would be if possible can we just use a single array with
another subscript, and also can we move the odd numbered properties back
inline so we have something like this:

int offsets[2][2][3] = {
    {
        /* legacy ch-b, legacy ch-a */
        { 0x4, 0x6, 0xa }, { 0x0, 0x2, 0x8 }
    },
    {
        /* ch-b, ch-a */
        { 0x20, 0x30, 0x50 }, { 0x00, 0x10, 0x40 }
    }
};

and then the loop just becomes:

props[0] = IO_ESCC_LEGACY_OFFSET + offsets[legacy][offset][0];
props[1] = 0x00000001;
props[2] = IO_ESCC_LEGACY_OFFSET + offsets[legacy][offset][1];
props[3] = 0x00000001;
props[4] = IO_ESCC_LEGACY_OFFSET + offsets[legacy][offset][2];
props[5] = 0x00000001;

Also perhaps it is also worth renaming offset to idx or similar for clarity?

I've just done a quick test on OS 9 and on the plus side I don't get a
crash into MacsBug, but I get a long hang followed by the mouse cursor
turning into a bomb - but at least that's progress :) If you can
resubmit a new version, I'll give it a quick spin around a few other OSs
to make sure there are no regressions.

Note that currently we don't add the DBDMA properties at the end of the
reg property because QEMU doesn't yet support serial DBDMA, but perhaps
it is time to start turning on DBDMA logging to see if this is what's is
causing OT to get stuck.

In fact, do we know if OT ever worked on MOL at all or did it provide
its own drivers?

ATB,

Mark.