[Skiboot] [PATCH v8 2/2] Add SBE driver support

[Vasant Hegde]

On 04/20/2018 11:51 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2018-04-19 at 13:58 +0530, Vasant Hegde wrote:
>> +}
>> +
>> +static void __p9_sbe_interrupt(struct p9_sbe *sbe)
>> +{
>> +       bool has_response = false;
>> +       int rc;
>> +       u64 data = 0, val;
>> +       struct p9_sbe_msg *msg = NULL;
>>   
>>          /* Read doorbell register */
>> -       rc = xscom_read(chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>> +       rc = xscom_read(sbe->chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>>          if (rc) {
>>                  prlog(PR_ERR, "Failed to read SBE to Host doorbell register "
>> -                     "[chip id = %x]\n", chip_id);
>> -               goto clr_interrupt;
>> +                     "[chip id = %x]\n", sbe->chip_id);
>> +               p9_sbe_reg_dump(sbe->chip_id);
>> +               return;
>>          }
>>   
>> -       /* SBE passtrhough command, call prd handler */
>> -       if (data & SBE_HOST_PASSTHROUGH) {
>> -               prd_sbe_passthrough(chip_id);
>> +       /* Called from poller path, nothing to process */
>> +       if (!data)
>> +               return;
>> +
>> +       /* Read SBE response before clearing doorbell register */
>> +       if (data & SBE_HOST_RESPONSE_WAITING) {
>> +               if (!list_empty(&sbe->msg_list)) {
>> +                       msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> +                       p9_sbe_handle_response(sbe->chip_id, msg);
>> +                       has_response = true;
>> +               }
>>          }
> 
> I still think you should process the ACKs before you look at responses.

Here I'm just reading response data.. But not processing it. I call ACK first 
(below) and then call response handler. Do you see any issues with that?

> 
> Didn't we also say we should have a separate variable for responses in
> this function ?

Yeah. But later I thought we don't need separate variable here. If message needs 
response
then we will have msg->resp allocated. So use that to read response data. Also in
p9_sbe_handle_response() I clear msg->resp. That should take care of later stage 
failures in clearing interrupt.


> 
> Generally I still prefer if we handle one bit at a time amd loop & re-
> read & retry, as discussed.

I thought about that.. But I don't see much benefits. In both cases we have to 
take care of
ordering (like process ACK before response). And we will end up clearing each 
bit separately. So we will do additional xscom_write to clear bits.

If you really want that method I can change the code. Please let me know.

Thanks
-Vasant