Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-28 Thread Tony Lindgren 
* kbuild test robot  [180219 07:47]:
>  > 354if (reset_gpio >= 0)
>355gpiod_set_value_cansleep(reset_gpio, 1);

Thanks this test can be just dropped. Will send out an updated
version shortly.

Regards,

Tony

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-28 Thread Tony Lindgren 
* kbuild test robot  [180219 07:47]:
>  > 354if (reset_gpio >= 0)
>355gpiod_set_value_cansleep(reset_gpio, 1);

Thanks this test can be just dropped. Will send out an updated
version shortly.

Regards,

Tony

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-28 Thread Tony Lindgren 
* Rob Herring  [180219 20:41]:
> On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> > The GPIOs controlling MDM6600 are used to power MDM660 on and off, to
> 
> MDM660 a typo?

Thanks fixing.

> > +Required properties:
> > +- compatible   Must be "motorola,mapphone-mdm6600"
> > +- enable-gpios GPIO to enable the USB PHY
> > +- power-gpios  GPIO to power on the device
> > +- reset-gpios  GPIO to reset the device
> 
> The are pretty standard, but...
> 
> > +- mode-gpios   Two GPIOs to configure MDM6600 USB start-up mode for
> > +   normal mode versus USB flashing mode
> > +- status-gpios Three GPIOs to read the power state of the MDM6600
> > +- cmd-gpiosThree GPIOs to control the power state of the MDM6600
> 
> These 3 should have vendor a prefix.

OK

> > +
> > +Example:
> > +
> > +fsusb1_phy: fsusb1_phy {
> 
> usb-phy {

Thanks will send out an updated version shortly.

Regards,

Tony

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-28 Thread Tony Lindgren 
* Rob Herring  [180219 20:41]:
> On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> > The GPIOs controlling MDM6600 are used to power MDM660 on and off, to
> 
> MDM660 a typo?

Thanks fixing.

> > +Required properties:
> > +- compatible   Must be "motorola,mapphone-mdm6600"
> > +- enable-gpios GPIO to enable the USB PHY
> > +- power-gpios  GPIO to power on the device
> > +- reset-gpios  GPIO to reset the device
> 
> The are pretty standard, but...
> 
> > +- mode-gpios   Two GPIOs to configure MDM6600 USB start-up mode for
> > +   normal mode versus USB flashing mode
> > +- status-gpios Three GPIOs to read the power state of the MDM6600
> > +- cmd-gpiosThree GPIOs to control the power state of the MDM6600
> 
> These 3 should have vendor a prefix.

OK

> > +
> > +Example:
> > +
> > +fsusb1_phy: fsusb1_phy {
> 
> usb-phy {

Thanks will send out an updated version shortly.

Regards,

Tony

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-19 Thread Rob Herring 
On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> Let's add support for the GPIO controlled USB PHY on the MDM6600 modem.
> It is used on some Motorola Mapphone series of phones and tablets such
> as Droid 4.
> 
> The MDM6600 is hardwired to the first OHCI port in the Droid 4 case, and
> is controlled by several GPIOs. The USB PHY is integrated into the MDM6600
> device it seems. We know this as we get L3 errors from omap-usb-host if
> trying to use the PHY before MDM6600 is configured.
> 
> The GPIOs controlling MDM6600 are used to power MDM660 on and off, to

MDM660 a typo?

> configure the USB start-up mode (normal mode versus USB flashing), and
> they also tell the state of the MDM6600 device.
> 
> The two start-up mode GPIOs are dual-purposed and used for out of band
> (OOB) wake-up for USB and TS 27.010 serial mux. But we need to configure
> the USB start-up mode first to get MDM6600 booted in the right mode to
> be usable in the first place.
> 
> For now, this driver just gives up the two start-up mode GPIOs after the
> modem has been configured to boot in normal mode. One of them we may
> want to configure for USB OOB wake in this driver later on, but that's a
> separate series of patches and needs more work.
> 
> Note that the Motorola Mapphone Linux kernel tree has a "radio-ctrl"
> driver for modems. But it really does not control the radio at all, it
> just controls the modem power and start-up mode for USB. So I came to
> the conclusion that we're better off having this done in the USB PHY
> driver. For adding support for USB flashing mode, we can later on add
> a kernel module option for flash_mode=1 or something similar.
> 
> Also note that currently there is no PM runtime support for the OHCI
> on omap variant SoCs. So for low(er) power idle states, currenty both
> ohci-platform and phy-mapphone-mdm6600 must be unloaded or unbound.
> 
> For reference here is what I measured for total power consumption on
> an idle Droid 4 with and without USB related MDM6600 modules:
> 
> idle lcd off  phy-mapphone-mdm6600ohci-platform
> 153mW 284mW   344mW
> 
> So it seems that MDM6600 is currently not yet idling even with it's
> radio turned off, but that's something that is beyond the control of
> this USB PHY driver.
> 
> Cc: devicet...@vger.kernel.org
> Cc: Mark Rutland 
> Cc: Marcel Partap 
> Cc: Michael Scott 
> Cc: Rob Herring 
> Cc: Sebastian Reichel 
> Signed-off-by: Tony Lindgren 
> ---
>  .../bindings/phy/phy-mapphone-mdm6600.txt  |  30 ++
>  drivers/phy/motorola/Kconfig   |   9 +
>  drivers/phy/motorola/Makefile  |   1 +
>  drivers/phy/motorola/phy-mapphone-mdm6600.c| 490 
> +
>  4 files changed, 530 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
>  create mode 100644 drivers/phy/motorola/phy-mapphone-mdm6600.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt 
> b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> @@ -0,0 +1,30 @@
> +Device tree binding documentation for Motorola Mapphone MDM6600 USB PHY
> +
> +Required properties:
> +- compatible Must be "motorola,mapphone-mdm6600"
> +- enable-gpios   GPIO to enable the USB PHY
> +- power-gpiosGPIO to power on the device
> +- reset-gpiosGPIO to reset the device

The are pretty standard, but...

> +- mode-gpios Two GPIOs to configure MDM6600 USB start-up mode for
> + normal mode versus USB flashing mode
> +- status-gpios   Three GPIOs to read the power state of the MDM6600
> +- cmd-gpios  Three GPIOs to control the power state of the MDM6600

These 3 should have vendor a prefix.

> +
> +Example:
> +
> +fsusb1_phy: fsusb1_phy {

usb-phy {

> + compatible = "motorola,mapphone-mdm6600";
> + enable-gpios = < 31 GPIO_ACTIVE_LOW>; /* gpio_95 */
> + power-gpios = < 22 GPIO_ACTIVE_HIGH>; /* gpio_54 */
> + reset-gpios = < 17 GPIO_ACTIVE_HIGH>; /* gpio_49 */
> + mode-gpios = < 20 GPIO_ACTIVE_HIGH>,  /* gpio_148 */
> +  < 21 GPIO_ACTIVE_HIGH>;  /* gpio_149 */
> + status-gpios = < 23 GPIO_ACTIVE_HIGH>,/* gpio_55 */
> +< 21 GPIO_ACTIVE_HIGH>,/* gpio_53 */
> +< 20 GPIO_ACTIVE_HIGH>;/* gpio_52 */
> + cmd-gpios = < 7 GPIO_ACTIVE_HIGH>,/* gpio_103 */
> + < 8 GPIO_ACTIVE_HIGH>,/* gpio_104 */
> + < 14 GPIO_ACTIVE_HIGH>;   /* gpio_142 */
> + #phy-cells = <0>;
> +};
> +

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-19 Thread Rob Herring 
On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> Let's add support for the GPIO controlled USB PHY on the MDM6600 modem.
> It is used on some Motorola Mapphone series of phones and tablets such
> as Droid 4.
> 
> The MDM6600 is hardwired to the first OHCI port in the Droid 4 case, and
> is controlled by several GPIOs. The USB PHY is integrated into the MDM6600
> device it seems. We know this as we get L3 errors from omap-usb-host if
> trying to use the PHY before MDM6600 is configured.
> 
> The GPIOs controlling MDM6600 are used to power MDM660 on and off, to

MDM660 a typo?

> configure the USB start-up mode (normal mode versus USB flashing), and
> they also tell the state of the MDM6600 device.
> 
> The two start-up mode GPIOs are dual-purposed and used for out of band
> (OOB) wake-up for USB and TS 27.010 serial mux. But we need to configure
> the USB start-up mode first to get MDM6600 booted in the right mode to
> be usable in the first place.
> 
> For now, this driver just gives up the two start-up mode GPIOs after the
> modem has been configured to boot in normal mode. One of them we may
> want to configure for USB OOB wake in this driver later on, but that's a
> separate series of patches and needs more work.
> 
> Note that the Motorola Mapphone Linux kernel tree has a "radio-ctrl"
> driver for modems. But it really does not control the radio at all, it
> just controls the modem power and start-up mode for USB. So I came to
> the conclusion that we're better off having this done in the USB PHY
> driver. For adding support for USB flashing mode, we can later on add
> a kernel module option for flash_mode=1 or something similar.
> 
> Also note that currently there is no PM runtime support for the OHCI
> on omap variant SoCs. So for low(er) power idle states, currenty both
> ohci-platform and phy-mapphone-mdm6600 must be unloaded or unbound.
> 
> For reference here is what I measured for total power consumption on
> an idle Droid 4 with and without USB related MDM6600 modules:
> 
> idle lcd off  phy-mapphone-mdm6600ohci-platform
> 153mW 284mW   344mW
> 
> So it seems that MDM6600 is currently not yet idling even with it's
> radio turned off, but that's something that is beyond the control of
> this USB PHY driver.
> 
> Cc: devicet...@vger.kernel.org
> Cc: Mark Rutland 
> Cc: Marcel Partap 
> Cc: Michael Scott 
> Cc: Rob Herring 
> Cc: Sebastian Reichel 
> Signed-off-by: Tony Lindgren 
> ---
>  .../bindings/phy/phy-mapphone-mdm6600.txt  |  30 ++
>  drivers/phy/motorola/Kconfig   |   9 +
>  drivers/phy/motorola/Makefile  |   1 +
>  drivers/phy/motorola/phy-mapphone-mdm6600.c| 490 
> +
>  4 files changed, 530 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
>  create mode 100644 drivers/phy/motorola/phy-mapphone-mdm6600.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt 
> b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> @@ -0,0 +1,30 @@
> +Device tree binding documentation for Motorola Mapphone MDM6600 USB PHY
> +
> +Required properties:
> +- compatible Must be "motorola,mapphone-mdm6600"
> +- enable-gpios   GPIO to enable the USB PHY
> +- power-gpiosGPIO to power on the device
> +- reset-gpiosGPIO to reset the device

The are pretty standard, but...

> +- mode-gpios Two GPIOs to configure MDM6600 USB start-up mode for
> + normal mode versus USB flashing mode
> +- status-gpios   Three GPIOs to read the power state of the MDM6600
> +- cmd-gpios  Three GPIOs to control the power state of the MDM6600

These 3 should have vendor a prefix.

> +
> +Example:
> +
> +fsusb1_phy: fsusb1_phy {

usb-phy {

> + compatible = "motorola,mapphone-mdm6600";
> + enable-gpios = < 31 GPIO_ACTIVE_LOW>; /* gpio_95 */
> + power-gpios = < 22 GPIO_ACTIVE_HIGH>; /* gpio_54 */
> + reset-gpios = < 17 GPIO_ACTIVE_HIGH>; /* gpio_49 */
> + mode-gpios = < 20 GPIO_ACTIVE_HIGH>,  /* gpio_148 */
> +  < 21 GPIO_ACTIVE_HIGH>;  /* gpio_149 */
> + status-gpios = < 23 GPIO_ACTIVE_HIGH>,/* gpio_55 */
> +< 21 GPIO_ACTIVE_HIGH>,/* gpio_53 */
> +< 20 GPIO_ACTIVE_HIGH>;/* gpio_52 */
> + cmd-gpios = < 7 GPIO_ACTIVE_HIGH>,/* gpio_103 */
> + < 8 GPIO_ACTIVE_HIGH>,/* gpio_104 */
> + < 14 GPIO_ACTIVE_HIGH>;   /* gpio_142 */
> + #phy-cells = <0>;
> +};
> +

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-18 Thread kbuild test robot 
Hi Tony,

I love your patch! Perhaps something to improve:

[auto build test WARNING on phy/next]
[also build test WARNING on v4.16-rc2 next-20180216]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Tony-Lindgren/phy-mapphone-mdm6600-Add-USB-PHY-driver-for-MDM6600-on-Droid-4/20180219-131127
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git 
next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: sparse: incompatible 
>> types for operation (>=)
   drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: left side has type 
struct gpio_desc
   drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: right side has type int
>> drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: sparse: incorrect type 
>> in conditional
   drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: got bad type

vim +354 drivers/phy/motorola/phy-mapphone-mdm6600.c

   340  
   341  /**
   342   * phy_mdm6600_device_power_off() - power off mdm6600 device
   343   * @ddata: device driver data
   344   */
   345  static void phy_mdm6600_device_power_off(struct phy_mdm6600 *ddata)
   346  {
   347  struct gpio_desc *reset_gpio =
   348  ddata->gpio[PHY_MDM6600_RESET];
   349  
   350  ddata->enabled = false;
   351  phy_mdm6600_cmd(ddata, AP_STATUS_BP_SHUTDOWN_REQ);
   352  msleep(100);
   353  
 > 354  if (reset_gpio >= 0)
   355  gpiod_set_value_cansleep(reset_gpio, 1);
   356  
   357  dev_info(ddata->dev, "Waiting for power down request to 
complete.. ");
   358  if (wait_for_completion_timeout(>ack,
   359  msecs_to_jiffies(5000))) {
   360  dev_info(ddata->dev, "Powered down OK\n");
   361  } else {
   362  dev_err(ddata->dev, "Timed out powering down\n");
   363  }
   364  }
   365  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-18 Thread kbuild test robot 
Hi Tony,

I love your patch! Perhaps something to improve:

[auto build test WARNING on phy/next]
[also build test WARNING on v4.16-rc2 next-20180216]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Tony-Lindgren/phy-mapphone-mdm6600-Add-USB-PHY-driver-for-MDM6600-on-Droid-4/20180219-131127
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git 
next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: sparse: incompatible 
>> types for operation (>=)
   drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: left side has type 
struct gpio_desc
   drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: right side has type int
>> drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: sparse: incorrect type 
>> in conditional
   drivers/phy/motorola/phy-mapphone-mdm6600.c:354:24: got bad type

vim +354 drivers/phy/motorola/phy-mapphone-mdm6600.c

   340  
   341  /**
   342   * phy_mdm6600_device_power_off() - power off mdm6600 device
   343   * @ddata: device driver data
   344   */
   345  static void phy_mdm6600_device_power_off(struct phy_mdm6600 *ddata)
   346  {
   347  struct gpio_desc *reset_gpio =
   348  ddata->gpio[PHY_MDM6600_RESET];
   349  
   350  ddata->enabled = false;
   351  phy_mdm6600_cmd(ddata, AP_STATUS_BP_SHUTDOWN_REQ);
   352  msleep(100);
   353  
 > 354  if (reset_gpio >= 0)
   355  gpiod_set_value_cansleep(reset_gpio, 1);
   356  
   357  dev_info(ddata->dev, "Waiting for power down request to 
complete.. ");
   358  if (wait_for_completion_timeout(>ack,
   359  msecs_to_jiffies(5000))) {
   360  dev_info(ddata->dev, "Powered down OK\n");
   361  } else {
   362  dev_err(ddata->dev, "Timed out powering down\n");
   363  }
   364  }
   365  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-18 Thread Tony Lindgren 
* Sebastian Reichel  [180218 00:32]:
> On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> > For reference here is what I measured for total power consumption on
> > an idle Droid 4 with and without USB related MDM6600 modules:
> > 
> > idle lcd offphy-mapphone-mdm6600ohci-platform
> > 153mW   284mW   344mW
> 
> So more than twice the idle power... We really want to get runtime
> PM working :/

Yes and we want' modem to idle too :)

> > +/*
> > + * MDM6600 status codes. These are copied from Motorola Mapphone Linux
> > + * kernel tree. The BB naming here refers to "BaseBand" for modem.
> > + */
> 
> Actually your status codes are BP_ (baseband processor) prefixed.

I'll just get rid of the naming and stick to MDM6600 prefixies.
No need to keep the confusing BP/AP prefixes.

> > +static void phy_mdm6600_init_irq(struct phy_mdm6600 *ddata)
> > +{
> > +   struct device *dev = ddata->dev;
> > +   int i, error, irq;
> > +
> > +   for (i = PHY_MDM6600_STATUS0;
> > +i <= PHY_MDM6600_STATUS2; i++) {
> > +   if (IS_ERR(ddata->gpio[i]))
> > +   continue;
> 
> This can be dropped, since the driver errors out of probe
> when there are gpio errors.

True will drop.

> > +static int phy_mdm6600_init_lines(struct phy_mdm6600 *ddata)
> > +{
> > +   struct device *dev = ddata->dev;
> > +   int i, j, nr_gpio = 0;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(phy_mdm6600_line_map); i++) {
> > +   const struct phy_mdm6600_map *map =
> > +   _mdm6600_line_map[i];
> > +
> > +   for (j = 0; j < map->nr_gpios; j++) {
> > +   struct gpio_desc **gpio = >gpio[nr_gpio];
> > +
> > +   *gpio = devm_gpiod_get_index(dev,
> > +map->name, j,
> > +map->direction);
> > +   if (IS_ERR(*gpio)) {
> > +   dev_info(dev,
> > +"gpio %s error %li, already taken?\n",
> > +map->name, PTR_ERR(*gpio));
> > +   return PTR_ERR(*gpio);
> > +   }
> > +   nr_gpio++;
> > +   }
> 
> I think the code should use the gpiod_get_array() API.

OK thanks will take a look.

> > +   /* Give up shared GPIOs now, they will be used for OOB wake */
> > +   devm_gpiod_put(ddata->dev, mode_gpio0);
> > +   ddata->gpio[PHY_MDM6600_MODE0] = ERR_PTR(-ENODEV);
> > +   devm_gpiod_put(ddata->dev, mode_gpio1);
> > +   ddata->gpio[PHY_MDM6600_MODE0] = ERR_PTR(-ENODEV);
> 
> You want PHY_MDM6600_MODE1 instead. Also I would just use NULL.
> NULL is used by gpiod_get_optional and is handled by the gpiod
> functions, so you don't need to check for gpio errors everywhere.

Oops yup let's just drop this until we know what to do in the
further patches.

> > +#ifdef CONFIG_OF
> > +static const struct of_device_id phy_mdm6600_id_table[] = {
> > +   { .compatible = "motorola,mapphone-mdm6600", },
> > +   {},
> > +};
> > +MODULE_DEVICE_TABLE(of, phy_mdm6600_id_table);
> > +#endif
> 
> I suggest to just depend on CONFIG_OF in Kconfig and drop the ifdef
> and of_match_ptr() parts. It's very unlikely, that this will be
> used without DT and would need quite some rework anyways.

OK

> > +static int phy_mdm6600_probe(struct platform_device *pdev)
> > +{
> > +   struct phy_mdm6600 *ddata;
> > +   struct usb_otg *otg;
> > +   const struct of_device_id *of_id;
> > +   int error;
> > +
> > +   of_id = of_match_device(of_match_ptr(phy_mdm6600_id_table),
> > +   >dev);
> > +   if (!of_id)
> > +   return -EINVAL;
> 
> I suggest to drop the of_match_device(). The driver will error
> out anyways when it can't get the gpios.

OK

> Generally I'm a bit worried about handling the mode gpios in two
> different drivers. It looks like it might become a dependency hell.

Yeah you're right. That will require the modules to be loaded
in the right order. It's probably best that we handle the mdm6600
to SoC wake-up in this driver. And then maybe export a function for
toggling the SoC to mdm660 wake to make sure the dependencies are
clear for the modules.

Regards,

Tony

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-18 Thread Tony Lindgren 
* Sebastian Reichel  [180218 00:32]:
> On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> > For reference here is what I measured for total power consumption on
> > an idle Droid 4 with and without USB related MDM6600 modules:
> > 
> > idle lcd offphy-mapphone-mdm6600ohci-platform
> > 153mW   284mW   344mW
> 
> So more than twice the idle power... We really want to get runtime
> PM working :/

Yes and we want' modem to idle too :)

> > +/*
> > + * MDM6600 status codes. These are copied from Motorola Mapphone Linux
> > + * kernel tree. The BB naming here refers to "BaseBand" for modem.
> > + */
> 
> Actually your status codes are BP_ (baseband processor) prefixed.

I'll just get rid of the naming and stick to MDM6600 prefixies.
No need to keep the confusing BP/AP prefixes.

> > +static void phy_mdm6600_init_irq(struct phy_mdm6600 *ddata)
> > +{
> > +   struct device *dev = ddata->dev;
> > +   int i, error, irq;
> > +
> > +   for (i = PHY_MDM6600_STATUS0;
> > +i <= PHY_MDM6600_STATUS2; i++) {
> > +   if (IS_ERR(ddata->gpio[i]))
> > +   continue;
> 
> This can be dropped, since the driver errors out of probe
> when there are gpio errors.

True will drop.

> > +static int phy_mdm6600_init_lines(struct phy_mdm6600 *ddata)
> > +{
> > +   struct device *dev = ddata->dev;
> > +   int i, j, nr_gpio = 0;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(phy_mdm6600_line_map); i++) {
> > +   const struct phy_mdm6600_map *map =
> > +   _mdm6600_line_map[i];
> > +
> > +   for (j = 0; j < map->nr_gpios; j++) {
> > +   struct gpio_desc **gpio = >gpio[nr_gpio];
> > +
> > +   *gpio = devm_gpiod_get_index(dev,
> > +map->name, j,
> > +map->direction);
> > +   if (IS_ERR(*gpio)) {
> > +   dev_info(dev,
> > +"gpio %s error %li, already taken?\n",
> > +map->name, PTR_ERR(*gpio));
> > +   return PTR_ERR(*gpio);
> > +   }
> > +   nr_gpio++;
> > +   }
> 
> I think the code should use the gpiod_get_array() API.

OK thanks will take a look.

> > +   /* Give up shared GPIOs now, they will be used for OOB wake */
> > +   devm_gpiod_put(ddata->dev, mode_gpio0);
> > +   ddata->gpio[PHY_MDM6600_MODE0] = ERR_PTR(-ENODEV);
> > +   devm_gpiod_put(ddata->dev, mode_gpio1);
> > +   ddata->gpio[PHY_MDM6600_MODE0] = ERR_PTR(-ENODEV);
> 
> You want PHY_MDM6600_MODE1 instead. Also I would just use NULL.
> NULL is used by gpiod_get_optional and is handled by the gpiod
> functions, so you don't need to check for gpio errors everywhere.

Oops yup let's just drop this until we know what to do in the
further patches.

> > +#ifdef CONFIG_OF
> > +static const struct of_device_id phy_mdm6600_id_table[] = {
> > +   { .compatible = "motorola,mapphone-mdm6600", },
> > +   {},
> > +};
> > +MODULE_DEVICE_TABLE(of, phy_mdm6600_id_table);
> > +#endif
> 
> I suggest to just depend on CONFIG_OF in Kconfig and drop the ifdef
> and of_match_ptr() parts. It's very unlikely, that this will be
> used without DT and would need quite some rework anyways.

OK

> > +static int phy_mdm6600_probe(struct platform_device *pdev)
> > +{
> > +   struct phy_mdm6600 *ddata;
> > +   struct usb_otg *otg;
> > +   const struct of_device_id *of_id;
> > +   int error;
> > +
> > +   of_id = of_match_device(of_match_ptr(phy_mdm6600_id_table),
> > +   >dev);
> > +   if (!of_id)
> > +   return -EINVAL;
> 
> I suggest to drop the of_match_device(). The driver will error
> out anyways when it can't get the gpios.

OK

> Generally I'm a bit worried about handling the mode gpios in two
> different drivers. It looks like it might become a dependency hell.

Yeah you're right. That will require the modules to be loaded
in the right order. It's probably best that we handle the mdm6600
to SoC wake-up in this driver. And then maybe export a function for
toggling the SoC to mdm660 wake to make sure the dependencies are
clear for the modules.

Regards,

Tony

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-17 Thread Sebastian Reichel 
Hi Tony,

On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> Let's add support for the GPIO controlled USB PHY on the MDM6600 modem.
> It is used on some Motorola Mapphone series of phones and tablets such
> as Droid 4.
> 
> The MDM6600 is hardwired to the first OHCI port in the Droid 4 case, and
> is controlled by several GPIOs. The USB PHY is integrated into the MDM6600
> device it seems. We know this as we get L3 errors from omap-usb-host if
> trying to use the PHY before MDM6600 is configured.
> 
> The GPIOs controlling MDM6600 are used to power MDM660 on and off, to
> configure the USB start-up mode (normal mode versus USB flashing), and
> they also tell the state of the MDM6600 device.
> 
> The two start-up mode GPIOs are dual-purposed and used for out of band
> (OOB) wake-up for USB and TS 27.010 serial mux. But we need to configure
> the USB start-up mode first to get MDM6600 booted in the right mode to
> be usable in the first place.
> 
> For now, this driver just gives up the two start-up mode GPIOs after the
> modem has been configured to boot in normal mode. One of them we may
> want to configure for USB OOB wake in this driver later on, but that's a
> separate series of patches and needs more work.
> 
> Note that the Motorola Mapphone Linux kernel tree has a "radio-ctrl"
> driver for modems. But it really does not control the radio at all, it
> just controls the modem power and start-up mode for USB. So I came to
> the conclusion that we're better off having this done in the USB PHY
> driver. For adding support for USB flashing mode, we can later on add
> a kernel module option for flash_mode=1 or something similar.
> 
> Also note that currently there is no PM runtime support for the OHCI
> on omap variant SoCs. So for low(er) power idle states, currenty both
> ohci-platform and phy-mapphone-mdm6600 must be unloaded or unbound.
> 
> For reference here is what I measured for total power consumption on
> an idle Droid 4 with and without USB related MDM6600 modules:
> 
> idle lcd off  phy-mapphone-mdm6600ohci-platform
> 153mW 284mW   344mW

So more than twice the idle power... We really want to get runtime
PM working :/

> So it seems that MDM6600 is currently not yet idling even with it's
> radio turned off, but that's something that is beyond the control of
> this USB PHY driver.
> 
> Cc: devicet...@vger.kernel.org
> Cc: Mark Rutland 
> Cc: Marcel Partap 
> Cc: Michael Scott 
> Cc: Rob Herring 
> Cc: Sebastian Reichel 
> Signed-off-by: Tony Lindgren 
> ---
>  .../bindings/phy/phy-mapphone-mdm6600.txt  |  30 ++
>  drivers/phy/motorola/Kconfig   |   9 +
>  drivers/phy/motorola/Makefile  |   1 +
>  drivers/phy/motorola/phy-mapphone-mdm6600.c| 490 
> +
>  4 files changed, 530 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
>  create mode 100644 drivers/phy/motorola/phy-mapphone-mdm6600.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt 
> b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> @@ -0,0 +1,30 @@
> +Device tree binding documentation for Motorola Mapphone MDM6600 USB PHY
> +
> +Required properties:
> +- compatible Must be "motorola,mapphone-mdm6600"
> +- enable-gpios   GPIO to enable the USB PHY
> +- power-gpiosGPIO to power on the device
> +- reset-gpiosGPIO to reset the device
> +- mode-gpios Two GPIOs to configure MDM6600 USB start-up mode for
> + normal mode versus USB flashing mode
> +- status-gpios   Three GPIOs to read the power state of the MDM6600
> +- cmd-gpios  Three GPIOs to control the power state of the MDM6600
> +
> +Example:
> +
> +fsusb1_phy: fsusb1_phy {
> + compatible = "motorola,mapphone-mdm6600";
> + enable-gpios = < 31 GPIO_ACTIVE_LOW>; /* gpio_95 */
> + power-gpios = < 22 GPIO_ACTIVE_HIGH>; /* gpio_54 */
> + reset-gpios = < 17 GPIO_ACTIVE_HIGH>; /* gpio_49 */
> + mode-gpios = < 20 GPIO_ACTIVE_HIGH>,  /* gpio_148 */
> +  < 21 GPIO_ACTIVE_HIGH>;  /* gpio_149 */
> + status-gpios = < 23 GPIO_ACTIVE_HIGH>,/* gpio_55 */
> +< 21 GPIO_ACTIVE_HIGH>,/* gpio_53 */
> +< 20 GPIO_ACTIVE_HIGH>;/* gpio_52 */
> + cmd-gpios = < 7 GPIO_ACTIVE_HIGH>,/* gpio_103 */
> + < 8 GPIO_ACTIVE_HIGH>,/* gpio_104 */
> + < 14 GPIO_ACTIVE_HIGH>;   /* gpio_142 */
> + #phy-cells = <0>;
> +};
> +
> diff --git a/drivers/phy/motorola/Kconfig b/drivers/phy/motorola/Kconfig
> --- a/drivers/phy/motorola/Kconfig
> +++ b/drivers/phy/motorola/Kconfig
> @@ -10,3

Re: [PATCH] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

2018-02-17 Thread Sebastian Reichel 
Hi Tony,

On Sat, Feb 17, 2018 at 01:07:23PM -0800, Tony Lindgren wrote:
> Let's add support for the GPIO controlled USB PHY on the MDM6600 modem.
> It is used on some Motorola Mapphone series of phones and tablets such
> as Droid 4.
> 
> The MDM6600 is hardwired to the first OHCI port in the Droid 4 case, and
> is controlled by several GPIOs. The USB PHY is integrated into the MDM6600
> device it seems. We know this as we get L3 errors from omap-usb-host if
> trying to use the PHY before MDM6600 is configured.
> 
> The GPIOs controlling MDM6600 are used to power MDM660 on and off, to
> configure the USB start-up mode (normal mode versus USB flashing), and
> they also tell the state of the MDM6600 device.
> 
> The two start-up mode GPIOs are dual-purposed and used for out of band
> (OOB) wake-up for USB and TS 27.010 serial mux. But we need to configure
> the USB start-up mode first to get MDM6600 booted in the right mode to
> be usable in the first place.
> 
> For now, this driver just gives up the two start-up mode GPIOs after the
> modem has been configured to boot in normal mode. One of them we may
> want to configure for USB OOB wake in this driver later on, but that's a
> separate series of patches and needs more work.
> 
> Note that the Motorola Mapphone Linux kernel tree has a "radio-ctrl"
> driver for modems. But it really does not control the radio at all, it
> just controls the modem power and start-up mode for USB. So I came to
> the conclusion that we're better off having this done in the USB PHY
> driver. For adding support for USB flashing mode, we can later on add
> a kernel module option for flash_mode=1 or something similar.
> 
> Also note that currently there is no PM runtime support for the OHCI
> on omap variant SoCs. So for low(er) power idle states, currenty both
> ohci-platform and phy-mapphone-mdm6600 must be unloaded or unbound.
> 
> For reference here is what I measured for total power consumption on
> an idle Droid 4 with and without USB related MDM6600 modules:
> 
> idle lcd off  phy-mapphone-mdm6600ohci-platform
> 153mW 284mW   344mW

So more than twice the idle power... We really want to get runtime
PM working :/

> So it seems that MDM6600 is currently not yet idling even with it's
> radio turned off, but that's something that is beyond the control of
> this USB PHY driver.
> 
> Cc: devicet...@vger.kernel.org
> Cc: Mark Rutland 
> Cc: Marcel Partap 
> Cc: Michael Scott 
> Cc: Rob Herring 
> Cc: Sebastian Reichel 
> Signed-off-by: Tony Lindgren 
> ---
>  .../bindings/phy/phy-mapphone-mdm6600.txt  |  30 ++
>  drivers/phy/motorola/Kconfig   |   9 +
>  drivers/phy/motorola/Makefile  |   1 +
>  drivers/phy/motorola/phy-mapphone-mdm6600.c| 490 
> +
>  4 files changed, 530 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
>  create mode 100644 drivers/phy/motorola/phy-mapphone-mdm6600.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt 
> b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-mapphone-mdm6600.txt
> @@ -0,0 +1,30 @@
> +Device tree binding documentation for Motorola Mapphone MDM6600 USB PHY
> +
> +Required properties:
> +- compatible Must be "motorola,mapphone-mdm6600"
> +- enable-gpios   GPIO to enable the USB PHY
> +- power-gpiosGPIO to power on the device
> +- reset-gpiosGPIO to reset the device
> +- mode-gpios Two GPIOs to configure MDM6600 USB start-up mode for
> + normal mode versus USB flashing mode
> +- status-gpios   Three GPIOs to read the power state of the MDM6600
> +- cmd-gpios  Three GPIOs to control the power state of the MDM6600
> +
> +Example:
> +
> +fsusb1_phy: fsusb1_phy {
> + compatible = "motorola,mapphone-mdm6600";
> + enable-gpios = < 31 GPIO_ACTIVE_LOW>; /* gpio_95 */
> + power-gpios = < 22 GPIO_ACTIVE_HIGH>; /* gpio_54 */
> + reset-gpios = < 17 GPIO_ACTIVE_HIGH>; /* gpio_49 */
> + mode-gpios = < 20 GPIO_ACTIVE_HIGH>,  /* gpio_148 */
> +  < 21 GPIO_ACTIVE_HIGH>;  /* gpio_149 */
> + status-gpios = < 23 GPIO_ACTIVE_HIGH>,/* gpio_55 */
> +< 21 GPIO_ACTIVE_HIGH>,/* gpio_53 */
> +< 20 GPIO_ACTIVE_HIGH>;/* gpio_52 */
> + cmd-gpios = < 7 GPIO_ACTIVE_HIGH>,/* gpio_103 */
> + < 8 GPIO_ACTIVE_HIGH>,/* gpio_104 */
> + < 14 GPIO_ACTIVE_HIGH>;   /* gpio_142 */
> + #phy-cells = <0>;
> +};
> +
> diff --git a/drivers/phy/motorola/Kconfig b/drivers/phy/motorola/Kconfig
> --- a/drivers/phy/motorola/Kconfig
> +++ b/drivers/phy/motorola/Kconfig
> @@ -10,3 +10,12 @@ config PHY_CPCAP_USB
>   help
> Enable this for USB to work on Motorola phones and tablets
>