Michael Niewöhner 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:
(1 comment)
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; : } : }
Nico:
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() but only for MSC* registers (0xf0-0xfe), *not* for 0xc* for example, so I would expect no pnp_set_*() being called for 0xc* at all, except I miss something.
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?
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