Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ba04bb7b-6599-6f41-09a8-834ee280830d@quicinc.com>
Date:   Mon, 3 Jul 2023 21:25:21 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v4 08/21] dt-bindings: reserved-memory: Add qcom,ramoops
 binding
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
CC:     Rob Herring <robh+dt@kernel.org>, <corbet@lwn.net>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <conor+dt@kernel.org>,
        <keescook@chromium.org>, <tony.luck@intel.com>,
        <gpiccoli@igalia.com>, <mathieu.poirier@linaro.org>,
        <catalin.marinas@arm.com>, <will@kernel.org>,
        <linus.walleij@linaro.org>, <andy.shevchenko@gmail.com>,
        <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-hardening@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-gpio@vger.kernel.org>
References: <1687955688-20809-1-git-send-email-quic_mojha@quicinc.com>
 <1687955688-20809-9-git-send-email-quic_mojha@quicinc.com>
 <CAL_JsqJ_TTnGjjB2d8_FKHpWBRG5GHLoWnabCKjsdeZ4QFdNEg@mail.gmail.com>
 <cacbbb02-732e-076e-50bf-292d20a4d722@quicinc.com>
 <58a26b9e-a48d-d567-c310-193a2c52521e@linaro.org>
 <5447f9f8-55b4-8bed-66a6-1c9d62b02c79@quicinc.com>
 <CAGE=qrq0CuO4J-6yC=YZ4xjL67o9QTqpei0ovX-X_8MLVeEH6g@mail.gmail.com>
From:   Mukesh Ojha <quic_mojha@quicinc.com>
In-Reply-To: <CAGE=qrq0CuO4J-6yC=YZ4xjL67o9QTqpei0ovX-X_8MLVeEH6g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/3/2023 12:50 PM, Krzysztof Kozlowski wrote:
> On Mon, 3 Jul 2023 at 08:22, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>> On 7/2/2023 1:42 PM, Krzysztof Kozlowski wrote:
>>>>> The big difference is if firmware is not deciding where this log
>>>>> lives, then it doesn't need to be in DT. How does anything except the
>>>>> kernel that allocates the log find the logs?
>>>>
>>>> Yes, you are correct, firmware is not deciding where the logs lives
>>>> instead here, Kernel has reserved the region where the ramoops region
>>>> lives and later with the minidump registration where, physical
>>>> address/size/virtual address(for parsing) are passed and that is how
>>>> firmware is able to know and dump those region before triggering system
>>>> reset.
>>>
>>> Your explanation does not justify storing all this in DT. Kernel can
>>> allocate any memory it wishes, store there logs and pass the address to
>>> the firmware. That's it, no need for DT.
>>
>> If you go through the driver, you will know that what it does, is
> 
> We talk about bindings and I should not be forced to look at the
> driver to be able to understand them. Bindings should stand on their
> own.

Why can't ramoops binding have one more feature where it can add a flag 
*dynamic* to indicate the regions are dynamic and it is for platforms
where there is another entity 'minidump' who is interested in these
regions.

> 
>> just create platform device for actual ramoops driver to probe and to
> 
> Not really justification for Devicetree anyway. Whatever your driver
> is doing, is driver's business, not bindings.
> 
>> provide this it needs exact set of parameters of input what original
>> ramoops DT provides, we need to keep it in DT as maintaining this in
>> driver will not scale well with different size/parameter size
>> requirement for different targets.
> 
> Really? Why? I don't see a problem in scaling. At all.

I had attempted it here,

https://lore.kernel.org/lkml/1683133352-10046-10-git-send-email-quic_mojha@quicinc.com/

but got comments related to hard coding and some in favor of having
the same set of properties what ramoops has/provides

https://lore.kernel.org/lkml/e25723bf-be85-b458-a84c-1a45392683bb@gmail.com/

https://lore.kernel.org/lkml/202305161347.80204C1A0E@keescook/
> 
>>
>>>
>>>>
>>>> A part of this registration code you can find in 11/21
>>>>
>>>>> I'm pretty sure I already said all this before.
>>>>
>>>> Yes, you said this before but that's the reason i came up with vendor
>>>> ramoops instead of changing traditional ramoops binding.
>>>
>>> That's unexpected conclusion. Adding more bindings is not the answer to
>>> comment that it should not be in the DTS in the first place.
>>
>> Please suggest, what is the other way being above text as requirement..
> 
> I do not see any requirement for us there. Forcing me to figure out
> how to add non-hardware property to DT is not the way to convince
> reviewers. But if you insist - we have ABI for this, called sysfs. If
> it is debugging feature, then debugfs.

ramoops already support module params and a way to pass these parameters
from bootargs but it also need to know the hard-codes addresses, so, 
doing something in sysfs will be again duplication with ramoops driver..

If this can be accommodated under ramoops, this will be very small 
change, like this

https://lore.kernel.org/lkml/20230622005213.458236-1-isaacmanjarres@google.com/

-- Mukesh
> 
> Best regards,
> Krzysztof