RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> > I don't understand "TMIO_MMC_BIG_DATA_PORT" (How do you know 32-bit?? > How do you know 64-bit??) > > I think 16 << host->bus_shift should work; need to verify this, though. Good idea.
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> I don't understand "TMIO_MMC_BIG_DATA_PORT" (How do you know 32-bit?? How do > you know 64-bit??) I think 16 << host->bus_shift should work; need to verify this, though. > So..maybe you can change it later and your idea will be more clear to me > then. Yes, fine with me. signature.asc Description: PGP signature
RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
Hi Wolfram, > Okay, I have a sketch how to add this feature to R-Car SoCs at a later > stage. This patch is fine with one minor nit which can be done now or I > can do it later when I add the R-Car support. > > > + * Some controllers have a 32-bit wide data port register */ > > +#define TMIO_MMC_32BIT_DATA_PORT (1 << 9) > > Since R-Car Gen3 has 64 bit port, I'd suggest to use > TMIO_MMC_BIG_DATA_PORT or something. But as I said, I can also do this > later. So I don't understand "TMIO_MMC_BIG_DATA_PORT" (How do you know 32-bit?? How do you know 64-bit??) So..maybe you can change it later and your idea will be more clear to me then. Thank you, Chris
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
Hi Chris, On Mon, Sep 12, 2016 at 10:15:06AM -0400, Chris Brandt wrote: > For the r7s72100 SOC, the DATA_PORT register was changed to 32-bits wide. > Therefore a new flag has been created that will allow 32-bit reads/writes > to the DATA_PORT register instead of 16-bit (because 16-bits accesses are > not supported). > > Signed-off-by: Chris Brandt Okay, I have a sketch how to add this feature to R-Car SoCs at a later stage. This patch is fine with one minor nit which can be done now or I can do it later when I add the R-Car support. > + * Some controllers have a 32-bit wide data port register > + */ > +#define TMIO_MMC_32BIT_DATA_PORT (1 << 9) Since R-Car Gen3 has 64 bit port, I'd suggest to use TMIO_MMC_BIG_DATA_PORT or something. But as I said, I can also do this later. So Reviewed-by: Wolfram Sang signature.asc Description: PGP signature
RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> > > This SWAP register exists on R-Car as well. Out of curiosity, what > > > is the register value of 0xE804E0E4? > > > > 0xE804E0E4 = 0x0001 > > > > So...something is there! > > I am quite convinced there is this BUSWIDTH register. If you are > interested, try setting this to 0 and see if it works without the 32 bit > register patches. Well, it looks like I can't write that register location to 0. It always keeps a value of 0x0001. So, it must be read-only. It was a good idea though. Still, I might try to find out what is there at that location out of curiosity. Without my the 32-bit write patch, I get a lot of these messages: sh_mobile_sdhi e804e800.sd: timeout waiting for SD bus idle Chris
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
On Mon, Oct 24, 2016 at 01:06:38PM +0200, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Fri, Oct 21, 2016 at 11:56 PM, Wolfram Sang wrote: > >> For the VERSION register, the low byte is the version of the IP, but > >> the upper byte is a number that the design group that made the part > > > > I know. It is just that I haven't seen this one "in the wild" so far. > > I think you can also see it on your tamed Genmai ;-) I intentionally did not write "couldn't have seen this" ;) signature.asc Description: PGP signature
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
Hi Wolfram,
On Fri, Oct 21, 2016 at 11:56 PM, Wolfram Sang wrote:
>> For the VERSION register, the low byte is the version of the IP, but
>> the upper byte is a number that the design group that made the part
>
> I know. It is just that I haven't seen this one "in the wild" so far.
I think you can also see it on your tamed Genmai ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> For the VERSION register, the low byte is the version of the IP, but > the upper byte is a number that the design group that made the part I know. It is just that I haven't seen this one "in the wild" so far. > > This SWAP register exists on R-Car as well. Out of curiosity, what is the > > register value of 0xE804E0E4? > > 0xE804E0E4 = 0x0001 > > So...something is there! I am quite convinced there is this BUSWIDTH register. If you are interested, try setting this to 0 and see if it works without the 32 bit register patches. signature.asc Description: PGP signature
RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> > > * Does it have a version register (CTL_VERSION)? If so, what does it > > > say? > > > > 0x820B > > Okay, this is a version I have not seen before. The SDHI IP came from the SH7269 (SH-2A device). For the VERSION register, the low byte is the version of the IP, but the upper byte is a number that the design group that made the part gives it any time they make a change. So, since this IP was from a SH2A team, the numbering might look different than what the SH4A or R-Car teams have used in the past. > This SWAP register exists on R-Car as well. Out of curiosity, what is the > register value of 0xE804E0E4? 0xE804E0E4 = 0x0001 So...something is there! Chris
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> However, for version 3.0 that will be posted shortly, it will start to > include the SDHI section. Nice. Could you ping me when this comes out? > > * Does it have a version register (CTL_VERSION)? If so, what does it > > say? > > 0x820B Okay, this is a version I have not seen before. > > * Does it have SD_BUF0 width setting (named either EXT_ACC or HOST_MODE, > > so far always comes after the version register)? If so, what is its > > layout? > > There is a "EXT_SWAP" register. It is the only thing after VERSION. >VERSION = 0xE804E0E2 > EXT_SWAP = 0xE804E0F0 This SWAP register exists on R-Car as well. Out of curiosity, what is the register value of 0xE804E0E4? signature.asc Description: PGP signature
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> I'll submit a v4 of this patch series tomorrow. Please wait a little. I'd like to use that feature on R-Car, too, so I wonder if the additional feature flag is the proper path. signature.asc Description: PGP signature
RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
Hello Wolfram, > > > + /* if count was multiple of 4 */ > > > + if (!(count & 0x3)) > > > + return; > > > + buf8 = (u8 *)(buf + (count >> 2)); > > > + count %= 4; > > > > To skip the same operation done on 'count' twice, maybe? > > > > buf8 = (u8 *)(buf + (count >> 2)); > > count &= 3; > > > > if (!count) > > return; > > > That looks like it should work. > I can try it out and see if there is something we are missing. Seems to work fine. Although I admit that because the driver is mostly being used as a block device (SD card), all the data requests are an even multiple of 4 (I put a printk after "if (!count) return;" ). Not sure if that will be the case when I test SDIO devices, but I'll find out once I start porting SDIO device drivers to 4.9+. I'll submit a v4 of this patch series tomorrow. Thank you. Chris
RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
On 10/20/2016, Wolfram Sang wrote: > Sadly, I don't have SDHI documentation for this SoC. In the current hardware manual on renesas.com, the SHDI section is not included. However, for version 3.0 that will be posted shortly, it will start to include the SDHI section. > * Does it have a version register (CTL_VERSION)? If so, what does it > say? 0x820B > * Does it have SD_BUF0 width setting (named either EXT_ACC or HOST_MODE, > so far always comes after the version register)? If so, what is its > layout? There is a "EXT_SWAP" register. It is the only thing after VERSION. VERSION = 0xE804E0E2 EXT_SWAP = 0xE804E0F0 Here are the bits in that register: DMASE (bit 8) DMA Transfer Size Select Selects the transfer unit for SD_BUF read/write DMA transfer. Set this bit in combination with the transfer size set in the DMA Channel Configuration register. 0: 4-byte (longword) unit 1: 64-byte (longword 16) unit SDBRSWAP (bit 7) SD_BUF0 Swap Read*1 When reading from SD_BUF0, data stored in SD_BUF0 can be replaced and then read.*2 0: The current data is read without replacement. 1: Data replacement for reading proceeds in bytes SDBWSWAP (bit 6) SD_BUF0 Swap Write*1 When writing to SD_BUF0, data to be written can be replaced and then stored in SD_BUF0.*2 0: The current data is written without replacement. 1: Data replacement for writing proceeds in bytes. > * Does it support DMA? Yes. > Is it compatible to the current implementation? I don't know yet. The current BSP is on 3.14 and it uses DMA, but I'm not sure what's going to happen as I start moving things to 4.9+. > > + /* if count was multiple of 4 */ > > + if (!(count & 0x3)) > > + return; > > + buf8 = (u8 *)(buf + (count >> 2)); > > + count %= 4; > > To skip the same operation done on 'count' twice, maybe? > > buf8 = (u8 *)(buf + (count >> 2)); > count &= 3; > > if (!count) > return; That looks like it should work. I can try it out and see if there is something we are missing. Thanks Chris
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
On Mon, Sep 12, 2016 at 10:15:06AM -0400, Chris Brandt wrote: > For the r7s72100 SOC, the DATA_PORT register was changed to 32-bits wide. > Therefore a new flag has been created that will allow 32-bit reads/writes > to the DATA_PORT register instead of 16-bit (because 16-bits accesses are > not supported). > > Signed-off-by: Chris Brandt > --- > v3: > * changed loops to memcpy > v2: > * changed 'data * 0xFF' to 'data & 0xFF' > * added 'const' for sd_ctrl_write32_rep Sadly, I don't have SDHI documentation for this SoC. * Does it have a version register (CTL_VERSION)? If so, what does it say? * Does it have SD_BUF0 width setting (named either EXT_ACC or HOST_MODE, so far always comes after the version register)? If so, what is its layout? Unrelated to this patch but nice to know: * Does it support DMA? Is it compatible to the current implementation? That being asked, R-Car SoCs can have SDBUF0 32-bit wide as well (Gen3 even 64-bit). So far, this is only used for DMA, though. Thanks, Wolfram > + /* if count was multiple of 4 */ > + if (!(count & 0x3)) > + return; > + buf8 = (u8 *)(buf + (count >> 2)); > + count %= 4; To skip the same operation done on 'count' twice, maybe? buf8 = (u8 *)(buf + (count >> 2)); count &= 3; if (!count) return; signature.asc Description: PGP signature
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
> > Is this still waiting on an ACK from Lee Jones because it touches the > > tmio.h file under include/linux/mfd/ ? > > > > Yes, but more important is Wolfram's ack as he maintains > drivers/mmc/host/tmio_mmc* Sorry, LinuxCon and ELC Europe kept me very busy. Will do this week. signature.asc Description: PGP signature
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
On 14 October 2016 at 15:18, Chris Brandt wrote: > On 9/22/2016, Ulf Hansson wrote: >> To: Chris Brandt >> Cc: Wolfram Sang ; Sergei Shtylyov >> ; Geert Uytterhoeven > m68k.org>; Simon Horman ; linux-mmc > m...@vger.kernel.org>; Linux-Renesas ; >> Lee Jones >> Subject: Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port >> >> + Lee >> >> On 12 September 2016 at 16:15, Chris Brandt >> wrote: >> > For the r7s72100 SOC, the DATA_PORT register was changed to 32-bits wide. >> > Therefore a new flag has been created that will allow 32-bit >> > reads/writes to the DATA_PORT register instead of 16-bit (because >> > 16-bits accesses are not supported). >> > >> > Signed-off-by: Chris Brandt >> > --- >> > v3: >> > * changed loops to memcpy >> > v2: >> > * changed 'data * 0xFF' to 'data & 0xFF' >> > * added 'const' for sd_ctrl_write32_rep >> > --- >> > drivers/mmc/host/tmio_mmc.h | 12 >> > drivers/mmc/host/tmio_mmc_pio.c | 30 ++ >> > include/linux/mfd/tmio.h| 5 + >> >> This header file needs to be split up. The mmc specific bits, should be >> moved to a local header file under driver/mmc/host/*. >> >> Sure, we don't need to do that as part of $subject patch, but then I need >> an ack from Lee Jones, the mfd maintainer. I have added him on cc. > > > Is this still waiting on an ACK from Lee Jones because it touches the tmio.h > file under include/linux/mfd/ ? > Yes, but more important is Wolfram's ack as he maintains drivers/mmc/host/tmio_mmc* Kind regards Uffe
RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
On 9/22/2016, Ulf Hansson wrote:
> To: Chris Brandt
> Cc: Wolfram Sang ; Sergei Shtylyov
> ; Geert Uytterhoeven m68k.org>; Simon Horman ; linux-mmc m...@vger.kernel.org>; Linux-Renesas ;
> Lee Jones
> Subject: Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
>
> + Lee
>
> On 12 September 2016 at 16:15, Chris Brandt
> wrote:
> > For the r7s72100 SOC, the DATA_PORT register was changed to 32-bits wide.
> > Therefore a new flag has been created that will allow 32-bit
> > reads/writes to the DATA_PORT register instead of 16-bit (because
> > 16-bits accesses are not supported).
> >
> > Signed-off-by: Chris Brandt
> > ---
> > v3:
> > * changed loops to memcpy
> > v2:
> > * changed 'data * 0xFF' to 'data & 0xFF'
> > * added 'const' for sd_ctrl_write32_rep
> > ---
> > drivers/mmc/host/tmio_mmc.h | 12
> > drivers/mmc/host/tmio_mmc_pio.c | 30 ++
> > include/linux/mfd/tmio.h| 5 +
>
> This header file needs to be split up. The mmc specific bits, should be
> moved to a local header file under driver/mmc/host/*.
>
> Sure, we don't need to do that as part of $subject patch, but then I need
> an ack from Lee Jones, the mfd maintainer. I have added him on cc.
Is this still waiting on an ACK from Lee Jones because it touches the tmio.h
file under include/linux/mfd/ ?
> > 3 files changed, 47 insertions(+)
> >
> > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> > index ecb99fc..a99e634 100644
> > --- a/drivers/mmc/host/tmio_mmc.h
> > +++ b/drivers/mmc/host/tmio_mmc.h
> > @@ -237,6 +237,12 @@ static inline u32
> sd_ctrl_read16_and_16_as_32(struct tmio_mmc_host *host, int ad
> >readw(host->ctl + ((addr + 2) << host->bus_shift)) <<
> > 16; }
> >
> > +static inline void sd_ctrl_read32_rep(struct tmio_mmc_host *host, int
> addr,
> > + u32 *buf, int count)
> > +{
> > + readsl(host->ctl + (addr << host->bus_shift), buf, count); }
> > +
> > static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int
> > addr, u16 val) {
> > /* If there is a hook and it returns non-zero then there @@
> > -259,4 +265,10 @@ static inline void sd_ctrl_write32_as_16_and_16(struct
> tmio_mmc_host *host, int
> > writew(val >> 16, host->ctl + ((addr + 2) <<
> > host->bus_shift)); }
> >
> > +static inline void sd_ctrl_write32_rep(struct tmio_mmc_host *host, int
> addr,
> > + const u32 *buf, int count) {
> > + writesl(host->ctl + (addr << host->bus_shift), buf, count); }
> > +
> > #endif
> > diff --git a/drivers/mmc/host/tmio_mmc_pio.c
> > b/drivers/mmc/host/tmio_mmc_pio.c index 017a4dc..59fac7d 100644
> > --- a/drivers/mmc/host/tmio_mmc_pio.c
> > +++ b/drivers/mmc/host/tmio_mmc_pio.c
> > @@ -443,6 +443,36 @@ static void tmio_mmc_transfer_data(struct
> tmio_mmc_host *host,
> > /*
> > * Transfer the data
> > */
> > + if (host->pdata->flags & TMIO_MMC_32BIT_DATA_PORT) {
> > + u8 data[4] = { };
> > +
> > + if (is_read)
> > + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, (u32
> *)buf,
> > + count >> 2);
> > + else
> > + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, (u32
> *)buf,
> > + count >> 2);
> > +
> > + /* if count was multiple of 4 */
> > + if (!(count & 0x3))
> > + return;
> > +
> > + buf8 = (u8 *)(buf + (count >> 2));
> > + count %= 4;
> > +
> > + if (is_read) {
> > + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT,
> > + (u32 *)data, 1);
> > + memcpy(buf8, data, count);
> > + } else {
> > + memcpy(data, buf8, count);
> > + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT,
> > + (u32 *)data, 1);
> > + }
> > +
> > + return;
> > + }
> > +
> > if (is_read)
> > sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >>
> 1);
> > else
> > diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h index
> > 3b95dc7..0dbcb7e 100644
> > --- a/include/linux/mfd/tmio.h
> > +++ b/include/linux/mfd/tmio.h
> > @@ -100,6 +100,11 @@
> > #define TMIO_MMC_SDIO_STATUS_QUIRK (1 << 8)
> >
> > /*
> > + * Some controllers have a 32-bit wide data port register */
> > +#define TMIO_MMC_32BIT_DATA_PORT (1 << 9)
> > +
> > +/*
> > * Some controllers allows to set SDx actual clock
> > */
> > #define TMIO_MMC_CLK_ACTUAL(1 << 10)
> > --
> > 2.9.2
> >
> >
>
> Kind regards
> Uffe
Re: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
+ Lee
On 12 September 2016 at 16:15, Chris Brandt wrote:
> For the r7s72100 SOC, the DATA_PORT register was changed to 32-bits wide.
> Therefore a new flag has been created that will allow 32-bit reads/writes
> to the DATA_PORT register instead of 16-bit (because 16-bits accesses are
> not supported).
>
> Signed-off-by: Chris Brandt
> ---
> v3:
> * changed loops to memcpy
> v2:
> * changed 'data * 0xFF' to 'data & 0xFF'
> * added 'const' for sd_ctrl_write32_rep
> ---
> drivers/mmc/host/tmio_mmc.h | 12
> drivers/mmc/host/tmio_mmc_pio.c | 30 ++
> include/linux/mfd/tmio.h| 5 +
This header file needs to be split up. The mmc specific bits, should
be moved to a local header file under driver/mmc/host/*.
Sure, we don't need to do that as part of $subject patch, but then I
need an ack from Lee Jones, the mfd maintainer. I have added him on
cc.
> 3 files changed, 47 insertions(+)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index ecb99fc..a99e634 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -237,6 +237,12 @@ static inline u32 sd_ctrl_read16_and_16_as_32(struct
> tmio_mmc_host *host, int ad
>readw(host->ctl + ((addr + 2) << host->bus_shift)) << 16;
> }
>
> +static inline void sd_ctrl_read32_rep(struct tmio_mmc_host *host, int addr,
> + u32 *buf, int count)
> +{
> + readsl(host->ctl + (addr << host->bus_shift), buf, count);
> +}
> +
> static inline void sd_ctrl_write16(struct tmio_mmc_host *host, int addr, u16
> val)
> {
> /* If there is a hook and it returns non-zero then there
> @@ -259,4 +265,10 @@ static inline void sd_ctrl_write32_as_16_and_16(struct
> tmio_mmc_host *host, int
> writew(val >> 16, host->ctl + ((addr + 2) << host->bus_shift));
> }
>
> +static inline void sd_ctrl_write32_rep(struct tmio_mmc_host *host, int addr,
> + const u32 *buf, int count)
> +{
> + writesl(host->ctl + (addr << host->bus_shift), buf, count);
> +}
> +
> #endif
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 017a4dc..59fac7d 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -443,6 +443,36 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host
> *host,
> /*
> * Transfer the data
> */
> + if (host->pdata->flags & TMIO_MMC_32BIT_DATA_PORT) {
> + u8 data[4] = { };
> +
> + if (is_read)
> + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, (u32 *)buf,
> + count >> 2);
> + else
> + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, (u32
> *)buf,
> + count >> 2);
> +
> + /* if count was multiple of 4 */
> + if (!(count & 0x3))
> + return;
> +
> + buf8 = (u8 *)(buf + (count >> 2));
> + count %= 4;
> +
> + if (is_read) {
> + sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT,
> + (u32 *)data, 1);
> + memcpy(buf8, data, count);
> + } else {
> + memcpy(data, buf8, count);
> + sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT,
> + (u32 *)data, 1);
> + }
> +
> + return;
> + }
> +
> if (is_read)
> sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1);
> else
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index 3b95dc7..0dbcb7e 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -100,6 +100,11 @@
> #define TMIO_MMC_SDIO_STATUS_QUIRK (1 << 8)
>
> /*
> + * Some controllers have a 32-bit wide data port register
> + */
> +#define TMIO_MMC_32BIT_DATA_PORT (1 << 9)
> +
> +/*
> * Some controllers allows to set SDx actual clock
> */
> #define TMIO_MMC_CLK_ACTUAL(1 << 10)
> --
> 2.9.2
>
>
Kind regards
Uffe
