| Message ID | 20210104211816.420602-1-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: pru: future-proof PRU ID matching | expand |
On 1/4/21 3:18 PM, David Lechner wrote: > Currently, to determine the ID (0 or 1) of a PRU core, the last 19 bits > of the physical address of the cores IRAM are compared to known values. > However, the PRUs on TI AM18XX have IRAM at 0x01c38000 and 0x01c3c000 > respectively. The former conflicts with PRU1_IRAM_ADDR_MASK which could > cause PRU0 to be detected as PRU1. (The latter also conflicts with > TX_PRU1_IRAM_ADDR_MASK but it would still be correctly detected as > PRU1.) > > This fixes the problem by moving the address matching offset values to > the device-specific data. This way the compatible string does half of > the work of narrowing down the addresses to two possibilities instead > of checking the address against all possible PRU types. This also lets > us narrow down the scope of the match from 19 bits to 16 bits for all > PRU types. > > After this, the TI AM18XX PRUs will be able to be added without running > into the problems stated above. > > We can also drop the local ret variable while touching this code. > > Signed-off-by: David Lechner <david@lechnology.com> Will test this patch on Mon/Tue on various platforms. Bjorn, Please wait for my Ack on this before you pick this up. regards Suman > --- > drivers/remoteproc/pru_rproc.c | 49 ++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 29 deletions(-) > > diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c > index 2667919d76b3..94ce48df2f48 100644 > --- a/drivers/remoteproc/pru_rproc.c > +++ b/drivers/remoteproc/pru_rproc.c > @@ -46,15 +46,6 @@ > #define PRU_DEBUG_GPREG(x) (0x0000 + (x) * 4) > #define PRU_DEBUG_CT_REG(x) (0x0080 + (x) * 4) > > -/* PRU/RTU/Tx_PRU Core IRAM address masks */ > -#define PRU_IRAM_ADDR_MASK 0x3ffff > -#define PRU0_IRAM_ADDR_MASK 0x34000 > -#define PRU1_IRAM_ADDR_MASK 0x38000 > -#define RTU0_IRAM_ADDR_MASK 0x4000 > -#define RTU1_IRAM_ADDR_MASK 0x6000 > -#define TX_PRU0_IRAM_ADDR_MASK 0xa000 > -#define TX_PRU1_IRAM_ADDR_MASK 0xc000 > - > /* PRU device addresses for various type of PRU RAMs */ > #define PRU_IRAM_DA 0 /* Instruction RAM */ > #define PRU_PDRAM_DA 0 /* Primary Data RAM */ > @@ -96,10 +87,14 @@ enum pru_type { > /** > * struct pru_private_data - device data for a PRU core > * @type: type of the PRU core (PRU, RTU, Tx_PRU) > + * @pru0_iram_offset: used to identify PRU core 0 > + * @pru1_iram_offset: used to identify PRU core 1 > * @is_k3: flag used to identify the need for special load handling > */ > struct pru_private_data { > enum pru_type type; > + u16 pru0_iram_offset; > + u16 pru1_iram_offset; > unsigned int is_k3 : 1; > }; > > @@ -693,33 +688,21 @@ static int pru_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) > } > > /* > - * Compute PRU id based on the IRAM addresses. The PRU IRAMs are > + * Compute PRU id based on the last 16 bits of IRAM addresses. The PRU IRAMs are > * always at a particular offset within the PRUSS address space. > */ > static int pru_rproc_set_id(struct pru_rproc *pru) > { > - int ret = 0; > - > - switch (pru->mem_regions[PRU_IOMEM_IRAM].pa & PRU_IRAM_ADDR_MASK) { > - case TX_PRU0_IRAM_ADDR_MASK: > - fallthrough; > - case RTU0_IRAM_ADDR_MASK: > - fallthrough; > - case PRU0_IRAM_ADDR_MASK: > + u16 offset = pru->mem_regions[PRU_IOMEM_IRAM].pa; > + > + if (offset == pru->data->pru0_iram_offset) > pru->id = 0; > - break; > - case TX_PRU1_IRAM_ADDR_MASK: > - fallthrough; > - case RTU1_IRAM_ADDR_MASK: > - fallthrough; > - case PRU1_IRAM_ADDR_MASK: > + else if (offset == pru->data->pru1_iram_offset) > pru->id = 1; > - break; > - default: > - ret = -EINVAL; > - } > + else > + return -EINVAL; > > - return ret; > + return 0; > } > > static int pru_rproc_probe(struct platform_device *pdev) > @@ -825,20 +808,28 @@ static int pru_rproc_remove(struct platform_device *pdev) > > static const struct pru_private_data pru_data = { > .type = PRU_TYPE_PRU, > + .pru0_iram_offset = 0x4000, > + .pru1_iram_offset = 0x8000, > }; > > static const struct pru_private_data k3_pru_data = { > .type = PRU_TYPE_PRU, > + .pru0_iram_offset = 0x4000, > + .pru1_iram_offset = 0x8000, > .is_k3 = 1, > }; > > static const struct pru_private_data k3_rtu_data = { > .type = PRU_TYPE_RTU, > + .pru0_iram_offset = 0x4000, > + .pru1_iram_offset = 0x6000, > .is_k3 = 1, > }; > > static const struct pru_private_data k3_tx_pru_data = { > .type = PRU_TYPE_TX_PRU, > + .pru0_iram_offset = 0xa000, > + .pru1_iram_offset = 0xc000, > .is_k3 = 1, > }; > >
Hi David, On 1/15/21 6:53 PM, Suman Anna wrote: > On 1/4/21 3:18 PM, David Lechner wrote: >> Currently, to determine the ID (0 or 1) of a PRU core, the last 19 bits >> of the physical address of the cores IRAM are compared to known values. >> However, the PRUs on TI AM18XX have IRAM at 0x01c38000 and 0x01c3c000 >> respectively. The former conflicts with PRU1_IRAM_ADDR_MASK which could >> cause PRU0 to be detected as PRU1. (The latter also conflicts with >> TX_PRU1_IRAM_ADDR_MASK but it would still be correctly detected as >> PRU1.) >> >> This fixes the problem by moving the address matching offset values to >> the device-specific data. This way the compatible string does half of >> the work of narrowing down the addresses to two possibilities instead >> of checking the address against all possible PRU types. This also lets >> us narrow down the scope of the match from 19 bits to 16 bits for all >> PRU types. >> >> After this, the TI AM18XX PRUs will be able to be added without running >> into the problems stated above. >> >> We can also drop the local ret variable while touching this code. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > Will test this patch on Mon/Tue on various platforms. > > Bjorn, > Please wait for my Ack on this before you pick this up. > > regards > Suman > >> --- >> drivers/remoteproc/pru_rproc.c | 49 ++++++++++++++-------------------- >> 1 file changed, 20 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c >> index 2667919d76b3..94ce48df2f48 100644 >> --- a/drivers/remoteproc/pru_rproc.c >> +++ b/drivers/remoteproc/pru_rproc.c >> @@ -46,15 +46,6 @@ >> #define PRU_DEBUG_GPREG(x) (0x0000 + (x) * 4) >> #define PRU_DEBUG_CT_REG(x) (0x0080 + (x) * 4) >> >> -/* PRU/RTU/Tx_PRU Core IRAM address masks */ >> -#define PRU_IRAM_ADDR_MASK 0x3ffff >> -#define PRU0_IRAM_ADDR_MASK 0x34000 >> -#define PRU1_IRAM_ADDR_MASK 0x38000 >> -#define RTU0_IRAM_ADDR_MASK 0x4000 >> -#define RTU1_IRAM_ADDR_MASK 0x6000 >> -#define TX_PRU0_IRAM_ADDR_MASK 0xa000 >> -#define TX_PRU1_IRAM_ADDR_MASK 0xc000 >> - >> /* PRU device addresses for various type of PRU RAMs */ >> #define PRU_IRAM_DA 0 /* Instruction RAM */ >> #define PRU_PDRAM_DA 0 /* Primary Data RAM */ >> @@ -96,10 +87,14 @@ enum pru_type { >> /** >> * struct pru_private_data - device data for a PRU core >> * @type: type of the PRU core (PRU, RTU, Tx_PRU) >> + * @pru0_iram_offset: used to identify PRU core 0 >> + * @pru1_iram_offset: used to identify PRU core 1 >> * @is_k3: flag used to identify the need for special load handling >> */ >> struct pru_private_data { >> enum pru_type type; >> + u16 pru0_iram_offset; >> + u16 pru1_iram_offset; >> unsigned int is_k3 : 1; >> }; >> >> @@ -693,33 +688,21 @@ static int pru_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) >> } >> >> /* >> - * Compute PRU id based on the IRAM addresses. The PRU IRAMs are >> + * Compute PRU id based on the last 16 bits of IRAM addresses. The PRU IRAMs are >> * always at a particular offset within the PRUSS address space. >> */ >> static int pru_rproc_set_id(struct pru_rproc *pru) >> { >> - int ret = 0; >> - >> - switch (pru->mem_regions[PRU_IOMEM_IRAM].pa & PRU_IRAM_ADDR_MASK) { >> - case TX_PRU0_IRAM_ADDR_MASK: >> - fallthrough; >> - case RTU0_IRAM_ADDR_MASK: >> - fallthrough; >> - case PRU0_IRAM_ADDR_MASK: >> + u16 offset = pru->mem_regions[PRU_IOMEM_IRAM].pa; >> + >> + if (offset == pru->data->pru0_iram_offset) >> pru->id = 0; >> - break; >> - case TX_PRU1_IRAM_ADDR_MASK: >> - fallthrough; >> - case RTU1_IRAM_ADDR_MASK: >> - fallthrough; >> - case PRU1_IRAM_ADDR_MASK: >> + else if (offset == pru->data->pru1_iram_offset) >> pru->id = 1; >> - break; >> - default: >> - ret = -EINVAL; >> - } >> + else >> + return -EINVAL; >> >> - return ret; >> + return 0; While this logic does work, it is a bit convoluted IMO for any one reading the code. The offset here is kind of a misnomer, this logic is relying on only the least 16-bits of the address. I have actually used a different logic in our downstream code, and we chose the current approach to minimize the static data. https://git.ti.com/gitweb?p=rpmsg/remoteproc.git;a=blob;f=drivers/remoteproc/pru_rproc.c;h=cbcd8a6e420e61d60218a01ea0e76f0c9989a337;hb=refs/heads/rproc-linux-5.4.y#l1135 >> } >> >> static int pru_rproc_probe(struct platform_device *pdev) >> @@ -825,20 +808,28 @@ static int pru_rproc_remove(struct platform_device *pdev) >> >> static const struct pru_private_data pru_data = { >> .type = PRU_TYPE_PRU, >> + .pru0_iram_offset = 0x4000, >> + .pru1_iram_offset = 0x8000, The offsets for the PRU cores are actually 0x34000 and 0x38000 respectively from the base of the PRUSS on non-Davinci SoCs. If we were to use this static data approach, then we might as well continue to use the current address masking logic with the appropriate masks for Davinci (0x38000 and 0x3C000, not true offsets but as masks they would work). Davinci PRUSS is the only one with its differences being the first PRUSS IP, and I would prefer to keep the logic aligned to the IPs on all the recent SoCs on 3 different TI SoC families (OMAP, Keystone 2 and K3). Let me know what you think. regards Suman >> }; >> >> static const struct pru_private_data k3_pru_data = { >> .type = PRU_TYPE_PRU, >> + .pru0_iram_offset = 0x4000, >> + .pru1_iram_offset = 0x8000, >> .is_k3 = 1, >> }; >> >> static const struct pru_private_data k3_rtu_data = { >> .type = PRU_TYPE_RTU, >> + .pru0_iram_offset = 0x4000, >> + .pru1_iram_offset = 0x6000, >> .is_k3 = 1, >> }; >> >> static const struct pru_private_data k3_tx_pru_data = { >> .type = PRU_TYPE_TX_PRU, >> + .pru0_iram_offset = 0xa000, >> + .pru1_iram_offset = 0xc000, >> .is_k3 = 1, >> }; >> >> >
On 1/28/21 4:55 PM, Suman Anna wrote: > Hi David, > > On 1/15/21 6:53 PM, Suman Anna wrote: >> On 1/4/21 3:18 PM, David Lechner wrote: >>> static int pru_rproc_probe(struct platform_device *pdev) >>> @@ -825,20 +808,28 @@ static int pru_rproc_remove(struct platform_device *pdev) >>> >>> static const struct pru_private_data pru_data = { >>> .type = PRU_TYPE_PRU, >>> + .pru0_iram_offset = 0x4000, >>> + .pru1_iram_offset = 0x8000, > > The offsets for the PRU cores are actually 0x34000 and 0x38000 respectively from > the base of the PRUSS on non-Davinci SoCs. > > If we were to use this static data approach, then we might as well continue to > use the current address masking logic with the appropriate masks for Davinci > (0x38000 and 0x3C000, not true offsets but as masks they would work). Davinci > PRUSS is the only one with its differences being the first PRUSS IP, and I would > prefer to keep the logic aligned to the IPs on all the recent SoCs on 3 > different TI SoC families (OMAP, Keystone 2 and K3). > > Let me know what you think. I'm not too picky as long as it works. :-) If keeping the static data to a minimum is really important, I suppose we could introduce a new type = PRU_TYPE_PRU_V1 for these PRUSSs instead. It sounds like this information might be useful elsewhere anyway.
On 1/28/21 5:21 PM, David Lechner wrote: > On 1/28/21 4:55 PM, Suman Anna wrote: >> Hi David, >> >> On 1/15/21 6:53 PM, Suman Anna wrote: >>> On 1/4/21 3:18 PM, David Lechner wrote: >>>> static int pru_rproc_probe(struct platform_device *pdev) >>>> @@ -825,20 +808,28 @@ static int pru_rproc_remove(struct platform_device *pdev) >>>> static const struct pru_private_data pru_data = { >>>> .type = PRU_TYPE_PRU, >>>> + .pru0_iram_offset = 0x4000, >>>> + .pru1_iram_offset = 0x8000, >> >> The offsets for the PRU cores are actually 0x34000 and 0x38000 respectively from >> the base of the PRUSS on non-Davinci SoCs. >> >> If we were to use this static data approach, then we might as well continue to >> use the current address masking logic with the appropriate masks for Davinci >> (0x38000 and 0x3C000, not true offsets but as masks they would work). Davinci >> PRUSS is the only one with its differences being the first PRUSS IP, and I would >> prefer to keep the logic aligned to the IPs on all the recent SoCs on 3 >> different TI SoC families (OMAP, Keystone 2 and K3). >> >> Let me know what you think. > > I'm not too picky as long as it works. :-) > > If keeping the static data to a minimum is really important, I suppose we could > introduce a new type = PRU_TYPE_PRU_V1 for these PRUSSs instead. It sounds like > this information might be useful elsewhere anyway. Yeah, let me look into this further and see if there is any other way possible as well. I had some patches for DA850 lying around on some older kernels, and I know that I definitely needed to introduce a variable to distinguish the programmability of certain CONST registers (C26 and C27 are fixed on Davinci IIRC). Will need that variable with the current PRU consumer series. Perhaps, the same PRU type variable can be used for that as well. regards Suman
diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c index 2667919d76b3..94ce48df2f48 100644 --- a/drivers/remoteproc/pru_rproc.c +++ b/drivers/remoteproc/pru_rproc.c @@ -46,15 +46,6 @@ #define PRU_DEBUG_GPREG(x) (0x0000 + (x) * 4) #define PRU_DEBUG_CT_REG(x) (0x0080 + (x) * 4) -/* PRU/RTU/Tx_PRU Core IRAM address masks */ -#define PRU_IRAM_ADDR_MASK 0x3ffff -#define PRU0_IRAM_ADDR_MASK 0x34000 -#define PRU1_IRAM_ADDR_MASK 0x38000 -#define RTU0_IRAM_ADDR_MASK 0x4000 -#define RTU1_IRAM_ADDR_MASK 0x6000 -#define TX_PRU0_IRAM_ADDR_MASK 0xa000 -#define TX_PRU1_IRAM_ADDR_MASK 0xc000 - /* PRU device addresses for various type of PRU RAMs */ #define PRU_IRAM_DA 0 /* Instruction RAM */ #define PRU_PDRAM_DA 0 /* Primary Data RAM */ @@ -96,10 +87,14 @@ enum pru_type { /** * struct pru_private_data - device data for a PRU core * @type: type of the PRU core (PRU, RTU, Tx_PRU) + * @pru0_iram_offset: used to identify PRU core 0 + * @pru1_iram_offset: used to identify PRU core 1 * @is_k3: flag used to identify the need for special load handling */ struct pru_private_data { enum pru_type type; + u16 pru0_iram_offset; + u16 pru1_iram_offset; unsigned int is_k3 : 1; }; @@ -693,33 +688,21 @@ static int pru_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) } /* - * Compute PRU id based on the IRAM addresses. The PRU IRAMs are + * Compute PRU id based on the last 16 bits of IRAM addresses. The PRU IRAMs are * always at a particular offset within the PRUSS address space. */ static int pru_rproc_set_id(struct pru_rproc *pru) { - int ret = 0; - - switch (pru->mem_regions[PRU_IOMEM_IRAM].pa & PRU_IRAM_ADDR_MASK) { - case TX_PRU0_IRAM_ADDR_MASK: - fallthrough; - case RTU0_IRAM_ADDR_MASK: - fallthrough; - case PRU0_IRAM_ADDR_MASK: + u16 offset = pru->mem_regions[PRU_IOMEM_IRAM].pa; + + if (offset == pru->data->pru0_iram_offset) pru->id = 0; - break; - case TX_PRU1_IRAM_ADDR_MASK: - fallthrough; - case RTU1_IRAM_ADDR_MASK: - fallthrough; - case PRU1_IRAM_ADDR_MASK: + else if (offset == pru->data->pru1_iram_offset) pru->id = 1; - break; - default: - ret = -EINVAL; - } + else + return -EINVAL; - return ret; + return 0; } static int pru_rproc_probe(struct platform_device *pdev) @@ -825,20 +808,28 @@ static int pru_rproc_remove(struct platform_device *pdev) static const struct pru_private_data pru_data = { .type = PRU_TYPE_PRU, + .pru0_iram_offset = 0x4000, + .pru1_iram_offset = 0x8000, }; static const struct pru_private_data k3_pru_data = { .type = PRU_TYPE_PRU, + .pru0_iram_offset = 0x4000, + .pru1_iram_offset = 0x8000, .is_k3 = 1, }; static const struct pru_private_data k3_rtu_data = { .type = PRU_TYPE_RTU, + .pru0_iram_offset = 0x4000, + .pru1_iram_offset = 0x6000, .is_k3 = 1, }; static const struct pru_private_data k3_tx_pru_data = { .type = PRU_TYPE_TX_PRU, + .pru0_iram_offset = 0xa000, + .pru1_iram_offset = 0xc000, .is_k3 = 1, };
Currently, to determine the ID (0 or 1) of a PRU core, the last 19 bits of the physical address of the cores IRAM are compared to known values. However, the PRUs on TI AM18XX have IRAM at 0x01c38000 and 0x01c3c000 respectively. The former conflicts with PRU1_IRAM_ADDR_MASK which could cause PRU0 to be detected as PRU1. (The latter also conflicts with TX_PRU1_IRAM_ADDR_MASK but it would still be correctly detected as PRU1.) This fixes the problem by moving the address matching offset values to the device-specific data. This way the compatible string does half of the work of narrowing down the addresses to two possibilities instead of checking the address against all possible PRU types. This also lets us narrow down the scope of the match from 19 bits to 16 bits for all PRU types. After this, the TI AM18XX PRUs will be able to be added without running into the problems stated above. We can also drop the local ret variable while touching this code. Signed-off-by: David Lechner <david@lechnology.com> --- drivers/remoteproc/pru_rproc.c | 49 ++++++++++++++-------------------- 1 file changed, 20 insertions(+), 29 deletions(-)