Hello Nicolas, Am Donnerstag, 2. September 2021, 17:33:50 CEST schrieb Nicolas Ferre: > Hi Alexander, > > On 24/08/2021 at 08:37, Alexander Dahl wrote: > > Unlike other SoC series featuring the 'atmel,at91sam9g45-ehci' USB EHCI > > controller, which have embedded USB high-speed transceivers for each > > port, the third port on the SAMA5D2 series is HSIC only. That HSIC > > interface is not enabled after a power-on reset, but can be enabled by > > setting a flag in a vendor specific EHCI register. > > > > The register offsets added to the new header file were compared with > > those for the SAM9G45, SAM9X25, SAMA5D3, SAMA5D4, and SAM9X60 series and > > there are no differences in the offsets or contents of those registers. > > Which of those additional vendor specific registers are supported, > > differs by SoC family. So while the HSIC enable feature is currently > > only present for SAMA5D2, it probably does not hurt to set it on the > > other families, hence no additional check for SoC family here. > > > > Tested on a custom board featuring a SAMA5D27C-D5M SiP connected to an > > USB3503 hub with an upstream HSIC interface. > > > > Link: https://community.atmel.com/forum/sama5d2-using-hsic-under-linux > > Signed-off-by: Alexander Dahl <ada@xxxxxxxxxxx> > > Sorry for not having coming back to you earlier, summertime... I had one week off last week due to a mild infection myself, so we just proceed here and now. (-: > What you are looking for is what Cristian developed in our "vendor tree" > and that needs to be "Mainlined": > https://github.com/linux4sam/linux-at91/commit/ca368f544899c14b03df9ce751068 > 4f03acf1bf9 Looks like it does what it should from quick code inspection. One could nitpick some things, maybe I add some comments on GitHub. > It allows us to have a gigabit Ethernet HSIC connected on our sama5d2 > ICP board. It works well for some time. Good to hear. > For DT, we rely on the standard "phy_type" property set to "hsic" as > highlighted in this DT node on the ICP board precisely: > https://github.com/linux4sam/linux-at91/blob/master/arch/arm/boot/dts/at91-s > ama5d2_icp.dts#L766 > > This way we can use the of_usb_get_phy_mode() standard function: > https://github.com/linux4sam/linux-at91/blob/master/drivers/usb/phy/of.c#L28 I noticed that phy_type property, but did not follow that approach, because that USB block in SAMA5D2 has three ports, where one (A) is shared with a device port, two (A and B) have embedded transceivers, and only the third (C) has that HSIC interface, but nothing else. So the flag has no effect on port A and B anyways, and I would have found it misleading to set phy_type to HSIC for the whole USB block. > All this tells me that I would prefer Cristi's approach. If agreed, > we'll make sure to make progress on the mainlining part soon. I don't mind. If that's your preferred approach, I will happily test it. Was the series already posted to upstream? > Hope that it helps. Best regards, > Nicolas Yes, indeed. Thanks for your feedback. Greets Alex > > > --- > > > > Notes: > > - for introducing new dt binding, would be nice to convert old one > > > > first, probably needs split up and multiple iteration review? > > > > - name of that new dt property? > > - register definitions put to a separate file, like > > > > 'drivers/usb/host/ehci-fsl.h' > > > > - unsure where exactly in the probe process that register write > > should > > > > happen, datasheet gives no hint > > > > - should suspend/resume be considered? > > > > drivers/usb/host/ehci-atmel.c | 17 +++++++++++++++++ > > drivers/usb/host/ehci-atmel.h | 19 +++++++++++++++++++ > > 2 files changed, 36 insertions(+) > > create mode 100644 drivers/usb/host/ehci-atmel.h > > > > diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c > > index e893467d659c..f8d9e686c082 100644 > > --- a/drivers/usb/host/ehci-atmel.c > > +++ b/drivers/usb/host/ehci-atmel.c > > @@ -20,6 +20,7 @@ > > > > #include <linux/usb/hcd.h> > > > > #include "ehci.h" > > > > +#include "ehci-atmel.h" > > > > #define DRIVER_DESC "EHCI Atmel driver" > > > > @@ -85,6 +86,7 @@ static void atmel_stop_ehci(struct platform_device > > *pdev) > > > > static int ehci_atmel_drv_probe(struct platform_device *pdev) > > { > > > > + struct device_node *np = pdev->dev.of_node; > > > > struct usb_hcd *hcd; > > const struct hc_driver *driver = &ehci_atmel_hc_driver; > > struct resource *res; > > > > @@ -149,6 +151,14 @@ static int ehci_atmel_drv_probe(struct > > platform_device *pdev)> > > atmel_start_ehci(pdev); > > > > + if (of_property_read_bool(np, "atmel,enable-hsic")) { > > + u32 tmp; > > + > > + tmp = ehci_readl(ehci, hcd->regs + AT91_UHPHS_INSNREG08); > > + tmp |= AT91_UHPHS_HSIC_EN; > > + ehci_writel(ehci, tmp, hcd->regs + AT91_UHPHS_INSNREG08); > > + } > > + > > > > retval = usb_add_hcd(hcd, irq, IRQF_SHARED); > > if (retval) > > > > goto fail_add_hcd; > > > > @@ -170,10 +180,17 @@ static int ehci_atmel_drv_probe(struct > > platform_device *pdev)> > > static int ehci_atmel_drv_remove(struct platform_device *pdev) > > { > > > > struct usb_hcd *hcd = platform_get_drvdata(pdev); > > > > + struct ehci_hcd *ehci; > > + u32 tmp; > > > > usb_remove_hcd(hcd); > > usb_put_hcd(hcd); > > > > + ehci = hcd_to_ehci(hcd); > > + tmp = ehci_readl(ehci, hcd->regs + AT91_UHPHS_INSNREG08); > > + tmp &= ~AT91_UHPHS_HSIC_EN; > > + ehci_writel(ehci, tmp, hcd->regs + AT91_UHPHS_INSNREG08); > > + > > > > atmel_stop_ehci(pdev); > > > > return 0; > > > > diff --git a/drivers/usb/host/ehci-atmel.h b/drivers/usb/host/ehci-atmel.h > > new file mode 100644 > > index 000000000000..4c4998c2a6dd > > --- /dev/null > > +++ b/drivers/usb/host/ehci-atmel.h > > @@ -0,0 +1,19 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Vendor specific definitions for EHCI on Atmel/Microchip SoCs. > > + * > > + * © 2021 Alexander Dahl <ada@xxxxxxxxxxx> > > + */ > > +#ifndef EHCI_ATMEL_H > > +#define EHCI_ATMEL_H > > + > > +/* device specific register offsets, taken from SAMA5D2 datasheet */ > > + > > +#define AT91_UHPHS_INSNREG06 0xA8 /* AHB Error Status Register > > */ + > > +#define AT91_UHPHS_INSNREG07 0xAC /* AHB Master Error Address > > Register */ + > > +#define AT91_UHPHS_INSNREG08 0xB0 /* HSIC Enable/Disable > > Register */ +#define AT91_UHPHS_HSIC_EN (1 << 2) /* HSIC > > Enable/Disable */ + > > +#endif /* ECHI_ATMEL_H */ > > > > base-commit: e22ce8eb631bdc47a4a4ea7ecf4e4ba499db4f93 > > -- > > 2.30.2 --
