Re: [PATCH v2 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[Conor Dooley]

On Thu, Jan 18, 2024 at 11:53:47AM -0800, dregan@xxxxxxxxxxxx wrote:
> From: William Zhang <william.zhang@xxxxxxxxxxxx>
> 
> Update the descriptions to reflect different families of broadband SoC and
> use the general name bcmbca for ARM based SoC.
> 
> Add brcm,nand-use-wp property to have an option for disabling this
> feature on broadband board design that does not use write protection.
> 
> Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
> broadband board designs because they do not specify ecc setting in dts
> but rather using the strap setting.
> 
> Remove the requirement of interrupts property to reflect the driver
> code. Also add myself to the list of maintainers.
> 
> Signed-off-by: William Zhang <william.zhang@xxxxxxxxxxxx>
> Reviewed-by: David Regan <dregan@xxxxxxxxxxxx>
> ---
> Changes in v2:
> - Revert the new compatible string nand-bcmbca
> - Drop the BCM63168 compatible fix to avoid any potential ABI
> incompatibility issue
> - Simplify the explanation for brcm,nand-use-wp
> - Keep the interrupt name requirement when interrupt number is specified
> ---
>  .../bindings/mtd/brcm,brcmnand.yaml           | 36 +++++++++++++++----
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> index f57e96374e67..56176ec1a992 100644
> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> @@ -9,6 +9,7 @@ title: Broadcom STB NAND Controller
>  maintainers:
>    - Brian Norris <computersforpeace@xxxxxxxxx>
>    - Kamal Dasu <kdasu.kdev@xxxxxxxxx>
> +  - William Zhang <william.zhang@xxxxxxxxxxxx>
>  
>  description: |
>    The Broadcom Set-Top Box NAND controller supports low-level access to raw NAND
> @@ -18,9 +19,10 @@ description: |
>    supports basic PROGRAM and READ functions, among other features.
>  
>    This controller was originally designed for STB SoCs (BCM7xxx) but is now
> -  available on a variety of Broadcom SoCs, including some BCM3xxx, BCM63xx, and
> -  iProc/Cygnus. Its history includes several similar (but not fully register
> -  compatible) versions.
> +  available on a variety of Broadcom SoCs, including some BCM3xxx, MIPS based
> +  Broadband SoC (BCM63xx), ARM based Broadband SoC (BCMBCA) and iProc/Cygnus.
> +  Its history includes several similar (but not fully register compatible)
> +  versions.
>  
>    -- Additional SoC-specific NAND controller properties --
>  
> @@ -53,7 +55,7 @@ properties:
>                - brcm,brcmnand-v7.2
>                - brcm,brcmnand-v7.3
>            - const: brcm,brcmnand
> -      - description: BCM63138 SoC-specific NAND controller
> +      - description: BCMBCA SoC-specific NAND controller
>          items:
>            - const: brcm,nand-bcm63138
>            - enum:
> @@ -65,7 +67,7 @@ properties:
>            - const: brcm,nand-iproc
>            - const: brcm,brcmnand-v6.1
>            - const: brcm,brcmnand
> -      - description: BCM63168 SoC-specific NAND controller
> +      - description: BCM63xx SoC-specific NAND controller
>          items:
>            - const: brcm,nand-bcm63168
>            - const: brcm,nand-bcm6368
> @@ -111,6 +113,17 @@ properties:
>        earlier versions of this core that include WP
>      type: boolean
>  
> +  brcm,nand-use-wp:
> +    description:
> +      Use this property to indicate if board design uses
> +      controller's write protection feature and connects its
> +      NAND_WPb pin to nand chip's WP_L pin. Driver defaults to
> +      use this feature when this property does not exist.
> +      Set to 0 if WP pins are not connected and feature is not
> +      used. Set to 1 if WP pins are connected and feature is used.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

This property does not make sense to me. "Driver defaults to use this
feature when the property does not exist" - either the property name is
backwards or the description is. Secondly, I don't get why the property
is an enum in the first place - depending on which if the name or
description is wrong, either 0 or 1 would overlap with the default.

>  patternProperties:
>    "^nand@[a-f0-9]$":
>      type: object
> @@ -137,6 +150,16 @@ patternProperties:
>            layout.
>          $ref: /schemas/types.yaml#/definitions/uint32
>  
> +      brcm,nand-ecc-use-strap:
> +        description:
> +          This flag is used by the driver to get the ecc strength and
> +          spare area size from the SoC NAND boot strap setting. This
> +          is commonly used by the BCMBCA SoC board design. If ecc
> +          strength and spare area size are set by nand-ecc-strength
> +          and brcm,nand-oob-sector-size in the dts, these settings
> +          have precedence and override this flag.
> +        $ref: /schemas/types.yaml#/definitions/flag

This property I'm not all that sure about either. If the specific
properties that you mention here are not set in the DT what happens at
the moment?

I suppose what I am getting at is why are the bootstrap settings not
always used in the absence of specific values provided in the DT?

Thanks,
Conor.

> +
>      unevaluatedProperties: false
>  
>  allOf:
> @@ -177,6 +200,8 @@ allOf:
>              - const: iproc-idm
>              - const: iproc-ext
>    - if:
> +      required:
> +        - interrupts
>        properties:
>          interrupts:
>            minItems: 2
> @@ -189,7 +214,6 @@ unevaluatedProperties: false
>  required:
>    - reg
>    - reg-names
> -  - interrupts
>  
>  examples:
>    - |
> -- 
> 2.37.3
>

Attachment: signature.asc
Description: PGP signature