[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()
Pass the entire source data pointer to tmio_sd_addr_is_dmaable() so we don't have to apply casts throughout the code.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Masahiro Yamada yamada.masahiro@socionext.com --- drivers/mmc/tmio-common.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c index b311b80be8..6b21941991 100644 --- a/drivers/mmc/tmio-common.c +++ b/drivers/mmc/tmio-common.c @@ -372,8 +372,10 @@ static int tmio_sd_dma_xfer(struct udevice *dev, struct mmc_data *data) }
/* check if the address is DMA'able */ -static bool tmio_sd_addr_is_dmaable(unsigned long addr) +static bool tmio_sd_addr_is_dmaable(const char *src) { + uintptr_t addr = (uintptr_t)src; + if (!IS_ALIGNED(addr, TMIO_SD_DMA_MINALIGN)) return false;
@@ -486,7 +488,7 @@ int tmio_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, if (data) { /* use DMA if the HW supports it and the buffer is aligned */ if (priv->caps & TMIO_SD_CAP_DMA_INTERNAL && - tmio_sd_addr_is_dmaable((long)data->src)) + tmio_sd_addr_is_dmaable(data->src)) ret = tmio_sd_dma_xfer(dev, data); else ret = tmio_sd_pio_xfer(dev, data);
The internal DMAC on Gen3 is 32bit only, limit the DMA address range to 32bit.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Masahiro Yamada yamada.masahiro@socionext.com --- drivers/mmc/tmio-common.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c index 6b21941991..138de59470 100644 --- a/drivers/mmc/tmio-common.c +++ b/drivers/mmc/tmio-common.c @@ -379,6 +379,12 @@ static bool tmio_sd_addr_is_dmaable(const char *src) if (!IS_ALIGNED(addr, TMIO_SD_DMA_MINALIGN)) return false;
+#if defined(CONFIG_RCAR_GEN3) + /* Gen3 DMA has 32bit limit */ + if (addr >> 32) + return false; +#endif + #if defined(CONFIG_ARCH_UNIPHIER) && !defined(CONFIG_ARM64) && \ defined(CONFIG_SPL_BUILD) /*
Hi Marek,
On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut marek.vasut@gmail.com wrote:
Pass the entire source data pointer to tmio_sd_addr_is_dmaable()
This statement sounds like the current code is passing the pointer address only partially. Is it right?
so we don't have to apply casts throughout the code.
I do not understand this either since I see a cast in your code too.
In the previous code, the caller casts src->address when it passes it to tmio_sd_addr_is_dmaable().
In the new code, 'src' is casted in tmio_sd_addr_is_dmaable().
To me, you just moved the location of casting. What is the difference (i.e. benefit)?
If you want to change this code, I am fine. But, I'd like to know the reason.
At least, I am so confused with your commit description.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mmc/tmio-common.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c index b311b80be8..6b21941991 100644 --- a/drivers/mmc/tmio-common.c +++ b/drivers/mmc/tmio-common.c @@ -372,8 +372,10 @@ static int tmio_sd_dma_xfer(struct udevice *dev, struct mmc_data *data) }
/* check if the address is DMA'able */ -static bool tmio_sd_addr_is_dmaable(unsigned long addr) +static bool tmio_sd_addr_is_dmaable(const char *src) {
uintptr_t addr = (uintptr_t)src;if (!IS_ALIGNED(addr, TMIO_SD_DMA_MINALIGN)) return false;@@ -486,7 +488,7 @@ int tmio_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, if (data) { /* use DMA if the HW supports it and the buffer is aligned */ if (priv->caps & TMIO_SD_CAP_DMA_INTERNAL &&
tmio_sd_addr_is_dmaable((long)data->src))
tmio_sd_addr_is_dmaable(data->src)) ret = tmio_sd_dma_xfer(dev, data); else ret = tmio_sd_pio_xfer(dev, data);-- 2.18.0
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 10/09/2018 02:24 PM, Masahiro Yamada wrote:
Hi Marek,
Hi,
On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut marek.vasut@gmail.com wrote:
Pass the entire source data pointer to tmio_sd_addr_is_dmaable()
This statement sounds like the current code is passing the pointer address only partially. Is it right?
With this change it is.
so we don't have to apply casts throughout the code.
I do not understand this either since I see a cast in your code too.
There is a cast, but it's isolated to this function.
In the previous code, the caller casts src->address when it passes it to tmio_sd_addr_is_dmaable().
In the new code, 'src' is casted in tmio_sd_addr_is_dmaable().
To me, you just moved the location of casting. What is the difference (i.e. benefit)?
I moved the cast from the code into the function, which I think is cleaner.
If you want to change this code, I am fine. But, I'd like to know the reason.
At least, I am so confused with your commit description.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mmc/tmio-common.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c index b311b80be8..6b21941991 100644 --- a/drivers/mmc/tmio-common.c +++ b/drivers/mmc/tmio-common.c @@ -372,8 +372,10 @@ static int tmio_sd_dma_xfer(struct udevice *dev, struct mmc_data *data) }
/* check if the address is DMA'able */ -static bool tmio_sd_addr_is_dmaable(unsigned long addr) +static bool tmio_sd_addr_is_dmaable(const char *src) {
uintptr_t addr = (uintptr_t)src;if (!IS_ALIGNED(addr, TMIO_SD_DMA_MINALIGN)) return false;@@ -486,7 +488,7 @@ int tmio_sd_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, if (data) { /* use DMA if the HW supports it and the buffer is aligned */ if (priv->caps & TMIO_SD_CAP_DMA_INTERNAL &&
tmio_sd_addr_is_dmaable((long)data->src))
tmio_sd_addr_is_dmaable(data->src)) ret = tmio_sd_dma_xfer(dev, data); else ret = tmio_sd_pio_xfer(dev, data);-- 2.18.0
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Tue, Oct 9, 2018 at 11:55 PM Marek Vasut marek.vasut@gmail.com wrote:
On 10/09/2018 02:24 PM, Masahiro Yamada wrote:
Hi Marek,
Hi,
On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut marek.vasut@gmail.com wrote:
Pass the entire source data pointer to tmio_sd_addr_is_dmaable()
This statement sounds like the current code is passing the pointer address only partially. Is it right?
With this change it is.
Is anything wrong with my code?
How about your patch title "mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()" ?
Does it mean my code is not passing full address?
so we don't have to apply casts throughout the code.
I do not understand this either since I see a cast in your code too.
There is a cast, but it's isolated to this function.
In the previous code, the caller casts src->address when it passes it to tmio_sd_addr_is_dmaable().
In the new code, 'src' is casted in tmio_sd_addr_is_dmaable().
To me, you just moved the location of casting. What is the difference (i.e. benefit)?
I moved the cast from the code into the function, which I think is cleaner.
I do not think so.
If you like this patch, just go for it.
But, I believe you need to update the patch title and description since this is just a matter of personal preference.
On 10/09/2018 05:35 PM, Masahiro Yamada wrote:
On Tue, Oct 9, 2018 at 11:55 PM Marek Vasut marek.vasut@gmail.com wrote:
On 10/09/2018 02:24 PM, Masahiro Yamada wrote:
Hi Marek,
Hi,
On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut marek.vasut@gmail.com wrote:
Pass the entire source data pointer to tmio_sd_addr_is_dmaable()
This statement sounds like the current code is passing the pointer address only partially. Is it right?
With this change it is.
Is anything wrong with my code?
Don't think so.
How about your patch title "mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()" ?
Does it mean my code is not passing full address?
Could use a rephrasing, yeah
so we don't have to apply casts throughout the code.
I do not understand this either since I see a cast in your code too.
There is a cast, but it's isolated to this function.
In the previous code, the caller casts src->address when it passes it to tmio_sd_addr_is_dmaable().
In the new code, 'src' is casted in tmio_sd_addr_is_dmaable().
To me, you just moved the location of casting. What is the difference (i.e. benefit)?
I moved the cast from the code into the function, which I think is cleaner.
I do not think so.
So would you prefer to see stuff like
function foo(long bar) {...}
foo((cast)baz);
...
foo((cast)quux);
In the code :)
If you like this patch, just go for it.
But, I believe you need to update the patch title and description since this is just a matter of personal preference.
On Wed, Oct 10, 2018 at 1:17 AM Marek Vasut marek.vasut@gmail.com wrote:
On 10/09/2018 05:35 PM, Masahiro Yamada wrote:
On Tue, Oct 9, 2018 at 11:55 PM Marek Vasut marek.vasut@gmail.com wrote:
On 10/09/2018 02:24 PM, Masahiro Yamada wrote:
Hi Marek,
Hi,
On Tue, Oct 9, 2018 at 8:26 PM Marek Vasut marek.vasut@gmail.com wrote:
Pass the entire source data pointer to tmio_sd_addr_is_dmaable()
This statement sounds like the current code is passing the pointer address only partially. Is it right?
With this change it is.
Is anything wrong with my code?
Don't think so.
How about your patch title "mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()" ?
Does it mean my code is not passing full address?
Could use a rephrasing, yeah
so we don't have to apply casts throughout the code.
I do not understand this either since I see a cast in your code too.
There is a cast, but it's isolated to this function.
In the previous code, the caller casts src->address when it passes it to tmio_sd_addr_is_dmaable().
In the new code, 'src' is casted in tmio_sd_addr_is_dmaable().
To me, you just moved the location of casting. What is the difference (i.e. benefit)?
I moved the cast from the code into the function, which I think is cleaner.
I do not think so.
So would you prefer to see stuff like
function foo(long bar) {...}
foo((cast)baz);
...
foo((cast)quux);
In the code :)
It is a hypothetical situation.
If there were multiple function calls, I would agree with you.
participants (2)
-
Marek Vasut -
Masahiro Yamada