| Message ID | 20240628070216.92609-91-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd/sdcard: Add eMMC support | expand |
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 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. (Similar pressure I'm putting on you to review this huge series...)
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 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.
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
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(+)