Re: [PATCH v5] phy: Renesas R-Car Gen2 PHY driver

[Kishon Vijay Abraham I <kishon@ti.com>]

Hi,

On Wednesday 23 July 2014 12:57 AM, Sergei Shtylyov wrote:
.
.
<snip>
.
.

> Index: linux-phy/drivers/phy/phy-rcar-gen2.c
> =================================> --- /dev/null
> +++ linux-phy/drivers/phy/phy-rcar-gen2.c
> @@ -0,0 +1,341 @@
> +/*
> + * Renesas R-Car Gen2 PHY driver
> + *
> + * Copyright (C) 2014 Renesas Solutions Corp.
> + * Copyright (C) 2014 Cogent Embedded, Inc.
> + *
> + * 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/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#include <asm/cmpxchg.h>
> +
> +#define USBHS_LPSTS			0x02
> +#define USBHS_UGCTRL			0x80
> +#define USBHS_UGCTRL2			0x84
> +#define USBHS_UGSTS			0x88	/* The manuals have 0x90 */
> +
> +/* Low Power Status register (LPSTS) */
> +#define USBHS_LPSTS_SUSPM		0x4000
> +
> +/* USB General control register (UGCTRL) */
> +#define USBHS_UGCTRL_CONNECT		0x00000004
> +#define USBHS_UGCTRL_PLLRESET		0x00000001
> +
> +/* USB General control register 2 (UGCTRL2) */
> +#define USBHS_UGCTRL2_USB2SEL		0x80000000
> +#define USBHS_UGCTRL2_USB2SEL_PCI	0x00000000
> +#define USBHS_UGCTRL2_USB2SEL_USB30	0x80000000
> +#define USBHS_UGCTRL2_USB0SEL		0x00000030
> +#define USBHS_UGCTRL2_USB0SEL_PCI	0x00000010
> +#define USBHS_UGCTRL2_USB0SEL_HS_USB	0x00000030
> +
> +/* USB General status register (UGSTS) */
> +#define USBHS_UGSTS_LOCK		0x00000300 /* The manuals have 0x3 */
> +
> +#define PHYS_PER_CHANNEL	2
> +
> +struct rcar_gen2_phy {
> +	struct phy *phy;
> +	struct rcar_gen2_channel *channel;
> +	int number;
> +	u32 select_value;
> +};
> +
> +struct rcar_gen2_channel {
> +	struct device_node *of_node;
> +	struct rcar_gen2_phy_driver *drv;
> +	struct rcar_gen2_phy phys[PHYS_PER_CHANNEL];
> +	int selected_phy;
> +	u32 select_mask;
> +};
> +
> +struct rcar_gen2_phy_driver {
> +	void __iomem *base;
> +	struct clk *clk;
> +	spinlock_t lock;
> +	int num_channels;
> +	struct rcar_gen2_channel *channels;
> +};
> +
> +static int rcar_gen2_phy_init(struct phy *p)
> +{
> +	struct rcar_gen2_phy *phy = phy_get_drvdata(p);
> +	struct rcar_gen2_channel *channel = phy->channel;
> +	struct rcar_gen2_phy_driver *drv = channel->drv;
> +	unsigned long flags;
> +	u32 ugctrl2;
> +
> +	/*
> +	 * Try to acquire exclusive access to PHY.  The first driver calling
> +	 * phy_init()  on a given channel wins, and all attempts  to use another
> +	 * PHY on this channel will fail until phy_exit() is called by the first
> +	 * driver.   Achieving this with cmpxcgh() should be SMP-safe.
> +	 */
> +	if (cmpxchg(&channel->selected_phy, -1, phy->number) != -1)
> +		return -EBUSY;

This should be done in phy_get no?

Can we add this in phy-core? There might be other users who want to have
exclusive access within the same phy provider.

Rest of it looks fine to me.

Thanks
Kishon