[coreboot-gerrit] [S] Change in coreboot[master]: drivers/ipmi: retry ipmi_get_device_id in ipmi_kcs_init

[Paul Menzel (Code Review)]

Attention is currently required from: Yiwei Tang, Ziang Wang.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75739?usp=email )

Change subject: drivers/ipmi: retry ipmi_get_device_id in ipmi_kcs_init
......................................................................

Patch Set 2:

(7 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/75739/comment/04f9afb9_08303387 :
PS2, Line 9: our test
Please elaborate, how you tested things.

https://review.coreboot.org/c/coreboot/+/75739/comment/719683d9_db8cf507 :
PS2, Line 9: fails
How exactly?

https://review.coreboot.org/c/coreboot/+/75739/comment/e000da48_d510b039 :
PS2, Line 10: this retry mechanism
Please elaborate. How many tries, and why did you choose this heuristic?

https://review.coreboot.org/c/coreboot/+/75739/comment/a74cea63_b9fdc21a :
PS2, Line 13: Signed-off-by: wanghao11 
Please use the full name.

Patchset:

PS2: 
Please elaborate much more in the commit message.

File src/drivers/ipmi/ipmi_kcs_ops.c:

https://review.coreboot.org/c/coreboot/+/75739/comment/2fbab825_5fe6b35f :
PS2, Line 124: if (!wait_ms(IPMI_GET_DID_RETRY_MS, !ipmi_get_device_id(dev, &rsp))) {
             : 		printk(BIOS_ERR, "IPMI: BMC does not respond to get device id even "
             : 			"after %d ms.\n", IPMI_GET_DID_RETRY_MS);
             : 		dev->enabled = 0;
             : 		return;
             : 	}
Isn’t this code trying to do exactly the same, and shouldn’t you need to just increase IPMI_GET_DID_RETRY_MS?

https://review.coreboot.org/c/coreboot/+/75739/comment/db1d10b4_28108846 :
PS2, Line 139: 		mdelay(1000);
That’s too much of a delay for coreboot.