Re: 4byte aligned com(4) and PCI_MAPREG_TYPE_MEM

[Masanobu SAITOH <msaitoh%execsw.org@localhost>]

Hi.

(2014/02/15 1:58), Izumi Tsutsui wrote:
> I'd suggest to clarify what problems you are trying to solve
> and consider how it should be solved, before updating your patch.
>
> The problems you mentioned are:
> (1) merge initialization of sparse register mappings (with 4 byte stride)

Right.

> (2) defer consinit() for puc com to use uvm_km_alloc() in it

Right though the real goal is not to defer consinit but to support
memory mapped I/O in com(4).

And,

  (3) Almost all drivers don't clear "struct com_regs regs". It's not
     a real bug but should be cleard for further change.

> Your patch is trying to solve them by:
> (1) change COM_INIT_REGS() (and comprobe1()) APIs to pass stride byte
> (2) add an MI "extent_initted" global variable and check it in MD consinit()
>
> My vote is:
> (1) leave existing APIs and handle the quirk in MD attachment
> (2) add an x86 specific MD variable to defer consinit() till cpu_startup()

 I think "static int iniited" variable is used to do that, but there
is no way to know whether extent_init() (or uvm_init())  is called
or not.

> Because:
> (1)
>   - it's really pain to change the MI APIs
>     (so many attachments and most of them will rarely be tested unfortunately)
>   - only three or four attachments can share the new API
>     while such embedded devices often might have more other quirks

 Before writing that patch, I wrote another patch which did't change
the arguments of COM_INIT_REGS(). We have two choices

        a) Use COM_INIT_REGS() for all drivers.
        b) Copy COM_INIT_REGS() and modify to support 4 byte stride.

I couldn't decice which one was better or not. Now I think not change
the argument and make a new macro is better than adding new argument,
because considering the byte order is not required for 1 byte slide
devices.

>   - even if stride handling is really necessary in MI part,
>     it's much better to prepare new wrap functions,
>     like wdcprobe1() and wdcprobe() in wdc.c
>     (i.e. prepare a new COM_INIT_REGS_STRIDE() macro with a new arg
>      and make exiting COM_INIT_REGS() macro use it)
>
> (2)
>   - it's unclear what functions actually require the extent_init()
>     (I guess uvm_init() is enough to call uvm_km_alloc())

 Me neither... I had thought that uvm_init() was enough, but someone
advived me that exten_init() should be called.

>   - in general MI code assumes that console devices are properly
>     mapped by MD bootstrap or its firmware

 Yes. But, it's little hard to make such mapping as PCI bus_space_tag
and handle in eary boot stage.

>   - some ports already has MD flags to switch malloc(9) or static
>     memory in early device mappings and initialization

 It can be used, but IMHO, checking whether bus_space_map() can be
called is more generic than it.

> Just my two cents,
>
> ---
> Izumi Tsutsui

 Please give me a little time to write another patch.

--
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)