Hi,
On Wed, 22 Jan 2014 20:28:26 +0100
Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
> On 01/21/2014 08:39 PM, Florian Fainelli wrote:
> > 2014/1/21 Hans de Goede <hdegoede@xxxxxxxxxx>:
> >> This uses the already documented devicetree booleans for this.
> >
> > (I would greatly appreciate if you could CC people who gave you
> > feedback on this before)
>
> Will do.
>
> > A more informative commit message would be welcome, along with a
> > reference to which Device Tree binding documentation you are referring
> > to.
>
> I've added a reference to the bindings doc in the commit msg for my next version.
>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> drivers/usb/host/Kconfig | 3 +++
> >> drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++++--
> >> 2 files changed, 34 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> >> index 237d7b1..4af41f3 100644
> >> --- a/drivers/usb/host/Kconfig
> >> +++ b/drivers/usb/host/Kconfig
> >> @@ -256,6 +256,9 @@ config USB_EHCI_ATH79
> >> config USB_EHCI_HCD_PLATFORM
> >> tristate "Generic EHCI driver for a platform device"
> >> depends on !PPC_OF
> >> + # Support BE on architectures which have readl_be
> >> + select USB_EHCI_BIG_ENDIAN_DESC if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64)
> >> + select USB_EHCI_BIG_ENDIAN_MMIO if (AVR32 || MIPS || MICROBLAZE || SPARC || PPC32 || PPC64)
> >
> > I do not think this is that simple nor correct for at least Microblaze
> > and MIPS since they can run in either BE or LE mode, and those
> > specific platforms should already do the proper select at the
> > board/SoC level. This *might* be correct for SPARC, PPC32 and PPC64,
> > although I believe some specific PPC64 boards can run in little-endian
> > mode like the P-series, SPARC might too.
> >
> > It seems to me that you should not touch this and keep the existing
> > selects in place, if it turns out that the selects are missing the
> > error messages you added below are catching those misuses.
>
> As discussed with Alan, I will drop these lines from my next version.
>
> >> default n
> >> ---help---
> >> Adds an EHCI host driver for a generic platform device, which
> >> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> >> index d8aebc0..5888abb 100644
> >> --- a/drivers/usb/host/ehci-platform.c
> >> +++ b/drivers/usb/host/ehci-platform.c
> >> @@ -55,8 +55,10 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> >>
> >> hcd->has_tt = pdata->has_tt;
> >> ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
> >> - ehci->big_endian_desc = pdata->big_endian_desc;
> >> - ehci->big_endian_mmio = pdata->big_endian_mmio;
> >> + if (pdata->big_endian_desc)
> >> + ehci->big_endian_desc = 1;
> >> + if (pdata->big_endian_mmio)
> >> + ehci->big_endian_mmio = 1;
> >>
> >> if (pdata->pre_setup) {
> >> retval = pdata->pre_setup(hcd);
> >> @@ -142,6 +144,7 @@ static int ehci_platform_probe(struct platform_device *dev)
> >> struct resource *res_mem;
> >> struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
> >> struct ehci_platform_priv *priv;
> >> + struct ehci_hcd *ehci;
> >> int err, irq, clk = 0;
> >>
> >> if (usb_disabled())
> >> @@ -177,8 +180,34 @@ static int ehci_platform_probe(struct platform_device *dev)
> >> platform_set_drvdata(dev, hcd);
> >> dev->dev.platform_data = pdata;
> >> priv = hcd_to_ehci_priv(hcd);
> >> + ehci = hcd_to_ehci(hcd);
> >>
> >> if (pdata == &ehci_platform_defaults && dev->dev.of_node) {
> >> + if (of_property_read_bool(dev->dev.of_node, "big-endian-regs"))
> >> + ehci->big_endian_mmio = 1;
> >> +
> >> + if (of_property_read_bool(dev->dev.of_node, "big-endian-desc"))
> >> + ehci->big_endian_desc = 1;
> >> +
> >> + if (of_property_read_bool(dev->dev.of_node, "big-endian"))
> >> + ehci->big_endian_mmio = ehci->big_endian_desc = 1;
> >
> > Ok, so I am confused now, should you update
> > pdata->ehci_big_endian_{desc,mmio} here or is it valid to directly
> > modify ehci->big_endian_{desc,mmio}, is not there any risk to undo
> > what is done in ehci_platform_reset(), or is ehci_platform_reset()
> > only called for non-DT cases?
>
> Both the pdata checks in ehci_platform_reset() and the dt checks here only
> ever set these flags, neither code path clears them. And in the dt case pdata
> will be NULL and vice versa.
If it's safe to set ehci->big_endian_{desc,mmio} from the _probe()
routine, then maybe the pdata sets in _reset() should be moved into here
instead of adding extra cludges/checks into _reset().
>
> >
> >> +
> >> +#ifndef CONFIG_USB_EHCI_BIG_ENDIAN_MMIO
> >> + if (ehci->big_endian_mmio) {
> >> + dev_err(&dev->dev,
> >> + "Error big-endian-regs not compiled in\n");
> >
> > I do not think using the Device Tree property name would be very
> > informative since this is supposed to guard against misconfigurations
> > for both DT and non-DT enabled platforms
>
> Nope this is in a dt only code path.
Then these built-in checks could be done for both paths, as !DT also
has the issue, just doesn't warn/error out with it.
I also agree with Florian that the error message could be better
worded, or should be at least have a ":" after "Error". When seeing
the error message the fist time, I would wonder what "error
big-endian-regs" are, and why I would want to have them ;)
Finally, IS_ENABLED()* allows you to drop those ugly #ifndefs:
if (ehci->big_endian_mmio &&
!IS_ENABLED(CONFIG_USB_EHCI_BIG_ENDIAN_MMIO)) {
dev_err(&dev->dev,
"Error big-endian-regs not compiled in\n");
looks much nicer IMHO ;)
Regards
Jonas
*Yes, I know IS_BUILTIN() would be more formal correct, but IS_ENABLED()
reads IMHO nicer, and I doubt these config symbols will ever go
tristate ;)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
