Re: [PATCH v1 1/7] dt-bindings: i2c: qcom,i2c-geni: Document DT properties for QUP firmware loading

[Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>]

On 12/10/2024 11:12 PM, Doug Anderson wrote:
> Hi,
>
> On Mon, Dec 9, 2024 at 9:28 PM Viken Dadhaniya
> <quic_vdadhani@xxxxxxxxxxx> wrote:
> > On 12/4/2024 10:55 PM, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Dec 4, 2024 at 7:03 AM Viken Dadhaniya
> > > <quic_vdadhani@xxxxxxxxxxx> wrote:
> > > > Document the 'qcom,load-firmware' and 'qcom,xfer-mode' properties to
> > > > support SE(Serial Engine) firmware loading from the protocol driver and to
> > > > select the data transfer mode, either GPI DMA (Generic Packet Interface)
> > > > or non-GPI mode (PIO/CPU DMA).
> > > >
> > > > I2C controller can operate in one of two modes based on the
> > > > 'qcom,xfer-mode' property, and the firmware is loaded accordingly.
> > > >
> > > > Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> > > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> > > > Signed-off-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
> > > > ---
> > > >    .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml   | 11 +++++++++++
> > > >    1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> > > > index 9f66a3bb1f80..a26f34fce1bb 100644
> > > > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> > > > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> > > > @@ -66,6 +66,15 @@ properties:
> > > >      required-opps:
> > > >        maxItems: 1
> > > >
> > > > +  qcom,load-firmware:
> > > > +    type: boolean
> > > > +    description: Optional property to load SE (serial engine) Firmware from protocol driver.
> > > > +
> > > > +  qcom,xfer-mode:
> > > > +    description: Value 1,2 and 3 represents FIFO, CPU DMA and GSI DMA mode respectively.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [1, 2, 3]
> > >
> > > I'm a little confused about this. I'll admit I haven't fully analyzed
> > > your patch with actual code in it, but in the past "CPU DMA" mode and
> > > "FIFO" mode were compatible with each other and then it was up to the
> > > driver to decide which of the two modes made sense in any given
> > > situation. For instance, last I looked at the i2c driver it tried to
> > > use DMA for large transfers and FIFO for small transfers. The SPI
> > > driver also has some cases where it will use DMA mode and then
> > > fallback to FIFO mode.
> > >
> > > ...so what exactly is the point of differentiating between "FIFO" and
> > > "CPU DMA" mode here?
> >
> > Yes, correct, Will update in V2.
> > I plan to add 2 modes, GSI and non-GSI(PIO or DMA based on length).
> >
> > > Then when it comes to "GSI DMA" mode, my understanding is that the
> > > firmware for "GSI DMA" mode is always loaded by Trustzone because the
> > > whole point is that the GSI mode arbitrates between multiple clients.
> > > Presumably if the firmware already loaded the GSI firmware then the
> > > code would just detect that case. ...so there shouldn't need to be any
> > > reason to specify GSI mode here either, right?
> > >
> > > -Doug
> >
> > GSI firmware is loaded from TZ per QUP, but to use GSI mode,
> > we need to configure the SE to use GSI mode by writing into SE register
> > QUPV3_SE_GENI_DMA_MODE_EN and SE_GSI_EVENT_EN. This register is
> > used to configure data transfer mode for Serial Engine.
>
> Can't you detect it's in GSI mode without any device tree property
> like the code does today?
>
> -Doug

No, we can't detect GSI mode in the current design. The GSI firmware is 
loaded from the TZ side, while mode selection occurs on the APPS side 
based on the Device Tree property.