lkml.org 
[lkml]   [2014]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/4] phy: miphy365x: Provide support for the MiPHY356x Generic PHY
> > The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> > devices. It has 2 ports which it can use for either; both SATA, both
>
> various SATA or PCIe devices in STMicroelectronics STiH41x SoC series?

To tell you the truth, I'm not sure if it is limited to ST's h/w, but
I think it can only be found there, so I'm happy to fixup.

<snip>

> > +config PHY_MIPHY365X
> > + tristate "STMicroelectronics MIPHY365X PHY driver for STiH41x series"
> > + depends on ARCH_STI
> > + depends on GENERIC_PHY
> depends on CONFIG_OF and HAS_IOMEM?

Sure, I'll fix.

<snip>

> > + * Copyright (C) 2014 STMicroelectronics
> > + *
> > + * STMicroelectronics PHY driver MiPHY365 (for SoC STiH416).
> > + *
> > + * Author: Alexandre Torgue <alexandre.torgue@st.com>
>
> The author of this patch is not Alexandre Torgue?

The history of this driver is long and the authors are many. Alex
did the last internal over-haul and converted it to use Generic PHY. I
took Alex's driver and made significant changes in order to upstream.

<snip>

> > +#define HFC_TIMEOUT 50
> > +
> > +#define SYSCFG_2521 0x824
> > +#define SYSCFG_2522 0x828
> > +#define SYSCFG_PCIE_SATA_MASK BIT(1)
> > +#define SYSCFG_PCIE_SATA_POS 1
> > +
> > +/* MiPHY365x register definitiona */
> > +#define RESET_REG 0x00
> > +#define RST_PLL BIT(1)
>
> There are quite a few alignment problems with these macros. It needs
> to be fixed.

This is just Git playing up.

In reality everything is perfectly aligned and all using tabs.

<snip>

> > +/*
> > + * This function selects the system configuration,
> > + * either two SATA, one SATA and one PCIe, or two PCIe lanes.
> > + */
> > +static int miphy365x_set_path(struct miphy365x_phy *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
> > +{
> > + u8 config = miphy_phy->type | miphy_phy->port;
> > + u32 mask = SYSCFG_PCIE_SATA_MASK;
> > + u32 reg;
> > + bool sata;
> > +
> > + switch (config) {
> > + case MIPHY_SATA_PORT0:
> > + reg = SYSCFG_2521;
> > + sata = true;
>
> How do we configure PORT1 for SATA here? Do we really support all the system
> configuration?

Good spot eagle-eye!

Actually in the current version there is a h/w bug which only allows
the SATA_PORT0 and PCIE_PORT1 configuration. When a new version fixing
this is released I will add version detection to the driver and we can
support full intended configuration options.

<snip>

> > +static inline int miphy365x_phy_hfc_not_rdy(struct miphy365x_phy *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
> > +{
> > + int timeout = HFC_TIMEOUT;
> > + u8 mask = IDLL_RDY | PLL_RDY;
> > + u8 regval;
> > +
> > + do {
> > + regval = readb_relaxed(miphy_phy->base + STATUS_REG);
> > + usleep_range(2000, 2500);
>
> Any comment on how this delay value is obtained?

I don't have any specific comments, I believe the 2000us it taken from
the datasheet and the 2500 is us playing nice with the scheduler.

<snip>

> > +static inline int miphy365x_phy_rdy(struct miphy365x_phy *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
> > +{
> > + int timeout = HFC_TIMEOUT;
> > + u8 mask = mask = IDLL_RDY | PLL_RDY;
>
> just u8 mask = IDLL_RDY | PLL_RDY; would suffice.

Hmm... not sure how this slipped through - will fix.

> > + u8 regval;
> > +
> > + do {
> > + regval = readb_relaxed(miphy_phy->base + STATUS_REG);
> > + usleep_range(2000, 2500);
>
> same here.

As above.

<snip>

> > + mask = DES_BIT_LOCK | DES_SYMBOL_LOCK;
> > + while ((readb_relaxed(miphy_phy->base + COMP_CTRL1_REG) & mask) != mask)
> > + cpu_relax();
>
> Don't we need to break from here at some point if the LOCK's are never set?

I'm sure sure that's possible, but I will invesigate and fixup if req'd.

<snip>

> > +static int miphy365x_phy_power_on(struct phy *phy)
> > +{
> > + return 0;
> > +}
> > +
> > +static int miphy365x_phy_power_off(struct phy *phy)
> > +{
> > + return 0;
> > +}
>
> Both these empty functions can be removed.

You're right, I see the NULL checks, thanks.

<snip>

> > +static int miphy365x_phy_get_base_addr(struct platform_device *pdev,
> > + struct miphy365x_phy *phy, u8 port)
> > +{
> > + struct resource *res;
> > + char sata[16];
> > + char pcie[16];
>
> It can be done with a single variable ;-)

Right. :)

<snip>

> Phy provider register should be the last step in registering the PHY.

Okay, will fix, thanks.

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2014-02-13 12:21    [W:0.056 / U:1.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site