| Message ID | 1382365111-6533-5-git-send-email-t.stanislaws@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Monday 21 October 2013 07:48 PM, Tomasz Stanislawski wrote: > Add simple-phy driver to support a single register > PHY interfaces present on Exynos4 SoC. How are these PHY interfaces modelled in the SoC? Where does the register actually reside? > > Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> > --- > drivers/phy/Kconfig | 5 ++ > drivers/phy/Makefile | 1 + > drivers/phy/phy-simple.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 134 insertions(+) > create mode 100644 drivers/phy/phy-simple.c > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig > index ac239ac..619c657 100644 > --- a/drivers/phy/Kconfig > +++ b/drivers/phy/Kconfig > @@ -38,4 +38,9 @@ config TWL4030_USB > This transceiver supports high and full speed devices plus, > in host mode, low speed. > > +config PHY_SIMPLE > + tristate "Simple PHY driver" This is too generic a name to be used. Lets name it something specific to what it is used for (EXYNOS/HDMI.. ?). > + help > + Support for PHY controllers configured using single register. > + > endmenu > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile > index 0dd8a98..3d68e19 100644 > --- a/drivers/phy/Makefile > +++ b/drivers/phy/Makefile > @@ -5,3 +5,4 @@ > obj-$(CONFIG_GENERIC_PHY) += phy-core.o > obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o > obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o > +obj-$(CONFIG_PHY_SIMPLE) += phy-simple.o > diff --git a/drivers/phy/phy-simple.c b/drivers/phy/phy-simple.c > new file mode 100644 > index 0000000..4a28af7 > --- /dev/null > +++ b/drivers/phy/phy-simple.c > @@ -0,0 +1,128 @@ > +/* > + * Simple PHY driver > + * > + * Copyright (C) 2013 Samsung Electronics Co., Ltd. > + * Author: Tomasz Stanislawski <t.stanislaws@samsung.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/spinlock.h> > + > +struct simple_phy { > + spinlock_t slock; > + u32 on_value; > + u32 off_value; > + u32 mask; > + void __iomem *regs; > +}; > + > +static int sphy_set(struct simple_phy *sphy, bool on) > +{ > + u32 reg; > + > + spin_lock(&sphy->slock); Lets add spin_lock only when it is absolutely necessary. When your PHY provider implements only a single PHY, it is not needed. phy_power_on and phy_power_off is already protected by the framework. > + > + reg = readl(sphy->regs); > + reg &= ~sphy->mask; > + reg |= sphy->mask & (on ? sphy->on_value : sphy->off_value); > + writel(reg, sphy->regs); > + > + spin_unlock(&sphy->slock); > + > + return 0; > +} > + > +static int simple_phy_power_on(struct phy *phy) > +{ > + return sphy_set(phy_get_drvdata(phy), 1); > +} > + > +static int simple_phy_power_off(struct phy *phy) > +{ > + return sphy_set(phy_get_drvdata(phy), 0); > +} > + > +static struct phy_ops simple_phy_ops = { > + .power_on = simple_phy_power_on, > + .power_off = simple_phy_power_off, > + .owner = THIS_MODULE, > +}; > + > +static int simple_phy_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct simple_phy *sphy; > + struct resource *res; > + struct phy_provider *phy_provider; > + struct phy *phy; > + > + sphy = devm_kzalloc(dev, sizeof(*sphy), GFP_KERNEL); > + if (!sphy) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + sphy->regs = devm_ioremap_resource(dev, res); > + if (IS_ERR(sphy->regs)) { > + dev_err(dev, "failed to ioremap registers\n"); > + return PTR_ERR(sphy->regs); > + } > + > + spin_lock_init(&sphy->slock); > + > + phy_provider = devm_of_phy_provider_register(dev, NULL); pass 'of_phy_simple_xlate' instead of NULL. > + if (IS_ERR(phy_provider)) { > + dev_err(dev, "failed to register PHY provider\n"); > + return PTR_ERR(phy_provider); > + } > + > + phy = devm_phy_create(dev, &simple_phy_ops, NULL); > + if (IS_ERR(phy)) { > + dev_err(dev, "failed to create PHY\n"); > + return PTR_ERR(phy); > + } > + > + sphy->mask = 1; > + sphy->on_value = ~0; > + sphy->off_value = 0; > + > + of_property_read_u32(dev->of_node, "mask", &sphy->mask); This means your driver will depend on dt data to describe how the register should look like. Not a good idea. > + of_property_read_u32(dev->of_node, "on-value", &sphy->on_value); > + of_property_read_u32(dev->of_node, "off-value", &sphy->off_value); > + > + phy_set_drvdata(phy, sphy); > + > + dev_info(dev, "probe successful\n"); Lets not make the boot noisy. Thanks Kishon
Hi, Please refer to the comments below. On 10/24/2013 05:52 PM, Kishon Vijay Abraham I wrote: > Hi, > > On Monday 21 October 2013 07:48 PM, Tomasz Stanislawski wrote: >> Add simple-phy driver to support a single register >> PHY interfaces present on Exynos4 SoC. > > How are these PHY interfaces modelled in the SoC? Where does the register > actually reside? Initially, I was planning to add PHY for HDMI_PHY register in power management register set on s5pv310 soc. However other PHYs use very similar interface (setting bit 0). This includes DAC_PHY, ADC_PHY, PCIe_PHY, SATA_PHY. Moreover it suits well to USBDEVICE_PHY, USBHOST_PHY. That is why I thought about using something more generic to handle all those phys without introducing a herd of 200-lines-long drivers. >> >> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> >> --- >> drivers/phy/Kconfig | 5 ++ >> drivers/phy/Makefile | 1 + >> drivers/phy/phy-simple.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 134 insertions(+) >> create mode 100644 drivers/phy/phy-simple.c >> >> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >> index ac239ac..619c657 100644 >> --- a/drivers/phy/Kconfig >> +++ b/drivers/phy/Kconfig >> @@ -38,4 +38,9 @@ config TWL4030_USB >> This transceiver supports high and full speed devices plus, >> in host mode, low speed. >> >> +config PHY_SIMPLE >> + tristate "Simple PHY driver" > > This is too generic a name to be used. Lets name it something specific to what > it is used for (EXYNOS/HDMI.. ?). Ok. It could be renamed to EXYNOS-SIMPLE-PHY or EXYNOS-1BIT-PHY or EXYNOS-GENERIC-PHY or something similar. Any ideas? >> + help >> + Support for PHY controllers configured using single register. >> + >> endmenu >> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile >> index 0dd8a98..3d68e19 100644 >> --- a/drivers/phy/Makefile >> +++ b/drivers/phy/Makefile >> @@ -5,3 +5,4 @@ >> obj-$(CONFIG_GENERIC_PHY) += phy-core.o >> obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o >> obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o >> +obj-$(CONFIG_PHY_SIMPLE) += phy-simple.o >> diff --git a/drivers/phy/phy-simple.c b/drivers/phy/phy-simple.c >> new file mode 100644 >> index 0000000..4a28af7 >> --- /dev/null >> +++ b/drivers/phy/phy-simple.c >> @@ -0,0 +1,128 @@ >> +/* >> + * Simple PHY driver >> + * >> + * Copyright (C) 2013 Samsung Electronics Co., Ltd. >> + * Author: Tomasz Stanislawski <t.stanislaws@samsung.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include <linux/io.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/phy/phy.h> >> +#include <linux/platform_device.h> >> +#include <linux/spinlock.h> >> + >> +struct simple_phy { >> + spinlock_t slock; >> + u32 on_value; >> + u32 off_value; >> + u32 mask; >> + void __iomem *regs; >> +}; >> + >> +static int sphy_set(struct simple_phy *sphy, bool on) >> +{ >> + u32 reg; >> + >> + spin_lock(&sphy->slock); > > Lets add spin_lock only when it is absolutely necessary. When your PHY provider > implements only a single PHY, it is not needed. phy_power_on and phy_power_off > is already protected by the framework. ok >> + >> + reg = readl(sphy->regs); >> + reg &= ~sphy->mask; >> + reg |= sphy->mask & (on ? sphy->on_value : sphy->off_value); >> + writel(reg, sphy->regs); >> + >> + spin_unlock(&sphy->slock); >> + >> + return 0; >> +} >> + >> +static int simple_phy_power_on(struct phy *phy) >> +{ >> + return sphy_set(phy_get_drvdata(phy), 1); >> +} >> + >> +static int simple_phy_power_off(struct phy *phy) >> +{ >> + return sphy_set(phy_get_drvdata(phy), 0); >> +} >> + >> +static struct phy_ops simple_phy_ops = { >> + .power_on = simple_phy_power_on, >> + .power_off = simple_phy_power_off, >> + .owner = THIS_MODULE, >> +}; >> + >> +static int simple_phy_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct simple_phy *sphy; >> + struct resource *res; >> + struct phy_provider *phy_provider; >> + struct phy *phy; >> + >> + sphy = devm_kzalloc(dev, sizeof(*sphy), GFP_KERNEL); >> + if (!sphy) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + >> + sphy->regs = devm_ioremap_resource(dev, res); >> + if (IS_ERR(sphy->regs)) { >> + dev_err(dev, "failed to ioremap registers\n"); >> + return PTR_ERR(sphy->regs); >> + } >> + >> + spin_lock_init(&sphy->slock); >> + >> + phy_provider = devm_of_phy_provider_register(dev, NULL); > > pass 'of_phy_simple_xlate' instead of NULL. >> + if (IS_ERR(phy_provider)) { >> + dev_err(dev, "failed to register PHY provider\n"); >> + return PTR_ERR(phy_provider); >> + } >> + >> + phy = devm_phy_create(dev, &simple_phy_ops, NULL); >> + if (IS_ERR(phy)) { >> + dev_err(dev, "failed to create PHY\n"); >> + return PTR_ERR(phy); >> + } >> + >> + sphy->mask = 1; >> + sphy->on_value = ~0; >> + sphy->off_value = 0; >> + >> + of_property_read_u32(dev->of_node, "mask", &sphy->mask); > > This means your driver will depend on dt data to describe how the register > should look like. Not a good idea. I can remove it. No problem. The driver can justt use fixed mask=1,on-value=1,off-value=0. Adding mentioned attributes greatly improves driver flexibility but such a flexibility is not needed currently for s5pv310 phys. But frankly, I do not exactly follow what is a rationale for such police in DT. It forces developer to write a lot of redundant code. Moreover, some clock drivers seams to violate it. Clock "picochip,pc3x3-gated-clk" is an example. One can find similar tricks in pinctrl. >> + of_property_read_u32(dev->of_node, "on-value", &sphy->on_value); >> + of_property_read_u32(dev->of_node, "off-value", &sphy->off_value); >> + >> + phy_set_drvdata(phy, sphy); >> + >> + dev_info(dev, "probe successful\n"); > Lets not make the boot noisy. > ok. s/dev_info/dev_dbg is good enough? > Thanks > Kishon > Could you take a look on other patches in this RFC? Regards, Tomasz Stanislawski
Hi, On Friday 25 October 2013 01:21 PM, Tomasz Stanislawski wrote: > Hi, > Please refer to the comments below. > > On 10/24/2013 05:52 PM, Kishon Vijay Abraham I wrote: >> Hi, >> >> On Monday 21 October 2013 07:48 PM, Tomasz Stanislawski wrote: >>> Add simple-phy driver to support a single register >>> PHY interfaces present on Exynos4 SoC. >> >> How are these PHY interfaces modelled in the SoC? Where does the register >> actually reside? > > Initially, I was planning to add PHY for HDMI_PHY register in > power management register set on s5pv310 soc. If that register is part of the power management register space, I don't think it justifies creating a PHY driver for it. > > However other PHYs use very similar interface (setting bit 0). > This includes DAC_PHY, ADC_PHY, PCIe_PHY, SATA_PHY. > Moreover it suits well to USBDEVICE_PHY, USBHOST_PHY. How is it currently being done for these drivers? Is it being done in the patches sent by Kamil or Vivek? Thanks Kishon
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index ac239ac..619c657 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -38,4 +38,9 @@ config TWL4030_USB This transceiver supports high and full speed devices plus, in host mode, low speed. +config PHY_SIMPLE + tristate "Simple PHY driver" + help + Support for PHY controllers configured using single register. + endmenu diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index 0dd8a98..3d68e19 100644 --- a/drivers/phy/Makefile +++ b/drivers/phy/Makefile @@ -5,3 +5,4 @@ obj-$(CONFIG_GENERIC_PHY) += phy-core.o obj-$(CONFIG_OMAP_USB2) += phy-omap-usb2.o obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o +obj-$(CONFIG_PHY_SIMPLE) += phy-simple.o diff --git a/drivers/phy/phy-simple.c b/drivers/phy/phy-simple.c new file mode 100644 index 0000000..4a28af7 --- /dev/null +++ b/drivers/phy/phy-simple.c @@ -0,0 +1,128 @@ +/* + * Simple PHY driver + * + * Copyright (C) 2013 Samsung Electronics Co., Ltd. + * Author: Tomasz Stanislawski <t.stanislaws@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/phy/phy.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +struct simple_phy { + spinlock_t slock; + u32 on_value; + u32 off_value; + u32 mask; + void __iomem *regs; +}; + +static int sphy_set(struct simple_phy *sphy, bool on) +{ + u32 reg; + + spin_lock(&sphy->slock); + + reg = readl(sphy->regs); + reg &= ~sphy->mask; + reg |= sphy->mask & (on ? sphy->on_value : sphy->off_value); + writel(reg, sphy->regs); + + spin_unlock(&sphy->slock); + + return 0; +} + +static int simple_phy_power_on(struct phy *phy) +{ + return sphy_set(phy_get_drvdata(phy), 1); +} + +static int simple_phy_power_off(struct phy *phy) +{ + return sphy_set(phy_get_drvdata(phy), 0); +} + +static struct phy_ops simple_phy_ops = { + .power_on = simple_phy_power_on, + .power_off = simple_phy_power_off, + .owner = THIS_MODULE, +}; + +static int simple_phy_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct simple_phy *sphy; + struct resource *res; + struct phy_provider *phy_provider; + struct phy *phy; + + sphy = devm_kzalloc(dev, sizeof(*sphy), GFP_KERNEL); + if (!sphy) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + sphy->regs = devm_ioremap_resource(dev, res); + if (IS_ERR(sphy->regs)) { + dev_err(dev, "failed to ioremap registers\n"); + return PTR_ERR(sphy->regs); + } + + spin_lock_init(&sphy->slock); + + phy_provider = devm_of_phy_provider_register(dev, NULL); + if (IS_ERR(phy_provider)) { + dev_err(dev, "failed to register PHY provider\n"); + return PTR_ERR(phy_provider); + } + + phy = devm_phy_create(dev, &simple_phy_ops, NULL); + if (IS_ERR(phy)) { + dev_err(dev, "failed to create PHY\n"); + return PTR_ERR(phy); + } + + sphy->mask = 1; + sphy->on_value = ~0; + sphy->off_value = 0; + + of_property_read_u32(dev->of_node, "mask", &sphy->mask); + of_property_read_u32(dev->of_node, "on-value", &sphy->on_value); + of_property_read_u32(dev->of_node, "off-value", &sphy->off_value); + + phy_set_drvdata(phy, sphy); + + dev_info(dev, "probe successful\n"); + + return 0; +} + +static const struct of_device_id simple_phy_of_match[] = { + { .compatible = "simple-phy" }, + { }, +}; +MODULE_DEVICE_TABLE(of, simple_phy_of_match); + +static struct platform_driver simple_phy_driver = { + .probe = simple_phy_probe, + .driver = { + .of_match_table = simple_phy_of_match, + .name = "simple-phy", + .owner = THIS_MODULE, + } +}; +module_platform_driver(simple_phy_driver); + +MODULE_DESCRIPTION("Simple PHY driver"); +MODULE_AUTHOR("Tomasz Stanislawski <t.stanislaws@samsung.com>"); +MODULE_LICENSE("GPL v2");
Add simple-phy driver to support a single register PHY interfaces present on Exynos4 SoC. Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> --- drivers/phy/Kconfig | 5 ++ drivers/phy/Makefile | 1 + drivers/phy/phy-simple.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 drivers/phy/phy-simple.c