Hi, thanks. Some nitpicks below: On Sun, Jun 21, 2020 at 7:40 AM Waldemar Kozaczuk <[email protected]> wrote:
> This patch extracts generic logic of serial console into a base class > isa-serial-base that abstracts reading and writing a byte. The > specializations > like isa-serial (add mmio-isa-serial in later patch) provide specific way > of > reading and writing a byte in form of lambda functions. > > Signed-off-by: Waldemar Kozaczuk <[email protected]> > --- > Makefile | 1 + > drivers/isa-serial-base.cc | 85 ++++++++++++++++++++++++++++ > drivers/isa-serial-base.hh | 76 +++++++++++++++++++++++++ > drivers/isa-serial.cc | 110 +++---------------------------------- > drivers/isa-serial.hh | 17 +----- > 5 files changed, 174 insertions(+), 115 deletions(-) > create mode 100644 drivers/isa-serial-base.cc > create mode 100644 drivers/isa-serial-base.hh > > diff --git a/Makefile b/Makefile > index 20ddf3b1..99923a34 100644 > --- a/Makefile > +++ b/Makefile > @@ -776,6 +776,7 @@ drivers += drivers/line-discipline.o > drivers += drivers/clock.o > drivers += drivers/clock-common.o > drivers += drivers/clockevent.o > +drivers += drivers/isa-serial-base.o > drivers += core/elf.o > drivers += drivers/random.o > drivers += drivers/zfs.o > diff --git a/drivers/isa-serial-base.cc b/drivers/isa-serial-base.cc > new file mode 100644 > index 00000000..19358f0b > --- /dev/null > +++ b/drivers/isa-serial-base.cc > @@ -0,0 +1,85 @@ > +/* > + * Copyright (C) 2020 Waldemar Kozaczuk. > + * > + * This work is open source software, licensed under the terms of the > + * BSD license as described in the LICENSE file in the top-level > directory. > + */ > + > +#include "isa-serial-base.hh" > + > +namespace console { > + > +void isa_serial_console_base::common_early_init( > + const std::function<u8 (const int&)> _read_byte, > + const std::function<void (const u8&, const int&)> _write_byte) > +{ > + read_byte = _read_byte; > + write_byte = _write_byte; > + > + // Set the UART speed to to 115,200 bps, This is done by writing 1,0 > to > + // Divisor Latch registers, but to access these we need to temporarily > + // set the Divisor Latch Access Bit (DLAB) on the LSR register, > because > + // the UART has fewer ports than registers... > + write_byte(lcr::LEN_8BIT | lcr::DLAB, regs::LCR); > + write_byte(1, regs::DLL); > + write_byte(0, regs::DLM); > + write_byte(lcr::LEN_8BIT, regs::LCR); > + > + // interrupt threshold > + write_byte(0, regs::FCR); > + > + // disable interrupts > + write_byte(0, regs::IER); > + > + // Most physical UARTs need the MCR AUX_OUTPUT_2 bit set to 1 for > + // interrupts to be generated. QEMU doesn't bother checking this > + // bit, but interestingly VMWare does, so we must set it. > + write_byte(mcr::AUX_OUTPUT_2, regs::MCR); > +} > + > +void isa_serial_console_base::write(const char *str, size_t len) > +{ > + while (len-- > 0) > + putchar(*str++); > +} > + > +bool isa_serial_console_base::input_ready() > +{ > + u8 val = read_byte(regs::LSR); > + // On VMWare hosts without a serial port, this register always > + // returns 0xff. Just ignore it instead of spinning incessantly. > + return (val != 0xff && (val & lsr::RECEIVE_DATA_READY)); > +} > + > +char isa_serial_console_base::readch() > +{ > + u8 val; > + char letter; > + > + do { > + val = read_byte(regs::LSR); > + } while (!(val & (lsr::RECEIVE_DATA_READY | lsr::OVERRUN | > lsr::PARITY_ERROR | lsr::FRAME_ERROR))); > + > + letter = read_byte(0); > + > + return letter; > +} > + > +void isa_serial_console_base::putchar(const char ch) > +{ > + u8 val; > + > + do { > + val = read_byte(regs::LSR); > + } while (!(val & lsr::TRANSMIT_HOLD_EMPTY)); > + > + write_byte(ch, 0); > +} > + > +void isa_serial_console_base::enable_interrupt() > +{ > + // enable interrupts > + write_byte(1, regs::IER); > +} > + > +} > diff --git a/drivers/isa-serial-base.hh b/drivers/isa-serial-base.hh > new file mode 100644 > index 00000000..f518b366 > --- /dev/null > +++ b/drivers/isa-serial-base.hh > @@ -0,0 +1,76 @@ > +/* > + * Copyright (C) 2020 Waldemar Kozaczuk. > + * > + * This work is open source software, licensed under the terms of the > + * BSD license as described in the LICENSE file in the top-level > directory. > + */ > + > +#ifndef DRIVERS_ISA_SERIAL_BASE_HH > +#define DRIVERS_ISA_SERIAL_BASE_HH > + > +#include "console-driver.hh" > +#include <osv/pci.hh> > +#include <osv/sched.hh> > +#include <osv/interrupt.hh> > + > +namespace console { > + > +// UART registers, offsets to ioport: > +enum regs { > + IER = 1, // Interrupt Enable Register > + FCR = 2, // FIFO Control Register > + LCR = 3, // Line Control Register > + MCR = 4, // Modem Control Register > + LSR = 5, // Line Control Register > + MSR = 6, // Modem Status Register > + SCR = 7, // Scratch Register > + DLL = 0, // Divisor Latch LSB Register > + DLM = 1, // Divisor Latch MSB Register > +}; > All of this stuff was previously in the source file, not the header file. Why do we need to move it to the header file? Will the different ISA and MMIO implementations use these definitions and the ones below? + > +enum lcr { > + // When bit 7 (DLAB) of LCR is set to 1, the two registers 0 and 1 > + // change their meaning and become two bytes controlling the baud rate > + DLAB = 0x80, // Divisor Latch Access Bit in LCR register > + LEN_8BIT = 3, > +}; > + > +// Various bits of the Line Status Register > +enum lsr { > + RECEIVE_DATA_READY = 0x1, > + OVERRUN = 0x2, > + PARITY_ERROR = 0x4, > + FRAME_ERROR = 0x8, > + BREAK_INTERRUPT = 0x10, > + TRANSMIT_HOLD_EMPTY = 0x20, > + TRANSMIT_EMPTY = 0x40, > + FIFO_ERROR = 0x80, > +}; > + > +// Various bits of the Modem Control Register > +enum mcr { > + DTR = 0x1, > + RTS = 0x2, > + AUX_OUTPUT_1 = 0x4, > + AUX_OUTPUT_2 = 0x8, > + LOOPBACK_MODE = 0x16, > +}; > + > +class isa_serial_console_base : public console_driver { > +public: > + virtual void write(const char *str, size_t len); > + virtual void flush() {} > + virtual bool input_ready() override; > + virtual char readch(); > +protected: > + static void common_early_init(const std::function<u8 (const int&)> > _read_byte, > + const std::function<void (const u8&, const int&)> > _write_byte); > + void enable_interrupt(); > +private: > + static std::function<u8 (const int&)> read_byte; > + static std::function<void (const u8&, const int&)> write_byte; > Why are these things "static"? It seems you set them in the constructor of a specific instance? An unrelated question is do we really need std::function? If you don't plan to capture anything into this function, it could be a simple function pointer insted of std::function. Finally, why "const int&" and "const u8&" instead of simply, "int", "u8"? > + void putchar(const char ch); > +}; > +} > + > +#endif > diff --git a/drivers/isa-serial.cc b/drivers/isa-serial.cc > index b20b30f3..16da3736 100644 > --- a/drivers/isa-serial.cc > +++ b/drivers/isa-serial.cc > @@ -9,113 +9,21 @@ > > namespace console { > > -// UART registers, offsets to ioport: > -enum regs { > - IER = 1, // Interrupt Enable Register > - FCR = 2, // FIFO Control Register > - LCR = 3, // Line Control Register > - MCR = 4, // Modem Control Register > - LSR = 5, // Line Control Register > - MSR = 6, // Modem Status Register > - SCR = 7, // Scratch Register > - DLL = 0, // Divisor Latch LSB Register > - DLM = 1, // Divisor Latch MSB Register > +std::function<u8 (const int&)> isa_serial_console_base::read_byte = > [](const int& reg) { > + return pci::inb(isa_serial_console::ioport + reg); > }; > > -enum lcr { > - // When bit 7 (DLAB) of LCR is set to 1, the two registers 0 and 1 > - // change their meaning and become two bytes controlling the baud rate > - DLAB = 0x80, // Divisor Latch Access Bit in LCR register > - LEN_8BIT = 3, > -}; > - > -// Various bits of the Line Status Register > -enum lsr { > - RECEIVE_DATA_READY = 0x1, > - OVERRUN = 0x2, > - PARITY_ERROR = 0x4, > - FRAME_ERROR = 0x8, > - BREAK_INTERRUPT = 0x10, > - TRANSMIT_HOLD_EMPTY = 0x20, > - TRANSMIT_EMPTY = 0x40, > - FIFO_ERROR = 0x80, > -}; > - > -// Various bits of the Modem Control Register > -enum mcr { > - DTR = 0x1, > - RTS = 0x2, > - AUX_OUTPUT_1 = 0x4, > - AUX_OUTPUT_2 = 0x8, > - LOOPBACK_MODE = 0x16, > +std::function<void (const u8&, const int&)> > isa_serial_console_base::write_byte = [](const u8& val, const int& reg) { > + pci::outb(val, isa_serial_console::ioport + reg); > }; > > void isa_serial_console::early_init() > { > - // Set the UART speed to to 115,200 bps, This is done by writing 1,0 > to > - // Divisor Latch registers, but to access these we need to temporarily > - // set the Divisor Latch Access Bit (DLAB) on the LSR register, > because > - // the UART has fewer ports than registers... > - pci::outb(lcr::LEN_8BIT | lcr::DLAB, ioport + regs::LCR); > - pci::outb(1, ioport + regs::DLL); > - pci::outb(0, ioport + regs::DLM); > - pci::outb(lcr::LEN_8BIT, ioport + regs::LCR); > - > - // interrupt threshold > - pci::outb(0, ioport + regs::FCR); > - > - // disable interrupts > - pci::outb(0, ioport + regs::IER); > - > - // Most physical UARTs need the MCR AUX_OUTPUT_2 bit set to 1 for > - // interrupts to be generated. QEMU doesn't bother checking this > - // bit, but interestingly VMWare does, so we must set it. > - pci::outb(mcr::AUX_OUTPUT_2, ioport + regs::MCR); > -} > - > -void isa_serial_console::write(const char *str, size_t len) > -{ > - while (len-- > 0) > - putchar(*str++); > -} > - > -bool isa_serial_console::input_ready() > -{ > - u8 val = pci::inb(ioport + regs::LSR); > - // On VMWare hosts without a serial port, this register always > - // returns 0xff. Just ignore it instead of spinning incessantly. > - return (val != 0xff && (val & lsr::RECEIVE_DATA_READY)); > -} > - > -char isa_serial_console::readch() > -{ > - u8 val; > - char letter; > - > - do { > - val = pci::inb(ioport + regs::LSR); > - } while (!(val & (lsr::RECEIVE_DATA_READY | lsr::OVERRUN | > lsr::PARITY_ERROR | lsr::FRAME_ERROR))); > - > - letter = pci::inb(ioport); > - > - return letter; > -} > - > -void isa_serial_console::putchar(const char ch) > -{ > - u8 val; > - > - do { > - val = pci::inb(ioport + regs::LSR); > - } while (!(val & lsr::TRANSMIT_HOLD_EMPTY)); > - > - pci::outb(ch, ioport); > -} > - > -void isa_serial_console::enable_interrupt() > -{ > - // enable interrupts > - pci::outb(1, ioport + regs::IER); > + common_early_init( [](const int& reg) { > + return pci::inb(ioport + reg); > + }, [](const u8& val, const int& reg) { > + pci::outb(val, ioport + reg); > + }); > Here I see no capture, so it could have been a normal function pointer instead of std::function. } > > void isa_serial_console::dev_start() { > diff --git a/drivers/isa-serial.hh b/drivers/isa-serial.hh > index e31d125a..d7908be2 100644 > --- a/drivers/isa-serial.hh > +++ b/drivers/isa-serial.hh > @@ -8,30 +8,19 @@ > #ifndef DRIVERS_ISA_SERIAL_HH > #define DRIVERS_ISA_SERIAL_HH > > -#include "console-driver.hh" > -#include <osv/pci.hh> > -#include <osv/sched.hh> > -#include <osv/interrupt.hh> > +#include "isa-serial-base.hh" > > namespace console { > > -class isa_serial_console : public console_driver { > +class isa_serial_console : public isa_serial_console_base { > public: > static void early_init(); > - virtual void write(const char *str, size_t len); > - virtual void flush() {} > - virtual bool input_ready() override; > - virtual char readch(); > + static const u16 ioport = 0x3f8; > private: > std::unique_ptr<gsi_edge_interrupt> _irq; > - static const u16 ioport = 0x3f8; > - > virtual void dev_start(); > - void enable_interrupt(); > - static void putchar(const char ch); > virtual const char *thread_name() { return "isa-serial-input"; } > }; > - > } > > #endif > -- > 2.25.1 > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/osv-dev/20200621044018.18378-1-jwkozaczuk%40gmail.com > . > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjvj1HK-gAfqM-yDO%2BL03Qvx5trAy_n%2BDHgH8cxAB_kvUQ%40mail.gmail.com.
