Message ID | 1431693634.13197.29.camel@transmode.se (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> How about this one: > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001 > From: Joakim Tjernlund <joakim.tjernlund@transmode.se> > Date: Mon, 20 Apr 2015 22:36:55 +0200 > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width. > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT. Do we really need this for the 8bit bus support? There is already a specific API for setting the bus width, this change seems unnecessary to me. That is also why I choose to revert that patch. Did I miss something? Thanks, Kevin
On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote: > > > > How about this one: > > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001 > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se> > > Date: Mon, 20 Apr 2015 22:36:55 +0200 > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width. > > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT. > > Do we really need this for the 8bit bus support? There is already a specific > API for setting the bus width, this change seems unnecessary to me. That is > also why I choose to revert that patch. Did I miss something? We do, the bus API really only works well when the bus bits are in another register but the HOST_CONTROL register. The only reason 4BIT works is because its bit placement is where SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate for that they will overwrite what the bus API set earlier. Atleast this is my understanding, Ulf? Didn't this patch work for you either? Jocke -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17 May 2015 at 10:36, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote: >> > >> > How about this one: >> > >> > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001 >> > From: Joakim Tjernlund <joakim.tjernlund@transmode.se> >> > Date: Mon, 20 Apr 2015 22:36:55 +0200 >> > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width. >> > >> > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT. >> >> Do we really need this for the 8bit bus support? There is already a specific >> API for setting the bus width, this change seems unnecessary to me. That is >> also why I choose to revert that patch. Did I miss something? > > We do, the bus API really only works well when the bus bits are in another > register but the HOST_CONTROL register. > The only reason 4BIT works is because its bit placement is where > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate > for that they will overwrite what the bus API set earlier. > > Atleast this is my understanding, Ulf? Sorry, I don't have enough knowledge about sdhci to give you good answer. > > Didn't this patch work for you either? I have dropped the "sdhci-of-edsdhc: Support 8-bit..." from my next branch for now. A "Tested-by" confirmation of a new version of $subject patch, would help me to know when it can go in. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote: > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote: > > > > > > How about this one: > > > > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001 > > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se> > > > Date: Mon, 20 Apr 2015 22:36:55 +0200 > > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width. > > > > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT. > > > > Do we really need this for the 8bit bus support? There is already a specific > > API for setting the bus width, this change seems unnecessary to me. That is > > also why I choose to revert that patch. Did I miss something? > > We do, the bus API really only works well when the bus bits are in another > register but the HOST_CONTROL register. Sorry, I didn't get what you mean. Could you elaborate a bit more? > The only reason 4BIT works is because its bit placement is where > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate > for that they will overwrite what the bus API set earlier. But the implementation of esdhc_pltfm_set_bus_width() already take care about this. And in the current kernel, we only set the bus width via this API. So why do you think that it can be overwrote? static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width) { u32 ctrl; switch (width) { case MMC_BUS_WIDTH_8: ctrl = ESDHC_CTRL_8BITBUS; break; case MMC_BUS_WIDTH_4: ctrl = ESDHC_CTRL_4BITBUS; break; default: ctrl = 0; break; } clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL, ESDHC_CTRL_BUSWIDTH_MASK, ctrl); } > > Atleast this is my understanding, Ulf? > > Didn't this patch work for you either? I don't have a 8bit card at hand. For 4bit card, it works with or without this change. Thanks, Kevin > > Jocke
On Tue, 2015-05-19 at 17:20 +0800, Kevin Hao wrote: > On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote: > > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote: > > > > > > > > How about this one: > > > > > > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001 > > > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se> > > > > Date: Mon, 20 Apr 2015 22:36:55 +0200 > > > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width. > > > > > > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT. > > > > > > Do we really need this for the 8bit bus support? There is already a specific > > > API for setting the bus width, this change seems unnecessary to me. That is > > > also why I choose to revert that patch. Did I miss something? > > > > We do, the bus API really only works well when the bus bits are in another > > register but the HOST_CONTROL register. > > Sorry, I didn't get what you mean. Could you elaborate a bit more? clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL, ESDHC_CTRL_BUSWIDTH_MASK, ctrl); and esdhc_writeb(struct sdhci_host *host, int reg) with reg == SDHCI_HOST_CONTROL is the same register so esdhc_writeb() will stomp on the settings esdhc_pltfm_set_bus_width() did earlier. > > > The only reason 4BIT works is because its bit placement is where > > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate > > for that they will overwrite what the bus API set earlier. > > But the implementation of esdhc_pltfm_set_bus_width() already take care about > this. And in the current kernel, we only set the bus width via this API. So why > do you think that it can be overwrote? > > static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width) > { > u32 ctrl; > > switch (width) { > case MMC_BUS_WIDTH_8: > ctrl = ESDHC_CTRL_8BITBUS; > break; > > case MMC_BUS_WIDTH_4: > ctrl = ESDHC_CTRL_4BITBUS; > break; > > default: > ctrl = 0; > break; > } > > clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL, > ESDHC_CTRL_BUSWIDTH_MASK, ctrl); > } > > > > > Atleast this is my understanding, Ulf? > > > > Didn't this patch work for you either? > > I don't have a 8bit card at hand. For 4bit card, it works with or without this > change. Sure it does, that was not the question though: Does it still work with this version of the patch? Jocke-- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 20, 2015 at 02:54:27PM +0000, Joakim Tjernlund wrote: > On Tue, 2015-05-19 at 17:20 +0800, Kevin Hao wrote: > > On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote: > > > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote: > > > > > > > > > > How about this one: > > > > > > > > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001 > > > > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se> > > > > > Date: Mon, 20 Apr 2015 22:36:55 +0200 > > > > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width. > > > > > > > > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT. > > > > > > > > Do we really need this for the 8bit bus support? There is already a specific > > > > API for setting the bus width, this change seems unnecessary to me. That is > > > > also why I choose to revert that patch. Did I miss something? > > > > > > We do, the bus API really only works well when the bus bits are in another > > > register but the HOST_CONTROL register. > > > > Sorry, I didn't get what you mean. Could you elaborate a bit more? > clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL, > ESDHC_CTRL_BUSWIDTH_MASK, ctrl); > and > esdhc_writeb(struct sdhci_host *host, int reg) with reg == SDHCI_HOST_CONTROL > is the same register so esdhc_writeb() will stomp on the settings esdhc_pltfm_set_bus_width() > did earlier. The .set_bus_width() is the only API we use to set the bus width. We never do something like this: esdhc_writeb(host, SDHCI_CTRL_8BITBUS, SDHCI_HOST_CONTROL); So the scenario you mentioned above should not happen. That is also why I think your changes is not necessary. > > > > > > The only reason 4BIT works is because its bit placement is where > > > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate > > > for that they will overwrite what the bus API set earlier. > > > > But the implementation of esdhc_pltfm_set_bus_width() already take care about > > this. And in the current kernel, we only set the bus width via this API. So why > > do you think that it can be overwrote? > > > > static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width) > > { > > u32 ctrl; > > > > switch (width) { > > case MMC_BUS_WIDTH_8: > > ctrl = ESDHC_CTRL_8BITBUS; > > break; > > > > case MMC_BUS_WIDTH_4: > > ctrl = ESDHC_CTRL_4BITBUS; > > break; > > > > default: > > ctrl = 0; > > break; > > } > > > > clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL, > > ESDHC_CTRL_BUSWIDTH_MASK, ctrl); > > } > > > > > > > > Atleast this is my understanding, Ulf? > > > > > > Didn't this patch work for you either? > > > > I don't have a 8bit card at hand. For 4bit card, it works with or without this > > change. > > Sure it does, that was not the question though: > Does it still work with this version of the patch? I think I already answered your question. For 4bit card, it works with or without this version of patch. Thanks, Kevin > > Jocke
On Thu, 2015-05-21 at 09:07 +0800, Kevin Hao wrote: > On Wed, May 20, 2015 at 02:54:27PM +0000, Joakim Tjernlund wrote: > > On Tue, 2015-05-19 at 17:20 +0800, Kevin Hao wrote: > > > On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote: > > > > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote: > > > > > > > > > > > > How about this one: > > > > > > > > > > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001 > > > > > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se> > > > > > > Date: Mon, 20 Apr 2015 22:36:55 +0200 > > > > > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width. > > > > > > > > > > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT. > > > > > > > > > > Do we really need this for the 8bit bus support? There is already a specific > > > > > API for setting the bus width, this change seems unnecessary to me. That is > > > > > also why I choose to revert that patch. Did I miss something? > > > > > > > > We do, the bus API really only works well when the bus bits are in another > > > > register but the HOST_CONTROL register. > > > > > > Sorry, I didn't get what you mean. Could you elaborate a bit more? > > clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL, > > ESDHC_CTRL_BUSWIDTH_MASK, ctrl); > > and > > esdhc_writeb(struct sdhci_host *host, int reg) with reg == SDHCI_HOST_CONTROL > > is the same register so esdhc_writeb() will stomp on the settings esdhc_pltfm_set_bus_width() > > did earlier. > > The .set_bus_width() is the only API we use to set the bus width. We never do > something like this: > esdhc_writeb(host, SDHCI_CTRL_8BITBUS, SDHCI_HOST_CONTROL); ATM, you might at some point. > > So the scenario you mentioned above should not happen. That is also why I think > your changes is not necessary. The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit. However, now I see that esdhc_writeb() has /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */ if (reg == SDHCI_HOST_CONTROL) val &= ~ESDHC_HOST_CONTROL_RES; and #define ESDHC_HOST_CONTROL_RES 0x05 so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT. What a mess. > > > > > > > > > > The only reason 4BIT works is because its bit placement is where > > > > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate > > > > for that they will overwrite what the bus API set earlier. > > > > > > But the implementation of esdhc_pltfm_set_bus_width() already take care about > > > this. And in the current kernel, we only set the bus width via this API. So why > > > do you think that it can be overwrote? > > > > > > static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width) > > > { > > > u32 ctrl; > > > > > > switch (width) { > > > case MMC_BUS_WIDTH_8: > > > ctrl = ESDHC_CTRL_8BITBUS; > > > break; > > > > > > case MMC_BUS_WIDTH_4: > > > ctrl = ESDHC_CTRL_4BITBUS; > > > break; > > > > > > default: > > > ctrl = 0; > > > break; > > > } > > > > > > clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL, > > > ESDHC_CTRL_BUSWIDTH_MASK, ctrl); > > > } > > > > > > > > > > > Atleast this is my understanding, Ulf? > > > > > > > > Didn't this patch work for you either? > > > > > > I don't have a 8bit card at hand. For 4bit card, it works with or without this > > > change. > > > > Sure it does, that was not the question though: > > Does it still work with this version of the patch? > > I think I already answered your question. For 4bit card, it works with or > without this version of patch. Ahh, I misread. Sorry for the confusion. Jocke -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 21, 2015 at 09:24:43AM +0000, Joakim Tjernlund wrote: > The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit. > > However, now I see that esdhc_writeb() has > /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */ > if (reg == SDHCI_HOST_CONTROL) > val &= ~ESDHC_HOST_CONTROL_RES; > and > #define ESDHC_HOST_CONTROL_RES 0x05 > > so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT. > What a mess. Aha, I see. Thanks for the explanation. Thanks, Kevin
On Thu, 2015-05-21 at 18:56 +0800, Kevin Hao wrote: > On Thu, May 21, 2015 at 09:24:43AM +0000, Joakim Tjernlund wrote: > > The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit. > > > > However, now I see that esdhc_writeb() has > > /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */ > > if (reg == SDHCI_HOST_CONTROL) > > val &= ~ESDHC_HOST_CONTROL_RES; > > and > > #define ESDHC_HOST_CONTROL_RES 0x05 > > > > so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT. > > What a mess. > > Aha, I see. Thanks for the explanation. There is also the common patten of val = esdhc_readb(..) val |= some bit esdhc_writeb(val) This will also stomp on 8BIT I cannot find a Maintainer, is there one? This driver really needs a Freescale maintainer who can navigate the subtile differences between Freescale SOCs Jocke -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21 May 2015 at 13:45, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > On Thu, 2015-05-21 at 18:56 +0800, Kevin Hao wrote: >> On Thu, May 21, 2015 at 09:24:43AM +0000, Joakim Tjernlund wrote: >> > The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit. >> > >> > However, now I see that esdhc_writeb() has >> > /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */ >> > if (reg == SDHCI_HOST_CONTROL) >> > val &= ~ESDHC_HOST_CONTROL_RES; >> > and >> > #define ESDHC_HOST_CONTROL_RES 0x05 >> > >> > so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT. >> > What a mess. >> >> Aha, I see. Thanks for the explanation. > > There is also the common patten of > val = esdhc_readb(..) > val |= some bit > esdhc_writeb(val) > This will also stomp on 8BIT > > I cannot find a Maintainer, is there one? Nope. > This driver really needs a Freescale maintainer who can navigate > the subtile differences between Freescale SOCs I certainly would welcome that! Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c index 1804bdb..233e38d 100644 --- a/drivers/mmc/host/sdhci-of-esdhc.c +++ b/drivers/mmc/host/sdhci-of-esdhc.c @@ -82,6 +82,10 @@ static u8 esdhc_readb(struct sdhci_host *host, int reg) /* fixup the result */ ret &= ~SDHCI_CTRL_DMA_MASK; ret |= dma_bits; + + /* 8BIT is bit 29 in Control register */ + ret |= ((ret << 3) & SDHCI_CTRL_8BITBUS); + ret &= ~(SDHCI_CTRL_8BITBUS >> 3); } return ret; @@ -134,6 +138,10 @@ static void esdhc_writeb(struct sdhci_host *host, u8 val, int reg) dma_bits); val &= ~SDHCI_CTRL_DMA_MASK; val |= in_be32(host->ioaddr + reg) & SDHCI_CTRL_DMA_MASK; + + /* 8BIT is bit 29 in Control register */ + val |= ((val & SDHCI_CTRL_8BITBUS) >> 3); + val = (val & ~SDHCI_CTRL_8BITBUS); } /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */