diff mbox series

[V2,11/14] ARM: dts: stm32: Add missing ethernet PHY reset on AV96

Message ID 20200331004851.282583-12-marex@denx.de
State Superseded
Delegated to: Patrick Delaunay
Headers show
Series ARM: stm32: Fix Avenger96 | expand

Commit Message

Marek Vasut March 31, 2020, 12:48 a.m. UTC
Add PHY reset GPIO on AV96 ethernet PHY.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
---
V2: No change
---
 arch/arm/dts/stm32mp157a-avenger96.dts | 1 +
 1 file changed, 1 insertion(+)

Comments

Patrice CHOTARD March 31, 2020, 8:35 a.m. UTC | #1
Hi Marek

On 3/31/20 2:48 AM, Marek Vasut wrote:
> Add PHY reset GPIO on AV96 ethernet PHY.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>

Reviewed-by: Patrice Chotard <patrice.chotard@st.com>

Thanks

> ---
> V2: No change
> ---
>  arch/arm/dts/stm32mp157a-avenger96.dts | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/dts/stm32mp157a-avenger96.dts b/arch/arm/dts/stm32mp157a-avenger96.dts
> index fb8f0021b3..7a4b6e6a2c 100644
> --- a/arch/arm/dts/stm32mp157a-avenger96.dts
> +++ b/arch/arm/dts/stm32mp157a-avenger96.dts
> @@ -101,6 +101,7 @@
>  	phy-mode = "rgmii";
>  	max-speed = <1000>;
>  	phy-handle = <&phy0>;
> +	phy-reset-gpios = <&gpioz 2 GPIO_ACTIVE_LOW>;
>  
>  	mdio0 {
>  		#address-cells = <1>;
Patrick Delaunay March 31, 2020, 2:06 p.m. UTC | #2
Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 31 mars 2020 02:49
> 
> Add PHY reset GPIO on AV96 ethernet PHY.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> ---
> V2: No change
> ---
>  arch/arm/dts/stm32mp157a-avenger96.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/dts/stm32mp157a-avenger96.dts
> b/arch/arm/dts/stm32mp157a-avenger96.dts
> index fb8f0021b3..7a4b6e6a2c 100644
> --- a/arch/arm/dts/stm32mp157a-avenger96.dts
> +++ b/arch/arm/dts/stm32mp157a-avenger96.dts
> @@ -101,6 +101,7 @@
>  	phy-mode = "rgmii";
>  	max-speed = <1000>;
>  	phy-handle = <&phy0>;
> +	phy-reset-gpios = <&gpioz 2 GPIO_ACTIVE_LOW>;

This obsolete binding is (and won't be) supported for STM32MP15x variant of dwc_eth_qos driver.

I am currenty upstream the support in U-boot of the support of the final binding
aligned with Kernel one.

"net: dwc_eth_qos: implement reset-gpios for stm32"
http://patchwork.ozlabs.org/patch/1257377/

=> phy-handle and "reset-gpios" in sub node PHY

>  	mdio0 {
>  		#address-cells = <1>;
> --
> 2.25.1

Regards

Patrick
Marek Vasut March 31, 2020, 5:06 p.m. UTC | #3
On 3/31/20 4:06 PM, Patrick DELAUNAY wrote:
> Hi Marek,
> 
>> From: Marek Vasut <marex@denx.de>
>> Sent: mardi 31 mars 2020 02:49
>>
>> Add PHY reset GPIO on AV96 ethernet PHY.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Patrick Delaunay <patrick.delaunay@st.com>
>> Cc: Patrice Chotard <patrice.chotard@st.com>
>> ---
>> V2: No change
>> ---
>>  arch/arm/dts/stm32mp157a-avenger96.dts | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/dts/stm32mp157a-avenger96.dts
>> b/arch/arm/dts/stm32mp157a-avenger96.dts
>> index fb8f0021b3..7a4b6e6a2c 100644
>> --- a/arch/arm/dts/stm32mp157a-avenger96.dts
>> +++ b/arch/arm/dts/stm32mp157a-avenger96.dts
>> @@ -101,6 +101,7 @@
>>  	phy-mode = "rgmii";
>>  	max-speed = <1000>;
>>  	phy-handle = <&phy0>;
>> +	phy-reset-gpios = <&gpioz 2 GPIO_ACTIVE_LOW>;
> 
> This obsolete binding is (and won't be) supported for STM32MP15x variant of dwc_eth_qos driver.
> 
> I am currenty upstream the support in U-boot of the support of the final binding
> aligned with Kernel one.
> 
> "net: dwc_eth_qos: implement reset-gpios for stm32"
> http://patchwork.ozlabs.org/patch/1257377/

That's probably gonna go in the next cycle, right? So you likely want to
grab this for this cycle to fix the ethernet here and then in next cycle
rework the bindings.

btw you also want to look at this series:
[PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3
Patrick Delaunay April 1, 2020, 8:57 a.m. UTC | #4
Hi,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 31 mars 2020 19:07
> 
> On 3/31/20 4:06 PM, Patrick DELAUNAY wrote:
> > Hi Marek,
> >
> >> From: Marek Vasut <marex@denx.de>
> >> Sent: mardi 31 mars 2020 02:49
> >>
> >> Add PHY reset GPIO on AV96 ethernet PHY.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> >> Cc: Patrice Chotard <patrice.chotard@st.com>
> >> ---
> >> V2: No change
> >> ---
> >>  arch/arm/dts/stm32mp157a-avenger96.dts | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/arm/dts/stm32mp157a-avenger96.dts
> >> b/arch/arm/dts/stm32mp157a-avenger96.dts
> >> index fb8f0021b3..7a4b6e6a2c 100644
> >> --- a/arch/arm/dts/stm32mp157a-avenger96.dts
> >> +++ b/arch/arm/dts/stm32mp157a-avenger96.dts
> >> @@ -101,6 +101,7 @@
> >>  	phy-mode = "rgmii";
> >>  	max-speed = <1000>;
> >>  	phy-handle = <&phy0>;
> >> +	phy-reset-gpios = <&gpioz 2 GPIO_ACTIVE_LOW>;
> >
> > This obsolete binding is (and won't be) supported for STM32MP15x variant of
> dwc_eth_qos driver.
> >
> > I am currenty upstream the support in U-boot of the support of the
> > final binding aligned with Kernel one.
> >
> > "net: dwc_eth_qos: implement reset-gpios for stm32"
> > http://patchwork.ozlabs.org/patch/1257377/
> 
> That's probably gonna go in the next cycle, right? So you likely want to grab this
> for this cycle to fix the ethernet here and then in next cycle rework the bindings.

Yes in the next cycle probably.

But the ethernet can't be not fixed with this patch, at least in U-boot....
as the reset by gpio is not executed in driver: it is not supported.

Only present in eqos_probe_resources_tegra186
but not in eqos_probe_resources_stm32.

Or you have perhaps a local patch on dwc_eth_qos.c

I think I can drop this unneeded patch for the release...
Or merge it, as is no effect.

And after binding rework, the AV96 board DT should be updated
(in kernel also, I think, as the old binding should be refused).

> btw you also want to look at this series:
> [PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3

Yes I see the serie....

I want to review and test it since 2 weeks.

And particularly check if I can remove the CONFIG_SYS_NONCACHED_MEMORY
in stm32mp1.h with this serie.

I am not happy with this configuration (it is more a workaround 
for cache management issue in this driver that a real solution).

But I am at home since 3 weeks with COVID-19, and my test 
capacity is reduced.

I will review this serie this week.

Thanks for reminder.

Patrick
Marek Vasut April 1, 2020, 10:52 a.m. UTC | #5
On 4/1/20 10:57 AM, Patrick DELAUNAY wrote:
> Hi,

Hi,

>> From: Marek Vasut <marex@denx.de>
>> Sent: mardi 31 mars 2020 19:07
>>
>> On 3/31/20 4:06 PM, Patrick DELAUNAY wrote:
>>> Hi Marek,
>>>
>>>> From: Marek Vasut <marex@denx.de>
>>>> Sent: mardi 31 mars 2020 02:49
>>>>
>>>> Add PHY reset GPIO on AV96 ethernet PHY.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Patrick Delaunay <patrick.delaunay@st.com>
>>>> Cc: Patrice Chotard <patrice.chotard@st.com>
>>>> ---
>>>> V2: No change
>>>> ---
>>>>  arch/arm/dts/stm32mp157a-avenger96.dts | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/arm/dts/stm32mp157a-avenger96.dts
>>>> b/arch/arm/dts/stm32mp157a-avenger96.dts
>>>> index fb8f0021b3..7a4b6e6a2c 100644
>>>> --- a/arch/arm/dts/stm32mp157a-avenger96.dts
>>>> +++ b/arch/arm/dts/stm32mp157a-avenger96.dts
>>>> @@ -101,6 +101,7 @@
>>>>  	phy-mode = "rgmii";
>>>>  	max-speed = <1000>;
>>>>  	phy-handle = <&phy0>;
>>>> +	phy-reset-gpios = <&gpioz 2 GPIO_ACTIVE_LOW>;
>>>
>>> This obsolete binding is (and won't be) supported for STM32MP15x variant of
>> dwc_eth_qos driver.
>>>
>>> I am currenty upstream the support in U-boot of the support of the
>>> final binding aligned with Kernel one.
>>>
>>> "net: dwc_eth_qos: implement reset-gpios for stm32"
>>> http://patchwork.ozlabs.org/patch/1257377/
>>
>> That's probably gonna go in the next cycle, right? So you likely want to grab this
>> for this cycle to fix the ethernet here and then in next cycle rework the bindings.
> 
> Yes in the next cycle probably.
> 
> But the ethernet can't be not fixed with this patch, at least in U-boot....
> as the reset by gpio is not executed in driver: it is not supported.
> 
> Only present in eqos_probe_resources_tegra186
> but not in eqos_probe_resources_stm32.
> 
> Or you have perhaps a local patch on dwc_eth_qos.c

Oh doh, I don't.

> I think I can drop this unneeded patch for the release...
> Or merge it, as is no effect.
> 
> And after binding rework, the AV96 board DT should be updated
> (in kernel also, I think, as the old binding should be refused).

Agreed. Drop it then.

>> btw you also want to look at this series:
>> [PATCH 1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3
> 
> Yes I see the serie....
> 
> I want to review and test it since 2 weeks.
> 
> And particularly check if I can remove the CONFIG_SYS_NONCACHED_MEMORY
> in stm32mp1.h with this serie.

We should consider that, yes, it looks somewhat crappy.

Basically, your problem is that the descriptor is shorter than your
cacheline, that means you cannot write into the descriptor right away
after the MAC indicates the descriptor is complete, because you would
evict the entire cache line and likely corrupt the neighboring
descriptors too.

What you would have to do is
- invalidate the cacheline which contains the currently finished desc
- read the desc status
- IFF and only IFF this is the last desc in the cacheline, flag all
  the descriptors as ready for the DMA engine to use again, and only
  then flush the entire cacheline
And you probably also want to make sure that the DWMAC4 "last usable
descriptor" pointer does not point at these currently updated
descriptors, so that the DWMAC DMA engine won't access them while you
are flushing the cache line.

This is certainly doable, one just has to be careful about the cache
intricacies.

> I am not happy with this configuration (it is more a workaround 
> for cache management issue in this driver that a real solution).
> 
> But I am at home since 3 weeks with COVID-19, and my test 
> capacity is reduced.

Get better soon!
diff mbox series

Patch

diff --git a/arch/arm/dts/stm32mp157a-avenger96.dts b/arch/arm/dts/stm32mp157a-avenger96.dts
index fb8f0021b3..7a4b6e6a2c 100644
--- a/arch/arm/dts/stm32mp157a-avenger96.dts
+++ b/arch/arm/dts/stm32mp157a-avenger96.dts
@@ -101,6 +101,7 @@ 
 	phy-mode = "rgmii";
 	max-speed = <1000>;
 	phy-handle = <&phy0>;
+	phy-reset-gpios = <&gpioz 2 GPIO_ACTIVE_LOW>;
 
 	mdio0 {
 		#address-cells = <1>;