optoe: Add CMIS Bank support for transceivers with >8 lanes #473
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prgeor PTAL |
|
The optoe linear address space is extended as below to support Bank without changing how the user space interact with optoe.
|
| + * 2 byte writes are acceptable for PE and Vout changes per | ||
| + * Application Note AN-2071. | ||
| + * | ||
| + * use 2 bytes for CMIS devices since CMIS 5.3 requires write both BankSelect and PageSlect in one WRITE acess. |
| - The default bank size is set to 4, and can be modified via the 'bank_size' sysfs entry. | ||
| - For 'optoe3', the 'write_max' value is updated to 2 to comply with CMIS requirements, | ||
| which mandate that both bank and page values be updated in a single WRITE operation. | ||
|
|
There was a problem hiding this comment.
It’d be great if you could add a paragraph how to verify/test this to the commit message too.
ishidawataru
force-pushed
the
optoe-bank-support
branch
from
b380e95 to
28fa045
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Wataru Ishida <[email protected]>
ishidawataru
force-pushed
the
optoe-bank-support
branch
from
28fa045 to
4f10614
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prgeor I updated the patch based on our discussion PTAL. |
@ishidawataru Ack. reviewing. |
|
@ishidawataru can you define what do you mean by |
Signed-off-by: Wataru Ishida <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
For example, if I also added a commit that explains this in the comment. |
| /* fundamental unit of addressing for EEPROM */ | ||
| #define OPTOE_PAGE_SIZE 128 | ||
| + | ||
| +#define OPTOE_DEFAULT_BANK_SIZE 0 |
There was a problem hiding this comment.
@ishidawataru the default bank is 0, which means modules that support 8 lanes, the bank size = 1?
There was a problem hiding this comment.
@ishidawataru This probably can be removed if we implement full range of linear address for all bank pages. See my comments below
| +#define OPTOE_MAX_SUPPORTED_BANK_SIZE 8 | ||
| +#define OPTOE_NON_BANKED_PAGE_SIZE 16 /* page 00h-0Fh are not banked */ | ||
| +#define OPTOE_BANKED_PAGE_SIZE 240 /* page 10h-FFh are banked */ |
There was a problem hiding this comment.
@ishidawataru I am bit confused with respect to naming these by _SIZE. From the code use, it looks like these should be _COUNT?
There was a problem hiding this comment.
If these constants are still necessary after the discussion below, I’ll rename them as you suggested.
| /* 0x80 places the offset in the top half, offset is last 7 bits */ | ||
| *offset = OPTOE_PAGE_SIZE + (*offset & 0x7f); | ||
| - | ||
| - return page; /* note also returning client and offset */ |
There was a problem hiding this comment.
@ishidawataru The computation does not consider full range of linear address for all banks. Its OK to use all page address for bank 0 i.e Assume that page 0x10, 0x11, 0x12... 0xFF exist for Bank 0 as well.
So that the getaddr() can be implemented as follows:-
def getaddr(self, bank=0, page, offset, page_size=128):
return ((uint32_t)bank * PAGES_PER_BANK + page) * page_size + offset;
In your case you are not wasting the address space for OPTOE_NON_BANKED_PAGE_SIZE but its OK to loose those many address range to keep the use space implementation of getaddr() simple
Here is the simple decoding of the above linear address that optoe needs to do:-
static uint8_t optoe_translate_offset(struct optoe_data *optoe,
loff_t *offset, struct i2c_client **client, uint8_t *bank)
{
unsigned int page = 0;
*bank = *offset / (PAGES_PER_BANK * PAGE_SIZE);
page = ((*offset / PAGE_SIZE) % PAGES_PER_BANK) - 1;
*offset = PAGE_SIZE + linear_address % PAGE_SIZE;
return page;
}
Assumption here is the user application will provide only the valid pages and the module will return IO error if incorrect Page address is selected.
There was a problem hiding this comment.
@prgeor This is the linear address map of the current implementation.
Are you suggesting to change it like below?
Returning I/O errors for the grayed-out area will prevent us from using cat on the EEPROM file as shown below.
admin@sonic:~$ hexdump /sys/class/i2c-dev/i2c-1/device/1-0050/eeprom
0000000 0000 0000 0000 0000 0000 0000 0000 0000
*
001e880
I suggest keeping the current implementation or returning the value from bank 0, rather than returning I/O errors.
What do you think?
| + * For CMIS transceivers that support Banked Pages, access to these pages | ||
| + * is also supported. To access the banked pages, set the number of banks | ||
| + * to access via the `bank_size` sysfs entry. | ||
| + * By default, `bank_size` is set to 0, which disables this feature. |
There was a problem hiding this comment.
@ishidawataru We probably don't need this. see my comments above.
This patch adds CMIS Bank support to the 'optoe3' device class in order
to enable access to CMIS transceivers with more than 8 lanes (e.g., OSFP-XD, CPO OEs).
The default bank size is set to 4, and can be modified via the 'bank_size' sysfs entry.For 'optoe3', the 'write_max' value is updated to 2 to comply with CMIS requirements,which mandate that both bank and page values be updated in a single WRITE operation.
Updated the behavior as below after discussing with @prgeor offline
automatically updated to 2 to comply with CMIS requirements,
which mandate that both bank and page values be updated in a single WRITE operation.
Only tested with the SONiC VM + i2c-stub.
Snip of dmesg. See bank:3 is accessed.
Not tested with real hardware yet.