Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

["Antoine Ténart" <antoine.tenart@free-electrons.com>]

Arnd,

On Wed, May 14, 2014 at 03:02:34PM +0200, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> > +static int phy_berlin_sata_power_on(struct phy *phy)
> > +{
> > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > +       u32 regval;
> > +
> > +       spin_lock(&priv->lock);
> > +
> > +       /* Power up PHY */
> > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > +       regval = readl(priv->base + HOST_VSA_DATA);
> > +       regval &= ~(desc->val);
> > +       writel(regval, priv->base + HOST_VSA_DATA);
> > +
> > +       /* Configure MBus */
> > +       writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
> > +       regval = readl(priv->base + HOST_VSA_DATA);
> > +       regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
> > +       writel(regval, priv->base + HOST_VSA_DATA);
> > +
> > +       spin_unlock(&priv->lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static int phy_berlin_sata_power_off(struct phy *phy)
> > +{
> > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > +       u32 regval;
> > +
> > +       spin_lock(&priv->lock);
> > +
> > +       /* Power down PHY */
> > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > +       regval = readl(priv->base + HOST_VSA_DATA);
> > +       regval |= desc->val;
> > +       writel(regval, priv->base + HOST_VSA_DATA);
> > +
> > +       spin_unlock(&priv->lock);
> > +
> > +       return 0;
> 
> I don't get this part: you have a reference to the phy here,
> but then you go poking the phy registers from the SATA driver
> rather than calling a PHY API function.

The v1 only introduced an AHCI driver. I somewhat agree the PHY
operations done in the AHCI driver could be in there.

I can move the initialization done in the AHCI driver here, but I'll
still need the driver: the Berlin AHCI needs to call the framework
generic functions with a custom mask and has custom pm_ops. So I'll
end up with a nearly empty AHCI driver, not able to control the port
parameters.

Or I can put all this in the AHCI driver, but then we'll need to
describe the PHYs there (to be able to enable each PHY independently)
and add bindings to the SATA ones.

What do you think? I prefer the first solution, but we'll have SATA
port related configuration in the PHY and a very tiny AHCI driver
because I can't really use the default behaviour of the ahci_platform.

> > +                * By default the PHY node is used to request and match a PHY.
> > +                * We describe one PHY per sub-node here. Use the right node.
> > +                */
> > +               phy->dev.of_node = child;
> > +
> > +               priv->phys[phy_id].phy = phy;
> > +               priv->phys[phy_id].val = desc[phy_id].val;
> > +               priv->phys[phy_id].index = phy_id;
> > +               phy_set_drvdata(phy, &priv->phys[phy_id]);
> 
> And here, you set a driver specific value into a structure used by the
> PHY.

Values in priv->phys[] are related to the PHYs. phy_set_drvdata() allows
to store PHY related data, which is what I'm doing there. Nearly all PHY
drivers are doing this.

Or am I missing something?

> 
> Both of these are layering violations. You should either use the PHY
> interfaces correctly so the SATA driver doesn't have to know about the
> specific, or not use a PHY device node at all and do everything in
> the SATA front-end.

To be sure: you mean using the PHY init() interface in the AHCI driver?

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com