I've compiled support for the Siemens 'sitemp' Mainboard. The board is similar to the AMD dbm690t. I adapt the code from dbm690t according to customer requirement specification. Linux is booting with seabios/filo. Windows XP is booting with seabios.
This board comes with two hardware variants so I have to set up subvendor/device ids accordingly. Also I've found an issue: The default 'pci_set_subsystem' sets the subvendor/device id register# 0x2c. However, for a few devices in the RS690 the register is 0x4c respectively 0x50.
Moved 'pci_set_subsystem' to a weak function in 'pci_device.c'. So boards can add an override for the default function in mainboard/../override_df.c (or mainboard.c) and set it up too.
Hoping to receive a 'ack'.
Thanks
Signed-off-by: Josef Kellermann joseph.kellermann@heitec.de
********************************************************************************************** IMPORTANT NOTICE / WICHTIGER HINWEIS This communication contains information which is confidential and may also be privileged. It is for the exclusive use of the intended recipient(s). If you are not the intended recipient(s) please note that any distribution, copying or use of this communication or the information in it is strictly prohibited. If you have received this communication in error please notify us immediately by email or by telephone and then delete this email and any copies of it. Diese E-Mail koennte vertrauliche und/oder rechtlich geschuetzte Informationen enthalten. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail sind nicht gestattet.
HEITEC AG, Werner-von-Siemens-Str. 61, 91052 Erlangen, HRB 7754, RG Fuerth, Vorstandsvorsitzender: Richard Heindl, Vorstand: Harald Preiml, Finanzvorstand: Rene-Oliver Rosner, Aufsichtsratsvorsitzender: Dr. Lorenz M. Raith, WWW: http://www.heitec.de **********************************************************************************************
Am Dienstag, den 25.01.2011, 10:44 +0000 schrieb Joseph Kellermann:
Also I've found an issue: The default 'pci_set_subsystem' sets the subvendor/device id register# 0x2c. However, for a few devices in the RS690 the register is 0x4c respectively 0x50.
Moved 'pci_set_subsystem' to a weak function in 'pci_device.c'. So boards can add an override for the default function in mainboard/../override_df.c (or mainboard.c) and set it up too.
Given that these implement chipset specific behaviour (either the device requires writing to 0x4c instead of 0x2c or it doesn't), and that pci_dev_set_subsystem is always called indirectly (via function pointers in the respective chipset drivers), couldn't you just modify the chipset drivers to point to a different set_subsystem implementation where necessary?
It seems like the GFX function of RS690 is affected (just skimmed through the AMD docs, so others might also require such a change). In this case, it should work to change src/southbridge/amd/rs690/gfx.c to point lops.set_subsystem to your own implementation which does whatever their GFX device needs (write to 0x4c in extended config space in that case, if I understand the docs), no?
Hoping to receive a 'ack'. Signed-off-by: Josef Kellermann joseph.kellermann@heitec.de
If this change is actually required, I'll ack and commit it, but I'd like to understand your use case better first. We already have a layer of indirection in place, introducing another one will only create confusion down the road.
Regards, Patrick
couldn't you just modify the chipset drivers to point to a different set_subsystem implementation where necessary?
Yes, I can do, but how can I implement these requirement:
board comes with two hardware variants so I have to set up subvendor/device ids accordingly.
that's, in 'mainboard.c' I check for 'PLX device present' and set up subdevice ids accordingly.
Regards, Joseph ________________________________________ Von: Georgi, Patrick [Patrick.Georgi@secunet.com] Gesendet: Dienstag, 25. Januar 2011 12:16 Bis: Joseph Kellermann Cc: coreboot@coreboot.org Betreff: Re: [coreboot] Moved 'pci_set_subsystem' to a weak function: Support for Siemens Mainboard
Am Dienstag, den 25.01.2011, 10:44 +0000 schrieb Joseph Kellermann:
Also I've found an issue: The default 'pci_set_subsystem' sets the subvendor/device id register# 0x2c. However, for a few devices in the RS690 the register is 0x4c respectively 0x50.
Moved 'pci_set_subsystem' to a weak function in 'pci_device.c'. So boards can add an override for the default function in mainboard/../override_df.c (or mainboard.c) and set it up too.
Given that these implement chipset specific behaviour (either the device requires writing to 0x4c instead of 0x2c or it doesn't), and that pci_dev_set_subsystem is always called indirectly (via function pointers in the respective chipset drivers), couldn't you just modify the chipset drivers to point to a different set_subsystem implementation where necessary?
It seems like the GFX function of RS690 is affected (just skimmed through the AMD docs, so others might also require such a change). In this case, it should work to change src/southbridge/amd/rs690/gfx.c to point lops.set_subsystem to your own implementation which does whatever their GFX device needs (write to 0x4c in extended config space in that case, if I understand the docs), no?
Hoping to receive a 'ack'. Signed-off-by: Josef Kellermann joseph.kellermann@heitec.de
If this change is actually required, I'll ack and commit it, but I'd like to understand your use case better first. We already have a layer of indirection in place, introducing another one will only create confusion down the road.
Regards, Patrick
********************************************************************************************** IMPORTANT NOTICE / WICHTIGER HINWEIS This communication contains information which is confidential and may also be privileged. It is for the exclusive use of the intended recipient(s). If you are not the intended recipient(s) please note that any distribution, copying or use of this communication or the information in it is strictly prohibited. If you have received this communication in error please notify us immediately by email or by telephone and then delete this email and any copies of it. Diese E-Mail koennte vertrauliche und/oder rechtlich geschuetzte Informationen enthalten. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail sind nicht gestattet.
HEITEC AG, Werner-von-Siemens-Str. 61, 91052 Erlangen, HRB 7754, RG Fuerth, Vorstandsvorsitzender: Richard Heindl, Vorstand: Harald Preiml, Finanzvorstand: Rene-Oliver Rosner, Aufsichtsratsvorsitzender: Dr. Lorenz M. Raith, WWW: http://www.heitec.de **********************************************************************************************
Am Dienstag, den 25.01.2011, 12:03 +0000 schrieb Joseph Kellermann:
Yes, I can do, but how can I implement these requirement:
board comes with two hardware variants so I have to set up subvendor/device ids accordingly.
that's, in 'mainboard.c' I check for 'PLX device present' and set up subdevice ids accordingly.
Hm.. we could route the CONFIG_* values through weak functions, that default to identity functions.
This way, we don't intermix one issue (different register numbers to program subsystem IDs) with another (different IDs depending on what hardware the build runs on).
This mixing things has two disadvantages: 1. Devices that override pci_dev_set_subsystem with their own function won't see your subsystem ID modification, 2. We have to copy&paste the register ID change between boards to have them all benefit from it (or suffer #1).
I'm not quite sure if using weak functions to wrap the subsystem CONFIG_* values is actually the right approach, I'll work on a patch to discuss.
Will the subvendor/subdevice ids be the same for all devices or is that modification special for certain devices (eg. gfx only)?
Regards, Patrick
Am Dienstag, den 25.01.2011, 13:24 +0100 schrieb Georgi, Patrick:
I'm not quite sure if using weak functions to wrap the subsystem CONFIG_* values is actually the right approach, I'll work on a patch to discuss.
Here it is. I tested it by providing a mainboard_pci_subsystem_vendor_id for my board in its mainboard.c which returned a different constant value - this was picked up by properly and reported on boot. mainboard_pci_subsystem_device_id will work just the same.
Joseph, will this suffice to help you implement your requirement?
Everyone, any opinion on the design? My main issue is that other code can still use the CONFIG_* values directly. Maybe our lint mechanism should look for that? Any other issues you have with this?
Signed-off-by: Patrick Georgi patrick.georgi@secunet.com
Great, i will test it.
Thanks, Joseph ________________________________________ Von: Georgi, Patrick [Patrick.Georgi@secunet.com] Gesendet: Dienstag, 25. Januar 2011 13:54 Bis: Joseph Kellermann Cc: coreboot@coreboot.org Betreff: Re: [coreboot] Moved 'pci_set_subsystem' to a weak function: Support for Siemens Mainboard
Am Dienstag, den 25.01.2011, 13:24 +0100 schrieb Georgi, Patrick:
I'm not quite sure if using weak functions to wrap the subsystem CONFIG_* values is actually the right approach, I'll work on a patch to discuss.
Here it is. I tested it by providing a mainboard_pci_subsystem_vendor_id for my board in its mainboard.c which returned a different constant value - this was picked up by properly and reported on boot. mainboard_pci_subsystem_device_id will work just the same.
Joseph, will this suffice to help you implement your requirement?
Everyone, any opinion on the design? My main issue is that other code can still use the CONFIG_* values directly. Maybe our lint mechanism should look for that? Any other issues you have with this?
Signed-off-by: Patrick Georgi patrick.georgi@secunet.com
********************************************************************************************** IMPORTANT NOTICE / WICHTIGER HINWEIS This communication contains information which is confidential and may also be privileged. It is for the exclusive use of the intended recipient(s). If you are not the intended recipient(s) please note that any distribution, copying or use of this communication or the information in it is strictly prohibited. If you have received this communication in error please notify us immediately by email or by telephone and then delete this email and any copies of it. Diese E-Mail koennte vertrauliche und/oder rechtlich geschuetzte Informationen enthalten. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail sind nicht gestattet.
HEITEC AG, Werner-von-Siemens-Str. 61, 91052 Erlangen, HRB 7754, RG Fuerth, Vorstandsvorsitzender: Richard Heindl, Vorstand: Harald Preiml, Finanzvorstand: Rene-Oliver Rosner, Aufsichtsratsvorsitzender: Dr. Lorenz M. Raith, WWW: http://www.heitec.de **********************************************************************************************
Ok, it works.
I will provide patches for the RS690 device specific register.
Regards, Joseph ________________________________________ Von: Georgi, Patrick [Patrick.Georgi@secunet.com] Gesendet: Dienstag, 25. Januar 2011 13:54 Bis: Joseph Kellermann Cc: coreboot@coreboot.org Betreff: Re: [coreboot] Moved 'pci_set_subsystem' to a weak function: Support for Siemens Mainboard
Am Dienstag, den 25.01.2011, 13:24 +0100 schrieb Georgi, Patrick:
I'm not quite sure if using weak functions to wrap the subsystem CONFIG_* values is actually the right approach, I'll work on a patch to discuss.
Here it is. I tested it by providing a mainboard_pci_subsystem_vendor_id for my board in its mainboard.c which returned a different constant value - this was picked up by properly and reported on boot. mainboard_pci_subsystem_device_id will work just the same.
Joseph, will this suffice to help you implement your requirement?
Everyone, any opinion on the design? My main issue is that other code can still use the CONFIG_* values directly. Maybe our lint mechanism should look for that? Any other issues you have with this?
Signed-off-by: Patrick Georgi patrick.georgi@secunet.com
********************************************************************************************** IMPORTANT NOTICE / WICHTIGER HINWEIS This communication contains information which is confidential and may also be privileged. It is for the exclusive use of the intended recipient(s). If you are not the intended recipient(s) please note that any distribution, copying or use of this communication or the information in it is strictly prohibited. If you have received this communication in error please notify us immediately by email or by telephone and then delete this email and any copies of it. Diese E-Mail koennte vertrauliche und/oder rechtlich geschuetzte Informationen enthalten. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail sind nicht gestattet.
HEITEC AG, Werner-von-Siemens-Str. 61, 91052 Erlangen, HRB 7754, RG Fuerth, Vorstandsvorsitzender: Richard Heindl, Vorstand: Harald Preiml, Finanzvorstand: Rene-Oliver Rosner, Aufsichtsratsvorsitzender: Dr. Lorenz M. Raith, WWW: http://www.heitec.de **********************************************************************************************
Patrick,
could you commit this patch ?
Regards, Joseph
________________________________________ Von: Georgi, Patrick [Patrick.Georgi@secunet.com] Gesendet: Dienstag, 25. Januar 2011 13:54 Bis: Joseph Kellermann Cc: coreboot@coreboot.org Betreff: Re: [coreboot] Moved 'pci_set_subsystem' to a weak function: Support for Siemens Mainboard
Am Dienstag, den 25.01.2011, 13:24 +0100 schrieb Georgi, Patrick:
I'm not quite sure if using weak functions to wrap the subsystem CONFIG_* values is actually the right approach, I'll work on a patch to discuss.
Here it is. I tested it by providing a mainboard_pci_subsystem_vendor_id for my board in its mainboard.c which returned a different constant value - this was picked up by properly and reported on boot. mainboard_pci_subsystem_device_id will work just the same.
Joseph, will this suffice to help you implement your requirement?
Everyone, any opinion on the design? My main issue is that other code can still use the CONFIG_* values directly. Maybe our lint mechanism should look for that? Any other issues you have with this?
Signed-off-by: Patrick Georgi patrick.georgi@secunet.com
********************************************************************************************** IMPORTANT NOTICE / WICHTIGER HINWEIS This communication contains information which is confidential and may also be privileged. It is for the exclusive use of the intended recipient(s). If you are not the intended recipient(s) please note that any distribution, copying or use of this communication or the information in it is strictly prohibited. If you have received this communication in error please notify us immediately by email or by telephone and then delete this email and any copies of it. Diese E-Mail koennte vertrauliche und/oder rechtlich geschuetzte Informationen enthalten. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail sind nicht gestattet.
HEITEC AG, Werner-von-Siemens-Str. 61, 91052 Erlangen, HRB 7754, RG Fuerth, Vorstandsvorsitzender: Richard Heindl, Vorstand: Harald Preiml, Finanzvorstand: Rene-Oliver Rosner, Aufsichtsratsvorsitzender: Dr. Lorenz M. Raith, WWW: http://www.heitec.de **********************************************************************************************
-
Georgi, Patrick -
Joseph Kellermann