Re: [PATCH 02/11] nvmem: qfprom: Mark core clk as optional

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 1.09.2023 17:08, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 1, 2023 at 7:54 AM Luca Weiss <luca.weiss@fairphone.com> 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?
> 
> 
>> 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?
Luca, if you feel like wasting some time, you can probably bruteforce
this.

I assume this keepout thing could be expanded in a generic way and
made into a dt property.

Other than that, I think it'd be fine to skip that for now, as it
sounds like it's functional so long as you don't intentionally access
forbidden regs.

Konrad