Skip to contentSkip to conversation

Some IDE issues with 2.6.28 on PC-Engines ALIX2

DISCUSSION Karl Auerbach 5 participants 25 messages Jan 5, 2009 – Feb 1, 2009 libata

lore
Share & tools
lore

Karl Auerbach

author
#0 lore raw

I've found what I believe are some problems with the IDE drivers on the 2.6.8 kernel on the PC-Engines ALIX2 platform.

There are two points in particular:

1. The ide_generic driver no longer finds the IDE drive (an EXT3 formatted compact flash on an IDE channel on an AMD CS5536 controller) unless the kernel is given the ide_generic.probe_mask=0x## command line parameter. (I'll get to the values of ## later.)

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

I will provide more details below.

I'd submit these directly into the bugzilla but as I'm not familar with the process and conventions I thought I'd begin with this email.

Thanks,
--karl--
Karl Auerbach (Santa Cruz, CA)
[email protected]

=================================================

Here's the scripts/ver_linux output:

[root@board-1 linux-2.6.28]# sh scripts/ver_linux If some fields are empty or look unusual you may have an old version. Compare to the current minimal requirements in Documentation/Changes.

Linux board-1.cavebear.com 2.6.28-ALIX2 #1 Fri Jan 2 16:58:36 PST 2009 i586 unknown

Gnu C                  4.2.4
Gnu make               3.81
binutils               2.19
util-linux             fdformat: unrecognized option `--version'

BusyBox v1.13.1 (2009-01-02 17:00:22 PST) multi-call binary

Usage: fdformat [-n] DEVICE

Format floppy disk

Options:
-n Don't verify after format

mount (rw
(rw,errors=continue,data=ordered
(rw
(rw
(rw,size=1024k,mode=755
(rw
(rw,gid=5,mode=620
(rw
(rw,size=32768k
(ro,noatime,nodiratime,errors=continue,data=ordered (rw,vers=3,rsize=8192,wsize=8192,namlen=255,hard,nointr,nolock,proto=udp,timeo=7,retrans=3,sec=sys,addr=192.203.17.130

module-init-tools      found
e2fsprogs              1.41.3
Linux C Library        2.6.1
Dynamic linker (ldd)   2.6.1
Procps                 `--version'
Kbd                    78:
Sh-utils               --v
Modules Loaded         ebtable_filter ebtables x_tables bridge stp llc geodewdt cs5535_gpio geode_rng

=================================================

I. The ide_generic issue:

[1.] One line summary of the problem:

ide_generic driver under 2.6.28 no longer finds IDE drives on PC-Engines ALIX2.

[2.] Full description of the problem/report:

The PC-Engines ALIX2 platform uses an AMD Geode LX processor with the AMD CS5536 companion chip for things like IDE support. The IDE drive is a compact flash.

On the 2.6.27.8 kernel the ide_generic driver found the IDE and compact flash without the need for any kernel command line parameters.

On the 2.6.28 kernel the ide_generic driver no longer finds the IDE drive unless a ide_generic.probe_mask=0x## command line parameter is given to the kernel.

I've tried several values for ##, I have not yet found one that exactly replicates the behaviour of the 2.6.27.8 kernel.

However, values 0x3f, 0x31, and 0x11 all seem to work, although the last of these (0x11 has the closest behaviour to the 2.6.27.8 kernel.)

Here's the relevant kernel output when using the 0x11 value in the following kernel command line. (The 0x31 value gives the same results. The 0x3f value finds several more IDE items.)

Kernel command line: console=ttyS0,38400 root=/dev/hda2 ramdisk_size=16384 initrd=/initrd.gz rw ide_generic.probe_mask=0x11 BOOT_IMAGE=/linux

Uniform Multi-Platform E-IDE driver
ide_generic: enforcing probing of I/O ports upon user request
hda: TRANSCEND, ATA DISK drive
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
ide1 at 0x1e0-0x1e7,0x3e6 on irq 8
ide2 at 0x160-0x167,0x366 on irq 12
ide-gd driver 1.18
hda: max request size: 128KiB
hda: 3915072 sectors (2004 MB) w/1KiB Cache, CHS=3884/16/63
hda: hda1 hda2 hda3
Driver 'sd' needs updating - please use bus_type methods

When the ide_generic.probe_mask=0x11 is not given no ide items are found under 2.6.28 (although they were found under 2.6.27.8)

[3.] Keywords (i.e., modules, networking, kernel):

ide_generic, ALIX2

[4.] Kernel version (from /proc/version):

2.6.28

[7.7.] Other information that might be relevant to the problem
(please look in /proc and include all information that you think to be relevant):

I can provide kernel .config files if you want.

=================================================

II. The cs5536 controller issue.

[1.] One line summary of the problem:

The cs5535 ide driver doesn't seem to recognize the AMD CS5536 controller.

[2.] Full description of the problem/report

The old AMD Geode used the CS5535 companion chipset for several purposes including IDE.

The newer AMD Geode LX uses the CS5536 companion chipset.

It is not clear that the cs5535 ide kernel driver is supposed to handle the newer chipset. But in any event, it seems that the cs5535 driver, when it is looking for the hardware, is not seeing anything it recognizes.

There is some ambiguity created becaause at least one "cs5535" driver works on the The CS5536: cs5535_gpio

This is not a particularly signifcant issue - the ide_generic driver handles the CS5536 IDE just fine (modulo the command line issue I reported above.) But it would be nice if any ambiguity about what the cs5535 driver supports and what it does not support were clarified.

	Thanks,
		--karl--
#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Karl" == Karl Auerbach <[email protected]> writes:

Karl> 2. The cs5535 ide driver doesn't seem to be able to recognize the Karl> newer CS5536 controller for IDE.

The IDE interface of the Geode CS5536 companion chip is a completely different beast. It's a more or less standard AMD 8111 PCI IDE device with slightly different timings. There's a dedicated driver called pata_cs5536.c (CONFIG_PATA_CS5536).

The driver was developed on a Soekris net5501. To my knowledge it's working fine on ALIX boards. If that's not the case there may be a problem with the ALIX BIOS VSA PCI config space emulation. In that case you can load the driver with msr=1 to force it to program the IDE function using machine specific registers instead of relying on PCI.

Let me know how it goes.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
#2 lore raw

Hello.

Quoted 9 lines · Martin K. Petersen
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

"Karl" == Karl Auerbach <[email protected]> writes:

Karl> 2. The cs5535 ide driver doesn't seem to be able to recognize the Karl> newer CS5536 controller for IDE.

The IDE interface of the Geode CS5536 companion chip is a completely different beast. It's a more or less standard AMD 8111 PCI IDE device with slightly different timings.

I'm not sure I uderstand the passage about "slightly different
timings"...

Quoted 2 lines

There's a dedicated driver called pata_cs5536.c (CONFIG_PATA_CS5536).

I must note thst using libata for driving (presumably PIO only) CF
seems an overkill (if not a slowdown).

Quoted 6 lines

The driver was developed on a Soekris net5501. To my knowledge it's working fine on ALIX boards. If that's not the case there may be a problem with the ALIX BIOS VSA PCI config space emulation. In that case you can load the driver with msr=1 to force it to program the IDE function using machine specific registers instead of relying on PCI.

It's stange that the driver is using CPI config. space by default
which not even documented in the datasheet. I guess the MSR accesses are also somewhat faster because one doesn't have to access the address/data register pair at ports 0xcf8/0xcfc...

MBR, Sergei

#3 lore raw
Quoted 6 lines
Quoted 3 lines

The IDE interface of the Geode CS5536 companion chip is a completely different beast. It's a more or less standard AMD 8111 PCI IDE device with slightly different timings.

I'm not sure I uderstand the passage about "slightly different
timings"...

The timing values used are different to those on the AMD 8111 even though the interface is similar.

Quoted 2 lines

It's stange that the driver is using CPI config. space by default
which not even documented in the datasheet. I guess the MSR accesses are

The PCI configuration is the portable way to drive it.

Quoted 2 lines

also somewhat faster because one doesn't have to access the address/data register pair at ports 0xcf8/0xcfc...

This is a Geode. What makes you think there is even a PCI bus or that the 8111 interface isn't entirely a software construct. Geode PCI hardware is mostly smoke and mirrors ;)

Alan

#4 lore raw

Hello.

Quoted 9 lines · Alan Cox
Quoted 6 lines
Quoted 3 lines

The IDE interface of the Geode CS5536 companion chip is a completely different beast. It's a more or less standard AMD 8111 PCI IDE device with slightly different timings.

I'm not sure I uderstand the passage about "slightly different
timings"...

The timing values used are different to those on the AMD 8111 even though the interface is similar.

What do you mean by timing values anyway, clock counts or the register
encoding itself?

Quoted 4 lines
Quoted 2 lines

It's stange that the driver is using CPI config. space by default
which not even documented in the datasheet. I guess the MSR accesses are

The PCI configuration is the portable way to drive it.

Portable to what? As you have yorself noted below, this is Geode -- and I
highly doubt that CS5536 will be used for anything else.

Quoted 3 lines
Quoted 1 line

also somewhat faster because one doesn't have to access the address/data register pair at ports 0xcf8/0xcfc...

This is a Geode.

I know. :-)
I have hacked on older Geode GX2 for some time back in 2004 (although not
on something related to PCI); my "toy" board has RIP just recently.)

Quoted 1 line

What makes you think there is even a PCI bus

I sure know that it's not PCI (IIUC, it's the common tendency in the x86
north-to-south-bridge busses has been emulating the PCI config. space with less and less PCI compatible "physical layer"). That does mean emulating PCI config. space mechanism #1 and Geode's not an exception here, see:

http://www.amd.com/files/connectivitysolutions/geode/geode_gx/32663C_lx_gx_pciconfig.pdf

Quoted 1 line

or that the 8111 interface isn't entirely a software construct.

If that's the case (looks like so), it makes using it even appealing...

Quoted 1 line

Geode PCI hardware is mostly smoke and mirrors ;)

When it comes to their internal controllers, yes (though e.g. their OHCI
seem to have the real PCI config. space).

Quoted 1 line

Alan

WBR, Sergei

#5 lore raw
Quoted 2 lines

What do you mean by timing values anyway, clock counts or the register
encoding itself?

The counts, the encoding is the same.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

It's stange that the driver is using CPI config. space by default
which not even documented in the datasheet. I guess the MSR accesses are

The PCI configuration is the portable way to drive it.

Portable to what? As you have yorself noted below, this is Geode -- and I
highly doubt that CS5536 will be used for anything else.

The Geode hardware and software interfaces are rather independant. The hardware one may change in future product and the software one stay the same (eg the way softvga does compared with touching the display controllers on the older devices)

Alan

#6 lore raw
In reply to Sergei Shtylyov
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Sergei" == Sergei Shtylyov <[email protected]> writes:

The driver was developed on a Soekris net5501. To my knowledge it's working fine on ALIX boards. If that's not the case there may be a problem with the ALIX BIOS VSA PCI config space emulation. In that case you can load the driver with msr=1 to force it to program the IDE function using machine specific registers instead of relying on PCI.

Sergei> It's stange that the driver is using CPI config. space by Sergei> default which not even documented in the datasheet.

Using PCI config space was explicitly requested by AMD for the reasons Alan pointed out.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
#7 lore raw
In reply to Karl Auerbach
Quoted 2 lines

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

Switch to libata as most distributions have already done and enable the pata_cs5536 driver, which is designed for this hardware.

Alan

author
#8 lore raw
Quoted 5 lines · Alan Cox
Quoted 2 lines

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

Switch to libata as most distributions have already done and enable the pata_cs5536 driver, which is designed for this hardware.

Thanks for the suggestion - It made a *big* difference.

Here's a couple of timings (sequential reads from the CF) that show the difference in performance. I didn't do writes because I'm never sure what various CF's do inside and don't have confidence that seemingly similar CF writes have consistent times.

Using the generic IDE driver:

[root@board-1 /root]# time dd if=/dev/hda2 of=/dev/null bs=512 3572352+0 records in
3572352+0 records out

real      16m 18.64s
user      0m 0.76s
sys       1m 28.08s

Using the PATA CS5536 driver:

[root@board-1 /root]# time dd if=/dev/sda2 of=/dev/null bs=512 3572352+0 records in
3572352+0 records out

real    1m 11.41s
user    0m 0.47s
sys     0m 3.56s

In other words, on sequential reads, I'm seeing a roughly 13x fold improvement in clock time and an 11x improvement in sys+user cpu.

[It also seemed to fix another weird thing that started with 2.6.28, the disappearance of the stdout from linuxrc and rc.sysinit - the kernel messages still hit the console but the stuff from those two startup scripts went into never never land (even though, as apparent from their side effects, they still executed properly.)]

I guess I now need to go through my system and make sure that any hard coded "hda" are changed to "sda".

As for the other issues mentioned during this very helpful discussion, of which I comprehended about 50%:

Do folks still want me to try that patch? (Now that the problem I've been having seems fixed, and being lazy by nature, I'll do it if it is helpful but otherwise I'd prefer to skip it.)

As for the question of why I have the SD driver - The ALIX2 boards (which, by the way are Geode LX, not the older Geode) has USB and I want to sometimes plug external USB drives into 'em. I thought that the SD driver is needed for that, am I in error?

By-the-way, I'm happy (modulo my previously mentioned lazy nature) to run tests on ALIX2 and ALIX3 boards should anyone need it.

	Thanks,
		--karl--
		Karl Auerbach
		Santa Cruz, CA
		http://www.cavebear.com/
#9 lore raw
Quoted 3 lines

As for the question of why I have the SD driver - The ALIX2 boards (which, by the way are Geode LX, not the older Geode) has USB and I want to sometimes plug external USB drives into 'em. I thought that the SD driver is needed for that, am I in error?

usb-storage + sd (as in scsi-disk not SDcard)

#10 lore raw
In reply to Karl Auerbach

Hello.

Quoted 11 lines · Karl Auerbach
Quoted 5 lines
Quoted 2 lines

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

Switch to libata as most distributions have already done and enable the pata_cs5536 driver, which is designed for this hardware.

Thanks for the suggestion - It made a *big* difference.

Here's a couple of timings (sequential reads from the CF) that show the difference in performance. I didn't do writes because I'm never sure what various CF's do inside and don't have confidence that seemingly similar CF writes have consistent times.

Using the generic IDE driver:

It was a total mistake from the very start that you used ide-generic
driver and you should really have used ide-pci-generic.

Quoted 7 lines

[root@board-1 /root]# time dd if=/dev/hda2 of=/dev/null bs=512 3572352+0 records in
3572352+0 records out

real      16m 18.64s
user      0m 0.76s
sys       1m 28.08s
Quoted 8 lines

Using the PATA CS5536 driver:

[root@board-1 /root]# time dd if=/dev/sda2 of=/dev/null bs=512 3572352+0 records in
3572352+0 records out

real    1m 11.41s
user    0m 0.47s
sys     0m 3.56s

Hm, looks like your CF drive is rather advanced and might be
supporting UltraDMA (and your CF slot supports DMA). Care to post the output of 'hdparm -i /dev/hda'?

Quoted 1 line

Do folks still want me to try that patch? (Now that the problem I've been having seems fixed, and being lazy by nature, I'll do it if it is helpful but otherwise I'd prefer to skip it.)

Of course, it'll be helpful.

Quoted 1 line

As for the question of why I have the SD driver - The ALIX2 boards (which, by the way are Geode LX, not the older Geode)

I didn't doubt it. CS5536 isn't used with LX it seems...

Quoted 1 line

has USB and I want to sometimes plug external USB drives into 'em. I thought that the SD driver is needed for that, am I in error?

No. That explained it.

Quoted 1 line

By-the-way, I'm happy (modulo my previously mentioned lazy nature) to run tests on ALIX2 and ALIX3 boards should anyone need it.

Judging on what you have posted, IDE driver would not probably give
any speed improvement, only soewhat lesser memory footprint...

MBR, Sergei

#11 lore raw
Quoted 2 lines

Judging on what you have posted, IDE driver would not probably give
any speed improvement, only soewhat lesser memory footprint...

Except he is also using USB storage so the shared stack with the libata code is already loaded ...

#12 lore raw
Quoted 5 lines · Alan Cox
Quoted 2 lines

Judging on what you have posted, IDE driver would not probably give
any speed improvement, only soewhat lesser memory footprint...

Except he is also using USB storage so the shared stack with the libata code is already loaded ...

Doesn't really matter because libata itself is quite heavy:

$ size drivers/ata/built-in.o drivers/ide/built-in.o
   text    data     bss     dec     hex filename
  84100    6584    4196   94880   172a0 drivers/ata/built-in.o
  50198    1032     240   51470    c90e drivers/ide/built-in.o

[ I'm pretty sure that runtime memory footprint is also lower for IDE. ]

Possible additional gains if usb-storage wouldn't be needed:

$ size drivers/scsi/built-in.o
   text    data     bss     dec     hex filename
  61136    4660     532   66328   10318 drivers/scsi/built-in.o

This is with Linus' tree from today plus "IDE updates part 3" pull request posted today + CS5536 IDE host driver. Enabled config options:

* libata

CONFIG_ATA=y
CONFIG_ATA_ACPI=y
CONFIG_ATA_SFF=y
CONFIG_PATA_CS5536=y

* IDE

CONFIG_IDE_GD=y
CONFIG_IDE_GD_ATA=y
CONFIG_BLK_DEV_IDEACPI=y
CONFIG_IDE_TASK_IOCTL=y
CONFIG_BLK_DEV_IDEDMA_SFF=y
CONFIG_BLK_DEV_IDEPCI=y
CONFIG_BLK_DEV_IDEDMA_PCI=y
CONFIG_BLK_DEV_CS5536=y
CONFIG_BLK_DEV_IDEDMA=y

* SCSI

CONFIG_SCSI=y
CONFIG_SCSI_DMA=y
CONFIG_BLK_DEV_SD=y
CONFIG_SCSI_MULTI_LUN=y
CONFIG_SCSI_WAIT_SCAN=m

--
Thanks,
Bart

#13 lore raw
In reply to Karl Auerbach

Hello.

Quoted 9 lines · Karl Auerbach

I've found what I believe are some problems with the IDE drivers on the 2.6.8 kernel on the PC-Engines ALIX2 platform.

There are two points in particular:

1. The ide_generic driver no longer finds the IDE drive (an EXT3 formatted compact flash on an IDE channel on an AMD CS5536 controller) unless the kernel is given the ide_generic.probe_mask=0x## command line parameter. (I'll get to the values of ## later.)

The probing behavior was changes on purpose, IIRC.

Quoted 2 lines

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

No wonder, it's even impossible to determine CS5536 IDE controller's
device ID from the datasheet; include/linux/pci_ids.h tells me that the device ID is 0x209A, so adding another ID to the 'cs5535' driver's ID table shouldn't be an issue -- if they are indeed compatible. Looking at the datasheets, they are not -- bad luck, we need a new driver... BTW, libata seems to already have support for this chipset.

Quoted 4 lines

I will provide more details below.

I'd submit these directly into the bugzilla but as I'm not familar with the process and conventions I thought I'd begin with this email.

I don't think these are bugs.

Quoted 13 lines

I. The ide_generic issue:

[1.] One line summary of the problem:

ide_generic driver under 2.6.28 no longer finds IDE drives on PC-Engines ALIX2.

[2.] Full description of the problem/report:

The PC-Engines ALIX2 platform uses an AMD Geode LX processor with the AMD CS5536 companion chip for things like IDE support. The IDE drive is a compact flash.

Quoted 6 lines

On the 2.6.27.8 kernel the ide_generic driver found the IDE and compact flash without the need for any kernel command line parameters.

On the 2.6.28 kernel the ide_generic driver no longer finds the IDE drive unless a ide_generic.probe_mask=0x## command line parameter is given to the kernel.

Because ide-generic now avoids IDE ports already known to belong to
the PCI controllers.

Quoted 5 lines

I've tried several values for ##, I have not yet found one that exactly replicates the behaviour of the 2.6.27.8 kernel.

However, values 0x3f, 0x31, and 0x11 all seem to work, although the last of these (0x11 has the closest behaviour to the 2.6.27.8 kernel.)

Hm, doesn't plain 1 also work?

Quoted 4 lines

Here's the relevant kernel output when using the 0x11 value in the following kernel command line. (The 0x31 value gives the same results. The 0x3f value finds several more IDE items.)

Quoted 8 lines

Kernel command line: console=ttyS0,38400 root=/dev/hda2 ramdisk_size=16384 initrd=/initrd.gz rw ide_generic.probe_mask=0x11 BOOT_IMAGE=/linux

Uniform Multi-Platform E-IDE driver
ide_generic: enforcing probing of I/O ports upon user request
hda: TRANSCEND, ATA DISK drive
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
ide1 at 0x1e0-0x1e7,0x3e6 on irq 8
ide2 at 0x160-0x167,0x366 on irq 12

But this rather matches 0x31 mask.

Quoted 5 lines

ide-gd driver 1.18
hda: max request size: 128KiB
hda: 3915072 sectors (2004 MB) w/1KiB Cache, CHS=3884/16/63
hda: hda1 hda2 hda3
Driver 'sd' needs updating - please use bus_type methods

Hm, why are you using 'sd' driver at all?

Quoted 6 lines

II. The cs5536 controller issue.

[1.] One line summary of the problem:

The cs5535 ide driver doesn't seem to recognize the AMD CS5536 controller.

And it shouldn't -- the controllers are incompatible.

Quoted 7 lines

[2.] Full description of the problem/report

The old AMD Geode used the CS5535 companion chipset for several purposes including IDE.

The newer AMD Geode LX uses the CS5536 companion chipset.

Quoted 7 lines

It is not clear that the cs5535 ide kernel driver is supposed to handle the newer chipset. But in any event, it seems that the cs5535 driver, when it is looking for the hardware, is not seeing anything it recognizes.

There is some ambiguity created becaause at least one "cs5535" driver works on the The CS5536: cs5535_gpio

That only means that *some* fucntins of CS5536 are compatible with
CS5535.

Quoted 4 lines

This is not a particularly signifcant issue - the ide_generic driver handles the CS5536 IDE just fine (modulo the command line issue I reported above.) But it would be nice if any ambiguity about what the cs5535 driver supports and what it does not support were clarified.

Hopefully, it's clarified now. :-)

Quoted 2 lines
    Thanks,
        --karl--

MBR, Sergei

#14 lore raw
Quoted 8 lines · Sergei Shtylyov

Hello.

Quoted 4 lines · Karl Auerbach

I've found what I believe are some problems with the IDE drivers on the 2.6.8 kernel on
the PC-Engines ALIX2 platform.

Quoted 8 lines
Quoted 6 lines

There are two points in particular:

1. The ide_generic driver no longer finds the IDE drive (an EXT3 formatted compact flash on an IDE channel on an AMD CS5536 controller) unless the kernel is given the ide_generic.probe_mask=0x## command line parameter. (I'll get to the values of ## later.)

The probing behavior was changes on purpose, IIRC.

Yes, sometimes people were (by accident) using ide_generic for controllers that have designated PCI host drivers (which offer better support/performance and know about such things like errata workarounds).

[ In this particular case ide_pci_generic host driver with "all_generic_ide"
parameter may be used if BIOS programmed controller correctly. ]

Quoted 9 lines
Quoted 2 lines

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

No wonder, it's even impossible to determine CS5536 IDE controller's
device ID from the datasheet; include/linux/pci_ids.h tells me that the device ID is 0x209A, so adding another ID to the 'cs5535' driver's ID table shouldn't be an issue -- if they are indeed compatible. Looking at the datasheets, they are not -- bad luck, we need a new driver... BTW, libata seems to already have support for this chipset.

Indeed...

Karl, care to give a try to the following patch (it is completely untested so please backup your data first if necessary)?

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: add CS5536 host driver

This is a port of libata's pata_cs5536.c (written by Martin K. Petersen) to IDE subsystem.

Changes done while at it:

* Reprogram PIO/MWDMA timings if needed before and after DMA transfer (chipset uses shared PIO/MWDMA timings).

* Fix cable detection to report 80-wires cable if BIOS set it for any device on a port (IDE core will do drive-side cable detection later).

* Don't disable UDMA while programming PIO timings.

* Simplify PCI/MSR support.

Pros of having IDE host driver in addition to libata's one:

* CS5536 is used in embedded applications and IDE is much lighter than SCSI+libata, the host driver itself is also a bit smaller:

   text    data     bss     dec     hex filename
   1237     500       4    1741     6cd drivers/ata/pata_cs5536.o
   1164     132       4    1300     514 drivers/ide/cs5536.o

* CS5536 is used mostly for driving CF cards and this allows use of IDE features which are unavailable under libata (32-bit I/O support, non-rotational media optimizations, etc).

Cc Martin K. Petersen <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Karl Auerbach <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/Kconfig  |   10 +
 drivers/ide/Makefile |    1 
 drivers/ide/cs5536.c |  303 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 314 insertions(+)
drivers/ide/Kconfig +10 -0
Index: b/drivers/ide/Kconfig
===================================================================
@@ -465,6 +465,16 @@ config BLK_DEV_CS5535
 
 	  It is safe to say Y to this question.
 
+config BLK_DEV_CS5536
+	tristate "CS5536 chipset support"
+	depends on X86 && !X86_64
+	select BLK_DEV_IDEDMA_PCI
+	help
+	  This option enables support for the AMD CS5536
+	  companion chip used with the Geode LX processor family.
+
+	  If unsure, say N.
+
 config BLK_DEV_HPT366
 	tristate "HPT36X/37X chipset support"
 	select BLK_DEV_IDEDMA_PCI
drivers/ide/Makefile +1 -0
Index: b/drivers/ide/Makefile
===================================================================
@@ -43,6 +43,7 @@ obj-$(CONFIG_BLK_DEV_CMD64X)		+= cmd64x.
 obj-$(CONFIG_BLK_DEV_CS5520)		+= cs5520.o
 obj-$(CONFIG_BLK_DEV_CS5530)		+= cs5530.o
 obj-$(CONFIG_BLK_DEV_CS5535)		+= cs5535.o
+obj-$(CONFIG_BLK_DEV_CS5536)		+= cs5536.o
 obj-$(CONFIG_BLK_DEV_SC1200)		+= sc1200.o
 obj-$(CONFIG_BLK_DEV_CY82C693)		+= cy82c693.o
 obj-$(CONFIG_BLK_DEV_DELKIN)		+= delkin_cb.o
drivers/ide/cs5536.c new file +303 -0

303 lines not on this page · raw message

#15 lore raw
Quoted 3 lines

* CS5536 is used mostly for driving CF cards and this allows use of IDE features which are unavailable under libata (32-bit I/O support, non-rotational media optimizations, etc).

Non rotational media handling is done by the block layer and so works for everything. Just set the right queue according to the device policy you want in hal/udev.

Alan

#16 lore raw
Quoted 7 lines · Alan Cox
Quoted 3 lines

* CS5536 is used mostly for driving CF cards and this allows use of IDE features which are unavailable under libata (32-bit I/O support, non-rotational media optimizations, etc).

Non rotational media handling is done by the block layer and so works for everything. Just set the right queue according to the device policy you want in hal/udev.

You seem to confuse no merges policy with non rotational media support.

Both are done by block layer. The former is controlled by the user-space while the latter is controlled by the block driver.

Thanks,
Bart

#17 lore raw
Quoted 2 lines

Both are done by block layer. The former is controlled by the user-space while the latter is controlled by the block driver.

And in the old IDE case erroneously set for any device reporting CFA via the kernel. Queue flag management belongs in user space because its far more complex than you seem to think to get it right.

Alan

#18 lore raw
Quoted 6 lines · Alan Cox
Quoted 2 lines

Both are done by block layer. The former is controlled by the user-space while the latter is controlled by the block driver.

And in the old IDE case erroneously set for any device reporting CFA via the kernel. Queue flag management belongs in user space because its far more complex than you seem to think to get it right.

Please stop playing straw man in almost every your mail. Doing it once in a while can be intriguing or even enjoyable but doing it on daily basis kills any sensible debate...

There is nothing incorrect in setting non-rotational queue flag for CF devices [1] and I wasn't suggesting where it should belong -- I was solely describing how things look up currently.

[1] in fact some SSD devices present themselves as CF devices

Thanks,
Bart

#19 lore raw
Quoted 2 lines

[1] in fact some SSD devices present themselves as CF devices

As do some rotating disks.

#20 lore raw
Quoted 4 lines · Alan Cox
Quoted 2 lines

[1] in fact some SSD devices present themselves as CF devices

As do some rotating disks.

Hmm. I now recall that Microdrives can indeed do this.

What a mess... I'll remove CFA check for now.

Thanks,
Bart

#21 lore raw
In reply to Bartlomiej Zolnierkiewicz

Hello.

Quoted 18 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines
Quoted 3 lines

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

No wonder, it's even impossible to determine CS5536 IDE controller's
device ID from the datasheet; include/linux/pci_ids.h tells me that the device ID is 0x209A, so adding another ID to the 'cs5535' driver's ID table shouldn't be an issue -- if they are indeed compatible. Looking at the datasheets, they are not -- bad luck, we need a new driver... BTW, libata seems to already have support for this chipset.

Indeed...

Karl, care to give a try to the following patch (it is completely untested so please backup your data first if necessary)?

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: add CS5536 host driver

This is a port of libata's pata_cs5536.c (written by Martin K. Petersen) to IDE subsystem.

Quoted 8 lines

Changes done while at it:

* Reprogram PIO/MWDMA timings if needed before and after DMA transfer (chipset uses shared PIO/MWDMA timings).

* Fix cable detection to report 80-wires cable if BIOS set it for any device on a port (IDE core will do drive-side cable detection later).

The original pata_cs5536 cable detection is indeed somewhat bogus...

Quoted 2 lines

* CS5536 is used mostly for driving CF cards and this allows use of

I woulnd't be so sure -- but anyway, I mostly see the development
boards... :-)

Quoted 5 lines
Cc Martin K. Petersen <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Karl Auerbach <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Looks good but needs some polishing...

drivers/ide/Kconfig config BLK_DEV_CS5535 · line 471

Quoted 12 lines

Index: b/drivers/ide/Kconfig
===================================================================

drivers/ide/Kconfig +3 -0
@@ -465,6 +465,16 @@ config BLK_DEV_CS5535
 
 	  It is safe to say Y to this question.
 
+config BLK_DEV_CS5536
+	tristate "CS5536 chipset support"
+	depends on X86 && !X86_64

Why not just depend on X86_32?

Quoted 6 lines

Index: b/drivers/ide/cs5536.c
===================================================================

drivers/ide/cs5536.c new file +0 -0
@@ -0,0 +1,303 @@

[...]

Quoted 6 lines

+ * The IDE timing registers for the CS5536 live in the Geode Machine
+ * Specific Register file and not PCI config space. Most BIOSes
+ * virtualize the PCI registers so the chip looks like a standard IDE
+ * controller. Unfortunately not all implementations get this right.
+ * In particular some have problems with unaligned accesses to the

I'd say that people doing unaligned accesses are just looking for
trouble... :-)

Quoted 14 lines
drivers/ide/cs5536.c new file +13 -0
+enum {
+	MSR_IDE_CFG		= 0x51300010,
+	PCI_IDE_CFG		= 0x40,
+
+	CFG			= 0,
+	DTC			= 2,
+	CAST			= 3,
+	ETC			= 4,
+
+	IDE_CFG_CHANEN		= (1 << 1),
+	IDE_CFG_CABLE		= (1 << 17) | (1 << 16),
+
+	IDE_D0_SHIFT		= 24,

drivers/ide/cs5536.c line 64

Quoted 10 lines
drivers/ide/cs5536.c new file +10 -0
+	IDE_D1_SHIFT		= 16,
+	IDE_DRV_MASK		= 0xff,
+
+	IDE_CAST_D0_SHIFT	= 6,
+	IDE_CAST_D1_SHIFT	= 4,
+	IDE_CAST_DRV_MASK	= 0x3,
+
+	IDE_CAST_CMD_SHIFT	= 24,
+	IDE_CAST_CMD_MASK	= 0xff,
+};

Declaring a lot of semi-related constants is not what enum was
intended for I think...

drivers/ide/cs5536.c line 93

Quoted 5 lines
drivers/ide/cs5536.c new file +4 -0
+static void cs5536_program_dtc(ide_drive_t *drive, u8 tim)
+{
+	struct pci_dev *pdev = to_pci_dev(drive->hwif->dev);
+	int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;

Masking with 1 is pointless (though harmless) for this single-channel
controller.

Quoted 14 lines
drivers/ide/cs5536.c new file +13 -0
+/**
+ *	cs5536_set_pio_mode		-	PIO timing setup
+ *	@drive: ATA device
+ *	@pio: PIO mode number
+ */
+
+static void cs5536_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+	static const u8 drv_timings[5] = {
+		0x98, 0x55, 0x32, 0x21, 0x20,
+	};
+
+	static const u8 addr_timings[5] = {

drivers/ide/cs5536.c line 146

Quoted 10 lines
drivers/ide/cs5536.c new file +10 -0
+		0x2, 0x1, 0x0, 0x0, 0x0,
+	};
+
+	static const u8 cmd_timings[5] = {
+		0x99, 0x92, 0x90, 0x22, 0x20,
+	};
+
+	struct pci_dev *pdev = to_pci_dev(drive->hwif->dev);
+	ide_drive_t *pair = ide_get_pair_dev(drive);
+	int cshift = (drive->dn & 1) ? IDE_CAST_D1_SHIFT : IDE_CAST_D0_SHIFT;

Same comment...

drivers/ide/cs5536.c line 153

Quoted 7 lines
drivers/ide/cs5536.c new file +7 -0
+	u32 cast;
+	u8 cmd_pio = pio;
+
+	if (pair)
+		cmd_pio = min(pio, ide_get_best_pio_mode(pair, 255, 4));
+
+	drive->drive_data &= 0xff00;

IDE_DRV_MASK << 8?

Quoted 20 lines
drivers/ide/cs5536.c new file +19 -0
+/**
+ *	cs5536_set_dma_mode		-	DMA timing setup
+ *	@drive: ATA device
+ *	@mode: DMA mode
+ */
+
+static void cs5536_set_dma_mode(ide_drive_t *drive, const u8 mode)
+{
+	static const u8 udma_timings[6] = {
+		0xc2, 0xc1, 0xc0, 0xc4, 0xc5, 0xc6,
+	};
+
+	static const u8 mwdma_timings[3] = {
+		0x67, 0x21, 0x20,
+	};
+
+	struct pci_dev *pdev = to_pci_dev(drive->hwif->dev);
+	int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;
+	u32 etc;

drivers/ide/cs5536.c line 197

Quoted 10 lines
drivers/ide/cs5536.c new file +10 -0
+
+	if (mode >= XFER_UDMA_0) {
+		cs5536_read(pdev, ETC, &etc);
+
+		etc &= ~(IDE_DRV_MASK << dshift);
+		etc |= udma_timings[mode - XFER_UDMA_0] << dshift;
+
+		cs5536_write(pdev, ETC, etc);
+	} else { /* MWDMA */
+		drive->drive_data &= 0xff;

IDE_DRV_MASK?

drivers/ide/cs5536.c line 198

Quoted 2 lines
drivers/ide/cs5536.c new file +1 -0
+		drive->drive_data |= mwdma_timings[mode - XFER_MW_DMA_0] << 8;

How about disabling UltraDMA mode? While not an issue in the original
driver with the set_{dma|pio}mode() method call order strictly determined and the latter method disabling UltraDMA, here this becames an issue...

drivers/ide/cs5536.c line 205

Quoted 5 lines
drivers/ide/cs5536.c new file +4 -0
+static void cs5536_dma_start(ide_drive_t *drive)
+{
+	if (drive->current_speed < XFER_UDMA_0)
+		cs5536_program_dtc(drive, drive->drive_data >> 8);

Worth comparing the values as PIO4 and MWDMA2 timings exactly
correspond. Hm, PIO3 and MWDMA1 too...

drivers/ide/cs5536.c line 258

Quoted 13 lines
drivers/ide/cs5536.c new file +12 -0
+/**
+ *	cs5536_init_one
+ *	@dev: PCI device
+ *	@id: Entry in match table
+ */
+
+static int cs5536_init_one(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	u32 cfg;
+
+	if (use_msr)
+		printk(KERN_ERR DRV_NAME ": Using MSR regs instead of PCI\n");

Why KERN_ERR? :-O

drivers/ide/cs5536.c line 264

Quoted 6 lines
drivers/ide/cs5536.c new file +5 -0
+	cs5536_read(dev, CFG, &cfg);
+
+	if ((cfg & IDE_CFG_CHANEN) == 0) {
+		printk(KERN_ERR DRV_NAME ": disabled by BIOS\n");
+		return -ENODEV;

Eh, why not do it via the usual .enablebits mechanism?

MBR, Sergei

#22 lore raw

Hi,

Quoted 35 lines · Sergei Shtylyov

Hello.

Quoted 22 lines · Bartlomiej Zolnierkiewicz
Quoted 10 lines
Quoted 3 lines

2. The cs5535 ide driver doesn't seem to be able to recognize the newer CS5536 controller for IDE.

No wonder, it's even impossible to determine CS5536 IDE controller's
device ID from the datasheet; include/linux/pci_ids.h tells me that the device ID is 0x209A, so adding another ID to the 'cs5535' driver's ID table shouldn't be an issue -- if they are indeed compatible. Looking at the datasheets, they are not -- bad luck, we need a new driver... BTW, libata seems to already have support for this chipset.

Indeed...

Karl, care to give a try to the following patch (it is completely untested so please backup your data first if necessary)?

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: add CS5536 host driver

This is a port of libata's pata_cs5536.c (written by Martin K. Petersen) to IDE subsystem.

Quoted 8 lines

Changes done while at it:

* Reprogram PIO/MWDMA timings if needed before and after DMA transfer (chipset uses shared PIO/MWDMA timings).

* Fix cable detection to report 80-wires cable if BIOS set it for any device on a port (IDE core will do drive-side cable detection later).

Quoted 7 lines

The original pata_cs5536 cable detection is indeed somewhat bogus...

Quoted 2 lines

* CS5536 is used mostly for driving CF cards and this allows use of

I woulnd't be so sure -- but anyway, I mostly see the development
boards... :-)

I cleaned patch description a bit based on your and Alan's concerns.

Quoted 14 lines
Quoted 5 lines
Cc Martin K. Petersen <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Karl Auerbach <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Looks good but needs some polishing...

Quoted 6 lines

Index: b/drivers/ide/Kconfig
===================================================================

drivers/ide/Kconfig +0 -0
@@ -465,6 +465,16 @@ config BLK_DEV_CS5535
Quoted 8 lines

drivers/ide/Kconfig config BLK_DEV_CS5535 · line 471

Quoted 6 lines
drivers/ide/Kconfig +3 -0
 	  It is safe to say Y to this question.
 
+config BLK_DEV_CS5536
+	tristate "CS5536 chipset support"
+	depends on X86 && !X86_64

Why not just depend on X86_32?

Fixed (it could be that original driver predated x86 merge).

Quoted 36 lines
Quoted 6 lines

Index: b/drivers/ide/cs5536.c
===================================================================

drivers/ide/cs5536.c new file +0 -0
@@ -0,0 +1,303 @@

[...]

Quoted 6 lines

+ * The IDE timing registers for the CS5536 live in the Geode Machine
+ * Specific Register file and not PCI config space. Most BIOSes
+ * virtualize the PCI registers so the chip looks like a standard IDE
+ * controller. Unfortunately not all implementations get this right.
+ * In particular some have problems with unaligned accesses to the

I'd say that people doing unaligned accesses are just looking for
trouble... :-)

Quoted 19 lines
drivers/ide/cs5536.c new file +19 -0
+enum {
+	MSR_IDE_CFG		= 0x51300010,
+	PCI_IDE_CFG		= 0x40,
+
+	CFG			= 0,
+	DTC			= 2,
+	CAST			= 3,
+	ETC			= 4,
+
+	IDE_CFG_CHANEN		= (1 << 1),
+	IDE_CFG_CABLE		= (1 << 17) | (1 << 16),
+
+	IDE_D0_SHIFT		= 24,
+	IDE_D1_SHIFT		= 16,
+	IDE_DRV_MASK		= 0xff,
+
+	IDE_CAST_D0_SHIFT	= 6,
+	IDE_CAST_D1_SHIFT	= 4,
+	IDE_CAST_DRV_MASK	= 0x3,
Quoted 8 lines

drivers/ide/cs5536.c

Quoted 5 lines
drivers/ide/cs5536.c new file +4 -0
+
+	IDE_CAST_CMD_SHIFT	= 24,
+	IDE_CAST_CMD_MASK	= 0xff,
+};

Declaring a lot of semi-related constants is not what enum was
intended for I think...

Don't know about that but it is still better than using defines.

Quoted 8 lines

drivers/ide/cs5536.c

Quoted 5 lines
drivers/ide/cs5536.c new file +4 -0
+static void cs5536_program_dtc(ide_drive_t *drive, u8 tim)
+{
+	struct pci_dev *pdev = to_pci_dev(drive->hwif->dev);
+	int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;

Masking with 1 is pointless (though harmless) for this single-channel
controller.

I prefer not to do such micro-optimizations (the gain of doing it for whole driver is only 20 bytes) as this keeps code consistent with other drivers and results in one gotcha less if somebody decides to use this particular driver as a base for a new one.

Quoted 27 lines
Quoted 14 lines
drivers/ide/cs5536.c new file +13 -0
+/**
+ *	cs5536_set_pio_mode		-	PIO timing setup
+ *	@drive: ATA device
+ *	@pio: PIO mode number
+ */
+
+static void cs5536_set_pio_mode(ide_drive_t *drive, const u8 pio)
+{
+	static const u8 drv_timings[5] = {
+		0x98, 0x55, 0x32, 0x21, 0x20,
+	};
+
+	static const u8 addr_timings[5] = {

drivers/ide/cs5536.c

Quoted 10 lines
drivers/ide/cs5536.c new file +10 -0
+		0x2, 0x1, 0x0, 0x0, 0x0,
+	};
+
+	static const u8 cmd_timings[5] = {
+		0x99, 0x92, 0x90, 0x22, 0x20,
+	};
+
+	struct pci_dev *pdev = to_pci_dev(drive->hwif->dev);
+	ide_drive_t *pair = ide_get_pair_dev(drive);
+	int cshift = (drive->dn & 1) ? IDE_CAST_D1_SHIFT : IDE_CAST_D0_SHIFT;

Same comment...

Quoted 9 lines

drivers/ide/cs5536.c

Quoted 7 lines
drivers/ide/cs5536.c new file +7 -0
+	u32 cast;
+	u8 cmd_pio = pio;
+
+	if (pair)
+		cmd_pio = min(pio, ide_get_best_pio_mode(pair, 255, 4));
+
+	drive->drive_data &= 0xff00;

   IDE_DRV_MASK << 8?

Fixed.

Quoted 24 lines
Quoted 24 lines
drivers/ide/cs5536.c new file +24 -0
+/**
+ *	cs5536_set_dma_mode		-	DMA timing setup
+ *	@drive: ATA device
+ *	@mode: DMA mode
+ */
+
+static void cs5536_set_dma_mode(ide_drive_t *drive, const u8 mode)
+{
+	static const u8 udma_timings[6] = {
+		0xc2, 0xc1, 0xc0, 0xc4, 0xc5, 0xc6,
+	};
+
+	static const u8 mwdma_timings[3] = {
+		0x67, 0x21, 0x20,
+	};
+
+	struct pci_dev *pdev = to_pci_dev(drive->hwif->dev);
+	int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;
+	u32 etc;
+
+	if (mode >= XFER_UDMA_0) {
+		cs5536_read(pdev, ETC, &etc);
+
+		etc &= ~(IDE_DRV_MASK << dshift);
Quoted 8 lines

drivers/ide/cs5536.c

Quoted 6 lines
drivers/ide/cs5536.c new file +5 -0
+		etc |= udma_timings[mode - XFER_UDMA_0] << dshift;
+
+		cs5536_write(pdev, ETC, etc);
+	} else { /* MWDMA */
+		drive->drive_data &= 0xff;

IDE_DRV_MASK?

Fixed.

Quoted 7 lines

drivers/ide/cs5536.c

Quoted 2 lines
drivers/ide/cs5536.c new file +1 -0
+		drive->drive_data |= mwdma_timings[mode - XFER_MW_DMA_0] << 8;

How about disabling UltraDMA mode? While not an issue in the original
driver with the set_{dma|pio}mode() method call order strictly determined and the latter method disabling UltraDMA, here this becames an issue...

Agreed, fixed.

Quoted 8 lines

drivers/ide/cs5536.c

Quoted 5 lines
drivers/ide/cs5536.c new file +4 -0
+static void cs5536_dma_start(ide_drive_t *drive)
+{
+	if (drive->current_speed < XFER_UDMA_0)
+		cs5536_program_dtc(drive, drive->drive_data >> 8);

Worth comparing the values as PIO4 and MWDMA2 timings exactly
correspond. Hm, PIO3 and MWDMA1 too...

Good catch, fixed.

Quoted 7 lines
Quoted 7 lines
drivers/ide/cs5536.c new file +7 -0
+/**
+ *	cs5536_init_one
+ *	@dev: PCI device
+ *	@id: Entry in match table
+ */
+
+static int cs5536_init_one(struct pci_dev *dev, const struct pci_device_id *id)
Quoted 8 lines

drivers/ide/cs5536.c

Quoted 6 lines
drivers/ide/cs5536.c new file +5 -0
+{
+	u32 cfg;
+
+	if (use_msr)
+		printk(KERN_ERR DRV_NAME ": Using MSR regs instead of PCI\n");

Why KERN_ERR? :-O

Copied from original driver. :)

I changed it to KERN_INFO.

Quoted 8 lines

drivers/ide/cs5536.c

Quoted 6 lines
drivers/ide/cs5536.c new file +5 -0
+	cs5536_read(dev, CFG, &cfg);
+
+	if ((cfg & IDE_CFG_CHANEN) == 0) {
+		printk(KERN_ERR DRV_NAME ": disabled by BIOS\n");
+		return -ENODEV;

Eh, why not do it via the usual .enablebits mechanism?

Because we may be using MSR access instead of PCI one.

Thanks for review Sergei, new patch description + v1->v2 interdiff below (v2 has been merged into pata-2.6 tree now if somebody wants to see the full patch):

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: add CS5536 host driver (v2)

This is a port of libata's pata_cs5536.c (written by Martin K. Petersen) to IDE subsystem.

Changes done while at it:

* Reprogram PIO/MWDMA timings if needed before and after DMA transfer (chipset uses shared PIO/MWDMA timings).

* Fix cable detection to report 80-wires cable if BIOS set it for any device on a port (IDE core will do drive-side cable detection later).

* Don't disable UDMA while programming PIO timings.

* Simplify PCI/MSR support.

Pros of having IDE host driver in addition to libata's one:

* IDE is much lighter than SCSI+libata, the host driver itself is also a bit smaller:

   text    data     bss     dec     hex filename
   1237     500       4    1741     6cd drivers/ata/pata_cs5536.o
   1214     128       4    1346     542 drivers/ide/cs5536.o

* This allows use of IDE features which are unavailable under libata.

v2:
* Fixes per review from Sergei:
- simplify dependency check in Kconfig
- use IDE_DRV_MASK also for ->drive_data
- disable UDMA when programming MWDMA
- program new DTC timings only when necessary
- fix printk() level in cs5536_init_one()

* Fix patch description according to comments from Alan and Sergei.

Cc Martin K. Petersen <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Karl Auerbach <[email protected]>
Cc Alan Cox <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
v1->v2 interdiff only

drivers/ide/Kconfig +1 -1
diff -u b/drivers/ide/Kconfig b/drivers/ide/Kconfig
@@ -467,7 +467,7 @@
 
 config BLK_DEV_CS5536
 	tristate "CS5536 chipset support"
-	depends on X86 && !X86_64
+	depends on X86_32
 	select BLK_DEV_IDEDMA_PCI
 	help
 	  This option enables support for the AMD CS5536
drivers/ide/cs5536.c +16 -10
diff -u b/drivers/ide/cs5536.c b/drivers/ide/cs5536.c
@@ -61,6 +61,9 @@
 
 	IDE_CAST_CMD_SHIFT	= 24,
 	IDE_CAST_CMD_MASK	= 0xff,
+
+	IDE_ETC_UDMA_RELSHIFT	= 6,
+	IDE_ETC_UDMA_MASK	= 0x3,
 };
 
 static int use_msr;

@@ -150,7 +153,7 @@
 	if (pair)
 		cmd_pio = min(pio, ide_get_best_pio_mode(pair, 255, 4));
 
-	drive->drive_data &= 0xff00;
+	drive->drive_data &= (IDE_DRV_MASK << 8);
 	drive->drive_data |= drv_timings[pio];
 
 	cs5536_program_dtc(drive, drv_timings[pio]);

@@ -186,22 +189,24 @@
 	int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;
 	u32 etc;
 
-	if (mode >= XFER_UDMA_0) {
-		cs5536_read(pdev, ETC, &etc);
+	cs5536_read(pdev, ETC, &etc);
 
+	if (mode >= XFER_UDMA_0) {
 		etc &= ~(IDE_DRV_MASK << dshift);
 		etc |= udma_timings[mode - XFER_UDMA_0] << dshift;
-
-		cs5536_write(pdev, ETC, etc);
 	} else { /* MWDMA */
-		drive->drive_data &= 0xff;
+		etc &= ~(IDE_ETC_UDMA_MASK << (dshift + IDE_ETC_UDMA_RELSHIFT));
+		drive->drive_data &= IDE_DRV_MASK;
 		drive->drive_data |= mwdma_timings[mode - XFER_MW_DMA_0] << 8;
 	}
+
+	cs5536_write(pdev, ETC, etc);
 }
 
 static void cs5536_dma_start(ide_drive_t *drive)
 {
-	if (drive->current_speed < XFER_UDMA_0)
+	if (drive->current_speed < XFER_UDMA_0 &&
+	    (drive->drive_data >> 8) != (drive->drive_data & IDE_DRV_MASK))
 		cs5536_program_dtc(drive, drive->drive_data >> 8);
 
 	ide_dma_start(drive);

@@ -211,8 +216,9 @@
 {
 	int ret = ide_dma_end(drive);
 
-	if (drive->current_speed < XFER_UDMA_0)
-		cs5536_program_dtc(drive, drive->drive_data & 0xff);
+	if (drive->current_speed < XFER_UDMA_0 &&
+	    (drive->drive_data >> 8) != (drive->drive_data & IDE_DRV_MASK))
+		cs5536_program_dtc(drive, drive->drive_data & IDE_DRV_MASK);
 
 	return ret;
 }

@@ -255,7 +261,7 @@
 	u32 cfg;
 
 	if (use_msr)
-		printk(KERN_ERR DRV_NAME ": Using MSR regs instead of PCI\n");
+		printk(KERN_INFO DRV_NAME ": Using MSR regs instead of PCI\n");
 
 	cs5536_read(dev, CFG, &cfg);
Acked-by
#23 lore raw

Hello.

Quoted 29 lines · Bartlomiej Zolnierkiewicz
Quoted 19 lines
Quoted 19 lines
(quoted diff) +19 -0
+enum {
+	MSR_IDE_CFG		= 0x51300010,
+	PCI_IDE_CFG		= 0x40,
+
+	CFG			= 0,
+	DTC			= 2,
+	CAST			= 3,
+	ETC			= 4,
+
+	IDE_CFG_CHANEN		= (1 << 1),
+	IDE_CFG_CABLE		= (1 << 17) | (1 << 16),
+
+	IDE_D0_SHIFT		= 24,
+	IDE_D1_SHIFT		= 16,
+	IDE_DRV_MASK		= 0xff,
+
+	IDE_CAST_D0_SHIFT	= 6,
+	IDE_CAST_D1_SHIFT	= 4,
+	IDE_CAST_DRV_MASK	= 0x3,
Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +4 -0
+
+	IDE_CAST_CMD_SHIFT	= 24,
+	IDE_CAST_CMD_MASK	= 0xff,
+};

Declaring a lot of semi-related constants is not what enum was
intended for I think...

Quoted 2 lines

Don't know about that but it is still better than using defines.

That's what I doubt...

Quoted 10 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +5 -0
+	cs5536_read(dev, CFG, &cfg);
+
+	if ((cfg & IDE_CFG_CHANEN) == 0) {
+		printk(KERN_ERR DRV_NAME ": disabled by BIOS\n");
+		return -ENODEV;

Eh, why not do it via the usual .enablebits mechanism?

Quoted 2 lines

Because we may be using MSR access instead of PCI one.

Ah...

Quoted 39 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: add CS5536 host driver (v2)

This is a port of libata's pata_cs5536.c (written by Martin K. Petersen) to IDE subsystem.

Changes done while at it:

* Reprogram PIO/MWDMA timings if needed before and after DMA transfer (chipset uses shared PIO/MWDMA timings).

* Fix cable detection to report 80-wires cable if BIOS set it for any device on a port (IDE core will do drive-side cable detection later).

* Don't disable UDMA while programming PIO timings.

* Simplify PCI/MSR support.

Pros of having IDE host driver in addition to libata's one:

* IDE is much lighter than SCSI+libata, the host driver itself is also a bit smaller:

   text    data     bss     dec     hex filename
   1237     500       4    1741     6cd drivers/ata/pata_cs5536.o
   1214     128       4    1346     542 drivers/ide/cs5536.o

* This allows use of IDE features which are unavailable under libata.

v2:
* Fixes per review from Sergei:
- simplify dependency check in Kconfig
- use IDE_DRV_MASK also for ->drive_data
- disable UDMA when programming MWDMA
- program new DTC timings only when necessary
- fix printk() level in cs5536_init_one()

* Fix patch description according to comments from Alan and Sergei.

Quoted 8 lines
Cc Martin K. Petersen <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Karl Auerbach <[email protected]>
Cc Alan Cox <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
v1->v2 interdiff only

Acked-by Sergei Shtylyov <[email protected]>

drivers/ide/cs5536.c line 67

Quoted 11 lines

diff -u b/drivers/ide/cs5536.c b/drivers/ide/cs5536.c

drivers/ide/cs5536.c +3 -0
@@ -61,6 +61,9 @@
 
 	IDE_CAST_CMD_SHIFT	= 24,
 	IDE_CAST_CMD_MASK	= 0xff,
+
+	IDE_ETC_UDMA_RELSHIFT	= 6,
+	IDE_ETC_UDMA_MASK	= 0x3,

Too many shifts and mask values to my taste, why not just use 0xC0?

drivers/ide/cs5536.c line 196

Quoted 11 lines
drivers/ide/cs5536.c +2 -2
@@ -186,22 +189,24 @@
 	int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;
 	u32 etc;
 
-	if (mode >= XFER_UDMA_0) {
-		cs5536_read(pdev, ETC, &etc);
+	cs5536_read(pdev, ETC, &etc);
 
+	if (mode >= XFER_UDMA_0) {
 		etc &= ~(IDE_DRV_MASK << dshift);

Er, I'm not sure using IDE_DRV_MASK here is completely correct as
only the mask 0xC7 actually controls UltraDMA enables and cycle time...

MBR, Sergei

#24 lore raw
Quoted 109 lines · Sergei Shtylyov

Hello.

Quoted 29 lines · Bartlomiej Zolnierkiewicz
Quoted 19 lines
Quoted 19 lines
(quoted diff) +19 -0
+enum {
+	MSR_IDE_CFG		= 0x51300010,
+	PCI_IDE_CFG		= 0x40,
+
+	CFG			= 0,
+	DTC			= 2,
+	CAST			= 3,
+	ETC			= 4,
+
+	IDE_CFG_CHANEN		= (1 << 1),
+	IDE_CFG_CABLE		= (1 << 17) | (1 << 16),
+
+	IDE_D0_SHIFT		= 24,
+	IDE_D1_SHIFT		= 16,
+	IDE_DRV_MASK		= 0xff,
+
+	IDE_CAST_D0_SHIFT	= 6,
+	IDE_CAST_D1_SHIFT	= 4,
+	IDE_CAST_DRV_MASK	= 0x3,
Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +4 -0
+
+	IDE_CAST_CMD_SHIFT	= 24,
+	IDE_CAST_CMD_MASK	= 0xff,
+};

Declaring a lot of semi-related constants is not what enum was
intended for I think...

Quoted 2 lines

Don't know about that but it is still better than using defines.

That's what I doubt...

Quoted 10 lines
Quoted 9 lines

Review

Quoted 7 lines
(quoted diff) +5 -0
+	cs5536_read(dev, CFG, &cfg);
+
+	if ((cfg & IDE_CFG_CHANEN) == 0) {
+		printk(KERN_ERR DRV_NAME ": disabled by BIOS\n");
+		return -ENODEV;

Eh, why not do it via the usual .enablebits mechanism?

Quoted 2 lines

Because we may be using MSR access instead of PCI one.

   Ah...
Quoted 39 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: add CS5536 host driver (v2)

This is a port of libata's pata_cs5536.c (written by Martin K. Petersen) to IDE subsystem.

Changes done while at it:

* Reprogram PIO/MWDMA timings if needed before and after DMA transfer (chipset uses shared PIO/MWDMA timings).

* Fix cable detection to report 80-wires cable if BIOS set it for any device on a port (IDE core will do drive-side cable detection later).

* Don't disable UDMA while programming PIO timings.

* Simplify PCI/MSR support.

Pros of having IDE host driver in addition to libata's one:

* IDE is much lighter than SCSI+libata, the host driver itself is also a bit smaller:

   text    data     bss     dec     hex filename
   1237     500       4    1741     6cd drivers/ata/pata_cs5536.o
   1214     128       4    1346     542 drivers/ide/cs5536.o

* This allows use of IDE features which are unavailable under libata.

v2:
* Fixes per review from Sergei:
- simplify dependency check in Kconfig
- use IDE_DRV_MASK also for ->drive_data
- disable UDMA when programming MWDMA
- program new DTC timings only when necessary
- fix printk() level in cs5536_init_one()

* Fix patch description according to comments from Alan and Sergei.

Quoted 8 lines
Cc Martin K. Petersen <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Karl Auerbach <[email protected]>
Cc Alan Cox <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
v1->v2 interdiff only

Acked-by Sergei Shtylyov <[email protected]>
Quoted 5 lines

diff -u b/drivers/ide/cs5536.c b/drivers/ide/cs5536.c

drivers/ide/cs5536.c +0 -0
@@ -61,6 +61,9 @@
Quoted 8 lines

drivers/ide/cs5536.c line 67

Quoted 6 lines
drivers/ide/cs5536.c +3 -0
 	IDE_CAST_CMD_SHIFT	= 24,
 	IDE_CAST_CMD_MASK	= 0xff,
+
+	IDE_ETC_UDMA_RELSHIFT	= 6,
+	IDE_ETC_UDMA_MASK	= 0x3,

Too many shifts and mask values to my taste, why not just use 0xC0?

Fixed, I have no idea why I did it this way originally... :)

Quoted 8 lines
Quoted 8 lines
drivers/ide/cs5536.c +1 -2
@@ -186,22 +189,24 @@
 	int dshift = (drive->dn & 1) ? IDE_D1_SHIFT : IDE_D0_SHIFT;
 	u32 etc;
 
-	if (mode >= XFER_UDMA_0) {
-		cs5536_read(pdev, ETC, &etc);
+	cs5536_read(pdev, ETC, &etc);
Quoted 6 lines

drivers/ide/cs5536.c line 196

Quoted 3 lines
drivers/ide/cs5536.c +1 -0
+	if (mode >= XFER_UDMA_0) {
 		etc &= ~(IDE_DRV_MASK << dshift);

Er, I'm not sure using IDE_DRV_MASK here is completely correct as
only the mask 0xC7 actually controls UltraDMA enables and cycle time...

Bits 0x38 are described to be hardwired to 0 in the datasheet so this should be OK (also pata_cs5536.c does the same thing)...

v2->v3 interdiff:
...
v3:
* Smarter masking of UDMA bits per Sergei's suggestion.
...

drivers/ide/cs5536.c +2 -3
diff -u b/drivers/ide/cs5536.c b/drivers/ide/cs5536.c
@@ -62,8 +62,7 @@
 	IDE_CAST_CMD_SHIFT	= 24,
 	IDE_CAST_CMD_MASK	= 0xff,
 
-	IDE_ETC_UDMA_RELSHIFT	= 6,
-	IDE_ETC_UDMA_MASK	= 0x3,
+	IDE_ETC_UDMA_MASK	= 0xc0,
 };
 
 static int use_msr;

@@ -195,7 +194,7 @@
 		etc &= ~(IDE_DRV_MASK << dshift);
 		etc |= udma_timings[mode - XFER_UDMA_0] << dshift;
 	} else { /* MWDMA */
-		etc &= ~(IDE_ETC_UDMA_MASK << (dshift + IDE_ETC_UDMA_RELSHIFT));
+		etc &= ~(IDE_ETC_UDMA_MASK << dshift);
 		drive->drive_data &= IDE_DRV_MASK;
 		drive->drive_data |= mwdma_timings[mode - XFER_MW_DMA_0] << 8;
 	}