| Message ID | 1436436947-11210-1-git-send-email-josh.wu@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: > As since sama5d3, to reset the chip, we don't need to shutdown the ddr > controller. > > So add a new compatible string and new restart function for sama5d3 and > later chips. As we don't use sama5d3 ddr controller, so remove it as > well. > > Signed-off-by: Josh Wu <josh.wu@atmel.com> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > > drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c > index 36dc52f..8944b63 100644 > --- a/drivers/power/reset/at91-reset.c > +++ b/drivers/power/reset/at91-reset.c > @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, > return NOTIFY_DONE; > } > > +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, > + void *cmd) > +{ > + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), > + at91_rstc_base); > + return NOTIFY_DONE; > +} > + > static void __init at91_reset_status(struct platform_device *pdev) > { > u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); > @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) > static const struct of_device_id at91_ramc_of_match[] = { > { .compatible = "atmel,at91sam9260-sdramc", }, > { .compatible = "atmel,at91sam9g45-ddramc", }, > - { .compatible = "atmel,sama5d3-ddramc", }, > { /* sentinel */ } > }; > > static const struct of_device_id at91_reset_of_match[] = { > { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, > { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, > + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, > { /* sentinel */ } > }; > > @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) > return -ENODEV; > } > > - for_each_matching_node(np, at91_ramc_of_match) { > - at91_ramc_base[idx] = of_iomap(np, 0); > - if (!at91_ramc_base[idx]) { > - dev_err(&pdev->dev, "Could not map ram controller address\n"); > - return -ENODEV; > + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); > + at91_restart_nb.notifier_call = match->data; > + > + if (match->data != sama5d3_restart) { Using of_device_is_compatible seems more appropriate. Also, why are you changing the order of this loop and the notifier registration? Maxime
Le 09/07/2015 14:03, Maxime Ripard a écrit : > Hi, > > On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: >> As since sama5d3, to reset the chip, we don't need to shutdown the ddr >> controller. >> >> So add a new compatible string and new restart function for sama5d3 and >> later chips. As we don't use sama5d3 ddr controller, so remove it as >> well. >> >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> >> drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- >> 1 file changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c >> index 36dc52f..8944b63 100644 >> --- a/drivers/power/reset/at91-reset.c >> +++ b/drivers/power/reset/at91-reset.c >> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, >> return NOTIFY_DONE; >> } >> >> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, >> + void *cmd) >> +{ >> + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), >> + at91_rstc_base); >> + return NOTIFY_DONE; >> +} >> + >> static void __init at91_reset_status(struct platform_device *pdev) >> { >> u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); >> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) >> static const struct of_device_id at91_ramc_of_match[] = { >> { .compatible = "atmel,at91sam9260-sdramc", }, >> { .compatible = "atmel,at91sam9g45-ddramc", }, >> - { .compatible = "atmel,sama5d3-ddramc", }, >> { /* sentinel */ } >> }; >> >> static const struct of_device_id at91_reset_of_match[] = { >> { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, >> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, >> + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, >> { /* sentinel */ } >> }; >> >> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> - for_each_matching_node(np, at91_ramc_of_match) { >> - at91_ramc_base[idx] = of_iomap(np, 0); >> - if (!at91_ramc_base[idx]) { >> - dev_err(&pdev->dev, "Could not map ram controller address\n"); >> - return -ENODEV; >> + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); >> + at91_restart_nb.notifier_call = match->data; >> + >> + if (match->data != sama5d3_restart) { > > Using of_device_is_compatible seems more appropriate. > > Also, why are you changing the order of this loop and the notifier > registration? Well, it's because the sama5d3 onwards controllers don't need ramc controller. So, reverting the order seems needed. Doesn't it address your question, or did I lost track? Bye,
On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: > As since sama5d3, to reset the chip, we don't need to shutdown the ddr > controller. > > So add a new compatible string and new restart function for sama5d3 and > later chips. As we don't use sama5d3 ddr controller, so remove it as > well. > That sounds like it should be two separate patches, or am I missing something ? Guenter > Signed-off-by: Josh Wu <josh.wu@atmel.com> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > > drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c > index 36dc52f..8944b63 100644 > --- a/drivers/power/reset/at91-reset.c > +++ b/drivers/power/reset/at91-reset.c > @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, > return NOTIFY_DONE; > } > > +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, > + void *cmd) > +{ > + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), > + at91_rstc_base); > + return NOTIFY_DONE; > +} > + > static void __init at91_reset_status(struct platform_device *pdev) > { > u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); > @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) > static const struct of_device_id at91_ramc_of_match[] = { > { .compatible = "atmel,at91sam9260-sdramc", }, > { .compatible = "atmel,at91sam9g45-ddramc", }, > - { .compatible = "atmel,sama5d3-ddramc", }, > { /* sentinel */ } > }; > > static const struct of_device_id at91_reset_of_match[] = { > { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, > { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, > + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, > { /* sentinel */ } > }; > > @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) > return -ENODEV; > } > > - for_each_matching_node(np, at91_ramc_of_match) { > - at91_ramc_base[idx] = of_iomap(np, 0); > - if (!at91_ramc_base[idx]) { > - dev_err(&pdev->dev, "Could not map ram controller address\n"); > - return -ENODEV; > + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); > + at91_restart_nb.notifier_call = match->data; > + > + if (match->data != sama5d3_restart) { > + /* we need to shutdown the ddr controller, so get ramc base */ > + for_each_matching_node(np, at91_ramc_of_match) { > + at91_ramc_base[idx] = of_iomap(np, 0); > + if (!at91_ramc_base[idx]) { > + dev_err(&pdev->dev, "Could not map ram controller address\n"); > + return -ENODEV; > + } > + idx++; > } > - idx++; > } > > - match = of_match_node(at91_reset_of_match, pdev->dev.of_node); > - at91_restart_nb.notifier_call = match->data; > return register_restart_handler(&at91_restart_nb); > } > > -- > 1.9.1 >
Hi, Guenter On 7/10/2015 1:37 AM, Guenter Roeck wrote: > On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: >> As since sama5d3, to reset the chip, we don't need to shutdown the ddr >> controller. >> >> So add a new compatible string and new restart function for sama5d3 and >> later chips. As we don't use sama5d3 ddr controller, so remove it as >> well. >> > That sounds like it should be two separate patches, or am I missing something ? I think using one patch makes more sense. Maybe the commit log is not clear enough. How about put it this way: This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the reset driver of sama5d3 and later chips. As in sama5d3 or later chips, we don't have to shutdown the DDR controller before reset. Shutdown the DDR controller before reset is a workaround to avoid DDR signal driving the bus, but since sama5d3 and later chips there is no such a conflict. That means: 1. the sama5d3 reset function only need to write the rstc register and return. 2. for sama5d3, we can remove the code related with DDR controller as we don't use it at all. Best Regards, Josh Wu > > Guenter > >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> >> drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- >> 1 file changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c >> index 36dc52f..8944b63 100644 >> --- a/drivers/power/reset/at91-reset.c >> +++ b/drivers/power/reset/at91-reset.c >> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, >> return NOTIFY_DONE; >> } >> >> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, >> + void *cmd) >> +{ >> + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), >> + at91_rstc_base); >> + return NOTIFY_DONE; >> +} >> + >> static void __init at91_reset_status(struct platform_device *pdev) >> { >> u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); >> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) >> static const struct of_device_id at91_ramc_of_match[] = { >> { .compatible = "atmel,at91sam9260-sdramc", }, >> { .compatible = "atmel,at91sam9g45-ddramc", }, >> - { .compatible = "atmel,sama5d3-ddramc", }, >> { /* sentinel */ } >> }; >> >> static const struct of_device_id at91_reset_of_match[] = { >> { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, >> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, >> + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, >> { /* sentinel */ } >> }; >> >> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> - for_each_matching_node(np, at91_ramc_of_match) { >> - at91_ramc_base[idx] = of_iomap(np, 0); >> - if (!at91_ramc_base[idx]) { >> - dev_err(&pdev->dev, "Could not map ram controller address\n"); >> - return -ENODEV; >> + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); >> + at91_restart_nb.notifier_call = match->data; >> + >> + if (match->data != sama5d3_restart) { >> + /* we need to shutdown the ddr controller, so get ramc base */ >> + for_each_matching_node(np, at91_ramc_of_match) { >> + at91_ramc_base[idx] = of_iomap(np, 0); >> + if (!at91_ramc_base[idx]) { >> + dev_err(&pdev->dev, "Could not map ram controller address\n"); >> + return -ENODEV; >> + } >> + idx++; >> } >> - idx++; >> } >> >> - match = of_match_node(at91_reset_of_match, pdev->dev.of_node); >> - at91_restart_nb.notifier_call = match->data; >> return register_restart_handler(&at91_restart_nb); >> } >> >> -- >> 1.9.1 >>
Hi, Maxime On 7/9/2015 8:03 PM, Maxime Ripard wrote: > Hi, > > On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: >> As since sama5d3, to reset the chip, we don't need to shutdown the ddr >> controller. >> >> So add a new compatible string and new restart function for sama5d3 and >> later chips. As we don't use sama5d3 ddr controller, so remove it as >> well. >> >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> >> drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- >> 1 file changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c >> index 36dc52f..8944b63 100644 >> --- a/drivers/power/reset/at91-reset.c >> +++ b/drivers/power/reset/at91-reset.c >> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, >> return NOTIFY_DONE; >> } >> >> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, >> + void *cmd) >> +{ >> + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), >> + at91_rstc_base); >> + return NOTIFY_DONE; >> +} >> + >> static void __init at91_reset_status(struct platform_device *pdev) >> { >> u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); >> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) >> static const struct of_device_id at91_ramc_of_match[] = { >> { .compatible = "atmel,at91sam9260-sdramc", }, >> { .compatible = "atmel,at91sam9g45-ddramc", }, >> - { .compatible = "atmel,sama5d3-ddramc", }, >> { /* sentinel */ } >> }; >> >> static const struct of_device_id at91_reset_of_match[] = { >> { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, >> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, >> + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, >> { /* sentinel */ } >> }; >> >> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> - for_each_matching_node(np, at91_ramc_of_match) { >> - at91_ramc_base[idx] = of_iomap(np, 0); >> - if (!at91_ramc_base[idx]) { >> - dev_err(&pdev->dev, "Could not map ram controller address\n"); >> - return -ENODEV; >> + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); >> + at91_restart_nb.notifier_call = match->data; >> + >> + if (match->data != sama5d3_restart) { > Using of_device_is_compatible seems more appropriate. > > Also, why are you changing the order of this loop and the notifier > registration? I moved this order because I use the match->data to compare whether is sama5d3_restart. So I need to move this function (of_match_node) up. Best Regards, Josh Wu > > Maxime >
On Fri, Jul 10, 2015 at 09:59:53AM +0800, Josh Wu wrote: > Hi, Guenter > > On 7/10/2015 1:37 AM, Guenter Roeck wrote: > >On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: > >>As since sama5d3, to reset the chip, we don't need to shutdown the ddr > >>controller. > >> > >>So add a new compatible string and new restart function for sama5d3 and > >>later chips. As we don't use sama5d3 ddr controller, so remove it as > >>well. > >> > >That sounds like it should be two separate patches, or am I missing something ? > > I think using one patch makes more sense. Maybe the commit log is not clear > enough. How about put it this way: > > This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the > reset driver of sama5d3 and later chips. > As in sama5d3 or later chips, we don't have to shutdown the DDR controller > before reset. Shutdown the DDR controller before reset is a workaround to > avoid DDR signal driving the bus, but since sama5d3 and later chips there is > no such a conflict. > That means: > 1. the sama5d3 reset function only need to write the rstc register and > return. > 2. for sama5d3, we can remove the code related with DDR controller as we > don't use it at all. > Sorry, I don't get it. Doesn't that mean there are two distinct logical changes, which would ask for two separate patches ? Guenter
Hi, Guenter On 7/10/2015 11:14 AM, Guenter Roeck wrote: > On Fri, Jul 10, 2015 at 09:59:53AM +0800, Josh Wu wrote: >> Hi, Guenter >> >> On 7/10/2015 1:37 AM, Guenter Roeck wrote: >>> On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: >>>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr >>>> controller. >>>> >>>> So add a new compatible string and new restart function for sama5d3 and >>>> later chips. As we don't use sama5d3 ddr controller, so remove it as >>>> well. >>>> >>> That sounds like it should be two separate patches, or am I missing something ? >> I think using one patch makes more sense. Maybe the commit log is not clear >> enough. How about put it this way: >> >> This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the >> reset driver of sama5d3 and later chips. >> As in sama5d3 or later chips, we don't have to shutdown the DDR controller >> before reset. Shutdown the DDR controller before reset is a workaround to >> avoid DDR signal driving the bus, but since sama5d3 and later chips there is >> no such a conflict. >> That means: >> 1. the sama5d3 reset function only need to write the rstc register and >> return. >> 2. for sama5d3, we can remove the code related with DDR controller as we >> don't use it at all. >> > Sorry, I don't get it. Doesn't that mean there are two distinct logical > changes, which would ask for two separate patches ? The rewritten reset function for sama5d3 has no need to access the ddr controller, so this patch rewrite the reset function and remove ddr access for sama5d3. Those two changes are related and so make it as one patch is reasonable. Best Regards, Josh Wu > > Guenter
Hi Guenter, On 09/07/2015 at 20:14:38 -0700, Guenter Roeck wrote : > > This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the > > reset driver of sama5d3 and later chips. > > As in sama5d3 or later chips, we don't have to shutdown the DDR controller > > before reset. Shutdown the DDR controller before reset is a workaround to > > avoid DDR signal driving the bus, but since sama5d3 and later chips there is > > no such a conflict. > > That means: > > 1. the sama5d3 reset function only need to write the rstc register and > > return. > > 2. for sama5d3, we can remove the code related with DDR controller as we > > don't use it at all. > > > Sorry, I don't get it. Doesn't that mean there are two distinct logical > changes, which would ask for two separate patches ? I would agree with Josh and I think that only one patch is needed. There is only one change, the removal of the workaround for sama5d3 and later. As the workaround is using a table of compatibles to remap the ram controller and the one for sama5d3 is not used because it is not needed, I think it makes sense to remove it in that same patch. The logical change here is the removal of the workaround.
Hi, On 09/07/2015 at 18:15:46 +0800, Josh Wu wrote : > As since sama5d3, to reset the chip, we don't need to shutdown the ddr > controller. > > So add a new compatible string and new restart function for sama5d3 and > later chips. As we don't use sama5d3 ddr controller, so remove it as > well. > > Signed-off-by: Josh Wu <josh.wu@atmel.com> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > > drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c > index 36dc52f..8944b63 100644 > --- a/drivers/power/reset/at91-reset.c > +++ b/drivers/power/reset/at91-reset.c > @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, > return NOTIFY_DONE; > } > > +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, > + void *cmd) Please align that line properly. > +{ > + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), > + at91_rstc_base); That one too. > + return NOTIFY_DONE; > +} > + > static void __init at91_reset_status(struct platform_device *pdev) > { > u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); > @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) > static const struct of_device_id at91_ramc_of_match[] = { > { .compatible = "atmel,at91sam9260-sdramc", }, > { .compatible = "atmel,at91sam9g45-ddramc", }, > - { .compatible = "atmel,sama5d3-ddramc", }, > { /* sentinel */ } > }; > > static const struct of_device_id at91_reset_of_match[] = { > { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, > { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, > + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, > { /* sentinel */ } > }; > > @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) > return -ENODEV; > } > > - for_each_matching_node(np, at91_ramc_of_match) { > - at91_ramc_base[idx] = of_iomap(np, 0); > - if (!at91_ramc_base[idx]) { > - dev_err(&pdev->dev, "Could not map ram controller address\n"); > - return -ENODEV; > + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); > + at91_restart_nb.notifier_call = match->data; > + > + if (match->data != sama5d3_restart) { This doesn't scale well. I would create a structure with a pointer to the restart function and a boolean or a bitfield to store whether the workaround is needed. Use that structure in your match data. Then, you won't need to reorder anything. > + /* we need to shutdown the ddr controller, so get ramc base */ > + for_each_matching_node(np, at91_ramc_of_match) { > + at91_ramc_base[idx] = of_iomap(np, 0); > + if (!at91_ramc_base[idx]) { > + dev_err(&pdev->dev, "Could not map ram controller address\n"); > + return -ENODEV; > + } > + idx++; > } > - idx++; > } > > - match = of_match_node(at91_reset_of_match, pdev->dev.of_node); > - at91_restart_nb.notifier_call = match->data; > return register_restart_handler(&at91_restart_nb); > }
On Fri, Jul 10, 2015 at 11:06:52AM +0800, Josh Wu wrote: > Hi, Maxime > > On 7/9/2015 8:03 PM, Maxime Ripard wrote: > >Hi, > > > >On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: > >>As since sama5d3, to reset the chip, we don't need to shutdown the ddr > >>controller. > >> > >>So add a new compatible string and new restart function for sama5d3 and > >>later chips. As we don't use sama5d3 ddr controller, so remove it as > >>well. > >> > >>Signed-off-by: Josh Wu <josh.wu@atmel.com> > >>Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > >>--- > >> > >> drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- > >> 1 file changed, 21 insertions(+), 9 deletions(-) > >> > >>diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c > >>index 36dc52f..8944b63 100644 > >>--- a/drivers/power/reset/at91-reset.c > >>+++ b/drivers/power/reset/at91-reset.c > >>@@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, > >> return NOTIFY_DONE; > >> } > >>+static int sama5d3_restart(struct notifier_block *this, unsigned long mode, > >>+ void *cmd) > >>+{ > >>+ writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), > >>+ at91_rstc_base); > >>+ return NOTIFY_DONE; > >>+} > >>+ > >> static void __init at91_reset_status(struct platform_device *pdev) > >> { > >> u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); > >>@@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) > >> static const struct of_device_id at91_ramc_of_match[] = { > >> { .compatible = "atmel,at91sam9260-sdramc", }, > >> { .compatible = "atmel,at91sam9g45-ddramc", }, > >>- { .compatible = "atmel,sama5d3-ddramc", }, > >> { /* sentinel */ } > >> }; > >> static const struct of_device_id at91_reset_of_match[] = { > >> { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, > >> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, > >>+ { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, > >> { /* sentinel */ } > >> }; > >>@@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) > >> return -ENODEV; > >> } > >>- for_each_matching_node(np, at91_ramc_of_match) { > >>- at91_ramc_base[idx] = of_iomap(np, 0); > >>- if (!at91_ramc_base[idx]) { > >>- dev_err(&pdev->dev, "Could not map ram controller address\n"); > >>- return -ENODEV; > >>+ match = of_match_node(at91_reset_of_match, pdev->dev.of_node); > >>+ at91_restart_nb.notifier_call = match->data; > >>+ > >>+ if (match->data != sama5d3_restart) { > >Using of_device_is_compatible seems more appropriate. > > > >Also, why are you changing the order of this loop and the notifier > >registration? > > I moved this order because I use the match->data to compare whether is > sama5d3_restart. So I need to move this function (of_match_node) up. Ah right, my bad. Still, testing against the kernel pointer is not that great. It would be great to use something explicit instead, like of_device_is_compatible. Maxime
On Fri, Jul 10, 2015 at 08:03:50AM +0200, Alexandre Belloni wrote: > > static const struct of_device_id at91_reset_of_match[] = { > > { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, > > { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, > > + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, > > { /* sentinel */ } > > }; > > > > @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) > > return -ENODEV; > > } > > > > - for_each_matching_node(np, at91_ramc_of_match) { > > - at91_ramc_base[idx] = of_iomap(np, 0); > > - if (!at91_ramc_base[idx]) { > > - dev_err(&pdev->dev, "Could not map ram controller address\n"); > > - return -ENODEV; > > + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); > > + at91_restart_nb.notifier_call = match->data; > > + > > + if (match->data != sama5d3_restart) { > > This doesn't scale well. I would create a structure with a pointer to > the restart function and a boolean or a bitfield to store whether the > workaround is needed. Use that structure in your match data. Then, you > won't need to reorder anything. Maybe it simply doesn't need to scale (yet). You have a single exception here. Maybe you will have only this one in the future, maybe you won't, but for now, that solution looks a bit overkill. Maxime
Hi, Alexandre On 7/10/2015 2:03 PM, Alexandre Belloni wrote: > Hi, > > On 09/07/2015 at 18:15:46 +0800, Josh Wu wrote : >> As since sama5d3, to reset the chip, we don't need to shutdown the ddr >> controller. >> >> So add a new compatible string and new restart function for sama5d3 and >> later chips. As we don't use sama5d3 ddr controller, so remove it as >> well. >> >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> >> drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- >> 1 file changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c >> index 36dc52f..8944b63 100644 >> --- a/drivers/power/reset/at91-reset.c >> +++ b/drivers/power/reset/at91-reset.c >> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, >> return NOTIFY_DONE; >> } >> >> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, >> + void *cmd) > Please align that line properly. Ok. > >> +{ >> + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), >> + at91_rstc_base); > That one too. I'll align them in v2. > >> + return NOTIFY_DONE; >> +} >> + >> static void __init at91_reset_status(struct platform_device *pdev) >> { >> u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); >> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) >> static const struct of_device_id at91_ramc_of_match[] = { >> { .compatible = "atmel,at91sam9260-sdramc", }, >> { .compatible = "atmel,at91sam9g45-ddramc", }, >> - { .compatible = "atmel,sama5d3-ddramc", }, >> { /* sentinel */ } >> }; >> >> static const struct of_device_id at91_reset_of_match[] = { >> { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, >> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, >> + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, >> { /* sentinel */ } >> }; >> >> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) >> return -ENODEV; >> } >> >> - for_each_matching_node(np, at91_ramc_of_match) { >> - at91_ramc_base[idx] = of_iomap(np, 0); >> - if (!at91_ramc_base[idx]) { >> - dev_err(&pdev->dev, "Could not map ram controller address\n"); >> - return -ENODEV; >> + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); >> + at91_restart_nb.notifier_call = match->data; >> + >> + if (match->data != sama5d3_restart) { > This doesn't scale well. I would create a structure with a pointer to > the restart function and a boolean or a bitfield to store whether the > workaround is needed. Use that structure in your match data. Then, you > won't need to reorder anything. I would agree with Maxime. Currently all latest chip reset function is compatible with the atmel,sama5d3-rstc. So check compatible string is enough for now. But of cause if we have other incompatible reset in future with new chip, the structure like you said is needed. Thanks. Best Regards, Josh Wu > >> + /* we need to shutdown the ddr controller, so get ramc base */ >> + for_each_matching_node(np, at91_ramc_of_match) { >> + at91_ramc_base[idx] = of_iomap(np, 0); >> + if (!at91_ramc_base[idx]) { >> + dev_err(&pdev->dev, "Could not map ram controller address\n"); >> + return -ENODEV; >> + } >> + idx++; >> } >> - idx++; >> } >> >> - match = of_match_node(at91_reset_of_match, pdev->dev.of_node); >> - at91_restart_nb.notifier_call = match->data; >> return register_restart_handler(&at91_restart_nb); >> }
On 7/10/2015 2:54 PM, Maxime Ripard wrote: > On Fri, Jul 10, 2015 at 11:06:52AM +0800, Josh Wu wrote: >> Hi, Maxime >> >> On 7/9/2015 8:03 PM, Maxime Ripard wrote: >>> Hi, >>> >>> On Thu, Jul 09, 2015 at 06:15:46PM +0800, Josh Wu wrote: >>>> As since sama5d3, to reset the chip, we don't need to shutdown the ddr >>>> controller. >>>> >>>> So add a new compatible string and new restart function for sama5d3 and >>>> later chips. As we don't use sama5d3 ddr controller, so remove it as >>>> well. >>>> >>>> Signed-off-by: Josh Wu <josh.wu@atmel.com> >>>> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >>>> --- >>>> >>>> drivers/power/reset/at91-reset.c | 30 +++++++++++++++++++++--------- >>>> 1 file changed, 21 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c >>>> index 36dc52f..8944b63 100644 >>>> --- a/drivers/power/reset/at91-reset.c >>>> +++ b/drivers/power/reset/at91-reset.c >>>> @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, >>>> return NOTIFY_DONE; >>>> } >>>> +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, >>>> + void *cmd) >>>> +{ >>>> + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), >>>> + at91_rstc_base); >>>> + return NOTIFY_DONE; >>>> +} >>>> + >>>> static void __init at91_reset_status(struct platform_device *pdev) >>>> { >>>> u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); >>>> @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) >>>> static const struct of_device_id at91_ramc_of_match[] = { >>>> { .compatible = "atmel,at91sam9260-sdramc", }, >>>> { .compatible = "atmel,at91sam9g45-ddramc", }, >>>> - { .compatible = "atmel,sama5d3-ddramc", }, >>>> { /* sentinel */ } >>>> }; >>>> static const struct of_device_id at91_reset_of_match[] = { >>>> { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, >>>> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, >>>> + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, >>>> { /* sentinel */ } >>>> }; >>>> @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) >>>> return -ENODEV; >>>> } >>>> - for_each_matching_node(np, at91_ramc_of_match) { >>>> - at91_ramc_base[idx] = of_iomap(np, 0); >>>> - if (!at91_ramc_base[idx]) { >>>> - dev_err(&pdev->dev, "Could not map ram controller address\n"); >>>> - return -ENODEV; >>>> + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); >>>> + at91_restart_nb.notifier_call = match->data; >>>> + >>>> + if (match->data != sama5d3_restart) { >>> Using of_device_is_compatible seems more appropriate. >>> >>> Also, why are you changing the order of this loop and the notifier >>> registration? >> I moved this order because I use the match->data to compare whether is >> sama5d3_restart. So I need to move this function (of_match_node) up. > Ah right, my bad. > > Still, testing against the kernel pointer is not that great. > > It would be great to use something explicit instead, like > of_device_is_compatible. I agree. I will use of_device_is_compatible() in v2. And that can avoid the order change in the loop as well. Thanks. Best Regards, Josh Wu > > Maxime >
Hi, On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : > I would agree with Maxime. Currently all latest chip reset function is > compatible with the atmel,sama5d3-rstc. > So check compatible string is enough for now. > But of cause if we have other incompatible reset in future with new chip, > the structure like you said is needed. We managed to avoid using of_machine_is_compatible() in all the at91 drivers. I'd like to keep it that way. It was painful enough to remove all those cpu_is_at91xxx calls. Also, using it is trying to match strings and will result in longer boot times.
On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: > Hi, > > On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : > > I would agree with Maxime. Currently all latest chip reset function is > > compatible with the atmel,sama5d3-rstc. > > So check compatible string is enough for now. > > But of cause if we have other incompatible reset in future with new chip, > > the structure like you said is needed. > > We managed to avoid using of_machine_is_compatible() in all the at91 > drivers. I'd like to keep it that way. It was painful enough to remove > all those cpu_is_at91xxx calls. That's your call... > Also, using it is trying to match strings and will result in longer boot > times. Have you looked at the implementation of of_match_device? If that's really a concern to you, you should actually avoid it. Maxime
On 10/07/2015 at 14:31:48 +0200, Maxime Ripard wrote : > On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: > > Hi, > > > > On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : > > > I would agree with Maxime. Currently all latest chip reset function is > > > compatible with the atmel,sama5d3-rstc. > > > So check compatible string is enough for now. > > > But of cause if we have other incompatible reset in future with new chip, > > > the structure like you said is needed. > > > > We managed to avoid using of_machine_is_compatible() in all the at91 > > drivers. I'd like to keep it that way. It was painful enough to remove > > all those cpu_is_at91xxx calls. > > That's your call... > > > Also, using it is trying to match strings and will result in longer boot > > times. > > Have you looked at the implementation of of_match_device? If that's > really a concern to you, you should actually avoid it. > Indeed, I misread. of_device_is_compatible is acceptable, of_machine_is_compatible is not :)
Le 10/07/2015 14:31, Maxime Ripard a écrit : > On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: >> Hi, >> >> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : >>> I would agree with Maxime. Currently all latest chip reset function is >>> compatible with the atmel,sama5d3-rstc. >>> So check compatible string is enough for now. >>> But of cause if we have other incompatible reset in future with new chip, >>> the structure like you said is needed. >> >> We managed to avoid using of_machine_is_compatible() in all the at91 >> drivers. I'd like to keep it that way. It was painful enough to remove >> all those cpu_is_at91xxx calls. > > That's your call... > >> Also, using it is trying to match strings and will result in longer boot >> times. > > Have you looked at the implementation of of_match_device? If that's > really a concern to you, you should actually avoid it. I agree: let's keep it simple and use of_match_device(). Bye,
On Fri, Jul 10, 2015 at 07:56:58AM +0200, Alexandre Belloni wrote: > Hi Guenter, > > On 09/07/2015 at 20:14:38 -0700, Guenter Roeck wrote : > > > This patch introduces a new compatible string: "atmel,sama5d3-rstc" for the > > > reset driver of sama5d3 and later chips. > > > As in sama5d3 or later chips, we don't have to shutdown the DDR controller > > > before reset. Shutdown the DDR controller before reset is a workaround to > > > avoid DDR signal driving the bus, but since sama5d3 and later chips there is > > > no such a conflict. > > > That means: > > > 1. the sama5d3 reset function only need to write the rstc register and > > > return. > > > 2. for sama5d3, we can remove the code related with DDR controller as we > > > don't use it at all. > > > > > Sorry, I don't get it. Doesn't that mean there are two distinct logical > > changes, which would ask for two separate patches ? > > I would agree with Josh and I think that only one patch is needed. There > is only one change, the removal of the workaround for sama5d3 and later. > > As the workaround is using a table of compatibles to remap the ram > controller and the one for sama5d3 is not used because it is not needed, > I think it makes sense to remove it in that same patch. The logical > change here is the removal of the workaround. > Ok, makes sense. Thanks, Guenter
On 7/11/2015 12:12 AM, Nicolas Ferre wrote: > Le 10/07/2015 14:31, Maxime Ripard a écrit : >> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: >>> Hi, >>> >>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : >>>> I would agree with Maxime. Currently all latest chip reset function is >>>> compatible with the atmel,sama5d3-rstc. >>>> So check compatible string is enough for now. >>>> But of cause if we have other incompatible reset in future with new chip, >>>> the structure like you said is needed. >>> We managed to avoid using of_machine_is_compatible() in all the at91 >>> drivers. I'd like to keep it that way. It was painful enough to remove >>> all those cpu_is_at91xxx calls. >> That's your call... >> >>> Also, using it is trying to match strings and will result in longer boot >>> times. >> Have you looked at the implementation of of_match_device? If that's >> really a concern to you, you should actually avoid it. > I agree: let's keep it simple and use of_match_device(). Ok. I will keep it as it is now: use the (match->data != sama5d3_restart) for the condition. About the of_match_device(), I prefer to keep not changing the code and still use of_match_node(). Since of_match_device() is a wrapper for the of_match_node(). And dev->of_node and at91_reset_of_match is valid, so we can just use of_match_node() directly. Is it sound okay for us? Best Regards, Josh Wu > > Bye,
Hi Josh, On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote: > On 7/11/2015 12:12 AM, Nicolas Ferre wrote: > >Le 10/07/2015 14:31, Maxime Ripard a écrit : > >>On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: > >>>Hi, > >>> > >>>On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : > >>>>I would agree with Maxime. Currently all latest chip reset function is > >>>>compatible with the atmel,sama5d3-rstc. > >>>>So check compatible string is enough for now. > >>>>But of cause if we have other incompatible reset in future with new chip, > >>>>the structure like you said is needed. > >>>We managed to avoid using of_machine_is_compatible() in all the at91 > >>>drivers. I'd like to keep it that way. It was painful enough to remove > >>>all those cpu_is_at91xxx calls. > >>That's your call... > >> > >>>Also, using it is trying to match strings and will result in longer boot > >>>times. > >>Have you looked at the implementation of of_match_device? If that's > >>really a concern to you, you should actually avoid it. > >I agree: let's keep it simple and use of_match_device(). > > Ok. I will keep it as it is now: use the (match->data != sama5d3_restart) > for the condition. I'm not just that's been an option in our discussion so far. Nicolas said that he was agreeing with me, but at the same time said the complete opposite of what I was arguing for, so I'm not really sure what's really on his mind, but the two options that were discussed were to remove that test, and either: - Use of_device_is_compatible to prevent the loop execution - define a structure with a flag to say whether you need the ram controller quirk or not, and test that flag. Maxime
Hi, Maxime On 7/20/2015 3:52 PM, Maxime Ripard wrote: > Hi Josh, > > On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote: >> On 7/11/2015 12:12 AM, Nicolas Ferre wrote: >>> Le 10/07/2015 14:31, Maxime Ripard a écrit : >>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: >>>>> Hi, >>>>> >>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : >>>>>> I would agree with Maxime. Currently all latest chip reset function is >>>>>> compatible with the atmel,sama5d3-rstc. >>>>>> So check compatible string is enough for now. >>>>>> But of cause if we have other incompatible reset in future with new chip, >>>>>> the structure like you said is needed. >>>>> We managed to avoid using of_machine_is_compatible() in all the at91 >>>>> drivers. I'd like to keep it that way. It was painful enough to remove >>>>> all those cpu_is_at91xxx calls. >>>> That's your call... >>>> >>>>> Also, using it is trying to match strings and will result in longer boot >>>>> times. >>>> Have you looked at the implementation of of_match_device? If that's >>>> really a concern to you, you should actually avoid it. >>> I agree: let's keep it simple and use of_match_device(). >> Ok. I will keep it as it is now: use the (match->data != sama5d3_restart) >> for the condition. > I'm not just that's been an option in our discussion so far. > > Nicolas said that he was agreeing with me, but at the same time said > the complete opposite of what I was arguing for, so I'm not really > sure what's really on his mind, but the two options that were > discussed were to remove that test, and either: > > - Use of_device_is_compatible to prevent the loop execution Thank you for explaining, it is clear to me. I'll take this above option. As the of_device_is_compatible() almost same as of_match_node()/of_match_device(). Except that of_device_is_compatible() is more efficient (in this case It calls __of_device_is_compatible() directly) than of_match_node/of_match_device. > > - define a structure with a flag to say whether you need the ram > controller quirk or not, and test that flag. > > Maxime > Best Regards, Josh Wu
Le 20/07/2015 10:35, Josh Wu a écrit : > Hi, Maxime > > On 7/20/2015 3:52 PM, Maxime Ripard wrote: >> Hi Josh, >> >> On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote: >>> On 7/11/2015 12:12 AM, Nicolas Ferre wrote: >>>> Le 10/07/2015 14:31, Maxime Ripard a écrit : >>>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : >>>>>>> I would agree with Maxime. Currently all latest chip reset function is >>>>>>> compatible with the atmel,sama5d3-rstc. >>>>>>> So check compatible string is enough for now. >>>>>>> But of cause if we have other incompatible reset in future with new chip, >>>>>>> the structure like you said is needed. >>>>>> We managed to avoid using of_machine_is_compatible() in all the at91 >>>>>> drivers. I'd like to keep it that way. It was painful enough to remove >>>>>> all those cpu_is_at91xxx calls. >>>>> That's your call... >>>>> >>>>>> Also, using it is trying to match strings and will result in longer boot >>>>>> times. >>>>> Have you looked at the implementation of of_match_device? If that's >>>>> really a concern to you, you should actually avoid it. >>>> I agree: let's keep it simple and use of_match_device(). >>> Ok. I will keep it as it is now: use the (match->data != sama5d3_restart) >>> for the condition. >> I'm not just that's been an option in our discussion so far. >> >> Nicolas said that he was agreeing with me, but at the same time said >> the complete opposite of what I was arguing for, so I'm not really >> sure what's really on his mind, but the two options that were >> discussed were to remove that test, and either: >> >> - Use of_device_is_compatible to prevent the loop execution > > Thank you for explaining, it is clear to me. > > I'll take this above option. As the of_device_is_compatible() almost > same as of_match_node()/of_match_device(). Except that > of_device_is_compatible() is more efficient (in this case It calls > __of_device_is_compatible() directly) than of_match_node/of_match_device. Yes, I was pushing for this solution... >> - define a structure with a flag to say whether you need the ram >> controller quirk or not, and test that flag. and not for this one, that's all. I wrongly added the name of the improper function to use too quickly picked from your discussion with Alex. So, all is clear now. Bye,
On 7/20/2015 4:35 PM, Josh Wu wrote: > Hi, Maxime > > On 7/20/2015 3:52 PM, Maxime Ripard wrote: >> Hi Josh, >> >> On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote: >>> On 7/11/2015 12:12 AM, Nicolas Ferre wrote: >>>> Le 10/07/2015 14:31, Maxime Ripard a écrit : >>>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : >>>>>>> I would agree with Maxime. Currently all latest chip reset >>>>>>> function is >>>>>>> compatible with the atmel,sama5d3-rstc. >>>>>>> So check compatible string is enough for now. >>>>>>> But of cause if we have other incompatible reset in future with >>>>>>> new chip, >>>>>>> the structure like you said is needed. >>>>>> We managed to avoid using of_machine_is_compatible() in all the at91 >>>>>> drivers. I'd like to keep it that way. It was painful enough to >>>>>> remove >>>>>> all those cpu_is_at91xxx calls. >>>>> That's your call... >>>>> >>>>>> Also, using it is trying to match strings and will result in >>>>>> longer boot >>>>>> times. >>>>> Have you looked at the implementation of of_match_device? If that's >>>>> really a concern to you, you should actually avoid it. >>>> I agree: let's keep it simple and use of_match_device(). >>> Ok. I will keep it as it is now: use the (match->data != >>> sama5d3_restart) >>> for the condition. >> I'm not just that's been an option in our discussion so far. >> >> Nicolas said that he was agreeing with me, but at the same time said >> the complete opposite of what I was arguing for, so I'm not really >> sure what's really on his mind, but the two options that were >> discussed were to remove that test, and either: >> >> - Use of_device_is_compatible to prevent the loop execution > > Thank you for explaining, it is clear to me. > > I'll take this above option. As the of_device_is_compatible() almost > same as of_match_node()/of_match_device(). Except that > of_device_is_compatible() is more efficient (in this case It calls > __of_device_is_compatible() directly) than of_match_node/of_match_device. Sorry, after checking the code a little, I'd say use the of_match_node instead of of_device_is_compatible() is better. Since After check the of_device_is_compatible() we also need to call of_match_node() again. So the simplest way is just get the match data by of_match_node() first, then check the match->data. like following: match = of_match_node(at91_reset_of_match, pdev->dev.of_node); if (match->data != sama5d3_restart) { /* we need to shutdown the ddr controller, so get ramc base */ for_each_matching_node(np, at91_ramc_of_match) { at91_ramc_base[idx] = of_iomap(np, 0); if (!at91_ramc_base[idx]) { dev_err(&pdev->dev, "Could not map ram controller address\n"); return -ENODEV; } idx++; } } at91_restart_nb.notifier_call = match->data; Best Regards, Josh Wu > >> >> - define a structure with a flag to say whether you need the ram >> controller quirk or not, and test that flag. >> >> Maxime >> > > Best Regards, > Josh Wu
On 7/20/2015 4:44 PM, Josh Wu wrote: > On 7/20/2015 4:35 PM, Josh Wu wrote: >> Hi, Maxime >> >> On 7/20/2015 3:52 PM, Maxime Ripard wrote: >>> Hi Josh, >>> >>> On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote: >>>> On 7/11/2015 12:12 AM, Nicolas Ferre wrote: >>>>> Le 10/07/2015 14:31, Maxime Ripard a écrit : >>>>>> On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote : >>>>>>>> I would agree with Maxime. Currently all latest chip reset >>>>>>>> function is >>>>>>>> compatible with the atmel,sama5d3-rstc. >>>>>>>> So check compatible string is enough for now. >>>>>>>> But of cause if we have other incompatible reset in future with >>>>>>>> new chip, >>>>>>>> the structure like you said is needed. >>>>>>> We managed to avoid using of_machine_is_compatible() in all the >>>>>>> at91 >>>>>>> drivers. I'd like to keep it that way. It was painful enough to >>>>>>> remove >>>>>>> all those cpu_is_at91xxx calls. >>>>>> That's your call... >>>>>> >>>>>>> Also, using it is trying to match strings and will result in >>>>>>> longer boot >>>>>>> times. >>>>>> Have you looked at the implementation of of_match_device? If that's >>>>>> really a concern to you, you should actually avoid it. >>>>> I agree: let's keep it simple and use of_match_device(). >>>> Ok. I will keep it as it is now: use the (match->data != >>>> sama5d3_restart) >>>> for the condition. >>> I'm not just that's been an option in our discussion so far. >>> >>> Nicolas said that he was agreeing with me, but at the same time said >>> the complete opposite of what I was arguing for, so I'm not really >>> sure what's really on his mind, but the two options that were >>> discussed were to remove that test, and either: >>> >>> - Use of_device_is_compatible to prevent the loop execution >> >> Thank you for explaining, it is clear to me. >> >> I'll take this above option. As the of_device_is_compatible() almost >> same as of_match_node()/of_match_device(). Except that >> of_device_is_compatible() is more efficient (in this case It calls >> __of_device_is_compatible() directly) than >> of_match_node/of_match_device. > > Sorry, after checking the code a little, I'd say use the of_match_node > instead of of_device_is_compatible() is better. Since After check the > of_device_is_compatible() we also need to call of_match_node() again. Okay, Please forget above reply. As Maxime said test the pointer is not good solution here. So I'll sent out v2 which use of_device_is_compatible(). Best Regards, Josh Wu
diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c index 36dc52f..8944b63 100644 --- a/drivers/power/reset/at91-reset.c +++ b/drivers/power/reset/at91-reset.c @@ -123,6 +123,14 @@ static int at91sam9g45_restart(struct notifier_block *this, unsigned long mode, return NOTIFY_DONE; } +static int sama5d3_restart(struct notifier_block *this, unsigned long mode, + void *cmd) +{ + writel(cpu_to_le32(AT91_RSTC_KEY | AT91_RSTC_PERRST | AT91_RSTC_PROCRST), + at91_rstc_base); + return NOTIFY_DONE; +} + static void __init at91_reset_status(struct platform_device *pdev) { u32 reg = readl(at91_rstc_base + AT91_RSTC_SR); @@ -155,13 +163,13 @@ static void __init at91_reset_status(struct platform_device *pdev) static const struct of_device_id at91_ramc_of_match[] = { { .compatible = "atmel,at91sam9260-sdramc", }, { .compatible = "atmel,at91sam9g45-ddramc", }, - { .compatible = "atmel,sama5d3-ddramc", }, { /* sentinel */ } }; static const struct of_device_id at91_reset_of_match[] = { { .compatible = "atmel,at91sam9260-rstc", .data = at91sam9260_restart }, { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart }, + { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart }, { /* sentinel */ } }; @@ -181,17 +189,21 @@ static int at91_reset_of_probe(struct platform_device *pdev) return -ENODEV; } - for_each_matching_node(np, at91_ramc_of_match) { - at91_ramc_base[idx] = of_iomap(np, 0); - if (!at91_ramc_base[idx]) { - dev_err(&pdev->dev, "Could not map ram controller address\n"); - return -ENODEV; + match = of_match_node(at91_reset_of_match, pdev->dev.of_node); + at91_restart_nb.notifier_call = match->data; + + if (match->data != sama5d3_restart) { + /* we need to shutdown the ddr controller, so get ramc base */ + for_each_matching_node(np, at91_ramc_of_match) { + at91_ramc_base[idx] = of_iomap(np, 0); + if (!at91_ramc_base[idx]) { + dev_err(&pdev->dev, "Could not map ram controller address\n"); + return -ENODEV; + } + idx++; } - idx++; } - match = of_match_node(at91_reset_of_match, pdev->dev.of_node); - at91_restart_nb.notifier_call = match->data; return register_restart_handler(&at91_restart_nb); }