| Message ID | a309a738-7fa7-3aab-4457-f7d693e6b37f@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sekhar, (And adding Sudeep since he becomes involved in this further down thread and at that point says he will re-work this proposed work around in a manner that is incorrect in a manner that is similar to this proposed work around.) On 11/21/16 08:33, Sekhar Nori wrote: > On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote: >> +static int da8xx_ddrctl_probe(struct platform_device *pdev) >> +{ >> + const struct da8xx_ddrctl_config_knob *knob; >> + const struct da8xx_ddrctl_setting *setting; >> + struct device_node *node; >> + struct resource *res; >> + void __iomem *ddrctl; >> + struct device *dev; >> + u32 reg; >> + >> + dev = &pdev->dev; >> + node = dev->of_node; >> + >> + setting = da8xx_ddrctl_get_board_settings(); >> + if (!setting) { >> + dev_err(dev, "no settings for board '%s'\n", >> + of_flat_dt_get_machine_name()); >> + return -EINVAL; >> + } > > This causes a section mismatch because of_flat_dt_get_machine_name() > has an __init annotation. I did not notice that before, sorry. > > It can be fixed with a patch like below: > > ---8<--- > diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c > index a20e7bbbcbe0..9ca5aab3ac54 100644 > --- a/drivers/memory/da8xx-ddrctl.c > +++ b/drivers/memory/da8xx-ddrctl.c > @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void) > return NULL; > } > > +static const char* da8xx_ddrctl_get_machine_name(void) > +{ > + const char *str; > + int ret; > + > + ret = of_property_read_string(of_root, "model", &str); > + if (ret) > + ret = of_property_read_string(of_root, "compatible", &str); > + > + return str; > +} > + > static int da8xx_ddrctl_probe(struct platform_device *pdev) > { > const struct da8xx_ddrctl_config_knob *knob; > @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev) > setting = da8xx_ddrctl_get_board_settings(); > if (!setting) { > dev_err(dev, "no settings for board '%s'\n", > - of_flat_dt_get_machine_name()); da8xx_ddrctl_get_board_settings() tries to match based on the "compatible" property in the root node. The "model" property in the root node has nothing to do with the failure to match. So creating and then using da8xx_ddrctl_get_machine_name() to potentially report model is not useful. It should be sufficient to simply report that no compatible matched. > + da8xx_ddrctl_get_machine_name()); > return -EINVAL; > } > ---8<--- > > A similar fix is required for the other driver in this series (patch > 2/5). I need some advise on whether I should introduce a common > function to get the machine name post kernel boot-up (I cannot see an > existing one). If yes, any advise on which file it should go into? > > Thanks, > Sekhar > >
Hi Frank, On Tuesday 22 November 2016 07:13 AM, Frank Rowand wrote: > On 11/21/16 08:33, Sekhar Nori wrote: >> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote: >>> +static int da8xx_ddrctl_probe(struct platform_device *pdev) >>> +{ >>> + const struct da8xx_ddrctl_config_knob *knob; >>> + const struct da8xx_ddrctl_setting *setting; >>> + struct device_node *node; >>> + struct resource *res; >>> + void __iomem *ddrctl; >>> + struct device *dev; >>> + u32 reg; >>> + >>> + dev = &pdev->dev; >>> + node = dev->of_node; >>> + >>> + setting = da8xx_ddrctl_get_board_settings(); >>> + if (!setting) { >>> + dev_err(dev, "no settings for board '%s'\n", >>> + of_flat_dt_get_machine_name()); >>> + return -EINVAL; >>> + } >> >> This causes a section mismatch because of_flat_dt_get_machine_name() >> has an __init annotation. I did not notice that before, sorry. >> >> It can be fixed with a patch like below: >> >> ---8<--- >> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c >> index a20e7bbbcbe0..9ca5aab3ac54 100644 >> --- a/drivers/memory/da8xx-ddrctl.c >> +++ b/drivers/memory/da8xx-ddrctl.c >> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void) >> return NULL; >> } >> >> +static const char* da8xx_ddrctl_get_machine_name(void) >> +{ >> + const char *str; >> + int ret; >> + >> + ret = of_property_read_string(of_root, "model", &str); >> + if (ret) >> + ret = of_property_read_string(of_root, "compatible", &str); >> + >> + return str; >> +} >> + >> static int da8xx_ddrctl_probe(struct platform_device *pdev) >> { >> const struct da8xx_ddrctl_config_knob *knob; >> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev) >> setting = da8xx_ddrctl_get_board_settings(); >> if (!setting) { >> dev_err(dev, "no settings for board '%s'\n", >> - of_flat_dt_get_machine_name()); > > da8xx_ddrctl_get_board_settings() tries to match based on the "compatible" > property in the root node. The "model" property in the root node has > nothing to do with the failure to match. So creating and then using > da8xx_ddrctl_get_machine_name() to potentially report model is not useful. > > It should be sufficient to simply report that no compatible matched. I agree with you on this. Even if model name is printed, you will have to go back and check the compatible anyway. But I think it will be useful to print the compatible instead of just reporting that nothing matched. Bartosz, if you agree too, could you send a fix patch just printing the compatible? Thanks, Sekhar
On 11/21/16 22:25, Sekhar Nori wrote: > Hi Frank, > > On Tuesday 22 November 2016 07:13 AM, Frank Rowand wrote: >> On 11/21/16 08:33, Sekhar Nori wrote: >>> On Monday 31 October 2016 08:15 PM, Bartosz Golaszewski wrote: >>>> +static int da8xx_ddrctl_probe(struct platform_device *pdev) >>>> +{ >>>> + const struct da8xx_ddrctl_config_knob *knob; >>>> + const struct da8xx_ddrctl_setting *setting; >>>> + struct device_node *node; >>>> + struct resource *res; >>>> + void __iomem *ddrctl; >>>> + struct device *dev; >>>> + u32 reg; >>>> + >>>> + dev = &pdev->dev; >>>> + node = dev->of_node; >>>> + >>>> + setting = da8xx_ddrctl_get_board_settings(); >>>> + if (!setting) { >>>> + dev_err(dev, "no settings for board '%s'\n", >>>> + of_flat_dt_get_machine_name()); >>>> + return -EINVAL; >>>> + } >>> >>> This causes a section mismatch because of_flat_dt_get_machine_name() >>> has an __init annotation. I did not notice that before, sorry. >>> >>> It can be fixed with a patch like below: >>> >>> ---8<--- >>> diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c >>> index a20e7bbbcbe0..9ca5aab3ac54 100644 >>> --- a/drivers/memory/da8xx-ddrctl.c >>> +++ b/drivers/memory/da8xx-ddrctl.c >>> @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void) >>> return NULL; >>> } >>> >>> +static const char* da8xx_ddrctl_get_machine_name(void) >>> +{ >>> + const char *str; >>> + int ret; >>> + >>> + ret = of_property_read_string(of_root, "model", &str); >>> + if (ret) >>> + ret = of_property_read_string(of_root, "compatible", &str); >>> + >>> + return str; >>> +} >>> + >>> static int da8xx_ddrctl_probe(struct platform_device *pdev) >>> { >>> const struct da8xx_ddrctl_config_knob *knob; >>> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev) >>> setting = da8xx_ddrctl_get_board_settings(); >>> if (!setting) { >>> dev_err(dev, "no settings for board '%s'\n", >>> - of_flat_dt_get_machine_name()); >> >> da8xx_ddrctl_get_board_settings() tries to match based on the "compatible" >> property in the root node. The "model" property in the root node has >> nothing to do with the failure to match. So creating and then using >> da8xx_ddrctl_get_machine_name() to potentially report model is not useful. >> >> It should be sufficient to simply report that no compatible matched. > > I agree with you on this. Even if model name is printed, you will have > to go back and check the compatible anyway. But I think it will be > useful to print the compatible instead of just reporting that nothing > matched. > > Bartosz, if you agree too, could you send a fix patch just printing the > compatible? Please note that the compatible property might contain several strings, not just a single string. > > Thanks, > Sekhar >
On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote: > Please note that the compatible property might contain several strings, not just > a single string. So I guess the best thing to do is to use of_property_read_string_index() and print the sting at index 0. Thanks, Sekhar
On 22/11/16 01:43, Frank Rowand wrote: > Hi Sekhar, > > (And adding Sudeep since he becomes involved in this further > down thread and at that point says he will re-work this > proposed work around in a manner that is incorrect in a > manner that is similar to this proposed work around.) > > On 11/21/16 08:33, Sekhar Nori wrote: [...] >> static int da8xx_ddrctl_probe(struct platform_device *pdev) >> { >> const struct da8xx_ddrctl_config_knob *knob; >> @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev) >> setting = da8xx_ddrctl_get_board_settings(); >> if (!setting) { >> dev_err(dev, "no settings for board '%s'\n", >> - of_flat_dt_get_machine_name()); > > da8xx_ddrctl_get_board_settings() tries to match based on the "compatible" > property in the root node. The "model" property in the root node has > nothing to do with the failure to match. So creating and then using > da8xx_ddrctl_get_machine_name() to potentially report model is not useful. > > It should be sufficient to simply report that no compatible matched. > Agreed.
On 11/22/16 21:55, Sekhar Nori wrote: > On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote: >> Please note that the compatible property might contain several strings, not just >> a single string. > > So I guess the best thing to do is to use > of_property_read_string_index() and print the sting at index 0. > > Thanks, > Sekhar If you want to print just one compatible value, you could use that method. To give all of the information needed to understand the problem, the error message would need to include all of the strings contained in the compatible property and all of the .board values in the da8xx_ddrctl_board_confs[] array (currently only one entry, but coded to allow additional entries in the future). It is hard to justify an error message that complex. I would just print an error that no match was found. -Frank
On 11/23/16 10:13, Frank Rowand wrote: > On 11/22/16 21:55, Sekhar Nori wrote: >> On Tuesday 22 November 2016 11:51 PM, Frank Rowand wrote: >>> Please note that the compatible property might contain several strings, not just >>> a single string. >> >> So I guess the best thing to do is to use >> of_property_read_string_index() and print the sting at index 0. >> >> Thanks, >> Sekhar > > If you want to print just one compatible value, you could use that method. > > To give all of the information needed to understand the problem, the error > message would need to include all of the strings contained in the compatible > property and all of the .board values in the da8xx_ddrctl_board_confs[] array > (currently only one entry, but coded to allow additional entries in the > future). > > It is hard to justify an error message that complex. > > I would just print an error that no match was found. > > -Frank I just needed to read some more emails. I see this approach was taken in the "[PATCH v4 0/2] da8xx: fix section mismatch in new drivers" series. -Frank
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c index a20e7bbbcbe0..9ca5aab3ac54 100644 --- a/drivers/memory/da8xx-ddrctl.c +++ b/drivers/memory/da8xx-ddrctl.c @@ -102,6 +102,18 @@ static const struct da8xx_ddrctl_setting *da8xx_ddrctl_get_board_settings(void) return NULL; } +static const char* da8xx_ddrctl_get_machine_name(void) +{ + const char *str; + int ret; + + ret = of_property_read_string(of_root, "model", &str); + if (ret) + ret = of_property_read_string(of_root, "compatible", &str); + + return str; +} + static int da8xx_ddrctl_probe(struct platform_device *pdev) { const struct da8xx_ddrctl_config_knob *knob; @@ -118,7 +130,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev) setting = da8xx_ddrctl_get_board_settings(); if (!setting) { dev_err(dev, "no settings for board '%s'\n", - of_flat_dt_get_machine_name()); + da8xx_ddrctl_get_machine_name()); return -EINVAL; } ---8<---