| Message ID | hqxjgkpwio5qyf8bvec1jku6.1461253502884@com.syntomo.email |
|---|---|
| State | RFC |
| Delegated to: | York Sun |
| Headers | show |
On 21-04-16 15:45:05, york sun wrote: > I prefer to use void *. > > York I'm not sure what you mean by "use void *". We need to compute the address of the registers, so you can't directly use void * as it does not allow pointer arithmetic. Either you use unsigned char * with full offset, uint32_t * with offset/sizeof(uint32_t) or the direct mapping using struct ccsr_scfg * and field access. Could you elaborate ? Vincent > > Sent from my phone > > -------- Original Message -------- > From: Vincent Siles <vincent.siles@provenrun.com> > Sent: Thursday, April 21, 2016 12:53 AM > To: york sun <york.sun@nxp.com> > Subject: Re: [U-Boot] [PATCH] arm: Fix SCFG ICID reg addresses > CC: u-boot@lists.denx.de,Alison Wang <alison.wang@freescale.com> > > Hi ! > > On 20-04-16 09:53:32, York Sun wrote: > > On 04/12/2016 05:28 AM, Vincent Siles wrote: > > > On the LS102x boards, in order to initialize the ICID values of > masters, > > > the dev_stream_id array holds absolute offsets from the base of SCFG. > > > > > > In ls102xa_config_ssmu_stream_id, the base pointer is cast to uint32_t > * > > > before adding the offset, leading to an invalid address. Casting it to > > > unsigned char * solves the issue. > > > > > > Also minor cosmetic renaming of uint32_t into u32 to be consistent in > > > the whole file. > > > > > > Signed-off-by: Vincent Siles <vincent.siles@provenrun.com> > > > --- > > > > > > board/freescale/common/ls102xa_stream_id.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/board/freescale/common/ls102xa_stream_id.c > b/board/freescale/common/ls102xa_stream_id.c > > > index f434269..2a4ef3e 100644 > > > --- a/board/freescale/common/ls102xa_stream_id.c > > > +++ b/board/freescale/common/ls102xa_stream_id.c > > > @@ -10,11 +10,11 @@ > > > > > > void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, > uint32_t num) > > > { > > > - uint32_t *scfg = (uint32_t *)CONFIG_SYS_FSL_SCFG_ADDR; > > > + unsigned char *scfg = (unsigned char *)CONFIG_SYS_FSL_SCFG_ADDR; > > > int i; > > > > > > for (i = 0; i < num; i++) > > > - out_be32(scfg + id[i].offset, id[i].stream_id); > > > + out_be32((u32 *)(scfg + id[i].offset), id[i].stream_id); > > > } > > > > > > void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int > size) > > > @@ -28,6 +28,6 @@ void ls1021x_config_caam_stream_id(struct > liodn_id_table *tbl, int size) > > > else > > > liodn = tbl[i].id[0]; > > > > > > - out_le32((uint32_t *)(tbl[i].reg_offset), liodn); > > > + out_le32((u32 *)(tbl[i].reg_offset), liodn); > > > } > > > } > > > > > > > If the size of the pointer is an issue, maybe you ca use "void *"? Can > you check > > if "struct ccsr_scfg" should/can be used? > > > > York > > By using the 'struct ccsr_scfg' type we won't be able to have the same > kind of loop. Since the code is board dependent, I'm not sure it really > matters. > > Here what I would do instead. Tell me which style do you prefer. > > Best, > Vincent > > --- > > diff --git a/arch/arm/cpu/armv7/ls102xa/soc.c > b/arch/arm/cpu/armv7/ls102xa/soc.c > index b1b0c71..9c78efc 100644 > --- a/arch/arm/cpu/armv7/ls102xa/soc.c > +++ b/arch/arm/cpu/armv7/ls102xa/soc.c > @@ -30,21 +30,21 @@ struct liodn_id_table sec_liodn_tbl[] = { > SET_SEC_DECO_LIODN_ENTRY(7, 0x10, 0x10), > }; > > -struct smmu_stream_id dev_stream_id[] = { > - { 0x100, 0x01, "ETSEC MAC1" }, > - { 0x104, 0x02, "ETSEC MAC2" }, > - { 0x108, 0x03, "ETSEC MAC3" }, > - { 0x10c, 0x04, "PEX1" }, > - { 0x110, 0x05, "PEX2" }, > - { 0x114, 0x06, "qDMA" }, > - { 0x118, 0x07, "SATA" }, > - { 0x11c, 0x08, "USB3" }, > - { 0x120, 0x09, "QE" }, > - { 0x124, 0x0a, "eSDHC" }, > - { 0x128, 0x0b, "eMA" }, > - { 0x14c, 0x0c, "2D-ACE" }, > - { 0x150, 0x0d, "USB2" }, > - { 0x18c, 0x0e, "DEBUG" }, > +struct smmu_stream_id dev_stream_id = { > + .mac1 = 0x01, > + .mac2 = 0x02, > + .mac3 = 0x03, > + .pex1 = 0x04, > + .pex2 = 0x05, > + .dma = 0x06, > + .sata = 0x07, > + .usb3 = 0x08, > + .qe = 0x09, > + .sdhc = 0x0a, > + .adma = 0x0b, > + .dcu = 0x0c, > + .usb2 = 0x0d, > + .debug = 0x0e > }; > > unsigned int get_soc_major_rev(void) > @@ -131,8 +131,7 @@ int ls102xa_smmu_stream_id_init(void) > ls1021x_config_caam_stream_id(sec_liodn_tbl, > ARRAY_SIZE(sec_liodn_tbl)); > > - ls102xa_config_smmu_stream_id(dev_stream_id, > - ARRAY_SIZE(dev_stream_id)); > + ls102xa_config_smmu_stream_id(&dev_stream_id); > > return 0; > } > diff --git a/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h > b/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h > index fa571b3..3815673 100644 > --- a/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h > +++ b/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h > @@ -64,11 +64,22 @@ struct liodn_id_table { > }; > > struct smmu_stream_id { > - uint16_t offset; > - uint16_t stream_id; > - char dev_name[32]; > + uint16_t mac1; > + uint16_t mac2; > + uint16_t mac3; > + uint16_t pex1; > + uint16_t pex2; > + uint16_t dma; > + uint16_t sata; > + uint16_t usb3; > + uint16_t qe; > + uint16_t sdhc; > + uint16_t adma; > + uint16_t dcu; > + uint16_t usb2; > + uint16_t debug; > }; > > void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int size); > -void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, uint32_t > num); > +void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id); > #endif > diff --git a/board/freescale/common/ls102xa_stream_id.c > b/board/freescale/common/ls102xa_stream_id.c > index f434269..941f22d 100644 > --- a/board/freescale/common/ls102xa_stream_id.c > +++ b/board/freescale/common/ls102xa_stream_id.c > @@ -7,14 +7,25 @@ > #include <common.h> > #include <asm/io.h> > #include <asm/arch/ls102xa_stream_id.h> > +#include <asm/arch/immap_ls102xa.h> > > -void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, uint32_t > num) > +void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id) > { > - uint32_t *scfg = (uint32_t *)CONFIG_SYS_FSL_SCFG_ADDR; > - int i; > - > - for (i = 0; i < num; i++) > - out_be32(scfg + id[i].offset, id[i].stream_id); > + struct ccsr_scfg *scfg = (struct ccsr_scfg *)CONFIG_SYS_FSL_SCFG_ADDR; > + scfg->mac1_streamid = id->mac1; > + scfg->mac2_streamid = id->mac2; > + scfg->mac3_streamid = id->mac3; > + scfg->pex1_streamid = id->pex1; > + scfg->pex2_streamid = id->pex2; > + scfg->dma_streamid = id->dma; > + scfg->sata_streamid = id->sata; > + scfg->usb3_streamid = id->usb3; > + scfg->qe_streamid = id->qe; > + scfg->sdhc_streamid = id->sdhc; > + scfg->adma_streamid = id->adma; > + scfg->dcu_streamid = id->dcu; > + scfg->usb2_streamid = id->usb2; > + scfg->debug_streamid = id->debug; > } > > void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int size)
On 04/21/2016 08:56 AM, Vincent Siles wrote: > On 21-04-16 15:45:05, york sun wrote: >> I prefer to use void *. >> >> York > > I'm not sure what you mean by "use void *". We need to compute the > address of the registers, so you can't directly use void * as it does not > allow pointer arithmetic. Either you use unsigned char * with full > offset, uint32_t * with offset/sizeof(uint32_t) or the direct mapping > using struct ccsr_scfg * and field access. > > Could you elaborate ? The reason of your change is (u32 *) + int doesn't yield the correct offset. So you fix it with (u8 *) + int. I was suggesting to use (void *) + int. Of course you need to cast to (u32 *) before using out_be32. York > > Vincent > >> >> Sent from my phone >> >> -------- Original Message -------- >> From: Vincent Siles <vincent.siles@provenrun.com> >> Sent: Thursday, April 21, 2016 12:53 AM >> To: york sun <york.sun@nxp.com> >> Subject: Re: [U-Boot] [PATCH] arm: Fix SCFG ICID reg addresses >> CC: u-boot@lists.denx.de,Alison Wang <alison.wang@freescale.com> >> >> Hi ! >> >> On 20-04-16 09:53:32, York Sun wrote: >> > On 04/12/2016 05:28 AM, Vincent Siles wrote: >> > > On the LS102x boards, in order to initialize the ICID values of >> masters, >> > > the dev_stream_id array holds absolute offsets from the base of SCFG. >> > > >> > > In ls102xa_config_ssmu_stream_id, the base pointer is cast to uint32_t >> * >> > > before adding the offset, leading to an invalid address. Casting it to >> > > unsigned char * solves the issue. >> > > >> > > Also minor cosmetic renaming of uint32_t into u32 to be consistent in >> > > the whole file. >> > > >> > > Signed-off-by: Vincent Siles <vincent.siles@provenrun.com> >> > > --- >> > > >> > > board/freescale/common/ls102xa_stream_id.c | 6 +++--- >> > > 1 file changed, 3 insertions(+), 3 deletions(-) >> > > >> > > diff --git a/board/freescale/common/ls102xa_stream_id.c >> b/board/freescale/common/ls102xa_stream_id.c >> > > index f434269..2a4ef3e 100644 >> > > --- a/board/freescale/common/ls102xa_stream_id.c >> > > +++ b/board/freescale/common/ls102xa_stream_id.c >> > > @@ -10,11 +10,11 @@ >> > > >> > > void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, >> uint32_t num) >> > > { >> > > - uint32_t *scfg = (uint32_t *)CONFIG_SYS_FSL_SCFG_ADDR; >> > > + unsigned char *scfg = (unsigned char *)CONFIG_SYS_FSL_SCFG_ADDR; >> > > int i; >> > > >> > > for (i = 0; i < num; i++) >> > > - out_be32(scfg + id[i].offset, id[i].stream_id); >> > > + out_be32((u32 *)(scfg + id[i].offset), id[i].stream_id); >> > > } >> > > >> > > void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int >> size) >> > > @@ -28,6 +28,6 @@ void ls1021x_config_caam_stream_id(struct >> liodn_id_table *tbl, int size) >> > > else >> > > liodn = tbl[i].id[0]; >> > > >> > > - out_le32((uint32_t *)(tbl[i].reg_offset), liodn); >> > > + out_le32((u32 *)(tbl[i].reg_offset), liodn); >> > > } >> > > } >> > > >> > >> > If the size of the pointer is an issue, maybe you ca use "void *"? Can >> you check >> > if "struct ccsr_scfg" should/can be used? >> > >> > York >> >> By using the 'struct ccsr_scfg' type we won't be able to have the same >> kind of loop. Since the code is board dependent, I'm not sure it really >> matters. >> >> Here what I would do instead. Tell me which style do you prefer. >> >> Best, >> Vincent >> >> --- >> >> diff --git a/arch/arm/cpu/armv7/ls102xa/soc.c >> b/arch/arm/cpu/armv7/ls102xa/soc.c >> index b1b0c71..9c78efc 100644 >> --- a/arch/arm/cpu/armv7/ls102xa/soc.c >> +++ b/arch/arm/cpu/armv7/ls102xa/soc.c >> @@ -30,21 +30,21 @@ struct liodn_id_table sec_liodn_tbl[] = { >> SET_SEC_DECO_LIODN_ENTRY(7, 0x10, 0x10), >> }; >> >> -struct smmu_stream_id dev_stream_id[] = { >> - { 0x100, 0x01, "ETSEC MAC1" }, >> - { 0x104, 0x02, "ETSEC MAC2" }, >> - { 0x108, 0x03, "ETSEC MAC3" }, >> - { 0x10c, 0x04, "PEX1" }, >> - { 0x110, 0x05, "PEX2" }, >> - { 0x114, 0x06, "qDMA" }, >> - { 0x118, 0x07, "SATA" }, >> - { 0x11c, 0x08, "USB3" }, >> - { 0x120, 0x09, "QE" }, >> - { 0x124, 0x0a, "eSDHC" }, >> - { 0x128, 0x0b, "eMA" }, >> - { 0x14c, 0x0c, "2D-ACE" }, >> - { 0x150, 0x0d, "USB2" }, >> - { 0x18c, 0x0e, "DEBUG" }, >> +struct smmu_stream_id dev_stream_id = { >> + .mac1 = 0x01, >> + .mac2 = 0x02, >> + .mac3 = 0x03, >> + .pex1 = 0x04, >> + .pex2 = 0x05, >> + .dma = 0x06, >> + .sata = 0x07, >> + .usb3 = 0x08, >> + .qe = 0x09, >> + .sdhc = 0x0a, >> + .adma = 0x0b, >> + .dcu = 0x0c, >> + .usb2 = 0x0d, >> + .debug = 0x0e >> }; >> >> unsigned int get_soc_major_rev(void) >> @@ -131,8 +131,7 @@ int ls102xa_smmu_stream_id_init(void) >> ls1021x_config_caam_stream_id(sec_liodn_tbl, >> ARRAY_SIZE(sec_liodn_tbl)); >> >> - ls102xa_config_smmu_stream_id(dev_stream_id, >> - ARRAY_SIZE(dev_stream_id)); >> + ls102xa_config_smmu_stream_id(&dev_stream_id); >> >> return 0; >> } >> diff --git a/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h >> b/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h >> index fa571b3..3815673 100644 >> --- a/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h >> +++ b/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h >> @@ -64,11 +64,22 @@ struct liodn_id_table { >> }; >> >> struct smmu_stream_id { >> - uint16_t offset; >> - uint16_t stream_id; >> - char dev_name[32]; >> + uint16_t mac1; >> + uint16_t mac2; >> + uint16_t mac3; >> + uint16_t pex1; >> + uint16_t pex2; >> + uint16_t dma; >> + uint16_t sata; >> + uint16_t usb3; >> + uint16_t qe; >> + uint16_t sdhc; >> + uint16_t adma; >> + uint16_t dcu; >> + uint16_t usb2; >> + uint16_t debug; >> }; >> >> void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int size); >> -void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, uint32_t >> num); >> +void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id); >> #endif >> diff --git a/board/freescale/common/ls102xa_stream_id.c >> b/board/freescale/common/ls102xa_stream_id.c >> index f434269..941f22d 100644 >> --- a/board/freescale/common/ls102xa_stream_id.c >> +++ b/board/freescale/common/ls102xa_stream_id.c >> @@ -7,14 +7,25 @@ >> #include <common.h> >> #include <asm/io.h> >> #include <asm/arch/ls102xa_stream_id.h> >> +#include <asm/arch/immap_ls102xa.h> >> >> -void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, uint32_t >> num) >> +void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id) >> { >> - uint32_t *scfg = (uint32_t *)CONFIG_SYS_FSL_SCFG_ADDR; >> - int i; >> - >> - for (i = 0; i < num; i++) >> - out_be32(scfg + id[i].offset, id[i].stream_id); >> + struct ccsr_scfg *scfg = (struct ccsr_scfg *)CONFIG_SYS_FSL_SCFG_ADDR; >> + scfg->mac1_streamid = id->mac1; >> + scfg->mac2_streamid = id->mac2; >> + scfg->mac3_streamid = id->mac3; >> + scfg->pex1_streamid = id->pex1; >> + scfg->pex2_streamid = id->pex2; >> + scfg->dma_streamid = id->dma; >> + scfg->sata_streamid = id->sata; >> + scfg->usb3_streamid = id->usb3; >> + scfg->qe_streamid = id->qe; >> + scfg->sdhc_streamid = id->sdhc; >> + scfg->adma_streamid = id->adma; >> + scfg->dcu_streamid = id->dcu; >> + scfg->usb2_streamid = id->usb2; >> + scfg->debug_streamid = id->debug; >> } >> >> void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int size)
diff --git a/arch/arm/cpu/armv7/ls102xa/soc.c b/arch/arm/cpu/armv7/ls102xa/soc.c index b1b0c71..9c78efc 100644 --- a/arch/arm/cpu/armv7/ls102xa/soc.c +++ b/arch/arm/cpu/armv7/ls102xa/soc.c @@ -30,21 +30,21 @@ struct liodn_id_table sec_liodn_tbl[] = { SET_SEC_DECO_LIODN_ENTRY(7, 0x10, 0x10), }; -struct smmu_stream_id dev_stream_id[] = { - { 0x100, 0x01, "ETSEC MAC1" }, - { 0x104, 0x02, "ETSEC MAC2" }, - { 0x108, 0x03, "ETSEC MAC3" }, - { 0x10c, 0x04, "PEX1" }, - { 0x110, 0x05, "PEX2" }, - { 0x114, 0x06, "qDMA" }, - { 0x118, 0x07, "SATA" }, - { 0x11c, 0x08, "USB3" }, - { 0x120, 0x09, "QE" }, - { 0x124, 0x0a, "eSDHC" }, - { 0x128, 0x0b, "eMA" }, - { 0x14c, 0x0c, "2D-ACE" }, - { 0x150, 0x0d, "USB2" }, - { 0x18c, 0x0e, "DEBUG" }, +struct smmu_stream_id dev_stream_id = { + .mac1 = 0x01, + .mac2 = 0x02, + .mac3 = 0x03, + .pex1 = 0x04, + .pex2 = 0x05, + .dma = 0x06, + .sata = 0x07, + .usb3 = 0x08, + .qe = 0x09, + .sdhc = 0x0a, + .adma = 0x0b, + .dcu = 0x0c, + .usb2 = 0x0d, + .debug = 0x0e }; unsigned int get_soc_major_rev(void) @@ -131,8 +131,7 @@ int ls102xa_smmu_stream_id_init(void) ls1021x_config_caam_stream_id(sec_liodn_tbl, ARRAY_SIZE(sec_liodn_tbl)); - ls102xa_config_smmu_stream_id(dev_stream_id, - ARRAY_SIZE(dev_stream_id)); + ls102xa_config_smmu_stream_id(&dev_stream_id); return 0; } diff --git a/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h b/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h index fa571b3..3815673 100644 --- a/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h +++ b/arch/arm/include/asm/arch-ls102xa/ls102xa_stream_id.h @@ -64,11 +64,22 @@ struct liodn_id_table { }; struct smmu_stream_id { - uint16_t offset; - uint16_t stream_id; - char dev_name[32]; + uint16_t mac1; + uint16_t mac2; + uint16_t mac3; + uint16_t pex1; + uint16_t pex2; + uint16_t dma; + uint16_t sata; + uint16_t usb3; + uint16_t qe; + uint16_t sdhc; + uint16_t adma; + uint16_t dcu; + uint16_t usb2; + uint16_t debug; }; void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int size); -void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, uint32_t num); +void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id); #endif diff --git a/board/freescale/common/ls102xa_stream_id.c b/board/freescale/common/ls102xa_stream_id.c index f434269..941f22d 100644 --- a/board/freescale/common/ls102xa_stream_id.c +++ b/board/freescale/common/ls102xa_stream_id.c @@ -7,14 +7,25 @@ #include <common.h> #include <asm/io.h> #include <asm/arch/ls102xa_stream_id.h> +#include <asm/arch/immap_ls102xa.h> -void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id, uint32_t num) +void ls102xa_config_smmu_stream_id(struct smmu_stream_id *id) { - uint32_t *scfg = (uint32_t *)CONFIG_SYS_FSL_SCFG_ADDR; - int i; - - for (i = 0; i < num; i++) - out_be32(scfg + id[i].offset, id[i].stream_id); + struct ccsr_scfg *scfg = (struct ccsr_scfg *)CONFIG_SYS_FSL_SCFG_ADDR; + scfg->mac1_streamid = id->mac1; + scfg->mac2_streamid = id->mac2; + scfg->mac3_streamid = id->mac3; + scfg->pex1_streamid = id->pex1; + scfg->pex2_streamid = id->pex2; + scfg->dma_streamid = id->dma; + scfg->sata_streamid = id->sata; + scfg->usb3_streamid = id->usb3; + scfg->qe_streamid = id->qe; + scfg->sdhc_streamid = id->sdhc; + scfg->adma_streamid = id->adma; + scfg->dcu_streamid = id->dcu; + scfg->usb2_streamid = id->usb2; + scfg->debug_streamid = id->debug; } void ls1021x_config_caam_stream_id(struct liodn_id_table *tbl, int size)