Received-SPF: pass (google.com: domain of linux-kernel+bounces-154201-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 23 Apr 2024 07:32:32 +0800
From: Inochi Amaoto <inochiama@outlook.com>
To: Conor Dooley <conor@kernel.org>, Inochi Amaoto <inochiama@outlook.com>
Cc: Vinod Koul <vkoul@kernel.org>, 
	Kishon Vijay Abraham I <kishon@kernel.org>, Rob Herring <robh@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Chen Wang <unicorn_wang@outlook.com>, Paul Walmsley <paul.walmsley@sifive.com>, 
	Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, 
	Jisheng Zhang <jszhang@kernel.org>, Liu Gui <kenneth.liu@sophgo.com>, 
	Jingbao Qiu <qiujingbao.dlmu@gmail.com>, dlan@gentoo.org, linux-phy@lists.infradead.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH 1/2] dt-bindings: phy: Add Sophgo CV1800 USB phy
Message-ID:
 <IA1PR20MB49531FB44161BC81C3DDACF6BB122@IA1PR20MB4953.namprd20.prod.outlook.com>
References: <IA1PR20MB4953780F4617823F7173B6D6BB042@IA1PR20MB4953.namprd20.prod.outlook.com>
 <IA1PR20MB4953040232A4D41E41F2D2A9BB042@IA1PR20MB4953.namprd20.prod.outlook.com>
 <20240419-harmful-neutron-d0db367cf659@spud>
 <IA1PR20MB49536E7548B65B41F4F33FDEBB0C2@IA1PR20MB4953.namprd20.prod.outlook.com>
 <20240422-folic-obtuse-570b1747e7b9@spud>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240422-folic-obtuse-570b1747e7b9@spud>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: IA1PR20MB4953:EE_|DS7PR20MB4855:EE_
X-MS-Office365-Filtering-Correlation-Id: 86fa9f86-2939-4242-232f-08dc63246ce2
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	1M21yHF5YVoVgm354X8USyx5OKgqS1FIviy0xIhZkSxuCeI0qfyb6s5G6XqWfeuqb5B4+JxZImjIg4t50L6sG1p8I0A8Uh+ZI1ZSHmgEnS2zU5/TB0n5Boi2DYdChI9S+wqR76Qnw7l/gYIudCstNUqMaqFOJuetSrz/WN+TT3lc8TAecRSKzWb2bW25fzbQQweR9BVbjHHlREj+JnQZzFTniB16moRBtsmNjBpmbAldSQluT3unOf1L+y+FZ0H1vYrsSnjvnuHw5/BEFp4uZaTHeysCBtTXx0dCgmEnQdDLgiiMBBFEKv1xKnTVRKtO+euOdZo9n6eUsdRWM335y+o3ji5nhm/rbrzOdPXaHI8cm3624jQzyKOUvcjV8HxARn8+RQ3iMeHHgA4FvvHO+sfNx44Yqqqpe/aOtATETvkIy/KHltvUd0sU96jgIDLp3yiWfW29m5QzmWaLOEFa3vBHihf5/C1KvaTBhvTtduC9Z058//B0kGv7mclWItZmzk1g0FBpl/e6jK+kRxLCd34qLphhvWI3mRPOG8IBNl90bjtZ0QZprWWZa8xyp1254b2OYNYVdbsbG78llqeKeJ6/br4kH0BbTCd0b4NX/LFknfGNt84fAlAGFqQ5K7CDinhJkvbMr838nt+yfK+hPQuHSwuEXUOPFMyiYRdByoujnSPDTFdgoi/NTwaUyzlmo4RyKZxctvx2EgAlAgPbZg==
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?RSsUv9I6Li2WEFmZAzYVm3aH1ZC5hkkMMwrm6x+df8UWaS3wSA/JU/O5u+Lk?=
 =?us-ascii?Q?N+Lhj+jGzPEnllSBj41dTgcnzIHJ2bX7MtTgt+azONM2u5RuO3pdlPSOJu5d?=
 =?us-ascii?Q?a1xMbHZGB4HCXO1omME44WlJNIWTs63yfASCpBI6Hxu/kRNMfLovjWXmviFW?=
 =?us-ascii?Q?7lx7yu/BL90py0TufbICfQH4pOH0GbbjaX2PDcIGUgdf00UvZmfVvcCNZPcp?=
 =?us-ascii?Q?x8stLKS/cXKlccG19DlK2FS+r9a9bH8L7slVJMi7tovtPr9zKYS1d3q+XC+f?=
 =?us-ascii?Q?VyU3/Y+smwCFDVZBX/J/VYKwbUjhHWaJqLASc0yRCUgERrYtWOs3knFucbn9?=
 =?us-ascii?Q?2gJVmt0jOwuFPdtSfMllKRqXyFL3PDydlVllFQhys0E358If/d+FxosmMGkN?=
 =?us-ascii?Q?tOUdGNRYPFxVeA2m3TzE6WYGRi0MK8gCcYc0ujZlh+huF3uqGmSFvXu2j/vm?=
 =?us-ascii?Q?bfqdjaKETO5N73CHfKV/OtSSh5kgnFwOTEawntMvIs+LcqTxY/3SkT4idRwV?=
 =?us-ascii?Q?zgWdpnI5jtWicQinrkRKT6mj3izcL+pAuWpt4/KyiUMdzGKtOCqHZyGtUxmS?=
 =?us-ascii?Q?v7YAruQvEoli3UZzqax91Ak7NEpIbR9CIJVn8/K3BaSEbqFbQjoO6qE94Iw7?=
 =?us-ascii?Q?2R7q+M/HCMWV01sTN7znCpNHhGyQnmHhFU2mkoYV/SxprffyhLUd+TYREHAc?=
 =?us-ascii?Q?F5jU9N0Ud7+dZr75Qd2m569kN/DIuGtrTYvrwqbTITdtVFQIWzuU6PrcktCj?=
 =?us-ascii?Q?9q7O2hB4EwvuTB9g4eRweCOwWehSg+tvuWqN1LoUgZxegBE9EtHpmwcNDWxn?=
 =?us-ascii?Q?iYdiTh7fFMmCkg1HZlRs9TBrMNPVHhZ7B70iO0Rj504fGC7RzHCQZCFqFoTu?=
 =?us-ascii?Q?PPlB8cFyEmm0jdM8kLkVSbs1Jd4Y2R3dIPqnyLfnw0RVte3NTCXxz+KaFVIg?=
 =?us-ascii?Q?z4oZah1SL4KSzdN9i++glXJa+6Ms60clWUYHzeXzrbo+1c8aCA5qpPw4V6iQ?=
 =?us-ascii?Q?vH+wpp/5+kXPQE9SQt05B5O0medY42nB3uE5syh9MEHwMfwpSzBuPakvDSgT?=
 =?us-ascii?Q?LF89jG5DwB2eY637izBdp1RO9KXA1SeII8WkGlPmvaLBYUDjv+0+CSww+vnN?=
 =?us-ascii?Q?2dfpUM5vntKapv20lgLwaQdO8c5eQjfKcmpZD/8SSxYIaSWnKYsL8NhZuJoW?=
 =?us-ascii?Q?JqNobOo2TOWakRzMg2HvVNvJusbGbdHUHNNBZrUs1Or0WvWkEEQKhT8SMXpQ?=
 =?us-ascii?Q?sZ+N7fcGWAuiJBAxYe2b?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 86fa9f86-2939-4242-232f-08dc63246ce2
X-MS-Exchange-CrossTenant-AuthSource: IA1PR20MB4953.namprd20.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Apr 2024 23:32:08.0462
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR20MB4855

On Mon, Apr 22, 2024 at 05:21:26PM GMT, Conor Dooley wrote:
> On Sat, Apr 20, 2024 at 09:39:18AM +0800, Inochi Amaoto wrote:
> > On Fri, Apr 19, 2024 at 03:26:53PM GMT, Conor Dooley wrote:
> > > On Fri, Apr 12, 2024 at 03:22:24PM +0800, Inochi Amaoto wrote:
> > > > The USB phy of Sophgo CV18XX series SoC needs to sense a pin called
> > > > "VBUS_DET" to get the right operation mode. If this pin is not
> > > > connected, it only supports setting the mode manually.
> > > > 
> > > > Add USB phy bindings for Sophgo CV18XX/SG200X series SoC.
> > > > 
> > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> > > > ---
> > > >  .../bindings/phy/sophgo,cv1800-usb-phy.yaml   | 90 +++++++++++++++++++
> > > >  1 file changed, 90 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > > new file mode 100644
> > > > index 000000000000..cb394ac5d8c4
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/sophgo,cv1800-usb-phy.yaml
> > > > @@ -0,0 +1,90 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/sophgo,cv1800-usb-phy.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Sophgo CV18XX/SG200X USB 2.0 PHY
> > > > +
> > > > +maintainers:
> > > > +  - Inochi Amaoto <inochiama@outlook.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: sophgo,cv1800-usb-phy
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  "#phy-cells":
> > > > +    const: 0
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: PHY clock
> > > > +      - description: PHY app clock
> > > > +      - description: PHY stb clock
> > > > +      - description: PHY lpm clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: phy
> > > > +      - const: app
> > > > +      - const: stb
> > > > +      - const: lpm
> > > > +
> > > > +  dr_mode:
> > > > +    description: PHY device mode when initing.
> > > 
> > > "initing" isn't a word, "initialising" is the correct word. Or
> > > "initializing" if you speak American. But if it is just the value during
> > > initialisation, why do we need to know - we can just overwrite it in
> > > software, right?
> > > 
> > > Are you sure that this is limited to initialisation? I would have
> > > thought that it describes the configuration that the board is in, and is
> > > a fixed property of the system?
> > > 
> > > > +    enum: [host, peripheral, otg]
> > > 
> > > Rob, I know this is a phy rather than a controller, so referencing
> > > usb-drd.yaml doesn't really make sense, but there are several PHYs using
> > > dr_mode so it feels like there should be something to reference here
> > > rather than defining the property anew.
> > > 
> > 
> > Yes, you are right. Using dr_mode in initialisation is not necessary.
> > We can just let it go and using the default mode. In fact, for most
> > boards with this SoC, host mode is just enough. And it is just easy 
> > to overwrite the mode value in the driver if we want another mode. 
> > For the OTG, it can just check the `vbus_det-gpios`. I will remove 
> > this property, thanks.
> 
> Just to be clear, it's valid to have a dr_mode property in cases that
> you cannot detect what the mode is dynamically. What I was questioning
> was the wording about only valid for init.
> 

OK, for the USB phy of CV1800, it always needs to start at host mode.
Because it needs to switch to both mode when initializing. As a result,
the "dr_mode" property is just added to decide which mode is set after
initializing (mostly for functionality). This is why I say it is for 
initializing.

Now, it is clean that setting dr_mode is just a function hint, and user 
can just overwrite the mode. So I decided to remove this.

> > > > +  vbus_det-gpios:
> > > > +    description: GPIO to the USB OTG VBUS detect pin. This should not be
> > > > +      defined if vbus_det gpio and switch gpio are connected.
> > > 
> > > I don't understand the second sentence here.
> 
> Ah, with your explanation below I now understand what you mean here. I
> think this needs to be re-written - I think it would be easier to
> understand with s/gpio/pin/ in the second line.
> 
> > > > +    maxItems: 1
> > > > +
> > > > +  sophgo,switch-gpios:
> > > > +    description: GPIO for the phy to control connected switch.
> > > > +    maxItems: 2
> > > 
> > > You've got two items here, they should be described. /But/ the property
> > > above says "switch gpio", which is singular. Which is it?
> > > 
> > 
> > `switch-gpios` is gpios to controll USB switch connected to the
> > phy. Sophgo recommends that phy use a switch to separate device
> > port and host port if it supports both. I know this is weird,
> > but many board follows this design, such as Huashan PI and 
> > Milkv Duo S. As for item number, it is just an array of gpios
> > to process one by one, I mark it as two just because Milkv 
> > Duo S use two gpios to controll the USB switch.
> 
> Right, but what I'm looking for is a description for what each GPIO
> does, so that someone can know how the dts should be written.
> 
> > For vbus_det gpio description, There is because the design of 
> > Milk-v Duo S, which connect the switch gpio and VBUS detect 
> > gpio. In this case the OTG function is broken and we can just 
> > change the mode by toggling switch gpios. So I suggest not 
> > defining this property.
> 
> Okay. See my comment on it above.
> 

Thanks, I will add some necessary comments for these two properties.

> Thanks,
> Conor.