14:02 <bamse> marc|gonzalez: you will hit the error on line 2205 in arm-smmu.c if you just reduce the number of interrupts...

14:03 <bamse> marc|gonzalez: but qcom_smmu_cfg_probe() is called inbetween smmu->num_context_banks is read from hardware and that check...so perhaps you could try to just adjust that

14:03 <bamse> marc|gonzalez: i.e. remove the last interrupts, and in qcom_smmu_cfg_probe() do if (smmu->num_context_banks == 12) smmu->num_context_banks = 11;

14:03 <bamse> (just for testing purposes)

14:25 <marc|gonzalez> bamse: is reducing the number of interrupts supposed to change smmu->num_context_banks ?

14:25 <marc|gonzalez> It doesn't in my test, but maybe I didn't understand what you wanted me to do...

14:26 <marc|gonzalez> smmu->num_context_banks = FIELD_GET(ARM_SMMU_ID1_NUMCB, id);

14:27 <marc|gonzalez> id = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_ID1);

14:27 <marc|gonzalez> The number of context bank is determined at run-time by querying the HW, as far as I can tell

14:28 <marc|gonzalez> bamse: downtream seems to check if the FW has marked some context banks as reserved

14:28 <marc|gonzalez> Can't we do something similar in mainline?

14:34 <bamse> marc|gonzalez: how do they check that?

14:35 <bamse> marc|gonzalez: no, i thought just reducing the number of interrupts would affect num_context_banks, but what i'm suggesting is that you do both - just for testing

14:37 <marc|gonzalez> bamse: apparently, vendor uses a function called arm_smmu_init_static_cbndx_list()

14:38 <marc|gonzalez> Reducing the number of interrupts does NOT affect smmu->num_context_banks

14:38 <bamse> marc|gonzalez: i know, that's what i'm telling you...

14:39 <marc|gonzalez> I'm sorry, then I didn't understand what you said

14:39 <marc|gonzalez> of course forcing the number down by hand will work

14:40 <bamse> so, setting num_context_banks = 11 removes all your problems?

14:41 <marc|gonzalez> Right now, I have just one (major) problem that accessing context bank 12 hoses the system. Any hack that results in not touching context bank 12 will make the system not be hosed...

14:42 <marc|gonzalez> But my intuition is that there's nothing special about CB12 other than some dude at qcom thought "hey, we should reserve the last context bank, it has less chance of clashing with the OS"

14:42 <z3ntu> bamse: quick question regarding qrtr nameserver, there's service number which identifies a let's say type of service (e.g. 4097 = DIAG service), there's version which I guess could be used for having multiple versions of the same service running with different function definitions. And there's 'instance' which is some semi-random number, what could be a decent description for that value? Also from what I can tell, the combination of

14:42 <z3ntu> service+version+instance is unique, and from that a client gets the port number for the service they want to talk to?

14:43 <marc|gonzalez> Except that the Linux dudes thought the same thing "hey, let's use the last CB to keep the FW happy"

14:43 <marc|gonzalez> and thus FW & OS are trying to use the last CB at the same time

14:43 <marc|gonzalez> and bad things happen(TM)(C)

14:46 <bamse> marc|gonzalez: it seems just "correcting" num_context_banks would be the least intrusive change...

14:46 <bamse> marc|gonzalez: said "linux dudes" is me...so, you're welcome :)

14:47 <bamse> marc|gonzalez: i needed to pick a context bank that wasn't in use by the system, i could have searched for the first free one, but this was cleaner...and it doesn't seem to matter for your case anyways

14:49 <marc|gonzalez> Proposed fix would be to use qsmmu->bypass_cbndx = smmu->num_context_banks - 2; instead of qsmmu->bypass_cbndx = smmu->num_context_banks - 1; ?

14:49 <bamse> z3ntu: the "version" part doesn't seem to be in use universally, so i'd say service+instance is what uniquely identifies a service...but it should be mentioned that instance seems to be able to differ between platforms

14:49 <bamse> marc|gonzalez: does that fix your problem?

14:49 <marc|gonzalez> With a comment saying that some FW reserve the last CB?

14:50 <marc|gonzalez> Well Angelo's work around was: "Use CB 11 instead of CB 12" so I would assume it fixes the issue, yes.

14:51 <marc|gonzalez> That will work as long as the FW doesn't decide to reserve CB 11 though

14:51 <marc|gonzalez> And it gets dirty when smmu->num_context_banks is 1 or 2

14:52 <bamse> but can you confirm if that it actually works?

14:52 <aka_[m]> qcom hypervisor probably reserves it for usage dedicated to trustzone trustlets

14:54 <aka_[m]> bamse: isn't goal of qcom-iommu pretty much not touching stuff we don't need?

14:55 <z3ntu> bamse: Ok, thanks! Hopefully I've got the description somewhat correct now :) will use this at a talk this weekend at FrOSCon about eSIM on Qualcomm hw

14:56 <marc|gonzalez> bamse: in his patches, angelo said he managed to get audio on the audio jack. We don't have an audio jack, but my plan was to get audio working on the HDMI link. But I wanted to submit a fix for the SMMU crash before jumping in the rabbit hole.

14:58 <marc|gonzalez> Basically, right now, if we enable lpass_q6_smmu, the driver code hoses the system

15:01 <marc|gonzalez> well, not really the driver's fault, but an assumption does not hold on all systems

15:02 <marc|gonzalez> bamse: there is another related issue, of course, in arm_smmu_device_reset()

15:02 <marc|gonzalez> System blows up if we try to reset CB12

15:04 <bamse> aka_[m]: that's the approach we take downstream, upstream we still let the arm-smmu driver initialize the hardware to some known state - to the best of our ability

15:05 <bamse> z3ntu: cool, i'm interested in hearing that talk

15:05 <bamse> marc|gonzalez: so, just setting last_s2cr = num_context_banks - 2; isn't fixing anything? but smmu->num_context_banks = 11; does?

15:10 <marc|gonzalez> Maybe it's better if I send a patch to show exactly what I change?

15:11 <marc|gonzalez> bamse: if you say it's OK to "hide" the last context bank in the driver code, then I think it will work to just do --smmu->num_context_banks;

15:12 <marc|gonzalez> Problem is: we don't want to do that for all SMMUs, just for the audio DSP SMMU.

15:12 <bamse> marc|gonzalez: we probably want to make that conditional on the specific smmu, but yes

15:12 <marc|gonzalez> With a DT prop?

15:13 <marc|gonzalez> last_context_bank_reserved_by_firmware ?

15:13 <marc|gonzalez> if (last_context_bank_reserved_by_firmware) --smmu->num_context_banks;

15:16 <marc|gonzalez> bamse: I will test unconditionally and we can discuss how to express the condition in parallel

15:18 <bamse> marc|gonzalez: sounds good (the testing and concluding that it's all we need)

15:21 <marc|gonzalez> bamse: OK but just to be explicit about the assumptions... The proposed change only helps when the last context bank is reserved by the FW. If the FW reserves any other CB, system blows up. We agree?

15:23 <marc|gonzalez> You think it is unnecessary to grok arm_smmu_init_static_cbndx_list() in the vendor code?

15:23 <bamse> marc|gonzalez: agreed

15:25 <bamse> marc|gonzalez: perhaps not unnecessary, but you have to decide if it's worth your time

15:26 <bamse> marc|gonzalez: that said, we do have things like protected-clocks and reserved-gpio-regions in dt already, so if there is an actual need we could argue in favor of something similar here

15:27 <marc|gonzalez> I think we do need something like that in DT. How else do we say "ADSP SMMU has last CB reserved for FW" ?

15:28 <bamse> marc|gonzalez: the code has been there for 4 years now, so how widespread is this problem?

15:28 <marc|gonzalez> I love it when you guys ask me that

15:29 <marc|gonzalez> It is widespread to my system :)

15:29 <marc|gonzalez> I dunno why MSM8996 seems unaffected, I thought 8996 & 8998 were "very close"

15:30 <marc|gonzalez> But since it's actually a FW diff, maybe "8996 vs 8998" is irrelevant

17:06 <marc|gonzalez> bamse: this does the trick: https://paste.debian.net/1326278/

17:06 <z3ntu> bamse: https://programm.froscon.org/2024/events/3079.html , I expect the live stream to be at https://streaming.media.ccc.de/ but the recording will also be available at some point if everything goes well

17:08 <bamse> marc|gonzalez: nice!

17:09 <marc|gonzalez> bamse: can I make a formal submission? Are you the qcom smmu maintainer?

17:10 <marc|gonzalez> Or do I need to run this by robclark perhaps?

17:10 <marc|gonzalez> Bah, I'll just send a formal submission to kick start the discussion...

17:10 <bamse> marc|gonzalez: you don't need to worry about who's the maintainer, just make a proper commit and ask us to review it...

17:11 <marc|gonzalez> Will do, tomorrow. Cheers!

17:11 <bamse> marc|gonzalez: but i don't think you should say "sometimes the last context bank is broken, remove it"...i think you should be more specific and say "this hardware reports 12 context banks, but only 11 are usable"

17:12 <marc|gonzalez> I'll make a lengthy commit message explaining WTF is going on ;)

17:14 <bamse> you mean "hardware reports 12 context banks, but only 11 are usable"?

17:45 <aka_[m]> *by EL1/EL0 user

17:47 <aka_[m]> Question to developers so to almost all ppl here.

17:47 <aka_[m]> I want to add support from some awinic crap amplifier and i see i could hack it inside other driver, from what i see register definitions are plain inside .h

17:47 <aka_[m]> Is in linux any specific struct for handling it?

17:47 <aka_[m]> i mean for making array

17:53 <aka_[m]> or just define struct with int index,int reg and build array per chip

17:54 <konradybcio> does the rest of the logic match for these two chis?

17:54 <konradybcio> chips

17:56 <konradybcio> you can check e.g. the qmp phy drivers for an example of handling regs

19:30 <aka_[m]> konradybcio: very similar stuff, going to verify also bitfields.

19:30 <aka_[m]> Tho i would need to probably add second struct with features so if some feature don't exist i can guard it to not get some random big value from un-initialized index

19:37 <aka_[m]> seems registers exist but are stuff is RD only

19:55 <aka_[m]> seems its painful and fields are shifted what a shame