Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <41489c26-75ae-4810-9668-47ab170836ea@ti.com>
Date:   Fri, 3 Nov 2023 10:36:54 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC] arm64: dts: ti: k3-j721e: Use nvmem to model the
 efuses
To:     Vignesh Raghavendra <vigneshr@ti.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>, Nishanth Menon <nm@ti.com>,
        Tero Kristo <kristo@kernel.org>,
        "Vadapalli, Siddharth" <s-vadapalli@ti.com>,
        "Vardhan, Vibhore" <vibhore@ti.com>
CC:     <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>
References: <20231025154339.1270246-1-afd@ti.com>
 <d4d79dea-2c7f-4160-9008-23f5bee1550a@ti.com>
Content-Language: en-US
From:   Andrew Davis <afd@ti.com>
In-Reply-To: <d4d79dea-2c7f-4160-9008-23f5bee1550a@ti.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/29/23 11:25 PM, Vignesh Raghavendra wrote:
> + Siddharth for CSPW driver, + Vibhore of CPUFreq
> 
> On 25/10/23 21:13, Andrew Davis wrote:
>> The nvmem subsystem is used to model efuse areas. Currently our efuse
>> areas are described using raw syscon nodes and offsets into that. The
>> use case for this efuse area is to store the SoC Ethernet MAC address.
>> This is already such a commonly supported use case that both the nvmem
>> and networking subsystem have built-in support for it. Use that here
>> to get the following:
>>
>> 1. More standard, complete, and accurate description of this hardware
>>     efuse area.
>>
>> 2. Remove the need for this custom 'ti,syscon-efuse' property (which
>>     I'm surprised was accepted into the binding in the first place..)
>>
>> 3. Allow the parent node to not have to be a syscon which fixes a DT
>>     check warning.
>>
> 
> Sounds like a good plan to me.
> 
>> RFC for now as we don't have a simple generic compatible for memory
>> mapped nvmem areas. For now we use the 'uniphier-efuse' compatible
>> as it is essentially just a generic mmio nvmem area, which means any
>> generic area is technically "compatible".
>>
> 
> Looking at drivers/nvmem/ its seems like each SoC arch seems to have
> their own individual efuse driver?
> 

Yes, but I'm not sure I see the need for a whole new driver/compatible
for every simple MMIO-based efuse ranges. Would be nice to have a
generic one.

> For K3 SoCs (at least AM62), there are other properties in addition to
> MAC address in the eFUSE which are useful for different drivers such as
> Speed grades. Can this be extended for the same?
> 

Yes, with the "nvmem-layout" being worked on right now by the nvmem
folks I see a clear path to extend this over the rest of our efuse
area and replace the remaining "syscon" hacks we have been using.

Andrew

>> If this all is acceptable I'll split this patch into three and do
>> the same for all the other K3 devices still using this odd
>> 'ti,syscon-efuse' pattern for v1.
>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   .../boot/dts/ti/k3-j721e-mcu-wakeup.dtsi      | 24 ++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
>> index 05d6ef127ba78..8f488d43d49df 100644
>> --- a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi
>> @@ -34,13 +34,30 @@ k3_reset: reset-controller {
>>   		};
>>   	};
>>   
>> -	mcu_conf: syscon@40f00000 {
>> -		compatible = "syscon", "simple-mfd";
>> +	mcu_conf: bus@40f00000 {
>> +		compatible = "simple-bus";
>>   		reg = <0x0 0x40f00000 0x0 0x20000>;
>>   		#address-cells = <1>;
>>   		#size-cells = <1>;
>>   		ranges = <0x0 0x0 0x40f00000 0x20000>;
>>   
>> +		efuse@200 {
>> +			compatible = "socionext,uniphier-efuse";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			reg = <0x200 0x40>;
>> +
>> +			nvmem-layout {
>> +				compatible = "fixed-layout";
>> +				#address-cells = <1>;
>> +				#size-cells = <1>;
>> +
>> +				mac_address: mac@0 {
>> +					reg = <0x0 0x6>;
>> +				};
>> +			};
>> +		};
>> +
>>   		phy_gmii_sel: phy@4040 {
>>   			compatible = "ti,am654-phy-gmii-sel";
>>   			reg = <0x4040 0x4>;
>> @@ -533,7 +550,8 @@ cpsw_port1: port@1 {
>>   				reg = <1>;
>>   				ti,mac-only;
>>   				label = "port1";
>> -				ti,syscon-efuse = <&mcu_conf 0x200>;
>> +				nvmem-cells = <&mac_address>;
>> +				nvmem-cell-names = "mac-address";
>>   				phys = <&phy_gmii_sel 1>;
>>   			};
>>   		};
>