Hi William,
[email protected] wrote on Tue, 6 Jun 2023 16:12:45 -0700:
> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
> product. The old compatible string is still kept in the driver so old
> dtb can still work.
>
> 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 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 and interrupt-names properties to
> reflect the driver code.
>
> This patch also includes a few minor fixes to the BCM63xx compatibles
> and add myself to the list of maintainers.
>
> Signed-off-by: William Zhang <[email protected]>
> ---
>
> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
> 1 file changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
> - Kamal Dasu <[email protected]>
> + - William Zhang <[email protected]>
>
> 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,9 +55,9 @@ 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
> + - const: brcm,nand-bcmbca
> - enum:
> - brcm,brcmnand-v7.0
> - brcm,brcmnand-v7.1
> @@ -65,11 +67,15 @@ 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
> - - const: brcm,brcmnand-v4.0
> + - enum:
> + - brcm,nand-bcm63168
> + - brcm,nand-bcm6368
> + - enum:
> + - brcm,brcmnand-v2.1
> + - brcm,brcmnand-v2.2
> + - brcm,brcmnand-v4.0
> - const: brcm,brcmnand
>
> reg:
> @@ -111,6 +117,19 @@ properties:
> earlier versions of this core that include WP
> type: boolean
>
> + brcm,nand-use-wp:
> + description:
> + Use this integer 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.
> + Set to 2 if WP pins are connected but disable this feature
> + through driver that sets controller to output high on NAND_WPb.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2]
Perhaps strings would be welcome. I'll let binding maintainers say what
they think of it.
> +
> patternProperties:
> "^nand@[a-f0-9]$":
> type: object
> @@ -136,13 +155,23 @@ 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
How in practice do you access the strap value? Don't you need a phandle
over a specific area in the SoC?
> +
> allOf:
> - $ref: nand-controller.yaml#
> - if:
> properties:
> compatible:
> contains:
> - const: brcm,nand-bcm63138
> + const: brcm,nand-bcmbca
> then:
> properties:
> reg-names:
> @@ -153,7 +182,9 @@ allOf:
> properties:
> compatible:
> contains:
> - const: brcm,nand-bcm6368
> + enum:
> + - brcm,nand-bcm63168
> + - brcm,nand-bcm6368
> then:
> properties:
> reg-names:
> @@ -173,20 +204,12 @@ allOf:
> - const: nand
> - const: iproc-idm
> - const: iproc-ext
> - - if:
> - properties:
> - interrupts:
> - minItems: 2
> - then:
> - required:
> - - interrupt-names
Why do you remove this? Removing "interrupts" from the required
properties is fine, but constraining the interrupts property when it is
relevant is still expected.
>
> unevaluatedProperties: false
>
> required:
> - reg
> - reg-names
> - - interrupts
This should be done in a separate patch.
>
> examples:
> - |
> @@ -215,8 +238,7 @@ examples:
> };
> - |
> nand-controller@10000200 {
> - compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
> - "brcm,brcmnand-v4.0", "brcm,brcmnand";
> + compatible = "brcm,nand-bcm6368", "brcm,brcmnand-v2.1", "brcm,brcmnand";
> reg = <0x10000200 0x180>,
> <0x100000b0 0x10>,
> <0x10000600 0x200>;
Thanks,
Miquèl
Hi Miquel,
On 06/07/2023 01:14 AM, Miquel Raynal wrote:
> Hi William,
>
> [email protected] wrote on Tue, 6 Jun 2023 16:12:45 -0700:
>
>> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
>> product. The old compatible string is still kept in the driver so old
>> dtb can still work.
>>
>> 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 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 and interrupt-names properties to
>> reflect the driver code.
>>
>> This patch also includes a few minor fixes to the BCM63xx compatibles
>> and add myself to the list of maintainers.
>>
>> Signed-off-by: William Zhang <[email protected]>
>> ---
>>
>> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
>> 1 file changed, 43 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
>> - Kamal Dasu <[email protected]>
>> + - William Zhang <[email protected]>
>>
>> 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,9 +55,9 @@ 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
>> + - const: brcm,nand-bcmbca
>> - enum:
>> - brcm,brcmnand-v7.0
>> - brcm,brcmnand-v7.1
>> @@ -65,11 +67,15 @@ 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
>> - - const: brcm,brcmnand-v4.0
>> + - enum:
>> + - brcm,nand-bcm63168
>> + - brcm,nand-bcm6368
>> + - enum:
>> + - brcm,brcmnand-v2.1
>> + - brcm,brcmnand-v2.2
>> + - brcm,brcmnand-v4.0
>> - const: brcm,brcmnand
>>
>> reg:
>> @@ -111,6 +117,19 @@ properties:
>> earlier versions of this core that include WP
>> type: boolean
>>
>> + brcm,nand-use-wp:
>> + description:
>> + Use this integer 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.
>> + Set to 2 if WP pins are connected but disable this feature
>> + through driver that sets controller to output high on NAND_WPb.
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + enum: [0, 1, 2]
>
> Perhaps strings would be welcome. I'll let binding maintainers say what
> they think of it.
>
Practically there is really just use cases of 0 and 1. I could use a
bool flag but to keep consistent with the driver code and in case there
is any existing usage of 2.
>> +
>> patternProperties:
>> "^nand@[a-f0-9]$":
>> type: object
>> @@ -136,13 +155,23 @@ 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
>
> How in practice do you access the strap value? Don't you need a phandle
> over a specific area in the SoC?
>
The strap value is latched and stored in the NAND controller register so
there is no extra phandle needed.
>> +
>> allOf:
>> - $ref: nand-controller.yaml#
>> - if:
>> properties:
>> compatible:
>> contains:
>> - const: brcm,nand-bcm63138
>> + const: brcm,nand-bcmbca
>> then:
>> properties:
>> reg-names:
>> @@ -153,7 +182,9 @@ allOf:
>> properties:
>> compatible:
>> contains:
>> - const: brcm,nand-bcm6368
>> + enum:
>> + - brcm,nand-bcm63168
>> + - brcm,nand-bcm6368
>> then:
>> properties:
>> reg-names:
>> @@ -173,20 +204,12 @@ allOf:
>> - const: nand
>> - const: iproc-idm
>> - const: iproc-ext
>> - - if:
>> - properties:
>> - interrupts:
>> - minItems: 2
>> - then:
>> - required:
>> - - interrupt-names
>
> Why do you remove this? Removing "interrupts" from the required
> properties is fine, but constraining the interrupts property when it is
> relevant is still expected.
>
There is no requirement for interrupt name even if it have two
interrupts. Driver code does not use interrupt name but the interrupt
index instead.
>>
>> unevaluatedProperties: false
>>
>> required:
>> - reg
>> - reg-names
>> - - interrupts
>
> This should be done in a separate patch.
>
I thought this is also related to my update for bcmbca chips because
they don't need to interrupt and interrupt name.
>>
>> examples:
>> - |
>> @@ -215,8 +238,7 @@ examples:
>> };
>> - |
>> nand-controller@10000200 {
>> - compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
>> - "brcm,brcmnand-v4.0", "brcm,brcmnand";
>> + compatible = "brcm,nand-bcm6368", "brcm,brcmnand-v2.1", "brcm,brcmnand";
>> reg = <0x10000200 0x180>,
>> <0x100000b0 0x10>,
>> <0x10000600 0x200>;
>
>
> Thanks,
> Miquèl
>
Hi William,
[email protected] wrote on Wed, 7 Jun 2023 13:01:56 -0700:
> Hi Miquel,
>
> On 06/07/2023 01:14 AM, Miquel Raynal wrote:
> > Hi William,
> >
> > [email protected] wrote on Tue, 6 Jun 2023 16:12:45 -0700:
> >
> >> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
> >> product. The old compatible string is still kept in the driver so old
> >> dtb can still work.
> >>
> >> 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 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 and interrupt-names properties to
> >> reflect the driver code.
> >>
> >> This patch also includes a few minor fixes to the BCM63xx compatibles
> >> and add myself to the list of maintainers.
> >>
> >> Signed-off-by: William Zhang <[email protected]>
> >> ---
> >>
> >> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
> >> 1 file changed, 43 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> >> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
> >> - Kamal Dasu <[email protected]>
> >> + - William Zhang <[email protected]>
> >> >> 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,9 +55,9 @@ 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
> >> + - const: brcm,nand-bcmbca
> >> - enum:
> >> - brcm,brcmnand-v7.0
> >> - brcm,brcmnand-v7.1
> >> @@ -65,11 +67,15 @@ 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
> >> - - const: brcm,brcmnand-v4.0
> >> + - enum:
> >> + - brcm,nand-bcm63168
> >> + - brcm,nand-bcm6368
> >> + - enum:
> >> + - brcm,brcmnand-v2.1
> >> + - brcm,brcmnand-v2.2
> >> + - brcm,brcmnand-v4.0
> >> - const: brcm,brcmnand
> >> >> reg:
> >> @@ -111,6 +117,19 @@ properties:
> >> earlier versions of this core that include WP
> >> type: boolean
> >> >> + brcm,nand-use-wp:
> >> + description:
> >> + Use this integer 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.
> >> + Set to 2 if WP pins are connected but disable this feature
> >> + through driver that sets controller to output high on NAND_WPb.
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> + enum: [0, 1, 2]
> >
> > Perhaps strings would be welcome. I'll let binding maintainers say what
> > they think of it.
> >
> Practically there is really just use cases of 0 and 1. I could use a bool flag but to keep consistent with the driver code and in case there is any existing usage of 2.
>
> >> +
> >> patternProperties:
> >> "^nand@[a-f0-9]$":
> >> type: object
> >> @@ -136,13 +155,23 @@ 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
> >
> > How in practice do you access the strap value? Don't you need a phandle
> > over a specific area in the SoC?
> >
> The strap value is latched and stored in the NAND controller register so there is no extra phandle needed.
Ok.
>
> >> +
> >> allOf:
> >> - $ref: nand-controller.yaml#
> >> - if:
> >> properties:
> >> compatible:
> >> contains:
> >> - const: brcm,nand-bcm63138
> >> + const: brcm,nand-bcmbca
> >> then:
> >> properties:
> >> reg-names:
> >> @@ -153,7 +182,9 @@ allOf:
> >> properties:
> >> compatible:
> >> contains:
> >> - const: brcm,nand-bcm6368
> >> + enum:
> >> + - brcm,nand-bcm63168
> >> + - brcm,nand-bcm6368
> >> then:
> >> properties:
> >> reg-names:
> >> @@ -173,20 +204,12 @@ allOf:
> >> - const: nand
> >> - const: iproc-idm
> >> - const: iproc-ext
> >> - - if:
> >> - properties:
> >> - interrupts:
> >> - minItems: 2
> >> - then:
> >> - required:
> >> - - interrupt-names
> >
> > Why do you remove this? Removing "interrupts" from the required
> > properties is fine, but constraining the interrupts property when it is
> > relevant is still expected.
> >
> There is no requirement for interrupt name even if it have two interrupts. Driver code does not use interrupt name but the interrupt index instead.
It does not matter in this case how the driver uses the interrupts. If
names have been provided once in the bindings, you could expect another
project using the same bindings to use the interrupt names instead of
the order. So you must keep the names. Just don't mark them required it
they are.
>
> >> >> unevaluatedProperties: false
> >> >> required:
> >> - reg
> >> - reg-names
> >> - - interrupts
> >
> > This should be done in a separate patch.
> >
> I thought this is also related to my update for bcmbca chips because they don't need to interrupt and interrupt name.
The fact that the driver does not use the interrupts does not mean they
should not be described.
>
> >> >> examples:
> >> - |
> >> @@ -215,8 +238,7 @@ examples:
> >> };
> >> - |
> >> nand-controller@10000200 {
> >> - compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
> >> - "brcm,brcmnand-v4.0", "brcm,brcmnand";
> >> + compatible = "brcm,nand-bcm6368", "brcm,brcmnand-v2.1", "brcm,brcmnand";
> >> reg = <0x10000200 0x180>,
> >> <0x100000b0 0x10>,
> >> <0x10000600 0x200>;
> >
> >
> > Thanks,
> > Miquèl
> >
Thanks,
Miquèl
Hi Miquel,
On 06/09/2023 01:58 AM, Miquel Raynal wrote:
> Hi William,
>
> [email protected] wrote on Wed, 7 Jun 2023 13:01:56 -0700:
>
>> Hi Miquel,
>>
>> On 06/07/2023 01:14 AM, Miquel Raynal wrote:
>>> Hi William,
>>>
>>> [email protected] wrote on Tue, 6 Jun 2023 16:12:45 -0700:
>>>
>>>> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
>>>> product. The old compatible string is still kept in the driver so old
>>>> dtb can still work.
>>>>
>>>> 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 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 and interrupt-names properties to
>>>> reflect the driver code.
>>>>
>>>> This patch also includes a few minor fixes to the BCM63xx compatibles
>>>> and add myself to the list of maintainers.
>>>>
>>>> Signed-off-by: William Zhang <[email protected]>
>>>> ---
>>>>
>>>> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
>>>> 1 file changed, 43 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>>>> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
>>>> - Kamal Dasu <[email protected]>
>>>> + - William Zhang <[email protected]>
>>>> >> 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,9 +55,9 @@ 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
>>>> + - const: brcm,nand-bcmbca
>>>> - enum:
>>>> - brcm,brcmnand-v7.0
>>>> - brcm,brcmnand-v7.1
>>>> @@ -65,11 +67,15 @@ 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
>>>> - - const: brcm,brcmnand-v4.0
>>>> + - enum:
>>>> + - brcm,nand-bcm63168
>>>> + - brcm,nand-bcm6368
>>>> + - enum:
>>>> + - brcm,brcmnand-v2.1
>>>> + - brcm,brcmnand-v2.2
>>>> + - brcm,brcmnand-v4.0
>>>> - const: brcm,brcmnand
>>>> >> reg:
>>>> @@ -111,6 +117,19 @@ properties:
>>>> earlier versions of this core that include WP
>>>> type: boolean
>>>> >> + brcm,nand-use-wp:
>>>> + description:
>>>> + Use this integer 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.
>>>> + Set to 2 if WP pins are connected but disable this feature
>>>> + through driver that sets controller to output high on NAND_WPb.
>>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>>> + enum: [0, 1, 2]
>>>
>>> Perhaps strings would be welcome. I'll let binding maintainers say what
>>> they think of it.
>>>
>> Practically there is really just use cases of 0 and 1. I could use a bool flag but to keep consistent with the driver code and in case there is any existing usage of 2.
>>
>>>> +
>>>> patternProperties:
>>>> "^nand@[a-f0-9]$":
>>>> type: object
>>>> @@ -136,13 +155,23 @@ 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
>>>
>>> How in practice do you access the strap value? Don't you need a phandle
>>> over a specific area in the SoC?
>>>
>> The strap value is latched and stored in the NAND controller register so there is no extra phandle needed.
>
> Ok.
>
>>
>>>> +
>>>> allOf:
>>>> - $ref: nand-controller.yaml#
>>>> - if:
>>>> properties:
>>>> compatible:
>>>> contains:
>>>> - const: brcm,nand-bcm63138
>>>> + const: brcm,nand-bcmbca
>>>> then:
>>>> properties:
>>>> reg-names:
>>>> @@ -153,7 +182,9 @@ allOf:
>>>> properties:
>>>> compatible:
>>>> contains:
>>>> - const: brcm,nand-bcm6368
>>>> + enum:
>>>> + - brcm,nand-bcm63168
>>>> + - brcm,nand-bcm6368
>>>> then:
>>>> properties:
>>>> reg-names:
>>>> @@ -173,20 +204,12 @@ allOf:
>>>> - const: nand
>>>> - const: iproc-idm
>>>> - const: iproc-ext
>>>> - - if:
>>>> - properties:
>>>> - interrupts:
>>>> - minItems: 2
>>>> - then:
>>>> - required:
>>>> - - interrupt-names
>>>
>>> Why do you remove this? Removing "interrupts" from the required
>>> properties is fine, but constraining the interrupts property when it is
>>> relevant is still expected.
>>>
>> There is no requirement for interrupt name even if it have two interrupts. Driver code does not use interrupt name but the interrupt index instead.
>
> It does not matter in this case how the driver uses the interrupts. If
> names have been provided once in the bindings, you could expect another
> project using the same bindings to use the interrupt names instead of
> the order. So you must keep the names. Just don't mark them required it
> they are.
>
Certainly any dts file can still have interrupts names with one or two
interrupts but to my understand it is not required to have the name when
there are two interrupts. Anyway I will add this back so this constrain
remains.
>>
>>>> >> unevaluatedProperties: false
>>>> >> required:
>>>> - reg
>>>> - reg-names
>>>> - - interrupts
>>>
>>> This should be done in a separate patch.
>>>
>> I thought this is also related to my update for bcmbca chips because they don't need to interrupt and interrupt name.
>
> The fact that the driver does not use the interrupts does not mean they
> should not be described.
>
>>
>>>> >> examples:
>>>> - |
>>>> @@ -215,8 +238,7 @@ examples:
>>>> };
>>>> - |
>>>> nand-controller@10000200 {
>>>> - compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
>>>> - "brcm,brcmnand-v4.0", "brcm,brcmnand";
>>>> + compatible = "brcm,nand-bcm6368", "brcm,brcmnand-v2.1", "brcm,brcmnand";
>>>> reg = <0x10000200 0x180>,
>>>> <0x100000b0 0x10>,
>>>> <0x10000600 0x200>;
>>>
>>>
>>> Thanks,
>>> Miquèl
>>>
>
>
> Thanks,
> Miquèl
>
Hi William,
[email protected] wrote on Fri, 9 Jun 2023 12:05:19 -0700:
> Hi Miquel,
>
> On 06/09/2023 01:58 AM, Miquel Raynal wrote:
> > Hi William,
> >
> > [email protected] wrote on Wed, 7 Jun 2023 13:01:56 -0700:
> >
> >> Hi Miquel,
> >>
> >> On 06/07/2023 01:14 AM, Miquel Raynal wrote:
> >>> Hi William,
> >>>
> >>> [email protected] wrote on Tue, 6 Jun 2023 16:12:45 -0700:
> >>> >>>> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
> >>>> product. The old compatible string is still kept in the driver so old
> >>>> dtb can still work.
> >>>>
> >>>> 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 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 and interrupt-names properties to
> >>>> reflect the driver code.
> >>>>
> >>>> This patch also includes a few minor fixes to the BCM63xx compatibles
> >>>> and add myself to the list of maintainers.
> >>>>
> >>>> Signed-off-by: William Zhang <[email protected]>
> >>>> ---
> >>>>
> >>>> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
> >>>> 1 file changed, 43 insertions(+), 21 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> >>>> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
> >>>> - Kamal Dasu <[email protected]>
> >>>> + - William Zhang <[email protected]>
> >>>> >> 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,9 +55,9 @@ 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
> >>>> + - const: brcm,nand-bcmbca
> >>>> - enum:
> >>>> - brcm,brcmnand-v7.0
> >>>> - brcm,brcmnand-v7.1
> >>>> @@ -65,11 +67,15 @@ 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
> >>>> - - const: brcm,brcmnand-v4.0
> >>>> + - enum:
> >>>> + - brcm,nand-bcm63168
> >>>> + - brcm,nand-bcm6368
> >>>> + - enum:
> >>>> + - brcm,brcmnand-v2.1
> >>>> + - brcm,brcmnand-v2.2
> >>>> + - brcm,brcmnand-v4.0
> >>>> - const: brcm,brcmnand
> >>>> >> reg:
> >>>> @@ -111,6 +117,19 @@ properties:
> >>>> earlier versions of this core that include WP
> >>>> type: boolean
> >>>> >> + brcm,nand-use-wp:
> >>>> + description:
> >>>> + Use this integer 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.
> >>>> + Set to 2 if WP pins are connected but disable this feature
> >>>> + through driver that sets controller to output high on NAND_WPb.
> >>>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>>> + enum: [0, 1, 2]
> >>>
> >>> Perhaps strings would be welcome. I'll let binding maintainers say what
> >>> they think of it.
> >>> >> Practically there is really just use cases of 0 and 1. I could use a bool flag but to keep consistent with the driver code and in case there is any existing usage of 2.
> >>
> >>>> +
> >>>> patternProperties:
> >>>> "^nand@[a-f0-9]$":
> >>>> type: object
> >>>> @@ -136,13 +155,23 @@ 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
> >>>
> >>> How in practice do you access the strap value? Don't you need a phandle
> >>> over a specific area in the SoC?
> >>> >> The strap value is latched and stored in the NAND controller register so there is no extra phandle needed.
> >
> > Ok.
> >
> >>
> >>>> +
> >>>> allOf:
> >>>> - $ref: nand-controller.yaml#
> >>>> - if:
> >>>> properties:
> >>>> compatible:
> >>>> contains:
> >>>> - const: brcm,nand-bcm63138
> >>>> + const: brcm,nand-bcmbca
> >>>> then:
> >>>> properties:
> >>>> reg-names:
> >>>> @@ -153,7 +182,9 @@ allOf:
> >>>> properties:
> >>>> compatible:
> >>>> contains:
> >>>> - const: brcm,nand-bcm6368
> >>>> + enum:
> >>>> + - brcm,nand-bcm63168
> >>>> + - brcm,nand-bcm6368
> >>>> then:
> >>>> properties:
> >>>> reg-names:
> >>>> @@ -173,20 +204,12 @@ allOf:
> >>>> - const: nand
> >>>> - const: iproc-idm
> >>>> - const: iproc-ext
> >>>> - - if:
> >>>> - properties:
> >>>> - interrupts:
> >>>> - minItems: 2
> >>>> - then:
> >>>> - required:
> >>>> - - interrupt-names
> >>>
> >>> Why do you remove this? Removing "interrupts" from the required
> >>> properties is fine, but constraining the interrupts property when it is
> >>> relevant is still expected.
> >>> >> There is no requirement for interrupt name even if it have two interrupts. Driver code does not use interrupt name but the interrupt index instead.
> >
> > It does not matter in this case how the driver uses the interrupts. If
> > names have been provided once in the bindings, you could expect another
> > project using the same bindings to use the interrupt names instead of
> > the order. So you must keep the names. Just don't mark them required it
> > they are.
> >
> Certainly any dts file can still have interrupts names with one or two interrupts but to my understand it is not required to have the name when there are two interrupts. Anyway I will add this back so this constrain remains.
That is true, until you enforce specific names. Then you are tied to
these names, otherwise you may break the bindings. It it is not
_really_ useful, then don't do it.
>
> >>
> >>>> >> unevaluatedProperties: false
> >>>> >> required:
> >>>> - reg
> >>>> - reg-names
> >>>> - - interrupts
> >>>
> >>> This should be done in a separate patch.
> >>> >> I thought this is also related to my update for bcmbca chips because they don't need to interrupt and interrupt name.
> >
> > The fact that the driver does not use the interrupts does not mean they
> > should not be described.
> >
> >>
> >>>> >> examples:
> >>>> - |
> >>>> @@ -215,8 +238,7 @@ examples:
> >>>> };
> >>>> - |
> >>>> nand-controller@10000200 {
> >>>> - compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
> >>>> - "brcm,brcmnand-v4.0", "brcm,brcmnand";
> >>>> + compatible = "brcm,nand-bcm6368", "brcm,brcmnand-v2.1", "brcm,brcmnand";
> >>>> reg = <0x10000200 0x180>,
> >>>> <0x100000b0 0x10>,
> >>>> <0x10000600 0x200>;
> >>>
> >>>
> >>> Thanks,
> >>> Miquèl
> >>> > >
> > Thanks,
> > Miquèl
> >
Thanks,
Miquèl
On Tue, Jun 06, 2023 at 04:12:45PM -0700, William Zhang wrote:
> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
> product. The old compatible string is still kept in the driver so old
> dtb can still work.
I don't understand why you need to change this.
> 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 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 and interrupt-names properties to
> reflect the driver code.
>
> This patch also includes a few minor fixes to the BCM63xx compatibles
> and add myself to the list of maintainers.
>
> Signed-off-by: William Zhang <[email protected]>
> ---
>
> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
> 1 file changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
> - Kamal Dasu <[email protected]>
> + - William Zhang <[email protected]>
>
> 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,9 +55,9 @@ 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
> + - const: brcm,nand-bcmbca
> - enum:
> - brcm,brcmnand-v7.0
> - brcm,brcmnand-v7.1
> @@ -65,11 +67,15 @@ 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
> - - const: brcm,brcmnand-v4.0
> + - enum:
> + - brcm,nand-bcm63168
> + - brcm,nand-bcm6368
> + - enum:
> + - brcm,brcmnand-v2.1
> + - brcm,brcmnand-v2.2
> + - brcm,brcmnand-v4.0
> - const: brcm,brcmnand
Completely changing what's valid here too. You're breaking the ABI.
Rob
On Wed, Jun 07, 2023 at 10:14:52AM +0200, Miquel Raynal wrote:
> Hi William,
>
> [email protected] wrote on Tue, 6 Jun 2023 16:12:45 -0700:
>
> > Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
> > product. The old compatible string is still kept in the driver so old
> > dtb can still work.
> >
> > 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 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 and interrupt-names properties to
> > reflect the driver code.
> >
> > This patch also includes a few minor fixes to the BCM63xx compatibles
> > and add myself to the list of maintainers.
> >
> > Signed-off-by: William Zhang <[email protected]>
> > ---
> >
> > .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
> > 1 file changed, 43 insertions(+), 21 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> > index 1571024aa119..1fe1c166a9db 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 <[email protected]>
> > - Kamal Dasu <[email protected]>
> > + - William Zhang <[email protected]>
> >
> > 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,9 +55,9 @@ 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
> > + - const: brcm,nand-bcmbca
> > - enum:
> > - brcm,brcmnand-v7.0
> > - brcm,brcmnand-v7.1
> > @@ -65,11 +67,15 @@ 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
> > - - const: brcm,brcmnand-v4.0
> > + - enum:
> > + - brcm,nand-bcm63168
> > + - brcm,nand-bcm6368
> > + - enum:
> > + - brcm,brcmnand-v2.1
> > + - brcm,brcmnand-v2.2
> > + - brcm,brcmnand-v4.0
> > - const: brcm,brcmnand
> >
> > reg:
> > @@ -111,6 +117,19 @@ properties:
> > earlier versions of this core that include WP
> > type: boolean
> >
> > + brcm,nand-use-wp:
> > + description:
> > + Use this integer 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.
> > + Set to 2 if WP pins are connected but disable this feature
> > + through driver that sets controller to output high on NAND_WPb.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 1, 2]
>
> Perhaps strings would be welcome. I'll let binding maintainers say what
> they think of it.
1 is redundant as it is the same as not present. Seems like a
'brcm,disable-wp' boolean would suffice here.
Rob
Hi Rob,
On 06/14/2023 03:43 PM, Rob Herring wrote:
> On Tue, Jun 06, 2023 at 04:12:45PM -0700, William Zhang wrote:
>> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
>> product. The old compatible string is still kept in the driver so old
>> dtb can still work.
>
> I don't understand why you need to change this.
>
>
Same as the iProc SoC NAND controller flavor for our BCMBCA SoC
controller. And the existing binding is wrong. It shows
brcm,nand-bcm63138 can be backward compatible to either v7.0 and v7.1
controller. But in fact 63138 only use v7.0.
I understand we had discussion before and chip specific name is the
preferred method but the nand controller here is different and it has
well defined and documented version control and our bcmbca SoCs
integrate the controller exactly the same way so no chip specific
compatible is needed. In the unlikely event that a future bcmbca SoC
has a different implementation, we can always add new chip specific
compatible.
If you are concerned about ABI breakage on the 63138 compatible, I can
add it back, although I find only a few 63138 and 4908 boards use it
and I did keep it in the driver so they can still bind.
>> 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 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 and interrupt-names properties to
>> reflect the driver code.
>>
>> This patch also includes a few minor fixes to the BCM63xx compatibles
>> and add myself to the list of maintainers.
>>
>> Signed-off-by: William Zhang <[email protected]>
>> ---
>>
>> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
>> 1 file changed, 43 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
>> - Kamal Dasu <[email protected]>
>> + - William Zhang <[email protected]>
>>
>> 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,9 +55,9 @@ 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
>> + - const: brcm,nand-bcmbca
>> - enum:
>> - brcm,brcmnand-v7.0
>> - brcm,brcmnand-v7.1
>> @@ -65,11 +67,15 @@ 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
>> - - const: brcm,brcmnand-v4.0
>> + - enum:
>> + - brcm,nand-bcm63168
>> + - brcm,nand-bcm6368
>> + - enum:
>> + - brcm,brcmnand-v2.1
>> + - brcm,brcmnand-v2.2
>> + - brcm,brcmnand-v4.0
>> - const: brcm,brcmnand
>
> Completely changing what's valid here too. You're breaking the ABI.
>
This binding is wrong, 63168 is not backward compatible to 6368. They
should backward compatible to their specific controller version. If you
looks arch/mips/boot/dts/brcm/bcm6328.dtsi, bcm6368.dtsi, bcm63268.dtsi
and bcm6362.dtsi, their compatible list is correct but the yaml was not
updated.
> Rob
>
Hi Rob,
On 06/14/2023 03:46 PM, Rob Herring wrote:
> On Wed, Jun 07, 2023 at 10:14:52AM +0200, Miquel Raynal wrote:
>> Hi William,
>>
>> [email protected] wrote on Tue, 6 Jun 2023 16:12:45 -0700:
>>
>>> Use new compatiable brcm,nand-bcmbca to support BCMBCA broadband
>>> product. The old compatible string is still kept in the driver so old
>>> dtb can still work.
>>>
>>> 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 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 and interrupt-names properties to
>>> reflect the driver code.
>>>
>>> This patch also includes a few minor fixes to the BCM63xx compatibles
>>> and add myself to the list of maintainers.
>>>
>>> Signed-off-by: William Zhang <[email protected]>
>>> ---
>>>
>>> .../bindings/mtd/brcm,brcmnand.yaml | 64 +++++++++++++------
>>> 1 file changed, 43 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>>> index 1571024aa119..1fe1c166a9db 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 <[email protected]>
>>> - Kamal Dasu <[email protected]>
>>> + - William Zhang <[email protected]>
>>>
>>> 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,9 +55,9 @@ 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
>>> + - const: brcm,nand-bcmbca
>>> - enum:
>>> - brcm,brcmnand-v7.0
>>> - brcm,brcmnand-v7.1
>>> @@ -65,11 +67,15 @@ 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
>>> - - const: brcm,brcmnand-v4.0
>>> + - enum:
>>> + - brcm,nand-bcm63168
>>> + - brcm,nand-bcm6368
>>> + - enum:
>>> + - brcm,brcmnand-v2.1
>>> + - brcm,brcmnand-v2.2
>>> + - brcm,brcmnand-v4.0
>>> - const: brcm,brcmnand
>>>
>>> reg:
>>> @@ -111,6 +117,19 @@ properties:
>>> earlier versions of this core that include WP
>>> type: boolean
>>>
>>> + brcm,nand-use-wp:
>>> + description:
>>> + Use this integer 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.
>>> + Set to 2 if WP pins are connected but disable this feature
>>> + through driver that sets controller to output high on NAND_WPb.
>>> + $ref: /schemas/types.yaml#/definitions/uint32
>>> + enum: [0, 1, 2]
>>
>> Perhaps strings would be welcome. I'll let binding maintainers say what
>> they think of it.
>
> 1 is redundant as it is the same as not present. Seems like a
> 'brcm,disable-wp' boolean would suffice here.
> WP feature present is indicated by the existing flag brcm,nand-has-wp.
1 is the opposite of 0 (use this feature vs not use this feature while
chip has this feature). 2 is kind of strange (for feature disabled
intentionally at run time while feature is used), maybe for testing
purpose or for certainly nand chip that has issue with WP but the board
design already connect the WP pin. I keep it here to match the driver
code that have these three states of the wp usage.
> Rob
>