| Message ID | 1542882521-18874-3-git-send-email-biao.huang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add Ethernet driver support for mt2712 | expand |
On Thu, Nov 22, 2018 at 06:28:41PM +0800, Biao Huang wrote: > The commit adds the device tree binding documentation for the MediaTek DWMAC > found on MediaTek MT2712. > > Signed-off-by: Biao Huang <biao.huang@mediatek.com> > +Optional properties: > +- mediatek,tx-delay: TX clock delay macro value. Range is 0~31. Default is 0. > + It should be defined for rgmii/rgmii-rxid/mii interface. > +- mediatek,rx-delay: RX clock delay macro value. Range is 0~31. Default is 0. > + It should be defined for rgmii/rgmii-txid/mii/rmii interface. You have received the same feedback at least twice now, from two different maintainers, that the delay should be specified in pS, and the driver should figure out what values to place into registers. You should not ignore feedback like that. If you don't understand the feedback, please ask us to explain it. If you don't agree with the feedback, you need to argue why you think it is wrong, or why what you are doing is better, etc. We are here to help, but just ignoring us won't get you anywhere. For the moment: NACK Andrew
Dear Andrew, Thanks for you remind. Sincerely, I respect any comment from any reviewer. If I didn't reply for any comment, really sorry for that. As to this "tx-delay" issue, the following reply in v3 maybe ignored. https://lkml.org/lkml/2018/11/19/158 "the delay time in mediatek dwmac design is not so accurate, the current mt2712 and the following ICs will not use the same delay design, but will use stages to indicate different delay time. so maybe "mediatek.tx-delay" represent the delay stage is a good choice" And to make it clearer here. In mt2712, there are two delay macro circuit: named fine-tune and coarse-tune. a. fine-tune, 170+/-50ps per stage, total 32 stages b. coarse-tune, 0.55+/-0.2ns per stage, total 32 stages If we only consider mt2712, delay in fine-tune select a integer multiple of 170ps, delay in coarse-tune select a integer multiple of 550ps, for stage 0~31, the delay in fine-tune will not have the same value with that in coarse-tune. OK, It seems the property "fine-tune" can be eliminated . But the following ic will not have the same accuracy as mt2712, and maybe will not have two delay macro circuit to be selected. 1. assume two delay macro circuit in the following ic, fine-tune, 100ps per stage, coarse-tune, 0.55ns per stage, if we want delay 2.2ns, fine-tune will get a 22, and coarse-tune get a 4. We can't distinguish which delay macro we are choosing. 2. assume only one delay macro circuit is used, a similar case as 1 will also increase the complexity of driver. Then, we need define more flag property to know which delay macro we are handling. The common things for all delay macro circuit in MediaTek mac design is the stages, not the accuracy. so if we maintain stage info in "mediatek, tx-delay", we only need care which stage we should choose. And for each IC, we will recommend a best stage as a candidate. Above is my personal opinion, may be my understanding is wrong, welcome for further discussion. Thanks a lot. On Thu, 2018-11-22 at 16:19 +0100, Andrew Lunn wrote: > On Thu, Nov 22, 2018 at 06:28:41PM +0800, Biao Huang wrote: > > The commit adds the device tree binding documentation for the MediaTek DWMAC > > found on MediaTek MT2712. > > > > Signed-off-by: Biao Huang <biao.huang@mediatek.com> > > > +Optional properties: > > +- mediatek,tx-delay: TX clock delay macro value. Range is 0~31. Default is 0. > > + It should be defined for rgmii/rgmii-rxid/mii interface. > > +- mediatek,rx-delay: RX clock delay macro value. Range is 0~31. Default is 0. > > + It should be defined for rgmii/rgmii-txid/mii/rmii interface. > > You have received the same feedback at least twice now, from two > different maintainers, that the delay should be specified in pS, and > the driver should figure out what values to place into registers. > > You should not ignore feedback like that. If you don't understand the > feedback, please ask us to explain it. If you don't agree with the > feedback, you need to argue why you think it is wrong, or why what you > are doing is better, etc. > > We are here to help, but just ignoring us won't get you anywhere. > > For the moment: > > NACK > > Andrew
On Fri, Nov 23, 2018 at 09:31:16AM +0800, biao huang wrote: > Dear Andrew, > > Thanks for you remind. > > Sincerely, I respect any comment from any reviewer. If I didn't reply > for any comment, really sorry for that. > > As to this "tx-delay" issue, the following reply in v3 maybe ignored. > https://lkml.org/lkml/2018/11/19/158 > > "the delay time in mediatek dwmac design is not so accurate, > the current mt2712 and the following ICs will not use the > same delay design, but will use stages to indicate different > delay time. > so maybe "mediatek.tx-delay" represent the delay stage is a > good choice" > > And to make it clearer here. > > In mt2712, there are two delay macro circuit: named fine-tune and > coarse-tune. > a. fine-tune, 170+/-50ps per stage, total 32 stages > b. coarse-tune, 0.55+/-0.2ns per stage, total 32 stages > If we only consider mt2712, delay in fine-tune select a integer > multiple of 170ps, delay in coarse-tune select a integer multiple of > 550ps, for stage 0~31, the delay in fine-tune will not have the same > value with that in coarse-tune. > OK, It seems the property "fine-tune" can be eliminated . > > But the following ic will not have the same accuracy as mt2712, > and maybe will not have two delay macro circuit to be selected. New IC will have new compatible string then. If it is different, then likely these properties would have to change or have different meaning unless you use time. > 1. assume two delay macro circuit in the following ic, > fine-tune, 100ps per stage, coarse-tune, 0.55ns per stage, > if we want delay 2.2ns, fine-tune will get a 22, and coarse-tune get a > 4. We can't distinguish which delay macro we are choosing. Why wouldn't you just choose fine-tune for anything less than the max range (3200ps in this example) and course for greater than 3100ps. > 2. assume only one delay macro circuit is used, a similar case as 1 > will also increase the complexity of driver. > Then, we need define more flag property to know which delay macro we > are handling. > > The common things for all delay macro circuit in MediaTek mac design is > the stages, not the accuracy. so if we maintain stage info in "mediatek, > tx-delay", we only need care which stage we should choose. > And for each IC, we will recommend a best stage as a candidate. What if you had a 3rd delay circuit? > Above is my personal opinion, may be my understanding is wrong, > welcome for further discussion. > > Thanks a lot.
Dear Rob, Thanks for your comments. On Mon, 2018-11-26 at 15:46 -0600, Rob Herring wrote: > On Fri, Nov 23, 2018 at 09:31:16AM +0800, biao huang wrote: > > Dear Andrew, > > > > Thanks for you remind. > > > > Sincerely, I respect any comment from any reviewer. If I didn't reply > > for any comment, really sorry for that. > > > > As to this "tx-delay" issue, the following reply in v3 maybe ignored. > > https://lkml.org/lkml/2018/11/19/158 > > > > "the delay time in mediatek dwmac design is not so accurate, > > the current mt2712 and the following ICs will not use the > > same delay design, but will use stages to indicate different > > delay time. > > so maybe "mediatek.tx-delay" represent the delay stage is a > > good choice" > > > > And to make it clearer here. > > > > In mt2712, there are two delay macro circuit: named fine-tune and > > coarse-tune. > > a. fine-tune, 170+/-50ps per stage, total 32 stages > > b. coarse-tune, 0.55+/-0.2ns per stage, total 32 stages > > If we only consider mt2712, delay in fine-tune select a integer > > multiple of 170ps, delay in coarse-tune select a integer multiple of > > 550ps, for stage 0~31, the delay in fine-tune will not have the same > > value with that in coarse-tune. > > OK, It seems the property "fine-tune" can be eliminated . > > > > But the following ic will not have the same accuracy as mt2712, > > and maybe will not have two delay macro circuit to be selected. > > New IC will have new compatible string then. If it is different, then > likely these properties would have to change or have different meaning > unless you use time. > OK, I'll use tx-delay-ps instead of tx-delay. > > 1. assume two delay macro circuit in the following ic, > > fine-tune, 100ps per stage, coarse-tune, 0.55ns per stage, > > if we want delay 2.2ns, fine-tune will get a 22, and coarse-tune get a > > 4. We can't distinguish which delay macro we are choosing. > > Why wouldn't you just choose fine-tune for anything less than the max > range (3200ps in this example) and course for greater than 3100ps. > The fine-tune circuit and coarse-tune circuit are parallel, and fine-tuen is a select switch. It depends on users to choose which circuit is take effect. I shouldn't assume users would choose fine-tune when delay < 3200ps, and coarse for > 3100ps. so, tx-delay-ps will be chosen, and "fine-tune" boolean property should be remained as a indicator. > > 2. assume only one delay macro circuit is used, a similar case as 1 > > will also increase the complexity of driver. > > Then, we need define more flag property to know which delay macro we > > are handling. > > > > The common things for all delay macro circuit in MediaTek mac design is > > the stages, not the accuracy. so if we maintain stage info in "mediatek, > > tx-delay", we only need care which stage we should choose. > > And for each IC, we will recommend a best stage as a candidate. > > What if you had a 3rd delay circuit? > OK, tx-delay-ps will be a meaningful property. If more delay circuit is added, fine-tune property(boolean --> u32) can still be a indicator. > > Above is my personal opinion, may be my understanding is wrong, > > welcome for further discussion. > > > > Thanks a lot.
diff --git a/Documentation/devicetree/bindings/net/mediatek-dwmac.txt b/Documentation/devicetree/bindings/net/mediatek-dwmac.txt new file mode 100644 index 0000000..0f8a915 --- /dev/null +++ b/Documentation/devicetree/bindings/net/mediatek-dwmac.txt @@ -0,0 +1,78 @@ +MediaTek DWMAC glue layer controller + +This file documents platform glue layer for stmmac. +Please see stmmac.txt for the other unchanged properties. + +The device node has following properties. + +Required properties: +- compatible: Should be "mediatek,mt2712-gmac" for MT2712 SoC +- reg: Address and length of the register set for the device +- interrupts: Should contain the MAC interrupts +- interrupt-names: Should contain a list of interrupt names corresponding to + the interrupts in the interrupts property, if available. + Should be "macirq" for the main MAC IRQ +- clocks: Must contain a phandle for each entry in clock-names. +- clock-names: The name of the clock listed in the clocks property. These are + "axi", "apb", "mac_main", "ptp_ref" for MT2712 SoC +- mac-address: See ethernet.txt in the same directory +- phy-mode: See ethernet.txt in the same directory + +Optional properties: +- mediatek,tx-delay: TX clock delay macro value. Range is 0~31. Default is 0. + It should be defined for rgmii/rgmii-rxid/mii interface. +- mediatek,rx-delay: RX clock delay macro value. Range is 0~31. Default is 0. + It should be defined for rgmii/rgmii-txid/mii/rmii interface. +- mediatek,fine-tune: boolean property, if present indicates that fine delay + is selected for rgmii interface. + If present, tx-delay/rx-delay is 170+/-50ps per stage. + Else tx-delay/rx-delay of coarse delay macro is 0.55+/-0.2ns per stage. + This property do not apply to non-rgmii PHYs. + Only coarse-tune delay is supported for mii/rmii PHYs. +- mediatek,rmii-rxc: boolean property, if present indicates that the rmii + reference clock, which is from external PHYs, is connected to RXC pin + on MT2712 SoC. + Otherwise, is connected to TXC pin. +- mediatek,txc-inverse: boolean property, if present indicates that + 1. tx clock will be inversed in mii/rgmii case, + 2. tx clock inside MAC will be inversed relative to reference clock + which is from external PHYs in rmii case, and it rarely happen. +- mediatek,rxc-inverse: boolean property, if present indicates that + 1. rx clock will be inversed in mii/rgmii case. + 2. reference clock will be inversed when arrived at MAC in rmii case. +- assigned-clocks: mac_main and ptp_ref clocks +- assigned-clock-parents: parent clocks of the assigned clocks + +Example: + eth: ethernet@1101c000 { + compatible = "mediatek,mt2712-gmac"; + reg = <0 0x1101c000 0 0x1300>; + interrupts = <GIC_SPI 237 IRQ_TYPE_LEVEL_LOW>; + interrupt-names = "macirq"; + phy-mode ="rgmii-id"; + mac-address = [00 55 7b b5 7d f7]; + clock-names = "axi", + "apb", + "mac_main", + "ptp_ref", + "ptp_top"; + clocks = <&pericfg CLK_PERI_GMAC>, + <&pericfg CLK_PERI_GMAC_PCLK>, + <&topckgen CLK_TOP_ETHER_125M_SEL>, + <&topckgen CLK_TOP_ETHER_50M_SEL>; + assigned-clocks = <&topckgen CLK_TOP_ETHER_125M_SEL>, + <&topckgen CLK_TOP_ETHER_50M_SEL>; + assigned-clock-parents = <&topckgen CLK_TOP_ETHERPLL_125M>, + <&topckgen CLK_TOP_APLL1_D3>; + mediatek,pericfg = <&pericfg>; + mediatek,tx-delay = <9>; + mediatek,rx-delay = <9>; + mediatek,fine-tune; + mediatek,rmii-rxc; + mediatek,txc-inverse; + mediatek,rxc-inverse; + snps,txpbl = <32>; + snps,rxpbl = <32>; + snps,reset-gpio = <&pio 87 GPIO_ACTIVE_LOW>; + snps,reset-active-low; + };
The commit adds the device tree binding documentation for the MediaTek DWMAC found on MediaTek MT2712. Signed-off-by: Biao Huang <biao.huang@mediatek.com> --- .../devicetree/bindings/net/mediatek-dwmac.txt | 78 ++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/mediatek-dwmac.txt