RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux