Re: 4byte aligned com(4) and PCI_MAPREG_TYPE_MEM

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

msaitoh@ wrote:

>  Registers of Quark X1000's com are 4byte aligned.
> Some other machines have such type of device, so
> I modified COM_INIT_REGS() macro to support both
> byte aligned and 4byte aligned. This change reduce
> special modifications done in atheros, rmi and
> marvell drivers.

Some bus_space(9) implementation handles such sparse space
mappings and that was the reason why the COM_REGMAP was introduced.
Is it checked properly?
(though probably it's time to make COM_REGMAP default anyway)

> What should I do?
> One of the solution is to check whether extent_init() was called
> or not. There is no easy way to know it, so I added a global
> variable "extent_initted". Is it acceptable?

Some ports have MD such "initted" variable and check it MD internal
functions so I wonder if it's ok to have MI global variable.
(IIRC there was similar discussion when MI genfb support was added)

> Index: dev/ic/comvar.h
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/ic/comvar.h,v
> retrieving revision 1.78
> diff -u -p -r1.78 comvar.h
> --- dev/ic/comvar.h   3 Oct 2013 13:23:03 -0000       1.78
> +++ dev/ic/comvar.h   9 Feb 2014 17:43:24 -0000
 :
> -#ifdef       COM_REGMAP
> +#ifndef      COM_NO_REGMAP

What is the reason to make the flag reversed?

> +     uint32_t                cr_flags;
> +#define COM_REGS_ALIGN4      0x01    

"ALIGN4" seems a bit confusing. "4BYTE_STRIDE" or other name?

> +#if BYTE_ORDER == BIG_ENDIAN
> +#define COM_INIT_REGS_OFFSET 3
> +#else
> +#define COM_INIT_REGS_OFFSET 0
> +#endif

As Dennis Ferguson wrote, I doubt this will work on PCI based ones.
IIRC PCI spec requires that the same byte registers must be
accecced via the same addresses on both endian.

Such byte lane offset adjust ment is required only if
- byte registers are wired to LSByte in 32bit bus
- byte lane wiring are swapped by hardware (like osiop(4))
but most (all?) PCI bus_space(9) implementations swap
byteorder of PCI access by software (i.e. in MD bus_space(9)).

I don't know what's done on bi-endian SoC chips, but I think
such byte lane offset should be switched by MD flags like
COM_REGS_ALIGN4.

> --- dev/ic/com.c      22 Dec 2013 18:20:46 -0000      1.322
> +++ dev/ic/com.c      9 Feb 2014 17:43:24 -0000
 :
> -comprobe1(bus_space_tag_t iot, bus_space_handle_t ioh)
> +comprobe1(bus_space_tag_t iot, bus_space_handle_t ioh, int align)

This new arg should also be "stride" rather than "align"?

> --- dev/pci/puc.c     7 Feb 2014 11:51:00 -0000       1.37
> +++ dev/pci/puc.c     9 Feb 2014 17:43:24 -0000
> @@ -206,8 +206,13 @@ puc_attach(device_t parent, device_t sel
>                   &sc->sc_bar_mappings[i].t, &sc->sc_bar_mappings[i].h,
>                   &sc->sc_bar_mappings[i].a, &sc->sc_bar_mappings[i].s)
>                     == 0);
> -             if (sc->sc_bar_mappings[i].mapped)
> +             if (sc->sc_bar_mappings[i].mapped) {
> +               if (type == PCI_MAPREG_TYPE_IO)
> +                 aprint_normal_dev(self, "I/O\n");
> +               else
> +                 aprint_normal_dev(self, "MEM\n");
>                       continue;
> +             }

Wrong Indent (or unnecessary debug printf)?

> --- dev/pci/puccn.c   26 Jan 2014 10:54:24 -0000      1.13
> +++ dev/pci/puccn.c   9 Feb 2014 17:43:24 -0000
 :
> @@ -86,8 +87,9 @@ static bus_addr_t pucgdbbase;
>   * resuming the search where it left off, never retrying the same adaptor.
>   */
> 
> +#include <machine/pio.h>

I doubt we can pull this MD header in MI driver.

> +     COM_INIT_REGS(sc->sc_regs, iot, ioh, iobase, sizeof(uint8_t));

I wonder which is better explicit "1" or sizeof(uint8_t)
if this means stride.  (you use "1" and "4" in com_puc.c)

> --- arch/hp300/dev/com_dio.c    28 Apr 2008 20:23:19 -0000      1.8
> +++ arch/hp300/dev/com_dio.c    9 Feb 2014 17:43:21 -0000

At least arch/hp300/dev/com_frodo.c was missed?

---
Izumi Tsutsui