Re: [EXTERNAL] Re: [PATCH v5 5/5] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Tue, 28 Mar 2023 at 05:28, Md Danish Anwar <a0501179@ti.com> wrote:
>
>
>
> On 28/03/23 02:34, Mathieu Poirier wrote:
> > On Thu, Mar 23, 2023 at 11:54:51AM +0530, MD Danish Anwar wrote:
> >> From: Tero Kristo <t-kristo@ti.com>
> >>
> >> Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
> >> to get and set the GP MUX mode for programming the PRUSS internal wrapper
> >> mux functionality as needed by usecases.
> >>
> >> Co-developed-by: Suman Anna <s-anna@ti.com>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> >> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> >> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> >> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> >> Reviewed-by: Roger Quadros <rogerq@kernel.org>
> >> ---
> >>  drivers/soc/ti/pruss.c           | 44 ++++++++++++++++++++++++++++++++
> >>  include/linux/remoteproc/pruss.h | 30 ++++++++++++++++++++++
> >>  2 files changed, 74 insertions(+)
> >>
> >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> >> index ac415442e85b..3aa3c38c6c79 100644
> >> --- a/drivers/soc/ti/pruss.c
> >> +++ b/drivers/soc/ti/pruss.c
> >> @@ -239,6 +239,50 @@ int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
> >>  }
> >>  EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable);
> >>
> >> +/**
> >> + * pruss_cfg_get_gpmux() - get the current GPMUX value for a PRU device
> >> + * @pruss: pruss instance
> >> + * @pru_id: PRU identifier (0-1)
> >> + * @mux: pointer to store the current mux value into
> >> + *
> >> + * Return: 0 on success, or an error code otherwise
> >> + */
> >> +int pruss_cfg_get_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 *mux)
> >> +{
> >> +    int ret = 0;
> >> +    u32 val;
> >> +
> >> +    if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
> >> +            return -EINVAL;
> >> +
> >> +    ret = pruss_cfg_read(pruss, PRUSS_CFG_GPCFG(pru_id), &val);
> >> +    if (!ret)
> >> +            *mux = (u8)((val & PRUSS_GPCFG_PRU_MUX_SEL_MASK) >>
> >> +                        PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
> >
> > What happens if @mux is NULL?
>
> @mux being null may result in some error here. I will add NULL check for mux
> before storing the value in mux.
>

It will result in a kernel panic.

> I will modify the above if condition to have NULL check for mux as well.
> The if condition will look like below.
>
>         if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS || !mux)
>                 return -EINVAL;
>

That will be fine.

> Please let me know if this looks OK.
>
> >
> > Thanks,
> > Mathieu
> >
> >
> >> +    return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pruss_cfg_get_gpmux);
> >> +
> >> +/**
> >> + * pruss_cfg_set_gpmux() - set the GPMUX value for a PRU device
> >> + * @pruss: pruss instance
> >> + * @pru_id: PRU identifier (0-1)
> >> + * @mux: new mux value for PRU
> >> + *
> >> + * Return: 0 on success, or an error code otherwise
> >> + */
> >> +int pruss_cfg_set_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 mux)
> >> +{
> >> +    if (mux >= PRUSS_GP_MUX_SEL_MAX ||
> >> +        pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
> >> +            return -EINVAL;
> >> +
> >> +    return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
> >> +                            PRUSS_GPCFG_PRU_MUX_SEL_MASK,
> >> +                            (u32)mux << PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
> >> +}
> >> +EXPORT_SYMBOL_GPL(pruss_cfg_set_gpmux);
> >> +
> >>  static void pruss_of_free_clk_provider(void *data)
> >>  {
> >>      struct device_node *clk_mux_np = data;
> >> diff --git a/include/linux/remoteproc/pruss.h b/include/linux/remoteproc/pruss.h
> >> index bb001f712980..42f1586c62ac 100644
> >> --- a/include/linux/remoteproc/pruss.h
> >> +++ b/include/linux/remoteproc/pruss.h
> >> @@ -16,6 +16,24 @@
> >>
> >>  #define PRU_RPROC_DRVNAME "pru-rproc"
> >>
> >> +/*
> >> + * enum pruss_gp_mux_sel - PRUSS GPI/O Mux modes for the
> >> + * PRUSS_GPCFG0/1 registers
> >> + *
> >> + * NOTE: The below defines are the most common values, but there
> >> + * are some exceptions like on 66AK2G, where the RESERVED and MII2
> >> + * values are interchanged. Also, this bit-field does not exist on
> >> + * AM335x SoCs
> >> + */
> >> +enum pruss_gp_mux_sel {
> >> +    PRUSS_GP_MUX_SEL_GP = 0,
> >> +    PRUSS_GP_MUX_SEL_ENDAT,
> >> +    PRUSS_GP_MUX_SEL_RESERVED,
> >> +    PRUSS_GP_MUX_SEL_SD,
> >> +    PRUSS_GP_MUX_SEL_MII2,
> >> +    PRUSS_GP_MUX_SEL_MAX,
> >> +};
> >> +
> >>  /*
> >>   * enum pruss_gpi_mode - PRUSS GPI configuration modes, used
> >>   *                   to program the PRUSS_GPCFG0/1 registers
> >> @@ -110,6 +128,8 @@ int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id,
> >>  int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable);
> >>  int pruss_cfg_xfr_enable(struct pruss *pruss, enum pru_type pru_type,
> >>                       bool enable);
> >> +int pruss_cfg_get_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 *mux);
> >> +int pruss_cfg_set_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 mux);
> >>
> >>  #else
> >>
> >> @@ -152,6 +172,16 @@ static inline int pruss_cfg_xfr_enable(struct pruss *pruss,
> >>      return ERR_PTR(-EOPNOTSUPP);
> >>  }
> >>
> >> +static inline int pruss_cfg_get_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 *mux)
> >> +{
> >> +    return ERR_PTR(-EOPNOTSUPP);
> >> +}
> >> +
> >> +static inline int pruss_cfg_set_gpmux(struct pruss *pruss, enum pruss_pru_id pru_id, u8 mux)
> >> +{
> >> +    return ERR_PTR(-EOPNOTSUPP);
> >> +}
> >> +
> >>  #endif /* CONFIG_TI_PRUSS */
> >>
> >>  #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)
> >> --
> >> 2.25.1
> >>
>
> --
> Thanks and Regards,
> Danish.