| Message ID | 20231029042712.520010-13-cristian.ciocaltea@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable networking support for StarFive JH7100 SoC | expand |
On 10/29/23 20:46, Andrew Lunn wrote: > On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote: >> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting >> RGMII-ID. >> >> TODO: Verify if manual adjustment of the RX internal delay is needed. If >> yes, add the mdio & phy sub-nodes. > > Please could you try to get this tested. It might shed some light on > what is going on here, since it is a different PHY. Actually, this is the main reason I added the patch. I don't have access to this board, so it would be great if we could get some help with testing. Thanks, Cristian
On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote: > On 10/30/23 00:53, Cristian Ciocaltea wrote: > > On 10/29/23 20:46, Andrew Lunn wrote: > >> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote: > >>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting > >>> RGMII-ID. > >>> > >>> TODO: Verify if manual adjustment of the RX internal delay is needed. If > >>> yes, add the mdio & phy sub-nodes. > >> > >> Please could you try to get this tested. It might shed some light on > >> what is going on here, since it is a different PHY. > > > > Actually, this is the main reason I added the patch. I don't have access > > to this board, so it would be great if we could get some help with testing. > > @Emil, @Conor: Any idea who might help us with a quick test on the > BeagleV Starlight board? I don't have one & I am not sure if Emil does. Geert (CCed) should have one though. Is there a specific test you need to have done?
Hi Cristian, On Fri, Nov 17, 2023 at 9:59 AM Cristian Ciocaltea <cristian.ciocaltea@collabora.com> wrote: > On 11/17/23 10:49, Cristian Ciocaltea wrote: > > On 11/17/23 10:37, Geert Uytterhoeven wrote: > >> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote: > >>> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote: > >>>> On 10/30/23 00:53, Cristian Ciocaltea wrote: > >>>>> On 10/29/23 20:46, Andrew Lunn wrote: > >>>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote: > >>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting > >>>>>>> RGMII-ID. > >>>>>>> > >>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If > >>>>>>> yes, add the mdio & phy sub-nodes. > >>>>>> > >>>>>> Please could you try to get this tested. It might shed some light on > >>>>>> what is going on here, since it is a different PHY. > >>>>> > >>>>> Actually, this is the main reason I added the patch. I don't have access > >>>>> to this board, so it would be great if we could get some help with testing. > >>>> > >>>> @Emil, @Conor: Any idea who might help us with a quick test on the > >>>> BeagleV Starlight board? > >>> > >>> I don't have one & I am not sure if Emil does. Geert (CCed) should have > >> > >> I believe Esmil has. > >> > >>> one though. Is there a specific test you need to have done? > >> > >> I gave it a try, on top of latest renesas-drivers[1]. > > [...] > > >> > >> Looks like it needs more non-coherent support before we can test > >> Ethernet. > > > > Hi Geert, > > > > Thanks for taking the time to test this! > > > > Could you please check if the following are enabled in your kernel config: > > > > CONFIG_DMA_GLOBAL_POOL > > CONFIG_RISCV_DMA_NONCOHERENT > > CONFIG_RISCV_NONSTANDARD_CACHE_OPS > > CONFIG_SIFIVE_CCACHE CONFIG_DMA_GLOBAL_POOL and CONFIG_RISCV_NONSTANDARD_CACHE_OPS were indeed no longer enabled, as they cannot be enabled manually. After cherry-picking commit e14ad9ff67fd51dc ("riscv: errata: Add StarFive JH7100 errata") in esmil/visionfive these options become enabled. Now it gets a bit further, but still lots of CCACHE DataFail errors. > Also please note the series requires the SiFive Composable Cache > controller patches provided by Emil [1]. > > [1]: https://lore.kernel.org/all/20231031141444.53426-1-emil.renner.berthing@canonical.com/ That series does not contain any Kconfig changes, so there must be other missing dependencies? Perhaps I should just defer to Emil ;-) Gr{oetje,eeting}s, Geert
On 11/17/23 13:19, Cristian Ciocaltea wrote: > On 11/17/23 11:12, Geert Uytterhoeven wrote: >> Hi Cristian, >> >> On Fri, Nov 17, 2023 at 9:59 AM Cristian Ciocaltea >> <cristian.ciocaltea@collabora.com> wrote: >>> On 11/17/23 10:49, Cristian Ciocaltea wrote: >>>> On 11/17/23 10:37, Geert Uytterhoeven wrote: >>>>> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote: >>>>>> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote: >>>>>>> On 10/30/23 00:53, Cristian Ciocaltea wrote: >>>>>>>> On 10/29/23 20:46, Andrew Lunn wrote: >>>>>>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote: >>>>>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting >>>>>>>>>> RGMII-ID. >>>>>>>>>> >>>>>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If >>>>>>>>>> yes, add the mdio & phy sub-nodes. >>>>>>>>> >>>>>>>>> Please could you try to get this tested. It might shed some light on >>>>>>>>> what is going on here, since it is a different PHY. >>>>>>>> >>>>>>>> Actually, this is the main reason I added the patch. I don't have access >>>>>>>> to this board, so it would be great if we could get some help with testing. >>>>>>> >>>>>>> @Emil, @Conor: Any idea who might help us with a quick test on the >>>>>>> BeagleV Starlight board? >>>>>> >>>>>> I don't have one & I am not sure if Emil does. Geert (CCed) should have >>>>> >>>>> I believe Esmil has. >>>>> >>>>>> one though. Is there a specific test you need to have done? >>>>> >>>>> I gave it a try, on top of latest renesas-drivers[1]. >>> >>> [...] >>> >>>>> >>>>> Looks like it needs more non-coherent support before we can test >>>>> Ethernet. >>>> >>>> Hi Geert, >>>> >>>> Thanks for taking the time to test this! >>>> >>>> Could you please check if the following are enabled in your kernel config: >>>> >>>> CONFIG_DMA_GLOBAL_POOL >>>> CONFIG_RISCV_DMA_NONCOHERENT >>>> CONFIG_RISCV_NONSTANDARD_CACHE_OPS >>>> CONFIG_SIFIVE_CCACHE >> >> CONFIG_DMA_GLOBAL_POOL and CONFIG_RISCV_NONSTANDARD_CACHE_OPS were >> indeed no longer enabled, as they cannot be enabled manually. >> >> After cherry-picking commit e14ad9ff67fd51dc ("riscv: errata: Add >> StarFive JH7100 errata") in esmil/visionfive these options become >> enabled. Now it gets a bit further, but still lots of CCACHE DataFail >> errors. > > Right, there is an open question [2] in PATCH v2 08/12 if this patch > should have been part of Emil's ccache series or I will send it in v3 > of my series. > > [2]: https://lore.kernel.org/lkml/4f661818-1585-41d8-a305-96fd359bc8b8@collabora.com/ > >>> Also please note the series requires the SiFive Composable Cache >>> controller patches provided by Emil [1]. >>> >>> [1]: https://lore.kernel.org/all/20231031141444.53426-1-emil.renner.berthing@canonical.com/ >> >> That series does not contain any Kconfig changes, so there must be >> other missing dependencies? > > There shouldn't be any additional Kconfig changes or dependencies as > those patches just extend an already existing driver. There were some > changes in v2, but they are still compatible with this series (I've > retested that to make sure). > > My tree is based on next-20231024, so I'm going to rebase it onto > next-20231117, to exclude the possibility of a regression somewhere. > > I will also test with renesas-drivers. I verified with both trees and didn't notice any issues with my VisionFive board, so I don't really understand why BeagleV Starlight shows a different behavior. For reference, please see [3] which contains all required patches applied on top of next-20231117. The top-most one 9d36dec7e6da ("riscv: dts: starfive: Add JH7100 MMC nodes") is optional, I added it to extend a bit the test suite (SD-card card access also works fine). [3]: https://gitlab.collabora.com/cristicc/linux-next/-/tree/visionfive-eth For configuring the kernel, I used: $ make [...] defconfig $ scripts/config --enable CONFIG_NONPORTABLE --enable ERRATA_STARFIVE_JH7100 I also noticed a warning message right before building starts, but it doesn't seem to be harmful: WARNING: unmet direct dependencies detected for DMA_GLOBAL_POOL Depends on [n]: !ARCH_HAS_DMA_SET_UNCACHED [=n] && !DMA_DIRECT_REMAP [=y] Selected by [y]: - ERRATA_STARFIVE_JH7100 [=y] && ARCH_STARFIVE [=y] && NONPORTABLE [=y] Thanks, Cristian
Cristian Ciocaltea wrote: > On 11/28/23 18:09, Emil Renner Berthing wrote: > > Cristian Ciocaltea wrote: > >> On 11/28/23 14:08, Emil Renner Berthing wrote: > >>> Cristian Ciocaltea wrote: > >>>> On 11/26/23 23:10, Emil Renner Berthing wrote: > >>>>> Cristian Ciocaltea wrote: > >>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting > >>>>>> RGMII-ID. > >>>>>> > >>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If > >>>>>> yes, add the mdio & phy sub-nodes. > >>>>> > >>>>> Sorry for being late here. I've tested that removing the mdio and phy nodes on > >>>>> the the Starlight board works fine, but the rx-internal-delay-ps = <900> > >>>>> property not needed on any of my VisionFive V1 boards either. > >>>> > >>>> No problem, thanks a lot for taking the time to help with the testing! > >>>> > >>>>> So I wonder why you need that on your board > >>>> > >>>> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable > >>>> rgmii rx delay") in your tree, hence I you please confirm the tests were > >>>> done with that commit reverted? > >>>> > >>>>> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here > >>>>> you still set it to "rgmii-id", so which is it? > >>>> > >>>> Please try with "rgmii-id" first. I added "rgmii-txid" to have a > >>>> fallback solution in case the former cannot be used. > >>> > >>> Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight > >>> board with the Microchip phy works with "rgmii-id" as is. And you're right, > >>> with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too. > >> > >> That's great, we have now a pretty clear indication that this uncommon behavior > >> stems from the Motorcomm PHY, and *not* from GMAC. > >> > >>>> > >>>>> You've alse removed the phy reset gpio on the Starlight board: > >>>>> > >>>>> snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW> > >>>>> > >>>>> Why? > >>>> > >>>> I missed this in v1 as the gmac handling was done exclusively in > >>>> jh7100-common. Thanks for noticing! > >>>> > >>>>>> > >>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > >>>>>> --- > >>>>>> arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++ > >>>>>> 1 file changed, 5 insertions(+) > >>>>>> > >>>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts > >>>>>> index 7cda3a89020a..d3f4c99d98da 100644 > >>>>>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts > >>>>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts > >>>>>> @@ -11,3 +11,8 @@ / { > >>>>>> model = "BeagleV Starlight Beta"; > >>>>>> compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100"; > >>>>>> }; > >>>>>> + > >>>>>> +&gmac { > >>>>>> + phy-mode = "rgmii-id"; > >>>>>> + status = "okay"; > >>>>>> +}; > >>>>> > >>>>> Lastly the phy-mode and status are the same for the VF1 and Starlight boards, > >>>>> so why can't these be set in the jh7100-common.dtsi? > >>>> > >>>> I wasn't sure "rgmii-id" can be used for both boards and I didn't want > >>>> to unconditionally enable gmac on Starlight before getting a > >>>> confirmation that this actually works. > >>>> > >>>> If there is no way to make it working with "rgmii-id" (w/ or w/o > >>>> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid". > >>> > >>> Yeah, I don't exactly know the difference, but both boards seem to work fine > >>> with "rgmii-id", so if that is somehow better and/or more correct let's just go > >>> with that. > >> > >> As Andrew already pointed out, going with "rgmii-id" would be the recommended > >> approach, as this passes the responsibility of adding both TX and RX delays to > >> the PHY. "rgmii-txid" requires the MAC to handle the RX delay, which might > >> break the boards having a conformant (aka well-behaving) PHY. For some reason > >> the Microchip PHY seems to work fine in both cases, but that's most likely an > >> exception, as other PHYs might expose a totally different and undesired > >> behavior. > >> > >> I will prepare a v3 soon, and will drop the patches you have already submitted > >> as part of [1]. > > > > Sounds good. Then what's missing for ethernet to work is just the clock patches: > > https://github.com/esmil/linux/commit/b5abe1cb3815765739aff7949deed6f65b952c4a > > https://github.com/esmil/linux/commit/3a7a423b15a9f796586cbbdc37010d2b83ff2367 > > > > You can either include those as part of your patch series enabling ethernet, or > > they can be submitted separately with the audio clocks. Either way is > > fine by me. > > I can cherry-pick them, but so far I couldn't identify any networking > related issues if those patches are not applied. Could it be something > specific to Starlight board only? No, it's the same for both boards. The dwmac-starfive driver adjusts the tx clock: 1000Mbit -> 125MHz 100Mbit -> 25MHz 10Mbit -> 2.5MHz The tx clock is given in the device tree as the gmac_tx_inv clock which derives from either the gmac_root_div or gmac_rmii_ref external clock like this: gmac_rmii_ref (external) -> gmac_rmii_txclk \ gmac_root_div (500MHz) -> gmac_gtxclk (div N) -> gmac_tx (mux) -> gmac_tx_inv ..where N defaults to 4 and the gmac_tx mux defaults to the gmac_gtxclk, so the gmac_tx_inv clock defaults to 125MHz suitable for 1000Mbit connections. See /sys/kernel/debug/clk/clk_summary for another overview. When the dwmac_starfive driver request to change gmac_tx_inv to 25MHz the clock framework will that it has the CLK_SET_RATE_PARENT flag set, so it will try the gmac_tx clock next. This is a mux that can choose either the 125MHz gmac_gtxclk or the external gmac_rmii_txclk which defaults to 0MHz in the current device trees, so the request cannot be met. That's why we need to set the CLK_SET_RATE_PARENT (and CLK_SET_RATE_NO_REPARENT) flags on the gmac_tx clock so the clock framework again goes to try setting the gmac_gtxclk to 25MHz, which it can because it's a divider and setting N=20 does the trick. On your board you can manually force a 100Mbit connection with ethtool -s eth0 speed 100 That fails on my boards without those two patches. /Emil > >> > >> Thanks again for your support, > >> Cristian > >> > >> [1]: https://lore.kernel.org/all/20231126232746.264302-1-emil.renner.berthing@canonical.com/
On 11/29/23 16:28, Emil Renner Berthing wrote: > Cristian Ciocaltea wrote: >> On 11/28/23 18:09, Emil Renner Berthing wrote: >>> Cristian Ciocaltea wrote: >>>> On 11/28/23 14:08, Emil Renner Berthing wrote: >>>>> Cristian Ciocaltea wrote: >>>>>> On 11/26/23 23:10, Emil Renner Berthing wrote: >>>>>>> Cristian Ciocaltea wrote: >>>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting >>>>>>>> RGMII-ID. >>>>>>>> >>>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If >>>>>>>> yes, add the mdio & phy sub-nodes. >>>>>>> >>>>>>> Sorry for being late here. I've tested that removing the mdio and phy nodes on >>>>>>> the the Starlight board works fine, but the rx-internal-delay-ps = <900> >>>>>>> property not needed on any of my VisionFive V1 boards either. >>>>>> >>>>>> No problem, thanks a lot for taking the time to help with the testing! >>>>>> >>>>>>> So I wonder why you need that on your board >>>>>> >>>>>> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable >>>>>> rgmii rx delay") in your tree, hence I you please confirm the tests were >>>>>> done with that commit reverted? >>>>>> >>>>>>> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here >>>>>>> you still set it to "rgmii-id", so which is it? >>>>>> >>>>>> Please try with "rgmii-id" first. I added "rgmii-txid" to have a >>>>>> fallback solution in case the former cannot be used. >>>>> >>>>> Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight >>>>> board with the Microchip phy works with "rgmii-id" as is. And you're right, >>>>> with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too. >>>> >>>> That's great, we have now a pretty clear indication that this uncommon behavior >>>> stems from the Motorcomm PHY, and *not* from GMAC. >>>> >>>>>> >>>>>>> You've alse removed the phy reset gpio on the Starlight board: >>>>>>> >>>>>>> snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW> >>>>>>> >>>>>>> Why? >>>>>> >>>>>> I missed this in v1 as the gmac handling was done exclusively in >>>>>> jh7100-common. Thanks for noticing! >>>>>> >>>>>>>> >>>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>>>>>> --- >>>>>>>> arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++ >>>>>>>> 1 file changed, 5 insertions(+) >>>>>>>> >>>>>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts >>>>>>>> index 7cda3a89020a..d3f4c99d98da 100644 >>>>>>>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts >>>>>>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts >>>>>>>> @@ -11,3 +11,8 @@ / { >>>>>>>> model = "BeagleV Starlight Beta"; >>>>>>>> compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100"; >>>>>>>> }; >>>>>>>> + >>>>>>>> +&gmac { >>>>>>>> + phy-mode = "rgmii-id"; >>>>>>>> + status = "okay"; >>>>>>>> +}; >>>>>>> >>>>>>> Lastly the phy-mode and status are the same for the VF1 and Starlight boards, >>>>>>> so why can't these be set in the jh7100-common.dtsi? >>>>>> >>>>>> I wasn't sure "rgmii-id" can be used for both boards and I didn't want >>>>>> to unconditionally enable gmac on Starlight before getting a >>>>>> confirmation that this actually works. >>>>>> >>>>>> If there is no way to make it working with "rgmii-id" (w/ or w/o >>>>>> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid". >>>>> >>>>> Yeah, I don't exactly know the difference, but both boards seem to work fine >>>>> with "rgmii-id", so if that is somehow better and/or more correct let's just go >>>>> with that. >>>> >>>> As Andrew already pointed out, going with "rgmii-id" would be the recommended >>>> approach, as this passes the responsibility of adding both TX and RX delays to >>>> the PHY. "rgmii-txid" requires the MAC to handle the RX delay, which might >>>> break the boards having a conformant (aka well-behaving) PHY. For some reason >>>> the Microchip PHY seems to work fine in both cases, but that's most likely an >>>> exception, as other PHYs might expose a totally different and undesired >>>> behavior. >>>> >>>> I will prepare a v3 soon, and will drop the patches you have already submitted >>>> as part of [1]. >>> >>> Sounds good. Then what's missing for ethernet to work is just the clock patches: >>> https://github.com/esmil/linux/commit/b5abe1cb3815765739aff7949deed6f65b952c4a >>> https://github.com/esmil/linux/commit/3a7a423b15a9f796586cbbdc37010d2b83ff2367 >>> >>> You can either include those as part of your patch series enabling ethernet, or >>> they can be submitted separately with the audio clocks. Either way is >>> fine by me. >> >> I can cherry-pick them, but so far I couldn't identify any networking >> related issues if those patches are not applied. Could it be something >> specific to Starlight board only? > > No, it's the same for both boards. The dwmac-starfive driver adjusts > the tx clock: > > 1000Mbit -> 125MHz > 100Mbit -> 25MHz > 10Mbit -> 2.5MHz > > The tx clock is given in the device tree as the gmac_tx_inv clock which derives > from either the gmac_root_div or gmac_rmii_ref external clock like this: > > gmac_rmii_ref (external) -> gmac_rmii_txclk \ > gmac_root_div (500MHz) -> gmac_gtxclk (div N) -> gmac_tx (mux) -> gmac_tx_inv > > ..where N defaults to 4 and the gmac_tx mux defaults to the gmac_gtxclk, so > the gmac_tx_inv clock defaults to 125MHz suitable for 1000Mbit connections. > See /sys/kernel/debug/clk/clk_summary for another overview. > > When the dwmac_starfive driver request to change gmac_tx_inv to 25MHz the clock > framework will that it has the CLK_SET_RATE_PARENT flag set, so it will try > the gmac_tx clock next. This is a mux that can choose either the > 125MHz gmac_gtxclk > or the external gmac_rmii_txclk which defaults to 0MHz in the current > device trees, > so the request cannot be met. > > That's why we need to set the CLK_SET_RATE_PARENT (and CLK_SET_RATE_NO_REPARENT) > flags on the gmac_tx clock so the clock framework again goes to try setting the > gmac_gtxclk to 25MHz, which it can because it's a divider and setting N=20 > does the trick. > > On your board you can manually force a 100Mbit connection with > ethtool -s eth0 speed 100 > > That fails on my boards without those two patches. > /Emil Thanks for the detailed explanation! I've been only verified with gigabit connectivity, that would explain why I didn't notice the issue. I will make sure to properly test this before sending v3. Regards, Cristian
On 11/28/23 02:40, Cristian Ciocaltea wrote: > On 11/26/23 23:10, Emil Renner Berthing wrote: >> Cristian Ciocaltea wrote: >>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting >>> RGMII-ID. >>> [...] >> You've alse removed the phy reset gpio on the Starlight board: >> >> snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW> >> >> Why? > > I missed this in v1 as the gmac handling was done exclusively in > jh7100-common. Thanks for noticing! Hi Emil, I think the reset doesn't actually trigger because "snps,reset-gpios" is not a valid property, it should have been "snps,reset-gpio" (without the trailing "s"). However, this seems to be deprecated now, and the recommended approach would be to define the reset gpio in the phy node, which I did in [1]. Hopefully this won't cause any unexpected behaviour. Otherwise we should probably simply drop it. [1]: https://lore.kernel.org/lkml/20231215204050.2296404-8-cristian.ciocaltea@collabora.com/
Cristian Ciocaltea wrote: > On 11/28/23 02:40, Cristian Ciocaltea wrote: > > On 11/26/23 23:10, Emil Renner Berthing wrote: > >> Cristian Ciocaltea wrote: > >>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting > >>> RGMII-ID. > >>> > > [...] > > >> You've alse removed the phy reset gpio on the Starlight board: > >> > >> snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW> > >> > >> Why? > > > > I missed this in v1 as the gmac handling was done exclusively in > > jh7100-common. Thanks for noticing! > > Hi Emil, > > I think the reset doesn't actually trigger because "snps,reset-gpios" is > not a valid property, it should have been "snps,reset-gpio" (without the > trailing "s"). > > However, this seems to be deprecated now, and the recommended approach > would be to define the reset gpio in the phy node, which I did in [1]. > > Hopefully this won't cause any unexpected behaviour. Otherwise we should > probably simply drop it. > > [1]: https://lore.kernel.org/lkml/20231215204050.2296404-8-cristian.ciocaltea@collabora.com/ Oh, nice catch! With your v3 patches the Starlight board still works fine and GPIO63 is correctly grabbed and used for "PHY reset". /Emil
On 12/16/23 21:24, Emil Renner Berthing wrote: > Cristian Ciocaltea wrote: >> On 11/28/23 02:40, Cristian Ciocaltea wrote: >>> On 11/26/23 23:10, Emil Renner Berthing wrote: >>>> Cristian Ciocaltea wrote: >>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting >>>>> RGMII-ID. >>>>> >> >> [...] >> >>>> You've alse removed the phy reset gpio on the Starlight board: >>>> >>>> snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW> >>>> >>>> Why? >>> >>> I missed this in v1 as the gmac handling was done exclusively in >>> jh7100-common. Thanks for noticing! >> >> Hi Emil, >> >> I think the reset doesn't actually trigger because "snps,reset-gpios" is >> not a valid property, it should have been "snps,reset-gpio" (without the >> trailing "s"). >> >> However, this seems to be deprecated now, and the recommended approach >> would be to define the reset gpio in the phy node, which I did in [1]. >> >> Hopefully this won't cause any unexpected behaviour. Otherwise we should >> probably simply drop it. >> >> [1]: https://lore.kernel.org/lkml/20231215204050.2296404-8-cristian.ciocaltea@collabora.com/ > > Oh, nice catch! With your v3 patches the Starlight board still works fine and > GPIO63 is correctly grabbed and used for "PHY reset". Great, thanks a lot for retesting this!
diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts index 7cda3a89020a..d3f4c99d98da 100644 --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts @@ -11,3 +11,8 @@ / { model = "BeagleV Starlight Beta"; compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100"; }; + +&gmac { + phy-mode = "rgmii-id"; + status = "okay"; +};
The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting RGMII-ID. TODO: Verify if manual adjustment of the RX internal delay is needed. If yes, add the mdio & phy sub-nodes. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++ 1 file changed, 5 insertions(+)