| Message ID | 1421945805-31129-5-git-send-email-sylvain.rochet@finsecur.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 22 Jan 2015 17:56:44 +0100 Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > Renamed struct usba_udc_errata to struct usba_udc_caps, we are adding a > new property which is not about errata, this way the struct is not > misnamed. > > New struct usba_udc_caps property: irq_single_edge_support, boolean, > set to true if the board supports IRQ_TYPE_EDGE_FALLING and > IRQ_TYPE_EDGE_RISING, otherwise set to false. > > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/usb/gadget/udc/atmel_usba_udc.c | 25 +++++++++++++++---------- > drivers/usb/gadget/udc/atmel_usba_udc.h | 5 +++-- > 2 files changed, 18 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c > index d554106..361f740 100644 > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > @@ -338,8 +338,8 @@ static int vbus_is_present(struct usba_udc *udc) > > static void toggle_bias(struct usba_udc *udc, int is_on) > { > - if (udc->errata && udc->errata->toggle_bias) > - udc->errata->toggle_bias(udc, is_on); > + if (udc->caps && udc->caps->toggle_bias) > + udc->caps->toggle_bias(udc, is_on); > } > > static void generate_bias_pulse(struct usba_udc *udc) > @@ -347,8 +347,8 @@ static void generate_bias_pulse(struct usba_udc *udc) > if (!udc->bias_pulse_needed) > return; > > - if (udc->errata && udc->errata->pulse_bias) > - udc->errata->pulse_bias(udc); > + if (udc->caps && udc->caps->pulse_bias) > + udc->caps->pulse_bias(udc); > > udc->bias_pulse_needed = false; > } > @@ -1901,18 +1901,23 @@ static void at91sam9g45_pulse_bias(struct usba_udc *udc) > at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN); > } > > -static const struct usba_udc_errata at91sam9rl_errata = { > +static const struct usba_udc_caps at91sam9rl_caps = { > .toggle_bias = at91sam9rl_toggle_bias, > }; > > -static const struct usba_udc_errata at91sam9g45_errata = { > +static const struct usba_udc_caps at91sam9g45_caps = { > .pulse_bias = at91sam9g45_pulse_bias, > + .irq_single_edge_support = true, > +}; > + > +static const struct usba_udc_caps sama5d3_caps = { > + .irq_single_edge_support = true, > }; > > static const struct of_device_id atmel_udc_dt_ids[] = { > - { .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata }, > - { .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata }, > - { .compatible = "atmel,sama5d3-udc" }, > + { .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_caps }, > + { .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_caps }, > + { .compatible = "atmel,sama5d3-udc", .data = &sama5d3_caps }, > { /* sentinel */ } > }; > > @@ -1934,7 +1939,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev, > if (!match) > return ERR_PTR(-EINVAL); > > - udc->errata = match->data; > + udc->caps = match->data; > > udc->num_ep = 0; > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h > index 085749a..4fe4c87 100644 > --- a/drivers/usb/gadget/udc/atmel_usba_udc.h > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h > @@ -304,9 +304,10 @@ struct usba_request { > unsigned int mapped:1; > }; > > -struct usba_udc_errata { > +struct usba_udc_caps { > void (*toggle_bias)(struct usba_udc *udc, int is_on); > void (*pulse_bias)(struct usba_udc *udc); > + bool irq_single_edge_support; > }; > > struct usba_udc { > @@ -322,7 +323,7 @@ struct usba_udc { > struct usb_gadget gadget; > struct usb_gadget_driver *driver; > struct platform_device *pdev; > - const struct usba_udc_errata *errata; > + const struct usba_udc_caps *caps; > int irq; > int vbus_pin; > int vbus_pin_inverted;
Le 22/01/2015 18:14, Boris Brezillon a écrit : > On Thu, 22 Jan 2015 17:56:44 +0100 > Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > >> Renamed struct usba_udc_errata to struct usba_udc_caps, we are adding a >> new property which is not about errata, this way the struct is not >> misnamed. >> >> New struct usba_udc_caps property: irq_single_edge_support, boolean, >> set to true if the board supports IRQ_TYPE_EDGE_FALLING and >> IRQ_TYPE_EDGE_RISING, otherwise set to false. >> >> Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> > > Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> Some comments: >> --- >> drivers/usb/gadget/udc/atmel_usba_udc.c | 25 +++++++++++++++---------- >> drivers/usb/gadget/udc/atmel_usba_udc.h | 5 +++-- >> 2 files changed, 18 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c >> index d554106..361f740 100644 >> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c >> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c >> @@ -338,8 +338,8 @@ static int vbus_is_present(struct usba_udc *udc) >> >> static void toggle_bias(struct usba_udc *udc, int is_on) >> { >> - if (udc->errata && udc->errata->toggle_bias) >> - udc->errata->toggle_bias(udc, is_on); >> + if (udc->caps && udc->caps->toggle_bias) >> + udc->caps->toggle_bias(udc, is_on); >> } >> >> static void generate_bias_pulse(struct usba_udc *udc) >> @@ -347,8 +347,8 @@ static void generate_bias_pulse(struct usba_udc *udc) >> if (!udc->bias_pulse_needed) >> return; >> >> - if (udc->errata && udc->errata->pulse_bias) >> - udc->errata->pulse_bias(udc); >> + if (udc->caps && udc->caps->pulse_bias) >> + udc->caps->pulse_bias(udc); >> >> udc->bias_pulse_needed = false; >> } >> @@ -1901,18 +1901,23 @@ static void at91sam9g45_pulse_bias(struct usba_udc *udc) >> at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN); >> } >> >> -static const struct usba_udc_errata at91sam9rl_errata = { >> +static const struct usba_udc_caps at91sam9rl_caps = { >> .toggle_bias = at91sam9rl_toggle_bias, >> }; >> >> -static const struct usba_udc_errata at91sam9g45_errata = { >> +static const struct usba_udc_caps at91sam9g45_caps = { >> .pulse_bias = at91sam9g45_pulse_bias, >> + .irq_single_edge_support = true, Nope! at91sam9g45 doesn't have this property. You'll have to create another compatible string and capabilities structure ("atmel,at91sam9x5-udc") But still, I don't know if it's the proper approach. The possibility tro trigger an IRQ on both edges or a single edge is a capacity of the gpio controller, not the USBA IP. So, it's a little bit strange to have this capability here. I don't know if it's actually feasible but trying to configure the IRQ on a single edge, testing if it's accepted by the GPIO irq controller and if not, falling back to a "both edge" pattern. Doesn't it look like a way to workaround this issue nicely? Can you give it a try? Bye, >> +}; >> + >> +static const struct usba_udc_caps sama5d3_caps = { >> + .irq_single_edge_support = true, >> }; >> >> static const struct of_device_id atmel_udc_dt_ids[] = { >> - { .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata }, >> - { .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata }, >> - { .compatible = "atmel,sama5d3-udc" }, >> + { .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_caps }, >> + { .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_caps }, >> + { .compatible = "atmel,sama5d3-udc", .data = &sama5d3_caps }, >> { /* sentinel */ } >> }; >> >> @@ -1934,7 +1939,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev, >> if (!match) >> return ERR_PTR(-EINVAL); >> >> - udc->errata = match->data; >> + udc->caps = match->data; >> >> udc->num_ep = 0; >> >> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h >> index 085749a..4fe4c87 100644 >> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h >> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h >> @@ -304,9 +304,10 @@ struct usba_request { >> unsigned int mapped:1; >> }; >> >> -struct usba_udc_errata { >> +struct usba_udc_caps { >> void (*toggle_bias)(struct usba_udc *udc, int is_on); >> void (*pulse_bias)(struct usba_udc *udc); >> + bool irq_single_edge_support; >> }; >> >> struct usba_udc { >> @@ -322,7 +323,7 @@ struct usba_udc { >> struct usb_gadget gadget; >> struct usb_gadget_driver *driver; >> struct platform_device *pdev; >> - const struct usba_udc_errata *errata; >> + const struct usba_udc_caps *caps; >> int irq; >> int vbus_pin; >> int vbus_pin_inverted; > > >
Hello Nicolas, On Thu, Feb 05, 2015 at 06:19:55PM +0100, Nicolas Ferre wrote: > Le 22/01/2015 18:14, Boris Brezillon a écrit : > > On Thu, 22 Jan 2015 17:56:44 +0100 > > Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > > > > > -static const struct usba_udc_errata at91sam9g45_errata = { > > > +static const struct usba_udc_caps at91sam9g45_caps = { > > > .pulse_bias = at91sam9g45_pulse_bias, > > > + .irq_single_edge_support = true, > > Nope! at91sam9g45 doesn't have this property. You'll have to create > another compatible string and capabilities structure > ("atmel,at91sam9x5-udc") Oops. > But still, I don't know if it's the proper approach. The possibility to > trigger an IRQ on both edges or a single edge is a capacity of the gpio > controller, not the USBA IP. So, it's a little bit strange to have this > capability here. I agree. > I don't know if it's actually feasible but trying to configure the IRQ > on a single edge, testing if it's accepted by the GPIO irq controller > and if not, falling back to a "both edge" pattern. Doesn't it look like > a way to workaround this issue nicely? Can you give it a try? Tried, it works, but it displays the following message from __irq_set_trigger() [1] during devm_request_threaded_irq(…, IRQF_TRIGGER_RISING, …) on boards which does not support single-edge IRQ: genirq: Setting trigger mode 1 for irq 176 failed (gpio_irq_type+0x0/0x34) Is it acceptable ? If not, is udc->caps->irq_single_edge_support boolean acceptable ? If not, I am ok to drop the feature, this is only a bonus. Sylvain [1] http://lxr.free-electrons.com/source/kernel/irq/manage.c#L619
On Sat, 7 Feb 2015 20:37:23 +0100 Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > Hello Nicolas, > > > On Thu, Feb 05, 2015 at 06:19:55PM +0100, Nicolas Ferre wrote: > > Le 22/01/2015 18:14, Boris Brezillon a écrit : > > > On Thu, 22 Jan 2015 17:56:44 +0100 > > > Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > > > > > > > -static const struct usba_udc_errata at91sam9g45_errata = { > > > > +static const struct usba_udc_caps at91sam9g45_caps = { > > > > .pulse_bias = at91sam9g45_pulse_bias, > > > > + .irq_single_edge_support = true, > > > > Nope! at91sam9g45 doesn't have this property. You'll have to create > > another compatible string and capabilities structure > > ("atmel,at91sam9x5-udc") > > Oops. > > > > But still, I don't know if it's the proper approach. The possibility to > > trigger an IRQ on both edges or a single edge is a capacity of the gpio > > controller, not the USBA IP. So, it's a little bit strange to have this > > capability here. > > I agree. Me too (even if I'm the one who proposed this approach in the first place ;-)). > > > > I don't know if it's actually feasible but trying to configure the IRQ > > on a single edge, testing if it's accepted by the GPIO irq controller > > and if not, falling back to a "both edge" pattern. Doesn't it look like > > a way to workaround this issue nicely? Can you give it a try? > > Tried, it works, but it displays the following message from > __irq_set_trigger() [1] during devm_request_threaded_irq(…, > IRQF_TRIGGER_RISING, …) on boards which does not support single-edge > IRQ: > > genirq: Setting trigger mode 1 for irq 176 failed (gpio_irq_type+0x0/0x34) > > Is it acceptable ? IMHO it's not. > If not, is udc->caps->irq_single_edge_support boolean acceptable ? Do you mean keeping the current approach ? If you do, then maybe you can rework a bit the way you detect the GPIO controller you depends on: instead of linking this information to the usba compatible string you could link it to the gpio controller compatible string. You can find the gpio controller node thanks to your "vbus-gpio" property: use the phandle defined in this property to find the gpio controller node, and once you have the device_node referencing the gpio controller you can match it with your internal device_id table (containing 2 entries: one for the at91rm9200 IP and the other for the at91sam9x5 IP). Another solution would be to add an irq_try_set_irq_type function that would not complain when it fails to set the requested trigger. Thomas, I know you did not follow the whole thread, but would you mind adding this irq_try_set_irq_type function (here is a reference implementation [1]), to prevent this error trace from happening when we're just trying a configuration ? > If not, I am ok to drop the feature, this is only a bonus. That could be a short term solution, to get this series accepted. We could then find a proper way to support that optimization. Best Regards, Boris [1]http://code.bulix.org/z4bu9k-87846
Hello Boris, On Sun, Feb 08, 2015 at 10:24:39AM +0100, Boris Brezillon wrote: > On Sat, 7 Feb 2015 20:37:23 +0100 > Sylvain Rochet <sylvain.rochet@finsecur.com> wrote: > > > If not, is udc->caps->irq_single_edge_support boolean acceptable ? > > Do you mean keeping the current approach ? Yes! > If you do, then maybe you can rework a bit the way you detect the GPIO > controller you depends on: instead of linking this information to the > usba compatible string you could link it to the gpio controller > compatible string. > > You can find the gpio controller node thanks to your "vbus-gpio" > property: use the phandle defined in this property to find the gpio > controller node, and once you have the device_node referencing the gpio > controller you can match it with your internal device_id table > (containing 2 entries: one for the at91rm9200 IP and the other for the > at91sam9x5 IP). I have a working PoC for that if this is the chosen solution. > Another solution would be to add an irq_try_set_irq_type function that > would not complain when it fails to set the requested trigger. > > Thomas, I know you did not follow the whole thread, but would you mind > adding this irq_try_set_irq_type function (here is a reference > implementation [1]), to prevent this error trace from happening when > we're just trying a configuration ? This would be great :-) > > If not, I am ok to drop the feature, this is only a bonus. > > That could be a short term solution, to get this series accepted. We > could then find a proper way to support that optimization. I agree, I have the feeling your proposed core change may takes a long time, I just sent a v7 without IRQ single edge support. Sylvain
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c index d554106..361f740 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.c +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c @@ -338,8 +338,8 @@ static int vbus_is_present(struct usba_udc *udc) static void toggle_bias(struct usba_udc *udc, int is_on) { - if (udc->errata && udc->errata->toggle_bias) - udc->errata->toggle_bias(udc, is_on); + if (udc->caps && udc->caps->toggle_bias) + udc->caps->toggle_bias(udc, is_on); } static void generate_bias_pulse(struct usba_udc *udc) @@ -347,8 +347,8 @@ static void generate_bias_pulse(struct usba_udc *udc) if (!udc->bias_pulse_needed) return; - if (udc->errata && udc->errata->pulse_bias) - udc->errata->pulse_bias(udc); + if (udc->caps && udc->caps->pulse_bias) + udc->caps->pulse_bias(udc); udc->bias_pulse_needed = false; } @@ -1901,18 +1901,23 @@ static void at91sam9g45_pulse_bias(struct usba_udc *udc) at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN); } -static const struct usba_udc_errata at91sam9rl_errata = { +static const struct usba_udc_caps at91sam9rl_caps = { .toggle_bias = at91sam9rl_toggle_bias, }; -static const struct usba_udc_errata at91sam9g45_errata = { +static const struct usba_udc_caps at91sam9g45_caps = { .pulse_bias = at91sam9g45_pulse_bias, + .irq_single_edge_support = true, +}; + +static const struct usba_udc_caps sama5d3_caps = { + .irq_single_edge_support = true, }; static const struct of_device_id atmel_udc_dt_ids[] = { - { .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata }, - { .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata }, - { .compatible = "atmel,sama5d3-udc" }, + { .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_caps }, + { .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_caps }, + { .compatible = "atmel,sama5d3-udc", .data = &sama5d3_caps }, { /* sentinel */ } }; @@ -1934,7 +1939,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev, if (!match) return ERR_PTR(-EINVAL); - udc->errata = match->data; + udc->caps = match->data; udc->num_ep = 0; diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h index 085749a..4fe4c87 100644 --- a/drivers/usb/gadget/udc/atmel_usba_udc.h +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h @@ -304,9 +304,10 @@ struct usba_request { unsigned int mapped:1; }; -struct usba_udc_errata { +struct usba_udc_caps { void (*toggle_bias)(struct usba_udc *udc, int is_on); void (*pulse_bias)(struct usba_udc *udc); + bool irq_single_edge_support; }; struct usba_udc { @@ -322,7 +323,7 @@ struct usba_udc { struct usb_gadget gadget; struct usb_gadget_driver *driver; struct platform_device *pdev; - const struct usba_udc_errata *errata; + const struct usba_udc_caps *caps; int irq; int vbus_pin; int vbus_pin_inverted;
Renamed struct usba_udc_errata to struct usba_udc_caps, we are adding a new property which is not about errata, this way the struct is not misnamed. New struct usba_udc_caps property: irq_single_edge_support, boolean, set to true if the board supports IRQ_TYPE_EDGE_FALLING and IRQ_TYPE_EDGE_RISING, otherwise set to false. Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> --- drivers/usb/gadget/udc/atmel_usba_udc.c | 25 +++++++++++++++---------- drivers/usb/gadget/udc/atmel_usba_udc.h | 5 +++-- 2 files changed, 18 insertions(+), 12 deletions(-)