Re: Microsemi VSC 8531/41 PHY Driver

[Nagaraju Lakkaraju <Raju.Lakkaraju@microsemi.com>]

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