[PATCH linux dev-4.7] drivers/fsi: Adjust slave ID based on address in command send

[Christopher Bostic]

On 2/8/17 8:54 PM, Alistair Popple wrote:
> On Thu, 9 Feb 2017 10:48:13 AM Joel Stanley wrote:
>> On Thu, Feb 9, 2017 at 10:03 AM, Christopher Bostic
>> <cbostic at linux.vnet.ibm.com> wrote:
>>> In order to access slave address ranges > 0x1FFFFF the slave ID
>>> must be adjusted accordingly in the command encoding:
>>>
>>> ID 0: 0x00000000 - 0x001FFFFF
>>> ID 1: 0x00200000 - 0x003FFFFF
>>> ID 2: 0x00400000 - 0x005FFFFF
>>> ID 3: 0x00600000 - 0x007FFFFF
>> Alistair and Eddie, can I please get an ack from both of you on this?
>>
>>> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
>>> ---
>>>   drivers/fsi/fsi-master-gpio.c | 20 ++++++++++++++++----
>>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
>>> index 73d9985..428f3db 100644
>>> --- a/drivers/fsi/fsi-master-gpio.c
>>> +++ b/drivers/fsi/fsi-master-gpio.c
>>> @@ -30,7 +30,9 @@
>>>   #define        FSI_GPIO_CMD_READ       0x0400000000000000ULL
>>>   #define        FSI_GPIO_CMD_SLAVE_MASK 0xC000000000000000ULL
>>>   #define        FSI_GPIO_CMD_ADDR_SHIFT 37
>>> -#define        FSI_GPIO_CMD_ADDR_MASK  0x001FFFFF
>>> +#define        FSI_GPIO_CMD_ADR8_MASK  0x001FFFFF
>>> +#define        FSI_GPIO_CMD_ADR16_MASK 0x001FFFFE
>>> +#define        FSI_GPIO_CMD_ADR32_MASK 0x001FFFFC
>>>   #define        FSI_GPIO_CMD_SLV_SHIFT  62
>>>   #define        FSI_GPIO_CMD_SIZE_16    0x0000001000000000ULL
>>>   #define        FSI_GPIO_CMD_SIZE_32    0x0000003000000000ULL
>>> @@ -40,6 +42,9 @@
>>>   #define        FSI_GPIO_CMD_DFLT_LEN   28
>>>   #define        FSI_GPIO_CMD_CRC_SHIFT  60
>>>
>>> +#define        FSI_SLAVE_SHIFT         21
>>> +#define        FSI_SLAVE_MASK          0x3
>>> +
>>>   /* Bus errors */
>>>   #define        FSI_GPIO_ERR_BUSY       1       /* Slave stuck in busy state */
>>>   #define        FSI_GPIO_RESP_ERRA      2       /* Any (misc) Error */
>>> @@ -301,15 +306,17 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
> Do we need to expose the slave-id outside of this function? Ie. does
> anything currently (or is expected to in future) pass anything other
> than slave=0 to this function? If not we should drop slave-id from the
> function parameters imho.
>
There are cases where the core scan code will need to pass in slave-id 3 
during
link initialization.  Also the core is structured to deal with the 
eventual case
where we may have multiple CFAMs per link so I think it makes sense to
keep it as a parameter.

>>>                  const void *data)
>>>   {
>>>          uint8_t crc;
>>> +       uint32_t addr_mask;
>>>
>>>          cmd->bits = FSI_GPIO_CMD_DFLT_LEN;
>>>          cmd->msg = FSI_GPIO_CMD_DEFAULT;
>>>          cmd->msg |= mode;
>>> -       cmd->msg &= ~FSI_GPIO_CMD_SLAVE_MASK;
>>> +       if (!slave)
>>> +               slave = (addr >> FSI_SLAVE_SHIFT) & FSI_SLAVE_MASK;
>>>          cmd->msg |= (((uint64_t)slave) << FSI_GPIO_CMD_SLV_SHIFT);
>>> -       addr &= FSI_GPIO_CMD_ADDR_MASK;
>>> -       cmd->msg |= (((uint64_t)addr) << FSI_GPIO_CMD_ADDR_SHIFT);
>>> +
>>>          if (size == sizeof(uint8_t)) {
>>> +               addr_mask = FSI_GPIO_CMD_ADR8_MASK;
>>>                  if (data) {
>>>                          uint8_t cmd_data = *((uint8_t *)data);
>>>
>>> @@ -317,6 +324,7 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>>>                                  ((uint64_t)cmd_data) << FSI_GPIO_CMD_DT8_SHIFT;
>>>                  }
>>>          } else if (size == sizeof(uint16_t)) {
>>> +               addr_mask = FSI_GPIO_CMD_ADR16_MASK;
>>>                  cmd->msg |= FSI_GPIO_CMD_SIZE_16;
>>>                  if (data) {
>>>                          uint16_t cmd_data;
>>> @@ -326,6 +334,7 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>>>                                  ((uint64_t)cmd_data) << FSI_GPIO_CMD_DT16_SHIFT;
>>>                  }
>>>          } else {
>>> +               addr_mask = FSI_GPIO_CMD_ADR32_MASK;
>>>                  cmd->msg |= FSI_GPIO_CMD_SIZE_32;
>>>                  if (data) {
>>>                          uint32_t cmd_data;
>>> @@ -336,6 +345,9 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>>>                  }
>>>          }
>>>
>>> +       addr &= addr_mask;
> How is the masking of the address different now that you are setting
> the slave-id? Ie. this change looks more general than just setting the
> slave-id so is it fixing a different probelm? Or is somehow related to
> setting the slave-id?
>
Its not directly related to the slave ID.  I added this to ensure that 
any callers
that request odd alignments, i.e.  32 bit write aligned on address 0xFFFF is
properly masked.

Since it is separate conceptually I'll break this part out into a 
separate patch.

Thanks,
Chris

> Regards,
>
> Alistair
>
>>> +       cmd->msg |= (((uint64_t)addr) << FSI_GPIO_CMD_ADDR_SHIFT);
>>> +
>>>          if (mode == FSI_GPIO_CMD_WRITE)
>>>                  cmd->bits += (8 * size);
>>>
>>> --
>>> 1.8.2.2
>>>