[haiku-commits] haiku: hrev52858 - src/add-ons/kernel/busses/mmc

  • From: waddlesplash <waddlesplash@xxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Mon, 11 Feb 2019 16:36:31 -0500 (EST)

hrev52858 adds 3 changesets to branch 'master'
old head: f167d21adcea20341d2080cc307f405dad82803c
new head: 601c0fcae2a23ce37841829b4861653e5d13acd9
overview: 
https://git.haiku-os.org/haiku/log/?qt=range&q=601c0fcae2a2+%5Ef167d21adcea

----------------------------------------------------------------------------

cdc4a175caaf: sdhci: refactor, get CMD0 working
  
  - Define a class for each register, allowing easier access to relevant
    values. This avoids dealing with bitshifts and magic constants all
    over the code.
  - Fix definition of CMD0 and the flags used to submit it. We now get a
    command completion interrupt, yay!
  - Make some changes to support v3 and v4 controllers:
    - Enable PLL (harmless for older versions)
    - Manage faster and more configurable clock settings
  
  Change-Id: I8a97edcb881acc1ac2a8b0a2593930f18e777594
  Reviewed-on: https://review.haiku-os.org/c/1029
  Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>

3aef22f7c8c6: sdhci: implement uninit_bus
  
  Also avoid leaking resources in case initialization fails.
  
  Change-Id: Ia533182eeeb81b7d52b49510b1f375a4fc55258f
  Reviewed-on: https://review.haiku-os.org/c/1030
  Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>

601c0fcae2a2: sdhci: silently handle empty interrupts
  
  On my system the interrupt line is shared with the PS/2 controller, so
  there are a lot of unhandled interrupts. Just silently ignore them.
  
  Change-Id: Ia6812a5a1d78907622ddebbd03165ed016e26e26
  Reviewed-on: https://review.haiku-os.org/c/1031
  Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>

                             [ Adrien Destugues <pulkomandy@xxxxxxxxxxxxx> ]

----------------------------------------------------------------------------

2 files changed, 287 insertions(+), 153 deletions(-)
src/add-ons/kernel/busses/mmc/sdhci_pci.cpp | 251 +++++++++++++-----------
src/add-ons/kernel/busses/mmc/sdhci_pci.h   | 189 ++++++++++++++----

############################################################################

Commit:      cdc4a175caaff3afe689057e7345a2ab06295dc3
URL:         https://git.haiku-os.org/haiku/commit/?id=cdc4a175caaf
Author:      Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
Date:        Sun Feb 10 16:47:40 2019 UTC
Committer:   waddlesplash <waddlesplash@xxxxxxxxx>
Commit-Date: Mon Feb 11 21:36:25 2019 UTC

sdhci: refactor, get CMD0 working

- Define a class for each register, allowing easier access to relevant
  values. This avoids dealing with bitshifts and magic constants all
  over the code.
- Fix definition of CMD0 and the flags used to submit it. We now get a
  command completion interrupt, yay!
- Make some changes to support v3 and v4 controllers:
  - Enable PLL (harmless for older versions)
  - Manage faster and more configurable clock settings

Change-Id: I8a97edcb881acc1ac2a8b0a2593930f18e777594
Reviewed-on: https://review.haiku-os.org/c/1029
Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>

----------------------------------------------------------------------------

diff --git a/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp 
b/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
index ccf1e3a4f6..bec3eb9719 100644
--- a/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
+++ b/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
@@ -59,33 +59,38 @@ static pci_x86_module_info* sPCIx86Module;
 static void
 sdhci_register_dump(uint8_t slot, struct registers* regs)
 {
+#ifdef TRACE_SDHCI
        TRACE("Register values for slot %d:\n", slot);
        TRACE("system_address: %d\n", regs->system_address);
        TRACE("%d blocks of size %d\n", regs->block_count, regs->block_size);
        TRACE("argument: %d\n", regs->argument);
        TRACE("transfer_mode: %d\n", regs->transfer_mode);
-       TRACE("command: %d\n", regs->command);
+       TRACE("command: %d\n", regs->command.Bits());
        TRACE("response:");
        for (int i = 0; i < 8; i++)
-               TRACE(" %d", regs->response[i]);
-       TRACE("\nbuffer_data_port: %d\n", regs->buffer_data_port);
-       TRACE("present_state: %d\n", regs->present_state);
-       TRACE("power_control: %d\n", regs->power_control);
+               dprintf(" %d", regs->response[i]);
+       dprintf("\n");
+       TRACE("buffer_data_port: %d\n", regs->buffer_data_port);
+       TRACE("present_state: %x\n", regs->present_state.Bits());
+       TRACE("power_control: %d\n", regs->power_control.Bits());
        TRACE("host_control: %d\n", regs->host_control);
        TRACE("wakeup_control: %d\n", regs->wakeup_control);
        TRACE("block_gap_control: %d\n", regs->block_gap_control);
-       TRACE("clock_control: %d\n", regs->clock_control);
-       TRACE("software_reset: %d\n", regs->software_reset);
+       TRACE("clock_control: %x\n", regs->clock_control.Bits());
+       TRACE("software_reset: %d\n", regs->software_reset.Bits());
        TRACE("timeout_control: %d\n", regs->timeout_control);
        TRACE("interrupt_status: %x enable: %x signal: %x\n",
                regs->interrupt_status, regs->interrupt_status_enable,
                regs->interrupt_signal_enable);
        TRACE("auto_cmd12_error_status: %d\n", regs->auto_cmd12_error_status);
-       TRACE("capabilities: %lld\n", regs->capabilities);
+       TRACE("capabilities: %lld\n", regs->capabilities.Bits());
        TRACE("max_current_capabilities: %lld\n",
                regs->max_current_capabilities);
        TRACE("slot_interrupt_status: %d\n", regs->slot_interrupt_status);
-       TRACE("host_controller_version %x\n", regs->host_controller_version);
+       TRACE("host_controller_version spec %x vendor %x\n",
+               regs->host_controller_version.specVersion,
+               regs->host_controller_version.vendorVersion);
+#endif
 }
 
 
@@ -93,73 +98,85 @@ static void
 sdhci_reset(struct registers* regs)
 {
        // if card is not present then no point of reseting the registers
-       if (!(regs->present_state & SDHCI_CARD_DETECT))
+       if (!regs->present_state.IsCardInserted())
                return;
 
        // enabling software reset all
-       regs->software_reset |= SDHCI_SOFTWARE_RESET_ALL;
-
-       // waiting for clock and power to get off
-       while (regs->clock_control != 0 && regs->power_control != 0);
+       regs->software_reset.ResetAll();
 }
 
 
 static void
-sdhci_set_clock(struct registers* regs, uint16_t base_clock_div)
+sdhci_set_clock(struct registers* regs)
 {
-       uint32_t clock_control = regs->clock_control;
-       int base_clock = SDHCI_BASE_CLOCK_FREQ(regs->capabilities);
+       int base_clock = regs->capabilities.BaseClockFrequency();
+       // Try to get as close to 400kHz as possible, but not faster
+       int divider = base_clock * 1000 / 400;
+
+       if (regs->host_controller_version.specVersion <= 1) {
+               // Old controller only support power of two dividers up to 256,
+               // round to next power of two up to 256
+               if (divider > 256)
+                       divider = 256;
+
+               divider--;
+               divider |= divider >> 1;
+               divider |= divider >> 2;
+               divider |= divider >> 4;
+               divider++;
+       }
+
+       divider = regs->clock_control.SetDivider(divider);
 
-       TRACE("SDCLK frequency: %dMHz\n", base_clock);
+       // Log the value after possible rounding by SetDivider (only even values
+       // are allowed).
+       TRACE("SDCLK frequency: %dMHz / %d = %dkHz\n", base_clock, divider,
+               base_clock * 1000 / divider);
 
-       // clearing previous frequency
-       clock_control &= SDHCI_CLR_FREQ_SEL;
-       clock_control |= base_clock_div;
+       // We have set the divider, now we can enable the internal clock.
+       regs->clock_control.EnableInternal();
 
-       // enabling internal clock
-       clock_control |= SDHCI_INTERNAL_CLOCK_ENABLE;
-       regs->clock_control = clock_control;
+       // wait until internal clock is stabilized
+       while (!(regs->clock_control.InternalStable()));
 
-       // waiting till internal clock gets stable
-       while (!(regs->clock_control & SDHCI_INTERNAL_CLOCK_STABLE));
+       regs->clock_control.EnablePLL();
+       while (!(regs->clock_control.InternalStable()));
 
-       regs->clock_control |= SDHCI_SD_CLOCK_ENABLE; // enabling the SD clock
+       // Finally, route the clock to the SD card
+       regs->clock_control.EnableSD();
 }
 
 
 static void
 sdhci_stop_clock(struct registers* regs)
 {
-       regs->clock_control &= SDHCI_SD_CLOCK_DISABLE;
+       regs->clock_control.DisableSD();
 }
 
 
 static void
 sdhci_set_power(struct registers* _regs)
 {
-       uint16_t command = _regs->command;
-
-       if (SDHCI_VOLTAGE_SUPPORTED(_regs->capabilities))
-               if (SDHCI_VOLTAGE_SUPPORTED_33(_regs->capabilities))
-                       _regs->power_control |= SDHCI_VOLTAGE_SUPPORT_33;
-               else if (SDHCI_VOLTAGE_SUPPORTED_30(_regs->capabilities))
-                       _regs->power_control |= SDHCI_VOLTAGE_SUPPORT_30;
-               else
-                       _regs->power_control |= SDHCI_VOLTAGE_SUPPORT_18;
-       else
-               TRACE("No voltage is supported\n");
-
-       if (SDHCI_CARD_INSERTED(_regs->present_state) == 0) {
-               TRACE("Card not inserted\n");
+       uint8_t supportedVoltages = _regs->capabilities.SupportedVoltages();
+       if ((supportedVoltages & Capabilities::k3v3) != 0)
+               _regs->power_control.SetVoltage(PowerControl::k3v3);
+       else if ((supportedVoltages & Capabilities::k3v0) != 0)
+               _regs->power_control.SetVoltage(PowerControl::k3v0);
+       else if ((supportedVoltages & Capabilities::k1v8) != 0)
+               _regs->power_control.SetVoltage(PowerControl::k1v8);
+       else {
+               _regs->power_control.PowerOff();
+               ERROR("No voltage is supported\n");
                return;
        }
 
-       _regs->power_control |= SDHCI_BUS_POWER_ON;
-       TRACE("Executed CMD0\n");
+       if (!_regs->present_state.IsCardInserted()) {
+               TRACE("Card not inserted\n");
+               return;
+       }
 
-       command = SDHCI_RESPONSE_R1 | SDHCI_CMD_CRC_EN
-               | SDHCI_CMD_INDEX_EN | SDHCI_CMD_0;
-       _regs->command |= command;
+       TRACE("Execute CMD0\n");
+       _regs->command.SendCommand(0, false);
 
        DELAY(1000);
 }
@@ -204,6 +221,8 @@ init_bus(device_node* node, void** bus_cookie)
                || gDeviceManager->get_attr_uint8(node, BAR_INDEX, &bar, false) 
< B_OK)
                return -1;
 
+       TRACE("Controller has %d slots, first bar is %d\n", slot + 1, bar);
+
        bus->node = node;
        bus->pci = pci;
        bus->device = device;
@@ -272,9 +291,8 @@ init_bus(device_node* node, void** bus_cookie)
                | SDHCI_INT_BUS_POWER | SDHCI_INT_END_BIT;
 
        sdhci_register_dump(slot, _regs);
-       sdhci_set_clock(_regs, SDHCI_BASE_CLOCK_DIV_128);
+       sdhci_set_clock(_regs);
        sdhci_set_power(_regs);
-       sdhci_register_dump(slot, _regs);
 
        *bus_cookie = bus;
        return status;
@@ -287,10 +305,8 @@ sdhci_error_interrupt_recovery(struct registers* _regs)
        _regs->interrupt_signal_enable &= ~(SDHCI_INT_CMD_CMP
                | SDHCI_INT_TRANS_CMP | SDHCI_INT_CARD_INS | 
SDHCI_INT_CARD_REM);
 
-       if (_regs->interrupt_status & 7) {
-               _regs->software_reset |= 1 << 1;
-               while (_regs->command);
-       }
+       if (_regs->interrupt_status & 7)
+               _regs->software_reset.ResetTransaction();
 
        int16_t erorr_status = _regs->interrupt_status;
        _regs->interrupt_status &= ~(erorr_status);
@@ -303,11 +319,11 @@ sdhci_generic_interrupt(void* data)
        TRACE("interrupt function called\n");
        sdhci_pci_mmc_bus_info* bus = (sdhci_pci_mmc_bus_info*)data;
 
-       uint16_t intmask, card_present;
+       uint32_t intmask, card_present;
 
        intmask = bus->_regs->slot_interrupt_status;
 
-       if (intmask == 0 || intmask == 0xffffffff) {
+       if ((intmask == 0) || (intmask == 0xffffffff)) {
                TRACE("invalid command interrupt\n");
 
                return B_UNHANDLED_INTERRUPT;
@@ -335,8 +351,8 @@ sdhci_generic_interrupt(void* data)
 
        // handling command interrupt
        if (intmask & SDHCI_INT_CMD_MASK) {
-               TRACE("interrupt status error: %d\n", 
bus->_regs->interrupt_status);
                bus->_regs->interrupt_status |= (intmask & SDHCI_INT_CMD_MASK);
+               // TODO do something with the interrupt
                TRACE("Command interrupt handled\n");
 
                return B_HANDLED_INTERRUPT;
diff --git a/src/add-ons/kernel/busses/mmc/sdhci_pci.h 
b/src/add-ons/kernel/busses/mmc/sdhci_pci.h
index ee94e99fbc..2c7680c2a6 100644
--- a/src/add-ons/kernel/busses/mmc/sdhci_pci.h
+++ b/src/add-ons/kernel/busses/mmc/sdhci_pci.h
@@ -20,37 +20,101 @@
 #define SDHCI_DEVICE_TYPE_ITEM                                                 
        "sdhci/type"
 #define SDHCI_BUS_TYPE_NAME                                                    
"bus/sdhci/v1"
 
-#define SDHCI_CARD_DETECT                                                      
        1 << 16
-
-#define SDHCI_SOFTWARE_RESET_ALL                                               
1 << 0
-
-#define SDHCI_BASE_CLOCK_FREQ(x)                                               
((x >> 8) & 63)
-#define SDHCI_VOLTAGE_SUPPORTED(x)                      ((x >> 24) & 7)
-#define SDHCI_VOLTAGE_SUPPORTED_33(x)                   ((x >> 24) & 1)
-#define SDHCI_VOLTAGE_SUPPORTED_30(x)                          ((x >> 24) & 3)
-#define SDHCI_VOLTAGE_SUPPORT_33                                               
7 << 1
-#define SDHCI_VOLTAGE_SUPPORT_30                                               
6 << 1
-#define SDHCI_VOLTAGE_SUPPORT_18                                               
5 << 1
-#define SDHCI_CARD_INSERTED(x)                                                 
((x >> 16) & 1)
-#define SDHCI_BASE_CLOCK_DIV_1                                                 
0 << 8
-#define SDHCI_BASE_CLOCK_DIV_2                                                 
1 << 8
-#define SDHCI_BASE_CLOCK_DIV_4                                                 
2 << 8
-#define SDHCI_BASE_CLOCK_DIV_8                                                 
4 << 8
-#define SDHCI_BASE_CLOCK_DIV_16                                                
        8 << 8
-#define SDHCI_BASE_CLOCK_DIV_32                                                
        16 << 8
-#define SDHCI_BASE_CLOCK_DIV_64                                                
        32 << 8
-#define SDHCI_BASE_CLOCK_DIV_128                                               
64 << 8
-#define SDHCI_BASE_CLOCK_DIV_256                                               
128 << 8
-#define SDHCI_INTERNAL_CLOCK_ENABLE                                            
1 << 0
-#define SDHCI_INTERNAL_CLOCK_STABLE                                            
1 << 1
-#define SDHCI_SD_CLOCK_ENABLE                                                  
1 << 2
-#define SDHCI_SD_CLOCK_DISABLE                                                 
~(1 << 2)
-#define SDHCI_CLR_FREQ_SEL                                                     
    ~(255 << 8)
-#define SDHCI_BUS_POWER_ON                                                     
    1
+
+class Command {
+       public:
+               uint16_t Bits() { return fBits; }
+
+               void SendCommand(uint8_t command, bool data)
+               {
+                       fBits = (command << 8) | (data << 5);
+               }
+
+       private:
+               volatile uint16_t fBits;
+} __attribute__((packed));
 #define SDHCI_RESPONSE_R1                               2
 #define SDHCI_CMD_CRC_EN                                1 << 3
 #define SDHCI_CMD_INDEX_EN                              1 << 4
-#define SDHCI_CMD_0                                     ~(63 << 8)
+
+
+class PresentState {
+       public:
+               uint32_t Bits() { return fBits; }
+
+               bool IsCardInserted() { return fBits & (1 << 16); }
+
+       private:
+               volatile uint32_t fBits;
+} __attribute__((packed));
+
+
+class PowerControl {
+       public:
+               uint8_t Bits() { return fBits; }
+
+               void SetVoltage(int voltage) {
+                       fBits |= voltage | kBusPowerOn;
+               }
+               void PowerOff() { fBits &= ~kBusPowerOn; }
+
+               static const uint8_t k3v3 = 7 << 1;
+               static const uint8_t k3v0 = 6 << 1;
+               static const uint8_t k1v8 = 5 << 1;
+       private:
+               volatile uint8_t fBits;
+
+               static const uint8_t kBusPowerOn = 1;
+} __attribute__((packed));
+
+
+class ClockControl
+{
+       public:
+               uint16_t Bits() { return fBits; }
+
+               uint16_t SetDivider(uint16_t divider) {
+                       if (divider == 1)
+                               divider = 0;
+                       else
+                               divider /= 2;
+                       uint16_t bits = fBits & ~0xffc0;
+                       bits |= divider << 8;
+                       bits |= (divider >> 8) & 0xc0;
+                       fBits = bits;
+
+                       return divider == 0 ? 1 : divider * 2;
+               }
+
+               void EnableInternal() { fBits |= 1 << 0; }
+               bool InternalStable() { return fBits & (1 << 1); }
+               void EnableSD() { fBits |= 1 << 2; }
+               void DisableSD() { fBits &= ~(1 << 2); }
+               void EnablePLL() { fBits |= 1 << 3; }
+       private:
+               volatile  uint16_t fBits;
+} __attribute__((packed));
+
+
+class SoftwareReset {
+       public:
+               uint8_t Bits() { return fBits; }
+
+               void ResetAll() {
+                       fBits |= 1;
+                       while(fBits & 1);
+               }
+
+               void ResetTransaction() {
+                       fBits |= 2;
+                       while(fBits & 2);
+               }
+
+       private:
+               volatile uint8_t fBits;
+} __attribute__((packed));
+
+
 /* Interrupt registers */
 #define SDHCI_INT_CMD_CMP              0x00000001              // command 
complete enable
 #define SDHCI_INT_TRANS_CMP            0x00000002              // transfer 
complete enable
@@ -68,40 +132,87 @@
 #define SDHCI_INT_CMD_MASK     (SDHCI_INT_CMD_CMP | SDHCI_INT_CMD_ERROR_MASK)
 
 
+class Capabilities
+{
+       public:
+               uint64_t Bits() { return fBits; }
+
+               uint8_t SupportedVoltages() { return (fBits >> 24) & 7; }
+               uint8_t BaseClockFrequency() { return (fBits >> 8) & 0xFF; }
+
+               static const uint8_t k3v3 = 1;
+               static const uint8_t k3v0 = 2;
+               static const uint8_t k1v8 = 4;
+
+       private:
+               const uint64_t fBits;
+} __attribute__((packed));
+
+
+class HostControllerVersion {
+       public:
+               const uint8_t specVersion;
+               const uint8_t vendorVersion;
+} __attribute__((packed));
+
+
 struct registers {
+       // SD command generation
        volatile uint32_t system_address;
        volatile uint16_t block_size;
        volatile uint16_t block_count;
        volatile uint32_t argument;
        volatile uint16_t transfer_mode;
-       volatile uint16_t command;
+       Command command;
+
+       // Response
        volatile uint16_t response[8];
+
+       // Buffer Data Port
        volatile uint32_t buffer_data_port;
-       volatile uint32_t present_state;
+
+       // Host control 1
+       PresentState present_state;
        volatile uint8_t host_control;
-       volatile uint8_t power_control;
+       PowerControl power_control;
        volatile uint8_t block_gap_control;
        volatile uint8_t wakeup_control;
-       volatile uint16_t clock_control;
+       ClockControl clock_control;
        volatile uint8_t timeout_control;
-       volatile uint8_t software_reset;
+       SoftwareReset software_reset;
+
+       // Interrupt control
        volatile uint32_t interrupt_status;
        volatile uint32_t interrupt_status_enable;
        volatile uint32_t interrupt_signal_enable;
        volatile uint16_t auto_cmd12_error_status;
+
+       // Host control 2
        volatile uint16_t host_control_2;
-       volatile uint64_t capabilities;
+
+       // Capabilities
+       Capabilities capabilities;
        volatile uint64_t max_current_capabilities;
+
+       // Force event
        volatile uint16_t force_event_acmd_status;
        volatile uint16_t force_event_error_status;
+
+       // ADMA2
        volatile uint8_t adma_error_status;
        volatile uint8_t padding[3];
        volatile uint64_t adma_system_address;
+
+       // Preset values
        volatile uint64_t preset_value[2];
        volatile uint32_t :32;
        volatile uint16_t uhs2_preset_value;
        volatile uint16_t :16;
+
+       // ADMA3
        volatile uint64_t adma3_id_address;
+
+       // UHS-II
        volatile uint16_t uhs2_block_size;
        volatile uint16_t :16;
        volatile uint32_t uhs2_block_count;
@@ -121,6 +232,8 @@ struct registers {
        volatile uint32_t uhs2_error_interrupt_status_enable;
        volatile uint32_t uhs2_error_interrupt_signal_enable;
        volatile uint8_t padding3[16];
+
+       // Pointers
        volatile uint16_t uhs2_settings_pointer;
        volatile uint16_t uhs2_host_capabilities_pointer;
        volatile uint16_t uhs2_test_pointer;
@@ -128,8 +241,10 @@ struct registers {
        volatile uint16_t vendor_specific_pointer;
        volatile uint16_t reserved_specific_pointer;
        volatile uint8_t padding4[16];
+
+       // Common area
        volatile uint16_t slot_interrupt_status;
-       volatile uint16_t host_controller_version;
+       HostControllerVersion host_controller_version;
 } __attribute__((packed));
 
 typedef void* sdhci_mmc_bus;
@@ -142,7 +257,7 @@ typedef void* sdhci_mmc_bus;
 
 static void sdhci_register_dump(uint8_t, struct registers*);
 static void sdhci_reset(struct registers*);
-static void sdhci_set_clock(struct registers*, uint16_t);
+static void sdhci_set_clock(struct registers*);
 static void sdhci_set_power(struct registers*);
 static void sdhci_stop_clock(struct registers*);
 void sdhci_error_interrupt_recovery(struct registers*);

############################################################################

Commit:      3aef22f7c8c6d20620019cf605673f467995b169
URL:         https://git.haiku-os.org/haiku/commit/?id=3aef22f7c8c6
Author:      Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
Date:        Sun Feb 10 19:41:35 2019 UTC
Committer:   waddlesplash <waddlesplash@xxxxxxxxx>
Commit-Date: Mon Feb 11 21:36:25 2019 UTC

sdhci: implement uninit_bus

Also avoid leaking resources in case initialization fails.

Change-Id: Ia533182eeeb81b7d52b49510b1f375a4fc55258f
Reviewed-on: https://review.haiku-os.org/c/1030
Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>

----------------------------------------------------------------------------

diff --git a/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp 
b/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
index bec3eb9719..0649cfe705 100644
--- a/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
+++ b/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright 2018 Haiku, Inc. All rights reserved.
+ * Copyright 2018-2019 Haiku, Inc. All rights reserved.
  * Distributed under the terms of the MIT License.
  *
  * Authors:
@@ -31,23 +31,13 @@
 #define SDHCI_PCI_DEVICE_MODULE_NAME "busses/mmc/sdhci_pci/driver_v1"
 #define SDHCI_PCI_MMC_BUS_MODULE_NAME "busses/mmc/sdhci_pci/device/v1"
 
-#define SDHCI_PCI_CONTROLLER_TYPE_NAME "sdhci pci controller"
-
 #define SLOTS_COUNT                            "device/slots_count"
 #define SLOT_NUMBER                            "device/slot"
 #define BAR_INDEX                              "device/bar"
 
 typedef struct {
-       pci_device_module_info* pci;
-       pci_device* device;
-       addr_t base_addr;
-       uint8 irq;
-       sdhci_mmc_bus mmc_bus;
-       area_id regs_area;
-       device_node* node;
-       pci_info info;
-       struct registers* _regs;
-
+       struct registers* fRegisters;
+       uint8_t fIrq;
 } sdhci_pci_mmc_bus_info;
 
 
@@ -195,7 +185,6 @@ init_bus(device_node* node, void** bus_cookie)
        if (bus == NULL)
                return B_NO_MEMORY;
 
-       pci_info* pciInfo = &bus->info;
        pci_device_module_info* pci;
        pci_device* device;
 
@@ -212,25 +201,17 @@ init_bus(device_node* node, void** bus_cookie)
                TRACE("PCIx86Module not loaded\n");
        }
 
-       int msiCount = sPCIx86Module->get_msi_count(pciInfo->bus,
-               pciInfo->device, pciInfo->function);
-
-       TRACE("interrupts count: %d\n",msiCount);
-
        if (gDeviceManager->get_attr_uint8(node, SLOT_NUMBER, &slot, false) < 
B_OK
                || gDeviceManager->get_attr_uint8(node, BAR_INDEX, &bar, false) 
< B_OK)
                return -1;
 
-       TRACE("Controller has %d slots, first bar is %d\n", slot + 1, bar);
-
-       bus->node = node;
-       bus->pci = pci;
-       bus->device = device;
+       TRACE("Register SD bus at slot %d, using bar %d\n", slot + 1, bar);
 
-       pci->get_pci_info(device, pciInfo);
-
-       // legacy interrupt
-       bus->base_addr = pciInfo->u.h0.base_registers[bar];
+       pci_info pciInfo;
+       pci->get_pci_info(device, &pciInfo);
+       int msiCount = sPCIx86Module->get_msi_count(pciInfo.bus,
+               pciInfo.device, pciInfo.function);
+       TRACE("interrupts count: %d\n",msiCount);
 
        // enable bus master and io
        uint16 pcicmd = pci->read_pci_config(device, PCI_command, 2);
@@ -239,14 +220,14 @@ init_bus(device_node* node, void** bus_cookie)
        pci->write_pci_config(device, PCI_command, 2, pcicmd);
 
        TRACE("init_bus() %p node %p pci %p device %p\n", bus, node,
-               bus->pci, bus->device);
+               pci, device);
 
        // mapping the registers by MMUIO method
        int bar_size = pciInfo->u.h0.base_register_sizes[bar];
 
        regs_area = map_physical_memory("sdhc_regs_map",
-               pciInfo->u.h0.base_registers[bar],
-               pciInfo->u.h0.base_register_sizes[bar], 
B_ANY_KERNEL_BLOCK_ADDRESS,
+               pciInfo.u.h0.base_registers[bar],
+               pciInfo.u.h0.base_register_sizes[bar], 
B_ANY_KERNEL_BLOCK_ADDRESS,
                B_KERNEL_READ_AREA | B_KERNEL_WRITE_AREA, (void**)&regs);
 
        if (regs_area < B_OK) {
@@ -254,29 +235,40 @@ init_bus(device_node* node, void** bus_cookie)
                return B_BAD_VALUE;
        }
 
-       bus->regs_area = regs_area;
        struct registers* _regs = (struct registers*)regs;
-       bus->_regs = _regs;
+       bus->fRegisters = _regs;
        sdhci_reset(_regs);
-       bus->irq = pciInfo->u.h0.interrupt_line;
 
-       TRACE("irq interrupt line: %d\n",bus->irq);
+       // the interrupt is shared between all busses in an SDHC controller, but
+       // they each register an handler. Not a problem, we will just test the
+       // interrupt registers for all busses one after the other and find no
+       // interrupts on the idle busses.
+       bus->fIrq = pciInfo.u.h0.interrupt_line;
 
-       if (bus->irq == 0 || bus->irq == 0xff) {
+       TRACE("irq interrupt line: %d\n", bus->fIrq);
+
+       if (bus->fIrq == 0 || bus->fIrq == 0xff) {
                TRACE("PCI IRQ not assigned\n");
                if (sPCIx86Module != NULL) {
                        put_module(B_PCI_X86_MODULE_NAME);
                        sPCIx86Module = NULL;
                }
+               delete_area(regs_area);
                delete bus;
                return B_ERROR;
        }
 
-       status = install_io_interrupt_handler(bus->irq,
+       status = install_io_interrupt_handler(bus->fIrq,
                sdhci_generic_interrupt, bus, 0);
 
        if (status != B_OK) {
                TRACE("can't install interrupt handler\n");
+               if (sPCIx86Module != NULL) {
+                       put_module(B_PCI_X86_MODULE_NAME);
+                       sPCIx86Module = NULL;
+               }
+               delete_area(regs_area);
+               delete bus;
                return status;
        }
        TRACE("interrupt handler installed\n");
@@ -299,6 +291,25 @@ init_bus(device_node* node, void** bus_cookie)
 }
 
 
+static void
+uninit_bus(void* bus_cookie)
+{
+       sdhci_pci_mmc_bus_info* bus = (sdhci_pci_mmc_bus_info*)bus_cookie;
+
+       bus->fRegisters->interrupt_signal_enable = 0;
+       bus->fRegisters->interrupt_status_enable = 0;
+
+       remove_io_interrupt_handler(bus->fIrq, sdhci_generic_interrupt, bus);
+
+       area_id regs_area = area_for(bus->fRegisters);
+       delete_area(regs_area);
+
+       // FIXME do we need to put() the PCI module here?
+
+       delete bus;
+}
+
+
 void
 sdhci_error_interrupt_recovery(struct registers* _regs)
 {
@@ -321,7 +332,7 @@ sdhci_generic_interrupt(void* data)
 
        uint32_t intmask, card_present;
 
-       intmask = bus->_regs->slot_interrupt_status;
+       intmask = bus->fRegisters->slot_interrupt_status;
 
        if ((intmask == 0) || (intmask == 0xffffffff)) {
                TRACE("invalid command interrupt\n");
@@ -332,17 +343,17 @@ sdhci_generic_interrupt(void* data)
        // handling card presence interrupt
        if (intmask & (SDHCI_INT_CARD_INS | SDHCI_INT_CARD_REM)) {
                card_present = ((intmask & SDHCI_INT_CARD_INS) != 0);
-               bus->_regs->interrupt_status_enable &= ~(SDHCI_INT_CARD_INS
+               bus->fRegisters->interrupt_status_enable &= ~(SDHCI_INT_CARD_INS
                        | SDHCI_INT_CARD_REM);
-               bus->_regs->interrupt_signal_enable &= ~(SDHCI_INT_CARD_INS
+               bus->fRegisters->interrupt_signal_enable &= ~(SDHCI_INT_CARD_INS
                        | SDHCI_INT_CARD_REM);
 
-               bus->_regs->interrupt_status_enable |= card_present
+               bus->fRegisters->interrupt_status_enable |= card_present
                        ? SDHCI_INT_CARD_REM : SDHCI_INT_CARD_INS;
-               bus->_regs->interrupt_signal_enable |= card_present
+               bus->fRegisters->interrupt_signal_enable |= card_present
                        ? SDHCI_INT_CARD_REM : SDHCI_INT_CARD_INS;
 
-               bus->_regs->interrupt_status |= (intmask &
+               bus->fRegisters->interrupt_status |= (intmask &
                        (SDHCI_INT_CARD_INS | SDHCI_INT_CARD_REM));
                TRACE("Card presence interrupt handled\n");
 
@@ -351,7 +362,7 @@ sdhci_generic_interrupt(void* data)
 
        // handling command interrupt
        if (intmask & SDHCI_INT_CMD_MASK) {
-               bus->_regs->interrupt_status |= (intmask & SDHCI_INT_CMD_MASK);
+               bus->fRegisters->interrupt_status |= (intmask & 
SDHCI_INT_CMD_MASK);
                // TODO do something with the interrupt
                TRACE("Command interrupt handled\n");
 
@@ -360,7 +371,7 @@ sdhci_generic_interrupt(void* data)
 
        // handling bus power interrupt
        if (intmask & SDHCI_INT_BUS_POWER) {
-               bus->_regs->interrupt_status |= SDHCI_INT_BUS_POWER;
+               bus->fRegisters->interrupt_status |= SDHCI_INT_BUS_POWER;
                TRACE("card is consuming too much power\n");
 
                return B_HANDLED_INTERRUPT;
@@ -373,14 +384,6 @@ sdhci_generic_interrupt(void* data)
 }
 
 
-static void
-uninit_bus(void* bus_cookie)
-{
-       sdhci_pci_mmc_bus_info* bus = (sdhci_pci_mmc_bus_info*)bus_cookie;
-       delete bus;
-}
-
-
 static void
 bus_removed(void* bus_cookie)
 {
@@ -453,7 +456,7 @@ static status_t
 register_device(device_node* parent)
 {
        device_attr attrs[] = {
-               {B_DEVICE_PRETTY_NAME, B_STRING_TYPE, {string: "SDHC PCI 
controller"}},
+               {B_DEVICE_PRETTY_NAME, B_STRING_TYPE, {string: "SD Host 
Controller"}},
                {}
        };
 
diff --git a/src/add-ons/kernel/busses/mmc/sdhci_pci.h 
b/src/add-ons/kernel/busses/mmc/sdhci_pci.h
index 2c7680c2a6..6035a1e2fa 100644
--- a/src/add-ons/kernel/busses/mmc/sdhci_pci.h
+++ b/src/add-ons/kernel/busses/mmc/sdhci_pci.h
@@ -1,5 +1,5 @@
 /*
- * Copyright 2018 Haiku, Inc. All rights reserved.
+ * Copyright 2018-2019 Haiku, Inc. All rights reserved.
  * Distributed under the terms of the MIT License.
  *
  * Authors:

############################################################################

Revision:    hrev52858
Commit:      601c0fcae2a23ce37841829b4861653e5d13acd9
URL:         https://git.haiku-os.org/haiku/commit/?id=601c0fcae2a2
Author:      Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
Date:        Sun Feb 10 20:20:55 2019 UTC
Committer:   waddlesplash <waddlesplash@xxxxxxxxx>
Commit-Date: Mon Feb 11 21:36:25 2019 UTC

sdhci: silently handle empty interrupts

On my system the interrupt line is shared with the PS/2 controller, so
there are a lot of unhandled interrupts. Just silently ignore them.

Change-Id: Ia6812a5a1d78907622ddebbd03165ed016e26e26
Reviewed-on: https://review.haiku-os.org/c/1031
Reviewed-by: waddlesplash <waddlesplash@xxxxxxxxx>

----------------------------------------------------------------------------

diff --git a/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp 
b/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
index 0649cfe705..8a8d494ba9 100644
--- a/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
+++ b/src/add-ons/kernel/busses/mmc/sdhci_pci.cpp
@@ -327,22 +327,18 @@ sdhci_error_interrupt_recovery(struct registers* _regs)
 int32
 sdhci_generic_interrupt(void* data)
 {
-       TRACE("interrupt function called\n");
        sdhci_pci_mmc_bus_info* bus = (sdhci_pci_mmc_bus_info*)data;
-
-       uint32_t intmask, card_present;
-
-       intmask = bus->fRegisters->slot_interrupt_status;
+       uint32_t intmask = bus->fRegisters->slot_interrupt_status;
 
        if ((intmask == 0) || (intmask == 0xffffffff)) {
-               TRACE("invalid command interrupt\n");
-
                return B_UNHANDLED_INTERRUPT;
        }
 
+       TRACE("interrupt function called\n");
+
        // handling card presence interrupt
        if (intmask & (SDHCI_INT_CARD_INS | SDHCI_INT_CARD_REM)) {
-               card_present = ((intmask & SDHCI_INT_CARD_INS) != 0);
+               uint32_t card_present = ((intmask & SDHCI_INT_CARD_INS) != 0);
                bus->fRegisters->interrupt_status_enable &= ~(SDHCI_INT_CARD_INS
                        | SDHCI_INT_CARD_REM);
                bus->fRegisters->interrupt_signal_enable &= ~(SDHCI_INT_CARD_INS
@@ -377,8 +373,12 @@ sdhci_generic_interrupt(void* data)
                return B_HANDLED_INTERRUPT;
        }
 
-       intmask &= ~(SDHCI_INT_BUS_POWER | SDHCI_INT_CARD_INS
-               | SDHCI_INT_CARD_REM | SDHCI_INT_CMD_MASK);
+       intmask = bus->fRegisters->slot_interrupt_status;
+       if (intmask != 0) {
+               ERROR("Remaining interrupts at end of handler: %x\n", intmask);
+               intmask &= ~(SDHCI_INT_BUS_POWER | SDHCI_INT_CARD_INS
+                       | SDHCI_INT_CARD_REM | SDHCI_INT_CMD_MASK);
+       }
 
        return B_UNHANDLED_INTERRUPT;
 }


Other related posts:

  • » [haiku-commits] haiku: hrev52858 - src/add-ons/kernel/busses/mmc - waddlesplash