On Fri Sep 1, 2023 at 5:08 PM CEST, Doug Anderson wrote: > Hi, > > On Fri, Sep 1, 2023 at 7:54 AM Luca Weiss <luca.weiss@xxxxxxxxxxxxx> wrote: > > > > > > > So maybe the right fix here is to just change your dts to specify one > > > > > memory region? > > > > > > > > I got feedback from Konrad that this here would be the preferred > > > > approach compared to having a different dts for ChromeOS vs non-ChromeOS > > > > devices. I don't feel strongly to either, for me it's also okay to > > > > remove the extra memory regions and only have the main one used on > > > > regular qcom devices. > > > > > > > > Let me know what you think. > > > > > > I don't hate the idea of leaving the extra memory regions in the dts. > > > They do describe the hardware, after all, even if the main OS can't > > > actually access those memory regions. ...though the same could also be > > > said about the clock you've removed. Said another way: if you want to > > > fully describe the hardware then the dts should have the extra memory > > > regions and the clock. If you are OK w/ just describing the hardware > > > in the way that the OS has access to then the dts should not have the > > > extra memory regions and not have the clock. Does that sound right? > > > > Not sure which of those memory regions are actually accessible on this > > board, but honestly I don't even want to try accessing it. Blowing fuses > > is not my wish there ;) > > > > On downstream the node is just described like the following: > > > > qfprom: qfprom@780000 { > > compatible = "qcom,qfprom"; > > reg = <0x780000 0x7000>; > > ... > > }; > > > > So we have 0x780000 - 0x786fff here. > > > > In sc7280.dtsi we have the following: > > > > qfprom: efuse@784000 { > > compatible = "qcom,sc7280-qfprom", "qcom,qfprom"; > > reg = <0 0x00784000 0 0xa20>, > > <0 0x00780000 0 0xa20>, > > <0 0x00782000 0 0x120>, > > <0 0x00786000 0 0x1fff>; > > ... > > }; > > > > So I guess this: > > * 0x780000 - 0x780a1f > > * 0x782000 - 0x78211f > > * 0x784000 - 0x784a1f > > * 0x786000 - 0x787ffe > > > > So at least the last memory region seems to be partially out of range > > according to downstream. > > From the other discussion, it sounds as if you _can_ leave the clock > in the device tree and then use "clk_get_optional" here. IMO then, the > right answer is to use "clk_get_optional" but then also modify the > check below so that instead of: > > /* Only enable writing if we have SoC data. */ > if (priv->soc_data) > econfig.reg_write = qfprom_reg_write; > > It is: > > /* Only enable writing if we have SoC data and a valid clock */ > if (priv->soc_data && priv->secclk) > econfig.reg_write = qfprom_reg_write; > > > Does that work for you? Thanks Doug, this feels like a good solution. I'll update v2 with that. > > > > So after reading all of this I tried running this commmand on the phone > > and the phone reboots into 900e mode. > > > > $ cat /sys/devices/platform/soc@0/784000.efuse/qfprom0/nvmem > > > > I guess normally this should work? So if I interpret this correctly, the > > Linux driver thinks it can access more than it can/should. But also > > should probably try this command on another chipset to see if it works > > on any really? > > Presumably your firmware needs a different "sc7280_qfprom_keepout". If > that's true then I guess you'll have to undergo negotiations with the > DT bindings folks and the nvmem maintainer to figure out how to > specify that your firmware protects different things than the ChromeOS > firmware? Right. But based on Konrad's reply here, I think I'll skip this for now since generally qfprom seems to be fine with the current data, at least for the purposes it's used for on my device. Regards Luca > > > -Doug