Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48925 )
Change subject: device/pnp,util/sconfig: introduce and use `REG8` resource type ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48925/8/src/device/pnp_device.c File src/device/pnp_device.c:
https://review.coreboot.org/c/coreboot/+/48925/8/src/device/pnp_device.c@286 PS8, Line 286: if (info->flags & PNP_MSC0) { : resource = new_resource(dev, PNP_IDX_MSC0); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC1) { : resource = new_resource(dev, PNP_IDX_MSC1); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC2) { : resource = new_resource(dev, PNP_IDX_MSC2); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC3) { : resource = new_resource(dev, PNP_IDX_MSC3); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC4) { : resource = new_resource(dev, PNP_IDX_MSC4); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC5) { : resource = new_resource(dev, PNP_IDX_MSC5); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC6) { : resource = new_resource(dev, PNP_IDX_MSC6); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC7) { : resource = new_resource(dev, PNP_IDX_MSC7); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC8) { : resource = new_resource(dev, PNP_IDX_MSC8); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSC9) { : resource = new_resource(dev, PNP_IDX_MSC9); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSCA) { : resource = new_resource(dev, PNP_IDX_MSCA); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSCB) { : resource = new_resource(dev, PNP_IDX_MSCB); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSCC) { : resource = new_resource(dev, PNP_IDX_MSCC); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSCD) { : resource = new_resource(dev, PNP_IDX_MSCD); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : if (info->flags & PNP_MSCE) { : resource = new_resource(dev, PNP_IDX_MSCE); : resource->size = 1; : resource->flags |= IORESOURCE_REG8; : } : }
AFAIK, all the resources are written to registers. Why wouldn't pnp_set_*()
above be called?
AFAICS pnp_set_resource() only calls pnp_set_*() if a resource type/flag is assigned. That assignment happens in get_resources()
Optionally, yes. But for everything in the `devicetree.cb` it's done in `sconfig` already.
This code here merely adds information beyond what is given in the `devicetree.cb`.
For MSC* it only sets some expectations, i.e. if an sio driver says there is MSC*, and no value is provided in the dt, we warn.
Ack on that but why would we only do that for 0xf* but not for 0x2*, 0xc*, 0xd*, and all the other ranges, like Felix mentioned, too?
I assume because no sio-driver author missed that, yet. Most might not care or might not know about the possibilities. We could drop it if we assume that nobody cares. Or keep it if there is any sio driver that actually sets something useful.
So, either should we handle *all* registers or none.
I think the author of the sio driver should decide that.
I'm talking about the common code above. What the sio driver expects (PNP_* added to LDNs) is the drivers decision, ofc
Me too. That common code above acts on what the sio driver tells it. Do you see all the `if`s?
Ofc, I marked them. Do you see it? ;) We're just talking past each other obviously
Yeah, let's back up a little. The sio-driver can add these PNP_MSC* flags to show that the mainboard is supposed to set the regs. This here is just support code so we can do the check (if the mainboard set a value) in a single place, pnp_set_resource() above. Otherwise, each sio driver would have to do its own checks.
So I'd say we can decide if the checks make sense. If we drop them, the support code would vanish as well. But I see no reason (yet) to discuss the support code separately.
https://review.coreboot.org/c/coreboot/+/48925/8/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/48925/8/util/sconfig/main.c@1037 PS8, Line 1037: flags Here the resource's `flags` get set.