Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1363569 > unrolled thread

Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY Sekhar Nori <nsekhar@ti.com> - 2016-03-23 18:30 +0100
    Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY David Lechner <david@lechnology.com> - 2016-03-23 19:10 +0100
      RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB  PHY David Laight <David.Laight@ACULAB.COM> - 2016-03-24 15:10 +0100

#1363569 — Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> This is a new phy driver for the SoC USB controllers on the TI DA8XX
> family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
> The USB 2.0 PHY also allows overriding the VBUS and ID pins.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
> new file mode 100644
> index 0000000..93a5f4d
> --- /dev/null
> +++ b/drivers/phy/phy-da8xx-usb.c
> @@ -0,0 +1,295 @@
> +/*
> + * phy-da8xx-usb - TI DaVinci DA8XX USB PHY driver
> + *
> + * Copyright (C) 2016 David Lechner <david@lechnology.com>
> + *
> + * 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/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/musb.h>
> +#include <linux/usb/otg.h>
> +
> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> +#define PHYCLKGD		(1 << 17)
> +#define VBUSSENSE		(1 << 16)
> +#define RESET			(1 << 15)
> +#define OTGMODE_MASK		(3 << 13)
> +#define NO_OVERRIDE		(0 << 13)
> +#define FORCE_HOST		(1 << 13)
> +#define FORCE_DEVICE		(2 << 13)
> +#define FORCE_HOST_VBUS_LOW	(3 << 13)
> +#define USB1PHYCLKMUX		(1 << 12)
> +#define USB2PHYCLKMUX		(1 << 11)
> +#define PHYPWRDN		(1 << 10)
> +#define OTGPWRDN		(1 << 9)
> +#define DATPOL			(1 << 8)
> +#define USB1SUSPENDM		(1 << 7)
> +#define PHY_PLLON		(1 << 6)
> +#define SESENDEN		(1 << 5)
> +#define VBDTCTEN		(1 << 4)
> +#define REFFREQ_MASK		(0xf << 0)
> +#define REFFREQ_12MHZ		(1 << 0)
> +#define REFFREQ_24MHZ		(2 << 0)
> +#define REFFREQ_48MHZ		(3 << 0)
> +#define REFFREQ_19_2MHZ		(4 << 0)
> +#define REFFREQ_38_4MHZ		(5 << 0)
> +#define REFFREQ_13MHZ		(6 << 0)
> +#define REFFREQ_26MHZ		(7 << 0)
> +#define REFFREQ_20MHZ		(8 << 0)
> +#define REFFREQ_40MHZ		(9 << 0)

Many of these register bits are unused. I guess opinion varies around
this, but I get confused with unnecessary bit definitions and register
offsets. I tend to search for it and its sort of disappointing to see
that its basically unused. Of course, you should wait for PHY
maintainers preference.

> +
> +struct da8xx_usbphy {
> +	struct phy_provider	*phy_provider;
> +	struct phy		*usb11_phy;
> +	struct phy		*usb20_phy;
> +	struct clk		*usb11_clk;
> +	struct clk		*usb20_clk;
> +	void __iomem		*phy_ctrl;
> +};
> +
> +static inline u32 da8xx_usbphy_readl(void __iomem *base)
> +{
> +	return readl(base);
> +}
> +
> +static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
> +{
> +	writel(value, base);
> +}

Agree with Sergei that these don't add much value.

> +int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode)
> +{
> +	struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> +	u32 val;
> +
> +	val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> +
> +	val &= ~OTGMODE_MASK;
> +	switch (mode) {
> +	case MUSB_HOST:		/* Force VBUS valid, ID = 0 */
> +		val |= FORCE_HOST;
> +		break;
> +	case MUSB_PERIPHERAL:	/* Force VBUS valid, ID = 1 */
> +		val |= FORCE_DEVICE;
> +		break;
> +	case MUSB_OTG:		/* Don't override the VBUS/ID comparators */
> +		val |= NO_OVERRIDE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);

Hmm, since this driver exports this symbol, I think it should also
provide an include file in include/linux/phy for users of the symbol. Or
perhaps there should be a generic API around this since it looks like
most USB phys will need something similar?

Thanks,
Sekhar

[toc] | [next] | [standalone]

#1363592

On 03/23/2016 12:21 PM, Sekhar Nori wrote:
>> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
>> +#define PHYCLKGD		(1 << 17)
>> +#define VBUSSENSE		(1 << 16)
>> +#define RESET			(1 << 15)
>> +#define OTGMODE_MASK		(3 << 13)
>> +#define NO_OVERRIDE		(0 << 13)
>> +#define FORCE_HOST		(1 << 13)
>> +#define FORCE_DEVICE		(2 << 13)
>> +#define FORCE_HOST_VBUS_LOW	(3 << 13)
>> +#define USB1PHYCLKMUX		(1 << 12)
>> +#define USB2PHYCLKMUX		(1 << 11)
>> +#define PHYPWRDN		(1 << 10)
>> +#define OTGPWRDN		(1 << 9)
>> +#define DATPOL			(1 << 8)
>> +#define USB1SUSPENDM		(1 << 7)
>> +#define PHY_PLLON		(1 << 6)
>> +#define SESENDEN		(1 << 5)
>> +#define VBDTCTEN		(1 << 4)
>> +#define REFFREQ_MASK		(0xf << 0)
>> +#define REFFREQ_12MHZ		(1 << 0)
>> +#define REFFREQ_24MHZ		(2 << 0)
>> +#define REFFREQ_48MHZ		(3 << 0)
>> +#define REFFREQ_19_2MHZ		(4 << 0)
>> +#define REFFREQ_38_4MHZ		(5 << 0)
>> +#define REFFREQ_13MHZ		(6 << 0)
>> +#define REFFREQ_26MHZ		(7 << 0)
>> +#define REFFREQ_20MHZ		(8 << 0)
>> +#define REFFREQ_40MHZ		(9 << 0)
>
> Many of these register bits are unused. I guess opinion varies around
> this, but I get confused with unnecessary bit definitions and register
> offsets. I tend to search for it and its sort of disappointing to see
> that its basically unused. Of course, you should wait for PHY
> maintainers preference.

My thinking was that since this driver *owns* the CFGCHIP2 register that 
is would eventually register clocks using the common clock framework, in 
which case, it would use many of these registers.

But, based on feedback on another commit, if we go the syscon route, 
then yes, I will remove the unused values.


>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>
> Hmm, since this driver exports this symbol, I think it should also
> provide an include file in include/linux/phy for users of the symbol. Or
> perhaps there should be a generic API around this since it looks like
> most USB phys will need something similar?
>

The reason I didn't make a header file is that this function is only 
meant to be use in one place and would likely cause a crash if used 
anywhere else.

I agree that it would be nice if the generic phy driver had a mechanism 
for user-defined callbacks such as this, however, I think the scope of 
this patch series has grown enough already.

[toc] | [prev] | [next] | [standalone]

#1364181 — RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

From: David Lechner
> Sent: 23 March 2016 18:07
> On 03/23/2016 12:21 PM, Sekhar Nori wrote:
> >> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> >> +#define PHYCLKGD		(1 << 17)
> >> +#define VBUSSENSE		(1 << 16)
> >> +#define RESET			(1 << 15)
> >> +#define OTGMODE_MASK		(3 << 13)
> >> +#define NO_OVERRIDE		(0 << 13)
> >> +#define FORCE_HOST		(1 << 13)
> >> +#define FORCE_DEVICE		(2 << 13)
> >> +#define FORCE_HOST_VBUS_LOW	(3 << 13)

I think I'd define the above as:
#define OTG_MODE(x) ((x) << 13)
#define OTGMODE_MASK		OTG_MODE(3)
#define NO_OVERRIDE		OTG_MODE(0)
#define FORCE_HOST		OTG_MODE(1)
#define FORCE_DEVICE		OTG_MODE(2)
#define FORCE_HOST_VBUS_LOW	OTG_MODE(3)
Then realise that all the names need changing (to include OTG).

> >> +#define USB1PHYCLKMUX		(1 << 12)
> >> +#define USB2PHYCLKMUX		(1 << 11)
> >> +#define PHYPWRDN		(1 << 10)
> >> +#define OTGPWRDN		(1 << 9)
> >> +#define DATPOL			(1 << 8)
> >> +#define USB1SUSPENDM		(1 << 7)
> >> +#define PHY_PLLON		(1 << 6)
> >> +#define SESENDEN		(1 << 5)
> >> +#define VBDTCTEN		(1 << 4)
> >> +#define REFFREQ_MASK		(0xf << 0)
> >> +#define REFFREQ_12MHZ		(1 << 0)
> >> +#define REFFREQ_24MHZ		(2 << 0)
> >> +#define REFFREQ_48MHZ		(3 << 0)
> >> +#define REFFREQ_19_2MHZ		(4 << 0)
> >> +#define REFFREQ_38_4MHZ		(5 << 0)
> >> +#define REFFREQ_13MHZ		(6 << 0)
> >> +#define REFFREQ_26MHZ		(7 << 0)
> >> +#define REFFREQ_20MHZ		(8 << 0)
> >> +#define REFFREQ_40MHZ		(9 << 0)

I'd define these similarly to the OTGxxx values.

> > Many of these register bits are unused. I guess opinion varies around
> > this, but I get confused with unnecessary bit definitions and register
> > offsets. I tend to search for it and its sort of disappointing to see
> > that its basically unused. Of course, you should wait for PHY
> > maintainers preference.

It can be useful to see what isn't being set.
Sometimes this can be very useful when making changes to a driver.
For status values it is particularly important to know what all the bits mean.

> My thinking was that since this driver *owns* the CFGCHIP2 register that
> is would eventually register clocks using the common clock framework, in
> which case, it would use many of these registers.
> 
> But, based on feedback on another commit, if we go the syscon route,
> then yes, I will remove the unused values.
> 
> 
> >> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
> >
> > Hmm, since this driver exports this symbol, I think it should also
> > provide an include file in include/linux/phy for users of the symbol. Or
> > perhaps there should be a generic API around this since it looks like
> > most USB phys will need something similar?
> >
> 
> The reason I didn't make a header file is that this function is only
> meant to be use in one place and would likely cause a crash if used
> anywhere else.

And a compilation error if compiled with -Wmissing-prototypes.

Sounds like you need a header file just for this driver's 'private' exports.

IMHO .c files shouldn't contain 'extern' anywhere.
I removed a load from some local code and found quite a few lurking bugs
where the types didn't match.

	David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web