| Message ID | 20230313111127.1229187-5-danishanwar@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce PRU platform consumer API | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 13/03/2023 13:11, MD Danish Anwar wrote: > From: Suman Anna <s-anna@ti.com> > > The PRUSS CFG module is represented as a syscon node and is currently > managed by the PRUSS platform driver. Add easy accessor functions to set > GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable > to enable the PRUSS Ethernet usecase. These functions reuse the generic > pruss_cfg_update() API function. > > Signed-off-by: Suman Anna <s-anna@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> > --- > drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++ > include/linux/remoteproc/pruss.h | 22 ++++++++++++ > 2 files changed, 82 insertions(+) > > diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c > index 26d8129b515c..2f04b7922ddb 100644 > --- a/drivers/soc/ti/pruss.c > +++ b/drivers/soc/ti/pruss.c > @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, > return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); > } > > +/** > + * pruss_cfg_gpimode() - set the GPI mode of the PRU > + * @pruss: the pruss instance handle > + * @pru_id: id of the PRU core within the PRUSS > + * @mode: GPI mode to set > + * > + * Sets the GPI mode for a given PRU by programming the > + * corresponding PRUSS_CFG_GPCFGx register > + * > + * Return: 0 on success, or an error code otherwise > + */ > +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, > + enum pruss_gpi_mode mode) > +{ > + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) > + return -EINVAL; > + > + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) > + return -EINVAL; > + > + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), > + PRUSS_GPCFG_PRU_GPI_MODE_MASK, > + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); > +} > +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); > + > +/** > + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events > + * @pruss: the pruss instance > + * @enable: enable/disable > + * > + * Enable/disable the MII RT Events for the PRUSS. > + * > + * Return: 0 on success, or an error code otherwise > + */ > +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) > +{ > + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; > + > + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, > + PRUSS_MII_RT_EVENT_EN, set); > +} > +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); > + > +/** > + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality > + * @pruss: the pruss instance > + * @enable: enable/disable > + * @mask: Mask for PRU / RTU You should not expect the user to provide the mask but only the core type e.g. enum pru_type { PRU_TYPE_PRU = 0, PRU_TYPE_RTU, PRU_TYPE_TX_PRU, PRU_TYPE_MAX, }; Then you figure out the mask in the function. Also check for invalid pru_type and return error if so. > + * > + * Return: 0 on success, or an error code otherwise > + */ > +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask) re-arrange so it is (struct pruss, enum pru_type, bool enable) > +{ > + u32 set = enable ? mask : 0; > + > + return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set); > +} > +EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable); > + > 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 12ef10b9fe9a..51a3eedd2be6 100644 > --- a/include/linux/remoteproc/pruss.h > +++ b/include/linux/remoteproc/pruss.h > @@ -101,6 +101,7 @@ enum pruss_gpi_mode { > PRUSS_GPI_MODE_PARALLEL, > PRUSS_GPI_MODE_28BIT_SHIFT, > PRUSS_GPI_MODE_MII, > + PRUSS_GPI_MODE_MAX, This could have come as part of patch 3. > }; > > /** > @@ -165,6 +166,10 @@ int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, > struct pruss_mem_region *region); > int pruss_release_mem_region(struct pruss *pruss, > struct pruss_mem_region *region); > +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, > + enum pruss_gpi_mode mode); > +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable); > +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask); > > #else > > @@ -188,6 +193,23 @@ static inline int pruss_release_mem_region(struct pruss *pruss, > return -EOPNOTSUPP; > } > > +static inline int pruss_cfg_gpimode(struct pruss *pruss, > + enum pruss_pru_id pru_id, > + enum pruss_gpi_mode mode) > +{ > + return ERR_PTR(-EOPNOTSUPP); > +} > + > +static inline int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) > +{ > + return ERR_PTR(-EOPNOTSUPP); > +} > + > +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask) > +{ > + return ERR_PTR(-EOPNOTSUPP); > +} > + > #endif /* CONFIG_TI_PRUSS */ > > #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) cheers, -roger
Hi Roger, On 15/03/23 17:52, Roger Quadros wrote: > > > On 13/03/2023 13:11, MD Danish Anwar wrote: >> From: Suman Anna <s-anna@ti.com> >> >> The PRUSS CFG module is represented as a syscon node and is currently >> managed by the PRUSS platform driver. Add easy accessor functions to set >> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable >> to enable the PRUSS Ethernet usecase. These functions reuse the generic >> pruss_cfg_update() API function. >> >> Signed-off-by: Suman Anna <s-anna@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> >> --- >> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++ >> include/linux/remoteproc/pruss.h | 22 ++++++++++++ >> 2 files changed, 82 insertions(+) >> >> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >> index 26d8129b515c..2f04b7922ddb 100644 >> --- a/drivers/soc/ti/pruss.c >> +++ b/drivers/soc/ti/pruss.c >> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, >> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); >> } >> >> +/** >> + * pruss_cfg_gpimode() - set the GPI mode of the PRU >> + * @pruss: the pruss instance handle >> + * @pru_id: id of the PRU core within the PRUSS >> + * @mode: GPI mode to set >> + * >> + * Sets the GPI mode for a given PRU by programming the >> + * corresponding PRUSS_CFG_GPCFGx register >> + * >> + * Return: 0 on success, or an error code otherwise >> + */ >> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, >> + enum pruss_gpi_mode mode) >> +{ >> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) >> + return -EINVAL; >> + >> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) >> + return -EINVAL; >> + >> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), >> + PRUSS_GPCFG_PRU_GPI_MODE_MASK, >> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); >> +} >> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); >> + >> +/** >> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events >> + * @pruss: the pruss instance >> + * @enable: enable/disable >> + * >> + * Enable/disable the MII RT Events for the PRUSS. >> + * >> + * Return: 0 on success, or an error code otherwise >> + */ >> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) >> +{ >> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; >> + >> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, >> + PRUSS_MII_RT_EVENT_EN, set); >> +} >> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); >> + >> +/** >> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality >> + * @pruss: the pruss instance >> + * @enable: enable/disable >> + * @mask: Mask for PRU / RTU > > You should not expect the user to provide the mask but only > the core type e.g. > > enum pru_type { > PRU_TYPE_PRU = 0, > PRU_TYPE_RTU, > PRU_TYPE_TX_PRU, > PRU_TYPE_MAX, > }; > > Then you figure out the mask in the function. > Also check for invalid pru_type and return error if so. > Sure Roger, I will create a enum and take it as parameter in API. Based on these enum I will calculate mask and do XFR shifting inside the API pruss_cfg_xfr_enable(). There are two registers for XFR shift. #define PRUSS_SPP_XFER_SHIFT_EN BIT(1) #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3) For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN, for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN) So the enum would be something like this. /** * enum xfr_shift_type - XFR shift type * @XFR_SHIFT_PRU: Enables XFR shift for PRU * @XFR_SHIFT_RTU: Enables XFR shift for RTU * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU * @XFR_SHIFT_MAX: Total number of XFR shift types available. * */ enum xfr_shift_type { XFR_SHIFT_PRU = 0, XFR_SHIFT_RTU, XFR_SHIFT_PRU_RTU, XFR_SHIFT_MAX, }; In pruss_cfg_xfr_enable() API, I will use switch case, and for first three enums, I will calculate the mask. If input is anything other than first three, I will retun -EINVAL. This will serve as check for valid xfr_shift_type. The API will look like this. int pruss_cfg_xfr_enable(struct pruss *pruss, enum xfr_shift_type xfr_type, bool enable); { u32 mask; switch (xfr_type) { case XFR_SHIFT_PRU: mask = PRUSS_SPP_XFER_SHIFT_EN; break; case XFR_SHIFT_RTU: mask = PRUSS_SPP_RTU_XFR_SHIFT_EN; break; case XFR_SHIFT_PRU_RTU: mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; break; default: return -EINVAL; } u32 set = enable ? mask : 0; return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set); } This entire change I will keep as part of this patch only. Please let me know if this looks OK to you. >> + * >> + * Return: 0 on success, or an error code otherwise >> + */ >> +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask) > > re-arrange so it is (struct pruss, enum pru_type, bool enable) > >> +{ >> + u32 set = enable ? mask : 0; >> + >> + return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set); >> +} >> +EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable); >> + >> 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 12ef10b9fe9a..51a3eedd2be6 100644 >> --- a/include/linux/remoteproc/pruss.h >> +++ b/include/linux/remoteproc/pruss.h >> @@ -101,6 +101,7 @@ enum pruss_gpi_mode { >> PRUSS_GPI_MODE_PARALLEL, >> PRUSS_GPI_MODE_28BIT_SHIFT, >> PRUSS_GPI_MODE_MII, >> + PRUSS_GPI_MODE_MAX, > > This could have come as part of patch 3. > >> }; >> >> /** >> @@ -165,6 +166,10 @@ int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, >> struct pruss_mem_region *region); >> int pruss_release_mem_region(struct pruss *pruss, >> struct pruss_mem_region *region); >> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, >> + enum pruss_gpi_mode mode); >> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable); >> +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask); >> >> #else >> >> @@ -188,6 +193,23 @@ static inline int pruss_release_mem_region(struct pruss *pruss, >> return -EOPNOTSUPP; >> } >> >> +static inline int pruss_cfg_gpimode(struct pruss *pruss, >> + enum pruss_pru_id pru_id, >> + enum pruss_gpi_mode mode) >> +{ >> + return ERR_PTR(-EOPNOTSUPP); >> +} >> + >> +static inline int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) >> +{ >> + return ERR_PTR(-EOPNOTSUPP); >> +} >> + >> +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask) >> +{ >> + return ERR_PTR(-EOPNOTSUPP); >> +} >> + >> #endif /* CONFIG_TI_PRUSS */ >> >> #if IS_ENABLED(CONFIG_PRU_REMOTEPROC) > > cheers, > -roger
Hi, On 16/03/2023 13:05, Md Danish Anwar wrote: > Hi Roger, > > On 15/03/23 17:52, Roger Quadros wrote: >> >> >> On 13/03/2023 13:11, MD Danish Anwar wrote: >>> From: Suman Anna <s-anna@ti.com> >>> >>> The PRUSS CFG module is represented as a syscon node and is currently >>> managed by the PRUSS platform driver. Add easy accessor functions to set >>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable >>> to enable the PRUSS Ethernet usecase. These functions reuse the generic >>> pruss_cfg_update() API function. >>> >>> Signed-off-by: Suman Anna <s-anna@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> >>> --- >>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++ >>> include/linux/remoteproc/pruss.h | 22 ++++++++++++ >>> 2 files changed, 82 insertions(+) >>> >>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>> index 26d8129b515c..2f04b7922ddb 100644 >>> --- a/drivers/soc/ti/pruss.c >>> +++ b/drivers/soc/ti/pruss.c >>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, >>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); >>> } >>> >>> +/** >>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU >>> + * @pruss: the pruss instance handle >>> + * @pru_id: id of the PRU core within the PRUSS >>> + * @mode: GPI mode to set >>> + * >>> + * Sets the GPI mode for a given PRU by programming the >>> + * corresponding PRUSS_CFG_GPCFGx register >>> + * >>> + * Return: 0 on success, or an error code otherwise >>> + */ >>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, >>> + enum pruss_gpi_mode mode) >>> +{ >>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) >>> + return -EINVAL; >>> + >>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) >>> + return -EINVAL; >>> + >>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), >>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK, >>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); >>> +} >>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); >>> + >>> +/** >>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events >>> + * @pruss: the pruss instance >>> + * @enable: enable/disable >>> + * >>> + * Enable/disable the MII RT Events for the PRUSS. >>> + * >>> + * Return: 0 on success, or an error code otherwise >>> + */ >>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) >>> +{ >>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; >>> + >>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, >>> + PRUSS_MII_RT_EVENT_EN, set); >>> +} >>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); >>> + >>> +/** >>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality >>> + * @pruss: the pruss instance >>> + * @enable: enable/disable >>> + * @mask: Mask for PRU / RTU >> >> You should not expect the user to provide the mask but only >> the core type e.g. >> >> enum pru_type { >> PRU_TYPE_PRU = 0, >> PRU_TYPE_RTU, >> PRU_TYPE_TX_PRU, >> PRU_TYPE_MAX, >> }; >> >> Then you figure out the mask in the function. >> Also check for invalid pru_type and return error if so. >> > > Sure Roger, I will create a enum and take it as parameter in API. Based on > these enum I will calculate mask and do XFR shifting inside the API > pruss_cfg_xfr_enable(). > > There are two registers for XFR shift. > > #define PRUSS_SPP_XFER_SHIFT_EN BIT(1) > #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3) > > For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN, > for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU > shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN) > > So the enum would be something like this. > > /** > * enum xfr_shift_type - XFR shift type > * @XFR_SHIFT_PRU: Enables XFR shift for PRU > * @XFR_SHIFT_RTU: Enables XFR shift for RTU > * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU This is not required. User can call the API twice. once for PRU and once for RTU. > * @XFR_SHIFT_MAX: Total number of XFR shift types available. > * > */ > > enum xfr_shift_type { > XFR_SHIFT_PRU = 0, > XFR_SHIFT_RTU, > XFR_SHIFT_PRU_RTU, > XFR_SHIFT_MAX, > }; Why do you need this new enum definition? We already have pru_type defined somewhere. You can move it to a public header if not there yet. enum pru_type { PRU_TYPE_PRU = 0, PRU_TYPE_RTU, PRU_TYPE_TX_PRU, PRU_TYPE_MAX, }; > > In pruss_cfg_xfr_enable() API, I will use switch case, and for first three > enums, I will calculate the mask. > > If input is anything other than first three, I will retun -EINVAL. This will > serve as check for valid xfr_shift_type. > > The API will look like this. > > int pruss_cfg_xfr_enable(struct pruss *pruss, enum xfr_shift_type xfr_type, > bool enable); > { > u32 mask; > > switch (xfr_type) { > case XFR_SHIFT_PRU: > mask = PRUSS_SPP_XFER_SHIFT_EN; > break; > case XFR_SHIFT_RTU: > mask = PRUSS_SPP_RTU_XFR_SHIFT_EN; > break; > case XFR_SHIFT_PRU_RTU: > mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; > break; > default: > return -EINVAL; > } > > u32 set = enable ? mask : 0; > > return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set); > } > > This entire change I will keep as part of this patch only. > > Please let me know if this looks OK to you. > > cheers, -roger
On 16/03/23 17:06, Roger Quadros wrote: > Hi, > > On 16/03/2023 13:05, Md Danish Anwar wrote: >> Hi Roger, >> >> On 15/03/23 17:52, Roger Quadros wrote: >>> >>> >>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>> From: Suman Anna <s-anna@ti.com> >>>> >>>> The PRUSS CFG module is represented as a syscon node and is currently >>>> managed by the PRUSS platform driver. Add easy accessor functions to set >>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable >>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic >>>> pruss_cfg_update() API function. >>>> >>>> Signed-off-by: Suman Anna <s-anna@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> >>>> --- >>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++ >>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++ >>>> 2 files changed, 82 insertions(+) >>>> >>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>> index 26d8129b515c..2f04b7922ddb 100644 >>>> --- a/drivers/soc/ti/pruss.c >>>> +++ b/drivers/soc/ti/pruss.c >>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, >>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); >>>> } >>>> >>>> +/** >>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU >>>> + * @pruss: the pruss instance handle >>>> + * @pru_id: id of the PRU core within the PRUSS >>>> + * @mode: GPI mode to set >>>> + * >>>> + * Sets the GPI mode for a given PRU by programming the >>>> + * corresponding PRUSS_CFG_GPCFGx register >>>> + * >>>> + * Return: 0 on success, or an error code otherwise >>>> + */ >>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, >>>> + enum pruss_gpi_mode mode) >>>> +{ >>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) >>>> + return -EINVAL; >>>> + >>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) >>>> + return -EINVAL; >>>> + >>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), >>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK, >>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); >>>> +} >>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); >>>> + >>>> +/** >>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events >>>> + * @pruss: the pruss instance >>>> + * @enable: enable/disable >>>> + * >>>> + * Enable/disable the MII RT Events for the PRUSS. >>>> + * >>>> + * Return: 0 on success, or an error code otherwise >>>> + */ >>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) >>>> +{ >>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; >>>> + >>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, >>>> + PRUSS_MII_RT_EVENT_EN, set); >>>> +} >>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); >>>> + >>>> +/** >>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality >>>> + * @pruss: the pruss instance >>>> + * @enable: enable/disable >>>> + * @mask: Mask for PRU / RTU >>> >>> You should not expect the user to provide the mask but only >>> the core type e.g. >>> >>> enum pru_type { >>> PRU_TYPE_PRU = 0, >>> PRU_TYPE_RTU, >>> PRU_TYPE_TX_PRU, >>> PRU_TYPE_MAX, >>> }; >>> >>> Then you figure out the mask in the function. >>> Also check for invalid pru_type and return error if so. >>> >> >> Sure Roger, I will create a enum and take it as parameter in API. Based on >> these enum I will calculate mask and do XFR shifting inside the API >> pruss_cfg_xfr_enable(). >> >> There are two registers for XFR shift. >> >> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1) >> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3) >> >> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN, >> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU >> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN) >> >> So the enum would be something like this. >> >> /** >> * enum xfr_shift_type - XFR shift type >> * @XFR_SHIFT_PRU: Enables XFR shift for PRU >> * @XFR_SHIFT_RTU: Enables XFR shift for RTU >> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU > > This is not required. User can call the API twice. once for PRU and once for RTU. > >> * @XFR_SHIFT_MAX: Total number of XFR shift types available. >> * >> */ >> >> enum xfr_shift_type { >> XFR_SHIFT_PRU = 0, >> XFR_SHIFT_RTU, >> XFR_SHIFT_PRU_RTU, >> XFR_SHIFT_MAX, >> }; > > Why do you need this new enum definition? > We already have pru_type defined somewhere. You can move it to a public header > if not there yet. > > enum pru_type { > PRU_TYPE_PRU = 0, > PRU_TYPE_RTU, > PRU_TYPE_TX_PRU, > PRU_TYPE_MAX, > }; > This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem with this enum is that in [1] we need to enable XFR shift for both PRU and RTU for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU). Now this enum doesn't have a field for both PRU and RTU. Also we don't need need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are defined. That is why I thought of introducing new enum. [1] drivers/net/ethernet/ti/icssg_config.c /* enable XFR shift for PRU and RTU */ mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; > >> >> In pruss_cfg_xfr_enable() API, I will use switch case, and for first three >> enums, I will calculate the mask. >> >> If input is anything other than first three, I will retun -EINVAL. This will >> serve as check for valid xfr_shift_type. >> >> The API will look like this. >> >> int pruss_cfg_xfr_enable(struct pruss *pruss, enum xfr_shift_type xfr_type, >> bool enable); >> { >> u32 mask; >> >> switch (xfr_type) { >> case XFR_SHIFT_PRU: >> mask = PRUSS_SPP_XFER_SHIFT_EN; >> break; >> case XFR_SHIFT_RTU: >> mask = PRUSS_SPP_RTU_XFR_SHIFT_EN; >> break; >> case XFR_SHIFT_PRU_RTU: >> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; >> break; >> default: >> return -EINVAL; >> } >> >> u32 set = enable ? mask : 0; >> >> return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set); >> } >> >> This entire change I will keep as part of this patch only. >> >> Please let me know if this looks OK to you. >> >> > > cheers, > -roger
On 16/03/2023 13:44, Md Danish Anwar wrote: > > On 16/03/23 17:06, Roger Quadros wrote: >> Hi, >> >> On 16/03/2023 13:05, Md Danish Anwar wrote: >>> Hi Roger, >>> >>> On 15/03/23 17:52, Roger Quadros wrote: >>>> >>>> >>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>> From: Suman Anna <s-anna@ti.com> >>>>> >>>>> The PRUSS CFG module is represented as a syscon node and is currently >>>>> managed by the PRUSS platform driver. Add easy accessor functions to set >>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable >>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic >>>>> pruss_cfg_update() API function. >>>>> >>>>> Signed-off-by: Suman Anna <s-anna@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> >>>>> --- >>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++ >>>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++ >>>>> 2 files changed, 82 insertions(+) >>>>> >>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>>> index 26d8129b515c..2f04b7922ddb 100644 >>>>> --- a/drivers/soc/ti/pruss.c >>>>> +++ b/drivers/soc/ti/pruss.c >>>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, >>>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); >>>>> } >>>>> >>>>> +/** >>>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU >>>>> + * @pruss: the pruss instance handle >>>>> + * @pru_id: id of the PRU core within the PRUSS >>>>> + * @mode: GPI mode to set >>>>> + * >>>>> + * Sets the GPI mode for a given PRU by programming the >>>>> + * corresponding PRUSS_CFG_GPCFGx register >>>>> + * >>>>> + * Return: 0 on success, or an error code otherwise >>>>> + */ >>>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, >>>>> + enum pruss_gpi_mode mode) >>>>> +{ >>>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) >>>>> + return -EINVAL; >>>>> + >>>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) >>>>> + return -EINVAL; >>>>> + >>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), >>>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK, >>>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); >>>>> + >>>>> +/** >>>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events >>>>> + * @pruss: the pruss instance >>>>> + * @enable: enable/disable >>>>> + * >>>>> + * Enable/disable the MII RT Events for the PRUSS. >>>>> + * >>>>> + * Return: 0 on success, or an error code otherwise >>>>> + */ >>>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) >>>>> +{ >>>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; >>>>> + >>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, >>>>> + PRUSS_MII_RT_EVENT_EN, set); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); >>>>> + >>>>> +/** >>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality >>>>> + * @pruss: the pruss instance >>>>> + * @enable: enable/disable >>>>> + * @mask: Mask for PRU / RTU >>>> >>>> You should not expect the user to provide the mask but only >>>> the core type e.g. >>>> >>>> enum pru_type { >>>> PRU_TYPE_PRU = 0, >>>> PRU_TYPE_RTU, >>>> PRU_TYPE_TX_PRU, >>>> PRU_TYPE_MAX, >>>> }; >>>> >>>> Then you figure out the mask in the function. >>>> Also check for invalid pru_type and return error if so. >>>> >>> >>> Sure Roger, I will create a enum and take it as parameter in API. Based on >>> these enum I will calculate mask and do XFR shifting inside the API >>> pruss_cfg_xfr_enable(). >>> >>> There are two registers for XFR shift. >>> >>> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1) >>> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3) >>> >>> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN, >>> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU >>> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN) >>> >>> So the enum would be something like this. >>> >>> /** >>> * enum xfr_shift_type - XFR shift type >>> * @XFR_SHIFT_PRU: Enables XFR shift for PRU >>> * @XFR_SHIFT_RTU: Enables XFR shift for RTU >>> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU >> >> This is not required. User can call the API twice. once for PRU and once for RTU. >> >>> * @XFR_SHIFT_MAX: Total number of XFR shift types available. >>> * >>> */ >>> >>> enum xfr_shift_type { >>> XFR_SHIFT_PRU = 0, >>> XFR_SHIFT_RTU, >>> XFR_SHIFT_PRU_RTU, >>> XFR_SHIFT_MAX, >>> }; >> >> Why do you need this new enum definition? >> We already have pru_type defined somewhere. You can move it to a public header >> if not there yet. >> >> enum pru_type { >> PRU_TYPE_PRU = 0, >> PRU_TYPE_RTU, >> PRU_TYPE_TX_PRU, >> PRU_TYPE_MAX, >> }; >> > > This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem > with this enum is that in [1] we need to enable XFR shift for both PRU and RTU > for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and > PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU). Is there any limitation that you have to enable both simultaneously? The driver can first do enable for PRU and then later for RTU. As you will do a read modify write, the previous enable state of register shouldn't be affected. > > Now this enum doesn't have a field for both PRU and RTU. Also we don't need > need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are > defined. That is OK. You can return error if not RTU or PRU. > > That is why I thought of introducing new enum. > > [1] drivers/net/ethernet/ti/icssg_config.c > > /* enable XFR shift for PRU and RTU */ > mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; Driver can do like so pruss_cfg_xfr_enable(pruss, PRU_TYPE_PRU, true); pruss_cfg_xfr_enable(pruss, PRU_TYPE_RTU, true); The second call should not disable the PRU XFR as you will do a read-modify-write only affecting the RTU bit. cheers, -roger
On 16/03/23 17:49, Roger Quadros wrote: > > > On 16/03/2023 13:44, Md Danish Anwar wrote: >> >> On 16/03/23 17:06, Roger Quadros wrote: >>> Hi, >>> >>> On 16/03/2023 13:05, Md Danish Anwar wrote: >>>> Hi Roger, >>>> >>>> On 15/03/23 17:52, Roger Quadros wrote: >>>>> >>>>> >>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>> From: Suman Anna <s-anna@ti.com> >>>>>> >>>>>> The PRUSS CFG module is represented as a syscon node and is currently >>>>>> managed by the PRUSS platform driver. Add easy accessor functions to set >>>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable >>>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic >>>>>> pruss_cfg_update() API function. >>>>>> >>>>>> Signed-off-by: Suman Anna <s-anna@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> >>>>>> --- >>>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++ >>>>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++ >>>>>> 2 files changed, 82 insertions(+) >>>>>> >>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>>>> index 26d8129b515c..2f04b7922ddb 100644 >>>>>> --- a/drivers/soc/ti/pruss.c >>>>>> +++ b/drivers/soc/ti/pruss.c >>>>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, >>>>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); >>>>>> } >>>>>> >>>>>> +/** >>>>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU >>>>>> + * @pruss: the pruss instance handle >>>>>> + * @pru_id: id of the PRU core within the PRUSS >>>>>> + * @mode: GPI mode to set >>>>>> + * >>>>>> + * Sets the GPI mode for a given PRU by programming the >>>>>> + * corresponding PRUSS_CFG_GPCFGx register >>>>>> + * >>>>>> + * Return: 0 on success, or an error code otherwise >>>>>> + */ >>>>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, >>>>>> + enum pruss_gpi_mode mode) >>>>>> +{ >>>>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), >>>>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK, >>>>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); >>>>>> + >>>>>> +/** >>>>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events >>>>>> + * @pruss: the pruss instance >>>>>> + * @enable: enable/disable >>>>>> + * >>>>>> + * Enable/disable the MII RT Events for the PRUSS. >>>>>> + * >>>>>> + * Return: 0 on success, or an error code otherwise >>>>>> + */ >>>>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) >>>>>> +{ >>>>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; >>>>>> + >>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, >>>>>> + PRUSS_MII_RT_EVENT_EN, set); >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); >>>>>> + >>>>>> +/** >>>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality >>>>>> + * @pruss: the pruss instance >>>>>> + * @enable: enable/disable >>>>>> + * @mask: Mask for PRU / RTU >>>>> >>>>> You should not expect the user to provide the mask but only >>>>> the core type e.g. >>>>> >>>>> enum pru_type { >>>>> PRU_TYPE_PRU = 0, >>>>> PRU_TYPE_RTU, >>>>> PRU_TYPE_TX_PRU, >>>>> PRU_TYPE_MAX, >>>>> }; >>>>> >>>>> Then you figure out the mask in the function. >>>>> Also check for invalid pru_type and return error if so. >>>>> >>>> >>>> Sure Roger, I will create a enum and take it as parameter in API. Based on >>>> these enum I will calculate mask and do XFR shifting inside the API >>>> pruss_cfg_xfr_enable(). >>>> >>>> There are two registers for XFR shift. >>>> >>>> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1) >>>> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3) >>>> >>>> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN, >>>> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU >>>> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN) >>>> >>>> So the enum would be something like this. >>>> >>>> /** >>>> * enum xfr_shift_type - XFR shift type >>>> * @XFR_SHIFT_PRU: Enables XFR shift for PRU >>>> * @XFR_SHIFT_RTU: Enables XFR shift for RTU >>>> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU >>> >>> This is not required. User can call the API twice. once for PRU and once for RTU. >>> >>>> * @XFR_SHIFT_MAX: Total number of XFR shift types available. >>>> * >>>> */ >>>> >>>> enum xfr_shift_type { >>>> XFR_SHIFT_PRU = 0, >>>> XFR_SHIFT_RTU, >>>> XFR_SHIFT_PRU_RTU, >>>> XFR_SHIFT_MAX, >>>> }; >>> >>> Why do you need this new enum definition? >>> We already have pru_type defined somewhere. You can move it to a public header >>> if not there yet. >>> >>> enum pru_type { >>> PRU_TYPE_PRU = 0, >>> PRU_TYPE_RTU, >>> PRU_TYPE_TX_PRU, >>> PRU_TYPE_MAX, >>> }; >>> >> >> This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem >> with this enum is that in [1] we need to enable XFR shift for both PRU and RTU >> for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and >> PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU). > > Is there any limitation that you have to enable both simultaneously? > The driver can first do enable for PRU and then later for RTU. > > As you will do a read modify write, the previous enable state of register > shouldn't be affected. > >> >> Now this enum doesn't have a field for both PRU and RTU. Also we don't need >> need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are >> defined. > > That is OK. You can return error if not RTU or PRU. > >> >> That is why I thought of introducing new enum. >> >> [1] drivers/net/ethernet/ti/icssg_config.c >> >> /* enable XFR shift for PRU and RTU */ >> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; > > Driver can do like so > > pruss_cfg_xfr_enable(pruss, PRU_TYPE_PRU, true); > pruss_cfg_xfr_enable(pruss, PRU_TYPE_RTU, true); > > The second call should not disable the PRU XFR as you will do a > read-modify-write only affecting the RTU bit. Sure, then I will use the existing enum pru_type. The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this enum definition from there to include/linux/remoteproc/pruss.h > > cheers, > -roger
Hi, On 16/03/2023 15:11, Md Danish Anwar wrote: > > > On 16/03/23 17:49, Roger Quadros wrote: >> >> >> On 16/03/2023 13:44, Md Danish Anwar wrote: >>> >>> On 16/03/23 17:06, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 16/03/2023 13:05, Md Danish Anwar wrote: >>>>> Hi Roger, >>>>> >>>>> On 15/03/23 17:52, Roger Quadros wrote: >>>>>> >>>>>> >>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>>> From: Suman Anna <s-anna@ti.com> >>>>>>> >>>>>>> The PRUSS CFG module is represented as a syscon node and is currently >>>>>>> managed by the PRUSS platform driver. Add easy accessor functions to set >>>>>>> GPI mode, MII_RT event enable/disable and XFR (XIN XOUT) enable/disable >>>>>>> to enable the PRUSS Ethernet usecase. These functions reuse the generic >>>>>>> pruss_cfg_update() API function. >>>>>>> >>>>>>> Signed-off-by: Suman Anna <s-anna@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> >>>>>>> --- >>>>>>> drivers/soc/ti/pruss.c | 60 ++++++++++++++++++++++++++++++++ >>>>>>> include/linux/remoteproc/pruss.h | 22 ++++++++++++ >>>>>>> 2 files changed, 82 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c >>>>>>> index 26d8129b515c..2f04b7922ddb 100644 >>>>>>> --- a/drivers/soc/ti/pruss.c >>>>>>> +++ b/drivers/soc/ti/pruss.c >>>>>>> @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, >>>>>>> return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); >>>>>>> } >>>>>>> >>>>>>> +/** >>>>>>> + * pruss_cfg_gpimode() - set the GPI mode of the PRU >>>>>>> + * @pruss: the pruss instance handle >>>>>>> + * @pru_id: id of the PRU core within the PRUSS >>>>>>> + * @mode: GPI mode to set >>>>>>> + * >>>>>>> + * Sets the GPI mode for a given PRU by programming the >>>>>>> + * corresponding PRUSS_CFG_GPCFGx register >>>>>>> + * >>>>>>> + * Return: 0 on success, or an error code otherwise >>>>>>> + */ >>>>>>> +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, >>>>>>> + enum pruss_gpi_mode mode) >>>>>>> +{ >>>>>>> + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), >>>>>>> + PRUSS_GPCFG_PRU_GPI_MODE_MASK, >>>>>>> + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); >>>>>>> + >>>>>>> +/** >>>>>>> + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events >>>>>>> + * @pruss: the pruss instance >>>>>>> + * @enable: enable/disable >>>>>>> + * >>>>>>> + * Enable/disable the MII RT Events for the PRUSS. >>>>>>> + * >>>>>>> + * Return: 0 on success, or an error code otherwise >>>>>>> + */ >>>>>>> +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) >>>>>>> +{ >>>>>>> + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; >>>>>>> + >>>>>>> + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, >>>>>>> + PRUSS_MII_RT_EVENT_EN, set); >>>>>>> +} >>>>>>> +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); >>>>>>> + >>>>>>> +/** >>>>>>> + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality >>>>>>> + * @pruss: the pruss instance >>>>>>> + * @enable: enable/disable >>>>>>> + * @mask: Mask for PRU / RTU >>>>>> >>>>>> You should not expect the user to provide the mask but only >>>>>> the core type e.g. >>>>>> >>>>>> enum pru_type { >>>>>> PRU_TYPE_PRU = 0, >>>>>> PRU_TYPE_RTU, >>>>>> PRU_TYPE_TX_PRU, >>>>>> PRU_TYPE_MAX, >>>>>> }; >>>>>> >>>>>> Then you figure out the mask in the function. >>>>>> Also check for invalid pru_type and return error if so. >>>>>> >>>>> >>>>> Sure Roger, I will create a enum and take it as parameter in API. Based on >>>>> these enum I will calculate mask and do XFR shifting inside the API >>>>> pruss_cfg_xfr_enable(). >>>>> >>>>> There are two registers for XFR shift. >>>>> >>>>> #define PRUSS_SPP_XFER_SHIFT_EN BIT(1) >>>>> #define PRUSS_SPP_RTU_XFR_SHIFT_EN BIT(3) >>>>> >>>>> For PRU XFR shifting, the mask should be PRUSS_SPP_XFER_SHIFT_EN, >>>>> for RTU shifting mask should be PRUSS_SPP_RTU_XFR_SHIFT_EN and for PRU and RTU >>>>> shifting mask should be (PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN) >>>>> >>>>> So the enum would be something like this. >>>>> >>>>> /** >>>>> * enum xfr_shift_type - XFR shift type >>>>> * @XFR_SHIFT_PRU: Enables XFR shift for PRU >>>>> * @XFR_SHIFT_RTU: Enables XFR shift for RTU >>>>> * @XFR_SHIFT_PRU_RTU: Enables XFR shift for both PRU and RTU >>>> >>>> This is not required. User can call the API twice. once for PRU and once for RTU. >>>> >>>>> * @XFR_SHIFT_MAX: Total number of XFR shift types available. >>>>> * >>>>> */ >>>>> >>>>> enum xfr_shift_type { >>>>> XFR_SHIFT_PRU = 0, >>>>> XFR_SHIFT_RTU, >>>>> XFR_SHIFT_PRU_RTU, >>>>> XFR_SHIFT_MAX, >>>>> }; >>>> >>>> Why do you need this new enum definition? >>>> We already have pru_type defined somewhere. You can move it to a public header >>>> if not there yet. >>>> >>>> enum pru_type { >>>> PRU_TYPE_PRU = 0, >>>> PRU_TYPE_RTU, >>>> PRU_TYPE_TX_PRU, >>>> PRU_TYPE_MAX, >>>> }; >>>> >>> >>> This enum is present in drivers/remoteproc/pru_rproc.c file. But the problem >>> with this enum is that in [1] we need to enable XFR shift for both PRU and RTU >>> for which the mask will be OR of PRUSS_SPP_XFER_SHIFT_EN (mask for PRU) and >>> PRUSS_SPP_RTU_XFR_SHIFT_EN (mask of RTU). >> >> Is there any limitation that you have to enable both simultaneously? >> The driver can first do enable for PRU and then later for RTU. >> >> As you will do a read modify write, the previous enable state of register >> shouldn't be affected. >> >>> >>> Now this enum doesn't have a field for both PRU and RTU. Also we don't need >>> need the XFR shift for PRU_TYPE_TX_PRU as only two XFR shift register bits are >>> defined. >> >> That is OK. You can return error if not RTU or PRU. >> >>> >>> That is why I thought of introducing new enum. >>> >>> [1] drivers/net/ethernet/ti/icssg_config.c >>> >>> /* enable XFR shift for PRU and RTU */ >>> mask = PRUSS_SPP_XFER_SHIFT_EN | PRUSS_SPP_RTU_XFR_SHIFT_EN; >> >> Driver can do like so >> >> pruss_cfg_xfr_enable(pruss, PRU_TYPE_PRU, true); >> pruss_cfg_xfr_enable(pruss, PRU_TYPE_RTU, true); >> >> The second call should not disable the PRU XFR as you will do a >> read-modify-write only affecting the RTU bit. > > Sure, then I will use the existing enum pru_type. > > The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this > enum definition from there to include/linux/remoteproc/pruss.h There are 2 public pruss.h files. include/linux/remoteproc/pruss.h and include/linux/pruss_driver.h Why is that and when to use what? cheers, -roger
On 16/03/23 19:34, Roger Quadros wrote: > > Hi, > > On 16/03/2023 15:11, Md Danish Anwar wrote: >> >> >> On 16/03/23 17:49, Roger Quadros wrote: >>> >>> >>> On 16/03/2023 13:44, Md Danish Anwar wrote: >>>> >>>> On 16/03/23 17:06, Roger Quadros wrote: >>>>> Hi, >>>>> >>>>> On 16/03/2023 13:05, Md Danish Anwar wrote: >>>>>> Hi Roger, >>>>>> >>>>>> On 15/03/23 17:52, Roger Quadros wrote: >>>>>>> >>>>>>> >>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>>>> From: Suman Anna <s-anna@ti.com> > [..] >> Sure, then I will use the existing enum pru_type. >> >> The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this >> enum definition from there to include/linux/remoteproc/pruss.h > > There are 2 public pruss.h files. > include/linux/remoteproc/pruss.h > and > include/linux/pruss_driver.h > > Why is that and when to use what? > The include/linux/remoteproc/pruss.h file was introduced in series [1] as a public header file for PRU_RPROC driver (drivers/remoteproc/pru_rproc.c) The second header file include/linux/pruss_driver.h was introduced much earlier as part of [2] , "soc: ti: pruss: Add a platform driver for PRUSS in TI SoCs". As far as I can see, seems like pruss_driver.h was added as a public header file for PRUSS platform driver (drivers/soc/ti/pruss.c) [1] https://lore.kernel.org/all/20230106121046.886863-1-danishanwar@ti.com/ [2] https://lore.kernel.org/all/1542886753-17625-7-git-send-email-rogerq@ti.com/ > cheers, > -roger
On 17/03/2023 07:02, Md Danish Anwar wrote: > > > On 16/03/23 19:34, Roger Quadros wrote: >> >> Hi, >> >> On 16/03/2023 15:11, Md Danish Anwar wrote: >>> >>> >>> On 16/03/23 17:49, Roger Quadros wrote: >>>> >>>> >>>> On 16/03/2023 13:44, Md Danish Anwar wrote: >>>>> >>>>> On 16/03/23 17:06, Roger Quadros wrote: >>>>>> Hi, >>>>>> >>>>>> On 16/03/2023 13:05, Md Danish Anwar wrote: >>>>>>> Hi Roger, >>>>>>> >>>>>>> On 15/03/23 17:52, Roger Quadros wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>>>>> From: Suman Anna <s-anna@ti.com> >> > > [..] > >>> Sure, then I will use the existing enum pru_type. >>> >>> The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this >>> enum definition from there to include/linux/remoteproc/pruss.h >> >> There are 2 public pruss.h files. >> include/linux/remoteproc/pruss.h >> and >> include/linux/pruss_driver.h >> >> Why is that and when to use what? >> > > The include/linux/remoteproc/pruss.h file was introduced in series [1] as a > public header file for PRU_RPROC driver (drivers/remoteproc/pru_rproc.c) > > The second header file include/linux/pruss_driver.h was introduced much earlier > as part of [2] , "soc: ti: pruss: Add a platform driver for PRUSS in TI SoCs". > > As far as I can see, seems like pruss_driver.h was added as a public header > file for PRUSS platform driver (drivers/soc/ti/pruss.c) > > [1] https://lore.kernel.org/all/20230106121046.886863-1-danishanwar@ti.com/ > [2] https://lore.kernel.org/all/1542886753-17625-7-git-send-email-rogerq@ti.com/ Thanks. "include/linux/remoteproc/pruss.h" seems appropriate for enum pru_type. cheers, -roger
On 17/03/23 14:01, Roger Quadros wrote: > > > On 17/03/2023 07:02, Md Danish Anwar wrote: >> >> >> On 16/03/23 19:34, Roger Quadros wrote: >>> >>> Hi, >>> >>> On 16/03/2023 15:11, Md Danish Anwar wrote: >>>> >>>> >>>> On 16/03/23 17:49, Roger Quadros wrote: >>>>> >>>>> >>>>> On 16/03/2023 13:44, Md Danish Anwar wrote: >>>>>> >>>>>> On 16/03/23 17:06, Roger Quadros wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 16/03/2023 13:05, Md Danish Anwar wrote: >>>>>>>> Hi Roger, >>>>>>>> >>>>>>>> On 15/03/23 17:52, Roger Quadros wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 13/03/2023 13:11, MD Danish Anwar wrote: >>>>>>>>>> From: Suman Anna <s-anna@ti.com> >>> >> >> [..] >> >>>> Sure, then I will use the existing enum pru_type. >>>> >>>> The enum pru_type is currently in drivers/remoteproc/pruss.c I will move this >>>> enum definition from there to include/linux/remoteproc/pruss.h >>> >>> There are 2 public pruss.h files. >>> include/linux/remoteproc/pruss.h >>> and >>> include/linux/pruss_driver.h >>> >>> Why is that and when to use what? >>> >> >> The include/linux/remoteproc/pruss.h file was introduced in series [1] as a >> public header file for PRU_RPROC driver (drivers/remoteproc/pru_rproc.c) >> >> The second header file include/linux/pruss_driver.h was introduced much earlier >> as part of [2] , "soc: ti: pruss: Add a platform driver for PRUSS in TI SoCs". >> >> As far as I can see, seems like pruss_driver.h was added as a public header >> file for PRUSS platform driver (drivers/soc/ti/pruss.c) >> >> [1] https://lore.kernel.org/all/20230106121046.886863-1-danishanwar@ti.com/ >> [2] https://lore.kernel.org/all/1542886753-17625-7-git-send-email-rogerq@ti.com/ > > Thanks. "include/linux/remoteproc/pruss.h" seems appropriate for enum pru_type. > > cheers, > -roger Yes, enum pru_type is located in pru_rproc.c, I will move enum pru_type to include/linux/remoteproc/pruss.h and then include include/linux/remoteproc/pruss.h" in pru_rproc.c and any other file that needs this enum.
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c index 26d8129b515c..2f04b7922ddb 100644 --- a/drivers/soc/ti/pruss.c +++ b/drivers/soc/ti/pruss.c @@ -203,6 +203,66 @@ static int pruss_cfg_update(struct pruss *pruss, unsigned int reg, return regmap_update_bits(pruss->cfg_regmap, reg, mask, val); } +/** + * pruss_cfg_gpimode() - set the GPI mode of the PRU + * @pruss: the pruss instance handle + * @pru_id: id of the PRU core within the PRUSS + * @mode: GPI mode to set + * + * Sets the GPI mode for a given PRU by programming the + * corresponding PRUSS_CFG_GPCFGx register + * + * Return: 0 on success, or an error code otherwise + */ +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, + enum pruss_gpi_mode mode) +{ + if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS) + return -EINVAL; + + if (mode < 0 || mode > PRUSS_GPI_MODE_MAX) + return -EINVAL; + + return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id), + PRUSS_GPCFG_PRU_GPI_MODE_MASK, + mode << PRUSS_GPCFG_PRU_GPI_MODE_SHIFT); +} +EXPORT_SYMBOL_GPL(pruss_cfg_gpimode); + +/** + * pruss_cfg_miirt_enable() - Enable/disable MII RT Events + * @pruss: the pruss instance + * @enable: enable/disable + * + * Enable/disable the MII RT Events for the PRUSS. + * + * Return: 0 on success, or an error code otherwise + */ +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) +{ + u32 set = enable ? PRUSS_MII_RT_EVENT_EN : 0; + + return pruss_cfg_update(pruss, PRUSS_CFG_MII_RT, + PRUSS_MII_RT_EVENT_EN, set); +} +EXPORT_SYMBOL_GPL(pruss_cfg_miirt_enable); + +/** + * pruss_cfg_xfr_enable() - Enable/disable XIN XOUT shift functionality + * @pruss: the pruss instance + * @enable: enable/disable + * @mask: Mask for PRU / RTU + * + * Return: 0 on success, or an error code otherwise + */ +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask) +{ + u32 set = enable ? mask : 0; + + return pruss_cfg_update(pruss, PRUSS_CFG_SPP, mask, set); +} +EXPORT_SYMBOL_GPL(pruss_cfg_xfr_enable); + 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 12ef10b9fe9a..51a3eedd2be6 100644 --- a/include/linux/remoteproc/pruss.h +++ b/include/linux/remoteproc/pruss.h @@ -101,6 +101,7 @@ enum pruss_gpi_mode { PRUSS_GPI_MODE_PARALLEL, PRUSS_GPI_MODE_28BIT_SHIFT, PRUSS_GPI_MODE_MII, + PRUSS_GPI_MODE_MAX, }; /** @@ -165,6 +166,10 @@ int pruss_request_mem_region(struct pruss *pruss, enum pruss_mem mem_id, struct pruss_mem_region *region); int pruss_release_mem_region(struct pruss *pruss, struct pruss_mem_region *region); +int pruss_cfg_gpimode(struct pruss *pruss, enum pruss_pru_id pru_id, + enum pruss_gpi_mode mode); +int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable); +int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask); #else @@ -188,6 +193,23 @@ static inline int pruss_release_mem_region(struct pruss *pruss, return -EOPNOTSUPP; } +static inline int pruss_cfg_gpimode(struct pruss *pruss, + enum pruss_pru_id pru_id, + enum pruss_gpi_mode mode) +{ + return ERR_PTR(-EOPNOTSUPP); +} + +static inline int pruss_cfg_miirt_enable(struct pruss *pruss, bool enable) +{ + return ERR_PTR(-EOPNOTSUPP); +} + +static inline int pruss_cfg_xfr_enable(struct pruss *pruss, bool enable, u32 mask) +{ + return ERR_PTR(-EOPNOTSUPP); +} + #endif /* CONFIG_TI_PRUSS */ #if IS_ENABLED(CONFIG_PRU_REMOTEPROC)