| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 30 Nov 2022 18:33:11 +0100
From: Andreas Färber <afaerber@...e.de>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Chester Lin <clin@...e.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jan Petrous <jan.petrous@....com>, Andrew Lunn <andrew@...n.ch>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>,
Giuseppe Cavallaro <peppe.cavallaro@...com>,
Jose Abreu <joabreu@...opsys.com>, netdev@...r.kernel.org,
s32@....com, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
Matthias Brugger <mbrugger@...e.com>
Subject: Re: [PATCH v2 2/5] dt-bindings: net: add schema for NXP S32CC dwmac
glue driver
Hi Krysztof,
Am 30.11.22 um 16:51 schrieb Krzysztof Kozlowski:
> On 28/11/2022 06:49, Chester Lin wrote:
>> Add the DT schema for the DWMAC Ethernet controller on NXP S32 Common
>> Chassis.
>>
>> Signed-off-by: Jan Petrous <jan.petrous@....com>
>> Signed-off-by: Chester Lin <clin@...e.com>
>
> Thank you for your patch. There is something to discuss/improve.
>
>> ---
>>
>> Changes in v2:
>> - Fix schema issues.
>> - Add minItems to clocks & clock-names.
>> - Replace all sgmii/SGMII terms with pcs/PCS.
>>
>> .../bindings/net/nxp,s32cc-dwmac.yaml | 135 ++++++++++++++++++
>> 1 file changed, 135 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml b/Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml
>> new file mode 100644
>> index 000000000000..c6839fd3df40
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/nxp,s32cc-dwmac.yaml
[...]
>> +properties:
>> + compatible:
>> + enum:
>> + - nxp,s32cc-dwmac
>> +
>> + reg:
>> + items:
>> + - description: Main GMAC registers
>> + - description: S32 MAC control registers
>> +
>> + dma-coherent: true
>> +
>> + clocks:
>> + minItems: 5
>
> Why only 5 clocks are required? Receive clocks don't have to be there?
> Is such system - only with clocks for transmit - usable?
>
>> + items:
>> + - description: Main GMAC clock
>> + - description: Peripheral registers clock
>> + - description: Transmit PCS clock
>> + - description: Transmit RGMII clock
>> + - description: Transmit RMII clock
>> + - description: Transmit MII clock
>> + - description: Receive PCS clock
>> + - description: Receive RGMII clock
>> + - description: Receive RMII clock
>> + - description: Receive MII clock
>> + - description:
>> + PTP reference clock. This clock is used for programming the
>> + Timestamp Addend Register. If not passed then the system
>> + clock will be used.
>> +
>> + clock-names:
>> + minItems: 5
>> + items:
>> + - const: stmmaceth
>> + - const: pclk
>> + - const: tx_pcs
>> + - const: tx_rgmii
>> + - const: tx_rmii
>> + - const: tx_mii
>> + - const: rx_pcs
>> + - const: rx_rgmii
>> + - const: rx_rmii
>> + - const: rx_mii
>> + - const: ptp_ref
>> +
>> + tx-fifo-depth:
>> + const: 20480
>> +
>> + rx-fifo-depth:
>> + const: 20480
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - tx-fifo-depth
>> + - rx-fifo-depth
>> + - clocks
>> + - clock-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> + #define S32GEN1_SCMI_CLK_GMAC0_AXI
>> + #define S32GEN1_SCMI_CLK_GMAC0_TX_PCS
>> + #define S32GEN1_SCMI_CLK_GMAC0_TX_RGMII
>> + #define S32GEN1_SCMI_CLK_GMAC0_TX_RMII
>> + #define S32GEN1_SCMI_CLK_GMAC0_TX_MII
>> + #define S32GEN1_SCMI_CLK_GMAC0_RX_PCS
>> + #define S32GEN1_SCMI_CLK_GMAC0_RX_RGMII
>> + #define S32GEN1_SCMI_CLK_GMAC0_RX_RMII
>> + #define S32GEN1_SCMI_CLK_GMAC0_RX_MII
>> + #define S32GEN1_SCMI_CLK_GMAC0_TS
>
> Why defines? Your clock controller is not ready? If so, just use raw
> numbers.
Please compare v1: There is no Linux-driven clock controller here but
rather a fluid SCMI firmware interface. Work towards getting clocks into
a kernel-hosted .dtsi was halted in favor of (downstream) TF-A, which
also explains the ugly examples here and for pinctrl.
Logically there are only 5 input clocks; however due to SCMI not
supporting re-parenting today, some clocks got duplicated at SCMI level.
Andrew appeared to approve of that approach. I still dislike it but
don't have a better proposal that would work today. So the two values
above indeed seem wrong and should be 11 rather than 5.
Cheers,
Andreas
>> +
>> + soc {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + gmac0: ethernet@...3c000 {
>> + compatible = "nxp,s32cc-dwmac";
>> + reg = <0x4033c000 0x2000>, /* gmac IP */
>> + <0x4007C004 0x4>; /* S32 CTRL_STS reg */
>
> Lowercase hex.
>
>> + interrupt-parent = <&gic>;
>> + interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "macirq";
>> + phy-mode = "rgmii-id";
>> + tx-fifo-depth = <20480>;
>> + rx-fifo-depth = <20480>;
>> + dma-coherent;
>> + clocks = <&clks S32GEN1_SCMI_CLK_GMAC0_AXI>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_AXI>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_TX_PCS>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_TX_RGMII>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_TX_RMII>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_TX_MII>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_RX_PCS>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_RX_RGMII>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_RX_RMII>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_RX_MII>,
>> + <&clks S32GEN1_SCMI_CLK_GMAC0_TS>;
>
>
> Best regards,
> Krzysztof
>
--
SUSE Software Solutions Germany GmbH
Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nürnberg)
Powered by blists - more mailing lists