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
