| Message ID | 20230709080950.92489-1-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers | expand |
On 7/9/23 01:09, Bernhard Beschow wrote: > Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller > interfaces" sdhci_common_realize() forces all SD card controllers to use either > sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. > However, there are device models which use different MMIO ops: TYPE_IMX_USDHC > uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. > > Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for > example. Fix this by defaulting the io_ops to little endian and switch to big > endian in sdhci_common_realize() only if there is a matchig big endian variant > available. > > Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller > interfaces") > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> Tested-by: Guenter Roeck <linux@roeck-us.net> > --- > hw/sd/sdhci.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 6811f0f1a8..362c2c86aa 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) > > s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); > + > + s->io_ops = &sdhci_mmio_le_ops; > } > > void sdhci_uninitfn(SDHCIState *s) > @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) > > switch (s->endianness) { > case DEVICE_LITTLE_ENDIAN: > - s->io_ops = &sdhci_mmio_le_ops; > + /* s->io_ops is little endian by default */ > break; > case DEVICE_BIG_ENDIAN: > + if (s->io_ops != &sdhci_mmio_le_ops) { > + error_setg(errp, "SD controller doesn't support big endianness"); > + return; > + } > s->io_ops = &sdhci_mmio_be_ops; > break; > default:
On 9/7/23 10:09, Bernhard Beschow wrote: > Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller > interfaces" sdhci_common_realize() forces all SD card controllers to use either > sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. > However, there are device models which use different MMIO ops: TYPE_IMX_USDHC > uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. > > Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for > example. Fix this by defaulting the io_ops to little endian and switch to big > endian in sdhci_common_realize() only if there is a matchig big endian variant > available. > > Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller > interfaces") > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/sd/sdhci.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 6811f0f1a8..362c2c86aa 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) > > s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); > + > + s->io_ops = &sdhci_mmio_le_ops; > } > > void sdhci_uninitfn(SDHCIState *s) > @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) > What about simply keeping the same code guarded with 'if (!s->io_ops)'? > switch (s->endianness) { > case DEVICE_LITTLE_ENDIAN: > - s->io_ops = &sdhci_mmio_le_ops; > + /* s->io_ops is little endian by default */ > break; > case DEVICE_BIG_ENDIAN: > + if (s->io_ops != &sdhci_mmio_le_ops) { > + error_setg(errp, "SD controller doesn't support big endianness"); > + return; > + } > s->io_ops = &sdhci_mmio_be_ops; > break; > default:
On Mon, Jul 10, 2023 at 12:16:35PM +0200, Philippe Mathieu-Daudé wrote: > On 9/7/23 10:09, Bernhard Beschow wrote: > > Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller > > interfaces" sdhci_common_realize() forces all SD card controllers to use either > > sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. > > However, there are device models which use different MMIO ops: TYPE_IMX_USDHC > > uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. > > > > Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for > > example. Fix this by defaulting the io_ops to little endian and switch to big > > endian in sdhci_common_realize() only if there is a matchig big endian variant > > available. > > > > Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller > > interfaces") > > > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > > --- > > hw/sd/sdhci.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > > index 6811f0f1a8..362c2c86aa 100644 > > --- a/hw/sd/sdhci.c > > +++ b/hw/sd/sdhci.c > > @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) > > s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); > > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); > > + > > + s->io_ops = &sdhci_mmio_le_ops; > > } > > void sdhci_uninitfn(SDHCIState *s) > > @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) > > What about simply keeping the same code guarded with 'if (!s->io_ops)'? > That was my quick fix which I considered a hack, and I didn't submit it because I thought it was a hack ;-). On the other side, this solution would probably break on big endian systems which have their own io ops, so I am not sure if it is any better. Guenter > > switch (s->endianness) { > > case DEVICE_LITTLE_ENDIAN: > > - s->io_ops = &sdhci_mmio_le_ops; > > + /* s->io_ops is little endian by default */ > > break; > > case DEVICE_BIG_ENDIAN: > > + if (s->io_ops != &sdhci_mmio_le_ops) { > > + error_setg(errp, "SD controller doesn't support big endianness"); > > + return; > > + } > > s->io_ops = &sdhci_mmio_be_ops; > > break; > > default: >
Am 10. Juli 2023 10:16:35 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 9/7/23 10:09, Bernhard Beschow wrote: >> Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller >> interfaces" sdhci_common_realize() forces all SD card controllers to use either >> sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. >> However, there are device models which use different MMIO ops: TYPE_IMX_USDHC >> uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. >> >> Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for >> example. Fix this by defaulting the io_ops to little endian and switch to big >> endian in sdhci_common_realize() only if there is a matchig big endian variant >> available. >> >> Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller >> interfaces") >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/sd/sdhci.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >> index 6811f0f1a8..362c2c86aa 100644 >> --- a/hw/sd/sdhci.c >> +++ b/hw/sd/sdhci.c >> @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) >> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); >> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); >> + >> + s->io_ops = &sdhci_mmio_le_ops; >> } >> void sdhci_uninitfn(SDHCIState *s) >> @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) >> > >What about simply keeping the same code guarded with 'if (!s->io_ops)'? I chose below approach since it provides an error message when one attempts to set one of the other device models to BE rather than just silently ignoring it. Also, I didn't want to make the assumption that `s->io_ops == NULL` implied that sdhci_mmio_*_ops is needed. That's similar material the bug fixed is made of, so I wanted to prevent that in the first place by being more explicit. In combination with the new error message the limitations of the current code become hopefully very apparent now, and at the same time should provide enough hints for adding BE support to the other device models if ever needed. Best regards, Bernhard > >> switch (s->endianness) { >> case DEVICE_LITTLE_ENDIAN: >> - s->io_ops = &sdhci_mmio_le_ops; >> + /* s->io_ops is little endian by default */ >> break; >> case DEVICE_BIG_ENDIAN: >> + if (s->io_ops != &sdhci_mmio_le_ops) { >> + error_setg(errp, "SD controller doesn't support big endianness"); >> + return; >> + } >> s->io_ops = &sdhci_mmio_be_ops; >> break; >> default: >
Am 10. Juli 2023 16:01:46 UTC schrieb Bernhard Beschow <shentey@gmail.com>: > > >Am 10. Juli 2023 10:16:35 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>On 9/7/23 10:09, Bernhard Beschow wrote: >>> Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller >>> interfaces" sdhci_common_realize() forces all SD card controllers to use either >>> sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. >>> However, there are device models which use different MMIO ops: TYPE_IMX_USDHC >>> uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. >>> >>> Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for >>> example. Fix this by defaulting the io_ops to little endian and switch to big >>> endian in sdhci_common_realize() only if there is a matchig big endian variant >>> available. >>> >>> Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller >>> interfaces") >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/sd/sdhci.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >>> index 6811f0f1a8..362c2c86aa 100644 >>> --- a/hw/sd/sdhci.c >>> +++ b/hw/sd/sdhci.c >>> @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) >>> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); >>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); >>> + >>> + s->io_ops = &sdhci_mmio_le_ops; >>> } >>> void sdhci_uninitfn(SDHCIState *s) >>> @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) >>> >> >>What about simply keeping the same code guarded with 'if (!s->io_ops)'? > >I chose below approach since it provides an error message when one attempts to set one of the other device models to BE rather than just silently ignoring it. > >Also, I didn't want to make the assumption that `s->io_ops == NULL` implied that sdhci_mmio_*_ops is needed. That's similar material the bug fixed is made of, so I wanted to prevent that in the first place by being more explicit. > >In combination with the new error message the limitations of the current code become hopefully very apparent now, and at the same time should provide enough hints for adding BE support to the other device models if ever needed. > >Best regards, >Bernhard Ping + qemu--stable > >> >>> switch (s->endianness) { >>> case DEVICE_LITTLE_ENDIAN: >>> - s->io_ops = &sdhci_mmio_le_ops; >>> + /* s->io_ops is little endian by default */ >>> break; >>> case DEVICE_BIG_ENDIAN: >>> + if (s->io_ops != &sdhci_mmio_le_ops) { >>> + error_setg(errp, "SD controller doesn't support big endianness"); >>> + return; >>> + } >>> s->io_ops = &sdhci_mmio_be_ops; >>> break; >>> default: >>
Am 16. Juli 2023 19:53:37 UTC schrieb Bernhard Beschow <shentey@gmail.com>: > > >Am 10. Juli 2023 16:01:46 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >> >> >>Am 10. Juli 2023 10:16:35 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>On 9/7/23 10:09, Bernhard Beschow wrote: >>>> Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller >>>> interfaces" sdhci_common_realize() forces all SD card controllers to use either >>>> sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. >>>> However, there are device models which use different MMIO ops: TYPE_IMX_USDHC >>>> uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. >>>> >>>> Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for >>>> example. Fix this by defaulting the io_ops to little endian and switch to big >>>> endian in sdhci_common_realize() only if there is a matchig big endian variant >>>> available. >>>> >>>> Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller >>>> interfaces") >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/sd/sdhci.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >>>> index 6811f0f1a8..362c2c86aa 100644 >>>> --- a/hw/sd/sdhci.c >>>> +++ b/hw/sd/sdhci.c >>>> @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) >>>> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); >>>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); >>>> + >>>> + s->io_ops = &sdhci_mmio_le_ops; >>>> } >>>> void sdhci_uninitfn(SDHCIState *s) >>>> @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) >>>> >>> >>>What about simply keeping the same code guarded with 'if (!s->io_ops)'? >> >>I chose below approach since it provides an error message when one attempts to set one of the other device models to BE rather than just silently ignoring it. >> >>Also, I didn't want to make the assumption that `s->io_ops == NULL` implied that sdhci_mmio_*_ops is needed. That's similar material the bug fixed is made of, so I wanted to prevent that in the first place by being more explicit. >> >>In combination with the new error message the limitations of the current code become hopefully very apparent now, and at the same time should provide enough hints for adding BE support to the other device models if ever needed. >> >>Best regards, >>Bernhard > >Ping Ping^2 I would like to have the bug fixed in 8.1. Best regards, Bernhard > >+ qemu--stable > >> >>> >>>> switch (s->endianness) { >>>> case DEVICE_LITTLE_ENDIAN: >>>> - s->io_ops = &sdhci_mmio_le_ops; >>>> + /* s->io_ops is little endian by default */ >>>> break; >>>> case DEVICE_BIG_ENDIAN: >>>> + if (s->io_ops != &sdhci_mmio_le_ops) { >>>> + error_setg(errp, "SD controller doesn't support big endianness"); >>>> + return; >>>> + } >>>> s->io_ops = &sdhci_mmio_be_ops; >>>> break; >>>> default: >>>
On 7/24/23 00:18, Bernhard Beschow wrote: > > > Am 16. Juli 2023 19:53:37 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >> >> >> Am 10. Juli 2023 16:01:46 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >>> >>> >>> Am 10. Juli 2023 10:16:35 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>> On 9/7/23 10:09, Bernhard Beschow wrote: >>>>> Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller >>>>> interfaces" sdhci_common_realize() forces all SD card controllers to use either >>>>> sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. >>>>> However, there are device models which use different MMIO ops: TYPE_IMX_USDHC >>>>> uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. >>>>> >>>>> Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for >>>>> example. Fix this by defaulting the io_ops to little endian and switch to big >>>>> endian in sdhci_common_realize() only if there is a matchig big endian variant >>>>> available. >>>>> >>>>> Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller >>>>> interfaces") >>>>> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>> --- >>>>> hw/sd/sdhci.c | 8 +++++++- >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >>>>> index 6811f0f1a8..362c2c86aa 100644 >>>>> --- a/hw/sd/sdhci.c >>>>> +++ b/hw/sd/sdhci.c >>>>> @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) >>>>> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); >>>>> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); >>>>> + >>>>> + s->io_ops = &sdhci_mmio_le_ops; >>>>> } >>>>> void sdhci_uninitfn(SDHCIState *s) >>>>> @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) >>>>> >>>> >>>> What about simply keeping the same code guarded with 'if (!s->io_ops)'? >>> >>> I chose below approach since it provides an error message when one attempts to set one of the other device models to BE rather than just silently ignoring it. >>> >>> Also, I didn't want to make the assumption that `s->io_ops == NULL` implied that sdhci_mmio_*_ops is needed. That's similar material the bug fixed is made of, so I wanted to prevent that in the first place by being more explicit. >>> >>> In combination with the new error message the limitations of the current code become hopefully very apparent now, and at the same time should provide enough hints for adding BE support to the other device models if ever needed. >>> >>> Best regards, >>> Bernhard >> >> Ping > > Ping^2 > > I would like to have the bug fixed in 8.1. > +1 Not that I care too much - I build qemu myself anyway and carry the patch locally - but this really should get fixed. Guenter
On 24/7/23 15:44, Guenter Roeck wrote: > On 7/24/23 00:18, Bernhard Beschow wrote: >> Ping^2 >> >> I would like to have the bug fixed in 8.1. >> > > +1 > > Not that I care too much - I build qemu myself anyway and carry the > patch locally - > but this really should get fixed. > > Guenter > Patch queued, thanks!
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 6811f0f1a8..362c2c86aa 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s) s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s); s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s); + + s->io_ops = &sdhci_mmio_le_ops; } void sdhci_uninitfn(SDHCIState *s) @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp) switch (s->endianness) { case DEVICE_LITTLE_ENDIAN: - s->io_ops = &sdhci_mmio_le_ops; + /* s->io_ops is little endian by default */ break; case DEVICE_BIG_ENDIAN: + if (s->io_ops != &sdhci_mmio_le_ops) { + error_setg(errp, "SD controller doesn't support big endianness"); + return; + } s->io_ops = &sdhci_mmio_be_ops; break; default:
Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller interfaces" sdhci_common_realize() forces all SD card controllers to use either sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property. However, there are device models which use different MMIO ops: TYPE_IMX_USDHC uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops. Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for example. Fix this by defaulting the io_ops to little endian and switch to big endian in sdhci_common_realize() only if there is a matchig big endian variant available. Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller interfaces") Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/sd/sdhci.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)