On 2023-12-14 14:25:57 +0200, claudiu beznea wrote:
> Hi, Niklas,
>
> On 14.12.2023 14:11, Niklas Söderlund wrote:
> > Hi Claudiu,
> >
> > Thanks for your patch.
> >
> > On 2023-12-14 13:31:36 +0200, Claudiu wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> CSR.OPS bits specify the current operating mode and (according to
> >> documentation) they are updated when the operating mode change request
> >> is processed. Thus, check CSR.OPS before proceeding.
> >>
> >> Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code")
> >> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
> >> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L")
> >> Fixes: 3e3d647715d4 ("ravb: add wake-on-lan support via magic packet")
> >> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> >
> > I think the list of fixes tags can be reduced. The last item in the list
> > is the patch which adds the RAVB driver so what's the point of listing
> > the rest?
>
> In commit c156633f1353 ("Renesas Ethernet AVB driver proper") different
> features that were touched by the rest of commits in the fixes list might
> not be present. So, it might be possible that this patch to not be
> back-portable to c156633f1353 ("Renesas Ethernet AVB driver proper") but to
> other commits in the list.
All the other commits depends on commit c156633f1353 ("Renesas Ethernet
AVB driver proper"). It would be hard to add wake-on-lan to a driver
that do not exists :-)
I do not feel strongly about this so keep it as if if you wish, I just
think it looks odd.
>
> >
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >> ---
> >> drivers/net/ethernet/renesas/ravb_main.c | 47 ++++++++++++++++++++----
> >> 1 file changed, 39 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index 9178f6d60e74..ce95eb5af354 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -683,8 +683,11 @@ static int ravb_dmac_init(struct net_device *ndev)
> >>
> >> /* Setting the control will start the AVB-DMAC process. */
> >> ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
> >> + error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_OPERATION);
> >> + if (error)
> >> + netdev_err(ndev, "failed to switch device to operation mode\n");
> >
> > As you add ravb_set_reset_mode() to compliment the existing
> > ravb_set_config_mode(), would it not be coherent to also add a
> > ravb_set_operation_mode() instead of open coding it here?
>
> CSR_OPS_OPERATION is set only in this place. Reset is done in more than one
> place. Due to this I've added a function for it.
OK. Then maybe add a generic change mode operation like rswitch do in
rswitch_gwca_change_mode() ? That way you ensure any future mode changes
will always confirm the change is successful ? I'm a but worried that
future changes might forget to add the ravb_wait() to confirm a mode
change is successful and a good helper could avoid that.
>
> >
> >>
> >> - return 0;
> >> + return error;
> >> }
> >>
> >> static void ravb_get_tx_tstamp(struct net_device *ndev)
> >> @@ -1744,6 +1747,18 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
> >> return error;
> >> }
> >>
> >> +static int ravb_set_reset_mode(struct net_device *ndev)
> >
> > nit: Maybe move this to be close to ravb_set_config_mode() to co-locate
> > all mode changing logic?
>
> I've did this but not in this patch. It could be found on the final version
> of the driver proposed by
> https://lore.kernel.org/all/20231214114600.2451162-1-claudiu.beznea.uj@xxxxxxxxxxxxxx/
Why not add it in the final intended location straight away then moving
it around in a later patch? This just makes the later patch harder to
review as it moves more code around.
>
> Thank you for your review,
> Claudiu Beznea
>
> >
> >> +{
> >> + int error;
> >> +
> >> + ravb_write(ndev, CCC_OPC_RESET, CCC);
> >> + error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_RESET);
> >> + if (error)
> >> + netdev_err(ndev, "failed to switch device to reset mode\n");
> >> +
> >> + return error;
> >> +}
> >> +
> >> /* Network device open function for Ethernet AVB */
> >> static int ravb_open(struct net_device *ndev)
> >> {
> >> @@ -2551,10 +2566,11 @@ static int ravb_set_gti(struct net_device *ndev)
> >> return 0;
> >> }
> >>
> >> -static void ravb_set_config_mode(struct net_device *ndev)
> >> +static int ravb_set_config_mode(struct net_device *ndev)
> >> {
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> const struct ravb_hw_info *info = priv->info;
> >> + int error;
> >>
> >> if (info->gptp) {
> >> ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
> >> @@ -2566,6 +2582,12 @@ static void ravb_set_config_mode(struct net_device *ndev)
> >> } else {
> >> ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
> >> }
> >> +
> >> + error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
> >> + if (error)
> >> + netdev_err(ndev, "failed to switch device to config mode\n");
> >> +
> >> + return error;
> >> }
> >>
> >> /* Set tx and rx clock internal delay modes */
> >> @@ -2785,7 +2807,9 @@ static int ravb_probe(struct platform_device *pdev)
> >> ndev->ethtool_ops = &ravb_ethtool_ops;
> >>
> >> /* Set AVB config mode */
> >> - ravb_set_config_mode(ndev);
> >> + error = ravb_set_config_mode(ndev);
> >> + if (error)
> >> + goto out_disable_refclk;
> >>
> >> if (info->gptp || info->ccc_gac) {
> >> /* Set GTI value */
> >> @@ -2893,6 +2917,7 @@ static void ravb_remove(struct platform_device *pdev)
> >> struct net_device *ndev = platform_get_drvdata(pdev);
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> const struct ravb_hw_info *info = priv->info;
> >> + int error;
> >>
> >> unregister_netdev(ndev);
> >> if (info->nc_queues)
> >> @@ -2908,8 +2933,9 @@ static void ravb_remove(struct platform_device *pdev)
> >> dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
> >> priv->desc_bat_dma);
> >>
> >> - /* Set reset mode */
> >> - ravb_write(ndev, CCC_OPC_RESET, CCC);
> >> + error = ravb_set_reset_mode(ndev);
> >> + if (error)
> >> + netdev_err(ndev, "Failed to reset ndev\n");
> >>
> >> clk_disable_unprepare(priv->gptp_clk);
> >> clk_disable_unprepare(priv->refclk);
> >> @@ -2991,8 +3017,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
> >> int ret = 0;
> >>
> >> /* If WoL is enabled set reset mode to rearm the WoL logic */
> >> - if (priv->wol_enabled)
> >> - ravb_write(ndev, CCC_OPC_RESET, CCC);
> >> + if (priv->wol_enabled) {
> >> + ret = ravb_set_reset_mode(ndev);
> >> + if (ret)
> >> + return ret;
> >> + }
> >>
> >> /* All register have been reset to default values.
> >> * Restore all registers which where setup at probe time and
> >> @@ -3000,7 +3029,9 @@ static int __maybe_unused ravb_resume(struct device *dev)
> >> */
> >>
> >> /* Set AVB config mode */
> >> - ravb_set_config_mode(ndev);
> >> + ret = ravb_set_config_mode(ndev);
> >> + if (ret)
> >> + return ret;
> >>
> >> if (info->gptp || info->ccc_gac) {
> >> /* Set GTI value */
> >> --
> >> 2.39.2
> >>
> >
--
Kind Regards,
Niklas Söderlund
