Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <6f1c1a59-cec0-46d1-8ecb-a82d9d444ccf@ti.com>
Date:   Thu, 14 Dec 2023 11:14:13 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
CC:     <vigneshr@ti.com>, <kristo@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <danishanwar@ti.com>,
        <r-gunasekaran@ti.com>, <srk@ti.com>, <s-vadapalli@ti.com>
Subject: Re: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts
 for ICSSG2
Content-Language: en-US
To:     Nishanth Menon <nm@ti.com>
References: <20231213080216.1710730-1-s-vadapalli@ti.com>
 <20231213123819.tqh3lm2ceir3qjbk@swimmer>
From:   Siddharth Vadapalli <s-vadapalli@ti.com>
In-Reply-To: <20231213123819.tqh3lm2ceir3qjbk@swimmer>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Nishanth,

On 13/12/23 18:08, Nishanth Menon wrote:
> On 13:32-20231213, Siddharth Vadapalli wrote:
>> Enable interrupt mode of operation of the DP83867 Ethernet PHY which is
>> used by ICSSG2. The DP83867 PHY driver already supports interrupt handling
>> for interrupts generated by the PHY. Thus, add the necessary device-tree
>> support to enable it.
>>
>> Since the GPIO1_87 line is muxed with EXT_REFCLK1 and SYNC1_OUT, update
>> the pinmux to select GPIO1_87 for routing the interrupt.
>>
>> As the same interrupt line and therefore the same pinmux configuration is
>> applicable to both Ethernet PHYs used by ICSSG2, allocate the pinmux
>> resource to the first Ethernet PHY alone.

...

> 
> https://www.ti.com/lit/ds/symlink/dp83867ir.pdf -> it looks like the
> interrupt pin is level event. but drivers/gpio/gpio-davinci.c::
> gpio_irq_type() -> The SoC cannot handle level, only edge.
> 
> A bit confused here..  GPIO 87 is shared between two phys. isn't it a
> case of race?
> 
> PHY1 assets low
> phy1 handler starts, but before the driver it clears the condition:
> PHY2 asserts low - but since the signal is already low, there is no
> pulse
> phy1 handler clears phy1 condition, but signal is still low due to phy2?
> now phy2 OR phy1 never gets handled since there is never a pulse event
> ever again.

Yes, you are right! Edge-Triggered interrupts shouldn't be shared. I missed
noticing this. Thank you for pointing it out. Since the SoC only supports
Edge-Triggered interrupts, I believe that the correct decision would be to use
the interrupt for only one of the two PHYs, while leaving the other PHY in
polled mode of operation which is the default.

Kindly let me know if this is acceptable and I shall update this patch accordingly.

> 
> 
>>  		ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>>  		ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
>>  	};
>> -- 
>> 2.34.1
>>
> 

-- 
Regards,
Siddharth.