Re: [Intel-xe] [PATCH 1/3] drm/xe: Add a couple of pcode helpers

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 12, 2023 at 10:38:50AM +0530, Sundaresan, Sujaritha wrote:
> 
> On 9/3/2023 7:00 PM, Sundaresan, Sujaritha wrote:
> > 
> > On 9/2/2023 2:04 AM, Rodrigo Vivi wrote:
> > > On Fri, Sep 01, 2023 at 05:45:43PM +0530, Sujaritha Sundaresan wrote:
> > > > Some pcode commands take additional sub-commands and parameters. Add a
> > > > couple of helpers to help formatting these commands to improve code
> > > > readability.
> > > > 
> > > > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_pcode.c | 28 ++++++++++++++++++++++++++++
> > > >   drivers/gpu/drm/xe/xe_pcode.h |  3 +++
> > > >   2 files changed, 31 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_pcode.c
> > > > b/drivers/gpu/drm/xe/xe_pcode.c
> > > > index 7f1bf2297f51..e45169f47500 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pcode.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pcode.c
> > > > @@ -104,6 +104,34 @@ int xe_pcode_read(struct xe_gt *gt, u32
> > > > mbox, u32 *val, u32 *val1)
> > > >       return err;
> > > >   }
> > > a doc would be required...
> > > 
> > > > +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 *val)
> > > a better name would be nice....
> > > 
> > > > +{
> > > > +    u32 mbox;
> > > > +    int err;
> > > > +
> > > > +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> > > > +
> > > > +    err = xe_pcode_read(gt, mbox, val, NULL);
> > > but why not simply modifying the existent one to accept 2 params?
> > > 
> > > int xe_pcode_read(struct xe_gt *gt, u32 mbox_param1, u32 mbox_param2,
> > >                u32 *val, u32 *val1)
> > > 
> > > and the equivalent write...
> > > 
> > > oh, and while doing that, could you please add the missing documentation
> > > to these 2 functions?
> > > 
> > > Thanks,
> > > Rodrigo.
> > 
> > Sure that would work. Will add the docs as well.
> > 
> > Thanks,
> > 
> > Suja
> 
> Hi Rodrigo,
> 
> Another question,
> 
> I can change the existing pcode_read function, but would it be better to
> have a separate new write equivalent ?

I wonder if we should do s/xe_pcode_write_timeout(/xe_pcode_write(

where timeout is still an argument but it can be null.
And then we merge with your options here and make a single write fn.

> 
> > 
> > > 
> > > > +
> > > > +    return err;
> > > > +}
> > > > +
> > > > +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 val)
> > > > +{
> > > > +    u32 mbox;
> > > > +    int err;
> > > > +
> > > > +    mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, mbcmd)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM1, p1)
> > > > +        | REG_FIELD_PREP(PCODE_MB_PARAM2, p2);
> > > > +
> > > > +    err = xe_pcode_write(gt, mbox, val);
> > > > +
> > > > +    return err;
> > > > +}
> > > > +
> > > >   static int xe_pcode_try_request(struct xe_gt *gt, u32 mbox,
> > > >                   u32 request, u32 reply_mask, u32 reply,
> > > >                   u32 *status, bool atomic, int timeout_us)
> > > > diff --git a/drivers/gpu/drm/xe/xe_pcode.h
> > > > b/drivers/gpu/drm/xe/xe_pcode.h
> > > > index 3b4aa8c1a3ba..8d4103afd7e0 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pcode.h
> > > > +++ b/drivers/gpu/drm/xe/xe_pcode.h
> > > > @@ -19,6 +19,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt,
> > > > u32 mbox, u32 val,
> > > >   #define xe_pcode_write(gt, mbox, val) \
> > > >       xe_pcode_write_timeout(gt, mbox, val, 1)
> > > >   +int xe_pcode_read_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 *val);
> > > > +int xe_pcode_write_p(struct xe_gt *gt, u32 mbcmd, u32 p1, u32
> > > > p2, u32 val);
> > > > +
> > > >   int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
> > > >                u32 reply_mask, u32 reply, int timeout_ms);
> > > >   --
> > > > 2.25.1
> > > >