Messages in this thread | | | Date | Mon, 3 Jul 2023 21:25:21 +0530 | Subject | Re: [PATCH v4 08/21] dt-bindings: reserved-memory: Add qcom,ramoops binding | From | Mukesh Ojha <> |
| |
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
| |