re: Yet another dirct config proposal for i2c busses
i like the look of this. have you tested it on anything besides sparc64 yet? .mrg.
Re: blocksizes
On Wed, Feb 03, 2010 at 08:53:14PM +0900, Izumi Tsutsui wrote: > says: > >> * A directory consists of some number of blocks of DIRBLKSIZ > >> * bytes, where DIRBLKSIZ is chosen such that it can be transferred > >> * to disk in a single atomic operation (e.g. 512 bytes on most machines). > > Probably DEV_BSIZE which could be smaller than physical sector size > might be able to handle atomic ops, but I wonder if someone actually > considered how it should work on >DEV_BSIZE sector media. With physical sectors larger than DEV_BSIZE you are still updating the directory blocks atomically, but obviously you may change multiple blocks of DIRBLKSIZ at the same time. > Is there any working code used on disks with different blocks sizes > in the past? Probably some code like msdosfs might work, but > not a whole kernel. msdofs was 'fixed' some time ago and used on USB media with different block sizes. ffs has been designed for different block sizes (but using native blocks for addressing). On other systems I have used UFS/FFS derivates (SunOS, Ultrix) with different block sizes 20 years ago. The original adosfs has been used on media with differnet block sizes. That doesn't say anything about our implementation, but different block sizes have been designed in from the beginning. lfs is pretty complex, I would imagine issues. cd9660 and ufs have always been used with different block sizes. If there are issues then with non-2K-block media :) No idea about nilfs or efs. Most issues I have seen were not in the main filesystem code but in the glue that gets it started (i.e. loading superblocks and deducing parameters from information blocks). VM code is somewhat unclear to me. While paging from file objects looks fine, I haven't looked at e.g. the device pager. > I wondered not only if current code would work or not but also > how all related APIs had been (or should be) designed and defined. > FFS sources used both DEV_BSIZE and fs_fsbtodb (or physical sector > size stored in disklabel) to access disk blocks, and you just "fixed" > the latter ones as "bugs". I mainly fixed code that accessed blocks in terms of sector sizes. The fsbtodb change is just a minor thing, in particular to match it with the driver API. > Yes, after fixing one more "bug" which was > there since initial import ffs seems working on a raw partition on > 4KB/sect disk, but there are still several sources that pass > physical block numbers stored in disklabel to I/O functions directly, > like subr_disk.c etc. Yes, there is some really weird code originating from some attempt of writing common code to bootstrap a disklabel, in particular from systems with MBR. Fortunately that's all in early phases of booting or accessing disks and as such, well contained. > --- ufs/ffs/ffs_vfsops.c 31 Jan 2010 10:54:10 - 1.256 > +++ ufs/ffs/ffs_vfsops.c 3 Feb 2010 11:07:52 - > @@ -1931,7 +1931,7 @@ > u_int32_t saveflag; > > error = ffs_getblk(mp->um_devvp, > - fs->fs_sblockloc >> (fs->fs_fshift - fs->fs_fsbtodb), FFS_NOBLK, > + fs->fs_sblockloc / DEV_BSIZE, FFS_NOBLK, > fs->fs_sbsize, false, &bp); > if (error) > return error; Please commit. -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: Yet another dirct config proposal for i2c busses
I like this approach. I'll try to find some time in the next few days
to build and run my amd64 boxes with your patches; at the very least,
that should verify that indirect config still works.
BTW, there are several other i2c drivers that do the address-and-mask
hack. :) Eventually, they should all be modified in the same way as
your spdmem.c changes...
On Wed, 3 Feb 2010, Martin Husemann wrote:
Folks,
we still have to solve the problem of "scanning" i2c busses, especially
on machines where no scan is needed since the firmware happily tells
us everything we might want to know.
In the past (as far as I remember) two proposal where presented and
both shot down. Now I have a machine where I realy needed the i2c
based fan controll to make the noise bearble - so I put on my asbestos
suit and wrote another implementation, which I'd like to present here.
The previous proposals:
- use the OpenBSD way: an optional "scan" callback provided by the
i2c controller driver. Downsides: needs changes ~every i2c controller
driver in-tree
- the macppc way: see macppc/dev/kiic* - basically a slightly different
bus, needs frontend/backend split of i2c device drivers and a lot
of additional frontends to be written
I may misrember details and critics raised against one of those.
Goals I tried to achieve:
- Allow both direct and indirect config at the i2c bus layer,
depending on availability of firmware provided locators
- Allow unmodified i2c device drivers to continue working
- Keep MD changes as simple and small as possible
- No changes to MI i2c bus controllers
- Allow MD i2c bus controllers to easily override the generic
behaviour (i.e.: provide additional locators or modify firmware
provided ones)
Seems like it worked out, and the changes are pretty small in the end.
Quick overview how it works:
- If we are doing direct config, MD code (via generic support routines,
or by overriding those) adds a prop_array to the device properties
of the i2c bus controller (the parent(!) of the i2c bus). This array
contains a dictionary for each i2c device on the bus. Entries in
this dictionary are:
"name" -> string, device name
"address" -> uint32, i2c address
"size" -> uint32 (optional)
"compatibility" -> a list of names, i.e. the chip used, used for
matching a hardware driver (think: alternative "name" props)
- When the i2c bus attaches, it queries the device properties of it's
parent device and checks the "i2c-child-devices" property (the array
described above), and if it is present, iterates over the array
creating i2c_attach_arg from it. To allow direct config matches,
the i2c_attach_args structure has been extended.
If the device property is not available, indirect config is done.
- An i2c device driver for a proper device will need no changes, but
for i.e. write-only devices matching based on strings can be added.
A generic helper function to match the "compatible" string list against
a driver specific list is provided.
A few more details:
Let's start with setting up the device property. In the attached sparc64 MD
code the setup is done inside device_register() whenever a "iic" device
attaches and there is not yet a "i2c-child-devices" property at the parent.
This check is needed to allow MD i2c controller drivers to override
the generic behaviour. For OpenFirmware based machines, a convenience
funtction "of_enter_i2c_devs()" to do the device property setup is provided.
Next step: the i2c bus attaches and checks for the device property. This is
all done in the i2c bus code, no i2c controller driver needs modifications.
Last part of the puzzle: the i2c device drivers can check for the (new)
ia_name pointer in the i2c_attach_args structure to find out if direct
config is available. For example the spdmem driver does a (nasty/stupid?)
check for certain address values - which does not make any sense in the
direct config case:
@@ -164,8 +165,17 @@ spdmem_match(device_t parent, cfdata_t m
int spd_len, spd_crc_cover;
uint16_t crc_calc, crc_spd;
- if ((ia->ia_addr & SPDMEM_ADDRMASK) != SPDMEM_ADDR)
- return 0;
+ if (ia->ia_name) {
+ /* add other names as we find more firmware variations */
+ if (strcmp(ia->ia_name, "dimm-spd"))
+ return 0;
+ }
+
+ /* only do this lame test when not using direct config */
+ if (ia->ia_name == NULL) {
+ if ((ia->ia_addr & SPDMEM_ADDRMASK) != SPDMEM_ADDR)
+ return 0;
+ }
sc.sc_tag = ia->ia_tag;
sc.sc_addr = ia->ia_addr;
If the firmware name is not a good indicator of the driver to use, the
"compatible" list can be used, via the generic iic_compat_match()
function.
I have tested this on a few sparc64 machines, it works for me.
I won't mind if we decide to not used this but go with one of the older
proposals instead - but we need to move on.
Comments?
Yet another dirct config proposal for i2c busses
Folks,
we still have to solve the problem of "scanning" i2c busses, especially
on machines where no scan is needed since the firmware happily tells
us everything we might want to know.
In the past (as far as I remember) two proposal where presented and
both shot down. Now I have a machine where I realy needed the i2c
based fan controll to make the noise bearble - so I put on my asbestos
suit and wrote another implementation, which I'd like to present here.
The previous proposals:
- use the OpenBSD way: an optional "scan" callback provided by the
i2c controller driver. Downsides: needs changes ~every i2c controller
driver in-tree
- the macppc way: see macppc/dev/kiic* - basically a slightly different
bus, needs frontend/backend split of i2c device drivers and a lot
of additional frontends to be written
I may misrember details and critics raised against one of those.
Goals I tried to achieve:
- Allow both direct and indirect config at the i2c bus layer,
depending on availability of firmware provided locators
- Allow unmodified i2c device drivers to continue working
- Keep MD changes as simple and small as possible
- No changes to MI i2c bus controllers
- Allow MD i2c bus controllers to easily override the generic
behaviour (i.e.: provide additional locators or modify firmware
provided ones)
Seems like it worked out, and the changes are pretty small in the end.
Quick overview how it works:
- If we are doing direct config, MD code (via generic support routines,
or by overriding those) adds a prop_array to the device properties
of the i2c bus controller (the parent(!) of the i2c bus). This array
contains a dictionary for each i2c device on the bus. Entries in
this dictionary are:
"name" -> string, device name
"address" -> uint32, i2c address
"size" -> uint32 (optional)
"compatibility" -> a list of names, i.e. the chip used, used for
matching a hardware driver (think: alternative "name" props)
- When the i2c bus attaches, it queries the device properties of it's
parent device and checks the "i2c-child-devices" property (the array
described above), and if it is present, iterates over the array
creating i2c_attach_arg from it. To allow direct config matches,
the i2c_attach_args structure has been extended.
If the device property is not available, indirect config is done.
- An i2c device driver for a proper device will need no changes, but
for i.e. write-only devices matching based on strings can be added.
A generic helper function to match the "compatible" string list against
a driver specific list is provided.
A few more details:
Let's start with setting up the device property. In the attached sparc64 MD
code the setup is done inside device_register() whenever a "iic" device
attaches and there is not yet a "i2c-child-devices" property at the parent.
This check is needed to allow MD i2c controller drivers to override
the generic behaviour. For OpenFirmware based machines, a convenience
funtction "of_enter_i2c_devs()" to do the device property setup is provided.
Next step: the i2c bus attaches and checks for the device property. This is
all done in the i2c bus code, no i2c controller driver needs modifications.
Last part of the puzzle: the i2c device drivers can check for the (new)
ia_name pointer in the i2c_attach_args structure to find out if direct
config is available. For example the spdmem driver does a (nasty/stupid?)
check for certain address values - which does not make any sense in the
direct config case:
@@ -164,8 +165,17 @@ spdmem_match(device_t parent, cfdata_t m
int spd_len, spd_crc_cover;
uint16_t crc_calc, crc_spd;
- if ((ia->ia_addr & SPDMEM_ADDRMASK) != SPDMEM_ADDR)
- return 0;
+ if (ia->ia_name) {
+ /* add other names as we find more firmware variations */
+ if (strcmp(ia->ia_name, "dimm-spd"))
+ return 0;
+ }
+
+ /* only do this lame test when not using direct config */
+ if (ia->ia_name == NULL) {
+ if ((ia->ia_addr & SPDMEM_ADDRMASK) != SPDMEM_ADDR)
+ return 0;
+ }
sc.sc_tag = ia->ia_tag;
sc.sc_addr = ia->ia_addr;
If the firmware name is not a good indicator of the driver to use, the
"compatible" list can be used, via the generic iic_compat_match()
function.
I have tested this on a few sparc64 machines, it works for me.
I won't mind if we decide to not used this but go with one of the older
proposals instead - but we need to move on.
Comments?
Martin
Index: dev/ofw/openfirm.h
===
RCS file: /cvsroot/src/sys/dev/ofw/openfirm.h,v
retrieving revision 1.27
diff -c -u -p -r1.27 openfirm.h
--- dev/ofw/openfirm.h 11 Nov 2009 16:56:52 - 1.27
+++ dev/ofw/openfirm.h 3 Feb 2010 19:46:14 -
@@ -115,4 +115,6 @@ boolean_t of_to_dataprop(prop_dicti
Re: blocksizes
> > (2) block count stored in dinode > > (3) DIRBLKSIZ > > > > API issue you mentioned is about (1), but what about (2) and (3) > > that should be defined in file system layer itself? > > DIRBLKSIZE it is the chunk size used to access directory 'files'. > The function ufs_blkatoff() translates the offset into block numbers. says: >> * A directory consists of some number of blocks of DIRBLKSIZ >> * bytes, where DIRBLKSIZ is chosen such that it can be transferred >> * to disk in a single atomic operation (e.g. 512 bytes on most machines). Probably DEV_BSIZE which could be smaller than physical sector size might be able to handle atomic ops, but I wonder if someone actually considered how it should work on >DEV_BSIZE sector media. > Block counts stored in dinode are measured in DEV_BSIZE blocks. > This is what a stat() or fstatvfs() report and what quotas > should count and how it is documented for NetBSD. I verified > that FreeBSD choses the same units, so the on-disk data should > be the same. > > So, both numbers are not related to addressing disk blocks. > > I would not expect issues there as the code has been used on disks > with different block sizes in the past. Maybe there are bugs in > the dirhash code. Is there any working code used on disks with different blocks sizes in the past? Probably some code like msdosfs might work, but not a whole kernel. I wondered not only if current code would work or not but also how all related APIs had been (or should be) designed and defined. FFS sources used both DEV_BSIZE and fs_fsbtodb (or physical sector size stored in disklabel) to access disk blocks, and you just "fixed" the latter ones as "bugs". Yes, after fixing one more "bug" which was there since initial import ffs seems working on a raw partition on 4KB/sect disk, but there are still several sources that pass physical block numbers stored in disklabel to I/O functions directly, like subr_disk.c etc. It's fine if you have a certain plan how to fix rest of bugs, but it might be better to have some review before proceeding. Actaully it would not look so difficult to fix them, but I wonder if we should have some proper names which explicitly describe physical or logical blocks because passing logical block numbers in DEV_BSIZE to a function named "read_sector()" looks a bit confusing, for example. --- Izumi Tsutsui Index: ufs/ffs/ffs_vfsops.c === RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vfsops.c,v retrieving revision 1.256 diff -u -r1.256 ffs_vfsops.c --- ufs/ffs/ffs_vfsops.c31 Jan 2010 10:54:10 - 1.256 +++ ufs/ffs/ffs_vfsops.c3 Feb 2010 11:07:52 - @@ -1931,7 +1931,7 @@ u_int32_t saveflag; error = ffs_getblk(mp->um_devvp, - fs->fs_sblockloc >> (fs->fs_fshift - fs->fs_fsbtodb), FFS_NOBLK, + fs->fs_sblockloc / DEV_BSIZE, FFS_NOBLK, fs->fs_sbsize, false, &bp); if (error) return error;
