On Fri, Jul 29, 2016 at 10:17:36AM +0200, Andrew Lunn wrote: > EXTERNAL EMAIL > > > > > +/* RGMII Rx Clock delay value change with board lay-out */ static u8 > > > +rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS; > > > > Doesn't this stop you from having a board with two PHYs with different > > layouts? You should be getting this value from the device tree. > > > > Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as > > optimized/recommended value. > > We tested on Beaglebone Black with VSC 8531 PHY. > > We would like to provide new function to configure correct/require value > > based on PHY layouts > > alone with other RGMII configuration parameters as part of our next > > implementation. > > Please either do it properly now or hard code it as the default, and > then later replace it with device tree, etc. We don't like to see half > finished features. >
I accepted your review comment. I do hard code it as the default values. > > What are you locking against? > > > > Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY > > control registers, > > first set the page number then read/write the register address. Default > > page should be Page 0. > > When I want to access not default page register, I have to lock phy device > > access and change > > the page number and register access as atomic operation. > > I understand all that, which is why i asked, "what are you locking > against?", not "why are you locking?" What are the other call paths? I > don't see you taking this lock anywhere else? Should you be? I would > just like to see a comment which suggests you understand when this > lock is needed, and when not. > This is Read Modify Write (RMW) operation on register MSCC_PHY_RGMII_CNTL. This register in different page i.e. EXTENDED-2. I would like to execute all these operations in atomic. I also use the mutex in different functions. But not in this patch. > Andrew
