Re: [PATCH v2 2/6] dt-bindings: memory-controllers: ti,gpmc: Convert to yaml

[Roger Quadros <rogerq@kernel.org>]

Hi Rob,

On 02/09/2021 22:56, Rob Herring wrote:
> On Thu, Sep 02, 2021 at 12:56:05PM +0300, Roger Quadros wrote:
>> Convert omap-gpmc.txt to ti,gpmc.yaml.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  .../bindings/memory-controllers/omap-gpmc.txt | 157 --------
>>  .../bindings/memory-controllers/ti,gpmc.yaml  | 364 ++++++++++++++++++
>>  .../devicetree/bindings/mtd/gpmc-nand.txt     |   2 +-
>>  .../devicetree/bindings/mtd/gpmc-nor.txt      |   4 +-
>>  .../devicetree/bindings/mtd/gpmc-onenand.txt  |   2 +-
>>  .../devicetree/bindings/net/gpmc-eth.txt      |   4 +-
>>  6 files changed, 370 insertions(+), 163 deletions(-)
>>  delete mode 100644 Documentation/devicetree/bindings/memory-controllers/omap-gpmc.txt
>>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml
> 
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml
>> new file mode 100644
>> index 000000000000..b7d43370a95d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml
>> @@ -0,0 +1,364 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/memory-controllers/ti,gpmc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments GPMC Memory Controller device-tree bindings
>> +
>> +maintainers:
>> +  - Tony Lindgren <tony@atomide.com>
>> +  - Roger Quadros <rogerq@kernel.org>
>> +
>> +description:
>> +  The GPMC is a unified memory controller dedicated for interfacing
>> +  with external memory devices like
>> +  - Asynchronous SRAM-like memories and ASICs
>> +  - Asynchronous, synchronous, and page mode burst NOR flash
>> +  - NAND flash
>> +  - Pseudo-SRAM devices
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - ti,omap2420-gpmc
>> +          - ti,omap2430-gpmc
>> +          - ti,omap3430-gpmc
>> +          - ti,omap4430-gpmc
>> +          - ti,am3352-gpmc
>> +
>> +  reg:
>> +    items:
>> +      - description:
>> +          Configuration registers for the controller.
> 
> Just 'maxItems: 1' is sufficient.
> 
>> +
>> +  interrupts: true
> 
> Need to define how many.
> 
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: |
>> +      Functional clock. Used for bus timing calculations and
>> +      GPMC configuration.
>> +
>> +  clock-names:
>> +    items:
>> +      - const: fck
>> +
>> +  dmas:
>> +    items:
>> +      - description: DMA channel for GPMC NAND prefetch
>> +
>> +  dma-names:
>> +    items:
>> +      - const: rxtx
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 1
>> +
>> +  gpmc,num-cs:
>> +    description: maximum number of supported chip-select lines.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  gpmc,num-waitpins:
>> +    description: maximum number of supported wait pins.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +  ranges:
>> +    minItems: 1
>> +    description: |
>> +      Must be set up to reflect the memory layout with four
>> +      integer values for each chip-select line in use,
>> +      <cs-number> 0 <physical address of mapping> <size>
>> +
>> +    items:
>> +      - description: NAND bank 0
>> +      - description: NOR/SRAM bank 0
>> +      - description: NOR/SRAM bank 1
>> +
>> +  '#interrupt-cells':
>> +    const: 2
>> +
>> +  interrupt-controller:
>> +    description: |
>> +      The GPMC driver implements and interrupt controller for
>> +      the NAND events "fifoevent" and "termcount" plus the
>> +      rising/falling edges on the GPMC_WAIT pins.
>> +      The interrupt number mapping is as follows
>> +      0 - NAND_fifoevent
>> +      1 - NAND_termcount
>> +      2 - GPMC_WAIT0 pin edge
>> +      3 - GPMC_WAIT1 pin edge, and so on.
>> +
>> +  '#gpio-cells':
>> +     const: 2
>> +
>> +  gpio-controller:
>> +    description: |
>> +      The GPMC driver implements a GPIO controller for the
>> +      GPMC WAIT pins that can be used as general purpose inputs.
>> +      0 maps to GPMC_WAIT0 pin.
>> +
>> +  ti,hwmods:
>> +    description:
>> +      Name of the HWMOD associated with GPMC. This is for legacy
>> +      omap2/3 platforms only.
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    deprecated: true
>> +
>> +  ti,no-idle-on-init:
>> +    description:
>> +      Prevent idling the module at init. This is for legacy omap2/3
>> +      platforms only.
>> +    type: boolean
>> +    deprecated: true
>> +
>> +patternProperties:
>> +#  "@[0-3],[a-f0-9]+$":>> +  "^[a-zA-Z][a-zA-Z0-9,+\\-._]{0,63}@[0-9a-fA-F]+(,[0-9a-fA-F]+)*$":

> 
> Why the commented regex. There's no need for a full regex as we already 
> do that elsewhere. You only need to define the unit-address format.

This should be
  "@[0-7],[a-f0-9]+$":

I added the full regex during debug but forgot to take it off.

> 
>> +    type: object
>> +    description: |
>> +      The child device node represents the device connected to the GPMC
>> +      bus. The device can be a NAND controller, SRAM device, NOR device
>> +      or an ASIC.
>> +
>> +    properties:
>> +      compatible:
>> +        description:
>> +          Compatible of attached device.
> 
> Duh. Drop.
> 
>> +
>> +      reg:
>> +        items:
>> +        - description: Register access space for the device
> 
> A device with 2 register ranges isn't allowed?

GPMC is actually a memory controller and we are describing the children here.
Each child has to have a register range.

> 
>> +
>> +# GPMC Timing properties for child nodes. All are optional and default to 0.
>> +
>> +      gpmc,sync-clk-ps:
>> +        description: Minimum clock period for synchronous mode, in picoseconds
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>  
>            default: 0
> 
> And elsewhere...
> 
>> +
>> +# Chip-select signal timings (in nanoseconds) corresponding to GPMC_CONFIG2:
>> +      gpmc,cs-on-ns:
>> +        description: Assertion time
>> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> Blank line between DT properties.
> 
> Don't need a type ref with a standard unit suffix.
> 
> Is there a maximum value for all these?
> 

There is a maximum value but it is not an absolute value in time unit (ns)
as it depends on the GPMC functional clock rate which is platform specific.

e.g. for gpmc,cs-on-ns
	maximum: GPMC clock period * 15

>> +      gpmc,cs-rd-off-ns:
>> +        description: Read deassertion time
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +      gpmc,cs-wr-off-ns:
>> +        description: Write deassertion time
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +

<snip>

>>  
>> diff --git a/Documentation/devicetree/bindings/net/gpmc-eth.txt b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>> index 32821066a85b..5e2f610455fa 100644
>> --- a/Documentation/devicetree/bindings/net/gpmc-eth.txt
>> +++ b/Documentation/devicetree/bindings/net/gpmc-eth.txt
>> @@ -9,7 +9,7 @@ the GPMC controller with an "ethernet" name.
>>  
>>  All timing relevant properties as well as generic GPMC child properties are
>>  explained in a separate documents. Please refer to
>> -Documentation/devicetree/bindings/memory-controllers/omap-gpmc.txt
>> +Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml
>>  
>>  For the properties relevant to the ethernet controller connected to the GPMC
>>  refer to the binding documentation of the device. For example, the documentation
>> @@ -43,7 +43,7 @@ Required properties:
>>  
>>  Optional properties:
>>  - gpmc,XXX		Additional GPMC timings and settings parameters. See
>> -			Documentation/devicetree/bindings/memory-controllers/omap-gpmc.txt
>> +			Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml
> 
> Perhaps delete this file first in the series so you don't have to 
> update it.
> 

Yes. Thanks for review :)

cheers,
-roger