Message-ID: <CAD=FV=VcfYVQMmWvSkBG0VNBcqt5a3Y_b4eNBgDenPB5wUNGaw@mail.gmail.com>
Date:   Mon, 1 Jun 2020 11:08:44 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     "Ravi Kumar Bokka (Temp)" <rbokka@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        dhavalp@...eaurora.org, mturney@...eaurora.org,
        sparate@...eaurora.org, c_rbokka@...eaurora.org,
        mkurumel@...eaurora.org
Subject: Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

Hi,

On Mon, Jun 1, 2020 at 2:25 AM Srinivas Kandagatla
<srinivas.kandagatla@...aro.org> wrote:
>
>
>
> On 26/05/2020 23:31, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, May 22, 2020 at 4:18 AM Srinivas Kandagatla
> > <srinivas.kandagatla@...aro.org> wrote:
> >>
> >> On 21/05/2020 22:28, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Thu, May 21, 2020 at 8:56 AM Srinivas Kandagatla
> >>> <srinivas.kandagatla@...aro.org> wrote:
> >>>>
> >>>> On 21/05/2020 16:10, Doug Anderson wrote:
> >>>>>> On 20/05/2020 23:48, Doug Anderson wrote:
> >>>>>>>> Is this only applicable for corrected address space?
> >>>>>>> I guess I was proposing a two dts-node / two drive approach here.
> >>>>>>>
> >>>>>>> dts node #1:just covers the memory range for accessing the FEC-corrected data
> >>>>>>> driver #1: read-only and reads the FEC-corrected data
> >>>>>>>
> >>>>>>> dts node #2: covers the memory range that's_not_  the FEC-corrected
> >>>>>>> memory range.
> >>>>>>> driver #2: read-write.  reading reads uncorrected data
> >>>>>>>
> >>>>>>> Does that seem sane?
> >>>>>> I see your point but it does not make sense to have two node for same thing.
> >>>>> OK, so that sounds as if we want to go with the proposal where we
> >>>>> "deprecate the old driver and/or bindings and say that there really
> >>>>> should just be one node and one driver".
> >>>>>
> >>>>> Would this be acceptable to you?
> >>>>>
> >>>>> 1. Officially mark the old bindings as deprecated.
> >>>>
> >>>> Possibly Yes for some reasons below!
> >>>>
> >>>>>
> >>>>> 2. Leave the old driver there to support the old deprecated bindings,
> >>>>> at least until everyone can be transferred over.  There seem to be
> >>>>> quite a few existing users of "qcom,qfprom" and we're supposed to make
> >>>>> an attempt at keeping the old device trees working, at least for a
> >>>>> little while.  Once everyone is transferred over we could decide to
> >>>>> delete the old driver.
> >>>> we could consider "qcom,qfrom" to be only passing corrected address
> >>>> space. Till we transition users to new bindings!
> >>>>
> >>>>>
> >>>> Yes.
> >>>>
> >>>>> 3. We will have a totally new driver here.
> >>>> No, we should still be using the same driver. But the exiting driver
> >>>> seems to incorrect and is need of fixing.
> >>>>
> >>>> Having a look at the memory map for old SoCs like msm8996 and msm8916
> >>>> shows that memory map that was passed to qfprom driver is corrected
> >>>> address space. Writes will not obviously work!
> >>>>
> >>>> This should also be true with sdm845 or sc7180
> >>>>
> >>>> That needs to be fixed first!
> >>>
> >>> OK, so to summarize:
> >>>
> >>
> >>> 1. We will have one driver: "drivers/nvmem/qfprom.c"
> >>
> >> Yes, we should one driver for this because we are dealing with exactly
> >> same IP.
> >>
> >>>
> >>> 2. If the driver detects that its reg is pointing to the corrected
> >>> address space then it should operate in read-only mode.  Maybe it can
> >>> do this based on the compatible string being just "qcom,qfprom" or
> >>> maybe it can do this based on the size of the "reg".
> >>
> >> I found out that there is a version register at offset of 0x6000 which
> >> can give MAJOR, MINOR and STEP numbers.
> >>
> >> So we could still potentially continue using "qcom,qfprom"
> >
> > OK, sounds good.  I think it's still good practice to include both the
> > SoC specific and the generic.  Even if the driver never does anything
> > with the SoC-specific compatible string it doesn't hurt to have it
> > there.  Thus, we'd want:
> >
> > compatible = "qcom,msm8996-qfprom", "qcom,qfprom"
> >
> > The driver itself would never need to refer to the SoC-specific name
> > but that does give us more flexibility.
> >
> >
> >> The address space can be split into 3 resources, which is inline with
> >> Specs as well
> >>
> >> 1. Raw address space ("raw")
> >> 2. Configuration address space ("conf" or "core")
> >> 3. Corrected address space ("corrected")
> >
> > Sure, this is OK with me then.  Originally Ravi had 3 ranges but then
> > he was (in the driver) treating it as one range.  As long as the
> > driver truly treats it as 3 ranges I have no problem doing it like
> > this.
> >
> > In general, over the years, there has been a push to keep
> > implementation details out of the dts and rely more on the "of_match"
> > table to add SoC-specific details.  This becomes really important when
> > 1 year down the road you realize that you need one more random
> > property or address range to fix some bug and then you need to figure
> > out how to try to keep old "dts" files compatible.  It's not a
> > hard-and-fast rule, though.
>
> Am not 100% sure if "qcom,fuse-blow-frequency" is something integration
> specific or SoC Specific, My idea was that this will give more
> flexibility in future. As adding new SoC Support does not need driver
> changes.
>
> Having said that, Am okay either way!

Yeah, it's always a balance.  I guess the question is: why do we think
driver changes are worse than dts changes?  The value still needs to
be somewhere and having it in the driver isn't a terrible place.


> Incase we go compatible way, I would like to see compatible strings
> having proper IP versions to have ip version rather than SoC names.
>
> Having SoC names in compatible string means both driver and bindings
> need update for every new SoC which can be overhead very soon!

Almost certainly the compatible strings should have SoC names in them.
Yes it means a binding update every time a new SoC comes up but that
is just how device tree works.  Presumably there's enough chatter on
this that Rob H has totally tuned it out at this point in time, but
there are many other instances of this.

NOTE: just because we have the SoC name in the compatible string
_doesn't_ mean that the driver has to change.  You already said that
the IP version can be detected earlier in this thread, right?  You
said:

I found out that there is a version register at offset of 0x6000 which
can give MAJOR, MINOR and STEP numbers.

So how about this:

a) Compatible contains "SoC" version and the generic "qcom,qfrom", so:

compatible = "qcom,sdm845-qfprom", "qcom,qfrom"

b) Bindings will need to be updated for every new SoC, but that's
normal and should be a trivial patch to just add a new SoC to the
list.

c) If the driver can be made to make its decisions about frequencies /
timings completely by MAJOR/MINOR/STEP numbers then it can use those
in its decision and it will never need to use the SoC-specific
compatible string.  The SoC-specific compatible string will only be
present as a fallback "oops we have to workaround a bug that we didn't
know about".


> Rob can help review once we have v2 bindings out!

Sounds good.  If you're still not convinced by my arguments we can see
if we can get Rob to clarify once we have a v2.  :-)


> >> Exiting qfprom entries or read-only qfprom  will have "corrected"
> >> address space which can be the only resource provided by device tree
> >> entries.
> >> Other two entries("raw" and "conf") are optional.
> >>
> >> qfprom: qfprom@...000 {
> >>           compatible = "qcom,qfprom";
> >>          reg = <0 0x00780000 0 0x8ff>,
> >>                  <0 0x00782000 0 0x100>,
> >>                  <0 0x00784000 0 0x8ff>;
> >>          reg-names = "raw", "conf", "corrected";
> >>
> >>          vcc-supply = <&vreg_xyz>;
> >>
> >>          clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> >>          clock-names = "secclk";
> >>
> >>          assigned-clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> >>           assigned-clock-rates = <19200000>;
> >>
> >>          qcom,fuse-blow-frequency = <4800000>
> >>
> >>           #address-cells = <1>;
> >>           #size-cells = <1>;
> >>
> >>          qusb2p_hstx_trim: hstx-trim-primary@25b {
> >>                  reg = <0x25b 0x1>;
> >>                  bits = <1 3>;
> >>          };
> >> };
> >>
> >> Regarding clk rate setting, the default rate can be set using
> >> assigned-clock-rates property, however the blow frequency can go as new
> >> binding.
> >> regarding voltage range for regulator, it should come as part of board
> >> specific voltage regulator node. In worst case we can discuss on adding
> >> new bindings for allowing specific range.
> >
> > I'd up to you (and Rob H, who probably will wait for the next rev of
> > the binding before chiming in) but the "qcom,fuse-blow-frequency" is
> > the type of property that feels better in the driver and achieved from
> > the of_match table.  Then you don't need to worry about adding it to
> > the bindings.  Voltage (if needed) would be similar, but I would hope
> > we don't need it.
> >
> >
> >> for Older SoCs: we still continue to use old style with just one
> >> resource corresponding to corrected by default.
> >>
> >> qfprom: qfprom@...000 {
> >>           compatible = "qcom,qfprom";
> >>           reg = <0 0x00784000 0 0x8ff>;
> >>           #address-cells = <1>;
> >>           #size-cells = <1>;
> >>
> >>           qusb2p_hstx_trim: hstx-trim-primary@1eb {
> >>                   reg = <0x1eb 0x1>;
> >>                   bits = <1 4>;
> >>           };
> >>
> >>           qusb2s_hstx_trim: hstx-trim-secondary@1eb {
> >>                   reg = <0x1eb 0x2>;
> >>                   bits = <6 4>;
> >>           };
> >> };
> >>
> >>
> >> I see the patch as adding write support to qfprom, rather than adding
> >> new driver or new SoC support.
> >>
> >> This in summary should give us good direction for this patch!
> >>
> >> Correct me if I miss understood something here!
> >
> > Sounds sane to me.
>
> Cool! lets see how v2 will endup like!
>
> --srini
> >
> >
> > -Doug
> >

Powered by blists - more mailing lists