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

[Jonas Gorski]

On Mon, 22 Jan 2024 at 18:34, William Zhang <william.zhang@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 1/22/24 03:48, Jonas Gorski wrote:
> > Hi,
> >
> > On Thu, 18 Jan 2024 at 20:56, <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
> >
> > Only the BCM63268 family has a v4.0 NAND controller with support for
> > ONFI and raw access; BCM6368 has a v2.1, and BCM6328 and BCM6362 have
> > a v2.2.
> >
> > So claiming this is a generic binding is wrong; you would need to add
> > the appropriate variants first. Or add another one for the BCM6368
> > NAND v2.x controllers, which is missing. You can find them used in
> > arch/mips/boot/dts/brcm/bcm63{28,62,68}.dtsi.
> >
> I am not changing binding here but jsut update the description to
> identify these MIPS based chip as bcm63xx family. This convention is
> used in other IP blocks too.  And yes this binding is not correct and I
> noticed the same v2.x usage in the dtsi files you pointed out. So I
> actually updated this binding in my v1 here
> https://lore.kernel.org/lkml/20230606231252.94838-6-william.zhang@xxxxxxxxxxxx/
> but there was some concern as it could possibly break the ABI in that
> thread's discussion.

That change did change the ABI that would have (AFAICT) broken it for BCM63168.

My point is: This binding in its current state is valid *only* for the
BCM63168 NAND controller, and not for any other BCM63xx NAND
controllers.

So changing the description to BCM63xx is wrong, unless you extend it
to also match the BCM6328/6362/6368 NAND controller bindings as used.

I can send a patch adding the missing binding parts, I actually have a
WIP one lying around as I started trying to address dts issues in the
MIPS bcm63xx dts(i) files a while ago.

Best Regards,
Jonas