| Message ID | 20181119131357.28648-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: tmio: introduce mask for 'always 1' bits | expand |
Hello! On 11/19/2018 04:13 PM, Wolfram Sang wrote: > Some variants (namely Renesas SDHI) have bits in the STATS and IRQ_MASK > registers which are 'always 1' and should be written as such. Introduce > a seperate mask for this and apply it whenever such a register is Separate. :-) > written. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > This is my response to BSP patches modifying all the register writes directly > (BSP commits 2d339800bbc73d and f73d5eb86097df). This seems much more future > proof to me. > > Yamada-san: could you check if your HW has 'always 1' bits, too? > > Tested on a R-Car M3-N. No regression and no performance impacts when reading > large files from eMMC or UHS-SD cards. [...] MBR, Sergei
On Mon, Nov 19, 2018 at 02:13:57PM +0100, Wolfram Sang wrote: > Some variants (namely Renesas SDHI) have bits in the STATS and IRQ_MASK > registers which are 'always 1' and should be written as such. Introduce > a seperate mask for this and apply it whenever such a register is > written. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > This is my response to BSP patches modifying all the register writes directly > (BSP commits 2d339800bbc73d and f73d5eb86097df). This seems much more future > proof to me. > > Yamada-san: could you check if your HW has 'always 1' bits, too? > > Tested on a R-Car M3-N. No regression and no performance impacts when reading > large files from eMMC or UHS-SD cards. > > drivers/mmc/host/renesas_sdhi_core.c | 1 + > drivers/mmc/host/tmio_mmc.h | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index a049b01206f1..31a351a20dc0 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -722,6 +722,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, > host->ops.card_busy = renesas_sdhi_card_busy; > host->ops.start_signal_voltage_switch = > renesas_sdhi_start_signal_voltage_switch; > + host->sdcard_irq_setbit_mask = TMIO_STAT_ALWAYS_SET_27; > } > > /* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */ > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index 5f6dfb86e43e..c03529e3f01a 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -70,6 +70,7 @@ > #define TMIO_STAT_DAT0 BIT(23) /* only known on R-Car so far */ > #define TMIO_STAT_RXRDY BIT(24) > #define TMIO_STAT_TXRQ BIT(25) > +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ The _27 seems to be odd to include in the name, but I assume it was the least bad option you could come up with. Other that that this looks good to me. Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > #define TMIO_STAT_ILL_FUNC BIT(29) /* only when !TMIO_MMC_HAS_IDLE_WAIT */ > #define TMIO_STAT_SCLKDIVEN BIT(29) /* only when TMIO_MMC_HAS_IDLE_WAIT */ > #define TMIO_STAT_CMD_BUSY BIT(30) > @@ -154,6 +155,7 @@ struct tmio_mmc_host { > u32 sdcard_irq_mask; > u32 sdio_irq_mask; > unsigned int clk_cache; > + u32 sdcard_irq_setbit_mask; > > spinlock_t lock; /* protect host private data */ > unsigned long last_req_ts; > @@ -268,6 +270,9 @@ static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr, > static inline void sd_ctrl_write32_as_16_and_16(struct tmio_mmc_host *host, > int addr, u32 val) > { > + if (addr == CTL_IRQ_MASK || addr == CTL_STATUS) > + val |= host->sdcard_irq_setbit_mask; > + > iowrite16(val & 0xffff, host->ctl + (addr << host->bus_shift)); > iowrite16(val >> 16, host->ctl + ((addr + 2) << host->bus_shift)); > } > -- > 2.11.0 >
> > +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ > > The _27 seems to be odd to include in the name, but I assume it > was the least bad option you could come up with. Yes :) I am open for better suggestions.
On Thu, Nov 22, 2018 at 03:06:59PM +0100, Wolfram Sang wrote: > > > > +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ > > > > The _27 seems to be odd to include in the name, but I assume it > > was the least bad option you could come up with. > > Yes :) I am open for better suggestions. Simply dropping '_27' would be an improvement in my opinion.
Hi Wolfram, Thanks for your work. On 2018-11-23 13:57:32 +0100, Simon Horman wrote: > On Thu, Nov 22, 2018 at 03:06:59PM +0100, Wolfram Sang wrote: > > > > > > +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ > > > > > > The _27 seems to be odd to include in the name, but I assume it > > > was the least bad option you could come up with. > > > > Yes :) I am open for better suggestions. > > Simply dropping '_27' would be an improvement in my opinion. I'm slightly agreeing with Simon here to drop the _27. With or without that change I think this patch looks good so feel free to add. Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ > > > > > > The _27 seems to be odd to include in the name, but I assume it > > > was the least bad option you could come up with. > > > > Yes :) I am open for better suggestions. > > Simply dropping '_27' would be an improvement in my opinion. And if we get a second one later? Or make it a mask then?
On Fri, Nov 23, 2018 at 02:15:38PM +0100, Wolfram Sang wrote: > > > > > > +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ > > > > > > > > The _27 seems to be odd to include in the name, but I assume it > > > > was the least bad option you could come up with. > > > > > > Yes :) I am open for better suggestions. > > > > Simply dropping '_27' would be an improvement in my opinion. > > And if we get a second one later? Or make it a mask then? Lets not spend too much time debating a better macro name. But in my opinion we can cross the second bit bridge if and when we reach it.
On Mon, 19 Nov 2018 at 14:14, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Some variants (namely Renesas SDHI) have bits in the STATS and IRQ_MASK > registers which are 'always 1' and should be written as such. Introduce > a seperate mask for this and apply it whenever such a register is > written. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Applied for next, thanks! Kind regards Uffe > --- > > This is my response to BSP patches modifying all the register writes directly > (BSP commits 2d339800bbc73d and f73d5eb86097df). This seems much more future > proof to me. > > Yamada-san: could you check if your HW has 'always 1' bits, too? > > Tested on a R-Car M3-N. No regression and no performance impacts when reading > large files from eMMC or UHS-SD cards. > > drivers/mmc/host/renesas_sdhi_core.c | 1 + > drivers/mmc/host/tmio_mmc.h | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index a049b01206f1..31a351a20dc0 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -722,6 +722,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, > host->ops.card_busy = renesas_sdhi_card_busy; > host->ops.start_signal_voltage_switch = > renesas_sdhi_start_signal_voltage_switch; > + host->sdcard_irq_setbit_mask = TMIO_STAT_ALWAYS_SET_27; > } > > /* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */ > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index 5f6dfb86e43e..c03529e3f01a 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -70,6 +70,7 @@ > #define TMIO_STAT_DAT0 BIT(23) /* only known on R-Car so far */ > #define TMIO_STAT_RXRDY BIT(24) > #define TMIO_STAT_TXRQ BIT(25) > +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ > #define TMIO_STAT_ILL_FUNC BIT(29) /* only when !TMIO_MMC_HAS_IDLE_WAIT */ > #define TMIO_STAT_SCLKDIVEN BIT(29) /* only when TMIO_MMC_HAS_IDLE_WAIT */ > #define TMIO_STAT_CMD_BUSY BIT(30) > @@ -154,6 +155,7 @@ struct tmio_mmc_host { > u32 sdcard_irq_mask; > u32 sdio_irq_mask; > unsigned int clk_cache; > + u32 sdcard_irq_setbit_mask; > > spinlock_t lock; /* protect host private data */ > unsigned long last_req_ts; > @@ -268,6 +270,9 @@ static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr, > static inline void sd_ctrl_write32_as_16_and_16(struct tmio_mmc_host *host, > int addr, u32 val) > { > + if (addr == CTL_IRQ_MASK || addr == CTL_STATUS) > + val |= host->sdcard_irq_setbit_mask; > + > iowrite16(val & 0xffff, host->ctl + (addr << host->bus_shift)); > iowrite16(val >> 16, host->ctl + ((addr + 2) << host->bus_shift)); > } > -- > 2.11.0 >
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index a049b01206f1..31a351a20dc0 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -722,6 +722,7 @@ int renesas_sdhi_probe(struct platform_device *pdev, host->ops.card_busy = renesas_sdhi_card_busy; host->ops.start_signal_voltage_switch = renesas_sdhi_start_signal_voltage_switch; + host->sdcard_irq_setbit_mask = TMIO_STAT_ALWAYS_SET_27; } /* Orginally registers were 16 bit apart, could be 32 or 64 nowadays */ diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 5f6dfb86e43e..c03529e3f01a 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -70,6 +70,7 @@ #define TMIO_STAT_DAT0 BIT(23) /* only known on R-Car so far */ #define TMIO_STAT_RXRDY BIT(24) #define TMIO_STAT_TXRQ BIT(25) +#define TMIO_STAT_ALWAYS_SET_27 BIT(27) /* only known on R-Car 2+ so far */ #define TMIO_STAT_ILL_FUNC BIT(29) /* only when !TMIO_MMC_HAS_IDLE_WAIT */ #define TMIO_STAT_SCLKDIVEN BIT(29) /* only when TMIO_MMC_HAS_IDLE_WAIT */ #define TMIO_STAT_CMD_BUSY BIT(30) @@ -154,6 +155,7 @@ struct tmio_mmc_host { u32 sdcard_irq_mask; u32 sdio_irq_mask; unsigned int clk_cache; + u32 sdcard_irq_setbit_mask; spinlock_t lock; /* protect host private data */ unsigned long last_req_ts; @@ -268,6 +270,9 @@ static inline void sd_ctrl_write16_rep(struct tmio_mmc_host *host, int addr, static inline void sd_ctrl_write32_as_16_and_16(struct tmio_mmc_host *host, int addr, u32 val) { + if (addr == CTL_IRQ_MASK || addr == CTL_STATUS) + val |= host->sdcard_irq_setbit_mask; + iowrite16(val & 0xffff, host->ctl + (addr << host->bus_shift)); iowrite16(val >> 16, host->ctl + ((addr + 2) << host->bus_shift)); }
Some variants (namely Renesas SDHI) have bits in the STATS and IRQ_MASK registers which are 'always 1' and should be written as such. Introduce a seperate mask for this and apply it whenever such a register is written. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- This is my response to BSP patches modifying all the register writes directly (BSP commits 2d339800bbc73d and f73d5eb86097df). This seems much more future proof to me. Yamada-san: could you check if your HW has 'always 1' bits, too? Tested on a R-Car M3-N. No regression and no performance impacts when reading large files from eMMC or UHS-SD cards. drivers/mmc/host/renesas_sdhi_core.c | 1 + drivers/mmc/host/tmio_mmc.h | 5 +++++ 2 files changed, 6 insertions(+)