Re: 4byte aligned com(4) and PCI_MAPREG_TYPE_MEM

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

Hello, Dennis, Tsutusi and others.

 I'm sorry that I didn't reply quickly. While I was
absent, this thread became long :-)
I was unable to make a decision which mail I should reply to...

 As I write below, adding byte order check for PCI devices
is my fault. So I'd reply to this mail to separate com's change
from bus_space's discussion.

(2014/02/10 23:54), Izumi Tsutsui wrote:
> 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?

It's hpcmips/dev/com_hpcio.c. It shold work with my patch.

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

I can't remember such discussion, sorry. And, I'm not familiar with
other arch's consinit(), so I have no opinion about it.

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

 The intention is for people who don't want to use current COM_REGMAP.
COM_REGMAP adds small overhead. For some old machine which has no
extensibility, COM_NO_REGMAP is usefu to avoid such overhead.
If there is no objection, COM_NO_REGMAP can be removed.

>> +    uint32_t                cr_flags;
>> +#define COM_REGS_ALIGN4     0x01    
> 
> "ALIGN4" seems a bit confusing.

 I agree with you. When I was writing the code, I thought it's not
suit the meaning, but I couldn't think any altenative :-|

> "4BYTE_STRIDE" or other name?

might be better than align.


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

As I wrote above, it's my fault. I'll modify my patch to remove
such #ifdefs from PCI stuff.



>> --- 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)?

Wrong Indent.

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

Sorry. It was a part of test code with LED blink code using with inl(), outl()
because it's reuired to debug serial console function on Intel Galileo
(it has no VGA video). I'll remove it.

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

When the first time I made that patch it was "1" and "4".
And when I saw the code:

        COM_INIT_REGS(sc->sc_regs, iot, ioh, iobase, 1);

I thought that someone should think "What is 1 mean? Number or flag?",
so I rewrite to use sizeof() to make clear it's not a flag.

 I'd like to know other people's thought.

>   (you use "1" and "4" in com_puc.c)

inconsistent.

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

Right. My falut. I'll add in the next patch.

> ---
> Izumi Tsutsui
> 


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

  * 英語 - 自動検出
  * 英語
  * 日本語

  * 英語
  * 日本語

 <javascript:void(0);> <#>