Garrett D'Amore wrote: > Alan Perry wrote: >> I am sponsoring this self-review case. >> >> This case documents additional changes to an existing case. I believe >> that >> this case qualifies for self-review because the interfaces are >> Consolidation >> Private and backwards compatible with the existing interfaces. >> > > This is not the only criteria for self-review. Self-review cases must > also be so obvious and self explanatory that no further review is > desired or required. They should usually should not be introducing new > architecture. > > I believe this case exceed this threshold, and I would like to make sure > it is properly reviewed. Please convert this to a fast track with a one > week timer.
The case has been reviewed by at least two of the engineers that work on the sata framework, including the owner of the framework, and presented to the entire sata dev team. Who would review this case outside of the sata dev team? alan > > -- Garrett >> Template Version: @(#)sac_nextcase 1.68 02/23/09 SMI >> This information is Copyright 2009 Sun Microsystems >> 1. Introduction >> 1.1. Project/Component Working Name: >> SATA Framework Port Multiplier Support >> 1.2. Name of Document Author/Supplier: >> Author: Xiaoyu Zhang >> 1.3 Date of This Document: >> 14 July, 2009 >> 4. Technical Description >> >> 4.1. Background >> --------------- >> PSARC/2004/779 [1] established the SATA HBA Framework interface. >> PSARC/2005/679 [2] expanded this interface as needed to suport the >> marvell88sx >> and si3124 SATA HBA drivers. PSARC 2007/274 [3] expanded the interface >> further >> to support Native Command Queuing (NCQ) and ATAPI devices. >> PSARC/2007/448 [4] >> expanded the interface again to support releasing DMA framework-allocated >> resources associated with a command's buffer. >> >> This fast-track describes further interface changes required to >> support SATA >> port multipliers. >> >> 4.2. The Problem >> ---------------- >> >> 4.2.1. Required to support READ/WRITE PORTMULT command >> ------------------------------------------------------ >> According to the SATA Specification 2.6 [5] and the Port Multiplier >> Specification [6], READ PORT MULTIPLIER and WRITE PORT MULIPLIER are >> used to >> access the registers of the port multiplier. These two commands are >> necessary >> to the successful enumeration of the the port multiplier. >> >> Both SATA HBA drivers as well as the SATA module have internal functions >> operating on sata_pkt rather than isolated SATA commands. Therefore, >> READ/WRITE PORTMULT commands should be delivered to SATA HBA drivers in >> sata_pkt structures. >> >> 4.2.2. The insufficient sata_device interface >> --------------------------------------------- >> The sata_device is used as a parameter to the HBA probe entry point. >> Existing >> HBA implementations update only SATA Control Registers values in >> response to >> probe port operation. The port multiplier has its own Global Status & >> Control >> Registers, in which the parameters and status of the port multiplier >> itself >> are stored. The sata module needs this register information, so there >> needs >> to be a method for the SATA HBA driver to provide this information. >> >> 4.2.3. Required to handle port multiplier quirks >> ------------------------------------------------ >> The SATA specification 2.6 [5] defines the registers and enumeration >> process >> for port multipliers, however, some existing port multiplier models >> have quirks >> and might break the general enumeration or initialization process. The >> HBA >> driver should have idea of these quirks and determine its reaction. >> Additions >> to the interface are required to handle Port Multiplier behavior that is >> different the specification. >> >> >> 4.3. The Proposal >> ----------------- >> >> 4.3.1. Summary >> -------------- >> Revise the SATA interface to support port multiplier. Port multiplier >> support >> was partially designed and implemented inside SATA module. The proposal >> defines new interface functions necessary to support new SATA commands >> and new >> fields in sata_device structure to support port multiplier device. A >> blacklist >> structure is defined is added to to dealing with port multiplier >> discrepancies >> from the SATA specification. >> >> The initial consumer of the modified interface will be AHCI driver >> implementing SATA port multiplier support. >> >> Since the existing SATA HBA drivers and SATA module do not currently >> implement >> the structure version checking (except for sata_hba_tran structure), the >> proposed change will maintain the binary compatibility of the modified >> interface structure (sata_device). >> >> A SATA module supporting these interface changes will operate with >> SATA HBA >> drivers using the unmodified interface as well as SATA HBA drivers >> using the >> modified interface. >> >> 4.3.2. Interface Modifications >> ------------------------------ >> a) Add new structure sata_gscr. >> (See section 5.2.1 for more details) >> >> b) Modified sata_device structure. Add new field satadev_pmult_gscr >> for port >> multiplier device. >> (See section 5.3.1 for more details) >> >> c) The sata_device structure version (SATA_DEVICE_REV) will be >> increased to >> indicate added functionality. Current veriosn level is 1 - it >> will be increased to 2. >> (See section 5.3.1 for more details). >> >> d) The sata_hba_tran structure version (SATA_TRAN_HBA_REV) will be >> increased >> to 3 to indicate new functionality level of the entire SATA framework >> interface. >> (See section 5.3.2 for more details) >> >> e) Add three new SATA module interface functions. >> + sata_get_rdwr_pmult_pkt(); >> + sata_free_rdwr_pmult_pkt(); >> + sata_check_pmult_blacklist(); >> (See section 5.4 for more details) >> >> 4.5. Stability level >> -------------------- >> The stability level of the new interfaces will be the same as the >> other interfaces between SATA HBA Framework (SATA module) and >> SATA HBA driver, i.e. Consolidation Private. >> The requested release binding is micro release and patch release. >> >> >> 5. Interface Table >> ================== >> >> 5.1. Exported Interfaces >> ------------------------ >> >> ------------------------------------------------------------------------ >> Interface Level Comments >> ------------------------------------------------------------------------ >> SATA_DEVICE_REV Consolidation Symbol >> Private (redefine) >> SATA_DEVICE_REV_2 Consolidation sata_device version >> Private (new) >> SATA_TRAN_HBA_REV Consolidation Symbol >> Private (redefine) >> SATA_TRAN_HBA_REV_3 Consolidation sata_hba_tran version >> Private (new) >> sata_pmult_gscr Consolidation Interface structure >> Private (new) >> sata_device Consolidation Interface structure >> Private (modified) >> satadev_gscr Consolidation Interface structure >> Private (new) >> sata_get_rdwr_pmult_pkt Consolidation Interface function >> Private (new) >> sata_free_rdwr_pmult_pkt Consolidation Interface function >> Private (new) >> sata_check_pmult_blacklist Consolidation Interface function >> Private (new) >> >> 5.2. New structures >> ------------------- >> >> 5.2.1. New structure: sata_pmult_gscr >> ------------------------------------- >> struct sata_pmult_gscr { >> uint32_t gscr0; /* Product Identifier register */ >> uint32_t gscr1; /* Resrved Information register */ >> uint32_t gscr2; /* Port Information register */ >> uint32_t gscr64; /* Feature register */ >> }; >> >> >> 5.3. Redefined symbols >> ---------------------- >> >> 5.3.1. Redefine: SATA_DEVICE_REV & sata_device >> ---------------------------------------------- >> Modified field is indicated by a change bar. >> >> Old definition: >> #define SATA_DEVICE_REV_1 1 >> |#define SATA_DEVICE_REV SATA_DEVICE_REV_1 >> >> struct sata_device >> { >> int satadev_rev; /* structure version */ >> struct sata_address satadev_addr; /* sata port/device address */ >> uint32_t satadev_state; /* Port or device state */ >> uint32_t satadev_type; /* Attached device type */ >> struct sata_port_scr satadev_scr; /* Port status and ctrl regs */ >> uint32_t satadev_add_info; /* additional information, */ >> /* function specific */ >> }; >> >> New definition: >> #define SATA_DEVICE_REV_1 1 >> |#define SATA_DEVICE_REV_2 2 >> |#define SATA_DEVICE_REV SATA_DEVICE_REV_1 >> >> struct sata_device >> { >> int satadev_rev; /* structure version */ >> struct sata_address satadev_addr; /* sata port/device address */ >> uint32_t satadev_state; /* Port or device state */ >> uint32_t satadev_type; /* Attached device type */ >> struct sata_port_scr satadev_scr; /* Port status and ctrl regs */ >> uint32_t satadev_add_info; /* additional information, */ >> /* function specific */ >> | struct sata_pmult_gscr satadev_gscr; /* Port multiplier specific >> | global status and control >> | registers */ >> }; >> >> Implementation Notes: >> >> The satadev_gscr block is added for port multiplier's global status and >> control registers. >> >> >> 5.3.2. SATA_TRAN_HBA_REV redefinition >> ------------------------------------- >> Old definition: >> #define SATA_TRAN_HBA_REV SATA_TRAN_HBA_REV_2 >> >> New definitions: >> #define SATA_TRAN_HBA_REV_3 3 >> #define SATA_TRAN_HBA_REV SATA_TRAN_HBA_REV_3 >> >> Only version level of the sata_hba_tran structure is modified to indicate >> new functionality level of the entire SATA framework interface. >> New functionality includes: >> a) SATA module functions to get and free READ/WRITE PORTMULT sata >> packets. >> b) SATA port multiplier blacklist in SATA module >> c) Enable sata_device structure to support port multiplier device. >> New interface version (SATA_TRAN_HBA_REV_3) level also implies version >> 2 of >> the sata_device structure definition. >> >> >> 5.4. New Interface Functions >> ---------------------------- >> >> 5.4.1. sata_get_rdwr_pmult_pkt >> ------------------------------ >> #define SATA_RDWR_PMULT_PKT_TYPE_READ 1 >> #define SATA_RDWR_PMULT_PKT_TYPE_WRITE 2 >> >> NAME >> >> sata_get_rdwr_pmult_pkt - get sata packet to execute READ/WRITE >> PORTMULT >> command >> >> SYNOPSIS >> >> #include <sys/sata/impl/sata_hba.h> >> >> sata_pkt_t *sata_get_rdwr_pmult_pkt(dev_info_t *dip, >> sata_device_t *sata_device, uint8_t regn, >> uint32_t regv, uint32_t type); >> >> INTERFACE LEVEL >> >> Consolidation Private >> >> PARAMETERS >> >> dip >> Pointer to a dev_info_t structure, referring to the HBA device >> instance. >> >> sata_device >> Pointer to the structure specifying the SATA device address. >> >> regn >> Register that is supposed to be read/write. >> >> regv >> The value of the target register. This parameter is only used >> when the type parameter is set to SATA_RDWR_PMULT_PKT_TYPE_WRITE. >> >> type >> SATA_RDWR_PMULT_PKT_TYPE_READ Returned sata_pkt structure should >> contain >> READ PORTMULT command. >> >> SATA_RDWR_PMULT_PKT_TYPE_WRITE Returned sata_pkt structure should >> contain >> WRITE PORTMULT command. >> >> DESCRIPTION >> >> The sata_get_rdwr_pmult_pkt function is called by SATA HBA driver >> to obtain >> a fully initialized sata_pkt containing a READ/WRITE PORTMULT >> command, as >> well as a DMA-capable data buffer and DMA resources for the data >> buffer. >> The data buffer will satisfy HBA DMA attributes restrictions. The >> same >> data buffer could be also used for programmed I/O. >> >> The target register is specified by the regn argument. The command >> type is >> specified by type argument. >> >> The initialized sata packet does not specify any completion callback >> routine. No packet completion reason nor packet status is to be >> returned >> to SATA module. Once the SATA HBA completes sata_pkt usage, it >> should call >> sata_free_rdwr_pmult_pkt() function to free the packet and allocated >> resources. >> >> RETURN VALUES >> Returns a pointer to initialized sata_pkt if the function succeeds, >> and returns Null, if packet could not be allocated and/or >> initialized. >> >> CONTEXT >> This function cannot be called from the interrupt context. >> >> >> 5.4.2. sata_free_rdwr_pmult_pkt >> ------------------------------- >> >> NAME >> sata_free_rdwr_pmult_pkt - free sata packet allocated >> for READ/WRITE PORTMULT command >> >> SYNOPSIS >> #include <sys/sata/impl/sata_hba.h> >> >> void sata_free_rdwr_pmult_pkt(sata_pkt_t *sata_pkt); >> >> INTERFACE LEVEL >> >> Consolidation Private >> >> PARAMETERS >> sata_pkt sata_pkt allocated previoulsy by the >> sata_get_rdwr_pmult_pkt(). >> >> DESCRIPTION >> sata_free_rdwr_pmult_pkt function is called by the SATA HBA driver in >> order to release the sata_pkt structure allocated previously by >> sata_get_rdwr_pmult_pkt(). All resources associated with the >> packet are >> freed. After calling this function, the SATA HBA driver should not >> attempt >> to access any field and/or data buffer associated with the freed >> sata_pkt. >> >> RETURN VALUES >> Void >> >> CONTEXT >> This function may be called from the interrupt context. >> >> 5.4.3. sata_check_pmult_blacklist >> --------------------------------- >> >> NAME >> sata_check_pmult_blacklist - check if a port multiplier is on the >> blacklist >> >> SYNOPSIS >> #include <sys/sata/impl/sata_hba.h> >> >> int sata_check_pmult_blacklist(sata_device_t *sata_device); >> >> INTERFACE LEVEL >> >> Consolidation Private >> >> PARAMETERS >> sata_device Pointer to the sata_device structure that contains >> the global >> status and control register values of a port >> multiplier. >> >> DESCRIPTION >> sata_check_pmult_blacklist function is called by the SATA HBA >> driver in order to check if a port multiplier has any quirk. >> >> Some port multipliers have quirks, e.x. register values are not >> correctly >> configured. The SATA framework maintains a blacklist hence these port >> multiplier could be identified and properly handled. >> >> The model of a port multiplier can be uniquely identified by its >> read-only >> Global Status and Control Registers (GSCR[0,1,2]). In case a port >> multiplier >> is on the blacklist, this function will write the corresponding >> flags into >> satadev_add_info. >> >> RETURN VALUES >> SATA_SUCCESS The device is found on the blacklist and the >> sata_device >> structure is successfully updated. >> >> SATA_FAILURE The device is not on the blacklist. >> >> CONTEXT >> This function may be called from the interrupt context. >> >> >> 5.5. Release Summary >> >> S11, S10 Update >> >> 5.6. Packaging Changes >> >> 5.6.1. Binaries Modified >> ---------------------- >> /kernel/misc/sata >> /kernel/misc/amd64/sata >> /usr/include/sys/sata/sata_hba.h >> >> 5.6.2. Packages Affected >> ---------------------- >> SUNWckr >> SUNWhea >> >> 5.7. References >> >> [1] PSARC/2004/779 - SATA Framework Support >> [2] PSARC/2005/679 - SATA Framework Support (Updated) >> [3] PSARC/2007/274 - SATA Framework Interface Revision >> [4] PSARC/2008/448 - SATA Framework Addition >> [5] SATA Specification 2.6, Serial ATA International Organization >> [6] SATA Port Multiplier Specification 1.2, Serial ATA International >> Organization >> >> 6. Resources and Schedule >> 6.4. Steering Committee requested information >> 6.4.1. Consolidation C-team Name: >> ON >> 6.5. ARC review type: Automatic >> 6.6. ARC Exposure: open >> >> >