Message-ID: <83c3e403-7e4c-a29a-95de-f30d74863769@gmail.com>
Date:   Wed, 8 Mar 2023 16:42:55 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:96.0) Gecko/20100101
 Thunderbird/96.0
Subject: Re: [PATCH V4 2/2] nvmem: add generic driver for devices with MMIO
 access
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        Kunihiko Hayashi <hayashi.kunihiko@socionext.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        linux-mediatek@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
References: <20230228072936.1108-1-zajec5@gmail.com>
 <20230228072936.1108-3-zajec5@gmail.com>
 <7853ff04-02cf-9430-d84a-c8fe8b1d6725@linaro.org>
From:   =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
In-Reply-To: <7853ff04-02cf-9430-d84a-c8fe8b1d6725@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8.03.2023 14:31, Srinivas Kandagatla wrote:
> Thanks for doing this,

Thank you for reviewing. Sadly it seems it still isn't clear if we can
have this generic driver.

I guess I missed some important questions or comments. In previous
series we were discussing implementation details so I thought it's OK to
have this driver after all. Not sure if I didn't waste time working on
V4. I'll see if I can I address your concerns (see below).


> On 28/02/2023 07:29, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Some NVMEM devices can be accessed by simply mapping memory and reading
>> from / writing to it. This driver adds support for a generic
>> "mmio-nvmem" DT binding used by such devices.
>>
>> One of such devices is Broadcom's NVRAM. It's already supported (see
>> NVMEM_BRCM_NVRAM) but existing driver covers both:
> 
> What will happen to the old "brcm,nvram" compatible and the dt firmware that already have this node?

I treat backward compatibility with previouly used bindings very
seriously. I'm going to keep it. I may make an attempt to drop it in
few years if it's very unlikely to break any setups.


> If there is only one user for this then one would object that why do we need this DT level of abstraction to start with?
> If this is not the case please consider adding those patches to this series.

Existing Linux drivers prove that there is more hardware with MMIO based
read access: brcm_nvram, mtk-efuse, uniphier-efuse. Migration of other
drivers (mtk, unipher) is on hold as apparently there may be support for
writing support soon. In any case this MMIO solution isn't completely
unique to Broadcom.
I don't have other patches to add to it right now.


>> 1. NVMEM device access
>> 2. NVMEM content parsing
>>
>> Once we get support for NVMEM layouts then existing NVRAM driver will
>> get converted into a layout and generic driver will take over
>> responsibility for data access.
>>
> 
> Even though this series is simple, but it is really confusing for two reasons.
> 
> 1> Generic mmio nvmem bindings are incomplete and potentially change/evolve on every new user. Ex clks, regulators, endianess ... So it looks really fragile and incomplete to me as a generic bindings.
> Is this want you are expecting?

All 3 existing hardware support MMIO reads without extra clocks or
regulators. I'm not sure if endianess belongs to this layer. Isn't that
NVMEM content thing?

I'm not claiming this driver is in its final and perfect state. For
simple hardware that needs minor fixups we can add those later to this
generic driver. Adding clocks should be possible, fine and easy.

I'm sure there will be more complex hardware that we will not be able
to support with this driver. It's require another driver and I'm fine
with that.


> 2> As you mentioned that this will replace broadcom NVMRAM, but this patch does nothing in relation to updating that driver, so the code is dead as it is. If you are considering to use it for Broadcom NVMRAM, please add those patches to this series so that we could see the real user for this code.

Of course it does nothing because there are no layouts yet. I could
migrate brcm_nvram into layout once there is layouts support.

I don't agree this code is dead. It support new binding. It works.
Every new binding and its driver are "dead" until you add first DT
users.

Here is real use:

nvmem@1eff0000 {
	compatible = "mmio-nvmem";
	reg = <0x1eff0000 0x10000>;
};