Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32356 )
Change subject: drivers/usb/ucsi: Add driver to generate UCSI memory region ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
I'm struggling to see why you made the cut between generated and static ASL code where you made it. It all depends on the CBMEM address, right? Wouldn't it be enough to generate a named object, e.g.
Name (CBMU, 0x...)and use that in static code like
OperationRegion (UCSM, SystemMemory, CBMU, UCSI_SIZE)and something similar for _CRS, etc.
Yes that could work too.
What I really wanted was to put almost everything in the SSDT and just have the EC code provide the _DSM method, but DSDT ASL can't scope back into the SSDT so I couldn't declare the device here. Since I couldn't do that I decided to try and provide a framework and split the memory side into the a driver that could be reused if another EC implements UCSI in the future.
But it was just one of many possible paths. Making it into a library function that just allocates the cbmem region and adds a basic Name() for it is another, I will look at it and see how it turns out.
Defining an operationregion in the DSDT with an external IntObj does not seem to work, I end up with undefined references when the kernel boots.
I ended up removing the pointless drivers/usb/ucsi driver and just doing it in the EC driver directly though.