Sergei,
On Wed, Jun 25, 2014 at 11:03:25PM +0400, Sergei Shtylyov wrote:
> On 06/23/2014 05:39 PM, Antoine Ténart wrote:
>
> >The Berlin SoC has a two SATA ports. Add a PHY driver to handle them.
>
> >The mode selection can let us think this PHY can be configured to fit
> >other purposes. But there are reasons to think the SATA mode will be
> >the only one usable: the PHY registers are only accessible indirectly
> >through two registers in the SATA range, the PHY seems to be integrated
> >and no information tells us the contrary. For these reasons, make the
> >driver a SATA PHY driver.
>
> I'm not even sure why you want to make it a separate driver if
> the registers are mapped to SATA controller's range.
We discussed this before and decided to move all the PHY related
functions to a dedicated PHY driver. This allows to have a generic
ahci_platform driver only using the common functions defined in the
libahci. And the PHY subsystem is there to handle PHYs, so it's a good
idea to use it, right?
> [...]
>
> >diff --git a/drivers/phy/phy-berlin-sata.c b/drivers/phy/phy-berlin-sata.c
> >new file mode 100644
> >index 000000000000..317f62358165
> >--- /dev/null
> >+++ b/drivers/phy/phy-berlin-sata.c
> >@@ -0,0 +1,246 @@
> [...]
> +#define HOST_VSA_ADDR 0x0
> +#define HOST_VSA_DATA 0x4
> +#define PORT_VSR_ADDR 0x78
> +#define PORT_VSR_DATA 0x7c
> +#define PORT_SCR_CTL 0x2c
>
> Could you keep this list sorted?
Sure.
>
> [...]
>
> +struct phy_berlin_desc {
> + struct phy *phy;
> + u32 val;
>
> Hm, aren't these power down bits? Why not call the field accordingly?
Yes. I'll update.
>
> [...]
>
> >+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);
> >+ void __iomem *ctrl_reg = priv->base + 0x60 + (desc->index * 0x80);
> >+ int ret = 0;
> >+ u32 regval;
> >+
> >+ clk_prepare_enable(priv->clk);
> >+
> >+ spin_lock(&priv->lock);
> >+
> >+ /* Power on PHY */
> >+ writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> >+ regval = readl(priv->base + HOST_VSA_DATA);
> >+ regval &= ~(desc->val);
>
> Parens not needed here.
>
> >+ 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);
>
> It probably makes sense to factor these address/data register
> writes into a separate function like phy_berlin_sata_reg_setbits().
I'm not sure. phy_berlin_sata_reg_setbits() is there for common access,
but the way to configure MBus and p[ower on the PHY is specific to them.
It would add functions only used once.
>
> [...]
> >+ /* set the controller speed */
> >+ writel(0x31, ctrl_reg + PORT_SCR_CTL);
>
> Value undocumented? Or is this the SATA SControl register by chance?
Some magic is still there...
>
> [...]
>
> >+static int phy_berlin_sata_probe(struct platform_device *pdev)
> >+{
> >+ struct device *dev = &pdev->dev;
> >+ struct phy *phy;
> >+ struct phy_provider *phy_provider;
> >+ struct phy_berlin_priv *priv;
> >+ struct resource *res;
> >+ int i;
> >+
> >+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >+ if (!priv)
> >+ return -ENOMEM;
> >+
> >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+ if (!res)
> >+ return -EINVAL;
> >+
> >+ priv->base = devm_ioremap(dev, res->start, resource_size(res));
>
> Can't you use devm_ioremap_resource()?
The SATA PHY registers are inside the SATA ones. We can't use
devm_ioremap_resource() then.
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
