On Wed, Jul 13, 2016 at 11:14:21AM +0200, Charles-Antoine Couret wrote:
Hi Charles-Antoine
> >> +#define LPA_FIBER_1000HALF 0x40
> >> +#define LPA_FIBER_1000FULL 0x20
> >> +
> >> +#define LPA_PAUSE_FIBER 0x180
> >> +#define LPA_PAUSE_ASYM_FIBER 0x100
> >> +
> >> +#define ADVERTISE_FIBER_1000HALF 0x40
> >> +#define ADVERTISE_FIBER_1000FULL 0x20
> >> +
> >> +#define ADVERTISE_PAUSE_FIBER 0x180
> >> +#define ADVERTISE_PAUSE_ASYM_FIBER 0x100
> >
> > Are these standardised anywhere? If they are following a standard,
> > they should be put into include/uapi/linux/mii.h.
> I don't find any standard about this, I think it should be Marvell specific.
O.K.
> >> +static inline u32 ethtool_adv_to_fiber_adv_t(u32 ethadv)
> >> +{
> >> + u32 result = 0;
> >> +
> >> + if (ethadv & ADVERTISED_1000baseT_Half)
> >> + result |= ADVERTISE_FIBER_1000HALF;
> >
> > Dumb question: Does 1000baseT_Half even make sense for fibre? Can you
> > do half duplex? Would that not mean you have a single fibre, both
> > ends are using the same laser frequency, and you are doing some form
> > of CSMA/CD?
>
> It's strange, I agree, but the register about that exists in the datasheet
> and the value is not fixed.
> In practice, I don't have a component to test this case correctly.
O.K, just implement it according to the data sheet.
> >> *
> >> * Generic status code does not detect Fiber correctly!
> >> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device
> >> *phydev)
> >> int lpa;
> >> int lpagb;
> >> int status = 0;
> >> + int page, fiber;
> >>
> >> - /* Update the link, but return if there
> >> + /* Detect and update the link, but return if there
> >> * was an error */
> >> - err = genphy_update_link(phydev);
> >> - if (err)
> >> - return err;
> >> + page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> >> + if (page == MII_M1111_FIBER)
> >> + fiber = 1;
> >> + else
> >> + fiber = 0;
> >
> > This read is expensive, since the MDIO bus is slow. It would be better
> > just to pass fibre as a parameter.
>
> But this function is used for other Marvell's phy, without fiber link for
> example.
> And this function should has only the struct phy_device as parameter.
>
> I don't have idea to avoid that, without create a custom function for that
> which would be very similar to this function.
> Or used a phy_device field for that? I think it's awful idea...
So i would have
static int marvell_read_status_page(struct phy_device *phydev, int page)
{}
basically doing what you have above, but without the read.
static int marvell_read_status(struct phy_device *phydev)
{
if (phydev->supported & SUPPORTED_FIBRE) {
marvell_read_status_page(phydev, MII_M1111_FIBER);
if (phydev->link)
return;
return marvell_read_status_page(phydev, MII_M1111_COPPER);
}
> > I think it would be better to look for SUPPORTED_FIBRE in
> > drv->features, rather than have two different functions.
> >
> > In fact, i would do that in general, rather than add your _fibre()
> > functions.
>
> So, you suggest to do that in genphy_* functions or create marvell_*
> functions with this condition?
> I'm agree with the second suggestion.
The second.
>
> >> +
> >> +/* marvell_resume_fiber
> >> + *
> >> + * Some Marvell's phys have two modes: fiber and copper.
> >> + * Both need to be resumed
> >> + */
> >> +static int marvell_resume_fiber(struct phy_device *phydev)
> >> +{
> >> + int err;
> >> +
> >> + /* Resume the fiber mode first */
> >> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> >> + if (err < 0)
> >> + goto error;
> >> +
> >> + err = genphy_resume(phydev);
> >> + if (err < 0)
> >> + goto error;
> >> +
> >> + /* Then, the copper link */
> >> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> >> + if (err < 0)
> >> + goto error;
> >> +
> >> + return genphy_resume(phydev);
> >
> > Should it be resumed twice? Or just once at the end? Same question
> > for suspend.
>
> I don't understand your question.
You call genphy_resume(phydev) twice. Once is sufficient.
Andrew
