| Message ID | 1400060942-10588-2-git-send-email-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wednesday 14 May 2014 03:18 PM, Antoine Ténart wrote: > The Berlin SoC has a two SATA ports. Add a PHY driver to handle them. > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > --- > drivers/phy/Kconfig | 5 ++ > drivers/phy/Makefile | 1 + > drivers/phy/phy-berlin-sata.c | 180 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 186 insertions(+) > create mode 100644 drivers/phy/phy-berlin-sata.c > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig > index 4906c27fa3bd..b31b1986fda4 100644 > --- a/drivers/phy/Kconfig > +++ b/drivers/phy/Kconfig > @@ -15,6 +15,11 @@ 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 > + bool > + depends on ARCH_BERLIN && OF > + select GENERIC_PHY > + > 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 7728518572a4..40278706ac1b 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..b08f76329a34 > --- /dev/null > +++ b/drivers/phy/phy-berlin-sata.c > @@ -0,0 +1,180 @@ > +/* > + * 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/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 CONTROL_REGISTER 0x0 > +#define MBUS_SIZE_CONTROL 0x4 > + > +#define POWER_DOWN_SATA0 BIT(6) > +#define POWER_DOWN_SATA1 BIT(14) > +#define MBUS_WRITE_REQUEST_SIZE_128 (BIT(2) << 16) > +#define MBUS_READ_REQUEST_SIZE_128 (BIT(2) << 19) > + > +#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 phy_berlin_desc phys[BERLIN_SATA_PHY_NB]; Can't we do away with hardcoding BERLIN_SATA_PHY_NB? > +}; > + > +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; > +} > + > +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 struct phy_berlin_desc desc[] = { > + { .val = POWER_DOWN_SATA0 }, > + { .val = POWER_DOWN_SATA1 }, > + { }, > +}; > + > +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; > + struct device_node *child; > + u8 phy_id; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->base = devm_ioremap(dev, res->start, resource_size(res)); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + dev_set_drvdata(dev, priv); > + spin_lock_init(&priv->lock); > + > + for_each_child_of_node(dev->of_node, child) { > + if (of_property_read_u8(child, "reg", &phy_id)) > + return -EINVAL; > + > + if (phy_id >= BERLIN_SATA_PHY_NB) { > + dev_err(dev, "invalid reg in node %s\n", child->name); > + return -EINVAL; > + } > + > + phy = devm_phy_create(dev, &phy_berlin_sata_ops, NULL); > + if (IS_ERR(phy)) { > + dev_err(dev, "failed to create PHY for node %s\n", > + child->name); > + return PTR_ERR(phy); > + } > + > + /* > + * 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]); > + > + /* Make sure the PHY is off */ > + phy_berlin_sata_power_off(phy); > + > + phy_provider = devm_of_phy_provider_register(&phy->dev, > + of_phy_simple_xlate); > + if (IS_ERR(phy_provider)) > + return PTR_ERR(phy_provider); was this intentional? registering multiple PHY providers? Thanks Kishon
Hi, On Wed, May 14, 2014 at 03:43:03PM +0530, Kishon Vijay Abraham I wrote: > Hi, > > On Wednesday 14 May 2014 03:18 PM, Antoine Ténart wrote: […] > > +#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 phy_berlin_desc phys[BERLIN_SATA_PHY_NB]; > > Can't we do away with hardcoding BERLIN_SATA_PHY_NB? We can't if we want to be able to use the container_of macro in to_berlin_sata_phy_priv(). And we want a common structure to store the common spinlock and base address. […] > > + /* > > + * 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]); > > + > > + /* Make sure the PHY is off */ > > + phy_berlin_sata_power_off(phy); > > + > > + phy_provider = devm_of_phy_provider_register(&phy->dev, > > + of_phy_simple_xlate); > > + if (IS_ERR(phy_provider)) > > + return PTR_ERR(phy_provider); > > was this intentional? registering multiple PHY providers? Yes. Each sub-node describe a PHY and register as a PHY provider. This allow to reference the PHY as <&sata_phy0> and not <&sata_phy 0>. It would be confusing to have a sub-node sata_phy0 and to use its parent to access it. Antoine
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. > + * 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. 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. Arnd
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
On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote: > 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. I just noticed I quoted the wrong driver with my comment, but I think you got what I meant. Why do you need a custom mask? Is that something you could pass as the argument in the phy descriptor using #phy-cells=<1>? > > > + * 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? This part is really ok, I got confused when I replied to the wrong email. Sorry about this. > > 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? If this PHY is specific to the ahci-berlin hardware and not shared with anything else, you don't really need to split out a phy driver. That would somewhat simplify what you ahve here. The alternative is to make it as generic as you can. If you can manage to move all the phy code into phy-berlin-sata driver, it should be possible to just extend the ahci-platform driver resume function to reinitialize the phy if there is one. Arnd
On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: > On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote: > > 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. > > I just noticed I quoted the wrong driver with my comment, but I think > you got what I meant. > > Why do you need a custom mask? Is that something you could pass > as the argument in the phy descriptor using #phy-cells=<1>? I meant a custom mask in the AHCI driver, when calling the ahci_platform_init_host() function. Otherwise we'll have problems on the BG2Q DMP (it only has one PHY available, and not initializing it is not enough). > > > > > + * 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? > > This part is really ok, I got confused when I replied to the wrong email. > Sorry about this. > > > > 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? > > If this PHY is specific to the ahci-berlin hardware and not shared with > anything else, you don't really need to split out a phy driver. That > would somewhat simplify what you ahve here. I don't have lots of info about that, but we set the PHY to PHY_MODE_SATA in the AHCI driver. So I guess there are other modes. Maybe Jisheng can help us with this? > > The alternative is to make it as generic as you can. If you can manage > to move all the phy code into phy-berlin-sata driver, it should be > possible to just extend the ahci-platform driver resume function to > reinitialize the phy if there is one. It is possible to move all the PHY code the phy-berlin-sata. Then I'll need to hack a bit the AHCI framework so it can handle more than one PHY. But as I said, I'll still need to set a custom mask, and adding a quirk to the AHCI platform or framework does not seem to be a very good idea, imho. Or I can add a computed mask to the ahci-platform driver, like I did in the Berlin one, but I don't know what would be the consequences. For each PHY I have: + mask |= 1 << i; Antoine
On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: > On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: > > On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote: > > > 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: > > > > 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. > > > > I just noticed I quoted the wrong driver with my comment, but I think > > you got what I meant. > > > > Why do you need a custom mask? Is that something you could pass > > as the argument in the phy descriptor using #phy-cells=<1>? > > I meant a custom mask in the AHCI driver, when calling the > ahci_platform_init_host() function. Otherwise we'll have problems on the > BG2Q DMP (it only has one PHY available, and not initializing it is not > enough). Ah, I see what you mean now. Of course, this could also be an optional property in the generic AHCI binding, which would require that you specify the available ports in DT for nonstandard masks. Just to confirm: The HOST_PORTS_IMPL register on BG2Q DMP has an value that we can't use here to determine the available ports, right? > > > > > + * 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? > > > > This part is really ok, I got confused when I replied to the wrong email. > > Sorry about this. > > > > > > 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? > > > > If this PHY is specific to the ahci-berlin hardware and not shared with > > anything else, you don't really need to split out a phy driver. That > > would somewhat simplify what you ahve here. > > I don't have lots of info about that, but we set the PHY to > PHY_MODE_SATA in the AHCI driver. So I guess there are other modes. Ok, good point. This does sound a lot like there are multiple modes. This would also imply that the "compatible" string should not have "sata" in it either, but describe a generic PHY. You can pass the mode as part of the phy descriptor from DT then. > > The alternative is to make it as generic as you can. If you can manage > > to move all the phy code into phy-berlin-sata driver, it should be > > possible to just extend the ahci-platform driver resume function to > > reinitialize the phy if there is one. > > It is possible to move all the PHY code the phy-berlin-sata. Then I'll > need to hack a bit the AHCI framework so it can handle more than one > PHY. But as I said, I'll still need to set a custom mask, and adding a > quirk to the AHCI platform or framework does not seem to be a very good > idea, imho. > > Or I can add a computed mask to the ahci-platform driver, like I did in > the Berlin one, but I don't know what would be the consequences. For > each PHY I have: > > + mask |= 1 << i; It does sound generic enough that the same would be done on other ahci-platform variants with a generic phy attached, even though currently each one has only one phy. We can probably define this as an extension in some form. Do you think we need to handle the case where the first port is unavailable but some of the other ports should be used? Arnd
On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote: > On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: > > On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: > > > On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote: > > > > 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: > > > > > 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. > > > > > > I just noticed I quoted the wrong driver with my comment, but I think > > > you got what I meant. > > > > > > Why do you need a custom mask? Is that something you could pass > > > as the argument in the phy descriptor using #phy-cells=<1>? > > > > I meant a custom mask in the AHCI driver, when calling the > > ahci_platform_init_host() function. Otherwise we'll have problems on the > > BG2Q DMP (it only has one PHY available, and not initializing it is not > > enough). > > Ah, I see what you mean now. Of course, this could also be an optional > property in the generic AHCI binding, which would require that you > specify the available ports in DT for nonstandard masks. > > Just to confirm: The HOST_PORTS_IMPL register on BG2Q DMP has an > value that we can't use here to determine the available ports, right? Right. The register is here but saying there are 2 available ports, which is true for the BG2Q SoC but not true for the BG2Q DMP board. So the AHCI framework reads the register and performs a misconfiguration of the SATA. > > > > > > > + * 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? > > > > > > This part is really ok, I got confused when I replied to the wrong email. > > > Sorry about this. > > > > > > > > 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? > > > > > > If this PHY is specific to the ahci-berlin hardware and not shared with > > > anything else, you don't really need to split out a phy driver. That > > > would somewhat simplify what you ahve here. > > > > I don't have lots of info about that, but we set the PHY to > > PHY_MODE_SATA in the AHCI driver. So I guess there are other modes. > > Ok, good point. This does sound a lot like there are multiple modes. > > This would also imply that the "compatible" string should not have > "sata" in it either, but describe a generic PHY. You can pass the mode > as part of the phy descriptor from DT then. Yes sure, with something like: phys = <&foo_phy0 FOO_SATA>; > > > > The alternative is to make it as generic as you can. If you can manage > > > to move all the phy code into phy-berlin-sata driver, it should be > > > possible to just extend the ahci-platform driver resume function to > > > reinitialize the phy if there is one. > > > > It is possible to move all the PHY code the phy-berlin-sata. Then I'll > > need to hack a bit the AHCI framework so it can handle more than one > > PHY. But as I said, I'll still need to set a custom mask, and adding a > > quirk to the AHCI platform or framework does not seem to be a very good > > idea, imho. > > > > Or I can add a computed mask to the ahci-platform driver, like I did in > > the Berlin one, but I don't know what would be the consequences. For > > each PHY I have: > > > > + mask |= 1 << i; > > It does sound generic enough that the same would be done on other > ahci-platform variants with a generic phy attached, even though > currently each one has only one phy. > > We can probably define this as an extension in some form. > Do you think we need to handle the case where the first port is > unavailable but some of the other ports should be used? The Berlin AHCI driver handles the case. I don't know if we'll come across a use case for this but supporting it should'nt be difficult. So why not ... :) I think we can either add a binding enabling an automatic mask computation in the framework or match the compatible and do this in the ahci-platform driver. Or we can make this the default behaviour when multiple PHYs are used in an AHCI node, but I'll have to check if that's possible. Maybe the SATA subsystem maintainer has an opinion on the matter? Antoine
On 05/14/2014 06:57 PM, Antoine Ténart wrote: > On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote: >> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: >>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: >>>> Why do you need a custom mask? Is that something you could pass >>>> as the argument in the phy descriptor using #phy-cells=<1>? >>> >>> I meant a custom mask in the AHCI driver, when calling the >>> ahci_platform_init_host() function. Otherwise we'll have problems on the >>> BG2Q DMP (it only has one PHY available, and not initializing it is not >>> enough). >> >> Ah, I see what you mean now. Of course, this could also be an optional >> property in the generic AHCI binding, which would require that you >> specify the available ports in DT for nonstandard masks. >> >> Just to confirm: The HOST_PORTS_IMPL register on BG2Q DMP has an >> value that we can't use here to determine the available ports, right? > > Right. The register is here but saying there are 2 available ports, > which is true for the BG2Q SoC but not true for the BG2Q DMP board. So > the AHCI framework reads the register and performs a misconfiguration of > the SATA. *Disclaimer:* All PHY IP related comments below are *pure* guessing. I have no clue how that PHY really looks like or how it is wired up. I think it is time to sum this up a little bit and help Antoine carry on this patches. From what I understand from the conversation, we have a single PHY register set dealing with both SATA ports available on the SoC. Also, from the name of the PHY bits we assume the PHY may be able to work in different modes than just SATA. And we currently have an AHCI-compatible SATA IP that supports up to two ports, with one actually connected to a SATA plug on the DMP board. Now, thinking about the PHY binding and the (possible) multi-protocol support, it can be possible that on BG2Q there is a generic 2-lane LVDS PHY that can be configured to support SATA or PCIe. Both are electrically and bit-level compatible, so they could be internally wired-up with AHCI and PCIe controller. From a DT point-of-view, we need a way to (a) link each SATA or PCIe port to the PHY, (b) specify the PHY lane to be used, and (c) specify the protocol to be used on that lane. If I got it right, Arnd already mentioned to use the phy-specifier to deal with it: e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE> Let's assume we have one dual-port SATA controller and one PCIe controller with either x1 or x2 support. The only sane DT binding, I can think of then would be: berlin2q.dtsi: genphy: lvds@ea00ff { compatible = "marvell,berlin-lvds-phy"; reg = <0xea00ff 0x100>; #phy-cells = <2>; }; sata: sata@ab00ff { compatible = "ahci-platform"; reg = <0xab00ff 0x100>; sata0: sata-port@0 { reg = <0>; phy = <&genphy 0 MODE_SATA>; status = "disabled"; }; sata1: sata-port@1 { reg = <1>; phy = <&genphy 1 MODE_SATA>; status = "disabled"; }; }; pcie: pcie@ab01ff { compatible = "marvell,berlin-pcie"; reg = <0xab01ff 0x100>; pcie0: pcie-port@0 { reg = <0>; /* set phy on a per-board basis */ /* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */ /* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>; */ status = "disabled"; }; }; berlin2q-dmp.dts: &sata1 { status = "okay"; }; &pcie0 { phy = <&genphy 1 MODE_PCIE>; }; berlin2q-foo.dts: &pcie0 { phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>; }; For the driver, Antoine then would have to squeeze all PHY register mangling in phy-berlin2.c and see how to make ahci-platform aware of individual port nodes (I haven't looked up if it already exists, sorry) and announce only enabled port child nodes, right? Sebastian
On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote: > On 05/14/2014 06:57 PM, Antoine Ténart wrote: > > On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote: > >> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: > >>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: > > From what I understand from the conversation, we have a single PHY > register set dealing with both SATA ports available on the SoC. > Also, from the name of the PHY bits we assume the PHY may be able > to work in different modes than just SATA. And we currently have > an AHCI-compatible SATA IP that supports up to two ports, with one > actually connected to a SATA plug on the DMP board. > > Now, thinking about the PHY binding and the (possible) multi-protocol > support, it can be possible that on BG2Q there is a generic 2-lane > LVDS PHY that can be configured to support SATA or PCIe. Both are > electrically and bit-level compatible, so they could be internally > wired-up with AHCI and PCIe controller. Sounds like a reasonable guess. We have other PHY drivers doing the same thing already. > From a DT point-of-view, we need a way to (a) link each SATA or PCIe > port to the PHY, (b) specify the PHY lane to be used, and (c) specify > the protocol to be used on that lane. If I got it right, Arnd already > mentioned to use the phy-specifier to deal with it: > > e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE> Right. > Let's assume we have one dual-port SATA controller and one PCIe > controller with either x1 or x2 support. The only sane DT binding, > I can think of then would be: > > berlin2q.dtsi: > > genphy: lvds@ea00ff { > compatible = "marvell,berlin-lvds-phy"; > reg = <0xea00ff 0x100>; > #phy-cells = <2>; > }; > > sata: sata@ab00ff { > compatible = "ahci-platform"; > reg = <0xab00ff 0x100>; > > sata0: sata-port@0 { > reg = <0>; > phy = <&genphy 0 MODE_SATA>; > status = "disabled"; > }; > > sata1: sata-port@1 { > reg = <1>; > phy = <&genphy 1 MODE_SATA>; > status = "disabled"; > }; > }; > > pcie: pcie@ab01ff { > compatible = "marvell,berlin-pcie"; > reg = <0xab01ff 0x100>; > > pcie0: pcie-port@0 { > reg = <0>; > /* set phy on a per-board basis */ > /* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */ > /* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1 > MODE_PCIE>; */ > status = "disabled"; > }; > }; > > berlin2q-dmp.dts: > > &sata1 { > status = "okay"; > }; > > &pcie0 { > phy = <&genphy 1 MODE_PCIE>; > }; > > berlin2q-foo.dts: > > &pcie0 { > phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>; > }; Exactly. I would also be fine with keeping the sub-nodes of the phy device as in v3 and using #phy-cells=<1> instead of #phy-cells. The result would be pretty much the same, it just depends on how closely connected the two logical phys are. IIRC for the ST microelectronics PHY we recently reviewed, the same PHY could be driving multiple SATA ports, or a single multi-lane PCIe. In that case, it makes more sense to have #phy-cells=<2>, like your example. > For the driver, Antoine then would have to squeeze all PHY register > mangling in phy-berlin2.c and see how to make ahci-platform aware of > individual port nodes (I haven't looked up if it already exists, sorry) > and announce only enabled port child nodes, right? I've been thinking some more about this aspect. I don't actually have a strong opinion on whether it's better to use the generic ahci-platform driver, or to keep the multi-phy support as a special variant for berlin. If we do the latter, it would however be good to define the binding in a way that lets us later merge things into the generic phy driver in case we get more of the same. Arnd
On 05/14/2014 08:12 PM, Arnd Bergmann wrote: > On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote: >> On 05/14/2014 06:57 PM, Antoine Ténart wrote: >>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote: >>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: >>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: >> >> From what I understand from the conversation, we have a single PHY >> register set dealing with both SATA ports available on the SoC. >> Also, from the name of the PHY bits we assume the PHY may be able >> to work in different modes than just SATA. And we currently have >> an AHCI-compatible SATA IP that supports up to two ports, with one >> actually connected to a SATA plug on the DMP board. >> >> Now, thinking about the PHY binding and the (possible) multi-protocol >> support, it can be possible that on BG2Q there is a generic 2-lane >> LVDS PHY that can be configured to support SATA or PCIe. Both are >> electrically and bit-level compatible, so they could be internally >> wired-up with AHCI and PCIe controller. > > Sounds like a reasonable guess. We have other PHY drivers doing the > same thing already. Well, I based that on what I know about FPGA LVDS transceivers, so I wasn't guessing out of the blue ;) >> From a DT point-of-view, we need a way to (a) link each SATA or PCIe >> port to the PHY, (b) specify the PHY lane to be used, and (c) specify >> the protocol to be used on that lane. If I got it right, Arnd already >> mentioned to use the phy-specifier to deal with it: >> >> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE> > > Right. > >> Let's assume we have one dual-port SATA controller and one PCIe >> controller with either x1 or x2 support. The only sane DT binding, >> I can think of then would be: >> >> berlin2q.dtsi: >> >> genphy: lvds@ea00ff { >> compatible = "marvell,berlin-lvds-phy"; >> reg = <0xea00ff 0x100>; >> #phy-cells = <2>; >> }; >> >> sata: sata@ab00ff { >> compatible = "ahci-platform"; >> reg = <0xab00ff 0x100>; >> >> sata0: sata-port@0 { >> reg = <0>; >> phy = <&genphy 0 MODE_SATA>; >> status = "disabled"; >> }; >> >> sata1: sata-port@1 { >> reg = <1>; >> phy = <&genphy 1 MODE_SATA>; >> status = "disabled"; >> }; >> }; >> >> pcie: pcie@ab01ff { >> compatible = "marvell,berlin-pcie"; >> reg = <0xab01ff 0x100>; >> >> pcie0: pcie-port@0 { >> reg = <0>; >> /* set phy on a per-board basis */ >> /* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */ >> /* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1 >> MODE_PCIE>; */ >> status = "disabled"; >> }; >> }; >> >> berlin2q-dmp.dts: >> >> &sata1 { >> status = "okay"; >> }; >> >> &pcie0 { >> phy = <&genphy 1 MODE_PCIE>; >> }; >> >> berlin2q-foo.dts: >> >> &pcie0 { >> phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>; >> }; > > Exactly. I would also be fine with keeping the sub-nodes of the > phy device as in v3 and using #phy-cells=<1> instead of #phy-cells. > The result would be pretty much the same, it just depends on how > closely connected the two logical phys are. > > IIRC for the ST microelectronics PHY we recently reviewed, the > same PHY could be driving multiple SATA ports, or a single > multi-lane PCIe. In that case, it makes more sense to have > #phy-cells=<2>, like your example. I guess the final call is up to Kishon, but I personally prefer #phy-cells = <2>. It makes no functional difference though. >> For the driver, Antoine then would have to squeeze all PHY register >> mangling in phy-berlin2.c and see how to make ahci-platform aware of >> individual port nodes (I haven't looked up if it already exists, sorry) >> and announce only enabled port child nodes, right? > > I've been thinking some more about this aspect. I don't actually have > a strong opinion on whether it's better to use the generic ahci-platform > driver, or to keep the multi-phy support as a special variant for > berlin. If we do the latter, it would however be good to define the > binding in a way that lets us later merge things into the generic phy > driver in case we get more of the same. Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it? ahci-platform needs to know about the phy property and calls some helper that deals with the phy-specifier? About a generic _phy_ driver, I am not so sure if berlin is the best template right now ;) So, my call would be: - make ahci-platform aware of port sub-nodes and phy properties - have a berlin specific PHY driver Sebastian
On Wednesday 14 May 2014 20:42:16 Sebastian Hesselbarth wrote: > >> For the driver, Antoine then would have to squeeze all PHY register > >> mangling in phy-berlin2.c and see how to make ahci-platform aware of > >> individual port nodes (I haven't looked up if it already exists, sorry) > >> and announce only enabled port child nodes, right? > > > > I've been thinking some more about this aspect. I don't actually have > > a strong opinion on whether it's better to use the generic ahci-platform > > driver, or to keep the multi-phy support as a special variant for > > berlin. If we do the latter, it would however be good to define the > > binding in a way that lets us later merge things into the generic phy > > driver in case we get more of the same. > > Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it? > ahci-platform needs to know about the phy property and calls some > helper that deals with the phy-specifier? > > About a generic _phy_ driver, I am not so sure if berlin is the best > template right now > > So, my call would be: > - make ahci-platform aware of port sub-nodes and phy properties > - have a berlin specific PHY driver I'm not sure if we need sub-nodes per port, it should be enough to have an array of phys, plus a way to match them up with the ports. Arnd
On 05/14/2014 08:51 PM, Arnd Bergmann wrote: > On Wednesday 14 May 2014 20:42:16 Sebastian Hesselbarth wrote: >>>> For the driver, Antoine then would have to squeeze all PHY register >>>> mangling in phy-berlin2.c and see how to make ahci-platform aware of >>>> individual port nodes (I haven't looked up if it already exists, sorry) >>>> and announce only enabled port child nodes, right? >>> >>> I've been thinking some more about this aspect. I don't actually have >>> a strong opinion on whether it's better to use the generic ahci-platform >>> driver, or to keep the multi-phy support as a special variant for >>> berlin. If we do the latter, it would however be good to define the >>> binding in a way that lets us later merge things into the generic phy >>> driver in case we get more of the same. >> >> Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it? >> ahci-platform needs to know about the phy property and calls some >> helper that deals with the phy-specifier? >> >> About a generic _phy_ driver, I am not so sure if berlin is the best >> template right now >> >> So, my call would be: >> - make ahci-platform aware of port sub-nodes and phy properties >> - have a berlin specific PHY driver > > I'm not sure if we need sub-nodes per port, it should be enough > to have an array of phys, plus a way to match them up with the > ports. Actually, I'd love to see sub-nodes per port as it will allow to disabled unused ports on a per-board basis. I have this in mind for a long time for Kirkwood's SATA node already: Consider a board where you have the one available SATA plug connected to port 1. How would that work out with status = "disabled"/"okay" that doesn't allow array of strings obviously? Sebastian
On Wednesday 14 May 2014 20:56:29 Sebastian Hesselbarth wrote: > On 05/14/2014 08:51 PM, Arnd Bergmann wrote: > > On Wednesday 14 May 2014 20:42:16 Sebastian Hesselbarth wrote: > >>>> For the driver, Antoine then would have to squeeze all PHY register > >>>> mangling in phy-berlin2.c and see how to make ahci-platform aware of > >>>> individual port nodes (I haven't looked up if it already exists, sorry) > >>>> and announce only enabled port child nodes, right? > >>> > >>> I've been thinking some more about this aspect. I don't actually have > >>> a strong opinion on whether it's better to use the generic ahci-platform > >>> driver, or to keep the multi-phy support as a special variant for > >>> berlin. If we do the latter, it would however be good to define the > >>> binding in a way that lets us later merge things into the generic phy > >>> driver in case we get more of the same. > >> > >> Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it? > >> ahci-platform needs to know about the phy property and calls some > >> helper that deals with the phy-specifier? > >> > >> About a generic _phy_ driver, I am not so sure if berlin is the best > >> template right now > >> > >> So, my call would be: > >> - make ahci-platform aware of port sub-nodes and phy properties > >> - have a berlin specific PHY driver > > > > I'm not sure if we need sub-nodes per port, it should be enough > > to have an array of phys, plus a way to match them up with the > > ports. > > Actually, I'd love to see sub-nodes per port as it will allow to > disabled unused ports on a per-board basis. > > I have this in mind for a long time for Kirkwood's SATA node already: > Consider a board where you have the one available SATA plug connected > to port 1. How would that work out with status = "disabled"/"okay" that > doesn't allow array of strings obviously? A simple bit mask would work fine, but I see your point. Doing status="disabled" per port sounds nice. Arnd
Hi, On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote: > On 05/14/2014 08:12 PM, Arnd Bergmann wrote: >> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote: >>> On 05/14/2014 06:57 PM, Antoine Ténart wrote: >>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote: >>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: >>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: >>> >>> From what I understand from the conversation, we have a single PHY >>> register set dealing with both SATA ports available on the SoC. >>> Also, from the name of the PHY bits we assume the PHY may be able >>> to work in different modes than just SATA. And we currently have >>> an AHCI-compatible SATA IP that supports up to two ports, with one >>> actually connected to a SATA plug on the DMP board. >>> >>> Now, thinking about the PHY binding and the (possible) multi-protocol >>> support, it can be possible that on BG2Q there is a generic 2-lane >>> LVDS PHY that can be configured to support SATA or PCIe. Both are >>> electrically and bit-level compatible, so they could be internally >>> wired-up with AHCI and PCIe controller. >> >> Sounds like a reasonable guess. We have other PHY drivers doing the >> same thing already. > > Well, I based that on what I know about FPGA LVDS transceivers, so > I wasn't guessing out of the blue ;) > >>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe >>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify >>> the protocol to be used on that lane. If I got it right, Arnd already >>> mentioned to use the phy-specifier to deal with it: >>> >>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE> >> >> Right. >> >>> Let's assume we have one dual-port SATA controller and one PCIe >>> controller with either x1 or x2 support. The only sane DT binding, >>> I can think of then would be: >>> >>> berlin2q.dtsi: >>> >>> genphy: lvds@ea00ff { >>> compatible = "marvell,berlin-lvds-phy"; >>> reg = <0xea00ff 0x100>; >>> #phy-cells = <2>; >>> }; >>> >>> sata: sata@ab00ff { >>> compatible = "ahci-platform"; >>> reg = <0xab00ff 0x100>; >>> >>> sata0: sata-port@0 { >>> reg = <0>; >>> phy = <&genphy 0 MODE_SATA>; >>> status = "disabled"; >>> }; >>> >>> sata1: sata-port@1 { >>> reg = <1>; >>> phy = <&genphy 1 MODE_SATA>; >>> status = "disabled"; >>> }; >>> }; >>> >>> pcie: pcie@ab01ff { >>> compatible = "marvell,berlin-pcie"; >>> reg = <0xab01ff 0x100>; >>> >>> pcie0: pcie-port@0 { >>> reg = <0>; >>> /* set phy on a per-board basis */ >>> /* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */ >>> /* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1 >>> MODE_PCIE>; */ >>> status = "disabled"; >>> }; >>> }; >>> >>> berlin2q-dmp.dts: >>> >>> &sata1 { >>> status = "okay"; >>> }; >>> >>> &pcie0 { >>> phy = <&genphy 1 MODE_PCIE>; >>> }; >>> >>> berlin2q-foo.dts: >>> >>> &pcie0 { >>> phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>; >>> }; >> >> Exactly. I would also be fine with keeping the sub-nodes of the >> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells. >> The result would be pretty much the same, it just depends on how >> closely connected the two logical phys are. huh.. even with sub-nodes you'll need #phy-cells=<2> if we use a single *PHY PROVIDER*. Because with just PHYs node pointer we won't be able to get the PHY. We'll need PHY providers node pointer. However I'd prefer to have sub-nodes for each individual PHYs and register a single PHY PROVIDER. Thanks Kishon
On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote: > On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote: >> On 05/14/2014 08:12 PM, Arnd Bergmann wrote: >>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote: >>>> On 05/14/2014 06:57 PM, Antoine Ténart wrote: >>>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote: >>>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: >>>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: [...] >>>> Now, thinking about the PHY binding and the (possible) multi-protocol >>>> support, it can be possible that on BG2Q there is a generic 2-lane >>>> LVDS PHY that can be configured to support SATA or PCIe. Both are >>>> electrically and bit-level compatible, so they could be internally >>>> wired-up with AHCI and PCIe controller. >>> >>> Sounds like a reasonable guess. We have other PHY drivers doing the >>> same thing already. [...] >>>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe >>>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify >>>> the protocol to be used on that lane. If I got it right, Arnd already >>>> mentioned to use the phy-specifier to deal with it: >>>> >>>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE> >>> >>> Right. >>> >>>> Let's assume we have one dual-port SATA controller and one PCIe >>>> controller with either x1 or x2 support. The only sane DT binding, >>>> I can think of then would be: >>>> >>>> berlin2q.dtsi: >>>> >>>> genphy: lvds@ea00ff { >>>> compatible = "marvell,berlin-lvds-phy"; >>>> reg = <0xea00ff 0x100>; >>>> #phy-cells = <2>; >>>> }; >>>> >>>> sata: sata@ab00ff { >>>> compatible = "ahci-platform"; >>>> reg = <0xab00ff 0x100>; >>>> >>>> sata0: sata-port@0 { >>>> reg = <0>; >>>> phy = <&genphy 0 MODE_SATA>; >>>> status = "disabled"; >>>> }; >>>> >>>> sata1: sata-port@1 { >>>> reg = <1>; >>>> phy = <&genphy 1 MODE_SATA>; >>>> status = "disabled"; >>>> }; >>>> }; >>>> >>>> pcie: pcie@ab01ff { >>>> compatible = "marvell,berlin-pcie"; >>>> reg = <0xab01ff 0x100>; >>>> >>>> pcie0: pcie-port@0 { >>>> reg = <0>; >>>> /* set phy on a per-board basis */ >>>> /* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */ >>>> /* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1 >>>> MODE_PCIE>; */ >>>> status = "disabled"; >>>> }; >>>> }; >>>> >>>> berlin2q-dmp.dts: >>>> >>>> &sata1 { >>>> status = "okay"; >>>> }; >>>> >>>> &pcie0 { >>>> phy = <&genphy 1 MODE_PCIE>; >>>> }; >>>> >>>> berlin2q-foo.dts: >>>> >>>> &pcie0 { >>>> phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>; >>>> }; >>> >>> Exactly. I would also be fine with keeping the sub-nodes of the >>> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells. >>> The result would be pretty much the same, it just depends on how >>> closely connected the two logical phys are. > > huh.. even with sub-nodes you'll need #phy-cells=<2> if we use a single *PHY > PROVIDER*. Because with just PHYs node pointer we won't be able to get the PHY. > We'll need PHY providers node pointer. > > However I'd prefer to have sub-nodes for each individual PHYs and register a > single PHY PROVIDER. Depends on what you call PHY. In the example above the PHY is what allows you to control both lanes. So you want sub-nodes for each individual lane given the nomenclature of the example? Or like it is used in the example above, a single PHY node with an index in the phy-specifier to pick an individual lane. IMHO, having both phy-specifier index _and_ PHY sub-node per lane has no benefit at all. You cannot even use the PHY sub-nodes for any setup properties, as they depend on the consumer claiming the lane. Sebastian
Hi, On Thursday 15 May 2014 12:32 PM, Sebastian Hesselbarth wrote: > On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote: >> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote: >>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote: >>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote: >>>>> On 05/14/2014 06:57 PM, Antoine Ténart wrote: >>>>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote: >>>>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote: >>>>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote: > [...] >>>>> Now, thinking about the PHY binding and the (possible) multi-protocol >>>>> support, it can be possible that on BG2Q there is a generic 2-lane >>>>> LVDS PHY that can be configured to support SATA or PCIe. Both are >>>>> electrically and bit-level compatible, so they could be internally >>>>> wired-up with AHCI and PCIe controller. >>>> >>>> Sounds like a reasonable guess. We have other PHY drivers doing the >>>> same thing already. > [...] >>>>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe >>>>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify >>>>> the protocol to be used on that lane. If I got it right, Arnd already >>>>> mentioned to use the phy-specifier to deal with it: >>>>> >>>>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE> >>>> >>>> Right. >>>> >>>>> Let's assume we have one dual-port SATA controller and one PCIe >>>>> controller with either x1 or x2 support. The only sane DT binding, >>>>> I can think of then would be: >>>>> >>>>> berlin2q.dtsi: >>>>> >>>>> genphy: lvds@ea00ff { >>>>> compatible = "marvell,berlin-lvds-phy"; >>>>> reg = <0xea00ff 0x100>; >>>>> #phy-cells = <2>; >>>>> }; >>>>> >>>>> sata: sata@ab00ff { >>>>> compatible = "ahci-platform"; >>>>> reg = <0xab00ff 0x100>; >>>>> >>>>> sata0: sata-port@0 { >>>>> reg = <0>; >>>>> phy = <&genphy 0 MODE_SATA>; >>>>> status = "disabled"; >>>>> }; >>>>> >>>>> sata1: sata-port@1 { >>>>> reg = <1>; >>>>> phy = <&genphy 1 MODE_SATA>; >>>>> status = "disabled"; >>>>> }; >>>>> }; >>>>> >>>>> pcie: pcie@ab01ff { >>>>> compatible = "marvell,berlin-pcie"; >>>>> reg = <0xab01ff 0x100>; >>>>> >>>>> pcie0: pcie-port@0 { >>>>> reg = <0>; >>>>> /* set phy on a per-board basis */ >>>>> /* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */ >>>>> /* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1 >>>>> MODE_PCIE>; */ >>>>> status = "disabled"; >>>>> }; >>>>> }; >>>>> >>>>> berlin2q-dmp.dts: >>>>> >>>>> &sata1 { >>>>> status = "okay"; >>>>> }; >>>>> >>>>> &pcie0 { >>>>> phy = <&genphy 1 MODE_PCIE>; >>>>> }; >>>>> >>>>> berlin2q-foo.dts: >>>>> >>>>> &pcie0 { >>>>> phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>; >>>>> }; >>>> >>>> Exactly. I would also be fine with keeping the sub-nodes of the >>>> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells. >>>> The result would be pretty much the same, it just depends on how >>>> closely connected the two logical phys are. >> >> huh.. even with sub-nodes you'll need #phy-cells=<2> if we use a single *PHY >> PROVIDER*. Because with just PHYs node pointer we won't be able to get the PHY. >> We'll need PHY providers node pointer. >> >> However I'd prefer to have sub-nodes for each individual PHYs and register a >> single PHY PROVIDER. > > Depends on what you call PHY. In the example above the PHY is what > allows you to control both lanes. > > So you want sub-nodes for each individual lane given the nomenclature > of the example? > > Or like it is used in the example above, a single PHY node with an index > in the phy-specifier to pick an individual lane. > > IMHO, having both phy-specifier index _and_ PHY sub-node per lane > has no benefit at all. You cannot even use the PHY sub-nodes for any > setup properties, as they depend on the consumer claiming the lane. IMO the dt data should completely describe the HW. So just by looking at the PHY node, we won't be able to tell the no of PHYs implemented in the IP if we have a single PHY node (In this case the lanes in the IP). However if you think it's an overkill for having sub-nodes for each lane then single PHY node is fine too. Thanks Kishon
On 05/15/2014 10:46 AM, Kishon Vijay Abraham I wrote: > On Thursday 15 May 2014 12:32 PM, Sebastian Hesselbarth wrote: >> On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote: >>> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote: >>>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote: >>>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote: >>>>>> Let's assume we have one dual-port SATA controller and one PCIe >>>>>> controller with either x1 or x2 support. The only sane DT binding, >>>>>> I can think of then would be: >>>>>> >>>>>> berlin2q.dtsi: >>>>>> >>>>>> genphy: lvds@ea00ff { >>>>>> compatible = "marvell,berlin-lvds-phy"; >>>>>> reg = <0xea00ff 0x100>; >>>>>> #phy-cells = <2>; >>>>>> }; [...] >> >> Depends on what you call PHY. In the example above the PHY is what >> allows you to control both lanes. >> >> So you want sub-nodes for each individual lane given the nomenclature >> of the example? >> >> Or like it is used in the example above, a single PHY node with an index >> in the phy-specifier to pick an individual lane. >> >> IMHO, having both phy-specifier index _and_ PHY sub-node per lane >> has no benefit at all. You cannot even use the PHY sub-nodes for any >> setup properties, as they depend on the consumer claiming the lane. > > IMO the dt data should completely describe the HW. So just by looking at the > PHY node, we won't be able to tell the no of PHYs implemented in the IP if we > have a single PHY node (In this case the lanes in the IP). > > However if you think it's an overkill for having sub-nodes for each lane then > single PHY node is fine too. Yeah, I see your point. I just wonder how many Marvell PHYs we may hit that require the _same_ magic setup inside but have _different_ number of lanes. And even if, we can deal with it using a different compatible string. Currently, I feel a single PHY provider node and a set of compatibles will be most likely, i.e. no per-lane sub-nodes. OTOH, the per-lane sub-nodes is more generic as it allows us to deal PHYs that may suddenly skip one lane in the numbering scheme. The difference for the driver is marginal, i.e. some SoC-specific struct with a field for the number of lanes vs. of_count_child_nodes() and a reg = <n> property for the per-lane sub-nodes. I used to agree to "DT should descibe HW", but with no datasheet available, it quickly becomes fuzzy what it really looks like. Anyway, I'll have some discussion with Antoine and Alexandre to sort out the differences and the things in common for the PHY and SoCs in question. Sebastian
On Thursday 15 May 2014 02:47 PM, Sebastian Hesselbarth wrote: > On 05/15/2014 10:46 AM, Kishon Vijay Abraham I wrote: >> On Thursday 15 May 2014 12:32 PM, Sebastian Hesselbarth wrote: >>> On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote: >>>> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote: >>>>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote: >>>>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote: >>>>>>> Let's assume we have one dual-port SATA controller and one PCIe >>>>>>> controller with either x1 or x2 support. The only sane DT binding, >>>>>>> I can think of then would be: >>>>>>> >>>>>>> berlin2q.dtsi: >>>>>>> >>>>>>> genphy: lvds@ea00ff { >>>>>>> compatible = "marvell,berlin-lvds-phy"; >>>>>>> reg = <0xea00ff 0x100>; >>>>>>> #phy-cells = <2>; >>>>>>> }; > [...] >>> >>> Depends on what you call PHY. In the example above the PHY is what >>> allows you to control both lanes. >>> >>> So you want sub-nodes for each individual lane given the nomenclature >>> of the example? >>> >>> Or like it is used in the example above, a single PHY node with an index >>> in the phy-specifier to pick an individual lane. >>> >>> IMHO, having both phy-specifier index _and_ PHY sub-node per lane >>> has no benefit at all. You cannot even use the PHY sub-nodes for any >>> setup properties, as they depend on the consumer claiming the lane. >> >> IMO the dt data should completely describe the HW. So just by looking at the >> PHY node, we won't be able to tell the no of PHYs implemented in the IP if we >> have a single PHY node (In this case the lanes in the IP). >> >> However if you think it's an overkill for having sub-nodes for each lane then >> single PHY node is fine too. > > Yeah, I see your point. I just wonder how many Marvell PHYs we may hit > that require the _same_ magic setup inside but have _different_ number > of lanes. And even if, we can deal with it using a different compatible > string. > > Currently, I feel a single PHY provider node and a set of compatibles > will be most likely, i.e. no per-lane sub-nodes. OTOH, the per-lane > sub-nodes is more generic as it allows us to deal PHYs that may > suddenly skip one lane in the numbering scheme. The difference for the > driver is marginal, i.e. some SoC-specific struct with a field for the > number of lanes vs. of_count_child_nodes() and a reg = <n> property for > the per-lane sub-nodes. > > I used to agree to "DT should descibe HW", but with no datasheet > available, it quickly becomes fuzzy what it really looks like. > > Anyway, I'll have some discussion with Antoine and Alexandre to sort out > the differences and the things in common for the PHY and SoCs in > question. cool, thanks. -Kishon
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 4906c27fa3bd..b31b1986fda4 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -15,6 +15,11 @@ 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 + bool + depends on ARCH_BERLIN && OF + select GENERIC_PHY + 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 7728518572a4..40278706ac1b 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..b08f76329a34 --- /dev/null +++ b/drivers/phy/phy-berlin-sata.c @@ -0,0 +1,180 @@ +/* + * 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/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 CONTROL_REGISTER 0x0 +#define MBUS_SIZE_CONTROL 0x4 + +#define POWER_DOWN_SATA0 BIT(6) +#define POWER_DOWN_SATA1 BIT(14) +#define MBUS_WRITE_REQUEST_SIZE_128 (BIT(2) << 16) +#define MBUS_READ_REQUEST_SIZE_128 (BIT(2) << 19) + +#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 phy_berlin_desc phys[BERLIN_SATA_PHY_NB]; +}; + +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; +} + +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 struct phy_berlin_desc desc[] = { + { .val = POWER_DOWN_SATA0 }, + { .val = POWER_DOWN_SATA1 }, + { }, +}; + +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; + struct device_node *child; + u8 phy_id; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + priv->base = devm_ioremap(dev, res->start, resource_size(res)); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + + dev_set_drvdata(dev, priv); + spin_lock_init(&priv->lock); + + for_each_child_of_node(dev->of_node, child) { + if (of_property_read_u8(child, "reg", &phy_id)) + return -EINVAL; + + if (phy_id >= BERLIN_SATA_PHY_NB) { + dev_err(dev, "invalid reg in node %s\n", child->name); + return -EINVAL; + } + + phy = devm_phy_create(dev, &phy_berlin_sata_ops, NULL); + if (IS_ERR(phy)) { + dev_err(dev, "failed to create PHY for node %s\n", + child->name); + return PTR_ERR(phy); + } + + /* + * 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]); + + /* Make sure the PHY is off */ + phy_berlin_sata_power_off(phy); + + phy_provider = devm_of_phy_provider_register(&phy->dev, + of_phy_simple_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,berlin-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. Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> --- drivers/phy/Kconfig | 5 ++ drivers/phy/Makefile | 1 + drivers/phy/phy-berlin-sata.c | 180 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 186 insertions(+) create mode 100644 drivers/phy/phy-berlin-sata.c