[ipxe-devel] [PATCH 1/2] [pci] Add enable_pci_device and disable_pci_device
Michael S. Tsirkin
mst at redhat.com
Mon May 2 11:30:23 UTC 2016
On Mon, May 02, 2016 at 09:39:34AM +0200, Ladi Prosek wrote:
> On Sun, May 1, 2016 at 2:24 PM, Marcel Apfelbaum <marcel at redhat.com> wrote:
> > Hi Ladi,
> >
> > On 04/28/2016 06:34 PM, Ladi Prosek wrote:
> >>
> >> adjust_pci_device unconditionally enables both memory and I/O space
> >> access, which may not be necessary.
> >
> >
> > The above problem is not addressed in this patch,
> > people my get confused.
>
> I'll fix the commit message, thanks!
>
> > There was also no way to revert
> >>
> >> the device back to the original state.
> >
> >
> >>
> >> This commit adds two new functions. enable_pci_device gives drivers
> >> finer control over how the PCI device is initialized and
> >> disable_pci_device is the shutdown path counterpart.
> >>
> >> Signed-off-by: Ladi Prosek <lprosek at redhat.com>
> >> Suggested-by: Michael S. Tsirkin <mst at redhat.com>
> >> ---
> >> src/drivers/bus/pci.c | 41 +++++++++++++++++++++++++++++++++++------
> >> src/include/ipxe/pci.h | 2 ++
> >> 2 files changed, 37 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/drivers/bus/pci.c b/src/drivers/bus/pci.c
> >> index 6fbedd9..bdff2b9 100644
> >> --- a/src/drivers/bus/pci.c
> >> +++ b/src/drivers/bus/pci.c
> >> @@ -143,16 +143,32 @@ static void pci_read_bases ( struct pci_device *pci
> >> ) {
> >> *
> >> * @v pci PCI device
> >> *
> >> - * Set device to be a busmaster in case BIOS neglected to do so. Also
> >> - * adjust PCI latency timer to a reasonable value, 32.
> >> + * Set device to be a busmaster in case BIOS neglected to do so. Enable
> >
> >
> > You can use the opportunity for changing busmaster to 'bus master' or 'Bus
> > Master'
>
> Will do.
>
> >> + * both memory and I/O space access. Also adjust PCI latency timer to a
> >> + * reasonable value, 32.
> >
> >
> > Adjusting the latency to 32 is done by an inner function, you may want to
> > remove
> > this comment here.
>
> Hmm.. not sure about this one. It's still done by the function,
> doesn't matter if directly or somewhere down the calltree, why would
> you not want it documented in the comment?
>
> By the way, I noticed that the latency timer writes don't stick. Is it
> expected for virtual devices to have a read-only implementation of
> this register?
It's legal for it to be RO.
Latency Timer
This register specifies, in units of PCI bus clocks, the value of the
Latency Timer for this
PCI bus master (refer to Section 3.5.4). This register must be
implemented as writable by
any master that can burst more than two data phases. This register may
be implemented as
read-only for devices that burst two or fewer data phases, but the
hardwired value must be
limited to 16 or less. A typical implementation would be to build the
five high-order bits
(leaving the bottom three as read-only), resulting in a timer
granularity of eight clocks. At
RST# , the register must be initialized to 0 (if programmable).
> >> */
> >> void adjust_pci_device ( struct pci_device *pci ) {
> >> + enable_pci_device ( pci,
> >> + PCI_COMMAND_MASTER | PCI_COMMAND_MEM |
> >> + PCI_COMMAND_IO );
> >> +}
> >> +
> >> +/**
> >> + * Enable PCI device
> >> + *
> >> + * @v pci PCI device
> >> + * @v flags PCI command flags to enable
> >> + * @ret old_flags Original PCI command flags
> >> + *
> >> + * Enable device in case BIOS neglected to do so. Also adjust PCI
> >> latency
> >> + * timer to a reasonable value, 32.
> >> + */
> >> +unsigned enable_pci_device ( struct pci_device *pci, unsigned flags ) {
> >> unsigned short new_command, pci_command;
> >> unsigned char pci_latency;
> >>
> >> pci_read_config_word ( pci, PCI_COMMAND, &pci_command );
> >> - new_command = ( pci_command | PCI_COMMAND_MASTER |
> >> - PCI_COMMAND_MEM | PCI_COMMAND_IO );
> >> + new_command = ( pci_command | flags );
> >> if ( pci_command != new_command ) {
> >> DBGC ( pci, PCI_FMT " device not enabled by BIOS! Updating
> >> "
> >> "PCI command %04x->%04x\n",
> >
> >
> > The debug message is not true anymore. Maybe the BIOS enabled IO and now we
> > want to enable also MEM.
>
> Will fix, thanks.
>
> >> @@ -160,12 +176,25 @@ void adjust_pci_device ( struct pci_device *pci ) {
> >> pci_write_config_word ( pci, PCI_COMMAND, new_command );
> >> }
> >>
> >> - pci_read_config_byte ( pci, PCI_LATENCY_TIMER, &pci_latency);
> >> + pci_read_config_byte ( pci, PCI_LATENCY_TIMER, &pci_latency );
> >> if ( pci_latency < 32 ) {
> >> DBGC ( pci, PCI_FMT " latency timer is unreasonably low at
> >> "
> >> "%d. Setting to 32.\n", PCI_ARGS ( pci ),
> >> pci_latency );
> >> - pci_write_config_byte ( pci, PCI_LATENCY_TIMER, 32);
> >> + pci_write_config_byte ( pci, PCI_LATENCY_TIMER, 32 );
> >> }
> >> + return pci_command;
> >> +}
> >> +
> >> +/**
> >> + * Disable PCI device
> >> + *
> >> + * @v pci PCI device
> >> + * @v old_flags Flags to restore, returned from
> >> enable_pci_device
> >> + *
> >> + * Restore device to state it had before we enabled it.
> >> + */
> >> +void disable_pci_device ( struct pci_device *pci, unsigned old_flags ) {
> >> + pci_write_config_word ( pci, PCI_COMMAND, old_flags );
> >> }
> >
> >
> > I have a slight problem with the name of the function which does no really
> > reflects what the function does. If we look at the debug message example,
> > it is possible the device remains enabled, but only using IO.
> >
> > A similar but smaller issue has 'enable_pci_device. You don't check
> > that at least an "enabling" bit is passed.
> >
> > I would suggest renaming this function to something like 'pci_restore_cmd'
> > or simply call pci_write_config_word directly.
>
> I'll rename them, thanks.
>
> >
> >>
> >> /**
> >> diff --git a/src/include/ipxe/pci.h b/src/include/ipxe/pci.h
> >> index 0c6bc7e..3c613c2 100644
> >> --- a/src/include/ipxe/pci.h
> >> +++ b/src/include/ipxe/pci.h
> >> @@ -278,6 +278,8 @@ struct pci_driver {
> >> PCI_FUNC ( (pci)->busdevfn )
> >>
> >> extern void adjust_pci_device ( struct pci_device *pci );
> >> +extern unsigned enable_pci_device ( struct pci_device *pci, unsigned
> >> flags );
> >> +extern void disable_pci_device ( struct pci_device *pci, unsigned flags
> >> );
> >> extern unsigned long pci_bar_start ( struct pci_device *pci,
> >> unsigned int reg );
> >> extern int pci_read_config ( struct pci_device *pci );
> >>
> >
More information about the ipxe-devel
mailing list