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

[William Zhang <william.zhang@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 5285 bytes --]



On 1/23/24 02:34, Jonas Gorski wrote:
> On Mon, 22 Jan 2024 at 18:34, William Zhang <william.zhang@broadcom.com> wrote:
>>
>> Hi,
>>
>> On 1/22/24 03:48, Jonas Gorski wrote:
>>> Hi,
>>>
>>> On Thu, 18 Jan 2024 at 20:56, <dregan@broadcom.com> wrote:
>>>>
>>>> From: William Zhang <william.zhang@broadcom.com>
>>>>
>>>> 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@broadcom.com>
>>>> Reviewed-by: David Regan <dregan@broadcom.com>
>>>> ---
>>>> 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@gmail.com>
>>>>      - Kamal Dasu <kdasu.kdev@gmail.com>
>>>> +  - William Zhang <william.zhang@broadcom.com>
>>>>
>>>>    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@broadcom.com/
>> 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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]