Message-ID: <796a528b23aded95c1a647317c277b1f@walle.cc>
Date:   Fri, 16 Dec 2022 10:03:19 +0100
From:   Michael Walle <michael@...le.cc>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Rob Herring <robh@...nel.org>, Xu Liang <lxu@...linear.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v1 3/4] dt-bindings: net: phy: add MaxLinear
 GPY2xx bindings

Am 2022-12-06 10:44, schrieb Michael Walle:
> Am 2022-12-06 09:38, schrieb Krzysztof Kozlowski:
> 
>>>>> Just omit the interrupt property if you don't want interrupts and
>>>>> add it if you do.
>>>> 
>>>> How does that work together with "the device tree describes
>>>> the hardware and not the configuration". The interrupt line
>>>> is there, its just broken sometimes and thus it's disabled
>>>> by default for these PHY revisions/firmwares. With this
>>>> flag the user can say, "hey on this hardware it is not
>>>> relevant because we don't have shared interrupts or because
>>>> I know what I'm doing".
>> 
>> Yeah, that's a good question. In your case broken interrupts could be
>> understood the same as "not connected", so property not present. When
>> things are broken, you do not describe them fully in DTS for the
>> completeness of hardware description, right?
> 
> I'd agree here, but in this case it's different. First, it isn't
> obvious in the first place that things are broken and boards in
> the field wouldn't/couldn't get that update. I'd really expect
> an erratum from MaxLinear here. And secondly, (which I
> just noticed right now, sorry), is that the interrupt line
> is also used for wake-on-lan, which can also be used even for
> the "broken" PHYs.
> 
> To work around this, the basic idea was to just disable the
> normal interrupts and fall back to polling mode, as the PHY
> driver just use it for link detection and don't offer any
> advanced features like PTP (for now). But still get the system
> integrator a knob to opt-in to the old behavior on new device
> trees.
> 
>>> Specifically you can't do the following: Have the same device
>>> tree and still being able to use it with a future PHY firmware
>>> update/revision. Because according to your suggestion, this
>>> won't have the interrupt property set. With this flag you can
>>> have the following cases:
>>>   (1) the interrupt information is there and can be used in the
>>>       future by non-broken PHY revisions,
>>>   (2) broken PHYs will ignore the interrupt line
>>>   (3) except the system designer opts-in with this flag (because
>>>       maybe this is the only PHY on the interrupt line etc).
>> 
>> I am not sure if I understand the case. You want to have a DTS with
>> interrupts and "maxlinear,use-broken-interrupts", where the latter 
>> will
>> be ignored by some future firmware?
> 
> Yes, that's correct.
> 
>> Isn't then the property not really correct? Broken for one firmware
>> on the same device, working for other firmware on the same device?
> 
> Arguable, but you can interpret "use broken-interrupts" as no-op
> if there are no broken interrupts.
> 
>> I would assume that in such cases you (or bootloader or overlay)
>> should patch the DTS...
> 
> I think this would turn the opt-in into an opt-out and we'd rely
> on the bootloader to workaround the erratum. Which isn't what we
> want here.

Just a recap what happened on IRC:
  (1) Krzysztof signalled that such a property might be ok but the
      commit message should be explain it better. For reference
      here is what I explained there:

       maybe that property has a wrong name, but ultimately, it's just
       a hint that the systems designer wants to use the interrupts
       even if they don't work as expected, because they work on that
       particular hardware.
       the interrupt line is there but it's broken, there are device
       trees out there with that property, so all we can do is to not
       use the interrupts for that PHY. but as a systems designer who
       is aware of the consequences and knowing that they don't apply
       to my board, how could i then tell the driver to use it anyway.

  (2) Krzysztof pointed out that there is still the issue raised by
      Rob, that the schemas haven't any compatible and cannot be
      validated. I think that applies to all the network PHY bindings
      in the tree right now. I don't know how to fix them.

  (3) The main problem with the broken interrupt handling of the PHY
      is that it will disturb other devices on that interrupt line.
      IOW if the interrupt line is shared the PHY should fall back
      to polling mode. I haven't found anything in the interrupt
      subsys to query if a line is shared and I guess it's also
      conceptually impossible to do such a thing, because there
      might be any driver probed at a later time which also uses
      that line.
      Rob had the idea to walk the device tree and determine if
      a particular interrupt is used by other devices, too. If
      feasable, this sounds like a good enough heuristic for our
      problem. Although there might be some edge cases, like
      DT overlays loaded at linux runtime (?!).

So this is what I'd do now: I'd skip a new device tree property
for now and determine if the interrupt line is shared (by solely
looking at the DT) and then disable the interrupt in the PHY
driver. This begs the question what we do if there is no DT,
interrupts disabled or enabled?

Andrew, what do you think?

-michael

Powered by blists - more mailing lists