Hi Michael,
On Fri, 9 May 2025 at 14:07, Michael Dege <michael.dege@xxxxxxxxxxx> wrote:
> Adding support for a new SOC device.
>
> Signed-off-by: Michael Dege <michael.dege@xxxxxxxxxxx>
Thanks for your patch!
> --- a/drivers/phy/renesas/renesas-ether-serdes.c
> +++ b/drivers/phy/renesas/renesas-ether-serdes.c
> @@ -14,11 +14,13 @@
> #include <linux/platform_device.h>
> #include <linux/reset.h>
>
> +#define RENESAS_ETH_SERDES_MAX_NUM R8A78000_ETH_SERDES_NUM
I think you can just fix this to 8:
#define RENESAS_ETH_SERDES_MAX_NUM 8
> #define R8A779F0_ETH_SERDES_NUM 3
> +#define R8A78000_ETH_SERDES_NUM 8
These are not really used.
> #define RENESAS_ETH_SERDES_OFFSET 0x0400
> #define RENESAS_ETH_SERDES_BANK_SELECT 0x03fc
> #define RENESAS_ETH_SERDES_TIMEOUT_US 100000
> -#define RENESAS_ETH_SERDES_LOCAL_OFFSET 0x2600
> +#define RENESAS_ETH_SERDES_LOCAL_OFFSET 0x2600
> #define RENESAS_ETH_SERDES_NUM_RETRY_LINKUP 3
>
> struct renesas_eth_serdes_drv_data;
> @@ -35,10 +37,21 @@ struct renesas_eth_serdes_drv_data {
> void __iomem *addr;
> struct platform_device *pdev;
> struct reset_control *reset;
> - struct renesas_eth_serdes_channel channel[R8A779F0_ETH_SERDES_NUM];
> + struct renesas_eth_serdes_channel channel[RENESAS_ETH_SERDES_MAX_NUM];
> + const struct renesas_serdes_hw_info *info;
> bool initialized;
> };
>
> +enum DEVICE_CODE {
> + r8a779f0,
> + r8a78000,
> +};
Unused (see below), so please drop.
> +
> +struct renesas_serdes_hw_info {
> + int renesas_eth_serdes_num;
That's a rather long name. "num_serdes"?
> + int device_type;
Unused. And it's cleaner to use "feature flags" (like num_serdes above)
than to check for a specific device type.
> +};
> +
> /*
> * The datasheet describes initialization procedure without any information
> * about registers' name/bits. So, this is all black magic to initialize
> @@ -82,7 +95,7 @@ renesas_eth_serdes_common_init_ram(struct renesas_eth_serdes_drv_data *dd)
> struct renesas_eth_serdes_channel *channel;
> int i, ret;
>
> - for (i = 0; i < R8A779F0_ETH_SERDES_NUM; i++) {
> + for (i = 0; i < dd->info->renesas_eth_serdes_num; i++) {
> channel = &dd->channel[i];
> ret = renesas_eth_serdes_reg_wait(channel, 0x026c, 0x180, BIT(0), 0x01);
> if (ret)
> @@ -265,32 +278,34 @@ static int renesas_eth_serdes_hw_init(struct renesas_eth_serdes_channel *channel
>
> reset_control_reset(dd->reset);
>
> + mdelay(1);
> +
> /* There is a slight difference in SerDes hardware behavior between
> * each version after resetting. This step is to ensure the stable
> * condition of initialization, especially for R-Car S4 v1.1.
> */
> - mdelay(1);
Why move this up?
> - iowrite32(0, dd->addr + RENESAS_ETH_SERDES_LOCAL_OFFSET);
> + if (dd->info->renesas_eth_serdes_num == r8a779f0)
That test doesn't look right to me...
> + iowrite32(0, dd->addr + RENESAS_ETH_SERDES_LOCAL_OFFSET);
>
> ret = renesas_eth_serdes_common_init_ram(dd);
> if (ret)
> return ret;
> @@ -430,12 +456,19 @@ static int renesas_eth_serdes_probe(struct platform_device *pdev)
> {
> struct renesas_eth_serdes_drv_data *dd;
> struct phy_provider *provider;
> + const struct renesas_serdes_hw_info *info;
Please use "reverse Christmas tree"-order.
> int i;
>
> dd = devm_kzalloc(&pdev->dev, sizeof(*dd), GFP_KERNEL);
> if (!dd)
> return -ENOMEM;
>
> + info = of_device_get_match_data(&pdev->dev);
> + if (info > 0)
> + dd->info = info;
> + else
> + return PTR_ERR(info);
of_device_get_match_data() returns NULL on failure instead of an error
pointer. But it can never fail here, so there is no need to check.
> +
> platform_set_drvdata(pdev, dd);
> dd->pdev = pdev;
> dd->addr = devm_platform_ioremap_resource(pdev, 0);
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
