diff mbox

[2/2] m48t59: add mem_base value to m48t59_init_isa()

Message ID 1421667328-11800-3-git-send-email-mark.cave-ayland@ilande.co.uk
State New
Headers show

Commit Message

Mark Cave-Ayland Jan. 19, 2015, 11:35 a.m. UTC
Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
MMIO rather than ioport if required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/prep.c             |    2 +-
 hw/sparc64/sun4u.c        |    2 +-
 hw/timer/m48t59.c         |    9 +++++++--
 include/hw/timer/m48t59.h |    4 ++--
 4 files changed, 11 insertions(+), 6 deletions(-)

Comments

Paolo Bonzini Jan. 19, 2015, 12:45 p.m. UTC | #1
On 19/01/2015 12:35, Mark Cave-Ayland wrote:
> Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
> MMIO rather than ioport if required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

Is it really ISA if it's MMIO?  In other words, why can't this be a
sysbus device?

Paolo
Artyom Tarasenko Jan. 19, 2015, 12:57 p.m. UTC | #2
On Mon, Jan 19, 2015 at 1:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 19/01/2015 12:35, Mark Cave-Ayland wrote:
>> Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
>> MMIO rather than ioport if required.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>
> Is it really ISA if it's MMIO?  In other words, why can't this be a
> sysbus device?

On physical machines it's EBus, which is pretty much like 8-bit ISA.
So, I think modelling it as ISA is closer to to the reality.
But out of curiosity, would it be possible to have a sysbus device
somewhere in a middle of PCI space? Do sysbus devices have higher
priority if the address spaces overlap? Or do you mean that the PCI
controller needs to be modified to have a hole for a sysbus device?

Artyom
Paolo Bonzini Jan. 19, 2015, 12:59 p.m. UTC | #3
On 19/01/2015 13:57, Artyom Tarasenko wrote:
>> > Is it really ISA if it's MMIO?  In other words, why can't this be a
>> > sysbus device?
> On physical machines it's EBus, which is pretty much like 8-bit ISA.
> So, I think modelling it as ISA is closer to to the reality.
> But out of curiosity, would it be possible to have a sysbus device
> somewhere in a middle of PCI space? Do sysbus devices have higher
> priority if the address spaces overlap? Or do you mean that the PCI
> controller needs to be modified to have a hole for a sysbus device?

What does the memory map look like (simplifying to "where can BARs be"
and "where is the RTC")?

Paolo
Artyom Tarasenko Jan. 19, 2015, 1:12 p.m. UTC | #4
On Mon, Jan 19, 2015 at 1:59 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 19/01/2015 13:57, Artyom Tarasenko wrote:
>>> > Is it really ISA if it's MMIO?  In other words, why can't this be a
>>> > sysbus device?
>> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>> So, I think modelling it as ISA is closer to to the reality.
>> But out of curiosity, would it be possible to have a sysbus device
>> somewhere in a middle of PCI space? Do sysbus devices have higher
>> priority if the address spaces overlap? Or do you mean that the PCI
>> controller needs to be modified to have a hole for a sysbus device?
>
> What does the memory map look like (simplifying to "where can BARs be"
> and "where is the RTC")?

 dev: pbm, id ""
    mmio 000001fe00000000/0000000000010000
    mmio 000001fe01000000/0000000001000000
    mmio 000001fe02000000/0000000000010000
    bus: pci
      dev: ebus, id ""
        addr = 03.0
        class Bridge, addr 00:03.0, pci id 108e:1000 (sub 1af4:1100)
        bar 0: mem at 0x3000000 [0x3ffffff]
        bar 1: i/o at 0x4000 [0x7fff]
        bus: isa.0
          type ISA
          dev: m48t59_isa, id ""
            size = 8192 (0x2000)
            io_base = 8192 (0x2000)

(actually it should be better to have it at the beginning of the
ISA-space, 0x0, but it is not critical, since QEMU's sun4u machine
doesn't exactly match any known physical sun4u machine)


Artyom
Andreas Färber Jan. 19, 2015, 3:01 p.m. UTC | #5
Am 19.01.2015 um 13:57 schrieb Artyom Tarasenko:
> On Mon, Jan 19, 2015 at 1:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 19/01/2015 12:35, Mark Cave-Ayland wrote:
>>> Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
>>> MMIO rather than ioport if required.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>
>> Is it really ISA if it's MMIO?  In other words, why can't this be a
>> sysbus device?
> 
> On physical machines it's EBus, which is pretty much like 8-bit ISA.
> So, I think modelling it as ISA is closer to to the reality.
> But out of curiosity, would it be possible to have a sysbus device
> somewhere in a middle of PCI space? [...]

Why would you want to use a SysBusDevice in the first place? I
previously discussed with Mark that it should be an EBusDevice, not an
ISADevice or SysBusDevice. IndustryPack is an example of a custom bus
that sits behind a PCI bridge and doesn't need a global variable.

Also, wasn't Hervé's(?) plan to get rid of mem_base completely by always
passing a pointer to ISADevice/ISABus around? It should only be needed
when somewhere NULL is being passed, no?

Regards,
Andreas
Peter Maydell Jan. 19, 2015, 3:04 p.m. UTC | #6
On 19 January 2015 at 12:57, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
> But out of curiosity, would it be possible to have a sysbus device
> somewhere in a middle of PCI space? Do sysbus devices have higher
> priority if the address spaces overlap? Or do you mean that the PCI
> controller needs to be modified to have a hole for a sysbus device?

You can specify the priority when you map devices into a MemoryRegion,
so you can handle this by either making the sysbus device a positive
priority or by making the PCI window have a negative priority.

(We don't actually get this right on x86 currently,
which has resulted in some awkwardness for the PPC desire to
make PCI address 0 valid.)

-- PMM
Artyom Tarasenko Jan. 19, 2015, 3:22 p.m. UTC | #7
On Mon, Jan 19, 2015 at 4:01 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 19.01.2015 um 13:57 schrieb Artyom Tarasenko:
>> On Mon, Jan 19, 2015 at 1:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 19/01/2015 12:35, Mark Cave-Ayland wrote:
>>>> Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
>>>> MMIO rather than ioport if required.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>
>>> Is it really ISA if it's MMIO?  In other words, why can't this be a
>>> sysbus device?
>>
>> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>> So, I think modelling it as ISA is closer to to the reality.
>> But out of curiosity, would it be possible to have a sysbus device
>> somewhere in a middle of PCI space? [...]
>
> Why would you want to use a SysBusDevice in the first place?

Ask Paolo. :-) For me it's only important to have a MMIO device in the
proper address range.

> I previously discussed with Mark that it should be an EBusDevice, not an
> ISADevice or SysBusDevice.

Interesting. I can't find this discussion in the list archive. Do you suggest to
create EBusDevices for all ISA devices (serial, parallel, keyboard,
floppy) used in sun4u, or only for m48t59?
What would be the advantage of using EBusDevice over ISADevice?

Artyom

> IndustryPack is an example of a custom bus
> that sits behind a PCI bridge and doesn't need a global variable.
>
> Also, wasn't Hervé's(?) plan to get rid of mem_base completely by always
> passing a pointer to ISADevice/ISABus around? It should only be needed
> when somewhere NULL is being passed, no?
>
> Regards,
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
> Graham Norton; HRB 21284 (AG Nürnberg)
Paolo Bonzini Jan. 19, 2015, 3:31 p.m. UTC | #8
On 19/01/2015 16:22, Artyom Tarasenko wrote:
>>> >>
>>> >> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>>> >> So, I think modelling it as ISA is closer to to the reality.
>>> >> But out of curiosity, would it be possible to have a sysbus device
>>> >> somewhere in a middle of PCI space? [...]
>> >
>> > Why would you want to use a SysBusDevice in the first place?
> Ask Paolo. :-) For me it's only important to have a MMIO device in the
> proper address range.

The reason I asked is simply because ISA devices never do MMIO (apart
for the VGA window).

>> > I previously discussed with Mark that it should be an EBusDevice, not an
>> > ISADevice or SysBusDevice.
> Interesting. I can't find this discussion in the list archive. Do you suggest to
> create EBusDevices for all ISA devices (serial, parallel, keyboard,
> floppy) used in sun4u, or only for m48t59?
> What would be the advantage of using EBusDevice over ISADevice?

Is there a description of EBus and the sun4u memory map somewhere?

Is there an "EBus bridge" PCI device similar to the PCI-to-ISA bridge?

Paolo
Andreas Färber Jan. 19, 2015, 3:38 p.m. UTC | #9
Am 19.01.2015 um 16:31 schrieb Paolo Bonzini:
> Is there an "EBus bridge" PCI device similar to the PCI-to-ISA bridge?

In the previous dump, the "ebus" entry looked like one to me?

    bus: pci
      dev: ebus, id ""
        addr = 03.0
        class Bridge, addr 00:03.0, pci id 108e:1000 (sub 1af4:1100)
        bar 0: mem at 0x3000000 [0x3ffffff]
        bar 1: i/o at 0x4000 [0x7fff]
        bus: isa.0
          type ISA

Confusingly named, as "ebus" would be a better type name for the actual
bus, but "EBus" might work as bus name if we actually need one (does the
user add devices to this bus or are these all chipset features
recursively set up by the machine?). Otherwise they could just be
child<> properties on the ebus device.

Andreas
Paolo Bonzini Jan. 19, 2015, 4:01 p.m. UTC | #10
On 19/01/2015 16:38, Andreas Färber wrote:
>> > Is there an "EBus bridge" PCI device similar to the PCI-to-ISA bridge?
> In the previous dump, the "ebus" entry looked like one to me?
> 
>     bus: pci
>       dev: ebus, id ""
>         addr = 03.0
>         class Bridge, addr 00:03.0, pci id 108e:1000 (sub 1af4:1100)
>         bar 0: mem at 0x3000000 [0x3ffffff]
>         bar 1: i/o at 0x4000 [0x7fff]
>         bus: isa.0
>           type ISA
> 
> Confusingly named, as "ebus" would be a better type name for the actual
> bus, but "EBus" might work as bus name if we actually need one (does the
> user add devices to this bus or are these all chipset features
> recursively set up by the machine?). Otherwise they could just be
> child<> properties on the ebus device.

So is this m48t59 device mapped inside a BAR of its parent ebus device?

What value will the sun4u patch pass for mem_base?

Paolo
Artyom Tarasenko Jan. 19, 2015, 4:17 p.m. UTC | #11
On Mon, Jan 19, 2015 at 4:31 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 19/01/2015 16:22, Artyom Tarasenko wrote:
>>>> >>
>>>> >> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>>>> >> So, I think modelling it as ISA is closer to to the reality.
>>>> >> But out of curiosity, would it be possible to have a sysbus device
>>>> >> somewhere in a middle of PCI space? [...]
>>> >
>>> > Why would you want to use a SysBusDevice in the first place?
>> Ask Paolo. :-) For me it's only important to have a MMIO device in the
>> proper address range.
>
> The reason I asked is simply because ISA devices never do MMIO (apart
> for the VGA window).

You mean in the QEMU world? At least physical SCSI and Ethernet
adapters had a MMIO space for the onboard ROM.

>>> > I previously discussed with Mark that it should be an EBusDevice, not an
>>> > ISADevice or SysBusDevice.
>> Interesting. I can't find this discussion in the list archive. Do you suggest to
>> create EBusDevices for all ISA devices (serial, parallel, keyboard,
>> floppy) used in sun4u, or only for m48t59?
>> What would be the advantage of using EBusDevice over ISADevice?
>
> Is there a description of EBus and the sun4u memory map somewhere?

I could find only sparse pieces. "Uniprocessor System Controller
User's Manual" (805-0170.pdf) has some brief description, it's also
mentioned in the STP2223BGA  and STP2200ABGA data sheets.

> Is there an "EBus bridge" PCI device similar to the PCI-to-ISA bridge?

As physical devices there are integrated SBus-to-EBus and PCI-to-EBus bridges.

But actually I may have been wrong about NVRAM always sitting on the
EBus: looking at the page 28 of "UltraSPARC™-IIi User's Manual"
(805-0087.pdf), I see that NVRAM, Serial and other controllers reside
in a "PC compatible SuperIO" chip which sits on a PCI bus.

Artyom
Paolo Bonzini Jan. 19, 2015, 4:34 p.m. UTC | #12
On 19/01/2015 17:17, Artyom Tarasenko wrote:
> On Mon, Jan 19, 2015 at 4:31 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 19/01/2015 16:22, Artyom Tarasenko wrote:
>>>>>>>
>>>>>>> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>>>>>>> So, I think modelling it as ISA is closer to to the reality.
>>>>>>> But out of curiosity, would it be possible to have a sysbus device
>>>>>>> somewhere in a middle of PCI space? [...]
>>>>>
>>>>> Why would you want to use a SysBusDevice in the first place?
>>> Ask Paolo. :-) For me it's only important to have a MMIO device in the
>>> proper address range.
>>
>> The reason I asked is simply because ISA devices never do MMIO (apart
>> for the VGA window).
> 
> You mean in the QEMU world? At least physical SCSI and Ethernet
> adapters had a MMIO space for the onboard ROM.

Uh right, ROMs count as MMIO too.

>>>>> I previously discussed with Mark that it should be an EBusDevice, not an
>>>>> ISADevice or SysBusDevice.
>>> Interesting. I can't find this discussion in the list archive. Do you suggest to
>>> create EBusDevices for all ISA devices (serial, parallel, keyboard,
>>> floppy) used in sun4u, or only for m48t59?
>>> What would be the advantage of using EBusDevice over ISADevice?
>>
>> Is there a description of EBus and the sun4u memory map somewhere?
> 
> I could find only sparse pieces. "Uniprocessor System Controller
> User's Manual" (805-0170.pdf) has some brief description, it's also
> mentioned in the STP2223BGA  and STP2200ABGA data sheets.
> 
>> Is there an "EBus bridge" PCI device similar to the PCI-to-ISA bridge?
> 
> As physical devices there are integrated SBus-to-EBus and PCI-to-EBus bridges.
> 
> But actually I may have been wrong about NVRAM always sitting on the
> EBus: looking at the page 28 of "UltraSPARC™-IIi User's Manual"
> (805-0087.pdf), I see that NVRAM, Serial and other controllers reside
> in a "PC compatible SuperIO" chip which sits on a PCI bus.

That's an ISA bridge basically.  I understand a little more of how this
is supposed to work now, but I think it makes little sense to add this
patch without the corresponding user.

Paolo
Mark Cave-Ayland Jan. 19, 2015, 4:42 p.m. UTC | #13
On 19/01/15 15:01, Andreas Färber wrote:

> Am 19.01.2015 um 13:57 schrieb Artyom Tarasenko:
>> On Mon, Jan 19, 2015 at 1:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 19/01/2015 12:35, Mark Cave-Ayland wrote:
>>>> Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
>>>> MMIO rather than ioport if required.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>
>>> Is it really ISA if it's MMIO?  In other words, why can't this be a
>>> sysbus device?
>>
>> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>> So, I think modelling it as ISA is closer to to the reality.
>> But out of curiosity, would it be possible to have a sysbus device
>> somewhere in a middle of PCI space? [...]
> 
> Why would you want to use a SysBusDevice in the first place? I
> previously discussed with Mark that it should be an EBusDevice, not an
> ISADevice or SysBusDevice. IndustryPack is an example of a custom bus
> that sits behind a PCI bridge and doesn't need a global variable.

I can see this makes logical sense - I guess the reason it hasn't been
done was to avoid having to write EBus-specific initialisation code for
every device which would only be used on one platform. So you're
suggesting that IndustryPack is a way of doing this?

> Also, wasn't Hervé's(?) plan to get rid of mem_base completely by always
> passing a pointer to ISADevice/ISABus around? It should only be needed
> when somewhere NULL is being passed, no?

That would definitely be better for wiring things up with -device but
wouldn't that involve changing all of the existing ISA devices?


ATB,

Mark.
Mark Cave-Ayland Jan. 19, 2015, 4:48 p.m. UTC | #14
On 19/01/15 15:04, Peter Maydell wrote:

> On 19 January 2015 at 12:57, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
>> But out of curiosity, would it be possible to have a sysbus device
>> somewhere in a middle of PCI space? Do sysbus devices have higher
>> priority if the address spaces overlap? Or do you mean that the PCI
>> controller needs to be modified to have a hole for a sysbus device?
> 
> You can specify the priority when you map devices into a MemoryRegion,
> so you can handle this by either making the sysbus device a positive
> priority or by making the PCI window have a negative priority.
> 
> (We don't actually get this right on x86 currently,
> which has resulted in some awkwardness for the PPC desire to
> make PCI address 0 valid.)

I'm not sure this would work for SPARC64 since potentially OpenBIOS can
program the I/O BAR for the ebus anywhere (and the NVRAM is located on
the ebus). At the moment we cheat by creating an alias to I/O space at
the top of memory so that OpenBIOS can always access it at a fixed address.


ATB,

Mark.
Peter Maydell Jan. 19, 2015, 4:50 p.m. UTC | #15
On 19 January 2015 at 16:48, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> I'm not sure this would work for SPARC64 since potentially OpenBIOS can
> program the I/O BAR for the ebus anywhere (and the NVRAM is located on
> the ebus). At the moment we cheat by creating an alias to I/O space at
> the top of memory so that OpenBIOS can always access it at a fixed address.

The MemoryRegion APIs should provide enough flexibility that you
can make QEMU do whatever the hardware does here (whether that is
"builtin devices higher priority than PCI window" or vice versa).
All you have to do is figure out what that actually is...

-- PMM
Mark Cave-Ayland Jan. 19, 2015, 4:55 p.m. UTC | #16
On 19/01/15 15:31, Paolo Bonzini wrote:

> On 19/01/2015 16:22, Artyom Tarasenko wrote:
>>>>>>
>>>>>> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>>>>>> So, I think modelling it as ISA is closer to to the reality.
>>>>>> But out of curiosity, would it be possible to have a sysbus device
>>>>>> somewhere in a middle of PCI space? [...]
>>>>
>>>> Why would you want to use a SysBusDevice in the first place?
>> Ask Paolo. :-) For me it's only important to have a MMIO device in the
>> proper address range.
> 
> The reason I asked is simply because ISA devices never do MMIO (apart
> for the VGA window).
> 
>>>> I previously discussed with Mark that it should be an EBusDevice, not an
>>>> ISADevice or SysBusDevice.
>> Interesting. I can't find this discussion in the list archive. Do you suggest to
>> create EBusDevices for all ISA devices (serial, parallel, keyboard,
>> floppy) used in sun4u, or only for m48t59?
>> What would be the advantage of using EBusDevice over ISADevice?
> 
> Is there a description of EBus and the sun4u memory map somewhere?

There are sample device trees from real hardware floating around, and
also snippets from various parts of Sun documentation (although sadly
Oracle have removed these now).

> Is there an "EBus bridge" PCI device similar to the PCI-to-ISA bridge?

Yes, that describes it pretty well. Also we can't just attach the NVRAM
to sysbus directly without some custom hacking to the OpenBIOS PCI
enumeration code as some OSs (particularly the *BSDs) locate the Ebus by
traversing down the PCI bus, locating the bridge device, and then
generating the physical addresses from calculating offsets from the
relevant address/reg properties.


ATB,

Mark.
Mark Cave-Ayland Jan. 19, 2015, 4:57 p.m. UTC | #17
On 19/01/15 16:17, Artyom Tarasenko wrote:

>> Is there an "EBus bridge" PCI device similar to the PCI-to-ISA bridge?
> 
> As physical devices there are integrated SBus-to-EBus and PCI-to-EBus bridges.
> 
> But actually I may have been wrong about NVRAM always sitting on the
> EBus: looking at the page 28 of "UltraSPARC™-IIi User's Manual"
> (805-0087.pdf), I see that NVRAM, Serial and other controllers reside
> in a "PC compatible SuperIO" chip which sits on a PCI bus.

There's definitely a PCI device for a PCI-EBus bridge that we discover
during enumeration and attach to that (for example the EBus properties
in OpenBIOS can be found in the attach callback for the bridge PCI
device in drivers/pci.c).


ATB,

Mark.
Maciej W. Rozycki Jan. 19, 2015, 6:17 p.m. UTC | #18
On Mon, 19 Jan 2015, Paolo Bonzini wrote:

> >> The reason I asked is simply because ISA devices never do MMIO (apart
> >> for the VGA window).
> > 
> > You mean in the QEMU world? At least physical SCSI and Ethernet
> > adapters had a MMIO space for the onboard ROM.
> 
> Uh right, ROMs count as MMIO too.

 Some ISA Ethernet cards also used MMIO for r/w access, probably to get at 
packet memory more efficiently (I don't remember the details offhand) as 
port I/O transactions were notoriously slow; in any case this is where the 
"Memory" field printed by `ifconfig' under Linux comes from.  I'm sure 
there was other ISA equipment too using MMIO for one purpose or another.

 On a PC/AT class x86 computer these resources would normally be allocated 
somehow to the memory space in the 0xd0000-0xeffff range, to work with 
real-mode software.  With "somehow" usually meaning jumpers, though newer 
cards may have had DOS configuration software available to set it up, in a 
similar manner to how ECU configured port I/O and MMIO resources for EISA 
equipment.

 BTW there were ISA DRAM expansion cards in existence too.

  Maciej
Andreas Färber Jan. 19, 2015, 8:03 p.m. UTC | #19
Am 19.01.2015 um 16:22 schrieb Artyom Tarasenko:
> On Mon, Jan 19, 2015 at 4:01 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 19.01.2015 um 13:57 schrieb Artyom Tarasenko:
>>> On Mon, Jan 19, 2015 at 1:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> On 19/01/2015 12:35, Mark Cave-Ayland wrote:
>>>>> Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
>>>>> MMIO rather than ioport if required.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>
>>>> Is it really ISA if it's MMIO?  In other words, why can't this be a
>>>> sysbus device?
>>>
>>> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>>> So, I think modelling it as ISA is closer to to the reality.
>>> But out of curiosity, would it be possible to have a sysbus device
>>> somewhere in a middle of PCI space? [...]
>>
>> Why would you want to use a SysBusDevice in the first place?
> 
> Ask Paolo. :-) For me it's only important to have a MMIO device in the
> proper address range.
> 
>> I previously discussed with Mark that it should be an EBusDevice, not an
>> ISADevice or SysBusDevice.
> 
> Interesting. I can't find this discussion in the list archive.

Hm, am I mixing that up with SBus then? There were some helper functions
related to ROM loading being added as context to my suggestion that I
thought could become class fields.

> Do you suggest to
> create EBusDevices for all ISA devices (serial, parallel, keyboard,
> floppy) used in sun4u, or only for m48t59?
> What would be the advantage of using EBusDevice over ISADevice?

For all devices that are in fact EBus devices. The general idea is
encapsulation and abstraction - hiding the implementation detail of
whether it is internally an ISADevice or something else. Such a patch
should be quite trivial. Similarly it gives helper functions and
potential class methods a natural place to live.

Andreas
Hervé Poussineau Jan. 19, 2015, 9:16 p.m. UTC | #20
Le 19/01/2015 16:01, Andreas Färber a écrit :
> Also, wasn't Hervé's(?) plan to get rid of mem_base completely by always
> passing a pointer to ISADevice/ISABus around? It should only be needed
> when somewhere NULL is being passed, no?

Yes, I've a patch series which is removing the isa_mem_base variable. I'll send it on ML.
ISA functions already take either a ISABus or a ISADevice, so they are good.
Next step would be to allow multiple ISA busses in hw/isa/isa-bus.c

Regards,

Hervé
Artyom Tarasenko Jan. 20, 2015, 9:54 a.m. UTC | #21
On Mon, Jan 19, 2015 at 9:03 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 19.01.2015 um 16:22 schrieb Artyom Tarasenko:
>> On Mon, Jan 19, 2015 at 4:01 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 19.01.2015 um 13:57 schrieb Artyom Tarasenko:
>>>> On Mon, Jan 19, 2015 at 1:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> On 19/01/2015 12:35, Mark Cave-Ayland wrote:
>>>>>> Similar to m48t59_init(), add a mem_base value so that NVRAM can be mapped via
>>>>>> MMIO rather than ioport if required.
>>>>>>
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>> ---
>>>>>
>>>>> Is it really ISA if it's MMIO?  In other words, why can't this be a
>>>>> sysbus device?
>>>>
>>>> On physical machines it's EBus, which is pretty much like 8-bit ISA.
>>>> So, I think modelling it as ISA is closer to to the reality.
>>>> But out of curiosity, would it be possible to have a sysbus device
>>>> somewhere in a middle of PCI space? [...]
>>>
>>> Why would you want to use a SysBusDevice in the first place?
>>
>> Ask Paolo. :-) For me it's only important to have a MMIO device in the
>> proper address range.
>>
>>> I previously discussed with Mark that it should be an EBusDevice, not an
>>> ISADevice or SysBusDevice.
>>
>> Interesting. I can't find this discussion in the list archive.
>
> Hm, am I mixing that up with SBus then? There were some helper functions
> related to ROM loading being added as context to my suggestion that I
> thought could become class fields.

Yes, for SBus it totally makes sense.

>> Do you suggest to
>> create EBusDevices for all ISA devices (serial, parallel, keyboard,
>> floppy) used in sun4u, or only for m48t59?
>> What would be the advantage of using EBusDevice over ISADevice?
>
> For all devices that are in fact EBus devices. The general idea is
> encapsulation and abstraction - hiding the implementation detail of
> whether it is internally an ISADevice or something else. Such a patch
> should be quite trivial. Similarly it gives helper functions and
> potential class methods a natural place to live.

Yes, the patch is trivial. But EBus is nothing else but an ISA bus
with 8 data lines.
To me it looks like the bus width is an implementation detail we can
hide. (It's not documented what should happen if an EBus device is
accessed with a non 8-bit width. I tried 16 bit access on a physical
machine and it seemed to work).

Currently we can just use all the ISA devices unmodified if we like.
With EBus we would only be able to use a subset of ISA devices, no?

Artyom
diff mbox

Patch

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 378a368..4ed22ba 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -543,7 +543,7 @@  static void ppc_prep_init(MachineState *machine)
         pci_create_simple(pci_bus, -1, "pci-ohci");
     }
 
-    m48t59 = m48t59_init_isa(isa_bus, 0x0074, NVRAM_SIZE, 0, 59);
+    m48t59 = m48t59_init_isa(isa_bus, 0, 0x0074, NVRAM_SIZE, 0, 59);
     if (m48t59 == NULL)
         return;
     sysctrl->nvram = m48t59;
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 6b46511..86f5861 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -873,7 +873,7 @@  static void sun4uv_init(MemoryRegion *address_space_mem,
         fd[i] = drive_get(IF_FLOPPY, 0, i);
     }
     fdctrl_init_isa(isa_bus, fd);
-    nvram = m48t59_init_isa(isa_bus, 0x0074, NVRAM_SIZE, 0, 59);
+    nvram = m48t59_init_isa(isa_bus, 0, 0x0074, NVRAM_SIZE, 0, 59);
 
     initrd_size = 0;
     initrd_addr = 0;
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index 0a05100..dc52ce3 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -664,8 +664,8 @@  M48t59State *m48t59_init(qemu_irq IRQ, hwaddr mem_base, uint32_t io_base,
     return state;
 }
 
-M48t59State *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size,
-                             uint32_t year_offset, int model)
+M48t59State *m48t59_init_isa(ISABus *bus, hwaddr mem_base, uint32_t io_base,
+                             uint16_t size, uint32_t year_offset, int model)
 {
     M48t59ISAState *d;
     ISADevice *isadev;
@@ -686,6 +686,11 @@  M48t59State *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size,
     if (io_base != 0) {
         isa_register_ioport(isadev, &d->io, io_base);
     }
+    if (mem_base != 0) {
+        memory_region_init_io(&s->iomem, OBJECT(d), &nvram_ops, s,
+                              "m48t59.nvram", s->size);
+        isa_register_ioport(isadev, &s->iomem, mem_base);
+    }
 
     return s;
 }
diff --git a/include/hw/timer/m48t59.h b/include/hw/timer/m48t59.h
index 08252b6..4bcd8fc 100644
--- a/include/hw/timer/m48t59.h
+++ b/include/hw/timer/m48t59.h
@@ -29,8 +29,8 @@  typedef struct M48t59State M48t59State;
 void m48t59_write (void *private, uint32_t addr, uint32_t val);
 uint32_t m48t59_read (void *private, uint32_t addr);
 void m48t59_toggle_lock (void *private, int lock);
-M48t59State *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size,
-                             uint32_t year_offset, int type);
+M48t59State *m48t59_init_isa(ISABus *bus, hwaddr mem_base, uint32_t io_base,
+                             uint16_t size, uint32_t year_offset, int type);
 M48t59State *m48t59_init(qemu_irq IRQ, hwaddr mem_base,
                          uint32_t io_base, uint16_t size,
                          uint32_t year_offset, int type);