This patch series adds ethernet support for the StarFive JH7100 SoC and
makes it available for the StarFive VisionFive V1 and BeagleV Starlight
boards, although I could only validate on the former SBC.
The work is heavily based on the reference implementation [1] and depends
the non-coherent DMA support provided by Emil via the SiFive Composable
Cache controller [2].
[1] https://github.com/starfive-tech/linux/commits/visionfive
[2] https://lore.kernel.org/all/CAJM55Z_pdoGxRXbmBgJ5GbVWyeM1N6+LHihbNdT26Oo_qA5VYA@mail.gmail.com/
Changes in v2:
- Dropped ccache PATCH 01-05 reworked by Emil via [2]
- Dropped already applied PATCH 06/12
- Added PATCH v2 01 to prepare snps-dwmac binding for JH7100 support
- Added PATCH v2 02-03 to provide some jh7110-dwmac binding optimizations
- Handled JH7110 conflicting work in PATCH 07 via PATCH v2 04
- Reworked PATCH 8 via PATCH v2 05, adding JH7100 quirk and dropped
- starfive,gtxclk-dlychain DT property, also fixed register naming
- Added PATCH v2 08 providing DMA coherency related DT changes
- Updated PATCH 9 commit msg:
s/OF_DMA_DEFAULT_COHERENT/ARCH_DMA_DEFAULT_COHERENT/
- Replaced 'uncached-offset' property with 'sifive,cache-ops'
in PATCH 10/12 and dropped 'sideband' reg
- Add new patch providing coherent DMA memory pool (PATCH v2 10)
- Updated PATCH 11/12 according to the stmmac glue layer changes in
upstream
- Split PATCH 12/12 into PATCH v2 10-12 to handle individual gmac setup of
VisionFive v1 and BeagleV boards as they use different PHYs; also
switched phy-mode from "rgmii-tx" to "rgmii-id" (requires a reduction of
rx-internal-delay-ps by ~50%)
- Rebased series onto next-20231024
- v1: https://lore.kernel.org/lkml/[email protected]/
Cristian Ciocaltea (11):
dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset
dt-bindings: net: starfive,jh7110-dwmac: Drop superfluous select
dt-bindings: net: starfive,jh7110-dwmac: Drop redundant reset
description
dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible
net: stmmac: dwmac-starfive: Add support for JH7100 SoC
riscv: dts: starfive: jh7100: Add dma-noncoherent property
riscv: dts: starfive: jh7100: Add ccache DT node
riscv: dts: starfive: jh7100: Add sysmain and gmac DT nodes
riscv: dts: starfive: jh7100-common: Setup gmac pinmux
riscv: dts: starfive: visionfive-v1: Enable gmac and setup phy
[UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac
Emil Renner Berthing (1):
riscv: dts: starfive: Add pool for coherent DMA memory on JH7100
boards
.../devicetree/bindings/net/snps,dwmac.yaml | 3 +-
.../bindings/net/starfive,jh7110-dwmac.yaml | 84 +++++++++------
.../dts/starfive/jh7100-beaglev-starlight.dts | 5 +
.../boot/dts/starfive/jh7100-common.dtsi | 100 ++++++++++++++++++
.../jh7100-starfive-visionfive-v1.dts | 17 +++
arch/riscv/boot/dts/starfive/jh7100.dtsi | 51 +++++++++
drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 +-
.../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 +++++-
8 files changed, 259 insertions(+), 39 deletions(-)
--
2.42.0
The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
similar to the newer JH7110, but it requires only two interrupts and a
single reset line.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
.../devicetree/bindings/net/snps,dwmac.yaml | 1 +
.../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------
2 files changed, 54 insertions(+), 21 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index a4d7172ea701..c1380ff1c054 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -95,6 +95,7 @@ properties:
- snps,dwmac-5.20
- snps,dwxgmac
- snps,dwxgmac-2.10
+ - starfive,jh7100-dwmac
- starfive,jh7110-dwmac
reg:
diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
index 44e58755a5a2..70e35a3401f4 100644
--- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
@@ -13,10 +13,14 @@ maintainers:
properties:
compatible:
- items:
- - enum:
- - starfive,jh7110-dwmac
- - const: snps,dwmac-5.20
+ oneOf:
+ - items:
+ - const: starfive,jh7100-dwmac
+ - const: snps,dwmac
+ - items:
+ - enum:
+ - starfive,jh7110-dwmac
+ - const: snps,dwmac-5.20
reg:
maxItems: 1
@@ -37,23 +41,6 @@ properties:
- const: tx
- const: gtx
- interrupts:
- minItems: 3
- maxItems: 3
-
- interrupt-names:
- minItems: 3
- maxItems: 3
-
- resets:
- minItems: 2
- maxItems: 2
-
- reset-names:
- items:
- - const: stmmaceth
- - const: ahb
-
starfive,tx-use-rgmii-clk:
description:
Tx clock is provided by external rgmii clock.
@@ -84,6 +71,51 @@ required:
allOf:
- $ref: snps,dwmac.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh7100-dwmac
+ then:
+ properties:
+ interrupts:
+ minItems: 2
+ maxItems: 2
+
+ interrupt-names:
+ minItems: 2
+ maxItems: 2
+
+ resets:
+ maxItems: 1
+
+ reset-names:
+ const: ahb
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh7110-dwmac
+ then:
+ properties:
+ interrupts:
+ minItems: 3
+ maxItems: 3
+
+ interrupt-names:
+ minItems: 3
+ maxItems: 3
+
+ resets:
+ minItems: 2
+ maxItems: 2
+
+ reset-names:
+ items:
+ - const: stmmaceth
+ - const: ahb
+
unevaluatedProperties: false
examples:
--
2.42.0
Add a missing quirk to enable support for the StarFive JH7100 SoC.
Additionally, for greater flexibility in operation, allow using the
rgmii-rxid and rgmii-txid phy modes.
Co-developed-by: Emil Renner Berthing <[email protected]>
Signed-off-by: Emil Renner Berthing <[email protected]>
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 ++--
.../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 ++++++++++++++++---
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index a2b9e289aa36..c3c2c8360047 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -165,9 +165,9 @@ config DWMAC_STARFIVE
help
Support for ethernet controllers on StarFive RISC-V SoCs
- This selects the StarFive platform specific glue layer support for
- the stmmac device driver. This driver is used for StarFive JH7110
- ethernet controller.
+ This selects the StarFive platform specific glue layer support
+ for the stmmac device driver. This driver is used for the
+ StarFive JH7100 and JH7110 ethernet controllers.
config DWMAC_STI
tristate "STi GMAC support"
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index 5d630affb4d1..88c431edcea0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -15,13 +15,20 @@
#include "stmmac_platform.h"
-#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
-#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
-#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
+#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
+#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
+#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
+
+#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8
+
+struct starfive_dwmac_data {
+ unsigned int gtxclk_dlychain;
+};
struct starfive_dwmac {
struct device *dev;
struct clk *clk_tx;
+ const struct starfive_dwmac_data *data;
};
static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
@@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
mode = STARFIVE_DWMAC_PHY_INFT_RGMII;
break;
@@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
if (err)
return dev_err_probe(dwmac->dev, err, "error setting phy mode\n");
+ if (dwmac->data) {
+ err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN,
+ dwmac->data->gtxclk_dlychain);
+ if (err)
+ return dev_err_probe(dwmac->dev, err,
+ "error selecting gtxclk delay chain\n");
+ }
+
return 0;
}
@@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
if (!dwmac)
return -ENOMEM;
+ dwmac->data = device_get_match_data(&pdev->dev);
+
dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
if (IS_ERR(dwmac->clk_tx))
return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
@@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
}
+static const struct starfive_dwmac_data jh7100_data = {
+ .gtxclk_dlychain = 4
+};
+
static const struct of_device_id starfive_dwmac_match[] = {
- { .compatible = "starfive,jh7110-dwmac" },
+ { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data },
+ { .compatible = "starfive,jh7110-dwmac" },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, starfive_dwmac_match);
--
2.42.0
Provide a DT node for the SiFive Composable Cache controller found on
the StarFive JH7100 SoC.
Note this is also used to support non-coherent DMA, via the
sifive,cache-ops cache flushing operations.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
arch/riscv/boot/dts/starfive/jh7100.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi
index 06bb157ce111..a8a5bb00b0d8 100644
--- a/arch/riscv/boot/dts/starfive/jh7100.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi
@@ -32,6 +32,7 @@ U74_0: cpu@0 {
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
+ next-level-cache = <&ccache>;
riscv,isa = "rv64imafdc";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr",
@@ -60,6 +61,7 @@ U74_1: cpu@1 {
i-tlb-sets = <1>;
i-tlb-size = <32>;
mmu-type = "riscv,sv39";
+ next-level-cache = <&ccache>;
riscv,isa = "rv64imafdc";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr",
@@ -147,6 +149,18 @@ soc {
dma-noncoherent;
ranges;
+ ccache: cache-controller@2010000 {
+ compatible = "starfive,jh7100-ccache", "sifive,ccache0", "cache";
+ reg = <0x0 0x2010000 0x0 0x1000>;
+ interrupts = <128>, <130>, <131>, <129>;
+ cache-block-size = <64>;
+ cache-level = <2>;
+ cache-sets = <2048>;
+ cache-size = <2097152>;
+ cache-unified;
+ sifive,cache-ops;
+ };
+
clint: clint@2000000 {
compatible = "starfive,jh7100-clint", "sifive,clint0";
reg = <0x0 0x2000000 0x0 0x10000>;
--
2.42.0
The StarFive VisionFive V1 SBC has a Motorcomm YT8521 PHY supporting
RGMII-ID, but requires manual adjustment of the RX internal delay to
work properly.
The default RX delay provided by the driver is 1.95 ns, which proves to
be too high. Applying a 50% reduction seems to mitigate the issue.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
.../starfive/jh7100-starfive-visionfive-v1.dts | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts b/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts
index e82af72f1aaf..e043a27f7612 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts
+++ b/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts
@@ -18,3 +18,20 @@ gpio-restart {
priority = <224>;
};
};
+
+&gmac {
+ phy-handle = <&phy>;
+ phy-mode = "rgmii-id";
+ status = "okay";
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "snps,dwmac-mdio";
+
+ phy: ethernet-phy@0 {
+ reg = <0>;
+ rx-internal-delay-ps = <900>;
+ };
+ };
+};
--
2.42.0
Add pinmux configuration for the DWMAC found on the JH7100 based boards.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
.../boot/dts/starfive/jh7100-common.dtsi | 76 +++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
index 504c73f01f14..b556e28069c4 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
@@ -65,7 +65,83 @@ soc {
};
};
+&gmac {
+ pinctrl-names = "default";
+ pinctrl-0 = <&gmac_pins>;
+};
+
&gpio {
+ gmac_pins: gmac-0 {
+ gtxclk-pins {
+ pins = <PAD_FUNC_SHARE(115)>;
+ bias-pull-up;
+ drive-strength = <35>;
+ input-enable;
+ input-schmitt-enable;
+ slew-rate = <0>;
+ };
+ miitxclk-pins {
+ pins = <PAD_FUNC_SHARE(116)>;
+ bias-pull-up;
+ drive-strength = <14>;
+ input-enable;
+ input-schmitt-disable;
+ slew-rate = <0>;
+ };
+ tx-pins {
+ pins = <PAD_FUNC_SHARE(117)>,
+ <PAD_FUNC_SHARE(119)>,
+ <PAD_FUNC_SHARE(120)>,
+ <PAD_FUNC_SHARE(121)>,
+ <PAD_FUNC_SHARE(122)>,
+ <PAD_FUNC_SHARE(123)>,
+ <PAD_FUNC_SHARE(124)>,
+ <PAD_FUNC_SHARE(125)>,
+ <PAD_FUNC_SHARE(126)>;
+ bias-pull-up;
+ drive-strength = <35>;
+ input-disable;
+ input-schmitt-disable;
+ slew-rate = <0>;
+ };
+ rxclk-pins {
+ pins = <PAD_FUNC_SHARE(127)>;
+ bias-pull-up;
+ drive-strength = <14>;
+ input-enable;
+ input-schmitt-disable;
+ slew-rate = <6>;
+ };
+ rxer-pins {
+ pins = <PAD_FUNC_SHARE(129)>;
+ bias-pull-up;
+ drive-strength = <14>;
+ input-enable;
+ input-schmitt-disable;
+ slew-rate = <0>;
+ };
+ rx-pins {
+ pins = <PAD_FUNC_SHARE(128)>,
+ <PAD_FUNC_SHARE(130)>,
+ <PAD_FUNC_SHARE(131)>,
+ <PAD_FUNC_SHARE(132)>,
+ <PAD_FUNC_SHARE(133)>,
+ <PAD_FUNC_SHARE(134)>,
+ <PAD_FUNC_SHARE(135)>,
+ <PAD_FUNC_SHARE(136)>,
+ <PAD_FUNC_SHARE(137)>,
+ <PAD_FUNC_SHARE(138)>,
+ <PAD_FUNC_SHARE(139)>,
+ <PAD_FUNC_SHARE(140)>,
+ <PAD_FUNC_SHARE(141)>;
+ bias-pull-up;
+ drive-strength = <14>;
+ input-enable;
+ input-schmitt-enable;
+ slew-rate = <0>;
+ };
+ };
+
i2c0_pins: i2c0-0 {
i2c-pins {
pinmux = <GPIOMUX(62, GPO_LOW,
--
2.42.0
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 <[email protected]>
---
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";
+};
--
2.42.0
On 29/10/2023 05:27, Cristian Ciocaltea wrote:
> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
> similar to the newer JH7110, but it requires only two interrupts and a
> single reset line.
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
> .../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------
> 2 files changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index a4d7172ea701..c1380ff1c054 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -95,6 +95,7 @@ properties:
> - snps,dwmac-5.20
> - snps,dwxgmac
> - snps,dwxgmac-2.10
> + - starfive,jh7100-dwmac
> - starfive,jh7110-dwmac
>
> reg:
> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> index 44e58755a5a2..70e35a3401f4 100644
> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> @@ -13,10 +13,14 @@ maintainers:
>
> properties:
> compatible:
> - items:
> - - enum:
> - - starfive,jh7110-dwmac
> - - const: snps,dwmac-5.20
> + oneOf:
> + - items:
> + - const: starfive,jh7100-dwmac
> + - const: snps,dwmac
> + - items:
> + - enum:
> + - starfive,jh7110-dwmac
> + - const: snps,dwmac-5.20
Why do you use different fallback?
>
> reg:
> maxItems: 1
> @@ -37,23 +41,6 @@ properties:
> - const: tx
> - const: gtx
>
> - interrupts:
> - minItems: 3
> - maxItems: 3
> -
> - interrupt-names:
> - minItems: 3
> - maxItems: 3
> -
> - resets:
> - minItems: 2
> - maxItems: 2
You just changed it in previous patches... So the previous code allowing
one item was correct?
Best regards,
Krzysztof
On Sun, Oct 29, 2023 at 06:27:11AM +0200, Cristian Ciocaltea wrote:
> The StarFive VisionFive V1 SBC has a Motorcomm YT8521 PHY supporting
> RGMII-ID, but requires manual adjustment of the RX internal delay to
> work properly.
>
> The default RX delay provided by the driver is 1.95 ns, which proves to
> be too high. Applying a 50% reduction seems to mitigate the issue.
I'm not so happy this cannot be explained. You are potentially heading
into horrible backwards compatibility problems with old DT blobs and
new kernels once this is explained and fixed.
Andrew
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.
Andrew
On 10/29/23 13:24, Krzysztof Kozlowski wrote:
> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>> similar to the newer JH7110, but it requires only two interrupts and a
>> single reset line.
>>
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
>> .../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------
>> 2 files changed, 54 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> index a4d7172ea701..c1380ff1c054 100644
>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> @@ -95,6 +95,7 @@ properties:
>> - snps,dwmac-5.20
>> - snps,dwxgmac
>> - snps,dwxgmac-2.10
>> + - starfive,jh7100-dwmac
>> - starfive,jh7110-dwmac
>>
>> reg:
>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>> index 44e58755a5a2..70e35a3401f4 100644
>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>> @@ -13,10 +13,14 @@ maintainers:
>>
>> properties:
>> compatible:
>> - items:
>> - - enum:
>> - - starfive,jh7110-dwmac
>> - - const: snps,dwmac-5.20
>> + oneOf:
>> + - items:
>> + - const: starfive,jh7100-dwmac
>> + - const: snps,dwmac
>> + - items:
>> + - enum:
>> + - starfive,jh7110-dwmac
>> + - const: snps,dwmac-5.20
>
> Why do you use different fallback?
AFAIK, dwmac-5.20 is currently only used by JH7110.
>>
>> reg:
>> maxItems: 1
>> @@ -37,23 +41,6 @@ properties:
>> - const: tx
>> - const: gtx
>>
>> - interrupts:
>> - minItems: 3
>> - maxItems: 3
>> -
>> - interrupt-names:
>> - minItems: 3
>> - maxItems: 3
>> -
>> - resets:
>> - minItems: 2
>> - maxItems: 2
>
> You just changed it in previous patches... So the previous code allowing
> one item was correct?
I mentioned the possible use-cases in the previous email. So yes, JH7110
requires 2 resets, while JH7100 just one.
Regards,
Cristian
On 10/29/23 20:45, Andrew Lunn wrote:
> On Sun, Oct 29, 2023 at 06:27:11AM +0200, Cristian Ciocaltea wrote:
>> The StarFive VisionFive V1 SBC has a Motorcomm YT8521 PHY supporting
>> RGMII-ID, but requires manual adjustment of the RX internal delay to
>> work properly.
>>
>> The default RX delay provided by the driver is 1.95 ns, which proves to
>> be too high. Applying a 50% reduction seems to mitigate the issue.
>
> I'm not so happy this cannot be explained. You are potentially heading
> into horrible backwards compatibility problems with old DT blobs and
> new kernels once this is explained and fixed.
It seems the visionfive-v2 board also required setting some delays, but
unfortunately no details were provided:
0104340a67b1 ("riscv: dts: starfive: visionfive 2: Add configuration of
mac and phy")
Thanks,
Cristian
On Mon, Oct 30, 2023 at 12:41:23AM +0200, Cristian Ciocaltea wrote:
> On 10/29/23 20:45, Andrew Lunn wrote:
> > On Sun, Oct 29, 2023 at 06:27:11AM +0200, Cristian Ciocaltea wrote:
> >> The StarFive VisionFive V1 SBC has a Motorcomm YT8521 PHY supporting
> >> RGMII-ID, but requires manual adjustment of the RX internal delay to
> >> work properly.
> >>
> >> The default RX delay provided by the driver is 1.95 ns, which proves to
> >> be too high. Applying a 50% reduction seems to mitigate the issue.
> >
> > I'm not so happy this cannot be explained. You are potentially heading
> > into horrible backwards compatibility problems with old DT blobs and
> > new kernels once this is explained and fixed.
>
> It seems the visionfive-v2 board also required setting some delays, but
> unfortunately no details were provided:
>
> 0104340a67b1 ("riscv: dts: starfive: visionfive 2: Add configuration of
> mac and phy")
That board also uses a YT8531 PHY. Its possible this is somehow to do
with the PHY. Which is why testing with the Microchip PHY is
important. That should answer the question is it a SoC or a PHY
problem.
Andrew
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 10/30/23 00:50, Andrew Lunn wrote:
> On Mon, Oct 30, 2023 at 12:41:23AM +0200, Cristian Ciocaltea wrote:
>> On 10/29/23 20:45, Andrew Lunn wrote:
>>> On Sun, Oct 29, 2023 at 06:27:11AM +0200, Cristian Ciocaltea wrote:
>>>> The StarFive VisionFive V1 SBC has a Motorcomm YT8521 PHY supporting
>>>> RGMII-ID, but requires manual adjustment of the RX internal delay to
>>>> work properly.
>>>>
>>>> The default RX delay provided by the driver is 1.95 ns, which proves to
>>>> be too high. Applying a 50% reduction seems to mitigate the issue.
>>>
>>> I'm not so happy this cannot be explained. You are potentially heading
>>> into horrible backwards compatibility problems with old DT blobs and
>>> new kernels once this is explained and fixed.
>>
>> It seems the visionfive-v2 board also required setting some delays, but
>> unfortunately no details were provided:
>>
>> 0104340a67b1 ("riscv: dts: starfive: visionfive 2: Add configuration of
>> mac and phy")
>
> That board also uses a YT8531 PHY. Its possible this is somehow to do
> with the PHY. Which is why testing with the Microchip PHY is
> important. That should answer the question is it a SoC or a PHY
> problem.
There is also YT8531S, which looks compatible with YT8521, but YT8531
seems to be a bit different. Regardless of what VisionFive v2 is using,
it would be indeed interesting to find out how the Microchip PHY behaves
in this context.
Regards,
Cristian
On Sun, 29 Oct 2023 06:27:04 +0200, Cristian Ciocaltea wrote:
> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
> similar to the newer JH7110, but it requires only two interrupts and a
> single reset line.
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
> .../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------
> 2 files changed, 54 insertions(+), 21 deletions(-)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: compatible: 'oneOf' conditional failed, one must be fixed:
'starfive,jh7100-dwmac' was expected
'amlogic,meson-gxbb-dwmac' is not one of ['starfive,jh7110-dwmac']
'snps,dwmac-5.20' was expected
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: reg: [[3376480256, 65536], [3364046144, 8]] is too long
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clocks: [[4294967295], [4294967295], [4294967295], [4294967295]] is too short
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names:1: 'pclk' was expected
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names:2: 'ptp_ref' was expected
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names:3: 'tx' was expected
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names: ['stmmaceth', 'clkin0', 'clkin1', 'timing-adjustment'] is too short
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: 'resets' is a required property
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: 'reset-names' is a required property
from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On 30/10/2023 02:37, Rob Herring wrote:
>
> On Sun, 29 Oct 2023 06:27:04 +0200, Cristian Ciocaltea wrote:
>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>> similar to the newer JH7110, but it requires only two interrupts and a
>> single reset line.
>>
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
>> .../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------
>> 2 files changed, 54 insertions(+), 21 deletions(-)
>>
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: compatible: 'oneOf' conditional failed, one must be fixed:
> 'starfive,jh7100-dwmac' was expected
> 'amlogic,meson-gxbb-dwmac' is not one of ['starfive,jh7110-dwmac']
> 'snps,dwmac-5.20' was expected
> from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
And here you have proof of missing testing of patch #2.
Best regards,
Krzysztof
On 29/10/2023 23:15, Cristian Ciocaltea wrote:
> On 10/29/23 13:24, Krzysztof Kozlowski wrote:
>> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>>> similar to the newer JH7110, but it requires only two interrupts and a
>>> single reset line.
>>>
>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>> ---
>>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
>>> .../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------
>>> 2 files changed, 54 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> index a4d7172ea701..c1380ff1c054 100644
>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> @@ -95,6 +95,7 @@ properties:
>>> - snps,dwmac-5.20
>>> - snps,dwxgmac
>>> - snps,dwxgmac-2.10
>>> + - starfive,jh7100-dwmac
>>> - starfive,jh7110-dwmac
>>>
>>> reg:
>>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>> index 44e58755a5a2..70e35a3401f4 100644
>>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>> @@ -13,10 +13,14 @@ maintainers:
>>>
>>> properties:
>>> compatible:
>>> - items:
>>> - - enum:
>>> - - starfive,jh7110-dwmac
>>> - - const: snps,dwmac-5.20
>>> + oneOf:
>>> + - items:
>>> + - const: starfive,jh7100-dwmac
>>> + - const: snps,dwmac
>>> + - items:
>>> + - enum:
>>> + - starfive,jh7110-dwmac
>>> + - const: snps,dwmac-5.20
>>
>> Why do you use different fallback?
>
> AFAIK, dwmac-5.20 is currently only used by JH7110.
What is used by JH7000?
>
>>>
>>> reg:
>>> maxItems: 1
>>> @@ -37,23 +41,6 @@ properties:
>>> - const: tx
>>> - const: gtx
>>>
>>> - interrupts:
>>> - minItems: 3
>>> - maxItems: 3
>>> -
>>> - interrupt-names:
>>> - minItems: 3
>>> - maxItems: 3
>>> -
>>> - resets:
>>> - minItems: 2
>>> - maxItems: 2
>>
>> You just changed it in previous patches... So the previous code allowing
>> one item was correct?
>
> I mentioned the possible use-cases in the previous email. So yes, JH7110
> requires 2 resets, while JH7100 just one.
Your previous patch does not make sense now...
Best regards,
Krzysztof
On 10/30/23 09:30, Krzysztof Kozlowski wrote:
> On 29/10/2023 23:15, Cristian Ciocaltea wrote:
>> On 10/29/23 13:24, Krzysztof Kozlowski wrote:
>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>>>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>>>> similar to the newer JH7110, but it requires only two interrupts and a
>>>> single reset line.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 +
>>>> .../bindings/net/starfive,jh7110-dwmac.yaml | 74 +++++++++++++------
>>>> 2 files changed, 54 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>> index a4d7172ea701..c1380ff1c054 100644
>>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>> @@ -95,6 +95,7 @@ properties:
>>>> - snps,dwmac-5.20
>>>> - snps,dwxgmac
>>>> - snps,dwxgmac-2.10
>>>> + - starfive,jh7100-dwmac
>>>> - starfive,jh7110-dwmac
>>>>
>>>> reg:
>>>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>>> index 44e58755a5a2..70e35a3401f4 100644
>>>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>>> @@ -13,10 +13,14 @@ maintainers:
>>>>
>>>> properties:
>>>> compatible:
>>>> - items:
>>>> - - enum:
>>>> - - starfive,jh7110-dwmac
>>>> - - const: snps,dwmac-5.20
>>>> + oneOf:
>>>> + - items:
>>>> + - const: starfive,jh7100-dwmac
>>>> + - const: snps,dwmac
>>>> + - items:
>>>> + - enum:
>>>> + - starfive,jh7110-dwmac
>>>> + - const: snps,dwmac-5.20
>>>
>>> Why do you use different fallback?
>>
>> AFAIK, dwmac-5.20 is currently only used by JH7110.
>
> What is used by JH7000?
Driver reports "Synopsys ID: 0x37", so it could be 3.70a or 3.710, as
those are the only compatibles available for 3.7x.
It's worth noting the driver does not rely on the compatibles for
implementing version specific logic, as it gets the IDs directly from
chip registers.
The usage of generic snps,dwmac fallback was borrowed from downstream code.
Regards,
Cristian
Cristian Ciocaltea wrote:
> Add a missing quirk to enable support for the StarFive JH7100 SoC.
>
> Additionally, for greater flexibility in operation, allow using the
> rgmii-rxid and rgmii-txid phy modes.
>
> Co-developed-by: Emil Renner Berthing <[email protected]>
> Signed-off-by: Emil Renner Berthing <[email protected]>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
Hi Cristian,
Thanks for working on this! This driver has code to update the phy clock for
different line speeds. I don't think that will work without the
CLK_SET_RATE_PARENT flag added to the clock in [1] which in turn depends on
[2].
[1]: https://github.com/esmil/linux/commit/b200c3054b58a49ba25af67aff82d9045e3c3666
[2]: https://github.com/esmil/linux/commit/dce189542c16bf0eb8533d96c0305cb59d149dae
Two more comments below..
> ---
> drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 ++--
> .../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 ++++++++++++++++---
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index a2b9e289aa36..c3c2c8360047 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -165,9 +165,9 @@ config DWMAC_STARFIVE
> help
> Support for ethernet controllers on StarFive RISC-V SoCs
>
> - This selects the StarFive platform specific glue layer support for
> - the stmmac device driver. This driver is used for StarFive JH7110
> - ethernet controller.
> + This selects the StarFive platform specific glue layer support
> + for the stmmac device driver. This driver is used for the
> + StarFive JH7100 and JH7110 ethernet controllers.
>
> config DWMAC_STI
> tristate "STi GMAC support"
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> index 5d630affb4d1..88c431edcea0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> @@ -15,13 +15,20 @@
>
> #include "stmmac_platform.h"
>
> -#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
> -#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
> -#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
> +#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
> +#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
> +#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
> +
> +#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8
> +
> +struct starfive_dwmac_data {
> + unsigned int gtxclk_dlychain;
> +};
>
> struct starfive_dwmac {
> struct device *dev;
> struct clk *clk_tx;
> + const struct starfive_dwmac_data *data;
> };
>
> static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
> @@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
>
> case PHY_INTERFACE_MODE_RGMII:
> case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> mode = STARFIVE_DWMAC_PHY_INFT_RGMII;
> break;
>
> @@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
> if (err)
> return dev_err_probe(dwmac->dev, err, "error setting phy mode\n");
>
> + if (dwmac->data) {
I think you want something like this so future quirks don't need to touch this
code:
if (dwmac->data && dwmac->data->gtxclk_dlychain)
> + err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN,
> + dwmac->data->gtxclk_dlychain);
> + if (err)
> + return dev_err_probe(dwmac->dev, err,
> + "error selecting gtxclk delay chain\n");
> + }
> +
> return 0;
> }
>
> @@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
> if (!dwmac)
> return -ENOMEM;
>
> + dwmac->data = device_get_match_data(&pdev->dev);
> +
> dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
> if (IS_ERR(dwmac->clk_tx))
> return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
> @@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
> return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> }
>
> +static const struct starfive_dwmac_data jh7100_data = {
> + .gtxclk_dlychain = 4
Please add a , at the end of this line. I know it's unlikely that we need to
add more properties, but it's still good practice to do. This way such patches
won't need to touch this line.
> +};
> +
> static const struct of_device_id starfive_dwmac_match[] = {
> - { .compatible = "starfive,jh7110-dwmac" },
> + { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data },
> + { .compatible = "starfive,jh7110-dwmac" },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, starfive_dwmac_match);
> --
> 2.42.0
>
Cristian Ciocaltea wrote:
> Provide a DT node for the SiFive Composable Cache controller found on
> the StarFive JH7100 SoC.
>
> Note this is also used to support non-coherent DMA, via the
> sifive,cache-ops cache flushing operations.
This property is no longer needed:
https://lore.kernel.org/linux-riscv/[email protected]/
Also it would be nice to mention that these nodes are copied from my
visionfive patches ;)
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> arch/riscv/boot/dts/starfive/jh7100.dtsi | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi
> index 06bb157ce111..a8a5bb00b0d8 100644
> --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi
> @@ -32,6 +32,7 @@ U74_0: cpu@0 {
> i-tlb-sets = <1>;
> i-tlb-size = <32>;
> mmu-type = "riscv,sv39";
> + next-level-cache = <&ccache>;
> riscv,isa = "rv64imafdc";
> riscv,isa-base = "rv64i";
> riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr",
> @@ -60,6 +61,7 @@ U74_1: cpu@1 {
> i-tlb-sets = <1>;
> i-tlb-size = <32>;
> mmu-type = "riscv,sv39";
> + next-level-cache = <&ccache>;
> riscv,isa = "rv64imafdc";
> riscv,isa-base = "rv64i";
> riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr",
> @@ -147,6 +149,18 @@ soc {
> dma-noncoherent;
> ranges;
>
> + ccache: cache-controller@2010000 {
> + compatible = "starfive,jh7100-ccache", "sifive,ccache0", "cache";
> + reg = <0x0 0x2010000 0x0 0x1000>;
> + interrupts = <128>, <130>, <131>, <129>;
> + cache-block-size = <64>;
> + cache-level = <2>;
> + cache-sets = <2048>;
> + cache-size = <2097152>;
> + cache-unified;
> + sifive,cache-ops;
> + };
> +
> clint: clint@2000000 {
> compatible = "starfive,jh7100-clint", "sifive,clint0";
> reg = <0x0 0x2000000 0x0 0x10000>;
> --
> 2.42.0
>
On 10/31/23 16:33, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> Add a missing quirk to enable support for the StarFive JH7100 SoC.
>>
>> Additionally, for greater flexibility in operation, allow using the
>> rgmii-rxid and rgmii-txid phy modes.
>>
>> Co-developed-by: Emil Renner Berthing <[email protected]>
>> Signed-off-by: Emil Renner Berthing <[email protected]>
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>
> Hi Cristian,
>
> Thanks for working on this! This driver has code to update the phy clock for
> different line speeds. I don't think that will work without the
> CLK_SET_RATE_PARENT flag added to the clock in [1] which in turn depends on
> [2].
>
> [1]: https://github.com/esmil/linux/commit/b200c3054b58a49ba25af67aff82d9045e3c3666
> [2]: https://github.com/esmil/linux/commit/dce189542c16bf0eb8533d96c0305cb59d149dae
>
> Two more comments below..
Hi Emil,
Thanks for the review!
I've been only testing this at 1000 Mbps and so far it seems to work
fine. I will try with different line speeds and report back.
>> ---
>> drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 ++--
>> .../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 ++++++++++++++++---
>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> index a2b9e289aa36..c3c2c8360047 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>> @@ -165,9 +165,9 @@ config DWMAC_STARFIVE
>> help
>> Support for ethernet controllers on StarFive RISC-V SoCs
>>
>> - This selects the StarFive platform specific glue layer support for
>> - the stmmac device driver. This driver is used for StarFive JH7110
>> - ethernet controller.
>> + This selects the StarFive platform specific glue layer support
>> + for the stmmac device driver. This driver is used for the
>> + StarFive JH7100 and JH7110 ethernet controllers.
>>
>> config DWMAC_STI
>> tristate "STi GMAC support"
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>> index 5d630affb4d1..88c431edcea0 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>> @@ -15,13 +15,20 @@
>>
>> #include "stmmac_platform.h"
>>
>> -#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
>> -#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
>> -#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
>> +#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1
>> +#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4
>> +#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U
>> +
>> +#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8
>> +
>> +struct starfive_dwmac_data {
>> + unsigned int gtxclk_dlychain;
>> +};
>>
>> struct starfive_dwmac {
>> struct device *dev;
>> struct clk *clk_tx;
>> + const struct starfive_dwmac_data *data;
>> };
>>
>> static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
>> @@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
>>
>> case PHY_INTERFACE_MODE_RGMII:
>> case PHY_INTERFACE_MODE_RGMII_ID:
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> mode = STARFIVE_DWMAC_PHY_INFT_RGMII;
>> break;
>>
>> @@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
>> if (err)
>> return dev_err_probe(dwmac->dev, err, "error setting phy mode\n");
>>
>> + if (dwmac->data) {
>
> I think you want something like this so future quirks don't need to touch this
> code:
>
> if (dwmac->data && dwmac->data->gtxclk_dlychain)
Yes, but that would prevent having a quirk that explicitly wants to write 0.
I was initially thinking to something more generic, like providing a
list of register-value pairs, but I'm not sure this is going to be ever
needed.
I'm still open to apply the suggested change, though.
>> + err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN,
>> + dwmac->data->gtxclk_dlychain);
>> + if (err)
>> + return dev_err_probe(dwmac->dev, err,
>> + "error selecting gtxclk delay chain\n");
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
>> if (!dwmac)
>> return -ENOMEM;
>>
>> + dwmac->data = device_get_match_data(&pdev->dev);
>> +
>> dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
>> if (IS_ERR(dwmac->clk_tx))
>> return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
>> @@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
>> return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
>> }
>>
>> +static const struct starfive_dwmac_data jh7100_data = {
>> + .gtxclk_dlychain = 4
>
> Please add a , at the end of this line. I know it's unlikely that we need to
> add more properties, but it's still good practice to do. This way such patches
> won't need to touch this line.
Sure, will do.
>> +};
>> +
>> static const struct of_device_id starfive_dwmac_match[] = {
>> - { .compatible = "starfive,jh7110-dwmac" },
>> + { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data },
>> + { .compatible = "starfive,jh7110-dwmac" },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, starfive_dwmac_match);
>> --
>> 2.42.0
>>
On 10/31/23 16:38, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> Provide a DT node for the SiFive Composable Cache controller found on
>> the StarFive JH7100 SoC.
>>
>> Note this is also used to support non-coherent DMA, via the
>> sifive,cache-ops cache flushing operations.
>
> This property is no longer needed:
> https://lore.kernel.org/linux-riscv/[email protected]/
Thanks for the heads up! I actually noticed that from v1 reviews and was
just waiting for v2. :)
> Also it would be nice to mention that these nodes are copied from my
> visionfive patches ;)
Ups, sorry about that! Those were initially taken from a patch adding a
full DT (the repo is mentioned in the cover letter) with many
contributors mentioned, without being clear who did what. That's why I
didn't provide a Co-developed-by tag and, unfortunately, I also missed
to add it in v2 (will handle this in v3 and also provide the link to the
new repo), but I'm still not sure about the gmac stuff.
Thanks,
Cristian
>>
>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>> ---
>> arch/riscv/boot/dts/starfive/jh7100.dtsi | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi
>> index 06bb157ce111..a8a5bb00b0d8 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi
>> @@ -32,6 +32,7 @@ U74_0: cpu@0 {
>> i-tlb-sets = <1>;
>> i-tlb-size = <32>;
>> mmu-type = "riscv,sv39";
>> + next-level-cache = <&ccache>;
>> riscv,isa = "rv64imafdc";
>> riscv,isa-base = "rv64i";
>> riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr",
>> @@ -60,6 +61,7 @@ U74_1: cpu@1 {
>> i-tlb-sets = <1>;
>> i-tlb-size = <32>;
>> mmu-type = "riscv,sv39";
>> + next-level-cache = <&ccache>;
>> riscv,isa = "rv64imafdc";
>> riscv,isa-base = "rv64i";
>> riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr",
>> @@ -147,6 +149,18 @@ soc {
>> dma-noncoherent;
>> ranges;
>>
>> + ccache: cache-controller@2010000 {
>> + compatible = "starfive,jh7100-ccache", "sifive,ccache0", "cache";
>> + reg = <0x0 0x2010000 0x0 0x1000>;
>> + interrupts = <128>, <130>, <131>, <129>;
>> + cache-block-size = <64>;
>> + cache-level = <2>;
>> + cache-sets = <2048>;
>> + cache-size = <2097152>;
>> + cache-unified;
>> + sifive,cache-ops;
>> + };
>> +
>> clint: clint@2000000 {
>> compatible = "starfive,jh7100-clint", "sifive,clint0";
>> reg = <0x0 0x2000000 0x0 0x10000>;
>> --
>> 2.42.0
>>
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?
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?
On 11/16/23 19:55, Conor Dooley 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
> one though. Is there a specific test you need to have done?
As Andrew already pointed out, we'd like to know if networking for this
board works without any further adjustment of the RX internal delay.
This was necessary for VisionFive (see previous PATCH v2 11/12), but the
PHY is different (Motorcomm YT8521), hence this test might help us
understand if there's a potential issue with the SoC or the PHY.
Thanks,
Cristian
On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <[email protected]> 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].
------------[ cut here ]------------
simple-pm-bus soc: device non-coherent but no non-coherent operations supported
WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
arch_setup_dma_ops+0x7e/0xa2
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
Hardware name: BeagleV Starlight Beta (DT)
epc : arch_setup_dma_ops+0x7e/0xa2
ra : arch_setup_dma_ops+0x7e/0xa2
epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b90
gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217238
t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013bc0
s1 : 0000000000000000 a0 : 000000000000004f a1 : 0000000200000020
a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
s2 : ffffffd880111410 s3 : ffffffff812d712c s4 : 00000000ffffffff
s5 : ffffffffffffffed s6 : ffffffd9fffeb530 s7 : ffffffff80a00d70
s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
s11: 0000000000000000 t3 : ffffffff812ebb5c t4 : ffffffff812ebb5c
t5 : ffffffff812ebb38 t6 : ffffffff812ebbb7
status: 0000000200000120 badaddr: ffffffff812d712c cause: 0000000000000003
[<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
[<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
[<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
[<ffffffff803ec382>] really_probe+0x54/0x20c
[<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
[<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
[<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
[<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
[<ffffffff803ebdc8>] driver_attach+0x1a/0x22
[<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
[<ffffffff803ed476>] driver_register+0x3e/0xd8
[<ffffffff803ee590>] __platform_driver_register+0x1c/0x24
[<ffffffff8081db32>] simple_pm_bus_driver_init+0x1a/0x22
[<ffffffff800020f0>] do_one_initcall+0x38/0x17c
[<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
[<ffffffff806b1328>] kernel_init+0x1e/0x112
[<ffffffff806b891e>] ret_from_fork+0xe/0x1c
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------
clk-starfive-jh7100 11800000.clock-controller: device non-coherent but
no non-coherent operations supported
WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
arch_setup_dma_ops+0x7e/0xa2
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
Hardware name: BeagleV Starlight Beta (DT)
epc : arch_setup_dma_ops+0x7e/0xa2
ra : arch_setup_dma_ops+0x7e/0xa2
epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217f48
t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
s1 : 0000000000000000 a0 : 000000000000006b a1 : 0000000200000020
a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
s2 : ffffffd880112410 s3 : ffffffff812d712c s4 : 0000000fffffffff
s5 : 0000000000000000 s6 : ffffffd9fffed3c8 s7 : ffffffff80a00d70
s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
s11: 0000000000000000 t3 : ffffffff812ec564 t4 : ffffffff812ec564
t5 : ffffffff812ec540 t6 : ffffffff812ec5db
status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
[<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
[<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
[<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
[<ffffffff803ec382>] really_probe+0x54/0x20c
[<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
[<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
[<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
[<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
[<ffffffff803ebdc8>] driver_attach+0x1a/0x22
[<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
[<ffffffff803ed476>] driver_register+0x3e/0xd8
[<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
[<ffffffff8081eb80>] clk_starfive_jh7100_driver_init+0x22/0x2a
[<ffffffff800020f0>] do_one_initcall+0x38/0x17c
[<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
[<ffffffff806b1328>] kernel_init+0x1e/0x112
[<ffffffff806b891e>] ret_from_fork+0xe/0x1c
---[ end trace 0000000000000000 ]---
CCACHE: DataFail @ 0x00000000.7FEB8930
CCACHE: 2 banks, 16 ways, sets/bank=1024, bytes/block=64
CCACHE: Index of the largest way enabled: 0
------------[ cut here ]------------
jh7100-reset 11840000.reset-controller: device non-coherent but no
non-coherent operations supported
WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
arch_setup_dma_ops+0x7e/0xa2
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
Hardware name: BeagleV Starlight Beta (DT)
epc : arch_setup_dma_ops+0x7e/0xa2
ra : arch_setup_dma_ops+0x7e/0xa2
epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81218d60
t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
s1 : 0000000000000000 a0 : 0000000000000064 a1 : 0000000200000020
a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
s2 : ffffffd880112c10 s3 : ffffffff812d712c s4 : 0000000fffffffff
s5 : 0000000000000000 s6 : ffffffd9fffed7a8 s7 : ffffffff80a00d70
s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
s11: 0000000000000000 t3 : ffffffff812ed054 t4 : ffffffff812ed054
t5 : ffffffff812ed030 t6 : ffffffff812ed0c4
status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
[<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
[<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
[<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
[<ffffffff803ec382>] really_probe+0x54/0x20c
[<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
[<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
[<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
[<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
[<ffffffff803ebdc8>] driver_attach+0x1a/0x22
[<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
[<ffffffff803ed476>] driver_register+0x3e/0xd8
[<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
[<ffffffff8081f284>] jh7100_reset_driver_init+0x22/0x2a
[<ffffffff800020f0>] do_one_initcall+0x38/0x17c
[<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
[<ffffffff806b1328>] kernel_init+0x1e/0x112
[<ffffffff806b891e>] ret_from_fork+0xe/0x1c
---[ end trace 0000000000000000 ]---
CCACHE: DataFail @ 0x00000000.7FEB0830
CCACHE: DataFail @ 0x00000000.7FEB07F0
CCACHE: DataFail @ 0x00000000.7FEB07F0
CCACHE: DataFail @ 0x00000000.7FEB07F0
CCACHE: DataFail @ 0x00000000.7FEB0830
CCACHE: DataFail @ 0x00000000.7FEB07F0
[...]
Looks like it needs more non-coherent support before we can test
Ethernet.
Note that it boots fine into Debian nfsroot when merging Emil's[2]
visionfive branch instead.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tag/?h=renesas-drivers-2023-11-14-v6.7-rc1
[2] https://github.com/esmil/linux
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 11/17/23 10:37, Geert Uytterhoeven wrote:
> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <[email protected]> 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].
>
> ------------[ cut here ]------------
> simple-pm-bus soc: device non-coherent but no non-coherent operations supported
> WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
> arch_setup_dma_ops+0x7e/0xa2
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
> Hardware name: BeagleV Starlight Beta (DT)
> epc : arch_setup_dma_ops+0x7e/0xa2
> ra : arch_setup_dma_ops+0x7e/0xa2
> epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b90
> gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217238
> t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013bc0
> s1 : 0000000000000000 a0 : 000000000000004f a1 : 0000000200000020
> a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
> a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
> s2 : ffffffd880111410 s3 : ffffffff812d712c s4 : 00000000ffffffff
> s5 : ffffffffffffffed s6 : ffffffd9fffeb530 s7 : ffffffff80a00d70
> s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
> s11: 0000000000000000 t3 : ffffffff812ebb5c t4 : ffffffff812ebb5c
> t5 : ffffffff812ebb38 t6 : ffffffff812ebbb7
> status: 0000000200000120 badaddr: ffffffff812d712c cause: 0000000000000003
> [<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
> [<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
> [<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
> [<ffffffff803ec382>] really_probe+0x54/0x20c
> [<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
> [<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
> [<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
> [<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
> [<ffffffff803ebdc8>] driver_attach+0x1a/0x22
> [<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
> [<ffffffff803ed476>] driver_register+0x3e/0xd8
> [<ffffffff803ee590>] __platform_driver_register+0x1c/0x24
> [<ffffffff8081db32>] simple_pm_bus_driver_init+0x1a/0x22
> [<ffffffff800020f0>] do_one_initcall+0x38/0x17c
> [<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
> [<ffffffff806b1328>] kernel_init+0x1e/0x112
> [<ffffffff806b891e>] ret_from_fork+0xe/0x1c
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> clk-starfive-jh7100 11800000.clock-controller: device non-coherent but
> no non-coherent operations supported
> WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
> arch_setup_dma_ops+0x7e/0xa2
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
> 6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
> Hardware name: BeagleV Starlight Beta (DT)
> epc : arch_setup_dma_ops+0x7e/0xa2
> ra : arch_setup_dma_ops+0x7e/0xa2
> epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
> gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217f48
> t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
> s1 : 0000000000000000 a0 : 000000000000006b a1 : 0000000200000020
> a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
> a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
> s2 : ffffffd880112410 s3 : ffffffff812d712c s4 : 0000000fffffffff
> s5 : 0000000000000000 s6 : ffffffd9fffed3c8 s7 : ffffffff80a00d70
> s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
> s11: 0000000000000000 t3 : ffffffff812ec564 t4 : ffffffff812ec564
> t5 : ffffffff812ec540 t6 : ffffffff812ec5db
> status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
> [<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
> [<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
> [<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
> [<ffffffff803ec382>] really_probe+0x54/0x20c
> [<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
> [<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
> [<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
> [<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
> [<ffffffff803ebdc8>] driver_attach+0x1a/0x22
> [<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
> [<ffffffff803ed476>] driver_register+0x3e/0xd8
> [<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
> [<ffffffff8081eb80>] clk_starfive_jh7100_driver_init+0x22/0x2a
> [<ffffffff800020f0>] do_one_initcall+0x38/0x17c
> [<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
> [<ffffffff806b1328>] kernel_init+0x1e/0x112
> [<ffffffff806b891e>] ret_from_fork+0xe/0x1c
> ---[ end trace 0000000000000000 ]---
> CCACHE: DataFail @ 0x00000000.7FEB8930
> CCACHE: 2 banks, 16 ways, sets/bank=1024, bytes/block=64
> CCACHE: Index of the largest way enabled: 0
> ------------[ cut here ]------------
> jh7100-reset 11840000.reset-controller: device non-coherent but no
> non-coherent operations supported
> WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
> arch_setup_dma_ops+0x7e/0xa2
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
> 6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
> Hardware name: BeagleV Starlight Beta (DT)
> epc : arch_setup_dma_ops+0x7e/0xa2
> ra : arch_setup_dma_ops+0x7e/0xa2
> epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
> gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81218d60
> t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
> s1 : 0000000000000000 a0 : 0000000000000064 a1 : 0000000200000020
> a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
> a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
> s2 : ffffffd880112c10 s3 : ffffffff812d712c s4 : 0000000fffffffff
> s5 : 0000000000000000 s6 : ffffffd9fffed7a8 s7 : ffffffff80a00d70
> s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
> s11: 0000000000000000 t3 : ffffffff812ed054 t4 : ffffffff812ed054
> t5 : ffffffff812ed030 t6 : ffffffff812ed0c4
> status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
> [<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
> [<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
> [<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
> [<ffffffff803ec382>] really_probe+0x54/0x20c
> [<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
> [<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
> [<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
> [<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
> [<ffffffff803ebdc8>] driver_attach+0x1a/0x22
> [<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
> [<ffffffff803ed476>] driver_register+0x3e/0xd8
> [<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
> [<ffffffff8081f284>] jh7100_reset_driver_init+0x22/0x2a
> [<ffffffff800020f0>] do_one_initcall+0x38/0x17c
> [<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
> [<ffffffff806b1328>] kernel_init+0x1e/0x112
> [<ffffffff806b891e>] ret_from_fork+0xe/0x1c
> ---[ end trace 0000000000000000 ]---
> CCACHE: DataFail @ 0x00000000.7FEB0830
> CCACHE: DataFail @ 0x00000000.7FEB07F0
> CCACHE: DataFail @ 0x00000000.7FEB07F0
> CCACHE: DataFail @ 0x00000000.7FEB07F0
> CCACHE: DataFail @ 0x00000000.7FEB0830
> CCACHE: DataFail @ 0x00000000.7FEB07F0
>
> [...]
>
> 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
Regards,
Cristian
> Note that it boots fine into Debian nfsroot when merging Emil's[2]
> visionfive branch instead.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tag/?h=renesas-drivers-2023-11-14-v6.7-rc1
> [2] https://github.com/esmil/linux
>
> Gr{oetje,eeting}s,
>
> Geert
>
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 <[email protected]> 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
Also please note the series requires the SiFive Composable Cache
controller patches provided by Emil [1].
[1]: https://lore.kernel.org/all/[email protected]/
Hi Cristian,
On Fri, Nov 17, 2023 at 9:59 AM Cristian Ciocaltea
<[email protected]> 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 <[email protected]> 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/[email protected]/
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 11/17/23 11:12, Geert Uytterhoeven wrote:
> Hi Cristian,
>
> On Fri, Nov 17, 2023 at 9:59 AM Cristian Ciocaltea
> <[email protected]> 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 <[email protected]> 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/[email protected]/
>> Also please note the series requires the SiFive Composable Cache
>> controller patches provided by Emil [1].
>>
>> [1]: https://lore.kernel.org/all/[email protected]/
>
> 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.
Thanks,
Cristian
> 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
>> <[email protected]> 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 <[email protected]> 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/[email protected]/
>
>>> Also please note the series requires the SiFive Composable Cache
>>> controller patches provided by Emil [1].
>>>
>>> [1]: https://lore.kernel.org/all/[email protected]/
>>
>> 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:
> 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. So I wonder why
you need that on your board
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?
You've alse removed the phy reset gpio on the Starlight board:
snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
Why?
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
> ---
> 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?
/Emil
> --
> 2.42.0
>
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.
> 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 <[email protected]>
>> ---
>> 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".
Thanks,
Cristian
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.
>
> > 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 <[email protected]>
> >> ---
> >> 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.
Thanks!
/Emil
>
> Thanks,
> Cristian
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 <[email protected]>
>>>> ---
>>>> 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].
Thanks again for your support,
Cristian
[1]: https://lore.kernel.org/all/[email protected]/
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 <[email protected]>
> >>>> ---
> >>>> 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.
/Emil
>
> Thanks again for your support,
> Cristian
>
> [1]: https://lore.kernel.org/all/[email protected]/
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 <[email protected]>
>>>>>> ---
>>>>>> 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?
> /Emil
>
>>
>> Thanks again for your support,
>> Cristian
>>
>> [1]: https://lore.kernel.org/all/[email protected]/
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 <[email protected]>
> >>>>>> ---
> >>>>>> 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/[email protected]/
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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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/[email protected]/
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/[email protected]/
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/[email protected]/
>
> 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!