| Message ID | 6c72eae7c1fbfd2b96d5dc1c7feb251bdbe3e591.1502091561.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 07, 2017 at 09:39:24AM +0200, Michal Simek wrote: > From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> > > Adds platform specific structures, so that we can add > different IP support later using quirks. > > Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > Changes in v2: Subject says "2/6" but I can't find 1/6 in my mbox. Nothing in spam folder either. What's up? Are you using the --cover-letter option to git format-patch ?
On 11.8.2017 11:09, Borislav Petkov wrote: > On Mon, Aug 07, 2017 at 09:39:24AM +0200, Michal Simek wrote: >> From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> >> >> Adds platform specific structures, so that we can add >> different IP support later using quirks. >> >> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> Changes in v2: > > Subject says "2/6" but I can't find 1/6 in my mbox. Nothing in spam > folder either. What's up? https://lkml.org/lkml/2017/8/7/105 ACK by Rob yesterday. > > Are you using the --cover-letter option to git format-patch ? I am using patman - u-boot tools for sending patch. It read MAINTAINERS file and copy right people. It has option to explicitly CC people. I need to resend this as v3 anyway that's why I will add you there. Thanks, Michal
On Fri, Aug 11, 2017 at 11:16:15AM +0200, Michal Simek wrote: > https://lkml.org/lkml/2017/8/7/105 > ACK by Rob yesterday. Ok, pls bounce it to me as I don't have it. Also, pls refrain from using lkml.org. Simply do: http://lkml.kernel.org/r/<Message-ID> > I am using patman - u-boot tools for sending patch. It read MAINTAINERS > file and copy right people. > It has option to explicitly CC people. I need to resend this as v3 > anyway that's why I will add you there. Yes, if you send cross-maintainer changes, please make sure to CC them on all patches because otherwise stuff like that happens and people start wondering where is the rest and so on. Thx.
On 11.8.2017 11:22, Borislav Petkov wrote: > On Fri, Aug 11, 2017 at 11:16:15AM +0200, Michal Simek wrote: >> https://lkml.org/lkml/2017/8/7/105 >> ACK by Rob yesterday. > > Ok, pls bounce it to me as I don't have it. ok. > > Also, pls refrain from using lkml.org. Simply do: > > http://lkml.kernel.org/r/<Message-ID> Interesting. I have never seen this before. http://lkml.kernel.org/r/91fd6532076e4c905b5a228d852bba4941c54a28.1502091561.git.michal.simek@xilinx.com > >> I am using patman - u-boot tools for sending patch. It read MAINTAINERS >> file and copy right people. >> It has option to explicitly CC people. I need to resend this as v3 >> anyway that's why I will add you there. > > Yes, if you send cross-maintainer changes, please make sure to CC them on all > patches because otherwise stuff like that happens and people start wondering > where is the rest and so on. ok. Anyway do you see something else what I should fix in v3? Thanks, Michal
On Mon, Aug 07, 2017 at 09:39:24AM +0200, Michal Simek wrote: > From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> > > Adds platform specific structures, so that we can add "Add platform-specific structures, ..." i.e., "Do this and do that" formulation. Fix that in the following patches too pls. > different IP support later using quirks. > > Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > Changes in v2: > - Update commit message > - Update patch subject > - Update kernel-doc description for struct synps_platform_data > - Change synps_platform_data pointer names and update code > > drivers/edac/synopsys_edac.c | 70 ++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 58 insertions(+), 12 deletions(-) > > diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c > index 0c9c59e2b5a3..293380f884fe 100644 > --- a/drivers/edac/synopsys_edac.c > +++ b/drivers/edac/synopsys_edac.c > @@ -22,6 +22,7 @@ > #include <linux/edac.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/of.h> > > #include "edac_module.h" > > @@ -95,6 +96,9 @@ > #define SCRUB_MODE_MASK 0x7 > #define SCRUB_MODE_SECDED 0x4 > > +/* DDR ECC Quirks */ quirks? How is reporting ECC with an interrupt a quirk? Or whatever that define is... > +#define DDR_ECC_INTR_SUPPORT BIT(0) > + > /** > * struct ecc_error_info - ECC error log information > * @row: Row number > @@ -130,6 +134,7 @@ struct synps_ecc_status { > * @baseaddr: Base address of the DDR controller > * @message: Buffer for framing the event specific info > * @stat: ECC status information > + * @p_data: Pointer to platform data > * @ce_cnt: Correctable Error count > * @ue_cnt: Uncorrectable Error count > */ > @@ -137,11 +142,29 @@ struct synps_edac_priv { > void __iomem *baseaddr; > char message[SYNPS_EDAC_MSG_SIZE]; > struct synps_ecc_status stat; > + const struct synps_platform_data *p_data; > u32 ce_cnt; > u32 ue_cnt; > }; > > /** > + * struct synps_platform_data - synps platform data structure > + * @edac_geterror_info: function which returns the current ecc error info > + * @edac_get_mtype: function which returns the memory type > + * @edac_get_dtype: function which returns the DIMM type > + * @edac_get_eccstate: function which returns the ecc enable/disable status > + * @quirks: a bitfield of quirks > + */ > +struct synps_platform_data { > + int (*edac_geterror_info)(void __iomem *base, > + struct synps_ecc_status *p); > + enum mem_type (*edac_get_mtype)(const void __iomem *base); > + enum dev_type (*edac_get_dtype)(const void __iomem *base); > + bool (*edac_get_eccstate)(void __iomem *base); > + int quirks; You can just as well drop the "edac_" prefix too - those are driver-local things and not EDAC-core. > +}; > + > +/** > * synps_edac_geterror_info - Get the current ecc error info > * @base: Pointer to the base address of the ddr memory controller > * @p: Pointer to the synopsys ecc status structure > @@ -242,7 +265,8 @@ static void synps_edac_check(struct mem_ctl_info *mci) > struct synps_edac_priv *priv = mci->pvt_info; > int status; > > - status = synps_edac_geterror_info(priv->baseaddr, &priv->stat); > + status = priv->p_data->edac_geterror_info(priv->baseaddr, > + &priv->stat); > if (status) > return; > > @@ -372,10 +396,12 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci) > for (j = 0; j < csi->nr_channels; j++) { > dimm = csi->channels[j]->dimm; > dimm->edac_mode = EDAC_FLAG_SECDED; > - dimm->mtype = synps_edac_get_mtype(priv->baseaddr); > + dimm->mtype = priv->p_data->edac_get_mtype( > + priv->baseaddr); Always let it stick out, never break on the opening brace. checkpatch should catch that but it doesn't, for some reason. > dimm->nr_pages = (size >> PAGE_SHIFT) / csi->nr_channels; > dimm->grain = SYNPS_EDAC_ERR_GRAIN; > - dimm->dtype = synps_edac_get_dtype(priv->baseaddr); > + dimm->dtype = priv->p_data->edac_get_dtype( > + priv->baseaddr); > } > } > > @@ -423,6 +449,21 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci, > return status; > } > > +static const struct synps_platform_data zynq_edac_def = { > + .edac_geterror_info = synps_edac_geterror_info, .geterror_info = synps_geterror_info, and so on looks perfectly fine to me. > + .edac_get_mtype = synps_edac_get_mtype, > + .edac_get_dtype = synps_edac_get_dtype, > + .edac_get_eccstate = synps_edac_get_eccstate, > + .quirks = 0, > +}; > + > +static const struct of_device_id synps_edac_match[] = { > + { .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def }, > + { /* end of table */ } > +}; > + > +MODULE_DEVICE_TABLE(of, synps_edac_match); > + > /** > * synps_edac_mc_probe - Check controller and bind driver > * @pdev: Pointer to the platform_device struct > @@ -440,13 +481,22 @@ static int synps_edac_mc_probe(struct platform_device *pdev) > int rc; > struct resource *res; > void __iomem *baseaddr; > + const struct of_device_id *match; > + const struct synps_platform_data *p_data; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > baseaddr = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(baseaddr)) > return PTR_ERR(baseaddr); > > - if (!synps_edac_get_eccstate(baseaddr)) { > + match = of_match_node(synps_edac_match, pdev->dev.of_node); > + if (!match && !match->data) { > + dev_err(&pdev->dev, "of_match_node() failed\n"); > + return -EINVAL; > + } > + > + p_data = (struct synps_platform_data *)match->data; > + if (!(p_data->edac_get_eccstate(baseaddr))) { This patch does more than just adding platform-specific structures and the commit message is not talking about it. > edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n"); > return -ENXIO; > } > @@ -468,6 +518,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev) > > priv = mci->pvt_info; > priv->baseaddr = baseaddr; > + priv->p_data = match->data; > + > rc = synps_edac_mc_init(mci, pdev); > if (rc) { > edac_printk(KERN_ERR, EDAC_MC, > @@ -486,7 +538,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev) > * Start capturing the correctable and uncorrectable errors. A write of > * 0 starts the counters. > */ > - writel(0x0, baseaddr + ECC_CTRL_OFST); > + if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)) > + writel(0x0, baseaddr + ECC_CTRL_OFST); Ditto.
diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c index 0c9c59e2b5a3..293380f884fe 100644 --- a/drivers/edac/synopsys_edac.c +++ b/drivers/edac/synopsys_edac.c @@ -22,6 +22,7 @@ #include <linux/edac.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/of.h> #include "edac_module.h" @@ -95,6 +96,9 @@ #define SCRUB_MODE_MASK 0x7 #define SCRUB_MODE_SECDED 0x4 +/* DDR ECC Quirks */ +#define DDR_ECC_INTR_SUPPORT BIT(0) + /** * struct ecc_error_info - ECC error log information * @row: Row number @@ -130,6 +134,7 @@ struct synps_ecc_status { * @baseaddr: Base address of the DDR controller * @message: Buffer for framing the event specific info * @stat: ECC status information + * @p_data: Pointer to platform data * @ce_cnt: Correctable Error count * @ue_cnt: Uncorrectable Error count */ @@ -137,11 +142,29 @@ struct synps_edac_priv { void __iomem *baseaddr; char message[SYNPS_EDAC_MSG_SIZE]; struct synps_ecc_status stat; + const struct synps_platform_data *p_data; u32 ce_cnt; u32 ue_cnt; }; /** + * struct synps_platform_data - synps platform data structure + * @edac_geterror_info: function which returns the current ecc error info + * @edac_get_mtype: function which returns the memory type + * @edac_get_dtype: function which returns the DIMM type + * @edac_get_eccstate: function which returns the ecc enable/disable status + * @quirks: a bitfield of quirks + */ +struct synps_platform_data { + int (*edac_geterror_info)(void __iomem *base, + struct synps_ecc_status *p); + enum mem_type (*edac_get_mtype)(const void __iomem *base); + enum dev_type (*edac_get_dtype)(const void __iomem *base); + bool (*edac_get_eccstate)(void __iomem *base); + int quirks; +}; + +/** * synps_edac_geterror_info - Get the current ecc error info * @base: Pointer to the base address of the ddr memory controller * @p: Pointer to the synopsys ecc status structure @@ -242,7 +265,8 @@ static void synps_edac_check(struct mem_ctl_info *mci) struct synps_edac_priv *priv = mci->pvt_info; int status; - status = synps_edac_geterror_info(priv->baseaddr, &priv->stat); + status = priv->p_data->edac_geterror_info(priv->baseaddr, + &priv->stat); if (status) return; @@ -372,10 +396,12 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci) for (j = 0; j < csi->nr_channels; j++) { dimm = csi->channels[j]->dimm; dimm->edac_mode = EDAC_FLAG_SECDED; - dimm->mtype = synps_edac_get_mtype(priv->baseaddr); + dimm->mtype = priv->p_data->edac_get_mtype( + priv->baseaddr); dimm->nr_pages = (size >> PAGE_SHIFT) / csi->nr_channels; dimm->grain = SYNPS_EDAC_ERR_GRAIN; - dimm->dtype = synps_edac_get_dtype(priv->baseaddr); + dimm->dtype = priv->p_data->edac_get_dtype( + priv->baseaddr); } } @@ -423,6 +449,21 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci, return status; } +static const struct synps_platform_data zynq_edac_def = { + .edac_geterror_info = synps_edac_geterror_info, + .edac_get_mtype = synps_edac_get_mtype, + .edac_get_dtype = synps_edac_get_dtype, + .edac_get_eccstate = synps_edac_get_eccstate, + .quirks = 0, +}; + +static const struct of_device_id synps_edac_match[] = { + { .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def }, + { /* end of table */ } +}; + +MODULE_DEVICE_TABLE(of, synps_edac_match); + /** * synps_edac_mc_probe - Check controller and bind driver * @pdev: Pointer to the platform_device struct @@ -440,13 +481,22 @@ static int synps_edac_mc_probe(struct platform_device *pdev) int rc; struct resource *res; void __iomem *baseaddr; + const struct of_device_id *match; + const struct synps_platform_data *p_data; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); baseaddr = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(baseaddr)) return PTR_ERR(baseaddr); - if (!synps_edac_get_eccstate(baseaddr)) { + match = of_match_node(synps_edac_match, pdev->dev.of_node); + if (!match && !match->data) { + dev_err(&pdev->dev, "of_match_node() failed\n"); + return -EINVAL; + } + + p_data = (struct synps_platform_data *)match->data; + if (!(p_data->edac_get_eccstate(baseaddr))) { edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n"); return -ENXIO; } @@ -468,6 +518,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev) priv = mci->pvt_info; priv->baseaddr = baseaddr; + priv->p_data = match->data; + rc = synps_edac_mc_init(mci, pdev); if (rc) { edac_printk(KERN_ERR, EDAC_MC, @@ -486,7 +538,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev) * Start capturing the correctable and uncorrectable errors. A write of * 0 starts the counters. */ - writel(0x0, baseaddr + ECC_CTRL_OFST); + if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT)) + writel(0x0, baseaddr + ECC_CTRL_OFST); return rc; free_edac_mc: @@ -511,13 +564,6 @@ static int synps_edac_mc_remove(struct platform_device *pdev) return 0; } -static const struct of_device_id synps_edac_match[] = { - { .compatible = "xlnx,zynq-ddrc-a05", }, - { /* end of table */ } -}; - -MODULE_DEVICE_TABLE(of, synps_edac_match); - static struct platform_driver synps_edac_mc_driver = { .driver = { .name = "synopsys-edac",