lkml.org 
[lkml]   [2023]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v4 08/21] dt-bindings: reserved-memory: Add qcom,ramoops binding
    From
    On 03/07/2023 17:55, Mukesh Ojha wrote:
    >
    >
    > 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.

    Because we do not define dynamic stuff in Devicetree. Dynamic means
    defined by SW or runtime configurable. It is against the entire idea of
    Devicetree which is for non-discoverable hardware.

    >
    >>
    >>> 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/

    Then you were tricked. I don't get why someone else suggests that
    non-hardware property should be part of Devicetree, but anyway it's the
    call of Devicetree binding maintainers, not someone else. DT is not
    dumping ground for all the system configuration variables.


    >>
    >>>
    >>>>
    >>>>>
    >>>>> 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..

    Why do you need hard-coded addresses?

    >
    > 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/

    That's also funny patch - missing bindings updated, missing CC DT
    maintainers.

    Best regards,
    Krzysztof

    \
     
     \ /
      Last update: 2023-07-04 07:57    [W:4.122 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site