* [PATCH 0/3] can: xilinx_can: Add ECC feature support @ 2023-06-12 11:42 Srinivas Goud 2023-06-12 11:42 ` [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ Srinivas Goud ` (3 more replies) 0 siblings, 4 replies; 22+ messages in thread From: Srinivas Goud @ 2023-06-12 11:42 UTC (permalink / raw) To: wg, mkl, davem, edumazet, kuba, pabeni, gcnu.goud Cc: git, michal.simek, linux-can, linux-arm-kernel, linux-kernel, Srinivas Goud [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="y", Size: 871 bytes --] ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. Part of this feature configuration and counter registers added in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset. Please find more details in PG096 v5.1 document. xlnx,has-ecc is optional property and added to Xilinx CAN Controller node if ECC block enabled in the HW. Driver reports 1bit/2bit ECC errors for FIFO's based on ECC error interrupts and also create debugfs entry for reading all the ECC errors. Srinivas Goud (3): dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ can: xilinx_can: Add ECC support can: xilinx_can: Add debugfs support for ECC .../devicetree/bindings/net/can/xilinx,can.yaml | 5 + drivers/net/can/xilinx_can.c | 169 ++++++++++++++++++++- 2 files changed, 169 insertions(+), 5 deletions(-) -- 2.1.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-12 11:42 [PATCH 0/3] can: xilinx_can: Add ECC feature support Srinivas Goud @ 2023-06-12 11:42 ` Srinivas Goud 2023-06-13 7:52 ` Marc Kleine-Budde 2023-06-13 8:46 ` Krzysztof Kozlowski 2023-06-12 11:42 ` [PATCH 2/3] can: xilinx_can: Add ECC support Srinivas Goud ` (2 subsequent siblings) 3 siblings, 2 replies; 22+ messages in thread From: Srinivas Goud @ 2023-06-12 11:42 UTC (permalink / raw) To: wg, mkl, davem, edumazet, kuba, pabeni, gcnu.goud Cc: git, michal.simek, linux-can, linux-arm-kernel, linux-kernel, Srinivas Goud ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. Part of this feature configuration and counter registers added in IP for 1bit/2bit ECC errors. Please find more details in PG096 v5.1 document. xlnx,has-ecc is optional property and added to Xilinx CAN Controller node if ECC block enabled in the HW. Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> --- Documentation/devicetree/bindings/net/can/xilinx,can.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml index 897d2cb..13503ae 100644 --- a/Documentation/devicetree/bindings/net/can/xilinx,can.yaml +++ b/Documentation/devicetree/bindings/net/can/xilinx,can.yaml @@ -46,6 +46,10 @@ properties: $ref: /schemas/types.yaml#/definitions/uint32 description: CAN Tx mailbox buffer count (CAN FD) + xlnx,has-ecc: + $ref: /schemas/types.yaml#/definitions/flag + description: CAN Tx and Rx fifo ECC enable flag (AXI CAN) + required: - compatible - reg @@ -134,6 +138,7 @@ examples: interrupts = <GIC_SPI 59 IRQ_TYPE_EDGE_RISING>; tx-fifo-depth = <0x40>; rx-fifo-depth = <0x40>; + xlnx,has-ecc; }; - | -- 2.1.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-12 11:42 ` [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ Srinivas Goud @ 2023-06-13 7:52 ` Marc Kleine-Budde 2023-06-14 10:22 ` Goud, Srinivas 2023-06-13 8:46 ` Krzysztof Kozlowski 1 sibling, 1 reply; 22+ messages in thread From: Marc Kleine-Budde @ 2023-06-13 7:52 UTC (permalink / raw) To: Srinivas Goud Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git, michal.simek, linux-can, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 807 bytes --] On 12.06.2023 17:12:55, Srinivas Goud wrote: > ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. > Part of this feature configuration and counter registers > added in IP for 1bit/2bit ECC errors. > Please find more details in PG096 v5.1 document. > > xlnx,has-ecc is optional property and added to Xilinx CAN > Controller node if ECC block enabled in the HW. > > Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> Is there a way to introspect the IP core to check if this feature is compiled in? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-13 7:52 ` Marc Kleine-Budde @ 2023-06-14 10:22 ` Goud, Srinivas 2023-06-14 11:11 ` Krzysztof Kozlowski 0 siblings, 1 reply; 22+ messages in thread From: Goud, Srinivas @ 2023-06-14 10:22 UTC (permalink / raw) To: Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel Hi, >-----Original Message----- >From: Marc Kleine-Budde <mkl@pengutronix.de> >Sent: Tuesday, June 13, 2023 1:23 PM >To: Goud, Srinivas <srinivas.goud@amd.com> >Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com; >kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD- >Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org; >linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property >‘xlnx,has-ecc’ > >On 12.06.2023 17:12:55, Srinivas Goud wrote: >> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. >> Part of this feature configuration and counter registers added in IP >> for 1bit/2bit ECC errors. >> Please find more details in PG096 v5.1 document. >> >> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >> node if ECC block enabled in the HW. >> >> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> > >Is there a way to introspect the IP core to check if this feature is compiled in? There is no way(IP registers) to indicate whether ECC feature is enabled or not. > >Marc > >-- >Pengutronix e.K. | Marc Kleine-Budde | >Embedded Linux | https://www.pengutronix.de | >Vertretung Nürnberg | Phone: +49-5121-206917-129 | >Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | Thanks, Srinivas ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-14 10:22 ` Goud, Srinivas @ 2023-06-14 11:11 ` Krzysztof Kozlowski 2023-06-16 10:13 ` Goud, Srinivas 2023-06-17 7:32 ` Krzysztof Kozlowski 0 siblings, 2 replies; 22+ messages in thread From: Krzysztof Kozlowski @ 2023-06-14 11:11 UTC (permalink / raw) To: Goud, Srinivas, Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel On 14/06/2023 12:22, Goud, Srinivas wrote: > Hi, > >> -----Original Message----- >> From: Marc Kleine-Budde <mkl@pengutronix.de> >> Sent: Tuesday, June 13, 2023 1:23 PM >> To: Goud, Srinivas <srinivas.goud@amd.com> >> Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com; >> kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD- >> Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org; >> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property >> ‘xlnx,has-ecc’ >> >> On 12.06.2023 17:12:55, Srinivas Goud wrote: >>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. >>> Part of this feature configuration and counter registers added in IP >>> for 1bit/2bit ECC errors. >>> Please find more details in PG096 v5.1 document. >>> >>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >>> node if ECC block enabled in the HW. >>> >>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >> >> Is there a way to introspect the IP core to check if this feature is compiled in? > There is no way(IP registers) to indicate whether ECC feature is enabled or not. Isn't this then deductible from compatible? Your binding claims it is only for AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the binding. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-14 11:11 ` Krzysztof Kozlowski @ 2023-06-16 10:13 ` Goud, Srinivas 2023-06-16 10:38 ` Krzysztof Kozlowski 2023-06-17 7:32 ` Krzysztof Kozlowski 1 sibling, 1 reply; 22+ messages in thread From: Goud, Srinivas @ 2023-06-16 10:13 UTC (permalink / raw) To: Krzysztof Kozlowski, Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel Hi >-----Original Message----- >From: Krzysztof Kozlowski <krzk@kernel.org> >Sent: Wednesday, June 14, 2023 4:41 PM >To: Goud, Srinivas <srinivas.goud@amd.com>; Marc Kleine-Budde ><mkl@pengutronix.de> >Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com; >kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD- >Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org; >linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property >‘xlnx,has-ecc’ > >On 14/06/2023 12:22, Goud, Srinivas wrote: >> Hi, >> >>> -----Original Message----- >>> From: Marc Kleine-Budde <mkl@pengutronix.de> >>> Sent: Tuesday, June 13, 2023 1:23 PM >>> To: Goud, Srinivas <srinivas.goud@amd.com> >>> Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com; >>> kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD- >>> Xilinx) <git@amd.com>; michal.simek@xilinx.com; >>> linux-can@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>> linux-kernel@vger.kernel.org >>> Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC >>> property ‘xlnx,has-ecc’ >>> >>> On 12.06.2023 17:12:55, Srinivas Goud wrote: >>>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. >>>> Part of this feature configuration and counter registers added in IP >>>> for 1bit/2bit ECC errors. >>>> Please find more details in PG096 v5.1 document. >>>> >>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >>>> node if ECC block enabled in the HW. >>>> >>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >>> >>> Is there a way to introspect the IP core to check if this feature is compiled in? >> There is no way(IP registers) to indicate whether ECC feature is enabled or >not. > >Isn't this then deductible from compatible? Your binding claims it is only for >AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the >binding. Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is configurable option to the user. ECC is added as optional configuration(enable/disable) feature to the existing AXI CAN. Thanks, Srinivas ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-16 10:13 ` Goud, Srinivas @ 2023-06-16 10:38 ` Krzysztof Kozlowski 2023-06-16 10:44 ` Michal Simek 0 siblings, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2023-06-16 10:38 UTC (permalink / raw) To: Goud, Srinivas, Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel On 16/06/2023 12:13, Goud, Srinivas wrote: >>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >>>>> node if ECC block enabled in the HW. >>>>> >>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >>>> >>>> Is there a way to introspect the IP core to check if this feature is compiled in? >>> There is no way(IP registers) to indicate whether ECC feature is enabled or >> not. >> >> Isn't this then deductible from compatible? Your binding claims it is only for >> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the >> binding. > Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is > configurable option to the user. > ECC is added as optional configuration(enable/disable) feature > to the existing AXI CAN. Why boards would like not to have ECC? I understand that someone told you "make it configurable in DTS", but that's not really a reason for us. Why this is suitable for DTS? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-16 10:38 ` Krzysztof Kozlowski @ 2023-06-16 10:44 ` Michal Simek 2023-06-17 7:31 ` Krzysztof Kozlowski 0 siblings, 1 reply; 22+ messages in thread From: Michal Simek @ 2023-06-16 10:44 UTC (permalink / raw) To: Krzysztof Kozlowski, Goud, Srinivas, Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel On 6/16/23 12:38, Krzysztof Kozlowski wrote: > On 16/06/2023 12:13, Goud, Srinivas wrote: >>>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >>>>>> node if ECC block enabled in the HW. >>>>>> >>>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >>>>> >>>>> Is there a way to introspect the IP core to check if this feature is compiled in? >>>> There is no way(IP registers) to indicate whether ECC feature is enabled or >>> not. >>> >>> Isn't this then deductible from compatible? Your binding claims it is only for >>> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the >>> binding. >> Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is >> configurable option to the user. >> ECC is added as optional configuration(enable/disable) feature >> to the existing AXI CAN. > > Why boards would like not to have ECC? I understand that someone told > you "make it configurable in DTS", but that's not really a reason for > us. Why this is suitable for DTS? Let me jump to this. This is core for programmable logic where HW designers of this IP added couple of feature which can be enabled or disable based on customer need. It means it is not SW option if ECC is enable but it is HW choice if ECC is present in the HW or not. Selection if ECC should be used is up to every customer to decide. We are not able to get information why customers choosing ECC enabled/disabled but I can imagine that with ECC disable less fpga resources are used. Thanks, Michal ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-16 10:44 ` Michal Simek @ 2023-06-17 7:31 ` Krzysztof Kozlowski 2023-06-19 6:37 ` Michal Simek 0 siblings, 1 reply; 22+ messages in thread From: Krzysztof Kozlowski @ 2023-06-17 7:31 UTC (permalink / raw) To: Michal Simek, Goud, Srinivas, Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel On 16/06/2023 12:44, Michal Simek wrote: > > > On 6/16/23 12:38, Krzysztof Kozlowski wrote: >> On 16/06/2023 12:13, Goud, Srinivas wrote: >>>>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >>>>>>> node if ECC block enabled in the HW. >>>>>>> >>>>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >>>>>> >>>>>> Is there a way to introspect the IP core to check if this feature is compiled in? >>>>> There is no way(IP registers) to indicate whether ECC feature is enabled or >>>> not. >>>> >>>> Isn't this then deductible from compatible? Your binding claims it is only for >>>> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the >>>> binding. >>> Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is >>> configurable option to the user. >>> ECC is added as optional configuration(enable/disable) feature >>> to the existing AXI CAN. >> >> Why boards would like not to have ECC? I understand that someone told >> you "make it configurable in DTS", but that's not really a reason for >> us. Why this is suitable for DTS? > > Let me jump to this. This is core for programmable logic where HW designers of > this IP added couple of feature which can be enabled or disable based on > customer need. It means it is not SW option if ECC is enable but it is HW choice > if ECC is present in the HW or not. > Selection if ECC should be used is up to every customer to decide. > We are not able to get information why customers choosing ECC enabled/disabled > but I can imagine that with ECC disable less fpga resources are used. Thanks for the explanation. Apologies for being picky, but you are in minority when adding such properties with true hardware meaning. Most of the submissions of such properties add them to control the bits in register. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-17 7:31 ` Krzysztof Kozlowski @ 2023-06-19 6:37 ` Michal Simek 0 siblings, 0 replies; 22+ messages in thread From: Michal Simek @ 2023-06-19 6:37 UTC (permalink / raw) To: Krzysztof Kozlowski, Goud, Srinivas, Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel On 6/17/23 09:31, Krzysztof Kozlowski wrote: > On 16/06/2023 12:44, Michal Simek wrote: >> >> >> On 6/16/23 12:38, Krzysztof Kozlowski wrote: >>> On 16/06/2023 12:13, Goud, Srinivas wrote: >>>>>>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >>>>>>>> node if ECC block enabled in the HW. >>>>>>>> >>>>>>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >>>>>>> >>>>>>> Is there a way to introspect the IP core to check if this feature is compiled in? >>>>>> There is no way(IP registers) to indicate whether ECC feature is enabled or >>>>> not. >>>>> >>>>> Isn't this then deductible from compatible? Your binding claims it is only for >>>>> AXI CAN, so xlnx,axi-can-1.00.a, even though you did not restrict it in the >>>>> binding. >>>> Agree it is only for AXI CAN(xlnx,axi-can-1.00.a) but ECC feature is >>>> configurable option to the user. >>>> ECC is added as optional configuration(enable/disable) feature >>>> to the existing AXI CAN. >>> >>> Why boards would like not to have ECC? I understand that someone told >>> you "make it configurable in DTS", but that's not really a reason for >>> us. Why this is suitable for DTS? >> >> Let me jump to this. This is core for programmable logic where HW designers of >> this IP added couple of feature which can be enabled or disable based on >> customer need. It means it is not SW option if ECC is enable but it is HW choice >> if ECC is present in the HW or not. >> Selection if ECC should be used is up to every customer to decide. >> We are not able to get information why customers choosing ECC enabled/disabled >> but I can imagine that with ECC disable less fpga resources are used. > > Thanks for the explanation. Apologies for being picky, but you are in > minority when adding such properties with true hardware meaning. Most of > the submissions of such properties add them to control the bits in register. No issue at all. We are talking to HW designers to change their mindset and help us with automatic detection of these features but truth is that every such a feature means fpga resources that's why they are trying to avoid it to save them and help customers to fit as much as possible to their fpgas. Because bigger fpga is more expensive and also consumes more power. Thanks, Michal ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-14 11:11 ` Krzysztof Kozlowski 2023-06-16 10:13 ` Goud, Srinivas @ 2023-06-17 7:32 ` Krzysztof Kozlowski 1 sibling, 0 replies; 22+ messages in thread From: Krzysztof Kozlowski @ 2023-06-17 7:32 UTC (permalink / raw) To: Goud, Srinivas, Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel On 14/06/2023 13:11, Krzysztof Kozlowski wrote: > On 14/06/2023 12:22, Goud, Srinivas wrote: >> Hi, >> >>> -----Original Message----- >>> From: Marc Kleine-Budde <mkl@pengutronix.de> >>> Sent: Tuesday, June 13, 2023 1:23 PM >>> To: Goud, Srinivas <srinivas.goud@amd.com> >>> Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com; >>> kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD- >>> Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org; >>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >>> Subject: Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property >>> ‘xlnx,has-ecc’ >>> >>> On 12.06.2023 17:12:55, Srinivas Goud wrote: >>>> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. >>>> Part of this feature configuration and counter registers added in IP >>>> for 1bit/2bit ECC errors. >>>> Please find more details in PG096 v5.1 document. >>>> >>>> xlnx,has-ecc is optional property and added to Xilinx CAN Controller >>>> node if ECC block enabled in the HW. >>>> >>>> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >>> >>> Is there a way to introspect the IP core to check if this feature is compiled in? >> There is no way(IP registers) to indicate whether ECC feature is enabled or not. > > Isn't this then deductible from compatible? Your binding claims it is > only for AXI CAN, so xlnx,axi-can-1.00.a, even though you did not > restrict it in the binding. BTW, this is not an ACK, because this was not tested by automation. I don't understand why get_maintainers.pl are so tricky to use, but nevertheless I require resend to satisfy automation. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ 2023-06-12 11:42 ` [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ Srinivas Goud 2023-06-13 7:52 ` Marc Kleine-Budde @ 2023-06-13 8:46 ` Krzysztof Kozlowski 1 sibling, 0 replies; 22+ messages in thread From: Krzysztof Kozlowski @ 2023-06-13 8:46 UTC (permalink / raw) To: Srinivas Goud, wg, mkl, davem, edumazet, kuba, pabeni, gcnu.goud Cc: git, michal.simek, linux-can, linux-arm-kernel, linux-kernel On 12/06/2023 13:42, Srinivas Goud wrote: > ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. > Part of this feature configuration and counter registers > added in IP for 1bit/2bit ECC errors. > Please find more details in PG096 v5.1 document. > > xlnx,has-ecc is optional property and added to Xilinx CAN > Controller node if ECC block enabled in the HW. 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. You missed at least DT list (maybe more), so this won't be tested by our tools. Performing review on untested code might be a waste of time, thus I will skip this patch entirely till you follow the process allowing the patch to be tested. Please kindly resend and include all necessary To/Cc entries. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] can: xilinx_can: Add ECC support 2023-06-12 11:42 [PATCH 0/3] can: xilinx_can: Add ECC feature support Srinivas Goud 2023-06-12 11:42 ` [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ Srinivas Goud @ 2023-06-12 11:42 ` Srinivas Goud 2023-06-13 11:30 ` Marc Kleine-Budde 2023-06-16 11:06 ` Marc Kleine-Budde 2023-06-12 11:42 ` [PATCH 3/3] can: xilinx_can: Add debugfs support for ECC Srinivas Goud 2023-06-16 11:12 ` [PATCH 0/3] can: xilinx_can: Add ECC feature support Marc Kleine-Budde 3 siblings, 2 replies; 22+ messages in thread From: Srinivas Goud @ 2023-06-12 11:42 UTC (permalink / raw) To: wg, mkl, davem, edumazet, kuba, pabeni, gcnu.goud Cc: git, michal.simek, linux-can, linux-arm-kernel, linux-kernel, Srinivas Goud Add ECC support for Xilinx CAN Controller, so this driver reports 1bit/2bit ECC errors for FIFO's based on ECC error interrupt. ECC feature for Xilinx CAN Controller selected through 'xlnx,has-ecc' DT property Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> --- drivers/net/can/xilinx_can.c | 109 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 5 deletions(-) diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 43c812e..311e435 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -63,6 +63,12 @@ enum xcan_reg { XCAN_RXMSG_2_BASE_OFFSET = 0x2100, /* RX Message Space */ XCAN_AFR_2_MASK_OFFSET = 0x0A00, /* Acceptance Filter MASK */ XCAN_AFR_2_ID_OFFSET = 0x0A04, /* Acceptance Filter ID */ + + /* only on AXI CAN cores */ + XCAN_ECC_CFG_OFFSET = 0xC8, /* ECC Configuration */ + XCAN_TXTLFIFO_ECC_OFFSET = 0xCC, /* TXTL FIFO ECC error counter */ + XCAN_TXOLFIFO_ECC_OFFSET = 0xD0, /* TXOL FIFO ECC error counter */ + XCAN_RXFIFO_ECC_OFFSET = 0xD4, /* RX FIFO ECC error counter */ }; #define XCAN_FRAME_ID_OFFSET(frame_base) ((frame_base) + 0x00) @@ -135,6 +141,17 @@ enum xcan_reg { #define XCAN_2_FSR_RI_MASK 0x0000003F /* RX Read Index */ #define XCAN_DLCR_EDL_MASK 0x08000000 /* EDL Mask in DLC */ #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC */ +#define XCAN_IXR_E2BERX_MASK BIT(23) /* RX FIFO two bit ECC error */ +#define XCAN_IXR_E1BERX_MASK BIT(22) /* RX FIFO one bit ECC error */ +#define XCAN_IXR_E2BETXOL_MASK BIT(21) /* TXOL FIFO two bit ECC error */ +#define XCAN_IXR_E1BETXOL_MASK BIT(20) /* TXOL FIFO One bit ECC error */ +#define XCAN_IXR_E2BETXTL_MASK BIT(19) /* TXTL FIFO Two bit ECC error */ +#define XCAN_IXR_E1BETXTL_MASK BIT(18) /* TXTL FIFO One bit ECC error */ +#define XCAN_ECC_CFG_REECRX_MASK BIT(2) /* Reset RX FIFO ECC error counters */ +#define XCAN_ECC_CFG_REECTXOL_MASK BIT(1) /* Reset TXOL FIFO ECC error counters */ +#define XCAN_ECC_CFG_REECTXTL_MASK BIT(0) /* Reset TXTL FIFO ECC error counters */ +#define XCAN_ECC_1BIT_CNT_MASK GENMASK(15, 0) /* FIFO ECC 1bit count mask */ +#define XCAN_ECC_2BIT_CNT_MASK GENMASK(31, 16) /* FIFO ECC 2bit count mask */ /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ #define XCAN_BRPR_TDC_ENABLE BIT(16) /* Transmitter Delay Compensation (TDC) Enable */ @@ -198,6 +215,13 @@ struct xcan_devtype_data { * @bus_clk: Pointer to struct clk * @can_clk: Pointer to struct clk * @devtype: Device type specific constants + * @ecc_enable: ECC enable flag + * @ecc_2bit_rxfifo_cnt: RXFIFO 2bit ECC count + * @ecc_1bit_rxfifo_cnt: RXFIFO 1bit ECC count + * @ecc_2bit_txolfifo_cnt: TXOLFIFO 2bit ECC count + * @ecc_1bit_txolfifo_cnt: TXOLFIFO 1bit ECC count + * @ecc_2bit_txtlfifo_cnt: TXTLFIFO 2bit ECC count + * @ecc_1bit_txtlfifo_cnt: TXTLFIFO 1bit ECC count */ struct xcan_priv { struct can_priv can; @@ -215,6 +239,13 @@ struct xcan_priv { struct clk *bus_clk; struct clk *can_clk; struct xcan_devtype_data devtype; + bool ecc_enable; + u32 ecc_2bit_rxfifo_cnt; + u32 ecc_1bit_rxfifo_cnt; + u32 ecc_2bit_txolfifo_cnt; + u32 ecc_1bit_txolfifo_cnt; + u32 ecc_2bit_txtlfifo_cnt; + u32 ecc_1bit_txtlfifo_cnt; }; /* CAN Bittiming constants as per Xilinx CAN specs */ @@ -517,6 +548,11 @@ static int xcan_chip_start(struct net_device *ndev) XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | XCAN_IXR_ARBLST_MASK | xcan_rx_int_mask(priv); + if (priv->ecc_enable) + ier |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK | + XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK | + XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK; + if (priv->devtype.flags & XCAN_FLAG_RXMNF) ier |= XCAN_IXR_RXMNF_MASK; @@ -1121,6 +1157,56 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr) priv->can.can_stats.bus_error++; } + if (priv->ecc_enable) { + u32 reg_ecc; + + reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET); + if (isr & XCAN_IXR_E2BERX_MASK) { + priv->ecc_2bit_rxfifo_cnt += + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc); + netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count %d\n", + __func__, priv->ecc_2bit_rxfifo_cnt); + } + if (isr & XCAN_IXR_E1BERX_MASK) { + priv->ecc_1bit_rxfifo_cnt += reg_ecc & + XCAN_ECC_1BIT_CNT_MASK; + netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count %d\n", + __func__, priv->ecc_1bit_rxfifo_cnt); + } + + reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET); + if (isr & XCAN_IXR_E2BETXOL_MASK) { + priv->ecc_2bit_txolfifo_cnt += + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc); + netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count %d\n", + __func__, priv->ecc_2bit_txolfifo_cnt); + } + if (isr & XCAN_IXR_E1BETXOL_MASK) { + priv->ecc_1bit_txolfifo_cnt += reg_ecc & + XCAN_ECC_1BIT_CNT_MASK; + netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count %d\n", + __func__, priv->ecc_1bit_txolfifo_cnt); + } + + reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET); + if (isr & XCAN_IXR_E2BETXTL_MASK) { + priv->ecc_2bit_txtlfifo_cnt += + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc); + netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count %d\n", + __func__, priv->ecc_2bit_txtlfifo_cnt); + } + if (isr & XCAN_IXR_E1BETXTL_MASK) { + priv->ecc_1bit_txtlfifo_cnt += reg_ecc & + XCAN_ECC_1BIT_CNT_MASK; + netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count %d\n", + __func__, priv->ecc_1bit_txtlfifo_cnt); + } + + /* Reset FIFO ECC counters */ + priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK | + XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK); + } + if (cf.can_id) { struct can_frame *skb_cf; struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf); @@ -1348,9 +1434,8 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id) { struct net_device *ndev = (struct net_device *)dev_id; struct xcan_priv *priv = netdev_priv(ndev); - u32 isr, ier; - u32 isr_errors; u32 rx_int_mask = xcan_rx_int_mask(priv); + u32 isr, ier, isr_errors, mask; /* Get the interrupt status from Xilinx CAN */ isr = priv->read_reg(priv, XCAN_ISR_OFFSET); @@ -1368,10 +1453,18 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id) if (isr & XCAN_IXR_TXOK_MASK) xcan_tx_interrupt(ndev, isr); + mask = XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | + XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK | + XCAN_IXR_RXMNF_MASK; + + if (priv->ecc_enable) + mask |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK | + XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK | + XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK; + /* Check for the type of error interrupt and Processing it */ - isr_errors = isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | - XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK | - XCAN_IXR_RXMNF_MASK); + isr_errors = isr & mask; + if (isr_errors) { priv->write_reg(priv, XCAN_ICR_OFFSET, isr_errors); xcan_err_interrupt(ndev, isr); @@ -1783,6 +1876,7 @@ static int xcan_probe(struct platform_device *pdev) return -ENOMEM; priv = netdev_priv(ndev); + priv->ecc_enable = of_property_read_bool(pdev->dev.of_node, "xlnx,has-ecc"); priv->dev = &pdev->dev; priv->can.bittiming_const = devtype->bittiming_const; priv->can.do_set_mode = xcan_do_set_mode; @@ -1880,6 +1974,11 @@ static int xcan_probe(struct platform_device *pdev) priv->reg_base, ndev->irq, priv->can.clock.freq, hw_tx_max, priv->tx_max); + if (priv->ecc_enable) + /* Reset FIFO ECC counters */ + priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK | + XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK); + return 0; err_disableclks: -- 2.1.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] can: xilinx_can: Add ECC support 2023-06-12 11:42 ` [PATCH 2/3] can: xilinx_can: Add ECC support Srinivas Goud @ 2023-06-13 11:30 ` Marc Kleine-Budde 2023-07-21 5:23 ` Goud, Srinivas 2023-06-16 11:06 ` Marc Kleine-Budde 1 sibling, 1 reply; 22+ messages in thread From: Marc Kleine-Budde @ 2023-06-13 11:30 UTC (permalink / raw) To: Srinivas Goud Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git, michal.simek, linux-can, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 9397 bytes --] On 12.06.2023 17:12:56, Srinivas Goud wrote: > Add ECC support for Xilinx CAN Controller, so this driver reports > 1bit/2bit ECC errors for FIFO's based on ECC error interrupt. > ECC feature for Xilinx CAN Controller selected through > 'xlnx,has-ecc' DT property > > Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> > --- > drivers/net/can/xilinx_can.c | 109 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 104 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index 43c812e..311e435 100644 > --- a/drivers/net/can/xilinx_can.c > +++ b/drivers/net/can/xilinx_can.c > @@ -63,6 +63,12 @@ enum xcan_reg { > XCAN_RXMSG_2_BASE_OFFSET = 0x2100, /* RX Message Space */ > XCAN_AFR_2_MASK_OFFSET = 0x0A00, /* Acceptance Filter MASK */ > XCAN_AFR_2_ID_OFFSET = 0x0A04, /* Acceptance Filter ID */ > + > + /* only on AXI CAN cores */ > + XCAN_ECC_CFG_OFFSET = 0xC8, /* ECC Configuration */ > + XCAN_TXTLFIFO_ECC_OFFSET = 0xCC, /* TXTL FIFO ECC error counter */ > + XCAN_TXOLFIFO_ECC_OFFSET = 0xD0, /* TXOL FIFO ECC error counter */ > + XCAN_RXFIFO_ECC_OFFSET = 0xD4, /* RX FIFO ECC error counter */ > }; > > #define XCAN_FRAME_ID_OFFSET(frame_base) ((frame_base) + 0x00) > @@ -135,6 +141,17 @@ enum xcan_reg { > #define XCAN_2_FSR_RI_MASK 0x0000003F /* RX Read Index */ > #define XCAN_DLCR_EDL_MASK 0x08000000 /* EDL Mask in DLC */ > #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC */ > +#define XCAN_IXR_E2BERX_MASK BIT(23) /* RX FIFO two bit ECC error */ > +#define XCAN_IXR_E1BERX_MASK BIT(22) /* RX FIFO one bit ECC error */ > +#define XCAN_IXR_E2BETXOL_MASK BIT(21) /* TXOL FIFO two bit ECC error */ > +#define XCAN_IXR_E1BETXOL_MASK BIT(20) /* TXOL FIFO One bit ECC error */ > +#define XCAN_IXR_E2BETXTL_MASK BIT(19) /* TXTL FIFO Two bit ECC error */ > +#define XCAN_IXR_E1BETXTL_MASK BIT(18) /* TXTL FIFO One bit ECC error */ > +#define XCAN_ECC_CFG_REECRX_MASK BIT(2) /* Reset RX FIFO ECC error counters */ > +#define XCAN_ECC_CFG_REECTXOL_MASK BIT(1) /* Reset TXOL FIFO ECC error counters */ > +#define XCAN_ECC_CFG_REECTXTL_MASK BIT(0) /* Reset TXTL FIFO ECC error counters */ > +#define XCAN_ECC_1BIT_CNT_MASK GENMASK(15, 0) /* FIFO ECC 1bit count mask */ > +#define XCAN_ECC_2BIT_CNT_MASK GENMASK(31, 16) /* FIFO ECC 2bit count mask */ > > /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ > #define XCAN_BRPR_TDC_ENABLE BIT(16) /* Transmitter Delay Compensation (TDC) Enable */ > @@ -198,6 +215,13 @@ struct xcan_devtype_data { > * @bus_clk: Pointer to struct clk > * @can_clk: Pointer to struct clk > * @devtype: Device type specific constants > + * @ecc_enable: ECC enable flag > + * @ecc_2bit_rxfifo_cnt: RXFIFO 2bit ECC count > + * @ecc_1bit_rxfifo_cnt: RXFIFO 1bit ECC count > + * @ecc_2bit_txolfifo_cnt: TXOLFIFO 2bit ECC count > + * @ecc_1bit_txolfifo_cnt: TXOLFIFO 1bit ECC count > + * @ecc_2bit_txtlfifo_cnt: TXTLFIFO 2bit ECC count > + * @ecc_1bit_txtlfifo_cnt: TXTLFIFO 1bit ECC count > */ > struct xcan_priv { > struct can_priv can; > @@ -215,6 +239,13 @@ struct xcan_priv { > struct clk *bus_clk; > struct clk *can_clk; > struct xcan_devtype_data devtype; > + bool ecc_enable; > + u32 ecc_2bit_rxfifo_cnt; > + u32 ecc_1bit_rxfifo_cnt; > + u32 ecc_2bit_txolfifo_cnt; > + u32 ecc_1bit_txolfifo_cnt; > + u32 ecc_2bit_txtlfifo_cnt; > + u32 ecc_1bit_txtlfifo_cnt; > }; > > /* CAN Bittiming constants as per Xilinx CAN specs */ > @@ -517,6 +548,11 @@ static int xcan_chip_start(struct net_device *ndev) > XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | > XCAN_IXR_ARBLST_MASK | xcan_rx_int_mask(priv); > > + if (priv->ecc_enable) > + ier |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK | > + XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK | > + XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK; > + > if (priv->devtype.flags & XCAN_FLAG_RXMNF) > ier |= XCAN_IXR_RXMNF_MASK; > > @@ -1121,6 +1157,56 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr) > priv->can.can_stats.bus_error++; > } > > + if (priv->ecc_enable) { > + u32 reg_ecc; > + > + reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET); > + if (isr & XCAN_IXR_E2BERX_MASK) { > + priv->ecc_2bit_rxfifo_cnt += > + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc); > + netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count %d\n", > + __func__, priv->ecc_2bit_rxfifo_cnt); > + } > + if (isr & XCAN_IXR_E1BERX_MASK) { > + priv->ecc_1bit_rxfifo_cnt += reg_ecc & > + XCAN_ECC_1BIT_CNT_MASK; Please use FIELD_GET here, too. > + netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count %d\n", > + __func__, priv->ecc_1bit_rxfifo_cnt); > + } > + > + reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET); > + if (isr & XCAN_IXR_E2BETXOL_MASK) { > + priv->ecc_2bit_txolfifo_cnt += > + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc); > + netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count %d\n", > + __func__, priv->ecc_2bit_txolfifo_cnt); > + } > + if (isr & XCAN_IXR_E1BETXOL_MASK) { > + priv->ecc_1bit_txolfifo_cnt += reg_ecc & > + XCAN_ECC_1BIT_CNT_MASK; same here > + netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count %d\n", > + __func__, priv->ecc_1bit_txolfifo_cnt); > + } > + > + reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET); > + if (isr & XCAN_IXR_E2BETXTL_MASK) { > + priv->ecc_2bit_txtlfifo_cnt += > + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, reg_ecc); > + netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count %d\n", > + __func__, priv->ecc_2bit_txtlfifo_cnt); > + } > + if (isr & XCAN_IXR_E1BETXTL_MASK) { > + priv->ecc_1bit_txtlfifo_cnt += reg_ecc & > + XCAN_ECC_1BIT_CNT_MASK; same here > + netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count %d\n", > + __func__, priv->ecc_1bit_txtlfifo_cnt); > + } > + > + /* Reset FIFO ECC counters */ > + priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK | > + XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK); This is racy - you will lose events that occur between reading the register value and clearing the register. You can save the old value and add the difference between the new and the old value to the total counter. What happens when the counter overflows? The following pseudocode should handle the u16 counter rolling over to 0: u16 old, new; s16 inc; u64 total; ... inc = (s16)(new - old); if (inc < 0) /* error handling */ total += inc; BTW: When converting to ethtool statistics, a lock is required to keep the u64 values correct on 32-bit systems and the individual statistics consistent. > + } > + > if (cf.can_id) { > struct can_frame *skb_cf; > struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf); > @@ -1348,9 +1434,8 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id) > { > struct net_device *ndev = (struct net_device *)dev_id; > struct xcan_priv *priv = netdev_priv(ndev); > - u32 isr, ier; > - u32 isr_errors; > u32 rx_int_mask = xcan_rx_int_mask(priv); > + u32 isr, ier, isr_errors, mask; > > /* Get the interrupt status from Xilinx CAN */ > isr = priv->read_reg(priv, XCAN_ISR_OFFSET); > @@ -1368,10 +1453,18 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id) > if (isr & XCAN_IXR_TXOK_MASK) > xcan_tx_interrupt(ndev, isr); > > + mask = XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | > + XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK | > + XCAN_IXR_RXMNF_MASK; > + > + if (priv->ecc_enable) > + mask |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK | > + XCAN_IXR_E2BETXOL_MASK | XCAN_IXR_E1BETXOL_MASK | > + XCAN_IXR_E2BETXTL_MASK | XCAN_IXR_E1BETXTL_MASK; > + > /* Check for the type of error interrupt and Processing it */ > - isr_errors = isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | > - XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK | > - XCAN_IXR_RXMNF_MASK); > + isr_errors = isr & mask; > + nitpick: don't add this extra newline > if (isr_errors) { > priv->write_reg(priv, XCAN_ICR_OFFSET, isr_errors); > xcan_err_interrupt(ndev, isr); > @@ -1783,6 +1876,7 @@ static int xcan_probe(struct platform_device *pdev) > return -ENOMEM; > > priv = netdev_priv(ndev); > + priv->ecc_enable = of_property_read_bool(pdev->dev.of_node, "xlnx,has-ecc"); > priv->dev = &pdev->dev; > priv->can.bittiming_const = devtype->bittiming_const; > priv->can.do_set_mode = xcan_do_set_mode; > @@ -1880,6 +1974,11 @@ static int xcan_probe(struct platform_device *pdev) > priv->reg_base, ndev->irq, priv->can.clock.freq, > hw_tx_max, priv->tx_max); > > + if (priv->ecc_enable) > + /* Reset FIFO ECC counters */ > + priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK | > + XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK); > + > return 0; > > err_disableclks: > -- > 2.1.1 > > -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 2/3] can: xilinx_can: Add ECC support 2023-06-13 11:30 ` Marc Kleine-Budde @ 2023-07-21 5:23 ` Goud, Srinivas 2023-07-24 8:54 ` Marc Kleine-Budde 0 siblings, 1 reply; 22+ messages in thread From: Goud, Srinivas @ 2023-07-21 5:23 UTC (permalink / raw) To: Marc Kleine-Budde Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel Hi, >-----Original Message----- >From: Marc Kleine-Budde <mkl@pengutronix.de> >Sent: Tuesday, June 13, 2023 5:00 PM >To: Goud, Srinivas <srinivas.goud@amd.com> >Cc: wg@grandegger.com; davem@davemloft.net; edumazet@google.com; >kuba@kernel.org; pabeni@redhat.com; gcnu.goud@gmail.com; git (AMD- >Xilinx) <git@amd.com>; michal.simek@xilinx.com; linux-can@vger.kernel.org; >linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org >Subject: Re: [PATCH 2/3] can: xilinx_can: Add ECC support > >On 12.06.2023 17:12:56, Srinivas Goud wrote: >> Add ECC support for Xilinx CAN Controller, so this driver reports >> 1bit/2bit ECC errors for FIFO's based on ECC error interrupt. >> ECC feature for Xilinx CAN Controller selected through 'xlnx,has-ecc' >> DT property >> >> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >> --- >> drivers/net/can/xilinx_can.c | 109 >> +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 104 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/can/xilinx_can.c >> b/drivers/net/can/xilinx_can.c index 43c812e..311e435 100644 >> --- a/drivers/net/can/xilinx_can.c >> +++ b/drivers/net/can/xilinx_can.c >> @@ -63,6 +63,12 @@ enum xcan_reg { >> XCAN_RXMSG_2_BASE_OFFSET = 0x2100, /* RX Message Space */ >> XCAN_AFR_2_MASK_OFFSET = 0x0A00, /* Acceptance Filter MASK */ >> XCAN_AFR_2_ID_OFFSET = 0x0A04, /* Acceptance Filter ID */ >> + >> + /* only on AXI CAN cores */ >> + XCAN_ECC_CFG_OFFSET = 0xC8, /* ECC Configuration */ >> + XCAN_TXTLFIFO_ECC_OFFSET = 0xCC, /* TXTL FIFO ECC error counter >*/ >> + XCAN_TXOLFIFO_ECC_OFFSET = 0xD0, /* TXOL FIFO ECC error counter >*/ >> + XCAN_RXFIFO_ECC_OFFSET = 0xD4, /* RX FIFO ECC error >counter */ >> }; >> >> #define XCAN_FRAME_ID_OFFSET(frame_base) ((frame_base) + 0x00) >> @@ -135,6 +141,17 @@ enum xcan_reg { >> #define XCAN_2_FSR_RI_MASK 0x0000003F /* RX Read Index >*/ >> #define XCAN_DLCR_EDL_MASK 0x08000000 /* EDL Mask in >DLC */ >> #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in >DLC */ >> +#define XCAN_IXR_E2BERX_MASK BIT(23) /* RX FIFO two bit ECC >error */ >> +#define XCAN_IXR_E1BERX_MASK BIT(22) /* RX FIFO one bit ECC >error */ >> +#define XCAN_IXR_E2BETXOL_MASK BIT(21) /* TXOL FIFO two bit >ECC error */ >> +#define XCAN_IXR_E1BETXOL_MASK BIT(20) /* TXOL FIFO One bit >ECC error */ >> +#define XCAN_IXR_E2BETXTL_MASK BIT(19) /* TXTL FIFO Two bit >ECC error */ >> +#define XCAN_IXR_E1BETXTL_MASK BIT(18) /* TXTL FIFO One bit >ECC error */ >> +#define XCAN_ECC_CFG_REECRX_MASK BIT(2) /* Reset RX FIFO ECC >error counters */ >> +#define XCAN_ECC_CFG_REECTXOL_MASK BIT(1) /* Reset TXOL FIFO ECC >error counters */ >> +#define XCAN_ECC_CFG_REECTXTL_MASK BIT(0) /* Reset TXTL FIFO ECC >error counters */ >> +#define XCAN_ECC_1BIT_CNT_MASK GENMASK(15, 0) /* >FIFO ECC 1bit count mask */ >> +#define XCAN_ECC_2BIT_CNT_MASK GENMASK(31, 16) /* >FIFO ECC 2bit count mask */ >> >> /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ >> #define XCAN_BRPR_TDC_ENABLE BIT(16) /* Transmitter Delay >Compensation (TDC) Enable */ >> @@ -198,6 +215,13 @@ struct xcan_devtype_data { >> * @bus_clk: Pointer to struct clk >> * @can_clk: Pointer to struct clk >> * @devtype: Device type specific constants >> + * @ecc_enable: ECC enable flag >> + * @ecc_2bit_rxfifo_cnt: RXFIFO 2bit ECC count >> + * @ecc_1bit_rxfifo_cnt: RXFIFO 1bit ECC count >> + * @ecc_2bit_txolfifo_cnt: TXOLFIFO 2bit ECC count >> + * @ecc_1bit_txolfifo_cnt: TXOLFIFO 1bit ECC count >> + * @ecc_2bit_txtlfifo_cnt: TXTLFIFO 2bit ECC count >> + * @ecc_1bit_txtlfifo_cnt: TXTLFIFO 1bit ECC count >> */ >> struct xcan_priv { >> struct can_priv can; >> @@ -215,6 +239,13 @@ struct xcan_priv { >> struct clk *bus_clk; >> struct clk *can_clk; >> struct xcan_devtype_data devtype; >> + bool ecc_enable; >> + u32 ecc_2bit_rxfifo_cnt; >> + u32 ecc_1bit_rxfifo_cnt; >> + u32 ecc_2bit_txolfifo_cnt; >> + u32 ecc_1bit_txolfifo_cnt; >> + u32 ecc_2bit_txtlfifo_cnt; >> + u32 ecc_1bit_txtlfifo_cnt; >> }; >> >> /* CAN Bittiming constants as per Xilinx CAN specs */ @@ -517,6 >> +548,11 @@ static int xcan_chip_start(struct net_device *ndev) >> XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | >> XCAN_IXR_ARBLST_MASK | xcan_rx_int_mask(priv); >> >> + if (priv->ecc_enable) >> + ier |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK | >> + XCAN_IXR_E2BETXOL_MASK | >XCAN_IXR_E1BETXOL_MASK | >> + XCAN_IXR_E2BETXTL_MASK | >XCAN_IXR_E1BETXTL_MASK; >> + >> if (priv->devtype.flags & XCAN_FLAG_RXMNF) >> ier |= XCAN_IXR_RXMNF_MASK; >> >> @@ -1121,6 +1157,56 @@ static void xcan_err_interrupt(struct net_device >*ndev, u32 isr) >> priv->can.can_stats.bus_error++; >> } >> >> + if (priv->ecc_enable) { >> + u32 reg_ecc; >> + >> + reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET); >> + if (isr & XCAN_IXR_E2BERX_MASK) { >> + priv->ecc_2bit_rxfifo_cnt += >> + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, >reg_ecc); >> + netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count >%d\n", >> + __func__, priv->ecc_2bit_rxfifo_cnt); >> + } >> + if (isr & XCAN_IXR_E1BERX_MASK) { >> + priv->ecc_1bit_rxfifo_cnt += reg_ecc & >> + XCAN_ECC_1BIT_CNT_MASK; > >Please use FIELD_GET here, too. > >> + netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count >%d\n", >> + __func__, priv->ecc_1bit_rxfifo_cnt); >> + } >> + >> + reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET); >> + if (isr & XCAN_IXR_E2BETXOL_MASK) { >> + priv->ecc_2bit_txolfifo_cnt += >> + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, >reg_ecc); >> + netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count >%d\n", >> + __func__, priv->ecc_2bit_txolfifo_cnt); >> + } >> + if (isr & XCAN_IXR_E1BETXOL_MASK) { >> + priv->ecc_1bit_txolfifo_cnt += reg_ecc & >> + XCAN_ECC_1BIT_CNT_MASK; > >same here > >> + netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count >%d\n", >> + __func__, priv->ecc_1bit_txolfifo_cnt); >> + } >> + >> + reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET); >> + if (isr & XCAN_IXR_E2BETXTL_MASK) { >> + priv->ecc_2bit_txtlfifo_cnt += >> + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, >reg_ecc); >> + netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count >%d\n", >> + __func__, priv->ecc_2bit_txtlfifo_cnt); >> + } >> + if (isr & XCAN_IXR_E1BETXTL_MASK) { >> + priv->ecc_1bit_txtlfifo_cnt += reg_ecc & >> + XCAN_ECC_1BIT_CNT_MASK; > >same here > >> + netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count >%d\n", >> + __func__, priv->ecc_1bit_txtlfifo_cnt); >> + } >> + >> + /* Reset FIFO ECC counters */ >> + priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, >XCAN_ECC_CFG_REECRX_MASK | >> + XCAN_ECC_CFG_REECTXOL_MASK | >XCAN_ECC_CFG_REECTXTL_MASK); > >This is racy - you will lose events that occur between reading the register value >and clearing the register. You can save the old value and add the difference >between the new and the old value to the total counter. What happens when >the counter overflows? The following pseudocode should handle the u16 >counter rolling over to 0: As per IP specifications when counter reaching maximum value of 0xFFFF will stays there until reset. Not sure we can avoid this race condition completely, as we need to reset the counters after reaching the 0xFFFF to avoid losing the events. To minimize the race condition, we can reset the counters after reaching the events to 0xFFFF instead of resetting for every interrupt. Can you please suggest if we can go this approach. Regards, Srinivas > > u16 old, new; > s16 inc; > u64 total; > > ... > > inc = (s16)(new - old); > if (inc < 0) > /* error handling */ > total += inc; > >BTW: When converting to ethtool statistics, a lock is required to keep the u64 >values correct on 32-bit systems and the individual statistics consistent. > >> + } >> + >> if (cf.can_id) { >> struct can_frame *skb_cf; >> struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf); @@ - >1348,9 >> +1434,8 @@ static irqreturn_t xcan_interrupt(int irq, void *dev_id) { >> struct net_device *ndev = (struct net_device *)dev_id; >> struct xcan_priv *priv = netdev_priv(ndev); >> - u32 isr, ier; >> - u32 isr_errors; >> u32 rx_int_mask = xcan_rx_int_mask(priv); >> + u32 isr, ier, isr_errors, mask; >> >> /* Get the interrupt status from Xilinx CAN */ >> isr = priv->read_reg(priv, XCAN_ISR_OFFSET); @@ -1368,10 +1453,18 >@@ >> static irqreturn_t xcan_interrupt(int irq, void *dev_id) >> if (isr & XCAN_IXR_TXOK_MASK) >> xcan_tx_interrupt(ndev, isr); >> >> + mask = XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK | >> + XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK | >> + XCAN_IXR_RXMNF_MASK; >> + >> + if (priv->ecc_enable) >> + mask |= XCAN_IXR_E2BERX_MASK | XCAN_IXR_E1BERX_MASK >| >> + XCAN_IXR_E2BETXOL_MASK | >XCAN_IXR_E1BETXOL_MASK | >> + XCAN_IXR_E2BETXTL_MASK | >XCAN_IXR_E1BETXTL_MASK; >> + >> /* Check for the type of error interrupt and Processing it */ >> - isr_errors = isr & (XCAN_IXR_ERROR_MASK | >XCAN_IXR_RXOFLW_MASK | >> - XCAN_IXR_BSOFF_MASK | XCAN_IXR_ARBLST_MASK >| >> - XCAN_IXR_RXMNF_MASK); >> + isr_errors = isr & mask; >> + > >nitpick: don't add this extra newline > >> if (isr_errors) { >> priv->write_reg(priv, XCAN_ICR_OFFSET, isr_errors); >> xcan_err_interrupt(ndev, isr); >> @@ -1783,6 +1876,7 @@ static int xcan_probe(struct platform_device >*pdev) >> return -ENOMEM; >> >> priv = netdev_priv(ndev); >> + priv->ecc_enable = of_property_read_bool(pdev->dev.of_node, >> +"xlnx,has-ecc"); >> priv->dev = &pdev->dev; >> priv->can.bittiming_const = devtype->bittiming_const; >> priv->can.do_set_mode = xcan_do_set_mode; @@ -1880,6 +1974,11 >@@ >> static int xcan_probe(struct platform_device *pdev) >> priv->reg_base, ndev->irq, priv->can.clock.freq, >> hw_tx_max, priv->tx_max); >> >> + if (priv->ecc_enable) >> + /* Reset FIFO ECC counters */ >> + priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, >XCAN_ECC_CFG_REECRX_MASK | >> + XCAN_ECC_CFG_REECTXOL_MASK | >XCAN_ECC_CFG_REECTXTL_MASK); >> + >> return 0; >> >> err_disableclks: >> -- >> 2.1.1 >> >> > >-- >Pengutronix e.K. | Marc Kleine-Budde | >Embedded Linux | https://www.pengutronix.de | >Vertretung Nürnberg | Phone: +49-5121-206917-129 | >Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: RE: [PATCH 2/3] can: xilinx_can: Add ECC support 2023-07-21 5:23 ` Goud, Srinivas @ 2023-07-24 8:54 ` Marc Kleine-Budde 0 siblings, 0 replies; 22+ messages in thread From: Marc Kleine-Budde @ 2023-07-24 8:54 UTC (permalink / raw) To: Goud, Srinivas Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git (AMD-Xilinx), michal.simek, linux-can, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3689 bytes --] On 21.07.2023 05:23:02, Goud, Srinivas wrote: > >> + if (priv->ecc_enable) { > >> + u32 reg_ecc; > >> + > >> + reg_ecc = priv->read_reg(priv, XCAN_RXFIFO_ECC_OFFSET); > >> + if (isr & XCAN_IXR_E2BERX_MASK) { > >> + priv->ecc_2bit_rxfifo_cnt += > >> + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, > >reg_ecc); > >> + netdev_dbg(ndev, "%s: RX FIFO 2bit ECC error count > >%d\n", > >> + __func__, priv->ecc_2bit_rxfifo_cnt); > >> + } > >> + if (isr & XCAN_IXR_E1BERX_MASK) { > >> + priv->ecc_1bit_rxfifo_cnt += reg_ecc & > >> + XCAN_ECC_1BIT_CNT_MASK; > > > >Please use FIELD_GET here, too. > > > >> + netdev_dbg(ndev, "%s: RX FIFO 1bit ECC error count > >%d\n", > >> + __func__, priv->ecc_1bit_rxfifo_cnt); > >> + } > >> + > >> + reg_ecc = priv->read_reg(priv, XCAN_TXOLFIFO_ECC_OFFSET); > >> + if (isr & XCAN_IXR_E2BETXOL_MASK) { > >> + priv->ecc_2bit_txolfifo_cnt += > >> + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, > >reg_ecc); > >> + netdev_dbg(ndev, "%s: TXOL FIFO 2bit ECC error count > >%d\n", > >> + __func__, priv->ecc_2bit_txolfifo_cnt); > >> + } > >> + if (isr & XCAN_IXR_E1BETXOL_MASK) { > >> + priv->ecc_1bit_txolfifo_cnt += reg_ecc & > >> + XCAN_ECC_1BIT_CNT_MASK; > > > >same here > > > >> + netdev_dbg(ndev, "%s: TXOL FIFO 1bit ECC error count > >%d\n", > >> + __func__, priv->ecc_1bit_txolfifo_cnt); > >> + } > >> + > >> + reg_ecc = priv->read_reg(priv, XCAN_TXTLFIFO_ECC_OFFSET); > >> + if (isr & XCAN_IXR_E2BETXTL_MASK) { > >> + priv->ecc_2bit_txtlfifo_cnt += > >> + FIELD_GET(XCAN_ECC_2BIT_CNT_MASK, > >reg_ecc); > >> + netdev_dbg(ndev, "%s: TXTL FIFO 2bit ECC error count > >%d\n", > >> + __func__, priv->ecc_2bit_txtlfifo_cnt); > >> + } > >> + if (isr & XCAN_IXR_E1BETXTL_MASK) { > >> + priv->ecc_1bit_txtlfifo_cnt += reg_ecc & > >> + XCAN_ECC_1BIT_CNT_MASK; > > > >same here > > > >> + netdev_dbg(ndev, "%s: TXTL FIFO 1bit ECC error count > >%d\n", > >> + __func__, priv->ecc_1bit_txtlfifo_cnt); > >> + } > >> + > >> + /* Reset FIFO ECC counters */ > >> + priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, > >XCAN_ECC_CFG_REECRX_MASK | > >> + XCAN_ECC_CFG_REECTXOL_MASK | > >XCAN_ECC_CFG_REECTXTL_MASK); > > > >This is racy - you will lose events that occur between reading the register value > >and clearing the register. You can save the old value and add the difference > >between the new and the old value to the total counter. What happens when > >the counter overflows? The following pseudocode should handle the u16 > >counter rolling over to 0: > As per IP specifications when counter reaching maximum value of 0xFFFF will > stays there until reset. > > Not sure we can avoid this race condition completely, as we need to reset > the counters after reaching the 0xFFFF to avoid losing the events. > > To minimize the race condition, we can reset the counters after reaching > the events to 0xFFFF instead of resetting for every interrupt. > Can you please suggest if we can go this approach. Ok, the counter doesn't overflow. To keep the logic in the driver simple, I think it's best to read the value and reset the counter directly in the next line. Please add a comment like: "The counter reaches its maximum at 0xffff and does not overflow. Accept the small race window between reading and resetting." regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] can: xilinx_can: Add ECC support 2023-06-12 11:42 ` [PATCH 2/3] can: xilinx_can: Add ECC support Srinivas Goud 2023-06-13 11:30 ` Marc Kleine-Budde @ 2023-06-16 11:06 ` Marc Kleine-Budde 1 sibling, 0 replies; 22+ messages in thread From: Marc Kleine-Budde @ 2023-06-16 11:06 UTC (permalink / raw) To: Srinivas Goud Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git, michal.simek, linux-can, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2349 bytes --] On 12.06.2023 17:12:56, Srinivas Goud wrote: > Add ECC support for Xilinx CAN Controller, so this driver reports > 1bit/2bit ECC errors for FIFO's based on ECC error interrupt. > ECC feature for Xilinx CAN Controller selected through > 'xlnx,has-ecc' DT property > > Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> > --- > drivers/net/can/xilinx_can.c | 109 +++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 104 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c > index 43c812e..311e435 100644 > --- a/drivers/net/can/xilinx_can.c > +++ b/drivers/net/can/xilinx_can.c > @@ -63,6 +63,12 @@ enum xcan_reg { > XCAN_RXMSG_2_BASE_OFFSET = 0x2100, /* RX Message Space */ > XCAN_AFR_2_MASK_OFFSET = 0x0A00, /* Acceptance Filter MASK */ > XCAN_AFR_2_ID_OFFSET = 0x0A04, /* Acceptance Filter ID */ > + > + /* only on AXI CAN cores */ > + XCAN_ECC_CFG_OFFSET = 0xC8, /* ECC Configuration */ > + XCAN_TXTLFIFO_ECC_OFFSET = 0xCC, /* TXTL FIFO ECC error counter */ > + XCAN_TXOLFIFO_ECC_OFFSET = 0xD0, /* TXOL FIFO ECC error counter */ > + XCAN_RXFIFO_ECC_OFFSET = 0xD4, /* RX FIFO ECC error counter */ Please keep the enum sorted by address. > }; > > #define XCAN_FRAME_ID_OFFSET(frame_base) ((frame_base) + 0x00) > @@ -135,6 +141,17 @@ enum xcan_reg { > #define XCAN_2_FSR_RI_MASK 0x0000003F /* RX Read Index */ > #define XCAN_DLCR_EDL_MASK 0x08000000 /* EDL Mask in DLC */ > #define XCAN_DLCR_BRS_MASK 0x04000000 /* BRS Mask in DLC */ > +#define XCAN_IXR_E2BERX_MASK BIT(23) /* RX FIFO two bit ECC error */ > +#define XCAN_IXR_E1BERX_MASK BIT(22) /* RX FIFO one bit ECC error */ > +#define XCAN_IXR_E2BETXOL_MASK BIT(21) /* TXOL FIFO two bit ECC error */ > +#define XCAN_IXR_E1BETXOL_MASK BIT(20) /* TXOL FIFO One bit ECC error */ > +#define XCAN_IXR_E2BETXTL_MASK BIT(19) /* TXTL FIFO Two bit ECC error */ > +#define XCAN_IXR_E1BETXTL_MASK BIT(18) /* TXTL FIFO One bit ECC error */ Please move the XCAN_IXR next to the other XCAN_IXR. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] can: xilinx_can: Add debugfs support for ECC 2023-06-12 11:42 [PATCH 0/3] can: xilinx_can: Add ECC feature support Srinivas Goud 2023-06-12 11:42 ` [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ Srinivas Goud 2023-06-12 11:42 ` [PATCH 2/3] can: xilinx_can: Add ECC support Srinivas Goud @ 2023-06-12 11:42 ` Srinivas Goud 2023-06-13 7:51 ` Marc Kleine-Budde 2023-06-16 11:12 ` [PATCH 0/3] can: xilinx_can: Add ECC feature support Marc Kleine-Budde 3 siblings, 1 reply; 22+ messages in thread From: Srinivas Goud @ 2023-06-12 11:42 UTC (permalink / raw) To: wg, mkl, davem, edumazet, kuba, pabeni, gcnu.goud Cc: git, michal.simek, linux-can, linux-arm-kernel, linux-kernel, Srinivas Goud Create debugfs entry for reading all the FIFO ECC errors. Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> --- drivers/net/can/xilinx_can.c | 62 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c index 311e435..f7bf31a 100644 --- a/drivers/net/can/xilinx_can.c +++ b/drivers/net/can/xilinx_can.c @@ -29,6 +29,7 @@ #include <linux/can/dev.h> #include <linux/can/error.h> #include <linux/pm_runtime.h> +#include <linux/debugfs.h> #define DRIVER_NAME "xilinx_can" @@ -183,6 +184,9 @@ enum xcan_reg { #define XCAN_FLAG_RX_FIFO_MULTI 0x0010 #define XCAN_FLAG_CANFD_2 0x0020 +/* ECC counters buffer length */ +#define XCAN_ECC_CNT_BUF_LEN 100 + enum xcan_ip_type { XAXI_CAN = 0, XZYNQ_CANPS, @@ -222,6 +226,7 @@ struct xcan_devtype_data { * @ecc_1bit_txolfifo_cnt: TXOLFIFO 1bit ECC count * @ecc_2bit_txtlfifo_cnt: TXTLFIFO 2bit ECC count * @ecc_1bit_txtlfifo_cnt: TXTLFIFO 1bit ECC count + * @debugfs: Directory entry for debugfs */ struct xcan_priv { struct can_priv can; @@ -246,6 +251,7 @@ struct xcan_priv { u32 ecc_1bit_txolfifo_cnt; u32 ecc_2bit_txtlfifo_cnt; u32 ecc_1bit_txtlfifo_cnt; + struct dentry *debugfs; }; /* CAN Bittiming constants as per Xilinx CAN specs */ @@ -1736,6 +1742,56 @@ static int __maybe_unused xcan_runtime_resume(struct device *dev) return 0; } +static ssize_t read_ecc_cnt_status(struct file *file, char __user *user_buf, + size_t count, loff_t *ppos) +{ + unsigned int len = 0, buf_len = XCAN_ECC_CNT_BUF_LEN; + struct net_device *ndev = file->private_data; + struct xcan_priv *priv = netdev_priv(ndev); + ssize_t ret_cnt; + char *buf; + + buf = kzalloc(buf_len, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + len = scnprintf(buf + len, buf_len - len, + "%d\n", priv->ecc_2bit_rxfifo_cnt); + len += scnprintf(buf + len, buf_len - len, + "%d\n", priv->ecc_1bit_rxfifo_cnt); + len += scnprintf(buf + len, buf_len - len, + "%d\n", priv->ecc_2bit_txolfifo_cnt); + len += scnprintf(buf + len, buf_len - len, + "%d\n", priv->ecc_1bit_txolfifo_cnt); + len += scnprintf(buf + len, buf_len - len, + "%d\n", priv->ecc_2bit_txtlfifo_cnt); + len += scnprintf(buf + len, buf_len - len, + "%d\n", priv->ecc_1bit_txtlfifo_cnt); + ret_cnt = simple_read_from_buffer(user_buf, count, ppos, buf, len); + + kfree(buf); + + return ret_cnt; +} + +static const struct file_operations read_ecc_fops = { + .open = simple_open, + .read = read_ecc_cnt_status, + .llseek = generic_file_llseek, +}; + +static void setup_debugfs(struct net_device *ndev) +{ + struct xcan_priv *priv = netdev_priv(ndev); + + priv->debugfs = debugfs_create_dir(dev_name(priv->dev), NULL); + if (!priv->debugfs) + return; + + debugfs_create_file("read_ecc_cnt", 0644, priv->debugfs, + ndev, &read_ecc_fops); +} + static const struct dev_pm_ops xcan_dev_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume) SET_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL) @@ -1974,10 +2030,12 @@ static int xcan_probe(struct platform_device *pdev) priv->reg_base, ndev->irq, priv->can.clock.freq, hw_tx_max, priv->tx_max); - if (priv->ecc_enable) + if (priv->ecc_enable) { + setup_debugfs(ndev); /* Reset FIFO ECC counters */ priv->write_reg(priv, XCAN_ECC_CFG_OFFSET, XCAN_ECC_CFG_REECRX_MASK | XCAN_ECC_CFG_REECTXOL_MASK | XCAN_ECC_CFG_REECTXTL_MASK); + } return 0; @@ -2000,7 +2058,9 @@ static int xcan_probe(struct platform_device *pdev) static int xcan_remove(struct platform_device *pdev) { struct net_device *ndev = platform_get_drvdata(pdev); + struct xcan_priv *priv = netdev_priv(ndev); + debugfs_remove_recursive(priv->debugfs); unregister_candev(ndev); pm_runtime_disable(&pdev->dev); free_candev(ndev); -- 2.1.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] can: xilinx_can: Add debugfs support for ECC 2023-06-12 11:42 ` [PATCH 3/3] can: xilinx_can: Add debugfs support for ECC Srinivas Goud @ 2023-06-13 7:51 ` Marc Kleine-Budde 0 siblings, 0 replies; 22+ messages in thread From: Marc Kleine-Budde @ 2023-06-13 7:51 UTC (permalink / raw) To: Srinivas Goud Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git, michal.simek, linux-can, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 519 bytes --] On 12.06.2023 17:12:57, Srinivas Goud wrote: > Create debugfs entry for reading all the FIFO ECC errors. Please attach the stats to the ethtool stats interface: | https://elixir.bootlin.com/linux/v6.3/source/include/linux/ethtool.h#L826 Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] can: xilinx_can: Add ECC feature support 2023-06-12 11:42 [PATCH 0/3] can: xilinx_can: Add ECC feature support Srinivas Goud ` (2 preceding siblings ...) 2023-06-12 11:42 ` [PATCH 3/3] can: xilinx_can: Add debugfs support for ECC Srinivas Goud @ 2023-06-16 11:12 ` Marc Kleine-Budde 2023-06-23 7:48 ` Michal Simek 3 siblings, 1 reply; 22+ messages in thread From: Marc Kleine-Budde @ 2023-06-16 11:12 UTC (permalink / raw) To: Srinivas Goud Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git, michal.simek, linux-can, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1045 bytes --] On 12.06.2023 17:12:54, Srinivas Goud wrote: > ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. > Part of this feature configuration and counter registers added > in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset. > Please find more details in PG096 v5.1 document. The document "PG096 (v5.1) May 16, 2023 CAN v5.1" [1] lists the XCAN_ECC_CFG_OFFSET as reserved, although it has a section "ECC Configuration Register". [1] https://docs.xilinx.com/viewer/book-attachment/Bv6XZP9HRonCGi58fl10dw/ch1ZLpOt4UKWNub7DXjJ7Q The other registers (XCAN_TXTLFIFO_ECC_OFFSET, XCAN_TXOLFIFO_ECC_OFFSET, XCAN_TXOLFIFO_ECC_OFFSET) are also listed as reserved and not even mentioned on the document. Am I missing something? regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] can: xilinx_can: Add ECC feature support 2023-06-16 11:12 ` [PATCH 0/3] can: xilinx_can: Add ECC feature support Marc Kleine-Budde @ 2023-06-23 7:48 ` Michal Simek 2023-06-30 8:04 ` Marc Kleine-Budde 0 siblings, 1 reply; 22+ messages in thread From: Michal Simek @ 2023-06-23 7:48 UTC (permalink / raw) To: Marc Kleine-Budde, Srinivas Goud Cc: wg, davem, edumazet, kuba, pabeni, gcnu.goud, git, michal.simek, linux-can, linux-arm-kernel, linux-kernel Hi Marc, On 6/16/23 13:12, Marc Kleine-Budde wrote: > On 12.06.2023 17:12:54, Srinivas Goud wrote: >> ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. >> Part of this feature configuration and counter registers added >> in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset. >> Please find more details in PG096 v5.1 document. > > The document "PG096 (v5.1) May 16, 2023 CAN v5.1" [1] lists the > XCAN_ECC_CFG_OFFSET as reserved, although it has a section "ECC > Configuration Register". > > [1] https://docs.xilinx.com/viewer/book-attachment/Bv6XZP9HRonCGi58fl10dw/ch1ZLpOt4UKWNub7DXjJ7Q > > The other registers (XCAN_TXTLFIFO_ECC_OFFSET, XCAN_TXOLFIFO_ECC_OFFSET, > XCAN_TXOLFIFO_ECC_OFFSET) are also listed as reserved and not even > mentioned on the document. Am I missing something? We cross check available public documentation with HW team and there is no public documentation for this feature yet. We didn't get any exact day when documentation is going to be released. Unfortunately it is not the first or even last time when this is happening but I still think is good to get this feature done properly till the time when documentation catch it up. Please let me know if you have any concern about it. Thanks, Michal ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] can: xilinx_can: Add ECC feature support 2023-06-23 7:48 ` Michal Simek @ 2023-06-30 8:04 ` Marc Kleine-Budde 0 siblings, 0 replies; 22+ messages in thread From: Marc Kleine-Budde @ 2023-06-30 8:04 UTC (permalink / raw) To: Michal Simek Cc: Srinivas Goud, wg, davem, edumazet, kuba, pabeni, gcnu.goud, git, michal.simek, linux-can, linux-arm-kernel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1902 bytes --] On 23.06.2023 09:48:16, Michal Simek wrote: > Hi Marc, > > On 6/16/23 13:12, Marc Kleine-Budde wrote: > > On 12.06.2023 17:12:54, Srinivas Goud wrote: > > > ECC feature added to Tx and Rx FIFO’s for Xilinx CAN Controller. > > > Part of this feature configuration and counter registers added > > > in Xilinx CAN Controller for 1bit/2bit ECC errors count and reset. > > > Please find more details in PG096 v5.1 document. > > > > The document "PG096 (v5.1) May 16, 2023 CAN v5.1" [1] lists the > > XCAN_ECC_CFG_OFFSET as reserved, although it has a section "ECC > > Configuration Register". > > > > [1] https://docs.xilinx.com/viewer/book-attachment/Bv6XZP9HRonCGi58fl10dw/ch1ZLpOt4UKWNub7DXjJ7Q > > > > The other registers (XCAN_TXTLFIFO_ECC_OFFSET, XCAN_TXOLFIFO_ECC_OFFSET, > > XCAN_TXOLFIFO_ECC_OFFSET) are also listed as reserved and not even > > mentioned on the document. Am I missing something? > > We cross check available public documentation with HW team and there is no > public documentation for this feature yet. We didn't get any exact day when > documentation is going to be released. It's a pity, but's that the way it's sometimes is. > Unfortunately it is not the first or even last time when this is happening > but I still think is good to get this feature done properly till the time > when documentation catch it up. Please let me know if you have any concern > about it. No problem, update the cover letter and mention that the documentation is not public available, yet. For reference we can keep the reference to the internal doc (and mention that). regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-07-24 8:55 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-12 11:42 [PATCH 0/3] can: xilinx_can: Add ECC feature support Srinivas Goud 2023-06-12 11:42 ` [PATCH 1/3] dt-bindings: can: xilinx_can: Add ECC property ‘xlnx,has-ecc’ Srinivas Goud 2023-06-13 7:52 ` Marc Kleine-Budde 2023-06-14 10:22 ` Goud, Srinivas 2023-06-14 11:11 ` Krzysztof Kozlowski 2023-06-16 10:13 ` Goud, Srinivas 2023-06-16 10:38 ` Krzysztof Kozlowski 2023-06-16 10:44 ` Michal Simek 2023-06-17 7:31 ` Krzysztof Kozlowski 2023-06-19 6:37 ` Michal Simek 2023-06-17 7:32 ` Krzysztof Kozlowski 2023-06-13 8:46 ` Krzysztof Kozlowski 2023-06-12 11:42 ` [PATCH 2/3] can: xilinx_can: Add ECC support Srinivas Goud 2023-06-13 11:30 ` Marc Kleine-Budde 2023-07-21 5:23 ` Goud, Srinivas 2023-07-24 8:54 ` Marc Kleine-Budde 2023-06-16 11:06 ` Marc Kleine-Budde 2023-06-12 11:42 ` [PATCH 3/3] can: xilinx_can: Add debugfs support for ECC Srinivas Goud 2023-06-13 7:51 ` Marc Kleine-Budde 2023-06-16 11:12 ` [PATCH 0/3] can: xilinx_can: Add ECC feature support Marc Kleine-Budde 2023-06-23 7:48 ` Michal Simek 2023-06-30 8:04 ` Marc Kleine-Budde
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).