[coreboot-gerrit] Change in coreboot[master]: superio/fintek: Add support for Fintek F71889A.

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/17669 )

Change subject: superio/fintek: Add support for Fintek F71889A.
......................................................................


Patch Set 9: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/17669/9/src/superio/fintek/f71889a/superio.c
File src/superio/fintek/f71889a/superio.c:

PS9, Line 57: 0x07f8
After thinking a bit about the problem here, I'd say that 0x7ff is still the better option here. Both options have their downsides.

The keyboard controller uses two addresses in the io space, 0x60 and 0x64. I have no idea what the reasons were behind this decision, but that's what we have to deal with.

If a system also has an embedded controller (EC), the EC uses the io addresses 0x62 and 0x66. Again, don't ask me who thought that this is a good idea.

In this SIO chip there is only one register for setting the base io address for the keyboard controller and the second address is automatically the address of the first one plus 4 bytes.

When using 0x7ff here, only the io space for the first register will be reserved. If someone maps another peripheral to the address 0x64, you might see some weird effects. So the developers have to make sure to use sane values here, but that's something I consider acceptable.

When using 0x7f8, the io space for bot keyboard controller registers is reserved, but (and that's the problem in systems that also have an EC) the range also covers the addresses of the EC, so that will probably fail. I didn't think about this when I recommended using 0x7f8 as a mask here.

So all in all I'd use 0x7ff here, since the side effects of this are less bad.

tl;dr: I'd use 0x7ff here


-- 
To view, visit https://review.coreboot.org/17669
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I91c60a3b48cd4872ae7a27de8f49faa40e877a27
Gerrit-PatchSet: 9
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Marty Plummer <ntzrmtthihu777 at gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan at koparo.com>
Gerrit-Reviewer: Felix Held <felix-coreboot at felixheld.de>
Gerrit-Reviewer: Idwer Vollering <vidwer at gmail.com>
Gerrit-Reviewer: Marty Plummer <ntzrmtthihu777 at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes