Re: [PATCH 1/2] mailbox: pcc: Add processing platform notification for slave subspaces

[lihuisong (C)]

在 2023/3/2 21:52, Sudeep Holla 写道:
> On Thu, Mar 02, 2023 at 09:57:35AM +0800, lihuisong (C) wrote:
> > 在 2023/3/1 21:24, Sudeep Holla 写道:
> [...]
>
> > > +static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
> > > +{
> > > +       u64 val;
> > > +       int ret;
> > > +
> > > +       ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
> > > +       if (ret)
> > > +               return false;
> > > +
> > we indeed already check if cmd_complete register is exist.
> > IMO, it can simply the code logic and reduce the risk of problems if we
> > return true here for the type without this register.
> > what do you think?
> IIUC, your concern is about returning true for type 4 when the register
> doesn't exist, right ?
Return true in advance for the type without the cmd_complete register.
If support the register, we judge if the channel should respond the 
interrupt based on the value of cmd_complete, like bellow.

-->8
+static bool pcc_mbox_cmd_complete_check(struct pcc_chan_info *pchan)
+{
+       u64 val;
+       int ret;
+
+       ret = pcc_chan_reg_read(&pchan->cmd_complete, &val);
+       if (ret)
+               return false;
+
+        if (!pchan->cmd_complete.gas)
+                return true;
+
+       /*
+         * Judge if the channel respond the interrupt based on the value of
+         * command complete.
+         */
+       val &= pchan->cmd_complete.status_mask;
+       /*
+        * If this is PCC slave subspace channel, then the command complete
+        * bit 0 indicates that Platform is sending a notification and OSPM
+        * needs to respond this interrupt to process this command.
+        */
+       if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+               return !val;
+       else
+               return !!val;
+}
> I am saying it won't happen as we bail out if there is no GAS register
> from pcc_chan_reg_init(). Or am I missing something here ?
Yes, what you say is also ok. Just wondering if it is better to simply 
the logic.
> > > +       val &= pchan->cmd_complete.status_mask;
> > > +
> > > +       /*
> > > +        * If this is PCC slave subspace channel, then the command complete
> > > +        * bit 0 indicates that Platform is sending a notification and OSPM
> > > +        * needs to respond this interrupt to process this command.
> > > +        */
> > > +       if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> > > +               return !val;
> > > +       else
> > > +               return !!val;
> > This else branch is not applicable to type 3. type 3 will cannot respond
> > interrupt.
> Sorry I don't understand what you mean by that.
Sorry for my mistake.
I meant that the type2 channel always return false in this function and
never respond the interrupt if no check for the GAS register.
Because the 'val' for the type without the register is zero.
>