Izumi Tsutsui wrote:
> Sorry for a bit late response, but I'd appreciate if I could have
> more time for review before merge. Thanks,
>
> garrett_damore@tadpole.com wrote:
>
>   
>>> - Why don't you put map[] member to struct com_softc?
>>>       
>> Because lots of com routines don't actually have/use the softc. 
>> Particularly stuff that might be used with console/kgdb support.  This
>> common structure lives in the softc, and can also live outside the softc
>> for use in code that doesn't have the softc.
>>     
>
> IMHO, it looks a bit inconsistent to have both functions,
> ones take com_softc and the others do com_regs.
> (of course current implementation which passes only iot, ioh
>  and iobase is also ugly though)
>   
> Actually you have changed such functions (which don't take com_softc)
> into wrapper ones to prepare struct com_regs and create other real
> functions which take struct com_regs.
> Why don't you make them wrapper functions which prepare struct softc?
> How about structures like this?
>   

The "old" routines are around for compatibility.  Preparing a struct
softc is a bigger deal and may have other impacts.   I was shooting for
the least effort, admittedly.  But then I wasn't exactly making these
changes in a vacuum either -- there was a lot of preexisting code to modify.

A lot of external MD code used this stuff too.

struct com_regs contains the minimum amount of information to implement
the CSR access routines.  Since those other routines that take a handle
and tag do the same thing, rather than adding a new argument to them, I
converted them to take take the com_regs instead.

Actually, I made new ones that could take the com_regs, and left the old
ones around as wrappers to minimize impact on MD code.

> ---
> struct com_regs {
> 	bus_space_tag_t		cr_iot;
> 	bus_space_handle_t	cr_ioh;
> 	bus_addr_t		cr_iobase;
> 	bus_size_t		cr_nports;
> 	bus_size_t		cr_regmap[16];
> };
>
> struct com_softc {
> 	struct device sc_dev;
> 	void *sc_si;
> 	struct tty *sc_tty;
>
> 	struct callout sc_diag_callout;
>
> 	int sc_frequency;
>
> 	struct com_regs sc_regs;
> #define sc_iot		sc_regs.cr_iot
> #define sc_ioh		sc_regs.cr_ioh
> #define sc_iobase	sc_regs.cr_iobase
> #define sc_nports	sc_regs.cr_nports
> #define sc_regmap	sc_regs.cr_regmap
>
> 	bus_space_handle_t sc_hayespioh;
>  :
>
> ---
>  :
> #define	CSR_WRITE_1(sc, o, v)	\
> 	bus_space_write_1((sc)->sc_iot, (sc)->sc_ioh, (sc)->sc_regmap[o], v)
> #define	CSR_READ_1(sc, o)	\
> 	bus_space_read_1((sc)->sc_iot, (sc)->cr_ioh, (sc)->cr_map[o])
>  :
> int
> cominit(struct com_softc *sc, int rate, int frequency, int type,
>     tcflag_t cflag)
> {
>
> 	if (bus_space_map(sc->sc_iot, sc->sc_iobase, sc->sc_nports, 0,
> 		&sc->sc_ioh))
> 		return (ENOMEM); /* ??? */
>
> 	rate = comspeed(rate, frequency, type);
> 	if (type != COM_TYPE_AU1x00) {
> 		/* no EFR on alchemy */
> 		CSR_WRITE_1(sc, COM_REG_LCR, LCR_EERS);
>  :
> }
>  :
> int
> comcnattach1(struct com_softc *sc, int rate, int frequency, int type,
>     tcflag_t cflag)
> {
> 	int res;
>
> 	comconsregs = sc->sc_reg;
>
> 	res = cominit(sc, rate, frequency, type, cflag);
>  :
> }
>
> int
> comcnattach(bus_space_tag_t iot, bus_addr_t iobase, int rate, int frequency,
>     int type, tcflag_t cflag)
> {
> 	struct com_softc sc_store;
> 	struct com_softc *sc = &sc_store;
>
> 	sc->sc_iot = iot;
> 	sc->sc_iobase = iobase;
> 	sc->sc_nports = COM_NPORTS;
> #ifdef	COM_REGMAP
> 	memcpy(sc->sc_regmap, com_std_map, sizeof(sc->sc_regmap));
> #endif
>
> 	return comcnattach1(sc, rate, frequency, type, cflag);
> }
>
> ---
>
> In this implementation, you have to copy struct com_regs
> from comconsregs to com_softc for com_common_{put,get}c().
> If you don't like the copy, we could have some hacked
> struct com_regs which can be passed as struct softc:
>
> ---
> struct com_regs {
> 	/* these members should be sync'ed with ones in struct com_softc */
> 	struct device		cr_dev;	/* dummy */
>
> 	bus_space_tag_t		cr_iot;
> 	bus_space_handle_t	cr_ioh;
> 	bus_addr_t		cr_iobase;
> 	bus_size_t		cr_nports;
> 	bus_size_t		cr_regmap[16];
> };
>
> struct com_softc {
> 	struct device sc_dev;
>
> 	bus_space_tag_t sc_iot;
> 	bus_space_handle_t sc_ioh;
> 	bus_addr_t sc_iobase;
> 	bus_size_t sc_nports;
> 	bus_size_t sc_regmap[16];
>
> 	bus_space_handle_t sc_hayespioh;
>
> 	void *sc_si;
> 	struct tty *sc_tty;
>
> 	struct callout sc_diag_callout;
>
> 	int sc_frequency;
>  :
> ---
>
> I'm not sure if these implementations are good idea
> (because I'm not a programmer but just a dumb reader),
> but I think it could reduce changes from -current common style.
> (note I don't test these changes at all though)
>   

Heh.  The original com code "deviates" by having these routines which
take a bus_tag_t and a bus_addr_t.  Its ugly.  I'd like to clean them up
even further, but that is even more change.  I'm trying to minimize the
number of changes I have to make.  I spent pretty much an entire workday
yesterday touching a bunch of code that I can't even test.

>   
>>> - Where is "COM_REGSMAP" defined in your code?
>>>       
>> In conf/files (or rather, opt_com.h)
>>
>>     
>>>   Is it possible to define it only if a particular MD
>>>   attachment which requires special layout is configured?
>>>       
>> Yes.  Stick it in MD config file (e.g. I have it in files.alchemy and
>> files.atheros, but not in i386.)
>>     
>
> Hmm. What happens if we have to support another com variant which
> is attached to MI bus but also has odd regster mapping?
>   

Then you better define COM_REGSMAP.   Right now the "weird" variants are
all on MD busses.

>   
>>> - Do all MD attachments have to initialize map[] array
>>>   if COM_REGSMAP is defined?
>>>       
>> No, there is a default "COM_INIT_REGS" that initializes this for
>> standard 16550 layout.
>>     
>
> Well, you are adding COM_INIT_REGS() macro in _all_ MD attachments.
> That was my concern. Some people may complain about it if they have
> their own private drivers, or if you miss some attachments on the merge.
>
> I wonder if we can initialize the regmap array in com_attach_subr()
> according to some proper quirk flag in softc:
> ---
> 	if ((sc->sc_flags & COM_F_HAVE_OWN_REGMAP) == 0)
> 		COM_INIT_REGMAP(sc->sc_regmap);
> ---
>
> Then I guess we could use most current MD attachments without
> extra changes.
>   

I considered this even.   But I decided to go and touch all the MD
attachments, because frankly a bunch of them go and modify the softc
structure directly anyway.

    -- Garrett
> ---
> Izumi Tsutsui
>   


-- 
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: 951 325-2134  Fax: 951 325-2191