From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BBA10C433F5 for ; Mon, 6 Sep 2021 07:20:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9E2F460F21 for ; Mon, 6 Sep 2021 07:20:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240058AbhIFHVz (ORCPT ); Mon, 6 Sep 2021 03:21:55 -0400 Received: from mail.thorsis.com ([92.198.35.195]:51298 "EHLO mail.thorsis.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233040AbhIFHVx (ORCPT ); Mon, 6 Sep 2021 03:21:53 -0400 Received: from localhost (localhost [127.0.0.1]) by mail.thorsis.com (Postfix) with ESMTP id EB34D1C60; Mon, 6 Sep 2021 09:20:47 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mail.thorsis.com Received: from mail.thorsis.com ([127.0.0.1]) by localhost (mail.thorsis.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Na__r9QRpKGM; Mon, 6 Sep 2021 09:20:47 +0200 (CEST) Received: by mail.thorsis.com (Postfix, from userid 109) id 4FF701C69; Mon, 6 Sep 2021 09:20:45 +0200 (CEST) From: Alexander Dahl To: Nicolas Ferre Cc: Alan Stern , linux-usb@vger.kernel.org, Cristian Birsan , linux-arm-kernel@lists.infradead.org, Greg Kroah-Hartman , Alexandre Belloni , Ludovic Desroches , linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH] USB: host: ehci-atmel: Allow enabling HSIC on SAMA5D2 Date: Mon, 06 Sep 2021 09:20:38 +0200 Message-ID: <2753502.AcZM6cElzO@ada> In-Reply-To: <7c3d1248-b708-68f9-a76a-712e345b8218@microchip.com> References: <20210823140052.GA120849@rowland.harvard.edu> <20210824063702.24586-1-ada@thorsis.com> <7c3d1248-b708-68f9-a76a-712e345b8218@microchip.com> Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Nicolas, Am Donnerstag, 2. September 2021, 17:33:50 CEST schrieb Nicolas Ferre: > Hi Alexander, >=20 > 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. > >=20 > > 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. > >=20 > > Tested on a custom board featuring a SAMA5D27C-D5M SiP connected to an > > USB3503 hub with an upstream HSIC interface. > >=20 > > Link: https://community.atmel.com/forum/sama5d2-using-hsic-under-linux > > Signed-off-by: Alexander Dahl >=20 > 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=20 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/ca368f544899c14b03df9ce751= 068 > 4f03acf1bf9 Looks like it does what it should from quick code inspection. One could=20 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/at9= 1-s > ama5d2_icp.dts#L766 >=20 > 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= =20 that USB block in SAMA5D2 has three ports, where one (A) is shared with a=20 device port, two (A and B) have embedded transceivers, and only the third (= C)=20 has that HSIC interface, but nothing else. So the flag has no effect on por= t A=20 and B anyways, and I would have found it misleading to set phy_type to HSIC= =20 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. Wa= s=20 the series already posted to upstream? > Hope that it helps. Best regards, > Nicolas Yes, indeed. Thanks for your feedback. Greets Alex >=20 > > --- > >=20 > > Notes: > > - for introducing new dt binding, would be nice to convert old one > > =20 > > first, probably needs split up and multiple iteration review? > > =20 > > - name of that new dt property? > > - register definitions put to a separate file, like > > =20 > > 'drivers/usb/host/ehci-fsl.h' > > =20 > > - unsure where exactly in the probe process that register write > > should > > =20 > > happen, datasheet gives no hint > > =20 > > - should suspend/resume be considered? > > =20 > > 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 > >=20 > > diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atme= l.c > > index e893467d659c..f8d9e686c082 100644 > > --- a/drivers/usb/host/ehci-atmel.c > > +++ b/drivers/usb/host/ehci-atmel.c > > @@ -20,6 +20,7 @@ > >=20 > > #include > > =20 > > #include "ehci.h" > >=20 > > +#include "ehci-atmel.h" > >=20 > > #define DRIVER_DESC "EHCI Atmel driver" > >=20 > > @@ -85,6 +86,7 @@ static void atmel_stop_ehci(struct platform_device > > *pdev) > >=20 > > static int ehci_atmel_drv_probe(struct platform_device *pdev) > > { > >=20 > > + struct device_node *np =3D pdev->dev.of_node; > >=20 > > struct usb_hcd *hcd; > > const struct hc_driver *driver =3D &ehci_atmel_hc_driver; > > struct resource *res; > >=20 > > @@ -149,6 +151,14 @@ static int ehci_atmel_drv_probe(struct > > platform_device *pdev)>=20 > > atmel_start_ehci(pdev); > >=20 > > + if (of_property_read_bool(np, "atmel,enable-hsic")) { > > + u32 tmp; > > + > > + tmp =3D ehci_readl(ehci, hcd->regs + AT91_UHPHS_INSNREG= 08); > > + tmp |=3D AT91_UHPHS_HSIC_EN; > > + ehci_writel(ehci, tmp, hcd->regs + AT91_UHPHS_INSNREG08= ); > > + } > > + > >=20 > > retval =3D usb_add_hcd(hcd, irq, IRQF_SHARED); > > if (retval) > > =20 > > goto fail_add_hcd; > >=20 > > @@ -170,10 +180,17 @@ static int ehci_atmel_drv_probe(struct > > platform_device *pdev)>=20 > > static int ehci_atmel_drv_remove(struct platform_device *pdev) > > { > > =20 > > struct usb_hcd *hcd =3D platform_get_drvdata(pdev); > >=20 > > + struct ehci_hcd *ehci; > > + u32 tmp; > >=20 > > usb_remove_hcd(hcd); > > usb_put_hcd(hcd); > >=20 > > + ehci =3D hcd_to_ehci(hcd); > > + tmp =3D ehci_readl(ehci, hcd->regs + AT91_UHPHS_INSNREG08); > > + tmp &=3D ~AT91_UHPHS_HSIC_EN; > > + ehci_writel(ehci, tmp, hcd->regs + AT91_UHPHS_INSNREG08); > > + > >=20 > > atmel_stop_ehci(pdev); > > =20 > > return 0; > >=20 > > diff --git a/drivers/usb/host/ehci-atmel.h b/drivers/usb/host/ehci-atme= l.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. > > + * > > + * =A9 2021 Alexander Dahl > > + */ > > +#ifndef EHCI_ATMEL_H > > +#define EHCI_ATMEL_H > > + > > +/* device specific register offsets, taken from SAMA5D2 datasheet */ > > + > > +#define AT91_UHPHS_INSNREG06 0xA8 /* AHB Error Status Regist= er > > */ + > > +#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 */ > >=20 > > base-commit: e22ce8eb631bdc47a4a4ea7ecf4e4ba499db4f93 > > -- > > 2.30.2 =2D-=20 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-19.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46E2DC433EF for ; Mon, 6 Sep 2021 07:27:31 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 141B9606A5 for ; Mon, 6 Sep 2021 07:27:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 141B9606A5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=thorsis.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:MIME-Version:List-Subscribe:List-Help: List-Post:List-Archive:List-Unsubscribe:List-Id:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Dn08GyhZsre6rRxI8m7NhtmFNUoYFR6fwHSIi9w4t7U=; b=vXfjGIxZbNP91L uJCHS8LDAWIAPPp5nU+7+5ISfMpX8golp9yhCaW0o6dJ7omAEDJwMHNgIF7/dZSAUZvx1+iuYFaBI +JaDagv43tQL1kuEdNRcskNhkJHS/QGU956+3bXzkpyeb7KUQDhEOKJvQfQPXwseigR4CAUnqPzpf MWOzhgSTuFcPIYAHpfcnkTtyndmTzQi6w4l4Tgp/D57Dt44eOUqFwlP+smg+8FWEFfBewSMRfZQN4 iGfUmbXBc1RDnzW9pcor2im51Pajzt3343XA3Zemypy2NFI0fa4/E2KaMD9aXRVRp7fvEq/I6m5si nhYxrX1CpNTQheYki5zQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mN907-0000pb-2k; Mon, 06 Sep 2021 07:25:07 +0000 Received: from mail.thorsis.com ([92.198.35.195]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mN8w1-00HZzM-Bo for linux-arm-kernel@lists.infradead.org; Mon, 06 Sep 2021 07:20:55 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.thorsis.com (Postfix) with ESMTP id DBFCC1AB6 for ; Mon, 6 Sep 2021 09:20:47 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mail.thorsis.com Received: from mail.thorsis.com ([127.0.0.1]) by localhost (mail.thorsis.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id srGnNDewFi8Q for ; Mon, 6 Sep 2021 09:20:47 +0200 (CEST) Received: by mail.thorsis.com (Postfix, from userid 109) id 32BAF1D93; Mon, 6 Sep 2021 09:20:45 +0200 (CEST) From: Alexander Dahl To: Nicolas Ferre Cc: Alan Stern , linux-usb@vger.kernel.org, Cristian Birsan , linux-arm-kernel@lists.infradead.org, Greg Kroah-Hartman , Alexandre Belloni , Ludovic Desroches , linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH] USB: host: ehci-atmel: Allow enabling HSIC on SAMA5D2 Date: Mon, 06 Sep 2021 09:20:38 +0200 Message-ID: <2753502.AcZM6cElzO@ada> In-Reply-To: <7c3d1248-b708-68f9-a76a-712e345b8218@microchip.com> References: <20210823140052.GA120849@rowland.harvard.edu> <20210824063702.24586-1-ada@thorsis.com> <7c3d1248-b708-68f9-a76a-712e345b8218@microchip.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210906_002053_771307_BEF34A6B X-CRM114-Status: GOOD ( 48.97 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > = > 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/ca368f544899c14b03df9ce751= 068 > 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/at9= 1-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 por= t 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. Wa= s = 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-atme= l.c > > index e893467d659c..f8d9e686c082 100644 > > --- a/drivers/usb/host/ehci-atmel.c > > +++ b/drivers/usb/host/ehci-atmel.c > > @@ -20,6 +20,7 @@ > > = > > #include > > = > > #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 =3D pdev->dev.of_node; > > = > > struct usb_hcd *hcd; > > const struct hc_driver *driver =3D &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 =3D ehci_readl(ehci, hcd->regs + AT91_UHPHS_INSNREG= 08); > > + tmp |=3D AT91_UHPHS_HSIC_EN; > > + ehci_writel(ehci, tmp, hcd->regs + AT91_UHPHS_INSNREG08= ); > > + } > > + > > = > > retval =3D 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 =3D platform_get_drvdata(pdev); > > = > > + struct ehci_hcd *ehci; > > + u32 tmp; > > = > > usb_remove_hcd(hcd); > > usb_put_hcd(hcd); > > = > > + ehci =3D hcd_to_ehci(hcd); > > + tmp =3D ehci_readl(ehci, hcd->regs + AT91_UHPHS_INSNREG08); > > + tmp &=3D ~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-atme= l.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. > > + * > > + * =A9 2021 Alexander Dahl > > + */ > > +#ifndef EHCI_ATMEL_H > > +#define EHCI_ATMEL_H > > + > > +/* device specific register offsets, taken from SAMA5D2 datasheet */ > > + > > +#define AT91_UHPHS_INSNREG06 0xA8 /* AHB Error Status Regist= er > > */ + > > +#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 -- = _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel