On Thu, Jan 09, 2025 at 08:25:28AM -0600, Ninad Palsule wrote: > Hello Andrew, > > On 1/9/25 07:21, Andrew Lunn wrote: > > On Thu, Jan 09, 2025 at 10:33:20AM +0000, Jacky Chou wrote: > > > Hi Andrew, > > > > > > > > There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii" > > > > > (some of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only > > > > mine. > > > > > No one in aspeed SOC using "rgmii-id". > > > > O.K, so we have to be careful how we fix this. But the fact they are all equally > > > > broken might help here. > > > > > > > > > > Humm, interesting. Looking at ftgmac100.c, i don't see where you > > > > > > configure the RGMII delays in the MAC? > > > > This is going to be important. How are delays configured if they are not in the > > > > MAC driver? > > > The RGMII delay is adjusted on clk-ast2600 driver. Please refer to the following link. > > > https://github.com/AspeedTech-BMC/linux/blob/f52a0cf7c475dc576482db46759e2d854c1f36e4/drivers/clk/clk-ast2600.c#L1008 > > O.K. So in your vendor tree, you have additional DT properties > > mac1-clk-delay, mac2-clk-delay, mac3-clk-delay. Which is fine, you can > > do whatever you want in your vendor tree, it is all open source. > > > > But for mainline, this will not be accepted. We have standard > > properties defined for configuring MAC delays in picoseconds: > > > > rx-internal-delay-ps: > > description: > > RGMII Receive Clock Delay defined in pico seconds. This is used for > > controllers that have configurable RX internal delays. If this > > property is present then the MAC applies the RX delay. > > tx-internal-delay-ps: > > description: > > RGMII Transmit Clock Delay defined in pico seconds. This is used for > > controllers that have configurable TX internal delays. If this > > property is present then the MAC applies the TX delay. > > > > > > You need to use these, and in the MAC driver, not a clock driver. That > > is also part of the issue. Your MAC driver looks correct, it just > > silently passes phy-mode to the PHY just like every other MAC > > driver. But you have some code hidden away in the clock controller > > which adds the delays. If this was in the MAC driver, where it should > > be, this broken behaviour would of been found earlier. > > > > So, looking at mainline, i see where you create a gated clock. But > > what i do not see is where you set the delays. > > > > How does this work in mainline? Is there more hidden code somewhere > > setting the ASPEED_MAC12_CLK_DLY register? > > I think the code already exist in the mainline: > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-ast2600.c#L595 > > It is configuring SCU register in the ast2600 SOC to introduce delays. The > mac is part of the SOC. I could be reading this wrong, but that appears to create a gated clock. hw = clk_hw_register_gate(dev, "mac1rclk", "mac12rclk", 0, scu_g6_base + ASPEED_MAC12_CLK_DLY, 29, 0, &aspeed_g6_clk_lock); /** * clk_hw_register_gate - register a gate clock with the clock framework * @dev: device that is registering this clock * @name: name of this clock * @parent_name: name of this clock's parent * @flags: framework-specific flags for this clock * @reg: register address to control gating of this clock * @bit_idx: which bit in the register controls gating of this clock * @clk_gate_flags: gate-specific flags for this clock * @lock: shared register lock for this clock */ There is nothing here about writing a value into @reg at creation time to give it a default value. If you look at the vendor code, it has extra writes, but i don't see anything like that in mainline. Andrew
