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

[Doug Anderson]

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?


> 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?


-Doug