| Message ID | 1403530783-17180-2-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
Hello. 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. > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> [...] > 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? [...] +struct phy_berlin_desc { + struct phy *phy; + u32 val; Hm, aren't these power down bits? Why not call the field accordingly? [...] > +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(). [...] > + /* set the controller speed */ > + writel(0x31, ctrl_reg + PORT_SCR_CTL); Value undocumented? Or is this the SATA SControl register by chance? [...] > +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()? > + if (!priv->base) > + return -ENOMEM; > + > + priv->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->clk)) > + return PTR_ERR(priv->clk); > + > + dev_set_drvdata(dev, priv); > + spin_lock_init(&priv->lock); > + > + for (i = 0; i < BERLIN_SATA_PHY_NB; i++) { > + phy = devm_phy_create(dev, &phy_berlin_sata_ops, NULL); > + if (IS_ERR(phy)) { > + dev_err(dev, "failed to create PHY %d\n", i); > + return PTR_ERR(phy); > + } > + > + priv->phys[i].phy = phy; > + priv->phys[i].val = phy_berlin_power_down_bits[i]; > + priv->phys[i].index = i; > + phy_set_drvdata(phy, &priv->phys[i]); > + > + /* Make sure the PHY is off */ > + phy_berlin_sata_power_off(phy); > + } > + > + phy_provider = > + devm_of_phy_provider_register(dev, phy_berlin_sata_phy_xlate); > + if (IS_ERR(phy_provider)) No dev_err() here? > + return PTR_ERR(phy_provider); > + > + return 0; > +} WBR, Sergei
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
On 06/30/2014 11:59 AM, Antoine Ténart wrote: > On Wed, Jun 25, 2014 at 11:03:25PM +0400, Sergei Shtylyov wrote: >> On 06/23/2014 05:39 PM, Antoine Ténart wrote: >>> + /* 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... Antoine, I guess Sergei was referring to AHCI spec here. PORT_SCR bits are documented in AHCI spec as: 7:4 = 0x3 Limit speed negotiation to a rate not greater than Gen3 communication rate. 3:0 = 0x1 Perform interface communication sequence [...]. This is functionally equivalent to a hard reset [...]. So, the question is: Should we really need to reset controller in the PHY driver or is it already done in AHCI common code? At least we should change the comment to something like /* set Gen3 controller speed and perform hard reset */ Sebastian
Hi Sebastian, On Mon, Jun 30, 2014 at 04:40:49PM +0200, Sebastian Hesselbarth wrote: > On 06/30/2014 11:59 AM, Antoine Ténart wrote: > >On Wed, Jun 25, 2014 at 11:03:25PM +0400, Sergei Shtylyov wrote: > >>On 06/23/2014 05:39 PM, Antoine Ténart wrote: > >>>+ /* 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... > > I guess Sergei was referring to AHCI spec here. PORT_SCR bits are > documented in AHCI spec as: > > 7:4 = 0x3 Limit speed negotiation to a rate not greater than Gen3 > communication rate. > > 3:0 = 0x1 Perform interface communication sequence [...]. This is > functionally equivalent to a hard reset [...]. > > So, the question is: Should we really need to reset controller in the > PHY driver or is it already done in AHCI common code? At least we > should change the comment to something like > /* set Gen3 controller speed and perform hard reset */ I just checked, the AHCI common code has a function to do the reset: ahci_reset_controller(). As of the max speed negociation rate, I did not see it in the common AHCI functions. The eSATA port on the Berlin2Q works without this line, but it may be a good idea to keep the max speed negociation rate. Anyway, we can remove the reset part. Nice catch! Antoine
Hello. On 06/30/2014 07:44 PM, Antoine Ténart wrote: >>>>> + /* 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... >> I guess Sergei was referring to AHCI spec here. Actually, even to the SATA specs. :-) >> PORT_SCR bits are documented in AHCI spec as: >> 7:4 = 0x3 Limit speed negotiation to a rate not greater than Gen3 >> communication rate. >> 3:0 = 0x1 Perform interface communication sequence [...]. This is >> functionally equivalent to a hard reset [...]. >> So, the question is: Should we really need to reset controller in the >> PHY driver or is it already done in AHCI common code? At least we >> should change the comment to something like >> /* set Gen3 controller speed and perform hard reset */ > I just checked, the AHCI common code has a function to do the reset: > ahci_reset_controller(). As of the max speed negociation rate, I did not > see it in the common AHCI functions. You've looked in a wrong place -- since SControl is a standard *SATA* register, it gets read/written by the libata core. The low-level driver only provides scr_{read|write}() methods. > The eSATA port on the Berlin2Q works without this line, but it may be a > good idea to keep the max speed negociation rate. It's usually libata's task to negotiate the SATA speed. > Anyway, we can remove the reset part. Nice catch! Thanks. > Antoine WBR, Sergei
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 16a2f067c242..365ad3651e1c 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -15,6 +15,13 @@ config GENERIC_PHY phy users can obtain reference to the PHY. All the users of this framework should select this config. +config PHY_BERLIN_SATA + tristate "Marvell Berlin SATA PHY driver" + depends on ARCH_BERLIN && OF + select GENERIC_PHY + help + Enable this to support the SATA PHY on Marvell Berlin SoCs. + config PHY_EXYNOS_MIPI_VIDEO tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver" depends on HAS_IOMEM diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index b4f1d5770601..a137a2e23218 100644 --- a/drivers/phy/Makefile +++ b/drivers/phy/Makefile @@ -3,6 +3,7 @@ # obj-$(CONFIG_GENERIC_PHY) += phy-core.o +obj-$(CONFIG_PHY_BERLIN_SATA) += phy-berlin-sata.o obj-$(CONFIG_BCM_KONA_USB2_PHY) += phy-bcm-kona-usb2.o obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-exynos-dp-video.o obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o 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 @@ +/* + * Marvell Berlin SATA PHY driver + * + * Copyright (C) 2014 Marvell Technology Group Ltd. + * + * Antoine Ténart <antoine.tenart@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/clk.h> +#include <linux/module.h> +#include <linux/phy/phy.h> +#include <linux/io.h> +#include <linux/platform_device.h> + +#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 + +#define CONTROL_REGISTER 0x0 +#define MBUS_SIZE_CONTROL 0x4 + +#define POWER_DOWN_PHY0 BIT(6) +#define POWER_DOWN_PHY1 BIT(14) +#define MBUS_WRITE_REQUEST_SIZE_128 (BIT(2) << 16) +#define MBUS_READ_REQUEST_SIZE_128 (BIT(2) << 19) + +#define PHY_BASE 0x200 + +/* register 0x01 */ +#define REF_FREF_SEL_25 BIT(0) +#define PHY_MODE_SATA (0x0 << 5) + +/* register 0x02 */ +#define USE_MAX_PLL_RATE BIT(12) + +/* register 0x23 */ +#define DATA_BIT_WIDTH_10 (0x0 << 10) +#define DATA_BIT_WIDTH_20 (0x1 << 10) +#define DATA_BIT_WIDTH_40 (0x2 << 10) + +/* register 0x25 */ +#define PHY_GEN_MAX_1_5 (0x0 << 10) +#define PHY_GEN_MAX_3_0 (0x1 << 10) +#define PHY_GEN_MAX_6_0 (0x2 << 10) + +#define BERLIN_SATA_PHY_NB 2 + +#define to_berlin_sata_phy_priv(desc) \ + container_of((desc), struct phy_berlin_priv, phys[(desc)->index]) + +struct phy_berlin_desc { + struct phy *phy; + u32 val; + unsigned index; +}; + +struct phy_berlin_priv { + void __iomem *base; + spinlock_t lock; + struct clk *clk; + struct phy_berlin_desc phys[BERLIN_SATA_PHY_NB]; +}; + +static inline void phy_berlin_sata_reg_setbits(void __iomem *ctrl_reg, u32 reg, + u32 mask, u32 val) +{ + u32 regval; + + /* select register */ + writel(PHY_BASE + reg, ctrl_reg + PORT_VSR_ADDR); + + /* set bits */ + regval = readl(ctrl_reg + PORT_VSR_DATA); + regval &= ~mask; + regval |= val; + writel(regval, ctrl_reg + PORT_VSR_DATA); +} + +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); + 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); + + /* set PHY mode and ref freq to 25 MHz */ + phy_berlin_sata_reg_setbits(ctrl_reg, 0x1, 0xff, + REF_FREF_SEL_25 | PHY_MODE_SATA); + + /* set PHY up to 6 Gbps */ + phy_berlin_sata_reg_setbits(ctrl_reg, 0x25, 0xc00, PHY_GEN_MAX_6_0); + + /* set 40 bits width */ + phy_berlin_sata_reg_setbits(ctrl_reg, 0x23, 0xc00, DATA_BIT_WIDTH_40); + + /* use max pll rate */ + phy_berlin_sata_reg_setbits(ctrl_reg, 0x2, 0x0, USE_MAX_PLL_RATE); + + /* set the controller speed */ + writel(0x31, ctrl_reg + PORT_SCR_CTL); + + spin_unlock(&priv->lock); + + clk_disable_unprepare(priv->clk); + + return ret; +} + +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; + + clk_prepare_enable(priv->clk); + + 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); + + clk_disable_unprepare(priv->clk); + + return 0; +} + +static struct phy *phy_berlin_sata_phy_xlate(struct device *dev, + struct of_phandle_args *args) +{ + struct phy_berlin_priv *priv = dev_get_drvdata(dev); + + if (WARN_ON(args->args[0] >= BERLIN_SATA_PHY_NB)) + return ERR_PTR(-ENODEV); + + return priv->phys[args->args[0]].phy; +} + +static struct phy_ops phy_berlin_sata_ops = { + .power_on = phy_berlin_sata_power_on, + .power_off = phy_berlin_sata_power_off, + .owner = THIS_MODULE, +}; + +static u32 phy_berlin_power_down_bits[] = { + POWER_DOWN_PHY0, + POWER_DOWN_PHY1, +}; + +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)); + if (!priv->base) + return -ENOMEM; + + priv->clk = devm_clk_get(dev, NULL); + if (IS_ERR(priv->clk)) + return PTR_ERR(priv->clk); + + dev_set_drvdata(dev, priv); + spin_lock_init(&priv->lock); + + for (i = 0; i < BERLIN_SATA_PHY_NB; i++) { + phy = devm_phy_create(dev, &phy_berlin_sata_ops, NULL); + if (IS_ERR(phy)) { + dev_err(dev, "failed to create PHY %d\n", i); + return PTR_ERR(phy); + } + + priv->phys[i].phy = phy; + priv->phys[i].val = phy_berlin_power_down_bits[i]; + priv->phys[i].index = i; + phy_set_drvdata(phy, &priv->phys[i]); + + /* Make sure the PHY is off */ + phy_berlin_sata_power_off(phy); + } + + phy_provider = + devm_of_phy_provider_register(dev, phy_berlin_sata_phy_xlate); + if (IS_ERR(phy_provider)) + return PTR_ERR(phy_provider); + + return 0; +} + +static const struct of_device_id phy_berlin_sata_of_match[] = { + { .compatible = "marvell,berlin2q-sata-phy" }, + { }, +}; + +static struct platform_driver phy_berlin_sata_driver = { + .probe = phy_berlin_sata_probe, + .driver = { + .name = "phy-berlin-sata", + .owner = THIS_MODULE, + .of_match_table = phy_berlin_sata_of_match, + }, +}; +module_platform_driver(phy_berlin_sata_driver); + +MODULE_DESCRIPTION("Marvell Berlin SATA PHY driver"); +MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>"); +MODULE_LICENSE("GPL v2");
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. Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> --- drivers/phy/Kconfig | 7 ++ drivers/phy/Makefile | 1 + drivers/phy/phy-berlin-sata.c | 246 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 254 insertions(+) create mode 100644 drivers/phy/phy-berlin-sata.c