musb: Add support for the Allwinner sunxi musb controller

drivers/phy/phy-sun4i-usb.c +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Quoted 9 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

drivers/phy/phy-sun4i-usb.c +9 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 94

Quoted 5 lines

drivers/phy/phy-sun4i-usb.c +4 -0

+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

and why can't there be a high-level
PHY API for this?

Hi,

Quoted 10 lines · Arnd Bergmann

Quoted 9 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

drivers/phy/phy-sun4i-usb.c +9 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 94

Quoted 5 lines

drivers/phy/phy-sun4i-usb.c +4 -0

+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

But the musb-core still needs to know the status of the id and vbus pins, and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val)

{
         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

         if (val)
                 val = SUNXI_ISCR_FORCE_ID_HIGH;
         else
                 val = SUNXI_ISCR_FORCE_ID_LOW;

         sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK, val);
}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val)

{
         struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

         if (val)
                 val = SUNXI_ISCR_FORCE_VBUS_HIGH;
         else
                 val = SUNXI_ISCR_FORCE_VBUS_LOW;

         sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK, val);
}

I will happily admit that these 2 functions are a better API between the sunxi musb glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr approach as I wanted to keep the API as small as possible, but having 2 functions like the one above, which actually reflect what is happening would indeed be better.

Note that the polling of the pins cannot (easily) be moved into the phy driver for various reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that in the long run people want to get rid of struct usb_phy, so maybe we should consider adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Functions in the generic phy API is a good idea, or if this is too sunxi specific, I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and 1 doing the sunxi implementation.

If people believe this is too sunxi specific (I believe it is, but as said I'm fine with doing this either way). I'll respin this patch to remove the too generic sun4i_usb_phy_update_iscr function, and instead add these 2:

void sun4i_usb_phy_set_id_detect(struct phy *phy, bool val); void sun4i_usb_phy_set_vbus_detect(struct phy *phy, bool val);

Quoted 67 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 10 lines · Arnd Bergmann

Quoted 9 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

drivers/phy/phy-sun4i-usb.c +9 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 94

Quoted 5 lines

drivers/phy/phy-sun4i-usb.c +4 -0

+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

But the musb-core still needs to know the status of the id and vbus pins, and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val)

{
          struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK, val);
}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val)

{
          struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK, val);
}

Quoted 4 lines

I will happily admit that these 2 functions are a better API between the sunxi musb glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr approach as I wanted to keep the API as small as possible, but having 2 functions like the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 32 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that in the long run people want to get rid of struct usb_phy, so maybe we should consider adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Functions in the generic phy API is a good idea, or if this is too sunxi specific, I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and 1 doing the sunxi implementation.

If people believe this is too sunxi specific (I believe it is, but as said I'm fine with doing this either way). I'll respin this patch to remove the too generic sun4i_usb_phy_update_iscr function, and instead add these 2:

Quoted 2 lines

void sun4i_usb_phy_set_id_detect(struct phy *phy, bool val); void sun4i_usb_phy_set_vbus_detect(struct phy *phy, bool val);

Thanks for your detailed explanations. I think this is something for Kishon to decide, based on where he wants to take the phy API in the long run. I'm fine with it either way, and it seems easy enough to change between those two interfaces if we make up our minds about it later.

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

drivers/phy/phy-sun4i-usb.c +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 94

Quoted 10 lines

drivers/phy/phy-sun4i-usb.c +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

Quoted 6 lines

Quoted 6 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

Quoted 6 lines

Quoted 6 lines

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

Quoted 12 lines

Quoted 12 lines

and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val)

{
          struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

Quoted 6 lines

Quoted 6 lines

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK, val);

Quoted 52 lines

Quoted 15 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val)

{
          struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK,

val);
}

Quoted 7 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 27 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Quoted 6 lines

Quoted 6 lines

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Hi,

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

drivers/phy/phy-sun4i-usb.c +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 94

Quoted 10 lines

drivers/phy/phy-sun4i-usb.c +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

right. That would bind the PHY driver and the controller driver and would start creating problems when a different PHY is connected with the controller.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

The polling can still be done in PHY driver and can use the extcon framework to report the status to controller driver no?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

musb-core or the musb-glue (musb/sunxi.c in this case)?

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val)

{
          struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

Quoted 1 line

Quoted 1 line

Quoted 1 line

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK, val);

What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq interrupt handler?

Quoted 52 lines

Quoted 52 lines

Quoted 15 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val)

{
          struct sunxi_glue *glue = dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK,

val);
}

Quoted 7 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 27 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Please don't do that. We don't want to be bloating phy framework with platform specific hooks.

Hi,

Quoted 42 lines · Kishon Vijay Abraham I

Hi,

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

(quoted diff) +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Review

Quoted 10 lines

(quoted diff) +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

right. That would bind the PHY driver and the controller driver and would start creating problems when a different PHY is connected with the controller.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

The polling can still be done in PHY driver and can use the extcon framework to report the status to controller driver no?

Technically the polling can be moved to the phy driver yes, but it is not easy, e.g. we only need to poll when we're in otg mode rather then host-only or peripheral-only mode, and the mode gets set by the musb driver not phy the phy driver. The sunxi musb implementation uses an integrated phy, so it is just much easier and more logical to have all control / polling happening from a single module rather then from 2 different modules.

Quoted 22 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

musb-core or the musb-glue (musb/sunxi.c in this case)?

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK,

val);

What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq interrupt handler?

No this changes the vbus and id status as seen by the musb core, and the musb responds to this, by e.g. starting a session, or when vbus does not get high after a session request by reporting a vbus-error interrupt.

The sunxi_musb_id_vbus_det_irq handler gets triggered by edges on the gpio pins which are used to monitor the id and vbus status.

Quoted 61 lines

Quoted 60 lines

Quoted 60 lines

Quoted 16 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK,

val);
}

Quoted 8 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 33 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Quoted 2 lines

Please don't do that. We don't want to be bloating phy framework with platform specific hooks.

Right, that was my feeling too. I believe that using sunxi specific phy functions here is best given that we're dealing with an otg controller + phy both if which

Hi,

Quoted 155 lines · Hans de Goede

Hi,

Quoted 42 lines · Kishon Vijay Abraham I

Hi,

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

(quoted diff) +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Review

Quoted 10 lines

(quoted diff) +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

right. That would bind the PHY driver and the controller driver and would start creating problems when a different PHY is connected with the controller.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

The polling can still be done in PHY driver and can use the extcon framework to report the status to controller driver no?

Technically the polling can be moved to the phy driver yes, but it is not easy, e.g. we only need to poll when we're in otg mode rather then host-only or peripheral-only mode, and the mode gets set by the musb driver not phy the phy driver. The sunxi musb implementation uses an integrated phy, so it is just much easier and more logical to have all control / polling happening from a single module rather then from 2 different modules.

Quoted 22 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

musb-core or the musb-glue (musb/sunxi.c in this case)?

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK,

val);

What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq interrupt handler?

No this changes the vbus and id status as seen by the musb core, and the musb responds to this, by e.g. starting a session, or when vbus does not get high after a session request by reporting a vbus-error interrupt.

The sunxi_musb_id_vbus_det_irq handler gets triggered by edges on the gpio pins which are used to monitor the id and vbus status.

Quoted 61 lines

Quoted 60 lines

Quoted 60 lines

Quoted 16 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK,

val);
}

Quoted 8 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 33 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Quoted 5 lines

Quoted 2 lines

Please don't do that. We don't want to be bloating phy framework with platform specific hooks.

Right, that was my feeling too. I believe that using sunxi specific phy functions here is best given that we're dealing with an otg controller + phy both if which

Using platform specific PHY functions dissolves the very purpose of creating the PHY framework. We have to find a better way.

Hi,

Quoted 169 lines · Kishon Vijay Abraham I

Hi,

Quoted 165 lines · Hans de Goede

Hi,

Quoted 42 lines · Kishon Vijay Abraham I

Hi,

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

(quoted diff) +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Review

Quoted 10 lines

(quoted diff) +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

right. That would bind the PHY driver and the controller driver and would start creating problems when a different PHY is connected with the controller.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

The polling can still be done in PHY driver and can use the extcon framework to report the status to controller driver no?

Technically the polling can be moved to the phy driver yes, but it is not easy, e.g. we only need to poll when we're in otg mode rather then host-only or peripheral-only mode, and the mode gets set by the musb driver not phy the phy driver. The sunxi musb implementation uses an integrated phy, so it is just much easier and more logical to have all control / polling happening from a single module rather then from 2 different modules.

Quoted 22 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

musb-core or the musb-glue (musb/sunxi.c in this case)?

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK,

val);

What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq interrupt handler?

No this changes the vbus and id status as seen by the musb core, and the musb responds to this, by e.g. starting a session, or when vbus does not get high after a session request by reporting a vbus-error interrupt.

The sunxi_musb_id_vbus_det_irq handler gets triggered by edges on the gpio pins which are used to monitor the id and vbus status.

Quoted 64 lines

Quoted 57 lines

Quoted 57 lines

Quoted 16 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK,

val);
}

Quoted 8 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 30 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on
your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should
consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Quoted 2 lines

Please don't do that. We don't want to be bloating phy framework with platform specific hooks.

Right, that was my feeling too. I believe that using sunxi specific phy functions
here is best given that we're dealing with an otg controller + phy both if which

Quoted 2 lines

Using platform specific PHY functions dissolves the very purpose of creating the PHY framework. We have to find a better way.

"We have to find a better way" is not really a helpful answer to the problem at hand. We're talking about a phy integrated on the same die, as, and closely working

together with the otg controller, so we need some sort of coordination between the two.

The 2 logical options for coordinatinon are:

1) Add generic phy framework functions for such coordination -> Which you've nacked

2) Use platform private functions for this
-> Which you are essentially nacking above.

Which leaves us with what as solution exactly ?

I could make the phy-sun4i-usb.c driver register a struct usb_phy, as drivers/phy/phy-omap-usb2.c and drivers/phy/phy-twl4030-usb.c does,

Hi,

Quoted 172 lines · Hans de Goede

Hi,

Quoted 169 lines · Kishon Vijay Abraham I

Hi,

Quoted 165 lines · Hans de Goede

Hi,

Quoted 42 lines · Kishon Vijay Abraham I

Hi,

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

(quoted diff) +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Review

Quoted 10 lines

(quoted diff) +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

right. That would bind the PHY driver and the controller driver and would start creating problems when a different PHY is connected with the controller.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided to not use the dedicated id-detect and vusb-sense pins of the phy they are using (these pins are not routed to the outside).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

The polling can still be done in PHY driver and can use the extcon framework to report the status to controller driver no?

Technically the polling can be moved to the phy driver yes, but it is not easy, e.g. we only need to poll when we're in otg mode rather then host-only or peripheral-only mode, and the mode gets set by the musb driver not phy the phy driver. The sunxi musb implementation uses an integrated phy, so it is just much easier and more logical to have all control / polling happening from a single module rather then from 2 different modules.

Quoted 22 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

musb-core or the musb-glue (musb/sunxi.c in this case)?

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

and gets this from the usb0-phy iscr register, which reflects the status of the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK,

val);

What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq interrupt handler?

No this changes the vbus and id status as seen by the musb core, and the musb responds to this, by e.g. starting a session, or when vbus does not get high after a session request by reporting a vbus-error interrupt.

The sunxi_musb_id_vbus_det_irq handler gets triggered by edges on the gpio pins which are used to monitor the id and vbus status.

Quoted 64 lines

Quoted 57 lines

Quoted 57 lines

Quoted 16 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_VBUS_MASK,

val);
}

Quoted 8 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 30 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on
your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should
consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Quoted 2 lines

Please don't do that. We don't want to be bloating phy framework with platform specific hooks.

Right, that was my feeling too. I believe that using sunxi specific phy functions
here is best given that we're dealing with an otg controller + phy both if which

Quoted 6 lines

Quoted 2 lines

Using platform specific PHY functions dissolves the very purpose of creating the PHY framework. We have to find a better way.

"We have to find a better way" is not really a helpful answer to the problem at hand. We're talking about a phy integrated on the same die, as, and closely working

It's integrated in the same die but it is not within the controller IP. It's still possible to replace the PHY and have the same controller in the next SoC.

Quoted 8 lines

together with the otg controller, so we need some sort of coordination between the two.

The 2 logical options for coordinatinon are:

1) Add generic phy framework functions for such coordination -> Which you've nacked

Quoted 4 lines

2) Use platform private functions for this
-> Which you are essentially nacking above.

Which leaves us with what as solution exactly ?

We could poll on the GPIO in PHY driver to update the iscr register. I think it's alright to poll on the GPIO in multiple modules for serving it's own purpose IMHO.

Quoted 2 lines

I could make the phy-sun4i-usb.c driver register a struct usb_phy, as drivers/phy/phy-omap-usb2.c and drivers/phy/phy-twl4030-usb.c does,

I'm not sure how will that help.

Hi,

Quoted 201 lines · Kishon Vijay Abraham I

Hi,

Quoted 180 lines · Hans de Goede

Hi,

Quoted 177 lines · Kishon Vijay Abraham I

Hi,

Quoted 173 lines · Hans de Goede

Hi,

Quoted 44 lines · Kishon Vijay Abraham I

Hi,

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

(quoted diff) +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Review

Quoted 10 lines

(quoted diff) +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

right. That would bind the PHY driver and the controller driver and would start creating problems when a different PHY is connected with the controller.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided
to not use the dedicated id-detect and vusb-sense pins of the phy they are
using (these pins are not routed to the outside).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

The polling can still be done in PHY driver and can use the extcon framework to report the status to controller driver no?

Technically the polling can be moved to the phy driver yes, but it is not easy,
e.g. we only need to poll when we're in otg mode rather then host-only or peripheral-only mode, and the mode gets set by the musb driver not phy the phy driver. The sunxi musb implementation uses an integrated phy, so it is just much easier and more logical to have all control / polling happening from a single module rather then from 2 different modules.

Quoted 23 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

musb-core or the musb-glue (musb/sunxi.c in this case)?

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

and gets this from the usb0-phy iscr register, which reflects the status of
the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK,

val);

What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq
interrupt handler?

No this changes the vbus and id status as seen by the musb core, and the musb responds to this, by e.g. starting a session, or when vbus does not get high after a session request by reporting a vbus-error interrupt.

The sunxi_musb_id_vbus_det_irq handler gets triggered by edges on the gpio pins which are used to monitor the id and vbus status.

Quoted 66 lines

Quoted 58 lines

Quoted 58 lines

Quoted 17 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy,

SUNXI_ISCR_FORCE_VBUS_MASK,
val);
}

Quoted 8 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 30 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on
your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should
consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route
I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Quoted 3 lines

Please don't do that. We don't want to be bloating phy framework with platform
specific hooks.

Right, that was my feeling too. I believe that using sunxi specific phy functions
here is best given that we're dealing with an otg controller + phy both if which

Quoted 6 lines

Quoted 2 lines

Using platform specific PHY functions dissolves the very purpose of creating the PHY framework. We have to find a better way.

"We have to find a better way" is not really a helpful answer to the problem at hand. We're talking about a phy integrated on the same die, as, and closely working

It's integrated in the same die but it is not within the controller IP. It's still possible to replace the PHY and have the same controller in the next SoC.

Quoted 9 lines

together with the otg controller, so we need some sort of coordination between the
two.

The 2 logical options for coordinatinon are:

1) Add generic phy framework functions for such coordination -> Which you've nacked

Quoted 8 lines

Quoted 4 lines

2) Use platform private functions for this
-> Which you are essentially nacking above.

Which leaves us with what as solution exactly ?

We could poll on the GPIO in PHY driver to update the iscr register. I think it's alright to poll on the GPIO in multiple modules for serving it's own purpose IMHO.

No, polling in multiple places is just plain and simple wrong, also where-ever possible we use edge triggered gpio-interrupts for this and having multiple handlers for the same interrupt is even more wrong.

I'm fine with moving all the polling to the phy driver, but then we need to have a way to notify the sunxi musb glue about id pin changes.

Quoted 5 lines

Quoted 3 lines

I could make the phy-sun4i-usb.c driver register a struct usb_phy, as drivers/phy/phy-omap-usb2.c and drivers/phy/phy-twl4030-usb.c does,

I'm not sure how will that help.

It helps when we move the polling to the phy driver as it has a notification mechanism for events which we can then use to tell the musb-sunxi glue about id pin changes.

Hi,

Quoted 213 lines · Hans de Goede

Hi,

Quoted 201 lines · Kishon Vijay Abraham I

Hi,

Quoted 180 lines · Hans de Goede

Hi,

Quoted 177 lines · Kishon Vijay Abraham I

Hi,

Quoted 173 lines · Hans de Goede

Hi,

Quoted 44 lines · Kishon Vijay Abraham I

Hi,

Quoted 25 lines · Hans de Goede

Hi,

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · On Tuesday 10 March 2015 09:04:43 Hans de Goede wrote:

Hi,

Quoted 18 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:15 Hans de Goede wrote:

(quoted diff) +3 -0

+void sun4i_usb_phy_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);

Review

Quoted 10 lines

(quoted diff) +10 -0

+       struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+       u32 iscr;
+
+       iscr = readl(data->base + REG_ISCR);
+       iscr &= ~clr;
+       iscr |= set;
+       writel(iscr, data->base + REG_ISCR);
+}
+EXPORT_SYMBOL(sun4i_usb_phy_update_iscr);
+

I would generally consider this a bad design. What is the purpose of calling sun4i_usb_phy_update_iscr()

right. That would bind the PHY driver and the controller driver and would start creating problems when a different PHY is connected with the controller.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

There are 2 different use cases for this one is to enable the dataline pull-ups at driver init and disable them at driver exit, this could / should probably be moved to the phy_init / phy_exit code for the usb0 phy removing the need to do this from within the sunxi musb glue.

The second use-case is more tricky, for some reasons Allwinner has decided
to not use the dedicated id-detect and vusb-sense pins of the phy they are
using (these pins are not routed to the outside).

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Instead id-detect and vusb-sense are done through any $random gpio pins (including non irq capable pins on some designs requiring polling).

The polling can still be done in PHY driver and can use the extcon framework to report the status to controller driver no?

Technically the polling can be moved to the phy driver yes, but it is not easy,
e.g. we only need to poll when we're in otg mode rather then host-only or peripheral-only mode, and the mode gets set by the musb driver not phy the phy driver. The sunxi musb implementation uses an integrated phy, so it is just much easier and more logical to have all control / polling happening from a single module rather then from 2 different modules.

Quoted 23 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

But the musb-core still needs to know the status of the id and vbus pins,

musb-core or the musb-glue (musb/sunxi.c in this case)?

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

and gets this from the usb0-phy iscr register, which reflects the status of
the not connected dedicated pins of the phy. The reason this can still work at all is because the iscr register allows the user to override whatever the not connected phy pins are seeing and forcing a value to report to the musb core as id and vbus status.

This is done by these 2 functions in the musb sunxi glue:

static void sunxi_musb_force_id(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_ID_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_ID_LOW;

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

          sun4i_usb_phy_update_iscr(glue->phy, SUNXI_ISCR_FORCE_ID_MASK,

val);

What will writing to this register lead to? call to sunxi_musb_id_vbus_det_irq
interrupt handler?

No this changes the vbus and id status as seen by the musb core, and the musb responds to this, by e.g. starting a session, or when vbus does not get high after a session request by reporting a vbus-error interrupt.

The sunxi_musb_id_vbus_det_irq handler gets triggered by edges on the gpio pins which are used to monitor the id and vbus status.

Quoted 66 lines

Quoted 58 lines

Quoted 58 lines

Quoted 17 lines

}

static void sunxi_musb_force_vbus(struct musb *musb, u32 val) {
struct sunxi_glue *glue =
dev_get_drvdata(musb->controller->parent);

          if (val)
                  val = SUNXI_ISCR_FORCE_VBUS_HIGH;
          else
                  val = SUNXI_ISCR_FORCE_VBUS_LOW;

          sun4i_usb_phy_update_iscr(glue->phy,

SUNXI_ISCR_FORCE_VBUS_MASK,
val);
}

Quoted 8 lines

I will happily admit that these 2 functions are a better API between the sunxi musb
glue and the sunxi usb phy driver. I started with the minimal sun4i_usb_phy_update_iscr
approach as I wanted to keep the API as small as possible, but having 2 functions like
the one above, which actually reflect what is happening would indeed be better.

Ok, that would definitely improve things.

Quoted 30 lines

Note that the polling of the pins cannot (easily) be moved into the phy driver for various
reasons:

1) It depends on dr_mode, the otg may be used in host only mode in which case there are no
pins at all.
2) the musb set_vbus callback needs access to the pins
3) When id changes some musb core state changes are necessary.

I'll respin the patch set to do things this way as soon as we've agreement on
your second point.

Quoted 2 lines

and why can't there be a high-level
PHY API for this?

The current generic phy API seems to not have any bus specific methods, I know that
in the long run people want to get rid of struct usb_phy, so maybe we should
consider
adding bus specific methods to the generic phy API for things like otg.

If we decide to add bus specific methods, then the question becomes if having

int phy_usb_set_id_detect(struct phy *phy, bool val); int phy_usb_set_vbus_detect(struct phy *phy, bool val);

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Functions in the generic phy API is a good idea, or if this is too sunxi specific,
I'm fine with doing this either way. If we want to go the generic phy route
I'll split this in 2 patches, one adding these 2 generic functions & phy-ops, and
1 doing the sunxi implementation.

Quoted 3 lines

Please don't do that. We don't want to be bloating phy framework with platform
specific hooks.

Right, that was my feeling too. I believe that using sunxi specific phy functions
here is best given that we're dealing with an otg controller + phy both if which

Quoted 6 lines

Quoted 2 lines

Using platform specific PHY functions dissolves the very purpose of creating the PHY framework. We have to find a better way.

"We have to find a better way" is not really a helpful answer to the problem at hand. We're talking about a phy integrated on the same die, as, and closely working

It's integrated in the same die but it is not within the controller IP. It's still possible to replace the PHY and have the same controller in the next SoC.

Quoted 9 lines

together with the otg controller, so we need some sort of coordination between the
two.

The 2 logical options for coordinatinon are:

1) Add generic phy framework functions for such coordination -> Which you've nacked

Quoted 8 lines

Quoted 4 lines

2) Use platform private functions for this
-> Which you are essentially nacking above.

Which leaves us with what as solution exactly ?

We could poll on the GPIO in PHY driver to update the iscr register. I think it's alright to poll on the GPIO in multiple modules for serving it's own purpose IMHO.

Quoted 6 lines

No, polling in multiple places is just plain and simple wrong, also where-ever possible we use edge triggered gpio-interrupts for this and having multiple handlers for the same interrupt is even more wrong.

I'm fine with moving all the polling to the phy driver, but then we need to have a way to notify the sunxi musb glue about id pin changes.

As I mentioned before, we can use extcon framework for it.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

I could make the phy-sun4i-usb.c driver register a struct usb_phy, as drivers/phy/phy-omap-usb2.c and drivers/phy/phy-twl4030-usb.c does,

Quoted 5 lines

Quoted 1 line

I'm not sure how will that help.

It helps when we move the polling to the phy driver as it has a notification mechanism for events which we can then use to tell the musb-sunxi glue about id pin changes.

extcon framework might help here.

The generic fifo functions already use non wrapped accesses in various cases through the iowrite#_rep functions, and all platforms which override the default musb_read[b|w] / _write[b|w] functions also provide their own fifo access functions, so we can safely drop the unnecessary indirection from the fifo access functions.

Signed-off-by Hans de Goede <[email protected]>

Quoted 6 lines · On Monday 09 March 2015 21:40:18 Hans de Goede wrote:

The generic fifo functions already use non wrapped accesses in various cases through the iowrite#_rep functions, and all platforms which override the default musb_read[b|w] / _write[b|w] functions also provide their own fifo access functions, so we can safely drop the unnecessary indirection from the fifo access functions.

Quoted 2 lines

Signed-off-by Hans de Goede <[email protected]>

The patch looks reasonably, but the description seem misleading. I believe the real reason why it's ok to use __raw_writew for the FIFO is that a FIFO by definition is using CPU endian access for copying byte streams from memory, which is unlike any other MMIO register that requires fixed-endian accessors.

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · On Monday 09 March 2015 21:40:18 Hans de Goede wrote:

The generic fifo functions already use non wrapped accesses in various cases through the iowrite#_rep functions, and all platforms which override the default musb_read[b|w] / _write[b|w] functions also provide their own fifo access functions, so we can safely drop the unnecessary indirection from the fifo access functions.

Quoted 8 lines

Quoted 2 lines

Signed-off-by Hans de Goede <[email protected]>

The patch looks reasonably, but the description seem misleading. I believe the real reason why it's ok to use __raw_writew for the FIFO is that a FIFO by definition is using CPU endian access for copying byte streams from memory, which is unlike any other MMIO register that requires fixed-endian accessors.

I'm not sure that that is the case here, this fifo allows reading 4 bytes at a time using 32 bit word access, so endianness may come into play. This patch is safe however since all existing users of the generic fifo_read / write helpers which this patch touches, are also using the generic musb_read[b|w] / _write[b|w] functions which are just __raw_foo wrappers, so there is no functional change, which is what the commit message tries to say.

Note that sunxi needs this function because of the register address translation the sunxi_musb_readb / writeb wrappers are doing which does not know how to deal with fifo data, and besides that it should make the generic read / write fifo helpers somewhat faster by removing an indirect function call.

Quoted 17 lines · On Tuesday 10 March 2015 08:43:22 Hans de Goede wrote:

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · On Monday 09 March 2015 21:40:18 Hans de Goede wrote:

The generic fifo functions already use non wrapped accesses in various cases through the iowrite#_rep functions, and all platforms which override the default musb_read[b|w] / _write[b|w] functions also provide their own fifo access functions, so we can safely drop the unnecessary indirection from the fifo access functions.

Quoted 8 lines

Quoted 2 lines

Signed-off-by Hans de Goede <[email protected]>

The patch looks reasonably, but the description seem misleading. I believe the real reason why it's ok to use __raw_writew for the FIFO is that a FIFO by definition is using CPU endian access for copying byte streams from memory, which is unlike any other MMIO register that requires fixed-endian accessors.

Quoted 7 lines

I'm not sure that that is the case here, this fifo allows reading 4 bytes at a time using 32 bit word access, so endianness may come into play. This patch is safe however since all existing users of the generic fifo_read / write helpers which this patch touches, are also using the generic musb_read[b|w] / _write[b|w] functions which are just __raw_foo wrappers, so there is no functional change, which is what the commit message tries to say.

This probably means that the generic musb helpers are not safe to use on big-endian ARM systems (but may work on MIPS). The only one currently not using the generic helpers is blackfin, which is fixed to little endian.

Quoted 5 lines

Note that sunxi needs this function because of the register address translation the sunxi_musb_readb / writeb wrappers are doing which does not know how to deal with fifo data, and besides that it should make the generic read / write fifo helpers somewhat faster by removing an indirect function call.

Your sunxi_musb_readw/writew functions however also are endian-safe and seem to get this part right, unlike all other platforms that use the generic __raw_*() accessors. With this patch applied, it should actually work fine, and it would work on other platforms as well if we change all __raw_*() calls outside of musb_default_write_fifo() and musb_default_read_fifo() to use *_relaxed() instead.

Quoted 37 lines · Arnd Bergmann

Quoted 17 lines · On Tuesday 10 March 2015 08:43:22 Hans de Goede wrote:

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · On Monday 09 March 2015 21:40:18 Hans de Goede wrote:

The generic fifo functions already use non wrapped accesses in various cases through the iowrite#_rep functions, and all platforms which override the default musb_read[b|w] / _write[b|w] functions also provide their own fifo access functions, so we can safely drop the unnecessary indirection from the fifo access functions.

Quoted 8 lines

Quoted 2 lines

Signed-off-by Hans de Goede <[email protected]>

The patch looks reasonably, but the description seem misleading. I believe the real reason why it's ok to use __raw_writew for the FIFO is that a FIFO by definition is using CPU endian access for copying byte streams from memory, which is unlike any other MMIO register that requires fixed-endian accessors.

Quoted 7 lines

I'm not sure that that is the case here, this fifo allows reading 4 bytes at a time using 32 bit word access, so endianness may come into play. This patch is safe however since all existing users of the generic fifo_read / write helpers which this patch touches, are also using the generic musb_read[b|w] / _write[b|w] functions which are just __raw_foo wrappers, so there is no functional change, which is what the commit message tries to say.

This probably means that the generic musb helpers are not safe to use on big-endian ARM systems (but may work on MIPS). The only one currently not using the generic helpers is blackfin, which is fixed to little endian.

Quoted 5 lines

Note that sunxi needs this function because of the register address translation the sunxi_musb_readb / writeb wrappers are doing which does not know how to deal with fifo data, and besides that it should make the generic read / write fifo helpers somewhat faster by removing an indirect function call.

Quoted 6 lines

Your sunxi_musb_readw/writew functions however also are endian-safe and seem to get this part right, unlike all other platforms that use the generic __raw_*() accessors. With this patch applied, it should actually work fine, and it would work on other platforms as well if we change all __raw_*() calls outside of musb_default_write_fifo() and musb_default_read_fifo() to use *_relaxed() instead.

I think that that change falls outside of the scope of this patchset.

I agree that it would be probably a good idea to get rid of the __raw_foo usage in musb, which is why I've used the non __raw versions in the sunxi glue, but as said I believe this falls outside of the scope of this patchset. All my preparation patches for adding sunxi support carefully do not make any functional changes, as I do not want to cause regressions on hardware which I cannot test.

Add a node for the otg/drc usb controller to sun4i-a10.dtsi.

Signed-off-by Hans de Goede <[email protected]>

---
 arch/arm/boot/dts/sun4i-a10.dtsi | 12 ++++++++++++
 drivers/usb/musb/Kconfig         |  1 +
 2 files changed, 13 insertions(+)

drivers/usb/musb/Kconfig +0 -0

index 8180a3a..fab8e9a 100644

Hi Hans,

Quoted 8 lines · Hans de Goede

Add a node for the otg/drc usb controller to sun4i-a10.dtsi.

Signed-off-by Hans de Goede <[email protected]>

---
  arch/arm/boot/dts/sun4i-a10.dtsi | 12 ++++++++++++
  drivers/usb/musb/Kconfig         |  1 +
  2 files changed, 13 insertions(+)

drivers/usb/musb/Kconfig comment "Platform Glue Layer" · line 68

Quoted 9 lines

drivers/usb/musb/Kconfig +1 -0

index 8180a3a..fab8e9a 100644
@@ -65,6 +65,7 @@ comment "Platform Glue Layer"
  config USB_MUSB_SUNXI
         tristate "Allwinner (sunxi)"
         depends on ARCH_SUNXI
+       depends on NOP_USB_XCEIV

Shouldn't this be in a different patch?

Enable the otg/drc usb controller on the Chuwi V7 CW0825 tablet.

Signed-off-by Hans de Goede <[email protected]>

---
 arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts | 31 +++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts +20 -0

index 97fca89..034396c 100644
@@ -111,6 +111,26 @@
 	status = "okay";
 };
 
+&pio {
+	usb0_id_detect_pin: usb0_id_detect_pin at 0 {
+		allwinner,pins = "PH4";
+		allwinner,function = "gpio_in";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
+	};
+
+	usb0_vbus_detect_pin: usb0_vbus_detect_pin at 0 {
+		allwinner,pins = "PH5";
+		allwinner,function = "gpio_in";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_PULL_DOWN>;
+	};
+};
+
+&reg_usb0_vbus {
+	status = "okay";
+};
+
 &reg_usb2_vbus {
 	status = "okay";
 };

Hi Hans,

Quoted 42 lines · Hans de Goede

Enable the otg/drc usb controller on the Chuwi V7 CW0825 tablet.

Signed-off-by Hans de Goede <[email protected]>

---
  arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts | 31 +++++++++++++++++++++++++
  1 file changed, 31 insertions(+)

arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts +20 -0

index 97fca89..034396c 100644
@@ -111,6 +111,26 @@
  	status = "okay";
  };
 
+&pio {
+	usb0_id_detect_pin: usb0_id_detect_pin at 0 {
+		allwinner,pins = "PH4";
+		allwinner,function = "gpio_in";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
+	};
+
+	usb0_vbus_detect_pin: usb0_vbus_detect_pin at 0 {
+		allwinner,pins = "PH5";
+		allwinner,function = "gpio_in";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_PULL_DOWN>;
+	};
+};
+
+&reg_usb0_vbus {
+	status = "okay";
+};
+
  &reg_usb2_vbus {
  	status = "okay";
  };

@@ -121,7 +141,18 @@
  	status = "okay";
  };
 

arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts line 150

Quoted 7 lines

arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts +7 -0

+&usb_otg {
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb0_id_detect_pin
+		     &usb0_vbus_detect_pin>;
+	id_det-gpio = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
+	vbus_det-gpio = <&pio 7 5 GPIO_ACTIVE_HIGH>; /* PH5 */
+	dr_mode = "otg";

Is there a reason to not put that in the DTSI?

All the users seem to be wiring it as OTG.

Hi,

Quoted 45 lines · Maxime Ripard

Hi Hans,

Quoted 42 lines · Hans de Goede

Enable the otg/drc usb controller on the Chuwi V7 CW0825 tablet.

Signed-off-by Hans de Goede <[email protected]>

---
 arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts | 31 +++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts +20 -0

index 97fca89..034396c 100644
@@ -111,6 +111,26 @@
 	status = "okay";
 };
 
+&pio {
+	usb0_id_detect_pin: usb0_id_detect_pin at 0 {
+		allwinner,pins = "PH4";
+		allwinner,function = "gpio_in";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
+	};
+
+	usb0_vbus_detect_pin: usb0_vbus_detect_pin at 0 {
+		allwinner,pins = "PH5";
+		allwinner,function = "gpio_in";
+		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+		allwinner,pull = <SUN4I_PINCTRL_PULL_DOWN>;
+	};
+};
+
+&reg_usb0_vbus {
+	status = "okay";
+};
+
 &reg_usb2_vbus {
 	status = "okay";
 };

@@ -121,7 +141,18 @@
 	status = "okay";
 };
 

Quoted 9 lines

arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts line 150

Quoted 7 lines

arch/arm/boot/dts/sun4i-a10-chuwi-v7-cw0825.dts +7 -0

+&usb_otg {
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb0_id_detect_pin
+		     &usb0_vbus_detect_pin>;
+	id_det-gpio = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
+	vbus_det-gpio = <&pio 7 5 GPIO_ACTIVE_HIGH>; /* PH5 */
+	dr_mode = "otg";

Is there a reason to not put that in the DTSI?

This is not in the dtsi because it is board specific.

Quoted 1 line

All the users seem to be wiring it as OTG.

Sofar I've only enabled the otg on a handful of boards, the otg on the Mele-M9 for example is connected to a usb->sata bridge and as such should be brought up in host only mode.

And on the wobo-i5 (which I still need to create a dts file for) the otg is connected to an usb wifi module.

Hi All,

This patch set has been a while in the making, so I'm very happy to present the end result here, and I hope everyone likes it.

Before talking about merging this there are 2 things which I would like to point out:

a) The musb controller in the sunxi SoCs uses some SRAM which needs to be mapped to the musb controller by poking some bits in the SRAM controller, just like the EMAC patches which were send a while back I've chosen to use syscon for this, actually 2 of the patches in this set come directly from the SRAM mapping patchset for the EMAC.

I know that Maxime is not 100% in favor of using syscon:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320221.html

But I disagree with his arguments for writing a special driver for the SRAM controller:
1) syscon was specifically designed for global system control registers like this and is fine to use as long as there are no conflicts where 1 bit is of interest to multiple drivers, and there is no such conflict here
2) Maxime's other arguments seem to boil down to it would be nice / prettier to have a specific driver for this, without really proving a hard need for such a driver. But writing such a driver is going to be a lot of work, and we've a ton of other work to do, and as said there is no real need for a separate driver, syscon works fine for this.
3) I actually believe that having a specific driver for this is a bad idea, because that means inventing a whole new cross driver API for this, and getting those right is, hard, a lot of work, and even then one is still likely to get it wrong. We can avoid all this by going with the proven syscon solution.

Maxime, can we please have your ack for moving forward with this using syscon? (see above for my arguments why)

Quoted 4 lines · On Monday 09 March 2015 21:40:13 Hans de Goede wrote:

Hi All,

This patch set has been a while in the making, so I'm very happy to present the end result here, and I hope everyone likes it.

Awesome work!

Quoted 27 lines

Before talking about merging this there are 2 things which I would like to point out:

a) The musb controller in the sunxi SoCs uses some SRAM which needs to be mapped to the musb controller by poking some bits in the SRAM controller, just like the EMAC patches which were send a while back I've chosen to use syscon for this, actually 2 of the patches in this set come directly from the SRAM mapping patchset for the EMAC.

I know that Maxime is not 100% in favor of using syscon:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320221.html

But I disagree with his arguments for writing a special driver for the SRAM controller:
1) syscon was specifically designed for global system control registers like this and is fine to use as long as there are no conflicts where 1 bit is of interest to multiple drivers, and there is no such conflict here
2) Maxime's other arguments seem to boil down to it would be nice / prettier to have a specific driver for this, without really proving a hard need for such a driver. But writing such a driver is going to be a lot of work, and we've a ton of other work to do, and as said there is no real need for a separate driver, syscon works fine for this.
3) I actually believe that having a specific driver for this is a bad idea, because that means inventing a whole new cross driver API for this, and getting those right is, hard, a lot of work, and even then one is still likely to get it wrong. We can avoid all this by going with the proven syscon solution.

Quoted 2 lines

Maxime, can we please have your ack for moving forward with this using syscon? (see above for my arguments why)

I'd like to understand here why we can't use the existing SRAM DT binding instead of the syscon binding.

Hi Arnd,

Quoted 35 lines · Arnd Bergmann

Quoted 4 lines · On Monday 09 March 2015 21:40:13 Hans de Goede wrote:

Hi All,

This patch set has been a while in the making, so I'm very happy to present the end result here, and I hope everyone likes it.

Awesome work!

Quoted 27 lines

Before talking about merging this there are 2 things which I would like to point out:

a) The musb controller in the sunxi SoCs uses some SRAM which needs to be mapped to the musb controller by poking some bits in the SRAM controller, just like the EMAC patches which were send a while back I've chosen to use syscon for this, actually 2 of the patches in this set come directly from the SRAM mapping patchset for the EMAC.

I know that Maxime is not 100% in favor of using syscon:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320221.html

But I disagree with his arguments for writing a special driver for the SRAM controller:
1) syscon was specifically designed for global system control registers like this and is fine to use as long as there are no conflicts where 1 bit is of interest to multiple drivers, and there is no such conflict here
2) Maxime's other arguments seem to boil down to it would be nice / prettier to have a specific driver for this, without really proving a hard need for such a driver. But writing such a driver is going to be a lot of work, and we've a ton of other work to do, and as said there is no real need for a separate driver, syscon works fine for this.
3) I actually believe that having a specific driver for this is a bad idea, because that means inventing a whole new cross driver API for this, and getting those right is, hard, a lot of work, and even then one is still likely to get it wrong. We can avoid all this by going with the proven syscon solution.

Quoted 5 lines

Quoted 2 lines

Maxime, can we please have your ack for moving forward with this using syscon? (see above for my arguments why)

I'd like to understand here why we can't use the existing SRAM DT binding instead of the syscon binding.

I believe you are talking about "mmio-sram"?

The syscon here represents a switch, to toggle whether a block of SRAM is mapped into the CPU memory space, or to a specific devices private address space. It is not the actual SRAM.

The SRAM DT binding is orthogonal to this, if not irrelevant when the block is mapped privately, as it is no longer visible from the DT's PoV.

Coincidentally, on the A23 this is no longer needed. The SRAM for the FIFO is wholly owned by MUSB and not available to the CPU.

I believe you are talking about "mmio-sram"?

The syscon here represents a switch, to toggle whether a block of SRAM is mapped into the CPU memory space, or to a specific devices private address space. It is not the actual SRAM.

The SRAM DT binding is orthogonal to this, if not irrelevant when the block is mapped privately, as it is no longer visible from the DT's PoV.

Hi All,

This patch set has been a while in the making, so I'm very happy to present the end result here, and I hope everyone likes it.

Before talking about merging this there are 2 things which I would like to point out:

a) The musb controller in the sunxi SoCs uses some SRAM which needs to be mapped to the musb controller by poking some bits in the SRAM controller, just like the EMAC patches which were send a while back I've chosen to use syscon for this, actually 2 of the patches in this set come directly from the SRAM mapping patchset for the EMAC.

I know that Maxime is not 100% in favor of using syscon:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320221.html

But I disagree with his arguments for writing a special driver for the SRAM controller:

1) syscon was specifically designed for global system control registers like this and is fine to use as long as there are no conflicts where 1 bit is of interest to multiple drivers, and there is no such conflict here

2) Maxime's other arguments seem to boil down to it would be nice / prettier to have a specific driver for this, without really proving a hard need for such a driver. But writing such a driver is going to be a lot of work, and we've a ton of other work to do, and as said there is no real need for a separate driver, syscon works fine for this.

3) I actually believe that having a specific driver for this is a bad idea, because that means inventing a whole new cross driver API for this, and getting those right is, hard, a lot of work, and even then one is still likely to get it wrong. We can avoid all this by going with the proven syscon solution.

Maxime, can we please have your ack for moving forward with this using syscon? (see above for my arguments why)

Quoted 14 lines · Hans de Goede

Hi All,

This patch set has been a while in the making, so I'm very happy to present the end result here, and I hope everyone likes it.

Before talking about merging this there are 2 things which I would like to point out:

a) The musb controller in the sunxi SoCs uses some SRAM which needs to be mapped to the musb controller by poking some bits in the SRAM controller, just like the EMAC patches which were send a while back I've chosen to use syscon for this, actually 2 of the patches in this set come directly from the SRAM mapping patchset for the EMAC.

Quoted 5 lines

I know that Maxime is not 100% in favor of using syscon:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320221.html

But I disagree with his arguments for writing a special driver for the SRAM controller:

And I disagree with your disagreement :)

Quoted 3 lines

1) syscon was specifically designed for global system control registers like this and is fine to use as long as there are no conflicts where 1 bit is of interest to multiple drivers, and there is no such conflict here

No. Syscon has been designed for being lazy.

This is a huge abstraction non-sense. You have to put all the logic of dealing with some external register layout in clients drivers, including dealing with the different revisions/SoC that are different from that aspect, and duplicating that code across all client drivers.

Quoted 5 lines

2) Maxime's other arguments seem to boil down to it would be nice / prettier to have a specific driver for this, without really proving a hard need for such a driver. But writing such a driver is going to be a lot of work, and we've a ton of other work to do, and as said there is no real need for a separate driver, syscon works fine for this.

Actually, I already wrote some prototype for this. I'll clean this up and send it tonight/tomorrow.