Hi Petre, Thank you for the patch! I'm also developing a similar driver now :) (I don't submit it though...) My developing driver has SSC and VBUS_EN setting. Anyway, I have some comments about your patch. > -----Original Message----- > From: Petre Pircalabu > Sent: Thursday, May 18, 2017 2:58 AM > > The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock > source. The 2 available options are the differential input clock pair > supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip > clock source supplied through USB_XTAL/USB_EXTAL. > > The device can be configured to use the on-chip source by adding the > "renesas,use-on-chip-clk" option in the corresponding device tree node. > > Signed-off-by: Petre Pircalabu <Petre_Pircalabu@xxxxxxxxxx> > --- > .../devicetree/bindings/phy/rcar-gen3-phy-usb3.txt | 22 +++ > arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi | 8 + > arch/arm64/configs/defconfig | 1 + > drivers/phy/Kconfig | 8 + > drivers/phy/Makefile | 1 + > drivers/phy/phy-rcar-gen3-usb3.c | 166 +++++++++++++++++++++ I think you should separate 3 patches as below: 1) Add phy-rcar-gen3-usb3 driver 2) Add a device node to r8a7795-es1.dtsi 3) Enable the phy driver on defconfig And when we submit a generic phy driver's patch (e.g. 1) above), we should send to the phy maintainer(s). Also, when we submit a patch that is related to device tree(e.g. both 1) and 2) above), we should send to the device tree maintainer(s). > diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt > b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt > new file mode 100644 > index 0000000..aa9657c > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt > @@ -0,0 +1,22 @@ > +* Renesas R-Car generation 3 USB 3.0 PHY > + > +This file provides information on what the device node for the R-Car generation > +3 USB 3.0 PHY contains. > + > +Required properties: > +- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 compatible device. I would like to add "renesas,usb3-phy-r8a7795" and "renesas,usb3-phy-r8a7796". > +- reg: offset and length of the partial USB 3.0 Host PHY register block. > +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>. I think we should add "clocks" property as required. > +Optional properties: You should add "renesas,use-on-chip-clk" here. And, the name of "use-on-chip-clk" is not good to me. FYI, my developing patch names "renesas,usb-extal". > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile > index a534cf5..aeda949 100644 > --- a/drivers/phy/Makefile > +++ b/drivers/phy/Makefile > @@ -21,6 +21,7 @@ obj-$(CONFIG_PHY_MIPHY28LP) += phy-miphy28lp.o > obj-$(CONFIG_PHY_MIPHY365X) += phy-miphy365x.o > obj-$(CONFIG_PHY_RCAR_GEN2) += phy-rcar-gen2.o > obj-$(CONFIG_PHY_RCAR_GEN3_USB2) += phy-rcar-gen3-usb2.o > +obj-$(CONFIG_PHY_RCAR_GEN3_USB3) += phy-rcar-gen3-usb3.o The latest linux-phy.git / next branch doesn't update yet though, the directory will be changed to ./drivers/phy/renesas. http://www.spinics.net/lists/linux-usb/msg156875.html > diff --git a/drivers/phy/phy-rcar-gen3-usb3.c b/drivers/phy/phy-rcar-gen3-usb3.c > new file mode 100644 > index 0000000..87fa24d > --- /dev/null > +++ b/drivers/phy/phy-rcar-gen3-usb3.c > @@ -0,0 +1,166 @@ > +/* > + * Renesas R-Car Gen3 for USB3.0 PHY driver > + * > + * Copyright (C) 2017 Mentor Graphics Inc. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY > + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + */ > + > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> We can remove this "linux/of_address.h". > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > + > +#define USB30_CLKSET0 0x34 > +#define USB30_CLKSET1 0x36 > + > +#define USB30_CLKSET0_FSEL_MASK 0x003F > +#define USB30_CLKSET0_FSEL_USB_XTAL 0x0002 > +#define USB30_CLKSET0_FSEL_USB3S0_CLK 0x0027 > + > +#define USB30_CLKSET1_PLL_MULTI_MASK 0x1FC0 > +#define USB30_CLKSET1_PLL_MULTI_USB_XTAL (0x64 << 6) > +#define USB30_CLKSET1_PLL_MULTI_USB3S0_CLK (0x00 << 6) I prefer the "6" becomes a definition. > +#define USB30_CLKSET1_REF_CLKDIV BIT(3) > +#define USB30_CLKSET1_USB_SEL BIT(0) > + > +struct rcar_gen3_usb3_phy { > + void __iomem *base; > + struct phy *phy; > + u8 use_on_chip_clk; > +}; > + > +static int rcar_gen3_phy_usb3_init(struct phy *p) > +{ > + struct rcar_gen3_usb3_phy *phy_dev = phy_get_drvdata(p); > + void __iomem *usb3_base = phy_dev->base; > + > + u16 clkset0, clkset1; > + > + clkset0 = readw(usb3_base + USB30_CLKSET0); > + clkset1 = readw(usb3_base + USB30_CLKSET1); > + > + dev_dbg(&p->dev, "USB30_CLKSET0 initial value = 0x%04X\n", clkset0); > + dev_dbg(&p->dev, "USB30_CLKSET1 initial value = 0x%04X\n", clkset1); I guess the generic phy maintainer prefer dev_vdbg() (like phy-rcar-gen3-usb2.c). > + clkset0 &= ~USB30_CLKSET0_FSEL_MASK; > + clkset1 &= ~(USB30_CLKSET1_PLL_MULTI_MASK | USB30_CLKSET1_REF_CLKDIV | > + USB30_CLKSET1_USB_SEL); > + > + if (phy_dev->use_on_chip_clk) { > + /* Select 50MHz clock */ > + dev_info(&p->dev, "USE USB_XTAL clock (50MHz)\n"); > + clkset0 |= USB30_CLKSET0_FSEL_USB_XTAL; > + clkset1 |= USB30_CLKSET1_PLL_MULTI_USB_XTAL | > + USB30_CLKSET1_REF_CLKDIV; > + clkset1 &= ~USB30_CLKSET1_USB_SEL; Since USB30_CLKSET1_USB_SEL bit is cleared before, I don't think this code needs. > + } else { > + /* Select 100MHz clock */ > + dev_info(&p->dev, "USE USB3S0_CLK reference (100MHz)\n"); > + clkset0 |= USB30_CLKSET0_FSEL_USB3S0_CLK; > + clkset1 |= USB30_CLKSET1_PLL_MULTI_USB3S0_CLK | > + USB30_CLKSET1_USB_SEL; > + clkset1 &= ~USB30_CLKSET1_REF_CLKDIV; Since USB30_CLKSET1_REF_CLKDIV bit is cleared before, I don't think this code needs. > + } > + > + dev_dbg(&p->dev, "USB30_CLKSET0 new value = 0x%04X\n", clkset0); > + dev_dbg(&p->dev, "USB30_CLKSET1 new value = 0x%04X\n", clkset1); > + > + writew(clkset0, usb3_base + USB30_CLKSET0); > + writew(clkset1, usb3_base + USB30_CLKSET1); > + > + return 0; > +} > + > +static int rcar_gen3_phy_usb3_exit(struct phy *p) > +{ > + return 0; > +} We can remove this function. > + > +static struct phy_ops rcar_gen3_phy_usb3_ops = { > + .init = rcar_gen3_phy_usb3_init, > + .exit = rcar_gen3_phy_usb3_exit, We can remove this .exit. > + .owner = THIS_MODULE, > +}; > + > +static const struct of_device_id rcar_gen3_phy_usb3_match_table[] = { > + { .compatible = "renesas,rcar-gen3-usb3-phy" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, rcar_gen3_phy_usb3_match_table); > + > +static int rcar_gen3_phy_usb3_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rcar_gen3_usb3_phy *phy_dev; > + struct phy_provider *provider; > + struct resource *res; > + > + if (!dev->of_node) { > + dev_err(dev, "This driver needs a device tree node\n"); > + return -EINVAL; > + } > + > + phy_dev = devm_kzalloc(dev, sizeof(*phy_dev), GFP_KERNEL); > + if (!phy_dev) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + phy_dev->base = devm_ioremap_resource(dev, res); > + if (IS_ERR(phy_dev->base)) > + return PTR_ERR(phy_dev->base); > + > + phy_dev->use_on_chip_clk = device_property_read_bool(dev, > + "renesas,use-on-chip-clk"); > + > + pm_runtime_enable(dev); > + phy_dev->phy = devm_phy_create(dev, NULL, &rcar_gen3_phy_usb3_ops); > + if (IS_ERR(phy_dev->phy)) { You should call pm_runtime_disable(dev); here. I prefer using "goto" and though :) > + dev_err(dev, "Failed to create Rcar Gen3 USB3 PHY\n"); > + return PTR_ERR(phy_dev->phy); > + } > + > + platform_set_drvdata(pdev, phy_dev); > + phy_set_drvdata(phy_dev->phy, phy_dev); > + > + provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > + if (IS_ERR(provider)) { You should call pm_runtime_disable(dev); here. > + dev_err(dev, "Failed to register PHY provider\n"); > + return PTR_ERR(provider); > + } > + > + dev_info(&pdev->dev, "Initialized RCAR Gen3 USB3 PHY module\n"); > + return 0; > +} > + > +static int rcar_gen3_phy_usb3_remove(struct platform_device *pdev) > +{ > + pm_runtime_disable(&pdev->dev); > + return 0; > +} > + > +static struct platform_driver rcar_gen3_phy_usb3_driver = { > + .driver = { > + .name = "phy_rcar_gen3_usb3", > + .of_match_table = rcar_gen3_phy_usb3_match_table, > + }, > + .probe = rcar_gen3_phy_usb3_probe, > + .remove = rcar_gen3_phy_usb3_remove, > +}; > +module_platform_driver(rcar_gen3_phy_usb3_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Renesas R-Car Gen3 USB 3.0 PHY"); > +MODULE_AUTHOR("Petre Pircalabu <petre_pircalabu@xxxxxxxxxx>"); > -- > 1.9.1
