Re: 2.4 stuff.

Re: 2.4 stuff.

[Erwin Authried]

David Woodhouse[SMTP:dwmw2@infradead.org] wrote:
> eauth@softsys.co.at said:
> > * compatmac.h: For 2.0, the inter_module_* functions are defined as
> > empty macros if CONFIG_MODULES is not defined. If CONFIG_MODULES is
> > defined (for 2.0), compilation is stopped with #error saying that it's
> > not possible to use MTD in 2.0 kernels with module support enabled.
> 
> Careful not to make the #error break stuff that does actually work. 
> If you're compiling in all the stuff that you're actually going to need, 
> but also have CONFIG_MODULES, it should all work, shouldn't it?
> 
Yes, you are right, that's really too restrictive. You can use modules in 
general, but MTD must be linked statically. 

> It might be better to define the inter_module_get() functions to return 
> NULL but printk a warning that they've been used - which should explain to 
> the user why it's not actually working for them.
> 
Nevertheless, I think that configuration of non-working stuff should be
disallowed by the configuration script, or by aborting the compilation. 
Why should we guide the user into a trap if we know it doesn't work?

-Erwin



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[David Woodhouse]

eauth@softsys.co.at said:
> Nevertheless, I think that configuration of non-working stuff should
> be disallowed by the configuration script, or by aborting the
> compilation.  Why should we guide the user into a trap if we know it
> doesn't work?

Agreed. Would it suffice to do this in Config.in for the 2.0 version?

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[Erwin Authried]

David Woodhouse[SMTP:dwmw2@infradead.org] wrote:
> 
> eauth@softsys.co.at said:
> > Nevertheless, I think that configuration of non-working stuff should
> > be disallowed by the configuration script, or by aborting the
> > compilation.  Why should we guide the user into a trap if we know it
> > doesn't work?
> 
> Agreed. Would it suffice to do this in Config.in for the 2.0 version?
> 
I'm not sure about 2.2, but I believe all the module related stuff is working
there. At least, we shouldn't care about 2.1.

-Erwin



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[Erwin Authried]

I had to make a few changes to the inter_module stuff to get 
MTD working for uClinux.The changes are explained below. 
They are not yet comitted, please let me know what you think.

-Erwin

* map_rom.c
There is a im_name member used in map_rom.c, line 51. Has this
been added by mistake? In mtd.h, there's no such member.

* compatmac.h:
For 2.0, the inter_module_* functions are defined as empty macros
if CONFIG_MODULES is not defined. If CONFIG_MODULES is defined
(for 2.0), compilation is stopped with #error saying that it's not possible
to use MTD in 2.0 kernels with module support enabled.

* I have removed the "static" for the cfi_cmdset_0001/0002 probe functions.

* I have added a switch statement for compilation without module support
  in cfi_probe.c:

+#ifdef CONFIG_MODULES
        sprintf(probename, "cfi_cmdset_%4.4X", type);

        probe_function = inter_module_get_request(probename, probename);
@@ -743,6 +747,20 @@
                (*probe_function)(map, primary, base);
                return;
        }
+#else
+       switch(type){
+#ifdef CONFIG_MTD_CFI_INTELEXT
+       case 0x0001:
+               cfi_cmdset_0001(map,primary,base);
+               return;
+#endif
+#ifdef CONFIG_MTD_CFI_AMDSTD
+       case 0x0002:
+               cfi_cmdset_0002(map,primary,base);
+               return;
+#endif
+       }
+#endif /* CONFIG_MODULES */




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[David Woodhouse]

eauth@softsys.co.at said:
>  * map_rom.c There is a im_name member used in map_rom.c, line 51. Has
> this been added by mistake? In mtd.h, there's no such member. 

Mea Culpa. Should be map->im_name, not mtd->im_name. Fixed. Thanks.


eauth@softsys.co.at said:
> * compatmac.h: For 2.0, the inter_module_* functions are defined as
> empty macros if CONFIG_MODULES is not defined. If CONFIG_MODULES is
> defined (for 2.0), compilation is stopped with #error saying that it's
> not possible to use MTD in 2.0 kernels with module support enabled.

Careful not to make the #error break stuff that does actually work. 
If you're compiling in all the stuff that you're actually going to need, 
but also have CONFIG_MODULES, it should all work, shouldn't it?

It might be better to define the inter_module_get() functions to return 
NULL but printk a warning that they've been used - which should explain to 
the user why it's not actually working for them.

eauth@softsys.co.at said:
>  * I have removed the "static" for the cfi_cmdset_0001/0002 probe
> functions. 

OK.

eauth@softsys.co.at said:
>  * I have added a switch statement for compilation without module
> support in cfi_probe.c: 

Looks sensible, and could also help to get rid of the ugly link order 
dependencies that the inter_module_xxx stuff has introduced. I've modified 
it to:

	switch(type){
#ifdef CONFIG_MTD_CFI_INTELEXT
	case 0x0001:
		cfi_cmdset_0001(map,primary,base);
		return;
#endif
#ifdef CONFIG_MTD_CFI_AMDSTD
	case 0x0002:
		cfi_cmdset_0002(map,primary,base);
		return;
#endif
	default:
#ifdef CONFIG_MODULES
		sprintf(probename, "cfi_cmdset_%4.4X", type);
		
		probe_function = inter_module_get_request(probename, probename);
		if (probe_function) {
			(*probe_function)(map, primary, base);
			return;
		}	
#endif
	}


--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

2.4 stuff.

[David Woodhouse]

OK, I've just merged the inter_module_xxx changes from 2.4 as I said I was 
going to do last week - yes it took me over a week to do it because I got 
distracted and fixed PCMCIA in 2.4 instead.

This will break the build on 2.2 if you don't have Keith's inter_module_xxx 
patch applied. But you had to apply a patch anyway to make 
put_module_symbol() work. Hopefully, it'll end up in 2.2.1[89] and we'll 
all be happy. 

2.2.17 and above also contain the new module_init() code so I've (at least
partially) carried out my threat to stop maintaining the initcalls. If you
care, fix it. If you care and don't have CVS write access, send me a SSH
public key.

Note that a handful of drivers now ought to put the module_init() version 
inside #if LINUX_VERSION_CODE >= 0x20211 instead of 0x20300

My current plan is to give Linus a selective update of the MTD code
some time early in the 2.4 series. In it, I'm intending to include:
	- mtdpart stuff
	- New mtdblock
	- New NFTL
	- New DiskOnChip driver, _preferably_ with DOC_SINGLE_DRIVER
	- API changes - set_vpp() and anything else that's momentarily
		slipped my notoriously vague attention.
	- Any new map drivers which are appropriate - probably the
		SA1100 one. 
	- New CFI code iff it actually works next time I try it on my
		boards :)

Speak up if there's something missing which you think ought to be included, 
or alternatively if there's something in there which you think I ought to 
omit.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[Nicolas Pitre]

On Tue, 21 Nov 2000, David Woodhouse wrote:

> 	- New CFI code iff it actually works next time I try it on my
> 		boards :)

IMHO this is a must.  Since I'm probably the one who made it break for your
board please don't hesitate to bother me with that so we could fix it.


Nicolas



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[David Woodhouse]

nico@cam.org said:
>  IMHO this is a must.  Since I'm probably the one who made it break
> for your board please don't hesitate to bother me with that so we
> could fix it. 

OK, I've tested v1.46 on the same board with non-interleaved 16-bit flash
where it previously failed. It now appears to work.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[David Woodhouse]

nico@cam.org said (Re CFI code):
>  IMHO this is a must.  Since I'm probably the one who made it break
> for your board please don't hesitate to bother me with that so we
> could fix it.

OK, I've just given Linus a fairly large update for 2.4.0-test12. It's at 
ftp.uk.linux.org:/pub/people/dwmw2/forlinus/mtd-patch-whole

It's got everything in it that I said I was going to send except

1. New CFI code. That's not because I'm intending to leave the CFI code out,
but before I do it, because I wanted to let the discussion about block write
sizes settle, and test the AMD command set code on the new toy that landed
on my desk on Friday.

2. New map drivers. I haven't decided which ones to add yet, if any.



--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[Nicolas Pitre]

On Tue, 12 Dec 2000, David Woodhouse wrote:

> 1. New CFI code. That's not because I'm intending to leave the CFI code out,
> but before I do it, because I wanted to let the discussion about block write
> sizes settle, and test the AMD command set code on the new toy that landed
> on my desk on Friday.

I think the block write discussion has settled on the current code.


Nicolas



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[David Woodhouse]

nico@cam.org said:
>  I think the block write discussion has settled on the current code.

Cool. I'll get the AMD code up and running on the new toy on my desk and
send it off to Linus then.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[Nicolas Pitre]

On Wed, 13 Dec 2000, David Woodhouse wrote:

>
> nico@cam.org said:
> >  I think the block write discussion has settled on the current code.
>
> Cool. I'll get the AMD code up and running on the new toy on my desk and
> send it off to Linus then.

I just looked at the cfi_cmdset_0002.c code. This will goof on big endian as
far as I can see.  A major cleanup, like what happened for
cfi_cmdset_0001.c, would be welcome there as well.

First, those functions could be cleaned out:

amd_send_cmd_at()
-----------------
- referenced only once;
- it applies cpu_to_le() on values which have already been correctly
  converted in cfi_build_cmd()... oops!;
- it should use cfi_write() instead of the current
  switch(map->buswidth) {...};
- probably diserve to be killed anyway.

amd_write_val()
---------------
- cfi_write() already does the same.

amd_read_val()
--------------
- cfi-read() already does the same.

Next, cfi_amdstd_write_byte_16() and cfi_amdstd_write_bytes_32() could
greatly benefit from being generalized i.e. merged together and even folded
into cfi_amdstd_write().  The code in cfi-cmdset-001.c should be a great
example for that.  It also removes all the #ifdef noise while still
preserving the ability for the compiler to optimize away the unused code
when a specific bus geometry is selected. In addition, such code as

	tmp = map->read32(map, ofs);
	tmp = be32_to_cpu(tmp);
	memcpy((u_char *)&tmp, buf, len);
	tmp = cpu_to_be32(tmp);

or ...

	tmp = map->read32(map, ofs - 1);
	tmp = (be32_to_cpu(tmp) & 0xFF000000) | buf[2] | (buf[1]  << 8) | buf[0] << 16;
	tmp = cpu_to_be32(tmp);

looks quite awful and suspicious to me.

Next, all calls to udelay() should be combined with a test on
current->need_resched and call schedule() instead of udelay() when true.
Otherwise kiss goodbye to your system's low latency when writes occur.

The last observation is the difference between cmdset-0001 and cmdset-002
for the state in which the task is put while waiting for the chip.  For
cmdset-001 the task is set to TASK_UNINTERRUPTIBLE for some reasons which
are surely valuable for cmdset-002 as well.

So enough criticism for now.  I would have been happy to clean this up
myself, however I don't have any toys with AMD flash in it so couldn't test
the changes at all which lower my motivation to do such work...


Nicolas



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[Alice Hennessy]

Nicolas Pitre wrote:

> On Wed, 13 Dec 2000, David Woodhouse wrote:
>
> >
> > nico@cam.org said:
> > >  I think the block write discussion has settled on the current code.
> >
> > Cool. I'll get the AMD code up and running on the new toy on my desk and
> > send it off to Linus then.
>
> I just looked at the cfi_cmdset_0002.c code. This will goof on big endian as
> far as I can see.  A major cleanup, like what happened for
> cfi_cmdset_0001.c, would be welcome there as well.
>
> First, those functions could be cleaned out:
>
> amd_send_cmd_at()
> -----------------
> - referenced only once;
> - it applies cpu_to_le() on values which have already been correctly
>   converted in cfi_build_cmd()... oops!;
> - it should use cfi_write() instead of the current
>   switch(map->buswidth) {...};
> - probably diserve to be killed anyway.
>
> amd_write_val()
> ---------------
> - cfi_write() already does the same.
>
> amd_read_val()
> --------------
> - cfi-read() already does the same.
>
> Next, cfi_amdstd_write_byte_16() and cfi_amdstd_write_bytes_32() could
> greatly benefit from being generalized i.e. merged together and even folded
> into cfi_amdstd_write().  The code in cfi-cmdset-001.c should be a great
> example for that.  It also removes all the #ifdef noise while still
> preserving the ability for the compiler to optimize away the unused code
> when a specific bus geometry is selected. In addition, such code as
>
>         tmp = map->read32(map, ofs);
>         tmp = be32_to_cpu(tmp);
>         memcpy((u_char *)&tmp, buf, len);
>         tmp = cpu_to_be32(tmp);
>
> or ...
>
>         tmp = map->read32(map, ofs - 1);
>         tmp = (be32_to_cpu(tmp) & 0xFF000000) | buf[2] | (buf[1]  << 8) | buf[0] << 16;
>         tmp = cpu_to_be32(tmp);
>
> looks quite awful and suspicious to me.
>
> Next, all calls to udelay() should be combined with a test on
> current->need_resched and call schedule() instead of udelay() when true.
> Otherwise kiss goodbye to your system's low latency when writes occur.
>
> The last observation is the difference between cmdset-0001 and cmdset-002
> for the state in which the task is put while waiting for the chip.  For
> cmdset-001 the task is set to TASK_UNINTERRUPTIBLE for some reasons which
> are surely valuable for cmdset-002 as well.
>
> So enough criticism for now.  I would have been happy to clean this up
> myself, however I don't have any toys with AMD flash in it so couldn't test
> the changes at all which lower my motivation to do such work...
>
> Nicolas
>
> To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

I've made some  minor local changes so that both cfi_cmdset_0002.c and
cfi_probe.c work for my big endian, 4 byte AMD flash board.   I can carry it further
by making it more like cfi_cmdset_0001.c  if you need a volunteer and don't need
it by tomorrow  :-)

Alice



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: 2.4 stuff.

[David Woodhouse]

ahennessy@mvista.com said:
>  I've made some  minor local changes so that both cfi_cmdset_0002.c
> and cfi_probe.c work for my big endian, 4 byte AMD flash board.   I
> can carry it further by making it more like cfi_cmdset_0001.c  if you
> need a volunteer and don't need it by tomorrow  :-) 

Thanks, that'll be useful. I'll be looking at it soon, but I haven't 
managed to get this board to respond to the CFI probe yet, so I'm not sure 
when I'll get to do it. Hopefully soon.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org