RE: [PATCH 01/11] net: wwan: t9xx: Add PCIe core

["Wu. JackBB (GSM)" <JackBB_Wu@compal.com>]

Hi Jagielski,

Thank you for the detailed review. Below are the changes and responses
for v2.

> > +#define BAR_NUM6
>
> please add driver prefix

Renamed to MTK_PCI_BAR_NUM at v2.

> > +#define SET_HW_BITS(dest, chs, mhccif, dev)\
> > +({\
> > +if ((chs) & (dev))
>
> what if any of these is equal to 0?
> just skip do not log anything?

This macro converts SW event bits to HW channel bits. The callers
always pass valid non-zero values for chs and dev. Even if either
is zero, the bitwise AND produces zero, so dest remains unchanged
and writing zero to the hardware will not trigger any event. This is
a safe no-op and does not warrant a warning or error log.

> > +u32 mtk_pci_mac_read32(struct mtk_pci_priv *priv, u64 addr)
> > +{
> > +return ioread32(priv->mac_reg_base + addr);
> > +}
> > ...
>
> would be lovely to have kdoc of the non-static functions from the series

Converted the four MMIO wrapper functions (mtk_pci_mac_read32,
mtk_pci_mac_write32, mtk_pci_read32, mtk_pci_write32) to static
inline in the header. These are trivial one-line wrappers around
ioread32/iowrite32 and do not warrant separate function definitions.
This also reduces the overall line count.

For the remaining non-static functions, kernel-doc comments have been
added in v2.

> > +size_l = FIELD_GET(GENMASK_ULL(31, 0), cfg->size);
> > +size_h = FIELD_GET(GENMASK_ULL(63, 32), cfg->size);
> > +pos = ffs(size_l);
> > +if (pos) {
> > +atr_size = pos - 2;
> > +} else {
> > +pos = ffs(size_h);
> > +atr_size = pos + 30;
>
> i believe better would be to have some defines instead of magic

Replaced magic numbers in mtk_pci_setup_atr() with named defines:
ATR_SIZE_LO32_MASK, ATR_SIZE_HI32_MASK, ATR_SIZE_BIAS_FROM_LO32,
ATR_ADDR_ALIGN_MASK, ATR_EN, ATR_PARAM_OFFSET.

> > +}
>
> please put some breaks to have the code logically separated

Added blank lines to separate logical blocks in mtk_pci_setup_atr().

> > +/* SRC_ADDR_H */
> > +addr = REG_ATR_PCIE_WIN0_T0_SRC_ADDR_MSB + offset;
> > +...
> > +/* SRC_ADDR_L */
> > +addr = REG_ATR_PCIE_WIN0_T0_SRC_ADDR_LSB + offset;
> > +...
> > +/* TRSL_ADDR_H */
> > +...
> > +/* TRSL_ADDR_L */
>
> comments seem to be redundant imo; clearer would be to have just newline
> instead

Replaced redundant inline comments with blank line separators.

> > +/* TRSL_PARAM */
> > +addr = REG_ATR_PCIE_WIN0_T0_TRSL_PARAM + offset;
> > +val = (cfg->trsl_param << 16) | cfg->trsl_id;
>
> again a lot of magic here

Replaced with ATR_PARAM_OFFSET define.

> > +int nr = 0;
>
> what's the point of zeroiniting if the value is assigned at
> the next line?

Removed the zero initialization and simplified the function. Also
added a guard for irq_cnt == 0 and irq_id < 0.

> > +nr = irq_id % priv->irq_cnt;
>
> are we sure irq_cnt won't be equal to 0 in any scenario?

Added a !priv->irq_cnt guard that returns -EINVAL before the modulo
operation. Also added an irq_id < 0 check for completeness.

> > +if (unlikely(irq_id < 0 || irq_id >= MTK_IRQ_CNT_MAX))
> > +return -EINVAL;
>
> is it anyhow beneficial to put unlikely here and in case of other
> appearances within the series?

Removed unlikely() from the IRQ parameter validation checks in
mtk_pci_get_irq_id(), mtk_pci_register_irq(),
mtk_pci_unregister_irq(), mtk_pci_mask_irq(),
mtk_pci_unmask_irq(), and mtk_pci_clear_irq(). These are not
hot paths and the branch hint provides no measurable benefit here.

> > +if (unlikely((irq_id < 0 || irq_id >= MTK_IRQ_CNT_MAX) || priv->irq_type != PCI_IRQ_MSIX)) {
>
> same here

Same as above, removed unlikely().

> > +void mtk_pci_mask_ext_evt(struct mtk_md_dev *mdev, u32 chs)
> > +{
> > +struct mtk_pci_priv *priv = mdev->hw_priv;
> > +u32 hw_bits;
> > +
> > +hw_bits = mtk_pci_ext_d2h_evt_hw_bits(chs);
>
> one of these is inited at declaration, 2nd one isnt
> please stay consistant, @hw_bits can be inited as well

Fixed. hw_bits is now initialized at declaration in
mtk_pci_mask_ext_evt(), mtk_pci_unmask_ext_evt(), and
mtk_pci_clear_ext_evt().

> > +int mtk_pci_send_ext_evt(struct mtk_md_dev *mdev, u32 ch)
> > +{
> > +struct mtk_pci_priv *priv = mdev->hw_priv;
> > +u32 rc_base;
> > +u32 hw_bits;
>
> missing kdoc here and there

Added kernel-doc comments to all non-static functions in v2.

> please squash variables of the same type into single line

Merged rc_base and hw_bits into a single declaration line.

> > +#else
>
> #else /* !CONFIG_ACPI */

Added comments to #else and #endif preprocessor directives.

> > +#endif
>
> #endif /* CONFIG_ACPI */

Done.

> > +msleep(500);
>
> please dont use magic number
> also where this value has been derived from?

Replaced with MTK_PLDR_POWER_OFF_DELAY_MS define. This 500ms delay
is the minimum time required by the MediaTek modem hardware to
complete the power-off sequence before re-initialization can begin.

> > +acpi_os_free(buffer.pointer);
>
> pleae add some newlines

Added newlines in mtk_pci_pldr() for better readability.

> > +default:
> > +break;
> > +}
> > +
> > +return -EINVAL;
>
> please put return into default label

Moved return -EINVAL into the default case label.

> > +struct mtk_pci_priv *priv = container_of(work, struct mtk_pci_priv, mhccif_work);
>
> isn't this line > 80 chars?

Wrapped the container_of line to stay within 80 columns.

> > +dev_err((mdev)->dev, "Failed to get mhccif_irq_id. ret=%d\n", ret);
> > +goto err;
>
> why cannot just return ret?

Simplified mtk_mhccif_init() to return directly instead of using
a goto label.

> > +dev_err((mdev)->dev, "Failed to register mhccif_irq callback\n");
> > +goto err;
>
> it's redundant

Removed the redundant goto. The function now returns directly.

> > +do {
> > +irq_id = fls(irq_state) - 1;
>
> are we sure irq_state cannot be 0?

The caller mtk_pci_irq_msix() already checks !irq_state and returns
IRQ_NONE before reaching mtk_pci_irq_handler(). So irq_state is
guaranteed to be non-zero when the handler is invoked.

> > +if (mtk_pci_link_check(mdev)) {
> > +pci_save_state(pdev);
> > +} else {
> > +ret = -EFAULT;
> > +goto clear_master;
>
> #defineEFAULT14/* Bad address */
> does it suit here?

Changed the error code from -EFAULT to -ENOLINK, which better
describes a PCIe link failure.

> > +mdev = devm_kzalloc(dev, sizeof(*mdev), GFP_KERNEL);
> > +if (!mdev) {
> > +ret = -ENOMEM;
> > +goto out;
>
> as for the rest of the labels please name what is done
> eg log_err

Renamed goto label "out" to "log_err".

> please also take a look on sashiko notes, there is some number of them

Addressed. The items from sashiko's review have been incorporated
into this revision.

Thanks.

Jack Wu


================================================================================================================================================================
This message may contain information which is private, privileged or confidential of Compal Electronics, Inc. If you are not the intended recipient of this message, please notify the sender and destroy/delete the message. Any review, retransmission, dissemination or other use of, or taking of any action in reliance upon this information, by persons or entities other than the intended recipient is prohibited.
================================================================================================================================================================