When booting U-boot/Linux on Aspeed boards via eMMC,
some commands don't behave as expected from the spec.

Add the 'x-aspeed-emmc-kludge' property to allow non
standard uses until we figure out the reasons.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bd77853419..dc692fe1fa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -127,6 +127,7 @@ struct SDState {
 
     uint8_t spec_version;
     BlockBackend *blk;
+    bool aspeed_emmc_kludge;
 
     const SDProto *proto;
 
@@ -2567,6 +2568,8 @@ static Property sd_properties[] = {
     DEFINE_PROP_UINT8("spec_version", SDState,
                       spec_version, SD_PHY_SPECv3_01_VERS),
     DEFINE_PROP_DRIVE("drive", SDState, blk),
+    DEFINE_PROP_BOOL("x-aspeed-emmc-kludge", SDState,
+                     aspeed_emmc_kludge, false),
     /* We do not model the chip select pin, so allow the board to select
      * whether card should be in SSI or MMC/SD mode.  It is also up to the
      * board to ensure that ssi transfers only occur when the chip select
-- 
2.41.0

On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
> When booting U-boot/Linux on Aspeed boards via eMMC,
> some commands don't behave as expected from the spec.
> 
> Add the 'x-aspeed-emmc-kludge' property to allow non
> standard uses until we figure out the reasons.

I am not aware of any singularity in the eMMC logic provided by Aspeed.
U-Boot and Linux drivers seem very generic. May be others can tell.

Thanks,

C.


  
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sd/sd.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index bd77853419..dc692fe1fa 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -127,6 +127,7 @@ struct SDState {
>   
>       uint8_t spec_version;
>       BlockBackend *blk;
> +    bool aspeed_emmc_kludge;
>   
>       const SDProto *proto;
>   
> @@ -2567,6 +2568,8 @@ static Property sd_properties[] = {
>       DEFINE_PROP_UINT8("spec_version", SDState,
>                         spec_version, SD_PHY_SPECv3_01_VERS),
>       DEFINE_PROP_DRIVE("drive", SDState, blk),
> +    DEFINE_PROP_BOOL("x-aspeed-emmc-kludge", SDState,
> +                     aspeed_emmc_kludge, false),
>       /* We do not model the chip select pin, so allow the board to select
>        * whether card should be in SSI or MMC/SD mode.  It is also up to the
>        * board to ensure that ssi transfers only occur when the chip select

On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
> On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
> > When booting U-boot/Linux on Aspeed boards via eMMC,
> > some commands don't behave as expected from the spec.
> > 
> > Add the 'x-aspeed-emmc-kludge' property to allow non
> > standard uses until we figure out the reasons.
> 
> I am not aware of any singularity in the eMMC logic provided by Aspeed.
> U-Boot and Linux drivers seem very generic. May be others can tell.

I'm not aware of any command kludges. The main problem I had when I
wrote the Linux driver for the Aspeed controller was the phase tuning,
but that doesn't sound related.

Andrew

On 2/7/24 07:06, Andrew Jeffery wrote:
> On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
>> On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
>>> When booting U-boot/Linux on Aspeed boards via eMMC,
>>> some commands don't behave as expected from the spec.
>>>
>>> Add the 'x-aspeed-emmc-kludge' property to allow non
>>> standard uses until we figure out the reasons.
>>
>> I am not aware of any singularity in the eMMC logic provided by Aspeed.
>> U-Boot and Linux drivers seem very generic. May be others can tell.
> 
> I'm not aware of any command kludges. The main problem I had when I
> wrote the Linux driver for the Aspeed controller was the phase tuning,
> but that doesn't sound related.

Yeah I don't think anything Aspeed nor U-boot related, we
model CSD/CID registers per the SD spec, not MMC. Various
fields are identical, but few differ, this might be the
problem.

I rather respect the spec by default, so until we figure
the issue, are you OK to use a 'x-emmc-kludge' property
and set it on the Aspeed boards?

On Tue, 2024-07-02 at 18:15 +0200, Philippe Mathieu-Daudé wrote:
> On 2/7/24 07:06, Andrew Jeffery wrote:
> > On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
> > > On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
> > > > When booting U-boot/Linux on Aspeed boards via eMMC,
> > > > some commands don't behave as expected from the spec.
> > > > 
> > > > Add the 'x-aspeed-emmc-kludge' property to allow non
> > > > standard uses until we figure out the reasons.
> > > 
> > > I am not aware of any singularity in the eMMC logic provided by Aspeed.
> > > U-Boot and Linux drivers seem very generic. May be others can tell.
> > 
> > I'm not aware of any command kludges. The main problem I had when I
> > wrote the Linux driver for the Aspeed controller was the phase tuning,
> > but that doesn't sound related.
> 
> Yeah I don't think anything Aspeed nor U-boot related, we
> model CSD/CID registers per the SD spec, not MMC. Various
> fields are identical, but few differ, this might be the
> problem.
> 
> I rather respect the spec by default, so until we figure
> the issue, are you OK to use a 'x-emmc-kludge' property
> and set it on the Aspeed boards?

Dropping the implication that it's the fault of the Aspeed controller
seems reasonable (without further evidence that it's true).

Andrew

On 7/3/24 7:10 AM, Andrew Jeffery wrote:
> On Tue, 2024-07-02 at 18:15 +0200, Philippe Mathieu-Daudé wrote:
>> On 2/7/24 07:06, Andrew Jeffery wrote:
>>> On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
>>>> On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
>>>>> When booting U-boot/Linux on Aspeed boards via eMMC,
>>>>> some commands don't behave as expected from the spec.
>>>>>
>>>>> Add the 'x-aspeed-emmc-kludge' property to allow non
>>>>> standard uses until we figure out the reasons.
>>>>
>>>> I am not aware of any singularity in the eMMC logic provided by Aspeed.
>>>> U-Boot and Linux drivers seem very generic. May be others can tell.
>>>
>>> I'm not aware of any command kludges. The main problem I had when I
>>> wrote the Linux driver for the Aspeed controller was the phase tuning,
>>> but that doesn't sound related.
>>
>> Yeah I don't think anything Aspeed nor U-boot related, we
>> model CSD/CID registers per the SD spec, not MMC. Various
>> fields are identical, but few differ, this might be the
>> problem.
>>
>> I rather respect the spec by default, so until we figure
>> the issue, are you OK to use a 'x-emmc-kludge' property
>> and set it on the Aspeed boards?
> 
> Dropping the implication that it's the fault of the Aspeed controller
> seems reasonable (without further evidence that it's true).

Yes. Let's introduce 'x-emmc-kludge' and move on.

I dropped all emmc support from the aspeed-9.1 branch for now.

Thanks,

C.

On 7/2/24 6:15 PM, Philippe Mathieu-Daudé wrote:
> On 2/7/24 07:06, Andrew Jeffery wrote:
>> On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
>>> On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
>>>> When booting U-boot/Linux on Aspeed boards via eMMC,
>>>> some commands don't behave as expected from the spec.
>>>>
>>>> Add the 'x-aspeed-emmc-kludge' property to allow non
>>>> standard uses until we figure out the reasons.
>>>
>>> I am not aware of any singularity in the eMMC logic provided by Aspeed.
>>> U-Boot and Linux drivers seem very generic. May be others can tell.
>>
>> I'm not aware of any command kludges. The main problem I had when I
>> wrote the Linux driver for the Aspeed controller was the phase tuning,
>> but that doesn't sound related.
> 
> Yeah I don't think anything Aspeed nor U-boot related, we
> model CSD/CID registers per the SD spec, not MMC. Various
> fields are identical, but few differ, this might be the
> problem.
> 
> I rather respect the spec by default, so until we figure
> the issue, are you OK to use a 'x-emmc-kludge' property
> and set it on the Aspeed boards?

If these differences are eMMC related, why not simply test :

     if (sd_is_emmc(sd)) ...

in commands ALL_SEND_CID and APP_CMD ? The extra property looks
ambiguous to me.

Thanks,

C.

On 2/7/24 18:21, Cédric Le Goater wrote:
> On 7/2/24 6:15 PM, Philippe Mathieu-Daudé wrote:
>> On 2/7/24 07:06, Andrew Jeffery wrote:
>>> On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
>>>> On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
>>>>> When booting U-boot/Linux on Aspeed boards via eMMC,
>>>>> some commands don't behave as expected from the spec.
>>>>>
>>>>> Add the 'x-aspeed-emmc-kludge' property to allow non
>>>>> standard uses until we figure out the reasons.
>>>>
>>>> I am not aware of any singularity in the eMMC logic provided by Aspeed.
>>>> U-Boot and Linux drivers seem very generic. May be others can tell.
>>>
>>> I'm not aware of any command kludges. The main problem I had when I
>>> wrote the Linux driver for the Aspeed controller was the phase tuning,
>>> but that doesn't sound related.
>>
>> Yeah I don't think anything Aspeed nor U-boot related, we
>> model CSD/CID registers per the SD spec, not MMC. Various
>> fields are identical, but few differ, this might be the
>> problem.
>>
>> I rather respect the spec by default, so until we figure
>> the issue, are you OK to use a 'x-emmc-kludge' property
>> and set it on the Aspeed boards?
> 
> If these differences are eMMC related, why not simply test :
> 
>      if (sd_is_emmc(sd)) ...
> 
> in commands ALL_SEND_CID and APP_CMD ? The extra property looks
> ambiguous to me.

I'd like to keep the sd_is_emmc() check for code respecting
the eMMC spec. I believe the commands in sd_proto_emmc[] in
this series do respect it, modulo some register field
definitions that are SD specific. So 'x-emmc-kludge' would
be a property to allow eMMC use -- without delaying it further
--, by bypassing a *bug* in our current model. I'm willing to
figure out the problem and fix it, but /after/ the 9.1 release.
We are too close of the soft freeze and trying to fix that
before is too much pressure on my right now.

> Thanks,
> 
> C.
> 
> 

On 2/7/24 22:05, Philippe Mathieu-Daudé wrote:
> On 2/7/24 18:21, Cédric Le Goater wrote:
>> On 7/2/24 6:15 PM, Philippe Mathieu-Daudé wrote:
>>> On 2/7/24 07:06, Andrew Jeffery wrote:
>>>> On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
>>>>> On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
>>>>>> When booting U-boot/Linux on Aspeed boards via eMMC,
>>>>>> some commands don't behave as expected from the spec.
>>>>>>
>>>>>> Add the 'x-aspeed-emmc-kludge' property to allow non
>>>>>> standard uses until we figure out the reasons.
>>>>>
>>>>> I am not aware of any singularity in the eMMC logic provided by 
>>>>> Aspeed.
>>>>> U-Boot and Linux drivers seem very generic. May be others can tell.
>>>>
>>>> I'm not aware of any command kludges. The main problem I had when I
>>>> wrote the Linux driver for the Aspeed controller was the phase tuning,
>>>> but that doesn't sound related.
>>>
>>> Yeah I don't think anything Aspeed nor U-boot related, we
>>> model CSD/CID registers per the SD spec, not MMC. Various
>>> fields are identical, but few differ, this might be the
>>> problem.
>>>
>>> I rather respect the spec by default, so until we figure
>>> the issue, are you OK to use a 'x-emmc-kludge' property
>>> and set it on the Aspeed boards?
>>
>> If these differences are eMMC related, why not simply test :
>>
>>      if (sd_is_emmc(sd)) ...
>>
>> in commands ALL_SEND_CID and APP_CMD ? The extra property looks
>> ambiguous to me.
> 
> I'd like to keep the sd_is_emmc() check for code respecting
> the eMMC spec. I believe the commands in sd_proto_emmc[] in
> this series do respect it, modulo some register field
> definitions that are SD specific. So 'x-emmc-kludge' would
> be a property to allow eMMC use -- without delaying it further
> --, by bypassing a *bug* in our current model. I'm willing to
> figure out the problem and fix it, but /after/ the 9.1 release.
> We are too close of the soft freeze and trying to fix that
> before is too much pressure on my right now.

The problem is in the still unreviewed patch #86 of this series
"hw/sd/sdcard: Add emmc_cmd_SEND_OP_COND handler (CMD1)".

SEND_OP_COND should put the card in READY state. We are not
considering the BOOT_PARTITION_ENABLE feature:

   > When BOOT_PARTITION_ENABLE bits are set and master send
   > CMD1 (SEND_OP_COND), slave must enter Card Identification
   > Mode and respond to the command.
   > If the slave does not support boot operation mode, which
   > is compliant with v4.2 or before, or BOOT_PARTITION_ENABLE
   > bit is cleared, slave automatically enter Idle State after
   > power-on.

Then we don't need the change in the next patch (#91) in
ALL_SEND_CID.

And likely neither we need #92 (APP_CMD ) but I still need
to confirm that.

On 2/7/24 22:05, Philippe Mathieu-Daudé wrote:
> On 2/7/24 18:21, Cédric Le Goater wrote:
>> On 7/2/24 6:15 PM, Philippe Mathieu-Daudé wrote:
>>> On 2/7/24 07:06, Andrew Jeffery wrote:
>>>> On Fri, 2024-06-28 at 11:16 +0200, Cédric Le Goater wrote:
>>>>> On 6/28/24 9:02 AM, Philippe Mathieu-Daudé wrote:
>>>>>> When booting U-boot/Linux on Aspeed boards via eMMC,
>>>>>> some commands don't behave as expected from the spec.
>>>>>>
>>>>>> Add the 'x-aspeed-emmc-kludge' property to allow non
>>>>>> standard uses until we figure out the reasons.
>>>>>
>>>>> I am not aware of any singularity in the eMMC logic provided by 
>>>>> Aspeed.
>>>>> U-Boot and Linux drivers seem very generic. May be others can tell.
>>>>
>>>> I'm not aware of any command kludges. The main problem I had when I
>>>> wrote the Linux driver for the Aspeed controller was the phase tuning,
>>>> but that doesn't sound related.
>>>
>>> Yeah I don't think anything Aspeed nor U-boot related, we
>>> model CSD/CID registers per the SD spec, not MMC. Various
>>> fields are identical, but few differ, this might be the
>>> problem.
>>>
>>> I rather respect the spec by default, so until we figure
>>> the issue, are you OK to use a 'x-emmc-kludge' property
>>> and set it on the Aspeed boards?
>>
>> If these differences are eMMC related, why not simply test :
>>
>>      if (sd_is_emmc(sd)) ...
>>
>> in commands ALL_SEND_CID and APP_CMD ? The extra property looks
>> ambiguous to me.
> 
> I'd like to keep the sd_is_emmc() check for code respecting
> the eMMC spec. I believe the commands in sd_proto_emmc[] in
> this series do respect it, modulo some register field
> definitions that are SD specific. So 'x-emmc-kludge' would
> be a property to allow eMMC use -- without delaying it further
> --, by bypassing a *bug* in our current model. I'm willing to
> figure out the problem and fix it, but /after/ the 9.1 release.
> We are too close of the soft freeze and trying to fix that
> before is too much pressure on my right now.

(Similar pressure I'm putting on you to review this huge series...)