Hi, On 5/13/26 18:05, Varadarajan Narayanan wrote:
+static u8 cfg_reg_idx[] = { + /* 0 to 18 */ + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, + /* 64 to 113 */ + 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, + 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, + 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, + 112, 113, +};This [1] says "Configure GENI primitive table.", and I have a sneaky suspicion that only the first part is actually that (iirc the Linux driver even errors out on writes to anything >256? I don't see any bound checking in u-boot, which is worrying...).I see that the same loop is used both in U-Boot [1] and Linux [2].
Hm, must have misremembered...
Also, these are registers with names.For some reason, this space is arranged as 2 blocks. 1. 0 - 18 regs, from base to base+0x4c 2. 64 - 113 regs, from base+0x200 to base+0x2c4These registers have the name GENI_CFG_REG_n with 'n' ranging from 0 to max as aliases and that has been used. Since we are dumping the content to these registers don't think names are needed here. If the registers were in a single block instead of the above arrangment it is just a memcpy_toio().
I guess it's possible that the register names are weird like that, but the word "primitives" has to come from somewhere... Maybe the bitfields are named but the registers are not for some reason? Ideally the whole design would be less opaque, but that's not really feasible with having to support the existing firmware format :/ I suppose even if that wasn't the case, it would be wasteful to assemble the bitfields at runtime, compared to just or-ing them together at compile time maybe with some macros. So other than the weird index-value indirection, and magic hex values instead of defines or'd together (and/or macro'd), I guess this design isn't as wild as it originally seemed to me.
Since the firmware format can't be changed and this just mimics it, I'd say it would make sense to improve that comment to explain what it's actually doing (since it's setting more than just primitives, whatever that means),We are not privy to the information that is held in this array. We just get this code from the hardware block owners.
Yeah, that's a very downstream approach that I'd prefer to see less off in Qualcomm's upstream efforts... Not sure if it would help here to be fair, but neither the person writing the drivers nor the reviewer having access to hardware documentation has led to unfortunate outcomes in the past and I really hoped that this wouldn't be an issue with first party upstream work. To be clear, I'm sure this is just me, the maintainers are probably fine with magic values (at least in this context), though if the bitfield names are available to you maybe constructing the array using defines and macros wouldn't be the worst idea... then again, I'm sure there's a point at which the maintainers will prefer lower verbosity even if the alternative would get rid of magic values.
and if these indices are going to be in this array anyway, why not replace them with defines of the register names? Or are those SoC-specific, seems that whatever ones the driver is accessing by name are pretty stable at least.Registers accessed by name are not part of this block. [1] https://github.com/u-boot/u-boot/blob/master/drivers/misc/qcom_geni.c#L204 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/qcom/qcom-geni-se.c#n1251 Thanks Varada[ . . . ]
