Re: [PATCH V2 01/10] accel/amdxdna: Add a new driver for AMD AI Engine

[Carl Vanderlip <quic_carlv@quicinc.com>]

On 8/5/2024 10:39 AM, Lizhi Hou wrote:
 > +static int aie2_init(struct amdxdna_dev *xdna)
 > +{
 > +    struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
 > +    struct amdxdna_dev_hdl *ndev;
 > +    struct psp_config psp_conf;
 > +    const struct firmware *fw;
 > +    void __iomem * const *tbl;
 > +    int i, bars, nvec, ret;
 > +
 > +    ndev = devm_kzalloc(&pdev->dev, sizeof(*ndev), GFP_KERNEL);
 > +    if (!ndev)
 > +        return -ENOMEM;
 > +
 > +    ndev->priv = xdna->dev_info->dev_priv;
 > +    ndev->xdna = xdna;
 > +
 > +    ret = request_firmware(&fw, ndev->priv->fw_path, &pdev->dev);
 > +    if (ret) {
 > +        XDNA_ERR(xdna, "failed to request_firmware %s, ret %d",
 > +             ndev->priv->fw_path, ret);
 > +        return ret;
 > +    }
 > +
 > +    ret = pcim_enable_device(pdev);


Does request_firmware need to be the first thing here? Its not used 
until the end of init. Likewise, fw image is copied in *_create, but 
then not released until after *_hw_start; could release_firmware more 
closely wrap where it is used? I could see it being checked first 
because if the fw isn't there, what's the point, but that could be said 
about any of the other resources here.

On 8/5/2024 10:39 AM, Lizhi Hou wrote:
 > +enum aie2_smu_reg_idx {
 > +    SMU_CMD_REG = 0,
 > +    SMU_ARG_REG,
 > +    SMU_INTR_REG,
 > +    SMU_RESP_REG,
 > +    SMU_OUT_REG,
 > +    SMU_MAX_REGS /* Kepp this at the end */
 > +};

*Keep


-Carl V.

PS Sorry for double email Lizhi, forgot to send to list.