[PATCH v4 0/10] mmc: meson-gx: series with smaller improvements

[PATCH v4 0/10] mmc: meson-gx: series with smaller improvements

[Heiner Kallweit]

Am 04.03.2017 um 10:30 schrieb Helmut Klein:
> Hallo Heiner,
> 
> i had the same problem as mentioned from Kevin.
> 
> The solution for my box (netxeon MiniMX) was to partly revert your patch 8
> 
> --- meson-gx-mmc.c.heiner    2017-02-20 13:25:47.000000000 +0100
> +++ meson-gx-mmc.c.helmut    2017-02-21 11:30:30.360445366 +0100
> @@ -741,6 +742,10 @@ static int meson_mmc_probe(struct platfo
>      if (ret)
>          goto free_host;
> 
> +    ret = meson_mmc_clk_init(host);
> +    if (ret)
> +        goto err_core_clk;
> +
>      /* Stop execution */
>      writel(0, host->regs + SD_EMMC_START);
> 
> @@ -751,11 +756,7 @@ static int meson_mmc_probe(struct platfo
> 
>      /* set config to sane default */
>      meson_mmc_cfg_init(host);
> -
> -    ret = meson_mmc_clk_init(host);
> -    if (ret)
> -        goto free_host;
> -
> +
>      ret = devm_request_threaded_irq(&pdev->dev, irq, meson_mmc_irq,
>                      meson_mmc_irq_thread, IRQF_SHARED,
>                      DRIVER_NAME, host);
> 
> The rest of your patches are ok. Thank you for them.
> 
> Before applying the patches, i couldn't read the mmc (hs200 type) at all (got crc errors only). Now this is fixed.
> 
> regards
> Helmut
> 
> ps: i hope, it is ok, to send this mail directly to you??
> 
Sure. I just think other people would consider your findings interesting too, so I cc Kevin and the lists.
Let me check on my system, I'll come up with a corrected version of the patch set.

Rgds, Heiner

[PATCH v4 0/10] mmc: meson-gx: series with smaller improvements

[Heiner Kallweit]

New series is limited to smaller refactorings w/o functional changes.

Heiner Kallweit (10):
  mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
  mmc: meson-gx: make two functions return void
  mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
  mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
  mmc: meson-gx: remove member parent_mux from struct meson_host
  mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
  mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
  mmc: meson-gx: improve initial configuration
  mmc: meson-gx: remove member mrq from struct meson_host
  mmc: meson-gx: replace magic timeout numbers with constants

 drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 76 deletions(-)

-- 
2.11.1

[PATCH v4 0/10] mmc: meson-gx: series with smaller improvements

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> New series is limited to smaller refactorings w/o functional changes.

I'd reviewed this series before, but hadn't actually tested it until
today.  I applied this series onto today's linux-next, and tested on
meson-gxbb-odroidc2 and the kernel hangs up right after:

       meson-gx-mmc d0072000.mmc: Got CD GPIO

with no error message or oops/backtrace etc.

Could you clarify how you are testing this, on what tree/branch, on what
hardware etc.?

Also, in the cover letter, it's customary to include a summary of what
changed since the previous version(s)

Kevin

> Heiner Kallweit (10):
>   mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
>   mmc: meson-gx: make two functions return void
>   mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
>   mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
>   mmc: meson-gx: remove member parent_mux from struct meson_host
>   mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
>   mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
>   mmc: meson-gx: improve initial configuration
>   mmc: meson-gx: remove member mrq from struct meson_host
>   mmc: meson-gx: replace magic timeout numbers with constants
>
>  drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
>  1 file changed, 50 insertions(+), 76 deletions(-)

[PATCH v4 0/10] mmc: meson-gx: series with smaller improvements

[Heiner Kallweit]

Am 03.03.2017 um 22:19 schrieb Kevin Hilman:
> Heiner Kallweit <hkallweit1@gmail.com> writes:
> 
>> New series is limited to smaller refactorings w/o functional changes.
> 
> I'd reviewed this series before, but hadn't actually tested it until
> today.  I applied this series onto today's linux-next, and tested on
> meson-gxbb-odroidc2 and the kernel hangs up right after:
> 
>        meson-gx-mmc d0072000.mmc: Got CD GPIO
> 
> with no error message or oops/backtrace etc.
> 
> Could you clarify how you are testing this, on what tree/branch, on what
> hardware etc.?
> 
I'm testing on Odroid C2 with a self-built uboot based on the latest
mainline uboot incl. an own eMMC driver which was submitted but is
not yet applied to mainline uboot.
The system is running headless with a serial console attached.
Storage is a 128 GB Hardkernel eMMC card.

I use latest next kernel + the patches to test.

Does your system work w/o the current patch set?
And do you use HS200 or HS400 mode?

I also figured out that 200MHz w/o tuning is a little fragile and
reduced the clock to 60 MHz. This makes no difference in performance
as the driver currently is very slow anyway (only 10 - 15 MB/s).
When the clock is too high I see lots of CRC errors on the serial
console.

This will change with further patches I have in my tree.
They allow stable HS200/HS400 with quite basic tuning resulting in
140 MB/s read performance.
Stable configuration here is: 180? core clock phase, 0? tx clock
phase, 180? rx clock phase.

> Also, in the cover letter, it's customary to include a summary of what
> changed since the previous version(s)
> 
> Kevin
> 
>> Heiner Kallweit (10):
>>   mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
>>   mmc: meson-gx: make two functions return void
>>   mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
>>   mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
>>   mmc: meson-gx: remove member parent_mux from struct meson_host
>>   mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
>>   mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
>>   mmc: meson-gx: improve initial configuration
>>   mmc: meson-gx: remove member mrq from struct meson_host
>>   mmc: meson-gx: replace magic timeout numbers with constants
>>
>>  drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
>>  1 file changed, 50 insertions(+), 76 deletions(-)
> 

[PATCH v4 0/10] mmc: meson-gx: series with smaller improvements

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> Am 03.03.2017 um 22:19 schrieb Kevin Hilman:
>> Heiner Kallweit <hkallweit1@gmail.com> writes:
>> 
>>> New series is limited to smaller refactorings w/o functional changes.
>> 
>> I'd reviewed this series before, but hadn't actually tested it until
>> today.  I applied this series onto today's linux-next, and tested on
>> meson-gxbb-odroidc2 and the kernel hangs up right after:
>> 
>>        meson-gx-mmc d0072000.mmc: Got CD GPIO
>> 
>> with no error message or oops/backtrace etc.
>> 
>> Could you clarify how you are testing this, on what tree/branch, on what
>> hardware etc.?
>> 
> I'm testing on Odroid C2 with a self-built uboot based on the latest
> mainline uboot incl. an own eMMC driver which was submitted but is
> not yet applied to mainline uboot.
> The system is running headless with a serial console attached.
> Storage is a 128 GB Hardkernel eMMC card.
>
> I use latest next kernel + the patches to test.
>
> Does your system work w/o the current patch set?

Yes.

> And do you use HS200 or HS400 mode?

I don't remember what kind of card is plugged in, and I'm away from the
board currently.

Since you have a custom uboot, with a custom MMC driver, I suspect that
your uboot is initializing something that the kernel is not, so when a
kernel is used that's not using your uboot, something goes wrong.

Any chance you can try with the vendor uboot from Hardkernel?

I tested on odroid-c2 because someone on the #linux-amlogic IRC channel
(webczat) reported the hang when testing your patches, so I tried to
reproduce and got the same hang.

Until we figure out what the hang is, this series should not be merged.

Kevin

> I also figured out that 200MHz w/o tuning is a little fragile and
> reduced the clock to 60 MHz. This makes no difference in performance
> as the driver currently is very slow anyway (only 10 - 15 MB/s).
> When the clock is too high I see lots of CRC errors on the serial
> console.
>
> This will change with further patches I have in my tree.
> They allow stable HS200/HS400 with quite basic tuning resulting in
> 140 MB/s read performance.
> Stable configuration here is: 180? core clock phase, 0? tx clock
> phase, 180? rx clock phase.
>
>> Also, in the cover letter, it's customary to include a summary of what
>> changed since the previous version(s)
>> 
>> Kevin
>> 
>>> Heiner Kallweit (10):
>>>   mmc: meson-gx: simplify bounce buffer setting in meson_mmc_start_cmd
>>>   mmc: meson-gx: make two functions return void
>>>   mmc: meson-gx: remove unused members irq, ocr_mask from struct meson_host
>>>   mmc: meson-gx: remove unneeded variable in meson_mmc_clk_init
>>>   mmc: meson-gx: remove member parent_mux from struct meson_host
>>>   mmc: meson-gx: remove unneeded meson_mmc_clk_set in meson_mmc_clk_init
>>>   mmc: meson-gx: remove unneeded devm_kstrdup in meson_mmc_clk_init
>>>   mmc: meson-gx: improve initial configuration
>>>   mmc: meson-gx: remove member mrq from struct meson_host
>>>   mmc: meson-gx: replace magic timeout numbers with constants
>>>
>>>  drivers/mmc/host/meson-gx-mmc.c | 126 ++++++++++++++++------------------------
>>>  1 file changed, 50 insertions(+), 76 deletions(-)
>> 

[PATCH v4 0/10] mmc: meson-gx: series with smaller improvements

[Kevin Hilman]

On Fri, Mar 3, 2017 at 9:33 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> Heiner Kallweit <hkallweit1@gmail.com> writes:
>
>> Am 03.03.2017 um 22:19 schrieb Kevin Hilman:
>>> Heiner Kallweit <hkallweit1@gmail.com> writes:
>>>
>>>> New series is limited to smaller refactorings w/o functional changes.
>>>
>>> I'd reviewed this series before, but hadn't actually tested it until
>>> today.  I applied this series onto today's linux-next, and tested on
>>> meson-gxbb-odroidc2 and the kernel hangs up right after:
>>>
>>>        meson-gx-mmc d0072000.mmc: Got CD GPIO
>>>
>>> with no error message or oops/backtrace etc.
>>>
>>> Could you clarify how you are testing this, on what tree/branch, on what
>>> hardware etc.?
>>>
>> I'm testing on Odroid C2 with a self-built uboot based on the latest
>> mainline uboot incl. an own eMMC driver which was submitted but is
>> not yet applied to mainline uboot.
>> The system is running headless with a serial console attached.
>> Storage is a 128 GB Hardkernel eMMC card.
>>
>> I use latest next kernel + the patches to test.
>>
>> Does your system work w/o the current patch set?
>
> Yes.
>
>> And do you use HS200 or HS400 mode?
>
> I don't remember what kind of card is plugged in, and I'm away from the
> board currently.

>From a boot of linux-next without this series, look like it's detecting HS200:

root at buildroot:/# dmesg |grep -i mmc
[    1.359545] meson-gx-mmc d0074000.mmc: allocated mmc-pwrseq
[    1.632159] meson-gx-mmc d0072000.mmc: Got CD GPIO
[    1.778734] mmc1: new high speed SDHC card at address 1234
[    1.779007] mmcblk1: mmc1:1234 SA08G 7.21 GiB
[    1.784197]  mmcblk1: p1
[    1.925634] mmc0: new HS200 MMC card at address 0001
[    1.925875] mmcblk0: mmc0:0001 SDW16G 14.7 GiB
[    1.929550] mmcblk0boot0: mmc0:0001 SDW16G partition 1 4.00 MiB
[    1.935404] mmcblk0boot1: mmc0:0001 SDW16G partition 2 4.00 MiB
[    1.942428]  mmcblk0: p1 p2

Kevin