Hi Geert-san,
> From: Geert Uytterhoeven, Sent: Tuesday, April 2, 2024 4:53 PM
>
> Hi Shimoda-san,
>
> On Mon, Apr 1, 2024 at 4:40 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > This driver previously supported r8a779f0 (R-Car S4-8). Add support
> > for r8a779g0 (R-Car V4H).
> >
> > To support r8a779g0, it requires specific firmware.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> Thanks for your patch!
Thank you for your review!
> > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
>
> > +static int rcar_gen4_pcie_update_phy_firmware(struct rcar_gen4_pcie *rcar)
> > +{
> > + const u32 check_addr[] = { 0x00101018, 0x00101118, 0x00101021, 0x00101121};
> > + struct dw_pcie *dw = &rcar->dw;
> > + const struct firmware *fw;
> > + unsigned int i, timeout;
> > + u32 data;
> > + int ret;
> > +
> > + ret = request_firmware(&fw, RCAR_GEN4_PCIE_FIRMWARE_NAME, dw->dev);
> > + if (ret) {
> > + dev_err(dw->dev, "%s: Requesting firmware failed\n", __func__);
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < (fw->size / 2); i++) {
> > + data = fw->data[i * 2] | fw->data[(i * 2) + 1] << 8;
> > + timeout = 100;
> > +retry_data:
> > + dw_pcie_writel_dbi(dw, PRTLGC89, RCAR_GEN4_PCIE_FIRMWARE_BASE_ADDR + i);
> > + dw_pcie_writel_dbi(dw, PRTLGC90, data);
> > + if (rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC89, BIT(30)) < 0) {
>
> If you would invert the logic here, you could "break" here, ...
>
> > + if (!(--timeout)) {
> > + ret = -ETIMEDOUT;
> > + goto exit;
> > + }
> > + usleep_range(100, 200);
> > + goto retry_data;
>
> ... and convert "retry_data: ... goto retry_data" into "do { ... } while (1)",
> avoiding the goto.
Thank you for your suggestion. I'll fix it.
> > + }
> > + }
> > +
> > + rcar_gen4_pcie_phy_reg_update_bits(rcar, RCAR_GEN4_PCIE_PHY_0f8, BIT(17), BIT(17));
> > +
> > + for (i = 0; i < ARRAY_SIZE(check_addr); i++) {
> > + timeout = 100;
> > +retry_check:
> > + dw_pcie_writel_dbi(dw, PRTLGC89, check_addr[i]);
> > + ret = rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC89, BIT(30));
> > + ret |= rcar_gen4_pcie_reg_check_bit(rcar, PRTLGC90, BIT(0));
> > + if (ret < 0) {
> > + if (!(--timeout)) {
> > + ret = -ETIMEDOUT;
> > + goto exit;
> > + }
> > + usleep_range(100, 200);
> > + goto retry_check;
>
> Likewise.
I'll fix it.
Best regards,
Yoshihiro Shimoda
> > + }
> > + }
> > +
> > + ret = 0;
> > +exit:
> > + release_firmware(fw);
> > +
> > + return ret;
> > +}
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
