Message ID | 20221201090242.2381-1-yanhong.wang@starfivetech.com |
---|---|
Headers | show |
Series | Add Ethernet driver for StarFive JH7110 SoC | expand |
On 2022/12/2 0:18, Krzysztof Kozlowski wrote: > On 01/12/2022 10:02, Yanhong Wang wrote: >> Add dwmac-5.20 version to snps.dwmac.yaml > > Drop full stop from subject and add it here instead. > Will update in the next version. >> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> > > Two people contributed this one single line? > Emil made this patch and I submitted it. > Best regards, > Krzysztof >
On 2022/12/2 0:21, Krzysztof Kozlowski wrote: > On 01/12/2022 10:02, Yanhong Wang wrote: >> Add bindings for the StarFive dwmac module on the StarFive RISC-V SoCs. > > Subject: drop second, redundant "bindings". > I'll fix the title in the next version. >> >> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> >> --- >> .../devicetree/bindings/net/snps,dwmac.yaml | 1 + >> .../bindings/net/starfive,dwmac-plat.yaml | 106 ++++++++++++++++++ >> MAINTAINERS | 5 + >> 3 files changed, 112 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/starfive,dwmac-plat.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> index d8779d3de3d6..13c5928d7170 100644 >> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >> @@ -33,6 +33,7 @@ select: >> - snps,dwmac-5.20 >> - snps,dwxgmac >> - snps,dwxgmac-2.10 >> + - starfive,dwmac >> >> # Deprecated >> - st,spear600-gmac >> diff --git a/Documentation/devicetree/bindings/net/starfive,dwmac-plat.yaml b/Documentation/devicetree/bindings/net/starfive,dwmac-plat.yaml >> new file mode 100644 >> index 000000000000..561cf2a713ab >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/starfive,dwmac-plat.yaml > > Filename should base on compatible. > Will update file name. >> @@ -0,0 +1,106 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +# Copyright (C) 2022 StarFive Technology Co., Ltd. >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/net/dwmac-starfive.yaml#" > > Does not look like you tested the bindings. Please run `make > dt_binding_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > Drop quotes from both lines. > Will remove quotes. >> + >> +title: StarFive DWMAC glue layer >> + >> +maintainers: >> + - Yanhong Wang <yanhong.wang@starfivetech.com> >> + >> +select: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - starfive,dwmac >> + required: >> + - compatible >> + >> +allOf: >> + - $ref: "snps,dwmac.yaml#" > > Drop quotes. > Will remove quotes. >> + >> +properties: >> + compatible: >> + oneOf: > > Drop oneOf. You do not have more cases here. > Will remove oneOf. >> + - items: >> + - enum: >> + - starfive,dwmac > > Wrong indentation.... kind of expected since you did not test the bindings. > Will fix. >> + - const: snps,dwmac-5.20 >> + >> + clocks: >> + items: >> + - description: GMAC main clock >> + - description: GMAC AHB clock >> + - description: PTP clock >> + - description: TX clock >> + - description: GTXC clock >> + - description: GTX clock >> + >> + clock-names: >> + contains: >> + enum: >> + - stmmaceth >> + - pclk >> + - ptp_ref >> + - tx >> + - gtxc >> + - gtx > > Names should be specific and with fixed order, just like clocks are. > Will fix. >> + >> +required: >> + - compatible >> + - clocks >> + - clock-names >> + - resets >> + - reset-names >> + >> +unevaluatedProperties: false >> + > > Best regards, > Krzysztof >
On 2022/12/2 1:35, Conor Dooley wrote: > On Thu, Dec 01, 2022 at 05:21:04PM +0100, Krzysztof Kozlowski wrote: >> On 01/12/2022 10:02, Yanhong Wang wrote: >> > Add bindings for the StarFive dwmac module on the StarFive RISC-V SoCs. >> >> Subject: drop second, redundant "bindings". >> >> > >> > Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> >> > --- >> > .../devicetree/bindings/net/snps,dwmac.yaml | 1 + >> > +properties: >> > + compatible: >> > + oneOf: >> >> Drop oneOf. You do not have more cases here. >> >> > + - items: >> > + - enum: >> > + - starfive,dwmac >> >> Wrong indentation.... kind of expected since you did not test the bindings. >> >> > + - const: snps,dwmac-5.20 > > Disclaimer: no familiarity with the version info with DW stuff > > Is it a bit foolish to call this binding "starfive,dwmac"? Could there > not be another StarFive SoC in the future that uses another DW mac IP > version & this would be better off as "starfive,jh7110-dwmac" or similar? > The StarFive JH8100 SoC in the future that uses the same mac IP version, so call this binding "starfive,dwmac". > Thanks, > Conor. >
On 02/12/2022 03:53, yanhong wang wrote: > > > On 2022/12/2 0:18, Krzysztof Kozlowski wrote: >> On 01/12/2022 10:02, Yanhong Wang wrote: >>> Add dwmac-5.20 version to snps.dwmac.yaml >> >> Drop full stop from subject and add it here instead. >> > > Will update in the next version. > >>> >>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >>> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> >> >> Two people contributed this one single line? >> > > Emil made this patch and I submitted it. If Emil made this patch, then your From field is incorrect. Best regards, Krzysztof
On 02/12/2022 08:16, yanhong wang wrote: > > > On 2022/12/2 1:35, Conor Dooley wrote: >> On Thu, Dec 01, 2022 at 05:21:04PM +0100, Krzysztof Kozlowski wrote: >>> On 01/12/2022 10:02, Yanhong Wang wrote: >>>> Add bindings for the StarFive dwmac module on the StarFive RISC-V SoCs. >>> >>> Subject: drop second, redundant "bindings". >>> >>>> >>>> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> >>>> --- >>>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 + >>>> +properties: >>>> + compatible: >>>> + oneOf: >>> >>> Drop oneOf. You do not have more cases here. >>> >>>> + - items: >>>> + - enum: >>>> + - starfive,dwmac >>> >>> Wrong indentation.... kind of expected since you did not test the bindings. >>> >>>> + - const: snps,dwmac-5.20 >> >> Disclaimer: no familiarity with the version info with DW stuff >> >> Is it a bit foolish to call this binding "starfive,dwmac"? Could there >> not be another StarFive SoC in the future that uses another DW mac IP >> version & this would be better off as "starfive,jh7110-dwmac" or similar? >> > > The StarFive JH8100 SoC in the future that uses the same mac IP version, so call this binding "starfive,dwmac". It's not a reason to make compatible generic. Best regards, Krzysztof
On 01/12/2022 17:45, Conor Dooley wrote: > Hey Yanhong, > > On Thu, Dec 01, 2022 at 07:36:29AM -0600, Rob Herring wrote: >> >> On Thu, 01 Dec 2022 17:02:38 +0800, Yanhong Wang wrote: >>> Add bindings for the StarFive dwmac module on the StarFive RISC-V SoCs. >>> >>> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> >>> --- >>> .../devicetree/bindings/net/snps,dwmac.yaml | 1 + >>> .../bindings/net/starfive,dwmac-plat.yaml | 106 ++++++++++++++++++ >>> MAINTAINERS | 5 + >>> 3 files changed, 112 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/net/starfive,dwmac-plat.yaml >>> >> >> 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: >> ./Documentation/devicetree/bindings/net/starfive,dwmac-plat.yaml:30:16: [warning] wrong indentation: expected 14 but found 15 (indentation) >> >> dtschema/dtc warnings/errors: >> ./Documentation/devicetree/bindings/net/starfive,dwmac-plat.yaml: $id: relative path/filename doesn't match actual path or filename >> expected: http://devicetree.org/schemas/net/starfive,dwmac-plat.yaml# >> Documentation/devicetree/bindings/net/starfive,dwmac-plat.example.dts:21:18: fatal error: dt-bindings/clock/starfive-jh7110.h: No such file or directory >> 21 | #include <dt-bindings/clock/starfive-jh7110.h> > > Perhaps, rather than putting a long list of "prerequisite-patch-id" in > your cover letters etc, you drop the need for headers from your bindings > entirely? Otherwise, you need to wait for the clock bindings to be applied > before any of your other peripherals etc can have drivers/bindings upstream. > > AFAIU, and Rob/Krzk please correct me, the example in a dt-binding > really is an *example* and there's no requirement for it to match the > jh7110 dts exactly. Because of that you can drop the header & just do > something like `clocks = <&clk 7>, <&clk 77>;` etc and the example is > still valid. Same goes for all of the other driver patchsets for new > StarFive stuff, like the pmu or crypto, that also have dt-bindings. > > The only person who has to worry then about dependencies is me when I > apply the .dts patches :) It's nice when example looks good and matching final DTS, but that's not a requirement. Pretty often clocks/resets/domains are mocked for the example. Best regards, Krzysztof
On 2022/12/1 23:40, Andrew Lunn wrote: >> +static const struct ytphy_reg_field ytphy_rxtxd_grp[] = { >> + { "rx_delay_sel", GENMASK(13, 10), 0x0 }, >> + { "tx_delay_sel_fe", GENMASK(7, 4), 0xf }, >> + { "tx_delay_sel", GENMASK(3, 0), 0x1 } >> +}; >> + >> +static const struct ytphy_reg_field ytphy_txinver_grp[] = { >> + { "tx_inverted_1000", BIT(14), 0x0 }, >> + { "tx_inverted_100", BIT(14), 0x0 }, >> + { "tx_inverted_10", BIT(14), 0x0 } >> +}; >> + >> +static const struct ytphy_reg_field ytphy_rxden_grp[] = { >> + { "rxc_dly_en", BIT(8), 0x1 } >> +}; >> + >> +static int ytphy_config_init(struct phy_device *phydev) >> +{ >> + struct device_node *of_node; >> + u32 val; >> + u32 mask; >> + u32 cfg; >> + int ret; >> + int i = 0; >> + >> + of_node = phydev->mdio.dev.of_node; >> + if (of_node) { >> + ret = of_property_read_u32(of_node, ytphy_rxden_grp[0].name, &cfg); > > You need to document the device tree binding. > > Frank Sae always gets the locking wrong in this driver. Have you > tested your patch with lockdep enabled? > I will add document to describe details in the next version, such as "rx_delay_sel","tx_inverted_10" etc. > Andrew
On Fri, 2 Dec 2022 at 09:04, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 02/12/2022 03:53, yanhong wang wrote: > > > > > > On 2022/12/2 0:18, Krzysztof Kozlowski wrote: > >> On 01/12/2022 10:02, Yanhong Wang wrote: > >>> Add dwmac-5.20 version to snps.dwmac.yaml > >> > >> Drop full stop from subject and add it here instead. > >> > > > > Will update in the next version. > > > >>> > >>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > >>> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> > >> > >> Two people contributed this one single line? > >> > > > > Emil made this patch and I submitted it. > > If Emil made this patch, then your From field is incorrect. Yes, please don't change the author of the commits you cherry-picked from my tree. But now I'm curious. Did you check with your colleagues that the dwmac IP on the SoC is in fact version 5.20? This was just an educated guess from my side. /Emil > Best regards, > Krzysztof > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 2022/12/7 21:56, Emil Renner Berthing wrote: > On Fri, 2 Dec 2022 at 09:04, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 02/12/2022 03:53, yanhong wang wrote: >> > >> > >> > On 2022/12/2 0:18, Krzysztof Kozlowski wrote: >> >> On 01/12/2022 10:02, Yanhong Wang wrote: >> >>> Add dwmac-5.20 version to snps.dwmac.yaml >> >> >> >> Drop full stop from subject and add it here instead. >> >> >> > >> > Will update in the next version. >> > >> >>> >> >>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> >>> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> >> >> >> >> Two people contributed this one single line? >> >> >> > >> > Emil made this patch and I submitted it. >> >> If Emil made this patch, then your From field is incorrect. > > Yes, please don't change the author of the commits you cherry-picked > from my tree. > > But now I'm curious. Did you check with your colleagues that the dwmac > IP on the SoC is in fact version 5.20? I can confirm that the IP version is 5.20 on JH7110 SoC. > This was just an educated guess from my side. > > /Emil > >> Best regards, >> Krzysztof >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On 2022/12/2 3:30, Conor Dooley wrote: > On Thu, Dec 01, 2022 at 05:49:08PM +0000, Conor Dooley wrote: >> On Thu, Dec 01, 2022 at 05:02:42PM +0800, Yanhong Wang wrote: >> > riscv: dts: starfive: visionfive-v2: Add phy delay_chain configuration >> > >> > Add phy delay_chain configuration to support motorcomm phy driver for >> > StarFive VisionFive 2 board. > > nit: please re-word this commit next time around to actually say what > you're doing here. I didn't notice it initially, but this patch is doing > a lot more than adding `delay_chain` configuration. To my dwmac unaware > brain, there's nothing hits for that term outside of the changelog :( > I will re-word the commit message and add another dt-binding to describe the details that such as "rxc_dly_en","tx_inverted_10" etc. > Thanks, > Conor. > >> > >> > Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com> >> > --- >> > .../jh7110-starfive-visionfive-v2.dts | 46 +++++++++++++++++++ >> > 1 file changed, 46 insertions(+) >> > >> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> > index c8946cf3a268..2868ef4c74ef 100644 >> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> > @@ -15,6 +15,8 @@ >> > >> > aliases { >> > serial0 = &uart0; >> > + ethernet0=&gmac0; >> > + ethernet1=&gmac1; >> >> Please match the whitespace usage of the existing entry. >> >> > }; >> > >> > chosen { >> > @@ -114,3 +116,47 @@ >> > pinctrl-0 = <&uart0_pins>; >> > status = "okay"; >> > }; >> > + >> > +&gmac0 { >> > + status = "okay"; >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + phy-handle = <&phy0>; >> > + status = "okay"; >> > + mdio0 { >> >> A line of whitespace before the child nodes too please :) >> >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + compatible = "snps,dwmac-mdio"; >> > + phy0: ethernet-phy@0 { >> > + reg = <0>; >> > + rxc_dly_en = <1>; >> > + tx_delay_sel_fe = <5>; >> > + tx_delay_sel = <0xa>; >> > + tx_inverted_10 = <0x1>; >> > + tx_inverted_100 = <0x1>; >> > + tx_inverted_1000 = <0x1>; >> > + }; >> > + }; >> > +}; >> > + >> > +&gmac1 { >> > + status = "okay"; >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + phy-handle = <&phy1>; >> > + status = "okay"; >> > + mdio1 { >> > + #address-cells = <1>; >> > + #size-cells = <0>; >> > + compatible = "snps,dwmac-mdio"; >> > + phy1: ethernet-phy@1 { >> > + reg = <1>; >> > + tx_delay_sel_fe = <5>; >> > + tx_delay_sel = <0>; >> > + rxc_dly_en = <0>; >> > + tx_inverted_10 = <0x1>; >> > + tx_inverted_100 = <0x1>; >> > + tx_inverted_1000 = <0x0>; >> > + }; >> > + }; >> > +}; >> > -- >> > 2.17.1 >> >