On Wed, 02 Jul 2014, Kishon Vijay Abraham I wrote:
> On Monday 30 June 2014 06:31 PM, Lee Jones wrote:
> > 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
> > PCIe or one of each in any configuration.
> >
> > Acked-by: Kishon Vijay Abraham I <kis...@ti.com>
I added this (a long time ago) becuase you already accepted this patch
into your tree at one point. I guess things have changed since then,
I'll remove for the next submission.
> > Acked-by: Mark Rutland <mark.rutl...@arm.com>
> > Signed-off-by: Alexandre Torgue <alexandre.tor...@st.com>
> > Signed-off-by: Lee Jones <lee.jo...@linaro.org>
> > ---
> > drivers/phy/Kconfig | 10 +
> > drivers/phy/Makefile | 1 +
> > drivers/phy/phy-miphy365x.c | 630
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 641 insertions(+)
> > create mode 100644 drivers/phy/phy-miphy365x.c
[...]
> > +static void miphy365x_init_pcie_port(struct miphy365x *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
>
> I would have this return int so that it's API is similar to that of sata.
Seems a little pointless, but okay.
[...]
> > +static inline int miphy365x_hfc_not_rdy(struct miphy365x *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);
>
> msleep(2)?
usleep_range() is kinder to the scheduler than msleep().
[...]
> > +static inline void miphy365x_set_comp(struct miphy365x *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
> > +{
> > + u8 val, mask;
> > +
> > + if (miphy_dev->sata_gen == SATA_GEN1)
> > + writeb_relaxed(COMP_2MHZ_RAT_GEN1,
> > + miphy_phy->base + COMP_CTRL2_REG);
> > + else
> > + writeb_relaxed(COMP_2MHZ_RAT,
> > + miphy_phy->base + COMP_CTRL2_REG);
>
> Btw don't you think it will be safe to use readb/writeb instead of
> readb_relaxed/writeb_relaxed here and everywhere else?
{read,write}b_relaxed() calls are more efficient than {read,write}b().
The non-relaxed versions are only required on architectures which do
not guarantee access ordering. This driver only supports ARM, which
does this by design.
[...]
> > + if (WARN_ON(port >= ARRAY_SIZE(ports)))
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (type == MIPHY_TYPE_SATA)
> > + state->phys[port].base = state->phys[port].sata;
> > + else if (type == MIPHY_TYPE_PCIE)
> > + state->phys[port].base = state->phys[port].pcie;
>
> Sergei made an important point about overriding the PHY mode (like here) that
> might create problem for the first driver that got the PHY. This might need
> both the phy-core and the phy driver to maintain state to handle this
> properly.
Would you be kind enough to explain a little more about the problem
and what you think a valid solution might look like?
[...]
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
