On Thu, Aug 28, 2025 at 03:09:51PM +0200, Andrew Lunn wrote:
> > +/**
> > + * mucse_mbx_get_capability - Get hw abilities from fw
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_get_capability tries to get capabities from
> > + * hw. Many retrys will do if it is failed.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > +{
> > + struct hw_abilities ability = {};
> > + int try_cnt = 3;
> > + int err;
> > + /* It is called once in probe, if failed nothing
> > + * (register network) todo. Try more times to get driver
> > + * and firmware in sync.
> > + */
> > + do {
> > + err = mucse_fw_get_capability(hw, &ability);
> > + if (err)
> > + continue;
> > + break;
> > + } while (try_cnt--);
> > +
> > + if (!err)
> > + hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > + return err;
> > +}
>
> I still think this should be a dedicated function to get the MAC
> driver and firmware in sync, using a NOP or version request to the
> firmware. The name mucse_mbx_get_capability() does not indicate this
> function is special in any way, which is it.
>
Maybe I should rename it like this?
/**
* mucse_mbx_sync_fw_by_get_capability - Try to sync driver and fw
* @hw: pointer to the HW structure
*
* mucse_mbx_sync_fw_by_get_capability tries to sync driver and fw
* by get capabitiy mbx cmd. Many retrys will do if it is failed.
*
* Return: 0 on success, negative errno on failure
**/
int mucse_mbx_sync_fw_by_get_capability(struct mucse_hw *hw)
{
struct hw_abilities ability = {};
int try_cnt = 3;
int err;
/* It is called once in probe, if failed nothing
* (register network) todo. Try more times to get driver
* and firmware in sync.
*/
do {
err = mucse_fw_get_capability(hw, &ability);
if (err)
continue;
break;
} while (try_cnt--);
if (!err)
hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
return err;
}
> > +/**
> > + * build_ifinsmod - build req with insmod opcode
> > + * @req: pointer to the cmd req structure
> > + * @is_insmod: true for insmod, false for rmmod
> > + **/
> > +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> > + bool is_insmod)
> > +{
> > + req->flags = 0;
> > + req->opcode = cpu_to_le16(DRIVER_INSMOD);
> > + req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> > + MBX_REQ_HDR_LEN);
> > + req->reply_lo = 0;
> > + req->reply_hi = 0;
> > +#define FIXED_VERSION 0xFFFFFFFF
> > + req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> > + if (is_insmod)
> > + req->ifinsmod.status = cpu_to_le32(1);
> > + else
> > + req->ifinsmod.status = cpu_to_le32(0);
> > +}
>
> Why does the firmware care? What does the firmware do when there is no
> kernel driver? How does it behaviour change when the driver loads?
>
fw reduce working frequency to save power if no driver is probed to this
chip. And fw change frequency to normal after recieve insmod mbx cmd.
Maybe I should add the comment to func "mucse_mbx_ifinsmod"?
/**
* mucse_mbx_ifinsmod - Echo driver insmod status to fw
* @hw: pointer to the HW structure
* @is_insmod: true for insmod, false for rmmod
*
* mucse_mbx_ifinsmod echo driver insmod status to fw. fw changes working
* frequency to normal after recieve insmod status, and reduce working
* frequency if no driver is probed.
*
* Return: 0 on success, negative errno on failure
**/
int mucse_mbx_ifinsmod(struct mucse_hw *hw, bool is_insmod)
{
}
> Please try to ensure comment say why you are doing something, not what
> you are doing.
>
>
> Andrew
>
> ---
> pw-bot: cr
>
Thanks for your feedback.
