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

[William Zhang]

On 1/23/24 02:34, Jonas Gorski wrote:
> 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.
I didn't know that 6368 does not work. I will revert this change then.

> 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.
Yeah and if you can update it to bcm63xx, that will be great!

> Best Regards,
> Jonas

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature