Add Aspeed AST2700 ehci support
Subsystems (3)
v3 cover Add Aspeed AST2700 ehci support
This series adds support for the EHCI controller found in Aspeed's new AST2700 SoC.
v3:-ehci-platform.c
- Using dma_mask_64 as a local flag rather than modifying the global defaults, to avoid unintended side effects when multiple devices probe.
v2:-ehci-platform.c
- remove ehci_ast2700_platform replace by of_device_id data for dma_mask_64.
Ryan Chen (2):
dt-bindings: usb: ehci: Add Aspeed AST2700 compatible
usb: ehci: Add Aspeed AST2700 support
.../devicetree/bindings/usb/generic-ehci.yaml | 1 + drivers/usb/host/ehci-platform.c | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-)
Signature · 1 line
2.34.1
PATCH v3 1/2 dt-bindings: usb: ehci: Add Aspeed AST2700 compatible
Add the compatible string for Aspeed AST2700 SoC.
--- Documentation/devicetree/bindings/usb/generic-ehci.yaml | 1 + 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/usb/generic-ehci.yaml +1 -0
@@ -46,6 +46,7 @@ properties: - aspeed,ast2400-ehci - aspeed,ast2500-ehci - aspeed,ast2600-ehci + - aspeed,ast2700-ehci - brcm,bcm3384-ehci - brcm,bcm63268-ehci - brcm,bcm6328-ehci
Signature · 1 line
2.34.1
PATCH v3 2/2 usb: ehci: Add Aspeed AST2700 support
In reply to Ryan ChenUnlike earlier Aspeed SoCs (AST2400/2500/2600) which are limited to 32-bit DMA addressing, the EHCI controller in AST2700 supports 64-bit DMA. Update the EHCI platform driver to make use of this capability by selecting a 64-bit DMA mask when the "aspeed,ast2700-ehci" compatible is present in device tree.
--- drivers/usb/host/ehci-platform.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/usb/host/ehci-platform.c +13 -2
@@ -27,6 +27,7 @@#include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/reset.h> #include <linux/sys_soc.h>
@@ -239,9 +240,11 @@ static int ehci_platform_probe(struct platform_device *dev)struct usb_hcd *hcd;struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev); +const struct of_device_id *match;struct ehci_platform_priv *priv;struct ehci_hcd *ehci;int err, irq, clk =0 ; +bool dma_mask_64;if (usb_disabled ())return -ENODEV ;
@@ -253,8 +256,13 @@ static int ehci_platform_probe(struct platform_device *dev)if (!pdata) pdata = &ehci_platform_defaults; + dma_mask_64 = pdata->dma_mask_64; + match =of_match_device (dev->dev.driver->of_match_table, &dev->dev); +if (match && match->data) + dma_mask_64 = true; + err =dma_coerce_mask_and_coherent (&dev->dev, - pdata->dma_mask_64 ?DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 )); + dma_mask_64 ?DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 ));if (err) {dev_err (&dev->dev,"Error: DMA mask configuration failed\n" );return err;
@@ -298,7 +306,9 @@ static int ehci_platform_probe(struct platform_device *dev)if (of_device_is_compatible (dev->dev.of_node,"aspeed,ast2500-ehci" ) ||of_device_is_compatible (dev->dev.of_node, -"aspeed,ast2600-ehci" )) +"aspeed,ast2600-ehci" ) || +of_device_is_compatible (dev->dev.of_node, +"aspeed,ast2700-ehci" )) ehci->is_aspeed =1 ;if (soc_device_match (quirk_poll_match))
@@ -485,6 +495,7 @@ static const struct of_device_id vt8500_ehci_ids[] = { { .compatible = "wm,prizm-ehci", }, { .compatible = "generic-ehci", }, { .compatible = "cavium,octeon-6335-ehci", }, + { .compatible = "aspeed,ast2700-ehci", .data = (void *)1 }, {} }; MODULE_DEVICE_TABLE(of, vt8500_ehci_ids);
Signature · 1 line
2.34.1
Alan Stern
maintainerRe: [PATCH v3 2/2] usb: ehci: Add Aspeed AST2700 support
Quoted 8 lines · Ryan Chen
Unlike earlier Aspeed SoCs (AST2400/2500/2600) which are limited to 32-bit DMA addressing, the EHCI controller in AST2700 supports 64-bit DMA. Update the EHCI platform driver to make use of this capability by selecting a 64-bit DMA mask when the "aspeed,ast2700-ehci" compatible is present in device tree.
Signed-off-by Ryan Chen <[email protected]>---
This is basically good and it can be merged. However...
Quoted 28 lines
drivers/usb/host/ehci-platform.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)drivers/usb/host/ehci-platform.c +3
-0@@ -27,6 +27,7 @@#include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/reset.h> #include <linux/sys_soc.h> @@ -239,9 +240,11 @@ static int ehci_platform_probe(struct platform_device *dev)struct usb_hcd *hcd;struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev); +const struct of_device_id *match;struct ehci_platform_priv *priv;struct ehci_hcd *ehci;int err, irq, clk =0 ; +bool dma_mask_64;if (usb_disabled ())return -ENODEV ;
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 260
Quoted 5 lines
drivers/usb/host/ehci-platform.c +2
-0@@ -253,8 +256,13 @@ static int ehci_platform_probe(struct platform_device *dev)if (!pdata) pdata = &ehci_platform_defaults; + dma_mask_64 = pdata->dma_mask_64; + match =of_match_device (dev->dev.driver->of_match_table, &dev->dev);
(I just noticed this.) The "dev->dev.driver->of_match_table" part looks odd. Why not just write "vt8500_ehci_ids"? Do you expect that this could ever have a different value?
Alan Stern
Quoted 31 lines
drivers/usb/host/ehci-platform.c +8
-2+if (match && match->data) + dma_mask_64 = true; + err =dma_coerce_mask_and_coherent (&dev->dev, - pdata->dma_mask_64 ?DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 )); + dma_mask_64 ?DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 ));if (err) {dev_err (&dev->dev,"Error: DMA mask configuration failed\n" );return err;@@ -298,7 +306,9 @@ static int ehci_platform_probe(struct platform_device *dev)if (of_device_is_compatible (dev->dev.of_node,"aspeed,ast2500-ehci" ) ||of_device_is_compatible (dev->dev.of_node, -"aspeed,ast2600-ehci" )) +"aspeed,ast2600-ehci" ) || +of_device_is_compatible (dev->dev.of_node, +"aspeed,ast2700-ehci" )) ehci->is_aspeed =1 ;if (soc_device_match (quirk_poll_match))@@ -485,6 +495,7 @@ static const struct of_device_id vt8500_ehci_ids[] = { { .compatible = "wm,prizm-ehci", }, { .compatible = "generic-ehci", }, { .compatible = "cavium,octeon-6335-ehci", }, + { .compatible = "aspeed,ast2700-ehci", .data = (void *)1 }, {} }; MODULE_DEVICE_TABLE(of, vt8500_ehci_ids);Signature · 1 line
2.34.1
RE: [PATCH v3 2/2] usb: ehci: Add Aspeed AST2700 support
Quoted 48 lines
Subject: Re: [PATCH v3 2/2] usb: ehci: Add Aspeed AST2700 support
Quoted 8 lines · Ryan Chen
Unlike earlier Aspeed SoCs (AST2400/2500/2600) which are limited to 32-bit DMA addressing, the EHCI controller in AST2700 supports 64-bit DMA. Update the EHCI platform driver to make use of this capability by selecting a 64-bit DMA mask when the "aspeed,ast2700-ehci" compatible is present in device tree.
Signed-off-by Ryan Chen <[email protected]>---
This is basically good and it can be merged. However...
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct
Quoted 17 lines
drivers/usb/host/ehci-platform.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)drivers/usb/host/ehci-platform.c +1
-0@@ -27,6 +27,7 @@#include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/reset.h> #include <linux/sys_soc.h> @@ -239,9 +240,11 @@ static int ehci_platform_probe(struct
platform_device*dev)drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct
Quoted 12 lines
drivers/usb/host/ehci-platform.c +2
-0struct usb_hcd *hcd;struct resource *res_mem;struct usb_ehci_pdata *pdata =dev_get_platdata (&dev->dev); +const struct of_device_id *match;struct ehci_platform_priv *priv;struct ehci_hcd *ehci;int err, irq, clk =0 ; +bool dma_mask_64;if (usb_disabled ())return -ENODEV ;@@ -253,8 +256,13 @@ static int ehci_platform_probe(struct
platform_device*dev)Quoted 3 lines
drivers/usb/host/ehci-platform.c +0
-0if (!pdata) pdata = &ehci_platform_defaults;
Quoted 8 lines
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 260
Quoted 2 lines
drivers/usb/host/ehci-platform.c +2
-0+ dma_mask_64 = pdata->dma_mask_64; + match =of_match_device (dev->dev.driver->of_match_table, &dev->dev);(I just noticed this.) The "
dev->dev.driver->of_match_table" part looks odd. Why not just write "vt8500_ehci_ids"? Do you expect that this could ever have a different value?Alan Stern
Thanks your feedback.
I used dev->dev.driver->of_match_table rather than hard-coding vt8500_ehci_ids to keep the probe code generic and tied to the driver model, not to a specific symbol. Functionally it's the same here, but this pattern avoids coupling the probe to a particular table name.
How you think ?
Ryan.
Quoted 33 lines
drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct
Quoted 11 lines
drivers/usb/host/ehci-platform.c +4
-1+if (match && match->data) + dma_mask_64 = true; + err =dma_coerce_mask_and_coherent (&dev->dev, - pdata->dma_mask_64 ?DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 )); + dma_mask_64 ?DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 ));if (err) {dev_err (&dev->dev,"Error: DMA mask configuration failed\n" );return err;@@ -298,7 +306,9 @@ static int ehci_platform_probe(struct
platform_device*dev)Quoted 21 lines
drivers/usb/host/ehci-platform.c +4
-2if (of_device_is_compatible (dev->dev.of_node,"aspeed,ast2500-ehci" ) ||of_device_is_compatible (dev->dev.of_node, -"aspeed,ast2600-ehci" )) +"aspeed,ast2600-ehci" ) || +of_device_is_compatible (dev->dev.of_node, +"aspeed,ast2700-ehci" )) ehci->is_aspeed =1 ;if (soc_device_match (quirk_poll_match))@@ -485,6 +495,7 @@ static const struct of_device_id vt8500_ehci_ids[] = { { .compatible = "wm,prizm-ehci", }, { .compatible = "generic-ehci", }, { .compatible = "cavium,octeon-6335-ehci", }, + { .compatible = "aspeed,ast2700-ehci", .data = (void *)1 }, {} }; MODULE_DEVICE_TABLE(of, vt8500_ehci_ids); --2.34.1
Re: [PATCH v3 2/2] usb: ehci: Add Aspeed AST2700 support
Quoted 12 lines · Ryan Chen
Quoted 12 lines
Review static int ehci_platform_probe(struct
Quoted 1 line
(quoted diff) +0
-0@@ -253,8 +256,13 @@ static int ehci_platform_probe(struct
platform_device*dev)drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 260
Quoted 5 lines
drivers/usb/host/ehci-platform.c +2
-0if (!pdata) pdata = &ehci_platform_defaults; + dma_mask_64 = pdata->dma_mask_64; + match =of_match_device (dev->dev.driver->of_match_table, &dev->dev);(I just noticed this.) The "
dev->dev.driver->of_match_table" part looks odd. Why not just write "vt8500_ehci_ids"? Do you expect that this could ever have a different value?
Quoted 8 lines
Quoted 1 line
Alan Stern
Thanks your feedback.
I useddev->dev.driver->of_match_tablerather than hard-codingvt8500_ehci_idsto keep the probe code generic and tied to the driver model, not to a specific symbol. Functionally it's the same here, but this pattern avoids coupling the probe to a particular table name.How you think ?
The code doesn't need to be any more generic than the source file it containing it. This particular probe function will never be called for a different driver, or a device that matches an OF table different from vt8500_ehci_ids, right?
Interestingly, there are two other places in drivers/usb/host/ that call of_match_device(). fsl-mph-dr-of.c uses my approach and xhci-plat.c uses yours. The difference is that the xhci-platform probe routine _is_ called by other drivers, namely, by xhci-rcar.c.
I guess it's okay to keep this code as written, even though it's not strictly necessary
Alan Stern
RE: [PATCH v3 2/2] usb: ehci: Add Aspeed AST2700 support
Quoted 25 lines
Subject: Re: [PATCH v3 2/2] usb: ehci: Add Aspeed AST2700 support
Quoted 10 lines · Ryan Chen
Quoted 10 lines
Review static int ehci_platform_probe(struct
Quoted 1 line
(quoted diff) +0
-0@@ -253,8 +256,13 @@ static int ehci_platform_probe(struct
platform_device*dev)drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev)
Quoted 6 lines
drivers/usb/host/ehci-platform.c +3
-0if (!pdata) pdata = &ehci_platform_defaults; + dma_mask_64 = pdata->dma_mask_64; + match =of_match_device (dev->dev.driver->of_match_table, +&dev->dev);(I just noticed this.) The "
dev->dev.driver->of_match_table" part looksodd.
Quoted 7 lines
Quoted 4 lines
Why not just write "
vt8500_ehci_ids"? Do you expect that this could ever have a different value?Alan Stern
Thanks your feedback.
I useddev->dev.driver->of_match_tablerather than hard-codingvt8500_ehci_idsto keep the probe code generic and tied to the driver model,not to a specific symbol.
Quoted 3 lines
Functionally it's the same here, but this pattern avoids coupling the probe to a particular table name.
Quoted 6 lines
Quoted 1 line
How you think ?
The code doesn't need to be any more generic than the source file it containing it. This particular probe function will never be called for a different driver, or a device that matches an OF table different from
vt8500_ehci_ids, right?
Understood, I want to keep get from of_match_table. Appreciate your review. Thank a lot.
Quoted 12 lines
Interestingly, there are two other places in
drivers/usb/host/that callof_match_device().fsl-mph-dr-of.cuses my approach andxhci-plat.cuses yours. The difference is that the xhci-platform probe routine _is_ called by other drivers, namely, byxhci-rcar.c.I guess it's okay to keep this code as written, even though it's not strictly necessary
Reviewed-by Alan Stern <[email protected]>Alan Stern