Re: 4byte aligned com(4) and PCI_MAPREG_TYPE_MEM

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

msaitoh@ wrote:

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

It's unclear what's your plan of migration from the temporary workaround
in your patch to the real goal.

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

Can you describe this more specific?

In which part of your patch?
What does "clear" mean?

All softc stuff is allocated by kmem_zalloc(9) and com_regs for console
are allocated as static.

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

No reason to reuse such stupid MD workaround.

You can simply remove it and add and check a new MD bool global variable
that will be set in MD cpu_startup().

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

COM_INIT_REGS() is a macro for the standard (contiguous byte) layout.
It's pain to change it as I wrote before.

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

If there is "standard 4-byte stride layout" it would be worth to
add a new macro, but I doubt that there is any such standard.
You can not use #if BYTE_ORDER in the common macro because
byte lane wiring and its addressing are hardware implementation
dependent, not endianness dependent (as seen in PCI implementation).

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

My point is:

"If you have the real goal (early MD init for the console devices)
 but you'd like to add temporary workaround for now,
 such workaround should not be put into MI sources."

as suggested in my vote:
> > (2) add an x86 specific MD variable to defer consinit() till cpu_startup()

I don't see benefit to add MI "initted" global variable into the MI extent.
(it's still better to add an "is_initted" like function than global)

---
Izumi Tsutsui