hw/arm/aspeed.c | 7 +++++ hw/sd/sdhci.c | 70 ++++++++++++++++++++++++++++------------- include/hw/arm/aspeed.h | 1 + include/hw/sd/sdhci.h | 5 +++ 4 files changed, 61 insertions(+), 22 deletions(-)
change from v1: 1. Support RTC for AST2700. 2. Support SDHCI write protected pin inverted for AST2500 and AST2600. 3. Introduce Capabilities Register 2 for SD slot 0 and 1. 4. Support create flash devices via command line for AST1030. change from v2: replace wp-invert with wp-inverted and fix review issues. change from v3: 1. add reviewer suggestion about wp_inverted comment 2. AST2500 EVB does not need to set wp-inverted property of sdhci model https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2500-evb.dts#L110 Jamin Lin (3): hw/sd/sdhci: Fix coding style hw/sd/sdhci: Introduce a new Write Protected pin inverted property hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB hw/arm/aspeed.c | 7 +++++ hw/sd/sdhci.c | 70 ++++++++++++++++++++++++++++------------- include/hw/arm/aspeed.h | 1 + include/hw/sd/sdhci.h | 5 +++ 4 files changed, 61 insertions(+), 22 deletions(-) -- 2.34.1
On 11/14/24 10:48, Jamin Lin wrote: > change from v1: > 1. Support RTC for AST2700. > 2. Support SDHCI write protected pin inverted for AST2500 and AST2600. > 3. Introduce Capabilities Register 2 for SD slot 0 and 1. > 4. Support create flash devices via command line for AST1030. > > change from v2: > replace wp-invert with wp-inverted and fix review issues. > > change from v3: > 1. add reviewer suggestion about wp_inverted comment > 2. AST2500 EVB does not need to set wp-inverted property of sdhci model > https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2500-evb.dts#L110 > > Jamin Lin (3): > hw/sd/sdhci: Fix coding style > hw/sd/sdhci: Introduce a new Write Protected pin inverted property > hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB > > hw/arm/aspeed.c | 7 +++++ > hw/sd/sdhci.c | 70 ++++++++++++++++++++++++++++------------- > include/hw/arm/aspeed.h | 1 + > include/hw/sd/sdhci.h | 5 +++ > 4 files changed, 61 insertions(+), 22 deletions(-) > Philippe, I plan to queue patch 2-3 for QEMU 10.0. Is that ok for you ? Thanks, C.
On 27/11/24 10:44, Cédric Le Goater wrote:
> On 11/14/24 10:48, Jamin Lin wrote:
>> change from v1:
>> 1. Support RTC for AST2700.
>> 2. Support SDHCI write protected pin inverted for AST2500 and AST2600.
>> 3. Introduce Capabilities Register 2 for SD slot 0 and 1.
>> 4. Support create flash devices via command line for AST1030.
>>
>> change from v2:
>> replace wp-invert with wp-inverted and fix review issues.
>>
>> change from v3:
>> 1. add reviewer suggestion about wp_inverted comment
>> 2. AST2500 EVB does not need to set wp-inverted property of sdhci model
>>
>> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2500-evb.dts#L110
>>
>> Jamin Lin (3):
>> hw/sd/sdhci: Fix coding style
>> hw/sd/sdhci: Introduce a new Write Protected pin inverted property
>> hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB
>>
>> hw/arm/aspeed.c | 7 +++++
>> hw/sd/sdhci.c | 70 ++++++++++++++++++++++++++++-------------
>> include/hw/arm/aspeed.h | 1 +
>> include/hw/sd/sdhci.h | 5 +++
>> 4 files changed, 61 insertions(+), 22 deletions(-)
>>
>
> Philippe,
>
> I plan to queue patch 2-3 for QEMU 10.0. Is that ok for you ?
Having to modify sdhci.c internals is dubious, since inversion
occurs out of this block. If this is the soc/board layer, isn't
better to model at this level? Smth like:
-- >8 --
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index be3eb70cdd7..aad9be66b75 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -559,8 +559,9 @@ static void aspeed_soc_ast2600_realize(DeviceState
*dev, Error **errp)
}
aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
sc->memmap[ASPEED_DEV_SDHCI]);
+ irq = aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI);
sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
- aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+ sc->sdhci_wp_inverted ? qemu_irq_invert(irq) : irq);
/* eMMC */
if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
---
On 11/27/24 12:23, Philippe Mathieu-Daudé wrote: > On 27/11/24 10:44, Cédric Le Goater wrote: >> On 11/14/24 10:48, Jamin Lin wrote: >>> change from v1: >>> 1. Support RTC for AST2700. >>> 2. Support SDHCI write protected pin inverted for AST2500 and AST2600. >>> 3. Introduce Capabilities Register 2 for SD slot 0 and 1. >>> 4. Support create flash devices via command line for AST1030. >>> >>> change from v2: >>> replace wp-invert with wp-inverted and fix review issues. >>> >>> change from v3: >>> 1. add reviewer suggestion about wp_inverted comment >>> 2. AST2500 EVB does not need to set wp-inverted property of sdhci model >>> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2500-evb.dts#L110 >>> >>> Jamin Lin (3): >>> hw/sd/sdhci: Fix coding style >>> hw/sd/sdhci: Introduce a new Write Protected pin inverted property >>> hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB >>> >>> hw/arm/aspeed.c | 7 +++++ >>> hw/sd/sdhci.c | 70 ++++++++++++++++++++++++++++------------- >>> include/hw/arm/aspeed.h | 1 + >>> include/hw/sd/sdhci.h | 5 +++ >>> 4 files changed, 61 insertions(+), 22 deletions(-) >>> >> >> Philippe, >> >> I plan to queue patch 2-3 for QEMU 10.0. Is that ok for you ? > > Having to modify sdhci.c internals is dubious, since inversion > occurs out of this block. If this is the soc/board layer, isn't > better to model at this level? Smth like: This change is implementing the polarity of a WP pin as described in [1]. I think modeling that with a QOM property should be ok, since we don't have GPIO lines in the generic SDHCI device model. Or we could introduce a SDBusClass class attribute and define new types for it ? Thanks, C. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/mmc-controller.yaml#n52
Hi Philippe,
> Subject: Re: [PATCH v3 0/3] Introduce a new Write Protected pin inverted
> property
>
> On 27/11/24 10:44, Cédric Le Goater wrote:
> > On 11/14/24 10:48, Jamin Lin wrote:
> >> change from v1:
> >> 1. Support RTC for AST2700.
> >> 2. Support SDHCI write protected pin inverted for AST2500 and AST2600.
> >> 3. Introduce Capabilities Register 2 for SD slot 0 and 1.
> >> 4. Support create flash devices via command line for AST1030.
> >>
> >> change from v2:
> >> replace wp-invert with wp-inverted and fix review issues.
> >>
> >> change from v3:
> >> 1. add reviewer suggestion about wp_inverted comment 2. AST2500 EVB
> >> does not need to set wp-inverted property of sdhci model
> >>
> >> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/
> >> arm/boot/dts/aspeed/aspeed-ast2500-evb.dts#L110
> >>
> >> Jamin Lin (3):
> >> hw/sd/sdhci: Fix coding style
> >> hw/sd/sdhci: Introduce a new Write Protected pin inverted property
> >> hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB
> >>
> >> hw/arm/aspeed.c | 7 +++++
> >> hw/sd/sdhci.c | 70
> >> ++++++++++++++++++++++++++++-------------
> >> include/hw/arm/aspeed.h | 1 +
> >> include/hw/sd/sdhci.h | 5 +++
> >> 4 files changed, 61 insertions(+), 22 deletions(-)
> >>
> >
> > Philippe,
> >
> > I plan to queue patch 2-3 for QEMU 10.0. Is that ok for you ?
>
> Having to modify sdhci.c internals is dubious, since inversion occurs out of this
> block. If this is the soc/board layer, isn't better to model at this level? Smth
> like:
>
> -- >8 --
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index
> be3eb70cdd7..aad9be66b75 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -559,8 +559,9 @@ static void aspeed_soc_ast2600_realize(DeviceState
> *dev, Error **errp)
> }
> aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
> sc->memmap[ASPEED_DEV_SDHCI]);
> + irq = aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI);
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
> - aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
> + sc->sdhci_wp_inverted ? qemu_irq_invert(irq) :
> + irq);
>
Thanks for suggestion.
Unfortunately, I still got the "Write-Protected" status after I used "qemu_irq_invert(irq)".
root@ast2600-default# dmesg | grep "mmc"
[ 13.889764] mmc2: SDHCI controller on 1e740200.sdhci [1e740200.sdhci] using ADMA
[ 13.889848] mmc1: SDHCI controller on 1e740100.sdhci [1e740100.sdhci] using ADMA
[ 16.739901] mmc1: new high speed SD card at address 4dd7
[ 16.740330] mmc2: new high speed SD card at address e502
[ 16.745232] mmcblk2: mmc2:e502 QEMU! 128 MiB (ro)
[ 16.748765] mmcblk1: mmc1:4dd7 QEMU! 128 MiB (ro)
I dump RO status of SDHCI driver and I still got the READONLY status.
root@ast2600-default:/# cat /sys/bus/mmc/devices/mmc1\:4dd7/block/mmcblk1/ro
1
Our FW added “sdhci,wp-inverted” in SDHCI bus driver.
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts#L757
This property was defined here, https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/mmc/mmc-controller.yaml#L71
According to the design of SDHCI drivers, it read the PRESENT STATE REGISTER at SDHCI_WRITE_PROTECT bit.
If the value of this bit is 0, the status is readonly.
However, our FW added “wp-inverted” property, so it inverted the readonly status in line 2582.
Using qemu_irq_invert(irq) did not change the PRESENT STATE REGISTER value and
that was why I added a new property in SD common model(sdhci.c) to make users to change this bit value.
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-pltfm.c#L42
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-pltfm.c#L87
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci.c#L2574
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci.c#L2581
Thanks-Jamin
> /* eMMC */
> if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
> ---
> Having to modify sdhci.c internals is dubious, since inversion
> occurs out of this block. If this is the soc/board layer, isn't
> better to model at this level? Smth like:
>
> -- >8 --
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index be3eb70cdd7..aad9be66b75 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -559,8 +559,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> }
> aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
> sc->memmap[ASPEED_DEV_SDHCI]);
> + irq = aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI);
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
> - aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
> + sc->sdhci_wp_inverted ? qemu_irq_invert(irq) : irq);
>
> /* eMMC */
> if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
> ---
Nice ! I didn't know about qemu_irq_invert().
Jamin, could you please give it a try and respin ?
Thanks,
C.
On Wed, 27 Nov 2024 at 11:26, Cédric Le Goater <clg@kaod.org> wrote:
>
>
> > Having to modify sdhci.c internals is dubious, since inversion
> > occurs out of this block. If this is the soc/board layer, isn't
> > better to model at this level? Smth like:
> >
> > -- >8 --
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index be3eb70cdd7..aad9be66b75 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -559,8 +559,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> > }
> > aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
> > sc->memmap[ASPEED_DEV_SDHCI]);
> > + irq = aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI);
> > sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
> > - aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
> > + sc->sdhci_wp_inverted ? qemu_irq_invert(irq) : irq);
> >
> > /* eMMC */
> > if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
> > ---
>
> Nice ! I didn't know about qemu_irq_invert().
I am not a fan of qemu_irq_invert(). It's one of those ancient
pre-QOM APIs that we ideally would get rid of at some point.
Two problems with it:
(1) It allocates and returns a qemu_irq directly,
so in the patch above you're effectively leaking that
allocation. (Not a big deal since the SoC object is going to
be around for the life of the QEMU process, but probably
the clang leak-sanitizer will complain.)
(2) It calls qemu_irq_raise() directly, immediately. This is
kind of bogus in a realize function, where you're not supposed
to be raising IRQ lines yet; it also doesn't do anything about
reset, so if the device on the other end *did* care about seeing
that 0->1 transition then it will be broken on system-reset
because the transition won't happen. (Handling "device is supposed
to have an asserted-as-1 line coming out of reset" is not
something that we do very well in QEMU generally. In theory
3-phase reset is supposed to help with this by letting you do
the assert-the-line in the reset-exit phase, but in practice we
typically just don't model the line-assertion at all and
trust that the reset state of the device on the far end is
what it ought to be anyway...)
I would not recommend using qemu_irq_invert() in new code.
I guess in an ideal world we'd implement a QOM object
that encapsulated the the "not gate" logic, similar to
TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake
of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE,
so it doesn't get reset properly on system reset and so
the "what happens to the output on reset" is still not
really correct.)
thanks
-- PMM
Hello, > I would not recommend using qemu_irq_invert() in new code. > > I guess in an ideal world we'd implement a QOM object > that encapsulated the the "not gate" logic, similar to > TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake > of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE, > so it doesn't get reset properly on system reset and so > the "what happens to the output on reset" is still not > really correct.) I see how this would work with TYPE_PL181 model but I don't understand how this could work with TYPE_SYSBUS_SDHCI since we don't have a gpio line to invert the level. Am I missing something ? Thanks, C.
On Tue, 7 Jan 2025 at 17:55, Cédric Le Goater <clg@kaod.org> wrote: > > Hello, > > > I would not recommend using qemu_irq_invert() in new code. > > > > I guess in an ideal world we'd implement a QOM object > > that encapsulated the the "not gate" logic, similar to > > TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake > > of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE, > > so it doesn't get reset properly on system reset and so > > the "what happens to the output on reset" is still not > > really correct.) > > I see how this would work with TYPE_PL181 model but I don't > understand how this could work with TYPE_SYSBUS_SDHCI since > we don't have a gpio line to invert the level. Am I missing > something ? You have a gpio line, i.e. a qemu_irq, because you're passing it to qemu_irq_invert(), and you could instead connect it into a hypothetical TYPE_NOT_IRQ device. qemu_irq are GPIO lines, the type just has an odd name for historical reasons. -- PMM
On 1/7/25 23:36, Peter Maydell wrote: > On Tue, 7 Jan 2025 at 17:55, Cédric Le Goater <clg@kaod.org> wrote: >> >> Hello, >> >>> I would not recommend using qemu_irq_invert() in new code. >>> >>> I guess in an ideal world we'd implement a QOM object >>> that encapsulated the the "not gate" logic, similar to >>> TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake >>> of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE, >>> so it doesn't get reset properly on system reset and so >>> the "what happens to the output on reset" is still not >>> really correct.) >> >> I see how this would work with TYPE_PL181 model but I don't >> understand how this could work with TYPE_SYSBUS_SDHCI since >> we don't have a gpio line to invert the level. Am I missing >> something ? > > You have a gpio line, i.e. a qemu_irq, because you're passing > it to qemu_irq_invert(), This part was proposed by Philippe and is not related to the proposal. I am not very familiar with these devices and AFAUI, the SHDCIState irq being modified has a much broader scope than a write protection toggle/level. I suppose the irq covers all kind of transfers. > and you could instead connect it into > a hypothetical TYPE_NOT_IRQ device. qemu_irq are GPIO lines, > the type just has an odd name for historical reasons. Yes. I was looking for something like this but we don't have a "card-read-only" GPIO line like in TYPE_PL181. Hence my confusion. Thanks, C.
© 2016 - 2026 Red Hat, Inc.