Message ID | a8c567cf8c3ec6fef426b64fb1ab7f6e63a0cc07.1675779094.git.daniel@makrotopia.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2388:b0:96:219d:e725 with SMTP id i8csp4037739dyf; Tue, 7 Feb 2023 06:24:33 -0800 (PST) X-Google-Smtp-Source: AK7set/PgZ/m7CoUh6sNkw/E0a0hzNMTBYfjnxMxbvVvCNC8lS6BoqnP/WjUJZiSFKRvlrFxCHy6 X-Received: by 2002:a17:906:1e55:b0:88d:ba89:1832 with SMTP id i21-20020a1709061e5500b0088dba891832mr15371735ejj.3.1675779873223; Tue, 07 Feb 2023 06:24:33 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1675779873; cv=none; d=google.com; s=arc-20160816; b=HwCR4XzALfCZdQdtiO3cvketBiqIM6c6g0s2lvtRSgx/4X1t8iN0nImK+i1OBBZFkO oXXuaQg/xC7OkR2Dp//9Azp6TmoXRJoXBnnazc4NxZ2bgl+LYypdQD+Xlszo/u0dGoh2 nvtRh3jX4q4MvkXR4Ip/byXqrhrsM4NJVFnjFBbrVYIdn8GvLEzLyl3vdgO+h9pZhUst PW5VTM8emzxWnuH6QZcmHcvQ6sgf+KsnJvWXedQHnmTvUqK5XLZqW3LKOExp5RC6A+HI fUmBQ82/3l2wS1dZQs7AxpFrp3cQ5l5H5PQQDca7QNmwnTCtxxCT3VMUFyg4Yh45DSDW SHGw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=2kKqHS2/RfkMDfcJWS9LkdIEljgm6B/2/QEI3d1vhac=; b=ilRIV3PQq50Rx0eB2tB5kbkP05je7WKIu50LNlzsHdZNUIHkRLYwfTDlAayLnHikMp ZK7qBq1vcTSoKfu5LqFUtSOXm9TsFS4XgP6yJ2vthgP5+6aAiwyIpFMyEur6kesrJhb3 BjucNvYhHY7MFatjHlEppNDQ+OQ1PwlDz10YPno+uopFG70t1qsYTcxRpIJSwg8aKzVX 9hOQDYclGcUF4PZBA6V3KfXGvIeVCW1DPVotSNfE7pahp/IvDQNMuHeXi6LHka9zBlF0 0/soZ7/DeAfITErRVbbj+wCjfIz+Hz2cM15BbJZXufP62ah8wyVwujIg3mbt8XIJvg8A N2Cg== 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 f23-20020a1709063f5700b0087849c91571si18906240ejj.352.2023.02.07.06.24.10; Tue, 07 Feb 2023 06:24:33 -0800 (PST) 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 S232435AbjBGOVm (ORCPT <rfc822;kmanaouilinux@gmail.com> + 99 others); Tue, 7 Feb 2023 09:21:42 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33678 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232110AbjBGOVl (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 7 Feb 2023 09:21:41 -0500 Received: from fudo.makrotopia.org (fudo.makrotopia.org [IPv6:2a07:2ec0:3002::71]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 255AA3526F; Tue, 7 Feb 2023 06:21:35 -0800 (PST) Received: from local by fudo.makrotopia.org with esmtpsa (TLS1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.96) (envelope-from <daniel@makrotopia.org>) id 1pPOqi-00028S-2a; Tue, 07 Feb 2023 15:21:33 +0100 Date: Tue, 7 Feb 2023 14:19:56 +0000 From: Daniel Golle <daniel@makrotopia.org> To: netdev@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Russell King <linux@armlinux.org.uk>, Heiner Kallweit <hkallweit1@gmail.com>, Lorenzo Bianconi <lorenzo@kernel.org>, Mark Lee <Mark-MC.Lee@mediatek.com>, John Crispin <john@phrozen.org>, Felix Fietkau <nbd@nbd.name>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Matthias Brugger <matthias.bgg@gmail.com>, DENG Qingfang <dqfext@gmail.com>, Landen Chao <Landen.Chao@mediatek.com>, Sean Wang <sean.wang@mediatek.com>, Paolo Abeni <pabeni@redhat.com>, Jakub Kicinski <kuba@kernel.org>, Eric Dumazet <edumazet@google.com>, "David S. Miller" <davem@davemloft.net>, Vladimir Oltean <olteanv@gmail.com>, Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch> Cc: Jianhui Zhao <zhaojh329@gmail.com>, =?iso-8859-1?q?Bj=F8rn?= Mork <bjorn@mork.no> Subject: [PATCH v2 03/11] dt-bindings: arm: mediatek: add 'mediatek,pn_swap' property Message-ID: <a8c567cf8c3ec6fef426b64fb1ab7f6e63a0cc07.1675779094.git.daniel@makrotopia.org> References: <cover.1675779094.git.daniel@makrotopia.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <cover.1675779094.git.daniel@makrotopia.org> X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS 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?1757182555906631440?= X-GMAIL-MSGID: =?utf-8?q?1757182555906631440?= |
Series |
net: ethernet: mtk_eth_soc: various enhancements
|
|
Commit Message
Daniel Golle
Feb. 7, 2023, 2:19 p.m. UTC
Add documentation for the newly introduced 'mediatek,pn_swap' property
to mediatek,sgmiisys.txt.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
.../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++
1 file changed, 4 insertions(+)
Comments
On 07/02/2023 15:19, Daniel Golle wrote: > Add documentation for the newly introduced 'mediatek,pn_swap' property > to mediatek,sgmiisys.txt. > Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > index d2c24c277514..b38dd0fde21d 100644 > --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > @@ -14,6 +14,10 @@ Required Properties: > - "mediatek,mt7986-sgmiisys_1", "syscon" > - #clock-cells: Must be 1 > > +Optional Properties: > + > +- mediatek,pn_swap: Invert polarity of the SGMII data lanes. No: 1. No new properties for TXT bindings, 2. Underscore is not allowed. 3. Does not look like property of this node. This is a clock controller or system controller, not SGMII/phy etc. Best regards, Krzysztof
Hi Krzysztof, On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote: > On 07/02/2023 15:19, Daniel Golle wrote: > > Add documentation for the newly introduced 'mediatek,pn_swap' property > > to mediatek,sgmiisys.txt. > > > > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC. It might happen, that command when run on an older > kernel, gives you outdated entries. Therefore please be sure you base > your patches on recent Linux kernel. > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > > index d2c24c277514..b38dd0fde21d 100644 > > --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > > @@ -14,6 +14,10 @@ Required Properties: > > - "mediatek,mt7986-sgmiisys_1", "syscon" > > - #clock-cells: Must be 1 > > > > +Optional Properties: > > + > > +- mediatek,pn_swap: Invert polarity of the SGMII data lanes. > > No: > 1. No new properties for TXT bindings, So I'll have to convert the bindings to YAML, right? > 2. Underscore is not allowed. Ack, will change the name of the property. > 3. Does not look like property of this node. This is a clock controller > or system controller, not SGMII/phy etc. The register range referred to by this node *does* represent also an SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and are referenced in the node of the Ethernet core, and then used by drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. (This is the current situation already, and not related to the patchset now adding only a new property to support hardware which needs that) So: Should I introduce a new binding for the same compatible strings related to the SGMII PHY features? Or is it fine in this case to add this property to the existing binding? Thank you in advance for your advise! Daniel
On 07/02/2023 19:00, Daniel Golle wrote: > Hi Krzysztof, > > On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote: >> On 07/02/2023 15:19, Daniel Golle wrote: >>> Add documentation for the newly introduced 'mediatek,pn_swap' property >>> to mediatek,sgmiisys.txt. >>> >> >> Please use scripts/get_maintainers.pl to get a list of necessary people >> and lists to CC. It might happen, that command when run on an older >> kernel, gives you outdated entries. Therefore please be sure you base >> your patches on recent Linux kernel. >> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >>> --- >>> .../devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt >>> index d2c24c277514..b38dd0fde21d 100644 >>> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt >>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt >>> @@ -14,6 +14,10 @@ Required Properties: >>> - "mediatek,mt7986-sgmiisys_1", "syscon" >>> - #clock-cells: Must be 1 >>> >>> +Optional Properties: >>> + >>> +- mediatek,pn_swap: Invert polarity of the SGMII data lanes. >> >> No: >> 1. No new properties for TXT bindings, > > So I'll have to convert the bindings to YAML, right? Yes, please. > >> 2. Underscore is not allowed. > > Ack, will change the name of the property. > >> 3. Does not look like property of this node. This is a clock controller >> or system controller, not SGMII/phy etc. > > The register range referred to by this node *does* represent also an > SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and > are referenced in the node of the Ethernet core, and then used by > drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. > (This is the current situation already, and not related to the patchset > now adding only a new property to support hardware which needs that) Just because a register is located in syscon block, does not mean that SGMII configuration is a property of this device. > > So: Should I introduce a new binding for the same compatible strings > related to the SGMII PHY features? Or is it fine in this case to add > this property to the existing binding? The user of syscon should configure it. I don't think you need new binding. You just have to update the user of this syscon. Best regards, Krzysztof
On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote: > On 07/02/2023 19:00, Daniel Golle wrote: > > ... > >> 3. Does not look like property of this node. This is a clock controller > >> or system controller, not SGMII/phy etc. > > > > The register range referred to by this node *does* represent also an > > SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and > > are referenced in the node of the Ethernet core, and then used by > > drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. > > (This is the current situation already, and not related to the patchset > > now adding only a new property to support hardware which needs that) > > Just because a register is located in syscon block, does not mean that > SGMII configuration is a property of this device. It's not just one register, the whole SGMII PCS is located in those mediatek,sgmiisys syscon nodes. > > > > > So: Should I introduce a new binding for the same compatible strings > > related to the SGMII PHY features? Or is it fine in this case to add > > this property to the existing binding? > > The user of syscon should configure it. I don't think you need new > binding. You just have to update the user of this syscon. Excuse my confusion, but it's still not entirely clear to me. So in this case I should add the description of the added propterty of the individual SGMII units (there can be more than one) to Documentation/devicetree/bindings/net/mediatek,net.yaml eventhough the properties are in the sgmiisys syscon nodes? If so I will have to figure out how to describe properties of other nodes in the binding of the node referencing them. Are there any good examples for that? Or should the property itself be moved into yet another array of booleans which should be added in the node describing the ethernet controller and referencing these sgmiisys syscons using phandles?
On 08/02/2023 14:51, Daniel Golle wrote: > On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote: >> On 07/02/2023 19:00, Daniel Golle wrote: >>> ... >>>> 3. Does not look like property of this node. This is a clock controller >>>> or system controller, not SGMII/phy etc. >>> >>> The register range referred to by this node *does* represent also an >>> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and >>> are referenced in the node of the Ethernet core, and then used by >>> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. >>> (This is the current situation already, and not related to the patchset >>> now adding only a new property to support hardware which needs that) >> >> Just because a register is located in syscon block, does not mean that >> SGMII configuration is a property of this device. > > It's not just one register, the whole SGMII PCS is located in those > mediatek,sgmiisys syscon nodes. Then maybe this should be a PCS PHY device instead of adding properties unrelated to clock/system controller? I don't know, currently this binding says it is a provider of clocks... > >> >>> >>> So: Should I introduce a new binding for the same compatible strings >>> related to the SGMII PHY features? Or is it fine in this case to add >>> this property to the existing binding? >> >> The user of syscon should configure it. I don't think you need new >> binding. You just have to update the user of this syscon. > > Excuse my confusion, but it's still not entirely clear to me. > So in this case I should add the description of the added propterty of > the individual SGMII units (there can be more than one) to > Documentation/devicetree/bindings/net/mediatek,net.yaml > eventhough the properties are in the sgmiisys syscon nodes? I guess the property should be in the node representing the SGMII. You add it now to the clock (or system) controller, so it does not look right. It's not a property of a clock controller. Now which node should have this property depends on your devices - which I have no clue about, I read what is in the bindings. > > If so I will have to figure out how to describe properties of other > nodes in the binding of the node referencing them. Are there any > good examples for that? phys and pcs'es? > > Or should the property itself be moved into yet another array of > booleans which should be added in the node describing the ethernet > controller and referencing these sgmiisys syscons using phandles? Best regards, Krzysztof
Hi Krzysztof, thank you for taking the time to review and explain. On Wed, Feb 08, 2023 at 09:08:40PM +0100, Krzysztof Kozlowski wrote: > On 08/02/2023 14:51, Daniel Golle wrote: > > On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote: > >> On 07/02/2023 19:00, Daniel Golle wrote: > >>> ... > >>>> 3. Does not look like property of this node. This is a clock controller > >>>> or system controller, not SGMII/phy etc. > >>> > >>> The register range referred to by this node *does* represent also an > >>> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and > >>> are referenced in the node of the Ethernet core, and then used by > >>> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. > >>> (This is the current situation already, and not related to the patchset > >>> now adding only a new property to support hardware which needs that) > >> > >> Just because a register is located in syscon block, does not mean that > >> SGMII configuration is a property of this device. > > > > It's not just one register, the whole SGMII PCS is located in those > > mediatek,sgmiisys syscon nodes. > > Then maybe this should be a PCS PHY device instead of adding properties > unrelated to clock/system controller? I don't know, currently this > binding says it is a provider of clocks... As in reality it is really a clock provider and also SGMII PCS at the same time, maybe we should just update the description of the binding to match that: diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt index d2c24c2775141..db6f75df200ba 100644 --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt @@ -2,6 +2,7 @@ MediaTek SGMIISYS controller ============================ The MediaTek SGMIISYS controller provides various clocks to the system. +It also represents the SGMII PCS used by the Ethernet core. Required Properties: See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mediatek/mtk_sgmii.c#n179 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt7986a.dtsi#n409 > > > > >> > >>> > >>> So: Should I introduce a new binding for the same compatible strings > >>> related to the SGMII PHY features? Or is it fine in this case to add > >>> this property to the existing binding? > >> > >> The user of syscon should configure it. I don't think you need new > >> binding. You just have to update the user of this syscon. > > > > Excuse my confusion, but it's still not entirely clear to me. > > So in this case I should add the description of the added propterty of > > the individual SGMII units (there can be more than one) to > > Documentation/devicetree/bindings/net/mediatek,net.yaml > > eventhough the properties are in the sgmiisys syscon nodes? > > I guess the property should be in the node representing the SGMII. You > add it now to the clock (or system) controller, so it does not look > right. It's not a property of a clock controller. Well maybe this node needs to be split then into one node representing only the clock controller and another node representing the SGMII PCS? I'm not sure if this is even possible, some registers in this range represent clocks, other registers are accessed using regmap API in mtk_sgmii.c. And (see the rest of this series) the exact same SGMII PCS can also be found in MT7531 switch IC which has it's own (a bit odd) way to access 32-bit registers over MDIO, also in this case it is simply not easily possible to represent the SGMII PCS in device tree. > > Now which node should have this property depends on your devices - which > I have no clue about, I read what is in the bindings. There isn't any other node exclusively representing the SGMII PCS. I guess the only other option would be to move the property to the Ethernet controller node, which imho complicates things as it is really a property of an individual SGMII PHY (of which there can be more than one). > > > > > If so I will have to figure out how to describe properties of other > > nodes in the binding of the node referencing them. Are there any > > good examples for that? > > phys and pcs'es? Hm, none of the current PCS (or PHY) drivers are represented by a syscon node... (and maybe that's the mistake in first place?) > > > > > Or should the property itself be moved into yet another array of > > booleans which should be added in the node describing the ethernet > > controller and referencing these sgmiisys syscons using phandles? > > Best regards, > Krzysztof >
On 08/02/2023 23:30, Daniel Golle wrote: > Hi Krzysztof, > > thank you for taking the time to review and explain. > > On Wed, Feb 08, 2023 at 09:08:40PM +0100, Krzysztof Kozlowski wrote: >> On 08/02/2023 14:51, Daniel Golle wrote: >>> On Wed, Feb 08, 2023 at 10:32:53AM +0100, Krzysztof Kozlowski wrote: >>>> On 07/02/2023 19:00, Daniel Golle wrote: >>>>> ... >>>>>> 3. Does not look like property of this node. This is a clock controller >>>>>> or system controller, not SGMII/phy etc. >>>>> >>>>> The register range referred to by this node *does* represent also an >>>>> SGMII phy. These sgmiisys nodes also carry the 'syscon' compatible, and >>>>> are referenced in the node of the Ethernet core, and then used by >>>>> drivers/net/ethernet/mediatek/mtk_sgmii.c using syscon_node_to_regmap. >>>>> (This is the current situation already, and not related to the patchset >>>>> now adding only a new property to support hardware which needs that) >>>> >>>> Just because a register is located in syscon block, does not mean that >>>> SGMII configuration is a property of this device. >>> >>> It's not just one register, the whole SGMII PCS is located in those >>> mediatek,sgmiisys syscon nodes. >> >> Then maybe this should be a PCS PHY device instead of adding properties >> unrelated to clock/system controller? I don't know, currently this >> binding says it is a provider of clocks... > > As in reality it is really a clock provider and also SGMII PCS at the > same time, maybe we should just update the description of the binding > to match that: > > diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > index d2c24c2775141..db6f75df200ba 100644 > --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt > @@ -2,6 +2,7 @@ MediaTek SGMIISYS controller > ============================ > > The MediaTek SGMIISYS controller provides various clocks to the system. > +It also represents the SGMII PCS used by the Ethernet core. > > Required Properties: > > > See > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mediatek/mtk_sgmii.c#n179 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt7986a.dtsi#n409 But it is not used as phy or PCS. It is used as syscon. If it was a phy or PCS, then the property probably belonged here, but bindings and Linux implementation were created in different way, so it is not a phy or PCS, just a syscon. >>>>> So: Should I introduce a new binding for the same compatible strings >>>>> related to the SGMII PHY features? Or is it fine in this case to add >>>>> this property to the existing binding? >>>> >>>> The user of syscon should configure it. I don't think you need new >>>> binding. You just have to update the user of this syscon. >>> >>> Excuse my confusion, but it's still not entirely clear to me. >>> So in this case I should add the description of the added propterty of >>> the individual SGMII units (there can be more than one) to >>> Documentation/devicetree/bindings/net/mediatek,net.yaml >>> eventhough the properties are in the sgmiisys syscon nodes? >> >> I guess the property should be in the node representing the SGMII. You >> add it now to the clock (or system) controller, so it does not look >> right. It's not a property of a clock controller. > > Well maybe this node needs to be split then into one node representing > only the clock controller and another node representing the SGMII PCS? > I'm not sure if this is even possible, some registers in this range > represent clocks, other registers are accessed using regmap API in > mtk_sgmii.c. This can be the same node, but it must be used like PCS. syscon phandle for getting regmap is something entirely else. > > And (see the rest of this series) the exact same SGMII PCS can also be > found in MT7531 switch IC which has it's own (a bit odd) way to access > 32-bit registers over MDIO, also in this case it is simply not easily > possible to represent the SGMII PCS in device tree. > >> >> Now which node should have this property depends on your devices - which >> I have no clue about, I read what is in the bindings. > > There isn't any other node exclusively representing the SGMII PCS. > I guess the only other option would be to move the property to the > Ethernet controller node, which imho complicates things as it is > really a property of an individual SGMII PHY (of which there can be > more than one). > >> >>> >>> If so I will have to figure out how to describe properties of other >>> nodes in the binding of the node referencing them. Are there any >>> good examples for that? >> >> phys and pcs'es? > > Hm, none of the current PCS (or PHY) drivers are represented by a > syscon node... (and maybe that's the mistake in first place?) Yes. Best regards, Krzysztof
On Tue, Feb 07, 2023 at 06:38:23PM +0100, Krzysztof Kozlowski wrote: > On 07/02/2023 15:19, Daniel Golle wrote: > > +Optional Properties: > > + > > +- mediatek,pn_swap: Invert polarity of the SGMII data lanes. > > No: > 3. Does not look like property of this node. This is a clock controller > or system controller, not SGMII/phy etc. Wrong. Totally wrong. SGMII data lines consist of a "positive" and a "negative" signal. Clearly, this PHY supports swapping the sense of those. This CLEARLY has nothing to do with a clock controller. It's also not system-controller related either.
On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote: > On 08/02/2023 23:30, Daniel Golle wrote: > > Hm, none of the current PCS (or PHY) drivers are represented by a > > syscon node... (and maybe that's the mistake in first place?) > > Yes. Nos, it isn't.
On Fri, Feb 10, 2023 at 10:34:17AM +0000, Russell King (Oracle) wrote: > On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote: > > On 08/02/2023 23:30, Daniel Golle wrote: > > > Hm, none of the current PCS (or PHY) drivers are represented by a > > > syscon node... (and maybe that's the mistake in first place?) > > > > Yes. > > Nos, it isn't. To expand on this - I have no idea why you consider it a mistake that apparently all PCS aren't represented by a syscon node. PCS is a sub-block in an ethernet system, just the same as a MAC is a sub-block. PCS can appear in several locations of an ethernet system, but are generally found either side of a serial ethernet link such as 1000base-X, SGMII, USXGMII, 10Gbase-R etc. So, one can find PCS within an ethernet PHY - and there may be one facing the MAC connection, and there will be another facing the media. We generally do not need to separate these PCS from the PHY itself because we view the PHY as one whole device. The optional PCS on the MAC side of the link is something that we need to know about, because this has to be configured to talk to the PHY, or to configure and obtain negotiation results from in the case of fibre links. PCS on the MAC side are not a system level device, they are very much a specific piece of ethernet hardware in the same way that the MAC is, and we don't represent the MAC as a syscon node. There is no reason to do so with PCS. These PCS on the MAC side tend to be accessed via direct MMIO accesses, or over a MDIO bus. There's other blocks in the IEEE 802.3 ethernet layering, such as the PMA/PMD module (which for the MAC side we tend to model with the drivers/phy layer) - but again, these also appear in ethernet PHYs in order to produce the electrical signals for e.g. twisted pair ethernet. So, to effectively state that you consider that PCS should always be represented as a syscon node is rather naieve, and really as a DT reviewer you should not be making such decisions, but soliciting opinions from those who know this subject area in detail _whether_ they are some kind of system controller before making such a decision.
On 10/02/2023 13:23, Russell King (Oracle) wrote: > On Fri, Feb 10, 2023 at 10:34:17AM +0000, Russell King (Oracle) wrote: >> On Thu, Feb 09, 2023 at 12:30:27PM +0100, Krzysztof Kozlowski wrote: >>> On 08/02/2023 23:30, Daniel Golle wrote: >>>> Hm, none of the current PCS (or PHY) drivers are represented by a >>>> syscon node... (and maybe that's the mistake in first place?) >>> >>> Yes. >> >> Nos, it isn't. > > To expand on this - I have no idea why you consider it a mistake that > apparently all PCS aren't represented by a syscon node. > > PCS is a sub-block in an ethernet system, just the same as a MAC is a > sub-block. PCS can appear in several locations of an ethernet system, > but are generally found either side of a serial ethernet link such > as 1000base-X, SGMII, USXGMII, 10Gbase-R etc. > > So, one can find PCS within an ethernet PHY - and there may be one > facing the MAC connection, and there will be another facing the media. > We generally do not need to separate these PCS from the PHY itself > because we view the PHY as one whole device. > > The optional PCS on the MAC side of the link is something that we > need to know about, because this has to be configured to talk to the > PHY, or to configure and obtain negotiation results from in the case of > fibre links. > > PCS on the MAC side are not a system level device, they are very much a > specific piece of ethernet hardware in the same way that the MAC is, > and we don't represent the MAC as a syscon node. There is no reason > to do so with PCS. > > These PCS on the MAC side tend to be accessed via direct MMIO accesses, > or over a MDIO bus. > > There's other blocks in the IEEE 802.3 ethernet layering, such as the > PMA/PMD module (which for the MAC side we tend to model with the > drivers/phy layer) - but again, these also appear in ethernet PHYs > in order to produce the electrical signals for e.g. twisted pair > ethernet. > > So, to effectively state that you consider that PCS should always be > represented as a syscon node is rather naieve, and really as a DT > reviewer you should not be making such decisions, but soliciting > opinions from those who know this subject area in detail _whether_ > they are some kind of system controller before making such a > decision. Daniel switched to private emails, so unfortunately our talk is not visible here, nevertheless thanks for the feedback. Much appreciated! Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt index d2c24c277514..b38dd0fde21d 100644 --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,sgmiisys.txt @@ -14,6 +14,10 @@ Required Properties: - "mediatek,mt7986-sgmiisys_1", "syscon" - #clock-cells: Must be 1 +Optional Properties: + +- mediatek,pn_swap: Invert polarity of the SGMII data lanes. + The SGMIISYS controller uses the common clk binding from Documentation/devicetree/bindings/clock/clock-bindings.txt The available clocks are defined in dt-bindings/clock/mt*-clk.h.