hi! Allen, Jason.

Is a problem to change?


From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Date: Wed, 25 Aug 2004 19:55:34 +0900

> In article <20040825.003452.74747022.kiyohara@kk.iij4u.or.jp>
> kiyohara@kk.iij4u.or.jp wrote:
> 
> > From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
> > Date: Tue, 24 Aug 2004 23:23:10 +0900
> > 
> > > In article <20040824.013249.104033134.kiyohara@kk.iij4u.or.jp>
> > > kiyohara@kk.iij4u.or.jp wrote:
>  :
> > > Ah, I see, you mean that the firmware doesn't set
> > > PCI_COMMAND_SERR_ENABLE and PCI_COMMAND_PARITY_ENABLE
> > > but sys/dev/pci/pciconf.c:configure_bus() sets them implicitly,
> > > so we have to disable them again after pci_configure_bus(9), right?
> > >
> > > Hmm, I wonder if we should have some flags (in pci_conf_hook()?)
> > > to disable them in MI pci_configure_bus(), but for now I think
> > > it's okay to do it for each devices in gt_attach().
>  :
> > I am making the following change.
> > It will also influence MI. However, I think it strange that it is enabled
> > unconditionally.
> > 
> > MI must demand change.
> 
> Well, it's always good thing to make things done in MI,
> but if we would like to change MI code, we have to consult
> responsible people first ;-)
> 
> I thought the similar fix with your patch (adding PCI_CONF_ENABLE_PARITY
> and PCI_CONF_ENABLE_SERR in pciconf.h), but I wonder if the changes
> breaks existing code because:
> 
> - pciconf.c:setup_iowins() and setup_memwins() could implicitly
>   set pd->enable = 0 so added flags might be cleared:
> 
> 		if (!pb->io_32bit && pi->address > 0xFFFF) {
> 			pi->address = 0;
> 			pd->enable = 0;
> 		}
>  :
> 		if (pm->prefetch && !pb->pmem_64bit &&
> 		    pm->address > 0xFFFFFFFFULL) {
> 			pm->address = 0;
> 			pd->enable = 0;
> 
> - and pciconf.c:configure_bus() checks if pd->enable is zero:
> 
> 		if (!(pd->enable)) {
> 			print_tag(pd->pc, pd->tag);
> 			printf("Disabled due to lack of resources.\n");
> 			cmd &= ~(PCI_COMMAND_MASTER_ENABLE |
> 			    PCI_COMMAND_IO_ENABLE | PCI_COMMAND_MEM_ENABLE);
> 		}
> 
> (BTW, setup_iowins() always set PCI_CONF_ENABLE_IO even
>  after pd->enable is cleared...)
> 
> I guess setup_iowins() and setup_menwins() should clear only
> PCI_COMMAND_MEM_ENABLE or PCI_COMMAND_IO_ENABLE and
> configure_bus() should check only these two flags, but I'm not sure.
> 
> Allen, Jason, how do you think about this change?
> ---
> Izumi Tsutsui
> tsutsui@ceres.dti.ne.jp
> 
> Index: pciconf.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/pci/pciconf.c,v
> retrieving revision 1.23
> diff -u -r1.23 pciconf.c
> --- pciconf.c	17 Mar 2004 20:27:57 -0000	1.23
> +++ pciconf.c	25 Aug 2004 10:50:30 -0000
> @@ -705,10 +705,6 @@
>  			    PRIu64 " req)\n", pi->size);
>  			return -1;
>  		}
> -		if (!pb->io_32bit && pi->address > 0xFFFF) {
> -			pi->address = 0;
> -			pd->enable = 0;
> -		}
>  		if (pd->ppb && pi->reg == 0) {
>  			pd->ppb->ioext = extent_create("pciconf", pi->address,
>  			    pi->address + pi->size, M_DEVBUF, NULL, 0,
> @@ -721,7 +717,12 @@
>  			}
>  			continue;
>  		}
> -		pd->enable |= PCI_CONF_ENABLE_IO;
> +		if (!pb->io_32bit && pi->address > 0xFFFF) {
> +			pi->address = 0;
> +			pd->enable &= ~PCI_CONF_ENABLE_IO;
> +		} else {
> +			pd->enable |= PCI_CONF_ENABLE_IO;
> +		}
>  		if (pci_conf_debug) {
>  			print_tag(pd->pc, pd->tag);
>  			printf("Putting %" PRIu64 " I/O bytes @ %#" PRIx64
> @@ -775,7 +776,7 @@
>  		if (pm->prefetch && !pb->pmem_64bit &&
>  		    pm->address > 0xFFFFFFFFULL) {
>  			pm->address = 0;
> -			pd->enable = 0;
> +			pd->enable &= ~PCI_CONF_ENABLE_MEM;
>  		} else {
>  			pd->enable |= PCI_CONF_ENABLE_MEM;
>  		}
> @@ -1005,7 +1006,10 @@
>  		class = pci_conf_read(pd->pc, pd->tag, PCI_CLASS_REG);
>  		misc = pci_conf_read(pd->pc, pd->tag, PCI_BHLC_REG);
>  		cmd = pci_conf_read(pd->pc, pd->tag, PCI_COMMAND_STATUS_REG);
> -		cmd |= PCI_COMMAND_SERR_ENABLE | PCI_COMMAND_PARITY_ENABLE;
> +		if (pd->enable & PCI_CONF_ENABLE_PARITY)
> +			cmd |= PCI_COMMAND_PARITY_ENABLE;
> +		if (pd->enable & PCI_CONF_ENABLE_SERR)
> +			cmd |= PCI_COMMAND_SERR_ENABLE;
>  		if (pb->fast_b2b)
>  			cmd |= PCI_COMMAND_BACKTOBACK_ENABLE;
>  		if (PCI_CLASS(class) != PCI_CLASS_BRIDGE ||
> @@ -1022,7 +1026,8 @@
>  			cmd |= PCI_COMMAND_MASTER_ENABLE;
>  			ltim = MIN (pb->def_ltim, pb->max_ltim);
>  		}
> -		if (!(pd->enable)) {
> +		if ((pd->enable &
> +		    (PCI_CONF_ENABLE_MEM|PCI_CONF_ENABLE_IO)) == 0) {
>  			print_tag(pd->pc, pd->tag);
>  			printf("Disabled due to lack of resources.\n");
>  			cmd &= ~(PCI_COMMAND_MASTER_ENABLE |
> Index: pciconf.h
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/pci/pciconf.h,v
> retrieving revision 1.7
> diff -u -r1.7 pciconf.h
> --- pciconf.h	28 Sep 2002 10:31:02 -0000	1.7
> +++ pciconf.h	25 Aug 2004 10:50:30 -0000
> @@ -55,5 +55,7 @@
>  #define PCI_CONF_ENABLE_IO	0x08
>  #define PCI_CONF_ENABLE_MEM	0x10
>  #define PCI_CONF_ENABLE_BM	0x20
> +#define PCI_CONF_ENABLE_PARITY	0x40
> +#define PCI_CONF_ENABLE_SERR	0x80
>  
> -#define PCI_CONF_ALL		0x3f
> +#define PCI_CONF_ALL		0xff