From: Nicolas Ferre <nicolas.ferre@microchip.com> To: Alexander Dahl <ada@thorsis.com>, Alan Stern <stern@rowland.harvard.edu>, <linux-usb@vger.kernel.org>, Cristian Birsan <Cristian.Birsan@microchip.com> Cc: <linux-arm-kernel@lists.infradead.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Ludovic Desroches <ludovic.desroches@microchip.com>, <linux-kernel@vger.kernel.org> Subject: Re: [RFC PATCH] USB: host: ehci-atmel: Allow enabling HSIC on SAMA5D2 Date: Thu, 2 Sep 2021 17:33:50 +0200 [thread overview] Message-ID: <7c3d1248-b708-68f9-a76a-712e345b8218@microchip.com> (raw) In-Reply-To: <20210824063702.24586-1-ada@thorsis.com> 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@thorsis.com> Sorry for not having coming back to you earlier, summertime... 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/ca368f544899c14b03df9ce7510684f03acf1bf9 It allows us to have a gigabit Ethernet HSIC connected on our sama5d2 ICP board. It works well for some time. 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-sama5d2_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 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. Hope that it helps. Best regards, Nicolas > --- > > 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@thorsis.com> > + */ > +#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 > -- Nicolas Ferre
WARNING: multiple messages have this Message-ID (diff)
From: Nicolas Ferre <nicolas.ferre@microchip.com> To: Alexander Dahl <ada@thorsis.com>, Alan Stern <stern@rowland.harvard.edu>, <linux-usb@vger.kernel.org>, Cristian Birsan <Cristian.Birsan@microchip.com> Cc: <linux-arm-kernel@lists.infradead.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Alexandre Belloni <alexandre.belloni@bootlin.com>, Ludovic Desroches <ludovic.desroches@microchip.com>, <linux-kernel@vger.kernel.org> Subject: Re: [RFC PATCH] USB: host: ehci-atmel: Allow enabling HSIC on SAMA5D2 Date: Thu, 2 Sep 2021 17:33:50 +0200 [thread overview] Message-ID: <7c3d1248-b708-68f9-a76a-712e345b8218@microchip.com> (raw) In-Reply-To: <20210824063702.24586-1-ada@thorsis.com> 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@thorsis.com> Sorry for not having coming back to you earlier, summertime... 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/ca368f544899c14b03df9ce7510684f03acf1bf9 It allows us to have a gigabit Ethernet HSIC connected on our sama5d2 ICP board. It works well for some time. 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-sama5d2_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 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. Hope that it helps. Best regards, Nicolas > --- > > 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@thorsis.com> > + */ > +#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 > -- Nicolas Ferre _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-09-02 15:33 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-23 9:24 Enable HSIC Host Port on Atmel/Microchip SAMA5D2 Alexander Dahl 2021-08-23 9:24 ` Alexander Dahl 2021-08-23 14:00 ` Alan Stern 2021-08-23 14:00 ` Alan Stern 2021-08-24 6:37 ` [RFC PATCH] USB: host: ehci-atmel: Allow enabling HSIC on SAMA5D2 Alexander Dahl 2021-08-24 6:37 ` Alexander Dahl 2021-08-24 6:45 ` Greg Kroah-Hartman 2021-08-24 6:45 ` Greg Kroah-Hartman 2021-09-02 15:33 ` Nicolas Ferre [this message] 2021-09-02 15:33 ` Nicolas Ferre 2021-09-06 7:20 ` Alexander Dahl 2021-09-06 7:20 ` Alexander Dahl
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7c3d1248-b708-68f9-a76a-712e345b8218@microchip.com \ --to=nicolas.ferre@microchip.com \ --cc=Cristian.Birsan@microchip.com \ --cc=ada@thorsis.com \ --cc=alexandre.belloni@bootlin.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=ludovic.desroches@microchip.com \ --cc=stern@rowland.harvard.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.