[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[Marek Vasut]

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);
-- 
2.18.0

[U-Boot] [PATCH 2/2] mmc: tmio: Limit DMA to 32bit on R-Car Gen3

[Marek Vasut]

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)
 	/*
-- 
2.18.0

[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[Masahiro Yamada]

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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[Marek Vasut]

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 at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> 
> 
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[Masahiro Yamada]

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.


-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[Marek Vasut]

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.
> 
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[Masahiro Yamada]

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.




-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 1/2] mmc: tmio: Pass full address to tmio_sd_addr_is_dmaable()

[Marek Vasut]

Pass the entire source data pointer to tmio_sd_addr_is_dmaable()
to avoid losing top 32 bits on 64bit systems.

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] [PATCH 2/2] mmc: tmio: Limit DMA to 32bit on R-Car Gen3

[Marek Vasut]

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)
 	/*
-- 
2.18.0