| Message ID | 1417008759-32257-1-git-send-email-andy.yan@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Mittwoch, den 26.11.2014, 21:32 +0800 schrieb Andy Yan: > On rockchip rk3288, only word(32-bit) accesses are > permitted for hdmi registers. Byte width accesses (writeb, > readb) generate an imprecise external abort. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > --- > > Changes in v13: None > Changes in v12: None > Changes in v11: None > Changes in v10: None > Changes in v9: None > Changes in v8: None > Changes in v7: None > Changes in v6: > - refactor register access without reg_shift > > Changes in v5: > - refactor reg-io-width > > Changes in v4: None > Changes in v3: > - split multi-register access to one indepent patch > > drivers/gpu/drm/bridge/dw_hdmi.c | 57 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 51 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c > index a53bf63..5e88c8d 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi.c > +++ b/drivers/gpu/drm/bridge/dw_hdmi.c > @@ -100,6 +100,11 @@ struct hdmi_data_info { > struct hdmi_vmode video_mode; > }; > > +union dw_reg_ptr { > + u32 __iomem *p32; > + u8 __iomem *p8; > +}; I see no need to introduce this. Just explicitly multiply the offset in dw_hdmi_writel. > struct dw_hdmi { > struct drm_connector connector; > struct drm_encoder *encoder; > @@ -121,20 +126,43 @@ struct dw_hdmi { > > struct regmap *regmap; > struct i2c_adapter *ddc; > - void __iomem *regs; > + union dw_reg_ptr regs; Keep this as void __iomem * > unsigned int sample_rate; > int ratio; > + > + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); > + u8 (*read)(struct dw_hdmi *hdmi, int offset); > }; > > +static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset) > +{ > + writel(val, hdmi->regs.p32 + offset); hdmi->regs + 4 * offset > +} > + > +static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset) > +{ > + return readl(hdmi->regs.p32 + offset); same here > +} > + > +static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) > +{ > + writeb(val, hdmi->regs.p8 + offset); > +} > + > +static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset) > +{ > + return readb(hdmi->regs.p8 + offset); > +} > + > static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) > { > - writeb(val, hdmi->regs + offset); > + hdmi->write(hdmi, val, offset); > } > > static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) > { > - return readb(hdmi->regs + offset); > + return hdmi->read(hdmi, offset); > } > > static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) > @@ -1508,6 +1536,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master, > struct dw_hdmi *hdmi; > struct resource *iores; > int ret, irq; > + u32 val = 1; > > hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL); > if (!hdmi) > @@ -1520,6 +1549,22 @@ int dw_hdmi_bind(struct device *dev, struct device *master, > hdmi->ratio = 100; > hdmi->encoder = encoder; > > + of_property_read_u32(np, "reg-io-width", &val); > + > + switch (val) { > + case 4: > + hdmi->write = dw_hdmi_writel; > + hdmi->read = dw_hdmi_readl; > + break; > + case 1: > + hdmi->write = dw_hdmi_writeb; > + hdmi->read = dw_hdmi_readb; > + break; > + default: > + dev_err(dev, "reg-io-width must be 1 or 4\n"); > + return -EINVAL; > + } > + > ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); > if (ddc_node) { > hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node); > @@ -1544,9 +1589,9 @@ int dw_hdmi_bind(struct device *dev, struct device *master, > return ret; > > iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - hdmi->regs = devm_ioremap_resource(dev, iores); > - if (IS_ERR(hdmi->regs)) > - return PTR_ERR(hdmi->regs); > + hdmi->regs.p32 = devm_ioremap_resource(dev, iores); > + if (IS_ERR(hdmi->regs.p32)) > + return PTR_ERR(hdmi->regs.p32); > > /* Product and revision IDs */ > dev_info(dev, regards Philipp
Hi Zabel: On 2014?11?27? 00:34, Philipp Zabel wrote: > Am Mittwoch, den 26.11.2014, 21:32 +0800 schrieb Andy Yan: >> On rockchip rk3288, only word(32-bit) accesses are >> permitted for hdmi registers. Byte width accesses (writeb, >> readb) generate an imprecise external abort. >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> >> --- >> >> Changes in v13: None >> Changes in v12: None >> Changes in v11: None >> Changes in v10: None >> Changes in v9: None >> Changes in v8: None >> Changes in v7: None >> Changes in v6: >> - refactor register access without reg_shift >> >> Changes in v5: >> - refactor reg-io-width >> >> Changes in v4: None >> Changes in v3: >> - split multi-register access to one indepent patch >> >> drivers/gpu/drm/bridge/dw_hdmi.c | 57 +++++++++++++++++++++++++++++++++++----- >> 1 file changed, 51 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c >> index a53bf63..5e88c8d 100644 >> --- a/drivers/gpu/drm/bridge/dw_hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c >> @@ -100,6 +100,11 @@ struct hdmi_data_info { >> struct hdmi_vmode video_mode; >> }; >> >> +union dw_reg_ptr { >> + u32 __iomem *p32; >> + u8 __iomem *p8; >> +}; > I see no need to introduce this. Just explicitly multiply the offset in > dw_hdmi_writel. > Is there any disadvantage to do like this? The compiler can help us do the explicitly multiply by this way. >> struct dw_hdmi { >> struct drm_connector connector; >> struct drm_encoder *encoder; >> @@ -121,20 +126,43 @@ struct dw_hdmi { >> >> struct regmap *regmap; >> struct i2c_adapter *ddc; >> - void __iomem *regs; >> + union dw_reg_ptr regs; > Keep this as void __iomem * > >> unsigned int sample_rate; >> int ratio; >> + >> + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); >> + u8 (*read)(struct dw_hdmi *hdmi, int offset); >> }; >> >> +static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset) >> +{ >> + writel(val, hdmi->regs.p32 + offset); > hdmi->regs + 4 * offset > >> +} >> + >> +static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset) >> +{ >> + return readl(hdmi->regs.p32 + offset); > same here > >> +} >> + >> +static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) >> +{ >> + writeb(val, hdmi->regs.p8 + offset); >> +} >> + >> +static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset) >> +{ >> + return readb(hdmi->regs.p8 + offset); >> +} >> + >> static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) >> { >> - writeb(val, hdmi->regs + offset); >> + hdmi->write(hdmi, val, offset); >> } >> >> static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) >> { >> - return readb(hdmi->regs + offset); >> + return hdmi->read(hdmi, offset); >> } >> >> static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) >> @@ -1508,6 +1536,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master, >> struct dw_hdmi *hdmi; >> struct resource *iores; >> int ret, irq; >> + u32 val = 1; >> >> hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL); >> if (!hdmi) >> @@ -1520,6 +1549,22 @@ int dw_hdmi_bind(struct device *dev, struct device *master, >> hdmi->ratio = 100; >> hdmi->encoder = encoder; >> >> + of_property_read_u32(np, "reg-io-width", &val); >> + >> + switch (val) { >> + case 4: >> + hdmi->write = dw_hdmi_writel; >> + hdmi->read = dw_hdmi_readl; >> + break; >> + case 1: >> + hdmi->write = dw_hdmi_writeb; >> + hdmi->read = dw_hdmi_readb; >> + break; >> + default: >> + dev_err(dev, "reg-io-width must be 1 or 4\n"); >> + return -EINVAL; >> + } >> + >> ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); >> if (ddc_node) { >> hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node); >> @@ -1544,9 +1589,9 @@ int dw_hdmi_bind(struct device *dev, struct device *master, >> return ret; >> >> iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> - hdmi->regs = devm_ioremap_resource(dev, iores); >> - if (IS_ERR(hdmi->regs)) >> - return PTR_ERR(hdmi->regs); >> + hdmi->regs.p32 = devm_ioremap_resource(dev, iores); >> + if (IS_ERR(hdmi->regs.p32)) >> + return PTR_ERR(hdmi->regs.p32); >> >> /* Product and revision IDs */ >> dev_info(dev, > regards > Philipp > > > >
Am Freitag, den 28.11.2014, 17:43 +0800 schrieb Andy Yan: > Hi Zabel: > On 2014?11?27? 00:34, Philipp Zabel wrote: > > Am Mittwoch, den 26.11.2014, 21:32 +0800 schrieb Andy Yan: > >> On rockchip rk3288, only word(32-bit) accesses are > >> permitted for hdmi registers. Byte width accesses (writeb, > >> readb) generate an imprecise external abort. > >> > >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > >> > >> --- > >> > >> Changes in v13: None > >> Changes in v12: None > >> Changes in v11: None > >> Changes in v10: None > >> Changes in v9: None > >> Changes in v8: None > >> Changes in v7: None > >> Changes in v6: > >> - refactor register access without reg_shift > >> > >> Changes in v5: > >> - refactor reg-io-width > >> > >> Changes in v4: None > >> Changes in v3: > >> - split multi-register access to one indepent patch > >> > >> drivers/gpu/drm/bridge/dw_hdmi.c | 57 +++++++++++++++++++++++++++++++++++----- > >> 1 file changed, 51 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c > >> index a53bf63..5e88c8d 100644 > >> --- a/drivers/gpu/drm/bridge/dw_hdmi.c > >> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c > >> @@ -100,6 +100,11 @@ struct hdmi_data_info { > >> struct hdmi_vmode video_mode; > >> }; > >> > >> +union dw_reg_ptr { > >> + u32 __iomem *p32; > >> + u8 __iomem *p8; > >> +}; > > I see no need to introduce this. Just explicitly multiply the offset in > > dw_hdmi_writel. > > > Is there any disadvantage to do like this? > The compiler can help us do the explicitly multiply by this way. Four additional lines, a new defined type, a few more changes to struct dw_hdmi and dw_hdmi_bind necessary. Technically I see no problem to let the compiler do the multiplication, my issue is that it ever so slightly obfuscates the code. Instead of just writing "* 4" in two functions, we get a new union that you need to know about when looking at struct dw_hdmi and dw_hdmi_bind, regs.p8 is used but never assigned directly, it's just a tiny bit of additional effort needed to understand the code. But when the cost to avoid that is so small... regards Philipp
Hi Philipp: On 2014?12?01? 20:04, Philipp Zabel wrote: > Am Freitag, den 28.11.2014, 17:43 +0800 schrieb Andy Yan: >> Hi Zabel: >> On 2014?11?27? 00:34, Philipp Zabel wrote: >>> Am Mittwoch, den 26.11.2014, 21:32 +0800 schrieb Andy Yan: >>>> On rockchip rk3288, only word(32-bit) accesses are >>>> permitted for hdmi registers. Byte width accesses (writeb, >>>> readb) generate an imprecise external abort. >>>> >>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >>>> >>>> --- >>>> >>>> Changes in v13: None >>>> Changes in v12: None >>>> Changes in v11: None >>>> Changes in v10: None >>>> Changes in v9: None >>>> Changes in v8: None >>>> Changes in v7: None >>>> Changes in v6: >>>> - refactor register access without reg_shift >>>> >>>> Changes in v5: >>>> - refactor reg-io-width >>>> >>>> Changes in v4: None >>>> Changes in v3: >>>> - split multi-register access to one indepent patch >>>> >>>> drivers/gpu/drm/bridge/dw_hdmi.c | 57 +++++++++++++++++++++++++++++++++++----- >>>> 1 file changed, 51 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c >>>> index a53bf63..5e88c8d 100644 >>>> --- a/drivers/gpu/drm/bridge/dw_hdmi.c >>>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c >>>> @@ -100,6 +100,11 @@ struct hdmi_data_info { >>>> struct hdmi_vmode video_mode; >>>> }; >>>> >>>> +union dw_reg_ptr { >>>> + u32 __iomem *p32; >>>> + u8 __iomem *p8; >>>> +}; >>> I see no need to introduce this. Just explicitly multiply the offset in >>> dw_hdmi_writel. >>> >> Is there any disadvantage to do like this? >> The compiler can help us do the explicitly multiply by this way. > Four additional lines, a new defined type, a few more changes to struct > dw_hdmi and dw_hdmi_bind necessary. > > Technically I see no problem to let the compiler do the multiplication, > my issue is that it ever so slightly obfuscates the code. Instead of > just writing "* 4" in two functions, we get a new union that you need to > know about when looking at struct dw_hdmi and dw_hdmi_bind, regs.p8 is > used but never assigned directly, it's just a tiny bit of additional > effort needed to understand the code. But when the cost to avoid that is > so small... > > regards > Philipp > What you said is right, I will change it in PATCH V15 thanks . > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c index a53bf63..5e88c8d 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.c +++ b/drivers/gpu/drm/bridge/dw_hdmi.c @@ -100,6 +100,11 @@ struct hdmi_data_info { struct hdmi_vmode video_mode; }; +union dw_reg_ptr { + u32 __iomem *p32; + u8 __iomem *p8; +}; + struct dw_hdmi { struct drm_connector connector; struct drm_encoder *encoder; @@ -121,20 +126,43 @@ struct dw_hdmi { struct regmap *regmap; struct i2c_adapter *ddc; - void __iomem *regs; + union dw_reg_ptr regs; unsigned int sample_rate; int ratio; + + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); + u8 (*read)(struct dw_hdmi *hdmi, int offset); }; +static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset) +{ + writel(val, hdmi->regs.p32 + offset); +} + +static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset) +{ + return readl(hdmi->regs.p32 + offset); +} + +static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) +{ + writeb(val, hdmi->regs.p8 + offset); +} + +static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset) +{ + return readb(hdmi->regs.p8 + offset); +} + static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) { - writeb(val, hdmi->regs + offset); + hdmi->write(hdmi, val, offset); } static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) { - return readb(hdmi->regs + offset); + return hdmi->read(hdmi, offset); } static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) @@ -1508,6 +1536,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master, struct dw_hdmi *hdmi; struct resource *iores; int ret, irq; + u32 val = 1; hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL); if (!hdmi) @@ -1520,6 +1549,22 @@ int dw_hdmi_bind(struct device *dev, struct device *master, hdmi->ratio = 100; hdmi->encoder = encoder; + of_property_read_u32(np, "reg-io-width", &val); + + switch (val) { + case 4: + hdmi->write = dw_hdmi_writel; + hdmi->read = dw_hdmi_readl; + break; + case 1: + hdmi->write = dw_hdmi_writeb; + hdmi->read = dw_hdmi_readb; + break; + default: + dev_err(dev, "reg-io-width must be 1 or 4\n"); + return -EINVAL; + } + ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); if (ddc_node) { hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node); @@ -1544,9 +1589,9 @@ int dw_hdmi_bind(struct device *dev, struct device *master, return ret; iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); - hdmi->regs = devm_ioremap_resource(dev, iores); - if (IS_ERR(hdmi->regs)) - return PTR_ERR(hdmi->regs); + hdmi->regs.p32 = devm_ioremap_resource(dev, iores); + if (IS_ERR(hdmi->regs.p32)) + return PTR_ERR(hdmi->regs.p32); /* Product and revision IDs */ dev_info(dev,
On rockchip rk3288, only word(32-bit) accesses are permitted for hdmi registers. Byte width accesses (writeb, readb) generate an imprecise external abort. Signed-off-by: Andy Yan <andy.yan@rock-chips.com> --- Changes in v13: None Changes in v12: None Changes in v11: None Changes in v10: None Changes in v9: None Changes in v8: None Changes in v7: None Changes in v6: - refactor register access without reg_shift Changes in v5: - refactor reg-io-width Changes in v4: None Changes in v3: - split multi-register access to one indepent patch drivers/gpu/drm/bridge/dw_hdmi.c | 57 +++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 6 deletions(-)