[PATCH 1/2] USB: OHCI: Eliminate platform-specific test in ohci.h

[Kevin Cernekee]

OHCI_QUIRK_FRAME_NO is currently set under either of the following
conditions:

1) If a ppc-of-ohci DT node indicates a compatible string of
"fsl,mpc5200-ohci" or "mpc5200-ohci"

2) If usb_ohci_pdata->no_big_frame_no is set

For #1, the affected platforms already enable CONFIG_PPC_MPC52xx.
For #2, there are currently no in-tree users.

So we can safely remove the #ifdef, and thereby allow OHCI_QUIRK_FRAME_NO
to be used by other (non-PPC) platforms that have the same property.
bcm63xx and bcm3384 are two such users.

Signed-off-by: Kevin Cernekee <cernekee-***@public.gmane.org>
---
drivers/usb/host/ohci.h | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index 59f4245..bc46228 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -647,23 +647,22 @@ static inline u32 hc32_to_cpup (const struct ohci_hcd *ohci, const __hc32 *x)

/*-------------------------------------------------------------------------*/

-/* HCCA frame number is 16 bits, but is accessed as 32 bits since not all
- * hardware handles 16 bit reads. That creates a different confusion on
- * some big-endian SOC implementations. Same thing happens with PSW access.
+/*
+ * The HCCA frame number is 16 bits, but is accessed as 32 bits since not all
+ * hardware handles 16 bit reads. Depending on the SoC implementation, the
+ * frame number can wind up in either bits [31:16] (default) or
+ * [15:0] (OHCI_QUIRK_FRAME_NO) on big endian hosts.
+ *
+ * Somewhat similarly, the 16-bit PSW fields in a transfer descriptor are
+ * reordered on BE.
*/

-#ifdef CONFIG_PPC_MPC52xx
-#define big_endian_frame_no_quirk(ohci) (ohci->flags & OHCI_QUIRK_FRAME_NO)
-#else
-#define big_endian_frame_no_quirk(ohci) 0
-#endif

[Kevin Cernekee]

These quirks are currently set through platform_data; allow DT-based SoCs
to use them too.

Signed-off-by: Kevin Cernekee <cernekee-***@public.gmane.org>
---
Documentation/devicetree/bindings/usb/usb-ohci.txt | 2 ++
drivers/usb/host/ohci-platform.c | 6 ++++++
2 files changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ohci.txt b/Documentation/devicetree/bindings/usb/usb-ohci.txt
index b968a1a..19233b7 100644
--- a/Documentation/devicetree/bindings/usb/usb-ohci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ohci.txt
@@ -9,6 +9,8 @@ Optional properties:
- big-endian-regs : boolean, set this for hcds with big-endian registers
- big-endian-desc : boolean, set this for hcds with big-endian descriptors
- big-endian : boolean, for hcds with big-endian-regs + big-endian-desc
+- no-big-frame-no : boolean, set if frame_no lives in bits [15:0] of HCCA
+- num-ports : u32, to override the detected port count
- clocks : a list of phandle + clock specifier pairs
- phys : phandle + phy specifier pair
- phy-names : "usb"
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 4369299..6fb03f8 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -175,6 +175,12 @@ static int ohci_platform_probe(struct platform_device *dev)
if (of_property_read_bool(dev->dev.of_node, "big-endian"))
ohci->flags |= OHCI_QUIRK_BE_MMIO | OHCI_QUIRK_BE_DESC;

+ if (of_property_read_bool(dev->dev.of_node, "no-big-frame-no"))
+ ohci->flags |= OHCI_QUIRK_FRAME_NO;
+
+ of_property_read_u32(dev->dev.of_node, "num-ports",
+ &ohci->num_ports);
+
priv->phy = devm_phy_get(&dev->dev, "usb");
if (IS_ERR(priv->phy)) {
err = PTR_ERR(priv->phy);

[Alan Stern]

Post by Kevin Cernekee
These quirks are currently set through platform_data; allow DT-based SoCs
to use them too.

It looks strange to have the platform_data version of the quirks set in
one routine and the DT version set in a different routine. Is there
any reason not to set all of them in ohci_platform_probe? That would
allow us to eliminate ohci_platform_reset.

Alan Stern


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Kevin Cernekee]

Post by Alan Stern

Post by Kevin Cernekee
These quirks are currently set through platform_data; allow DT-based SoCs
to use them too.

It looks strange to have the platform_data version of the quirks set in
one routine and the DT version set in a different routine. Is there
any reason not to set all of them in ohci_platform_probe? That would
allow us to eliminate ohci_platform_reset.

I think it is mostly for historical reasons. In Hauke's original
driver submission (commit fa3364b5a2d79), all of the platform_data
checks were in ohci_platform_reset().

Prior to commit 928fb68e2357be (make ohci-platform a separate driver)
it looks like there were some ordering dependencies involving calls to
ohci_hcd_init():

commit 2b16e39ee0a431d6cf6e6ca33bb08ec7dc82073f
Author: Florian Fainelli <florian-***@public.gmane.org>
Date: Mon Oct 8 15:11:26 2012 +0200

USB: ohci: allow platform driver to specify the number of ports

This patch modifies the ohci platform driver to accept the num_ports
parameter to be set via platform_data. Setting the number of ports must be
done after the call to ohci_hcd_init().


But that doesn't seem to be the case anymore, and in my tests with the
DT num-ports patch, I never saw ohci->num_ports getting overwritten.

Would you like me to submit another patch to move the remaining
platform_data tests from ohci_platform_reset() into
ohci_platform_probe()?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Kevin Cernekee

Post by Alan Stern

Post by Kevin Cernekee
These quirks are currently set through platform_data; allow DT-based SoCs
to use them too.

It looks strange to have the platform_data version of the quirks set in
one routine and the DT version set in a different routine. Is there
any reason not to set all of them in ohci_platform_probe? That would
allow us to eliminate ohci_platform_reset.

I think it is mostly for historical reasons. In Hauke's original
driver submission (commit fa3364b5a2d79), all of the platform_data
checks were in ohci_platform_reset().
Prior to commit 928fb68e2357be (make ohci-platform a separate driver)
it looks like there were some ordering dependencies involving calls to
commit 2b16e39ee0a431d6cf6e6ca33bb08ec7dc82073f
Date: Mon Oct 8 15:11:26 2012 +0200
USB: ohci: allow platform driver to specify the number of ports
This patch modifies the ohci platform driver to accept the num_ports
parameter to be set via platform_data. Setting the number of ports must be
done after the call to ohci_hcd_init().
But that doesn't seem to be the case anymore, and in my tests with the
DT num-ports patch, I never saw ohci->num_ports getting overwritten.
Would you like me to submit another patch to move the remaining
platform_data tests from ohci_platform_reset() into
ohci_platform_probe()?

Yes, that would be good.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Kevin Cernekee
OHCI_QUIRK_FRAME_NO is currently set under either of the following
1) If a ppc-of-ohci DT node indicates a compatible string of
"fsl,mpc5200-ohci" or "mpc5200-ohci"
2) If usb_ohci_pdata->no_big_frame_no is set
For #1, the affected platforms already enable CONFIG_PPC_MPC52xx.
For #2, there are currently no in-tree users.
So we can safely remove the #ifdef, and thereby allow OHCI_QUIRK_FRAME_NO
to be used by other (non-PPC) platforms that have the same property.
bcm63xx and bcm3384 are two such users.

Sorry, but I can't understand this patch description. What #ifdef does
it refer to?

By comparing the description with the patch, it looks like you _wanted_
to say something along these lines:

The bcm63xx and bcm3384 platforms need to set
OHCI_QUIRK_FRAME_NO, but they are non-PPC platforms and
don't enable CONFIG_PPC_MPC52xx. Therefore this patch changes
the code that uses OHCI_QUIRK_FRAME_NO, making it not depend
on CONFIG_PPC_MPC52xx.

Does that properly describe what you are doing?

Alan Stern

Post by Kevin Cernekee
---
drivers/usb/host/ohci.h | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index 59f4245..bc46228 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -647,23 +647,22 @@ static inline u32 hc32_to_cpup (const struct ohci_hcd *ohci, const __hc32 *x)
/*-------------------------------------------------------------------------*/
-/* HCCA frame number is 16 bits, but is accessed as 32 bits since not all
- * hardware handles 16 bit reads. That creates a different confusion on
- * some big-endian SOC implementations. Same thing happens with PSW access.
+/*
+ * The HCCA frame number is 16 bits, but is accessed as 32 bits since not all
+ * hardware handles 16 bit reads. Depending on the SoC implementation, the
+ * frame number can wind up in either bits [31:16] (default) or
+ * [15:0] (OHCI_QUIRK_FRAME_NO) on big endian hosts.
+ *
+ * Somewhat similarly, the 16-bit PSW fields in a transfer descriptor are
+ * reordered on BE.
*/
-#ifdef CONFIG_PPC_MPC52xx
-#define big_endian_frame_no_quirk(ohci) (ohci->flags & OHCI_QUIRK_FRAME_NO)
-#else
-#define big_endian_frame_no_quirk(ohci) 0
-#endif
-
static inline u16 ohci_frame_no(const struct ohci_hcd *ohci)
{
u32 tmp;
if (big_endian_desc(ohci)) {
tmp = be32_to_cpup((__force __be32 *)&ohci->hcca->frame_no);
- if (!big_endian_frame_no_quirk(ohci))
+ if (!(ohci->flags & OHCI_QUIRK_FRAME_NO))
tmp >>= 16;
} else
tmp = le32_to_cpup((__force __le32 *)&ohci->hcca->frame_no);

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Kevin Cernekee]

Post by Alan Stern

Post by Kevin Cernekee
OHCI_QUIRK_FRAME_NO is currently set under either of the following
1) If a ppc-of-ohci DT node indicates a compatible string of
"fsl,mpc5200-ohci" or "mpc5200-ohci"
2) If usb_ohci_pdata->no_big_frame_no is set
For #1, the affected platforms already enable CONFIG_PPC_MPC52xx.
For #2, there are currently no in-tree users.
So we can safely remove the #ifdef, and thereby allow OHCI_QUIRK_FRAME_NO
to be used by other (non-PPC) platforms that have the same property.
bcm63xx and bcm3384 are two such users.

Sorry, but I can't understand this patch description. What #ifdef does
it refer to?

It refers to "#ifdef CONFIG_PPC_MPC52xx". This check appears to be
redundant: from what I can tell, it isn't ever necessary to gate the
workaround logic based on CONFIG_PPC_MPC52xx, because currently
OHCI_QUIRK_FRAME_NO is only getting set for (some) mpc52xx chips.

So, removing "#ifdef CONFIG_PPC_MPC52xx" both eliminates PPC-specific
code from the generic driver, and allows bcm63xx/bcm3384 to make use
of OHCI_QUIRK_FRAME_NO.

Post by Alan Stern
By comparing the description with the patch, it looks like you _wanted_
The bcm63xx and bcm3384 platforms need to set
OHCI_QUIRK_FRAME_NO, but they are non-PPC platforms and
don't enable CONFIG_PPC_MPC52xx. Therefore this patch changes
the code that uses OHCI_QUIRK_FRAME_NO, making it not depend
on CONFIG_PPC_MPC52xx.
Does that properly describe what you are doing?

Yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Alan Stern]

Post by Kevin Cernekee

Post by Alan Stern

Post by Kevin Cernekee
OHCI_QUIRK_FRAME_NO is currently set under either of the following
1) If a ppc-of-ohci DT node indicates a compatible string of
"fsl,mpc5200-ohci" or "mpc5200-ohci"
2) If usb_ohci_pdata->no_big_frame_no is set
For #1, the affected platforms already enable CONFIG_PPC_MPC52xx.
For #2, there are currently no in-tree users.
So we can safely remove the #ifdef, and thereby allow OHCI_QUIRK_FRAME_NO
to be used by other (non-PPC) platforms that have the same property.
bcm63xx and bcm3384 are two such users.

Sorry, but I can't understand this patch description. What #ifdef does
it refer to?

It refers to "#ifdef CONFIG_PPC_MPC52xx". This check appears to be
redundant: from what I can tell, it isn't ever necessary to gate the
workaround logic based on CONFIG_PPC_MPC52xx, because currently
OHCI_QUIRK_FRAME_NO is only getting set for (some) mpc52xx chips.

That's not the reason for the #ifdef. It's there to eliminate a
runtime test on systems where we know at build-time that the test can't
succeed. It isn't necessary; it's just a small optimization.

Post by Kevin Cernekee
So, removing "#ifdef CONFIG_PPC_MPC52xx" both eliminates PPC-specific
code from the generic driver, and allows bcm63xx/bcm3384 to make use
of OHCI_QUIRK_FRAME_NO.

Post by Alan Stern
By comparing the description with the patch, it looks like you _wanted_
The bcm63xx and bcm3384 platforms need to set
OHCI_QUIRK_FRAME_NO, but they are non-PPC platforms and
don't enable CONFIG_PPC_MPC52xx. Therefore this patch changes
the code that uses OHCI_QUIRK_FRAME_NO, making it not depend
on CONFIG_PPC_MPC52xx.
Does that properly describe what you are doing?

Yes.

All right, then please resubmit the patch with a more suitable
description. (You might also mention that the patch rephrases some
comments.) I have no objection to the patch itself.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html