| Message ID | 1389277919-15279-8-git-send-email-m.zalega@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
Hi Mateusz, On 01/09/2014 11:31 PM, Mateusz Zalega wrote: > In some cases MMC was still uninitialized while media capacity check, > leading to broken ums command. > > Change-Id: I4b86c2c59e430fb8b55272ea14f00316d8cb3dca > Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Minkyu Kang <mk7.kang@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > --- > board/samsung/common/ums.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/board/samsung/common/ums.c b/board/samsung/common/ums.c > index dc155ad..0d8f30d 100644 > --- a/board/samsung/common/ums.c > +++ b/board/samsung/common/ums.c > @@ -37,6 +37,9 @@ static struct ums ums_dev = { > > static struct ums *ums_disk_init(struct mmc *mmc) > { > + if (mmc_init(mmc)) > + return NULL; > + > uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE; > uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR; --> if (mmc_init(mmc)) return NULL; Locate this point. Best Regards, Jaehoon Chung > >
Hi Mateusz, > In some cases MMC was still uninitialized while media capacity check, > leading to broken ums command. > > Change-Id: I4b86c2c59e430fb8b55272ea14f00316d8cb3dca > Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Minkyu Kang <mk7.kang@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > --- > board/samsung/common/ums.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/board/samsung/common/ums.c b/board/samsung/common/ums.c > index dc155ad..0d8f30d 100644 > --- a/board/samsung/common/ums.c > +++ b/board/samsung/common/ums.c > @@ -37,6 +37,9 @@ static struct ums ums_dev = { > > static struct ums *ums_disk_init(struct mmc *mmc) > { > + if (mmc_init(mmc)) > + return NULL; > + > uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE; > uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR; > Tested-by: Lukasz Majewski <l.majewski@samsung.com> Tested at: Exynos4210 - TRATS.
On 01/10/14 06:08, Jaehoon Chung wrote: >> index dc155ad..0d8f30d 100644 >> --- a/board/samsung/common/ums.c >> +++ b/board/samsung/common/ums.c >> @@ -37,6 +37,9 @@ static struct ums ums_dev = { >> >> static struct ums *ums_disk_init(struct mmc *mmc) >> { >> + if (mmc_init(mmc)) >> + return NULL; >> + >> uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE; >> uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR; > > --> if (mmc_init(mmc)) > return NULL; > > Locate this point. If you're asking to put this if() block after variable declaration, NAK. It's perfectly fine C99 code. I'm not aware of any existing U-Boot style guidelines that would forbid me to leave it this way. These variables are only meaningful when mmc_init() returns a valid pointer. Regards,
Hi On Mon, Jan 13, 2014 at 3:39 PM, Mateusz Zalega <m.zalega@samsung.com> wrote: > On 01/10/14 06:08, Jaehoon Chung wrote: >>> index dc155ad..0d8f30d 100644 >>> --- a/board/samsung/common/ums.c >>> +++ b/board/samsung/common/ums.c >>> @@ -37,6 +37,9 @@ static struct ums ums_dev = { >>> >>> static struct ums *ums_disk_init(struct mmc *mmc) >>> { >>> + if (mmc_init(mmc)) >>> + return NULL; >>> + >>> uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE; >>> uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR; >> >> --> if (mmc_init(mmc)) >> return NULL; >> >> Locate this point. > > If you're asking to put this if() block after variable declaration, NAK. > I don't understand your point > It's perfectly fine C99 code. I'm not aware of any existing U-Boot style > guidelines that would forbid me to leave it this way. > > These variables are only meaningful when mmc_init() returns a valid pointer. > http://www.denx.de/wiki/U-Boot/CodingStyle Michael > Regards, > -- > Mateusz Zalega > Samsung R&D Institute Poland > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
On 01/13/14 15:43, Michael Trimarchi wrote: > On Mon, Jan 13, 2014 at 3:39 PM, Mateusz Zalega <m.zalega@samsung.com> wrote: >> On 01/10/14 06:08, Jaehoon Chung wrote: >>>> index dc155ad..0d8f30d 100644 >>>> --- a/board/samsung/common/ums.c >>>> +++ b/board/samsung/common/ums.c >>>> @@ -37,6 +37,9 @@ static struct ums ums_dev = { >>>> >>>> static struct ums *ums_disk_init(struct mmc *mmc) >>>> { >>>> + if (mmc_init(mmc)) >>>> + return NULL; >>>> + >>>> uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE; >>>> uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR; >>> >>> --> if (mmc_init(mmc)) >>> return NULL; >>> >>> Locate this point. >> >> If you're asking to put this if() block after variable declaration, NAK. >> > > I don't understand your point > >> It's perfectly fine C99 code. I'm not aware of any existing U-Boot style >> guidelines that would forbid me to leave it this way. >> >> These variables are only meaningful when mmc_init() returns a valid pointer. >> > > http://www.denx.de/wiki/U-Boot/CodingStyle > > Michael touché OK, I'll move declarations to the beginning of the closure, C89 style. Thanks,
diff --git a/board/samsung/common/ums.c b/board/samsung/common/ums.c index dc155ad..0d8f30d 100644 --- a/board/samsung/common/ums.c +++ b/board/samsung/common/ums.c @@ -37,6 +37,9 @@ static struct ums ums_dev = { static struct ums *ums_disk_init(struct mmc *mmc) { + if (mmc_init(mmc)) + return NULL; + uint64_t mmc_end_sector = mmc->capacity / SECTOR_SIZE; uint64_t ums_end_sector = UMS_NUM_SECTORS + UMS_START_SECTOR;
In some cases MMC was still uninitialized while media capacity check, leading to broken ums command. Change-Id: I4b86c2c59e430fb8b55272ea14f00316d8cb3dca Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> Cc: Lukasz Majewski <l.majewski@samsung.com> Cc: Minkyu Kang <mk7.kang@samsung.com> Cc: Kyungmin Park <kyungmin.park@samsung.com> --- board/samsung/common/ums.c | 3 +++ 1 file changed, 3 insertions(+)