| Message ID | 1410182892-18647-6-git-send-email-wens@csie.org |
|---|---|
| State | Superseded |
| Delegated to: | Ian Campbell |
| Headers | show |
On Mon, 2014-09-08 at 21:28 +0800, Chen-Yu Tsai wrote: > From: Hans de Goede <hdegoede@redhat.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > [wens@csie.org: use setbits_le32 for reset control, drop obsolete changes, > squash "sunxi-mmc: sun6i has its fifo at a different address"] > Signed-off-by: Chen-Yu Tsai <wens@csie.org> Adding CC to Pantelis (MMC custodian). Pantelis, once you are happy with this I propose we take this via the sunxi tree along with the rest of the series. For my part I only have nitpicks: > --- > arch/arm/include/asm/arch-sunxi/mmc.h | 2 -- > drivers/mmc/sunxi_mmc.c | 9 +++++++++ > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/arch-sunxi/mmc.h b/arch/arm/include/asm/arch-sunxi/mmc.h > index 53196e3..bafde4b 100644 > --- a/arch/arm/include/asm/arch-sunxi/mmc.h > +++ b/arch/arm/include/asm/arch-sunxi/mmc.h > @@ -42,8 +42,6 @@ struct sunxi_mmc { > u32 idie; /* 0x8c internal DMA interrupt enable */ > u32 chda; /* 0x90 */ > u32 cbda; /* 0x94 */ > - u32 res1[26]; > - u32 fifo; /* 0x100 FIFO access address */ This seems unrelated to the stated purpose of the commit, should probably be a separate cleanup. > }; > > #define SUNXI_MMC_CLK_POWERSAVE (0x1 << 17) > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c > index d4e574f..b035bba 100644 > --- a/drivers/mmc/sunxi_mmc.c > +++ b/drivers/mmc/sunxi_mmc.c > @@ -57,7 +57,11 @@ static int mmc_resource_init(int sdc_no) > printf("Wrong mmc number %d\n", sdc_no); > return -1; > } > +#ifdef CONFIG_SUN6I > + mmchost->database = (unsigned int)mmchost->reg + 0x200; > +#else > mmchost->database = (unsigned int)mmchost->reg + 0x100; > +#endif Adding a #define to ./include/configs/sun?i.h would be preferred, I think. Ian.
On Mon, Sep 22, 2014 at 2:44 AM, Ian Campbell <ijc@hellion.org.uk> wrote: > On Mon, 2014-09-08 at 21:28 +0800, Chen-Yu Tsai wrote: >> From: Hans de Goede <hdegoede@redhat.com> >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> [wens@csie.org: use setbits_le32 for reset control, drop obsolete changes, >> squash "sunxi-mmc: sun6i has its fifo at a different address"] >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > Adding CC to Pantelis (MMC custodian). > > Pantelis, once you are happy with this I propose we take this via the > sunxi tree along with the rest of the series. > > For my part I only have nitpicks: > >> --- >> arch/arm/include/asm/arch-sunxi/mmc.h | 2 -- >> drivers/mmc/sunxi_mmc.c | 9 +++++++++ >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/include/asm/arch-sunxi/mmc.h b/arch/arm/include/asm/arch-sunxi/mmc.h >> index 53196e3..bafde4b 100644 >> --- a/arch/arm/include/asm/arch-sunxi/mmc.h >> +++ b/arch/arm/include/asm/arch-sunxi/mmc.h >> @@ -42,8 +42,6 @@ struct sunxi_mmc { >> u32 idie; /* 0x8c internal DMA interrupt enable */ >> u32 chda; /* 0x90 */ >> u32 cbda; /* 0x94 */ >> - u32 res1[26]; >> - u32 fifo; /* 0x100 FIFO access address */ > > This seems unrelated to the stated purpose of the commit, should > probably be a separate cleanup. This was part of "sunxi-mmc: sun6i has its fifo at a different address", but yeah, it definitely looks like a separate cleanup now. I'll split it out. >> }; >> >> #define SUNXI_MMC_CLK_POWERSAVE (0x1 << 17) >> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c >> index d4e574f..b035bba 100644 >> --- a/drivers/mmc/sunxi_mmc.c >> +++ b/drivers/mmc/sunxi_mmc.c >> @@ -57,7 +57,11 @@ static int mmc_resource_init(int sdc_no) >> printf("Wrong mmc number %d\n", sdc_no); >> return -1; >> } >> +#ifdef CONFIG_SUN6I >> + mmchost->database = (unsigned int)mmchost->reg + 0x200; >> +#else >> mmchost->database = (unsigned int)mmchost->reg + 0x100; >> +#endif > > Adding a #define to ./include/configs/sun?i.h would be preferred, I > think. Sounds reasonable. I wonder what else (in other drivers) we should move over there. ChenYu
On Mon, Sep 22, 2014 at 10:11 AM, Chen-Yu Tsai <wens@csie.org> wrote: > On Mon, Sep 22, 2014 at 2:44 AM, Ian Campbell <ijc@hellion.org.uk> wrote: >> On Mon, 2014-09-08 at 21:28 +0800, Chen-Yu Tsai wrote: >>> From: Hans de Goede <hdegoede@redhat.com> >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> [wens@csie.org: use setbits_le32 for reset control, drop obsolete changes, >>> squash "sunxi-mmc: sun6i has its fifo at a different address"] >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> >> Adding CC to Pantelis (MMC custodian). >> >> Pantelis, once you are happy with this I propose we take this via the >> sunxi tree along with the rest of the series. >> >> For my part I only have nitpicks: >> >>> --- >>> arch/arm/include/asm/arch-sunxi/mmc.h | 2 -- >>> drivers/mmc/sunxi_mmc.c | 9 +++++++++ >>> 2 files changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/arch-sunxi/mmc.h b/arch/arm/include/asm/arch-sunxi/mmc.h >>> index 53196e3..bafde4b 100644 >>> --- a/arch/arm/include/asm/arch-sunxi/mmc.h >>> +++ b/arch/arm/include/asm/arch-sunxi/mmc.h >>> @@ -42,8 +42,6 @@ struct sunxi_mmc { >>> u32 idie; /* 0x8c internal DMA interrupt enable */ >>> u32 chda; /* 0x90 */ >>> u32 cbda; /* 0x94 */ >>> - u32 res1[26]; >>> - u32 fifo; /* 0x100 FIFO access address */ >> >> This seems unrelated to the stated purpose of the commit, should >> probably be a separate cleanup. > > This was part of "sunxi-mmc: sun6i has its fifo at a different address", > but yeah, it definitely looks like a separate cleanup now. I'll split it > out. > >>> }; >>> >>> #define SUNXI_MMC_CLK_POWERSAVE (0x1 << 17) >>> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c >>> index d4e574f..b035bba 100644 >>> --- a/drivers/mmc/sunxi_mmc.c >>> +++ b/drivers/mmc/sunxi_mmc.c >>> @@ -57,7 +57,11 @@ static int mmc_resource_init(int sdc_no) >>> printf("Wrong mmc number %d\n", sdc_no); >>> return -1; >>> } >>> +#ifdef CONFIG_SUN6I >>> + mmchost->database = (unsigned int)mmchost->reg + 0x200; >>> +#else >>> mmchost->database = (unsigned int)mmchost->reg + 0x100; >>> +#endif >> >> Adding a #define to ./include/configs/sun?i.h would be preferred, I >> think. > > Sounds reasonable. I wonder what else (in other drivers) we should > move over there. Ian, include/configs/sun?i.h and sunxi-common.h only have config related #defines. Are we sure this is the place for something like register offsets? For reference, drivers/i2c/mvtwsi.c has sunxi specific register offsets wrapped in a #define in the file itself. Cheers ChenYu
On Tue, 2014-09-23 at 19:50 +0800, Chen-Yu Tsai wrote: > Ian, include/configs/sun?i.h and sunxi-common.h only have config > related #defines. Are we sure this is the place for something > like register offsets? I guess not ;-) > For reference, drivers/i2c/mvtwsi.c has sunxi specific register > offsets wrapped in a #define in the file itself. How about either ./arch/arm/include/asm/arch-sunxi/mmc.h or near the top of this C file (i.e. outside the code itself)? Ian.
On Tue, Sep 23, 2014 at 7:54 PM, Ian Campbell <ijc@hellion.org.uk> wrote: > On Tue, 2014-09-23 at 19:50 +0800, Chen-Yu Tsai wrote: >> Ian, include/configs/sun?i.h and sunxi-common.h only have config >> related #defines. Are we sure this is the place for something >> like register offsets? > > I guess not ;-) > >> For reference, drivers/i2c/mvtwsi.c has sunxi specific register >> offsets wrapped in a #define in the file itself. > > How about either ./arch/arm/include/asm/arch-sunxi/mmc.h or near the top > of this C file (i.e. outside the code itself)? Adding it to the register definitions in ./arch/arm/include/asm/arch-sunxi/mmc.h seems like a good choice. The last bit of struct sunxi_mmc would be like: u32 idie; /* 0x8c internal DMA interrupt enable */ u32 chda; /* 0x90 */ u32 cbda; /* 0x94 */ #if defined(CONFIG_SUN6I) u32 res1[126]; #else u32 res1[26]; #endif u32 fifo; /* 0x100 (0x200 on sun6i) FIFO access address */ }; And have mmchost->database = &mmchost->reg->fifo; Or just get rid of ->database and use ->reg->fifo directly. The latter seems better. ChenYu
On Tue, 2014-09-23 at 20:07 +0800, Chen-Yu Tsai wrote: > On Tue, Sep 23, 2014 at 7:54 PM, Ian Campbell <ijc@hellion.org.uk> wrote: > > On Tue, 2014-09-23 at 19:50 +0800, Chen-Yu Tsai wrote: > >> Ian, include/configs/sun?i.h and sunxi-common.h only have config > >> related #defines. Are we sure this is the place for something > >> like register offsets? > > > > I guess not ;-) > > > >> For reference, drivers/i2c/mvtwsi.c has sunxi specific register > >> offsets wrapped in a #define in the file itself. > > > > How about either ./arch/arm/include/asm/arch-sunxi/mmc.h or near the top > > of this C file (i.e. outside the code itself)? > > Adding it to the register definitions in ./arch/arm/include/asm/arch-sunxi/mmc.h > seems like a good choice. The last bit of struct sunxi_mmc would be like: > > u32 idie; /* 0x8c internal DMA interrupt enable */ > u32 chda; /* 0x90 */ > u32 cbda; /* 0x94 */ > #if defined(CONFIG_SUN6I) > u32 res1[126]; > #else > u32 res1[26]; > #endif > u32 fifo; /* 0x100 (0x200 on sun6i) FIFO access address */ > }; > > And have > > mmchost->database = &mmchost->reg->fifo; > > Or just get rid of ->database and use ->reg->fifo directly. > The latter seems better. Yep, sounds good. You could also consider just #if defined(CONFIG_SUN6I) u32 res2[100]; #endif after the existing res1.
diff --git a/arch/arm/include/asm/arch-sunxi/mmc.h b/arch/arm/include/asm/arch-sunxi/mmc.h index 53196e3..bafde4b 100644 --- a/arch/arm/include/asm/arch-sunxi/mmc.h +++ b/arch/arm/include/asm/arch-sunxi/mmc.h @@ -42,8 +42,6 @@ struct sunxi_mmc { u32 idie; /* 0x8c internal DMA interrupt enable */ u32 chda; /* 0x90 */ u32 cbda; /* 0x94 */ - u32 res1[26]; - u32 fifo; /* 0x100 FIFO access address */ }; #define SUNXI_MMC_CLK_POWERSAVE (0x1 << 17) diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index d4e574f..b035bba 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -57,7 +57,11 @@ static int mmc_resource_init(int sdc_no) printf("Wrong mmc number %d\n", sdc_no); return -1; } +#ifdef CONFIG_SUN6I + mmchost->database = (unsigned int)mmchost->reg + 0x200; +#else mmchost->database = (unsigned int)mmchost->reg + 0x100; +#endif mmchost->mmc_no = sdc_no; return 0; @@ -75,6 +79,11 @@ static int mmc_clk_io_on(int sdc_no) /* config ahb clock */ setbits_le32(&ccm->ahb_gate0, 1 << AHB_GATE_OFFSET_MMC(sdc_no)); +#if defined(CONFIG_SUN6I) + /* unassert reset */ + setbits_le32(&ccm->ahb_reset0_cfg, 1 << AHB_RESET_OFFSET_MMC(sdc_no)); +#endif + /* config mod clock */ pll_clk = clock_get_pll6(); /* should be close to 100 MHz but no more, so round up */