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: <e47b3052-953a-1612-6c64-e2deaaed111b@baylibre.com>
Date:   Mon, 18 Jul 2022 10:06:29 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH 1/4] perf/amlogic: Add support for Amlogic meson G12 SoC
 DDR PMU driver
Content-Language: en-US
To:     Chris Healy <cphealy@gmail.com>,
        Robin Murphy <robin.murphy@arm.com>
Cc:     Jiucheng Xu <jiucheng.xu@amlogic.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux ARM <linux-arm-kernel@lists.infradead.org>,
        linux-amlogic@lists.infradead.org,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
References: <20220712063641.2790997-1-jiucheng.xu@amlogic.com>
 <55be073b-caf1-320b-dd42-165636b45a74@arm.com>
 <db31472c-0494-309a-0ac3-cab845b4e74f@amlogic.com>
 <a0c763c0-eaa0-999e-31bb-d2dbaeb988bb@arm.com>
 <CAFXsbZqQC7Vq3bO8GtqtY2hyLjh=+C1qa49pH+9BORmFk6P9XQ@mail.gmail.com>
From:   Neil Armstrong <narmstrong@baylibre.com>
Organization: Baylibre
In-Reply-To: <CAFXsbZqQC7Vq3bO8GtqtY2hyLjh=+C1qa49pH+9BORmFk6P9XQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Chris,

On 17/07/2022 22:58, Chris Healy wrote:
[...]

>>
>> [...]
>>>>> +        goto err2;
>>>>> +    }
>>>>> +
>>>>> +    irq_name = of_get_property(node, "interrupt-names", NULL);
>>>>> +    if (!irq_name)
>>>>> +        irq_name = "ddr_pmu";
>>>>
>>>> That's not how the "interrupt-names" property works. If you only have
>>>> a single interrupt then there's not much need for it to be named in
>>>> the DT at all. If you do want to use named interrupts then use
>>>> platform_get_irq_byname(), and the name should probably have a bnit
>>>> more functional meaning. Either way, please don't abuse the DT like this.
>>> Okay, actually there will be multiple interrupts , but not in current
>>> G12 series.
>>
>> That's fair enough, so we should try to anticipate it in the design of
>> the DT binding. If for instance future SoCs are going to move from
>> having a single combined overflow interrupt to a separate interrupt for
>> each counter, then the driver can reasonably continue to get them by
>> index and we'll effectively only need to update maxItems in the binding.
>> If on the other hand there's still going to be one combined overflow
>> interrupt, plus some other new interrupt for something completely
>> different, then it *could* be more appropriate to have names, and thus
>> to define and use a standard "overflow" name from the beginning even
>> when it is the only one present, so that we can remain consistent later
>> once more are added.
> 
> My assumption is that the goal of having this "interrupt-names" in DT
> is to cover future cases where there is more than one DRAM controller
> instance in the SoC and you want to be able to discriminate between
> the two instances with this driver's interrupt name.  If this is true,
> as an alternative, you could do something like this:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-mt65xx.c?h=v5.19-rc6&id=7fb9dc8109bf9713ffcda65617249099a1942f0f
> 
> This should result in each instance having a unique name that includes
> the base address as the prefix to the interrupt name which should be
> sufficient for determining which instance is which.

It's ok to introduce interrupt-names in the bindings for newer SoCs,
since it's useless for the current ones, there's no need to introduce it right now.

It's also why it's simpler to introduce a compatible per SoC, so we can add
different attributes in the bindings depending on the compatible.

Neil

[...]