drivers: stm32-fmc-nor-psram doesn't offer data hold parameter

[ts4ling]

st,stm32-fmc-nor-psram.yaml forces you to define 7 parameters. But there are 8 for H562:

The one missing is Data hold. It would be used if filled with data, but currently it's not possible to do so. See memc_stm32_nor_psram.c.

An easy fix would look like this

        .DataSetupTime = DT_PROP_BY_IDX(node_id, st_timing, 2),             \
        .DataHoldTime = DT_PROP_BY_IDX(node_id, st_timing, NEW_IDX),             \
        .BusTurnAroundDuration = DT_PROP_BY_IDX(node_id, st_timing, 3),     \

But at this point I have no idea what to suggest.

• Renumbering the indices would break stuff. But might be acceptable for the 4.0 release?
• Continuing with 8 looks ugly.

Maybe someone from ST could comment on this. Thanks.

[erwango]

On my side, I don't see issue with using index 8:

        .DataSetupTime = DT_PROP_BY_IDX(node_id, st_timing, 2),             \
        .DataHoldTime = DT_PROP_BY_IDX(node_id, st_timing, 8),             \
        .BusTurnAroundDuration = DT_PROP_BY_IDX(node_id, st_timing, 3),     \

Note that this is not H5 specific as this parameter also exists for G4/L4/L5/MP1/U5, with users (at least in tree) for L5 and U5, and it apparently works w/o for now. To better understand the demand, do you actually need to be able to configure this parameter or you're just asking as it seems wrong that it is missing ?

One idea would be to add a new optional property which would allow to fill this value if present (out of st-timings property, st,timing-data-hold for instance). Other idea would be to add a new dedicated compatible which would accept this new param in st-timings property but this would be much more heavy and would break existing users.

@GeorgeCGV any better idea or alternative view ?

[GeorgeCGV]

It wasn't added because it doesn't exist in used h730 during PR.

It is easier and cleaner to have an optional property for as long as it is just 1-2. A BUILD_ASSERT can be added to fail builds where DataHoldTime is required but not set.

If more stuff (i.e. more than 1-2 parameters) is missing for H5/G4/L4/L5/MP1/U5, then a new dedicated compatible would be better.

[erwango]

A BUILD_ASSERT can be added to fail builds where DataHoldTime is required but not set.

Alternatively we can make property required in a new compatible which could inherit other properties from the initial one. This way error is directly generated by dt parser.

[ts4ling]

I had issues making my display work. But it wasn't related to timing but configuration issues.

    ili9341: lcd-panel@0 {
        compatible = "ilitek,ili9341";
        reg = <0>;
        pixel-format = <ILI9XXX_PIXEL_FORMAT_RGB565>;
        mipi-mode = <MIPI_DBI_MODE_8080_BUS_16_BIT>;
        mipi-max-frequency = <DT_FREQ_M(10)>;
        rotation = <270>;
        width = <320>;
        height = <240>;
        display-inversion;
        pwctrla = [39 2c 00 34 02];//
        pwctrlb = [00 c1 30];//
        timctrla = [85 00 78];//
        timctrlb = [00 00];//
        pwseqctrl = [64 03 12 81];//
        pumpratioctrl = [20];//
        disctrl = [0a 82 27 04];//
        vmctrl1 = [31 3c];//
        vmctrl2 = [90];//
        enable3g = [00];//
        ifctl = [01 00 06]; <-- doesn't work, [01 00 00] does!
        ifmode = [40];//
        gamset = [01];//
        frmctr1 = [00 1b];//
        pwctrl1 = [21];//
        pwctrl2 = [10];//
        pgamctrl = [0f 29 24 0c 0e 09 4e 78 3c 09 13 05 17 11 00];//
        ngamctrl = [00 16 1b 04 11 07 31 33 42 05 0c 0a 28 2f 0f];//
    };

I'd close this issue if there's no objection.