Message ID | 20230707064725.25291-1-stanley_chang@realtek.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp3068180vqx; Thu, 6 Jul 2023 23:51:11 -0700 (PDT) X-Google-Smtp-Source: APBJJlE5wsUmfyvAZGNuVf80mfaDi43IOsH5UsOGWaZbsx7oJRJJF5IfDNVAGUiCpppW3SVEFSPB X-Received: by 2002:a05:6358:88b:b0:132:db25:bbfc with SMTP id m11-20020a056358088b00b00132db25bbfcmr5867102rwj.2.1688712670703; Thu, 06 Jul 2023 23:51:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688712670; cv=none; d=google.com; s=arc-20160816; b=XQTv7L5jhSOItuhQy/dZQW4zg/Bsi9sLNRUB90mLfL+X39GIUSLpAu+fFim/y7Ers6 fwbcIz52aizfnTIRFXpgSYXw836Kovb9z38xzMnX095wSrZLUPF3AaB2JObfLJzDGmPX 1jCagAZI/WnrW4QJPdR1lLkAfx+88uc+RGcAGljbzd6G7I0IEw7YVWIx6e3t0Xa4wdd1 qp02ij8onS52BnDJQDfHKPAS08m8ZXKAIxtIdz3D+wzrhQSuKI343OnhBuicxH0zLrRJ ohlPa6OIoRsp14f77s4LMLgWHVbsF0SOueu5xIZMZqAtvzw8e1J8ltFQzVelvIObJ9qb R3Sw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:authenticated-by; bh=7G2MgPXQoA/5JUZtUiO0EH6NM/TDwpUrrUmXPazkiEM=; fh=5Mg1G406BO0svV8B4/Ps9atvv5it2Qnsr4chJtNuGBg=; b=w3ymjK4eFFvTH10kGJYJ5ww+qPZDoRmwWeQeA4sVjvuX1dtRFaoXEREQJjgGcuSXf0 1y73yYLeW77loCErFZTvjSAVNDeQV6Ev0L6Nxw4Yx5FjVjKgkQ6uL9uvXRc3e+qxu3LX 3TCDp0eifJVpSYhHia6gVI2jkIhMKP6oTrgSb+pu30A7o4fHjmxcjo3zBau/agPyYe6d RhOv+rdEpsle9WDGiA3IBYt+YnquISGxydPfSu/A36m1iM+NA7rEN4WvFEeC1DFxDxml 4RJaS0yC7gavXST0Wxbd49L8Pw3wg3Aa+uMFVYGTpcZjOEk4xengGNRRWwwkzYyfLgBH f0HA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id w189-20020a6382c6000000b0055b49c1519asi3281706pgd.143.2023.07.06.23.50.56; Thu, 06 Jul 2023 23:51:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231334AbjGGGs0 (ORCPT <rfc822;daweilics@gmail.com> + 99 others); Fri, 7 Jul 2023 02:48:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42644 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229446AbjGGGsZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 7 Jul 2023 02:48:25 -0400 Received: from rtits2.realtek.com.tw (rtits2.realtek.com [211.75.126.72]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 9BCDB1FD8; Thu, 6 Jul 2023 23:48:21 -0700 (PDT) Authenticated-By: X-SpamFilter-By: ArmorX SpamTrap 5.77 with qID 3676lMI11007817, This message is accepted by code: ctloc85258 Received: from mail.realtek.com (rtexh36506.realtek.com.tw[172.21.6.27]) by rtits2.realtek.com.tw (8.15.2/2.81/5.90) with ESMTPS id 3676lMI11007817 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=FAIL); Fri, 7 Jul 2023 14:47:22 +0800 Received: from RTEXMBS01.realtek.com.tw (172.21.6.94) by RTEXH36506.realtek.com.tw (172.21.6.27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.17; Fri, 7 Jul 2023 14:47:26 +0800 Received: from RTEXH36505.realtek.com.tw (172.21.6.25) by RTEXMBS01.realtek.com.tw (172.21.6.94) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.7; Fri, 7 Jul 2023 14:47:25 +0800 Received: from localhost.localdomain (172.21.252.101) by RTEXH36505.realtek.com.tw (172.21.6.25) with Microsoft SMTP Server id 15.1.2375.32 via Frontend Transport; Fri, 7 Jul 2023 14:47:25 +0800 From: Stanley Chang <stanley_chang@realtek.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> CC: Stanley Chang <stanley_chang@realtek.com>, Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Alan Stern <stern@rowland.harvard.edu>, Roy Luo <royluo@google.com>, Matthias Kaehlcke <mka@chromium.org>, Douglas Anderson <dianders@chromium.org>, Flavio Suligoi <f.suligoi@asem.it>, Ray Chi <raychi@google.com>, <linux-phy@lists.infradead.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org> Subject: [PATCH v7 1/5] usb: phy: add usb phy notify port status API Date: Fri, 7 Jul 2023 14:47:00 +0800 Message-ID: <20230707064725.25291-1-stanley_chang@realtek.com> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-KSE-ServerInfo: RTEXMBS01.realtek.com.tw, 9 X-KSE-AntiSpam-Interceptor-Info: fallback X-KSE-Antivirus-Interceptor-Info: fallback X-KSE-AntiSpam-Interceptor-Info: fallback X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1770743577321200959?= X-GMAIL-MSGID: =?utf-8?q?1770743577321200959?= |
Series |
[v7,1/5] usb: phy: add usb phy notify port status API
|
|
Commit Message
Stanley Chang[昌育德]
July 7, 2023, 6:47 a.m. UTC
In Realtek SoC, the parameter of usb phy is designed to can dynamic
tuning base on port status. Therefore, add a notify callback of phy
driver when usb port status change.
The Realtek phy driver is designed to dynamically adjust disconnection
level and calibrate phy parameters. When the device connected bit changes
and when the disconnected bit changes, do port status change notification:
Check if portstatus is USB_PORT_STAT_CONNECTION and portchange is
USB_PORT_STAT_C_CONNECTION.
1. The device is connected, the driver lowers the disconnection level and
calibrates the phy parameters.
2. The device disconnects, the driver increases the disconnect level and
calibrates the phy parameters.
When controller to notify connect that device is already ready. If we
adjust the disconnection level in notify_connect, the disconnect may have
been triggered at this stage. So we need to change that as early as
possible. Therefore, we add an api to notify phy the port status changes.
Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
---
v6 to v7 change:
No change
v5 to v6 change:
No change
v4 to v5 change:
No change
v3 to v4 change:
Fix the warning for checkpatch with strict.
v2 to v3 change:
Add more comments about the reason for adding this api
v1 to v2 change:
No change
---
drivers/usb/core/hub.c | 13 +++++++++++++
include/linux/usb/phy.h | 13 +++++++++++++
2 files changed, 26 insertions(+)
Comments
On Fri, Jul 07, 2023 at 02:47:00PM +0800, Stanley Chang wrote: > In Realtek SoC, the parameter of usb phy is designed to can dynamic > tuning base on port status. Therefore, add a notify callback of phy > driver when usb port status change. > > The Realtek phy driver is designed to dynamically adjust disconnection > level and calibrate phy parameters. When the device connected bit changes > and when the disconnected bit changes, do port status change notification: > > Check if portstatus is USB_PORT_STAT_CONNECTION and portchange is > USB_PORT_STAT_C_CONNECTION. > 1. The device is connected, the driver lowers the disconnection level and > calibrates the phy parameters. > 2. The device disconnects, the driver increases the disconnect level and > calibrates the phy parameters. > > When controller to notify connect that device is already ready. If we > adjust the disconnection level in notify_connect, the disconnect may have > been triggered at this stage. So we need to change that as early as > possible. Therefore, we add an api to notify phy the port status changes. How do you know that the disconnect will not have already been triggered at this point, when the status changes? > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com> > --- > v6 to v7 change: > No change > v5 to v6 change: > No change > v4 to v5 change: > No change > v3 to v4 change: > Fix the warning for checkpatch with strict. > v2 to v3 change: > Add more comments about the reason for adding this api > v1 to v2 change: > No change > --- > drivers/usb/core/hub.c | 13 +++++++++++++ > include/linux/usb/phy.h | 13 +++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index a739403a9e45..8433ff89dea6 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, int port1, int type, > ret = 0; > } > mutex_unlock(&hub->status_mutex); > + > + if (!ret) { > + struct usb_device *hdev = hub->hdev; > + > + if (hdev && !hdev->parent) { Why the check for no parent? Please document that here in a comment. > + struct usb_hcd *hcd = bus_to_hcd(hdev->bus); > + > + if (hcd->usb_phy) > + usb_phy_notify_port_status(hcd->usb_phy, > + port1 - 1, *status, *change); > + } > + } > + This is safe to notify with the hub mutex unlocked? Again, a comment would be helpful to future people explaining why that is so. thanks, greg k-h
> > On Fri, Jul 07, 2023 at 02:47:00PM +0800, Stanley Chang wrote: > > In Realtek SoC, the parameter of usb phy is designed to can dynamic > > tuning base on port status. Therefore, add a notify callback of phy > > driver when usb port status change. > > > > The Realtek phy driver is designed to dynamically adjust disconnection > > level and calibrate phy parameters. When the device connected bit > > changes and when the disconnected bit changes, do port status change > notification: > > > > Check if portstatus is USB_PORT_STAT_CONNECTION and portchange is > > USB_PORT_STAT_C_CONNECTION. > > 1. The device is connected, the driver lowers the disconnection level and > > calibrates the phy parameters. > > 2. The device disconnects, the driver increases the disconnect level and > > calibrates the phy parameters. > > > > When controller to notify connect that device is already ready. If we > > adjust the disconnection level in notify_connect, the disconnect may > > have been triggered at this stage. So we need to change that as early > > as possible. Therefore, we add an api to notify phy the port status changes. > > How do you know that the disconnect will not have already been triggered at > this point, when the status changes? The status change of connection is before port reset. In this stage, the device is not port enable, and it will not trigger disconnection. > > > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com> > > --- > > v6 to v7 change: > > No change > > v5 to v6 change: > > No change > > v4 to v5 change: > > No change > > v3 to v4 change: > > Fix the warning for checkpatch with strict. > > v2 to v3 change: > > Add more comments about the reason for adding this api > > v1 to v2 change: > > No change > > --- > > drivers/usb/core/hub.c | 13 +++++++++++++ include/linux/usb/phy.h | > > 13 +++++++++++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index > > a739403a9e45..8433ff89dea6 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, > int port1, int type, > > ret = 0; > > } > > mutex_unlock(&hub->status_mutex); > > + > > + if (!ret) { > > + struct usb_device *hdev = hub->hdev; > > + > > + if (hdev && !hdev->parent) { > > Why the check for no parent? Please document that here in a comment. I will add a comment : /* Only notify roothub. That is, when hdev->parent is empty. */ > > + struct usb_hcd *hcd = bus_to_hcd(hdev->bus); > > + > > + if (hcd->usb_phy) > > + > usb_phy_notify_port_status(hcd->usb_phy, > > + port1 - > 1, *status, *change); > > + } > > + } > > + > > This is safe to notify with the hub mutex unlocked? Again, a comment would > be helpful to future people explaining why that is so. > I will add a comment: /* * There is no need to lock status_mutex here, because status_mutex * protects hub->status, and the phy driver only checks the port * status without changing the status. */ Thanks, Stanley .
On Mon, Jul 24, 2023 at 06:49:50AM +0000, Stanley Chang[昌育德] wrote: > > > > > On Fri, Jul 07, 2023 at 02:47:00PM +0800, Stanley Chang wrote: > > > In Realtek SoC, the parameter of usb phy is designed to can dynamic > > > tuning base on port status. Therefore, add a notify callback of phy > > > driver when usb port status change. > > > > > > The Realtek phy driver is designed to dynamically adjust disconnection > > > level and calibrate phy parameters. When the device connected bit > > > changes and when the disconnected bit changes, do port status change > > notification: > > > > > > Check if portstatus is USB_PORT_STAT_CONNECTION and portchange is > > > USB_PORT_STAT_C_CONNECTION. > > > 1. The device is connected, the driver lowers the disconnection level and > > > calibrates the phy parameters. > > > 2. The device disconnects, the driver increases the disconnect level and > > > calibrates the phy parameters. > > > > > > When controller to notify connect that device is already ready. If we > > > adjust the disconnection level in notify_connect, the disconnect may > > > have been triggered at this stage. So we need to change that as early > > > as possible. Therefore, we add an api to notify phy the port status changes. > > > > How do you know that the disconnect will not have already been triggered at > > this point, when the status changes? > > The status change of connection is before port reset. > In this stage, the device is not port enable, and it will not trigger disconnection. Ok, then say that here please :) > > > > > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com> > > > --- > > > v6 to v7 change: > > > No change > > > v5 to v6 change: > > > No change > > > v4 to v5 change: > > > No change > > > v3 to v4 change: > > > Fix the warning for checkpatch with strict. > > > v2 to v3 change: > > > Add more comments about the reason for adding this api > > > v1 to v2 change: > > > No change > > > --- > > > drivers/usb/core/hub.c | 13 +++++++++++++ include/linux/usb/phy.h | > > > 13 +++++++++++++ > > > 2 files changed, 26 insertions(+) > > > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index > > > a739403a9e45..8433ff89dea6 100644 > > > --- a/drivers/usb/core/hub.c > > > +++ b/drivers/usb/core/hub.c > > > @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, > > int port1, int type, > > > ret = 0; > > > } > > > mutex_unlock(&hub->status_mutex); > > > + > > > + if (!ret) { > > > + struct usb_device *hdev = hub->hdev; > > > + > > > + if (hdev && !hdev->parent) { > > > > Why the check for no parent? Please document that here in a comment. > > I will add a comment : > /* Only notify roothub. That is, when hdev->parent is empty. */ Also document this that this will only happen for root hub status changes, that's not obvious in the callback name or documentation or anywhere else here. > > > + struct usb_hcd *hcd = bus_to_hcd(hdev->bus); > > > + > > > + if (hcd->usb_phy) > > > + > > usb_phy_notify_port_status(hcd->usb_phy, > > > + port1 - > > 1, *status, *change); > > > + } > > > + } > > > + > > > > This is safe to notify with the hub mutex unlocked? Again, a comment would > > be helpful to future people explaining why that is so. > > > > I will add a comment: > /* > * There is no need to lock status_mutex here, because status_mutex > * protects hub->status, and the phy driver only checks the port > * status without changing the status. > */ Looks good, if you do it without the trailing whitespace :) thanks, greg k-h
Hi Greg, > > > > > > How do you know that the disconnect will not have already been > > > triggered at this point, when the status changes? > > > > The status change of connection is before port reset. > > In this stage, the device is not port enable, and it will not trigger > disconnection. > > Ok, then say that here please :) Okay. I will add it. > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index > > > > a739403a9e45..8433ff89dea6 100644 > > > > --- a/drivers/usb/core/hub.c > > > > +++ b/drivers/usb/core/hub.c > > > > @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub > > > > *hub, > > > int port1, int type, > > > > ret = 0; > > > > } > > > > mutex_unlock(&hub->status_mutex); > > > > + > > > > + if (!ret) { > > > > + struct usb_device *hdev = hub->hdev; > > > > + > > > > + if (hdev && !hdev->parent) { > > > > > > Why the check for no parent? Please document that here in a comment. > > > > I will add a comment : > > /* Only notify roothub. That is, when hdev->parent is empty. */ > > Also document this that this will only happen for root hub status changes, that's > not obvious in the callback name or documentation or anywhere else here. All usb phy notifications (connection, disconnection) are only for roothub. So I don't special to doc this. > > > > + struct usb_hcd *hcd = bus_to_hcd(hdev->bus); > > > > + > > > > + if (hcd->usb_phy) > > > > + > > > usb_phy_notify_port_status(hcd->usb_phy, > > > > + > port1 - > > > 1, *status, *change); > > > > + } > > > > + } > > > > + > > > > > > This is safe to notify with the hub mutex unlocked? Again, a > > > comment would be helpful to future people explaining why that is so. > > > > > > > I will add a comment: > > /* > > * There is no need to lock status_mutex here, because status_mutex > > * protects hub->status, and the phy driver only checks the port > > * status without changing the status. > > */ > > Looks good, if you do it without the trailing whitespace :) > Okay Thanks, Stanley
Hi Greg, > > > > > + > > > > > + if (!ret) { > > > > > + struct usb_device *hdev = hub->hdev; > > > > > + > > > > > + if (hdev && !hdev->parent) { > > > > > > > > Why the check for no parent? Please document that here in a > comment. > > > > > > I will add a comment : > > > /* Only notify roothub. That is, when hdev->parent is empty. */ > > > > Also document this that this will only happen for root hub status > > changes, that's not obvious in the callback name or documentation or > anywhere else here. > > All usb phy notifications (connection, disconnection) are only for roothub. > So I don't special to doc this. I will add the comment: /* * Applies to roothub only. That is, when hdev->parent is * empty. Only the roothub will be notified of port state * changes, since the USB PHY only cares about changes at * the next level. */ Thanks, Stanley
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index a739403a9e45..8433ff89dea6 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, int port1, int type, ret = 0; } mutex_unlock(&hub->status_mutex); + + if (!ret) { + struct usb_device *hdev = hub->hdev; + + if (hdev && !hdev->parent) { + struct usb_hcd *hcd = bus_to_hcd(hdev->bus); + + if (hcd->usb_phy) + usb_phy_notify_port_status(hcd->usb_phy, + port1 - 1, *status, *change); + } + } + return ret; } diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h index e4de6bc1f69b..b513749582d7 100644 --- a/include/linux/usb/phy.h +++ b/include/linux/usb/phy.h @@ -144,6 +144,10 @@ struct usb_phy { */ int (*set_wakeup)(struct usb_phy *x, bool enabled); + /* notify phy port status change */ + int (*notify_port_status)(struct usb_phy *x, int port, + u16 portstatus, u16 portchange); + /* notify phy connect status change */ int (*notify_connect)(struct usb_phy *x, enum usb_device_speed speed); @@ -316,6 +320,15 @@ usb_phy_set_wakeup(struct usb_phy *x, bool enabled) return 0; } +static inline int +usb_phy_notify_port_status(struct usb_phy *x, int port, u16 portstatus, u16 portchange) +{ + if (x && x->notify_port_status) + return x->notify_port_status(x, port, portstatus, portchange); + else + return 0; +} + static inline int usb_phy_notify_connect(struct usb_phy *x, enum usb_device_speed speed) {