| Message ID | 20180606232252.31583-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Niklas, On Thu, Jun 07, 2018 at 01:22:50AM +0200, Niklas Söderlund wrote: > From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com> > > The initial value of the interrupt mask register may be different from > the H/W manual at the startup of the kernel by setting of initial program. s/initial program/bootloader/ ? > Since the error interrupts may be unmasked, the driver sets initial value. > > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/mmc/host/tmio_mmc.h | 1 + > drivers/mmc/host/tmio_mmc_core.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index e7d651352dc90fb8..bfb76b2963f80894 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -102,6 +102,7 @@ > > /* Define some IRQ masks */ > /* This is the mask used at reset by the chip */ > +#define TMIO_MASK_INIT 0x8b7f031d /* H/W initial value */ known on R-Car 2+ only! > #define TMIO_MASK_ALL 0x837f031d > #define TMIO_MASK_READOP (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND) > #define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND) > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 3080299303045adc..2ede81476daf76ce 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1273,6 +1273,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > tmio_mmc_clk_stop(_host); > tmio_mmc_reset(_host); > > + sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK, TMIO_MASK_INIT); So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think. Maybe we should even move it to renesas_sdhi_core.c, but I am not too sure about that. Regards, Wolfram
Hi Wolfram, Thanks for your feedback. On 2018-06-09 20:15:52 +0200, Wolfram Sang wrote: > Hi Niklas, > > On Thu, Jun 07, 2018 at 01:22:50AM +0200, Niklas Söderlund wrote: > > From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com> > > > > The initial value of the interrupt mask register may be different from > > the H/W manual at the startup of the kernel by setting of initial program. > > s/initial program/bootloader/ ? Sounds much better. > > > Since the error interrupts may be unmasked, the driver sets initial value. > > > > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/mmc/host/tmio_mmc.h | 1 + > > drivers/mmc/host/tmio_mmc_core.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > > index e7d651352dc90fb8..bfb76b2963f80894 100644 > > --- a/drivers/mmc/host/tmio_mmc.h > > +++ b/drivers/mmc/host/tmio_mmc.h > > @@ -102,6 +102,7 @@ > > > > /* Define some IRQ masks */ > > /* This is the mask used at reset by the chip */ > > +#define TMIO_MASK_INIT 0x8b7f031d /* H/W initial value */ > > known on R-Car 2+ only! No it's also know on Gen3 :-) If you check the documents for the registers SD_INFO1_MASK and SD_INFO2_MASK they describe there respective initial values. If one combines them you get the same constant as documented here. The tricky part is that sd_ctrl_write32_as_16_and_16() is used to write them so you have to take 16 bits from each register description as that is how the are written. After the fun bus shift is applied this ends up writing the lower 16 bits of the constant to 0x80 and the higher 16 of it to 0x88. > > > #define TMIO_MASK_ALL 0x837f031d > > #define TMIO_MASK_READOP (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND) > > #define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND) > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > > index 3080299303045adc..2ede81476daf76ce 100644 > > --- a/drivers/mmc/host/tmio_mmc_core.c > > +++ b/drivers/mmc/host/tmio_mmc_core.c > > @@ -1273,6 +1273,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) > > tmio_mmc_clk_stop(_host); > > tmio_mmc_reset(_host); > > > > + sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK, TMIO_MASK_INIT); > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think. > Maybe we should even move it to renesas_sdhi_core.c, but I am not too > sure about that. > > Regards, > > Wolfram >
> > > +#define TMIO_MASK_INIT 0x8b7f031d /* H/W initial value */ > > > > known on R-Car 2+ only! > > No it's also know on Gen3 :-) Sure, 2+ means "R-Car 2 and later". Like with GPL2+. Sorry if this was not clear. And it is still my main argument to do... > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think. > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too > > sure about that. ... this.
Hi Wolfram, Thanks for your feedback. On 2018-06-27 09:27:02 +0200, Wolfram Sang wrote: > > > > > +#define TMIO_MASK_INIT 0x8b7f031d /* H/W initial value */ > > > > > > known on R-Car 2+ only! > > > > No it's also know on Gen3 :-) > > Sure, 2+ means "R-Car 2 and later". Like with GPL2+. Sorry if this was > not clear. And it is still my main argument to do... When you point it out it's clear, sorry I missed that. > > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think. > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too > > > sure about that. > > ... this. > I will look into this and send a new version :-)
> > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think. > > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too > > > > sure about that. > > > > ... this. > > > > I will look into this and send a new version :-) Did you send V2 of this? I can't find it.
Hi Wolfram, On 2018-09-16 16:48:07 +0200, Wolfram Sang wrote: > > > > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think. > > > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too > > > > > sure about that. > > > > > > ... this. > > > > > > > I will look into this and send a new version :-) > > Did you send V2 of this? I can't find it. > No I have not yet posted a v2 of this series. Perhaps I should break this patch out and send separate? I have handled the comments for this but patch 3/3 in this series still requires more work, what do you think?
On Thu, Sep 20, 2018 at 12:02:27AM +0200, Niklas Söderlund wrote: > Hi Wolfram, > > On 2018-09-16 16:48:07 +0200, Wolfram Sang wrote: > > > > > > > > So, we should at least protect this with TMIO_MMC_MIN_RCAR2, I'd think. > > > > > > Maybe we should even move it to renesas_sdhi_core.c, but I am not too > > > > > > sure about that. > > > > > > > > ... this. > > > > > > > > > > I will look into this and send a new version :-) > > > > Did you send V2 of this? I can't find it. > > > > No I have not yet posted a v2 of this series. Perhaps I should break > this patch out and send separate? I have handled the comments for this > but patch 3/3 in this series still requires more work, what do you > think? Yes, please.
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index e7d651352dc90fb8..bfb76b2963f80894 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -102,6 +102,7 @@ /* Define some IRQ masks */ /* This is the mask used at reset by the chip */ +#define TMIO_MASK_INIT 0x8b7f031d /* H/W initial value */ #define TMIO_MASK_ALL 0x837f031d #define TMIO_MASK_READOP (TMIO_STAT_RXRDY | TMIO_STAT_DATAEND) #define TMIO_MASK_WRITEOP (TMIO_STAT_TXRQ | TMIO_STAT_DATAEND) diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 3080299303045adc..2ede81476daf76ce 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1273,6 +1273,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host) tmio_mmc_clk_stop(_host); tmio_mmc_reset(_host); + sd_ctrl_write32_as_16_and_16(_host, CTL_IRQ_MASK, TMIO_MASK_INIT); _host->sdcard_irq_mask = sd_ctrl_read16_and_16_as_32(_host, CTL_IRQ_MASK); tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);